[PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[r66093]

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

SD card read was failing (data crc error)on some cards at
maximum possible frequency on P1010(CCB frequency set to 400MHz).
Some clock deviations are also observed at this frequency.
Hence reduced the mmc clock freq.

Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
Singed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
CC: Chris Ball <cjb@laptop.org>
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
---
changes for v2:
	- change the property to compatible for quirks
changes for v3:
	- fix one compile error
changes for v4:
	- use hooks to suspend/resume the special platform
changes for v5:
	- add the Acked-by
changes for v6:
	- move the workaround codes to special platform from header file
changes for v7:
	- don't use quirks to check the platform support

 drivers/mmc/host/sdhci-of-esdhc.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)
 drivers/mmc/host/sdhci-of-esdhc.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 2ef52f4..acac541 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -1,7 +1,7 @@
 /*
  * Freescale eSDHC controller driver.
  *
- * Copyright (c) 2007, 2010 Freescale Semiconductor, Inc.
+ * Copyright (c) 2007, 2010, 2012 Freescale Semiconductor, Inc.
  * Copyright (c) 2009 MontaVista Software, Inc.
  *
  * Authors: Xiaobo Xie <X.Xie@freescale.com>
@@ -14,6 +14,7 @@
  */
 
 #include <linux/io.h>
+#include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/mmc/host.h>
@@ -114,6 +115,25 @@ static unsigned int esdhc_of_get_min_clock(struct sdhci_host *host)
 	return pltfm_host->clock / 256 / 16;
 }
 
+static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	/* Workaround to reduce the clock frequency for p1010 esdhc */
+	if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
+		if (clock == 0) {
+			host->clock = clock;
+			return;
+		}
+
+		if (clock > 20000000)
+			clock -= 5000000;
+		if (clock > 40000000)
+			clock -= 5000000;
+	}
+
+	/* Set the clock */
+	esdhc_set_clock(host, clock);
+}
+
 #ifdef CONFIG_PM
 static u32 esdhc_proctl;
 static void esdhc_of_suspend(struct sdhci_host *host)
@@ -135,7 +155,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.write_l = sdhci_be32bs_writel,
 	.write_w = esdhc_writew,
 	.write_b = esdhc_writeb,
-	.set_clock = esdhc_set_clock,
+	.set_clock = esdhc_of_set_clock,
 	.enable_dma = esdhc_of_enable_dma,
 	.get_max_clock = esdhc_of_get_max_clock,
 	.get_min_clock = esdhc_of_get_min_clock,
-- 
1.7.5.4

Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Chris Ball]

Hi,

On Wed, Feb 08 2012, r66093@freescale.com wrote:
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>
> SD card read was failing (data crc error)on some cards at
> maximum possible frequency on P1010(CCB frequency set to 400MHz).
> Some clock deviations are also observed at this frequency.
> Hence reduced the mmc clock freq.
>
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> Singed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Chris Ball <cjb@laptop.org>
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>

Thanks, just one comment below:

> ---
> changes for v2:
> 	- change the property to compatible for quirks
> changes for v3:
> 	- fix one compile error
> changes for v4:
> 	- use hooks to suspend/resume the special platform
> changes for v5:
> 	- add the Acked-by
> changes for v6:
> 	- move the workaround codes to special platform from header file
> changes for v7:
> 	- don't use quirks to check the platform support
>
>  drivers/mmc/host/sdhci-of-esdhc.c |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
>  drivers/mmc/host/sdhci-of-esdhc.c |   24 ++++++++++++++++++++++--
>  1 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index 2ef52f4..acac541 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -1,7 +1,7 @@
>  /*
>   * Freescale eSDHC controller driver.
>   *
> - * Copyright (c) 2007, 2010 Freescale Semiconductor, Inc.
> + * Copyright (c) 2007, 2010, 2012 Freescale Semiconductor, Inc.
>   * Copyright (c) 2009 MontaVista Software, Inc.
>   *
>   * Authors: Xiaobo Xie <X.Xie@freescale.com>
> @@ -14,6 +14,7 @@
>   */
>  
>  #include <linux/io.h>
> +#include <linux/of.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/mmc/host.h>
> @@ -114,6 +115,25 @@ static unsigned int esdhc_of_get_min_clock(struct sdhci_host *host)
>  	return pltfm_host->clock / 256 / 16;
>  }
>  
> +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	/* Workaround to reduce the clock frequency for p1010 esdhc */
> +	if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
> +		if (clock == 0) {
> +			host->clock = clock;
> +			return;
> +		}

Is this if (clock == 0) necessary?  You'll be calling esdhc_set_clock()
immediately afterwards, and it performs the same test.

> +
> +		if (clock > 20000000)
> +			clock -= 5000000;
> +		if (clock > 40000000)
> +			clock -= 5000000;
> +	}
> +
> +	/* Set the clock */
> +	esdhc_set_clock(host, clock);
> +}
> +
>  #ifdef CONFIG_PM
>  static u32 esdhc_proctl;
>  static void esdhc_of_suspend(struct sdhci_host *host)
> @@ -135,7 +155,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>  	.write_l = sdhci_be32bs_writel,
>  	.write_w = esdhc_writew,
>  	.write_b = esdhc_writeb,
> -	.set_clock = esdhc_set_clock,
> +	.set_clock = esdhc_of_set_clock,
>  	.enable_dma = esdhc_of_enable_dma,
>  	.get_max_clock = esdhc_of_get_max_clock,
>  	.get_min_clock = esdhc_of_get_min_clock,

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Huang Changming-R66093]

> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Wednesday, February 08, 2012 11:07 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Jain Priyanka-
> B32167
> Subject: Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb
> 
> Hi,
> 
> On Wed, Feb 08 2012, r66093@freescale.com wrote:
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >
> > SD card read was failing (data crc error)on some cards at
> > maximum possible frequency on P1010(CCB frequency set to 400MHz).
> > Some clock deviations are also observed at this frequency.
> > Hence reduced the mmc clock freq.
> >
> > Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> > Singed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > CC: Chris Ball <cjb@laptop.org>
> > Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> 
> Thanks, just one comment below:
> 
> > ---
> > changes for v2:
> > 	- change the property to compatible for quirks
> > changes for v3:
> > 	- fix one compile error
> > changes for v4:
> > 	- use hooks to suspend/resume the special platform
> > changes for v5:
> > 	- add the Acked-by
> > changes for v6:
> > 	- move the workaround codes to special platform from header file
> > changes for v7:
> > 	- don't use quirks to check the platform support
> >
> >  drivers/mmc/host/sdhci-of-esdhc.c |   19 ++++++++++++++++++-
> >  1 files changed, 18 insertions(+), 1 deletions(-)
> >  drivers/mmc/host/sdhci-of-esdhc.c |   24 ++++++++++++++++++++++--
> >  1 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> b/drivers/mmc/host/sdhci-of-esdhc.c
> > index 2ef52f4..acac541 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -1,7 +1,7 @@
> >  /*
> >   * Freescale eSDHC controller driver.
> >   *
> > - * Copyright (c) 2007, 2010 Freescale Semiconductor, Inc.
> > + * Copyright (c) 2007, 2010, 2012 Freescale Semiconductor, Inc.
> >   * Copyright (c) 2009 MontaVista Software, Inc.
> >   *
> >   * Authors: Xiaobo Xie <X.Xie@freescale.com>
> > @@ -14,6 +14,7 @@
> >   */
> >
> >  #include <linux/io.h>
> > +#include <linux/of.h>
> >  #include <linux/delay.h>
> >  #include <linux/module.h>
> >  #include <linux/mmc/host.h>
> > @@ -114,6 +115,25 @@ static unsigned int esdhc_of_get_min_clock(struct
> sdhci_host *host)
> >  	return pltfm_host->clock / 256 / 16;
> >  }
> >
> > +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int
> clock)
> > +{
> > +	/* Workaround to reduce the clock frequency for p1010 esdhc */
> > +	if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
> > +		if (clock == 0) {
> > +			host->clock = clock;
> > +			return;
> > +		}
> 
> Is this if (clock == 0) necessary?  You'll be calling esdhc_set_clock()
> immediately afterwards, and it performs the same test.

Yes, I perform this test two times.
The first time, I test the original value (as the parameter from set_clock)
The second time, I test it again, that is because the clock value has been changed or reduced,
I think it is necessary to test it again. 

Thanks
Jerry Huang

Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Chris Ball]

Hi,

