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

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

[-- Attachment #1: Type: text/plain, Size: 3217 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]