Skip to contentSkip to conversation

cpufreq: dt: Allow platforms to provide suspend/resume hooks

PATCH New Viresh Kumar 2 participants 7 messages Apr 24, 2018

Subsystems (2)
lore
Share & tools
lore

Viresh Kumar

author maintainer
#0 lore raw

cover cpufreq: dt: Allow platforms to provide suspend/resume hooks

Hi Miquel,

This is in response to the patch[1] you sent.

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 your patch and applied that on top of all this. I haven't tested any of this and would need your help to get that done.

Thanks.

--
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 | 107 +++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq-dt.c          |  10 +++-
 drivers/cpufreq/cpufreq-dt.h          |   5 ++
 3 files changed, 104 insertions(+), 18 deletions(-)
Signature · 1 line
2.15.0.194.g9af6a3dea062
author maintainer
#1 lore raw

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

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

---
 drivers/cpufreq/armada-37xx-cpufreq.c | 40 +++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 14 deletions(-)
drivers/cpufreq/armada-37xx-cpufreq.c +26 -14
index 72a2975499db..9dafb3bbc334 100644
@@ -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;
@@ -202,38 +203,49 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	cur_frequency = clk_get_rate(clk);
 	if (!cur_frequency) {
 		dev_err(cpu_dev, "Failed to get clock rate for CPU\n");
-		clk_put(clk);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto put_clk;
 	}
 
 	dvfs = armada_37xx_cpu_freq_info_get(cur_frequency);
-	if (!dvfs)
-		return -EINVAL;
+	if (!dvfs) {
+		ret = -EINVAL;
+		goto put_clk;
+	}
 
 	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;
 
-	return PTR_ERR_OR_ZERO(pdev);
+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);
+	}
+put_clk:
+	clk_put(clk);
+	return ret;
 }
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
 late_initcall(armada37xx_cpufreq_driver_init);
Signature · 1 line
2.15.0.194.g9af6a3dea062
author maintainer
#2 lore raw

PATCH v2 3/3 cpufreq: add suspend/resume support in Armada 37xx DVFS driver

In reply to Viresh Kumar

From: Miquel Raynal <[email protected]>

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.

A syscore_ops is used to export the suspend/resume hooks.

---
V1->V2:
- Updated patch based on cpufreq-dt suspend/resume hooks.

 drivers/cpufreq/armada-37xx-cpufreq.c | 67 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)
drivers/cpufreq/armada-37xx-cpufreq.c +65 -2
index 9dafb3bbc334..c827d2437246 100644
@@ -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)
 		goto put_clk;
 	}
 
+	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;
@@ -243,6 +304,8 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		freq = cur_frequency / dvfs->divider[load_lvl];
 		dev_pm_opp_remove(cpu_dev, freq);
 	}
+
+	kfree(armada37xx_cpufreq_state);
 put_clk:
 	clk_put(clk);
 	return ret;
Signature · 1 line
2.15.0.194.g9af6a3dea062
#3 lore raw

PATCH v2 3/3 cpufreq: add suspend/resume support in Armada 37xx DVFS driver

Hi Viresh,

Quoted 8 lines · Viresh Kumar

From: Miquel Raynal <[email protected]>

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.

A syscore_ops is used to export the suspend/resume hooks.

I think you can remove this line too since there is no more syscore_ops.

Quoted 3 lines

Regards,
Miqu?l

Signature · 3 lines
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author maintainer
#4 lore raw

PATCH v2 3/3 cpufreq: add suspend/resume support in Armada 37xx DVFS driver

Quoted 7 lines · Miquel Raynal

Hi Viresh,

Quoted 2 lines · Viresh Kumar

From: Miquel Raynal <[email protected]>

Quoted 8 lines
Quoted 6 lines

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.

A syscore_ops is used to export the suspend/resume hooks.

I think you can remove this line too since there is no more syscore_ops.

That was stupid of me.

Signature · 1 line
viresh
#5 lore raw

cover cpufreq: dt: Allow platforms to provide suspend/resume hooks

In reply to Viresh Kumar

Hi Viresh,

Quoted 3 lines · Viresh Kumar

Hi Miquel,

This is in response to the patch[1] you sent.

Thank you very much for taking the time to look into it.

Quoted 5 lines

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 your patch and applied that on top of all this. I haven't tested any of this and would need your help to get that done.

I tested on an EspressoBin (Armada 3720), it works.

However, I first had a panic due to the clock being 'put' twice. I suggest you to merge this [1] into the second patch. With this (or something similar):

And the path error was reached because I did not select the cpufreq-dt driver in menuconfig, which I think should be selected by default when enabling the armada37xx-cpufreq driver. I wrote a small patch for that too, see [2]. Tell me if you want me to send it of if you want to include it in the next version ?

[1] http://code.bulix.org/auzdqa-323646
[2] http://code.bulix.org/e1yk5h-323647

Thank you,
Miqu?l

Quoted 20 lines

Thanks.

--
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 | 107 +++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq-dt.c          |  10 +++-
 drivers/cpufreq/cpufreq-dt.h          |   5 ++
 3 files changed, 104 insertions(+), 18 deletions(-)
Signature · 3 lines
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author maintainer
#6 lore raw

cover cpufreq: dt: Allow platforms to provide suspend/resume hooks

Quoted 1 line · Miquel Raynal

I tested on an EspressoBin (Armada 3720), it works.

Great.

Quoted 3 lines

However, I first had a panic due to the clock being 'put' twice. I suggest you to merge this [1] into the second patch. With this (or something similar):

NULL is a valid clock. I have fixed it now, a bit differently though.

Quoted 1 line

Thanks.

Quoted 5 lines

And the path error was reached because I did not select the cpufreq-dt driver in menuconfig, which I think should be selected by default when enabling the armada37xx-cpufreq driver. I wrote a small patch for that too, see [2]. Tell me if you want me to send it of if you want to include it in the next version ?

Maybe just send that separately (and you don't need to resend any of the patches which are part of this series).

Signature · 1 line
viresh