cpufreq: Register cooling device after policy is ready

PATCH In review Viresh Kumar 2 participants 7 messages Nov 27, 2014 CPU frequency scaling framework

lore

Share & tools

lore

Viresh Kumar

author maintainer

2014-11-27 00:38 UTC #0 lore raw

v2 cover cpufreq: Register cooling device after policy is ready

Hi Rafael/Eduardo,

Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.

One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.

Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence ->ready() callback.

The first patch fixes few formatting issues, so that the third patch doesn't throw any checkpatch warnings. Second one fixes a potential bug in cpufreq-dt driver. Third one introduces ->ready() callback which will be used in the fourth patch.

V1->V2:
- s/usable/ready
- dropped last three patches that broke thermal somehow.

Viresh Kumar (4):
cpufreq: Fix formatting issues in 'struct cpufreq_driver'
cpufreq-dt: pass 'policy->related_cpus' to
of_cpufreq_cooling_register()
cpufreq: Introduce ->ready() callback for cpufreq drivers
cpufreq-dt: register cooling device from ->ready() callback

 drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++----------------
 drivers/cpufreq/cpufreq.c    |  5 ++++
 include/linux/cpufreq.h      | 54 ++++++++++++++++++++++++--------------------
 3 files changed, 66 insertions(+), 44 deletions(-)

Signature · 1 line

2.0.3.693.g996b0fd

Viresh Kumar

author maintainer

2014-11-27 00:38 UTC #1 lore raw

PATCH v2 1/4 cpufreq: Fix formatting issues in 'struct cpufreq_driver'

Adding any new callback to 'struct cpufreq_driver' gives following checkpatch warning:

WARNING: Unnecessary space before function pointer arguments
+ void (*ready) (struct cpufreq_policy *policy);

This is because we have been using a tab spacing between function pointer name and its arguments and the new one tried to follow that.

Though we normally don't try to fix every checkpatch warning, specially around formatting issues as that creates unnecessary noise over lists. But I thought we better fix this so that new additions don't generate these warnings plus it looks far better/symmetric now.

So, remove these tab spacing issues in 'struct cpufreq_driver' only + fix alignment of all members.

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Eduardo Valentin <[email protected]>

Tested-by Eduardo Valentin <[email protected]>

---
 include/linux/cpufreq.h | 50 ++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

include/linux/cpufreq.h +25 ~~-25~~

