[PATCH 2/3] eSDHC: Fix errors when booting kernel with fsl esdhc

[Zang Roy-R61911]

> -----Original Message-----
> From: S, Venkatraman [mailto:svenkatr at ti.com]
> Sent: Tuesday, July 19, 2011 23:58 PM
> To: Zang Roy-R61911
> Cc: linux-mmc; linuxppc-dev; cbouatmailru; akpm; Xu Lei-B33228; Kumar Gala
> Subject: Re: [PATCH 2/3] eSDHC: Fix errors when booting kernel with fsl esdhc
> 
> On Mon, Jul 18, 2011 at 11:31 AM, Zang Roy-R61911 <r61911 at freescale.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: S, Venkatraman [mailto:svenkatr at ti.com]
> >> Sent: Tuesday, July 05, 2011 14:17 PM
> >> To: Zang Roy-R61911
> >> Cc: linux-mmc; linuxppc-dev; cbouatmailru; akpm; Xu Lei-B33228; Kumar Gala
> >> Subject: Re: [PATCH 2/3] eSDHC: Fix errors when booting kernel with fsl esdhc
> >>
> >> On Tue, Jul 5, 2011 at 9:49 AM, Roy Zang <tie-fei.zang at freescale.com> wrote:
> >> > From: Xu lei <B33228 at freescale.com>
> >> >
> >> > When esdhc module was enabled in p5020, there were following errors:
> >> >
> >> > mmc0: Timeout waiting for hardware interrupt.
> >> > mmc0: error -110 whilst initialising SD card
> >> > mmc0: Unexpected interrupt 0x02000000.
> >> > mmc0: Timeout waiting for hardware interrupt.
> >> > mmc0: error -110 whilst initialising SD card
> >> > mmc0: Unexpected interrupt 0x02000000.
> >> >
> >> > It is because ESDHC controller has different bit setting for PROCTL
> >> > register, when kernel sets Power Control Register by method for standard
> >> > SD Host Specification, it would overwritten FSL ESDHC PROCTL[DMAS];
> >> > when it set Host Control Registers[DMAS], it sets PROCTL[EMODE] and
> >> > PROCTL[D3CD]. These operations will set bad bits for PROCTL Register
> >> > on FSL ESDHC Controller and cause errors, so this patch will make esdhc
> >> > driver access FSL PROCTL Register according to block guide instead of
> >> > standard SD Host Specification.
> >> >
> >> > For some FSL chips, such as MPC8536/P2020, PROCTL[VOLT_SEL] and PROCTL[DMAS]
> >> > bits are reserved and even if they are set to wrong bits there is no error.
> >> > But considering that all FSL ESDHC Controller register map is not fully
> >> > compliant to standard SD Host Specification, we put the patch to all of
> >> > FSL ESDHC Controllers.
> >> >
> >> > Signed-off-by: Lei Xu <B33228 at freescale.com>
> >> > Signed-off-by: Roy Zang <tie-fei.zang at freescale.com>
> >> > Signed-off-by: Kumar Gala <galak at kernel.crashing.org>
> >> > ---
> >> >  drivers/mmc/host/sdhci-of-core.c |    3 ++
> >> >  drivers/mmc/host/sdhci.c         |   62 ++++++++++++++++++++++++++++++----
> -
> >> --
> >> >  include/linux/mmc/sdhci.h        |    6 ++-
> >> >  3 files changed, 57 insertions(+), 14 deletions(-)
> > [snip]
> >
> >> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> > index 58d5436..77174e5 100644
> >> > --- a/drivers/mmc/host/sdhci.c
> >> > +++ b/drivers/mmc/host/sdhci.c
> >> > @@ -674,7 +674,7 @@ static void sdhci_set_transfer_irqs(struct sdhci_host
> >> *host)
> >> >  static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command
> >> *cmd)
> >> >  {
> >> >        u8 count;
> >> > -       u8 ctrl;
> >> > +       u32 ctrl;
> >> >        struct mmc_data *data = cmd->data;
> >> >        int ret;
> >> >
> >> > @@ -807,14 +807,28 @@ static void sdhci_prepare_data(struct sdhci_host
> *host,
> >> struct mmc_command *cmd)
> >> >         * is ADMA.
> >> >         */
> >> >        if (host->version >= SDHCI_SPEC_200) {
> >> > -               ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> >> > -               ctrl &= ~SDHCI_CTRL_DMA_MASK;
> >> > -               if ((host->flags & SDHCI_REQ_USE_DMA) &&
> >> > -                       (host->flags & SDHCI_USE_ADMA))
> >> > -                       ctrl |= SDHCI_CTRL_ADMA32;
> >> > -               else
> >> > -                       ctrl |= SDHCI_CTRL_SDMA;
> >> > -               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> >> > +               if (host->quirks & SDHCI_QUIRK_QORIQ_PROCTL_WEIRD) {
> >> > +#define ESDHCI_PROCTL_DMAS_MASK                0x00000300
> >> > +#define ESDHCI_PROCTL_ADMA32           0x00000200
> >> > +#define ESDHCI_PROCTL_SDMA             0x00000000
> >>
> >> Breaks the code flow / readability. Can be moved to top of the file ?
> > The defines are only used in the following section. Why it will break
> > the readability?
> > I can also see this kind of define in the file
> > ...
> > #define SAMPLE_COUNT    5
> >
> > static int sdhci_get_ro(struct mmc_host *mmc)
> > ...
> >
> > Any rule should follow?
> >
> >
> > [snip]
> >> > @@ -1162,6 +1189,17 @@ static void sdhci_set_power(struct sdhci_host *host,
> >> unsigned short power)
> >> >
> >> >        host->pwr = pwr;
> >> >
> >> > +       /* Now FSL ESDHC Controller has no Bus Power bit,
> >> > +        * and PROCTL[21] bit is for voltage selection */
> >>
> >> Multiline comment style needed..
> > Will update.
> > please help to explain your previous comment.
> > Thanks.
> > Roy
> 
> There aren't very hard rules on this. Simple #defines are good, as a
> one off usage.
> These bit mask fields are very verbose, and they tend to grow more
> than a screenful.
> The remaining bits will never be defined ?
The mask fields and bits are only for some WERID defines. I do not think
they will grow more than a screen.
The remain bits will have little chance to be defined.
Thanks for the suggestion. I will update the comment style and post again.
Roy