[PATCH 0/2] MMCIF Clock Fixes

[PATCH 0/2] MMCIF Clock Fixes

[Simon Horman]

Hi,

the following two patches fix inaccuracies in the mmcif implementation
relating to treatment of the clock. They have been lightly tested and
they do not seem to result in any change in transfer speeds.

[PATCH 1/2] mmc: sh_mmcif: double clock speed

[Simon Horman]

This corrects an off-by one error when calculating the clock divisor.
The code previously assumed that for example a divisor of 2 is
set using a value of 0001 (the inverse of 1/2), a divisor of 4 is
set using a value of 0010 (the inverse of 1/4) etc.. However, the correct
values are 0000, 0001, etc...

The use of DIV_ROUND_UP() was suggested by Guennadi Liakhovetski to avoid
understating the divisor by one in the case where the host clock is not a
binary power of the MMCIF clock.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 drivers/mmc/host/sh_mmcif.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 8057bf3..5014bc4 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -453,7 +453,8 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
 	else
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
-				((fls(host->clk / clk) - 1) << 16));
+				((fls(DIV_ROUND_UP(host->clk,
+						   clk) - 1) - 1) << 16));
 
 	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
 }
-- 
1.7.6.3

Re: [PATCH 1/2] mmc: sh_mmcif: double clock speed

[Guennadi Liakhovetski]

Hi Simon

On Wed, 21 Mar 2012, Simon Horman wrote:

> This corrects an off-by one error when calculating the clock divisor.
> The code previously assumed that for example a divisor of 2 is
> set using a value of 0001 (the inverse of 1/2), a divisor of 4 is
> set using a value of 0010 (the inverse of 1/4) etc.. However, the correct
> values are 0000, 0001, etc...
> 
> The use of DIV_ROUND_UP() was suggested by Guennadi Liakhovetski to avoid
> understating the divisor by one in the case where the host clock is not a
> binary power of the MMCIF clock.

The patch looks good, but the commit message is not quite right. Cases of 
host->clk / clk != 2^n are handled by using fls(x - 1) instead of fls(x) - 
1, as you initially suggested. DIV_ROUND_UP() is needed for divisions with 
a residue. E.g., for host clock 13MHz, target clock 3MHz you want a 
divisor of 5, resulting in 2 << 16, not 4, resulting in 1 << 16. With this 
fixed

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> 
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> ---
>  drivers/mmc/host/sh_mmcif.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 8057bf3..5014bc4 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -453,7 +453,8 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
>  	else
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
> -				((fls(host->clk / clk) - 1) << 16));
> +				((fls(DIV_ROUND_UP(host->clk,
> +						   clk) - 1) - 1) << 16));
>  
>  	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
>  }
> -- 
> 1.7.6.3
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 1/2] mmc: sh_mmcif: double clock speed

[Simon Horman]

On Sat, Mar 24, 2012 at 06:56:16PM +0100, Guennadi Liakhovetski wrote:
> Hi Simon
> 
> On Wed, 21 Mar 2012, Simon Horman wrote:
> 
> > This corrects an off-by one error when calculating the clock divisor.
> > The code previously assumed that for example a divisor of 2 is
> > set using a value of 0001 (the inverse of 1/2), a divisor of 4 is
> > set using a value of 0010 (the inverse of 1/4) etc.. However, the correct
> > values are 0000, 0001, etc...
> > 
> > The use of DIV_ROUND_UP() was suggested by Guennadi Liakhovetski to avoid
> > understating the divisor by one in the case where the host clock is not a
> > binary power of the MMCIF clock.
> 
> The patch looks good, but the commit message is not quite right. Cases of 
> host->clk / clk != 2^n are handled by using fls(x - 1) instead of fls(x) - 
> 1, as you initially suggested. DIV_ROUND_UP() is needed for divisions with 
> a residue. E.g., for host clock 13MHz, target clock 3MHz you want a 
> divisor of 5, resulting in 2 << 16, not 4, resulting in 1 << 16. With this 
> fixed
> 
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Hi Guennadi, how about this?:

Correct an off-by one error when calculating the clock divisor in cases
where the host clock is a power of two of the target clock.  Previously the
divisor was one greater than the correct value in these cases leading to
the clock being set at half the desired speed.

Thanks to Guennadi Liakhovetski for working with me on
the logic for this change.

> > The use of DIV_ROUND_UP() was suggested by Guennadi Liakhovetski to avoid
> > understating the divisor by one in the case where the host clock is not a
> > binary power of the MMCIF clock.
> 
> The patch looks good, but the commit message is not quite right. Cases of 
> host->clk / clk != 2^n are handled by using fls(x - 1) instead of fls(x) - 
> 1, as you initially suggested. DIV_ROUND_UP() is needed for divisions with 
> a residue. E.g., for host clock 13MHz, target clock 3MHz you want a 
> divisor of 5, resulting in 2 << 16, not 4, resulting in 1 << 16. With this 
> fixed
> 
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

> Thanks
> Guennadi
> 
> > 
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> > ---
> >  drivers/mmc/host/sh_mmcif.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > index 8057bf3..5014bc4 100644
> > --- a/drivers/mmc/host/sh_mmcif.c
> > +++ b/drivers/mmc/host/sh_mmcif.c
> > @@ -453,7 +453,8 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
> >  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
> >  	else
> >  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
> > -				((fls(host->clk / clk) - 1) << 16));
> > +				((fls(DIV_ROUND_UP(host->clk,
> > +						   clk) - 1) - 1) << 16));
> >  
> >  	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
> >  }
> > -- 
> > 1.7.6.3
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 

[PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Simon Horman]

