From: <ilialin@codeaurora.org>
To: 'Julien Thierry' <julien.thierry@arm.com>,
linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org, sboyd@codeaurora.org
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
rnayak@codeaurora.org, will.deacon@arm.com,
tfinkel@codeaurora.org, qualcomm-lt@lists.linaro.org,
celster@codeaurora.org, 'Taniya Das' <tdas@codeaurora.org>
Subject: RE: [PATCH 02/10] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update
Date: Thu, 4 Jan 2018 13:14:50 +0200 [thread overview]
Message-ID: <027d01d3854d$3f94aac0$bebe0040$@codeaurora.org> (raw)
In-Reply-To: <ee78b47f-4fdb-01eb-ba04-c1172b7b7b00@arm.com>
This is address in the V2: https://patchwork.kernel.org/patch/10144477/
> -----Original Message-----
> From: Julien Thierry [mailto:julien.thierry@arm.com]
> Sent: Tuesday, December 12, 2017 5:06 PM
> To: Ilia Lin <ilialin@codeaurora.org>; linux-clk@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-arm-msm@vger.kernel.org;
> sboyd@codeaurora.org
> Cc: mark.rutland@arm.com; devicetree@vger.kernel.org;
> rnayak@codeaurora.org; will.deacon@arm.com; tfinkel@codeaurora.org;
> qualcomm-lt@lists.linaro.org; celster@codeaurora.org; Taniya Das
> <tdas@codeaurora.org>
> Subject: Re: [PATCH 02/10] clk: qcom: Fix .set_rate to handle alpha PLLs
> w/wo dynamic update
>
> Hi,
>
> On 12/12/17 12:31, Ilia Lin wrote:
> > From: Taniya Das <tdas@codeaurora.org>
> >
> > From: Taniya Das <tdas@codeaurora.org>
> >
> > Alpha PLLs which do not support dynamic update feature need to be
> > explicitly disabled before a rate change.
> > The ones which do support dynamic update do so within a single vco
> > range, so add a min/max freq check for such PLLs so they fall in the
> > vco range.
> >
> > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> > Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> > ---
> > drivers/clk/qcom/clk-alpha-pll.c | 71
> +++++++++++++++++++++++++++++++++-------
> > drivers/clk/qcom/clk-alpha-pll.h | 5 +++
> > 2 files changed, 65 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/clk/qcom/clk-alpha-pll.c
> > b/drivers/clk/qcom/clk-alpha-pll.c
> > index 47a1da3..ecb9e7f 100644
> > --- a/drivers/clk/qcom/clk-alpha-pll.c
> > +++ b/drivers/clk/qcom/clk-alpha-pll.c
> > @@ -376,19 +376,46 @@ static unsigned long alpha_pll_calc_rate(u64
> prate, u32 l, u32 a)
> > return alpha_pll_calc_rate(prate, l, a);
> > }
> >
> > -static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> > - unsigned long prate)
> > +static int alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long prate,
> > + int (*enable)(struct clk_hw *hw),
> > + void (*disable)(struct clk_hw *hw))
> > {
> > + bool enabled;
>
> Some remarks about this.
>
> > struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> > const struct pll_vco *vco;
> > u32 l, off = pll->offset;
> > u64 a;
> >
> > rate = alpha_pll_round_rate(rate, prate, &l, &a);
> > - vco = alpha_pll_find_vco(pll, rate);
> > - if (!vco) {
> > - pr_err("alpha pll not in a valid vco range\n");
> > - return -EINVAL;
> > + enabled = clk_hw_is_enabled(hw);
>
> This is not needed unless we go through the 'else' branch.
>
> > +
> > + if (pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
> > + /*
> > + * PLLs which support dynamic updates support one single
> > + * vco range, between min_rate and max_rate supported
> > + */
> > + if (rate < pll->min_rate || rate > pll->max_rate) {
> > + pr_err("alpha pll rate outside supported min/max
> range\n");
> > + return -EINVAL;
> > + }
> > + } else {
> > + /*
> > + * All alpha PLLs which do not support dynamic update,
> > + * should be disabled before a vco update.
> > + */
> > + if (enabled)
> > + disable(hw);
> > +
> > + vco = alpha_pll_find_vco(pll, rate);
> > + if (!vco) {
> > + pr_err("alpha pll not in a valid vco range\n");
> > + return -EINVAL;
> > + }
> > +
> > + regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
> > + PLL_VCO_MASK << PLL_VCO_SHIFT,
> > + vco->val << PLL_VCO_SHIFT);
> > }
> >
> > regmap_write(pll->clkr.regmap, off + PLL_L_VAL, l); @@ -401,16
> > +428,29 @@ static int clk_alpha_pll_set_rate(struct clk_hw *hw, unsigned
> long rate,
> > regmap_write(pll->clkr.regmap, off + PLL_ALPHA_VAL_U, a
> >> 32);
> > }
> >
> > - regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
> > - PLL_VCO_MASK << PLL_VCO_SHIFT,
> > - vco->val << PLL_VCO_SHIFT);
> > -
> > regmap_update_bits(pll->clkr.regmap, off + PLL_USER_CTL,
> PLL_ALPHA_EN,
> > PLL_ALPHA_EN);
> >
> > + if (!(pll->flags & SUPPORTS_DYNAMIC_UPDATE) && enabled)
> > + enable(hw);
> > +
>
> This condition is only "did we disable the clock and need to reenable it?".
>
> To make it clearer, I'd suggest renaming 'enabled' to something like
> 'need_reenabling' and the code look like this:
>
> static int alpha_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> unsigned long prate,
> int (*enable)(struct clk_hw *hw),
> void (*disable)(struct clk_hw *hw)) {
> bool need_reenabling = false;
>
> [...]
>
> if(pll->flags & SUPPORTS_DYNAMIC_UPDATE) {
> [...]
> } else {
> if (clk_hw_is_enabled(hw)) {
> disable(hw);
> need_reenabling = true;
> }
> [...]
> }
>
> [...]
>
> if (need_reenabling)
> enable(hw);
>
> }
>
>
> Cheers,
>
> --
> Julien Thierry
next prev parent reply other threads:[~2018-01-04 11:14 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-12 12:31 [PATCH 00/10] clk: qcom: CPU clock driver for msm8996 Ilia Lin
2017-12-12 12:31 ` [PATCH 01/10] soc: qcom: Separate kryo l2 accessors from PMU driver Ilia Lin
2017-12-12 14:03 ` Mark Rutland
2017-12-22 2:06 ` Stephen Boyd
2018-01-04 11:15 ` ilialin
2018-01-04 11:13 ` ilialin
2017-12-12 12:31 ` [PATCH 02/10] clk: qcom: Fix .set_rate to handle alpha PLLs w/wo dynamic update Ilia Lin
[not found] ` <1513081897-31612-3-git-send-email-ilialin-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-12-12 15:05 ` Julien Thierry
2018-01-04 11:14 ` ilialin [this message]
2017-12-12 12:31 ` [PATCH 03/10] clk: qcom: Make clk_alpha_pll_configure available to modules Ilia Lin
2017-12-12 12:31 ` [PATCH 04/10] clk: qcom: Add CPU clock driver for msm8996 Ilia Lin
2017-12-15 22:35 ` Rob Herring
2018-01-04 11:15 ` ilialin
[not found] ` <1513081897-31612-1-git-send-email-ilialin-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-12-12 12:31 ` [PATCH 05/10] clk: qcom: cpu-8996: Add support to switch to alternate PLL Ilia Lin
2017-12-12 12:31 ` [PATCH 06/10] clk: qcom: cpu-8996: Add support to switch below 600Mhz Ilia Lin
2017-12-12 12:31 ` [PATCH 08/10] clk: qcom: Add ACD path to CPU clock driver for msm8996 Ilia Lin
2017-12-12 12:31 ` [PATCH 07/10] clk: qcom: clk-cpu-8996: Prepare PLLs on probe Ilia Lin
2017-12-12 12:31 ` [PATCH 09/10] DT: QCOM: Add cpufreq-dt to msm8996 Ilia Lin
2017-12-12 12:31 ` [PATCH 10/10] DT: QCOM: Add thermal mitigation " Ilia Lin
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='027d01d3854d$3f94aac0$bebe0040$@codeaurora.org' \
--to=ilialin@codeaurora.org \
--cc=celster@codeaurora.org \
--cc=devicetree@vger.kernel.org \
--cc=julien.thierry@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=qualcomm-lt@lists.linaro.org \
--cc=rnayak@codeaurora.org \
--cc=sboyd@codeaurora.org \
--cc=tdas@codeaurora.org \
--cc=tfinkel@codeaurora.org \
--cc=will.deacon@arm.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).