[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Jaehoon Chung]

Hi Stefan,

On 12/30/2016 12:41 AM, Stefan Herbrechtsmeier wrote:
> Hi,
> 
> Am 29.12.2016 um 08:44 schrieb Jaehoon Chung:
>> Hi
>>
>> On 12/29/2016 09:53 AM, Kever Yang wrote:
>>> Hi Stefan,
>>>
>>>      Thanks for your review comment.
>>> On 12/29/2016 02:35 AM, Stefan Herbrechtsmeier wrote:
>>>> Hi,
>>>>
>>>> Am 28.12.2016 um 12:01 schrieb Jaehoon Chung:
>>>>> On 12/28/2016 12:32 PM, Kever Yang wrote:
>>>>>> Init the clock rate to max-frequency from dts with clock driver api.
>>>>>>
>>>>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>>>>> Reviewed-by: Jaehoon Chung <jh80.chung at samsung.com>
>>>> This is an incorrect use of the max-frequency property.
>>>>
>>>> The max-frequency value limit the output clock of the mmc interface and depends on the controller, circuit (level shifter), board and so on. It doesn't represents the clock frequency of the controller.
>>>>
>>>> The clock setup inside the clock framework should use the assigned-clock-rates property. The mmc driver should only enable the clock and pass the clock rate together with the max-frequency to the mmc framework.
>>> I'm not good at mmc controller and driver framework, but seems that the sdhci core treats the max-frequency as the clock input from clock module, right?
> This is true for the current u-boot implementation. But this code is wrong and differs from the kernel. The u-boot mmc framework doesn't distinguish between f_max of the mmc interface and max_clk of the host controller. I have already post a patch to fix this.
> 
>>> What if the mmc controller max-frequency is not equal to the clock module output which is possible? Does kernel deal with this, and how.
> The kernel distinguish between clock module output frequency (host->max_clk) and max-frequency of the mmc interface (mmc->f_max).
> 
>> If my understanding is right, some controller should be broken the CLOCK_BASE capability. (Refer to Linux kernel)
>> And then they needs to get value from CMU.
>>
>> host->max_clk should be used the card's maximum value.
> It represents the (input) base clock of the mmc controller and not the card. A divider of one leads to maximum value.
> 
>> In Linux Kernel's case
>> if max_frequency property is defined, assigned to mmc->f_max
>> and host->f_max is assigned to clk_get_rate() value. (If Broken clock_base capability)
> host->max_clk not host->f_max
> 
>> And check "mmc->f_max > host->f_max" or "mmc->f_max == 0"
>>     if true
>>     then mmc->_f_max = f_max;
>>     else
>>     then mmc->f_max is used to "max_frequency" value.
>>
>> In Conclusion,
>>     host's maximum value is used. ("max_frequency" property is used to QUIRK_BROKEN_CAP_CLOCK_BASE in Linux kernel.)
> The conclusion is wrong. The host->max_clk isn't influenced by the max-frequency. The mmc drivers supplies the host->max_clk via the get_max_clock function if QUIRK_BROKEN_CAP_CLOCK_BASE is set. The mmc->f_max is equal to host->max_clk or max-frequency if set. This means you only need max-frequency if it is lower than the host->max_clk.

host->max_clk is influenced by max-frequency.
get_max_clock function? where is get_max_clock() function used?
Your patches didn't apply yet. waiting for Michal's review.

Did you know what means the quirk for broken clock base?
It means host->max_clk can be 0 or other. Then it should be lower than min_clk likes your mentions.
To prevent this, getting from max_frequency property.

> 
> The host->max_clk is used for the calculation of the divider and multiplier. It represents the clock rate of the controller.
> The mmc->f_max limits the clock rate of the card.

Yes, mmc->f_max is card's maximum frequency.
You can see how to control the clock in drivers/mmc/mmc.c

Kernel and u-boot have to check the card and host's clock values.
And needs to choose the one of them.. f_max is not getting from card. Also it's assigned from host controller.

Really ideal approach is "Using the clk_set_rate()/clk_get_rate() from CMU"..but some controllers don't fully support yet the CMU.
Of_course, it needs to consider the base clock broken case.

> 
>> Kever's patch is not problem.
> The problem is that the patch "init the clock rate to max-frequency" and this is wrong and differs from the kernel which use the assigned-clock-rates. What happens if somebody sets the max-frequency to 400000? Does the clock controller supports such a low frequency? What happens if the clock controller use a different clock as requested and the mmc framework assume the requested clock rate?

Agreed this point, It needs to implement the clk_set_rate() in rockchip_sdhci.c. with value passed by set_ios().

> 
> The mmc drivers shouldn't use the max-frequency to request a clock rate. It should only request the current clock rate or set a default clock rate independent of the max-frequency.
> 
> Regards
>   Stefan
> 
> 
>