| Message ID | 6077cb33d7e078bb0e85776bad99fdb476f16d0c.1548737236.git.amit.kucheria@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cpufreq: Add flag to auto-register as cooling device | expand |
On 29-01-19, 10:25, 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 <swboyd@chromium.org> > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > Tested-by: Matthias Kaehlcke <mka@chromium.org> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > include/linux/cpufreq.h | 9 +++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index e35a886e00bc..0f9b50d3ee91 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -19,6 +19,7 @@ > > #include <linux/cpu.h> > #include <linux/cpufreq.h> > +#include <linux/cpu_cooling.h> > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/init.h> > @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) > if (cpufreq_driver->ready) > cpufreq_driver->ready(policy); > > +#if IS_ENABLED(CONFIG_CPU_THERMAL) > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > + policy->cdev = of_cpufreq_cooling_register(policy); > +#endif I am not sure if Rafael wanted it this way but maybe something like this: if (IS_ENABLED(CONFIG_CPU_THERMAL) && cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)) policy->cdev = of_cpufreq_cooling_register(policy); We never wanted ifdef hackery to be in there :) -- viresh
On Tue, Jan 29, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 29-01-19, 10:25, 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 <swboyd@chromium.org> > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > Tested-by: Matthias Kaehlcke <mka@chromium.org> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > > include/linux/cpufreq.h | 9 +++++++++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index e35a886e00bc..0f9b50d3ee91 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -19,6 +19,7 @@ > > > > #include <linux/cpu.h> > > #include <linux/cpufreq.h> > > +#include <linux/cpu_cooling.h> > > #include <linux/delay.h> > > #include <linux/device.h> > > #include <linux/init.h> > > @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) > > if (cpufreq_driver->ready) > > cpufreq_driver->ready(policy); > > > > +#if IS_ENABLED(CONFIG_CPU_THERMAL) > > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > > + policy->cdev = of_cpufreq_cooling_register(policy); > > +#endif > > I am not sure if Rafael wanted it this way but maybe something like this: > > if (IS_ENABLED(CONFIG_CPU_THERMAL) && > cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)) > policy->cdev = of_cpufreq_cooling_register(policy); > > We never wanted ifdef hackery to be in there :) OK, that makes more sense. Should I just send out a fixup patch or the entire series?
On Tue, Jan 29, 2019 at 11:16 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > On Tue, Jan 29, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 29-01-19, 10:25, 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 <swboyd@chromium.org> > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > > Tested-by: Matthias Kaehlcke <mka@chromium.org> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > --- > > > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > > > include/linux/cpufreq.h | 9 +++++++++ > > > 2 files changed, 22 insertions(+) > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > index e35a886e00bc..0f9b50d3ee91 100644 > > > --- a/drivers/cpufreq/cpufreq.c > > > +++ b/drivers/cpufreq/cpufreq.c > > > @@ -19,6 +19,7 @@ > > > > > > #include <linux/cpu.h> > > > #include <linux/cpufreq.h> > > > +#include <linux/cpu_cooling.h> > > > #include <linux/delay.h> > > > #include <linux/device.h> > > > #include <linux/init.h> > > > @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) > > > if (cpufreq_driver->ready) > > > cpufreq_driver->ready(policy); > > > > > > +#if IS_ENABLED(CONFIG_CPU_THERMAL) > > > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > > > + policy->cdev = of_cpufreq_cooling_register(policy); > > > +#endif > > > > I am not sure if Rafael wanted it this way but maybe something like this: > > > > if (IS_ENABLED(CONFIG_CPU_THERMAL) && > > cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)) > > policy->cdev = of_cpufreq_cooling_register(policy); > > > > We never wanted ifdef hackery to be in there :) > > OK, that makes more sense. Should I just send out a fixup patch or the > entire series? FWIW, I checked drivers/cpufreq and drivers/thermal before converting over and there is a mixed use of #if IS_ENABLED and if(IS_ENABLED). Perhaps we should clean it up?
On 29-01-19, 11:50, Amit Kucheria wrote: > On Tue, Jan 29, 2019 at 11:16 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > > > On Tue, Jan 29, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 29-01-19, 10:25, 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 <swboyd@chromium.org> > > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > > > Tested-by: Matthias Kaehlcke <mka@chromium.org> > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > --- > > > > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > > > > include/linux/cpufreq.h | 9 +++++++++ > > > > 2 files changed, 22 insertions(+) > > > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > > index e35a886e00bc..0f9b50d3ee91 100644 > > > > --- a/drivers/cpufreq/cpufreq.c > > > > +++ b/drivers/cpufreq/cpufreq.c > > > > @@ -19,6 +19,7 @@ > > > > > > > > #include <linux/cpu.h> > > > > #include <linux/cpufreq.h> > > > > +#include <linux/cpu_cooling.h> > > > > #include <linux/delay.h> > > > > #include <linux/device.h> > > > > #include <linux/init.h> > > > > @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) > > > > if (cpufreq_driver->ready) > > > > cpufreq_driver->ready(policy); > > > > > > > > +#if IS_ENABLED(CONFIG_CPU_THERMAL) > > > > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > > > > + policy->cdev = of_cpufreq_cooling_register(policy); > > > > +#endif > > > > > > I am not sure if Rafael wanted it this way but maybe something like this: > > > > > > if (IS_ENABLED(CONFIG_CPU_THERMAL) && > > > cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)) > > > policy->cdev = of_cpufreq_cooling_register(policy); > > > > > > We never wanted ifdef hackery to be in there :) > > > > OK, that makes more sense. Should I just send out a fixup patch or the > > entire series? Single patch should be fine I believe. > FWIW, I checked drivers/cpufreq and drivers/thermal before converting > over and there is a mixed use of #if IS_ENABLED and if(IS_ENABLED). > > Perhaps we should clean it up? No objections from me on that. -- viresh
On Tue, Jan 29, 2019 at 6:46 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > On Tue, Jan 29, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 29-01-19, 10:25, 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 <swboyd@chromium.org> > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > > Tested-by: Matthias Kaehlcke <mka@chromium.org> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > --- > > > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > > > include/linux/cpufreq.h | 9 +++++++++ > > > 2 files changed, 22 insertions(+) > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > index e35a886e00bc..0f9b50d3ee91 100644 > > > --- a/drivers/cpufreq/cpufreq.c > > > +++ b/drivers/cpufreq/cpufreq.c > > > @@ -19,6 +19,7 @@ > > > > > > #include <linux/cpu.h> > > > #include <linux/cpufreq.h> > > > +#include <linux/cpu_cooling.h> > > > #include <linux/delay.h> > > > #include <linux/device.h> > > > #include <linux/init.h> > > > @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) > > > if (cpufreq_driver->ready) > > > cpufreq_driver->ready(policy); > > > > > > +#if IS_ENABLED(CONFIG_CPU_THERMAL) > > > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > > > + policy->cdev = of_cpufreq_cooling_register(policy); > > > +#endif > > > > I am not sure if Rafael wanted it this way but maybe something like this: > > > > if (IS_ENABLED(CONFIG_CPU_THERMAL) && > > cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)) > > policy->cdev = of_cpufreq_cooling_register(policy); > > > > We never wanted ifdef hackery to be in there :) > > OK, that makes more sense. Should I just send out a fixup patch or the > entire series? Just a fixup patch, please.
On Tue, Jan 29, 2019 at 8:09 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 29-01-19, 11:50, Amit Kucheria wrote: > > On Tue, Jan 29, 2019 at 11:16 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > > > > > On Tue, Jan 29, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > On 29-01-19, 10:25, 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 <swboyd@chromium.org> > > > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > > > > Tested-by: Matthias Kaehlcke <mka@chromium.org> > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > --- > > > > > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > > > > > include/linux/cpufreq.h | 9 +++++++++ > > > > > 2 files changed, 22 insertions(+) > > > > > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > > > index e35a886e00bc..0f9b50d3ee91 100644 > > > > > --- a/drivers/cpufreq/cpufreq.c > > > > > +++ b/drivers/cpufreq/cpufreq.c > > > > > @@ -19,6 +19,7 @@ > > > > > > > > > > #include <linux/cpu.h> > > > > > #include <linux/cpufreq.h> > > > > > +#include <linux/cpu_cooling.h> > > > > > #include <linux/delay.h> > > > > > #include <linux/device.h> > > > > > #include <linux/init.h> > > > > > @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) > > > > > if (cpufreq_driver->ready) > > > > > cpufreq_driver->ready(policy); > > > > > > > > > > +#if IS_ENABLED(CONFIG_CPU_THERMAL) > > > > > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > > > > > + policy->cdev = of_cpufreq_cooling_register(policy); > > > > > +#endif > > > > > > > > I am not sure if Rafael wanted it this way but maybe something like this: > > > > > > > > if (IS_ENABLED(CONFIG_CPU_THERMAL) && > > > > cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)) > > > > policy->cdev = of_cpufreq_cooling_register(policy); > > > > > > > > We never wanted ifdef hackery to be in there :) > > > > > > OK, that makes more sense. Should I just send out a fixup patch or the > > > entire series? > > Single patch should be fine I believe. > > > FWIW, I checked drivers/cpufreq and drivers/thermal before converting > > over and there is a mixed use of #if IS_ENABLED and if(IS_ENABLED). > > > > Perhaps we should clean it up? > > No objections from me on that. Generally speaking, though, "if (IS_ENABLED(SYMBOL))" can only be used if all of the symbols in the conditional branch are defined regardless of whether or not SYMBOL itself is defined, so careful there.
On Tue, Jan 29, 2019 at 9:20 PM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > On Tue, Jan 29, 2019 at 2:43 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Tue, Jan 29, 2019 at 6:46 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > > > > > On Tue, Jan 29, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > On 29-01-19, 10:25, 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 <swboyd@chromium.org> > > > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > > > > Tested-by: Matthias Kaehlcke <mka@chromium.org> > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > --- > > > > > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > > > > > include/linux/cpufreq.h | 9 +++++++++ > > > > > 2 files changed, 22 insertions(+) > > > > > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > > > index e35a886e00bc..0f9b50d3ee91 100644 > > > > > --- a/drivers/cpufreq/cpufreq.c > > > > > +++ b/drivers/cpufreq/cpufreq.c > > > > > @@ -19,6 +19,7 @@ > > > > > > > > > > #include <linux/cpu.h> > > > > > #include <linux/cpufreq.h> > > > > > +#include <linux/cpu_cooling.h> > > > > > #include <linux/delay.h> > > > > > #include <linux/device.h> > > > > > #include <linux/init.h> > > > > > @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) > > > > > if (cpufreq_driver->ready) > > > > > cpufreq_driver->ready(policy); > > > > > > > > > > +#if IS_ENABLED(CONFIG_CPU_THERMAL) > > > > > + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) > > > > > + policy->cdev = of_cpufreq_cooling_register(policy); > > > > > +#endif > > > > > > > > I am not sure if Rafael wanted it this way but maybe something like this: > > > > > > > > if (IS_ENABLED(CONFIG_CPU_THERMAL) && > > > > cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV)) > > > > policy->cdev = of_cpufreq_cooling_register(policy); > > > > > > > > We never wanted ifdef hackery to be in there :) > > > > > > OK, that makes more sense. Should I just send out a fixup patch or the > > > entire series? > > > > Just a fixup patch, please. > > Please find attached a fixup patch suitable to add to the top of the > series and use git rebase --autosquash on. This isn't Patchwork-friendly, so can you send the complete patch instead, please?
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index e35a886e00bc..0f9b50d3ee91 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -19,6 +19,7 @@ #include <linux/cpu.h> #include <linux/cpufreq.h> +#include <linux/cpu_cooling.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/init.h> @@ -1318,6 +1319,11 @@ static int cpufreq_online(unsigned int cpu) if (cpufreq_driver->ready) cpufreq_driver->ready(policy); +#if IS_ENABLED(CONFIG_CPU_THERMAL) + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) + policy->cdev = of_cpufreq_cooling_register(policy); +#endif + pr_debug("initialization complete\n"); return 0; @@ -1405,6 +1411,13 @@ static int cpufreq_offline(unsigned int cpu) goto unlock; } +#if IS_ENABLED(CONFIG_CPU_THERMAL) + if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { + cpufreq_cooling_unregister(policy->cdev); + policy->cdev = NULL; + } +#endif + if (cpufreq_driver->stop_cpu) cpufreq_driver->stop_cpu(policy); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index bd7fbd6a4478..6078eb07a7e4 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_IS_COOLING_DEV BIT(7) + int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);