| 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: <DB3PR0402MB39163A24C4949FD9AD5D2553F5A20@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date: Thu, 29 Aug 2019 05:52: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
> 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@....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);
> > > > }
> > > >
Powered by blists - more mailing lists