Re: [PATCH v3 1/4] devfreq: Register devfreq as a cooling device on demand

2021-03-07 Thread Daniel Lezcano 
On 07/03/2021 16:14, Chanwoo Choi wrote:
> On 21. 3. 7. 오후 11:28, Daniel Lezcano wrote:

[ ... ]

> Thanks. And, please add 'PM /' prefix for patch in order to
> keep the consistent patch title format.
> 
> PM / devfreq: Register devfreq as a cooling device on demand

Sure, thanks

-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 1/4] devfreq: Register devfreq as a cooling device on demand

2021-03-07 Thread Chanwoo Choi 

On 21. 3. 7. 오후 11:28, Daniel Lezcano wrote:

On 07/03/2021 15:16, Chanwoo Choi wrote:

On 21. 3. 7. 오후 6:45, Daniel Lezcano wrote:

Currently the default behavior is to manually having the devfreq
backend to register themselves as a devfreq cooling device.

Instead of adding the code in the drivers for the thermal cooling
device registering, let's provide a flag in the devfreq's profile to
tell the common devfreq code to register the newly created devfreq as
a cooling device.

Suggested-by: Chanwoo Choi 
Signed-off-by: Daniel Lezcano 
---
   V3:
 - Rebased on linux-pm branch without units.h
 - Set the cdev to NULL in case of error
 - Added description for the cdev field in the devfreq structure
   V2:
 - Added is_cooling_device boolean in profile structure
 - Register cooling device when the is_cooling_device boolean is set
 - Remove devfreq cooling device registration in the backend drivers
   V1:
 - Register devfreq as a cooling device unconditionnally
---



[ ... ]


   return devfreq;

   err_init:
@@ -960,6 +971,8 @@ int devfreq_remove_device(struct devfreq *devfreq)
   if (!devfreq)
   return -EINVAL;

+ thermal_cooling_device_unregister(devfreq->cdev);


I have a question. Why don't you use devfreq_cooling_unregister()?
When thermal_cooling_device_unregister(), how can we remove
the pm_qos_request of devfreq device?


You are perfectly right. I failed to call the right function :/

Will fix it with a v4.



Thanks. And, please add 'PM /' prefix for patch in order to
keep the consistent patch title format.

PM / devfreq: Register devfreq as a cooling device on demand

--
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH v3 1/4] devfreq: Register devfreq as a cooling device on demand

2021-03-07 Thread Daniel Lezcano 
On 07/03/2021 15:16, Chanwoo Choi wrote:
> On 21. 3. 7. 오후 6:45, Daniel Lezcano wrote:
>> Currently the default behavior is to manually having the devfreq
>> backend to register themselves as a devfreq cooling device.
>>
>> Instead of adding the code in the drivers for the thermal cooling
>> device registering, let's provide a flag in the devfreq's profile to
>> tell the common devfreq code to register the newly created devfreq as
>> a cooling device.
>>
>> Suggested-by: Chanwoo Choi 
>> Signed-off-by: Daniel Lezcano 
>> ---
>>   V3:
>> - Rebased on linux-pm branch without units.h
>> - Set the cdev to NULL in case of error
>> - Added description for the cdev field in the devfreq structure
>>   V2:
>> - Added is_cooling_device boolean in profile structure
>> - Register cooling device when the is_cooling_device boolean is set
>> - Remove devfreq cooling device registration in the backend drivers
>>   V1:
>> - Register devfreq as a cooling device unconditionnally
>> ---


[ ... ]

>>   return devfreq;
>>
>>   err_init:
>> @@ -960,6 +971,8 @@ int devfreq_remove_device(struct devfreq *devfreq)
>>   if (!devfreq)
>>   return -EINVAL;
>>
>> + thermal_cooling_device_unregister(devfreq->cdev);
> 
> I have a question. Why don't you use devfreq_cooling_unregister()?
> When thermal_cooling_device_unregister(), how can we remove
> the pm_qos_request of devfreq device?

