[PATCH] mxs-mmc: fix clock rate setting

[PATCH] mxs-mmc: fix clock rate setting

[Koen Beel]

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.

Signed-off-by: Koen Beel <koen.beel.barco <at> gmail.com>
---
 drivers/mmc/host/mxs-mmc.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index 99d39a6..3575330 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -570,22 +570,22 @@ static void mxs_mmc_set_clk_rate(struct
mxs_mmc_host *host, unsigned int rate)
    ssp_rate = clk_get_rate(host->clk);
-   for (div1 = 2; div1 < 254; div1 += 2) {
+   for (div1 = 2; div1 <= 254; div1 += 2) {
        div2 = ssp_rate / rate / div1;
-       if (div2 < 0x100)
+       if (div2 <= 256)
            break;
    }
-   if (div1 >= 254) {
+   if (div1 > 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;
+       div2 = 1;
+
+   bit_rate = ssp_rate / div1 / div2;
    val = readl(host->base + HW_SSP_TIMING);
    val &= ~(BM_SSP_TIMING_CLOCK_DIVIDE | BM_SSP_TIMING_CLOCK_RATE);
--
1.7.4.1

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

[Wolfram Sang]

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

On Thu, Jun 30, 2011 at 12:13:34PM +0200, Koen Beel wrote:
> 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.
> 
> Signed-off-by: Koen Beel <koen.beel.barco <at> gmail.com>

Looks promising, but your tabs are garbled (0xa0 0x20 here?)

> ---
>  drivers/mmc/host/mxs-mmc.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index 99d39a6..3575330 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -570,22 +570,22 @@ static void mxs_mmc_set_clk_rate(struct
> mxs_mmc_host *host, unsigned int rate)
>     ssp_rate = clk_get_rate(host->clk);
> -   for (div1 = 2; div1 < 254; div1 += 2) {
> +   for (div1 = 2; div1 <= 254; div1 += 2) {
>         div2 = ssp_rate / rate / div1;
> -       if (div2 < 0x100)
> +       if (div2 <= 256)
>             break;
>     }
> -   if (div1 >= 254) {
> +   if (div1 > 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;
> +       div2 = 1;
> +
> +   bit_rate = ssp_rate / div1 / div2;
>     val = readl(host->base + HW_SSP_TIMING);
>     val &= ~(BM_SSP_TIMING_CLOCK_DIVIDE | BM_SSP_TIMING_CLOCK_RATE);
> --
> 1.7.4.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
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] mxs-mmc: fix clock rate setting

[Wolfram Sang]

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

On Thu, Jun 30, 2011 at 04:55:07PM +0200, Wolfram Sang wrote:
> On Thu, Jun 30, 2011 at 12:13:34PM +0200, Koen Beel wrote:
> > 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.
> > 
> > Signed-off-by: Koen Beel <koen.beel.barco <at> gmail.com>
> 
> Looks promising, but your tabs are garbled (0xa0 0x20 here?)

Can you repost a patch which applies? I'd like to test it.

-- 
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] mxs-mmc: fix clock rate setting

[Sachin Nikam]

How to increase the number blocks for SD Read operation? Any sysfs entry?

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Wolfram Sang
Sent: Friday, July 01, 2011 2:48 PM
To: Koen Beel
Cc: linux-mmc@vger.kernel.org; shawn.guo@freescale.com
Subject: Re: [PATCH] mxs-mmc: fix clock rate setting

* PGP Signed by an unknown key

On Thu, Jun 30, 2011 at 04:55:07PM +0200, Wolfram Sang wrote:
> On Thu, Jun 30, 2011 at 12:13:34PM +0200, Koen Beel wrote:
> > 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.
> > 
> > Signed-off-by: Koen Beel <koen.beel.barco <at> gmail.com>
> 
> Looks promising, but your tabs are garbled (0xa0 0x20 here?)

Can you repost a patch which applies? I'd like to test it.

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

* Unknown Key
* 0x7D7F551B
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

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

[Koen Beel]

Hi,

Think the tabs problem was due to the gmail web ui.
Hope it's better now.


Signed-off-by: Koen Beel <koen.beel <at> barco.com>
---
 drivers/mmc/host/mxs-mmc.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index 99d39a6..3575330 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -570,22 +570,22 @@ static void mxs_mmc_set_clk_rate(struct
mxs_mmc_host *host, unsigned int rate)
    ssp_rate = clk_get_rate(host->clk);
-   for (div1 = 2; div1 < 254; div1 += 2) {
+   for (div1 = 2; div1 <= 254; div1 += 2) {
        div2 = ssp_rate / rate / div1;
-       if (div2 < 0x100)
+       if (div2 <= 256)
            break;
    }
-   if (div1 >= 254) {
+   if (div1 > 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;
+       div2 = 1;
+
+   bit_rate = ssp_rate / div1 / div2;
    val = readl(host->base + HW_SSP_TIMING);
    val &= ~(BM_SSP_TIMING_CLOCK_DIVIDE | BM_SSP_TIMING_CLOCK_RATE);
--
1.7.4.1

On Fri, Jul 1, 2011 at 11:17 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Thu, Jun 30, 2011 at 04:55:07PM +0200, Wolfram Sang wrote:
>> On Thu, Jun 30, 2011 at 12:13:34PM +0200, Koen Beel wrote:
>> > 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.
>> >
>> > Signed-off-by: Koen Beel <koen.beel.barco <at> gmail.com>
>>
>> Looks promising, but your tabs are garbled (0xa0 0x20 here?)
>
> Can you repost a patch which applies? I'd like to test it.
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEUEARECAAYFAk4NkMEACgkQD27XaX1/VRvw0QCeP4F3oeKe1Ge3SLohJICLxZre
> LKYAmJ2sEztaKIVw4NsZMYNqCUbbwFQ=
> =C7kg
> -----END PGP SIGNATURE-----
>
>

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

[Wolfram Sang]

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


> Think the tabs problem was due to the gmail web ui.
> Hope it's better now.

No 0xa0 anymore, but still no tabs. Oh, and lines are wrapped.

Documentation/email-clients.txt says

===

Gmail (Web GUI)

Does not work for sending patches.

Gmail web client converts tabs to spaces automatically.

At the same time it wraps lines every 78 chars with CRLF style line
breaks although tab2space problem can be solved with external editor.

Another problem is that Gmail will base64-encode any message that has a
non-ASCII character. That includes things like European names.

===

I don't know if git-send-email can work directly with gmail, but it
seems you need some kind of alternative approach.

Regards,

   Wolfram

-- 
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] mxs-mmc: fix clock rate setting

[Koen Beel]

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

I send the patch as attachment for now.

But I'll have to look into another way of doing this. Corporate mail
system is adding stupid disclaimers, gmail web ui is not working ok
and corporate firewalls avoid using a different smtp server...



On Fri, Jul 1, 2011 at 2:27 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
>> Think the tabs problem was due to the gmail web ui.
>> Hope it's better now.
>
> No 0xa0 anymore, but still no tabs. Oh, and lines are wrapped.
>
> Documentation/email-clients.txt says
>
> ===
>
> Gmail (Web GUI)
>
> Does not work for sending patches.
>
> Gmail web client converts tabs to spaces automatically.
>
> At the same time it wraps lines every 78 chars with CRLF style line
> breaks although tab2space problem can be solved with external editor.
>
> Another problem is that Gmail will base64-encode any message that has a
> non-ASCII character. That includes things like European names.
>
> ===
>
> I don't know if git-send-email can work directly with gmail, but it
> seems you need some kind of alternative approach.
>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk4NvR0ACgkQD27XaX1/VRtjmACfQzU2j5+awQ78Ey7Ir+39uhxn
> j70AoJQG0D6tK3rYh4YeYsiyFAsLieC5
> =DqDV
> -----END PGP SIGNATURE-----
>
>

[-- Attachment #2: mxs-mmc-fix-clock-rate-setting.patch --]
[-- Type: text/x-patch, Size: 1673 bytes --]

From ab610437fade5b30fdeacd9e959ddf95bc42587a Mon Sep 17 00:00:00 2001
From: Koen Beel <koen.beel@barco.com>
Date: Thu, 30 Jun 2011 12:00:19 +0200
Subject: [PATCH] mxs-mmc: fix clock rate setting

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.

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

diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index 99d39a6..3575330 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -570,22 +570,22 @@ static void mxs_mmc_set_clk_rate(struct mxs_mmc_host *host, unsigned int rate)
 
 	ssp_rate = clk_get_rate(host->clk);
 
-	for (div1 = 2; div1 < 254; div1 += 2) {
+	for (div1 = 2; div1 <= 254; div1 += 2) {
 		div2 = ssp_rate / rate / div1;
-		if (div2 < 0x100)
+		if (div2 <= 256)
 			break;
 	}
 
-	if (div1 >= 254) {
+	if (div1 > 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;
+		div2 = 1;
+
+	bit_rate = ssp_rate / div1 / div2;
 
 	val = readl(host->base + HW_SSP_TIMING);
 	val &= ~(BM_SSP_TIMING_CLOCK_DIVIDE | BM_SSP_TIMING_CLOCK_RATE);
-- 
1.7.4.1

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

[Sachin Nikam]

Can  read_ahead_kb can be used to increase the number of read blocks?

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Sachin Nikam
Sent: Friday, July 01, 2011 5:12 PM
To: 'Wolfram Sang'; Koen Beel
Cc: linux-mmc@vger.kernel.org; shawn.guo@freescale.com
Subject: RE: [PATCH] mxs-mmc: fix clock rate setting

How to increase the number blocks for SD Read operation? Any sysfs entry?

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Wolfram Sang
Sent: Friday, July 01, 2011 2:48 PM
To: Koen Beel
Cc: linux-mmc@vger.kernel.org; shawn.guo@freescale.com
Subject: Re: [PATCH] mxs-mmc: fix clock rate setting

* PGP Signed by an unknown key

On Thu, Jun 30, 2011 at 04:55:07PM +0200, Wolfram Sang wrote:
> On Thu, Jun 30, 2011 at 12:13:34PM +0200, Koen Beel wrote:
> > 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.
> > 
> > Signed-off-by: Koen Beel <koen.beel.barco <at> gmail.com>
> 
> Looks promising, but your tabs are garbled (0xa0 0x20 here?)

Can you repost a patch which applies? I'd like to test it.

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

* Unknown Key
* 0x7D7F551B
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Wolfram Sang]

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

Hi,

On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
> I send the patch as attachment for now.

Fine with me in this case...

> But I'll have to look into another way of doing this. Corporate mail
> system is adding stupid disclaimers, gmail web ui is not working ok
> and corporate firewalls avoid using a different smtp server...

Good luck with that!

About the patch itself: I didn't verify the formulas, but it does solve one
special problem here. Thanks a lot! So:

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

@chris: If Shawn also likes the patch, I think this is a stable candidate.

Very minor nits:

> From ab610437fade5b30fdeacd9e959ddf95bc42587a Mon Sep 17 00:00:00 2001
> From: Koen Beel <koen.beel@barco.com>
> Date: Thu, 30 Jun 2011 12:00:19 +0200
> Subject: [PATCH] mxs-mmc: fix clock rate setting
> 
> 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.

Commit msg should linebreak at 72.


> Signed-off-by: Koen beel <koen.beel@barco.com>

Capital 'B'?

Regards,

   Wolfram

-- 
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] mxs-mmc: fix clock rate setting

[Wolfram Sang]

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

On Fri, Jul 01, 2011 at 08:08:48PM +0530, Sachin Nikam wrote:

> Can  read_ahead_kb can be used to increase the number of read blocks?

Unless you start a _new thread_ for your question and describe your problem in
a bit more detail, you probably won't get an answer.

-- 
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] mxs-mmc: fix clock rate setting

[Shawn Guo]

On Fri, Jul 01, 2011 at 04:44:09PM +0200, Wolfram Sang wrote:
> Hi,
> 
> On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
> > I send the patch as attachment for now.
> 
> Fine with me in this case...
> 
> > But I'll have to look into another way of doing this. Corporate mail
> > system is adding stupid disclaimers, gmail web ui is not working ok
> > and corporate firewalls avoid using a different smtp server...
> 
> Good luck with that!
> 
> About the patch itself: I didn't verify the formulas, but it does solve one
> special problem here. Thanks a lot! So:
> 
> Tested-by: Wolfram Sang <w.sang@pengutronix.de>
> 
> @chris: If Shawn also likes the patch, I think this is a stable candidate.
> 
Thanks for the fixing, Koen.

Acked-by: Shawn Guo <shawn.guo@freescale.com>

-- 
Regards,
Shawn

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

[Wolfram Sang]

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

On Fri, Jul 01, 2011 at 11:46:29PM +0800, Shawn Guo wrote:
> On Fri, Jul 01, 2011 at 04:44:09PM +0200, Wolfram Sang wrote:
> > Hi,
> > 
> > On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
> > > I send the patch as attachment for now.
> > 
> > Fine with me in this case...
> > 
> > > But I'll have to look into another way of doing this. Corporate mail
> > > system is adding stupid disclaimers, gmail web ui is not working ok
> > > and corporate firewalls avoid using a different smtp server...
> > 
> > Good luck with that!
> > 
> > About the patch itself: I didn't verify the formulas, but it does solve one
> > special problem here. Thanks a lot! So:
> > 
> > Tested-by: Wolfram Sang <w.sang@pengutronix.de>
> > 
> > @chris: If Shawn also likes the patch, I think this is a stable candidate.
> > 
> Thanks for the fixing, Koen.
> 
> Acked-by: Shawn Guo <shawn.guo@freescale.com>

Well, maybe not. My colleague complained and I think he is right that we are
mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
be wrong for one value per se.

-- 
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] mxs-mmc: fix clock rate setting

[Shawn Guo]

On Sat, Jul 02, 2011 at 03:12:25PM +0200, Wolfram Sang wrote:
> On Fri, Jul 01, 2011 at 11:46:29PM +0800, Shawn Guo wrote:
> > On Fri, Jul 01, 2011 at 04:44:09PM +0200, Wolfram Sang wrote:
> > > Hi,
> > > 
> > > On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
> > > > I send the patch as attachment for now.
> > > 
> > > Fine with me in this case...
> > > 
> > > > But I'll have to look into another way of doing this. Corporate mail
> > > > system is adding stupid disclaimers, gmail web ui is not working ok
> > > > and corporate firewalls avoid using a different smtp server...
> > > 
> > > Good luck with that!
> > > 
> > > About the patch itself: I didn't verify the formulas, but it does solve one
> > > special problem here. Thanks a lot! So:
> > > 
> > > Tested-by: Wolfram Sang <w.sang@pengutronix.de>
> > > 
> > > @chris: If Shawn also likes the patch, I think this is a stable candidate.
> > > 
> > Thanks for the fixing, Koen.
> > 
> > Acked-by: Shawn Guo <shawn.guo@freescale.com>
> 
> Well, maybe not. My colleague complained and I think he is right that we are
> mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
> be wrong for one value per se.
> 
If you look at the context of the patch, you will find it's 'div2 - 1'
than 'div2' gets written into register.

-- 
Regards,
Shawn

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

[Wolfram Sang]

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

On Sun, Jul 03, 2011 at 05:28:52PM +0800, Shawn Guo wrote:
> On Sat, Jul 02, 2011 at 03:12:25PM +0200, Wolfram Sang wrote:
> > On Fri, Jul 01, 2011 at 11:46:29PM +0800, Shawn Guo wrote:
> > > On Fri, Jul 01, 2011 at 04:44:09PM +0200, Wolfram Sang wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
> > > > > I send the patch as attachment for now.
> > > > 
> > > > Fine with me in this case...
> > > > 
> > > > > But I'll have to look into another way of doing this. Corporate mail
> > > > > system is adding stupid disclaimers, gmail web ui is not working ok
> > > > > and corporate firewalls avoid using a different smtp server...
> > > > 
> > > > Good luck with that!
> > > > 
> > > > About the patch itself: I didn't verify the formulas, but it does solve one
> > > > special problem here. Thanks a lot! So:
> > > > 
> > > > Tested-by: Wolfram Sang <w.sang@pengutronix.de>
> > > > 
> > > > @chris: If Shawn also likes the patch, I think this is a stable candidate.
> > > > 
> > > Thanks for the fixing, Koen.
> > > 
> > > Acked-by: Shawn Guo <shawn.guo@freescale.com>
> > 
> > Well, maybe not. My colleague complained and I think he is right that we are
> > mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
> > be wrong for one value per se.
> > 
> If you look at the context of the patch, you will find it's 'div2 - 1'
> than 'div2' gets written into register.

Exactly. The '- 1' is why Koen changed the upper limit from < 256 to <= 256.
The lower limit fix is currently 'if (div2 == 0) div2 == 1', which is a 2:1
mapping. Not good, or?

-- 
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] mxs-mmc: fix clock rate setting

[Shawn Guo]

On Sun, Jul 03, 2011 at 12:31:24PM +0200, Wolfram Sang wrote:
> On Sun, Jul 03, 2011 at 05:28:52PM +0800, Shawn Guo wrote:
> > On Sat, Jul 02, 2011 at 03:12:25PM +0200, Wolfram Sang wrote:
> > > On Fri, Jul 01, 2011 at 11:46:29PM +0800, Shawn Guo wrote:
> > > > On Fri, Jul 01, 2011 at 04:44:09PM +0200, Wolfram Sang wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
> > > > > > I send the patch as attachment for now.
> > > > > 
> > > > > Fine with me in this case...
> > > > > 
> > > > > > But I'll have to look into another way of doing this. Corporate mail
> > > > > > system is adding stupid disclaimers, gmail web ui is not working ok
> > > > > > and corporate firewalls avoid using a different smtp server...
> > > > > 
> > > > > Good luck with that!
> > > > > 
> > > > > About the patch itself: I didn't verify the formulas, but it does solve one
> > > > > special problem here. Thanks a lot! So:
> > > > > 
> > > > > Tested-by: Wolfram Sang <w.sang@pengutronix.de>
> > > > > 
> > > > > @chris: If Shawn also likes the patch, I think this is a stable candidate.
> > > > > 
> > > > Thanks for the fixing, Koen.
> > > > 
> > > > Acked-by: Shawn Guo <shawn.guo@freescale.com>
> > > 
> > > Well, maybe not. My colleague complained and I think he is right that we are
> > > mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
> > > be wrong for one value per se.
> > > 
> > If you look at the context of the patch, you will find it's 'div2 - 1'
> > than 'div2' gets written into register.
> 
> Exactly. The '- 1' is why Koen changed the upper limit from < 256 to <= 256.
> The lower limit fix is currently 'if (div2 == 0) div2 == 1', which is a 2:1
> mapping. Not good, or?
> 
So you are saying the patch is a right fix but not the most optimal
one?  In that case, it does not concern me.  I acked it as an valid
fix. 

-- 
Regards,
Shawn

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

[Wolfram Sang]

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


> > > > Well, maybe not. My colleague complained and I think he is right that we are
> > > > mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
> > > > be wrong for one value per se.
> > > > 
> > > If you look at the context of the patch, you will find it's 'div2 - 1'
> > > than 'div2' gets written into register.
> > 
> > Exactly. The '- 1' is why Koen changed the upper limit from < 256 to <= 256.
> > The lower limit fix is currently 'if (div2 == 0) div2 == 1', which is a 2:1
> > mapping. Not good, or?
> > 
> So you are saying the patch is a right fix but not the most optimal
> one?  In that case, it does not concern me.  I acked it as an valid
> fix. 

The patches fixes a few things, but for div2, it is curing the symptoms,
not the cause, I think. If you look at the formula in the datasheet:

rate = ssp / (clock_divide * (1 + clock_rate));

In the code, the calculation of div2 is equal to '1 + clock_rate' (the 1
gets subtracted when the value is written to the register which is a bit
unfortunate; doing it earlier would reduce confusion IMO). So that can
never be 0, it is a divisor. We should really use DIV_ROUND_UP here.
Even when not dealing with 0, this seems needed. Assume:

ssp = 57600000, wanted_rate = 25000000, div1 = 2

will give

div2 = int(1.152) = 1 (meaning 0 + 1, because div2 - 1 will be written)

The rate will thus be:

actual_rate = 57600000 / 2 * (0 + 1)
            = 28800000

-> too fast!

Or did I get something wrong?

Regards,

   Wolfram

-- 
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] mxs-mmc: fix clock rate setting

[Koen Beel]

On Sun, Jul 3, 2011 at 1:54 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Sun, Jul 03, 2011 at 12:31:24PM +0200, Wolfram Sang wrote:
>> On Sun, Jul 03, 2011 at 05:28:52PM +0800, Shawn Guo wrote:
>> > On Sat, Jul 02, 2011 at 03:12:25PM +0200, Wolfram Sang wrote:
>> > > On Fri, Jul 01, 2011 at 11:46:29PM +0800, Shawn Guo wrote:
>> > > > On Fri, Jul 01, 2011 at 04:44:09PM +0200, Wolfram Sang wrote:
>> > > > > Hi,
>> > > > >
>> > > > > On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
>> > > > > > I send the patch as attachment for now.
>> > > > >
>> > > > > Fine with me in this case...
>> > > > >
>> > > > > > But I'll have to look into another way of doing this. Corporate mail
>> > > > > > system is adding stupid disclaimers, gmail web ui is not working ok
>> > > > > > and corporate firewalls avoid using a different smtp server...
>> > > > >
>> > > > > Good luck with that!
>> > > > >
>> > > > > About the patch itself: I didn't verify the formulas, but it does solve one
>> > > > > special problem here. Thanks a lot! So:
>> > > > >
>> > > > > Tested-by: Wolfram Sang <w.sang@pengutronix.de>
>> > > > >
>> > > > > @chris: If Shawn also likes the patch, I think this is a stable candidate.
>> > > > >
>> > > > Thanks for the fixing, Koen.
>> > > >
>> > > > Acked-by: Shawn Guo <shawn.guo@freescale.com>
>> > >
>> > > Well, maybe not. My colleague complained and I think he is right that we are
>> > > mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
>> > > be wrong for one value per se.
>> > >
>> > If you look at the context of the patch, you will find it's 'div2 - 1'
>> > than 'div2' gets written into register.
>>
>> Exactly. The '- 1' is why Koen changed the upper limit from < 256 to <= 256.
>> The lower limit fix is currently 'if (div2 == 0) div2 == 1', which is a 2:1
>> mapping. Not good, or?
>>
> So you are saying the patch is a right fix but not the most optimal
> one?  In that case, it does not concern me.  I acked it as an valid
> fix.

According to the mx23/28 datasheet:
- actual mmc/sdio clock = ssp_sck = (sspclk) / ( clock_divide * (1+clock_rate) )
- clock_divide ranges from 2 to 254, by steps of 2
- clock_rate ranges from 0 to 255, so div2 ranges from 1 to 256
- if div2 is the range [0..1[, the only thing we can do is setting it
on 1. It just can't be lower. The actual clock rate will be lower then
requested.
- div2 will only be < 1 if div1 is 2. This is because div1 is tested
for a good value starting at the lowest possible value (2). Making
div2 higher will not solve the issue, it will only result in an even
lower actual clock
- you could choose the give dev_err ("cannot set clock to ...") but i
would not do that. Default requested clock for high speed sdio is 50
MHz and the default configuration for mx23 does not support this, so
you will also get an error. And having a lower clock then requested is
not that bad as having a higher clock.

In the code (as before):
- div1 = clock_divide (just written in hw)
- div2 = 1+clock_rate (clock_rate = div2 - 1 is written in hw)

So i think its correct. It work here in all tested condition.

Regards,
Koen

>
> --
> Regards,
> Shawn
>
>

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

[Koen Beel]

On Mon, Jul 4, 2011 at 12:34 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
>> > > > Well, maybe not. My colleague complained and I think he is right that we are
>> > > > mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
>> > > > be wrong for one value per se.
>> > > >
>> > > If you look at the context of the patch, you will find it's 'div2 - 1'
>> > > than 'div2' gets written into register.
>> >
>> > Exactly. The '- 1' is why Koen changed the upper limit from < 256 to <= 256.
>> > The lower limit fix is currently 'if (div2 == 0) div2 == 1', which is a 2:1
>> > mapping. Not good, or?
>> >
>> So you are saying the patch is a right fix but not the most optimal
>> one?  In that case, it does not concern me.  I acked it as an valid
>> fix.
>
> The patches fixes a few things, but for div2, it is curing the symptoms,
> not the cause, I think. If you look at the formula in the datasheet:
>
> rate = ssp / (clock_divide * (1 + clock_rate));
>
> In the code, the calculation of div2 is equal to '1 + clock_rate' (the 1
> gets subtracted when the value is written to the register which is a bit
> unfortunate; doing it earlier would reduce confusion IMO). So that can
> never be 0, it is a divisor. We should really use DIV_ROUND_UP here.
> Even when not dealing with 0, this seems needed. Assume:
>
> ssp = 57600000, wanted_rate = 25000000, div1 = 2
>
> will give
>
> div2 = int(1.152) = 1 (meaning 0 + 1, because div2 - 1 will be written)
>
> The rate will thus be:
>
> actual_rate = 57600000 / 2 * (0 + 1)
>            = 28800000
>
> -> too fast!
>
> Or did I get something wrong?
>
> Regards,
>
>   Wolfram

Well, right now the clock freq. is set to the minimum clock value
reaching the requested rate. In current implementation, the rate will
be higher in lot's of cases (all cases where the requested clock freq.
cannot exactly be made).

But the thing is, the driver now enters dev_err, and returns without
changing anything when the clock cannot be made as low as requested.
In that case you will almost certainly end up with a clock being even
higher then the lowest possible. So that not good I think.
I might be better to set the clock as low as possible not matter what
you to after that.

About the rounding. I don't think rounding is good. I think it would
be better to choose between having at least the requested rate (as it
is now; will result is somewhat higher clock then requested in many
cases), or having at maximum the requested clock rate. Both have there
problems.




>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk4RlxoACgkQD27XaX1/VRuf7ACgqQ4PvBf9d0am2ButvDT0ZHy1
> CQwAn1iHNXcl6t46IW7L/l3UkW7J2nxb
> =kbX8
> -----END PGP SIGNATURE-----
>
>

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

[Koen Beel]

On Fri, Jul 1, 2011 at 4:44 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi,
>
> On Fri, Jul 01, 2011 at 03:26:39PM +0200, Koen Beel wrote:
>> I send the patch as attachment for now.
>
> Fine with me in this case...
>
>> But I'll have to look into another way of doing this. Corporate mail
>> system is adding stupid disclaimers, gmail web ui is not working ok
>> and corporate firewalls avoid using a different smtp server...
>
> Good luck with that!
>
> About the patch itself: I didn't verify the formulas, but it does solve one
> special problem here. Thanks a lot! So:

Does it solve any other special problem except for the wrong clock rate setting?


>
> Tested-by: Wolfram Sang <w.sang@pengutronix.de>
>
> @chris: If Shawn also likes the patch, I think this is a stable candidate.
>
> Very minor nits:
>
>> From ab610437fade5b30fdeacd9e959ddf95bc42587a Mon Sep 17 00:00:00 2001
>> From: Koen Beel <koen.beel@barco.com>
>> Date: Thu, 30 Jun 2011 12:00:19 +0200
>> Subject: [PATCH] mxs-mmc: fix clock rate setting
>>
>> 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.
>
> Commit msg should linebreak at 72.
>
>
>> Signed-off-by: Koen beel <koen.beel@barco.com>
>
> Capital 'B'?

Indeed, it should have been:
"Signed-off-by: Koen Beel <koen.beel@barco.com>"

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

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

[Wolfram Sang]

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

> > About the patch itself: I didn't verify the formulas, but it does solve one
> > special problem here. Thanks a lot! So:
> 
> Does it solve any other special problem except for the wrong clock rate setting?

Nope, but that it does :)

-- 
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] mxs-mmc: fix clock rate setting

[Wolfram Sang]

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

On Mon, Jul 04, 2011 at 12:59:29PM +0200, Koen Beel wrote:
> On Mon, Jul 4, 2011 at 12:34 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> >
> >> > > > Well, maybe not. My colleague complained and I think he is right that we are
> >> > > > mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
> >> > > > be wrong for one value per se.
> >> > > >
> >> > > If you look at the context of the patch, you will find it's 'div2 - 1'
> >> > > than 'div2' gets written into register.
> >> >
> >> > Exactly. The '- 1' is why Koen changed the upper limit from < 256 to <= 256.
> >> > The lower limit fix is currently 'if (div2 == 0) div2 == 1', which is a 2:1
> >> > mapping. Not good, or?
> >> >
> >> So you are saying the patch is a right fix but not the most optimal
> >> one?  In that case, it does not concern me.  I acked it as an valid
> >> fix.
> >
> > The patches fixes a few things, but for div2, it is curing the symptoms,
> > not the cause, I think. If you look at the formula in the datasheet:
> >
> > rate = ssp / (clock_divide * (1 + clock_rate));
> >
> > In the code, the calculation of div2 is equal to '1 + clock_rate' (the 1
> > gets subtracted when the value is written to the register which is a bit
> > unfortunate; doing it earlier would reduce confusion IMO). So that can
> > never be 0, it is a divisor. We should really use DIV_ROUND_UP here.
> > Even when not dealing with 0, this seems needed. Assume:
> >
> > ssp = 57600000, wanted_rate = 25000000, div1 = 2
> >
> > will give
> >
> > div2 = int(1.152) = 1 (meaning 0 + 1, because div2 - 1 will be written)
> >
> > The rate will thus be:
> >
> > actual_rate = 57600000 / 2 * (0 + 1)
> >            = 28800000
> >
> > -> too fast!
> >
> > Or did I get something wrong?
> >
> > Regards,
> >
> >   Wolfram
> 
> Well, right now the clock freq. is set to the minimum clock value
> reaching the requested rate. 

Sorry, it gets a bit confusing: what do you mean with "right now"?

> In current implementation, the rate will
> be higher in lot's of cases (all cases where the requested clock freq.
> cannot exactly be made).

Yes.

> But the thing is, the driver now enters dev_err, and returns without
> changing anything when the clock cannot be made as low as requested.
> In that case you will almost certainly end up with a clock being even
> higher then the lowest possible. So that not good I think.
> I might be better to set the clock as low as possible not matter what
> you to after that.

Yes, might be a bit academic (who would want 4kHz ;)), but still
correct. Shawn, do you agree?

> About the rounding. I don't think rounding is good. I think it would

We do rounding already (int conversion). Just in the wrong direction.

> be better to choose between having at least the requested rate (as it
> is now; will result is somewhat higher clock then requested in many
> cases)

You want a rate faster than what the card could do?

>, or having at maximum the requested clock rate. Both have there
> problems.

If I understood you correctly, this is my DIV_ROUND_UP suggestion. Which
problems does it have?

Regards,

   Wolfram

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

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

[Koen Beel]

On Wed, Jul 6, 2011 at 11:38 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Mon, Jul 04, 2011 at 12:59:29PM +0200, Koen Beel wrote:
>> On Mon, Jul 4, 2011 at 12:34 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> >
>> >> > > > Well, maybe not. My colleague complained and I think he is right that we are
>> >> > > > mapping div2 from the range 0 to 256 (inclusive!) to an 8-bit range. This must
>> >> > > > be wrong for one value per se.
>> >> > > >
>> >> > > If you look at the context of the patch, you will find it's 'div2 - 1'
>> >> > > than 'div2' gets written into register.
>> >> >
>> >> > Exactly. The '- 1' is why Koen changed the upper limit from < 256 to <= 256.
>> >> > The lower limit fix is currently 'if (div2 == 0) div2 == 1', which is a 2:1
>> >> > mapping. Not good, or?
>> >> >
>> >> So you are saying the patch is a right fix but not the most optimal
>> >> one?  In that case, it does not concern me.  I acked it as an valid
>> >> fix.
>> >
>> > The patches fixes a few things, but for div2, it is curing the symptoms,
>> > not the cause, I think. If you look at the formula in the datasheet:
>> >
>> > rate = ssp / (clock_divide * (1 + clock_rate));
>> >
>> > In the code, the calculation of div2 is equal to '1 + clock_rate' (the 1
>> > gets subtracted when the value is written to the register which is a bit
>> > unfortunate; doing it earlier would reduce confusion IMO). So that can
>> > never be 0, it is a divisor. We should really use DIV_ROUND_UP here.
>> > Even when not dealing with 0, this seems needed. Assume:
>> >
>> > ssp = 57600000, wanted_rate = 25000000, div1 = 2
>> >
>> > will give
>> >
>> > div2 = int(1.152) = 1 (meaning 0 + 1, because div2 - 1 will be written)
>> >
>> > The rate will thus be:
>> >
>> > actual_rate = 57600000 / 2 * (0 + 1)
>> >            = 28800000
>> >
>> > -> too fast!
>> >
>> > Or did I get something wrong?
>> >
>> > Regards,
>> >
>> >   Wolfram
>>
>> Well, right now the clock freq. is set to the minimum clock value
>> reaching the requested rate.
>
> Sorry, it gets a bit confusing: what do you mean with "right now"?

I mean: in current mainline the actual clock is set to be at least the
requested clock rate.

>
>> In current implementation, the rate will
>> be higher in lot's of cases (all cases where the requested clock freq.
>> cannot exactly be made).
>
> Yes.

