Re: [PATCH v9 3/5] clk: imx: add SCCG PLL type

[Abel Vesa <abel.vesa@nxp.com>]

On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-11-07 03:54:45)
> > On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> > > Quoting Abel Vesa (2018-09-24 03:39:55)
> > > > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > > > +                                        unsigned long parent_rate)
> > > > +{
> > > > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > > > +       u32 val, ref, divr1, divf1, divr2, divf2;
> > > > +       u64 temp64;
> > > > +
> > > > +       val = readl_relaxed(pll->base + PLL_CFG0);
> > > > +       switch (FIELD_GET(PLL_REF_MASK, val)) {
> > > > +       case 0:
> > > > +               ref = OSC_25M;
> > > > +               break;
> > > > +       case 1:
> > > > +               ref = OSC_27M;
> > > > +               break;
> > > > +       default:
> > > > +               ref = OSC_25M;
> > > 
> > > Does this information not come through 'parent_rate'?
> > > 
> > 
> > No. So basically both pll1 and pll2 and the divider after it form together this SCCG:
> > 
> > https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
> > 
> > See: Figure 5-8. SSCG PLL Block Diagram
> 
> Thanks for the link!
> 
> > 
> > We're basically reading the input of the pll 1 in order to compute the output of the entire SCCG.
> > 
> > I know it's a mess. I'm working on cleaning it up, but for now we need this in in order to boot up.
> 
> What's the plan to clean it up?

So I'm doing this in our internal tree first to make sure I don't break the
other (newer) socs.

I already have a prototype in testing but it's a long way to upstream it.

Basically, I'm replacing all of this with a single, more like a composite,
more complex, clock type that does all the magic inside.

One of the problems is the fact that the bypasses can have the same sources
and in my case, I'm implementing that as a list of parents name, but the
parent names list doesn't work with duplicates, so I have to find some other
way to do it.

Once I have something clean and tested enough I'll send it upstream.

> 
> > 
> > > > +               break;
> > > > +       }
> > > > +
> > > > +       val = readl_relaxed(pll->base + PLL_CFG2);
> > > > +       divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > > > +       divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > > > +       divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > > > +       divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > > > +
> > > > +       temp64 = ref * 2;
> > > > +       temp64 *= (divf1 + 1) * (divf2 + 1);
> > > > +
> > > > +       do_div(temp64, (divr1 + 1) * (divr2 + 1));
> > > 
> > > Nitpicks: A comment with the equation may be helpful to newcomers.
> > 
> > Since the SCCG is contructed by multiple different types of clocks here, the equation doesn't help
> > since it is spread in all constructing blocks.
> 
> Ok.
> 

--