Re: [PATCH v6 3/5] clk: imx: add SCCG PLL type
From: Abel Vesa
Date: Tue Aug 28 2018 - 06:58:32 EST
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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxx>
> > ---
> > +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&data=02%7C01%7Cabel.vesa%40nxp.com%7C5e9c7c6f9e464ed522f108d60994d471%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636706932168087639&sdata=XbfFq5z%2FRtkq6yGgwSyk6dNCZVuXOzY%2Fiygmwle3C2I%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--