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
> + */
> + 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;
