Re: [PATCH v2] ACPI: cpufreq: use a platform device to load ACPI PPC and PCC drivers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/22/23 16:04, Rafael J. Wysocki wrote:
> On Mon, Feb 20, 2023 at 3:32 PM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote:
>> Both acpi-cpufreq and pcc-cpufreq drivers have their platform firmware
>> interface defined by ACPI. Allowed performance states and parameters
>> must be same for each CPU.
> 
> This is not a requirement set by the ACPI specification, though, but
> the assumption made by the drivers in question AFAICS.  It would be
> good to clarify this here.

I can simplify this paragraph to:
Both acpi-cpufreq and pcc-cpufreq drivers use platform firmware controls
which are defined by ACPI. It is possible to treat these interfaces as
platform devices.

>> This makes it possible to model these
>> interfaces as platform devices.
>>
>> The patch extends the ACPI parsing logic to check the ACPI namespace if
>> the PPC or PCC interface is present and creates a virtual platform
>> device for each if it is available.
> 
> I'm not sure that this is the best approach.
> 
> The ACPI subsystem already walks the ACPI namespace twice when
> enumerating devices and CPUs.  In particular, acpi_processor_add() is
> invoked for each of them in the first on these walks, so it might as
> well take care of creating the requisite platform device if _PCT is
> present, can't it?

Makes sense, I see that acpi_processor_get_info() has some logic for handling
the first CPU so that looks to me as a good place to hook a check for _PCT.

>> The acpi-cpufreq and pcc-cpufreq
>> drivers are then updated to map to these devices.
>>
>> This allows to try loading acpi-cpufreq and pcc-cpufreq only once during
>> boot and only if a given interface is available in the firmware.
>>
>> Signed-off-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
>> ---
>>
>> Changes since v1 [1]:
>> - Describe the worst case scenario without the recent fix 0254127ab977e
>>   ("module: Don't wait for GOING modules") and refer to its discussion
>>   in the commit message.
>> - Consider ACPI processor device objects when looking for _PCT, in
>>   addition to processor objects.
>> - Add a few more comments explaining the code.
>>
>> [1] https://lore.kernel.org/lkml/20230131130041.629-1-petr.pavlu@xxxxxxxx/
>>
>>  drivers/acpi/Makefile          |  1 +
>>  drivers/acpi/acpi_cpufreq.c    | 77 ++++++++++++++++++++++++++++++++++
>>  drivers/acpi/bus.c             |  1 +
>>  drivers/acpi/internal.h        |  2 +
>>  drivers/cpufreq/acpi-cpufreq.c | 39 +++++++++--------
>>  drivers/cpufreq/pcc-cpufreq.c  | 34 ++++++++++-----
>>  6 files changed, 127 insertions(+), 27 deletions(-)
>>  create mode 100644 drivers/acpi/acpi_cpufreq.c
>>
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index feb36c0b9446..880db1082c3e 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -57,6 +57,7 @@ acpi-y                                += evged.o
>>  acpi-y                         += sysfs.o
>>  acpi-y                         += property.o
>>  acpi-$(CONFIG_X86)             += acpi_cmos_rtc.o
>> +acpi-$(CONFIG_X86)             += acpi_cpufreq.o
>>  acpi-$(CONFIG_X86)             += x86/apple.o
>>  acpi-$(CONFIG_X86)             += x86/utils.o
>>  acpi-$(CONFIG_X86)             += x86/s2idle.o
>> diff --git a/drivers/acpi/acpi_cpufreq.c b/drivers/acpi/acpi_cpufreq.c
>> new file mode 100644
>> index 000000000000..4e4ceb7cd226
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_cpufreq.c
>> @@ -0,0 +1,77 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Registration of platform devices for ACPI Processor Performance Control and
>> + * Processor Clocking Control.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <acpi/processor.h>
>> +
>> +#include "internal.h"
>> +
>> +static void __init cpufreq_add_device(const char *name)
>> +{
>> +       struct platform_device *pdev;
>> +
>> +       pdev = platform_device_register_simple(name, PLATFORM_DEVID_NONE, NULL,
>> +                                              0);
>> +       if (IS_ERR(pdev))
>> +               pr_err("%s device creation failed: %ld\n", name, PTR_ERR(pdev));
>> +}
>> +
>> +static acpi_status __init acpi_pct_match(acpi_handle handle, u32 level,
>> +                                        void *context, void **return_value)
>> +{
>> +       bool *pct = context;
>> +       acpi_status status;
>> +       acpi_object_type acpi_type;
>> +       struct acpi_device *acpi_dev;
>> +
>> +       static const struct acpi_device_id processor_device_ids[] = {
>> +               { ACPI_PROCESSOR_OBJECT_HID, 0 },
>> +               { ACPI_PROCESSOR_DEVICE_HID, 0 },
>> +               { "", 0 },
>> +       };
>> +
>> +       /* Skip nodes that cannot be a processor. */
>> +       status = acpi_get_type(handle, &acpi_type);
>> +       if (ACPI_FAILURE(status))
>> +               return status;
>> +       if (acpi_type != ACPI_TYPE_PROCESSOR && acpi_type != ACPI_TYPE_DEVICE)
>> +               return AE_OK;
>> +
>> +       /* Look at the set IDs if it is really a one. */
>> +       acpi_dev = acpi_fetch_acpi_dev(handle);
>> +       if (acpi_dev == NULL ||
>> +           acpi_match_device_ids(acpi_dev, processor_device_ids))
>> +               return AE_OK;
>> +
>> +       /* Check if it has _PCT and stop the walk as all CPUs must be same. */
>> +       *pct = acpi_has_method(handle, "_PCT");
>> +       return AE_CTRL_TERMINATE;
>> +}
>> +
>> +void __init acpi_cpufreq_init(void)
>> +{
>> +       bool pct = false;
>> +       acpi_status status;
>> +       acpi_handle handle;
>> +
>> +       /*
>> +        * Check availability of the PPC by looking at the presence of the _PCT
>> +        * object under the first processor definition.
>> +        */
>> +       acpi_walk_namespace(ACPI_TYPE_ANY, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
>> +                           acpi_pct_match, NULL, &pct, NULL);
>> +       if (pct)
>> +               cpufreq_add_device("acpi-cpufreq");
> 
> It should be possible to combine this with CPU enumeration as stated above.

Ack.

>> +
>> +       /* Check availability of the PCC by searching for \_SB.PCCH. */
>> +       status = acpi_get_handle(NULL, "\\_SB", &handle);
>> +       if (ACPI_FAILURE(status))
>> +               return;
>> +       if (acpi_has_method(handle, "PCCH"))
>> +               cpufreq_add_device("pcc-cpufreq");
> 
> And the remaining part can be called acpi_pcc_cpufreq_init().

Ok. I guess it then makes sense to move both PPC and PCC checks to
acpi_processor.c instead of adding a new file. Function
acpi_pcc_cpufreq_init() can be called from acpi_processor_init().

Thanks,
Petr



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux