* [PATCH v2] mxs-mmc: fix clock rate setting
@ 2011-07-11 19:52 Koen Beel
2011-07-11 20:26 ` Wolfram Sang
0 siblings, 1 reply; 4+ messages in thread
From: Koen Beel @ 2011-07-11 19:52 UTC (permalink / raw)
To: linux-mmc; +Cc: Wolfram Sang, Shawn Guo, Chris Ball
[-- 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] mxs-mmc: fix clock rate setting
2011-07-11 19:52 [PATCH v2] mxs-mmc: fix clock rate setting Koen Beel
@ 2011-07-11 20:26 ` Wolfram Sang
2011-07-15 9:47 ` Koen Beel
0 siblings, 1 reply; 4+ messages in thread
From: Wolfram Sang @ 2011-07-11 20:26 UTC (permalink / raw)
To: Koen Beel; +Cc: linux-mmc, Shawn Guo, Chris Ball
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] mxs-mmc: fix clock rate setting
2011-07-11 20:26 ` Wolfram Sang
@ 2011-07-15 9:47 ` Koen Beel
2011-07-15 21:40 ` Chris Ball
0 siblings, 1 reply; 4+ messages in thread
From: Koen Beel @ 2011-07-15 9:47 UTC (permalink / raw)
To: Wolfram Sang, Chris Ball; +Cc: linux-mmc, Shawn Guo
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/ |
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-07-15 21:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-11 19:52 [PATCH v2] mxs-mmc: fix clock rate setting Koen Beel
2011-07-11 20:26 ` Wolfram Sang
2011-07-15 9:47 ` Koen Beel
2011-07-15 21:40 ` Chris Ball
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox