LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks
@ 2021-08-10 15:14 Ahmad Fatoum
  2021-08-18 15:14 ` Abel Vesa
  2021-08-24 12:15 ` Abel Vesa
  0 siblings, 2 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2021-08-10 15:14 UTC (permalink / raw)
  To: Abel Vesa, Michael Turquette, Stephen Boyd, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Peng Fan, Leonard Crestez
  Cc: Ahmad Fatoum, linux-arm-kernel, linux-kernel, linux-clk

On the i.MX8M*, the TF-A exposes a SiP (Silicon Provider) service
for DDR frequency scaling. The imx8m-ddrc-devfreq driver calls the
SiP and then does clk_set_parent on the DDR muxes to synchronize
the clock tree.

Since 936c383673b9 ("clk: imx: fix composite peripheral flags"),
these TF-A managed muxes have SET_PARENT_GATE set, which results
in imx8m-ddrc-devfreq's clk_set_parent after SiP failing with -EBUSY:

	echo 25000000 > userspace/set_freq
	imx8m-ddrc-devfreq 3d400000.memory-controller: failed to set
		dram_apb parent: -16

Fix this by adding a new i.MX composite flag for firmware managed
clocks, which clears SET_PARENT_GATE.

This is safe to do, because updating the Linux clock tree to reflect
reality will always be glitch-free.

Fixes: 936c383673b9 ("clk: imx: fix composite peripheral flags")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
To: Abel Vesa <abel.vesa@nxp.com>
To: Michael Turquette <mturquette@baylibre.com>
To: Stephen Boyd <sboyd@kernel.org>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: NXP Linux Team <linux-imx@nxp.com>
To: Peng Fan <peng.fan@nxp.com>
To: Leonard Crestez <leonard.crestez@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-clk@vger.kernel.org
---
 drivers/clk/imx/clk-composite-8m.c |  3 ++-
 drivers/clk/imx/clk-imx8mm.c       |  7 ++++---
 drivers/clk/imx/clk-imx8mn.c       |  7 ++++---
 drivers/clk/imx/clk-imx8mq.c       |  7 ++++---
 drivers/clk/imx/clk.h              | 16 ++++++++++++++--
 5 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index 2c309e3dc8e3..04e728538cef 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -216,7 +216,8 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
 		div->width = PCG_PREDIV_WIDTH;
 		divider_ops = &imx8m_clk_composite_divider_ops;
 		mux_ops = &clk_mux_ops;
-		flags |= CLK_SET_PARENT_GATE;
+		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
+			flags |= CLK_SET_PARENT_GATE;
 	}
 
 	div->lock = &imx_ccm_lock;
diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index f1919fafb124..73cc9805ec59 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -470,10 +470,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 
 	/*
 	 * DRAM clocks are manipulated from TF-A outside clock framework.
-	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
+	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
+	 * as div value should always be read from hardware
 	 */
