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

[Ulf Hansson]

>> 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().

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

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/