This is just the consequence of the fact that the actual clock is set
to be at least the requested clock rate.

>
>> But the thing is, the driver now enters dev_err, and returns without
>> changing anything when the clock cannot be made as low as requested.
>> In that case you will almost certainly end up with a clock being even
>> higher then the lowest possible. So that not good I think.
>> I might be better to set the clock as low as possible not matter what
>> you to after that.
>
> Yes, might be a bit academic (who would want 4kHz ;)), but still
> correct. Shawn, do you agree?
>
>> About the rounding. I don't think rounding is good. I think it would
>
> We do rounding already (int conversion). Just in the wrong direction.
>
>> be better to choose between having at least the requested rate (as it
>> is now; will result is somewhat higher clock then requested in many
>> cases)
>
> You want a rate faster than what the card could do?

Correct, you don't want the clock to be faster then the requested clock.
So the rounding is indeed in the wrong direction now.

>
>>, or having at maximum the requested clock rate. Both have there
>> problems.
>
> If I understood you correctly, this is my DIV_ROUND_UP suggestion. Which
> problems does it have?

I think I misunderstood this suggestion previously. Using DIV_ROUND_UP
would do the rounding in the correct direction.
This should result in: "the actual clock is as high as possible
without being higher then the requested clock."

>
> Regards,
>
>   Wolfram
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk4ULSgACgkQD27XaX1/VRsorQCfVRVm9Vpv4YSsfiMqIFctTKG9
> 7qkAnicSrjSzwObzBjyISDnXz6+Sze2s
> =5LIq
> -----END PGP SIGNATURE-----
>
>

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

[Wolfram Sang]

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


> I think I misunderstood this suggestion previously. Using DIV_ROUND_UP
> would do the rounding in the correct direction.
> This should result in: "the actual clock is as high as possible
> without being higher then the requested clock."

Yes, that should be done in V2. And if you could also put the '- 1' to
the line calculating div2, that would be an extra plus. I think it is a
lot more readable to have the whole formula in one place, and not split
up.

Regards,

   Wolfram

-- 
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] mxs-mmc: fix clock rate setting

[Chris Ball]

Hi Koen,

On Thu, Jun 30 2011, Koen Beel wrote:
> 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.

(I'm waiting for a v2 patch of this patch incorporating Wolfram's
requested changes before merging it.)

Thanks,

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