| Message ID | 1418111885-3623-1-git-send-email-b20788@freescale.com |
|---|---|
| State | New |
| Headers | show |
Hello Anson, [Cc += Mike Turquette] On Tue, Dec 09, 2014 at 03:58:05PM +0800, Anson Huang wrote: > For those clk gates which hold share count, since its is_enabled > callback is only checking the share count rather than reading > the hardware register setting, in the late phase of kernel bootup, > the clk_disable_unused action will NOT handle the scenario of > share_count is 0 but the hardware setting is enabled, actually, > uboot normally enables all clk gates, then those shared clk gates s/uboot/U-Boot/ IIRC > will be always enabled until they are used by some modules. What is the purpose of the shared clocks? Just to have different names for a single gate? Wouldn't it be better to fix this problem at the framework level? Then ideally clk_disable_unused would handle these shared clocks just fine. I guess the problem is that this shared clk implementation doesn't fulfill the expectations of the clk framework. This is found in arch/arm/mach-imx/clk-imx6q.c > clk[IMX6QDL_CLK_ASRC] = imx_clk_gate2_shared("asrc", "asrc_podf", base + 0x68, 6, &share_count_asrc); > clk[IMX6QDL_CLK_ASRC_IPG] = imx_clk_gate2_shared("asrc_ipg", "ahb", base + 0x68, 6, &share_count_asrc); > clk[IMX6QDL_CLK_ASRC_MEM] = imx_clk_gate2_shared("asrc_mem", "ahb", base + 0x68, 6, &share_count_asrc); For these clks is_enabled returns just enable_count instead of the status of the corresponding bit in the corresponding register. The reason for that probably is that "asrc" must not be disabled even if unused if "asrc_ipg" is in use. So the problem is there are three struct clk s with a common enable count for a single register bit. Interesting enough these clocks have different parents (the 2nd parameter to imx_clk_gate2_shared) which makes the easy solution to just make asrc_ipg and asrc_mem children of asrc not applicable. Any idea for a sane solution? A multi-parent-dummy-clk would do I think. Other than that the following might work: Ignore share_count in the is_enabled callback (i.e. report if the register bit is set as in the share_count == NULL case) and implement clk->ops->disable_unused that only unsets the bit if share_count == 0? Ugly but IMHO better and more robust than the idea implemented in this patch. Still I'd prefer to make the usual assumptions of the clk framework just work. > So the problem would be: when kernel boot up, the usecount cat > from clk tree is 0, but the clk gates actually is enabled in > hardware register, it will confuse user and bring unnecessary power > consumption, take i.MX6SX for example, the ESAI clk info > is as below, the use count is 0, but the hardware register read > from CCM_CCGR1_CG8 is ungated. > > cat /sys/kernel/debug/clk/clk_summary | grep esai Just a very minor nitpick: Unnecessary usage of cat, grep esai /sys/kernel/debug/clk/clk_summary just produces the same result. :-) > > esai_sel 0 0 393216000 0 0 > esai_pred 0 0 196608000 0 0 > esai_podf 0 0 24576000 0 0 > esai_extal 0 0 24576000 0 0 > esai_mem 0 0 132000000 0 0 > esai_ipg 0 0 132000000 0 0 > > Read CCM_CCGR1: > > 0x020C406C: F33FFF00 This is hardly useful if you don't have the reference manual at hand. I'd believe the commit log even without the examples. Best regards Uwe
Hi, Uwe Best regards! Anson Huang -----Original Message----- From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] Sent: 2014-12-09 4:58 PM To: Huang Yongcai-B20788 Cc: shawn.guo@linaro.org; kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org; Mike Turquette Subject: Re: [PATCH] arm: imx: correct the hardware clock gate setting for shared nodes Hello Anson, [Cc += Mike Turquette] On Tue, Dec 09, 2014 at 03:58:05PM +0800, Anson Huang wrote: > For those clk gates which hold share count, since its is_enabled > callback is only checking the share count rather than reading the > hardware register setting, in the late phase of kernel bootup, the > clk_disable_unused action will NOT handle the scenario of share_count > is 0 but the hardware setting is enabled, actually, uboot normally > enables all clk gates, then those shared clk gates s/uboot/U-Boot/ IIRC > will be always enabled until they are used by some modules. What is the purpose of the shared clocks? Just to have different names for a single gate? [Anson] The shared clocks is to meet the case that where one clk gate control multi clks path in module, for example, in esai module, there are three clk paths, esai_core, esai_ipg and esai_mem, but they share same clk gate in CCM_CCGR, and their parents are different...No very good hardware design for us. Wouldn't it be better to fix this problem at the framework level? Then ideally clk_disable_unused would handle these shared clocks just fine. [Anson] Agree, handle it in framework is much better than doing it in every platform, my bad:( I guess the problem is that this shared clk implementation doesn't fulfill the expectations of the clk framework. This is found in arch/arm/mach-imx/clk-imx6q.c > clk[IMX6QDL_CLK_ASRC] = imx_clk_gate2_shared("asrc", "asrc_podf", base + 0x68, 6, &share_count_asrc); > clk[IMX6QDL_CLK_ASRC_IPG] = imx_clk_gate2_shared("asrc_ipg", "ahb", base + 0x68, 6, &share_count_asrc); > clk[IMX6QDL_CLK_ASRC_MEM] = imx_clk_gate2_shared("asrc_mem", "ahb", base + 0x68, 6, &share_count_asrc); For these clks is_enabled returns just enable_count instead of the status of the corresponding bit in the corresponding register. The reason for that probably is that "asrc" must not be disabled even if unused if "asrc_ipg" is in use. So the problem is there are three struct clk s with a common enable count for a single register bit. Interesting enough these clocks have different parents (the 2nd parameter to imx_clk_gate2_shared) which makes the easy solution to just make asrc_ipg and asrc_mem children of asrc not applicable. Any idea for a sane solution? A multi-parent-dummy-clk would do I think. Other than that the following might work: Ignore share_count in the is_enabled callback (i.e. report if the register bit is set as in the share_count == NULL case) and implement clk->ops->disable_unused that only unsets the bit if share_count == 0? Ugly but IMHO better and more robust than the idea implemented in this patch. Still I'd prefer to make the usual assumptions of the clk framework just work. [Anson] I think this solution is feasible for us, as disable_unused is only used in late phase of kernel boot up, I think adding such callback is acceptable. > So the problem would be: when kernel boot up, the usecount cat from > clk tree is 0, but the clk gates actually is enabled in hardware > register, it will confuse user and bring unnecessary power > consumption, take i.MX6SX for example, the ESAI clk info is as below, > the use count is 0, but the hardware register read from CCM_CCGR1_CG8 > is ungated. > > cat /sys/kernel/debug/clk/clk_summary | grep esai Just a very minor nitpick: Unnecessary usage of cat, grep esai /sys/kernel/debug/clk/clk_summary just produces the same result. :-) [Anson] Thanks for sharing such knowledge:) > > esai_sel 0 0 393216000 0 0 > esai_pred 0 0 196608000 0 0 > esai_podf 0 0 24576000 0 0 > esai_extal 0 0 24576000 0 0 > esai_mem 0 0 132000000 0 0 > esai_ipg 0 0 132000000 0 0 > > Read CCM_CCGR1: > > 0x020C406C: F33FFF00 This is hardly useful if you don't have the reference manual at hand. I'd believe the commit log even without the examples. [Anson] Agreed, I will remove it in V2 patch. Many thanks for reviewing my patch and very good advice:) Please help review V2 patch. Best regards Uwe
Hello, On Wed, Dec 10, 2014 at 08:49:27AM +0000, Anson.Huang@freescale.com wrote: > > > > esai_sel 0 0 393216000 0 0 > > esai_pred 0 0 196608000 0 0 > > esai_podf 0 0 24576000 0 0 > > esai_extal 0 0 24576000 0 0 > > esai_mem 0 0 132000000 0 0 > > esai_ipg 0 0 132000000 0 0 > > > > Read CCM_CCGR1: > > > > 0x020C406C: F33FFF00 > This is hardly useful if you don't have the reference manual at hand. > I'd believe the commit log even without the examples. > [Anson] Agreed, I will remove it in V2 patch. > > Many thanks for reviewing my patch and very good advice:) Please help review V2 patch. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > If you want to do me (and most of the other readers of lakml I guess) another favour: please fix your quoting style. The way you replied to my mail is hard to parse in a console mail reader as you might be able to see from the text I quoted above. Best regards Uwe
Hello, On Wed, Dec 10, 2014 at 05:11:26PM +0800, Anson Huang wrote: > OK, very sorry for using OUTLOOK to reply such review mail, I will use console mail > to reply it from now:) Much better \o/. Note that Outlook can be tuned to behave better, too (still not perfect AFAIK). Back when I was forced to use Outlook I liked Outlook Quote Fix[1]. I hope this is not necessary any more today. Best regards Uwe [1] http://home.in.tum.de/~jain/software/outlook-quotefix/
diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c index 5951660..fe53473 100644 --- a/arch/arm/mach-imx/clk-imx6q.c +++ b/arch/arm/mach-imx/clk-imx6q.c @@ -369,6 +369,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) /* name parent_name reg shift */ clk[IMX6QDL_CLK_APBH_DMA] = imx_clk_gate2("apbh_dma", "usdhc3", base + 0x68, 4); + /* make sure the initial clock gate setting of share_count is off */ + writel_relaxed(readl_relaxed(base + 0x68) & (~(0x3 << 6)), base + 0x68); clk[IMX6QDL_CLK_ASRC] = imx_clk_gate2_shared("asrc", "asrc_podf", base + 0x68, 6, &share_count_asrc); clk[IMX6QDL_CLK_ASRC_IPG] = imx_clk_gate2_shared("asrc_ipg", "ahb", base + 0x68, 6, &share_count_asrc); clk[IMX6QDL_CLK_ASRC_MEM] = imx_clk_gate2_shared("asrc_mem", "ahb", base + 0x68, 6, &share_count_asrc); @@ -385,6 +387,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) else clk[IMX6Q_CLK_ECSPI5] = imx_clk_gate2("ecspi5", "ecspi_root", base + 0x6c, 8); clk[IMX6QDL_CLK_ENET] = imx_clk_gate2("enet", "ipg", base + 0x6c, 10); + /* make sure the initial clock gate setting of share_count is off */ + writel_relaxed(readl_relaxed(base + 0x6c) & (~(0x3 << 16)), base + 0x6c); clk[IMX6QDL_CLK_ESAI_EXTAL] = imx_clk_gate2_shared("esai_extal", "esai_podf", base + 0x6c, 16, &share_count_esai); clk[IMX6QDL_CLK_ESAI_IPG] = imx_clk_gate2_shared("esai_ipg", "ipg", base + 0x6c, 16, &share_count_esai); clk[IMX6QDL_CLK_ESAI_MEM] = imx_clk_gate2_shared("esai_mem", "ahb", base + 0x6c, 16, &share_count_esai); @@ -443,6 +447,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) clk[IMX6QDL_CLK_SDMA] = imx_clk_gate2("sdma", "ahb", base + 0x7c, 6); clk[IMX6QDL_CLK_SPBA] = imx_clk_gate2("spba", "ipg", base + 0x7c, 12); clk[IMX6QDL_CLK_SPDIF] = imx_clk_gate2("spdif", "spdif_podf", base + 0x7c, 14); + /* make sure the initial clock gate setting of share_count is off */ + writel_relaxed(readl_relaxed(base + 0x7c) & (~(0x3f << 18)), base + 0x7c); clk[IMX6QDL_CLK_SSI1_IPG] = imx_clk_gate2_shared("ssi1_ipg", "ipg", base + 0x7c, 18, &share_count_ssi1); clk[IMX6QDL_CLK_SSI2_IPG] = imx_clk_gate2_shared("ssi2_ipg", "ipg", base + 0x7c, 20, &share_count_ssi2); clk[IMX6QDL_CLK_SSI3_IPG] = imx_clk_gate2_shared("ssi3_ipg", "ipg", base + 0x7c, 22, &share_count_ssi3); diff --git a/arch/arm/mach-imx/clk-imx6sl.c b/arch/arm/mach-imx/clk-imx6sl.c index e982ebe..6af14a8 100644 --- a/arch/arm/mach-imx/clk-imx6sl.c +++ b/arch/arm/mach-imx/clk-imx6sl.c @@ -396,6 +396,8 @@ static void __init imx6sl_clocks_init(struct device_node *ccm_node) clks[IMX6SL_CLK_SDMA] = imx_clk_gate2("sdma", "ipg", base + 0x7c, 6); clks[IMX6SL_CLK_SPBA] = imx_clk_gate2("spba", "ipg", base + 0x7c, 12); clks[IMX6SL_CLK_SPDIF] = imx_clk_gate2("spdif", "spdif0_podf", base + 0x7c, 14); + /* make sure the initial clock gate setting of share_count is off */ + writel_relaxed(readl_relaxed(base + 0x7c) & (~(0x3f << 18)), base + 0x7c); clks[IMX6SL_CLK_SSI1_IPG] = imx_clk_gate2_shared("ssi1_ipg", "ipg", base + 0x7c, 18, &share_count_ssi1); clks[IMX6SL_CLK_SSI2_IPG] = imx_clk_gate2_shared("ssi2_ipg", "ipg", base + 0x7c, 20, &share_count_ssi2); clks[IMX6SL_CLK_SSI3_IPG] = imx_clk_gate2_shared("ssi3_ipg", "ipg", base + 0x7c, 22, &share_count_ssi3); diff --git a/arch/arm/mach-imx/clk-imx6sx.c b/arch/arm/mach-imx/clk-imx6sx.c index 17354a1..e42052e 100644 --- a/arch/arm/mach-imx/clk-imx6sx.c +++ b/arch/arm/mach-imx/clk-imx6sx.c @@ -379,6 +379,8 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node) clks[IMX6SX_CLK_AIPS_TZ1] = imx_clk_gate2("aips_tz1", "ahb", base + 0x68, 0); clks[IMX6SX_CLK_AIPS_TZ2] = imx_clk_gate2("aips_tz2", "ahb", base + 0x68, 2); clks[IMX6SX_CLK_APBH_DMA] = imx_clk_gate2("apbh_dma", "usdhc3", base + 0x68, 4); + /* make sure the initial clock gate setting of share_count is off */ + writel_relaxed(readl_relaxed(base + 0x68) & (~(0x3 << 6)), base + 0x68); clks[IMX6SX_CLK_ASRC_MEM] = imx_clk_gate2_shared("asrc_mem", "ahb", base + 0x68, 6, &share_count_asrc); clks[IMX6SX_CLK_ASRC_IPG] = imx_clk_gate2_shared("asrc_ipg", "ahb", base + 0x68, 6, &share_count_asrc); clks[IMX6SX_CLK_CAAM_MEM] = imx_clk_gate2("caam_mem", "ahb", base + 0x68, 8); @@ -400,6 +402,8 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node) clks[IMX6SX_CLK_ECSPI5] = imx_clk_gate2("ecspi5", "ecspi_podf", base + 0x6c, 8); clks[IMX6SX_CLK_EPIT1] = imx_clk_gate2("epit1", "perclk", base + 0x6c, 12); clks[IMX6SX_CLK_EPIT2] = imx_clk_gate2("epit2", "perclk", base + 0x6c, 14); + /* make sure the initial clock gate setting of share_count is off */ + writel_relaxed(readl_relaxed(base + 0x6c) & (~(0x3 << 16)), base + 0x6c); clks[IMX6SX_CLK_ESAI_EXTAL] = imx_clk_gate2_shared("esai_extal", "esai_podf", base + 0x6c, 16, &share_count_esai); clks[IMX6SX_CLK_ESAI_IPG] = imx_clk_gate2_shared("esai_ipg", "ahb", base + 0x6c, 16, &share_count_esai); clks[IMX6SX_CLK_ESAI_MEM] = imx_clk_gate2_shared("esai_mem", "ahb", base + 0x6c, 16, &share_count_esai); @@ -455,6 +459,8 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node) clks[IMX6SX_CLK_ROM] = imx_clk_gate2("rom", "ahb", base + 0x7c, 0); clks[IMX6SX_CLK_SDMA] = imx_clk_gate2("sdma", "ahb", base + 0x7c, 6); clks[IMX6SX_CLK_SPBA] = imx_clk_gate2("spba", "ipg", base + 0x7c, 12); + /* make sure the initial clock gate setting of share_count is off */ + writel_relaxed(readl_relaxed(base + 0x7c) & (~(0x3 << 14)) & (~(0x3f << 18)), base + 0x7c); clks[IMX6SX_CLK_AUDIO] = imx_clk_gate2_shared("audio", "audio_podf", base + 0x7c, 14, &share_count_audio); clks[IMX6SX_CLK_SPDIF] = imx_clk_gate2_shared("spdif", "spdif_podf", base + 0x7c, 14, &share_count_audio); clks[IMX6SX_CLK_SSI1_IPG] = imx_clk_gate2_shared("ssi1_ipg", "ipg", base + 0x7c, 18, &share_count_ssi1);
For those clk gates which hold share count, since its is_enabled callback is only checking the share count rather than reading the hardware register setting, in the late phase of kernel bootup, the clk_disable_unused action will NOT handle the scenario of share_count is 0 but the hardware setting is enabled, actually, uboot normally enables all clk gates, then those shared clk gates will be always enabled until they are used by some modules. So the problem would be: when kernel boot up, the usecount cat from clk tree is 0, but the clk gates actually is enabled in hardware register, it will confuse user and bring unnecessary power consumption, take i.MX6SX for example, the ESAI clk info is as below, the use count is 0, but the hardware register read from CCM_CCGR1_CG8 is ungated. cat /sys/kernel/debug/clk/clk_summary | grep esai esai_sel 0 0 393216000 0 0 esai_pred 0 0 196608000 0 0 esai_podf 0 0 24576000 0 0 esai_extal 0 0 24576000 0 0 esai_mem 0 0 132000000 0 0 esai_ipg 0 0 132000000 0 0 Read CCM_CCGR1: 0x020C406C: F33FFF00 This patch disables all those clk gates which hold share count before registering them, then the hardware status will match the clk tree info. Signed-off-by: Anson Huang <b20788@freescale.com> --- arch/arm/mach-imx/clk-imx6q.c | 6 ++++++ arch/arm/mach-imx/clk-imx6sl.c | 2 ++ arch/arm/mach-imx/clk-imx6sx.c | 6 ++++++ 3 files changed, 14 insertions(+)