| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1jsef0ydze.fsf@starbuckisacylon.baylibre.com>
Date: Thu, 30 Oct 2025 09:45:41 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Chuan Liu <chuan.liu@...ogic.com>
Cc: Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@...nel.org>, Neil
Armstrong <neil.armstrong@...aro.org>, Michael Turquette
<mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Kevin Hilman
<khilman@...libre.com>, Martin Blumenstingl
<martin.blumenstingl@...glemail.com>, linux-amlogic@...ts.infradead.org,
linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] clk: amlogic: Optimize PLL enable timing
On Wed 22 Oct 2025 at 22:07, Chuan Liu <chuan.liu@...ogic.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@...nel.org> wrote:
>>
>>> From: Chuan Liu <chuan.liu@...ogic.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@...ogic.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
Powered by blists - more mailing lists