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

[Anson Huang]

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