* RE: [PATCH] clk: imx8mq: prevent sys1_pll_266m gating
2021-05-07 17:10 [PATCH] clk: imx8mq: prevent sys1_pll_266m gating Lucas Stach
@ 2021-05-08 0:45 ` Jacky Bai
2021-05-10 8:54 ` Abel Vesa
2021-05-17 10:04 ` Abel Vesa
2 siblings, 0 replies; 4+ messages in thread
From: Jacky Bai @ 2021-05-08 0:45 UTC (permalink / raw)
To: Lucas Stach, Shawn Guo
Cc: Abel Vesa, dl-linux-imx, linux-clk, linux-arm-kernel, kernel,
patchwork-lst
> Subject: [PATCH] clk: imx8mq: prevent sys1_pll_266m gating
>
> Gating sys1_pll_266m while the usdhc_nand_bus clock is still active (due to
> being enabled in to bootloader) leads to spurious failures of the uSDHC
> module.
>
> b04383b6a558 ("clk: imx8mq: Define gates for pll1/2 fixed dividers")
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> We probably need some solution to keep parent clocks active on the i.MX8M
> clock architecture, as long as any consumers are active, as the reference
> manual states that disabling a parent clock may lead to undefined behavior.
No such limitation, should be Doc issue.
> This needs more work in the clock framework and/or driver. This patch fixes
> the obvious regression until we have such a solution.
Personally, I prefer to reverted the whole patch b04383b6a558 ("clk: imx8mq: Define gates for pll1/2 fixed dividers").
As instancing those gates increasing the complexity of AMP system clock management, no other obvious benefit.
BR
Jacky Bai
> ---
> drivers/clk/imx/clk-imx8mq.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index 4dd4ae9d022b..fce983add1fc 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -372,7 +372,6 @@ static int imx8mq_clocks_probe(struct
> platform_device *pdev)
> hws[IMX8MQ_SYS1_PLL_133M_CG] =
> imx_clk_hw_gate("sys1_pll_133m_cg", "sys1_pll_out", base + 0x30, 15);
> hws[IMX8MQ_SYS1_PLL_160M_CG] =
> imx_clk_hw_gate("sys1_pll_160m_cg", "sys1_pll_out", base + 0x30, 17);
> hws[IMX8MQ_SYS1_PLL_200M_CG] =
> imx_clk_hw_gate("sys1_pll_200m_cg", "sys1_pll_out", base + 0x30, 19);
> - hws[IMX8MQ_SYS1_PLL_266M_CG] =
> imx_clk_hw_gate("sys1_pll_266m_cg", "sys1_pll_out", base + 0x30, 21);
> hws[IMX8MQ_SYS1_PLL_400M_CG] =
> imx_clk_hw_gate("sys1_pll_400m_cg", "sys1_pll_out", base + 0x30, 23);
> hws[IMX8MQ_SYS1_PLL_800M_CG] =
> imx_clk_hw_gate("sys1_pll_800m_cg", "sys1_pll_out", base + 0x30, 25);
>
> @@ -382,7 +381,7 @@ static int imx8mq_clocks_probe(struct
> platform_device *pdev)
> hws[IMX8MQ_SYS1_PLL_133M] =
> imx_clk_hw_fixed_factor("sys1_pll_133m", "sys1_pll_133m_cg", 1, 6);
> hws[IMX8MQ_SYS1_PLL_160M] =
> imx_clk_hw_fixed_factor("sys1_pll_160m", "sys1_pll_160m_cg", 1, 5);
> hws[IMX8MQ_SYS1_PLL_200M] =
> imx_clk_hw_fixed_factor("sys1_pll_200m", "sys1_pll_200m_cg", 1, 4);
> - hws[IMX8MQ_SYS1_PLL_266M] =
> imx_clk_hw_fixed_factor("sys1_pll_266m", "sys1_pll_266m_cg", 1, 3);
> + hws[IMX8MQ_SYS1_PLL_266M] =
> imx_clk_hw_fixed_factor("sys1_pll_266m",
> +"sys1_pll_out", 1, 3);
> hws[IMX8MQ_SYS1_PLL_400M] =
> imx_clk_hw_fixed_factor("sys1_pll_400m", "sys1_pll_400m_cg", 1, 2);
> hws[IMX8MQ_SYS1_PLL_800M] =
> imx_clk_hw_fixed_factor("sys1_pll_800m", "sys1_pll_800m_cg", 1, 1);
>
> --
> 2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] clk: imx8mq: prevent sys1_pll_266m gating
2021-05-07 17:10 [PATCH] clk: imx8mq: prevent sys1_pll_266m gating Lucas Stach
2021-05-08 0:45 ` Jacky Bai
@ 2021-05-10 8:54 ` Abel Vesa
2021-05-17 10:04 ` Abel Vesa
2 siblings, 0 replies; 4+ messages in thread
From: Abel Vesa @ 2021-05-10 8:54 UTC (permalink / raw)
To: Lucas Stach
Cc: Shawn Guo, NXP Linux Team, linux-clk, linux-arm-kernel, kernel,
patchwork-lst
On 21-05-07 19:10:28, Lucas Stach wrote:
> Gating sys1_pll_266m while the usdhc_nand_bus clock is still active (due
> to being enabled in to bootloader) leads to spurious failures of the
> uSDHC module.
>
> b04383b6a558 ("clk: imx8mq: Define gates for pll1/2 fixed dividers")
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> We probably need some solution to keep parent clocks active on the i.MX8M clock
> architecture, as long as any consumers are active, as the reference manual
> states that disabling a parent clock may lead to undefined behavior. This needs
> more work in the clock framework and/or driver. This patch fixes the obvious
> regression until we have such a solution.
I believe the solution here should be a core clock flag that forces
the core to read the gate bit value from register. The problem with this approach
is the clock core doesn't check if the child clocks are enabled when
disabling the parent clock, instead it relies on cached enabled count of the parent.
Basically, the CCF, as of now, isn't able to cope with clocks that can be
enabled/disabled from outside. This should be improved at some point.
> ---
> drivers/clk/imx/clk-imx8mq.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index 4dd4ae9d022b..fce983add1fc 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -372,7 +372,6 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> hws[IMX8MQ_SYS1_PLL_133M_CG] = imx_clk_hw_gate("sys1_pll_133m_cg", "sys1_pll_out", base + 0x30, 15);
> hws[IMX8MQ_SYS1_PLL_160M_CG] = imx_clk_hw_gate("sys1_pll_160m_cg", "sys1_pll_out", base + 0x30, 17);
> hws[IMX8MQ_SYS1_PLL_200M_CG] = imx_clk_hw_gate("sys1_pll_200m_cg", "sys1_pll_out", base + 0x30, 19);
> - hws[IMX8MQ_SYS1_PLL_266M_CG] = imx_clk_hw_gate("sys1_pll_266m_cg", "sys1_pll_out", base + 0x30, 21);
> hws[IMX8MQ_SYS1_PLL_400M_CG] = imx_clk_hw_gate("sys1_pll_400m_cg", "sys1_pll_out", base + 0x30, 23);
> hws[IMX8MQ_SYS1_PLL_800M_CG] = imx_clk_hw_gate("sys1_pll_800m_cg", "sys1_pll_out", base + 0x30, 25);
>
> @@ -382,7 +381,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> hws[IMX8MQ_SYS1_PLL_133M] = imx_clk_hw_fixed_factor("sys1_pll_133m", "sys1_pll_133m_cg", 1, 6);
> hws[IMX8MQ_SYS1_PLL_160M] = imx_clk_hw_fixed_factor("sys1_pll_160m", "sys1_pll_160m_cg", 1, 5);
> hws[IMX8MQ_SYS1_PLL_200M] = imx_clk_hw_fixed_factor("sys1_pll_200m", "sys1_pll_200m_cg", 1, 4);
> - hws[IMX8MQ_SYS1_PLL_266M] = imx_clk_hw_fixed_factor("sys1_pll_266m", "sys1_pll_266m_cg", 1, 3);
> + hws[IMX8MQ_SYS1_PLL_266M] = imx_clk_hw_fixed_factor("sys1_pll_266m", "sys1_pll_out", 1, 3);
> hws[IMX8MQ_SYS1_PLL_400M] = imx_clk_hw_fixed_factor("sys1_pll_400m", "sys1_pll_400m_cg", 1, 2);
> hws[IMX8MQ_SYS1_PLL_800M] = imx_clk_hw_fixed_factor("sys1_pll_800m", "sys1_pll_800m_cg", 1, 1);
>
> --
> 2.29.2
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] clk: imx8mq: prevent sys1_pll_266m gating
2021-05-07 17:10 [PATCH] clk: imx8mq: prevent sys1_pll_266m gating Lucas Stach
2021-05-08 0:45 ` Jacky Bai
2021-05-10 8:54 ` Abel Vesa
@ 2021-05-17 10:04 ` Abel Vesa
2 siblings, 0 replies; 4+ messages in thread
From: Abel Vesa @ 2021-05-17 10:04 UTC (permalink / raw)
To: Lucas Stach
Cc: Shawn Guo, NXP Linux Team, linux-clk, linux-arm-kernel, kernel,
patchwork-lst, Stephen Boyd
On 21-05-07 19:10:28, Lucas Stach wrote:
> Gating sys1_pll_266m while the usdhc_nand_bus clock is still active (due
> to being enabled in to bootloader) leads to spurious failures of the
> uSDHC module.
>
> b04383b6a558 ("clk: imx8mq: Define gates for pll1/2 fixed dividers")
Maybe this should have a Fixes tag then.
Then Stephen will pick it up himself.
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> We probably need some solution to keep parent clocks active on the i.MX8M clock
> architecture, as long as any consumers are active, as the reference manual
> states that disabling a parent clock may lead to undefined behavior. This needs
> more work in the clock framework and/or driver. This patch fixes the obvious
> regression until we have such a solution.
> ---
> drivers/clk/imx/clk-imx8mq.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index 4dd4ae9d022b..fce983add1fc 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -372,7 +372,6 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> hws[IMX8MQ_SYS1_PLL_133M_CG] = imx_clk_hw_gate("sys1_pll_133m_cg", "sys1_pll_out", base + 0x30, 15);
> hws[IMX8MQ_SYS1_PLL_160M_CG] = imx_clk_hw_gate("sys1_pll_160m_cg", "sys1_pll_out", base + 0x30, 17);
> hws[IMX8MQ_SYS1_PLL_200M_CG] = imx_clk_hw_gate("sys1_pll_200m_cg", "sys1_pll_out", base + 0x30, 19);
> - hws[IMX8MQ_SYS1_PLL_266M_CG] = imx_clk_hw_gate("sys1_pll_266m_cg", "sys1_pll_out", base + 0x30, 21);
> hws[IMX8MQ_SYS1_PLL_400M_CG] = imx_clk_hw_gate("sys1_pll_400m_cg", "sys1_pll_out", base + 0x30, 23);
> hws[IMX8MQ_SYS1_PLL_800M_CG] = imx_clk_hw_gate("sys1_pll_800m_cg", "sys1_pll_out", base + 0x30, 25);
>
> @@ -382,7 +381,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> hws[IMX8MQ_SYS1_PLL_133M] = imx_clk_hw_fixed_factor("sys1_pll_133m", "sys1_pll_133m_cg", 1, 6);
> hws[IMX8MQ_SYS1_PLL_160M] = imx_clk_hw_fixed_factor("sys1_pll_160m", "sys1_pll_160m_cg", 1, 5);
> hws[IMX8MQ_SYS1_PLL_200M] = imx_clk_hw_fixed_factor("sys1_pll_200m", "sys1_pll_200m_cg", 1, 4);
> - hws[IMX8MQ_SYS1_PLL_266M] = imx_clk_hw_fixed_factor("sys1_pll_266m", "sys1_pll_266m_cg", 1, 3);
> + hws[IMX8MQ_SYS1_PLL_266M] = imx_clk_hw_fixed_factor("sys1_pll_266m", "sys1_pll_out", 1, 3);
> hws[IMX8MQ_SYS1_PLL_400M] = imx_clk_hw_fixed_factor("sys1_pll_400m", "sys1_pll_400m_cg", 1, 2);
> hws[IMX8MQ_SYS1_PLL_800M] = imx_clk_hw_fixed_factor("sys1_pll_800m", "sys1_pll_800m_cg", 1, 1);
>
> --
> 2.29.2
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread