[PATCH V3 0/3] cpufreq: dt: Allow platforms to provide suspend/resume hooks

[PATCH V3 0/3] cpufreq: dt: Allow platforms to provide suspend/resume hooks

[Viresh Kumar]

Hi Rafael,

These patches are already tested by Miquel and are ready to get merged.
Please pick them as part of 4.18 stuff whenever you do that.

I have updated the cpufreq-dt driver to allow platform specific
suspend/resume hooks and done some minor cleanup in the driver. I have
updated Miquel's patch and applied that on top of all this.

Thanks.

V2->V3:
- Don't double put the clock.
- Fix commit log of 3/3.
- Add tested-by from Miquel.
- There was no V2 of 1-2/3 but only 3/3 on the lists. All are marked as
  V3 to clear things out.

--
viresh

[1] https://lkml.kernel.org/r/20180421141943.25705-1-miquel.raynal@bootlin.com

Miquel Raynal (1):
  cpufreq: add suspend/resume support in Armada 37xx DVFS driver

Viresh Kumar (2):
  cpufreq: dt: Allow platform specific suspend/resume callbacks
  cpufreq: armada: Free resources on error paths

 drivers/cpufreq/armada-37xx-cpufreq.c | 100 +++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq-dt.c          |  10 +++-
 drivers/cpufreq/cpufreq-dt.h          |   5 ++
 3 files changed, 100 insertions(+), 15 deletions(-)

-- 
2.15.0.194.g9af6a3dea062

[PATCH V3 0/3] cpufreq: dt: Allow platforms to provide suspend/resume hooks

[Viresh Kumar]

Hi Rafael,

These patches are already tested by Miquel and are ready to get merged.
Please pick them as part of 4.18 stuff whenever you do that.

I have updated the cpufreq-dt driver to allow platform specific
suspend/resume hooks and done some minor cleanup in the driver. I have
updated Miquel's patch and applied that on top of all this.

Thanks.

V2->V3:
- Don't double put the clock.
- Fix commit log of 3/3.
- Add tested-by from Miquel.
- There was no V2 of 1-2/3 but only 3/3 on the lists. All are marked as
  V3 to clear things out.

--
viresh

[1] https://lkml.kernel.org/r/20180421141943.25705-1-miquel.raynal at bootlin.com

Miquel Raynal (1):
  cpufreq: add suspend/resume support in Armada 37xx DVFS driver

Viresh Kumar (2):
  cpufreq: dt: Allow platform specific suspend/resume callbacks
  cpufreq: armada: Free resources on error paths

 drivers/cpufreq/armada-37xx-cpufreq.c | 100 +++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq-dt.c          |  10 +++-
 drivers/cpufreq/cpufreq-dt.h          |   5 ++
 3 files changed, 100 insertions(+), 15 deletions(-)

-- 
2.15.0.194.g9af6a3dea062

[PATCH V3 2/3] cpufreq: armada: Free resources on error paths

[Viresh Kumar]

The resources weren't freed on failures, free them properly.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 72a2975499db..1d5db6f6ace9 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -166,6 +166,7 @@ static int __init armada37xx_cpufreq_driver_init(void)
 {
 	struct armada_37xx_dvfs *dvfs;
 	struct platform_device *pdev;
+	unsigned long freq;
 	unsigned int cur_frequency;
 	struct regmap *nb_pm_base;
 	struct device *cpu_dev;
@@ -207,33 +208,43 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	}
 
 	dvfs = armada_37xx_cpu_freq_info_get(cur_frequency);
-	if (!dvfs)
+	if (!dvfs) {
+		clk_put(clk);
 		return -EINVAL;
+	}
 
 	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
 	clk_put(clk);
 
 	for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR;
 	     load_lvl++) {
-		unsigned long freq = cur_frequency / dvfs->divider[load_lvl];
+		freq = cur_frequency / dvfs->divider[load_lvl];
 
 		ret = dev_pm_opp_add(cpu_dev, freq, 0);
-		if (ret) {
-			/* clean-up the already added opp before leaving */
-			while (load_lvl-- > ARMADA_37XX_DVFS_LOAD_0) {
-				freq = cur_frequency / dvfs->divider[load_lvl];
-				dev_pm_opp_remove(cpu_dev, freq);
-			}
-			return ret;
-		}
+		if (ret)
+			goto remove_opp;
 	}
 
 	/* Now that everything is setup, enable the DVFS at hardware level */
 	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
 
 	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	ret = PTR_ERR_OR_ZERO(pdev);
+	if (ret)
+		goto disable_dvfs;
+
+	return 0;
+
+disable_dvfs:
+	armada37xx_cpufreq_disable_dvfs(nb_pm_base);
+remove_opp:
+	/* clean-up the already added opp before leaving */
+	while (load_lvl-- > ARMADA_37XX_DVFS_LOAD_0) {
+		freq = cur_frequency / dvfs->divider[load_lvl];
+		dev_pm_opp_remove(cpu_dev, freq);
+	}
 
-	return PTR_ERR_OR_ZERO(pdev);
+	return ret;
 }
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
 late_initcall(armada37xx_cpufreq_driver_init);
-- 
2.15.0.194.g9af6a3dea062

[PATCH V3 2/3] cpufreq: armada: Free resources on error paths

[Viresh Kumar]

The resources weren't freed on failures, free them properly.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 72a2975499db..1d5db6f6ace9 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -166,6 +166,7 @@ static int __init armada37xx_cpufreq_driver_init(void)
 {
 	struct armada_37xx_dvfs *dvfs;
 	struct platform_device *pdev;
+	unsigned long freq;
 	unsigned int cur_frequency;
 	struct regmap *nb_pm_base;
 	struct device *cpu_dev;
@@ -207,33 +208,43 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	}
 
 	dvfs = armada_37xx_cpu_freq_info_get(cur_frequency);
