Messages in this thread | | | From | Anson Huang <> | Subject | RE: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during initialization | Date | Thu, 29 Aug 2019 05:52:38 +0000 |
| |
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@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); > > > > } > > > >
| |