| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DB3PR0402MB391635A22E8D4AF1E377A7CDF5380@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date: Sun, 28 Apr 2019 01:25:38 +0000
From: Anson Huang <anson.huang@....com>
To: Stephen Boyd <sboyd@...nel.org>,
"festevam@...il.com" <festevam@...il.com>,
"gustavo@...eddedor.com" <gustavo@...eddedor.com>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"mturquette@...libre.com" <mturquette@...libre.com>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
Aisheng Dong <aisheng.dong@....com>
CC: dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during
initialization
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@...nel.org>; festevam@...il.com;
> gustavo@...eddedor.com; kernel@...gutronix.de; linux-arm-
> kernel@...ts.infradead.org; linux-clk@...r.kernel.org; linux-
> kernel@...r.kernel.org; mturquette@...libre.com;
> s.hauer@...gutronix.de; shawnguo@...nel.org; Aisheng Dong
> <aisheng.dong@....com>
> Cc: dl-linux-imx <linux-imx@....com>
> Subject: RE: [PATCH 2/2] clk: imx: disable i.mx7ulp composite clock during
> initialization
>
> Hi, Stephen
>
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@...nel.org]
> > Sent: Friday, April 26, 2019 8:03 AM
> > To: festevam@...il.com; gustavo@...eddedor.com;
> kernel@...gutronix.de;
> > linux-arm-kernel@...ts.infradead.org; linux- clk@...r.kernel.org;
> > linux-kernel@...r.kernel.org; mturquette@...libre.com;
> > s.hauer@...gutronix.de; shawnguo@...nel.org; Aisheng Dong
> > <aisheng.dong@....com>; Anson Huang <anson.huang@....com>
> > Cc: dl-linux-imx <linux-imx@....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@....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);
> > > }
> > >
Powered by blists - more mailing lists