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

[Marek Vasut]

On 2/26/24 2:43 PM, Sumit Garg wrote:
> 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

See ^

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

Frankly, all the PLLs in the MX8M are very much similar, so either wait 
for someone from NXP to confirm this, or try and find some statement in 
the MX8MPRM .

>>>>> +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?

This is the enable part which is done in sequence, the part I quoted is 
the disable part.