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

[Wolfram Sang <w.sang@pengutronix.de>]

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