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 > > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot