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;
References: <20191206074052.15557-1-jian.hu@amlogic.com> <20191206074052.15557-3-jian.hu@amlogic.com>
User-agent: mu4e 1.3.3; emacs 26.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Jian Hu <jian.hu@amlogic.com>,
        Neil Armstrong <narmstrong@baylibre.com>
Cc:     Kevin Hilman <khilman@baylibre.com>,
        "Rob Herring" <robh@kernel.org>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Qiufang Dai <qiufang.dai@amlogic.com>,
        Jianxin Pan <jianxin.pan@amlogic.com>,
        Victor Wan <victor.wan@amlogic.com>,
        Chandle Zou <chandle.zou@amlogic.com>,
        linux-clk@vger.kernel.org, linux-amlogic@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
In-reply-to: <20191206074052.15557-3-jian.hu@amlogic.com>
Date:   Thu, 12 Dec 2019 11:16:11 +0100
Message-ID: <1j8snhluhg.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On Fri 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> wrote:

> The A1 PLL design is different with previous SoCs. The PLL
> internal analog modules Power-on sequence is different
> with previous, and thus requires a strict register sequence to
> enable the PLL.
>
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> ---
>  drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>  drivers/clk/meson/clk-pll.h |  1 +
>  drivers/clk/meson/parm.h    |  1 +
>  3 files changed, 23 insertions(+)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index ddb1e5634739..4aff31a51589 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -318,6 +318,23 @@ 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);
>  
> +	/*
> +	 * The A1 design is different with previous SoCs.The PLL
> +	 * internal analog modules Power-on sequence is different with
> +	 * previous, and thus requires a strict register sequence to
> +	 * enable the PLL.

The code does something more, not completly different. This comment is
not aligned with what the code does

> +	 */
> +	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> +		/* Enable the pll */
> +		meson_parm_write(clk->map, &pll->en, 1);
> +		udelay(10);
> +		/* Enable the pll self-adaption module current */
> +		meson_parm_write(clk->map, &pll->current_en, 1);
> +		udelay(40);
> +		meson_parm_write(clk->map, &pll->rst, 1);
> +		meson_parm_write(clk->map, &pll->rst, 0);

Here you enable the PLL and self adaptation module then reset the PLL.
However:
#1 when you enter this function, the PLL should already by in reset
and disabled
#2 the code after that will reset the PLL again

So if what you submited works, inserting the following should accomplish
the same thing:

---8<---
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 489092dde3a6..9b38df0a7682 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -330,6 +330,13 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
        /* Enable the pll */
        meson_parm_write(clk->map, &pll->en, 1);

+       if (MESON_PARM_APPLICABLE(&pll->current_en)) {
+               udelay(10);
+               /* Enable the pll self-adaption module current */
+               meson_parm_write(clk->map, &pll->current_en, 1);
+               udelay(40);
+       }
+
        /* Take the pll out reset */
        meson_parm_write(clk->map, &pll->rst, 0);
--->8---


> +	}
> +
>  	/* do nothing if the PLL is already enabled */
>  	if (clk_hw_is_enabled(hw))
>  		return 0;

In any case, nothing should be done on the clock before this check
otherwise you might just break the clock

> @@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>  
>  	/* Disable the pll */
>  	meson_parm_write(clk->map, &pll->en, 0);
> +
> +	/* Disable PLL internal self-adaption module current */
> +	if (MESON_PARM_APPLICABLE(&pll->current_en))
> +		meson_parm_write(clk->map, &pll->current_en, 0);
>  }
>  
>  static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
> index 367efd0f6410..30f039242a65 100644
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -36,6 +36,7 @@ struct meson_clk_pll_data {
>  	struct parm frac;
>  	struct parm l;
>  	struct parm rst;
> +	struct parm current_en;
>  	const struct reg_sequence *init_regs;
>  	unsigned int init_count;
>  	const struct pll_params_table *table;
> diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
> index 3c9ef1b505ce..c53fb26577e3 100644
> --- a/drivers/clk/meson/parm.h
> +++ b/drivers/clk/meson/parm.h
> @@ -20,6 +20,7 @@
>  	(((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
>  
>  #define MESON_PARM_APPLICABLE(p)		(!!((p)->width))
> +#define MESON_PARM_CURRENT(p)			(!!((p)->width))

Why do we need that ?

>  
>  struct parm {
>  	u16	reg_off;