linux-kernel - Re: [PATCH v6 3/5] clk: imx: add SCCG PLL type

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Aug 2018 10:58:22 +0000
From:   Abel Vesa <abel.vesa@....com>
To:     Sascha Hauer <s.hauer@...gutronix.de>
CC:     Lucas Stach <l.stach@...gutronix.de>,
        Sascha Hauer <kernel@...gutronix.de>,
        "A.s. Dong" <aisheng.dong@....com>,
        Fabio Estevam <fabio.estevam@....com>,
        Anson Huang <anson.huang@....com>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Abel Vesa <abelvesa@...ux.com>,
        dl-linux-imx <linux-imx@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        Andrey Smirnov <andrew.smirnov@...il.com>
Subject: Re: [PATCH v6 3/5] clk: imx: add SCCG PLL type

On Fri, Aug 24, 2018 at 09:40:11AM +0200, Sascha Hauer wrote:
> +Cc Andrey Smirnov who made me aware of this issue.
> 
> On Wed, Aug 22, 2018 at 04:48:21PM +0300, Abel Vesa wrote:
> > From: Lucas Stach <l.stach@...gutronix.de>
> > 
> > The SCCG is a new PLL type introduced on i.MX8. Add support for this.
> > The driver currently misses the PLL lock check, as the preliminary
> > documentation mentions lock configurations, but is quiet about where
> > to find the actual lock status signal.
> > 
> > Signed-off-by: Lucas Stach <l.stach@...gutronix.de>
> > Signed-off-by: Abel Vesa <abel.vesa@....com>
> > ---
> > +static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
> > +			    unsigned long parent_rate)
> > +{
> > +	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +	u32 val;
> > +	u32 divf;
> > +
> > +	divf = rate / (parent_rate * 2);
> > +
> > +	val = readl_relaxed(pll->base + PLL_CFG2);
> > +	val &= ~(PLL_DIVF_MASK << PLL_DIVF1_SHIFT);
> > +	val |= (divf - 1) << PLL_DIVF1_SHIFT;
> > +	writel_relaxed(val, pll->base + PLL_CFG2);
> > +
> > +	/* FIXME: PLL lock check */
> 
> Shouldn't be too hard to add, no?

Added to the next version which I intend to send today.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int clk_pll1_prepare(struct clk_hw *hw)
> > +{
> > +	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +	u32 val;
> > +
> > +	val = readl_relaxed(pll->base);
> > +	val &= ~(1 << PLL_PD);
> > +	writel_relaxed(val, pll->base);
> 
> pll->base + PLL_CFG0 please.

Same as above.

> 
> > +static const struct clk_ops clk_sccg_pll1_ops = {
> > +	.is_prepared	= clk_pll1_is_prepared,
> > +	.recalc_rate	= clk_pll1_recalc_rate,
> > +	.round_rate	= clk_pll1_round_rate,
> > +	.set_rate	= clk_pll1_set_rate,
> > +};
> > +
> > +static const struct clk_ops clk_sccg_pll2_ops = {
> > +	.prepare	= clk_pll1_prepare,
> > +	.unprepare	= clk_pll1_unprepare,
> > +	.recalc_rate	= clk_pll2_recalc_rate,
> > +	.round_rate	= clk_pll2_round_rate,
> > +	.set_rate	= clk_pll2_set_rate,
> > +};
> 
> So these are two PLLs that share the same enable register. Doing the
> prepare/unprepare for only one PLL can lead to all kinds of trouble.
> Finding a good abstraction the properly handles this case with the
> clock framework is probably also not easy.
> 
> I could imagine we'll need to track the enable state on both PLLs and
> only if both are disabled we disable it in hardware.
> 
> With the current code we disable the PLLs when all consumers are
> reparented to pll1, which probably has bad effects.
> 

So it took me a while to understand exactly why this needs to stay like it is.

The PLL1 is never used by any device, instead it is used as a source for PLL2.

But because the interlink between the two of them is too complicated,
the PLLs 1 and 2 need to be separate clocks.

As for the disabling, it's safe to disable just the second one because,
as mentioned above, the first PLL is never used alone by a device.

If there isn't anything else here to discuss, I would like to send the next
version of this patchset with all the other comments taken care of.

Abel

> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C5e9c7c6f9e464ed522f108d60994d471%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636706932168087639&amp;sdata=XbfFq5z%2FRtkq6yGgwSyk6dNCZVuXOzY%2Fiygmwle3C2I%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

--