On Sat, 2 Dec 2017, Oleksandr Tyshchenko wrote:
> On Sat, Dec 2, 2017 at 3:06 AM, Stefano Stabellini
> <[email protected]> wrote:
> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Dmytryshyn <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: Oleksandr Tyshchenko <[email protected]>
> >> CC: Jan Beulich <[email protected]>
> >> CC: Andrew Cooper <[email protected]>
> >> CC: Stefano Stabellini <[email protected]>
> >> CC: Julien Grall <[email protected]>
> >> ---
> >> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