mmc->f_max should be half of the bus clock.
And now that mmc->f_max is not equal to the bus clock the
latter should be used directly to calculate mmc->f_min.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 drivers/mmc/host/sh_mmcif.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 5014bc4..1410baa 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1297,14 +1297,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	spin_lock_init(&host->lock);
 
 	mmc->ops = &sh_mmcif_ops;
-	mmc->f_max = host->clk;
+	mmc->f_max = host->clk / 2;
 	/* close to 400KHz */
-	if (mmc->f_max < 51200000)
-		mmc->f_min = mmc->f_max / 128;
-	else if (mmc->f_max < 102400000)
-		mmc->f_min = mmc->f_max / 256;
+	if (host->clk < 51200000)
+		mmc->f_min = host->clk / 128;
+	else if (host->clk < 102400000)
+		mmc->f_min = host->clk / 256;
 	else
-		mmc->f_min = mmc->f_max / 512;
+		mmc->f_min = host->clk / 512;
 	if (pd->ocr)
 		mmc->ocr_avail = pd->ocr;
 	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
-- 
1.7.6.3

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Guennadi Liakhovetski]

On Wed, 21 Mar 2012, Simon Horman wrote:

> mmc->f_max should be half of the bus clock.
> And now that mmc->f_max is not equal to the bus clock the
> latter should be used directly to calculate mmc->f_min.

The patch seems correct as it stands, however, looking at it - does anyone 
understands why that "close to 400kHz" comment and such a complicated 
calculation? Shouldn't it be just host->clk / 512 always? Maybe this 
should be a separate patch, so, for this one

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi

> 
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> ---
>  drivers/mmc/host/sh_mmcif.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 5014bc4..1410baa 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1297,14 +1297,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	spin_lock_init(&host->lock);
>  
>  	mmc->ops = &sh_mmcif_ops;
> -	mmc->f_max = host->clk;
> +	mmc->f_max = host->clk / 2;
>  	/* close to 400KHz */
> -	if (mmc->f_max < 51200000)
> -		mmc->f_min = mmc->f_max / 128;
> -	else if (mmc->f_max < 102400000)
> -		mmc->f_min = mmc->f_max / 256;
> +	if (host->clk < 51200000)
> +		mmc->f_min = host->clk / 128;
> +	else if (host->clk < 102400000)
> +		mmc->f_min = host->clk / 256;
>  	else
> -		mmc->f_min = mmc->f_max / 512;
> +		mmc->f_min = host->clk / 512;
>  	if (pd->ocr)
>  		mmc->ocr_avail = pd->ocr;
>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> -- 
> 1.7.6.3
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Yusuke Goda]

Hi Simon-san, Guennadi-san

(2012/03/26 7:30), Simon Horman wrote:
> On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
>> On Wed, 21 Mar 2012, Simon Horman wrote:
>>
>>> mmc->f_max should be half of the bus clock.
>>> And now that mmc->f_max is not equal to the bus clock the
>>> latter should be used directly to calculate mmc->f_min.
>>
>> The patch seems correct as it stands, however, looking at it - does anyone 
>> understands why that "close to 400kHz" comment and such a complicated 
>> calculation? Shouldn't it be just host->clk / 512 always? Maybe this 
>> should be a separate patch, so, for this one
>>
>> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> Hi Guennadi,
> 
> that code seems to date back to the original driver submission
> made by Goda-san. I have CCed him as perhaps he recalls why
> the code is like it is.
I thought to get closer to 400kHz if possible.
Probably even host->clk / 512 does not have any problem.

cheers,
Goda

>>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
>>> Signed-off-by: Simon Horman <horms@verge.net.au>
>>> ---
>>>  drivers/mmc/host/sh_mmcif.c |   12 ++++++------
>>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
>>> index 5014bc4..1410baa 100644
>>> --- a/drivers/mmc/host/sh_mmcif.c
>>> +++ b/drivers/mmc/host/sh_mmcif.c
>>> @@ -1297,14 +1297,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>>>  	spin_lock_init(&host->lock);
>>>  
>>>  	mmc->ops = &sh_mmcif_ops;
>>> -	mmc->f_max = host->clk;
>>> +	mmc->f_max = host->clk / 2;
>>>  	/* close to 400KHz */
>>> -	if (mmc->f_max < 51200000)
>>> -		mmc->f_min = mmc->f_max / 128;
>>> -	else if (mmc->f_max < 102400000)
>>> -		mmc->f_min = mmc->f_max / 256;
>>> +	if (host->clk < 51200000)
>>> +		mmc->f_min = host->clk / 128;
>>> +	else if (host->clk < 102400000)
>>> +		mmc->f_min = host->clk / 256;
>>>  	else
>>> -		mmc->f_min = mmc->f_max / 512;
>>> +		mmc->f_min = host->clk / 512;
>>>  	if (pd->ocr)
>>>  		mmc->ocr_avail = pd->ocr;
>>>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
>>> -- 
>>> 1.7.6.3
>>>
>>
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>> http://www.open-technology.de/
>>
> 
> 

-- 
EC No.

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Simon Horman]

On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
> Hi Simon-san, Guennadi-san
> 
> (2012/03/26 7:30), Simon Horman wrote:
> > On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
> >> On Wed, 21 Mar 2012, Simon Horman wrote:
> >>
> >>> mmc->f_max should be half of the bus clock.
> >>> And now that mmc->f_max is not equal to the bus clock the
> >>> latter should be used directly to calculate mmc->f_min.
> >>
> >> The patch seems correct as it stands, however, looking at it - does anyone 
> >> understands why that "close to 400kHz" comment and such a complicated 
> >> calculation? Shouldn't it be just host->clk / 512 always? Maybe this 
> >> should be a separate patch, so, for this one
> >>
> >> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > Hi Guennadi,
> > 
> > that code seems to date back to the original driver submission
> > made by Goda-san. I have CCed him as perhaps he recalls why
> > the code is like it is.
> I thought to get closer to 400kHz if possible.
> Probably even host->clk / 512 does not have any problem.