-	if (!dvfs)
+	if (!dvfs) {
+		clk_put(clk);
 		return -EINVAL;
+	}
 
 	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
 	clk_put(clk);
 
 	for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR;
 	     load_lvl++) {
-		unsigned long freq = cur_frequency / dvfs->divider[load_lvl];
+		freq = cur_frequency / dvfs->divider[load_lvl];
 
 		ret = dev_pm_opp_add(cpu_dev, freq, 0);
-		if (ret) {
-			/* clean-up the already added opp before leaving */
-			while (load_lvl-- > ARMADA_37XX_DVFS_LOAD_0) {
-				freq = cur_frequency / dvfs->divider[load_lvl];
-				dev_pm_opp_remove(cpu_dev, freq);
-			}
-			return ret;
-		}
+		if (ret)
+			goto remove_opp;
 	}
 
 	/* Now that everything is setup, enable the DVFS at hardware level */
 	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
 
 	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	ret = PTR_ERR_OR_ZERO(pdev);
+	if (ret)
+		goto disable_dvfs;
+
+	return 0;
+
+disable_dvfs:
+	armada37xx_cpufreq_disable_dvfs(nb_pm_base);
+remove_opp:
+	/* clean-up the already added opp before leaving */
+	while (load_lvl-- > ARMADA_37XX_DVFS_LOAD_0) {
+		freq = cur_frequency / dvfs->divider[load_lvl];
+		dev_pm_opp_remove(cpu_dev, freq);
+	}
 
-	return PTR_ERR_OR_ZERO(pdev);
+	return ret;
 }
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
 late_initcall(armada37xx_cpufreq_driver_init);
-- 
2.15.0.194.g9af6a3dea062

[PATCH V3 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
operations. As there is currently no 'driver' structure, create one
to store both the regmap and the register values during suspend
operation.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 67 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 1d5db6f6ace9..5b808882ac8b 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -23,6 +23,8 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
+#include "cpufreq-dt.h"
+
 /* Power management in North Bridge register set */
 #define ARMADA_37XX_NB_L0L1	0x18
 #define ARMADA_37XX_NB_L2L3	0x1C
@@ -56,6 +58,16 @@
  */
 #define LOAD_LEVEL_NR	4
 
+struct armada37xx_cpufreq_state {
+	struct regmap *regmap;
+	u32 nb_l0l1;
+	u32 nb_l2l3;
+	u32 nb_dyn_mod;
+	u32 nb_cpu_load;
+};
+
+static struct armada37xx_cpufreq_state *armada37xx_cpufreq_state;
+
 struct armada_37xx_dvfs {
 	u32 cpu_freq_max;
 	u8 divider[LOAD_LEVEL_NR];
@@ -136,7 +148,7 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
 	clk_set_parent(clk, parent);
 }
 
-static void __init armada37xx_cpufreq_disable_dvfs(struct regmap *base)
+static void armada37xx_cpufreq_disable_dvfs(struct regmap *base)
 {
 	unsigned int reg = ARMADA_37XX_NB_DYN_MOD,
 		mask = ARMADA_37XX_NB_DFS_EN;
@@ -162,8 +174,44 @@ static void __init armada37xx_cpufreq_enable_dvfs(struct regmap *base)
 	regmap_update_bits(base, reg, mask, mask);
 }
 
+static int armada37xx_cpufreq_suspend(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	regmap_read(state->regmap, ARMADA_37XX_NB_L0L1, &state->nb_l0l1);
+	regmap_read(state->regmap, ARMADA_37XX_NB_L2L3, &state->nb_l2l3);
+	regmap_read(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		    &state->nb_cpu_load);
+	regmap_read(state->regmap, ARMADA_37XX_NB_DYN_MOD, &state->nb_dyn_mod);
+
+	return 0;
+}
+
+static int armada37xx_cpufreq_resume(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	/* Ensure DVFS is disabled otherwise the following registers are RO */
+	armada37xx_cpufreq_disable_dvfs(state->regmap);
+
+	regmap_write(state->regmap, ARMADA_37XX_NB_L0L1, state->nb_l0l1);
+	regmap_write(state->regmap, ARMADA_37XX_NB_L2L3, state->nb_l2l3);
+	regmap_write(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		     state->nb_cpu_load);
+
+	/*
+	 * NB_DYN_MOD register is the one that actually enable back DVFS if it
+	 * was enabled before the suspend operation. This must be done last
+	 * otherwise other registers are not writable.
+	 */
+	regmap_write(state->regmap, ARMADA_37XX_NB_DYN_MOD, state->nb_dyn_mod);
+
+	return 0;
+}
+
 static int __init armada37xx_cpufreq_driver_init(void)
 {
+	struct cpufreq_dt_platform_data pdata;
 	struct armada_37xx_dvfs *dvfs;
 	struct platform_device *pdev;
 	unsigned long freq;
@@ -213,6 +261,15 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		return -EINVAL;
 	}
 
+	armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
+					   GFP_KERNEL);
+	if (!armada37xx_cpufreq_state) {
+		ret = -ENOMEM;
+		goto put_clk;
+	}
+
+	armada37xx_cpufreq_state->regmap = nb_pm_base;
+
 	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
 	clk_put(clk);
 
@@ -228,7 +285,11 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	/* Now that everything is setup, enable the DVFS at hardware level */
 	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
 
-	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	pdata.suspend = armada37xx_cpufreq_suspend;
+	pdata.resume = armada37xx_cpufreq_resume;
+
+	pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
+					     sizeof(pdata));
 	ret = PTR_ERR_OR_ZERO(pdev);
 	if (ret)
 		goto disable_dvfs;
@@ -244,6 +305,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		dev_pm_opp_remove(cpu_dev, freq);
 	}
 
+	kfree(armada37xx_cpufreq_state);
+
 	return ret;
 }
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
-- 
2.15.0.194.g9af6a3dea062

