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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi Stefano

On Tue, Dec 5, 2017 at 12:18 AM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> 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

Great, I will drop this patch.

>
>
>> 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.

I just want to show you real example, where we have ARM SoC +
turbo-mode + > 1 turbo freq
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/arch/arm64/boot/dts/renesas/r8a7795.dtsi?h=v4.9/rcar-3.5.9#n197
As you can see, there are two freqs marked as turbo-freqs: 1600000000
Hz and 1700000000 Hz

https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/arch/arm64/boot/dts/renesas/r8a7796.dtsi?h=v4.9/rcar-3.5.9#n166
For M3 SoC three turbo-freqs are used: 1600000000 Hz, 1700000000 Hz
and 1800000000 Hz

If a proposed below patch is not an option then we should find another
way to clarify second_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
>



-- 
Regards,

Oleksandr Tyshchenko

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