[PATCH v2] mxs-mmc: fix clock rate setting

[PATCH v2] mxs-mmc: fix clock rate setting

[Koen Beel]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Hi,

(Sorry, patch in attachment as I still don't have a decent mail setup
at my company)

Fix clock rate setting on mxs-mmc driver.
Previously, if div2 was zero the value for TIMING_CLOCK_RATE would
have been 255 instead of 0.
Also the limits for div1 (TIMING_CLOCK_DIVIDE) and div2
(TIMING_CLOCK_RATE + 1) where not correctly defined.

Can easily be reproduced on mx23evk: default clock for high speed sdio
cards is 50 MHz. With a SSP_CLK of 28.8 MHz default), this resulted in
an actual clock rate of about 56 kHz.
Tested on mx23evk.

Changes in V2 patch:
- use DIV_ROUND_UP to make sure the actual clock rate is not higher
then the requested clock rate.
- rename variables to reflect naming in datasheet and to make things
more clear

Regards,
Koen

[-- Attachment #2: 0001-mxs-mmc-fix-clock-rate-setting.patch --]
[-- Type: application/octet-stream, Size: 2778 bytes --]

Fix clock rate setting on mxs-mmc driver.
Previously, if div2 was zero the value for TIMING_CLOCK_RATE would
have been 255 instead of 0.
Also the limits for div1 (TIMING_CLOCK_DIVIDE) and div2
(TIMING_CLOCK_RATE + 1) where not correctly defined.

Can easily be reproduced on mx23evk: default clock for high speed sdio
cards is 50 MHz. With a SSP_CLK of 28.8 MHz default), this resulted in
an actual clock rate of about 56 kHz.
Tested on mx23evk.

Changes in V2 patch:
- use DIV_ROUND_UP to make sure the actual clock rate is not higher
then the requested clock rate.
- rename variables to reflect naming in datasheet and to make things
more clear

Signed-off-by: Koen Beel <koen.beel@barco.com>
---
 drivers/mmc/host/mxs-mmc.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index 99d39a6..d513d47 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -564,40 +564,38 @@ static void mxs_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 static void mxs_mmc_set_clk_rate(struct mxs_mmc_host *host, unsigned int rate)
 {
-	unsigned int ssp_rate, bit_rate;
-	u32 div1, div2;
+	unsigned int ssp_clk, ssp_sck;
+	u32 clock_divide, clock_rate;
 	u32 val;
 
-	ssp_rate = clk_get_rate(host->clk);
+	ssp_clk = clk_get_rate(host->clk);
 
-	for (div1 = 2; div1 < 254; div1 += 2) {
-		div2 = ssp_rate / rate / div1;
-		if (div2 < 0x100)
+	for (clock_divide = 2; clock_divide <= 254; clock_divide += 2) {
+		clock_rate = DIV_ROUND_UP(ssp_clk, rate * clock_divide);
+		clock_rate = (clock_rate > 0) ? clock_rate - 1 : 0;
+		if (clock_rate <= 255)
 			break;
 	}
 
-	if (div1 >= 254) {
+	if (clock_divide > 254) {
 		dev_err(mmc_dev(host->mmc),
 			"%s: cannot set clock to %d\n", __func__, rate);
 		return;
 	}
 
-	if (div2 == 0)
-		bit_rate = ssp_rate / div1;
-	else
-		bit_rate = ssp_rate / div1 / div2;
+	ssp_sck = ssp_clk / clock_divide / (1 + clock_rate);
 
 	val = readl(host->base + HW_SSP_TIMING);
 	val &= ~(BM_SSP_TIMING_CLOCK_DIVIDE | BM_SSP_TIMING_CLOCK_RATE);
-	val |= BF_SSP(div1, TIMING_CLOCK_DIVIDE);
-	val |= BF_SSP(div2 - 1, TIMING_CLOCK_RATE);
+	val |= BF_SSP(clock_divide, TIMING_CLOCK_DIVIDE);
+	val |= BF_SSP(clock_rate, TIMING_CLOCK_RATE);
 	writel(val, host->base + HW_SSP_TIMING);
 
-	host->clk_rate = bit_rate;
+	host->clk_rate = ssp_sck;
 
 	dev_dbg(mmc_dev(host->mmc),
-		"%s: div1 %d, div2 %d, ssp %d, bit %d, rate %d\n",
-		__func__, div1, div2, ssp_rate, bit_rate, rate);
+		"%s: clock_divide %d, clock_rate %d, ssp_clk %d, rate_actual %d, rate_requested %d\n",
+		__func__, clock_divide, clock_rate, ssp_clk, ssp_sck, rate);
 }
 
 static void mxs_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
-- 
1.7.4.1

Re: [PATCH v2] mxs-mmc: fix clock rate setting

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3766 bytes --]

