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

[Jaehoon Chung]

Hi Stefan,

On 12/31/2016 12:07 AM, Stefan Herbrechtsmeier wrote:
> Hi,

[snip]

>>>> 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.
> My comments refer to the linux kernel sdhci implementation.

I knew it's referred to Linux kernel. 

> 
>> host->max_clk is influenced by max-frequency.
> Where is the host->max_clk influenced by the max-frequency?
> 
>> get_max_clock function? where is get_max_clock() function used?
> It is used in the kernel to set host->max_clk if the SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN is set.
> 
>> Your patches didn't apply yet. waiting for Michal's review.
> Only one patch is waiting Michal's review and the mmc clock separation could be applied without the other patches.

Yep, If there are no issue, i will pick the patch relevant to MMC.
My meaning isn't "your patch is wrong". Your patch is right way.
but if u-boot didn't implement everything like kernel, I'm believing that current u-boot is changing to status likes Linux kernel

> 
>> Did you know what means the quirk for broken clock base?
>> It means host->max_clk can be 0 or other.
> It means that the host->max_clk could not be extracted from the SDHCI_CAPABILITIES and need to be provided by the driver.
> 
>>   Then it should be lower than min_clk likes your mentions.
>> To prevent this, getting from max_frequency property.
> Why do you think that max-frequency influences the host->max_clk?
> The host->max_clk could be read from the SDHCI_CAPABILITIES or need to be provided by the driver. The host->max_clk is a fixed clock rate and the mmc->f_max limits the generated frequencies of the sdhci controller. The host->max_clk depends on the processor and the mmc->f_max depends on the board, card and so on.
> 
>>> 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.
> The mmc->f_max and host->max_clk have different purposes. The mmc->f_max limits the requested frequency. The host->max_clk represents the clock frequency in front of the divider and optional multiplier and is used to calculate the divider and multiplier. The host->max_clk depends on the CMU and is never changed. The mmc->f_max depends on the board.
> 
>> Really ideal approach is "Using the clk_set_rate()/clk_get_rate() from CMU"..but some controllers don't fully support yet the CMU.
> You only need the clk_get_rate() to get the host->max_clk or let the driver set this value as my patch does.

No. we needs to have both "clk_get_rate() and clk_set_rate()".

> 
>> Of_course, it needs to consider the base clock broken case.
> The whole discussion is about the base clock broken case. Otherwise the host->max_clk is extracted from the SDHCI_CAPABILITIES.
> The linux kernel use a callback to request the host->max_clk from the driver in the base clock broken case. The current u-boot implementation only supports the host->max_clk but call it unfortunately mmc->cfg->f_max which could be mistaken as mmc->f_max from the kernel which represents max-frequency.
> 

Linux kernel has two options. One is get_max_clock(), the other is "using 'max-frequency' property".

>>>> 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().
> Does we speak about the sdhci or dw_mmc controller? The sdhci don't change the host->max_clk and don't need the clk_set_rate(). It have its own divider and optional multiplier and doesn't change the base clock.
> 

sdhci and dwmmc controller are using the clk_set_rate() in each SoC's drivers.
Since driver's divider has a limitation. So i think it needs to use the "clk_set_rate()".

>>> 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.
> 
> Back to this patch. It should use the CONFIG_ROCKCHIP_SDHCI_MAX_FREQ in the clk_set_rate() or use the default rate and only request it with clk_get_rate().
> 
> Maybe my last patch could be generalized and the max-frequency support could be moved inside the sdhci driver.

I don't want to use CONFIG_ROCKCHIP_SDHCI_MAX_FREQ...i will remove the all CONFIG_xxxx for using value.

Best Regards,
Jaehoon Chung

> 
> Regards
>   Stefan
> 
> 
> 
>