On Wed, Jul 6, 2011 at 11:38 AM, Wolfram Sang <w.sang@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
