[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Kumar Gala]

From: Priyanka Jain <Priyanka.Jain@freescale.com>

- Timeout counter value is set as DTOCV bits in SYSCTL register
  For counter value set as x,
  Timeout period = (2^(13+x))/SD_CLOCK

- As per 4.6.2.2 section of SD Card specification v2.00, host should
  cofigure timeout period value to minimum 250 msec.

- SD_CLOCK = mmc->trans_speed

- Calculating x based on
  250 msec = (2^(13+x))/mmc->trans_speed

Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
Signed-off-by: Andy Fleming <afleming@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 drivers/mmc/fsl_esdhc.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index c6de751..6f8a09d 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -210,7 +210,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
 	esdhc_write32(&regs->blkattr, data->blocks << 16 | data->blocksize);
 
 	/* Calculate the timeout period for data transactions */
-	timeout = fls(mmc->tran_speed/10) - 1;
+	/* Timeout period = (2^(13+timeout))/mmc->trans_speed
+	 * Timeout period should be minimum 250msec as per SD Card spec
+	 */
+	timeout = fls(mmc->tran_speed/4);
 	timeout -= 13;
 
 	if (timeout > 14)
-- 
1.7.2.3

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Wolfgang Denk]

Dear Kumar Gala,

In message <1298537614-20797-1-git-send-email-galak@kernel.crashing.org> you wrote:
> 
> - Timeout counter value is set as DTOCV bits in SYSCTL register
>   For counter value set as x,
>   Timeout period = (2^(13+x))/SD_CLOCK
> 
> - As per 4.6.2.2 section of SD Card specification v2.00, host should
>   cofigure timeout period value to minimum 250 msec.
> 
> - SD_CLOCK = mmc->trans_speed
> 
> - Calculating x based on
>   250 msec = (2^(13+x))/mmc->trans_speed

OK, here the theory is given...

> +	/* Timeout period = (2^(13+timeout))/mmc->trans_speed
> +	 * Timeout period should be minimum 250msec as per SD Card spec
> +	 */
> +	timeout = fls(mmc->tran_speed/4);
>  	timeout -= 13;

But how does this translate intothis formula?  I think there must be
missing something.

Also, should there not be some checking for valid input data ranges?


Nitpick: incorrect multi-line comment style.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"There's always been Tower of Babel sort of  bickering  inside  Unix,
but  this  is the most extreme form ever. This means at least several
years of confusion." - Bill Gates, founder and chairman of Microsoft,
about the Open Systems Foundation

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Andy Fleming]

On Thu, Feb 24, 2011 at 3:50 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Kumar Gala,
>
> In message <1298537614-20797-1-git-send-email-galak@kernel.crashing.org> you wrote:
>>
>> - Timeout counter value is set as DTOCV bits in SYSCTL register
>> ? For counter value set as x,
>> ? Timeout period = (2^(13+x))/SD_CLOCK
>>
>> - As per 4.6.2.2 section of SD Card specification v2.00, host should
>> ? cofigure timeout period value to minimum 250 msec.
>>
>> - SD_CLOCK = mmc->trans_speed
>>
>> - Calculating x based on
>> ? 250 msec = (2^(13+x))/mmc->trans_speed
>
> OK, here the theory is given...
>
>> + ? ? /* Timeout period = (2^(13+timeout))/mmc->trans_speed
>> + ? ? ?* Timeout period should be minimum 250msec as per SD Card spec
>> + ? ? ?*/
>> + ? ? timeout = fls(mmc->tran_speed/4);
>> ? ? ? timeout -= 13;
>
> But how does this translate intothis formula? ?I think there must be
> missing something.

Yeah, that took me a while, too.  Maybe we should update it to make clear:

1) The formula ends up being (2^(13 + timeout))/mmc->trans_speed = (1/4) seconds
--> 2^(13 + timeout) = mmc->trans_speed/4
--> 13 + timeout = log2(mmc->trans_speed/4)
...etc

2) We are effectively rounding up to the next highest power of 2 to
make sure the timeout is *at least* 250ms

Andy

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Wolfgang Denk]

Dear Andy Fleming,

