Re: [PATCH v6 2/7] ACPI: Make ACPI processor driver more extensible

[Ashwin Chaugule <ashwin.chaugule@linaro.org>]

Hi Rafael,

On 8 July 2015 at 09:34, Rafael J. Wysocki <rafael@kernel.org> wrote:
> Hi Ashwin,
>
> On Wed, Jul 8, 2015 at 3:27 AM, Ashwin Chaugule
> <ashwin.chaugule@linaro.org> wrote:
>> On 7 July 2015 at 21:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Monday, June 15, 2015 04:09:06 PM Ashwin Chaugule wrote:
>>>> The ACPI processor driver is currently tied too closely
>>>> to the ACPI C-states (CST), P-states (PSS) and other related
>>>> constructs for controlling CPU idle and CPU performance.
>>>>
>>>> The newer ACPI specification (v5.1 onwards) introduces
>>>> alternative methods to CST and PSS. These new mechanisms
>>>> are described within each ACPI Processor object and so they
>>>> need to be scanned whenever a new Processor object is detected.
>>>> This patch introduces two new Kconfig symbols to allow for
>>>> finer configurability among the various options for controlling
>>>> CPU idle and performance states. There is no change in functionality
>>>> and these options are defaulted to enabled to maintain previous
>>>> behaviour.
>>>>
>>>> The following patchwork introduces CPPC: A newer method of
>>>> controlling CPU performance. The OS is not expected to support CPPC
>>>> and PSS at runtime. So the kconfig option lets us make these two
>>>> mutually exclusive at compile time.
>>>>
>>>> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
>>>> Reviewed-by: Al Stone <al.stone@linaro.org>
>>>> ---
>>>>  drivers/acpi/Kconfig            |  31 +++++++++--
>>>>  drivers/acpi/Makefile           |   7 ++-
>>>>  drivers/acpi/processor_driver.c |  86 ++++++++++++++++++----------
>>>>  drivers/cpufreq/Kconfig         |   2 +-
>>>>  drivers/cpufreq/Kconfig.x86     |   2 +
>>>>  include/acpi/processor.h        | 120 ++++++++++++++++++++++++++++------------
>>>>  6 files changed, 176 insertions(+), 72 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>>> index ab2cbb5..5942754 100644
>>>> --- a/drivers/acpi/Kconfig
>>>> +++ b/drivers/acpi/Kconfig
>>>> @@ -166,18 +166,39 @@ config ACPI_DOCK
>>>>         This driver supports ACPI-controlled docking stations and removable
>>>>         drive bays such as the IBM Ultrabay and the Dell Module Bay.
>>>>
>>>> -config ACPI_PROCESSOR
>>>> -     tristate "Processor"
>>>> -     select THERMAL
>>>> -     select CPU_IDLE
>>>> +config ACPI_CST
>>>> +     bool "ACPI C states (CST) driver"
>>>> +     depends on ACPI_PROCESSOR
>>>>       depends on X86 || IA64
>>>> +     select CPU_IDLE
>>>>       default y
>>>>       help
>>>>         This driver installs ACPI as the idle handler for Linux and uses
>>>>         ACPI C2 and C3 processor states to save power on systems that
>>>> -       support it.  It is required by several flavors of cpufreq
>>>> +       support it.
>>>> +
>>>> +config ACPI_PSS
>>>> +     bool "ACPI P States (PSS) driver"
>>>> +     depends on ACPI_PROCESSOR
>>>> +     depends on X86 || IA64
>>>> +     select THERMAL
>>>> +     default y
>>>> +     help
>>>> +       This driver implements ACPI methods for controlling CPU performance
>>>> +       using PSS methods as described in the ACPI spec. It also enables support
>>>> +       for ACPI based performance throttling (TSS) and ACPI based thermal
>>>> +       monitoring. It is required by several flavors of cpufreq
>>>>         performance-state drivers.
>>>
>>> For starters, I don't like these new Kconfig options.
>>>
>>> Isn't there a way to implement what you need without adding them?
>>
>> We need to use the ACPI Processor driver for CPPC without including
>> all its current dependencies. (i.e. PSS, TSS, CSS etc.). The upcoming
>> LPI work from Sudeep will also face the same issue. I considered the
>> alternative of adding a probe routine which matches
>> ACPI_PROCESSOR_OBJECT/DEVICE_HID to each driver, but this seemed like
>> a better option. Do you have any other ideas?
>
> First of all, I don't see what _CST has to do with CPPC.
>
> Second, it looks like the problem is that x86 probably won't use CPPC,
> while arm64 probably won't use _PSS.
>
> That can be addressed by adding Kconfig options, but those options
> should not be user-selectable (because quite honestly users don't have
> to know what those things are and they don't care *and* things break
> if they make a wrong choice).  Instead, I'd make architecture Kconfigs
> select those options automatically.
>

I've made changes locally. Basically s/ACPI_PSS/HAVE_ACPI_PSS/ and in
arch/x86/Kconfig; select HAVE_ACPI_PSS if ACPI. Similarly for CST.
Just wanted to check if there were any other comments before sending
out v7.

Thanks,
Ashwin.