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