On Wed, Feb 08 2012, Huang Changming-R66093 wrote:
>> > +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int
>> clock)
>> > +{
>> > +	/* Workaround to reduce the clock frequency for p1010 esdhc */
>> > +	if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
>> > +		if (clock == 0) {
>> > +			host->clock = clock;
>> > +			return;
>> > +		}
>> 
>> Is this if (clock == 0) necessary?  You'll be calling esdhc_set_clock()
>> immediately afterwards, and it performs the same test.
>
> Yes, I perform this test two times.
> The first time, I test the original value (as the parameter from set_clock)
> The second time, I test it again, that is because the clock value has
> been changed or reduced,
> I think it is necessary to test it again. 

I don't see how it's necessary to test *if it is zero* again, because
you're not going to change or reduce it to zero.  You're going to
subtract 5M/10M from it if it was over 20M/40M, so there is no chance
of it reaching zero.

This isn't a huge deal, but:  could you walk me through an example of
where it's necessary to have the first test for 0, because something
bad would happen if you didn't?  I'm talking about this piece of code:

+       if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
+               if (clock == 0) {
+                       host->clock = clock;
+                       return;
+               }

.. which I think will get caught by the same test in esdhc_set_clock()
when you call that immediately after this test.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Huang Changming-R66093]

Hi, Chris


> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Sunday, February 12, 2012 5:20 AM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Jain Priyanka-B32167
> Subject: Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb
> 
> Hi,
> 
> On Wed, Feb 08 2012, Huang Changming-R66093 wrote:
> >> > +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned
> >> > +int
> >> clock)
> >> > +{
> >> > +	/* Workaround to reduce the clock frequency for p1010 esdhc
> */
> >> > +	if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
> >> > +		if (clock == 0) {
> >> > +			host->clock = clock;
> >> > +			return;
> >> > +		}
> >>
> >> Is this if (clock == 0) necessary?  You'll be calling
> >> esdhc_set_clock() immediately afterwards, and it performs the same
> test.
> >
> > Yes, I perform this test two times.
> > The first time, I test the original value (as the parameter from
> > set_clock) The second time, I test it again, that is because the clock
> > value has been changed or reduced, I think it is necessary to test it
> > again.
> 
> I don't see how it's necessary to test *if it is zero* again, because
> you're not going to change or reduce it to zero.  You're going to
> subtract 5M/10M from it if it was over 20M/40M, so there is no chance of
> it reaching zero.
You are right.
I want to use the below code in header file "drivers/mmc/host/sdhci-esdhc.h":
if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc"))
then I can avoid the twice test of the clock.
do you think if it is reasonable?

Thanks
Jerry Huang

Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Chris Ball]

Hi,

On Sun, Feb 12 2012, Huang Changming-R66093 wrote:
> You are right.
> I want to use the below code in header file "drivers/mmc/host/sdhci-esdhc.h":
> if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc"))
> then I can avoid the twice test of the clock.
> do you think if it is reasonable?

Yes, I think that's okay; let's try it.  Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Chris Ball]

Hi,

On Sun, Feb 12 2012, Chris Ball wrote:
> Hi,
>
> On Sun, Feb 12 2012, Huang Changming-R66093 wrote:
>> You are right.
>> I want to use the below code in header file "drivers/mmc/host/sdhci-esdhc.h":
>> if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc"))
>> then I can avoid the twice test of the clock.
>> do you think if it is reasonable?
>
> Yes, I think that's okay; let's try it.  Thanks,

Actually, I think I'm still missing something.  Why doesn't this work?

+static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+       /* Workaround to reduce the clock frequency for p1010 esdhc */
+       if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
+               if (clock > 20000000)
+                       clock -= 5000000;
+               if (clock > 40000000)
+                       clock -= 5000000;
+       }
+
+       /* Set the clock */
+       esdhc_set_clock(host, clock);
+}

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Huang Changming-R66093]

> 
> On Sun, Feb 12 2012, Chris Ball wrote:
> > Hi,
> >
> > On Sun, Feb 12 2012, Huang Changming-R66093 wrote:
> >> You are right.
> >> I want to use the below code in header file "drivers/mmc/host/sdhci-
> esdhc.h":
> >> if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) then I
> >> can avoid the twice test of the clock.
> >> do you think if it is reasonable?
> >
> > Yes, I think that's okay; let's try it.  Thanks,
> 
> Actually, I think I'm still missing something.  Why doesn't this work?
> 
> +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int
> +clock) {
> +       /* Workaround to reduce the clock frequency for p1010 esdhc */
> +       if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
> +               if (clock > 20000000)
> +                       clock -= 5000000;
> +               if (clock > 40000000)
> +                       clock -= 5000000;
> +       }
> +
> +       /* Set the clock */
> +       esdhc_set_clock(host, clock);
> +}
> 
These codes can work, too.
I forget the clock will be reduced 5M/10M only when the clock is greater than 20MHz/45MHz.
So I should not test the clock in this function.

Thanks.
Jerry Huang

Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Chris Ball]

Hi,

