Re: [PATCH] platform: x86: Support Turbo Boost Max for non HWP systems

[Darren Hart]

On Wed, Jan 11, 2017 at 12:36:34PM -0800, Srinivas Pandruvada wrote:
> On platforms supporting Intel Turbo Boost Max Technology 3.0, the
> maximum turbo frequencies (turbo ratio) of some cores in a CPU package
> may be higher than the other cores in the same package.  In that case,
> better performance can be achieved by making the scheduler prefer to run
> tasks on the CPUs with higher max turbo frequencies.
> 
> On Intel® Broadwell Xeon systems, it is optional to turn on HWP
> (Hardware P-States). When HWP is not turned on, the BIOS doesn't
> present required CPPC (Collaborative Processor Performance Control)
> tables. This table is used to get the per CPU core maximum performance
> ratio and inform scheduler (in cpufreq/intel_pstate driver).
> 
> On such systems the maximum performance ratio can be read via over
> clocking (OC) mailbox interface for each CPU. This interface is not
> architectural and can change for every model of processors.
> 
> This driver reads maximum performance ratio of each CPU and set up
> the scheduler priority metrics. In this way scheduler can prefer CPU
> with higher performance to schedule tasks.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>

Thanks Srinivas,

Driver queued to testing with the following changes, but see below...

diff --git a/drivers/platform/x86/intel_turbo_boost_max_enum.c b/drivers/platform/x86/intel_turbo_boost_max_enum.c
index 5ad3257..5df43c9 100644
--- a/drivers/platform/x86/intel_turbo_boost_max_enum.c
+++ b/drivers/platform/x86/intel_turbo_boost_max_enum.c
@@ -1,6 +1,7 @@
 /*
  * Intel Turbo Boost Max Technology 3.0 legacy (non HWP) enumeration driver
  * Copyright (c) 2017, Intel Corporation.
+ * All rights reserved.

This is the preferred format last time I asked Intel legal.

  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -37,10 +38,8 @@
 
 static int get_oc_core_priority(unsigned int cpu)
 {
-       u64 value;
-       u64 cmd = OC_MAILBOX_FC_CONTROL_CMD;
-       int i;
-       int ret;
+       u64 value, cmd = OC_MAILBOX_FC_CONTROL_CMD;
+       int ret, i;
 
Subjective, but we prefer to save the lines.

        /* Issue favored core read command */
        value = cmd << MSR_OC_MAILBOX_CMD_OFFSET;


> ---
>  drivers/platform/x86/Kconfig                      |  10 ++
>  drivers/platform/x86/Makefile                     |   1 +
>  drivers/platform/x86/intel_turbo_boost_max_enum.c | 153 ++++++++++++++++++++++

Regarding the name, two nits:

1) It's soooooooooooooooo long..... and the CONFIG_* too.
2) Since it is BDW specifc, how about:

intel_bdw_turbo.c
CONFIG_INTEL_BDW_TURBO

I don't think "max enumeration" conveys any meaning to most readers.

Thoughts?

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center