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

[Rafael J. Wysocki]

On Tue, Feb 28, 2023 at 11:05 AM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote:
>
> 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().

That's correct.