Hi, Shawn

Subject: Re: [PATCH 1/4] clk: imx8mn: A53 core clock no need to be critical

On Tue, Feb 25, 2020 at 04:49:11PM +0800, Anson Huang wrote:

quoted

'A53_CORE' is just a mux and no need to be critical, being critical
will cause its parent clock always ON which does NOT make sense,

I do not quite understand what problem this patch is trying to fix.  In the end,
all the ancestor clocks of "arm", including "arm_a53_core" will still be ON, as
"arm" has CLK_IS_CRITICAL flag.  What is the difference you are trying to
make here?

This patch actually is to fix the clock parent switch flow of A53, previous flow is incorrect,
the reason why it works is the IMX8MN_CLK_A53_CORE is marked as CRITICAL,
but if with correct flow of parent switch, the "arm" as CLK_IS_CRITICAL flag is enough and
IMX8MN_CLK_A53_CORE will be enabled because it is "arm" clk's parent.

The A53 clock parent switch should be put after the "arm" clk creation, then no need to have
CLK_IS_CRITICAL flag for IMX8MN_CLK_A53_CORE, and its usecount will be 1 as expected.
Previous implementation will has use count equal 2 which is incorrect.

Anson

Shawn

quoted

to make sure CPU's hardware clock source NOT being disabled during
clock tree setup, need to move the 'A53_SRC'/'A53_CORE' reparent
operations to after critical clock 'ARM_CLK' setup finished.

Signed-off-by: Anson Huang <redacted>
---
 drivers/clk/imx/clk-imx8mn.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mn.c

b/drivers/clk/imx/clk-imx8mn.c index 83618af..0bc7070 100644

--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c

@@ -428,7 +428,7 @@ static int imx8mn_clocks_probe(struct

platform_device *pdev)

quoted

 	hws[IMX8MN_CLK_GPU_SHADER_DIV] =

hws[IMX8MN_CLK_GPU_SHADER];

quoted

 	/* CORE SEL */
-	hws[IMX8MN_CLK_A53_CORE] =

imx_clk_hw_mux2_flags("arm_a53_core", base + 0x9880, 24, 1,
imx8mn_a53_core_sels, ARRAY_SIZE(imx8mn_a53_core_sels),
CLK_IS_CRITICAL);

quoted

+	hws[IMX8MN_CLK_A53_CORE] = imx_clk_hw_mux2("arm_a53_core",

base +

quoted

+0x9880, 24, 1, imx8mn_a53_core_sels,
+ARRAY_SIZE(imx8mn_a53_core_sels));

 	/* BUS */
 	hws[IMX8MN_CLK_MAIN_AXI] =
imx8m_clk_hw_composite_critical("main_axi", imx8mn_main_axi_sels,

base

quoted

+ 0x8800); @@ -559,15 +559,15 @@ static int imx8mn_clocks_probe(struct
platform_device *pdev)

 	hws[IMX8MN_CLK_DRAM_ALT_ROOT] =
imx_clk_hw_fixed_factor("dram_alt_root", "dram_alt", 1, 4);

-	clk_hw_set_parent(hws[IMX8MN_CLK_A53_SRC],

hws[IMX8MN_SYS_PLL1_800M]);

quoted

-	clk_hw_set_parent(hws[IMX8MN_CLK_A53_CORE],

hws[IMX8MN_ARM_PLL_OUT]);

quoted

-
 	hws[IMX8MN_CLK_ARM] = imx_clk_hw_cpu("arm", "arm_a53_core",
 					   hws[IMX8MN_CLK_A53_CORE]->clk,
 					   hws[IMX8MN_CLK_A53_CORE]->clk,
 					   hws[IMX8MN_ARM_PLL_OUT]->clk,
 					   hws[IMX8MN_CLK_A53_DIV]->clk);

+	clk_hw_set_parent(hws[IMX8MN_CLK_A53_SRC],

hws[IMX8MN_SYS_PLL1_800M]);

quoted

+	clk_hw_set_parent(hws[IMX8MN_CLK_A53_CORE],
+hws[IMX8MN_ARM_PLL_OUT]);
+
 	imx_check_clk_hws(hws, IMX8MN_CLK_END);

 	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
clk_hw_data);
--
2.7.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel