On Thu, Oct 10, 2024 at 7:40 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Wed, Oct 9, 2024, at 19:40, Rafael J. Wysocki wrote:
> > On Mon, Oct 7, 2024 at 9:23 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index 61ca4afe83dc..132357815324 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -41,7 +41,7 @@ acpi-y += resource.o
> >> acpi-y += acpi_processor.o
> >> acpi-y += processor_core.o
> >> acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
> >> -acpi-y += ec.o
> >> +acpi-$(CONFIG_HAS_IOPORT) += ec.o
> >> acpi-$(CONFIG_ACPI_DOCK) += dock.o
> >> acpi-$(CONFIG_PCI) += pci_root.o pci_link.o pci_irq.o
> >> obj-$(CONFIG_ACPI_MCFG) += pci_mcfg.o
> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >> index 16917dc3ad60..535d6a72ce1b 100644
> >> --- a/drivers/acpi/bus.c
> >> +++ b/drivers/acpi/bus.c
> >> @@ -1356,7 +1356,8 @@ static int __init acpi_bus_init(void)
> >> * Do that before calling acpi_initialize_objects() which may trigger EC
> >> * address space accesses.
> >> */
> >> - acpi_ec_ecdt_probe();
> >> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> >> + acpi_ec_ecdt_probe();
> >>
> >> status = acpi_enable_subsystem(ACPI_NO_ACPI_ENABLE);
> >> if (ACPI_FAILURE(status)) {
> >> @@ -1391,7 +1392,8 @@ static int __init acpi_bus_init(void)
> >> * Maybe EC region is required at bus_scan/acpi_get_devices. So it
> >> * is necessary to enable it as early as possible.
> >> */
> >> - acpi_ec_dsdt_probe();
> >> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> >> + acpi_ec_dsdt_probe();
> >
> > The above two changes mean that it is not necessary to compile either
> > acpi_ec_ecdt_probe() or acpi_ec_dsdt_probe() at all if
> > CONFIG_HAS_IOPORT is not enabled.
>
> Correct, this is a result of removing ec.o from the list of objects.
>
> >> pr_info("Interpreter enabled\n");
> >>
> >> @@ -1464,7 +1466,8 @@ static int __init acpi_init(void)
> >> acpi_arm_init();
> >> acpi_riscv_init();
> >> acpi_scan_init();
> >> - acpi_ec_init();
> >> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> >> + acpi_ec_init();
> >
> > And this means that the whole EC driver is not going to work at all then.
>
> Correct. The way I read ec.c it makes no sense if acpi_ec_write_cmd()
> and acpi_ec_write_data() don't actually access the registers. Is
> there anything in ec.c that still makes sense without port I/O?
>
> >> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> >> index 4265814c74f8..8be453d89ef8 100644
> >> --- a/drivers/acpi/processor_perflib.c
> >> +++ b/drivers/acpi/processor_perflib.c
> >> @@ -455,7 +455,8 @@ int acpi_processor_pstate_control(void)
> >> {
> >> acpi_status status;
> >>
> >> - if (!acpi_gbl_FADT.smi_command || !acpi_gbl_FADT.pstate_control)
> >> + if (!IS_ENABLED(CONFIG_HAS_IOPORT) ||
> >> + !acpi_gbl_FADT.smi_command || !acpi_gbl_FADT.pstate_control)
> >> return 0;
> >
> > All of the existing callers of acpi_processor_pstate_control() are x86
> > which has CONFIG_HAS_IOPORT AFAICS.
> >
> > And if you care about the code size, acpi_processor_notify_smm() can
> > go away for !CONFIG_HAS_IOPORT too.
> >
> >> pr_debug("Writing pstate_control [0x%x] to smi_command [0x%x]\n",
> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> index 7ecc401fb97f..9d5e6dd542bf 100644
> >> --- a/drivers/acpi/scan.c
> >> +++ b/drivers/acpi/scan.c
> >> @@ -2293,7 +2293,8 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
> >> if (device->handler)
> >> goto ok;
> >>
> >> - acpi_ec_register_opregions(device);
> >> + if (IS_ENABLED(CONFIG_HAS_IOPORT))
> >> + acpi_ec_register_opregions(device);
> >
> > I'd rather have an empty stub of acpi_ec_register_opregions() for
> > !CONFIG_HAS_IOPORT instead.
>
> Fair enough. Should I add stubs for all the global EC functions then?
Yes, please.
> I also wonder if there should be a separate CONFIG_ACPI_EC symbol
> that allows the EC logic to be turned off on non-x86 architectures
> even when HAS_IOPORT is otherwise enabled.
Yes, CONFIG_ACPI_EC would make sense IMV.
Thanks!
