linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH V2] clk: imx: gate off peripheral clock slice
  2021-10-25 12:29 [PATCH V2] clk: imx: gate off peripheral clock slice Peng Fan (OSS)
@ 2021-10-25 11:54 ` Ahmad Fatoum
  2021-10-25 11:59   ` Ahmad Fatoum
  2021-10-25 12:08 ` Abel Vesa
  1 sibling, 1 reply; 7+ messages in thread
From: Ahmad Fatoum @ 2021-10-25 11:54 UTC (permalink / raw)
  To: Peng Fan (OSS), sboyd, mturquette, abel.vesa, s.hauer
  Cc: Peng Fan, linux-kernel, linux-imx, kernel, festevam, linux-clk,
	linux-arm-kernel

On 25.10.21 14:29, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The Peripheral clocks are default enabled when SoC power on, and
> bootloader not gate off the clocks when booting Linux Kernel.
> 
> So Linux Kernel is not aware the peripheral clocks are enabled and
> still take them as disabled because of enable count is zero.
> 
> Then Peripheral clock's source without clock gated off could be
> changed when have assigned-parents in device tree
> 
> However, per i.MX8M* reference mannual, "Peripheral clock slices must
> be stopped to change the clock source", so need to gate off the
> the peripheral clock when registering the clocks to avoid glitch.
> 
> Tested boot on i.MX8MM/P-EVK board
> 
> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

I've been running an i.MX8MM-based system with this patch for a few days
so far and no apparent issues:

Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
> 
> V2:
>  Add Fixes tag
> 
>  drivers/clk/imx/clk-composite-8m.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index 2dfd6149e528..ee41fbf90589 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -184,6 +184,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>  	struct clk_mux *mux = NULL;
>  	const struct clk_ops *divider_ops;
>  	const struct clk_ops *mux_ops;
> +	u32 val;
>  
>  	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>  	if (!mux)
> @@ -216,8 +217,14 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>  		div->width = PCG_PREDIV_WIDTH;
>  		divider_ops = &imx8m_clk_composite_divider_ops;
>  		mux_ops = &clk_mux_ops;
> -		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) {
>  			flags |= CLK_SET_PARENT_GATE;
> +			if (!(flags & CLK_IS_CRITICAL)) {
> +				val = readl(reg);
> +				val &= ~BIT(PCG_CGC_SHIFT);
> +				writel(val, reg);
> +			}
> +		}
>  	}
>  
>  	div->lock = &imx_ccm_lock;
> 


-- 
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] 7+ messages in thread

* Re: [PATCH V2] clk: imx: gate off peripheral clock slice
  2021-10-25 11:54 ` Ahmad Fatoum
@ 2021-10-25 11:59   ` Ahmad Fatoum
  0 siblings, 0 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2021-10-25 11:59 UTC (permalink / raw)
  To: Peng Fan (OSS), sboyd, mturquette, abel.vesa, s.hauer
  Cc: Peng Fan, linux-kernel, linux-imx, kernel, festevam, linux-clk,
	linux-arm-kernel

On 25.10.21 13:54, Ahmad Fatoum wrote:
> On 25.10.21 14:29, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> The Peripheral clocks are default enabled when SoC power on, and
>> bootloader not gate off the clocks when booting Linux Kernel.
>>
>> So Linux Kernel is not aware the peripheral clocks are enabled and
>> still take them as disabled because of enable count is zero.
>>
>> Then Peripheral clock's source without clock gated off could be
>> changed when have assigned-parents in device tree
>>
>> However, per i.MX8M* reference mannual, "Peripheral clock slices must
>> be stopped to change the clock source", so need to gate off the
>> the peripheral clock when registering the clocks to avoid glitch.
>>
>> Tested boot on i.MX8MM/P-EVK board
>>
>> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> I've been running an i.MX8MM-based system with this patch for a few days
> so far and no apparent issues:
> 
> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

I see now that the duration of test isn't as important as the number
of boots (as this is a probe-time change). Still, I booted it a dozen
or so times and didn't notice anything different.

> 
>> ---
>>
>> V2:
>>  Add Fixes tag
>>
>>  drivers/clk/imx/clk-composite-8m.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
>> index 2dfd6149e528..ee41fbf90589 100644
>> --- a/drivers/clk/imx/clk-composite-8m.c
>> +++ b/drivers/clk/imx/clk-composite-8m.c
>> @@ -184,6 +184,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>>  	struct clk_mux *mux = NULL;
>>  	const struct clk_ops *divider_ops;
>>  	const struct clk_ops *mux_ops;
>> +	u32 val;
>>  
>>  	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>>  	if (!mux)
>> @@ -216,8 +217,14 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>>  		div->width = PCG_PREDIV_WIDTH;
>>  		divider_ops = &imx8m_clk_composite_divider_ops;
>>  		mux_ops = &clk_mux_ops;
>> -		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
>> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) {
>>  			flags |= CLK_SET_PARENT_GATE;
>> +			if (!(flags & CLK_IS_CRITICAL)) {
>> +				val = readl(reg);
>> +				val &= ~BIT(PCG_CGC_SHIFT);
>> +				writel(val, reg);
>> +			}
>> +		}
>>  	}
>>  
>>  	div->lock = &imx_ccm_lock;
>>
> 
> 


-- 
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] 7+ messages in thread

* Re: [PATCH V2] clk: imx: gate off peripheral clock slice
  2021-10-25 12:29 [PATCH V2] clk: imx: gate off peripheral clock slice Peng Fan (OSS)
  2021-10-25 11:54 ` Ahmad Fatoum
