linux-kernel - Re: [RFC v1 3/7] clk: meson: clk-pll: check if the clock is already enabled

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]

Date:   Thu, 15 Nov 2018 10:14:13 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        linux-amlogic@...ts.infradead.org, linux-clk@...r.kernel.org,
        narmstrong@...libre.com
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        mturquette@...libre.com, sboyd@...nel.org
Subject: Re: [RFC v1 3/7] clk: meson: clk-pll: check if the clock is already
 enabled

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> Since commit 6f888e7bc7bd58 ("clk: meson: clk-pll: add enable bit") our
> PLLs also support the "enable" bit. Currently meson_clk_pll_enable
> unconditionally resets the PLL, enables it, takes it out of reset and
> waits until it is locked.
> 
> This works fine for our current clock trees. However, there will be a
> problem once we allow modifications to sys_pll on Meson8, Meson8b and
> Meson8m2 (which will be required for CPU frequency scaling):
> the CPU clock is derived from the sys_pll clock. Once clk_enable is
> called on the CPU clock this will be propagated by the common clock
> framework up until the sys_pll clock. If we reset the PLL
> unconditionally in meson_clk_pll_enable the CPU will be stopped (on
> Meson8, Meson8b and Meson8m2).
> To prevent this we simply check if the PLL is already enabled and do
> reset the PLL if it's already enabled and locked.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
> ---
>  drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index f5b5b3fabe3c..b46cca953f4f 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -200,11 +200,32 @@ static void meson_clk_pll_init(struct clk_hw *hw)
>  	}
>  }
>  
> +static int meson_clk_pll_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> +
> +	if (meson_parm_read(clk->map, &pll->rst))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->en))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->l))
> +		return 0;

Could you use an OR instead of these 3 seperate checks ?

> +
> +	return 1;
> +}
> +
>  static int meson_clk_pll_enable(struct clk_hw *hw)
>  {
>  	struct clk_regmap *clk = to_clk_regmap(hw);
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
> +	/* do nothing if the PLL is already enabled */
> +	if (meson_clk_pll_is_enabled(hw))
> +		return 0;
> +
>  	/* Make sure the pll is in reset */
>  	meson_parm_write(clk->map, &pll->rst, 1);
> 
>  

With the small comment above taken care of, it makes perfect sense
and it will be valuable to other PLLs, Thx Martin !

Reviewed-by: Jerome Brunet <jbrunet@...libre.com>