linux-kernel - Re: [PATCH] clk: meson: pll: Update the meson_clk_pll

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1jy13hxwp2.fsf@starbuckisacylon.baylibre.com>
Date: Tue, 24 Sep 2024 10:50:01 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@...nel.org>
Cc: 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>,  chuan.liu@...ogic.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 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 ...

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

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.

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