-----Original Message-----
From: S, Venkatraman [mailto:svenkatr@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 Gal=

a

Subject: Re: [PATCH 2/3] eSDHC: Fix errors when booting kernel with fsl e=

sdhc

=20
On Mon, Jul 18, 2011 at 11:31 AM, Zang Roy-R61911 [off-list ref] =

wrote:

quoted

quoted

-----Original Message-----
From: S, Venkatraman [mailto:svenkatr@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

quoted

quoted

Subject: Re: [PATCH 2/3] eSDHC: Fix errors when booting kernel with fs=

l esdhc

quoted

quoted

On Tue, Jul 5, 2011 at 9:49 AM, Roy Zang [off-list ref] =

wrote:

quoted

quoted

quoted

From: Xu lei <redacted>

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 stan=

dard

quoted

quoted

quoted

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 es=

dhc

quoted

quoted

quoted

driver access FSL PROCTL Register according to block guide instead o=

f

quoted

quoted

quoted

standard SD Host Specification.

For some FSL chips, such as MPC8536/P2020, PROCTL[VOLT_SEL] and PROC=

TL[DMAS]

quoted

quoted

quoted

bits are reserved and even if they are set to wrong bits there is no=

 error.

quoted

quoted

quoted

But considering that all FSL ESDHC Controller register map is not fu=

lly

quoted

quoted

quoted

compliant to standard SD Host Specification, we put the patch to all=

 of

quoted

quoted

quoted

FSL ESDHC Controllers.

Signed-off-by: Lei Xu <redacted>
Signed-off-by: Roy Zang <redacted>
Signed-off-by: Kumar Gala <redacted>
---
=A0drivers/mmc/host/sdhci-of-core.c | =A0 =A03 ++
=A0drivers/mmc/host/sdhci.c =A0 =A0 =A0 =A0 | =A0 62 +++++++++++++++=

+++++++++++++++----

-

quoted

quoted

--

quoted

=A0include/linux/mmc/sdhci.h =A0 =A0 =A0 =A0| =A0 =A06 ++-
=A03 files changed, 57 insertions(+), 14 deletions(-)

[snip]

quoted

quoted

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

quoted

quoted

*host)

quoted

=A0static void sdhci_prepare_data(struct sdhci_host *host, struct mm=

c_command

quoted

quoted

*cmd)

quoted

=A0{
=A0 =A0 =A0 =A0u8 count;
- =A0 =A0 =A0 u8 ctrl;
+ =A0 =A0 =A0 u32 ctrl;
=A0 =A0 =A0 =A0struct mmc_data *data =3D cmd->data;
=A0 =A0 =A0 =A0int ret;

@@ -807,14 +807,28 @@ static void sdhci_prepare_data(struct sdhci_ho=

st

*host,

quoted

quoted

struct mmc_command *cmd)

quoted

=A0 =A0 =A0 =A0 * is ADMA.
=A0 =A0 =A0 =A0 */
=A0 =A0 =A0 =A0if (host->version >=3D SDHCI_SPEC_200) {
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 ctrl =3D sdhci_readb(host, SDHCI_HOST_=

CONTROL);

quoted

quoted

quoted

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 ctrl &=3D ~SDHCI_CTRL_DMA_MASK;
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((host->flags & SDHCI_REQ_USE_DMA) =

&&

quoted

quoted

quoted

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (host->flags & SDHCI_U=

SE_ADMA))

quoted

quoted

quoted

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ctrl |=3D SDHCI_CTRL_A=

DMA32;

quoted

quoted

quoted

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ctrl |=3D SDHCI_CTRL_S=

DMA;

quoted

quoted

quoted

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 sdhci_writeb(host, ctrl, SDHCI_HOST_CO=

NTROL);

quoted

quoted

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (host->quirks & SDHCI_QUIRK_QORIQ_P=

ROCTL_WEIRD) {

quoted

quoted

quoted

+#define ESDHCI_PROCTL_DMAS_MASK =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A00x00=

000300

quoted

quoted

quoted

+#define ESDHCI_PROCTL_ADMA32 =A0 =A0 =A0 =A0 =A0 0x00000200
+#define ESDHCI_PROCTL_SDMA =A0 =A0 =A0 =A0 =A0 =A0 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 =A0 =A05

static int sdhci_get_ro(struct mmc_host *mmc)
...

Any rule should follow?


[snip]

quoted

quoted

@@ -1162,6 +1189,17 @@ static void sdhci_set_power(struct sdhci_host=

 *host,

quoted

quoted

unsigned short power)

quoted

=A0 =A0 =A0 =A0host->pwr =3D pwr;

+ =A0 =A0 =A0 /* Now FSL ESDHC Controller has no Bus Power bit,
+ =A0 =A0 =A0 =A0* and PROCTL[21] bit is for voltage selection */

Multiline comment style needed..

Will update.
please help to explain your previous comment.
Thanks.
Roy

=20
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