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 12:11:13 -0700
From:   Andrey Smirnov <andrew.smirnov@...il.com>
To:     Abel Vesa <abel.vesa@....com>
Cc:     Sascha Hauer <s.hauer@...gutronix.de>,
        Lucas Stach <l.stach@...gutronix.de>,
        Sascha Hauer <kernel@...gutronix.de>,
        Dong Aisheng <aisheng.dong@....com>,
        Fabio Estevam <fabio.estevam@....com>,
        Anson Huang <anson.huang@....com>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, sboyd@...nel.org,
        Michael Turquette <mturquette@...libre.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, abelvesa@...ux.com,
        linux-imx@....com, Shawn Guo <shawnguo@...nel.org>,
        linux-clk@...r.kernel.org
Subject: Re: [PATCH v6 3/5] clk: imx: add SCCG PLL type

On Tue, Aug 28, 2018 at 3:58 AM Abel Vesa <abel.vesa@....com> wrote:
>
> 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.
>

IMHO this means that, if nothing else, all of the below should be
documented in code as a comment, otherwise simpletons like me are
going to continue stumbling over it and wondering what's going on.

> 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.
>

Can you go a little bit more into detail as for why PLL1 needs to be
exposed in the first place and can't just be dealt with behind the
scenes as a part of PLL2 abstraction? Are there use-cases where the
rates of the two are going to be adjusted individually in Linux?

Thanks,
Andrey Smirnov