[v1,04/10] clk: qcom: apss-ipq6018: ipq5332: add safe source switch for a53pll

Commit Message

Varadarajan Narayanan Sept. 7, 2023, 5:21 a.m. UTC

Stromer Plus PLL found on IPQ53xx doesn't support dynamic
frequency scaling. To achieve the same, we need to park the APPS
PLL source to GPLL0, re configure the PLL and then switch the
source to APSS_PLL_EARLY.

To support this, register a clock notifier to get the PRE_RATE
and POST_RATE notification. Change the APSS PLL source to GPLL0
when PRE_RATE notification is received, then configure the PLL
and then change back the source to APSS_PLL_EARLY.

Signed-off-by: Kathiravan T <[e-mail]>
Signed-off-by: Varadarajan Narayanan <[e-mail]>
---
 drivers/clk/qcom/apss-ipq6018.c | 54 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

Comments

Konrad Dybcio Sept. 7, 2023, 8:31 a.m. UTC | #1

On 7.09.2023 07:21, Varadarajan Narayanan wrote:
> Stromer Plus PLL found on IPQ53xx doesn't support dynamic
> frequency scaling. To achieve the same, we need to park the APPS
> PLL source to GPLL0, re configure the PLL and then switch the
> source to APSS_PLL_EARLY.
> 
> To support this, register a clock notifier to get the PRE_RATE
> and POST_RATE notification. Change the APSS PLL source to GPLL0
> when PRE_RATE notification is received, then configure the PLL
> and then change back the source to APSS_PLL_EARLY.
> 
> Signed-off-by: Kathiravan T <[e-mail]>
> Signed-off-by: Varadarajan Narayanan <[e-mail]>
> ---
>  drivers/clk/qcom/apss-ipq6018.c | 54 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> index 4e13a08..ffb6ab5 100644
> --- a/drivers/clk/qcom/apss-ipq6018.c
> +++ b/drivers/clk/qcom/apss-ipq6018.c
> @@ -9,8 +9,11 @@
>  #include <linux/clk-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/soc/qcom/smem.h>
>  
>  #include <dt-bindings/clock/qcom,apss-ipq.h>
> +#include <dt-bindings/arm/qcom,ids.h>
>  
>  #include "common.h"
>  #include "clk-regmap.h"
> @@ -84,15 +87,64 @@ static const struct qcom_cc_desc apss_ipq6018_desc = {
>  	.num_clks = ARRAY_SIZE(apss_ipq6018_clks),
>  };
>  
> +static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
> +				void *data)
> +{
> +	u8 index;
> +	int err;
> +
> +	if (action == PRE_RATE_CHANGE)
> +		index = P_GPLL0;
> +	else if (action == POST_RATE_CHANGE)
> +		index = P_APSS_PLL_EARLY;
> +	else
> +		return 0;
> +
> +	err = clk_rcg2_mux_closest_ops.set_parent(&apcs_alias0_clk_src.clkr.hw,
> +						  index);
Adding a variable for clk_hw within the apcs_alias0 clock would
make this easier to digest, I think.

And if we wanna be even less error-prone, you can reference the
ops of this clock in an indirect way.

> +
> +	return notifier_from_errno(err);
> +}
> +
> +static struct notifier_block cpu_clk_notifier = {
> +	.notifier_call = cpu_clk_notifier_fn,
> +};
> +
>  static int apss_ipq6018_probe(struct platform_device *pdev)
>  {
>  	struct regmap *regmap;
> +	u32 soc_id;
> +	int ret;
> +
> +	ret = qcom_smem_get_soc_id(&soc_id);
> +	if (ret)
> +		return ret;
>  
>  	regmap = dev_get_regmap(pdev->dev.parent, NULL);
>  	if (!regmap)
>  		return -ENODEV;
>  
> -	return qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> +	ret = qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> +	if (ret)
> +		return ret;
> +
> +	switch (soc_id) {
> +	/*
> +	 * Only below variants of IPQ53xx support scaling
> +	 */
1. /* Keep this in a 1-line comment */

2. why? explain the reasoning in the commit message

Konrad
> +	case QCOM_ID_IPQ5332:
> +	case QCOM_ID_IPQ5322:
> +	case QCOM_ID_IPQ5300:
> +		ret = clk_notifier_register(apcs_alias0_clk_src.clkr.hw.clk,
> +						&cpu_clk_notifier);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
>  }
>  
>  static struct platform_driver apss_ipq6018_driver = {

kernel test robot Sept. 7, 2023, 8:51 a.m. UTC | #2

Hi Varadarajan,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.5 next-20230907]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Varadarajan-Narayanan/clk-qcom-clk-alpha-pll-introduce-stromer-plus-ops/20230907-132537
base:   linus/master
patch link:    https://lore.kernel.org/r/5e3c29df2b42cceb8072b00546a78e1b99b2d374.1693996662.git.quic_varada%40quicinc.com
patch subject: [PATCH v1 04/10] clk: qcom: apss-ipq6018: ipq5332: add safe source switch for a53pll
config: s390-randconfig-r031-20230907 (https://download.01.org/0day-ci/archive/20230907/[e-mail]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All errors (new ones prefixed by >>):

   In file included from drivers/clk/qcom/apss-ipq6018.c:10:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:78:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from drivers/clk/qcom/apss-ipq6018.c:10:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:78:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from drivers/clk/qcom/apss-ipq6018.c:10:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:78:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/clk/qcom/apss-ipq6018.c:94:11: error: use of undeclared identifier 'P_GPLL0'
                   index = P_GPLL0;
                           ^
   12 warnings and 1 error generated.


vim +/P_GPLL0 +94 drivers/clk/qcom/apss-ipq6018.c

    86	
    87	static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
    88					void *data)
    89	{
    90		u8 index;
    91		int err;
    92	
    93		if (action == PRE_RATE_CHANGE)
  > 94			index = P_GPLL0;
    95		else if (action == POST_RATE_CHANGE)
    96			index = P_APSS_PLL_EARLY;
    97		else
    98			return 0;
    99	
   100		err = clk_rcg2_mux_closest_ops.set_parent(&apcs_alias0_clk_src.clkr.hw,
   101							  index);
   102	
   103		return notifier_from_errno(err);
   104	}
   105

Dmitry Baryshkov Sept. 7, 2023, 1:54 p.m. UTC | #3

On Thu, 7 Sept 2023 at 08:22, Varadarajan Narayanan
<[e-mail]> wrote:
>
> Stromer Plus PLL found on IPQ53xx doesn't support dynamic
> frequency scaling. To achieve the same, we need to park the APPS
> PLL source to GPLL0, re configure the PLL and then switch the
> source to APSS_PLL_EARLY.
>
> To support this, register a clock notifier to get the PRE_RATE
> and POST_RATE notification. Change the APSS PLL source to GPLL0
> when PRE_RATE notification is received, then configure the PLL
> and then change back the source to APSS_PLL_EARLY.

This means that we are changing the parents behind the back of CCF,
which is not great.

>
> Signed-off-by: Kathiravan T <[e-mail]>
> Signed-off-by: Varadarajan Narayanan <[e-mail]>
> ---
>  drivers/clk/qcom/apss-ipq6018.c | 54 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> index 4e13a08..ffb6ab5 100644
> --- a/drivers/clk/qcom/apss-ipq6018.c
> +++ b/drivers/clk/qcom/apss-ipq6018.c
> @@ -9,8 +9,11 @@
>  #include <linux/clk-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/soc/qcom/smem.h>
>
>  #include <dt-bindings/clock/qcom,apss-ipq.h>
> +#include <dt-bindings/arm/qcom,ids.h>
>
>  #include "common.h"
>  #include "clk-regmap.h"
> @@ -84,15 +87,64 @@ static const struct qcom_cc_desc apss_ipq6018_desc = {
>         .num_clks = ARRAY_SIZE(apss_ipq6018_clks),
>  };
>
> +static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
> +                               void *data)
> +{
> +       u8 index;
> +       int err;
> +
> +       if (action == PRE_RATE_CHANGE)
> +               index = P_GPLL0;

I don't see P_GPLL0 being supported in the ipq6018 driver.

> +       else if (action == POST_RATE_CHANGE)
> +               index = P_APSS_PLL_EARLY;

You also have to handle ABORT_RATE_CHANGE here.

> +       else
> +               return 0;
> +
> +       err = clk_rcg2_mux_closest_ops.set_parent(&apcs_alias0_clk_src.clkr.hw,
> +                                                 index);
> +
> +       return notifier_from_errno(err);
> +}
> +
> +static struct notifier_block cpu_clk_notifier = {
> +       .notifier_call = cpu_clk_notifier_fn,
> +};
> +
>  static int apss_ipq6018_probe(struct platform_device *pdev)
>  {
>         struct regmap *regmap;
> +       u32 soc_id;
> +       int ret;
> +
> +       ret = qcom_smem_get_soc_id(&soc_id);
> +       if (ret)
> +               return ret;
>
>         regmap = dev_get_regmap(pdev->dev.parent, NULL);
>         if (!regmap)
>                 return -ENODEV;
>
> -       return qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> +       ret = qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> +       if (ret)
> +               return ret;
> +
> +       switch (soc_id) {
> +       /*
> +        * Only below variants of IPQ53xx support scaling
> +        */
> +       case QCOM_ID_IPQ5332:
> +       case QCOM_ID_IPQ5322:
> +       case QCOM_ID_IPQ5300:

Please use compat strings instead of using the soc-id.

> +               ret = clk_notifier_register(apcs_alias0_clk_src.clkr.hw.clk,
> +                                               &cpu_clk_notifier);
> +               if (ret)
> +                       return ret;
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       return 0;
>  }
>
>  static struct platform_driver apss_ipq6018_driver = {
> --
> 2.7.4
>


--
With best wishes

Dmitry

Varadarajan Narayanan Sept. 12, 2023, 8:59 a.m. UTC | #4

On Thu, Sep 07, 2023 at 04:54:56PM +0300, Dmitry Baryshkov wrote:
> On Thu, 7 Sept 2023 at 08:22, Varadarajan Narayanan
> <[e-mail]> wrote:
> >
> > Stromer Plus PLL found on IPQ53xx doesn't support dynamic
> > frequency scaling. To achieve the same, we need to park the APPS
> > PLL source to GPLL0, re configure the PLL and then switch the
> > source to APSS_PLL_EARLY.
> >
> > To support this, register a clock notifier to get the PRE_RATE
> > and POST_RATE notification. Change the APSS PLL source to GPLL0
> > when PRE_RATE notification is received, then configure the PLL
> > and then change back the source to APSS_PLL_EARLY.
>
> This means that we are changing the parents behind the back of CCF,
> which is not great.

Unfortunately, we are not aware of any other way to do this.
Please let me know if there is a better way to do this, will
implement that and post a revision.

> > Signed-off-by: Kathiravan T <[e-mail]>
> > Signed-off-by: Varadarajan Narayanan <[e-mail]>
> > ---
> >  drivers/clk/qcom/apss-ipq6018.c | 54 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> > index 4e13a08..ffb6ab5 100644
> > --- a/drivers/clk/qcom/apss-ipq6018.c
> > +++ b/drivers/clk/qcom/apss-ipq6018.c
> > @@ -9,8 +9,11 @@
> >  #include <linux/clk-provider.h>
> >  #include <linux/regmap.h>
> >  #include <linux/module.h>
> > +#include <linux/clk.h>
> > +#include <linux/soc/qcom/smem.h>
> >
> >  #include <dt-bindings/clock/qcom,apss-ipq.h>
> > +#include <dt-bindings/arm/qcom,ids.h>
> >
> >  #include "common.h"
> >  #include "clk-regmap.h"
> > @@ -84,15 +87,64 @@ static const struct qcom_cc_desc apss_ipq6018_desc = {
> >         .num_clks = ARRAY_SIZE(apss_ipq6018_clks),
> >  };
> >
> > +static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
> > +                               void *data)
> > +{
> > +       u8 index;
> > +       int err;
> > +
> > +       if (action == PRE_RATE_CHANGE)
> > +               index = P_GPLL0;
>
> I don't see P_GPLL0 being supported in the ipq6018 driver.

This comes from one of the dependency patches mentioned in the
cover letter [https://lore.kernel.org/linux-arm-msm/[e-mail]/].
Please refer to patch https://lore.kernel.org/linux-arm-msm/[e-mail]/.

>
> > +       else if (action == POST_RATE_CHANGE)
> > +               index = P_APSS_PLL_EARLY;
>
> You also have to handle ABORT_RATE_CHANGE here.

ok.

> > +       else
> > +               return 0;
> > +
> > +       err = clk_rcg2_mux_closest_ops.set_parent(&apcs_alias0_clk_src.clkr.hw,
> > +                                                 index);
> > +
> > +       return notifier_from_errno(err);
> > +}
> > +
> > +static struct notifier_block cpu_clk_notifier = {
> > +       .notifier_call = cpu_clk_notifier_fn,
> > +};
> > +
> >  static int apss_ipq6018_probe(struct platform_device *pdev)
> >  {
> >         struct regmap *regmap;
> > +       u32 soc_id;
> > +       int ret;
> > +
> > +       ret = qcom_smem_get_soc_id(&soc_id);
> > +       if (ret)
> > +               return ret;
> >
> >         regmap = dev_get_regmap(pdev->dev.parent, NULL);
> >         if (!regmap)
> >                 return -ENODEV;
> >
> > -       return qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> > +       ret = qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> > +       if (ret)
> > +               return ret;
> > +
> > +       switch (soc_id) {
> > +       /*
> > +        * Only below variants of IPQ53xx support scaling
> > +        */
> > +       case QCOM_ID_IPQ5332:
> > +       case QCOM_ID_IPQ5322:
> > +       case QCOM_ID_IPQ5300:
>
> Please use compat strings instead of using the soc-id.

We have a common compatible string for all IPQ53xx CPUs across
boards.  The CPU variant is identified from fuse settings. Not
sure how we can differentiate between the variants using compat
strings. Can you kindly help.

Thanks
varada

> > +               ret = clk_notifier_register(apcs_alias0_clk_src.clkr.hw.clk,
> > +                                               &cpu_clk_notifier);
> > +               if (ret)
> > +                       return ret;
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +
> > +       return 0;
> >  }
> >
> >  static struct platform_driver apss_ipq6018_driver = {
> > --
> > 2.7.4
> >
>
>
> --
> With best wishes
>
> Dmitry

Varadarajan Narayanan Sept. 29, 2023, 7:32 a.m. UTC | #5

On Thu, Sep 07, 2023 at 10:31:55AM +0200, Konrad Dybcio wrote:
> On 7.09.2023 07:21, Varadarajan Narayanan wrote:
> > Stromer Plus PLL found on IPQ53xx doesn't support dynamic
> > frequency scaling. To achieve the same, we need to park the APPS
> > PLL source to GPLL0, re configure the PLL and then switch the
> > source to APSS_PLL_EARLY.
> >
> > To support this, register a clock notifier to get the PRE_RATE
> > and POST_RATE notification. Change the APSS PLL source to GPLL0
> > when PRE_RATE notification is received, then configure the PLL
> > and then change back the source to APSS_PLL_EARLY.
> >
> > Signed-off-by: Kathiravan T <[e-mail]>
> > Signed-off-by: Varadarajan Narayanan <[e-mail]>
> > ---
> >  drivers/clk/qcom/apss-ipq6018.c | 54 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> > index 4e13a08..ffb6ab5 100644
> > --- a/drivers/clk/qcom/apss-ipq6018.c
> > +++ b/drivers/clk/qcom/apss-ipq6018.c
> > @@ -9,8 +9,11 @@
> >  #include <linux/clk-provider.h>
> >  #include <linux/regmap.h>
> >  #include <linux/module.h>
> > +#include <linux/clk.h>
> > +#include <linux/soc/qcom/smem.h>
> >
> >  #include <dt-bindings/clock/qcom,apss-ipq.h>
> > +#include <dt-bindings/arm/qcom,ids.h>
> >
> >  #include "common.h"
> >  #include "clk-regmap.h"
> > @@ -84,15 +87,64 @@ static const struct qcom_cc_desc apss_ipq6018_desc = {
> >  	.num_clks = ARRAY_SIZE(apss_ipq6018_clks),
> >  };
> >
> > +static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
> > +				void *data)
> > +{
> > +	u8 index;
> > +	int err;
> > +
> > +	if (action == PRE_RATE_CHANGE)
> > +		index = P_GPLL0;
> > +	else if (action == POST_RATE_CHANGE)
> > +		index = P_APSS_PLL_EARLY;
> > +	else
> > +		return 0;
> > +
> > +	err = clk_rcg2_mux_closest_ops.set_parent(&apcs_alias0_clk_src.clkr.hw,
> > +						  index);
> Adding a variable for clk_hw within the apcs_alias0 clock would
> make this easier to digest, I think.
>
> And if we wanna be even less error-prone, you can reference the
> ops of this clock in an indirect way.

Will change it as

	struct clk_hw *hw;

	hw = &apcs_alias0_clk_src.clkr.hw;
	err = hw->init->ops->set_parent(hw, index);

> > +	return notifier_from_errno(err);
> > +}
> > +
> > +static struct notifier_block cpu_clk_notifier = {
> > +	.notifier_call = cpu_clk_notifier_fn,
> > +};
> > +
> >  static int apss_ipq6018_probe(struct platform_device *pdev)
> >  {
> >  	struct regmap *regmap;
> > +	u32 soc_id;
> > +	int ret;
> > +
> > +	ret = qcom_smem_get_soc_id(&soc_id);
> > +	if (ret)
> > +		return ret;
> >
> >  	regmap = dev_get_regmap(pdev->dev.parent, NULL);
> >  	if (!regmap)
> >  		return -ENODEV;
> >
> > -	return qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> > +	ret = qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (soc_id) {
> > +	/*
> > +	 * Only below variants of IPQ53xx support scaling
> > +	 */
> 1. /* Keep this in a 1-line comment */

Ok

> 2. why? explain the reasoning in the commit message

Ok

Thanks
Varada

> > +	case QCOM_ID_IPQ5332:
> > +	case QCOM_ID_IPQ5322:
> > +	case QCOM_ID_IPQ5300:
> > +		ret = clk_notifier_register(apcs_alias0_clk_src.clkr.hw.clk,
> > +						&cpu_clk_notifier);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> >  }
> >
> >  static struct platform_driver apss_ipq6018_driver = {

Dmitry Baryshkov Sept. 29, 2023, 8:29 a.m. UTC | #6

On Fri, 29 Sept 2023 at 10:33, Varadarajan Narayanan
<[e-mail]> wrote:
>
> On Thu, Sep 07, 2023 at 10:31:55AM +0200, Konrad Dybcio wrote:
> > On 7.09.2023 07:21, Varadarajan Narayanan wrote:
> > > Stromer Plus PLL found on IPQ53xx doesn't support dynamic
> > > frequency scaling. To achieve the same, we need to park the APPS
> > > PLL source to GPLL0, re configure the PLL and then switch the
> > > source to APSS_PLL_EARLY.
> > >
> > > To support this, register a clock notifier to get the PRE_RATE
> > > and POST_RATE notification. Change the APSS PLL source to GPLL0
> > > when PRE_RATE notification is received, then configure the PLL
> > > and then change back the source to APSS_PLL_EARLY.
> > >
> > > Signed-off-by: Kathiravan T <[e-mail]>
> > > Signed-off-by: Varadarajan Narayanan <[e-mail]>
> > > ---
> > >  drivers/clk/qcom/apss-ipq6018.c | 54 ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 53 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> > > index 4e13a08..ffb6ab5 100644
> > > --- a/drivers/clk/qcom/apss-ipq6018.c
> > > +++ b/drivers/clk/qcom/apss-ipq6018.c
> > > @@ -9,8 +9,11 @@
> > >  #include <linux/clk-provider.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/module.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/soc/qcom/smem.h>
> > >
> > >  #include <dt-bindings/clock/qcom,apss-ipq.h>
> > > +#include <dt-bindings/arm/qcom,ids.h>
> > >
> > >  #include "common.h"
> > >  #include "clk-regmap.h"
> > > @@ -84,15 +87,64 @@ static const struct qcom_cc_desc apss_ipq6018_desc = {
> > >     .num_clks = ARRAY_SIZE(apss_ipq6018_clks),
> > >  };
> > >
> > > +static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
> > > +                           void *data)
> > > +{
> > > +   u8 index;
> > > +   int err;
> > > +
> > > +   if (action == PRE_RATE_CHANGE)
> > > +           index = P_GPLL0;
> > > +   else if (action == POST_RATE_CHANGE)
> > > +           index = P_APSS_PLL_EARLY;
> > > +   else
> > > +           return 0;
> > > +
> > > +   err = clk_rcg2_mux_closest_ops.set_parent(&apcs_alias0_clk_src.clkr.hw,
> > > +                                             index);
> > Adding a variable for clk_hw within the apcs_alias0 clock would
> > make this easier to digest, I think.
> >
> > And if we wanna be even less error-prone, you can reference the
> > ops of this clock in an indirect way.
>
> Will change it as
>
>         struct clk_hw *hw;
>
>         hw = &apcs_alias0_clk_src.clkr.hw;
>         err = hw->init->ops->set_parent(hw, index);

You can not do this, hw->init is cleared during registration.

>
> > > +   return notifier_from_errno(err);
> > > +}
> > > +
> > > +static struct notifier_block cpu_clk_notifier = {
> > > +   .notifier_call = cpu_clk_notifier_fn,
> > > +};
> > > +
> > >  static int apss_ipq6018_probe(struct platform_device *pdev)
> > >  {
> > >     struct regmap *regmap;
> > > +   u32 soc_id;
> > > +   int ret;
> > > +
> > > +   ret = qcom_smem_get_soc_id(&soc_id);
> > > +   if (ret)
> > > +           return ret;
> > >
> > >     regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > >     if (!regmap)
> > >             return -ENODEV;
> > >
> > > -   return qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> > > +   ret = qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
> > > +   if (ret)
> > > +           return ret;
> > > +
> > > +   switch (soc_id) {
> > > +   /*
> > > +    * Only below variants of IPQ53xx support scaling
> > > +    */
> > 1. /* Keep this in a 1-line comment */
>
> Ok
>
> > 2. why? explain the reasoning in the commit message
>
> Ok
>
> Thanks
> Varada
>
> > > +   case QCOM_ID_IPQ5332:
> > > +   case QCOM_ID_IPQ5322:
> > > +   case QCOM_ID_IPQ5300:
> > > +           ret = clk_notifier_register(apcs_alias0_clk_src.clkr.hw.clk,
> > > +                                           &cpu_clk_notifier);
> > > +           if (ret)
> > > +                   return ret;
> > > +           break;
> > > +   default:
> > > +           break;
> > > +   }
> > > +
> > > +   return 0;
> > >  }
> > >
> > >  static struct platform_driver apss_ipq6018_driver = {

Patch

diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
index 4e13a08..ffb6ab5 100644
--- a/drivers/clk/qcom/apss-ipq6018.c
+++ b/drivers/clk/qcom/apss-ipq6018.c
@@ -9,8 +9,11 @@ 
 #include <linux/clk-provider.h>
 #include <linux/regmap.h>
 #include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/soc/qcom/smem.h>
 
 #include <dt-bindings/clock/qcom,apss-ipq.h>
+#include <dt-bindings/arm/qcom,ids.h>
 
 #include "common.h"
 #include "clk-regmap.h"
@@ -84,15 +87,64 @@  static const struct qcom_cc_desc apss_ipq6018_desc = {
 	.num_clks = ARRAY_SIZE(apss_ipq6018_clks),
 };
 
+static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
+				void *data)
+{
+	u8 index;
+	int err;
+
+	if (action == PRE_RATE_CHANGE)
+		index = P_GPLL0;
+	else if (action == POST_RATE_CHANGE)
+		index = P_APSS_PLL_EARLY;
+	else
+		return 0;
+
+	err = clk_rcg2_mux_closest_ops.set_parent(&apcs_alias0_clk_src.clkr.hw,
+						  index);
+
+	return notifier_from_errno(err);
+}
+
+static struct notifier_block cpu_clk_notifier = {
+	.notifier_call = cpu_clk_notifier_fn,
+};
+
 static int apss_ipq6018_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
+	u32 soc_id;
+	int ret;
+
+	ret = qcom_smem_get_soc_id(&soc_id);
+	if (ret)
+		return ret;
 
 	regmap = dev_get_regmap(pdev->dev.parent, NULL);
 	if (!regmap)
 		return -ENODEV;
 
-	return qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &apss_ipq6018_desc, regmap);
+	if (ret)
+		return ret;
+
+	switch (soc_id) {
+	/*
+	 * Only below variants of IPQ53xx support scaling
+	 */
+	case QCOM_ID_IPQ5332:
+	case QCOM_ID_IPQ5322:
+	case QCOM_ID_IPQ5300:
+		ret = clk_notifier_register(apcs_alias0_clk_src.clkr.hw.clk,
+						&cpu_clk_notifier);
+		if (ret)
+			return ret;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
 }
 
 static struct platform_driver apss_ipq6018_driver = {