diff mbox series

[1/2] cpufreq,arm,arm64: restructure definitions of arch_set_freq_scale()

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

Commit Message

Ionela Voinescu Sept. 24, 2020, 12:30 p.m. UTC
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(-)

Comments

Viresh Kumar Oct. 6, 2020, 7:05 a.m. UTC | #1
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
Ionela Voinescu Oct. 6, 2020, 5:09 p.m. UTC | #2
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.
Viresh Kumar Oct. 7, 2020, 5:12 a.m. UTC | #3
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>
Catalin Marinas Oct. 7, 2020, 3:21 p.m. UTC | #4
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 mbox series

Patch

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;