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

[Anson Huang]

Hi, Stephen

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> Sent: Friday, April 26, 2019 8:03 AM
> To: festevam@xxxxxxxxx; gustavo@xxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> clk@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> mturquette@xxxxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx;
> Aisheng Dong <aisheng.dong@xxxxxxx>; Anson Huang
> <anson.huang@xxxxxxx>
> Cc: dl-linux-imx <linux-imx@xxxxxxx>
> 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 <Anson.Huang@xxxxxxx>
> > ---
> >  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);
> >         }
> >