[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

The low four bits of the SYSCTL register are reserved on the USDHC
controller on i.MX6 and i.MX7 processors, but are used for clocking
operations on earlier models.

Guard against their usage by hiding the bit mask macros on those
processors.

These bits are used to prevent glitches when changing clocks on
i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.

From the i.MX6DQ RM:
	To prevent possible glitch on the card clock, clear the
	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
	or DVS in System Control Register) or setting RSTA bit.

Signed-off-by: Eric Nelson <eric@nelint.com>
---
 drivers/mmc/fsl_esdhc.c | 15 +++++++++++++--
 include/fsl_esdhc.h     |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index c5054d6..1ccc576 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -502,15 +502,22 @@ static void set_sysctl(struct mmc *mmc, uint clock)
 
 	clk = (pre_div << 8) | (div << 4);
 
+#ifdef CONFIG_FSL_USDHC
+	esdhc_setbits32(&regs->sysctl, SYSCTL_RSTA);
+#else
 	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
+#endif
 
 	esdhc_clrsetbits32(&regs->sysctl, SYSCTL_CLOCK_MASK, clk);
 
 	udelay(10000);
 
-	clk = SYSCTL_PEREN | SYSCTL_CKEN;
+#ifdef CONFIG_FSL_USDHC
+	esdhc_clrbits32(&regs->sysctl, SYSCTL_RSTA);
+#else
+	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
+#endif
 
-	esdhc_setbits32(&regs->sysctl, clk);
 }
 
 #ifdef CONFIG_FSL_ESDHC_USE_PERIPHERAL_CLK
@@ -585,7 +592,9 @@ static int esdhc_init(struct mmc *mmc)
 	esdhc_write32(&regs->scr, 0x00000040);
 #endif
 
+#ifndef CONFIG_FSL_USDHC
 	esdhc_setbits32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);
+#endif
 
 	/* Set the initial clock speed */
 	mmc_set_clock(mmc, 400000);
@@ -657,8 +666,10 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg)
 	/* First reset the eSDHC controller */
 	esdhc_reset(regs);
 
+#ifndef CONFIG_FSL_USDHC
 	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_HCKEN
 				| SYSCTL_IPGEN | SYSCTL_CKEN);
+#endif
 
 	writel(SDHCI_IRQ_EN_BITS, &regs->irqstaten);
 	memset(&cfg->cfg, 0, sizeof(cfg->cfg));
diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
index aa1b4cf..a4b87ce 100644
--- a/include/fsl_esdhc.h
+++ b/include/fsl_esdhc.h
@@ -25,10 +25,12 @@
 #define SYSCTL_INITA		0x08000000
 #define SYSCTL_TIMEOUT_MASK	0x000f0000
 #define SYSCTL_CLOCK_MASK	0x0000fff0
+#if !defined(CONFIG_MX6)
 #define SYSCTL_CKEN		0x00000008
 #define SYSCTL_PEREN		0x00000004
 #define SYSCTL_HCKEN		0x00000002
 #define SYSCTL_IPGEN		0x00000001
+#endif
 #define SYSCTL_RSTA		0x01000000
 #define SYSCTL_RSTC		0x02000000
 #define SYSCTL_RSTD		0x04000000
-- 
2.6.2

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

On 12/04/2015 10:32 AM, Eric Nelson wrote:
> The low four bits of the SYSCTL register are reserved on the USDHC
> controller on i.MX6 and i.MX7 processors, but are used for clocking
> operations on earlier models.
> 
> Guard against their usage by hiding the bit mask macros on those
> processors.
> 
> These bits are used to prevent glitches when changing clocks on
> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
> 
> From the i.MX6DQ RM:
> 	To prevent possible glitch on the card clock, clear the
> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
> 	or DVS in System Control Register) or setting RSTA bit.
> 
> Signed-off-by: Eric Nelson <eric@nelint.com>

I forgot to add an in-reply-to header.

http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#236651

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Michael Trimarchi]

Hi

On Fri, Dec 4, 2015 at 6:32 PM, Eric Nelson <eric@nelint.com> wrote:
> The low four bits of the SYSCTL register are reserved on the USDHC
> controller on i.MX6 and i.MX7 processors, but are used for clocking
> operations on earlier models.
>
> Guard against their usage by hiding the bit mask macros on those
> processors.
>
> These bits are used to prevent glitches when changing clocks on
> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
>
> From the i.MX6DQ RM:
>         To prevent possible glitch on the card clock, clear the
>         FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
>         or DVS in System Control Register) or setting RSTA bit.
>
> Signed-off-by: Eric Nelson <eric@nelint.com>
> ---
>  drivers/mmc/fsl_esdhc.c | 15 +++++++++++++--
>  include/fsl_esdhc.h     |  2 ++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index c5054d6..1ccc576 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -502,15 +502,22 @@ static void set_sysctl(struct mmc *mmc, uint clock)
>
>         clk = (pre_div << 8) | (div << 4);
>
> +#ifdef CONFIG_FSL_USDHC
> +       esdhc_setbits32(&regs->sysctl, SYSCTL_RSTA);
> +#else
>         esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
> +#endif
>

I really prefer is_usdhc()

>         esdhc_clrsetbits32(&regs->sysctl, SYSCTL_CLOCK_MASK, clk);
>
>         udelay(10000);
>
> -       clk = SYSCTL_PEREN | SYSCTL_CKEN;
> +#ifdef CONFIG_FSL_USDHC
> +       esdhc_clrbits32(&regs->sysctl, SYSCTL_RSTA);
> +#else
> +       esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> +#endif
>

same here

Michael

> -       esdhc_setbits32(&regs->sysctl, clk);
>  }
>
>  #ifdef CONFIG_FSL_ESDHC_USE_PERIPHERAL_CLK
> @@ -585,7 +592,9 @@ static int esdhc_init(struct mmc *mmc)
>         esdhc_write32(&regs->scr, 0x00000040);
>  #endif
>
> +#ifndef CONFIG_FSL_USDHC
>         esdhc_setbits32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);
> +#endif
>
>         /* Set the initial clock speed */
>         mmc_set_clock(mmc, 400000);
> @@ -657,8 +666,10 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg)
>         /* First reset the eSDHC controller */
>         esdhc_reset(regs);
>
> +#ifndef CONFIG_FSL_USDHC
>         esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_HCKEN
>                                 | SYSCTL_IPGEN | SYSCTL_CKEN);
> +#endif
>
>         writel(SDHCI_IRQ_EN_BITS, &regs->irqstaten);
>         memset(&cfg->cfg, 0, sizeof(cfg->cfg));
> diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
> index aa1b4cf..a4b87ce 100644
> --- a/include/fsl_esdhc.h
> +++ b/include/fsl_esdhc.h
> @@ -25,10 +25,12 @@
>  #define SYSCTL_INITA           0x08000000
>  #define SYSCTL_TIMEOUT_MASK    0x000f0000
>  #define SYSCTL_CLOCK_MASK      0x0000fff0
> +#if !defined(CONFIG_MX6)
>  #define SYSCTL_CKEN            0x00000008
>  #define SYSCTL_PEREN           0x00000004
>  #define SYSCTL_HCKEN           0x00000002
>  #define SYSCTL_IPGEN           0x00000001
> +#endif
>  #define SYSCTL_RSTA            0x01000000
>  #define SYSCTL_RSTC            0x02000000
>  #define SYSCTL_RSTD            0x04000000
> --
> 2.6.2
>



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

On 12/04/2015 10:38 AM, Eric Nelson wrote:
> On 12/04/2015 10:32 AM, Eric Nelson wrote:
>> The low four bits of the SYSCTL register are reserved on the USDHC
>> controller on i.MX6 and i.MX7 processors, but are used for clocking
>> operations on earlier models.
>>
>> Guard against their usage by hiding the bit mask macros on those
>> processors.
>>
>> These bits are used to prevent glitches when changing clocks on
>> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
>>
>> From the i.MX6DQ RM:
>> 	To prevent possible glitch on the card clock, clear the
>> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
>> 	or DVS in System Control Register) or setting RSTA bit.
>>
>> Signed-off-by: Eric Nelson <eric@nelint.com>
> 
> I forgot to add an in-reply-to header.
> 
> http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#236651
> 
> 

Fabio, I haven't been able to reproduce the "mmc erase/ENGcm03648"
issue (with or without a code change) for a couple of hours now.

Can you give this a spin?

It seems unlikely to address the issue unless what we're seeing is a
side effect of a glitch while switching clocks.

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

