'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 <[email protected]>
---
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)
hws[IMX8MN_CLK_GPU_SHADER_DIV] = hws[IMX8MN_CLK_GPU_SHADER];
/* 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);
+ hws[IMX8MN_CLK_A53_CORE] = imx_clk_hw_mux2("arm_a53_core", base + 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 + 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]);
- clk_hw_set_parent(hws[IMX8MN_CLK_A53_CORE], hws[IMX8MN_ARM_PLL_OUT]);
-
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]);
+ 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
'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 <[email protected]>
---
drivers/clk/imx/clk-imx8mp.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 7d558d6..41469e2 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -557,7 +557,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
hws[IMX8MP_CLK_MEDIA_ISP_DIV] = imx_clk_hw_divider2("media_isp_div", "media_isp_cg", ccm_base + 0x8400, 0, 3);
/* CORE SEL */
- hws[IMX8MP_CLK_A53_CORE] = imx_clk_hw_mux2_flags("arm_a53_core", ccm_base + 0x9880, 24, 1, imx8mp_a53_core_sels, ARRAY_SIZE(imx8mp_a53_core_sels), CLK_IS_CRITICAL);
+ hws[IMX8MP_CLK_A53_CORE] = imx_clk_hw_mux2("arm_a53_core", ccm_base + 0x9880, 24, 1, imx8mp_a53_core_sels, ARRAY_SIZE(imx8mp_a53_core_sels));
hws[IMX8MP_CLK_MAIN_AXI] = imx8m_clk_hw_composite_critical("main_axi", imx8mp_main_axi_sels, ccm_base + 0x8800);
hws[IMX8MP_CLK_ENET_AXI] = imx8m_clk_hw_composite("enet_axi", imx8mp_enet_axi_sels, ccm_base + 0x8880);
@@ -729,15 +729,15 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
hws[IMX8MP_CLK_VPU_ROOT] = imx_clk_hw_gate4("vpu_root_clk", "vpu_bus", ccm_base + 0x4630, 0);
hws[IMX8MP_CLK_AUDIO_ROOT] = imx_clk_hw_gate4("audio_root_clk", "ipg_root", ccm_base + 0x4650, 0);
- clk_hw_set_parent(hws[IMX8MP_CLK_A53_SRC], hws[IMX8MP_SYS_PLL1_800M]);
- clk_hw_set_parent(hws[IMX8MP_CLK_A53_CORE], hws[IMX8MP_ARM_PLL_OUT]);
-
hws[IMX8MP_CLK_ARM] = imx_clk_hw_cpu("arm", "arm_a53_core",
hws[IMX8MP_CLK_A53_CORE]->clk,
hws[IMX8MP_CLK_A53_CORE]->clk,
hws[IMX8MP_ARM_PLL_OUT]->clk,
hws[IMX8MP_CLK_A53_DIV]->clk);
+ clk_hw_set_parent(hws[IMX8MP_CLK_A53_SRC], hws[IMX8MP_SYS_PLL1_800M]);
+ clk_hw_set_parent(hws[IMX8MP_CLK_A53_CORE], hws[IMX8MP_ARM_PLL_OUT]);
+
imx_check_clk_hws(hws, IMX8MP_CLK_END);
of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_hw_data);
--
2.7.4
Hi Anson,
One comment inline:
On 25.02.2020 10:49, Anson Huang wrote:
> '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 <[email protected]>
> ---
> 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)
> hws[IMX8MN_CLK_GPU_SHADER_DIV] = hws[IMX8MN_CLK_GPU_SHADER];
>
> /* 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);
> + hws[IMX8MN_CLK_A53_CORE] = imx_clk_hw_mux2("arm_a53_core", base + 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 + 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]);
> - 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.
> -
> 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]);
> + 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);
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:
> > '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 <[email protected]>
> > ---
> > 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)
> > hws[IMX8MN_CLK_GPU_SHADER_DIV] =
> hws[IMX8MN_CLK_GPU_SHADER];
> >
> > /* 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);
> > + hws[IMX8MN_CLK_A53_CORE] = imx_clk_hw_mux2("arm_a53_core",
> base +
> > +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
> > + 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]);
> > - 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
>
> > -
> > 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]);
> > + 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);
>
On Tue, Feb 25, 2020 at 04:49:11PM +0800, Anson Huang wrote:
> '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?
Shawn
> 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 <[email protected]>
> ---
> 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)
> hws[IMX8MN_CLK_GPU_SHADER_DIV] = hws[IMX8MN_CLK_GPU_SHADER];
>
> /* 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);
> + hws[IMX8MN_CLK_A53_CORE] = imx_clk_hw_mux2("arm_a53_core", base + 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 + 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]);
> - clk_hw_set_parent(hws[IMX8MN_CLK_A53_CORE], hws[IMX8MN_ARM_PLL_OUT]);
> -
> 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]);
> + 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
>
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:
> > '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
>
> > 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 <[email protected]>
> > ---
> > 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)
> > hws[IMX8MN_CLK_GPU_SHADER_DIV] =
> hws[IMX8MN_CLK_GPU_SHADER];
> >
> > /* 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);
> > + hws[IMX8MN_CLK_A53_CORE] = imx_clk_hw_mux2("arm_a53_core",
> base +
> > +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
> > + 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]);
> > - clk_hw_set_parent(hws[IMX8MN_CLK_A53_CORE],
> hws[IMX8MN_ARM_PLL_OUT]);
> > -
> > 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]);
> > + 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
> >
On Tue, Feb 25, 2020 at 04:49:11PM +0800, Anson Huang wrote:
> '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 <[email protected]>
Applied all, thanks.