[PATCH v2 4/8] imx8mp: power-domain: Expose high performance PLL clock

[Sumit Garg]

On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex at denx.de> wrote:
>
> On 2/26/24 9:04 AM, Sumit Garg wrote:
> > Expose the high performance PLL as a regular Linux clock, so the
> > PCIe PHY can use it when there is no external refclock provided.
> >
> > Inspired from counterpart Linux kernel v6.8-rc3 driver:
> > drivers/pmdomain/imx/imx8mp-blk-ctrl.c
>
> Commit ID, please, see previous comments in this series.
>
> [...]
>
> > +static int hsio_pll_clk_enable(struct clk *clk)
> > +{
> > +     void *base = (void *)dev_get_driver_data(clk->dev);
> > +     u32 val;
> > +     int ret;
> > +
> > +     /* Setup HSIO PLL */
> > +     clrsetbits_le32(base + GPR_REG2,
> > +                     P_PLL_MASK | M_PLL_MASK | S_PLL_MASK,
> > +                     FIELD_PREP(P_PLL_MASK, 12) |
> > +                     FIELD_PREP(M_PLL_MASK, 800) |
> > +                     FIELD_PREP(S_PLL_MASK, 4));
>
> These magic numbers 12, 800, 4 could use explanation (why these
> numbers?) and a dedicated macro .
>

I can't find any information in TRM as to how HSIO PLL computation is
done. However, I can extend the comment above to say that this
configuration leads to a 100 MHz PHY clock as per
clk_hsio_pll_recalc_rate() in Linux kernel driver.

> > +     /* de-assert PLL reset */
> > +     setbits_le32(base + GPR_REG3, PLL_RST);
> > +
> > +     /* enable PLL */
> > +     setbits_le32(base + GPR_REG3, PLL_CKE);
> > +
> > +     /* Check if PLL is locked */
> > +     ret = readl_poll_sleep_timeout(base + GPR_REG1, val,
> > +                                    val & PLL_LOCK, 10, 100000);
> > +     if (ret)
> > +             dev_err(clk->dev, "failed to lock HSIO PLL\n");
> > +
> > +     return ret;
> > +}
> > +
> > +static int hsio_pll_clk_disable(struct clk *clk)
> > +{
> > +     void *base = (void *)dev_get_driver_data(clk->dev);
> > +
> > +     clrbits_le32(base + GPR_REG3, PLL_CKE);
> > +     clrbits_le32(base + GPR_REG3, PLL_RST);
>
> Can you clear both at once, or do they have to be cleared in sequence ?

I generally follow the reverse sequence of how the configuration is
done during hsio_pll_clk_enable(). These bits have seperate functions
related to clock and reset. So I would prefer to keep them as they
are.

>
> [...]
>
> The series is starting to look much better compared to V1 btw , these ^
> are only nitpicks .

Thanks for your detailed review, I appreciate it.

-Sumit