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

From: Petr Pavlu
Date: Tue Feb 28 2023 - 05:05:26 EST

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