[PATCH V3 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
operations. As there is currently no 'driver' structure, create one
to store both the regmap and the register values during suspend
operation.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 67 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 1d5db6f6ace9..5b808882ac8b 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -23,6 +23,8 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
+#include "cpufreq-dt.h"
+
 /* Power management in North Bridge register set */
 #define ARMADA_37XX_NB_L0L1	0x18
 #define ARMADA_37XX_NB_L2L3	0x1C
@@ -56,6 +58,16 @@
  */
 #define LOAD_LEVEL_NR	4
 
+struct armada37xx_cpufreq_state {
+	struct regmap *regmap;
+	u32 nb_l0l1;
+	u32 nb_l2l3;
+	u32 nb_dyn_mod;
+	u32 nb_cpu_load;
+};
+
+static struct armada37xx_cpufreq_state *armada37xx_cpufreq_state;
+
 struct armada_37xx_dvfs {
 	u32 cpu_freq_max;
 	u8 divider[LOAD_LEVEL_NR];
@@ -136,7 +148,7 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
 	clk_set_parent(clk, parent);
 }
 
-static void __init armada37xx_cpufreq_disable_dvfs(struct regmap *base)
+static void armada37xx_cpufreq_disable_dvfs(struct regmap *base)
 {
 	unsigned int reg = ARMADA_37XX_NB_DYN_MOD,
 		mask = ARMADA_37XX_NB_DFS_EN;
@@ -162,8 +174,44 @@ static void __init armada37xx_cpufreq_enable_dvfs(struct regmap *base)
 	regmap_update_bits(base, reg, mask, mask);
 }
 
+static int armada37xx_cpufreq_suspend(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	regmap_read(state->regmap, ARMADA_37XX_NB_L0L1, &state->nb_l0l1);
+	regmap_read(state->regmap, ARMADA_37XX_NB_L2L3, &state->nb_l2l3);
+	regmap_read(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		    &state->nb_cpu_load);
+	regmap_read(state->regmap, ARMADA_37XX_NB_DYN_MOD, &state->nb_dyn_mod);
+
+	return 0;
+}
+
+static int armada37xx_cpufreq_resume(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	/* Ensure DVFS is disabled otherwise the following registers are RO */
+	armada37xx_cpufreq_disable_dvfs(state->regmap);
+
+	regmap_write(state->regmap, ARMADA_37XX_NB_L0L1, state->nb_l0l1);
+	regmap_write(state->regmap, ARMADA_37XX_NB_L2L3, state->nb_l2l3);
+	regmap_write(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		     state->nb_cpu_load);
+
+	/*
+	 * NB_DYN_MOD register is the one that actually enable back DVFS if it
+	 * was enabled before the suspend operation. This must be done last
+	 * otherwise other registers are not writable.
+	 */
+	regmap_write(state->regmap, ARMADA_37XX_NB_DYN_MOD, state->nb_dyn_mod);
+
+	return 0;
+}
+
 static int __init armada37xx_cpufreq_driver_init(void)
 {
+	struct cpufreq_dt_platform_data pdata;
 	struct armada_37xx_dvfs *dvfs;
 	struct platform_device *pdev;
 	unsigned long freq;
@@ -213,6 +261,15 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		return -EINVAL;
 	}
 
+	armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
+					   GFP_KERNEL);
+	if (!armada37xx_cpufreq_state) {
+		ret = -ENOMEM;
+		goto put_clk;
+	}
+
+	armada37xx_cpufreq_state->regmap = nb_pm_base;
+
 	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
 	clk_put(clk);
 
@@ -228,7 +285,11 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	/* Now that everything is setup, enable the DVFS at hardware level */
 	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
 
-	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	pdata.suspend = armada37xx_cpufreq_suspend;
+	pdata.resume = armada37xx_cpufreq_resume;
+
+	pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
+					     sizeof(pdata));
 	ret = PTR_ERR_OR_ZERO(pdev);
 	if (ret)
 		goto disable_dvfs;
@@ -244,6 +305,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		dev_pm_opp_remove(cpu_dev, freq);
 	}
 
+	kfree(armada37xx_cpufreq_state);
+
 	return ret;
 }
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
-- 
2.15.0.194.g9af6a3dea062

Re: [PATCH V3 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4562 bytes --]

Hi Miquel,

I love your patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.17-rc2 next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/cpufreq-dt-Allow-platforms-to-provide-suspend-resume-hooks/20180426-101030
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/cpufreq/armada-37xx-cpufreq.c: In function 'armada37xx_cpufreq_driver_init':
>> drivers/cpufreq/armada-37xx-cpufreq.c:268:3: error: label 'put_clk' used but not defined
      goto put_clk;
      ^~~~

vim +/put_clk +268 drivers/cpufreq/armada-37xx-cpufreq.c

   211	
   212	static int __init armada37xx_cpufreq_driver_init(void)
   213	{
   214		struct cpufreq_dt_platform_data pdata;
   215		struct armada_37xx_dvfs *dvfs;
   216		struct platform_device *pdev;
   217		unsigned long freq;
   218		unsigned int cur_frequency;
   219		struct regmap *nb_pm_base;
   220		struct device *cpu_dev;
   221		int load_lvl, ret;
   222		struct clk *clk;
   223	
   224		nb_pm_base =
   225			syscon_regmap_lookup_by_compatible("marvell,armada-3700-nb-pm");
   226	
   227		if (IS_ERR(nb_pm_base))
   228			return -ENODEV;
   229	
   230		/* Before doing any configuration on the DVFS first, disable it */
   231		armada37xx_cpufreq_disable_dvfs(nb_pm_base);
   232	
   233		/*
   234		 * On CPU 0 register the operating points supported (which are
   235		 * the nominal CPU frequency and full integer divisions of
   236		 * it).
   237		 */
   238		cpu_dev = get_cpu_device(0);
   239		if (!cpu_dev) {
   240			dev_err(cpu_dev, "Cannot get CPU\n");
   241			return -ENODEV;
   242		}
   243	
   244		clk = clk_get(cpu_dev, 0);
   245		if (IS_ERR(clk)) {
   246			dev_err(cpu_dev, "Cannot get clock for CPU0\n");
   247			return PTR_ERR(clk);
   248		}
   249	
   250		/* Get nominal (current) CPU frequency */
   251		cur_frequency = clk_get_rate(clk);
   252		if (!cur_frequency) {
   253			dev_err(cpu_dev, "Failed to get clock rate for CPU\n");
   254			clk_put(clk);
   255			return -EINVAL;
   256		}
   257	
   258		dvfs = armada_37xx_cpu_freq_info_get(cur_frequency);
   259		if (!dvfs) {
   260			clk_put(clk);
   261			return -EINVAL;
   262		}
   263	
   264		armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
   265						   GFP_KERNEL);
   266		if (!armada37xx_cpufreq_state) {
   267			ret = -ENOMEM;
 > 268			goto put_clk;
   269		}
   270	
   271		armada37xx_cpufreq_state->regmap = nb_pm_base;
   272	
   273		armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
   274		clk_put(clk);
   275	
   276		for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR;
   277		     load_lvl++) {
   278			freq = cur_frequency / dvfs->divider[load_lvl];
   279	
   280			ret = dev_pm_opp_add(cpu_dev, freq, 0);
   281			if (ret)
   282				goto remove_opp;
   283		}
   284	
   285		/* Now that everything is setup, enable the DVFS at hardware level */
   286		armada37xx_cpufreq_enable_dvfs(nb_pm_base);
   287	
   288		pdata.suspend = armada37xx_cpufreq_suspend;
   289		pdata.resume = armada37xx_cpufreq_resume;
   290	
   291		pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
   292						     sizeof(pdata));
   293		ret = PTR_ERR_OR_ZERO(pdev);
   294		if (ret)
   295			goto disable_dvfs;
   296	
   297		return 0;
   298	
   299	disable_dvfs:
   300		armada37xx_cpufreq_disable_dvfs(nb_pm_base);
   301	remove_opp:
   302		/* clean-up the already added opp before leaving */
   303		while (load_lvl-- > ARMADA_37XX_DVFS_LOAD_0) {
   304			freq = cur_frequency / dvfs->divider[load_lvl];
   305			dev_pm_opp_remove(cpu_dev, freq);
   306		}
   307	
   308		kfree(armada37xx_cpufreq_state);
   309	
   310		return ret;
   311	}
   312	/* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
   313	late_initcall(armada37xx_cpufreq_driver_init);
   314	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65227 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V3 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[kbuild test robot]

Hi Miquel,

I love your patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.17-rc2 next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/cpufreq-dt-Allow-platforms-to-provide-suspend-resume-hooks/20180426-101030
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/cpufreq/armada-37xx-cpufreq.c: In function 'armada37xx_cpufreq_driver_init':
>> drivers/cpufreq/armada-37xx-cpufreq.c:268:3: error: label 'put_clk' used but not defined
      goto put_clk;
      ^~~~

vim +/put_clk +268 drivers/cpufreq/armada-37xx-cpufreq.c

   211	
   212	static int __init armada37xx_cpufreq_driver_init(void)
   213	{
   214		struct cpufreq_dt_platform_data pdata;
   215		struct armada_37xx_dvfs *dvfs;
   216		struct platform_device *pdev;
   217		unsigned long freq;
   218		unsigned int cur_frequency;
   219		struct regmap *nb_pm_base;
   220		struct device *cpu_dev;
   221		int load_lvl, ret;
   222		struct clk *clk;
   223	
   224		nb_pm_base =
   225			syscon_regmap_lookup_by_compatible("marvell,armada-3700-nb-pm");
   226	
   227		if (IS_ERR(nb_pm_base))
   228			return -ENODEV;
   229	
   230		/* Before doing any configuration on the DVFS first, disable it */
   231		armada37xx_cpufreq_disable_dvfs(nb_pm_base);
   232	
   233		/*
   234		 * On CPU 0 register the operating points supported (which are
   235		 * the nominal CPU frequency and full integer divisions of
   236		 * it).
   237		 */
   238		cpu_dev = get_cpu_device(0);
   239		if (!cpu_dev) {
   240			dev_err(cpu_dev, "Cannot get CPU\n");
   241			return -ENODEV;
   242		}
   243	
   244		clk = clk_get(cpu_dev, 0);
   245		if (IS_ERR(clk)) {
   246			dev_err(cpu_dev, "Cannot get clock for CPU0\n");
   247			return PTR_ERR(clk);
   248		}
   249	
   250		/* Get nominal (current) CPU frequency */
   251		cur_frequency = clk_get_rate(clk);
   252		if (!cur_frequency) {
   253			dev_err(cpu_dev, "Failed to get clock rate for CPU\n");
   254			clk_put(clk);
   255			return -EINVAL;
   256		}
   257	
   258		dvfs = armada_37xx_cpu_freq_info_get(cur_frequency);
   259		if (!dvfs) {
   260			clk_put(clk);
   261			return -EINVAL;
   262		}
   263	
   264		armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
   265						   GFP_KERNEL);
   266		if (!armada37xx_cpufreq_state) {
   267			ret = -ENOMEM;
 > 268			goto put_clk;
   269		}
   270	
   271		armada37xx_cpufreq_state->regmap = nb_pm_base;
   272	
   273		armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
   274		clk_put(clk);
   275	
   276		for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR;
   277		     load_lvl++) {
   278			freq = cur_frequency / dvfs->divider[load_lvl];
   279	
   280			ret = dev_pm_opp_add(cpu_dev, freq, 0);
   281			if (ret)
   282				goto remove_opp;
   283		}
   284	
   285		/* Now that everything is setup, enable the DVFS at hardware level */
   286		armada37xx_cpufreq_enable_dvfs(nb_pm_base);
   287	
   288		pdata.suspend = armada37xx_cpufreq_suspend;
   289		pdata.resume = armada37xx_cpufreq_resume;
   290	
   291		pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
   292						     sizeof(pdata));
   293		ret = PTR_ERR_OR_ZERO(pdev);
   294		if (ret)
   295			goto disable_dvfs;
   296	
   297		return 0;
   298	
   299	disable_dvfs:
   300		armada37xx_cpufreq_disable_dvfs(nb_pm_base);
   301	remove_opp:
   302		/* clean-up the already added opp before leaving */
   303		while (load_lvl-- > ARMADA_37XX_DVFS_LOAD_0) {
   304			freq = cur_frequency / dvfs->divider[load_lvl];
   305			dev_pm_opp_remove(cpu_dev, freq);
   306		}
   307	
   308		kfree(armada37xx_cpufreq_state);
   309	
   310		return ret;
   311	}
   312	/* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
   313	late_initcall(armada37xx_cpufreq_driver_init);
   314	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 65227 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180426/314b0f77/attachment-0001.gz>

[PATCH V4 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
operations. As there is currently no 'driver' structure, create one
to store both the regmap and the register values during suspend
operation.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V3->V4:
- Fix the "unused put_clk label" build error that happened between V2
  and V3.

 drivers/cpufreq/armada-37xx-cpufreq.c | 67 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 1d5db6f6ace9..739da90ff3f6 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -23,6 +23,8 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
+#include "cpufreq-dt.h"
+
 /* Power management in North Bridge register set */
 #define ARMADA_37XX_NB_L0L1	0x18
 #define ARMADA_37XX_NB_L2L3	0x1C
