| Message ID | 20230404182037.863533-20-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add basic ACPI support for RISC-V | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes or riscv/for-next |
On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote: > Initialize the ACPI core for RISC-V during boot. > > ACPI tables and interpreter are initialized based on > the information passed from the firmware and the value of > the kernel parameter 'acpi'. > > With ACPI support added for RISC-V, the kernel parameter 'acpi' > is also supported on RISC-V. Hence, update the documentation. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > Acked-by: Conor Dooley <conor.dooley@microchip.com> > + /* Parse the ACPI tables for possible boot-time configuration */ > + acpi_boot_table_init(); > + if (acpi_disabled) { > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) { > + unflatten_and_copy_device_tree(); > + } else { > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > + unflatten_device_tree(); > + else > + pr_err("No DTB found in kernel mappings\n"); > + } > + } else { > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))); I'm probably forgetting something, but this seems very non-obvious to me: Why are you running early_init_dt_verify() when ACPI is enabled? I think that one deserves a comment so that next time someone looks at this (that doesn't live in ACPI land) they've know exactly why this is like it is. Doubly so since this is likely to change with some of Alex's bits moving the dtb back into the fixmap. Cheers, Conor.
On Tue, Apr 04, 2023 at 10:38:56PM +0100, Conor Dooley wrote: > On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote: > > Initialize the ACPI core for RISC-V during boot. > > > > ACPI tables and interpreter are initialized based on > > the information passed from the firmware and the value of > > the kernel parameter 'acpi'. > > > > With ACPI support added for RISC-V, the kernel parameter 'acpi' > > is also supported on RISC-V. Hence, update the documentation. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > > > + /* Parse the ACPI tables for possible boot-time configuration */ > > + acpi_boot_table_init(); > > + if (acpi_disabled) { > > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) { > > + unflatten_and_copy_device_tree(); > > + } else { > > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > > + unflatten_device_tree(); > > + else > > + pr_err("No DTB found in kernel mappings\n"); > > + } > > + } else { > > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))); > > I'm probably forgetting something, but this seems very non-obvious to > me: > Why are you running early_init_dt_verify() when ACPI is enabled? > I think that one deserves a comment so that next time someone looks at > this (that doesn't live in ACPI land) they've know exactly why this is > like it is. > > Doubly so since this is likely to change with some of Alex's bits moving > the dtb back into the fixmap. > Good question. The kernel creates a tiny DTB even when the FW didn't pass the FDT (ACPI systems). Please see update_fdt(). So, parse_dtb() would have set initial_boot_params to early VA and if we don't call early_init_dt_verify() again with __va, it panics since initial_boot_params can not be translated. Thanks, Sunil
On Wed, Apr 05, 2023 at 08:41:54PM +0530, Sunil V L wrote: > On Tue, Apr 04, 2023 at 10:38:56PM +0100, Conor Dooley wrote: > > On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote: > > > Initialize the ACPI core for RISC-V during boot. > > > > > > ACPI tables and interpreter are initialized based on > > > the information passed from the firmware and the value of > > > the kernel parameter 'acpi'. > > > > > > With ACPI support added for RISC-V, the kernel parameter 'acpi' > > > is also supported on RISC-V. Hence, update the documentation. > > > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > > > > > + /* Parse the ACPI tables for possible boot-time configuration */ > > > + acpi_boot_table_init(); > > > + if (acpi_disabled) { > > > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) { > > > + unflatten_and_copy_device_tree(); > > > + } else { > > > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) > > > + unflatten_device_tree(); > > > + else > > > + pr_err("No DTB found in kernel mappings\n"); > > > + } > > > + } else { > > > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))); > > > > I'm probably forgetting something, but this seems very non-obvious to > > me: > > Why are you running early_init_dt_verify() when ACPI is enabled? > > I think that one deserves a comment so that next time someone looks at > > this (that doesn't live in ACPI land) they've know exactly why this is > > like it is. > > > > Doubly so since this is likely to change with some of Alex's bits moving > > the dtb back into the fixmap. > > > Good question. The kernel creates a tiny DTB even when the FW didn't > pass the FDT (ACPI systems). Please see update_fdt(). Can you add a comment about this either on-location or in the commit message please? I think this counts as non-obvious behaviour. At least to me it does! Cheers, Conor.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 6221a1d057dd..047679554453 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1,17 +1,17 @@ - acpi= [HW,ACPI,X86,ARM64] + acpi= [HW,ACPI,X86,ARM64,RISCV64] Advanced Configuration and Power Interface Format: { force | on | off | strict | noirq | rsdt | copy_dsdt } force -- enable ACPI if default was off - on -- enable ACPI but allow fallback to DT [arm64] + on -- enable ACPI but allow fallback to DT [arm64,riscv64] off -- disable ACPI if default was on noirq -- do not use ACPI for IRQ routing strict -- Be less tolerant of platforms that are not strictly ACPI specification compliant. rsdt -- prefer RSDT over (default) XSDT copy_dsdt -- copy DSDT to memory - For ARM64, ONLY "acpi=off", "acpi=on" or "acpi=force" - are available + For ARM64 and RISCV64, ONLY "acpi=off", "acpi=on" or + "acpi=force" are available See also Documentation/power/runtime_pm.rst, pci=noacpi diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c index 40ab55309c70..890c30fb3dbe 100644 --- a/arch/riscv/kernel/acpi.c +++ b/arch/riscv/kernel/acpi.c @@ -16,6 +16,7 @@ #include <linux/acpi.h> #include <linux/io.h> #include <linux/pci.h> +#include <linux/efi.h> int acpi_noirq = 1; /* skip ACPI IRQ initialization */ int acpi_disabled = 1; @@ -25,6 +26,131 @@ int acpi_pci_disabled = 1; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled); static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS]; +static bool param_acpi_off __initdata; +static bool param_acpi_on __initdata; +static bool param_acpi_force __initdata; + +static int __init parse_acpi(char *arg) +{ + if (!arg) + return -EINVAL; + + /* "acpi=off" disables both ACPI table parsing and interpreter */ + if (strcmp(arg, "off") == 0) + param_acpi_off = true; + else if (strcmp(arg, "on") == 0) /* prefer ACPI over DT */ + param_acpi_on = true; + else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */ + param_acpi_force = true; + else + return -EINVAL; /* Core will print when we return error */ + + return 0; +} +early_param("acpi", parse_acpi); + +/* + * acpi_fadt_sanity_check() - Check FADT presence and carry out sanity + * checks on it + * + * Return 0 on success, <0 on failure + */ +static int __init acpi_fadt_sanity_check(void) +{ + struct acpi_table_header *table; + struct acpi_table_fadt *fadt; + acpi_status status; + int ret = 0; + + /* + * FADT is required on riscv; retrieve it to check its presence + * and carry out revision and ACPI HW reduced compliancy tests + */ + status = acpi_get_table(ACPI_SIG_FADT, 0, &table); + if (ACPI_FAILURE(status)) { + const char *msg = acpi_format_exception(status); + + pr_err("Failed to get FADT table, %s\n", msg); + return -ENODEV; + } + + fadt = (struct acpi_table_fadt *)table; + + /* + * The revision in the table header is the FADT's Major revision. The + * FADT also has a minor revision, which is stored in the FADT itself. + * + * TODO: Currently, we check for 6.5 as the minimum version to check + * for HW_REDUCED flag. However, once RISC-V updates are released in + * the ACPI spec, we need to update this check for exact minor revision + */ + if (table->revision < 6 || (table->revision == 6 && fadt->minor_revision < 5)) { + pr_err(FW_BUG "Unsupported FADT revision %d.%d, should be 6.5+\n", + table->revision, fadt->minor_revision); + } + + if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) { + pr_err("FADT not ACPI hardware reduced compliant\n"); + ret = -EINVAL; + } + + /* + * acpi_get_table() creates FADT table mapping that + * should be released after parsing and before resuming boot + */ + acpi_put_table(table); + return ret; +} + +/* + * acpi_boot_table_init() called from setup_arch(), always. + * 1. find RSDP and get its address, and then find XSDT + * 2. extract all tables and checksums them all + * 3. check ACPI FADT HW reduced flag + * + * We can parse ACPI boot-time tables such as MADT after + * this function is called. + * + * On return ACPI is enabled if either: + * + * - ACPI tables are initialized and sanity checks passed + * - acpi=force was passed in the command line and ACPI was not disabled + * explicitly through acpi=off command line parameter + * + * ACPI is disabled on function return otherwise + */ +void __init acpi_boot_table_init(void) +{ + /* + * Enable ACPI instead of device tree unless + * - ACPI has been disabled explicitly (acpi=off), or + * - firmware has not populated ACPI ptr in EFI system table + * and ACPI has not been [force] enabled (acpi=on|force) + */ + if (param_acpi_off || + (!param_acpi_on && !param_acpi_force && + efi.acpi20 == EFI_INVALID_TABLE_ADDR)) + return; + + /* + * ACPI is disabled at this point. Enable it in order to parse + * the ACPI tables and carry out sanity checks + */ + enable_acpi(); + + /* + * If ACPI tables are initialized and FADT sanity checks passed, + * leave ACPI enabled and carry on booting; otherwise disable ACPI + * on initialization error. + * If acpi=force was passed on the command line it forces ACPI + * to be enabled even if its initialization failed. + */ + if (acpi_table_init() || acpi_fadt_sanity_check()) { + pr_err("Failed to init ACPI tables\n"); + if (!param_acpi_force) + disable_acpi(); + } +} static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end) { diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index 2d45a416d283..7b2b065a9f70 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -8,6 +8,7 @@ * Nick Kossifidis <mick@ics.forth.gr> */ +#include <linux/acpi.h> #include <linux/init.h> #include <linux/mm.h> #include <linux/memblock.h> @@ -276,14 +277,22 @@ void __init setup_arch(char **cmdline_p) efi_init(); paging_init(); -#if IS_ENABLED(CONFIG_BUILTIN_DTB) - unflatten_and_copy_device_tree(); -#else - if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) - unflatten_device_tree(); - else - pr_err("No DTB found in kernel mappings\n"); -#endif + + /* Parse the ACPI tables for possible boot-time configuration */ + acpi_boot_table_init(); + if (acpi_disabled) { + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) { + unflatten_and_copy_device_tree(); + } else { + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)))) + unflatten_device_tree(); + else + pr_err("No DTB found in kernel mappings\n"); + } + } else { + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))); + } + early_init_fdt_scan_reserved_mem(); misc_mem_init();