Hi Michael,

On 12/04/2015 10:40 AM, Michael Trimarchi wrote:
> On Fri, Dec 4, 2015 at 6:32 PM, Eric Nelson <eric@nelint.com> wrote:
...

>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
>> index c5054d6..1ccc576 100644
>> --- a/drivers/mmc/fsl_esdhc.c
>> +++ b/drivers/mmc/fsl_esdhc.c
>> @@ -502,15 +502,22 @@ static void set_sysctl(struct mmc *mmc, uint clock)
>>
>>         clk = (pre_div << 8) | (div << 4);
>>
>> +#ifdef CONFIG_FSL_USDHC
>> +       esdhc_setbits32(&regs->sysctl, SYSCTL_RSTA);
>> +#else
>>         esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
>> +#endif
>>
> 
> I really prefer is_usdhc()
> 

Am I overlooking something?

I'm not seeing any such animal, and using a run-time test would prevent
the compiler from catching the use of the bits below:

...

>> diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
>> index aa1b4cf..a4b87ce 100644
>> --- a/include/fsl_esdhc.h
>> +++ b/include/fsl_esdhc.h
>> @@ -25,10 +25,12 @@
>>  #define SYSCTL_INITA           0x08000000
>>  #define SYSCTL_TIMEOUT_MASK    0x000f0000
>>  #define SYSCTL_CLOCK_MASK      0x0000fff0
>> +#if !defined(CONFIG_MX6)
>>  #define SYSCTL_CKEN            0x00000008
>>  #define SYSCTL_PEREN           0x00000004
>>  #define SYSCTL_HCKEN           0x00000002
>>  #define SYSCTL_IPGEN           0x00000001
>> +#endif

Please advise,


Eric

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Michael Trimarchi]

Hi

On Fri, Dec 4, 2015 at 6:49 PM, Eric Nelson <eric@nelint.com> wrote:
> Hi Michael,
>
> On 12/04/2015 10:40 AM, Michael Trimarchi wrote:
>> On Fri, Dec 4, 2015 at 6:32 PM, Eric Nelson <eric@nelint.com> wrote:
> ...
>
>>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
>>> index c5054d6..1ccc576 100644
>>> --- a/drivers/mmc/fsl_esdhc.c
>>> +++ b/drivers/mmc/fsl_esdhc.c
>>> @@ -502,15 +502,22 @@ static void set_sysctl(struct mmc *mmc, uint clock)
>>>
>>>         clk = (pre_div << 8) | (div << 4);
>>>
>>> +#ifdef CONFIG_FSL_USDHC
>>> +       esdhc_setbits32(&regs->sysctl, SYSCTL_RSTA);
>>> +#else
>>>         esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
>>> +#endif
>>>
>>
>> I really prefer is_usdhc()
>>
>
> Am I overlooking something?
>
> I'm not seeing any such animal, and using a run-time test would prevent
> the compiler from catching the use of the bits below:
>

If you create a static inline with your define compiler should be optimized but
I think that result will be much clean

Michael

> ...
>
>>> diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
>>> index aa1b4cf..a4b87ce 100644
>>> --- a/include/fsl_esdhc.h
>>> +++ b/include/fsl_esdhc.h
>>> @@ -25,10 +25,12 @@
>>>  #define SYSCTL_INITA           0x08000000
>>>  #define SYSCTL_TIMEOUT_MASK    0x000f0000
>>>  #define SYSCTL_CLOCK_MASK      0x0000fff0
>>> +#if !defined(CONFIG_MX6)
>>>  #define SYSCTL_CKEN            0x00000008
>>>  #define SYSCTL_PEREN           0x00000004
>>>  #define SYSCTL_HCKEN           0x00000002
>>>  #define SYSCTL_IPGEN           0x00000001
>>> +#endif
>
> Please advise,
>
>
> Eric



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Fabio Estevam]

On Fri, Dec 4, 2015 at 3:43 PM, Eric Nelson <eric@nelint.com> wrote:

> Fabio, I haven't been able to reproduce the "mmc erase/ENGcm03648"
> issue (with or without a code change) for a couple of hours now.
>
> Can you give this a spin?

Sure, just gave it a try and the 'mmc erase' issue still happens.

I think your patch is correct anyway, so:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

Thanks

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Hector Palacios]

Hi Eric,

On 12/04/2015 06:43 PM, Eric Nelson wrote:
> On 12/04/2015 10:38 AM, Eric Nelson wrote:
>> On 12/04/2015 10:32 AM, Eric Nelson wrote:
>>> The low four bits of the SYSCTL register are reserved on the USDHC
>>> controller on i.MX6 and i.MX7 processors, but are used for clocking
>>> operations on earlier models.
>>>
>>> Guard against their usage by hiding the bit mask macros on those
>>> processors.
>>>
>>> These bits are used to prevent glitches when changing clocks on
>>> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
>>>
>>> From the i.MX6DQ RM:
>>> 	To prevent possible glitch on the card clock, clear the
>>> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
>>> 	or DVS in System Control Register) or setting RSTA bit.
>>>
>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>
>> I forgot to add an in-reply-to header.
>>
>> http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#236651
>>
>>
> 
> Fabio, I haven't been able to reproduce the "mmc erase/ENGcm03648"
> issue (with or without a code change) for a couple of hours now.
> 
> Can you give this a spin?
> 
> It seems unlikely to address the issue unless what we're seeing is a
> side effect of a glitch while switching clocks.

As Fabio, I can reproduce this 100% of the times.
The patch does not fix it, though.

--
Hector Palacios

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

Hi Fabio and Hector,

On 12/04/2015 10:43 AM, Eric Nelson wrote:
> On 12/04/2015 10:38 AM, Eric Nelson wrote:
>> On 12/04/2015 10:32 AM, Eric Nelson wrote:
>>> The low four bits of the SYSCTL register are reserved on the USDHC
>>> controller on i.MX6 and i.MX7 processors, but are used for clocking
>>> operations on earlier models.
>>>
>>> Guard against their usage by hiding the bit mask macros on those
>>> processors.
>>>
>>> These bits are used to prevent glitches when changing clocks on
>>> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
>>>
>>> From the i.MX6DQ RM:
>>> 	To prevent possible glitch on the card clock, clear the
>>> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
>>> 	or DVS in System Control Register) or setting RSTA bit.
>>>
>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>
>> I forgot to add an in-reply-to header.
>>
>> http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#236651
>>
>>
> 
> Fabio, I haven't been able to reproduce the "mmc erase/ENGcm03648"
> issue (with or without a code change) for a couple of hours now.
> 
> Can you give this a spin?
> 
> It seems unlikely to address the issue unless what we're seeing is a
> side effect of a glitch while switching clocks.
> 
> 

I switched back to a v2014.10 release and am able to reproduce the
issue at will.

The sysctl patch had no effect, but adding an #ifndef around the
ENGcm03648 block allows things to proceed.

+#ifndef CONFIG_FSL_USDHC
        /* Workaround for ESDHC errata ENGcm03648 */
        if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
...

Since the need to poll the DAT0 line is only documented in the
errata for the i.MX35, I wonder if this isn't the right thing to do.

The CC bit of irqstat does indicate that the command completed
and without error (I'm seeing values of 1 in irqstat).

From what I can tell, the linux kernel doesn't do this test and
doesn't appear to have any trouble.

What code base are you running against (u-boot-imx/master)?

What do you see if you do the same?

I had been testing against v2015.10 and the board I'm testing isn't
upstream, so I can't easily bisect.

Please advise,


Eric

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Hector Palacios]

Hi,