You are perfectly right. I failed to call the right function :/

Will fix it with a v4.

-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v3 1/4] devfreq: Register devfreq as a cooling device on demand

2021-03-07 Thread Chanwoo Choi 
On 21. 3. 7. 오후 6:45, Daniel Lezcano wrote:
> Currently the default behavior is to manually having the devfreq
> backend to register themselves as a devfreq cooling device.
>
> Instead of adding the code in the drivers for the thermal cooling
> device registering, let's provide a flag in the devfreq's profile to
> tell the common devfreq code to register the newly created devfreq as
> a cooling device.
>
> Suggested-by: Chanwoo Choi 
> Signed-off-by: Daniel Lezcano 
> ---
>   V3:
> - Rebased on linux-pm branch without units.h
> - Set the cdev to NULL in case of error
> - Added description for the cdev field in the devfreq structure
>   V2:
> - Added is_cooling_device boolean in profile structure
> - Register cooling device when the is_cooling_device boolean is set
> - Remove devfreq cooling device registration in the backend drivers
>   V1:
> - Register devfreq as a cooling device unconditionnally
> ---
>   drivers/devfreq/devfreq.c | 13 +
>   include/linux/devfreq.h   |  8 
>   2 files changed, 21 insertions(+)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3047896e41..cf64aeec468f 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -11,6 +11,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   #include 
>   #include 
>   #include 
> @@ -26,6 +27,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   #include "governor.h"
>
>   #define CREATE_TRACE_POINTS
> @@ -935,6 +937,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>
>   mutex_unlock(_list_lock);
>
> + if (devfreq->profile->is_cooling_device) {
> + devfreq->cdev = devfreq_cooling_em_register(devfreq, NULL);
> + if (IS_ERR(devfreq->cdev)) {
> + dev_info(dev, "Failed to register devfreq "
> +  "cooling device\n");
> + devfreq->cdev = NULL;
> + }
> + }
> +
>   return devfreq;
>
>   err_init:
> @@ -960,6 +971,8 @@ int devfreq_remove_device(struct devfreq *devfreq)
>   if (!devfreq)
>   return -EINVAL;
>
> + thermal_cooling_device_unregister(devfreq->cdev);

I have a question. Why don't you use devfreq_cooling_unregister()?
When thermal_cooling_device_unregister(), how can we remove
the pm_qos_request of devfreq device?

