[PATCH 0/7] cpufreq: Register cooling device after policy is usable

[PATCH 0/7] cpufreq: Register cooling device after policy is usable

[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 ->usable() 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 ->usable() callback which will be used in the
fourth patch.

Last three are fixes for cooling core, which may be applied separately by
Eduardo if he wants. Sent them in this series as they were sort of connected
with cpufreq in general.

Let me know if it still doesn't work properly.

--
viresh

Viresh Kumar (7):
  cpufreq: Fix formatting issues in 'struct cpufreq_driver'
  cpufreq-dt: pass 'policy->related_cpus' to
    of_cpufreq_cooling_register()
  cpufreq: Introduce ->usable() callback for cpufreq drivers
  cpufreq-dt: register cooling device from ->usable() callback
  cpu_cooling: Don't match min/max frequencies for all CPUs on cooling
    register
  cpu_cooling: don't iterate over all allowed_cpus to update cpufreq
    policy
  cpu_cooling: No need to check is_cpufreq_valid()

 drivers/cpufreq/cpufreq-dt.c  | 51 +++++++++++++++++++++++++---------------
 drivers/cpufreq/cpufreq.c     |  5 ++++
 drivers/thermal/cpu_cooling.c | 44 ++++-------------------------------
 include/linux/cpufreq.h       | 54 +++++++++++++++++++++++--------------------
 4 files changed, 70 insertions(+), 84 deletions(-)

-- 
2.0.3.693.g996b0fd

[PATCH 1/7] 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	(*usable)	(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>
---
 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

Re: [PATCH 1/7] cpufreq: Fix formatting issues in 'struct cpufreq_driver'

[Eduardo Valentin]

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

On Wed, Nov 26, 2014 at 11:22:56AM +0530, Viresh Kumar wrote:
> Adding any new callback to 'struct cpufreq_driver' gives following checkpatch
> warning:
> 
> WARNING: Unnecessary space before function pointer arguments
> +	void	(*usable)	(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
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/7] 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>
---
 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

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

[Eduardo Valentin]

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

On Wed, Nov 26, 2014 at 11:22:57AM +0530, Viresh Kumar wrote:
> 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
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 3/7] cpufreq: Introduce ->usable() 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 ->usable()
callback.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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..4fb95b9 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->usable)
+		cpufreq_driver->usable(policy);
+
 	pr_debug("initialization complete\n");
 
 	return 0;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index db3c130..4795c0b 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		(*usable)(struct cpufreq_policy *policy);
+
 	struct freq_attr **attr;
 
 	/* platform specific boost support code */
-- 
2.0.3.693.g996b0fd

Re: [PATCH 3/7] cpufreq: Introduce ->usable() callback for cpufreq drivers

[Eduardo Valentin]

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

On Wed, Nov 26, 2014 at 11:22:58AM +0530, Viresh Kumar wrote:
> 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 ->usable()
> callback.
> 

Reviewed-by: Eduardo Valentin <edubezval@gmail.com>
Tested-by: Eduardo Valentin <edubezval@gmail.com>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  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..4fb95b9 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->usable)
> +		cpufreq_driver->usable(policy);
> +
>  	pr_debug("initialization complete\n");
>  
>  	return 0;
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index db3c130..4795c0b 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		(*usable)(struct cpufreq_policy *policy);
> +
>  	struct freq_attr **attr;
>  
>  	/* platform specific boost support code */
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/7] cpufreq: Introduce ->usable() callback for cpufreq drivers

[Rafael J. Wysocki]

On Wednesday, November 26, 2014 11:22:58 AM Viresh Kumar wrote:
> 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 ->usable()
> callback.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  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..4fb95b9 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->usable)
> +		cpufreq_driver->usable(policy);
> +
>  	pr_debug("initialization complete\n");
>  
>  	return 0;
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index db3c130..4795c0b 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		(*usable)(struct cpufreq_policy *policy);

What about

	void		(*ready)(struct cpufreq_policy *policy);

> +
>  	struct freq_attr **attr;
>  
>  	/* platform specific boost support code */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/7] cpufreq: Introduce ->usable() callback for cpufreq drivers

[Viresh Kumar]

On 27 November 2014 at 05:55, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> +     void            (*usable)(struct cpufreq_policy *policy);
>
> What about
>
>         void            (*ready)(struct cpufreq_policy *policy);

