Re: [PATCH v3 12/13] mmc: mmci: add explicit clk control

[Srinivas Kandagatla]

On 27/05/14 15:07, Ulf Hansson wrote:
> > > Hmm. Looking a bit deeper into this, we have some additional related
> > > code to fixup. :-)
> > >
> > > In ->probe(), we do clk_set_rate(100MHz), if the mclk > 100MHz. That's
> > > due to the current variants don't support higher frequency than this.
> > > It seems like the Qcom variant may support up to 208MHz? Now, if
> > > that's the case, we need to add "f_max" to the struct variant_data to
> > > store this information, so we can respect different values while doing
> > > clk_set_rate() at ->probe().
> > Yes, qcom SOCs support more than 100Hhz clock.
> >
> > Probe and clk_set_rate/set_ios should respect this.
> >
> > On the other thought, Should probe actually care about clocks which are
> > explicitly controlled? It should not even attempt to set any frequency to
> > start with.
>
> The 100 MHz is related to constraints set by the specification of the
> IP block, not the MMC/SD/SDIO spec.
>
> Thus at ->probe() we must perform the clk_set_rate().
I agree its valid for controllers which have this constraints.

> > mmc-core would set the right frequency depending on the mmc
> > state-machine respecting f_min and f_max.
> > I think for qcom, probe should just check the if f_max and f_min are valid
> > and set them to defaults if any in the same lines as existing code.
> >
> >
> > > While updating mmc->f_max for host->variant->explicit_mclk_control, we
> > > shouldn't care about using the host->mclk as a limiter, instead, use
> > > min(mmc->f_max, host->variant->f_max) and fallback to fmax.
> > Yes, that's correct, mclk should not be used as limiter. Adding f_max to the
> > variant looks useful.
> >
> >
> > > Not sure how that will affect the logic. :-)
> > >
> > > > > Additionally, this makes me wonder about f_min. I haven't seen
> > > > > anywhere in this patch were that value is being set to proper value,
> > > > > right?
> > > >
> > > > f_min should be 400000 for qcom, I think with the default mclk frequency
> > > > and
> > > > a divider of 512 used for calculating the f_min is bringing down the
> > > > f_min
> > > > to lessthan 400Kz. Which is why its working fine.
> > > > I think the possibility of mclk default frequency being greater than
> > > > 208Mhz
> > > > is very less. so I could either leave it as it is Or force this to 400000
> > > > all the time for qcom chips.
> > >
> > >
> > > No, this seems like a wrong approach.
> > >
> > > I think you would like to do use the clk_round_rate() find out the
> > > lowest possible rate. Or just use a fixed fallback value somehow.
> >
> >
> > clk_round_rate(mclk, 0) might be more generic, instead of fixed fallback.
> > Let the mmc-core figure out what frequency would be best from its table
> > starting from f_min.
>
> Agree!
>
> clk_round_rate(mclk, 100KHz), might be better though - since that is
> actually the lowest request frequency whatsoever.
Perfect.

--srini
> Kind regards
> Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/