-	hws[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
-	hws[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
+	hws[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
+	hws[IMX8MM_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
 
 	/* IP */
 	hws[IMX8MM_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mm_vpu_g1_sels, base + 0xa100);
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index 88f6630cd472..0a76f969b28b 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -453,10 +453,11 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
 
 	/*
 	 * DRAM clocks are manipulated from TF-A outside clock framework.
-	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
+	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
+	 * as div value should always be read from hardware
 	 */
-	hws[IMX8MN_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
-	hws[IMX8MN_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mn_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
+	hws[IMX8MN_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000);
+	hws[IMX8MN_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mn_dram_apb_sels, base + 0xa080);
 
 	hws[IMX8MN_CLK_DISP_PIXEL] = imx8m_clk_hw_composite("disp_pixel", imx8mn_disp_pixel_sels, base + 0xa500);
 	hws[IMX8MN_CLK_SAI2] = imx8m_clk_hw_composite("sai2", imx8mn_sai2_sels, base + 0xa600);
diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index c491bc9c61ce..83cc2b1c3294 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -449,11 +449,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
 
 	/*
 	 * DRAM clocks are manipulated from TF-A outside clock framework.
-	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
+	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
+	 * as div value should always be read from hardware
 	 */
 	hws[IMX8MQ_CLK_DRAM_CORE] = imx_clk_hw_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), CLK_IS_CRITICAL);
-	hws[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
-	hws[IMX8MQ_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mq_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
+	hws[IMX8MQ_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000);
+	hws[IMX8MQ_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mq_dram_apb_sels, base + 0xa080);
 
 	/* IP */
 	hws[IMX8MQ_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mq_vpu_g1_sels, base + 0xa100);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 7571603bee23..e144f983fd8c 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -530,8 +530,9 @@ struct clk_hw *imx_clk_hw_cpu(const char *name, const char *parent_name,
 		struct clk *div, struct clk *mux, struct clk *pll,
 		struct clk *step);
 
-#define IMX_COMPOSITE_CORE	BIT(0)
-#define IMX_COMPOSITE_BUS	BIT(1)
+#define IMX_COMPOSITE_CORE		BIT(0)
+#define IMX_COMPOSITE_BUS		BIT(1)
+#define IMX_COMPOSITE_FW_MANAGED	BIT(2)
 
 struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
 					    const char * const *parent_names,
@@ -567,6 +568,17 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
 		ARRAY_SIZE(parent_names), reg, 0, \
 		flags | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
 
+#define __imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, flags) \
+	imx8m_clk_hw_composite_flags(name, parent_names, \
+		ARRAY_SIZE(parent_names), reg, IMX_COMPOSITE_FW_MANAGED, \
+		flags | CLK_GET_RATE_NOCACHE | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
+
+#define imx8m_clk_hw_fw_managed_composite(name, parent_names, reg) \
+	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, 0)
+
+#define imx8m_clk_hw_fw_managed_composite_critical(name, parent_names, reg) \
+	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, CLK_IS_CRITICAL)
+
 #define __imx8m_clk_composite(name, parent_names, reg, flags) \
 	to_clk(__imx8m_clk_hw_composite(name, parent_names, reg, flags))
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks
  2021-08-10 15:14 [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks Ahmad Fatoum
@ 2021-08-18 15:14 ` Abel Vesa
  2021-08-18 15:28   ` Ahmad Fatoum
  2021-08-24 12:15 ` Abel Vesa
  1 sibling, 1 reply; 5+ messages in thread
From: Abel Vesa @ 2021-08-18 15:14 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Peng Fan,
	Leonard Crestez, linux-arm-kernel, linux-kernel, linux-clk

On 21-08-10 17:14:33, Ahmad Fatoum wrote:
> On the i.MX8M*, the TF-A exposes a SiP (Silicon Provider) service
> for DDR frequency scaling. The imx8m-ddrc-devfreq driver calls the
> SiP and then does clk_set_parent on the DDR muxes to synchronize
> the clock tree.
> 
> Since 936c383673b9 ("clk: imx: fix composite peripheral flags"),
> these TF-A managed muxes have SET_PARENT_GATE set, which results
> in imx8m-ddrc-devfreq's clk_set_parent after SiP failing with -EBUSY:
> 
> 	echo 25000000 > userspace/set_freq
> 	imx8m-ddrc-devfreq 3d400000.memory-controller: failed to set
> 		dram_apb parent: -16
> 
> Fix this by adding a new i.MX composite flag for firmware managed
> clocks, which clears SET_PARENT_GATE.
> 
> This is safe to do, because updating the Linux clock tree to reflect
> reality will always be glitch-free.
> 
> Fixes: 936c383673b9 ("clk: imx: fix composite peripheral flags")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

I'm OK with this.

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

BTW, let me know if you are interested in the interconnect+devfreq
(the replacement for NXP's internal busfreq). I have a new version of
that patchset which I intend to send soon for review.

> ---
> To: Abel Vesa <abel.vesa@nxp.com>
> To: Michael Turquette <mturquette@baylibre.com>
> To: Stephen Boyd <sboyd@kernel.org>
> To: Shawn Guo <shawnguo@kernel.org>
> To: Sascha Hauer <s.hauer@pengutronix.de>
> To: Pengutronix Kernel Team <kernel@pengutronix.de>
> To: Fabio Estevam <festevam@gmail.com>
> To: NXP Linux Team <linux-imx@nxp.com>
> To: Peng Fan <peng.fan@nxp.com>
> To: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/clk/imx/clk-composite-8m.c |  3 ++-
>  drivers/clk/imx/clk-imx8mm.c       |  7 ++++---
>  drivers/clk/imx/clk-imx8mn.c       |  7 ++++---
>  drivers/clk/imx/clk-imx8mq.c       |  7 ++++---
>  drivers/clk/imx/clk.h              | 16 ++++++++++++++--
>  5 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index 2c309e3dc8e3..04e728538cef 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -216,7 +216,8 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>  		div->width = PCG_PREDIV_WIDTH;
>  		divider_ops = &imx8m_clk_composite_divider_ops;
>  		mux_ops = &clk_mux_ops;
> -		flags |= CLK_SET_PARENT_GATE;
> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> +			flags |= CLK_SET_PARENT_GATE;
>  	}
>  
>  	div->lock = &imx_ccm_lock;
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index f1919fafb124..73cc9805ec59 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -470,10 +470,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> +	 * as div value should always be read from hardware
>  	 */
> -	hws[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> -	hws[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> +	hws[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
> +	hws[IMX8MM_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
>  
>  	/* IP */
>  	hws[IMX8MM_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mm_vpu_g1_sels, base + 0xa100);
> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> index 88f6630cd472..0a76f969b28b 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -453,10 +453,11 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> +	 * as div value should always be read from hardware
>  	 */
> -	hws[IMX8MN_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> -	hws[IMX8MN_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mn_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> +	hws[IMX8MN_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000);
> +	hws[IMX8MN_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mn_dram_apb_sels, base + 0xa080);
>  
>  	hws[IMX8MN_CLK_DISP_PIXEL] = imx8m_clk_hw_composite("disp_pixel", imx8mn_disp_pixel_sels, base + 0xa500);
>  	hws[IMX8MN_CLK_SAI2] = imx8m_clk_hw_composite("sai2", imx8mn_sai2_sels, base + 0xa600);
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index c491bc9c61ce..83cc2b1c3294 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -449,11 +449,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> +	 * as div value should always be read from hardware
>  	 */
>  	hws[IMX8MQ_CLK_DRAM_CORE] = imx_clk_hw_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), CLK_IS_CRITICAL);
> -	hws[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> -	hws[IMX8MQ_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mq_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> +	hws[IMX8MQ_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000);
> +	hws[IMX8MQ_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mq_dram_apb_sels, base + 0xa080);
>  
>  	/* IP */
>  	hws[IMX8MQ_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mq_vpu_g1_sels, base + 0xa100);
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index 7571603bee23..e144f983fd8c 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -530,8 +530,9 @@ struct clk_hw *imx_clk_hw_cpu(const char *name, const char *parent_name,
>  		struct clk *div, struct clk *mux, struct clk *pll,
>  		struct clk *step);
>  
> -#define IMX_COMPOSITE_CORE	BIT(0)
> -#define IMX_COMPOSITE_BUS	BIT(1)
> +#define IMX_COMPOSITE_CORE		BIT(0)
> +#define IMX_COMPOSITE_BUS		BIT(1)
> +#define IMX_COMPOSITE_FW_MANAGED	BIT(2)
>  
>  struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>  					    const char * const *parent_names,
> @@ -567,6 +568,17 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>  		ARRAY_SIZE(parent_names), reg, 0, \
>  		flags | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
>  
> +#define __imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, flags) \
> +	imx8m_clk_hw_composite_flags(name, parent_names, \
> +		ARRAY_SIZE(parent_names), reg, IMX_COMPOSITE_FW_MANAGED, \
> +		flags | CLK_GET_RATE_NOCACHE | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
> +
> +#define imx8m_clk_hw_fw_managed_composite(name, parent_names, reg) \
> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, 0)
> +
> +#define imx8m_clk_hw_fw_managed_composite_critical(name, parent_names, reg) \
> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, CLK_IS_CRITICAL)
> +
>  #define __imx8m_clk_composite(name, parent_names, reg, flags) \
>  	to_clk(__imx8m_clk_hw_composite(name, parent_names, reg, flags))
>  
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks
  2021-08-18 15:14 ` Abel Vesa
@ 2021-08-18 15:28   ` Ahmad Fatoum
  2021-08-18 15:39     ` Abel Vesa
  0 siblings, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2021-08-18 15:28 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Peng Fan,
	Leonard Crestez, linux-arm-kernel, linux-kernel, linux-clk

Hello Abel,

On 18.08.21 17:14, Abel Vesa wrote:
> On 21-08-10 17:14:33, Ahmad Fatoum wrote:
>> On the i.MX8M*, the TF-A exposes a SiP (Silicon Provider) service
>> for DDR frequency scaling. The imx8m-ddrc-devfreq driver calls the
>> SiP and then does clk_set_parent on the DDR muxes to synchronize
>> the clock tree.
>>
>> Since 936c383673b9 ("clk: imx: fix composite peripheral flags"),
>> these TF-A managed muxes have SET_PARENT_GATE set, which results
>> in imx8m-ddrc-devfreq's clk_set_parent after SiP failing with -EBUSY:
>>
>> 	echo 25000000 > userspace/set_freq
>> 	imx8m-ddrc-devfreq 3d400000.memory-controller: failed to set
>> 		dram_apb parent: -16
>>
>> Fix this by adding a new i.MX composite flag for firmware managed
>> clocks, which clears SET_PARENT_GATE.
>>
>> This is safe to do, because updating the Linux clock tree to reflect
>> reality will always be glitch-free.
>>
>> Fixes: 936c383673b9 ("clk: imx: fix composite peripheral flags")
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> I'm OK with this.
> 
> Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

Thanks for the review.

> BTW, let me know if you are interested in the interconnect+devfreq
> (the replacement for NXP's internal busfreq). I have a new version of
> that patchset which I intend to send soon for review.

Ye, add me to CC and I'll give it at least a test. It sounds like you
recently worked with devfreq? I am wondering why you didn't run into this issue.

Cheers,
Ahmad

> 
>> ---
>> To: Abel Vesa <abel.vesa@nxp.com>
>> To: Michael Turquette <mturquette@baylibre.com>
>> To: Stephen Boyd <sboyd@kernel.org>
>> To: Shawn Guo <shawnguo@kernel.org>
>> To: Sascha Hauer <s.hauer@pengutronix.de>
>> To: Pengutronix Kernel Team <kernel@pengutronix.de>
>> To: Fabio Estevam <festevam@gmail.com>
>> To: NXP Linux Team <linux-imx@nxp.com>
>> To: Peng Fan <peng.fan@nxp.com>
>> To: Leonard Crestez <leonard.crestez@nxp.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-clk@vger.kernel.org
>> ---
>>  drivers/clk/imx/clk-composite-8m.c |  3 ++-
>>  drivers/clk/imx/clk-imx8mm.c       |  7 ++++---
>>  drivers/clk/imx/clk-imx8mn.c       |  7 ++++---
>>  drivers/clk/imx/clk-imx8mq.c       |  7 ++++---
>>  drivers/clk/imx/clk.h              | 16 ++++++++++++++--
>>  5 files changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
>> index 2c309e3dc8e3..04e728538cef 100644
>> --- a/drivers/clk/imx/clk-composite-8m.c
>> +++ b/drivers/clk/imx/clk-composite-8m.c
>> @@ -216,7 +216,8 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>>  		div->width = PCG_PREDIV_WIDTH;
>>  		divider_ops = &imx8m_clk_composite_divider_ops;
>>  		mux_ops = &clk_mux_ops;
>> -		flags |= CLK_SET_PARENT_GATE;
>> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
>> +			flags |= CLK_SET_PARENT_GATE;
>>  	}
>>  
>>  	div->lock = &imx_ccm_lock;
>> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
>> index f1919fafb124..73cc9805ec59 100644
>> --- a/drivers/clk/imx/clk-imx8mm.c
>> +++ b/drivers/clk/imx/clk-imx8mm.c
>> @@ -470,10 +470,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>>  
>>  	/*
>>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
>> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
>> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
>> +	 * as div value should always be read from hardware
>>  	 */
>> -	hws[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
>> -	hws[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
>> +	hws[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
>> +	hws[IMX8MM_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
>>  
>>  	/* IP */
>>  	hws[IMX8MM_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mm_vpu_g1_sels, base + 0xa100);
>> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
>> index 88f6630cd472..0a76f969b28b 100644
>> --- a/drivers/clk/imx/clk-imx8mn.c
>> +++ b/drivers/clk/imx/clk-imx8mn.c
>> @@ -453,10 +453,11 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
>>  
>>  	/*
>>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
>> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
>> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
>> +	 * as div value should always be read from hardware
>>  	 */
>> -	hws[IMX8MN_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
>> -	hws[IMX8MN_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mn_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
>> +	hws[IMX8MN_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000);
>> +	hws[IMX8MN_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mn_dram_apb_sels, base + 0xa080);
>>  
>>  	hws[IMX8MN_CLK_DISP_PIXEL] = imx8m_clk_hw_composite("disp_pixel", imx8mn_disp_pixel_sels, base + 0xa500);
>>  	hws[IMX8MN_CLK_SAI2] = imx8m_clk_hw_composite("sai2", imx8mn_sai2_sels, base + 0xa600);
>> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
>> index c491bc9c61ce..83cc2b1c3294 100644
>> --- a/drivers/clk/imx/clk-imx8mq.c
>> +++ b/drivers/clk/imx/clk-imx8mq.c
>> @@ -449,11 +449,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>>  
>>  	/*
>>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
>> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
>> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
>> +	 * as div value should always be read from hardware
>>  	 */
>>  	hws[IMX8MQ_CLK_DRAM_CORE] = imx_clk_hw_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), CLK_IS_CRITICAL);
>> -	hws[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
>> -	hws[IMX8MQ_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mq_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
>> +	hws[IMX8MQ_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000);
>> +	hws[IMX8MQ_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mq_dram_apb_sels, base + 0xa080);
>>  
>>  	/* IP */
>>  	hws[IMX8MQ_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mq_vpu_g1_sels, base + 0xa100);
>> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
>> index 7571603bee23..e144f983fd8c 100644
>> --- a/drivers/clk/imx/clk.h
>> +++ b/drivers/clk/imx/clk.h
>> @@ -530,8 +530,9 @@ struct clk_hw *imx_clk_hw_cpu(const char *name, const char *parent_name,
>>  		struct clk *div, struct clk *mux, struct clk *pll,
>>  		struct clk *step);
>>  
>> -#define IMX_COMPOSITE_CORE	BIT(0)
>> -#define IMX_COMPOSITE_BUS	BIT(1)
>> +#define IMX_COMPOSITE_CORE		BIT(0)
>> +#define IMX_COMPOSITE_BUS		BIT(1)
>> +#define IMX_COMPOSITE_FW_MANAGED	BIT(2)
>>  
>>  struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>>  					    const char * const *parent_names,
>> @@ -567,6 +568,17 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>>  		ARRAY_SIZE(parent_names), reg, 0, \
>>  		flags | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
>>  
>> +#define __imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, flags) \
>> +	imx8m_clk_hw_composite_flags(name, parent_names, \
>> +		ARRAY_SIZE(parent_names), reg, IMX_COMPOSITE_FW_MANAGED, \
>> +		flags | CLK_GET_RATE_NOCACHE | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
>> +
>> +#define imx8m_clk_hw_fw_managed_composite(name, parent_names, reg) \
>> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, 0)
>> +
>> +#define imx8m_clk_hw_fw_managed_composite_critical(name, parent_names, reg) \
>> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, CLK_IS_CRITICAL)
>> +
>>  #define __imx8m_clk_composite(name, parent_names, reg, flags) \
>>  	to_clk(__imx8m_clk_hw_composite(name, parent_names, reg, flags))
>>  
>> -- 
>> 2.30.2
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks
  2021-08-18 15:28   ` Ahmad Fatoum
@ 2021-08-18 15:39     ` Abel Vesa
  0 siblings, 0 replies; 5+ messages in thread
From: Abel Vesa @ 2021-08-18 15:39 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Peng Fan,
	Leonard Crestez, linux-arm-kernel, linux-kernel, linux-clk

On 21-08-18 17:28:42, Ahmad Fatoum wrote:
> Hello Abel,
> 
> On 18.08.21 17:14, Abel Vesa wrote:
> > On 21-08-10 17:14:33, Ahmad Fatoum wrote:
> >> On the i.MX8M*, the TF-A exposes a SiP (Silicon Provider) service
> >> for DDR frequency scaling. The imx8m-ddrc-devfreq driver calls the
> >> SiP and then does clk_set_parent on the DDR muxes to synchronize
> >> the clock tree.
> >>
> >> Since 936c383673b9 ("clk: imx: fix composite peripheral flags"),
> >> these TF-A managed muxes have SET_PARENT_GATE set, which results
> >> in imx8m-ddrc-devfreq's clk_set_parent after SiP failing with -EBUSY:
> >>
> >> 	echo 25000000 > userspace/set_freq
> >> 	imx8m-ddrc-devfreq 3d400000.memory-controller: failed to set
> >> 		dram_apb parent: -16
> >>
> >> Fix this by adding a new i.MX composite flag for firmware managed
> >> clocks, which clears SET_PARENT_GATE.
> >>
> >> This is safe to do, because updating the Linux clock tree to reflect
> >> reality will always be glitch-free.
> >>
> >> Fixes: 936c383673b9 ("clk: imx: fix composite peripheral flags")
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > 
> > I'm OK with this.
> > 
> > Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
> 
> Thanks for the review.
> 
> > BTW, let me know if you are interested in the interconnect+devfreq
> > (the replacement for NXP's internal busfreq). I have a new version of
> > that patchset which I intend to send soon for review.
> 
> Ye, add me to CC and I'll give it at least a test. It sounds like you
> recently worked with devfreq? I am wondering why you didn't run into this issue.

I actually had a different fix for this issue actually. Have a look here:

https://git.kernel.org/pub/scm/linux/kernel/git/abelvesa/linux.git/commit/?h=imx8mq/icc_devfreq&id=bd39ccabdfdbf128670dea56094b0da7764be05a

And my fix will still be needed when the icc+devfreq gets in, since the
parent will be changed in TF-A but the kernel clock tree can't because
the clock is marked as CLK_IS_CRITICAL.

> 
> Cheers,
> Ahmad
> 
> > 
> >> ---
> >> To: Abel Vesa <abel.vesa@nxp.com>
> >> To: Michael Turquette <mturquette@baylibre.com>
> >> To: Stephen Boyd <sboyd@kernel.org>
> >> To: Shawn Guo <shawnguo@kernel.org>
> >> To: Sascha Hauer <s.hauer@pengutronix.de>
> >> To: Pengutronix Kernel Team <kernel@pengutronix.de>
> >> To: Fabio Estevam <festevam@gmail.com>
> >> To: NXP Linux Team <linux-imx@nxp.com>
> >> To: Peng Fan <peng.fan@nxp.com>
> >> To: Leonard Crestez <leonard.crestez@nxp.com>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-clk@vger.kernel.org
> >> ---
> >>  drivers/clk/imx/clk-composite-8m.c |  3 ++-
> >>  drivers/clk/imx/clk-imx8mm.c       |  7 ++++---
> >>  drivers/clk/imx/clk-imx8mn.c       |  7 ++++---
> >>  drivers/clk/imx/clk-imx8mq.c       |  7 ++++---
> >>  drivers/clk/imx/clk.h              | 16 ++++++++++++++--
> >>  5 files changed, 28 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> >> index 2c309e3dc8e3..04e728538cef 100644
> >> --- a/drivers/clk/imx/clk-composite-8m.c
> >> +++ b/drivers/clk/imx/clk-composite-8m.c
> >> @@ -216,7 +216,8 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
> >>  		div->width = PCG_PREDIV_WIDTH;
> >>  		divider_ops = &imx8m_clk_composite_divider_ops;
> >>  		mux_ops = &clk_mux_ops;
> >> -		flags |= CLK_SET_PARENT_GATE;
> >> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> >> +			flags |= CLK_SET_PARENT_GATE;
> >>  	}
> >>  
> >>  	div->lock = &imx_ccm_lock;
> >> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> >> index f1919fafb124..73cc9805ec59 100644
> >> --- a/drivers/clk/imx/clk-imx8mm.c
> >> +++ b/drivers/clk/imx/clk-imx8mm.c
> >> @@ -470,10 +470,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
> >>  
> >>  	/*
> >>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> >> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> >> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> >> +	 * as div value should always be read from hardware
> >>  	 */
> >> -	hws[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> >> -	hws[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> >> +	hws[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
> >> +	hws[IMX8MM_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
> >>  
> >>  	/* IP */
> >>  	hws[IMX8MM_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mm_vpu_g1_sels, base + 0xa100);
> >> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> >> index 88f6630cd472..0a76f969b28b 100644
> >> --- a/drivers/clk/imx/clk-imx8mn.c
> >> +++ b/drivers/clk/imx/clk-imx8mn.c
> >> @@ -453,10 +453,11 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
> >>  
> >>  	/*
> >>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> >> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> >> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> >> +	 * as div value should always be read from hardware
> >>  	 */
> >> -	hws[IMX8MN_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> >> -	hws[IMX8MN_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mn_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> >> +	hws[IMX8MN_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000);
> >> +	hws[IMX8MN_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mn_dram_apb_sels, base + 0xa080);
> >>  
> >>  	hws[IMX8MN_CLK_DISP_PIXEL] = imx8m_clk_hw_composite("disp_pixel", imx8mn_disp_pixel_sels, base + 0xa500);
> >>  	hws[IMX8MN_CLK_SAI2] = imx8m_clk_hw_composite("sai2", imx8mn_sai2_sels, base + 0xa600);
> >> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> >> index c491bc9c61ce..83cc2b1c3294 100644
> >> --- a/drivers/clk/imx/clk-imx8mq.c
> >> +++ b/drivers/clk/imx/clk-imx8mq.c
> >> @@ -449,11 +449,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> >>  
> >>  	/*
> >>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> >> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> >> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> >> +	 * as div value should always be read from hardware
> >>  	 */
> >>  	hws[IMX8MQ_CLK_DRAM_CORE] = imx_clk_hw_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), CLK_IS_CRITICAL);
> >> -	hws[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> >> -	hws[IMX8MQ_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mq_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> >> +	hws[IMX8MQ_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000);
> >> +	hws[IMX8MQ_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mq_dram_apb_sels, base + 0xa080);
> >>  
> >>  	/* IP */
> >>  	hws[IMX8MQ_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mq_vpu_g1_sels, base + 0xa100);
> >> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> >> index 7571603bee23..e144f983fd8c 100644
> >> --- a/drivers/clk/imx/clk.h
> >> +++ b/drivers/clk/imx/clk.h
> >> @@ -530,8 +530,9 @@ struct clk_hw *imx_clk_hw_cpu(const char *name, const char *parent_name,
> >>  		struct clk *div, struct clk *mux, struct clk *pll,
> >>  		struct clk *step);
> >>  
> >> -#define IMX_COMPOSITE_CORE	BIT(0)
> >> -#define IMX_COMPOSITE_BUS	BIT(1)
> >> +#define IMX_COMPOSITE_CORE		BIT(0)
> >> +#define IMX_COMPOSITE_BUS		BIT(1)
> >> +#define IMX_COMPOSITE_FW_MANAGED	BIT(2)
> >>  
> >>  struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
> >>  					    const char * const *parent_names,
> >> @@ -567,6 +568,17 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
> >>  		ARRAY_SIZE(parent_names), reg, 0, \
> >>  		flags | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
> >>  
> >> +#define __imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, flags) \
> >> +	imx8m_clk_hw_composite_flags(name, parent_names, \
> >> +		ARRAY_SIZE(parent_names), reg, IMX_COMPOSITE_FW_MANAGED, \
> >> +		flags | CLK_GET_RATE_NOCACHE | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
> >> +
> >> +#define imx8m_clk_hw_fw_managed_composite(name, parent_names, reg) \
> >> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, 0)
> >> +
> >> +#define imx8m_clk_hw_fw_managed_composite_critical(name, parent_names, reg) \
> >> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, CLK_IS_CRITICAL)
> >> +
> >>  #define __imx8m_clk_composite(name, parent_names, reg, flags) \
> >>  	to_clk(__imx8m_clk_hw_composite(name, parent_names, reg, flags))
> >>  
> >> -- 
> >> 2.30.2
> >>
> > 
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Cb8c21d0576b0482b181608d9625cdf49%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637648973293620984%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=D0efPtr5bHllXtgau1QWgMrzY8p2Zckeo43pvUztDPE%3D&amp;reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks
  2021-08-10 15:14 [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks Ahmad Fatoum
  2021-08-18 15:14 ` Abel Vesa
@ 2021-08-24 12:15 ` Abel Vesa
  1 sibling, 0 replies; 5+ messages in thread
From: Abel Vesa @ 2021-08-24 12:15 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Peng Fan,
	Leonard Crestez, linux-arm-kernel, linux-kernel, linux-clk

On 21-08-10 17:14:33, Ahmad Fatoum wrote:
> On the i.MX8M*, the TF-A exposes a SiP (Silicon Provider) service
> for DDR frequency scaling. The imx8m-ddrc-devfreq driver calls the
> SiP and then does clk_set_parent on the DDR muxes to synchronize
> the clock tree.
> 
> Since 936c383673b9 ("clk: imx: fix composite peripheral flags"),
> these TF-A managed muxes have SET_PARENT_GATE set, which results
> in imx8m-ddrc-devfreq's clk_set_parent after SiP failing with -EBUSY:
> 
> 	echo 25000000 > userspace/set_freq
> 	imx8m-ddrc-devfreq 3d400000.memory-controller: failed to set
> 		dram_apb parent: -16
> 
> Fix this by adding a new i.MX composite flag for firmware managed
> clocks, which clears SET_PARENT_GATE.
> 
> This is safe to do, because updating the Linux clock tree to reflect
> reality will always be glitch-free.
> 
> Fixes: 936c383673b9 ("clk: imx: fix composite peripheral flags")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>


Applied, thanks.

> ---
> To: Abel Vesa <abel.vesa@nxp.com>
> To: Michael Turquette <mturquette@baylibre.com>
> To: Stephen Boyd <sboyd@kernel.org>
> To: Shawn Guo <shawnguo@kernel.org>
> To: Sascha Hauer <s.hauer@pengutronix.de>
> To: Pengutronix Kernel Team <kernel@pengutronix.de>
> To: Fabio Estevam <festevam@gmail.com>
> To: NXP Linux Team <linux-imx@nxp.com>
> To: Peng Fan <peng.fan@nxp.com>
> To: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/clk/imx/clk-composite-8m.c |  3 ++-
>  drivers/clk/imx/clk-imx8mm.c       |  7 ++++---
>  drivers/clk/imx/clk-imx8mn.c       |  7 ++++---
>  drivers/clk/imx/clk-imx8mq.c       |  7 ++++---
>  drivers/clk/imx/clk.h              | 16 ++++++++++++++--
>  5 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index 2c309e3dc8e3..04e728538cef 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -216,7 +216,8 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>  		div->width = PCG_PREDIV_WIDTH;
>  		divider_ops = &imx8m_clk_composite_divider_ops;
>  		mux_ops = &clk_mux_ops;
> -		flags |= CLK_SET_PARENT_GATE;
> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> +			flags |= CLK_SET_PARENT_GATE;
>  	}
>  
>  	div->lock = &imx_ccm_lock;
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index f1919fafb124..73cc9805ec59 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -470,10 +470,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> +	 * as div value should always be read from hardware
>  	 */
> -	hws[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> -	hws[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> +	hws[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
> +	hws[IMX8MM_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
>  
>  	/* IP */
>  	hws[IMX8MM_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mm_vpu_g1_sels, base + 0xa100);
> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> index 88f6630cd472..0a76f969b28b 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -453,10 +453,11 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> +	 * as div value should always be read from hardware
>  	 */
> -	hws[IMX8MN_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> -	hws[IMX8MN_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mn_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> +	hws[IMX8MN_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mn_dram_alt_sels, base + 0xa000);
> +	hws[IMX8MN_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mn_dram_apb_sels, base + 0xa080);
>  
>  	hws[IMX8MN_CLK_DISP_PIXEL] = imx8m_clk_hw_composite("disp_pixel", imx8mn_disp_pixel_sels, base + 0xa500);
>  	hws[IMX8MN_CLK_SAI2] = imx8m_clk_hw_composite("sai2", imx8mn_sai2_sels, base + 0xa600);
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index c491bc9c61ce..83cc2b1c3294 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -449,11 +449,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * DRAM clocks are manipulated from TF-A outside clock framework.
> -	 * Mark with GET_RATE_NOCACHE to always read div value from hardware
> +	 * The fw_managed helper sets GET_RATE_NOCACHE and clears SET_PARENT_GATE
> +	 * as div value should always be read from hardware
>  	 */
>  	hws[IMX8MQ_CLK_DRAM_CORE] = imx_clk_hw_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mq_dram_core_sels, ARRAY_SIZE(imx8mq_dram_core_sels), CLK_IS_CRITICAL);
> -	hws[IMX8MQ_CLK_DRAM_ALT] = __imx8m_clk_hw_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000, CLK_GET_RATE_NOCACHE);
> -	hws[IMX8MQ_CLK_DRAM_APB] = __imx8m_clk_hw_composite("dram_apb", imx8mq_dram_apb_sels, base + 0xa080, CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> +	hws[IMX8MQ_CLK_DRAM_ALT] = imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mq_dram_alt_sels, base + 0xa000);
> +	hws[IMX8MQ_CLK_DRAM_APB] = imx8m_clk_hw_fw_managed_composite_critical("dram_apb", imx8mq_dram_apb_sels, base + 0xa080);
>  
>  	/* IP */
>  	hws[IMX8MQ_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1", imx8mq_vpu_g1_sels, base + 0xa100);
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index 7571603bee23..e144f983fd8c 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -530,8 +530,9 @@ struct clk_hw *imx_clk_hw_cpu(const char *name, const char *parent_name,
>  		struct clk *div, struct clk *mux, struct clk *pll,
>  		struct clk *step);
>  
> -#define IMX_COMPOSITE_CORE	BIT(0)
> -#define IMX_COMPOSITE_BUS	BIT(1)
> +#define IMX_COMPOSITE_CORE		BIT(0)
> +#define IMX_COMPOSITE_BUS		BIT(1)
> +#define IMX_COMPOSITE_FW_MANAGED	BIT(2)
>  
>  struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>  					    const char * const *parent_names,
> @@ -567,6 +568,17 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>  		ARRAY_SIZE(parent_names), reg, 0, \
>  		flags | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
>  
> +#define __imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, flags) \
> +	imx8m_clk_hw_composite_flags(name, parent_names, \
> +		ARRAY_SIZE(parent_names), reg, IMX_COMPOSITE_FW_MANAGED, \
> +		flags | CLK_GET_RATE_NOCACHE | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
> +
> +#define imx8m_clk_hw_fw_managed_composite(name, parent_names, reg) \
> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, 0)
> +
> +#define imx8m_clk_hw_fw_managed_composite_critical(name, parent_names, reg) \
> +	__imx8m_clk_hw_fw_managed_composite(name, parent_names, reg, CLK_IS_CRITICAL)
> +
>  #define __imx8m_clk_composite(name, parent_names, reg, flags) \
>  	to_clk(__imx8m_clk_hw_composite(name, parent_names, reg, flags))
>  
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-24 12:15 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 15:14 [PATCH] clk: imx8m: fix clock tree update of TF-A managed clocks Ahmad Fatoum
2021-08-18 15:14 ` Abel Vesa
2021-08-18 15:28   ` Ahmad Fatoum
2021-08-18 15:39     ` Abel Vesa
2021-08-24 12:15 ` Abel Vesa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).