@@ -56,6 +58,16 @@
  */
 #define LOAD_LEVEL_NR	4
 
+struct armada37xx_cpufreq_state {
+	struct regmap *regmap;
+	u32 nb_l0l1;
+	u32 nb_l2l3;
+	u32 nb_dyn_mod;
+	u32 nb_cpu_load;
+};
+
+static struct armada37xx_cpufreq_state *armada37xx_cpufreq_state;
+
 struct armada_37xx_dvfs {
 	u32 cpu_freq_max;
 	u8 divider[LOAD_LEVEL_NR];
@@ -136,7 +148,7 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
 	clk_set_parent(clk, parent);
 }
 
-static void __init armada37xx_cpufreq_disable_dvfs(struct regmap *base)
+static void armada37xx_cpufreq_disable_dvfs(struct regmap *base)
 {
 	unsigned int reg = ARMADA_37XX_NB_DYN_MOD,
 		mask = ARMADA_37XX_NB_DFS_EN;
@@ -162,8 +174,44 @@ static void __init armada37xx_cpufreq_enable_dvfs(struct regmap *base)
 	regmap_update_bits(base, reg, mask, mask);
 }
 
+static int armada37xx_cpufreq_suspend(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	regmap_read(state->regmap, ARMADA_37XX_NB_L0L1, &state->nb_l0l1);
+	regmap_read(state->regmap, ARMADA_37XX_NB_L2L3, &state->nb_l2l3);
+	regmap_read(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		    &state->nb_cpu_load);
+	regmap_read(state->regmap, ARMADA_37XX_NB_DYN_MOD, &state->nb_dyn_mod);
+
+	return 0;
+}
+
+static int armada37xx_cpufreq_resume(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	/* Ensure DVFS is disabled otherwise the following registers are RO */
+	armada37xx_cpufreq_disable_dvfs(state->regmap);
+
+	regmap_write(state->regmap, ARMADA_37XX_NB_L0L1, state->nb_l0l1);
+	regmap_write(state->regmap, ARMADA_37XX_NB_L2L3, state->nb_l2l3);
+	regmap_write(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		     state->nb_cpu_load);
+
+	/*
+	 * NB_DYN_MOD register is the one that actually enable back DVFS if it
+	 * was enabled before the suspend operation. This must be done last
+	 * otherwise other registers are not writable.
+	 */
+	regmap_write(state->regmap, ARMADA_37XX_NB_DYN_MOD, state->nb_dyn_mod);
+
+	return 0;
+}
+
 static int __init armada37xx_cpufreq_driver_init(void)
 {
+	struct cpufreq_dt_platform_data pdata;
 	struct armada_37xx_dvfs *dvfs;
 	struct platform_device *pdev;
 	unsigned long freq;
@@ -213,6 +261,15 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		return -EINVAL;
 	}
 
+	armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
+					   GFP_KERNEL);
+	if (!armada37xx_cpufreq_state) {
+		clk_put(clk);
+		return -ENOMEM;
+	}
+
+	armada37xx_cpufreq_state->regmap = nb_pm_base;
+
 	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
 	clk_put(clk);
 
@@ -228,7 +285,11 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	/* Now that everything is setup, enable the DVFS at hardware level */
 	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
 
-	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	pdata.suspend = armada37xx_cpufreq_suspend;
+	pdata.resume = armada37xx_cpufreq_resume;
+
+	pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
+					     sizeof(pdata));
 	ret = PTR_ERR_OR_ZERO(pdev);
 	if (ret)
 		goto disable_dvfs;
@@ -244,6 +305,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		dev_pm_opp_remove(cpu_dev, freq);
 	}
 
+	kfree(armada37xx_cpufreq_state);
+
 	return ret;
 }
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
-- 
2.15.0.194.g9af6a3dea062

[PATCH V4 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
operations. As there is currently no 'driver' structure, create one
to store both the regmap and the register values during suspend
operation.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V3->V4:
- Fix the "unused put_clk label" build error that happened between V2
  and V3.

 drivers/cpufreq/armada-37xx-cpufreq.c | 67 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 1d5db6f6ace9..739da90ff3f6 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -23,6 +23,8 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
+#include "cpufreq-dt.h"
+
 /* Power management in North Bridge register set */
 #define ARMADA_37XX_NB_L0L1	0x18
 #define ARMADA_37XX_NB_L2L3	0x1C
@@ -56,6 +58,16 @@
  */
 #define LOAD_LEVEL_NR	4
 
+struct armada37xx_cpufreq_state {
+	struct regmap *regmap;
+	u32 nb_l0l1;
+	u32 nb_l2l3;
+	u32 nb_dyn_mod;
+	u32 nb_cpu_load;
+};
+
+static struct armada37xx_cpufreq_state *armada37xx_cpufreq_state;
+
 struct armada_37xx_dvfs {
 	u32 cpu_freq_max;
 	u8 divider[LOAD_LEVEL_NR];
@@ -136,7 +148,7 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
 	clk_set_parent(clk, parent);
 }
 
-static void __init armada37xx_cpufreq_disable_dvfs(struct regmap *base)
+static void armada37xx_cpufreq_disable_dvfs(struct regmap *base)
 {
 	unsigned int reg = ARMADA_37XX_NB_DYN_MOD,
 		mask = ARMADA_37XX_NB_DFS_EN;
@@ -162,8 +174,44 @@ static void __init armada37xx_cpufreq_enable_dvfs(struct regmap *base)
 	regmap_update_bits(base, reg, mask, mask);
 }
 
+static int armada37xx_cpufreq_suspend(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	regmap_read(state->regmap, ARMADA_37XX_NB_L0L1, &state->nb_l0l1);
+	regmap_read(state->regmap, ARMADA_37XX_NB_L2L3, &state->nb_l2l3);
+	regmap_read(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		    &state->nb_cpu_load);
+	regmap_read(state->regmap, ARMADA_37XX_NB_DYN_MOD, &state->nb_dyn_mod);
+
+	return 0;
+}
+
+static int armada37xx_cpufreq_resume(struct cpufreq_policy *policy)
+{
+	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
+
+	/* Ensure DVFS is disabled otherwise the following registers are RO */
+	armada37xx_cpufreq_disable_dvfs(state->regmap);
+
+	regmap_write(state->regmap, ARMADA_37XX_NB_L0L1, state->nb_l0l1);
+	regmap_write(state->regmap, ARMADA_37XX_NB_L2L3, state->nb_l2l3);
+	regmap_write(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
+		     state->nb_cpu_load);
+
+	/*
+	 * NB_DYN_MOD register is the one that actually enable back DVFS if it
+	 * was enabled before the suspend operation. This must be done last
+	 * otherwise other registers are not writable.
+	 */
+	regmap_write(state->regmap, ARMADA_37XX_NB_DYN_MOD, state->nb_dyn_mod);
+
+	return 0;
+}
+
 static int __init armada37xx_cpufreq_driver_init(void)
 {
+	struct cpufreq_dt_platform_data pdata;
 	struct armada_37xx_dvfs *dvfs;
 	struct platform_device *pdev;
 	unsigned long freq;
@@ -213,6 +261,15 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		return -EINVAL;
 	}
 
+	armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
+					   GFP_KERNEL);
+	if (!armada37xx_cpufreq_state) {
+		clk_put(clk);
+		return -ENOMEM;
+	}
+
+	armada37xx_cpufreq_state->regmap = nb_pm_base;
+
 	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
 	clk_put(clk);
 
