Hi, Daniel

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

Hi Anson,

One comment inline:

On 25.02.2020 10:49, 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, 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]);


Why do you need to move this code? If there is a reason please add a
separate patch and explain why.

I have explained the reason in the commit message, maybe NOT detail enough
for you, if these two set parent is put before ARM_CLK register, it will cause ARM_PLL
being disabled as no consumer using it, so the changes must be done in this patch.
After ARM_CLK register done, as it is critical, its parent will be always ON, hence re-parent
is OK. 

Thanks,
Anson

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);

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