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

[Anson Huang]

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