index 503b085..db3c130 100644
@@ -217,26 +217,26 @@ __ATTR(_name, 0644, show_##_name, store_##_name)
 
 
 struct cpufreq_driver {
-	char			name[CPUFREQ_NAME_LEN];
-	u8			flags;
-	void			*driver_data;
+	char		name[CPUFREQ_NAME_LEN];
+	u8		flags;
+	void		*driver_data;
 
 	/* needed by all drivers */
-	int	(*init)		(struct cpufreq_policy *policy);
-	int	(*verify)	(struct cpufreq_policy *policy);
+	int		(*init)(struct cpufreq_policy *policy);
+	int		(*verify)(struct cpufreq_policy *policy);
 
 	/* define one out of two */
-	int	(*setpolicy)	(struct cpufreq_policy *policy);
+	int		(*setpolicy)(struct cpufreq_policy *policy);
 
 	/*
 	 * On failure, should always restore frequency to policy->restore_freq
 	 * (i.e. old freq).
 	 */
-	int	(*target)	(struct cpufreq_policy *policy,	/* Deprecated */
-				 unsigned int target_freq,
-				 unsigned int relation);
-	int	(*target_index)	(struct cpufreq_policy *policy,
-				 unsigned int index);
+	int		(*target)(struct cpufreq_policy *policy,
+				  unsigned int target_freq,
+				  unsigned int relation);	/* Deprecated */
+	int		(*target_index)(struct cpufreq_policy *policy,
+					unsigned int index);
 	/*
 	 * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION
 	 * unset.

@@ -252,27 +252,27 @@ struct cpufreq_driver {
 	 * wish to switch to intermediate frequency for some target frequency.
 	 * In that case core will directly call ->target_index().
 	 */
-	unsigned int (*get_intermediate)(struct cpufreq_policy *policy,
-					 unsigned int index);
-	int	(*target_intermediate)(struct cpufreq_policy *policy,
-				       unsigned int index);
+	unsigned int	(*get_intermediate)(struct cpufreq_policy *policy,
+					    unsigned int index);
+	int		(*target_intermediate)(struct cpufreq_policy *policy,
+					       unsigned int index);
 
 	/* should be defined, if possible */
-	unsigned int	(*get)	(unsigned int cpu);
+	unsigned int	(*get)(unsigned int cpu);
 
 	/* optional */
-	int	(*bios_limit)	(int cpu, unsigned int *limit);
+	int		(*bios_limit)(int cpu, unsigned int *limit);
 
-	int	(*exit)		(struct cpufreq_policy *policy);
-	void	(*stop_cpu)	(struct cpufreq_policy *policy);
-	int	(*suspend)	(struct cpufreq_policy *policy);
-	int	(*resume)	(struct cpufreq_policy *policy);
-	struct freq_attr	**attr;
+	int		(*exit)(struct cpufreq_policy *policy);
+	void		(*stop_cpu)(struct cpufreq_policy *policy);
+	int		(*suspend)(struct cpufreq_policy *policy);
+	int		(*resume)(struct cpufreq_policy *policy);
+	struct freq_attr **attr;
 
 	/* platform specific boost support code */
-	bool                    boost_supported;
-	bool                    boost_enabled;
-	int     (*set_boost)    (int state);
+	bool		boost_supported;
+	bool		boost_enabled;
+	int		(*set_boost)(int state);
 };
 
 /* flags */

Signature · 1 line

2.0.3.693.g996b0fd

Viresh Kumar

author maintainer

2014-11-27 00:38 UTC #2 lore raw

PATCH v2 2/4 cpufreq-dt: pass 'policy->related_cpus' to of_cpufreq_cooling_register()

In reply to Viresh Kumar

The second parameter of of_cpufreq_cooling_register() should be the CPUs to which the frequency constraint will apply. As the cpufreq-dt driver now supports platforms with multiple 'struct cpufreq_policy' instances (i.e. > 1 clock domains for CPUs), passing 'cpu_present_mask' isn't correct anymore. As every policy will have a set of CPUs and that may not be equal to 'cpu_present_mask' always.

So, pass only mask of CPUs which are controlled by current policy.

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Eduardo Valentin <[email protected]>

Tested-by Eduardo Valentin <[email protected]>

---
 drivers/cpufreq/cpufreq-dt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/cpufreq-dt.c +1 -1

index 8cba13d..7374fc4 100644
@@ -274,7 +274,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	 * thermal DT code takes care of matching them.
 	 */
 	if (of_find_property(np, "#cooling-cells", NULL)) {
-		cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
+		cdev = of_cpufreq_cooling_register(np, policy->related_cpus);
 		if (IS_ERR(cdev))
 			dev_err(cpu_dev,
 				"running cpufreq without cooling device: %ld\n",

Signature · 1 line

2.0.3.693.g996b0fd

Viresh Kumar

author maintainer

2014-11-27 00:38 UTC #3 lore raw

PATCH v2 3/4 cpufreq: Introduce ->ready() callback for cpufreq drivers

In reply to Viresh Kumar

Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.

Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence we introduce ->ready() callback.

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Eduardo Valentin <[email protected]>

Tested-by Eduardo Valentin <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 5 +++++
 include/linux/cpufreq.h   | 4 ++++
 2 files changed, 9 insertions(+)

drivers/cpufreq/cpufreq.c +5 -0

index de2c3e1..a09a29c 100644
@@ -1285,8 +1285,13 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	up_write(&policy->rwsem);
 
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
+
 	up_read(&cpufreq_rwsem);
 
+	/* Callback for handling stuff after policy is ready */
+	if (cpufreq_driver->ready)
+		cpufreq_driver->ready(policy);
+
 	pr_debug("initialization complete\n");
 
 	return 0;

include/linux/cpufreq.h +4 -0

index db3c130..4d078ce 100644
@@ -267,6 +267,10 @@ struct cpufreq_driver {
 	void		(*stop_cpu)(struct cpufreq_policy *policy);
 	int		(*suspend)(struct cpufreq_policy *policy);
 	int		(*resume)(struct cpufreq_policy *policy);
+
+	/* Will be called after the driver is fully initialized */
+	void		(*ready)(struct cpufreq_policy *policy);
+
 	struct freq_attr **attr;
 
 	/* platform specific boost support code */

Signature · 1 line

2.0.3.693.g996b0fd

Lukasz Majewski

Reviewed-by on v2 3/4

2014-11-27 11:00 UTC #4 lore raw

Re: [PATCH V2 3/4] cpufreq: Introduce ->ready() callback for cpufreq drivers

Hi Viresh,

Quoted 27 lines

Currently there is no callback for cpufreq drivers which is called once the policy is ready to be used. There are some requirements where such a callback is required.
One of them is registering a cooling device with the help of of_cpufreq_cooling_register(). This routine tries to get 'struct cpufreq_policy' for CPUs which isn't yet initialed at the time ->init() is called and so we face issues while registering the cooling device.
Because we can't register cooling device from ->init(), we need a callback that is called after the policy is ready to be used and hence we introduce ->ready() callback.
Signed-off-by Viresh Kumar <[email protected]>
Reviewed-by Eduardo Valentin <[email protected]>
Tested-by Eduardo Valentin <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 5 +++++
 include/linux/cpufreq.h   | 4 ++++
 2 files changed, 9 insertions(+)
drivers/cpufreq/cpufreq.c +0 -0
index de2c3e1..a09a29c 100644

drivers/cpufreq/cpufreq.c static int __cpufreq_add_dev(struct device · line 1288

Quoted 4 lines

drivers/cpufreq/cpufreq.c +1 -0

@@ -1285,8 +1285,13 @@ static int __cpufreq_add_dev(struct device
 *dev, struct subsys_interface *sif) up_write(&policy->rwsem);
 
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
+

Is this blank line intentional?

Quoted 14 lines

drivers/cpufreq/cpufreq.c +4 -0

 	up_read(&cpufreq_rwsem);
 
+	/* Callback for handling stuff after policy is ready */
+	if (cpufreq_driver->ready)
+		cpufreq_driver->ready(policy);
+
 	pr_debug("initialization complete\n");
 
 	return 0;

include/linux/cpufreq.h +0 -0

index db3c130..4d078ce 100644

include/linux/cpufreq.h struct cpufreq_driver { · line 276

Quoted 10 lines

include/linux/cpufreq.h +4 -0

@@ -267,6 +267,10 @@ struct cpufreq_driver {
 	void		(*stop_cpu)(struct cpufreq_policy
 *policy); int		(*suspend)(struct cpufreq_policy
 *policy); int		(*resume)(struct cpufreq_policy *policy);
+
+	/* Will be called after the driver is fully initialized */
+	void		(*ready)(struct cpufreq_policy *policy);
+
 	struct freq_attr **attr;
 
 	/* platform specific boost support code */

Reviewed-by Lukasz Majewski <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Viresh Kumar

author maintainer

2014-11-27 00:38 UTC #5 lore raw

PATCH v2 4/4 cpufreq-dt: register cooling device from ->ready() callback

In reply to Viresh Kumar

Currently we are calling of_cpufreq_cooling_register() from ->init() callback. At this point of time cpufreq driver's policy isn't completely ready to be used as few of its fields/structure/pointers aren't yet initialized.

Because of_cpufreq_cooling_register() tries to access policy with help of cpufreq_cpu_get() and then tries to get freq-table as well, these calls fail.

To fix this, register the cooling device after the policy is ready to be used. And the right callback for it is the newly added ->ready() one.

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Eduardo Valentin <[email protected]>

Tested-by Eduardo Valentin <[email protected]>

---
 drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 19 deletions(-)

drivers/cpufreq/cpufreq-dt.c +32 ~~-19~~

index 7374fc4..e720954 100644
@@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct cpufreq_dt_platform_data *pd;
 	struct cpufreq_frequency_table *freq_table;
-	struct thermal_cooling_device *cdev;
 	struct device_node *np;
 	struct private_data *priv;
 	struct device *cpu_dev;

@@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		goto out_free_priv;
 	}
 
-	/*
-	 * For now, just loading the cooling device;
-	 * thermal DT code takes care of matching them.
-	 */
-	if (of_find_property(np, "#cooling-cells", NULL)) {
-		cdev = of_cpufreq_cooling_register(np, policy->related_cpus);
-		if (IS_ERR(cdev))
-			dev_err(cpu_dev,
-				"running cpufreq without cooling device: %ld\n",
-				PTR_ERR(cdev));
-		else
-			priv->cdev = cdev;
-	}
-
 	priv->cpu_dev = cpu_dev;
 	priv->cpu_reg = cpu_reg;
 	policy->driver_data = priv;

@@ -292,7 +277,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	if (ret) {
 		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
 			ret);
-		goto out_cooling_unregister;
+		goto out_free_cpufreq_table;
 	}
 
 	policy->cpuinfo.transition_latency = transition_latency;

@@ -305,8 +290,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	return 0;
 
-out_cooling_unregister:
-	cpufreq_cooling_unregister(priv->cdev);
+out_free_cpufreq_table:
 	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
 out_free_priv:
 	kfree(priv);

@@ -324,7 +308,8 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 {
 	struct private_data *priv = policy->driver_data;
 
-	cpufreq_cooling_unregister(priv->cdev);
+	if (priv->cdev)
+		cpufreq_cooling_unregister(priv->cdev);
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	clk_put(policy->clk);
 	if (!IS_ERR(priv->cpu_reg))

@@ -334,6 +319,33 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	return 0;
 }
 
+static void cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct private_data *priv = policy->driver_data;
+	struct device_node *np = of_node_get(priv->cpu_dev->of_node);
+
+	if (WARN_ON(!np))
+		return;
+
+	/*
+	 * For now, just loading the cooling device;
+	 * thermal DT code takes care of matching them.
+	 */
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		priv->cdev = of_cpufreq_cooling_register(np,
+							 policy->related_cpus);
+		if (IS_ERR(priv->cdev)) {
+			dev_err(priv->cpu_dev,
+				"running cpufreq without cooling device: %ld\n",
+				PTR_ERR(priv->cdev));
+
+			priv->cdev = NULL;
+		}
+	}
+
+	of_node_put(np);
+}
+
 static struct cpufreq_driver dt_cpufreq_driver = {
 	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
 	.verify = cpufreq_generic_frequency_table_verify,

@@ -341,6 +353,7 @@ static struct cpufreq_driver dt_cpufreq_driver = {
 	.get = cpufreq_generic_get,
 	.init = cpufreq_init,
 	.exit = cpufreq_exit,
+	.ready = cpufreq_ready,
 	.name = "cpufreq-dt",
 	.attr = cpufreq_generic_attr,
 };

Signature · 1 line

2.0.3.693.g996b0fd

Lukasz Majewski

Reviewed-by on v2 4/4

2014-11-27 11:08 UTC #6 lore raw

Re: [PATCH V2 4/4] cpufreq-dt: register cooling device from ->ready() callback

Hi Viresh,

Quoted 122 lines

Because of_cpufreq_cooling_register() tries to access policy with help of cpufreq_cpu_get() and then tries to get freq-table as well, these calls fail.

To fix this, register the cooling device after the policy is ready to be used. And the right callback for it is the newly added ->ready() one.

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Eduardo Valentin <[email protected]>

Tested-by Eduardo Valentin <[email protected]>

---
 drivers/cpufreq/cpufreq-dt.c | 51

+++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 19 deletions(-)

(quoted diff) +0 -0

b/drivers/cpufreq/cpufreq-dt.c index 7374fc4..e720954 100644

drivers/cpufreq/cpufreq-dt.c +0 ~~-15~~

@@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy
 *policy) {
 	struct cpufreq_dt_platform_data *pd;
 	struct cpufreq_frequency_table *freq_table;
-	struct thermal_cooling_device *cdev;
 	struct device_node *np;
 	struct private_data *priv;
 	struct device *cpu_dev;

@@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy
 *policy) goto out_free_priv;
 	}
 
-	/*
-	 * For now, just loading the cooling device;
-	 * thermal DT code takes care of matching them.
-	 */
-	if (of_find_property(np, "#cooling-cells", NULL)) {
-		cdev = of_cpufreq_cooling_register(np,
 policy->related_cpus);
-		if (IS_ERR(cdev))
-			dev_err(cpu_dev,
-				"running cpufreq without cooling
 device: %ld\n",
-				PTR_ERR(cdev));
-		else
-			priv->cdev = cdev;
-	}
-
 	priv->cpu_dev = cpu_dev;

drivers/cpufreq/cpufreq-dt.c +31 -4

 	priv->cpu_reg = cpu_reg;
 	policy->driver_data = priv;

@@ -292,7 +277,7 @@ static int cpufreq_init(struct cpufreq_policy
 *policy) if (ret) {
 		dev_err(cpu_dev, "%s: invalid frequency table:
 %d\n", __func__, ret);
-		goto out_cooling_unregister;
+		goto out_free_cpufreq_table;
 	}
 
 	policy->cpuinfo.transition_latency = transition_latency;

@@ -305,8 +290,7 @@ static int cpufreq_init(struct cpufreq_policy
 *policy) 
 	return 0;
 
-out_cooling_unregister:
-	cpufreq_cooling_unregister(priv->cdev);
+out_free_cpufreq_table:
 	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
 out_free_priv:
 	kfree(priv);

@@ -324,7 +308,8 @@ static int cpufreq_exit(struct cpufreq_policy
 *policy) {
 	struct private_data *priv = policy->driver_data;
 
-	cpufreq_cooling_unregister(priv->cdev);
+	if (priv->cdev)
+		cpufreq_cooling_unregister(priv->cdev);
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev,
 &policy->freq_table); clk_put(policy->clk);
 	if (!IS_ERR(priv->cpu_reg))

@@ -334,6 +319,33 @@ static int cpufreq_exit(struct cpufreq_policy
 *policy) return 0;
 }
 
+static void cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct private_data *priv = policy->driver_data;
+	struct device_node *np = of_node_get(priv->cpu_dev->of_node);
+
+	if (WARN_ON(!np))
+		return;
+
+	/*
+	 * For now, just loading the cooling device;
+	 * thermal DT code takes care of matching them.
+	 */
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		priv->cdev = of_cpufreq_cooling_register(np,
+
 policy->related_cpus);
+		if (IS_ERR(priv->cdev)) {
+			dev_err(priv->cpu_dev,
+				"running cpufreq without cooling
 device: %ld\n",
+				PTR_ERR(priv->cdev));
+
+			priv->cdev = NULL;
+		}
+	}
+
+	of_node_put(np);
+}
+
 static struct cpufreq_driver dt_cpufreq_driver = {

drivers/cpufreq/cpufreq-dt.c +0 -0

 	.flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK,
 	.verify = cpufreq_generic_frequency_table_verify,

drivers/cpufreq/cpufreq-dt.c static struct cpufreq_driver dt_cpufreq_driver = { · line 359

Quoted 7 lines

drivers/cpufreq/cpufreq-dt.c +1 -0

@@ -341,6 +353,7 @@ static struct cpufreq_driver dt_cpufreq_driver = {
 	.get = cpufreq_generic_get,
 	.init = cpufreq_init,
 	.exit = cpufreq_exit,
+	.ready = cpufreq_ready,
 	.name = "cpufreq-dt",
 	.attr = cpufreq_generic_attr,
 };

Reviewed-by Lukasz Majewski <[email protected]>

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group