On 12/04/2015 06:32 PM, Eric Nelson wrote:
> The low four bits of the SYSCTL register are reserved on the USDHC
> controller on i.MX6 and i.MX7 processors, but are used for clocking
> operations on earlier models.
> 
> Guard against their usage by hiding the bit mask macros on those
> processors.
> 
> These bits are used to prevent glitches when changing clocks on
> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
> 
> From the i.MX6DQ RM:
> 	To prevent possible glitch on the card clock, clear the
> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
> 	or DVS in System Control Register) or setting RSTA bit.
> 
> Signed-off-by: Eric Nelson <eric@nelint.com>
> ---
>  drivers/mmc/fsl_esdhc.c | 15 +++++++++++++--
>  include/fsl_esdhc.h     |  2 ++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index c5054d6..1ccc576 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -502,15 +502,22 @@ static void set_sysctl(struct mmc *mmc, uint clock)
>  
>  	clk = (pre_div << 8) | (div << 4);
>  
> +#ifdef CONFIG_FSL_USDHC
> +	esdhc_setbits32(&regs->sysctl, SYSCTL_RSTA);
> +#else
>  	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
> +#endif
>  
>  	esdhc_clrsetbits32(&regs->sysctl, SYSCTL_CLOCK_MASK, clk);
>  
>  	udelay(10000);
>  
> -	clk = SYSCTL_PEREN | SYSCTL_CKEN;
> +#ifdef CONFIG_FSL_USDHC
> +	esdhc_clrbits32(&regs->sysctl, SYSCTL_RSTA);
> +#else
> +	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> +#endif
>  
> -	esdhc_setbits32(&regs->sysctl, clk);
>  }
>  
>  #ifdef CONFIG_FSL_ESDHC_USE_PERIPHERAL_CLK
> @@ -585,7 +592,9 @@ static int esdhc_init(struct mmc *mmc)
>  	esdhc_write32(&regs->scr, 0x00000040);
>  #endif
>  
> +#ifndef CONFIG_FSL_USDHC
>  	esdhc_setbits32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);
> +#endif
>  
>  	/* Set the initial clock speed */
>  	mmc_set_clock(mmc, 400000);
> @@ -657,8 +666,10 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg)
>  	/* First reset the eSDHC controller */
>  	esdhc_reset(regs);
>  
> +#ifndef CONFIG_FSL_USDHC
>  	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_HCKEN
>  				| SYSCTL_IPGEN | SYSCTL_CKEN);
> +#endif
>  
>  	writel(SDHCI_IRQ_EN_BITS, &regs->irqstaten);
>  	memset(&cfg->cfg, 0, sizeof(cfg->cfg));
> diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
> index aa1b4cf..a4b87ce 100644
> --- a/include/fsl_esdhc.h
> +++ b/include/fsl_esdhc.h
> @@ -25,10 +25,12 @@
>  #define SYSCTL_INITA		0x08000000
>  #define SYSCTL_TIMEOUT_MASK	0x000f0000
>  #define SYSCTL_CLOCK_MASK	0x0000fff0
> +#if !defined(CONFIG_MX6)

Per your commit message should this be
#if (!defined(CONFIG_MX6) && !defined(CONFIG_MX7))

>  #define SYSCTL_CKEN		0x00000008
>  #define SYSCTL_PEREN		0x00000004
>  #define SYSCTL_HCKEN		0x00000002
>  #define SYSCTL_IPGEN		0x00000001
> +#endif
>  #define SYSCTL_RSTA		0x01000000
>  #define SYSCTL_RSTC		0x02000000
>  #define SYSCTL_RSTD		0x04000000
> 


--
Hector Palacios

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Hector Palacios]

Hi Eric,

On 12/04/2015 07:33 PM, Eric Nelson wrote:
> Hi Fabio and Hector,
> 
> On 12/04/2015 10:43 AM, Eric Nelson wrote:
>> On 12/04/2015 10:38 AM, Eric Nelson wrote:
>>> On 12/04/2015 10:32 AM, Eric Nelson wrote:
>>>> The low four bits of the SYSCTL register are reserved on the USDHC
>>>> controller on i.MX6 and i.MX7 processors, but are used for clocking
>>>> operations on earlier models.
>>>>
>>>> Guard against their usage by hiding the bit mask macros on those
>>>> processors.
>>>>
>>>> These bits are used to prevent glitches when changing clocks on
>>>> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
>>>>
>>>> From the i.MX6DQ RM:
>>>> 	To prevent possible glitch on the card clock, clear the
>>>> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
>>>> 	or DVS in System Control Register) or setting RSTA bit.
>>>>
>>>> Signed-off-by: Eric Nelson <eric@nelint.com>
>>>
>>> I forgot to add an in-reply-to header.
>>>
>>> http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#236651
>>>
>>>
>>
>> Fabio, I haven't been able to reproduce the "mmc erase/ENGcm03648"
>> issue (with or without a code change) for a couple of hours now.
>>
>> Can you give this a spin?
>>
>> It seems unlikely to address the issue unless what we're seeing is a
>> side effect of a glitch while switching clocks.
>>
>>
> 
> I switched back to a v2014.10 release and am able to reproduce the
> issue at will.
> 
> The sysctl patch had no effect, but adding an #ifndef around the
> ENGcm03648 block allows things to proceed.
> 
> +#ifndef CONFIG_FSL_USDHC
>         /* Workaround for ESDHC errata ENGcm03648 */
>         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> ...
> 
> Since the need to poll the DAT0 line is only documented in the
> errata for the i.MX35, I wonder if this isn't the right thing to do.
> 
> The CC bit of irqstat does indicate that the command completed
> and without error (I'm seeing values of 1 in irqstat).
> 
> From what I can tell, the linux kernel doesn't do this test and
> doesn't appear to have any trouble.
> 
> What code base are you running against (u-boot-imx/master)?

I'm running v2015.04 on a non-upstream platform.

> What do you see if you do the same?

The command takes a while and it is erasing all blocks.
I still get a timeout error at the end and a zero number of sectors, though.

=> mmc erase 441000 10000

MMC erase: dev # 0, block # 4460544, count 65536 ... Timeout waiting card ready
0 blocks erased: ERROR


but all blocks were erased correctly.

-- 
H?ctor Palacios

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

Thanks Hector,

On 12/04/2015 11:39 AM, Hector Palacios wrote:
> Hi,
> 
> On 12/04/2015 06:32 PM, Eric Nelson wrote:
>> The low four bits of the SYSCTL register are reserved on the USDHC
>> controller on i.MX6 and i.MX7 processors, but are used for clocking
>> operations on earlier models.
>>
>> Guard against their usage by hiding the bit mask macros on those
>> processors.
>>
...

>> diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
>> index aa1b4cf..a4b87ce 100644
>> --- a/include/fsl_esdhc.h
>> +++ b/include/fsl_esdhc.h
>> @@ -25,10 +25,12 @@
>>  #define SYSCTL_INITA		0x08000000
>>  #define SYSCTL_TIMEOUT_MASK	0x000f0000
>>  #define SYSCTL_CLOCK_MASK	0x0000fff0
>> +#if !defined(CONFIG_MX6)
> 
> Per your commit message should this be
> #if (!defined(CONFIG_MX6) && !defined(CONFIG_MX7))
> 

Good catch.

Will fix in a V2.

[U-Boot] [PATCH] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

Thanks Hector,

On 12/04/2015 11:51 AM, Hector Palacios wrote:
> On 12/04/2015 07:33 PM, Eric Nelson wrote:
>> On 12/04/2015 10:43 AM, Eric Nelson wrote:
...

>> From what I can tell, the linux kernel doesn't do this test and
>> doesn't appear to have any trouble.
>>
>> What code base are you running against (u-boot-imx/master)?
> 
> I'm running v2015.04 on a non-upstream platform.
> 

Thanks.

>> What do you see if you do the same?
> 
> The command takes a while and it is erasing all blocks.
> I still get a timeout error at the end and a zero number of sectors, though.
> 
> => mmc erase 441000 10000
> 
> MMC erase: dev # 0, block # 4460544, count 65536 ... Timeout waiting card ready
> 0 blocks erased: ERROR
> 

Erasing 64K blocks should take a while, but I'm not sure why
you're seeing things die at the end.

U-Boot > time mmc erase 441000 10000
MMC erase: dev # 0, block # 4460544, count 65536 ... 65536 blocks erased: OK
time: 28.704 seconds

I did some more testing to see if the prsstat register is accurately
reflecting the state of dat0 by setting the SION bit in the mux ctrl
register for my DAT0 (SD4_DAT0 on an i.MX6Q).

I found that the pin is reading as low through the GPIO2_PSR register
(bit 8):

U-Boot > mmc erase 4000 1000
MMC erase: dev # 0, block # 16384, count 4096 ... Timeout waiting for
DAT0 to go high!
irqstat 0x00000001
GPIO2_PSR(020a0008) == 0xc80000d6
...

The pin shows up as high when idle, reflecting the internal pullup:

U-Boot > md.l 0x020a0008 1
020a0008: c80001d6                               ....

I've tested with stronger pull-ups without success, and testing
with a longer timeout indicates that this isn't a slew rate problem.

[U-Boot] [PATCH V2] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Eric Nelson]

The low four bits of the SYSCTL register are reserved on the USDHC
controller on i.MX6 and i.MX7 processors, but are used for clocking
operations on earlier models.

Guard against their usage by hiding the bit mask macros on those
processors.

These bits are used to prevent glitches when changing clocks on
i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.

From the i.MX6DQ RM:
	To prevent possible glitch on the card clock, clear the
	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
	or DVS in System Control Register) or setting RSTA bit.

Signed-off-by: Eric Nelson <eric@nelint.com>
---
V2 uses CONFIG_FSL_USDHC instead of CONFIG_MX6 in fsl_esdhc.h
 drivers/mmc/fsl_esdhc.c | 15 +++++++++++++--
 include/fsl_esdhc.h     |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index c5054d6..1ccc576 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -502,15 +502,22 @@ static void set_sysctl(struct mmc *mmc, uint clock)
 
 	clk = (pre_div << 8) | (div << 4);
 
+#ifdef CONFIG_FSL_USDHC
+	esdhc_setbits32(&regs->sysctl, SYSCTL_RSTA);
+#else
 	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
+#endif
 
 	esdhc_clrsetbits32(&regs->sysctl, SYSCTL_CLOCK_MASK, clk);
 
 	udelay(10000);
 
-	clk = SYSCTL_PEREN | SYSCTL_CKEN;
+#ifdef CONFIG_FSL_USDHC
+	esdhc_clrbits32(&regs->sysctl, SYSCTL_RSTA);
+#else
+	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
+#endif
 
-	esdhc_setbits32(&regs->sysctl, clk);
 }
 
 #ifdef CONFIG_FSL_ESDHC_USE_PERIPHERAL_CLK
@@ -585,7 +592,9 @@ static int esdhc_init(struct mmc *mmc)
 	esdhc_write32(&regs->scr, 0x00000040);
 #endif
 
+#ifndef CONFIG_FSL_USDHC
 	esdhc_setbits32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);
+#endif
 
 	/* Set the initial clock speed */
 	mmc_set_clock(mmc, 400000);
@@ -657,8 +666,10 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg)
 	/* First reset the eSDHC controller */
 	esdhc_reset(regs);
 
+#ifndef CONFIG_FSL_USDHC
 	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_HCKEN
 				| SYSCTL_IPGEN | SYSCTL_CKEN);
+#endif
 
 	writel(SDHCI_IRQ_EN_BITS, &regs->irqstaten);
 	memset(&cfg->cfg, 0, sizeof(cfg->cfg));
diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
index aa1b4cf..a4b87ce 100644
--- a/include/fsl_esdhc.h
+++ b/include/fsl_esdhc.h
@@ -25,10 +25,12 @@
 #define SYSCTL_INITA		0x08000000
 #define SYSCTL_TIMEOUT_MASK	0x000f0000
 #define SYSCTL_CLOCK_MASK	0x0000fff0
+#if !defined(CONFIG_FSL_USDHC)
 #define SYSCTL_CKEN		0x00000008
 #define SYSCTL_PEREN		0x00000004
 #define SYSCTL_HCKEN		0x00000002
 #define SYSCTL_IPGEN		0x00000001
+#endif
 #define SYSCTL_RSTA		0x01000000
 #define SYSCTL_RSTC		0x02000000
 #define SYSCTL_RSTD		0x04000000
-- 
2.6.2

[U-Boot] [PATCH V2] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Fabio Estevam]

On Fri, Dec 4, 2015 at 5:32 PM, Eric Nelson <eric@nelint.com> wrote:
> The low four bits of the SYSCTL register are reserved on the USDHC
> controller on i.MX6 and i.MX7 processors, but are used for clocking
> operations on earlier models.
>
> Guard against their usage by hiding the bit mask macros on those
> processors.
>
> These bits are used to prevent glitches when changing clocks on
> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
>
> From the i.MX6DQ RM:
>         To prevent possible glitch on the card clock, clear the
>         FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
>         or DVS in System Control Register) or setting RSTA bit.
>
> Signed-off-by: Eric Nelson <eric@nelint.com>

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>

[U-Boot] [PATCH V2] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Stefano Babic]

