Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
From: Jian Hu
Date: Fri Dec 20 2019 - 02:03:29 EST
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@xxxxxxxxxxx> wrote:
On 2019/12/12 18:16, Jerome Brunet wrote:
On Fri 06 Dec 2019 at 08:40, Jian Hu <jian.hu@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
ÂÂ 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;
.
.