@ 2021-10-25 12:08 ` Abel Vesa
  2021-10-29 16:47   ` Ahmad Fatoum
  1 sibling, 1 reply; 7+ messages in thread
From: Abel Vesa @ 2021-10-25 12:08 UTC (permalink / raw)
  To: Peng Fan (OSS)
  Cc: sboyd, mturquette, s.hauer, kernel, festevam, linux-imx,
	linux-clk, linux-arm-kernel, linux-kernel, Peng Fan

On 21-10-25 20:29:02, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The Peripheral clocks are default enabled when SoC power on, and
> bootloader not gate off the clocks when booting Linux Kernel.
> 
> So Linux Kernel is not aware the peripheral clocks are enabled and
> still take them as disabled because of enable count is zero.
> 
> Then Peripheral clock's source without clock gated off could be
> changed when have assigned-parents in device tree
> 
> However, per i.MX8M* reference mannual, "Peripheral clock slices must
> be stopped to change the clock source", so need to gate off the
> the peripheral clock when registering the clocks to avoid glitch.
> 
> Tested boot on i.MX8MM/P-EVK board
> 
> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  Add Fixes tag
> 
>  drivers/clk/imx/clk-composite-8m.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index 2dfd6149e528..ee41fbf90589 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -184,6 +184,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>  	struct clk_mux *mux = NULL;
>  	const struct clk_ops *divider_ops;
>  	const struct clk_ops *mux_ops;
> +	u32 val;
>  
>  	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>  	if (!mux)
> @@ -216,8 +217,14 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>  		div->width = PCG_PREDIV_WIDTH;
>  		divider_ops = &imx8m_clk_composite_divider_ops;
>  		mux_ops = &clk_mux_ops;
> -		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) {
>  			flags |= CLK_SET_PARENT_GATE;
> +			if (!(flags & CLK_IS_CRITICAL)) {
> +				val = readl(reg);
> +				val &= ~BIT(PCG_CGC_SHIFT);
> +				writel(val, reg);
> +			}
> +		}

Though I'm usually against special cases like this one. I think the clock
core needs some generic flag that would read the state from HW on probe
and/or another generic flag for disabling on probe.

But for now, I'm OK with this:

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

>  	}
>  
>  	div->lock = &imx_ccm_lock;
> -- 
> 2.30.0
>

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

* [PATCH V2] clk: imx: gate off peripheral clock slice
@ 2021-10-25 12:29 Peng Fan (OSS)
  2021-10-25 11:54 ` Ahmad Fatoum
  2021-10-25 12:08 ` Abel Vesa
  0 siblings, 2 replies; 7+ messages in thread
From: Peng Fan (OSS) @ 2021-10-25 12:29 UTC (permalink / raw)
  To: sboyd, mturquette, abel.vesa, s.hauer
  Cc: kernel, festevam, linux-imx, linux-clk, linux-arm-kernel,
	linux-kernel, Peng Fan

From: Peng Fan <peng.fan@nxp.com>

The Peripheral clocks are default enabled when SoC power on, and
bootloader not gate off the clocks when booting Linux Kernel.

So Linux Kernel is not aware the peripheral clocks are enabled and
still take them as disabled because of enable count is zero.

Then Peripheral clock's source without clock gated off could be
changed when have assigned-parents in device tree

However, per i.MX8M* reference mannual, "Peripheral clock slices must
be stopped to change the clock source", so need to gate off the
the peripheral clock when registering the clocks to avoid glitch.

