Re: [PATCH 2/3] clk: amlogic: Optimize PLL enable timing

[Jerome Brunet <jbrunet@baylibre.com>]

On Wed 22 Oct 2025 at 22:07, Chuan Liu <chuan.liu@amlogic.com> wrote:

> Hi Jerome,
>
>
> On 10/22/2025 8:01 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Wed 22 Oct 2025 at 14:58, Chuan Liu via B4 Relay
>> <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>> 
>>> From: Chuan Liu <chuan.liu@amlogic.com>
>>>
>>> Amlogic PLL locking procedure shall follow this timing sequence:
>>> 1 Assert reset signal: Ensures PLL circuits enter known initial state.
>>> 2 Deassert lock-detect signal: Avoid lock signal false triggering.
>>> 3 Assert enable signal: Powers up PLL supply.
>>> 4 udelay(20): Wait for Bandgap and LDO to power up and stabilize.
>>> 5 Enable self-adaptation current module (Optional).
>>> 6 Deassert reset signal: Releases PLL to begin normal operation.
>>> 7 udelay(20): Wait for PLL loop stabilization.
>>> 8 Assert lock-detect signal: lock detection circuit starts to work.
>>> 9 Monitor lock status signal: Wait for PLL lock completion.
>>> 10 If the PLL fails to lock, it should be disabled, This makes the
>>> logic more complete, and also helps save unnecessary power consumption
>>> when the PLL is malfunctioning.
>> Is this applicable to all supported SoC ? from meson8 to s4 ?
>
> Yes.
>
>> What did you test ?
>
> We have tested this on the G12A and later SoCs without any issues.
> Internally, we have adopted this configuration sequence in our branch
> and verified it on a large number of SoCs.
>
> We haven't maintained the meson series for a while, so it hasn't been
> included in our recent validation. I also don't have any meson boards
> on hand. If you have one available, it would be appreciated if you
> could help verify this, Thanks.

It's very interresting that you ask the community to test and validate
chips sold by your compagny, chip that are still sold *today* and readility
available on most market places.

I'd be happy to forward you a few links if that helps.

>
> This PLL sequence adjustment for meson SoCs adds a 20us delay after
> enabling the PLL and after releasing its reset. The delay addresses
> rare PLL lock failures observed under low temperatures. As a result,
> it slightly increases enable time but improves stability.
>
>> 
>>>
>>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>>> ---
>>>   drivers/clk/meson/clk-pll.c | 68 ++++++++++++++++++++++++++-------------------
>>>   1 file changed, 40 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>> index b07e1eb19d12..26c83db487e8 100644
>>> --- a/drivers/clk/meson/clk-pll.c
>>> +++ b/drivers/clk/meson/clk-pll.c
>>> @@ -353,6 +353,23 @@ static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
>>>        return -EIO;
>>>   }
>>>
>>> +static void meson_clk_pll_disable(struct clk_hw *hw)
>>> +{
>>> +     struct clk_regmap *clk = to_clk_regmap(hw);
>>> +     struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>> +
>>> +     /* Put the pll is in reset */
>>> +     if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +             meson_parm_write(clk->map, &pll->rst, 1);
>>> +
>>> +     /* Disable the pll */
>>> +     meson_parm_write(clk->map, &pll->en, 0);
>>> +
>>> +     /* Disable PLL internal self-adaption current module */
>>> +     if (MESON_PARM_APPLICABLE(&pll->current_en))
>>> +             meson_parm_write(clk->map, &pll->current_en, 0);
>>> +}
>> I don't get why you moved that code around and make the diff even harder
>> to read
>
> Sorry about the misaligned formatting. I moved meson_clk_pll_disable()
> earlier in the code because I added handling for PLL lock failures,
> which unintentionally broke the alignment.
>
> I'll split the PLL lock failure handling into a separate patch in the
> next version to make review easier.
>
>> 
>>> +
>>>   static int meson_clk_pll_enable(struct clk_hw *hw)
>>>   {
>>>        struct clk_regmap *clk = to_clk_regmap(hw);
>>> @@ -366,53 +383,48 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>>        if (MESON_PARM_APPLICABLE(&pll->rst))
>>>                meson_parm_write(clk->map, &pll->rst, 1);
>>>
>>> +     /* Disable the PLL lock-detect module */
>>> +     if (MESON_PARM_APPLICABLE(&pll->l_detect))
>>> +             meson_parm_write(clk->map, &pll->l_detect, 1);
>>> +
>>>        /* Enable the pll */
>>>        meson_parm_write(clk->map, &pll->en, 1);
>>> -
>>> -     /* Take the pll out reset */
>>> -     if (MESON_PARM_APPLICABLE(&pll->rst))
>>> -             meson_parm_write(clk->map, &pll->rst, 0);
>>> +     /* Wait for Bandgap and LDO to power up and stabilize */
>>> +     udelay(20);
>>>
>>>        /*
>>>         * Compared with the previous SoCs, self-adaption current module
>>>         * is newly added for A1, keep the new power-on sequence to enable the
>>>         * PLL. The sequence is:
>>> -      * 1. enable the pll, delay for 10us
>>> +      * 1. enable the pll, ensure a minimum delay of 10μs
>>>         * 2. enable the pll self-adaption current module, delay for 40us
>>>         * 3. enable the lock detect module
>>>         */
>>>        if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>>> -             udelay(10);
>>>                meson_parm_write(clk->map, &pll->current_en, 1);
>>> -             udelay(40);
>>> -     }
>>> -
>>> -     if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
>>> -             meson_parm_write(clk->map, &pll->l_detect, 1);
>>> -             meson_parm_write(clk->map, &pll->l_detect, 0);
>>> +             udelay(20);
>>>        }
>>>
>>> -     if (meson_clk_pll_wait_lock(hw))
>>> -             return -EIO;
>>> +     /* Take the pll out reset */
>>> +     if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +             meson_parm_write(clk->map, &pll->rst, 0);
>>>
>>> -     return 0;
>>> -}
>>> +     /* Wait for PLL loop stabilization */
>>> +     udelay(20);
>>>
>>> -static void meson_clk_pll_disable(struct clk_hw *hw)
>>> -{
>>> -     struct clk_regmap *clk = to_clk_regmap(hw);
>>> -     struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>> +     /* Enable the lock-detect module */
>>> +     if (MESON_PARM_APPLICABLE(&pll->l_detect))
>>> +             meson_parm_write(clk->map, &pll->l_detect, 0);
>>>
>>> -     /* Put the pll is in reset */
>>> -     if (MESON_PARM_APPLICABLE(&pll->rst))
>>> -             meson_parm_write(clk->map, &pll->rst, 1);
>>> +     if (meson_clk_pll_wait_lock(hw)) {
>>> +             /* disable PLL when PLL lock failed. */
>>> +             meson_clk_pll_disable(hw);
>>> +             pr_warn("%s: PLL lock failed!!!\n", clk_hw_get_name(hw));
>>>
>>> -     /* Disable the pll */
>>> -     meson_parm_write(clk->map, &pll->en, 0);
>>> +             return -EIO;
>>> +     }
>>>
>>> -     /* Disable PLL internal self-adaption current module */
>>> -     if (MESON_PARM_APPLICABLE(&pll->current_en))
>>> -             meson_parm_write(clk->map, &pll->current_en, 0);
>>> +     return 0;
>>>   }
>>>
>>>   static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>> --
>> Jerome

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic