| Message ID | BANLkTim5aHJHbtbw-ZWnqdjdj_nxWZuwog@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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>
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.
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.
> > 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 :)
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
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----- > > -- 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
> 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
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