Tested boot on i.MX8MM/P-EVK board

Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Add Fixes tag

 drivers/clk/imx/clk-composite-8m.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index 2dfd6149e528..ee41fbf90589 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -184,6 +184,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 	struct clk_mux *mux = NULL;
 	const struct clk_ops *divider_ops;
 	const struct clk_ops *mux_ops;
+	u32 val;
 
 	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
 	if (!mux)
@@ -216,8 +217,14 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 		div->width = PCG_PREDIV_WIDTH;
 		divider_ops = &imx8m_clk_composite_divider_ops;
 		mux_ops = &clk_mux_ops;
-		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
+		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) {
 			flags |= CLK_SET_PARENT_GATE;
+			if (!(flags & CLK_IS_CRITICAL)) {
+				val = readl(reg);
+				val &= ~BIT(PCG_CGC_SHIFT);
+				writel(val, reg);
+			}
+		}
 	}
 
 	div->lock = &imx_ccm_lock;
-- 
2.30.0


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

* Re: [PATCH V2] clk: imx: gate off peripheral clock slice
  2021-10-25 12:08 ` Abel Vesa
@ 2021-10-29 16:47   ` Ahmad Fatoum
  2021-11-01  2:26     ` Peng Fan
  2021-11-02  7:45     ` Peng Fan
  0 siblings, 2 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2021-10-29 16:47 UTC (permalink / raw)
  To: Abel Vesa, Peng Fan (OSS)
  Cc: Peng Fan, sboyd, mturquette, linux-kernel, linux-clk, linux-imx,
	kernel, festevam, s.hauer, linux-arm-kernel

Hello Peng, Abel,

On 25.10.21 14:08, Abel Vesa wrote:
> On 21-10-25 20:29:02, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> The Peripheral clocks are default enabled when SoC power on, and
>> bootloader not gate off the clocks when booting Linux Kernel.
>>
>> So Linux Kernel is not aware the peripheral clocks are enabled and
>> still take them as disabled because of enable count is zero.
>>
>> Then Peripheral clock's source without clock gated off could be
>> changed when have assigned-parents in device tree
>>
>> However, per i.MX8M* reference mannual, "Peripheral clock slices must
>> be stopped to change the clock source", so need to gate off the
>> the peripheral clock when registering the clocks to avoid glitch.
>>
>> Tested boot on i.MX8MM/P-EVK board

I just noticed this breaks earlycon on an i.MX8MN, I assume because
the earlycon driver will access unclocked registers.

On the i.MX8MN, I also noticed that disabling usb_phy_ref hangs the
system, whether via sysfs or via this patch. Still trying to figure
that one out.

Cheers,
Ahmad

>> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>
>> V2:
>>  Add Fixes tag
>>
>>  drivers/clk/imx/clk-composite-8m.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
>> index 2dfd6149e528..ee41fbf90589 100644
>> --- a/drivers/clk/imx/clk-composite-8m.c
>> +++ b/drivers/clk/imx/clk-composite-8m.c
>> @@ -184,6 +184,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>>  	struct clk_mux *mux = NULL;
>>  	const struct clk_ops *divider_ops;
>>  	const struct clk_ops *mux_ops;
>> +	u32 val;
>>  
>>  	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>>  	if (!mux)
>> @@ -216,8 +217,14 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>>  		div->width = PCG_PREDIV_WIDTH;
>>  		divider_ops = &imx8m_clk_composite_divider_ops;
>>  		mux_ops = &clk_mux_ops;
>> -		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
>> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) {
>>  			flags |= CLK_SET_PARENT_GATE;
>> +			if (!(flags & CLK_IS_CRITICAL)) {
>> +				val = readl(reg);
>> +				val &= ~BIT(PCG_CGC_SHIFT);
>> +				writel(val, reg);
>> +			}
>> +		}
> 
> Though I'm usually against special cases like this one. I think the clock
> core needs some generic flag that would read the state from HW on probe
> and/or another generic flag for disabling on probe.
> 
> But for now, I'm OK with this:
> 
> Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
> 
>>  	}
>>  
>>  	div->lock = &imx_ccm_lock;
>> -- 
>> 2.30.0
>>
> 
> 


-- 
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] 7+ messages in thread

