| Message ID | 20200924123016.13427-2-ionela.voinescu@arm.com |
|---|---|
| State | Accepted |
| Commit | a20b7053b5c47cd7de23288238ea4d23502f300a |
| Headers | show |
| Series | [1/2] cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale() | expand |
On 24-09-20, 13:30, Ionela Voinescu wrote: > Compared to other arch_* functions, arch_set_freq_scale() has an atypical > weak definition that can be replaced by a strong architecture specific > implementation. > > The more typical support for architectural functions involves defining > an empty stub in a header file if the symbol is not already defined in > architecture code. Some examples involve: > - #define arch_scale_freq_capacity topology_get_freq_scale > - #define arch_scale_freq_invariant topology_scale_freq_invariant > - #define arch_scale_cpu_capacity topology_get_cpu_scale > - #define arch_update_cpu_topology topology_update_cpu_topology > - #define arch_scale_thermal_pressure topology_get_thermal_pressure > - #define arch_set_thermal_pressure topology_set_thermal_pressure > > Bring arch_set_freq_scale() in line with these functions by renaming it to > topology_set_freq_scale() in the arch topology driver, and by defining the > arch_set_freq_scale symbol to point to the new function for arm and arm64. > > While there are other users of the arch_topology driver, this patch defines > arch_set_freq_scale for arm and arm64 only, due to their existing > definitions of arch_scale_freq_capacity. This is the getter function of the > frequency invariance scale factor and without a getter function, the > setter function - arch_set_freq_scale() has not purpose. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > --- > arch/arm/include/asm/topology.h | 1 + > arch/arm64/include/asm/topology.h | 1 + > drivers/base/arch_topology.c | 4 ++-- > drivers/cpufreq/cpufreq.c | 7 ------- > include/linux/arch_topology.h | 2 ++ > include/linux/cpufreq.h | 11 ++++++++--- > 6 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h > index 9219e67befbe..e5e3d5ce4d55 100644 > --- a/arch/arm/include/asm/topology.h > +++ b/arch/arm/include/asm/topology.h > @@ -8,6 +8,7 @@ > #include <linux/arch_topology.h> > > /* Replace task scheduler's default frequency-invariant accounting */ > +#define arch_set_freq_scale topology_set_freq_scale > #define arch_scale_freq_capacity topology_get_freq_scale > #define arch_scale_freq_invariant topology_scale_freq_invariant > > diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h > index 7cb519473fbd..11a465243f66 100644 > --- a/arch/arm64/include/asm/topology.h > +++ b/arch/arm64/include/asm/topology.h > @@ -26,6 +26,7 @@ void topology_scale_freq_tick(void); > #endif /* CONFIG_ARM64_AMU_EXTN */ > > /* Replace task scheduler's default frequency-invariant accounting */ > +#define arch_set_freq_scale topology_set_freq_scale > #define arch_scale_freq_capacity topology_get_freq_scale > #define arch_scale_freq_invariant topology_scale_freq_invariant > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index 1705c772c273..ae5a596bcf86 100644 > --- a/drivers/base/arch_topology.c > +++ b/drivers/base/arch_topology.c > @@ -33,8 +33,8 @@ __weak bool arch_freq_counters_available(const struct cpumask *cpus) > } > DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; > > -void arch_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, > - unsigned long max_freq) > +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, > + unsigned long max_freq) > { > unsigned long scale; > int i; > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 2ea245a6c0c0..f34d3a3d5ba6 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy) > } > EXPORT_SYMBOL_GPL(get_cpu_idle_time); > > -__weak void arch_set_freq_scale(const struct cpumask *cpus, > - unsigned long cur_freq, > - unsigned long max_freq) > -{ > -} > -EXPORT_SYMBOL_GPL(arch_set_freq_scale); > - Why can't we have this anymore ? Because it is a macro now instead of a routine for ARM ? > /* > * This is a generic cpufreq init() routine which can be used by cpufreq > * drivers of SMP systems. It will do following: > diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h > index 083df331a3c9..0f6cd6b73a61 100644 > --- a/include/linux/arch_topology.h > +++ b/include/linux/arch_topology.h > @@ -30,6 +30,8 @@ static inline unsigned long topology_get_freq_scale(int cpu) > return per_cpu(freq_scale, cpu); > } > > +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, > + unsigned long max_freq); > bool topology_scale_freq_invariant(void); > > bool arch_freq_counters_available(const struct cpumask *cpus); > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 9f779fbdbe7b..fa37b1c66443 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -1011,9 +1011,14 @@ static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy, > extern void arch_freq_prepare_all(void); > extern unsigned int arch_freq_get_on_cpu(int cpu); > > -extern void arch_set_freq_scale(const struct cpumask *cpus, > - unsigned long cur_freq, > - unsigned long max_freq); > +#ifndef arch_set_freq_scale > +static __always_inline > +void arch_set_freq_scale(const struct cpumask *cpus, > + unsigned long cur_freq, > + unsigned long max_freq) > +{ > +} > +#endif > > /* the following are really really optional */ > extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs; > -- > 2.17.1
Hi Viresh, On Tuesday 06 Oct 2020 at 12:35:51 (+0530), Viresh Kumar wrote: > On 24-09-20, 13:30, Ionela Voinescu wrote: > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 2ea245a6c0c0..f34d3a3d5ba6 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy) > > } > > EXPORT_SYMBOL_GPL(get_cpu_idle_time); > > > > -__weak void arch_set_freq_scale(const struct cpumask *cpus, > > - unsigned long cur_freq, > > - unsigned long max_freq) > > -{ > > -} > > -EXPORT_SYMBOL_GPL(arch_set_freq_scale); > > - > > Why can't we have this anymore ? Because it is a macro now instead of a routine > for ARM ? Yes, instead of having a strong counterpart in arch_topology.c to be used for both arm64 and arm, I separated the definitions of the arch_set_freq_scale symbol in each architecture's topology.h in order to be filtered if needed (as is the case for arm's BL_SWITCHER). I could have kept either this weak stub definition or the stub that is now in cpufreq.h under "#ifndef arch_set_freq_scale", but I chose the latter as it's more aligned with the other architectural functions referenced in the commit message. So this patch has both the purpose of enabling a nicer filtering of frequency invariance for BL_SWITCHER but also follows the definition pattern of the other functions so we don't keep having two mindsets when working with them. Thanks, Ionela.
On 24-09-20, 13:30, Ionela Voinescu wrote: > Compared to other arch_* functions, arch_set_freq_scale() has an atypical > weak definition that can be replaced by a strong architecture specific > implementation. > > The more typical support for architectural functions involves defining > an empty stub in a header file if the symbol is not already defined in > architecture code. Some examples involve: > - #define arch_scale_freq_capacity topology_get_freq_scale > - #define arch_scale_freq_invariant topology_scale_freq_invariant > - #define arch_scale_cpu_capacity topology_get_cpu_scale > - #define arch_update_cpu_topology topology_update_cpu_topology > - #define arch_scale_thermal_pressure topology_get_thermal_pressure > - #define arch_set_thermal_pressure topology_set_thermal_pressure > > Bring arch_set_freq_scale() in line with these functions by renaming it to > topology_set_freq_scale() in the arch topology driver, and by defining the > arch_set_freq_scale symbol to point to the new function for arm and arm64. > > While there are other users of the arch_topology driver, this patch defines > arch_set_freq_scale for arm and arm64 only, due to their existing > definitions of arch_scale_freq_capacity. This is the getter function of the > frequency invariance scale factor and without a getter function, the > setter function - arch_set_freq_scale() has not purpose. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > --- > arch/arm/include/asm/topology.h | 1 + > arch/arm64/include/asm/topology.h | 1 + > drivers/base/arch_topology.c | 4 ++-- > drivers/cpufreq/cpufreq.c | 7 ------- > include/linux/arch_topology.h | 2 ++ > include/linux/cpufreq.h | 11 ++++++++--- > 6 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h > index 9219e67befbe..e5e3d5ce4d55 100644 > --- a/arch/arm/include/asm/topology.h > +++ b/arch/arm/include/asm/topology.h > @@ -8,6 +8,7 @@ > #include <linux/arch_topology.h> > > /* Replace task scheduler's default frequency-invariant accounting */ > +#define arch_set_freq_scale topology_set_freq_scale > #define arch_scale_freq_capacity topology_get_freq_scale > #define arch_scale_freq_invariant topology_scale_freq_invariant > > diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h > index 7cb519473fbd..11a465243f66 100644 > --- a/arch/arm64/include/asm/topology.h > +++ b/arch/arm64/include/asm/topology.h > @@ -26,6 +26,7 @@ void topology_scale_freq_tick(void); > #endif /* CONFIG_ARM64_AMU_EXTN */ > > /* Replace task scheduler's default frequency-invariant accounting */ > +#define arch_set_freq_scale topology_set_freq_scale > #define arch_scale_freq_capacity topology_get_freq_scale > #define arch_scale_freq_invariant topology_scale_freq_invariant > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index 1705c772c273..ae5a596bcf86 100644 > --- a/drivers/base/arch_topology.c > +++ b/drivers/base/arch_topology.c > @@ -33,8 +33,8 @@ __weak bool arch_freq_counters_available(const struct cpumask *cpus) > } > DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; > > -void arch_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, > - unsigned long max_freq) > +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, > + unsigned long max_freq) > { > unsigned long scale; > int i; > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 2ea245a6c0c0..f34d3a3d5ba6 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy) > } > EXPORT_SYMBOL_GPL(get_cpu_idle_time); > > -__weak void arch_set_freq_scale(const struct cpumask *cpus, > - unsigned long cur_freq, > - unsigned long max_freq) > -{ > -} > -EXPORT_SYMBOL_GPL(arch_set_freq_scale); > - > /* > * This is a generic cpufreq init() routine which can be used by cpufreq > * drivers of SMP systems. It will do following: > diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h > index 083df331a3c9..0f6cd6b73a61 100644 > --- a/include/linux/arch_topology.h > +++ b/include/linux/arch_topology.h > @@ -30,6 +30,8 @@ static inline unsigned long topology_get_freq_scale(int cpu) > return per_cpu(freq_scale, cpu); > } > > +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, > + unsigned long max_freq); > bool topology_scale_freq_invariant(void); > > bool arch_freq_counters_available(const struct cpumask *cpus); > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 9f779fbdbe7b..fa37b1c66443 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -1011,9 +1011,14 @@ static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy, > extern void arch_freq_prepare_all(void); > extern unsigned int arch_freq_get_on_cpu(int cpu); > > -extern void arch_set_freq_scale(const struct cpumask *cpus, > - unsigned long cur_freq, > - unsigned long max_freq); > +#ifndef arch_set_freq_scale > +static __always_inline > +void arch_set_freq_scale(const struct cpumask *cpus, > + unsigned long cur_freq, > + unsigned long max_freq) > +{ > +} > +#endif > > /* the following are really really optional */ > extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs; Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Thu, Sep 24, 2020 at 01:30:15PM +0100, Ionela Voinescu wrote: > Compared to other arch_* functions, arch_set_freq_scale() has an atypical > weak definition that can be replaced by a strong architecture specific > implementation. > > The more typical support for architectural functions involves defining > an empty stub in a header file if the symbol is not already defined in > architecture code. Some examples involve: > - #define arch_scale_freq_capacity topology_get_freq_scale > - #define arch_scale_freq_invariant topology_scale_freq_invariant > - #define arch_scale_cpu_capacity topology_get_cpu_scale > - #define arch_update_cpu_topology topology_update_cpu_topology > - #define arch_scale_thermal_pressure topology_get_thermal_pressure > - #define arch_set_thermal_pressure topology_set_thermal_pressure > > Bring arch_set_freq_scale() in line with these functions by renaming it to > topology_set_freq_scale() in the arch topology driver, and by defining the > arch_set_freq_scale symbol to point to the new function for arm and arm64. > > While there are other users of the arch_topology driver, this patch defines > arch_set_freq_scale for arm and arm64 only, due to their existing > definitions of arch_scale_freq_capacity. This is the getter function of the > frequency invariance scale factor and without a getter function, the > setter function - arch_set_freq_scale() has not purpose. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> For the arm64 changes: Acked-by: Catalin Marinas <catalin.marinas@arm.com>
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h index 9219e67befbe..e5e3d5ce4d55 100644 --- a/arch/arm/include/asm/topology.h +++ b/arch/arm/include/asm/topology.h @@ -8,6 +8,7 @@ #include <linux/arch_topology.h> /* Replace task scheduler's default frequency-invariant accounting */ +#define arch_set_freq_scale topology_set_freq_scale #define arch_scale_freq_capacity topology_get_freq_scale #define arch_scale_freq_invariant topology_scale_freq_invariant diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h index 7cb519473fbd..11a465243f66 100644 --- a/arch/arm64/include/asm/topology.h +++ b/arch/arm64/include/asm/topology.h @@ -26,6 +26,7 @@ void topology_scale_freq_tick(void); #endif /* CONFIG_ARM64_AMU_EXTN */ /* Replace task scheduler's default frequency-invariant accounting */ +#define arch_set_freq_scale topology_set_freq_scale #define arch_scale_freq_capacity topology_get_freq_scale #define arch_scale_freq_invariant topology_scale_freq_invariant diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index 1705c772c273..ae5a596bcf86 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -33,8 +33,8 @@ __weak bool arch_freq_counters_available(const struct cpumask *cpus) } DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; -void arch_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, - unsigned long max_freq) +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, + unsigned long max_freq) { unsigned long scale; int i; diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2ea245a6c0c0..f34d3a3d5ba6 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -160,13 +160,6 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy) } EXPORT_SYMBOL_GPL(get_cpu_idle_time); -__weak void arch_set_freq_scale(const struct cpumask *cpus, - unsigned long cur_freq, - unsigned long max_freq) -{ -} -EXPORT_SYMBOL_GPL(arch_set_freq_scale); - /* * This is a generic cpufreq init() routine which can be used by cpufreq * drivers of SMP systems. It will do following: diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h index 083df331a3c9..0f6cd6b73a61 100644 --- a/include/linux/arch_topology.h +++ b/include/linux/arch_topology.h @@ -30,6 +30,8 @@ static inline unsigned long topology_get_freq_scale(int cpu) return per_cpu(freq_scale, cpu); } +void topology_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq, + unsigned long max_freq); bool topology_scale_freq_invariant(void); bool arch_freq_counters_available(const struct cpumask *cpus); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 9f779fbdbe7b..fa37b1c66443 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -1011,9 +1011,14 @@ static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy, extern void arch_freq_prepare_all(void); extern unsigned int arch_freq_get_on_cpu(int cpu); -extern void arch_set_freq_scale(const struct cpumask *cpus, - unsigned long cur_freq, - unsigned long max_freq); +#ifndef arch_set_freq_scale +static __always_inline +void arch_set_freq_scale(const struct cpumask *cpus, + unsigned long cur_freq, + unsigned long max_freq) +{ +} +#endif /* the following are really really optional */ extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs;
Compared to other arch_* functions, arch_set_freq_scale() has an atypical weak definition that can be replaced by a strong architecture specific implementation. The more typical support for architectural functions involves defining an empty stub in a header file if the symbol is not already defined in architecture code. Some examples involve: - #define arch_scale_freq_capacity topology_get_freq_scale - #define arch_scale_freq_invariant topology_scale_freq_invariant - #define arch_scale_cpu_capacity topology_get_cpu_scale - #define arch_update_cpu_topology topology_update_cpu_topology - #define arch_scale_thermal_pressure topology_get_thermal_pressure - #define arch_set_thermal_pressure topology_set_thermal_pressure Bring arch_set_freq_scale() in line with these functions by renaming it to topology_set_freq_scale() in the arch topology driver, and by defining the arch_set_freq_scale symbol to point to the new function for arm and arm64. While there are other users of the arch_topology driver, this patch defines arch_set_freq_scale for arm and arm64 only, due to their existing definitions of arch_scale_freq_capacity. This is the getter function of the frequency invariance scale factor and without a getter function, the setter function - arch_set_freq_scale() has not purpose. Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> --- arch/arm/include/asm/topology.h | 1 + arch/arm64/include/asm/topology.h | 1 + drivers/base/arch_topology.c | 4 ++-- drivers/cpufreq/cpufreq.c | 7 ------- include/linux/arch_topology.h | 2 ++ include/linux/cpufreq.h | 11 ++++++++--- 6 files changed, 14 insertions(+), 12 deletions(-)