On Mon, Feb 13 2012, Huang Changming-R66093 wrote:
>> Actually, I think I'm still missing something.  Why doesn't this work?
>> 
>> +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int
>> +clock) {
>> +       /* Workaround to reduce the clock frequency for p1010 esdhc */
>> +       if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
>> +               if (clock > 20000000)
>> +                       clock -= 5000000;
>> +               if (clock > 40000000)
>> +                       clock -= 5000000;
>> +       }
>> +
>> +       /* Set the clock */
>> +       esdhc_set_clock(host, clock);
>> +}
>> 
> These codes can work, too.
> I forget the clock will be reduced 5M/10M only when the clock is
> greater than 20MHz/45MHz.
> So I should not test the clock in this function.

Great, thanks -- please test and submit a patch with the code above,
and I'll merge it.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Huang Changming-R66093]

Thanks
Jerry Huang


> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Tuesday, February 14, 2012 4:44 AM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Jain Priyanka-B32167
> Subject: Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb
> 
> Hi,
> 
> On Mon, Feb 13 2012, Huang Changming-R66093 wrote:
> >> Actually, I think I'm still missing something.  Why doesn't this work?
> >>
> >> +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int
> >> +clock) {
> >> +       /* Workaround to reduce the clock frequency for p1010 esdhc */
> >> +       if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
> >> +               if (clock > 20000000)
> >> +                       clock -= 5000000;
> >> +               if (clock > 40000000)
> >> +                       clock -= 5000000;
> >> +       }
> >> +
> >> +       /* Set the clock */
> >> +       esdhc_set_clock(host, clock); }
> >>
> > These codes can work, too.
> > I forget the clock will be reduced 5M/10M only when the clock is
> > greater than 20MHz/45MHz.
> > So I should not test the clock in this function.
> 
> Great, thanks -- please test and submit a patch with the code above, and
> I'll merge it.


Thanks, Chris.
I have sent it again.


Jerry Huang

Latency Issue due to patch; ESDHC: Workaround for data crc error on p1010rdb

[Jain Priyanka-B32167]

Hi,

I am facing latency issue due to below implementation.
The function 'mmc_rescan' is getting called at regular intervals which is calling 'of_find_comaptible_node which is adding to the latency. Trace: 

=> of_find_compatible_node
=> esdhc_of_set_clock
=> sdhci_set_clock
=> sdhci_set_ios
=> mmc_power_up
=> mmc_rescan

'of_find_compatible_node' function call is required only once at Linux boot time. 


If I change the implementation to : 

static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int clock)
{
        static int reduce_clock_flag, check_only_once_flag;
        /* Workaround to reduce the clock frequency for p1010 esdhc */
        if (check_only_once_flag == 0)
        {
                if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc"))
                        reduce_clock_flag = 1;
                check_only_once_flag = 1;
        }
        if (reduce_clock_flag == 1)
        {
                if (clock > 20000000)
                        clock -= 5000000;
                if (clock > 40000000)
                        clock -= 5000000;
        }
        /* Set the clock */
        esdhc_set_clock(host, clock);
}


It works fine. No more latency issue. Please review this approach. If there is a better way, please do suggest.

Thanks 
Priyanka Jain

-----Original Message-----
From: Huang Changming-R66093 
Sent: Tuesday, February 14, 2012 11:41 AM
To: Chris Ball
Cc: linux-mmc@vger.kernel.org; Jain Priyanka-B32167
Subject: RE: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb



Thanks
Jerry Huang


> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Tuesday, February 14, 2012 4:44 AM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Jain Priyanka-B32167
> Subject: Re: [PATCH v7] ESDHC: Workaround for data crc error on 
> p1010rdb
> 
> Hi,
> 
> On Mon, Feb 13 2012, Huang Changming-R66093 wrote:
> >> Actually, I think I'm still missing something.  Why doesn't this work?
> >>
> >> +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned 
> >> +int
> >> +clock) {
> >> +       /* Workaround to reduce the clock frequency for p1010 esdhc */
> >> +       if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
> >> +               if (clock > 20000000)
> >> +                       clock -= 5000000;
> >> +               if (clock > 40000000)
> >> +                       clock -= 5000000;
> >> +       }
> >> +
> >> +       /* Set the clock */
> >> +       esdhc_set_clock(host, clock); }
> >>
> > These codes can work, too.
> > I forget the clock will be reduced 5M/10M only when the clock is 
> > greater than 20MHz/45MHz.
> > So I should not test the clock in this function.
> 
> Great, thanks -- please test and submit a patch with the code above, 
> and I'll merge it.


Thanks, Chris.
I have sent it again.


Jerry Huang