| Message ID | 20210507171028.157728-1-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: imx8mq: prevent sys1_pll_266m gating | expand |
> 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
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 >
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 >
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);
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. --- drivers/clk/imx/clk-imx8mq.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)