That was the second option doing rounds in my thoughts :)
As two people like it now (You and Me), lets move to that :)

--
viresh

[PATCH 4/7] cpufreq-dt: register cooling device from ->usable() 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 ->usable() one.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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..f7af5c8 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_usable(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,
+	.usable = cpufreq_usable,
 	.name = "cpufreq-dt",
 	.attr = cpufreq_generic_attr,
 };
-- 
2.0.3.693.g996b0fd

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

[Eduardo Valentin]

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

On Wed, Nov 26, 2014 at 11:22:59AM +0530, Viresh Kumar wrote:
> 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 ->usable() 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..f7af5c8 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_usable(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,
> +	.usable = cpufreq_usable,
>  	.name = "cpufreq-dt",
>  	.attr = cpufreq_generic_attr,
>  };
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 5/7] cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register

[Viresh Kumar]

In __cpufreq_cooling_register() we try to match min/max frequencies for all CPUs
passed in 'clip_cpus' mask. This mask is the cpumask of cpus where the frequency
constraints will be applied.

Same frequency constraint can be applied only to the CPUs belonging to the same
cluster (i.e. CPUs sharing clock line). For all such CPUs we have a single
'struct cpufreq_policy' structure managing them and so getting policies for all
CPUs wouldn't make any sense as they will all return the same pointer.

So, remove this useless check of checking min/max for all CPUs. Also update doc
comment to make this more obvious that clip_cpus should be same as
policy->related_cpus.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Sorry if I don't understand cpu-cooling well, haven't done much work on it :(

 drivers/thermal/cpu_cooling.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 1ab0018..1193cc4 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -420,6 +420,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
  * __cpufreq_cooling_register - helper function to create cpufreq cooling device
  * @np: a valid struct device_node to the cooling device device tree node
  * @clip_cpus: cpumask of cpus where the frequency constraints will happen.
+ * Normally this should be same as cpufreq policy->related_cpus.
  *
  * This interface function registers the cpufreq cooling device with the name
  * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq
@@ -435,25 +436,9 @@ __cpufreq_cooling_register(struct device_node *np,
 {
 	struct thermal_cooling_device *cool_dev;
 	struct cpufreq_cooling_device *cpufreq_dev = NULL;
-	unsigned int min = 0, max = 0;
 	char dev_name[THERMAL_NAME_LENGTH];
-	int ret = 0, i;
-	struct cpufreq_policy policy;
+	int ret = 0;
 
-	/* Verify that all the clip cpus have same freq_min, freq_max limit */
-	for_each_cpu(i, clip_cpus) {
-		/* continue if cpufreq policy not found and not return error */
-		if (!cpufreq_get_policy(&policy, i))
-			continue;
-		if (min == 0 && max == 0) {
-			min = policy.cpuinfo.min_freq;
-			max = policy.cpuinfo.max_freq;
-		} else {
-			if (min != policy.cpuinfo.min_freq ||
-			    max != policy.cpuinfo.max_freq)
-				return ERR_PTR(-EINVAL);
-		}
-	}
 	cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
 			      GFP_KERNEL);
 	if (!cpufreq_dev)
-- 
2.0.3.693.g996b0fd

Re: [PATCH 5/7] cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register

[Eduardo Valentin]

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

On Wed, Nov 26, 2014 at 11:23:00AM +0530, Viresh Kumar wrote:
> In __cpufreq_cooling_register() we try to match min/max frequencies for all CPUs
> passed in 'clip_cpus' mask. This mask is the cpumask of cpus where the frequency
> constraints will be applied.
> 
> Same frequency constraint can be applied only to the CPUs belonging to the same
> cluster (i.e. CPUs sharing clock line). For all such CPUs we have a single
> 'struct cpufreq_policy' structure managing them and so getting policies for all
> CPUs wouldn't make any sense as they will all return the same pointer.
> 
> So, remove this useless check of checking min/max for all CPUs. Also update doc
> comment to make this more obvious that clip_cpus should be same as
> policy->related_cpus.

Shall we also review the users of this API then? Simple to double check
if they are passing the correct value?


> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Sorry if I don't understand cpu-cooling well, haven't done much work on it :(
> 
>  drivers/thermal/cpu_cooling.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 1ab0018..1193cc4 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -420,6 +420,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
>   * __cpufreq_cooling_register - helper function to create cpufreq cooling device
>   * @np: a valid struct device_node to the cooling device device tree node
>   * @clip_cpus: cpumask of cpus where the frequency constraints will happen.
> + * Normally this should be same as cpufreq policy->related_cpus.
>   *
>   * This interface function registers the cpufreq cooling device with the name
>   * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq
> @@ -435,25 +436,9 @@ __cpufreq_cooling_register(struct device_node *np,
>  {
>  	struct thermal_cooling_device *cool_dev;
>  	struct cpufreq_cooling_device *cpufreq_dev = NULL;
> -	unsigned int min = 0, max = 0;
>  	char dev_name[THERMAL_NAME_LENGTH];
> -	int ret = 0, i;
> -	struct cpufreq_policy policy;
> +	int ret = 0;
>  
> -	/* Verify that all the clip cpus have same freq_min, freq_max limit */
> -	for_each_cpu(i, clip_cpus) {
> -		/* continue if cpufreq policy not found and not return error */
> -		if (!cpufreq_get_policy(&policy, i))
> -			continue;
> -		if (min == 0 && max == 0) {
> -			min = policy.cpuinfo.min_freq;
> -			max = policy.cpuinfo.max_freq;
> -		} else {
> -			if (min != policy.cpuinfo.min_freq ||
> -			    max != policy.cpuinfo.max_freq)
> -				return ERR_PTR(-EINVAL);
> -		}
> -	}
>  	cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
>  			      GFP_KERNEL);
>  	if (!cpufreq_dev)
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/7] cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register

[Viresh Kumar]

On 27 November 2014 at 21:05, Eduardo Valentin <edubezval@gmail.com> wrote:
> Shall we also review the users of this API then? Simple to double check
> if they are passing the correct value?

They weren't :).. Fixed them, patches would follow.

[PATCH 6/7] cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy

[Viresh Kumar]

All CPUs present in 'allowed_cpus' share the same 'struct cpufreq_policy'
structure and so calling cpufreq_update_policy() for each of them doesn't make
sense.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 1193cc4..41f5728 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -272,11 +272,10 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
 static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
 				 unsigned long cooling_state)
 {
-	unsigned int cpuid, clip_freq;
+	unsigned int clip_freq;
 	struct cpumask *mask = &cpufreq_device->allowed_cpus;
 	unsigned int cpu = cpumask_any(mask);
 
-
 	/* Check if the old cooling action is same as new cooling action */
 	if (cpufreq_device->cpufreq_state == cooling_state)
 		return 0;
@@ -289,10 +288,8 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
 	cpufreq_device->cpufreq_val = clip_freq;
 	notify_device = cpufreq_device;
 
-	for_each_cpu(cpuid, mask) {
-		if (is_cpufreq_valid(cpuid))
-			cpufreq_update_policy(cpuid);
-	}
+	if (is_cpufreq_valid(cpu))
+		cpufreq_update_policy(cpu);
 
 	notify_device = NOTIFY_INVALID;
 
-- 
2.0.3.693.g996b0fd

[PATCH 7/7] cpu_cooling: Don't check is_cpufreq_valid()

[Viresh Kumar]

Because get_cpu_frequency() has returned a valid frequency, it means that the
cpufreq policy is surely valid and so no point checking that again with
is_cpufreq_valid(). Get rid of the routine as well as there are no more users.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/thermal/cpu_cooling.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 41f5728..3740b61 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -98,23 +98,6 @@ static void release_idr(struct idr *idr, int id)
 
 /* Below code defines functions to be used for cpufreq as cooling device */
 
-/**
- * is_cpufreq_valid - function to check frequency transitioning capability.
- * @cpu: cpu for which check is needed.
- *
- * This function will check the current state of the system if
- * it is capable of changing the frequency for a given @cpu.
- *
- * Return: 0 if the system is not currently capable of changing
- * the frequency of given cpu. !0 in case the frequency is changeable.
- */
-static int is_cpufreq_valid(int cpu)
-{
-	struct cpufreq_policy policy;
-
-	return !cpufreq_get_policy(&policy, cpu);
-}
-
 enum cpufreq_cooling_property {
 	GET_LEVEL,
 	GET_FREQ,
@@ -288,8 +271,7 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device,
 	cpufreq_device->cpufreq_val = clip_freq;
 	notify_device = cpufreq_device;
 
-	if (is_cpufreq_valid(cpu))
-		cpufreq_update_policy(cpu);
+	cpufreq_update_policy(cpu);
 
 	notify_device = NOTIFY_INVALID;
 
-- 
2.0.3.693.g996b0fd

Re: [PATCH 0/7] cpufreq: Register cooling device after policy is usable

[Eduardo Valentin]

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

Hello Viresh,

Thanks for providing a proposal.

On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
> 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 ->usable() 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 ->usable() callback which will be used in the
> fourth patch.
> 
> Last three are fixes for cooling core, which may be applied separately by
> Eduardo if he wants. Sent them in this series as they were sort of connected
> with cpufreq in general.
> 
> Let me know if it still doesn't work properly.

For the series, the last three patches somehow breaks things. I didn't
not investigate the reason now, because, well, I think we should take
one thing at a time.

For the patches 1 to 4, I tried then and they do the trick. Now the
sequencing is correct between cpufreq-dt and cpu cooling. That means I
can also improve the thermal code by accepting the following patches:
https://patchwork.kernel.org/patch/5326991/
https://patchwork.kernel.org/patch/5387161/

on top of the four first patches.

Cheers,

Eduardo Valentin

> 
> --
> viresh
> 
> Viresh Kumar (7):
>   cpufreq: Fix formatting issues in 'struct cpufreq_driver'
>   cpufreq-dt: pass 'policy->related_cpus' to
>     of_cpufreq_cooling_register()
>   cpufreq: Introduce ->usable() callback for cpufreq drivers
>   cpufreq-dt: register cooling device from ->usable() callback
>   cpu_cooling: Don't match min/max frequencies for all CPUs on cooling
>     register
>   cpu_cooling: don't iterate over all allowed_cpus to update cpufreq
>     policy
>   cpu_cooling: No need to check is_cpufreq_valid()
> 
>  drivers/cpufreq/cpufreq-dt.c  | 51 +++++++++++++++++++++++++---------------
>  drivers/cpufreq/cpufreq.c     |  5 ++++
>  drivers/thermal/cpu_cooling.c | 44 ++++-------------------------------
>  include/linux/cpufreq.h       | 54 +++++++++++++++++++++++--------------------
>  4 files changed, 70 insertions(+), 84 deletions(-)
> 
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/7] cpufreq: Register cooling device after policy is usable

[Rafael J. Wysocki]

On Wednesday, November 26, 2014 01:54:31 PM Eduardo Valentin wrote:
> 
> --UugvWAfsgieZRqgk
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> Hello Viresh,
> 
> Thanks for providing a proposal.
> 
> On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
> > Hi Rafael/Eduardo,
> >=20
> > Currently there is no callback for cpufreq drivers which is called once t=
> he
> > policy is ready to be used. There are some requirements where such a call=
> back is
> > required.
> >=20
> > 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.
> >=20
> > Because we can't register cooling device from ->init(), we need a callbac=
> k that
> > is called after the policy is ready to be used and hence ->usable() callb=
> ack.
> >=20
> > The first patch fixes few formatting issues, so that the third patch does=
> n't
> > throw any checkpatch warnings. Second one fixes a potential bug in cpufre=
> q-dt
> > driver. Third one introduces ->usable() callback which will be used in the
> > fourth patch.
> >=20
> > Last three are fixes for cooling core, which may be applied separately by
> > Eduardo if he wants. Sent them in this series as they were sort of connec=
> ted
> > with cpufreq in general.
> >=20
> > Let me know if it still doesn't work properly.
> 
> For the series, the last three patches somehow breaks things. I didn't
> not investigate the reason now, because, well, I think we should take
> one thing at a time.

Agreed!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/7] cpufreq: Register cooling device after policy is usable

[Eduardo Valentin]

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

On Wed, Nov 26, 2014 at 01:54:31PM -0400, Eduardo Valentin wrote:
> Hello Viresh,
> 
> Thanks for providing a proposal.
> 
> On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
> > 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 ->usable() 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 ->usable() callback which will be used in the
> > fourth patch.
> > 
> > Last three are fixes for cooling core, which may be applied separately by
> > Eduardo if he wants. Sent them in this series as they were sort of connected
> > with cpufreq in general.
> > 
> > Let me know if it still doesn't work properly.
> 
> For the series, the last three patches somehow breaks things. I didn't
> not investigate the reason now, because, well, I think we should take
> one thing at a time.

It turns out that I could not reproduce the issue I saw. So, I am
assuming that was an issue in my environment.

Can you still post them separately?  


> 
> For the patches 1 to 4, I tried then and they do the trick. Now the
> sequencing is correct between cpufreq-dt and cpu cooling. That means I
> can also improve the thermal code by accepting the following patches:
> https://patchwork.kernel.org/patch/5326991/
> https://patchwork.kernel.org/patch/5387161/
> 
> on top of the four first patches.
> 
> Cheers,
> 
> Eduardo Valentin
> 
> > 
> > --
> > viresh
> > 
> > Viresh Kumar (7):
> >   cpufreq: Fix formatting issues in 'struct cpufreq_driver'
> >   cpufreq-dt: pass 'policy->related_cpus' to
> >     of_cpufreq_cooling_register()
> >   cpufreq: Introduce ->usable() callback for cpufreq drivers
> >   cpufreq-dt: register cooling device from ->usable() callback
> >   cpu_cooling: Don't match min/max frequencies for all CPUs on cooling
> >     register
> >   cpu_cooling: don't iterate over all allowed_cpus to update cpufreq
> >     policy
> >   cpu_cooling: No need to check is_cpufreq_valid()
> > 
> >  drivers/cpufreq/cpufreq-dt.c  | 51 +++++++++++++++++++++++++---------------
> >  drivers/cpufreq/cpufreq.c     |  5 ++++
> >  drivers/thermal/cpu_cooling.c | 44 ++++-------------------------------
> >  include/linux/cpufreq.h       | 54 +++++++++++++++++++++++--------------------
> >  4 files changed, 70 insertions(+), 84 deletions(-)
> > 
> > -- 
> > 2.0.3.693.g996b0fd
> > 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/7] cpufreq: Register cooling device after policy is usable

[Viresh Kumar]

On 27 November 2014 at 21:03, Eduardo Valentin <edubezval@gmail.com> wrote:
> It turns out that I could not reproduce the issue I saw. So, I am
> assuming that was an issue in my environment.

Good to hear that :)

> Can you still post them separately?

Yeah, I will..

I also have few more patches now and so will send them all together.

Re: [PATCH 0/7] cpufreq: Register cooling device after policy is usable

[Eduardo Valentin]

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

Viresh,

On Wed, Nov 26, 2014 at 11:22:55AM +0530, Viresh Kumar wrote:
> 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 ->usable() 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 ->usable() callback which will be used in the
> fourth patch.
> 
> Last three are fixes for cooling core, which may be applied separately by
> Eduardo if he wants. Sent them in this series as they were sort of connected
> with cpufreq in general.
> 

Thanks for taking the time to review that code. In fact, it is good to
have it under the eyes of someone with cpufreq experience. But, from my
smoke testing, looks like they introduce regressions. 

Would you mind splitting them from this series and sending a separate
trhead? They do not help anyway to the original purpose of this series.


BR, Eduardo Valentin

> Let me know if it still doesn't work properly.
> 
> --
> viresh
> 
> Viresh Kumar (7):
>   cpufreq: Fix formatting issues in 'struct cpufreq_driver'
>   cpufreq-dt: pass 'policy->related_cpus' to
>     of_cpufreq_cooling_register()
>   cpufreq: Introduce ->usable() callback for cpufreq drivers
>   cpufreq-dt: register cooling device from ->usable() callback
>   cpu_cooling: Don't match min/max frequencies for all CPUs on cooling
>     register
>   cpu_cooling: don't iterate over all allowed_cpus to update cpufreq
>     policy
>   cpu_cooling: No need to check is_cpufreq_valid()
> 
>  drivers/cpufreq/cpufreq-dt.c  | 51 +++++++++++++++++++++++++---------------
>  drivers/cpufreq/cpufreq.c     |  5 ++++
>  drivers/thermal/cpu_cooling.c | 44 ++++-------------------------------
>  include/linux/cpufreq.h       | 54 +++++++++++++++++++++++--------------------
>  4 files changed, 70 insertions(+), 84 deletions(-)
> 
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]