Messages in this thread | | | From | Anson Huang <> | Subject | RE: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during initialization | Date | Sun, 28 Apr 2019 01:25:38 +0000 |
| |
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); > > > } > > >
| |