Linux-Amlogic Archive on lore.kernel.org
 help / color / Atom feed
From: Jerome Brunet <jbrunet@baylibre.com>
To: Jian Hu <jian.hu@amlogic.com>, Neil Armstrong <narmstrong@baylibre.com>
Cc: Rob Herring <robh@kernel.org>,
	Victor Wan <victor.wan@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	devicetree@vger.kernel.org,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org, Stephen Boyd <sboyd@kernel.org>,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Chandle Zou <chandle.zou@amlogic.com>,
	linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v7 2/5] clk: meson: add support for A1 PLL clock ops
Date: Tue, 04 Feb 2020 11:24:11 +0100
Message-ID: <1jftfq7ir8.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <20200120034937.128600-3-jian.hu@amlogic.com>


On Mon 20 Jan 2020 at 04:49, Jian Hu <jian.hu@amlogic.com> wrote:

> Compared with the previous SoCs, self-adaption current module
> is newly added for A1, and there is no reset parm except the
> fixed pll. In A1 PLL, the PLL enable sequence is different, using
> the new power-on sequence to enable the PLL.

Things are getting clearer thanks to Martin's suggestions and I can
understand what your driver is doing now

However, I still have a problem with the fact that 2 different pll types
are getting intertwined in this driver. Parameters mandatory to one is
made optional to the other. Nothing clearly shows which needs what and
the combinatorial are quickly growing.

Apparently the only real difference is in enable/disable, So I would
prefer if the a1 had dedicated function for these ops.

I suppose you'll have to submit clk_hw_enable() and clk_hw_disable()
to the framework to call the appropriate ops dependind on the SoC.

>
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/clk-pll.c | 47 +++++++++++++++++++++++++++++++------
>  drivers/clk/meson/clk-pll.h |  2 ++
>  2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index ddb1e5634739..10926291440f 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -283,10 +283,14 @@ static void meson_clk_pll_init(struct clk_hw *hw)
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
>  	if (pll->init_count) {
> -		meson_parm_write(clk->map, &pll->rst, 1);
> +		if (MESON_PARM_APPLICABLE(&pll->rst))
> +			meson_parm_write(clk->map, &pll->rst, 1);
> +

replace by
        enabled = clk_hw_is_enabled(hw)
        if (enabled)
           clk_hw_disable(hw)

>  		regmap_multi_reg_write(clk->map, pll->init_regs,
>  				       pll->init_count);
> -		meson_parm_write(clk->map, &pll->rst, 0);
> +
> +		if (MESON_PARM_APPLICABLE(&pll->rst))
> +			meson_parm_write(clk->map, &pll->rst, 0);

       /* restore if necessary */
       if (enabled)
          clk_hw_enable(hw)

>  	}
>  }
>  
> @@ -295,8 +299,11 @@ 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) ||
> -	    !meson_parm_read(clk->map, &pll->en) ||
> +	if (MESON_PARM_APPLICABLE(&pll->rst) &&
> +	    meson_parm_read(clk->map, &pll->rst))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->en) ||
>  	    !meson_parm_read(clk->map, &pll->l))
>  		return 0;

I suppose the pll can't be locked if it was in reset, so we could drop
the check on `rst` entirely to simplify the function

>  
> @@ -323,13 +330,34 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  		return 0;
>  
>  	/* Make sure the pll is in reset */
> -	meson_parm_write(clk->map, &pll->rst, 1);
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 1);
>  
>  	/* Enable the pll */
>  	meson_parm_write(clk->map, &pll->en, 1);
>  
>  	/* Take the pll out reset */
> -	meson_parm_write(clk->map, &pll->rst, 0);
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 0);
> +
> +	/*
> +	 * Compared with the previous SoCs, self-adaption current module
> +	 * is newly added for A1, keep the new power-on sequence to enable the
> +	 * PLL. The sequence is:
> +	 * 1. enable the pll, delay for 10us
> +	 * 2. enable the pll self-adaption current module, delay for 40us
> +	 * 3. enable the lock detect module
> +	 */
> +	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> +		udelay(10);
> +		meson_parm_write(clk->map, &pll->current_en, 1);
> +		udelay(40);
> +	};
> +
> +	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
> +		meson_parm_write(clk->map, &pll->l_detect, 1);
> +		meson_parm_write(clk->map, &pll->l_detect, 0);
> +	}
>  
>  	if (meson_clk_pll_wait_lock(hw))
>  		return -EIO;
> @@ -343,10 +371,15 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
>  	/* Put the pll is in reset */
> -	meson_parm_write(clk->map, &pll->rst, 1);
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 1);
>  
>  	/* Disable the pll */
>  	meson_parm_write(clk->map, &pll->en, 0);
> +
> +	/* Disable PLL internal self-adaption current module */
> +	if (MESON_PARM_APPLICABLE(&pll->current_en))
> +		meson_parm_write(clk->map, &pll->current_en, 0);
>  }

With the above clarified, it should be easy to properly split the
functions between the legacy type and the a1 type.

You'll need to update meson_clk_pll_set_rate() to call
 - clk_hw_is_enabled()
 - clk_hw_enable() and clk_hw_disable() (again, you'll need to add
 those in the framework first)

>  
>  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..a2228c0fdce5 100644
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -36,6 +36,8 @@ struct meson_clk_pll_data {
>  	struct parm frac;
>  	struct parm l;
>  	struct parm rst;
> +	struct parm current_en;
> +	struct parm l_detect;
>  	const struct reg_sequence *init_regs;
>  	unsigned int init_count;
>  	const struct pll_params_table *table;


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-20  3:49 [PATCH v7 0/5] add Amlogic A1 clock controller driver Jian Hu
2020-01-20  3:49 ` [PATCH v7 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2020-01-21 22:00   ` Rob Herring
2020-02-04  3:12     ` Jian Hu
2020-01-20  3:49 ` [PATCH v7 2/5] clk: meson: add support for A1 PLL clock ops Jian Hu
2020-02-04 10:24   ` Jerome Brunet [this message]
2020-02-10  6:11     ` Jian Hu
2020-02-18  9:05       ` Jerome Brunet
2020-01-20  3:49 ` [PATCH v7 3/5] clk: meson: a1: add support for Amlogic A1 PLL clock driver Jian Hu
2020-02-04 13:13   ` Jerome Brunet
2020-02-10  6:16     ` Jian Hu
2020-01-20  3:49 ` [PATCH v7 4/5] dt-bindings: clock: meson: add A1 peripheral clock controller bindings Jian Hu
2020-01-29  5:44   ` Stephen Boyd
2020-01-20  3:49 ` [PATCH v7 5/5] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver Jian Hu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1jftfq7ir8.fsf@starbuckisacylon.baylibre.com \
    --to=jbrunet@baylibre.com \
    --cc=chandle.zou@amlogic.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jian.hu@amlogic.com \
    --cc=jianxin.pan@amlogic.com \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=mturquette@baylibre.com \
    --cc=narmstrong@baylibre.com \
    --cc=qiufang.dai@amlogic.com \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=victor.wan@amlogic.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org
	public-inbox-index linux-amlogic

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git