> +
>   if (devfreq->governor) {
>   devfreq->governor->event_handler(devfreq,
>DEVFREQ_GOV_STOP, NULL);
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 26ea0850be9b..aba7ace11b72 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -98,11 +98,15 @@ struct devfreq_dev_status {
>* @freq_table: Optional list of frequencies to support 
> statistics
>*  and freq_table must be generated in ascending order.
>* @max_state:  The size of freq_table.
> + *
> + * @is_cooling_device: A self-explanatory boolean giving the device a
> + * cooling effect property.
>*/
>   struct devfreq_dev_profile {
>   unsigned long initial_freq;
>   unsigned int polling_ms;
>   enum devfreq_timer timer;
> + bool is_cooling_device;
>
>   int (*target)(struct device *dev, unsigned long *freq, u32 flags);
>   int (*get_dev_status)(struct device *dev,
> @@ -156,6 +160,7 @@ struct devfreq_stats {
>* @suspend_count:   suspend requests counter for a device.
>* @stats:  Statistics of devfreq device behavior
>* @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER 
> notifier
> + * @cdev:Cooling device pointer if the devfreq has cooling property
>* @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY
>* @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY
>*
> @@ -198,6 +203,9 @@ struct devfreq {
>
>   struct srcu_notifier_head transition_notifier_list;
>
> + /* Pointer to the cooling device if used for thermal mitigation */
> + struct thermal_cooling_device *cdev;
> +
>   struct notifier_block nb_min;
>   struct notifier_block nb_max;
>   };
>


--
Best Regards,
Samsung Electronics

Chanwoo Choi

Re: [PATCH v3 1/4] devfreq: Register devfreq as a cooling device on demand

2021-03-07 Thread Chanwoo Choi 

On 21. 3. 7. 오후 6:45, Daniel Lezcano wrote:

Currently the default behavior is to manually having the devfreq
backend to register themselves as a devfreq cooling device.

Instead of adding the code in the drivers for the thermal cooling
device registering, let's provide a flag in the devfreq's profile to
tell the common devfreq code to register the newly created devfreq as
a cooling device.

Suggested-by: Chanwoo Choi 
Signed-off-by: Daniel Lezcano 
---
  V3:
- Rebased on linux-pm branch without units.h
- Set the cdev to NULL in case of error
- Added description for the cdev field in the devfreq structure
  V2:
- Added is_cooling_device boolean in profile structure
- Register cooling device when the is_cooling_device boolean is set
- Remove devfreq cooling device registration in the backend drivers
  V1:
- Register devfreq as a cooling device unconditionnally
---
  drivers/devfreq/devfreq.c | 13 +
  include/linux/devfreq.h   |  8 
  2 files changed, 21 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3047896e41..cf64aeec468f 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -11,6 +11,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -26,6 +27,7 @@
  #include 
  #include 
  #include 
+#include 
  #include "governor.h"
  
  #define CREATE_TRACE_POINTS

@@ -935,6 +937,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
  
  	mutex_unlock(_list_lock);
  
+	if (devfreq->profile->is_cooling_device) {

+   devfreq->cdev = devfreq_cooling_em_register(devfreq, NULL);
+   if (IS_ERR(devfreq->cdev)) {
+   dev_info(dev, "Failed to register devfreq "
+"cooling device\n");
+   devfreq->cdev = NULL;
+   }
+   }
+
return devfreq;
  
  err_init:

@@ -960,6 +971,8 @@ int devfreq_remove_device(struct devfreq *devfreq)
if (!devfreq)
return -EINVAL;
  
+	thermal_cooling_device_unregister(devfreq->cdev);


I have a question. Why don't you use devfreq_cooling_unregister()?
When thermal_cooling_device_unregister(), how can we remove
the pm_qos_request of devfreq device?


+
if (devfreq->governor) {
devfreq->governor->event_handler(devfreq,
 DEVFREQ_GOV_STOP, NULL);
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 26ea0850be9b..aba7ace11b72 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -98,11 +98,15 @@ struct devfreq_dev_status {
   * @freq_table:   Optional list of frequencies to support 
statistics
   *and freq_table must be generated in ascending order.
   * @max_state:The size of freq_table.
+ *
+ * @is_cooling_device: A self-explanatory boolean giving the device a
+ * cooling effect property.
   */
  struct devfreq_dev_profile {
unsigned long initial_freq;
unsigned int polling_ms;
enum devfreq_timer timer;
+   bool is_cooling_device;
  
  	int (*target)(struct device *dev, unsigned long *freq, u32 flags);

int (*get_dev_status)(struct device *dev,
@@ -156,6 +160,7 @@ struct devfreq_stats {
   * @suspend_count: suspend requests counter for a device.
   * @stats:Statistics of devfreq device behavior
   * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER 
notifier
+ * @cdev:  Cooling device pointer if the devfreq has cooling property
   * @nb_min:   Notifier block for DEV_PM_QOS_MIN_FREQUENCY
   * @nb_max:   Notifier block for DEV_PM_QOS_MAX_FREQUENCY
   *
@@ -198,6 +203,9 @@ struct devfreq {
  
  	struct srcu_notifier_head transition_notifier_list;
  
+	/* Pointer to the cooling device if used for thermal mitigation */

+   struct thermal_cooling_device *cdev;
+
struct notifier_block nb_min;
struct notifier_block nb_max;
  };




--
Best Regards,
Samsung Electronics
Chanwoo Choi