@@ -228,7 +285,11 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	/* Now that everything is setup, enable the DVFS at hardware level */
 	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
 
-	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	pdata.suspend = armada37xx_cpufreq_suspend;
+	pdata.resume = armada37xx_cpufreq_resume;
+
+	pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
+					     sizeof(pdata));
 	ret = PTR_ERR_OR_ZERO(pdev);
 	if (ret)
 		goto disable_dvfs;
@@ -244,6 +305,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		dev_pm_opp_remove(cpu_dev, freq);
 	}
 
+	kfree(armada37xx_cpufreq_state);
+
 	return ret;
 }
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
-- 
2.15.0.194.g9af6a3dea062

Re: [PATCH V3 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

On 26-04-18, 17:32, kbuild test robot wrote:
> Hi Miquel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on pm/linux-next]
> [also build test ERROR on v4.17-rc2 next-20180424]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/cpufreq-dt-Allow-platforms-to-provide-suspend-resume-hooks/20180426-101030
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/cpufreq/armada-37xx-cpufreq.c: In function 'armada37xx_cpufreq_driver_init':
> >> drivers/cpufreq/armada-37xx-cpufreq.c:268:3: error: label 'put_clk' used but not defined
>       goto put_clk;
>       ^~~~

Fixed, thanks.

-- 
viresh

[PATCH V3 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

On 26-04-18, 17:32, kbuild test robot wrote:
> Hi Miquel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on pm/linux-next]
> [also build test ERROR on v4.17-rc2 next-20180424]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/cpufreq-dt-Allow-platforms-to-provide-suspend-resume-hooks/20180426-101030
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/cpufreq/armada-37xx-cpufreq.c: In function 'armada37xx_cpufreq_driver_init':
> >> drivers/cpufreq/armada-37xx-cpufreq.c:268:3: error: label 'put_clk' used but not defined
>       goto put_clk;
>       ^~~~

Fixed, thanks.

-- 
viresh

Re: [PATCH V4 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Gregory CLEMENT]

Hi Viresh,
 
 On jeu., avril 26 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> From: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
> operations. As there is currently no 'driver' structure, create one
> to store both the regmap and the register values during suspend
> operation.

I tested this patch on a Armada 3720 based board (EspressoBin), and it
worked well. I even made some power consumption measurement and after
the resume, the power consumption had the same value that before the
suspend, for low CPU frequency and high CPU frequency.

Tested-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory



>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V3->V4:
> - Fix the "unused put_clk label" build error that happened between V2
>   and V3.
>
>  drivers/cpufreq/armada-37xx-cpufreq.c | 67 +++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index 1d5db6f6ace9..739da90ff3f6 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -23,6 +23,8 @@
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  
> +#include "cpufreq-dt.h"
> +
>  /* Power management in North Bridge register set */
>  #define ARMADA_37XX_NB_L0L1	0x18
>  #define ARMADA_37XX_NB_L2L3	0x1C
> @@ -56,6 +58,16 @@
>   */
>  #define LOAD_LEVEL_NR	4
>  
> +struct armada37xx_cpufreq_state {
> +	struct regmap *regmap;
> +	u32 nb_l0l1;
> +	u32 nb_l2l3;
> +	u32 nb_dyn_mod;
> +	u32 nb_cpu_load;
> +};
> +
> +static struct armada37xx_cpufreq_state *armada37xx_cpufreq_state;
> +
>  struct armada_37xx_dvfs {
>  	u32 cpu_freq_max;
>  	u8 divider[LOAD_LEVEL_NR];
> @@ -136,7 +148,7 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
>  	clk_set_parent(clk, parent);
>  }
>  
> -static void __init armada37xx_cpufreq_disable_dvfs(struct regmap *base)
> +static void armada37xx_cpufreq_disable_dvfs(struct regmap *base)
>  {
>  	unsigned int reg = ARMADA_37XX_NB_DYN_MOD,
>  		mask = ARMADA_37XX_NB_DFS_EN;
> @@ -162,8 +174,44 @@ static void __init armada37xx_cpufreq_enable_dvfs(struct regmap *base)
>  	regmap_update_bits(base, reg, mask, mask);
>  }
>  
> +static int armada37xx_cpufreq_suspend(struct cpufreq_policy *policy)
> +{
> +	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
> +
> +	regmap_read(state->regmap, ARMADA_37XX_NB_L0L1, &state->nb_l0l1);
> +	regmap_read(state->regmap, ARMADA_37XX_NB_L2L3, &state->nb_l2l3);
> +	regmap_read(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
> +		    &state->nb_cpu_load);
> +	regmap_read(state->regmap, ARMADA_37XX_NB_DYN_MOD, &state->nb_dyn_mod);
> +
> +	return 0;
> +}
> +
> +static int armada37xx_cpufreq_resume(struct cpufreq_policy *policy)
> +{
> +	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
> +
> +	/* Ensure DVFS is disabled otherwise the following registers are RO */
> +	armada37xx_cpufreq_disable_dvfs(state->regmap);
> +
> +	regmap_write(state->regmap, ARMADA_37XX_NB_L0L1, state->nb_l0l1);
> +	regmap_write(state->regmap, ARMADA_37XX_NB_L2L3, state->nb_l2l3);
> +	regmap_write(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
> +		     state->nb_cpu_load);
> +
> +	/*
> +	 * NB_DYN_MOD register is the one that actually enable back DVFS if it
> +	 * was enabled before the suspend operation. This must be done last
> +	 * otherwise other registers are not writable.
> +	 */
> +	regmap_write(state->regmap, ARMADA_37XX_NB_DYN_MOD, state->nb_dyn_mod);
> +
> +	return 0;
> +}
> +
>  static int __init armada37xx_cpufreq_driver_init(void)
>  {
> +	struct cpufreq_dt_platform_data pdata;
>  	struct armada_37xx_dvfs *dvfs;
>  	struct platform_device *pdev;
>  	unsigned long freq;
> @@ -213,6 +261,15 @@ static int __init armada37xx_cpufreq_driver_init(void)
>  		return -EINVAL;
>  	}
>  
> +	armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
> +					   GFP_KERNEL);
> +	if (!armada37xx_cpufreq_state) {
> +		clk_put(clk);
> +		return -ENOMEM;
> +	}
> +
> +	armada37xx_cpufreq_state->regmap = nb_pm_base;
> +
>  	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
>  	clk_put(clk);
>  
> @@ -228,7 +285,11 @@ static int __init armada37xx_cpufreq_driver_init(void)
>  	/* Now that everything is setup, enable the DVFS at hardware level */
>  	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
>  
> -	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> +	pdata.suspend = armada37xx_cpufreq_suspend;
> +	pdata.resume = armada37xx_cpufreq_resume;
> +
> +	pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
> +					     sizeof(pdata));
>  	ret = PTR_ERR_OR_ZERO(pdev);
>  	if (ret)
>  		goto disable_dvfs;
> @@ -244,6 +305,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
>  		dev_pm_opp_remove(cpu_dev, freq);
>  	}
>  
> +	kfree(armada37xx_cpufreq_state);
> +
>  	return ret;
>  }
>  /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
> -- 
> 2.15.0.194.g9af6a3dea062
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH V4 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Gregory CLEMENT]

