[U-Boot] [PATCH] mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648

[Dirk Behme <dirk.behme@de.bosch.com>]

On 15.03.2012 10:56, Stefano Babic wrote:
> On 08/03/2012 13:36, Dirk Behme wrote:
>> This patch imports three patches from the Freescale U-Boot with the following
>> commit messages:
>>
>> ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
>> http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
>> The errata, not applicable to USDHC, causes ESDHC to shut off clock to the card
>> when auto-clock gating is enabled for commands with busy signalling and no data
>> phase. The card might require the clock to exit the busy state, so the workaround
>> is to disable the auto-clock gate bits in SYSCTL register for such commands. The
>> workaround also entails polling on DAT0 bit in the PRSSTAT register to learn when
>> busy state is complete. Auto-clock gating is re-enabled at the end of busy state.
>>
>> ENGR00156670-1 ESDHC/USDHC: Remove delay before each cmd and some bug fixes
>> http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=a77c6fec8596891be96b2cdbc742c9824844b92a
>> Removed delay of 10 ms before each command. There should not be a need to have this
>> delay after the ENGR00156405 patch that polls until card is not busy anymore before
>> proceeding to next cmd.
>>
>> ENGR00161852: remove u-boot build warnings for mx6q
>> http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=97efee177f82b082db9d2019ed641c5b99b3f54b
>> Remove u-boot build warnings for mx6q.
>>
>> SYSCTL_RSTA was defined twice. Remove one definition.
>>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> CC: Andy Fleming <afleming@freescale.com>
>> CC: Fabio Estevam <fabio.estevam@freescale.com>
>> ---
>>  drivers/mmc/fsl_esdhc.c |   61 ++++++++++++++++++++++++++++++++++++++++------
> 
> This is not a blocking point for me, but is it maybe better to split
> this into three patches as it was originally ?

Hmm, I'm unsure about this. Looking at the 3 referenced Freescale 
patches and at the way the Freescale commits look, I was under the 
impression that the first patch was improved by Freescale two times to 
the then 'final' state.

At least the first patch

http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787

and the third

http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=97efee177f82b082db9d2019ed641c5b99b3f54b

definitely should be one patch. The third one wouldn't be there if there 
would have been a proper review of the first one ;)

And the commit message of the second patch

http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=a77c6fec8596891be96b2cdbc742c9824844b92a

mentions that the delay can be removed due to the first patch. So I was 
under the impression that this could have been done in the first patch, 
already, too.

But anyway, as you said, this shouldn't be a blocker.

> All together makes your
> patch not so easy to read.
> 
> 
>>  include/fsl_esdhc.h     |    4 ++-
>>  2 files changed, 56 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
>> index 30db030..694d6fd 100644
>> --- a/drivers/mmc/fsl_esdhc.c
>> +++ b/drivers/mmc/fsl_esdhc.c
>> @@ -259,6 +259,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>>  {
>>  	uint	xfertyp;
>>  	uint	irqstat;
>> +	uint	sysctl_restore = 0;
>>  	struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
>>  	volatile struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;
>>  
>> @@ -279,13 +280,6 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>>  	while (esdhc_read32(&regs->prsstat) & PRSSTAT_DLA)
>>  		;
>>  
>> -	/* Wait at least 8 SD clock cycles before the next command */
>> -	/*
>> -	 * Note: This is way more than 8 cycles, but 1ms seems to
>> -	 * resolve timing issues with some cards
>> -	 */
>> -	udelay(1000);
>> -
> 
> You drop a delay - is it ok for PowerPC, too ? Maybe some PowerPC guys
> can answer to this.

Regarding PowerPC I have no idea. Yes, it would be nice if the PowerPC 
guys could comment.

>>  	/* Set up for a data transfer if we have one */
>>  	if (data) {
>>  		int err;
>> @@ -298,6 +292,14 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>>  	/* Figure out the transfer arguments */
>>  	xfertyp = esdhc_xfertyp(cmd, data);
>>  
>> +	/* ESDHC errata ENGcm03648: Turn off auto-clock gate for commands
>> +	 * with busy signaling and no data
>> +	 */
> 
> Wrong multiline comment

Yes, ack.

>> +	if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
>> +		sysctl_restore = esdhc_read32(&regs->sysctl);
>> +		esdhc_write32(&regs->sysctl, sysctl_restore | 0xF);
>> +	}
>> +
> 
> I see two issue. There is not a SDCLKEN bit in the PowerPC ESDHC, as far
> as I can see (for example, in MPC8536). Should we not use the HOSTVER
> register to handle these cases ?
> 
> The comment says you are disabling auto-clock. However, in the register
> you are enabling all clocks (PEREN / SDCLKEN /..). Can you explain
> better the reason ? Do you want really disabling clocks ?

I have to admit that I have no idea :(

I'm no SD/MMC expert. This patch was created doing a functional diff. On 
a custom board we found that the imx_esdhc.c driver in Freescale's old 
U-Boot works fine, while the mainline fsl_esdhc.c we talk about here 
doesn't. Then, we traced it down to the changes we talk about here and 
ported them to the mainline fsl_esdhc.c. This fixes our issue. 
Unfortunately, there is no real understanding what these changes do.

Could any SD expert help with this?

>>  	/* Send the command */
>>  	esdhc_write32(&regs->cmdarg, cmd->cmdarg);
>>  #if defined(CONFIG_FSL_USDHC)
>> @@ -307,19 +309,62 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>>  #else
>>  	esdhc_write32(&regs->xfertyp, xfertyp);
>>  #endif
>> +
>> +	/* Mask all irqs */
>> +	esdhc_write32(&regs->irqsigen, 0);
>> +
>>  	/* Wait for the command to complete */
>> -	while (!(esdhc_read32(&regs->irqstat) & IRQSTAT_CC))
>> +	while (!(esdhc_read32(&regs->irqstat) & (IRQSTAT_CC | IRQSTAT_CTOE)))
>>  		;
>>  
>>  	irqstat = esdhc_read32(&regs->irqstat);
>>  	esdhc_write32(&regs->irqstat, irqstat);
>>  
>> +	/* Reset CMD and DATA portions on error */
>> +	if (irqstat & (CMD_ERR | IRQSTAT_CTOE)) {
>> +		esdhc_write32(&regs->sysctl, esdhc_read32(&regs->sysctl) |
>> +			      SYSCTL_RSTC);
>> +		while (esdhc_read32(&regs->sysctl) & SYSCTL_RSTC)
>> +			;
>> +
>> +		if (data) {
>> +			esdhc_write32(&regs->sysctl,
>> +				      esdhc_read32(&regs->sysctl) |
>> +				      SYSCTL_RSTD);
>> +			while ((esdhc_read32(&regs->sysctl) & SYSCTL_RSTD))
>> +				;
>> +		}
>> +
>> +		/* Restore auto-clock gate if error */
>> +		if (!data && (cmd->resp_type & MMC_RSP_BUSY))
>> +			esdhc_write32(&regs->sysctl, sysctl_restore);
>> +	}
>> +
> 
> Ok, you reset the DAT lines (according to the manual) before returning
> the error.
> 
>>  	if (irqstat & CMD_ERR)
>>  		return COMM_ERR;
>>  
>>  	if (irqstat & IRQSTAT_CTOE)
>>  		return TIMEOUT;
>>  
>> +	/* Workaround for ESDHC errata ENGcm03648 */
> 
> This ENGcm03648 is not mentioned in the commit message, it seems another
> issue...

Hmm? The subject is about "errata ENGcm03648" And the commit messages 
refers to

...
ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
...

Sorry if I overlooked anything.

Best regards

Dirk