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.
