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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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
>