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.
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);
+
+ /* 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;
--
2.7.4Quoting 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
--
With best wishes
Dmitry
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
© 2016 - 2025 Red Hat, Inc.