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

From: Anson Huang <anson.huang@nxp.com>
To: Stephen Boyd <sboyd@kernel.org>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"gustavo@embeddedor.com" <gustavo@embeddedor.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"shawnguo@kernel.org" <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
Date: Wed, 24 Jul 2019 08:27:03 +0000	[thread overview]
Message-ID: <DB3PR0402MB39165F69F8B684D323C683B1F5C60@DB3PR0402MB3916.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <155623699177.15276.12577395377027956830@swboyd.mtv.corp.google.com>

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@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.

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