Re: [Xen-devel] [RFC PATCH 04/31] cpufreq: make turbo settings to be configurable

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 2 Dec 2017, Oleksandr Tyshchenko wrote:
> On Sat, Dec 2, 2017 at 3:06 AM, Stefano Stabellini
> <sstabellini@xxxxxxxxxx> wrote:
> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
> >>
> >> This settings is not needed for some architectures.
> >> So make it to be configurable and use it for x86
> >> architecture.
> >>
> >> This is a rebased version of the original patch:
> >> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00942.html
> >>
> >> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >> CC: Jan Beulich <jbeulich@xxxxxxxx>
> >> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> CC: Julien Grall <julien.grall@xxxxxxxxxx>
> >> ---
> >>  xen/arch/x86/Kconfig          |  1 +
> >>  xen/drivers/cpufreq/Kconfig   |  3 +++
> >>  xen/drivers/cpufreq/utility.c | 11 ++++++++++-
> >>  xen/drivers/pm/stat.c         |  6 ++++++
> >>  xen/include/xen/cpufreq.h     |  6 ++++++
> >>  5 files changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> >> index 86c8eca..c1eac1d 100644
> >> --- a/xen/arch/x86/Kconfig
> >> +++ b/xen/arch/x86/Kconfig
> >> @@ -24,6 +24,7 @@ config X86
> >>       select NUMA
> >>       select VGA
> >>       select HAS_PM
> >> +     select HAS_CPU_TURBO
> >>
> >>  config ARCH_DEFCONFIG
> >>       string
> >> diff --git a/xen/drivers/cpufreq/Kconfig b/xen/drivers/cpufreq/Kconfig
> >> index cce80f4..427ea2a 100644
> >> --- a/xen/drivers/cpufreq/Kconfig
> >> +++ b/xen/drivers/cpufreq/Kconfig
> >> @@ -1,3 +1,6 @@
> >>
> >>  config HAS_CPUFREQ
> >>       bool
> >> +
> >> +config HAS_CPU_TURBO
> >> +     bool
> >> diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
> >> index a687e5a..25bf983 100644
> >> --- a/xen/drivers/cpufreq/utility.c
> >> +++ b/xen/drivers/cpufreq/utility.c
> >> @@ -209,7 +209,9 @@ int cpufreq_frequency_table_cpuinfo(struct 
> >> cpufreq_policy *policy,
> >>  {
> >>      unsigned int min_freq = ~0;
> >>      unsigned int max_freq = 0;
> >> +#ifdef CONFIG_HAS_CPU_TURBO
> >>      unsigned int second_max_freq = 0;
> >> +#endif
> >>      unsigned int i;
> >>
> >>      for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
> >> @@ -221,6 +223,7 @@ int cpufreq_frequency_table_cpuinfo(struct 
> >> cpufreq_policy *policy,
> >>          if (freq > max_freq)
> >>              max_freq = freq;
> >>      }
> >> +#ifdef CONFIG_HAS_CPU_TURBO
> >>      for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
> >>          unsigned int freq = table[i].frequency;
> >>          if (freq == CPUFREQ_ENTRY_INVALID || freq == max_freq)
> >> @@ -234,9 +237,13 @@ int cpufreq_frequency_table_cpuinfo(struct 
> >> cpufreq_policy *policy,
> >>          printk("max_freq: %u    second_max_freq: %u\n",
> >>                 max_freq, second_max_freq);
> >>
> >> +    policy->cpuinfo.second_max_freq = second_max_freq;
> >> +#else /* !CONFIG_HAS_CPU_TURBO */
> >> +    if (cpufreq_verbose)
> >> +        printk("max_freq: %u\n", max_freq);
> >> +#endif /* CONFIG_HAS_CPU_TURBO */
> >>      policy->min = policy->cpuinfo.min_freq = min_freq;
> >>      policy->max = policy->cpuinfo.max_freq = max_freq;
> >> -    policy->cpuinfo.second_max_freq = second_max_freq;
> >>
> >>      if (policy->min == ~0)
> >>          return -EINVAL;
> >> @@ -390,6 +397,7 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned 
> >> int flag)
> >>      return policy->cur;
> >>  }
> >>
> >> +#ifdef CONFIG_HAS_CPU_TURBO
> >>  int cpufreq_update_turbo(int cpuid, int new_state)
> >>  {
> >>      struct cpufreq_policy *policy;
> >> @@ -430,6 +438,7 @@ int cpufreq_get_turbo_status(int cpuid)
> >>      policy = per_cpu(cpufreq_cpu_policy, cpuid);
> >>      return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;
> >>  }
> >> +#endif /* CONFIG_HAS_CPU_TURBO */
> >>
> >>  /*********************************************************************
> >>   *                 POLICY                                            *
> >
> > I am wondering if we need to go as far as #ifdef'ing
> > cpufreq_update_turbo. For the sake of reducing the number if #ifdef's,
> > would it be enough if we only make sure it is disabled?
> >
> > In other words, I would keep the changes to stat.c but I would leave
> > utility.c and cpufreq.h pretty much untouched.
> 
> Yes. I was thinking about dropping this patch at all. If platform
> doesn't support CPU Boost, the platform
> driver should just inform framework about that (policy->turbo =
> CPUFREQ_TURBO_UNSUPPORTED).
> That's all.

Right


> cpufreq_update_turbo() will return -EOPNOTSUPP if someone tries to
> enable/disable turbo mode.
> cpufreq_get_turbo_status() will return that turbo mode "is not enabled".

Exactly what I was thinking


> Another question is second_max_freq. As I understand, it is highest
> non-turbo frequency calculated by framework to limit target frequency
> when
> turbo mode "is disabled". And Xen assumes that second_max_freq is
> always P1 if turbo mode is on.
> But, there might be a case when a few highest frequencies are
> turbo-frequencies. So, I propose to add an extra flag for handling
> that.
> So, each CPUFreq driver responsibility will be to mark
> turbo-frequency(ies) for the framework to properly calculate
> second_max_freq.

As Andre wrote, we can start simply assuming that ARM doesn't have
turbo. If turbo mode is assumed to be off, I don't think we need the
patch below and the new flag, because second_max_freq == max_freq.


> Something like that:
> 
> diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
> index 25bf983..122a88b 100644
> --- a/xen/drivers/cpufreq/utility.c
> +++ b/xen/drivers/cpufreq/utility.c
> @@ -226,7 +226,8 @@ int cpufreq_frequency_table_cpuinfo(struct
> cpufreq_policy *policy,
>  #ifdef CONFIG_HAS_CPU_TURBO
>      for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
>          unsigned int freq = table[i].frequency;
> -        if (freq == CPUFREQ_ENTRY_INVALID || freq == max_freq)
> +        if ((freq == CPUFREQ_ENTRY_INVALID) ||
> +            (table[i].flags & CPUFREQ_BOOST_FREQ))
>              continue;
>          if (freq > second_max_freq)
>              second_max_freq = freq;
> diff --git a/xen/include/xen/cpufreq.h b/xen/include/xen/cpufreq.h
> index 2e0c16a..77b29da 100644
> --- a/xen/include/xen/cpufreq.h
> +++ b/xen/include/xen/cpufreq.h
> @@ -204,7 +204,11 @@ void cpufreq_verify_within_limits(struct
> cpufreq_policy *policy,
>  #define CPUFREQ_ENTRY_INVALID ~0
>  #define CPUFREQ_TABLE_END     ~1
> 
> +/* Special Values of .flags field */
> +#define CPUFREQ_BOOST_FREQ    (1 << 0)
> +
>  struct cpufreq_frequency_table {
> +       unsigned int    flags;
>      unsigned int    index;     /* any */
>      unsigned int    frequency; /* kHz - doesn't need to be in ascending
>                                  * order */
> 
> Both existing on x86 CPUFreq drivers just need to mark P0 frequency as
> a turbo-frequency if turbo mode "is supported". Am I correct?
> 
> And the most important question is how to recognize in Xen on ARM
> (using SCPI protocol) which frequencies are turbo-frequencies
> actually? I couldn't find any information regarding that in protocol
> description.
> For DT-based CPUFreq it is not an issue, since there is a specific
> property "turbo-mode" to mark corresponding OPPs. [1].
> But neither SCPI DT bindings [2] nor the SCPI protocol itself [3]
> mentions about it. Perhaps, additional command should be added to pass
> such info.
> 
> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/opp/opp.txt
> [2] 
> http://elixir.free-electrons.com/linux/v4.15-rc1/source/Documentation/devicetree/bindings/arm/arm,scpi.txt
> [3] 
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0922g/scp_message_interface_v1_2_DUI0922G_en.pdf


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel