Re: [PATCH v4 2/9] cpufreq: Auto-register the driver as a thermal cooling device if asked

[Amit Kucheria]

On Mon, Jan 28, 2019 at 5:34 PM Rafael J. Wysocki  wrote:
>
> On Monday, January 28, 2019 9:32:44 AM CET Daniel Lezcano wrote:
> > On 28/01/2019 07:41, Amit Kucheria wrote:
> > > All cpufreq drivers do similar things to register as a cooling device.
> > > Provide a cpufreq driver flag so drivers can just ask the cpufreq core
> > > to register the cooling device on their behalf. This allows us to get
> > > rid of duplicated code in the drivers.
> > >
> > > In order to allow this, we add a struct thermal_cooling_device pointer
> > > to struct cpufreq_policy so that drivers don't need to store it in a
> > > private data structure.
> > >
> > > Suggested-by: Stephen Boyd 
> > > Suggested-by: Viresh Kumar 
> > > Signed-off-by: Amit Kucheria 
> > > Reviewed-by: Matthias Kaehlcke 
> > > Tested-by: Matthias Kaehlcke 
> > > ---
> >
> > [ ... ]
> >
> > > +/*
> > > + * Set by drivers that want the core to automatically register the 
> > > cpufreq
> > > + * driver as a thermal cooling device.
> > > + */
> > > +#define CPUFREQ_AUTO_REGISTER_COOLING_DEV  BIT(7)
> > > +
> >
> > Isn't the CPUFREQ_IS_COOLING_DEV more appropriate? We define a property
> > of the cpufreq driver and the resulting action is to auto-register, no?
>
> Yes.

OK, will send out another series with this change and the IS_ENABLED guards.

Thank you everyone for the review.

Re: [PATCH v4 2/9] cpufreq: Auto-register the driver as a thermal cooling device if asked

[Rafael J. Wysocki]

On Monday, January 28, 2019 9:32:44 AM CET Daniel Lezcano wrote:
> On 28/01/2019 07:41, Amit Kucheria wrote:
> > All cpufreq drivers do similar things to register as a cooling device.
> > Provide a cpufreq driver flag so drivers can just ask the cpufreq core
> > to register the cooling device on their behalf. This allows us to get
> > rid of duplicated code in the drivers.
> > 
> > In order to allow this, we add a struct thermal_cooling_device pointer
> > to struct cpufreq_policy so that drivers don't need to store it in a
> > private data structure.
> > 
> > Suggested-by: Stephen Boyd 
> > Suggested-by: Viresh Kumar 
> > Signed-off-by: Amit Kucheria 
> > Reviewed-by: Matthias Kaehlcke 
> > Tested-by: Matthias Kaehlcke 
> > ---
> 
> [ ... ]
> 
> > +/*
> > + * Set by drivers that want the core to automatically register the cpufreq
> > + * driver as a thermal cooling device.
> > + */
> > +#define CPUFREQ_AUTO_REGISTER_COOLING_DEV  BIT(7)
> > +
> 
> Isn't the CPUFREQ_IS_COOLING_DEV more appropriate? We define a property
> of the cpufreq driver and the resulting action is to auto-register, no?

Yes.

Cheers,
Rafael

Re: [PATCH v4 2/9] cpufreq: Auto-register the driver as a thermal cooling device if asked

[Rafael J. Wysocki]

On Mon, Jan 28, 2019 at 7:44 AM Amit Kucheria  wrote:
>
> All cpufreq drivers do similar things to register as a cooling device.
> Provide a cpufreq driver flag so drivers can just ask the cpufreq core
> to register the cooling device on their behalf. This allows us to get
> rid of duplicated code in the drivers.
>
> In order to allow this, we add a struct thermal_cooling_device pointer
> to struct cpufreq_policy so that drivers don't need to store it in a
> private data structure.
>
> Suggested-by: Stephen Boyd 
> Suggested-by: Viresh Kumar 
> Signed-off-by: Amit Kucheria 
> Reviewed-by: Matthias Kaehlcke 
> Tested-by: Matthias Kaehlcke 
> ---
>  drivers/cpufreq/cpufreq.c | 9 +
>  include/linux/cpufreq.h   | 9 +
>  2 files changed, 18 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e35a886e00bc..29ed78b0b77b 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -19,6 +19,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1318,6 +1319,9 @@ static int cpufreq_online(unsigned int cpu)
> if (cpufreq_driver->ready)
> cpufreq_driver->ready(policy);
>
> +   if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV)

It would be good to use IS_ENABLED(CONFIG_CPU_THERMAL) here and below
as a hint for the compiler.

> +   policy->cdev = of_cpufreq_cooling_register(policy);
> +
> pr_debug("initialization complete\n");
>
> return 0;
> @@ -1405,6 +1409,11 @@ static int cpufreq_offline(unsigned int cpu)
> goto unlock;
> }
>
> +   if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {
> +   cpufreq_cooling_unregister(policy->cdev);
> +   policy->cdev = NULL;
> +   }
> +
> if (cpufreq_driver->stop_cpu)
> cpufreq_driver->stop_cpu(policy);
>

Re: [PATCH v4 2/9] cpufreq: Auto-register the driver as a thermal cooling device if asked

[Daniel Lezcano]

On 28/01/2019 07:41, Amit Kucheria wrote:
> All cpufreq drivers do similar things to register as a cooling device.
> Provide a cpufreq driver flag so drivers can just ask the cpufreq core
> to register the cooling device on their behalf. This allows us to get
> rid of duplicated code in the drivers.
> 
> In order to allow this, we add a struct thermal_cooling_device pointer
> to struct cpufreq_policy so that drivers don't need to store it in a
> private data structure.
> 
> Suggested-by: Stephen Boyd 
> Suggested-by: Viresh Kumar 
> Signed-off-by: Amit Kucheria 
> Reviewed-by: Matthias Kaehlcke 
> Tested-by: Matthias Kaehlcke 
> ---

[ ... ]

> +/*
> + * Set by drivers that want the core to automatically register the cpufreq
> + * driver as a thermal cooling device.
> + */
> +#define CPUFREQ_AUTO_REGISTER_COOLING_DEVBIT(7)
> +

Isn't the CPUFREQ_IS_COOLING_DEV more appropriate? We define a property
of the cpufreq driver and the resulting action is to auto-register, no?



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

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v4 2/9] cpufreq: Auto-register the driver as a thermal cooling device if asked

[Daniel Lezcano]

On 28/01/2019 07:41, Amit Kucheria wrote:
> All cpufreq drivers do similar things to register as a cooling device.
> Provide a cpufreq driver flag so drivers can just ask the cpufreq core
> to register the cooling device on their behalf. This allows us to get
> rid of duplicated code in the drivers.
> 
> In order to allow this, we add a struct thermal_cooling_device pointer
> to struct cpufreq_policy so that drivers don't need to store it in a
> private data structure.
> 
> Suggested-by: Stephen Boyd 
> Suggested-by: Viresh Kumar 
> Signed-off-by: Amit Kucheria 
> Reviewed-by: Matthias Kaehlcke 
> Tested-by: Matthias Kaehlcke 
> ---

Reviewed-by: Daniel Lezcano 



>  drivers/cpufreq/cpufreq.c | 9 +
>  include/linux/cpufreq.h   | 9 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e35a886e00bc..29ed78b0b77b 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -19,6 +19,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1318,6 +1319,9 @@ static int cpufreq_online(unsigned int cpu)
>   if (cpufreq_driver->ready)
>   cpufreq_driver->ready(policy);
>  
> + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV)
> + policy->cdev = of_cpufreq_cooling_register(policy);
> +
>   pr_debug("initialization complete\n");
>  
>   return 0;
> @@ -1405,6 +1409,11 @@ static int cpufreq_offline(unsigned int cpu)
>   goto unlock;
>   }
>  
> + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {
> + cpufreq_cooling_unregister(policy->cdev);
> + policy->cdev = NULL;
> + }
> +
>   if (cpufreq_driver->stop_cpu)
>   cpufreq_driver->stop_cpu(policy);
>  
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index bd7fbd6a4478..55ca61a64fc2 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -151,6 +151,9 @@ struct cpufreq_policy {
>  
>   /* For cpufreq driver's internal use */
>   void*driver_data;
> +
> + /* Pointer to the cooling device if used for thermal mitigation */
> + struct thermal_cooling_device *cdev;
>  };
>  
>  /* Only for ACPI */
> @@ -386,6 +389,12 @@ struct cpufreq_driver {
>   */
>  #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHINGBIT(6)
>  
> +/*
> + * Set by drivers that want the core to automatically register the cpufreq
> + * driver as a thermal cooling device.
> + */
> +#define CPUFREQ_AUTO_REGISTER_COOLING_DEVBIT(7)
> +
>  int cpufreq_register_driver(struct cpufreq_driver *driver_data);
>  int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
>  
> 


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

Follow Linaro:   Facebook |
 Twitter |
 Blog

[PATCH v4 2/9] cpufreq: Auto-register the driver as a thermal cooling device if asked

[Amit Kucheria]

All cpufreq drivers do similar things to register as a cooling device.
Provide a cpufreq driver flag so drivers can just ask the cpufreq core
to register the cooling device on their behalf. This allows us to get
rid of duplicated code in the drivers.

In order to allow this, we add a struct thermal_cooling_device pointer
to struct cpufreq_policy so that drivers don't need to store it in a
private data structure.

Suggested-by: Stephen Boyd 
Suggested-by: Viresh Kumar 
Signed-off-by: Amit Kucheria 
Reviewed-by: Matthias Kaehlcke 
Tested-by: Matthias Kaehlcke 
---
 drivers/cpufreq/cpufreq.c | 9 +
 include/linux/cpufreq.h   | 9 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e35a886e00bc..29ed78b0b77b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1318,6 +1319,9 @@ static int cpufreq_online(unsigned int cpu)
if (cpufreq_driver->ready)
cpufreq_driver->ready(policy);
 
+   if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV)
+   policy->cdev = of_cpufreq_cooling_register(policy);
+
pr_debug("initialization complete\n");
 
return 0;
@@ -1405,6 +1409,11 @@ static int cpufreq_offline(unsigned int cpu)
goto unlock;
}
 
+   if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {
+   cpufreq_cooling_unregister(policy->cdev);
+   policy->cdev = NULL;
+   }
+
if (cpufreq_driver->stop_cpu)
cpufreq_driver->stop_cpu(policy);
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index bd7fbd6a4478..55ca61a64fc2 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -151,6 +151,9 @@ struct cpufreq_policy {
 
/* For cpufreq driver's internal use */
void*driver_data;
+
+   /* Pointer to the cooling device if used for thermal mitigation */
+   struct thermal_cooling_device *cdev;
 };
 
 /* Only for ACPI */
@@ -386,6 +389,12 @@ struct cpufreq_driver {
  */
 #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING  BIT(6)
 
+/*
+ * Set by drivers that want the core to automatically register the cpufreq
+ * driver as a thermal cooling device.
+ */
+#define CPUFREQ_AUTO_REGISTER_COOLING_DEV  BIT(7)
+
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
 
-- 
2.17.1