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

[Anson Huang <anson.huang@nxp.com>]

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 <sboyd@kernel.org>; festevam@gmail.com;
> gustavo@embeddedor.com; kernel@pengutronix.de; linux-arm-
> kernel@lists.infradead.org; linux-clk@vger.kernel.org; linux-
> kernel@vger.kernel.org; mturquette@baylibre.com;
> s.hauer@pengutronix.de; shawnguo@kernel.org; Aisheng Dong
> <aisheng.dong@nxp.com>
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during
> initialization
> 
> Hi, Stephen
> 
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Sent: Friday, April 26, 2019 8:03 AM
> > To: festevam@gmail.com; gustavo@embeddedor.com;
> kernel@pengutronix.de;
> > linux-arm-kernel@lists.infradead.org; linux- clk@vger.kernel.org;
> > linux-kernel@vger.kernel.org; mturquette@baylibre.com;
> > s.hauer@pengutronix.de; shawnguo@kernel.org; Aisheng Dong
> > <aisheng.dong@nxp.com>; Anson Huang <anson.huang@nxp.com>
> > Cc: dl-linux-imx <linux-imx@nxp.com>
> > 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@nxp.com>
> > > ---
> > >  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);
> > >         }
> > >