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

From: Jacky Bai <ping.bai@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>, Shawn Guo <shawnguo@kernel.org>
Cc: Abel Vesa <abel.vesa@nxp.com>, dl-linux-imx <linux-imx@nxp.com>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"patchwork-lst@pengutronix.de" <patchwork-lst@pengutronix.de>
Subject: RE: [PATCH] clk: imx8mq: prevent sys1_pll_266m gating
Date: Sat, 8 May 2021 00:45:36 +0000	[thread overview]
Message-ID: <DBBPR04MB7930F380B82AB65B81EACD9887569@DBBPR04MB7930.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20210507171028.157728-1-l.stach@pengutronix.de>

> 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