[PATCH V2 0/4] cpufreq: Register cooling device after policy is ready

[PATCH V2 0/4] cpufreq: Register cooling device after policy is ready

[Viresh Kumar]

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(-)

-- 
2.0.3.693.g996b0fd

[PATCH V2 1/4] cpufreq: Fix formatting issues in 'struct cpufreq_driver'

[Viresh Kumar]

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 <viresh.kumar@linaro.org>
Reviewed-by: Eduardo Valentin <edubezval@gmail.com>
Tested-by: Eduardo Valentin <edubezval@gmail.com>
---
 include/linux/cpufreq.h | 50 ++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 503b085..db3c130 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -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 */
-- 
2.0.3.693.g996b0fd

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

[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 <viresh.kumar@linaro.org>
Reviewed-by: Eduardo Valentin <edubezval@gmail.com>
Tested-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/cpufreq/cpufreq-dt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 8cba13d..7374fc4 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -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",
-- 
2.0.3.693.g996b0fd

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

[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.

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 <viresh.kumar@linaro.org>
Reviewed-by: Eduardo Valentin <edubezval@gmail.com>
Tested-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/cpufreq/cpufreq.c | 5 +++++
 include/linux/cpufreq.h   | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index de2c3e1..a09a29c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -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;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index db3c130..4d078ce 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -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 */
-- 
2.0.3.693.g996b0fd

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

[Lukasz Majewski]

Hi Viresh,

> 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 <viresh.kumar@linaro.org>
> Reviewed-by: Eduardo Valentin <edubezval@gmail.com>
> Tested-by: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/cpufreq/cpufreq.c | 5 +++++
>  include/linux/cpufreq.h   | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index de2c3e1..a09a29c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -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? 

>  	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;
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index db3c130..4d078ce 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -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 <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

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

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

[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 <viresh.kumar@linaro.org>
Reviewed-by: Eduardo Valentin <edubezval@gmail.com>
Tested-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/cpufreq/cpufreq-dt.c | 51 +++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 7374fc4..e720954 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -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,
 };
-- 
2.0.3.693.g996b0fd

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

[Lukasz Majewski]

Hi Viresh,

> 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 <viresh.kumar@linaro.org>
> Reviewed-by: Eduardo Valentin <edubezval@gmail.com>
> Tested-by: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/cpufreq/cpufreq-dt.c | 51
> +++++++++++++++++++++++++++----------------- 1 file changed, 32
> insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt.c
> b/drivers/cpufreq/cpufreq-dt.c index 7374fc4..e720954 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -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,
>  };

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

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