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. 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----- > > -- 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