Hi Koen,

thanks for v2.

On Mon, Jul 11, 2011 at 09:52:01PM +0200, Koen Beel wrote:

> (Sorry, patch in attachment as I still don't have a decent mail setup
> at my company)

That's bad :(

====

> Fix clock rate setting on mxs-mmc driver.
> Previously, if div2 was zero the value for TIMING_CLOCK_RATE would
> have been 255 instead of 0.
> Also the limits for div1 (TIMING_CLOCK_DIVIDE) and div2
> (TIMING_CLOCK_RATE + 1) where not correctly defined.

Very minor nit: No paragraphs I'd say.

> Can easily be reproduced on mx23evk: default clock for high speed sdio
> cards is 50 MHz. With a SSP_CLK of 28.8 MHz default), this resulted in
> an actual clock rate of about 56 kHz.
> Tested on mx23evk.
> 
> Changes in V2 patch:
> - use DIV_ROUND_UP to make sure the actual clock rate is not higher
> then the requested clock rate.
> - rename variables to reflect naming in datasheet and to make things
> more clear

Changelogs are good, but should go below "---"

> 
> Signed-off-by: Koen Beel <koen.beel@barco.com>
> ---
>  drivers/mmc/host/mxs-mmc.c |   30 ++++++++++++++----------------
>  1 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index 99d39a6..d513d47 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -564,40 +564,38 @@ static void mxs_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  
>  static void mxs_mmc_set_clk_rate(struct mxs_mmc_host *host, unsigned int rate)
>  {
> -	unsigned int ssp_rate, bit_rate;
> -	u32 div1, div2;
> +	unsigned int ssp_clk, ssp_sck;

ssp_sck is not really needed? Value could directly go to host->clk_rate.

> +	u32 clock_divide, clock_rate;
>  	u32 val;
>  
> -	ssp_rate = clk_get_rate(host->clk);
> +	ssp_clk = clk_get_rate(host->clk);
>  
> -	for (div1 = 2; div1 < 254; div1 += 2) {
> -		div2 = ssp_rate / rate / div1;
> -		if (div2 < 0x100)
> +	for (clock_divide = 2; clock_divide <= 254; clock_divide += 2) {
> +		clock_rate = DIV_ROUND_UP(ssp_clk, rate * clock_divide);
> +		clock_rate = (clock_rate > 0) ? clock_rate - 1 : 0;
> +		if (clock_rate <= 255)
>  			break;
>  	}
>  
> -	if (div1 >= 254) {
> +	if (clock_divide > 254) {
>  		dev_err(mmc_dev(host->mmc),
>  			"%s: cannot set clock to %d\n", __func__, rate);

Didn't you want to set some minimal values instead of just returning?
Just wondering, could be a seperate patch in my book...

>  		return;
>  	}
>  
> -	if (div2 == 0)
> -		bit_rate = ssp_rate / div1;
> -	else
> -		bit_rate = ssp_rate / div1 / div2;
> +	ssp_sck = ssp_clk / clock_divide / (1 + clock_rate);
>  
>  	val = readl(host->base + HW_SSP_TIMING);
>  	val &= ~(BM_SSP_TIMING_CLOCK_DIVIDE | BM_SSP_TIMING_CLOCK_RATE);
> -	val |= BF_SSP(div1, TIMING_CLOCK_DIVIDE);
> -	val |= BF_SSP(div2 - 1, TIMING_CLOCK_RATE);
> +	val |= BF_SSP(clock_divide, TIMING_CLOCK_DIVIDE);
> +	val |= BF_SSP(clock_rate, TIMING_CLOCK_RATE);
>  	writel(val, host->base + HW_SSP_TIMING);
>  
> -	host->clk_rate = bit_rate;
> +	host->clk_rate = ssp_sck;
>  
>  	dev_dbg(mmc_dev(host->mmc),
> -		"%s: div1 %d, div2 %d, ssp %d, bit %d, rate %d\n",
> -		__func__, div1, div2, ssp_rate, bit_rate, rate);
> +		"%s: clock_divide %d, clock_rate %d, ssp_clk %d, rate_actual %d, rate_requested %d\n",
> +		__func__, clock_divide, clock_rate, ssp_clk, ssp_sck, rate);
>  }
>  

Rest looks good to me, and test was also successful.

Chris, if you want to take this version already:

Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] mxs-mmc: fix clock rate setting

[Koen Beel]

Hi,

On Mon, Jul 11, 2011 at 10:26 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi Koen,
>
> thanks for v2.
>
> On Mon, Jul 11, 2011 at 09:52:01PM +0200, Koen Beel wrote:
>
>> (Sorry, patch in attachment as I still don't have a decent mail setup
>> at my company)
>
> That's bad :(
>
> ====
>
>> Fix clock rate setting on mxs-mmc driver.
>> Previously, if div2 was zero the value for TIMING_CLOCK_RATE would
>> have been 255 instead of 0.
>> Also the limits for div1 (TIMING_CLOCK_DIVIDE) and div2
>> (TIMING_CLOCK_RATE + 1) where not correctly defined.
>
> Very minor nit: No paragraphs I'd say.
>
>> Can easily be reproduced on mx23evk: default clock for high speed sdio
>> cards is 50 MHz. With a SSP_CLK of 28.8 MHz default), this resulted in
>> an actual clock rate of about 56 kHz.
>> Tested on mx23evk.
>>
>> Changes in V2 patch:
>> - use DIV_ROUND_UP to make sure the actual clock rate is not higher
>> then the requested clock rate.
>> - rename variables to reflect naming in datasheet and to make things
>> more clear
>
> Changelogs are good, but should go below "---"
>
>>
>> Signed-off-by: Koen Beel <koen.beel@barco.com>
>> ---
>>  drivers/mmc/host/mxs-mmc.c |   30 ++++++++++++++----------------
>>  1 files changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
>> index 99d39a6..d513d47 100644
>> --- a/drivers/mmc/host/mxs-mmc.c
>> +++ b/drivers/mmc/host/mxs-mmc.c
>> @@ -564,40 +564,38 @@ static void mxs_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>
>>  static void mxs_mmc_set_clk_rate(struct mxs_mmc_host *host, unsigned int rate)
>>  {
>> -     unsigned int ssp_rate, bit_rate;
>> -     u32 div1, div2;
>> +     unsigned int ssp_clk, ssp_sck;
>
> ssp_sck is not really needed? Value could directly go to host->clk_rate.
>
>> +     u32 clock_divide, clock_rate;
>>       u32 val;
>>
>> -     ssp_rate = clk_get_rate(host->clk);
>> +     ssp_clk = clk_get_rate(host->clk);
>>
>> -     for (div1 = 2; div1 < 254; div1 += 2) {
>> -             div2 = ssp_rate / rate / div1;
>> -             if (div2 < 0x100)
>> +     for (clock_divide = 2; clock_divide <= 254; clock_divide += 2) {
>> +             clock_rate = DIV_ROUND_UP(ssp_clk, rate * clock_divide);
>> +             clock_rate = (clock_rate > 0) ? clock_rate - 1 : 0;
>> +             if (clock_rate <= 255)
>>                       break;
>>       }
>>
>> -     if (div1 >= 254) {
>> +     if (clock_divide > 254) {
>>               dev_err(mmc_dev(host->mmc),
>>                       "%s: cannot set clock to %d\n", __func__, rate);
>
> Didn't you want to set some minimal values instead of just returning?
> Just wondering, could be a seperate patch in my book...
>
>>               return;
>>       }
>>
>> -     if (div2 == 0)
>> -             bit_rate = ssp_rate / div1;
>> -     else
>> -             bit_rate = ssp_rate / div1 / div2;
>> +     ssp_sck = ssp_clk / clock_divide / (1 + clock_rate);
>>
>>       val = readl(host->base + HW_SSP_TIMING);
>>       val &= ~(BM_SSP_TIMING_CLOCK_DIVIDE | BM_SSP_TIMING_CLOCK_RATE);
>> -     val |= BF_SSP(div1, TIMING_CLOCK_DIVIDE);
>> -     val |= BF_SSP(div2 - 1, TIMING_CLOCK_RATE);
>> +     val |= BF_SSP(clock_divide, TIMING_CLOCK_DIVIDE);
>> +     val |= BF_SSP(clock_rate, TIMING_CLOCK_RATE);
>>       writel(val, host->base + HW_SSP_TIMING);
>>
>> -     host->clk_rate = bit_rate;
>> +     host->clk_rate = ssp_sck;
>>
>>       dev_dbg(mmc_dev(host->mmc),
>> -             "%s: div1 %d, div2 %d, ssp %d, bit %d, rate %d\n",
>> -             __func__, div1, div2, ssp_rate, bit_rate, rate);
>> +             "%s: clock_divide %d, clock_rate %d, ssp_clk %d, rate_actual %d, rate_requested %d\n",
>> +             __func__, clock_divide, clock_rate, ssp_clk, ssp_sck, rate);
>>  }
>>
>
> Rest looks good to me, and test was also successful.
>
> Chris, if you want to take this version already:

Still need me to do something on this or is it ok?

Koen

>
> Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>

Re: [PATCH v2] mxs-mmc: fix clock rate setting

[Chris Ball]

Hi Koen,

On Fri, Jul 15 2011, Koen Beel wrote:
> Still need me to do something on this or is it ok?

Pushed to mmc-next for 3.1, thanks.  Please consider Wolfram's last
round of comments and send a follow-up patch if you agree with them.

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