| Message ID | 20191227094606.143637-3-jian.hu@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | add Amlogic A1 clock controller driver | expand |
Hi Jian, On Fri, Dec 27, 2019 at 10:46 AM Jian Hu <jian.hu@amlogic.com> wrote: [...] > @@ -294,9 +298,12 @@ 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); > + int ret = 0; > > - if (meson_parm_read(clk->map, &pll->rst) || > - !meson_parm_read(clk->map, &pll->en) || > + if (MESON_PARM_APPLICABLE(&pll->rst)) > + ret = meson_parm_read(clk->map, &pll->rst); > + > + if (ret || !meson_parm_read(clk->map, &pll->en) || > !meson_parm_read(clk->map, &pll->l)) > return 0; I had to read this part twice to understand what it's doing because I misunderstood what "ret" is used for (I thought that some "return ret" is missing) my proposal to make it easier to read: ... 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; ... please let me know what you think about this > @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw) > /* 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 for A1, 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; > + } in all other functions you are skipping the pll->rst register by checking for MESON_PARM_APPLICABLE(&pll->rst) I like that because it's a pattern which is easy to follow do you think we can make this part consistent with that? I'm thinking of something like this (not compile-tested and I dropped all comments, just so you get the idea): ... if (MESON_PARM_APPLICABLE(&pll->rst) meson_parm_write(clk->map, &pll->rst, 1); meson_parm_write(clk->map, &pll->en, 1); if (MESON_PARM_APPLICABLE(&pll->rst)) meson_parm_write(clk->map, &pll->rst, 0); if (MESON_PARM_APPLICABLE(&pll->current_en)) meson_parm_write(clk->map, &pll->current_en, 1); 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)) ... I see two (and a half) benefits here: - if there's a PLL with neither the pll->current_en nor the pll->rst registers then you get support for this implementation for free - the if (MESON_PARM_APPLICABLE(...)) pattern is already used in the driver, but only for one register (in your example when MESON_PARM_APPLICABLE(&pll->current_en) exists you also modify the pll->l_detect register, which I did not expect) - only counts half: no use of "goto", which in my opinion makes it very easy to read (just read from top to bottom, checking each "if") Martin
Hi Martin Thanks for your review On 2019/12/28 0:53, Martin Blumenstingl wrote: > Hi Jian, > > On Fri, Dec 27, 2019 at 10:46 AM Jian Hu <jian.hu@amlogic.com> wrote: > [...] >> @@ -294,9 +298,12 @@ 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); >> + int ret = 0; >> >> - if (meson_parm_read(clk->map, &pll->rst) || >> - !meson_parm_read(clk->map, &pll->en) || >> + if (MESON_PARM_APPLICABLE(&pll->rst)) >> + ret = meson_parm_read(clk->map, &pll->rst); >> + >> + if (ret || !meson_parm_read(clk->map, &pll->en) || >> !meson_parm_read(clk->map, &pll->l)) >> return 0; > I had to read this part twice to understand what it's doing because I > misunderstood what "ret" is used for (I thought that some "return ret" > is missing) > my proposal to make it easier to read: > ... > 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; > ... > > please let me know what you think about this I was intended to use 'ret' to store the return value of pll->rst. If pll->rst exists, it will get it. Otherwise, the ret will be zero. Your proposal is a good way for it. I will use it. > >> @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw) >> /* 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 for A1, 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; >> + } > in all other functions you are skipping the pll->rst register by > checking for MESON_PARM_APPLICABLE(&pll->rst) > I like that because it's a pattern which is easy to follow > > do you think we can make this part consistent with that? > I'm thinking of something like this (not compile-tested and I dropped > all comments, just so you get the idea): It is a good idea. I will test it. > ... > if (MESON_PARM_APPLICABLE(&pll->rst) > meson_parm_write(clk->map, &pll->rst, 1); > > meson_parm_write(clk->map, &pll->en, 1); > > if (MESON_PARM_APPLICABLE(&pll->rst)) > meson_parm_write(clk->map, &pll->rst, 0); > > if (MESON_PARM_APPLICABLE(&pll->current_en)) > meson_parm_write(clk->map, &pll->current_en, 1); > > 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)) > ... > > I see two (and a half) benefits here: > - if there's a PLL with neither the pll->current_en nor the pll->rst > registers then you get support for this implementation for free > - the if (MESON_PARM_APPLICABLE(...)) pattern is already used in the > driver, but only for one register (in your example when > MESON_PARM_APPLICABLE(&pll->current_en) exists you also modify the > pll->l_detect register, which I did not expect) > - only counts half: no use of "goto", which in my opinion makes it > very easy to read (just read from top to bottom, checking each "if") > I see, I will verify it. > > Martin > > . >
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c index ddb1e5634739..9eb7d465d123 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); + 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); } } @@ -294,9 +298,12 @@ 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); + int ret = 0; - if (meson_parm_read(clk->map, &pll->rst) || - !meson_parm_read(clk->map, &pll->en) || + if (MESON_PARM_APPLICABLE(&pll->rst)) + ret = meson_parm_read(clk->map, &pll->rst); + + if (ret || !meson_parm_read(clk->map, &pll->en) || !meson_parm_read(clk->map, &pll->l)) return 0; @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw) /* 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 for A1, 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); @@ -331,6 +355,7 @@ static int meson_clk_pll_enable(struct clk_hw *hw) /* Take the pll out reset */ meson_parm_write(clk->map, &pll->rst, 0); +out: if (meson_clk_pll_wait_lock(hw)) return -EIO; @@ -343,10 +368,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 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..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;
Compared with the previous SoCs, self-adaption module current 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. Signed-off-by: Jian Hu <jian.hu@amlogic.com> --- drivers/clk/meson/clk-pll.c | 40 ++++++++++++++++++++++++++++++++----- drivers/clk/meson/clk-pll.h | 2 ++ 2 files changed, 37 insertions(+), 5 deletions(-)