| Message ID | e195496f42375259f1b202eae39d20a56725f4a7.1547458732.git.amit.kucheria@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | Thermal throttling for SDM845 | expand |
Quoting Amit Kucheria (2019-01-14 02:21:06) > 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. > > Suggested-by: Stephen Boyd <swboyd@chromium.org> > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > include/linux/cpufreq.h | 6 ++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 6f23ebb395f1..7faebfc61e60 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -30,6 +30,7 @@ > #include <linux/syscore_ops.h> > #include <linux/tick.h> > #include <trace/events/power.h> > +#include <linux/cpu_cooling.h> Maybe this is supposed to be ordered alphabetically? If so, this should be much higher. > > static LIST_HEAD(cpufreq_policy_list); > > @@ -1318,6 +1319,12 @@ static int cpufreq_online(unsigned int cpu) > if (cpufreq_driver->ready) > cpufreq_driver->ready(policy); > > + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { > + struct thermal_cooling_device **cdev = &policy->cooldev; > + > + *cdev = of_cpufreq_cooling_register(policy); > + } This seems to be some complicated way of writing: policy->cooldev = of_cpufreq_cooling_register(policy); ? > + > pr_debug("initialization complete\n"); > > return 0; > @@ -1411,6 +1418,12 @@ static int cpufreq_offline(unsigned int cpu) > if (has_target()) > cpufreq_exit_governor(policy); > > + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { > + struct thermal_cooling_device **cdev = &policy->cooldev; > + > + cpufreq_cooling_unregister(*cdev); Similar? I'm confused!
On Tue, Jan 15, 2019 at 5:27 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Amit Kucheria (2019-01-14 02:21:06) > > 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. > > > > Suggested-by: Stephen Boyd <swboyd@chromium.org> > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > drivers/cpufreq/cpufreq.c | 13 +++++++++++++ > > include/linux/cpufreq.h | 6 ++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 6f23ebb395f1..7faebfc61e60 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -30,6 +30,7 @@ > > #include <linux/syscore_ops.h> > > #include <linux/tick.h> > > #include <trace/events/power.h> > > +#include <linux/cpu_cooling.h> > > Maybe this is supposed to be ordered alphabetically? If so, this should > be much higher. Fixed > > > > static LIST_HEAD(cpufreq_policy_list); > > > > @@ -1318,6 +1319,12 @@ static int cpufreq_online(unsigned int cpu) > > if (cpufreq_driver->ready) > > cpufreq_driver->ready(policy); > > > > + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { > > + struct thermal_cooling_device **cdev = &policy->cooldev; > > + > > + *cdev = of_cpufreq_cooling_register(policy); > > + } > > This seems to be some complicated way of writing: > > policy->cooldev = of_cpufreq_cooling_register(policy); > > ? Indeed. Fixed. > > + > > pr_debug("initialization complete\n"); > > > > return 0; > > @@ -1411,6 +1418,12 @@ static int cpufreq_offline(unsigned int cpu) > > if (has_target()) > > cpufreq_exit_governor(policy); > > > > + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { > > + struct thermal_cooling_device **cdev = &policy->cooldev; > > + > > + cpufreq_cooling_unregister(*cdev); > > Similar? I'm confused! > _un_register as opposed to register above. :-) Thanks for the review.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 6f23ebb395f1..7faebfc61e60 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -30,6 +30,7 @@ #include <linux/syscore_ops.h> #include <linux/tick.h> #include <trace/events/power.h> +#include <linux/cpu_cooling.h> static LIST_HEAD(cpufreq_policy_list); @@ -1318,6 +1319,12 @@ static int cpufreq_online(unsigned int cpu) if (cpufreq_driver->ready) cpufreq_driver->ready(policy); + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { + struct thermal_cooling_device **cdev = &policy->cooldev; + + *cdev = of_cpufreq_cooling_register(policy); + } + pr_debug("initialization complete\n"); return 0; @@ -1411,6 +1418,12 @@ static int cpufreq_offline(unsigned int cpu) if (has_target()) cpufreq_exit_governor(policy); + if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) { + struct thermal_cooling_device **cdev = &policy->cooldev; + + cpufreq_cooling_unregister(*cdev); + } + /* * Perform the ->exit() even during light-weight tear-down, * since this is a core component, and is essential for the diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 9fbc1d996967..0c51d5ce7350 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -388,6 +388,12 @@ struct cpufreq_driver { */ #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING (1 << 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 (1 << 7) + int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
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. Suggested-by: Stephen Boyd <swboyd@chromium.org> Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> --- drivers/cpufreq/cpufreq.c | 13 +++++++++++++ include/linux/cpufreq.h | 6 ++++++ 2 files changed, 19 insertions(+)