Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <b1d223c2d2a9229cfb27275c948a7ff4b3aef93d.camel@baylibre.com>
Subject: Re: [RFC v1 3/7] clk: meson: clk-pll: check if the clock is already
 enabled
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        narmstrong@baylibre.com
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        mturquette@baylibre.com, sboyd@kernel.org
Date:   Thu, 15 Nov 2018 10:14:13 +0100
In-Reply-To: <20181114225725.2821-4-martin.blumenstingl@googlemail.com>
References: <20181114225725.2821-1-martin.blumenstingl@googlemail.com>
         <20181114225725.2821-4-martin.blumenstingl@googlemail.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.2 (3.30.2-2.fc29) 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@googlemail.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@baylibre.com>