From: Peng Fan <[email protected]>
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 <[email protected]>
---
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
On 25.10.21 14:29, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
> ---
>
> 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 |
On 25.10.21 13:54, Ahmad Fatoum wrote:
> On 25.10.21 14:29, Peng Fan (OSS) wrote:
>> From: Peng Fan <[email protected]>
>>
>> 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 <[email protected]>
>
> 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 <[email protected]>
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 |
On 21-10-25 20:29:02, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
> }
>
> div->lock = &imx_ccm_lock;
> --
> 2.30.0
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>
>> 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 <[email protected]>
>
>> }
>>
>> 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 |
> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >>
> >> 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 <[email protected]>
> >
> >> }
> >>
> >> 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&data=04%7C01%7Cpeng.fan%40nxp.com%7C626714
> cd2773448a4c0608d99afbc80d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637711228448591454%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10
> 00&sdata=235%2BBGnDLUAIorqGKVVt%2BUKlSAf5jT68wq2s5FMDolY%
> 3D&reserved=0 |
> 31137 Hildesheim, Germany | Phone:
> +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >>
> >> 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 <[email protected]>
> >
> >> }
> >>
> >> 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&data=04%7C01%7Cpeng.fan%40nxp.com%7C626714
> cd2773448a4c0608d99afbc80d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637711228448591454%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C10
> 00&sdata=235%2BBGnDLUAIorqGKVVt%2BUKlSAf5jT68wq2s5FMDolY%
> 3D&reserved=0 |
> 31137 Hildesheim, Germany | Phone:
> +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |