From: Jian Hu <jian.hu@amlogic.com>
To: Jerome Brunet <jbrunet@baylibre.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 v4 2/6] clk: meson: add support for A1 PLL clock ops
Date: Fri, 20 Dec 2019 15:03:52 +0800 [thread overview]
Message-ID: <627a8d8b-f049-4cdd-6dd0-5c5acc58a90c@amlogic.com> (raw)
In-Reply-To: <0bc6176f-c0b8-5c31-4c6b-d3686eefe56e@amlogic.com>
Hi jerome
On 2019/12/18 16:37, Jian Hu wrote:
>
>
> On 2019/12/17 17:29, Jerome Brunet wrote:
>>
>> On Tue 17 Dec 2019 at 09:41, Jian Hu <jian.hu@amlogic.com> wrote:
>>
>>> 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.
>>>
>>
>> oh my ! What is it then ? Why do you need to toggle it ? What does is
>> do ?
>>
> The PLL enable flow:
> step1: enable the PLL
> step2: enable the self adaptation module
> step3: reset the lock detect module, let the lock detect module
> work,And then the PLL will work.
>
> Toggle the bit 6 in CTRL2 can reset the lock detect module.
#1 I intend to introduce a new PARM 'l_detect', and the
meson_clk_pll_enable is blow. Please help to review it.
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 (clk_hw_is_enabled(hw))
return 0;
/*
* Compared with the previous SoCs, self-adaption module current
* is newly added, keep the new power-on sequence to enable the
* PLL.
*/
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);
/* Enable lock detect module */
meson_parm_write(clk->map, &pll->l_detect, 1);
meson_parm_write(clk->map, &pll->l_detect, 0);
goto out;
}
/* Make sure the pll is in reset */
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);
out:
if (meson_clk_pll_wait_lock(hw))
return -EIO;
return 0;
}
#2 Add check for PARM 'rst' when
writing to it, the same with frac.
if (MESON_PARM_APPLICABLE(&pll->rst))
meson_parm_write(clk->map, &pll->rst, 1);
And I have tested it for it, The periphs and hifi pll works well.
Or any good idea about it?
>>> 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?
>>
>> This is not the point of my comment Jian !
>>
>> I'm assuming here that you have tested your v4 before sending and that
>> it work (hopefully)
>>
> Yes, it works wells. I have tested the drivers before sending every
> patchset version.
>> The fact is that with this code, when disabled the bit behind rst
>> (whatever it is) is set. So when you get to enable the bit is already
>> set.
>> The code you sent does the same as the snip I gave you in the reply.
>>
>> Now, if your PLL is THAT different, maybe it would be best if you could
>> clearly explain how it works, what bit should be set and why. Then we
>> will be able to figure out how the driver has to be restructured.
>>
> the same as 'The PLL enable flow' above
>>>
>>>>
>>>> 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;
>>>>
>>>> .
>>>>
>>
>> .
>>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
next prev parent reply other threads:[~2019-12-20 7:03 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
2019-12-17 9:29 ` Jerome Brunet
2019-12-18 8:37 ` Jian Hu
2019-12-20 7:03 ` Jian Hu [this message]
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=627a8d8b-f049-4cdd-6dd0-5c5acc58a90c@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).