On 04/12/2015 20:32, Eric Nelson wrote:
> The low four bits of the SYSCTL register are reserved on the USDHC
> controller on i.MX6 and i.MX7 processors, but are used for clocking
> operations on earlier models.
> 
> Guard against their usage by hiding the bit mask macros on those
> processors.
> 
> These bits are used to prevent glitches when changing clocks on
> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
> 
> From the i.MX6DQ RM:
> 	To prevent possible glitch on the card clock, clear the
> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
> 	or DVS in System Control Register) or setting RSTA bit.
> 
> Signed-off-by: Eric Nelson <eric@nelint.com>
> ---
> V2 uses CONFIG_FSL_USDHC instead of CONFIG_MX6 in fsl_esdhc.h
>  drivers/mmc/fsl_esdhc.c | 15 +++++++++++++--
>  include/fsl_esdhc.h     |  2 ++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index c5054d6..1ccc576 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -502,15 +502,22 @@ static void set_sysctl(struct mmc *mmc, uint clock)
>  
>  	clk = (pre_div << 8) | (div << 4);
>  
> +#ifdef CONFIG_FSL_USDHC
> +	esdhc_setbits32(&regs->sysctl, SYSCTL_RSTA);
> +#else
>  	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
> +#endif
>  
>  	esdhc_clrsetbits32(&regs->sysctl, SYSCTL_CLOCK_MASK, clk);
>  
>  	udelay(10000);
>  
> -	clk = SYSCTL_PEREN | SYSCTL_CKEN;
> +#ifdef CONFIG_FSL_USDHC
> +	esdhc_clrbits32(&regs->sysctl, SYSCTL_RSTA);
> +#else
> +	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> +#endif
>  
> -	esdhc_setbits32(&regs->sysctl, clk);
>  }
>  
>  #ifdef CONFIG_FSL_ESDHC_USE_PERIPHERAL_CLK
> @@ -585,7 +592,9 @@ static int esdhc_init(struct mmc *mmc)
>  	esdhc_write32(&regs->scr, 0x00000040);
>  #endif
>  
> +#ifndef CONFIG_FSL_USDHC
>  	esdhc_setbits32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);
> +#endif
>  
>  	/* Set the initial clock speed */
>  	mmc_set_clock(mmc, 400000);
> @@ -657,8 +666,10 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg)
>  	/* First reset the eSDHC controller */
>  	esdhc_reset(regs);
>  
> +#ifndef CONFIG_FSL_USDHC
>  	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_HCKEN
>  				| SYSCTL_IPGEN | SYSCTL_CKEN);
> +#endif
>  
>  	writel(SDHCI_IRQ_EN_BITS, &regs->irqstaten);
>  	memset(&cfg->cfg, 0, sizeof(cfg->cfg));
> diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h
> index aa1b4cf..a4b87ce 100644
> --- a/include/fsl_esdhc.h
> +++ b/include/fsl_esdhc.h
> @@ -25,10 +25,12 @@
>  #define SYSCTL_INITA		0x08000000
>  #define SYSCTL_TIMEOUT_MASK	0x000f0000
>  #define SYSCTL_CLOCK_MASK	0x0000fff0
> +#if !defined(CONFIG_FSL_USDHC)
>  #define SYSCTL_CKEN		0x00000008
>  #define SYSCTL_PEREN		0x00000004
>  #define SYSCTL_HCKEN		0x00000002
>  #define SYSCTL_IPGEN		0x00000001
> +#endif
>  #define SYSCTL_RSTA		0x01000000
>  #define SYSCTL_RSTC		0x02000000
>  #define SYSCTL_RSTD		0x04000000
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH V2] ARM: imx: fsl_esdhc: fix usage of low 4 bits of sysctl register

[Hector Palacios]

On 12/04/2015 08:32 PM, Eric Nelson wrote:
> The low four bits of the SYSCTL register are reserved on the USDHC
> controller on i.MX6 and i.MX7 processors, but are used for clocking
> operations on earlier models.
> 
> Guard against their usage by hiding the bit mask macros on those
> processors.
> 
> These bits are used to prevent glitches when changing clocks on
> i.MX35 et al. Use the RSTA bit instead for i.MX6 and i.MX7.
> 
> From the i.MX6DQ RM:
> 	To prevent possible glitch on the card clock, clear the
> 	FRC_SDCLK_ON bit when changing clock divisor value(SDCLKFS
> 	or DVS in System Control Register) or setting RSTA bit.
> 
> Signed-off-by: Eric Nelson <eric@nelint.com>

Reviewed-by: Hector Palacios <hector.palacios@digi.com>