In message <AANLkTimCt=qiPDC9s2BMYy4nM5R+JWcBiaLLnFo_WA9X@mail.gmail.com> you wrote:
>
> Yeah, that took me a while, too.  Maybe we should update it to make clear:
>
> 1) The formula ends up being (2^(13 + timeout))/mmc->trans_speed = (1/4) seconds
> --> 2^(13 + timeout) = mmc->trans_speed/4
> --> 13 + timeout = log2(mmc->trans_speed/4)
> ...etc

Does this not depend on the units used for speed, and thus in the end
on CONFIC_SYS_HZ ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offence.
          -- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Jain Priyanka-B32167]

Dear Wolgang Denk

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Friday, February 25, 2011 11:06 AM
> To: Andy Fleming
> Cc: Kumar Gala; u-boot at lists.denx.de; Fleming Andy-AFLEMING; Jain
> Priyanka-B32167
> Subject: Re: [U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter
> calculation
> 
> Dear Andy Fleming,
> 
> In message <AANLkTimCt=qiPDC9s2BMYy4nM5R+JWcBiaLLnFo_WA9X@mail.gmail.com>
> you wrote:
> >
> > Yeah, that took me a while, too.  Maybe we should update it to make
> clear:
> >
> > 1) The formula ends up being (2^(13 + timeout))/mmc->trans_speed =
> > (1/4) seconds
> > --> 2^(13 + timeout) = mmc->trans_speed/4
> > --> 13 + timeout = log2(mmc->trans_speed/4)
> > ...etc
> 
> Does this not depend on the units used for speed, and thus in the end on
> CONFIC_SYS_HZ ?
> 
We are using absolute figures. So this code is independent of units of speed.
For standard speed cards, mmc->tran_speed is 25000000.
Code snippet(mmc.c) of setting tran_speed is:

        /* divide frequency by 10, since the mults are 10x bigger */
        freq = fbase[(cmd.response[0] & 0x7)];
        mult = multipliers[((cmd.response[0] >> 3) & 0xf)];
        mmc->tran_speed = freq * mult;

> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de The
> use of COBOL cripples the mind; its teaching  should,  therefore, be
> regarded as a criminal offence.
>           -- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Wolfgang Denk]

Dear Jain Priyanka-B32167,

In message <470DB7CE2CD0944E9436E7ADEFC02FE3138B4C@039-SN1MPN1-003.039d.mgd.msft.net> you wrote:
> 
> > > 1) The formula ends up being (2^(13 + timeout))/mmc->trans_speed =3D
> > > (1/4) seconds
> > > --> 2^(13 + timeout) = mmc->trans_speed/4
> > > --> 13 + timeout = log2(mmc->trans_speed/4)
> > > ...etc
> > 
> > Does this not depend on the units used for speed, and thus in the end on
> > CONFIC_SYS_HZ ?
> > 
> We are using absolute figures. So this code is independent of units of speed.

Funny you should say that.

> For standard speed cards, mmc->tran_speed is 25000000.

"Speed" is always some unit per time.  Your 'absolute" number 25000000
here makes no sense if you cannot tell what it means.  Is it bits per
millisecond? Bytes per week? Nibbles per year?

>         /* divide frequency by 10, since the mults are 10x bigger */
>         freq = fbase[(cmd.response[0] & 0x7)];
>         mult = multipliers[((cmd.response[0] >> 3) & 0xf)];
>         mmc->tran_speed = freq * mult;

You don't have any "absolute figures" here. A frequency is defined to
measure events per time, so it is important to give the unit used.
Is frequency in Hz, or in kHz, or in MHz, or what?


It is impossible for the reader to verify the correctness of the code
if you don't provide such information, so please document your code in
a way that makes it possible to check it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I haven't lost my mind - it's backed up on tape somewhere."

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Fleming Andy-AFLEMING]

On Feb 24, 2011, at 23:35, "Wolfgang Denk" <wd@denx.de> wrote:

> Dear Andy Fleming,
> 
> In message <AANLkTimCt=qiPDC9s2BMYy4nM5R+JWcBiaLLnFo_WA9X@mail.gmail.com> you wrote:
>> 
>> Yeah, that took me a while, too.  Maybe we should update it to make clear:
>> 
>> 1) The formula ends up being (2^(13 + timeout))/mmc->trans_speed = (1/4) seconds
>> --> 2^(13 + timeout) = mmc->trans_speed/4
>> --> 13 + timeout = log2(mmc->trans_speed/4)
>> ...etc
> 
> Does this not depend on the units used for speed, and thus in the end
> on CONFIC_SYS_HZ ?


No, but that wasn't apparent because I didn't mention the units of 2^(13+timeout).  It is in units of sd clocks.

So: num sd clocks = (sd clocks per sec) * 0.25 sec = mmc->tran_speed/4 = 2^(13+regval).

Now, it is true that the actual speed of the sd clock is going to depend on CONFIG_SYS_HZ, in that a tran_speed of 25MHz may not be perfectly achievable with available dividers, but this code is not taking that into account, and the fact that it's rounding up to the next power of two should take care of that.

Andy

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Wolfgang Denk]

Dear Fleming Andy-AFLEMING,

In message <D4848E4B-3C47-4D5C-A171-A69439660C42@freescale.com> you wrote:
> 
> > Does this not depend on the units used for speed, and thus in the end
> > on CONFIC_SYS_HZ ?
> 
> No, but that wasn't apparent because I didn't mention the units of
> 2^(13+timeout). It is in units of sd clocks.
> 
> So: num sd clocks = (sd clocks per sec) * 0.25 sec =
> mmc->tran_speed/4 = 2^(13+regval).
> 
> Now, it is true that the actual speed of the sd clock is going to
> depend on CONFIG_SYS_HZ, in that a tran_speed of 25MHz may not be
> perfectly achievable with available dividers, but this code is not
> taking that into account, and the fact that it's rounding up to the
> next power of two should take care of that.

Fine.  Can we please add such explanation to the code?

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offence.
          -- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Kumar Gala]

On Feb 24, 2011, at 2:53 AM, Kumar Gala wrote:

> From: Priyanka Jain <Priyanka.Jain@freescale.com>
> 
> - Timeout counter value is set as DTOCV bits in SYSCTL register
>  For counter value set as x,
>  Timeout period = (2^(13+x))/SD_CLOCK
> 
> - As per 4.6.2.2 section of SD Card specification v2.00, host should
>  cofigure timeout period value to minimum 250 msec.
> 
> - SD_CLOCK = mmc->trans_speed
> 
> - Calculating x based on
>  250 msec = (2^(13+x))/mmc->trans_speed
> 
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
> Signed-off-by: Andy Fleming <afleming@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> drivers/mmc/fsl_esdhc.c |    5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)

Stefano,

Can you test this on iMX to make sure we didnt break anything there.

> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index c6de751..6f8a09d 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -210,7 +210,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
> 	esdhc_write32(&regs->blkattr, data->blocks << 16 | data->blocksize);
> 
> 	/* Calculate the timeout period for data transactions */
> -	timeout = fls(mmc->tran_speed/10) - 1;
> +	/* Timeout period = (2^(13+timeout))/mmc->trans_speed
> +	 * Timeout period should be minimum 250msec as per SD Card spec
> +	 */
> +	timeout = fls(mmc->tran_speed/4);
> 	timeout -= 13;
> 
> 	if (timeout > 14)
> -- 
> 1.7.2.3
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] fsl_esdhc: Correcting esdhc timeout counter calculation

[Stefano Babic]

On 02/24/2011 11:37 AM, Kumar Gala wrote:
> 
> On Feb 24, 2011, at 2:53 AM, Kumar Gala wrote:
> 
>> From: Priyanka Jain <Priyanka.Jain@freescale.com>
>>
>> - Timeout counter value is set as DTOCV bits in SYSCTL register
>>  For counter value set as x,
>>  Timeout period = (2^(13+x))/SD_CLOCK
>>
>> - As per 4.6.2.2 section of SD Card specification v2.00, host should
>>  cofigure timeout period value to minimum 250 msec.
>>
>> - SD_CLOCK = mmc->trans_speed
>>
>> - Calculating x based on
>>  250 msec = (2^(13+x))/mmc->trans_speed
>>
>> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
>> Signed-off-by: Andy Fleming <afleming@freescale.com>
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>> ---
>> drivers/mmc/fsl_esdhc.c |    5 ++++-
>> 1 files changed, 4 insertions(+), 1 deletions(-)
> 
> Stefano,
> 
> Can you test this on iMX to make sure we didnt break anything there.

Yes, I will test it and I will send my tested-by if everything is ok.

Stefano
-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================