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

[Sumit Garg]

On Mon, 26 Feb 2024 at 18:31, Marek Vasut <marex at denx.de> wrote:
>
> On 2/26/24 1:39 PM, Sumit Garg wrote:
> > 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.
>
> Try the CCM section 5.1.5.4.4 SSCG and Fractional PLLs I think,

That section also does mention following:

"The ARM PLL, GPU PLL, VPU PLL, DRAM PLL, Audio PLL1/2, and Video PLL1 are
fractional PLLs. The frequency on these can be tuned to be very
accurate to meet audio
and video interface requirements."

without any mention of HSIO PLL.

> this is
> likely PLL14xxx from Samsung, so
>
> f_out = f_in * M / P * 2^S
>
> I think f_in is something like 24 MHz, so 24 * 800 / 12 * 16 = 100 MHz
>
> P - predivider
> S - subdivider (output divider)
> M - multiplier

IMO, we shouldn't document something based on our analysis but rather
it should be based on facts.

>
> >>> +     /* 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.
>
> Linux does this in drivers/pmdomain/imx/imx8mp-blk-ctrl.c :
>
> 120 static void clk_hsio_pll_unprepare(struct clk_hw *hw)
> 121 {
> 122         struct clk_hsio_pll *clk = to_clk_hsio_pll(hw);
> 123
> 124         regmap_update_bits(clk->regmap, GPR_REG3, PLL_RST | PLL_CKE, 0);
> 125 }
>

And just above it:

static int clk_hsio_pll_prepare(struct clk_hw *hw)
{
...
        /* de-assert PLL reset */
        regmap_update_bits(clk->regmap, GPR_REG3, PLL_RST, PLL_RST);

        /* enable PLL */
        regmap_update_bits(clk->regmap, GPR_REG3, PLL_CKE, PLL_CKE);
...
}

That's an anti-pattern in the Linux kernel driver which we shouldn't
copy, right?

-Sumit

> [...]