Sorry for my ignorance, is ~400kHz desirable for some reason?

> cheers,
> Goda
> 
> >>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>> Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
> >>> Signed-off-by: Simon Horman <horms@verge.net.au>
> >>> ---
> >>>  drivers/mmc/host/sh_mmcif.c |   12 ++++++------
> >>>  1 files changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> >>> index 5014bc4..1410baa 100644
> >>> --- a/drivers/mmc/host/sh_mmcif.c
> >>> +++ b/drivers/mmc/host/sh_mmcif.c
> >>> @@ -1297,14 +1297,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >>>  	spin_lock_init(&host->lock);
> >>>  
> >>>  	mmc->ops = &sh_mmcif_ops;
> >>> -	mmc->f_max = host->clk;
> >>> +	mmc->f_max = host->clk / 2;
> >>>  	/* close to 400KHz */
> >>> -	if (mmc->f_max < 51200000)
> >>> -		mmc->f_min = mmc->f_max / 128;
> >>> -	else if (mmc->f_max < 102400000)
> >>> -		mmc->f_min = mmc->f_max / 256;
> >>> +	if (host->clk < 51200000)
> >>> +		mmc->f_min = host->clk / 128;
> >>> +	else if (host->clk < 102400000)
> >>> +		mmc->f_min = host->clk / 256;
> >>>  	else
> >>> -		mmc->f_min = mmc->f_max / 512;
> >>> +		mmc->f_min = host->clk / 512;
> >>>  	if (pd->ocr)
> >>>  		mmc->ocr_avail = pd->ocr;
> >>>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> >>> -- 
> >>> 1.7.6.3
> >>>
> >>
> >> ---
> >> Guennadi Liakhovetski, Ph.D.
> >> Freelance Open-Source Software Developer
> >> http://www.open-technology.de/
> >>
> > 
> > 
> 
> -- 
> EC No.
> 

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Yusuke Goda]

Hi Simon-san

(2012/03/26 14:52), Simon Horman wrote:
> On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
>> Hi Simon-san, Guennadi-san
>>
>> (2012/03/26 7:30), Simon Horman wrote:
>>> On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
>>>> On Wed, 21 Mar 2012, Simon Horman wrote:
>>>>
>>>>> mmc->f_max should be half of the bus clock.
>>>>> And now that mmc->f_max is not equal to the bus clock the
>>>>> latter should be used directly to calculate mmc->f_min.
>>>>
>>>> The patch seems correct as it stands, however, looking at it - does anyone 
>>>> understands why that "close to 400kHz" comment and such a complicated 
>>>> calculation? Shouldn't it be just host->clk / 512 always? Maybe this 
>>>> should be a separate patch, so, for this one
>>>>
>>>> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>
>>> Hi Guennadi,
>>>
>>> that code seems to date back to the original driver submission
>>> made by Goda-san. I have CCed him as perhaps he recalls why
>>> the code is like it is.
>> I thought to get closer to 400kHz if possible.
>> Probably even host->clk / 512 does not have any problem.
> 
> Sorry for my ignorance, is ~400kHz desirable for some reason?
Initialization time becomes short.

Thanks
Goda


>>>>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>>> Tested-by: Cao Minh Hiep <hiepcm@gmail.com>
>>>>> Signed-off-by: Simon Horman <horms@verge.net.au>
>>>>> ---
>>>>>  drivers/mmc/host/sh_mmcif.c |   12 ++++++------
>>>>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
>>>>> index 5014bc4..1410baa 100644
>>>>> --- a/drivers/mmc/host/sh_mmcif.c
>>>>> +++ b/drivers/mmc/host/sh_mmcif.c
>>>>> @@ -1297,14 +1297,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>>>>>  	spin_lock_init(&host->lock);
>>>>>  
>>>>>  	mmc->ops = &sh_mmcif_ops;
>>>>> -	mmc->f_max = host->clk;
>>>>> +	mmc->f_max = host->clk / 2;
>>>>>  	/* close to 400KHz */
>>>>> -	if (mmc->f_max < 51200000)
>>>>> -		mmc->f_min = mmc->f_max / 128;
>>>>> -	else if (mmc->f_max < 102400000)
>>>>> -		mmc->f_min = mmc->f_max / 256;
>>>>> +	if (host->clk < 51200000)
>>>>> +		mmc->f_min = host->clk / 128;
>>>>> +	else if (host->clk < 102400000)
>>>>> +		mmc->f_min = host->clk / 256;
>>>>>  	else
>>>>> -		mmc->f_min = mmc->f_max / 512;
>>>>> +		mmc->f_min = host->clk / 512;
>>>>>  	if (pd->ocr)
>>>>>  		mmc->ocr_avail = pd->ocr;
>>>>>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
>>>>> -- 
>>>>> 1.7.6.3
>>>>>
>>>>
>>>> ---
>>>> Guennadi Liakhovetski, Ph.D.
>>>> Freelance Open-Source Software Developer
>>>> http://www.open-technology.de/
>>>>
>>>
>>>
>>

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Magnus Damm]

