Message-ID: <1jtte5xjgk.fsf@starbuckisacylon.baylibre.com>
Date: Tue, 24 Sep 2024 15:35:55 +0200
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] clk: meson: pll: Update the meson_clk_pll_init
 execution judgment logic

On Tue 24 Sep 2024 at 18:27, Chuan Liu <chuan.liu@...ogic.com> wrote:

> On 2024/9/24 16:50, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Fri 20 Sep 2024 at 16:13, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@...nel.org> wrote:
>>
>>> From: Chuan Liu <chuan.liu@...ogic.com>
>>>
>>> The hardware property of PLL determines that PLL can only be enabled
>>> after PLL has been initialized. If PLL is not initialized, the
>>> corresponding lock bit will not be set to 1, resulting in
>>> meson_clk_pll_is_enabled() returning "false".
>>>
>>> Therefore, if PLL is already enabled, there is no need to repeat
>>> initialization, and the judgment "CLK_MESON_PLL_NOINIT_ENABLED" in
>>> meson_clk_pll_init() appears redundant.
>> Apparently you messed something up with b4 ...
>
>
> emmmm... I'm not familiar with this tool😂
>
>
>>> ---
>>> The hardware property of PLL determines that PLL can only be enabled
>>> after PLL has been initialized. If PLL is not initialized, the
>>> corresponding lock bit will not be set to 1, resulting in
>>> meson_clk_pll_is_enabled() returning "false".
>>>
>>> Therefore, if PLL is already enabled, there is no need to repeat
>>> initialization, and the judgment "CLK_MESON_PLL_NOINIT_ENABLED" in
>>> meson_clk_pll_init() appears redundant.
>
>
> You have a point, but we do get this kind of situation all the time:
>
> For example, hifi_pll provides a clock for audio, which needs to be
> configured
>
> in the bootloader phase in order to play audio as soon as possible after
> boot.
>
> After entering the kernel, the hifi_pll frequency may be dynamically
> adjusted

Whatever was feeding the audio fifo is gone by that stage and fifo, tdm
and everything will be reset as soon the audio drivers are probed.
I hardly see how keeping the PLL init through boot could apply to audio,
especially with the use of assigned-rate in DT.

>
> (to match the audio bit rate/audio and video synchronization, etc.). The
> gp_pll
>
> that provides the clock for eMMC and the hdmi_pll that provides the clock
> for
>
> HDMI are all configured during the bootloader phase and cannot be configured
>
> as RO in the kernel.

HDMI, you presumably want to avoid a glitch in video until you're ready
to reconfigure the pipeline. That would be a valid use-case for
CLK_MESON_PLL_NOINIT_ENABLED.

>
>
> My idea is to still describe the init_regs information in the kernel in the
> driver:
>
> 1) If the bootloader is not enabled, the PLL will be judged as unused
> during the
>
> bootloader phase, and then enter the kernel for initialization.
>
> 2) If the bootloader has enabled PLL, in order to ensure clock continuity
> after
>
> entering the kernel, it will not repeat initialization (re-initialization
> may cause the
>
> module that references PLL to work abnormally).

I understood your idea the first time around. I still do not agree.
Use CLK_MESON_PLL_NOINIT_ENABLED if you must keep an enabled PLL and
justify the use with a proper comment. For eMMC I'm not convinced.

>
> Can the coupling between bootloader and kernel be avoided on the premise of
>
> ensuring functional integrity.
>
>
>> If the PLL is enabled, it has been initiallized, to some extent
>> yes. However we have no idea what the setting was. In general, I really
>> don't like inheriting settings from bootloader. It brings all sorts of
>> issues depending on the bootloader origin and version used by the
>> specific platform.
>>
>> So in general a PLL should be re-initialized when possible. When it is
>> not possible, in most case it means the PLL should be RO and linux
>> should just use it.
>>
>> Someone brought a specific case in between, where they needed to keep
>> the PLL on boot, but still be able to relock it later on. The flag
>> properly identify those PLLs. Much like CLK_IS_CRITICAL or
>> CLK_IGNORE_UNUSED, each usage shall be properly documented.
>>
>>> In actual application scenarios, PLL configuration is determined during
>>> the bootloader phase. If PLL has been configured during the bootloader
>>> phase, you need to add a flag to the kernel to avoid PLL
>>> re-initialization, which will increase the coupling between the kernel
>>> and the bootloader.
>> The vast majority of those PLL should be RO then.
>> If you can relock it, you should be able to re-init it as well.
>
>
> re-init may cause glitch in the PLL, which affects module work at later PLL
> levels.
>
>
>>> Signed-off-by: Chuan Liu <chuan.liu@...ogic.com>
>>> ---
>>>   drivers/clk/meson/clk-pll.c | 3 +--
>>>   drivers/clk/meson/clk-pll.h | 1 -
>>>   2 files changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>> index 89f0f04a16ab..8df2add40b57 100644
>>> --- a/drivers/clk/meson/clk-pll.c
>>> +++ b/drivers/clk/meson/clk-pll.c
>>> @@ -316,8 +316,7 @@ static int meson_clk_pll_init(struct clk_hw *hw)
>>>         * Keep the clock running, which was already initialized and enabled
>>>         * from the bootloader stage, to avoid any glitches.
>>>         */
>>> -     if ((pll->flags & CLK_MESON_PLL_NOINIT_ENABLED) &&
>>> -         meson_clk_pll_is_enabled(hw))
>>> +     if (meson_clk_pll_is_enabled(hw))
>>>                return 0;
>> I'm not OK with this.
>>
>>>        if (pll->init_count) {
>>> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
>>> index 949157fb7bf5..cccbf52808b1 100644
>>> --- a/drivers/clk/meson/clk-pll.h
>>> +++ b/drivers/clk/meson/clk-pll.h
>>> @@ -28,7 +28,6 @@ struct pll_mult_range {
>>>        }
>>>
>>>   #define CLK_MESON_PLL_ROUND_CLOSEST  BIT(0)
>>> -#define CLK_MESON_PLL_NOINIT_ENABLED BIT(1)
>>>
>>>   struct meson_clk_pll_data {
>>>        struct parm en;
>>>
>>> ---
>>> base-commit: 0ef513560b53d499c824b77220c537eafe1df90d
>>> change-id: 20240918-optimize_pll_flag-678a88d23f82
>>>
>>> Best regards,
>> --
>> Jerome

-- 
Jerome

Powered by blists - more mailing lists