Re: [PATCH 2/2] mmc: sh_mmcif: mmc->f_max should be half of the bus clock

[Simon Horman <horms@verge.net.au>]

Hi Magnus,

On Tue, Mar 27, 2012 at 03:37:42PM +0900, Magnus Damm wrote:
> Hi Simon,
> 
> On Tue, Mar 27, 2012 at 3:01 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Mar 27, 2012 at 01:02:38PM +0900, Magnus Damm wrote:

[snip]

> >> Anyway, I was mainly wondering about the setup of the clocks in the
> >> driver for the MMC host hardware. I got the impression that f_min and
> >> f_max are supposed to be used to point out the minimum and maximum
> >> allowed frequencies that the hardware supports. On our SoCs this
> >> depends on the rate of the parent clock and the number of bits used
> >> for the MMC host hardware divider.
> >>
> >> So the parent clock rate may be rather high, and if it happens to be
> >> set too high on a board level then the MMC hardware block won't be
> >> able to program the frequencies as low as 400 kHz. So my point is only
> >> that we need to make sure that the parent clock rate is set in such a
> >> way that we can have some at least half-high clock frequency for
> >> decent general purpose throughput but if we try to increase the clock
> >> rate too much then we may force the lowest clock rate to go above 400
> >> kHz and then I suspect we may be out of spec. All this is based on
> >> that the parent clock is set statically to some frequency during boot
> >> by the SoC or board code.
> >
> > The current code assumes that the maximum divisor is 512.
> 
> That's good!
> 
> > This may lead to f_min being greater than 400Hz if the parent
> > clock is greater than 200MHz. It seems to me that empirically this
> > has not being occurring else the WARN_ON() in __mmc_set_clock() would
> > be activated. Though perhaps no one notices it.
> 
> Sure, 400 kHz * 512 = 204.8 MHz.
> 
> The reason why we don't hit WARN_ON() may be that mmc_power_up()
> blindly sets host->ios.clock to 400 kHz, 200 kHz and so on - this
> without checking against f_min.
> 
> > In any case, in the context of the current code it seems that changing
> > sh_mmcif_probe() to set f_min to host->clk / 512 will not alter the current
> > behaviour and will simplify the code.
> 
> Yep. It seems to me (only checked sh7372 data sheet) that f_min and
> f_max should be set like this:
> 
> f_min = parent_clk / 512
> f_max = parent_clk / 2
> 
> The real question is in my opinion is how to select a good parent
> clock rate, but that's sort of out of scope here.

Agreed on all counts. I will refresh my outstanding patch series
to set f_min accordingly, the series already sets f_max as you describe.