On Mon, Mar 26, 2012 at 2:52 PM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
>> Hi Simon-san, Guennadi-san
>>
>> (2012/03/26 7:30), Simon Horman wrote:
>> > On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
>> >> On Wed, 21 Mar 2012, Simon Horman wrote:
>> >>
>> >>> mmc->f_max should be half of the bus clock.
>> >>> And now that mmc->f_max is not equal to the bus clock the
>> >>> latter should be used directly to calculate mmc->f_min.
>> >>
>> >> The patch seems correct as it stands, however, looking at it - does anyone
>> >> understands why that "close to 400kHz" comment and such a complicated
>> >> calculation? Shouldn't it be just host->clk / 512 always? Maybe this
>> >> should be a separate patch, so, for this one
>> >>
>> >> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >
>> > Hi Guennadi,
>> >
>> > that code seems to date back to the original driver submission
>> > made by Goda-san. I have CCed him as perhaps he recalls why
>> > the code is like it is.
>> I thought to get closer to 400kHz if possible.
>> Probably even host->clk / 512 does not have any problem.
>
> Sorry for my ignorance, is ~400kHz desirable for some reason?

The 400kHz frequency is used during initialization of the SD card.
Simply put, the SD frequency starts low out low and is then changed to
something higher depending on the capability of the memory card and
the host controller. Please have a look at the simplified SD
specification for more details.

Cheers,

/ magnus

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Guennadi Liakhovetski]

On Mon, 26 Mar 2012, Magnus Damm wrote:

> On Mon, Mar 26, 2012 at 2:52 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
> >> Hi Simon-san, Guennadi-san
> >>
> >> (2012/03/26 7:30), Simon Horman wrote:
> >> > On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
> >> >> On Wed, 21 Mar 2012, Simon Horman wrote:
> >> >>
> >> >>> mmc->f_max should be half of the bus clock.
> >> >>> And now that mmc->f_max is not equal to the bus clock the
> >> >>> latter should be used directly to calculate mmc->f_min.
> >> >>
> >> >> The patch seems correct as it stands, however, looking at it - does anyone
> >> >> understands why that "close to 400kHz" comment and such a complicated
> >> >> calculation? Shouldn't it be just host->clk / 512 always? Maybe this
> >> >> should be a separate patch, so, for this one
> >> >>
> >> >> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> >
> >> > Hi Guennadi,
> >> >
> >> > that code seems to date back to the original driver submission
> >> > made by Goda-san. I have CCed him as perhaps he recalls why
> >> > the code is like it is.
> >> I thought to get closer to 400kHz if possible.
> >> Probably even host->clk / 512 does not have any problem.
> >
> > Sorry for my ignorance, is ~400kHz desirable for some reason?
> 
> The 400kHz frequency is used during initialization of the SD card.
> Simply put, the SD frequency starts low out low and is then changed to
> something higher depending on the capability of the memory card and
> the host controller. Please have a look at the simplified SD
> specification for more details.

I see, so, we want something like

	shift = fls(host->clk / 400000 - 1);
	mmc->f_min = host->clk >> shift;

eventually with some rounding, depending on our preferences.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Guennadi Liakhovetski]

On Mon, 26 Mar 2012, Guennadi Liakhovetski wrote:

> On Mon, 26 Mar 2012, Magnus Damm wrote:
> 
> > On Mon, Mar 26, 2012 at 2:52 PM, Simon Horman <horms@verge.net.au> wrote:
> > > On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
> > >> Hi Simon-san, Guennadi-san
> > >>
> > >> (2012/03/26 7:30), Simon Horman wrote:
> > >> > On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
> > >> >> On Wed, 21 Mar 2012, Simon Horman wrote:
> > >> >>
> > >> >>> mmc->f_max should be half of the bus clock.
> > >> >>> And now that mmc->f_max is not equal to the bus clock the
> > >> >>> latter should be used directly to calculate mmc->f_min.
> > >> >>
> > >> >> The patch seems correct as it stands, however, looking at it - does anyone
> > >> >> understands why that "close to 400kHz" comment and such a complicated
> > >> >> calculation? Shouldn't it be just host->clk / 512 always? Maybe this
> > >> >> should be a separate patch, so, for this one
> > >> >>
> > >> >> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > >> >
> > >> > Hi Guennadi,
> > >> >
> > >> > that code seems to date back to the original driver submission
> > >> > made by Goda-san. I have CCed him as perhaps he recalls why
> > >> > the code is like it is.
> > >> I thought to get closer to 400kHz if possible.
> > >> Probably even host->clk / 512 does not have any problem.
> > >
> > > Sorry for my ignorance, is ~400kHz desirable for some reason?
> > 
> > The 400kHz frequency is used during initialization of the SD card.
> > Simply put, the SD frequency starts low out low and is then changed to
> > something higher depending on the capability of the memory card and
> > the host controller. Please have a look at the simplified SD
> > specification for more details.
> 
> I see, so, we want something like
> 
> 	shift = fls(host->clk / 400000 - 1);
> 	mmc->f_min = host->clk >> shift;
> 
> eventually with some rounding, depending on our preferences.

As you see in the email header, that last email was sent about 24 hours 
ago, it just got stuck on my mail server due to some local problem... 
After the recent discussions on this topic, this mail is outdated and 
should be disregarded. I certainly agree to using clk / 512 
unconditionally for all host frequencies, which also was my original 
suggestion in

http://article.gmane.org/gmane.linux.kernel.mmc/13556

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Simon Horman]

