[2/2] clk: imx: disable i.mx7ulp composite clock during initialization

Commit Message

Anson Huang April 25, 2019, 5:19 a.m. UTC

i.MX7ULP peripheral clock ONLY allow parent/rate to be changed
with clock gated, however, during clock tree initialization, the
peripheral clock could be enabled by bootloader, but the prepare
count in clock tree is still zero, so clock core driver will allow
parent/rate changed even with CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE
set, but the change will fail due to HW NOT allow parent/rate change
with clock enabled. It will cause clock HW status mismatch with
clock tree info and lead to function issue. Below is an example:

usdhc0's pcc clock value is 0xC5000000 during kernel boot up, it
means usdhc0 clock is enabled, its parent is APLL_PFD1. In DT file,
the usdhc0 clock settings are as below:

assigned-clocks = <&pcc2 IMX7ULP_CLK_USDHC0>;
assigned-clock-parents = <&scg1 IMX7ULP_CLK_NIC1_DIV>;

when kernel boot up, the clock tree info is as below, but the usdhc0
PCC register is still 0xC5000000, which means its parent is still
from APLL_PFD1, which is incorrect and cause usdhc0 NOT work.

nic1_clk       2        2        0   176000000          0     0  50000
    usdhc0       0        0        0   176000000          0     0  50000

After making sure the peripheral clock is disabled during clock tree
initialization, the usdhc0 is working, and this change is necessary
for all i.MX7ULP peripheral clocks.

Signed-off-by: Anson Huang <[e-mail]>
---
 drivers/clk/imx/clk-composite-7ulp.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Stephen Boyd April 26, 2019, 12:03 a.m. UTC | #1

Quoting Anson Huang (2019-04-24 22:19:12)
> i.MX7ULP peripheral clock ONLY allow parent/rate to be changed
> with clock gated, however, during clock tree initialization, the
> peripheral clock could be enabled by bootloader, but the prepare
> count in clock tree is still zero, so clock core driver will allow
> parent/rate changed even with CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE

That's a bug. Can you send a patch to fix the core framework code to
fail an assigned rate or parent change if those flags are set? Or is
that because the core doesn't respect these flags when they're buried in
the middle of the clk tree and some rate or parent change comes in and
affects the middle of the tree that has the flag set on it?

> set, but the change will fail due to HW NOT allow parent/rate change
> with clock enabled. It will cause clock HW status mismatch with
> clock tree info and lead to function issue. Below is an example:
> 
> usdhc0's pcc clock value is 0xC5000000 during kernel boot up, it
> means usdhc0 clock is enabled, its parent is APLL_PFD1. In DT file,
> the usdhc0 clock settings are as below:
> 
> assigned-clocks = <&pcc2 IMX7ULP_CLK_USDHC0>;
> assigned-clock-parents = <&scg1 IMX7ULP_CLK_NIC1_DIV>;
> 
> when kernel boot up, the clock tree info is as below, but the usdhc0
> PCC register is still 0xC5000000, which means its parent is still
> from APLL_PFD1, which is incorrect and cause usdhc0 NOT work.
> 
> nic1_clk       2        2        0   176000000          0     0  50000
>     usdhc0       0        0        0   176000000          0     0  50000
> 
> After making sure the peripheral clock is disabled during clock tree
> initialization, the usdhc0 is working, and this change is necessary
> for all i.MX7ULP peripheral clocks.
> 
> Signed-off-by: Anson Huang <[e-mail]>
> ---
>  drivers/clk/imx/clk-composite-7ulp.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/clk/imx/clk-composite-7ulp.c b/drivers/clk/imx/clk-composite-7ulp.c
> index 060f860..1a05411 100644
> --- a/drivers/clk/imx/clk-composite-7ulp.c
> +++ b/drivers/clk/imx/clk-composite-7ulp.c
> @@ -32,6 +32,7 @@ struct clk_hw *imx7ulp_clk_composite(const char *name,
>         struct clk_gate *gate = NULL;
>         struct clk_mux *mux = NULL;
>         struct clk_hw *hw;
> +       u32 val;
>  
>         if (mux_present) {
>                 mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> @@ -70,6 +71,18 @@ struct clk_hw *imx7ulp_clk_composite(const char *name,
>                 gate_hw = &gate->hw;
>                 gate->reg = reg;
>                 gate->bit_idx = PCG_CGC_SHIFT;
> +               /*
> +                * make sure clock is gated during clock tree initialization,
> +                * the HW ONLY allow clock parent/rate changed with clock gated,
> +                * during clock tree initialization, clocks could be enabled
> +                * by bootloader, so the HW status will mismatch with clock tree
> +                * prepare count, then clock core driver will allow parent/rate
> +                * change since the prepare count is zero, but HW actually
> +                * prevent the parent/rate change due to the clock is enabled.
> +                */

Is it OK to forcibly gate the clk like this at init time? If so, why
can't we force the clk off when we change the rate and then restore the
on state after changing the rate? That would be more "robust" than doing
it once here. Plus then we could remove the CLK_SET_RATE_GATE flag.

> +               val = readl_relaxed(reg);
> +               val &= ~(1 << PCG_CGC_SHIFT);
> +               writel_relaxed(val, reg);
>         }
>

Anson Huang April 26, 2019, 12:50 a.m. UTC | #2

Hi, Stephen

> -----Original Message-----
> From: Stephen Boyd [mailto:[e-mail]]
> Sent: Friday, April 26, 2019 8:03 AM
> To: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Aisheng Dong <[e-mail]>; Anson Huang
> <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during
> initialization
> 
> Quoting Anson Huang (2019-04-24 22:19:12)
> > i.MX7ULP peripheral clock ONLY allow parent/rate to be changed with
> > clock gated, however, during clock tree initialization, the peripheral
> > clock could be enabled by bootloader, but the prepare count in clock
> > tree is still zero, so clock core driver will allow parent/rate
> > changed even with CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE
> 
> That's a bug. Can you send a patch to fix the core framework code to fail an
> assigned rate or parent change if those flags are set? Or is that because the
> core doesn't respect these flags when they're buried in the middle of the clk
> tree and some rate or parent change comes in and affects the middle of the
> tree that has the flag set on it?

The reason is the core framework ONLY checks the prepare count  and the flags
to determine whether the parent/rate change is allowed, during driver probe phase,
the clk prepare count is 0 by default since no one ever prepare it, but the default HW
status of the clk is actually prepared/enabled, so HW will prevent the parent/rate
change, and the register write will fail, but core framework does NOT know that,
it does NOT read back the register value to check.

If want to fix it from core framework, I think it should check the clk's HW status by
calling .is_enabled() instead of prepare_count, that can reflect the real HW status
during driver probe phase. Checking the prepare_count is OK for later operations
by modules clk parent/rate change after clock being prepared/unprepared, but it
does NOT cover the case of assigning clock from DT during driver probe phase I think.
So do we need to fix it from clk core framework?

         /* some clocks must be gated to change parent */
         if (parent != old_parent &&
             (core->flags & CLK_SET_PARENT_GATE) && core->prepare_count) {
                 pr_debug("%s: %s not gated but wants to reparent\n",
                          __func__, core->name);
                 return NULL;
         }

> 
> > set, but the change will fail due to HW NOT allow parent/rate change
> > with clock enabled. It will cause clock HW status mismatch with clock
> > tree info and lead to function issue. Below is an example:
> >
> > usdhc0's pcc clock value is 0xC5000000 during kernel boot up, it means
> > usdhc0 clock is enabled, its parent is APLL_PFD1. In DT file, the
> > usdhc0 clock settings are as below:
> >
> > assigned-clocks = <&pcc2 IMX7ULP_CLK_USDHC0>; assigned-clock-parents =
> > <&scg1 IMX7ULP_CLK_NIC1_DIV>;
> >
> > when kernel boot up, the clock tree info is as below, but the usdhc0
> > PCC register is still 0xC5000000, which means its parent is still from
> > APLL_PFD1, which is incorrect and cause usdhc0 NOT work.
> >
> > nic1_clk       2        2        0   176000000          0     0  50000
> >     usdhc0       0        0        0   176000000          0     0  50000
> >
> > After making sure the peripheral clock is disabled during clock tree
> > initialization, the usdhc0 is working, and this change is necessary
> > for all i.MX7ULP peripheral clocks.
> >
> > Signed-off-by: Anson Huang <[e-mail]>
> > ---
> >  drivers/clk/imx/clk-composite-7ulp.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > b/drivers/clk/imx/clk-composite-7ulp.c
> > index 060f860..1a05411 100644
> > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > @@ -32,6 +32,7 @@ struct clk_hw *imx7ulp_clk_composite(const char
> *name,
> >         struct clk_gate *gate = NULL;
> >         struct clk_mux *mux = NULL;
> >         struct clk_hw *hw;
> > +       u32 val;
> >
> >         if (mux_present) {
> >                 mux = kzalloc(sizeof(*mux), GFP_KERNEL); @@ -70,6
> > +71,18 @@ struct clk_hw *imx7ulp_clk_composite(const char *name,
> >                 gate_hw = &gate->hw;
> >                 gate->reg = reg;
> >                 gate->bit_idx = PCG_CGC_SHIFT;
> > +               /*
> > +                * make sure clock is gated during clock tree initialization,
> > +                * the HW ONLY allow clock parent/rate changed with clock gated,
> > +                * during clock tree initialization, clocks could be enabled
> > +                * by bootloader, so the HW status will mismatch with clock tree
> > +                * prepare count, then clock core driver will allow parent/rate
> > +                * change since the prepare count is zero, but HW actually
> > +                * prevent the parent/rate change due to the clock is enabled.
> > +                */
> 
> Is it OK to forcibly gate the clk like this at init time? If so, why can't we force
> the clk off when we change the rate and then restore the on state after
> changing the rate? That would be more "robust" than doing it once here. Plus
> then we could remove the CLK_SET_RATE_GATE flag.

It should be OK to gate clk during init time, but I need to double check whether it will
impact the early uart function, I missed this part, other drivers are OK since they will
enable clock before active.

Yes, we can force the clk off when changing the rate and then restore them ON, but it
can ONLY be handled in module drivers by calling clk API, it can NOT fix the case of  
assigned-clock-parents and assigned-clock-rates in DT, which is during driver probe phase
by common code.

Anson

> 
> > +               val = readl_relaxed(reg);
> > +               val &= ~(1 << PCG_CGC_SHIFT);
> > +               writel_relaxed(val, reg);
> >         }
> >

Anson Huang April 28, 2019, 1:25 a.m. UTC | #3

Hi, Stephen
	For this patch series, I think patch [1/2] can be picked up directly, for patch[2/2], it can be fixed from driver level, it means drivers should make sure the composite clock is gated before changing parent/rate, so no need to do any change to composite-7ulp clk driver or core framework. Then the assigned-clock-parents/rates may NOT be available for i.MX7ULP, is this OK? It is anyway something special on i.MX7ULP HW design, SW needs to follow it.

> -----Original Message-----
> From: Anson Huang
> Sent: Friday, April 26, 2019 8:50 AM
> To: Stephen Boyd <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Aisheng Dong
> <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during
> initialization
> 
> Hi, Stephen
> 
> > -----Original Message-----
> > From: Stephen Boyd [mailto:[e-mail]]
> > Sent: Friday, April 26, 2019 8:03 AM
> > To: [e-mail]; [e-mail];
> [e-mail];
> > [e-mail]; linux- [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Aisheng Dong
> > <[e-mail]>; Anson Huang <[e-mail]>
> > Cc: dl-linux-imx <[e-mail]>
> > Subject: Re: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock
> > during initialization
> >
> > Quoting Anson Huang (2019-04-24 22:19:12)
> > > i.MX7ULP peripheral clock ONLY allow parent/rate to be changed with
> > > clock gated, however, during clock tree initialization, the
> > > peripheral clock could be enabled by bootloader, but the prepare
> > > count in clock tree is still zero, so clock core driver will allow
> > > parent/rate changed even with
> CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE
> >
> > That's a bug. Can you send a patch to fix the core framework code to
> > fail an assigned rate or parent change if those flags are set? Or is
> > that because the core doesn't respect these flags when they're buried
> > in the middle of the clk tree and some rate or parent change comes in
> > and affects the middle of the tree that has the flag set on it?
> 
> The reason is the core framework ONLY checks the prepare count  and the
> flags to determine whether the parent/rate change is allowed, during driver
> probe phase, the clk prepare count is 0 by default since no one ever prepare
> it, but the default HW status of the clk is actually prepared/enabled, so HW
> will prevent the parent/rate change, and the register write will fail, but core
> framework does NOT know that, it does NOT read back the register value to
> check.
> 
> If want to fix it from core framework, I think it should check the clk's HW
> status by calling .is_enabled() instead of prepare_count, that can reflect the
> real HW status during driver probe phase. Checking the prepare_count is OK
> for later operations by modules clk parent/rate change after clock being
> prepared/unprepared, but it does NOT cover the case of assigning clock from
> DT during driver probe phase I think.
> So do we need to fix it from clk core framework?
> 
>          /* some clocks must be gated to change parent */
>          if (parent != old_parent &&
>              (core->flags & CLK_SET_PARENT_GATE) && core->prepare_count) {
>                  pr_debug("%s: %s not gated but wants to reparent\n",
>                           __func__, core->name);
>                  return NULL;
>          }
> 
> >
> > > set, but the change will fail due to HW NOT allow parent/rate change
> > > with clock enabled. It will cause clock HW status mismatch with
> > > clock tree info and lead to function issue. Below is an example:
> > >
> > > usdhc0's pcc clock value is 0xC5000000 during kernel boot up, it
> > > means
> > > usdhc0 clock is enabled, its parent is APLL_PFD1. In DT file, the
> > > usdhc0 clock settings are as below:
> > >
> > > assigned-clocks = <&pcc2 IMX7ULP_CLK_USDHC0>; assigned-clock-parents
> > > =
> > > <&scg1 IMX7ULP_CLK_NIC1_DIV>;
> > >
> > > when kernel boot up, the clock tree info is as below, but the usdhc0
> > > PCC register is still 0xC5000000, which means its parent is still
> > > from APLL_PFD1, which is incorrect and cause usdhc0 NOT work.
> > >
> > > nic1_clk       2        2        0   176000000          0     0  50000
> > >     usdhc0       0        0        0   176000000          0     0  50000
> > >
> > > After making sure the peripheral clock is disabled during clock tree
> > > initialization, the usdhc0 is working, and this change is necessary
> > > for all i.MX7ULP peripheral clocks.
> > >
> > > Signed-off-by: Anson Huang <[e-mail]>
> > > ---
> > >  drivers/clk/imx/clk-composite-7ulp.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > > b/drivers/clk/imx/clk-composite-7ulp.c
> > > index 060f860..1a05411 100644
> > > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > > @@ -32,6 +32,7 @@ struct clk_hw *imx7ulp_clk_composite(const char
> > *name,
> > >         struct clk_gate *gate = NULL;
> > >         struct clk_mux *mux = NULL;
> > >         struct clk_hw *hw;
> > > +       u32 val;
> > >
> > >         if (mux_present) {
> > >                 mux = kzalloc(sizeof(*mux), GFP_KERNEL); @@ -70,6
> > > +71,18 @@ struct clk_hw *imx7ulp_clk_composite(const char *name,
> > >                 gate_hw = &gate->hw;
> > >                 gate->reg = reg;
> > >                 gate->bit_idx = PCG_CGC_SHIFT;
> > > +               /*
> > > +                * make sure clock is gated during clock tree initialization,
> > > +                * the HW ONLY allow clock parent/rate changed with clock
> gated,
> > > +                * during clock tree initialization, clocks could be enabled
> > > +                * by bootloader, so the HW status will mismatch with clock tree
> > > +                * prepare count, then clock core driver will allow parent/rate
> > > +                * change since the prepare count is zero, but HW actually
> > > +                * prevent the parent/rate change due to the clock is enabled.
> > > +                */
> >
> > Is it OK to forcibly gate the clk like this at init time? If so, why
> > can't we force the clk off when we change the rate and then restore
> > the on state after changing the rate? That would be more "robust" than
> > doing it once here. Plus then we could remove the CLK_SET_RATE_GATE
> flag.
> 
> It should be OK to gate clk during init time, but I need to double check
> whether it will impact the early uart function, I missed this part, other drivers
> are OK since they will enable clock before active.
> 
> Yes, we can force the clk off when changing the rate and then restore them
> ON, but it can ONLY be handled in module drivers by calling clk API, it can
> NOT fix the case of assigned-clock-parents and assigned-clock-rates in DT,
> which is during driver probe phase by common code.
> 
> Anson
> 
> >
> > > +               val = readl_relaxed(reg);
> > > +               val &= ~(1 << PCG_CGC_SHIFT);
> > > +               writel_relaxed(val, reg);
> > >         }
> > >

Anson Huang July 24, 2019, 8:27 a.m. UTC | #4

Hi, Stephen

> Quoting Anson Huang (2019-04-24 22:19:12)
> > i.MX7ULP peripheral clock ONLY allow parent/rate to be changed with
> > clock gated, however, during clock tree initialization, the peripheral
> > clock could be enabled by bootloader, but the prepare count in clock
> > tree is still zero, so clock core driver will allow parent/rate
> > changed even with CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE
> 
> That's a bug. Can you send a patch to fix the core framework code to fail an
> assigned rate or parent change if those flags are set? Or is that because the
> core doesn't respect these flags when they're buried in the middle of the clk
> tree and some rate or parent change comes in and affects the middle of the
> tree that has the flag set on it?

If changing the core framework code to return fail for clk parent/rate assignment,
that means clk assignment in DT will NOT work for i.MX7ULP, then all the clk
rate/parent settings will be done in driver? That will lead to more issues/changes.

It is just because core framework ONLY checks the prepare_count and CLK_SET_PARENT_GATE
flag to determine if the parent switch is allowed, however, during clock tree initialization,
the prepare_count is always 0 but the HW status could be enabled actually, so the core framework
will allow the parent switch while HW status does NOT allow the parent switch, so core framework
will treat the parent switch successfully but HW is actually NOT. 

I think we can treat it as platform specific issue, if bootloader can guarantee all peripheral clocks
are disabled before jumping to kernel, then there will be no issue, but we can NOT assume that, so
I have to find some place in early kernel phase to disable those peripheral clocks.

> 
> > set, but the change will fail due to HW NOT allow parent/rate change
> > with clock enabled. It will cause clock HW status mismatch with clock
> > tree info and lead to function issue. Below is an example:
> >
> > usdhc0's pcc clock value is 0xC5000000 during kernel boot up, it means
> > usdhc0 clock is enabled, its parent is APLL_PFD1. In DT file, the
> > usdhc0 clock settings are as below:
> >
> > assigned-clocks = <&pcc2 IMX7ULP_CLK_USDHC0>; assigned-clock-parents =
> > <&scg1 IMX7ULP_CLK_NIC1_DIV>;
> >
> > when kernel boot up, the clock tree info is as below, but the usdhc0
> > PCC register is still 0xC5000000, which means its parent is still from
> > APLL_PFD1, which is incorrect and cause usdhc0 NOT work.
> >
> > nic1_clk       2        2        0   176000000          0     0  50000
> >     usdhc0       0        0        0   176000000          0     0  50000
> >
> > After making sure the peripheral clock is disabled during clock tree
> > initialization, the usdhc0 is working, and this change is necessary
> > for all i.MX7ULP peripheral clocks.
> >
> > Signed-off-by: Anson Huang <[e-mail]>
> > ---
> >  drivers/clk/imx/clk-composite-7ulp.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > b/drivers/clk/imx/clk-composite-7ulp.c
> > index 060f860..1a05411 100644
> > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > @@ -32,6 +32,7 @@ struct clk_hw *imx7ulp_clk_composite(const char
> *name,
> >         struct clk_gate *gate = NULL;
> >         struct clk_mux *mux = NULL;
> >         struct clk_hw *hw;
> > +       u32 val;
> >
> >         if (mux_present) {
> >                 mux = kzalloc(sizeof(*mux), GFP_KERNEL); @@ -70,6
> > +71,18 @@ struct clk_hw *imx7ulp_clk_composite(const char *name,
> >                 gate_hw = &gate->hw;
> >                 gate->reg = reg;
> >                 gate->bit_idx = PCG_CGC_SHIFT;
> > +               /*
> > +                * make sure clock is gated during clock tree initialization,
> > +                * the HW ONLY allow clock parent/rate changed with clock gated,
> > +                * during clock tree initialization, clocks could be enabled
> > +                * by bootloader, so the HW status will mismatch with clock tree
> > +                * prepare count, then clock core driver will allow parent/rate
> > +                * change since the prepare count is zero, but HW actually
> > +                * prevent the parent/rate change due to the clock is enabled.
> > +                */
> 
> Is it OK to forcibly gate the clk like this at init time? If so, why can't we force
> the clk off when we change the rate and then restore the on state after
> changing the rate? That would be more "robust" than doing it once here. Plus
> then we could remove the CLK_SET_RATE_GATE flag.

Yes, it is ONLY for composite clocks which are for peripheral clocks, ONLY earlycon
could be impacted but we can add imx_register_uart_clocks() call to make earlycon
also work.

Forcing the clk off and restore them ON for rate/parent change will need to change
common composite clock ops, currently i.MX7ULP all use common ops, so unless
i.MX7ULP implements composite clock ops, and the change will be very significant.  

Thanks,
Anson

> 
> > +               val = readl_relaxed(reg);
> > +               val &= ~(1 << PCG_CGC_SHIFT);
> > +               writel_relaxed(val, reg);
> >         }
> >

Anson Huang Aug. 6, 2019, 2:25 a.m. UTC | #5

Hi, Stephen
	I think we should resume this thread, without this patch, mainline kernel boot up will cause mmc timeout all the time. If it is NOT good to disabling those peripheral devices' clock in i.MX7ULP's clock driver, then we have to change the core framework to disable clock explicitly if the CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE is present, most likely it will impact other platforms I think, so the most safe way is just to do it inside our i.MX7ULP composite clock driver. What do you think?

Thanks,
Anson

> Hi, Stephen
> 
> > Quoting Anson Huang (2019-04-24 22:19:12)
> > > i.MX7ULP peripheral clock ONLY allow parent/rate to be changed with
> > > clock gated, however, during clock tree initialization, the
> > > peripheral clock could be enabled by bootloader, but the prepare
> > > count in clock tree is still zero, so clock core driver will allow
> > > parent/rate changed even with
> CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE
> >
> > That's a bug. Can you send a patch to fix the core framework code to
> > fail an assigned rate or parent change if those flags are set? Or is
> > that because the core doesn't respect these flags when they're buried
> > in the middle of the clk tree and some rate or parent change comes in
> > and affects the middle of the tree that has the flag set on it?
> 
> If changing the core framework code to return fail for clk parent/rate
> assignment, that means clk assignment in DT will NOT work for i.MX7ULP,
> then all the clk rate/parent settings will be done in driver? That will lead to
> more issues/changes.
> 
> It is just because core framework ONLY checks the prepare_count and
> CLK_SET_PARENT_GATE flag to determine if the parent switch is allowed,
> however, during clock tree initialization, the prepare_count is always 0 but
> the HW status could be enabled actually, so the core framework will allow
> the parent switch while HW status does NOT allow the parent switch, so core
> framework will treat the parent switch successfully but HW is actually NOT.
> 
> I think we can treat it as platform specific issue, if bootloader can guarantee
> all peripheral clocks are disabled before jumping to kernel, then there will be
> no issue, but we can NOT assume that, so I have to find some place in early
> kernel phase to disable those peripheral clocks.
> 
> >
> > > set, but the change will fail due to HW NOT allow parent/rate change
> > > with clock enabled. It will cause clock HW status mismatch with
> > > clock tree info and lead to function issue. Below is an example:
> > >
> > > usdhc0's pcc clock value is 0xC5000000 during kernel boot up, it
> > > means
> > > usdhc0 clock is enabled, its parent is APLL_PFD1. In DT file, the
> > > usdhc0 clock settings are as below:
> > >
> > > assigned-clocks = <&pcc2 IMX7ULP_CLK_USDHC0>; assigned-clock-parents
> > > =
> > > <&scg1 IMX7ULP_CLK_NIC1_DIV>;
> > >
> > > when kernel boot up, the clock tree info is as below, but the usdhc0
> > > PCC register is still 0xC5000000, which means its parent is still
> > > from APLL_PFD1, which is incorrect and cause usdhc0 NOT work.
> > >
> > > nic1_clk       2        2        0   176000000          0     0  50000
> > >     usdhc0       0        0        0   176000000          0     0  50000
> > >
> > > After making sure the peripheral clock is disabled during clock tree
> > > initialization, the usdhc0 is working, and this change is necessary
> > > for all i.MX7ULP peripheral clocks.
> > >
> > > Signed-off-by: Anson Huang <[e-mail]>
> > > ---
> > >  drivers/clk/imx/clk-composite-7ulp.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > > b/drivers/clk/imx/clk-composite-7ulp.c
> > > index 060f860..1a05411 100644
> > > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > > @@ -32,6 +32,7 @@ struct clk_hw *imx7ulp_clk_composite(const char
> > *name,
> > >         struct clk_gate *gate = NULL;
> > >         struct clk_mux *mux = NULL;
> > >         struct clk_hw *hw;
> > > +       u32 val;
> > >
> > >         if (mux_present) {
> > >                 mux = kzalloc(sizeof(*mux), GFP_KERNEL); @@ -70,6
> > > +71,18 @@ struct clk_hw *imx7ulp_clk_composite(const char *name,
> > >                 gate_hw = &gate->hw;
> > >                 gate->reg = reg;
> > >                 gate->bit_idx = PCG_CGC_SHIFT;
> > > +               /*
> > > +                * make sure clock is gated during clock tree initialization,
> > > +                * the HW ONLY allow clock parent/rate changed with clock
> gated,
> > > +                * during clock tree initialization, clocks could be enabled
> > > +                * by bootloader, so the HW status will mismatch with clock tree
> > > +                * prepare count, then clock core driver will allow parent/rate
> > > +                * change since the prepare count is zero, but HW actually
> > > +                * prevent the parent/rate change due to the clock is enabled.
> > > +                */
> >
> > Is it OK to forcibly gate the clk like this at init time? If so, why
> > can't we force the clk off when we change the rate and then restore
> > the on state after changing the rate? That would be more "robust" than
> > doing it once here. Plus then we could remove the CLK_SET_RATE_GATE
> flag.
> 
> Yes, it is ONLY for composite clocks which are for peripheral clocks, ONLY
> earlycon could be impacted but we can add imx_register_uart_clocks() call to
> make earlycon also work.
> 
> Forcing the clk off and restore them ON for rate/parent change will need to
> change common composite clock ops, currently i.MX7ULP all use common
> ops, so unless i.MX7ULP implements composite clock ops, and the change
> will be very significant.
> 
> Thanks,
> Anson
> 
> >
> > > +               val = readl_relaxed(reg);
> > > +               val &= ~(1 << PCG_CGC_SHIFT);
> > > +               writel_relaxed(val, reg);
> > >         }
> > >

Anson Huang Aug. 29, 2019, 5:52 a.m. UTC | #6

Hi, Stephen

> Subject: RE: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during
> initialization
> 
> Hi, Stephen
> 	I think we should resume this thread, without this patch, mainline
> kernel boot up will cause mmc timeout all the time. If it is NOT good to
> disabling those peripheral devices' clock in i.MX7ULP's clock driver, then we
> have to change the core framework to disable clock explicitly if the
> CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE is present, most likely it will
> impact other platforms I think, so the most safe way is just to do it inside our
> i.MX7ULP composite clock driver. What do you think?

What is your opinion on this?

Thanks,
Anson

> 
> Thanks,
> Anson
> 
> > Hi, Stephen
> >
> > > Quoting Anson Huang (2019-04-24 22:19:12)
> > > > i.MX7ULP peripheral clock ONLY allow parent/rate to be changed
> > > > with clock gated, however, during clock tree initialization, the
> > > > peripheral clock could be enabled by bootloader, but the prepare
> > > > count in clock tree is still zero, so clock core driver will allow
> > > > parent/rate changed even with
> > CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE
> > >
> > > That's a bug. Can you send a patch to fix the core framework code to
> > > fail an assigned rate or parent change if those flags are set? Or is
> > > that because the core doesn't respect these flags when they're
> > > buried in the middle of the clk tree and some rate or parent change
> > > comes in and affects the middle of the tree that has the flag set on it?
> >
> > If changing the core framework code to return fail for clk parent/rate
> > assignment, that means clk assignment in DT will NOT work for
> > i.MX7ULP, then all the clk rate/parent settings will be done in
> > driver? That will lead to more issues/changes.
> >
> > It is just because core framework ONLY checks the prepare_count and
> > CLK_SET_PARENT_GATE flag to determine if the parent switch is allowed,
> > however, during clock tree initialization, the prepare_count is always
> > 0 but the HW status could be enabled actually, so the core framework
> > will allow the parent switch while HW status does NOT allow the parent
> > switch, so core framework will treat the parent switch successfully but HW
> is actually NOT.
> >
> > I think we can treat it as platform specific issue, if bootloader can
> > guarantee all peripheral clocks are disabled before jumping to kernel,
> > then there will be no issue, but we can NOT assume that, so I have to
> > find some place in early kernel phase to disable those peripheral clocks.
> >
> > >
> > > > set, but the change will fail due to HW NOT allow parent/rate
> > > > change with clock enabled. It will cause clock HW status mismatch
> > > > with clock tree info and lead to function issue. Below is an example:
> > > >
> > > > usdhc0's pcc clock value is 0xC5000000 during kernel boot up, it
> > > > means
> > > > usdhc0 clock is enabled, its parent is APLL_PFD1. In DT file, the
> > > > usdhc0 clock settings are as below:
> > > >
> > > > assigned-clocks = <&pcc2 IMX7ULP_CLK_USDHC0>;
> > > > assigned-clock-parents =
> > > > <&scg1 IMX7ULP_CLK_NIC1_DIV>;
> > > >
> > > > when kernel boot up, the clock tree info is as below, but the
> > > > usdhc0 PCC register is still 0xC5000000, which means its parent is
> > > > still from APLL_PFD1, which is incorrect and cause usdhc0 NOT work.
> > > >
> > > > nic1_clk       2        2        0   176000000          0     0  50000
> > > >     usdhc0       0        0        0   176000000          0     0  50000
> > > >
> > > > After making sure the peripheral clock is disabled during clock
> > > > tree initialization, the usdhc0 is working, and this change is
> > > > necessary for all i.MX7ULP peripheral clocks.
> > > >
> > > > Signed-off-by: Anson Huang <[e-mail]>
> > > > ---
> > > >  drivers/clk/imx/clk-composite-7ulp.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > > > b/drivers/clk/imx/clk-composite-7ulp.c
> > > > index 060f860..1a05411 100644
> > > > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > > > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > > > @@ -32,6 +32,7 @@ struct clk_hw *imx7ulp_clk_composite(const char
> > > *name,
> > > >         struct clk_gate *gate = NULL;
> > > >         struct clk_mux *mux = NULL;
> > > >         struct clk_hw *hw;
> > > > +       u32 val;
> > > >
> > > >         if (mux_present) {
> > > >                 mux = kzalloc(sizeof(*mux), GFP_KERNEL); @@ -70,6
> > > > +71,18 @@ struct clk_hw *imx7ulp_clk_composite(const char *name,
> > > >                 gate_hw = &gate->hw;
> > > >                 gate->reg = reg;
> > > >                 gate->bit_idx = PCG_CGC_SHIFT;
> > > > +               /*
> > > > +                * make sure clock is gated during clock tree initialization,
> > > > +                * the HW ONLY allow clock parent/rate changed
> > > > + with clock
> > gated,
> > > > +                * during clock tree initialization, clocks could be enabled
> > > > +                * by bootloader, so the HW status will mismatch with clock
> tree
> > > > +                * prepare count, then clock core driver will allow parent/rate
> > > > +                * change since the prepare count is zero, but HW actually
> > > > +                * prevent the parent/rate change due to the clock is enabled.
> > > > +                */
> > >
> > > Is it OK to forcibly gate the clk like this at init time? If so, why
> > > can't we force the clk off when we change the rate and then restore
> > > the on state after changing the rate? That would be more "robust"
> > > than doing it once here. Plus then we could remove the
> > > CLK_SET_RATE_GATE
> > flag.
> >
> > Yes, it is ONLY for composite clocks which are for peripheral clocks,
> > ONLY earlycon could be impacted but we can add
> > imx_register_uart_clocks() call to make earlycon also work.
> >
> > Forcing the clk off and restore them ON for rate/parent change will
> > need to change common composite clock ops, currently i.MX7ULP all use
> > common ops, so unless i.MX7ULP implements composite clock ops, and the
> > change will be very significant.
> >
> > Thanks,
> > Anson
> >
> > >
> > > > +               val = readl_relaxed(reg);
> > > > +               val &= ~(1 << PCG_CGC_SHIFT);
> > > > +               writel_relaxed(val, reg);
> > > >         }
> > > >

Fabio Estevam Sept. 26, 2019, 6:10 p.m. UTC | #7

Hi Anson,

On Mon, Aug 5, 2019 at 11:25 PM Anson Huang <[e-mail]> wrote:
>
> Hi, Stephen
>         I think we should resume this thread, without this patch, mainline kernel boot up will cause mmc timeout all the time. If it is NOT good to disabling those peripheral devices' clock in i.MX7ULP's clock driver, then we have to change the core framework to disable clock explicitly if the CLK_SET_RATE_GATE/CLK_SET_PARENT_GATE is present, most likely it will impact other platforms I think, so the most safe way is just to do it inside our i.MX7ULP composite clock driver. What do you think?

Just tested your patch against 5.3 and mmc card can be detected with it.

Please note that I had to manually add:
#include <linux/io.h>

so that it can build.

I agree we need to come to a solution for this.

Stephen, please let us know your thoughts.

Thanks

Patch

diff --git a/drivers/clk/imx/clk-composite-7ulp.c b/drivers/clk/imx/clk-composite-7ulp.c
index 060f860..1a05411 100644
--- a/drivers/clk/imx/clk-composite-7ulp.c
+++ b/drivers/clk/imx/clk-composite-7ulp.c
@@ -32,6 +32,7 @@  struct clk_hw *imx7ulp_clk_composite(const char *name,
 	struct clk_gate *gate = NULL;
 	struct clk_mux *mux = NULL;
 	struct clk_hw *hw;
+	u32 val;
 
 	if (mux_present) {
 		mux = kzalloc(sizeof(*mux), GFP_KERNEL);
@@ -70,6 +71,18 @@  struct clk_hw *imx7ulp_clk_composite(const char *name,
 		gate_hw = &gate->hw;
 		gate->reg = reg;
 		gate->bit_idx = PCG_CGC_SHIFT;
+		/*
+		 * make sure clock is gated during clock tree initialization,
+		 * the HW ONLY allow clock parent/rate changed with clock gated,
+		 * during clock tree initialization, clocks could be enabled
+		 * by bootloader, so the HW status will mismatch with clock tree
+		 * prepare count, then clock core driver will allow parent/rate
+		 * change since the prepare count is zero, but HW actually
+		 * prevent the parent/rate change due to the clock is enabled.
+		 */
+		val = readl_relaxed(reg);
+		val &= ~(1 << PCG_CGC_SHIFT);
+		writel_relaxed(val, reg);
 	}
 
 	hw = clk_hw_register_composite(NULL, name, parent_names, num_parents,