linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jian Hu <jian.hu@amlogic.com>
To: Jerome Brunet <jbrunet@baylibre.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
Date: Tue, 17 Dec 2019 16:41:25 +0800	[thread overview]
Message-ID: <741284be-2ae8-1102-22bc-c510e822c883@amlogic.com> (raw)
In-Reply-To: <1j8snhluhg.fsf@starbuckisacylon.baylibre.com>



On 2019/12/12 18:16, Jerome Brunet wrote:
> 
> 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
ok, I will correct the comment.
> 
>> +	 */
>> +	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
For A1 PLLs, There is no reset bit, It will not reset the PLL.
And in V2, you mentioned PARM 'rst' can be used for one toggling, And 
'rst' is used for BIT(6) in CTRL2.

Quote V2 the HIFI PLL init_regs definition:


+static const struct reg_sequence a1_hifi_init_regs[] = {
+	{ .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f18440, .delay_us = 10 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f18440, .delay_us = 40 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
+};

So maybe another new PARM should be defined to avoid the ambiguity.
What do you think about it?

> 
> 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
> 
OK, I will put the enabled check ahead.
>> @@ -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 ?
OK, I will remove it ,and use 'MESON_PARM_APPLICABLE' instead
> 
>>   
>>   struct parm {
>>   	u16	reg_off;
> 
> .
> 

  reply	other threads:[~2019-12-17  8:40 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-06  7:40 [PATCH v4 0/6] add Amlogic A1 clock controller driver Jian Hu
2019-12-06  7:40 ` [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2019-12-12  9:57   ` Jerome Brunet
2019-12-13  9:35     ` Jian Hu
2019-12-13 10:38   ` Maxime Ripard
2019-12-18  8:00     ` Jian Hu
2019-12-18 12:57       ` Maxime Ripard
2019-12-06  7:40 ` [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops Jian Hu
2019-12-12 10:16   ` Jerome Brunet
2019-12-17  8:41     ` Jian Hu [this message]
2019-12-17  9:29       ` Jerome Brunet
2019-12-18  8:37         ` Jian Hu
2019-12-20  7:03           ` Jian Hu
2019-12-06  7:40 ` [PATCH v4 3/6] clk: meson: eeclk: refactor eeclk common driver to support A1 Jian Hu
2019-12-12 10:19   ` Jerome Brunet
2019-12-17  9:36     ` Jian Hu
2019-12-06  7:40 ` [PATCH v4 4/6] clk: meson: a1: add support for Amlogic A1 PLL clock driver Jian Hu
2019-12-12 10:28   ` Jerome Brunet
2019-12-12 10:42     ` Jerome Brunet
2019-12-18  6:01     ` Jian Hu
2019-12-06  7:40 ` [PATCH v4 5/6] dt-bindings: clock: meson: add A1 peripheral clock controller bindings Jian Hu
2019-12-12  9:59   ` Jerome Brunet
2019-12-18  7:00     ` Jian Hu
2019-12-06  7:40 ` [PATCH v4 6/6] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver Jian Hu
2019-12-12 11:01   ` Jerome Brunet
2019-12-18  7:20     ` 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=741284be-2ae8-1102-22bc-c510e822c883@amlogic.com \
    --to=jian.hu@amlogic.com \
    --cc=chandle.zou@amlogic.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jbrunet@baylibre.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).