On Tue, Mar 27, 2012 at 09:53:33AM +0200, Guennadi Liakhovetski wrote:
> On Mon, 26 Mar 2012, Guennadi Liakhovetski wrote:
> 
> > On Mon, 26 Mar 2012, Magnus Damm wrote:
> > 
> > > On Mon, Mar 26, 2012 at 2:52 PM, Simon Horman <horms@verge.net.au> wrote:
> > > > On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
> > > >> Hi Simon-san, Guennadi-san
> > > >>
> > > >> (2012/03/26 7:30), Simon Horman wrote:
> > > >> > On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
> > > >> >> On Wed, 21 Mar 2012, Simon Horman wrote:
> > > >> >>
> > > >> >>> mmc->f_max should be half of the bus clock.
> > > >> >>> And now that mmc->f_max is not equal to the bus clock the
> > > >> >>> latter should be used directly to calculate mmc->f_min.
> > > >> >>
> > > >> >> The patch seems correct as it stands, however, looking at it - does anyone
> > > >> >> understands why that "close to 400kHz" comment and such a complicated
> > > >> >> calculation? Shouldn't it be just host->clk / 512 always? Maybe this
> > > >> >> should be a separate patch, so, for this one
> > > >> >>
> > > >> >> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > >> >
> > > >> > Hi Guennadi,
> > > >> >
> > > >> > that code seems to date back to the original driver submission
> > > >> > made by Goda-san. I have CCed him as perhaps he recalls why
> > > >> > the code is like it is.
> > > >> I thought to get closer to 400kHz if possible.
> > > >> Probably even host->clk / 512 does not have any problem.
> > > >
> > > > Sorry for my ignorance, is ~400kHz desirable for some reason?
> > > 
> > > The 400kHz frequency is used during initialization of the SD card.
> > > Simply put, the SD frequency starts low out low and is then changed to
> > > something higher depending on the capability of the memory card and
> > > the host controller. Please have a look at the simplified SD
> > > specification for more details.
> > 
> > I see, so, we want something like
> > 
> > 	shift = fls(host->clk / 400000 - 1);
> > 	mmc->f_min = host->clk >> shift;
> > 
> > eventually with some rounding, depending on our preferences.
> 
> As you see in the email header, that last email was sent about 24 hours 
> ago, it just got stuck on my mail server due to some local problem... 
> After the recent discussions on this topic, this mail is outdated and 
> should be disregarded. I certainly agree to using clk / 512 
> unconditionally for all host frequencies, which also was my original 
> suggestion in
> 
> http://article.gmane.org/gmane.linux.kernel.mmc/13556

Indeed it was your suggestion and a good one too.

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Simon Horman]

On Mon, Mar 26, 2012 at 03:17:21PM +0900, Magnus Damm wrote:
> On Mon, Mar 26, 2012 at 2:52 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
> >> Hi Simon-san, Guennadi-san
> >>
> >> (2012/03/26 7:30), Simon Horman wrote:
> >> > On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
> >> >> On Wed, 21 Mar 2012, Simon Horman wrote:
> >> >>
> >> >>> mmc->f_max should be half of the bus clock.
> >> >>> And now that mmc->f_max is not equal to the bus clock the
> >> >>> latter should be used directly to calculate mmc->f_min.
> >> >>
> >> >> The patch seems correct as it stands, however, looking at it - does anyone
> >> >> understands why that "close to 400kHz" comment and such a complicated
> >> >> calculation? Shouldn't it be just host->clk / 512 always? Maybe this
> >> >> should be a separate patch, so, for this one
> >> >>
> >> >> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> >
> >> > Hi Guennadi,
> >> >
> >> > that code seems to date back to the original driver submission
> >> > made by Goda-san. I have CCed him as perhaps he recalls why
> >> > the code is like it is.
> >> I thought to get closer to 400kHz if possible.
> >> Probably even host->clk / 512 does not have any problem.
> >
> > Sorry for my ignorance, is ~400kHz desirable for some reason?
> 
> The 400kHz frequency is used during initialization of the SD card.
> Simply put, the SD frequency starts low out low and is then changed to
> something higher depending on the capability of the memory card and
> the host controller. Please have a look at the simplified SD
> specification for more details.

Thanks, got it.

Do you have a feeling of if it it worth trying to start with a value close
to 400kHz or if it would be better to simplify the code? I can try and
measure the difference in start up time for particular hardware
combinations, but I'm not sure how far that will get us.

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Magnus Damm]

On Tue, Mar 27, 2012 at 10:43 AM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Mar 26, 2012 at 03:17:21PM +0900, Magnus Damm wrote:
>> On Mon, Mar 26, 2012 at 2:52 PM, Simon Horman <horms@verge.net.au> wrote:
>> > On Mon, Mar 26, 2012 at 02:45:30PM +0900, Yusuke Goda wrote:
>> >> Hi Simon-san, Guennadi-san
>> >>
>> >> (2012/03/26 7:30), Simon Horman wrote:
>> >> > On Sat, Mar 24, 2012 at 07:06:31PM +0100, Guennadi Liakhovetski wrote:
>> >> >> On Wed, 21 Mar 2012, Simon Horman wrote:
>> >> >>
>> >> >>> mmc->f_max should be half of the bus clock.
>> >> >>> And now that mmc->f_max is not equal to the bus clock the
>> >> >>> latter should be used directly to calculate mmc->f_min.
>> >> >>
>> >> >> The patch seems correct as it stands, however, looking at it - does anyone
>> >> >> understands why that "close to 400kHz" comment and such a complicated
>> >> >> calculation? Shouldn't it be just host->clk / 512 always? Maybe this
>> >> >> should be a separate patch, so, for this one
>> >> >>
>> >> >> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >> >
>> >> > Hi Guennadi,
>> >> >
>> >> > that code seems to date back to the original driver submission
>> >> > made by Goda-san. I have CCed him as perhaps he recalls why
>> >> > the code is like it is.
>> >> I thought to get closer to 400kHz if possible.
>> >> Probably even host->clk / 512 does not have any problem.
>> >
>> > Sorry for my ignorance, is ~400kHz desirable for some reason?
>>
>> The 400kHz frequency is used during initialization of the SD card.
>> Simply put, the SD frequency starts low out low and is then changed to
>> something higher depending on the capability of the memory card and
>> the host controller. Please have a look at the simplified SD
>> specification for more details.
>
> Thanks, got it.
>
> Do you have a feeling of if it it worth trying to start with a value close
> to 400kHz or if it would be better to simplify the code? I can try and
> measure the difference in start up time for particular hardware
> combinations, but I'm not sure how far that will get us.