Hi Viresh,
 
 On jeu., avril 26 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> From: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
> operations. As there is currently no 'driver' structure, create one
> to store both the regmap and the register values during suspend
> operation.

I tested this patch on a Armada 3720 based board (EspressoBin), and it
worked well. I even made some power consumption measurement and after
the resume, the power consumption had the same value that before the
suspend, for low CPU frequency and high CPU frequency.

Tested-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory



>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V3->V4:
> - Fix the "unused put_clk label" build error that happened between V2
>   and V3.
>
>  drivers/cpufreq/armada-37xx-cpufreq.c | 67 +++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index 1d5db6f6ace9..739da90ff3f6 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -23,6 +23,8 @@
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  
> +#include "cpufreq-dt.h"
> +
>  /* Power management in North Bridge register set */
>  #define ARMADA_37XX_NB_L0L1	0x18
>  #define ARMADA_37XX_NB_L2L3	0x1C
> @@ -56,6 +58,16 @@
>   */
>  #define LOAD_LEVEL_NR	4
>  
> +struct armada37xx_cpufreq_state {
> +	struct regmap *regmap;
> +	u32 nb_l0l1;
> +	u32 nb_l2l3;
> +	u32 nb_dyn_mod;
> +	u32 nb_cpu_load;
> +};
> +
> +static struct armada37xx_cpufreq_state *armada37xx_cpufreq_state;
> +
>  struct armada_37xx_dvfs {
>  	u32 cpu_freq_max;
>  	u8 divider[LOAD_LEVEL_NR];
> @@ -136,7 +148,7 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
>  	clk_set_parent(clk, parent);
>  }
>  
> -static void __init armada37xx_cpufreq_disable_dvfs(struct regmap *base)
> +static void armada37xx_cpufreq_disable_dvfs(struct regmap *base)
>  {
>  	unsigned int reg = ARMADA_37XX_NB_DYN_MOD,
>  		mask = ARMADA_37XX_NB_DFS_EN;
> @@ -162,8 +174,44 @@ static void __init armada37xx_cpufreq_enable_dvfs(struct regmap *base)
>  	regmap_update_bits(base, reg, mask, mask);
>  }
>  
> +static int armada37xx_cpufreq_suspend(struct cpufreq_policy *policy)
> +{
> +	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
> +
> +	regmap_read(state->regmap, ARMADA_37XX_NB_L0L1, &state->nb_l0l1);
> +	regmap_read(state->regmap, ARMADA_37XX_NB_L2L3, &state->nb_l2l3);
> +	regmap_read(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
> +		    &state->nb_cpu_load);
> +	regmap_read(state->regmap, ARMADA_37XX_NB_DYN_MOD, &state->nb_dyn_mod);
> +
> +	return 0;
> +}
> +
> +static int armada37xx_cpufreq_resume(struct cpufreq_policy *policy)
> +{
> +	struct armada37xx_cpufreq_state *state = armada37xx_cpufreq_state;
> +
> +	/* Ensure DVFS is disabled otherwise the following registers are RO */
> +	armada37xx_cpufreq_disable_dvfs(state->regmap);
> +
> +	regmap_write(state->regmap, ARMADA_37XX_NB_L0L1, state->nb_l0l1);
> +	regmap_write(state->regmap, ARMADA_37XX_NB_L2L3, state->nb_l2l3);
> +	regmap_write(state->regmap, ARMADA_37XX_NB_CPU_LOAD,
> +		     state->nb_cpu_load);
> +
> +	/*
> +	 * NB_DYN_MOD register is the one that actually enable back DVFS if it
> +	 * was enabled before the suspend operation. This must be done last
> +	 * otherwise other registers are not writable.
> +	 */
> +	regmap_write(state->regmap, ARMADA_37XX_NB_DYN_MOD, state->nb_dyn_mod);
> +
> +	return 0;
> +}
> +
>  static int __init armada37xx_cpufreq_driver_init(void)
>  {
> +	struct cpufreq_dt_platform_data pdata;
>  	struct armada_37xx_dvfs *dvfs;
>  	struct platform_device *pdev;
>  	unsigned long freq;
> @@ -213,6 +261,15 @@ static int __init armada37xx_cpufreq_driver_init(void)
>  		return -EINVAL;
>  	}
>  
> +	armada37xx_cpufreq_state = kmalloc(sizeof(*armada37xx_cpufreq_state),
> +					   GFP_KERNEL);
> +	if (!armada37xx_cpufreq_state) {
> +		clk_put(clk);
> +		return -ENOMEM;
> +	}
> +
> +	armada37xx_cpufreq_state->regmap = nb_pm_base;
> +
>  	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
>  	clk_put(clk);
>  
> @@ -228,7 +285,11 @@ static int __init armada37xx_cpufreq_driver_init(void)
>  	/* Now that everything is setup, enable the DVFS at hardware level */
>  	armada37xx_cpufreq_enable_dvfs(nb_pm_base);
>  
> -	pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> +	pdata.suspend = armada37xx_cpufreq_suspend;
> +	pdata.resume = armada37xx_cpufreq_resume;
> +
> +	pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, &pdata,
> +					     sizeof(pdata));
>  	ret = PTR_ERR_OR_ZERO(pdev);
>  	if (ret)
>  		goto disable_dvfs;
> @@ -244,6 +305,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
>  		dev_pm_opp_remove(cpu_dev, freq);
>  	}
>  
> +	kfree(armada37xx_cpufreq_state);
> +
>  	return ret;
>  }
>  /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
> -- 
> 2.15.0.194.g9af6a3dea062
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH V4 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