* RE: [PATCH V2] clk: imx: gate off peripheral clock slice
  2021-10-29 16:47   ` Ahmad Fatoum
@ 2021-11-01  2:26     ` Peng Fan
  2021-11-02  7:45     ` Peng Fan
  1 sibling, 0 replies; 7+ messages in thread
From: Peng Fan @ 2021-11-01  2:26 UTC (permalink / raw)
  To: Ahmad Fatoum, Abel Vesa, Peng Fan (OSS)
  Cc: sboyd, mturquette, linux-kernel, linux-clk, dl-linux-imx, kernel,
	festevam, s.hauer, linux-arm-kernel

> Subject: Re: [PATCH V2] clk: imx: gate off peripheral clock slice
> 
> Hello Peng, Abel,
> 
> On 25.10.21 14:08, Abel Vesa wrote:
> > On 21-10-25 20:29:02, Peng Fan (OSS) wrote:
> >> From: Peng Fan <peng.fan@nxp.com>
> >>
> >> The Peripheral clocks are default enabled when SoC power on, and
> >> bootloader not gate off the clocks when booting Linux Kernel.
> >>
> >> So Linux Kernel is not aware the peripheral clocks are enabled and
> >> still take them as disabled because of enable count is zero.
> >>
> >> Then Peripheral clock's source without clock gated off could be
> >> changed when have assigned-parents in device tree
> >>
> >> However, per i.MX8M* reference mannual, "Peripheral clock slices must
> >> be stopped to change the clock source", so need to gate off the the
> >> peripheral clock when registering the clocks to avoid glitch.
> >>
> >> Tested boot on i.MX8MM/P-EVK board
> 
> I just noticed this breaks earlycon on an i.MX8MN, I assume because the
> earlycon driver will access unclocked registers.

Not able to reproduce your issue.

root@imx8mnevk:~# uname -a
Linux imx8mnevk 5.15.0-rc7-next-20211029-00001-gbd989abbb94c-dirty #152 SMP Mon Nov 1 10:48:06 CST 2021 aarch64 aarch64 aarch64 GNU/Linux
root@imx8mnevk:~# cat /proc/cmdline
earlycon console=ttymxc1,115200 root=/dev/mmcblk2p2 rootwait rw

> 
> On the i.MX8MN, I also noticed that disabling usb_phy_ref hangs the system,
> whether via sysfs or via this patch. Still trying to figure that one out.

root@imx8mnevk:~# cat /sys/kernel/debug/clk/clk_summary  | grep usb_phy_ref
          usb_phy_ref                 0        0        0   100000000          0     0  50000         N

I am using i.MX8MN-EVK board.

Regards,
Peng.

> 
> Cheers,
> Ahmad
> 
> >> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >>
> >> V2:
> >>  Add Fixes tag
> >>
> >>  drivers/clk/imx/clk-composite-8m.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/imx/clk-composite-8m.c
> >> b/drivers/clk/imx/clk-composite-8m.c
> >> index 2dfd6149e528..ee41fbf90589 100644
> >> --- a/drivers/clk/imx/clk-composite-8m.c
> >> +++ b/drivers/clk/imx/clk-composite-8m.c
> >> @@ -184,6 +184,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const
> char *name,
> >>  	struct clk_mux *mux = NULL;
> >>  	const struct clk_ops *divider_ops;
> >>  	const struct clk_ops *mux_ops;
> >> +	u32 val;
> >>
> >>  	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> >>  	if (!mux)
> >> @@ -216,8 +217,14 @@ struct clk_hw
> *__imx8m_clk_hw_composite(const char *name,
> >>  		div->width = PCG_PREDIV_WIDTH;
> >>  		divider_ops = &imx8m_clk_composite_divider_ops;
> >>  		mux_ops = &clk_mux_ops;
> >> -		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> >> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) {
> >>  			flags |= CLK_SET_PARENT_GATE;
> >> +			if (!(flags & CLK_IS_CRITICAL)) {
> >> +				val = readl(reg);
> >> +				val &= ~BIT(PCG_CGC_SHIFT);
> >> +				writel(val, reg);
> >> +			}
> >> +		}
> >
> > Though I'm usually against special cases like this one. I think the
> > clock core needs some generic flag that would read the state from HW
> > on probe and/or another generic flag for disabling on probe.
> >
> > But for now, I'm OK with this:
> >
> > Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
> >
> >>  	}
> >>
> >>  	div->lock = &imx_ccm_lock;
> >> --
> >> 2.30.0
> >>
> >
> >
> 
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=04%7C01%7Cpeng.fan%40nxp.com%7C626714
> cd2773448a4c0608d99afbc80d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637711228448591454%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10
> 00&amp;sdata=235%2BBGnDLUAIorqGKVVt%2BUKlSAf5jT68wq2s5FMDolY%
> 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] 7+ messages in thread

* RE: [PATCH V2] clk: imx: gate off peripheral clock slice
  2021-10-29 16:47   ` Ahmad Fatoum
  2021-11-01  2:26     ` Peng Fan
@ 2021-11-02  7:45     ` Peng Fan
  1 sibling, 0 replies; 7+ messages in thread
