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

Koen Beel
2011-07-04T10:59:31Z

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