Re: [PATCH] clk: imx8mq: prevent sys1_pll_266m gating

From: Abel Vesa <abel.vesa@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	kernel@pengutronix.de, patchwork-lst@pengutronix.de
Subject: Re: [PATCH] clk: imx8mq: prevent sys1_pll_266m gating
Date: Mon, 10 May 2021 11:54:42 +0300	[thread overview]
Message-ID: <20210510085442.wn4vz2fgstym4rrj@fsr-ub1664-175> (raw)
In-Reply-To: <20210507171028.157728-1-l.stach@pengutronix.de>

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