I believe the correct way is to program the hardware to be as close to
400 kHz as possible. I may be wrong, but I guess that slower than 400
kHz is also acceptable during the initial phase, but faster may mean
out of spec. For optimal performance the code may need to be reworked
to support both correct and slow 400 kHz as well as whatever high
frequencies needed for fast transfers. It is easier said than done
though. Whenever the common clock framework is in place then we should
be able to reprogram the parent rate and with that get a wider range
of frequencies compared with today. If this will work or not depends
on the hardware - on some SoCs the parent clock is shared with other
peripherals which means we may not be able to change it during
runtime. Other SoCs have individual clocks tied to each MMC controller
which gives us freedom to change the frequency during runtime.

I would not bother optimizing this unless you have deep knowledge
about a wide range of SoCs including clocks and the MMC controller
hardware. So for now I think it's best to aim for correctness only.

Also, what is correct or not varies with SoC model. Since our MMC
controller drivers run on a wide range of hardware it is important to
compare the MMC portion of the data sheet. So in this particular case
it is a good idea to make sure that whatever frequency limitations
that are present in one SoC is also true on other models.

Cheers,

/ magnus

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Chris Ball]

Hi,

On Mon, Mar 26 2012, Magnus Damm wrote:
>> Do you have a feeling of if it it worth trying to start with a value close
>> to 400kHz or if it would be better to simplify the code? I can try and
>> measure the difference in start up time for particular hardware
>> combinations, but I'm not sure how far that will get us.
>
> I believe the correct way is to program the hardware to be as close to
> 400 kHz as possible. I may be wrong, but I guess that slower than 400
> kHz is also acceptable during the initial phase, but faster may mean
> out of spec. For optimal performance the code may need to be reworked
> to support both correct and slow 400 kHz as well as whatever high
> frequencies needed for fast transfers.

Hm, I think I'm missing something -- you shouldn't need to optimize f_min
in the driver at all, because the core handles retrying at lower frequencies
in the init phase (before switching to the higher frequency that comes from
the CSD) and it always begins at 400KHz if that's above f_min.  In core.c:

void mmc_rescan(struct work_struct *work)
{
        static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
        ...
        for (i = 0; i < ARRAY_SIZE(freqs); i++) {
                if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
                ...

So, why would you want f_min to be near 400KHz?

Thanks,

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

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Magnus Damm]

Hi Chris,

On Tue, Mar 27, 2012 at 12:20 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Mon, Mar 26 2012, Magnus Damm wrote:
>>> Do you have a feeling of if it it worth trying to start with a value close
>>> to 400kHz or if it would be better to simplify the code? I can try and
>>> measure the difference in start up time for particular hardware
>>> combinations, but I'm not sure how far that will get us.
>>
>> I believe the correct way is to program the hardware to be as close to
>> 400 kHz as possible. I may be wrong, but I guess that slower than 400
>> kHz is also acceptable during the initial phase, but faster may mean
>> out of spec. For optimal performance the code may need to be reworked
>> to support both correct and slow 400 kHz as well as whatever high
>> frequencies needed for fast transfers.
>
> Hm, I think I'm missing something -- you shouldn't need to optimize f_min
> in the driver at all, because the core handles retrying at lower frequencies
> in the init phase (before switching to the higher frequency that comes from
> the CSD) and it always begins at 400KHz if that's above f_min.  In core.c:
>
> void mmc_rescan(struct work_struct *work)
> {
>        static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>        ...
>        for (i = 0; i < ARRAY_SIZE(freqs); i++) {
>                if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
>                ...
>
> So, why would you want f_min to be near 400KHz?

First of all, sorry to cause confusion. It is most likely me missing
something. =)

The reason we want f_min to be near 400 kHz is that this moves our
window of allowed frequencies as high as possible. We could go lower
than 400 kHz but due to hardware limitations that may also lower
f_max.

So thanks to your pointer I can see that a range of frequencies are
being tried in mmc_rescan(). I can see how f_init is being set in
mmc_rescan_try_freq() and the value is being passed down in
mmc_power_up() which in turn sets ios.clock to f_init and calls
mmc_set_ios(). This regardless of f_min in this case - perhaps worth
comparing to the WARN_ON() comparison with f_min in __mmc_set_clock().
I know too little about the MMC subsystem to say anything about that.

Anyway, I was mainly wondering about the setup of the clocks in the
driver for the MMC host hardware. I got the impression that f_min and
f_max are supposed to be used to point out the minimum and maximum
allowed frequencies that the hardware supports. On our SoCs this
depends on the rate of the parent clock and the number of bits used
for the MMC host hardware divider.

So the parent clock rate may be rather high, and if it happens to be
set too high on a board level then the MMC hardware block won't be
able to program the frequencies as low as 400 kHz. So my point is only
that we need to make sure that the parent clock rate is set in such a
way that we can have some at least half-high clock frequency for
decent general purpose throughput but if we try to increase the clock
rate too much then we may force the lowest clock rate to go above 400
kHz and then I suspect we may be out of spec. All this is based on
that the parent clock is set statically to some frequency during boot
by the SoC or board code.

Some device drivers may set the parent clock rate in ->set_ios() but
we do not at this point. We keep the parent rate fixed and just adjust
the frequency in the range that the on-MMC-controller divider allows
us to do without affecting the rest of the system. The code could be
extended to also set the parent clock but due to clock dependencies
related to the SoC clock topology we may not be able to change those
frequencies during runtime.

Anyway, I hope this clarifies how I see things!

Thanks,

/ magnus

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Simon Horman]

On Tue, Mar 27, 2012 at 01:02:38PM +0900, Magnus Damm wrote:
> Hi Chris,
> 
> On Tue, Mar 27, 2012 at 12:20 PM, Chris Ball <cjb@laptop.org> wrote:
> > Hi,
> >
> > On Mon, Mar 26 2012, Magnus Damm wrote:
> >>> Do you have a feeling of if it it worth trying to start with a value close
> >>> to 400kHz or if it would be better to simplify the code? I can try and
> >>> measure the difference in start up time for particular hardware
> >>> combinations, but I'm not sure how far that will get us.
> >>
> >> I believe the correct way is to program the hardware to be as close to
> >> 400 kHz as possible. I may be wrong, but I guess that slower than 400
> >> kHz is also acceptable during the initial phase, but faster may mean
> >> out of spec. For optimal performance the code may need to be reworked
> >> to support both correct and slow 400 kHz as well as whatever high
> >> frequencies needed for fast transfers.
> >
> > Hm, I think I'm missing something -- you shouldn't need to optimize f_min
> > in the driver at all, because the core handles retrying at lower frequencies
> > in the init phase (before switching to the higher frequency that comes from
> > the CSD) and it always begins at 400KHz if that's above f_min.  In core.c:
> >
> > void mmc_rescan(struct work_struct *work)
> > {
> >        static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
> >        ...
> >        for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> >                if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> >                ...
> >
> > So, why would you want f_min to be near 400KHz?

That does seem to neutralise setting f_min close to 400KHz.

> First of all, sorry to cause confusion. It is most likely me missing
> something. =)
> 
> The reason we want f_min to be near 400 kHz is that this moves our
> window of allowed frequencies as high as possible. We could go lower
> than 400 kHz but due to hardware limitations that may also lower
> f_max.
> 
> So thanks to your pointer I can see that a range of frequencies are
> being tried in mmc_rescan(). I can see how f_init is being set in
> mmc_rescan_try_freq() and the value is being passed down in
> mmc_power_up() which in turn sets ios.clock to f_init and calls
> mmc_set_ios(). This regardless of f_min in this case - perhaps worth
> comparing to the WARN_ON() comparison with f_min in __mmc_set_clock().
> I know too little about the MMC subsystem to say anything about that.
> 
> Anyway, I was mainly wondering about the setup of the clocks in the
> driver for the MMC host hardware. I got the impression that f_min and
> f_max are supposed to be used to point out the minimum and maximum
> allowed frequencies that the hardware supports. On our SoCs this
> depends on the rate of the parent clock and the number of bits used
> for the MMC host hardware divider.
>
> So the parent clock rate may be rather high, and if it happens to be
> set too high on a board level then the MMC hardware block won't be
> able to program the frequencies as low as 400 kHz. So my point is only
> that we need to make sure that the parent clock rate is set in such a
> way that we can have some at least half-high clock frequency for
> decent general purpose throughput but if we try to increase the clock
> rate too much then we may force the lowest clock rate to go above 400
> kHz and then I suspect we may be out of spec. All this is based on
> that the parent clock is set statically to some frequency during boot
> by the SoC or board code.

The current code assumes that the maximum divisor is 512.

This may lead to f_min being greater than 400Hz if the parent
clock is greater than 200MHz. It seems to me that empirically this
has not being occurring else the WARN_ON() in __mmc_set_clock() would
be activated. Though perhaps no one notices it.

In any case, in the context of the current code it seems that changing
sh_mmcif_probe() to set f_min to host->clk / 512 will not alter the current
behaviour and will simplify the code.

> Some device drivers may set the parent clock rate in ->set_ios() but
> we do not at this point. We keep the parent rate fixed and just adjust
> the frequency in the range that the on-MMC-controller divider allows
> us to do without affecting the rest of the system. The code could be
> extended to also set the parent clock but due to clock dependencies
> related to the SoC clock topology we may not be able to change those
> frequencies during runtime.
> 
> Anyway, I hope this clarifies how I see things!
> 
> Thanks,
> 
> / magnus
> 

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Magnus Damm]

Hi Simon,