From: Peng Fan @ 2021-11-02  7:45 UTC (permalink / raw)
  To: Ahmad Fatoum, Abel Vesa, Peng Fan (OSS)
  Cc: sboyd, mturquette, linux-kernel, linux-clk, dl-linux-imx, kernel,
	festevam, s.hauer, linux-arm-kernel

> Subject: Re: [PATCH V2] clk: imx: gate off peripheral clock slice
> 
> Hello Peng, Abel,
> 
> On 25.10.21 14:08, Abel Vesa wrote:
> > On 21-10-25 20:29:02, Peng Fan (OSS) wrote:
> >> From: Peng Fan <peng.fan@nxp.com>
> >>
> >> The Peripheral clocks are default enabled when SoC power on, and
> >> bootloader not gate off the clocks when booting Linux Kernel.
> >>
> >> So Linux Kernel is not aware the peripheral clocks are enabled and
> >> still take them as disabled because of enable count is zero.
> >>
> >> Then Peripheral clock's source without clock gated off could be
> >> changed when have assigned-parents in device tree
> >>
> >> However, per i.MX8M* reference mannual, "Peripheral clock slices must
> >> be stopped to change the clock source", so need to gate off the the
> >> peripheral clock when registering the clocks to avoid glitch.
> >>
> >> Tested boot on i.MX8MM/P-EVK board
> 
> I just noticed this breaks earlycon on an i.MX8MN, I assume because the
> earlycon driver will access unclocked registers.
> 
> On the i.MX8MN, I also noticed that disabling usb_phy_ref hangs the system,
> whether via sysfs or via this patch. Still trying to figure that one out.


This patch indeed breaks earlycon, need to think out how to fix this.
But I am still not sure why usb_phy_ref matters here.

Thanks,
Peng.

> 
> Cheers,
> Ahmad
> 
> >> Fixes: d3ff9728134e ("clk: imx: Add imx composite clock")
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >>
> >> V2:
> >>  Add Fixes tag
> >>
> >>  drivers/clk/imx/clk-composite-8m.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/imx/clk-composite-8m.c
> >> b/drivers/clk/imx/clk-composite-8m.c
> >> index 2dfd6149e528..ee41fbf90589 100644
> >> --- a/drivers/clk/imx/clk-composite-8m.c
> >> +++ b/drivers/clk/imx/clk-composite-8m.c
> >> @@ -184,6 +184,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const
> char *name,
> >>  	struct clk_mux *mux = NULL;
> >>  	const struct clk_ops *divider_ops;
> >>  	const struct clk_ops *mux_ops;
> >> +	u32 val;
> >>
> >>  	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> >>  	if (!mux)
> >> @@ -216,8 +217,14 @@ struct clk_hw
> *__imx8m_clk_hw_composite(const char *name,
> >>  		div->width = PCG_PREDIV_WIDTH;
> >>  		divider_ops = &imx8m_clk_composite_divider_ops;
> >>  		mux_ops = &clk_mux_ops;
> >> -		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED))
> >> +		if (!(composite_flags & IMX_COMPOSITE_FW_MANAGED)) {
> >>  			flags |= CLK_SET_PARENT_GATE;
> >> +			if (!(flags & CLK_IS_CRITICAL)) {
> >> +				val = readl(reg);
> >> +				val &= ~BIT(PCG_CGC_SHIFT);
> >> +				writel(val, reg);
> >> +			}
> >> +		}
> >
> > Though I'm usually against special cases like this one. I think the
> > clock core needs some generic flag that would read the state from HW
> > on probe and/or another generic flag for disabling on probe.
> >
> > But for now, I'm OK with this:
> >
> > Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
> >
> >>  	}
> >>
> >>  	div->lock = &imx_ccm_lock;
> >> --
> >> 2.30.0
> >>
> >
> >
> 
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=04%7C01%7Cpeng.fan%40nxp.com%7C626714
> cd2773448a4c0608d99afbc80d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637711228448591454%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10
> 00&amp;sdata=235%2BBGnDLUAIorqGKVVt%2BUKlSAf5jT68wq2s5FMDolY%
> 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] 7+ messages in thread

end of thread, other threads:[~2021-11-02  7:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 12:29 [PATCH V2] clk: imx: gate off peripheral clock slice Peng Fan (OSS)
2021-10-25 11:54 ` Ahmad Fatoum
2021-10-25 11:59   ` Ahmad Fatoum
2021-10-25 12:08 ` Abel Vesa
2021-10-29 16:47   ` Ahmad Fatoum
2021-11-01  2:26     ` Peng Fan
2021-11-02  7:45     ` Peng Fan

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).