On 02-05-18, 17:52, Gregory CLEMENT wrote:
> Hi Viresh,
>  
>  On jeu., avril 26 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> > Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
> > operations. As there is currently no 'driver' structure, create one
> > to store both the regmap and the register values during suspend
> > operation.
> 
> I tested this patch on a Armada 3720 based board (EspressoBin), and it
> worked well. I even made some power consumption measurement and after
> the resume, the power consumption had the same value that before the
> suspend, for low CPU frequency and high CPU frequency.
> 
> Tested-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks Gregory.

-- 
viresh

[PATCH V4 3/3] cpufreq: add suspend/resume support in Armada 37xx DVFS driver

[Viresh Kumar]

On 02-05-18, 17:52, Gregory CLEMENT wrote:
> Hi Viresh,
>  
>  On jeu., avril 26 2018, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> > Add suspend/resume hooks in Armada 37xx DVFS driver to handle S2RAM
> > operations. As there is currently no 'driver' structure, create one
> > to store both the regmap and the register values during suspend
> > operation.
> 
> I tested this patch on a Armada 3720 based board (EspressoBin), and it
> worked well. I even made some power consumption measurement and after
> the resume, the power consumption had the same value that before the
> suspend, for low CPU frequency and high CPU frequency.
> 
> Tested-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks Gregory.

-- 
viresh

Re: [PATCH V3 0/3] cpufreq: dt: Allow platforms to provide suspend/resume hooks

[Rafael J. Wysocki]

On Tuesday, April 24, 2018 11:39:44 AM CEST Viresh Kumar wrote:
> Hi Rafael,
> 
> These patches are already tested by Miquel and are ready to get merged.
> Please pick them as part of 4.18 stuff whenever you do that.
> 
> I have updated the cpufreq-dt driver to allow platform specific
> suspend/resume hooks and done some minor cleanup in the driver. I have
> updated Miquel's patch and applied that on top of all this.
> 
> Thanks.
> 
> V2->V3:
> - Don't double put the clock.
> - Fix commit log of 3/3.
> - Add tested-by from Miquel.
> - There was no V2 of 1-2/3 but only 3/3 on the lists. All are marked as
>   V3 to clear things out.
> 
> --
> viresh
> 
> [1] https://lkml.kernel.org/r/20180421141943.25705-1-miquel.raynal@bootlin.com
> 
> Miquel Raynal (1):
>   cpufreq: add suspend/resume support in Armada 37xx DVFS driver
> 
> Viresh Kumar (2):
>   cpufreq: dt: Allow platform specific suspend/resume callbacks
>   cpufreq: armada: Free resources on error paths
> 
>  drivers/cpufreq/armada-37xx-cpufreq.c | 100 +++++++++++++++++++++++++++++-----
>  drivers/cpufreq/cpufreq-dt.c          |  10 +++-
>  drivers/cpufreq/cpufreq-dt.h          |   5 ++
>  3 files changed, 100 insertions(+), 15 deletions(-)
> 
> 

Applied including the v4 of patch [3/3], thanks!

[PATCH V3 0/3] cpufreq: dt: Allow platforms to provide suspend/resume hooks

[Rafael J. Wysocki]

On Tuesday, April 24, 2018 11:39:44 AM CEST Viresh Kumar wrote:
> Hi Rafael,
> 
> These patches are already tested by Miquel and are ready to get merged.
> Please pick them as part of 4.18 stuff whenever you do that.
> 
> I have updated the cpufreq-dt driver to allow platform specific
> suspend/resume hooks and done some minor cleanup in the driver. I have
> updated Miquel's patch and applied that on top of all this.
> 
> Thanks.
> 
> V2->V3:
> - Don't double put the clock.
> - Fix commit log of 3/3.
> - Add tested-by from Miquel.
> - There was no V2 of 1-2/3 but only 3/3 on the lists. All are marked as
>   V3 to clear things out.
> 
> --
> viresh
> 
> [1] https://lkml.kernel.org/r/20180421141943.25705-1-miquel.raynal at bootlin.com
> 
> Miquel Raynal (1):
>   cpufreq: add suspend/resume support in Armada 37xx DVFS driver
> 
> Viresh Kumar (2):
>   cpufreq: dt: Allow platform specific suspend/resume callbacks
>   cpufreq: armada: Free resources on error paths
> 
>  drivers/cpufreq/armada-37xx-cpufreq.c | 100 +++++++++++++++++++++++++++++-----
>  drivers/cpufreq/cpufreq-dt.c          |  10 +++-
>  drivers/cpufreq/cpufreq-dt.h          |   5 ++
>  3 files changed, 100 insertions(+), 15 deletions(-)
> 
> 

Applied including the v4 of patch [3/3], thanks!