On Tue, Mar 27, 2012 at 3:01 PM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Mar 27, 2012 at 01:02:38PM +0900, Magnus Damm wrote:
>> Hi Chris,
>>
>> On Tue, Mar 27, 2012 at 12:20 PM, Chris Ball <cjb@laptop.org> wrote:
>> > Hi,
>> >
>> > On Mon, Mar 26 2012, Magnus Damm wrote:
>> >>> Do you have a feeling of if it it worth trying to start with a value close
>> >>> to 400kHz or if it would be better to simplify the code? I can try and
>> >>> measure the difference in start up time for particular hardware
>> >>> combinations, but I'm not sure how far that will get us.
>> >>
>> >> I believe the correct way is to program the hardware to be as close to
>> >> 400 kHz as possible. I may be wrong, but I guess that slower than 400
>> >> kHz is also acceptable during the initial phase, but faster may mean
>> >> out of spec. For optimal performance the code may need to be reworked
>> >> to support both correct and slow 400 kHz as well as whatever high
>> >> frequencies needed for fast transfers.
>> >
>> > Hm, I think I'm missing something -- you shouldn't need to optimize f_min
>> > in the driver at all, because the core handles retrying at lower frequencies
>> > in the init phase (before switching to the higher frequency that comes from
>> > the CSD) and it always begins at 400KHz if that's above f_min.  In core.c:
>> >
>> > void mmc_rescan(struct work_struct *work)
>> > {
>> >        static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>> >        ...
>> >        for (i = 0; i < ARRAY_SIZE(freqs); i++) {
>> >                if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
>> >                ...
>> >
>> > So, why would you want f_min to be near 400KHz?
>
> That does seem to neutralise setting f_min close to 400KHz.
>
>> First of all, sorry to cause confusion. It is most likely me missing
>> something. =)
>>
>> The reason we want f_min to be near 400 kHz is that this moves our
>> window of allowed frequencies as high as possible. We could go lower
>> than 400 kHz but due to hardware limitations that may also lower
>> f_max.
>>
>> So thanks to your pointer I can see that a range of frequencies are
>> being tried in mmc_rescan(). I can see how f_init is being set in
>> mmc_rescan_try_freq() and the value is being passed down in
>> mmc_power_up() which in turn sets ios.clock to f_init and calls
>> mmc_set_ios(). This regardless of f_min in this case - perhaps worth
>> comparing to the WARN_ON() comparison with f_min in __mmc_set_clock().
>> I know too little about the MMC subsystem to say anything about that.
>>
>> Anyway, I was mainly wondering about the setup of the clocks in the
>> driver for the MMC host hardware. I got the impression that f_min and
>> f_max are supposed to be used to point out the minimum and maximum
>> allowed frequencies that the hardware supports. On our SoCs this
>> depends on the rate of the parent clock and the number of bits used
>> for the MMC host hardware divider.
>>
>> So the parent clock rate may be rather high, and if it happens to be
>> set too high on a board level then the MMC hardware block won't be
>> able to program the frequencies as low as 400 kHz. So my point is only
>> that we need to make sure that the parent clock rate is set in such a
>> way that we can have some at least half-high clock frequency for
>> decent general purpose throughput but if we try to increase the clock
>> rate too much then we may force the lowest clock rate to go above 400
>> kHz and then I suspect we may be out of spec. All this is based on
>> that the parent clock is set statically to some frequency during boot
>> by the SoC or board code.
>
> The current code assumes that the maximum divisor is 512.

That's good!

> This may lead to f_min being greater than 400Hz if the parent
> clock is greater than 200MHz. It seems to me that empirically this
> has not being occurring else the WARN_ON() in __mmc_set_clock() would
> be activated. Though perhaps no one notices it.

Sure, 400 kHz * 512 = 204.8 MHz.

The reason why we don't hit WARN_ON() may be that mmc_power_up()
blindly sets host->ios.clock to 400 kHz, 200 kHz and so on - this
without checking against f_min.

> In any case, in the context of the current code it seems that changing
> sh_mmcif_probe() to set f_min to host->clk / 512 will not alter the current
> behaviour and will simplify the code.

Yep. It seems to me (only checked sh7372 data sheet) that f_min and
f_max should be set like this:

f_min = parent_clk / 512
f_max = parent_clk / 2

The real question is in my opinion is how to select a good parent
clock rate, but that's sort of out of scope here.

Thanks,

/ magnus

Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Simon Horman]

Hi Magnus,

On Tue, Mar 27, 2012 at 03:37:42PM +0900, Magnus Damm wrote:
> Hi Simon,
> 
> On Tue, Mar 27, 2012 at 3:01 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Mar 27, 2012 at 01:02:38PM +0900, Magnus Damm wrote:

[snip]

> >> Anyway, I was mainly wondering about the setup of the clocks in the
> >> driver for the MMC host hardware. I got the impression that f_min and
> >> f_max are supposed to be used to point out the minimum and maximum
> >> allowed frequencies that the hardware supports. On our SoCs this
> >> depends on the rate of the parent clock and the number of bits used
> >> for the MMC host hardware divider.
> >>
> >> So the parent clock rate may be rather high, and if it happens to be
> >> set too high on a board level then the MMC hardware block won't be
> >> able to program the frequencies as low as 400 kHz. So my point is only
> >> that we need to make sure that the parent clock rate is set in such a
> >> way that we can have some at least half-high clock frequency for
> >> decent general purpose throughput but if we try to increase the clock
> >> rate too much then we may force the lowest clock rate to go above 400
> >> kHz and then I suspect we may be out of spec. All this is based on
> >> that the parent clock is set statically to some frequency during boot
> >> by the SoC or board code.
> >
> > The current code assumes that the maximum divisor is 512.
> 
> That's good!
> 
> > This may lead to f_min being greater than 400Hz if the parent
> > clock is greater than 200MHz. It seems to me that empirically this
> > has not being occurring else the WARN_ON() in __mmc_set_clock() would
> > be activated. Though perhaps no one notices it.
> 
> Sure, 400 kHz * 512 = 204.8 MHz.
> 
> The reason why we don't hit WARN_ON() may be that mmc_power_up()
> blindly sets host->ios.clock to 400 kHz, 200 kHz and so on - this
> without checking against f_min.
> 
> > In any case, in the context of the current code it seems that changing
> > sh_mmcif_probe() to set f_min to host->clk / 512 will not alter the current
> > behaviour and will simplify the code.
> 
> Yep. It seems to me (only checked sh7372 data sheet) that f_min and
> f_max should be set like this:
> 
> f_min = parent_clk / 512
> f_max = parent_clk / 2
> 
> The real question is in my opinion is how to select a good parent
> clock rate, but that's sort of out of scope here.

Agreed on all counts. I will refresh my outstanding patch series
to set f_min accordingly, the series already sets f_max as you describe.