diff mbox

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

Message ID 1298537614-20797-1-git-send-email-galak@kernel.crashing.org
State Superseded
Headers show

Commit Message

Kumar Gala Feb. 24, 2011, 8:53 a.m. UTC 
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(-)

Comments

Wolfgang Denk Feb. 24, 2011, 9:50 a.m. UTC | #1 
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
Kumar Gala Feb. 24, 2011, 10:37 a.m. UTC | #2 
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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Stefano Babic Feb. 24, 2011, 11:11 a.m. UTC | #3 
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
Andy Fleming Feb. 25, 2011, 12:14 a.m. UTC | #4 
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
Wolfgang Denk Feb. 25, 2011, 5:35 a.m. UTC | #5 
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
Jain Priyanka-B32167 Feb. 25, 2011, 9:25 a.m. UTC | #6 
Dear Wolgang Denk

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd@denx.de]
> Sent: Friday, February 25, 2011 11:06 AM
> To: Andy Fleming
> Cc: Kumar Gala; u-boot@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@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
Wolfgang Denk Feb. 25, 2011, 10:19 a.m. UTC | #7 
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
Andy Fleming Feb. 25, 2011, 1:45 p.m. UTC | #8 
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
Wolfgang Denk Feb. 25, 2011, 1:56 p.m. UTC | #9 
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
diff mbox

Patch

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)