| Message ID | 8f578277cc015cfe9cdca06586b2c82f1a728bad.1697101543.git.quic_varada@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable cpufreq for IPQ5332 & IPQ9574 | expand |
Quoting Varadarajan Narayanan (2023-10-12 02:26:17) > Stromer plus APSS PLL does not support dynamic frequency scaling. > To switch between frequencies, we have to shut down the PLL, > configure the L and ALPHA values and turn on again. So introduce the > separate set of ops for Stromer Plus PLL. Does this assume the PLL is always on? > > Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > v2: Use clk_alpha_pll_stromer_determine_rate, instead of adding new > clk_alpha_pll_stromer_plus_determine_rate as the alpha pll width > is same for both > > Fix review comments > udelay(50) -> usleep_range(50, 60) > Remove SoC-specific from print message > --- > drivers/clk/qcom/clk-alpha-pll.c | 57 ++++++++++++++++++++++++++++++++++++++++ > drivers/clk/qcom/clk-alpha-pll.h | 1 + > 2 files changed, 58 insertions(+) > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > index 4edbf77..5221b6c 100644 > --- a/drivers/clk/qcom/clk-alpha-pll.c > +++ b/drivers/clk/qcom/clk-alpha-pll.c > @@ -2508,3 +2508,60 @@ const struct clk_ops clk_alpha_pll_stromer_ops = { > .set_rate = clk_alpha_pll_stromer_set_rate, > }; > EXPORT_SYMBOL_GPL(clk_alpha_pll_stromer_ops); > + > +static int clk_alpha_pll_stromer_plus_set_rate(struct clk_hw *hw, > + unsigned long rate, > + unsigned long prate) > +{ > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + u32 l, alpha_width = pll_alpha_width(pll); > + int ret; > + u64 a; > + > + rate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width); > + > + regmap_write(pll->clkr.regmap, PLL_MODE(pll), 0); There's a theoretical problem here if I understand correctly. A call to clk_enable() can happen while clk_set_rate() is in progress or vice versa. Probably we need some sort of spinlock for this PLL that synchronizes any enable/disable with the rate change so that when we restore the enable bit the clk isn't enabled when it was supposed to be off.
On Thu, Oct 12, 2023 at 01:55:36PM -0700, Stephen Boyd wrote: > Quoting Varadarajan Narayanan (2023-10-12 02:26:17) > > Stromer plus APSS PLL does not support dynamic frequency scaling. > > To switch between frequencies, we have to shut down the PLL, > > configure the L and ALPHA values and turn on again. So introduce the > > separate set of ops for Stromer Plus PLL. > > Does this assume the PLL is always on? Yes once the PLL is configured by apss-ipq-pll driver, it is always on. > > Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com> > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > v2: Use clk_alpha_pll_stromer_determine_rate, instead of adding new > > clk_alpha_pll_stromer_plus_determine_rate as the alpha pll width > > is same for both > > > > Fix review comments > > udelay(50) -> usleep_range(50, 60) > > Remove SoC-specific from print message > > --- > > drivers/clk/qcom/clk-alpha-pll.c | 57 ++++++++++++++++++++++++++++++++++++++++ > > drivers/clk/qcom/clk-alpha-pll.h | 1 + > > 2 files changed, 58 insertions(+) > > > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > > index 4edbf77..5221b6c 100644 > > --- a/drivers/clk/qcom/clk-alpha-pll.c > > +++ b/drivers/clk/qcom/clk-alpha-pll.c > > @@ -2508,3 +2508,60 @@ const struct clk_ops clk_alpha_pll_stromer_ops = { > > .set_rate = clk_alpha_pll_stromer_set_rate, > > }; > > EXPORT_SYMBOL_GPL(clk_alpha_pll_stromer_ops); > > + > > +static int clk_alpha_pll_stromer_plus_set_rate(struct clk_hw *hw, > > + unsigned long rate, > > + unsigned long prate) > > +{ > > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > > + u32 l, alpha_width = pll_alpha_width(pll); > > + int ret; > > + u64 a; > > + > > + rate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width); > > + > > + regmap_write(pll->clkr.regmap, PLL_MODE(pll), 0); > > There's a theoretical problem here if I understand correctly. A call to > clk_enable() can happen while clk_set_rate() is in progress or vice > versa. Probably we need some sort of spinlock for this PLL that > synchronizes any enable/disable with the rate change so that when we > restore the enable bit the clk isn't enabled when it was supposed to be > off. Since the PLL is always on, should we worry about enable/disable? If you feel it is better to synchronize with a spin lock, will add and post a new revision. Please let me know. Thanks Varada
On Mon, 16 Oct 2023 at 10:03, Varadarajan Narayanan <quic_varada@quicinc.com> wrote: > > On Thu, Oct 12, 2023 at 01:55:36PM -0700, Stephen Boyd wrote: > > Quoting Varadarajan Narayanan (2023-10-12 02:26:17) > > > Stromer plus APSS PLL does not support dynamic frequency scaling. > > > To switch between frequencies, we have to shut down the PLL, > > > configure the L and ALPHA values and turn on again. So introduce the > > > separate set of ops for Stromer Plus PLL. > > > > Does this assume the PLL is always on? > > Yes once the PLL is configured by apss-ipq-pll driver, it is always on. > > > > Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com> > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > > --- > > > v2: Use clk_alpha_pll_stromer_determine_rate, instead of adding new > > > clk_alpha_pll_stromer_plus_determine_rate as the alpha pll width > > > is same for both > > > > > > Fix review comments > > > udelay(50) -> usleep_range(50, 60) > > > Remove SoC-specific from print message > > > --- > > > drivers/clk/qcom/clk-alpha-pll.c | 57 ++++++++++++++++++++++++++++++++++++++++ > > > drivers/clk/qcom/clk-alpha-pll.h | 1 + > > > 2 files changed, 58 insertions(+) > > > > > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > > > index 4edbf77..5221b6c 100644 > > > --- a/drivers/clk/qcom/clk-alpha-pll.c > > > +++ b/drivers/clk/qcom/clk-alpha-pll.c > > > @@ -2508,3 +2508,60 @@ const struct clk_ops clk_alpha_pll_stromer_ops = { > > > .set_rate = clk_alpha_pll_stromer_set_rate, > > > }; > > > EXPORT_SYMBOL_GPL(clk_alpha_pll_stromer_ops); > > > + > > > +static int clk_alpha_pll_stromer_plus_set_rate(struct clk_hw *hw, > > > + unsigned long rate, > > > + unsigned long prate) > > > +{ > > > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > > > + u32 l, alpha_width = pll_alpha_width(pll); > > > + int ret; > > > + u64 a; > > > + > > > + rate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width); > > > + > > > + regmap_write(pll->clkr.regmap, PLL_MODE(pll), 0); > > > > There's a theoretical problem here if I understand correctly. A call to > > clk_enable() can happen while clk_set_rate() is in progress or vice > > versa. Probably we need some sort of spinlock for this PLL that > > synchronizes any enable/disable with the rate change so that when we > > restore the enable bit the clk isn't enabled when it was supposed to be > > off. > > Since the PLL is always on, should we worry about enable/disable? > If you feel it is better to synchronize with a spin lock, will > add and post a new revision. Please let me know. Probably another option might be to change stromer PLL ops to use prepare/unprepare instead of enable/disable. This way the clk_prepare_lock() in clk_set_rate() will take care of locking. > > Thanks > Varada
On Mon, Oct 16, 2023 at 11:46:56AM +0300, Dmitry Baryshkov wrote: > On Mon, 16 Oct 2023 at 10:03, Varadarajan Narayanan > <quic_varada@quicinc.com> wrote: > > > > On Thu, Oct 12, 2023 at 01:55:36PM -0700, Stephen Boyd wrote: > > > Quoting Varadarajan Narayanan (2023-10-12 02:26:17) > > > > Stromer plus APSS PLL does not support dynamic frequency scaling. > > > > To switch between frequencies, we have to shut down the PLL, > > > > configure the L and ALPHA values and turn on again. So introduce the > > > > separate set of ops for Stromer Plus PLL. > > > > > > Does this assume the PLL is always on? > > > > Yes once the PLL is configured by apss-ipq-pll driver, it is always on. > > > > > > Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com> > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > > > --- > > > > v2: Use clk_alpha_pll_stromer_determine_rate, instead of adding new > > > > clk_alpha_pll_stromer_plus_determine_rate as the alpha pll width > > > > is same for both > > > > > > > > Fix review comments > > > > udelay(50) -> usleep_range(50, 60) > > > > Remove SoC-specific from print message > > > > --- > > > > drivers/clk/qcom/clk-alpha-pll.c | 57 ++++++++++++++++++++++++++++++++++++++++ > > > > drivers/clk/qcom/clk-alpha-pll.h | 1 + > > > > 2 files changed, 58 insertions(+) > > > > > > > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > > > > index 4edbf77..5221b6c 100644 > > > > --- a/drivers/clk/qcom/clk-alpha-pll.c > > > > +++ b/drivers/clk/qcom/clk-alpha-pll.c > > > > @@ -2508,3 +2508,60 @@ const struct clk_ops clk_alpha_pll_stromer_ops = { > > > > .set_rate = clk_alpha_pll_stromer_set_rate, > > > > }; > > > > EXPORT_SYMBOL_GPL(clk_alpha_pll_stromer_ops); > > > > + > > > > +static int clk_alpha_pll_stromer_plus_set_rate(struct clk_hw *hw, > > > > + unsigned long rate, > > > > + unsigned long prate) > > > > +{ > > > > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > > > > + u32 l, alpha_width = pll_alpha_width(pll); > > > > + int ret; > > > > + u64 a; > > > > + > > > > + rate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width); > > > > + > > > > + regmap_write(pll->clkr.regmap, PLL_MODE(pll), 0); > > > > > > There's a theoretical problem here if I understand correctly. A call to > > > clk_enable() can happen while clk_set_rate() is in progress or vice > > > versa. Probably we need some sort of spinlock for this PLL that > > > synchronizes any enable/disable with the rate change so that when we > > > restore the enable bit the clk isn't enabled when it was supposed to be > > > off. > > > > Since the PLL is always on, should we worry about enable/disable? > > If you feel it is better to synchronize with a spin lock, will > > add and post a new revision. Please let me know. > > Probably another option might be to change stromer PLL ops to use > prepare/unprepare instead of enable/disable. This way the > clk_prepare_lock() in clk_set_rate() will take care of locking. Thanks for the suggestion. Have posted v3 with this and addressing Stephen Boyd's other comments. Please take a look. (https://lore.kernel.org/linux-arm-msm/cover.1697600121.git.quic_varada@quicinc.com/) Thanks Varada
diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c index 4edbf77..5221b6c 100644 --- a/drivers/clk/qcom/clk-alpha-pll.c +++ b/drivers/clk/qcom/clk-alpha-pll.c @@ -2508,3 +2508,60 @@ const struct clk_ops clk_alpha_pll_stromer_ops = { .set_rate = clk_alpha_pll_stromer_set_rate, }; EXPORT_SYMBOL_GPL(clk_alpha_pll_stromer_ops); + +static int clk_alpha_pll_stromer_plus_set_rate(struct clk_hw *hw, + unsigned long rate, + unsigned long prate) +{ + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); + u32 l, alpha_width = pll_alpha_width(pll); + int ret; + u64 a; + + rate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width); + + regmap_write(pll->clkr.regmap, PLL_MODE(pll), 0); + + /* Delay of 2 output clock ticks required until output is disabled */ + udelay(1); + + regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l); + + if (alpha_width > ALPHA_BITWIDTH) + a <<= alpha_width - ALPHA_BITWIDTH; + + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a); + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL_U(pll), + a >> ALPHA_BITWIDTH); + + regmap_write(pll->clkr.regmap, PLL_MODE(pll), PLL_BYPASSNL); + + /* Wait five micro seconds or more */ + udelay(5); + regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_RESET_N, + PLL_RESET_N); + + /* The lock time should be less than 50 micro seconds worst case */ + usleep_range(50, 60); + + ret = wait_for_pll_enable_lock(pll); + if (ret) { + pr_err("Wait for PLL enable lock failed [%s] %d\n", + clk_hw_get_name(hw), ret); + return ret; + } + regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_OUTCTRL, + PLL_OUTCTRL); + + return 0; +} + +const struct clk_ops clk_alpha_pll_stromer_plus_ops = { + .enable = clk_alpha_pll_enable, + .disable = clk_alpha_pll_disable, + .is_enabled = clk_alpha_pll_is_enabled, + .recalc_rate = clk_alpha_pll_recalc_rate, + .determine_rate = clk_alpha_pll_stromer_determine_rate, + .set_rate = clk_alpha_pll_stromer_plus_set_rate, +}; +EXPORT_SYMBOL_GPL(clk_alpha_pll_stromer_plus_ops); diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h index 3b24a66..a1a75bb 100644 --- a/drivers/clk/qcom/clk-alpha-pll.h +++ b/drivers/clk/qcom/clk-alpha-pll.h @@ -152,6 +152,7 @@ extern const struct clk_ops clk_alpha_pll_postdiv_ops; extern const struct clk_ops clk_alpha_pll_huayra_ops; extern const struct clk_ops clk_alpha_pll_postdiv_ro_ops; extern const struct clk_ops clk_alpha_pll_stromer_ops; +extern const struct clk_ops clk_alpha_pll_stromer_plus_ops; extern const struct clk_ops clk_alpha_pll_fabia_ops; extern const struct clk_ops clk_alpha_pll_fixed_fabia_ops;