From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Herbrechtsmeier Date: Mon, 2 Jan 2017 17:59:07 +0100 Subject: [U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc In-Reply-To: <21dab344-3ff8-f7a8-c39b-e8dde2e9204f@samsung.com> References: <1482895956-25171-1-git-send-email-kever.yang@rock-chips.com> <6bcdd2f6-82e6-36dc-8882-470dcfc83084@samsung.com> <50d2fa0b-ae37-c613-983e-6b8917bc4a8a@herbrechtsmeier.net> <58645E9F.60300@rock-chips.com> <290f9edc-2811-99d7-7ba5-fddf746cf130@samsung.com> <7e44ba33-4043-6a7c-9208-8f46a19bf378@herbrechtsmeier.net> <71fdc847-e179-f478-c613-11f3a9c9e47d@samsung.com> <21dab344-3ff8-f7a8-c39b-e8dde2e9204f@samsung.com> Message-ID: <7ce94c21-ec59-0872-b1e1-b637d3a12e48@herbrechtsmeier.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, Am 02.01.2017 um 02:29 schrieb Jaehoon Chung: > Hi Stefan, [snip] >>> 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". The 'max-frequency' is read in host.c into f_max and used to limit the maximum clock in mmc_set_clock() in core.c. In sdhci.c the f_max is set to max_clk if f_max is zero or higher then max_clk. The whole sdhci.c use only the max_clk to calculate the divider and multiplier for the requested clock. Most sdhci-*.c drivers use a preset clock rate or set a fixed clock rate. Only the sdhci-bcm-kona.c and sdhci-st.c use the f_max for clk_set_rate(). They should instead use the 'clock-frequency' or 'assigned-clock-rates'. Especially the last assume three specific clock rates and set the clock to a minimum of 50 MHz even for a lower 'max-frequency'. You always need to values. One to set the clock and one to limit the max-frequency independent of the base clock. >>>>> 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. Where can I find this code? Even in u-boot some drivers use the preset clock values. > Since driver's divider has a limitation. So i think it needs to use the "clk_set_rate()". Do you plan to change all Linux drivers? > >>>> 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. But then you need to encode this specific values inside the device tree and don't misuse the 'max-frequency' property. You need to support a fixed clock frequency for sdhci controller with a lower 'max-frequency' for the external mmc interface. Regards Stefan