Re: [PATCH V3 18/20] RISC-V: Add ACPI initialization in setup_arch()

Conor Dooley
2023-03-06T21:17:56Z

[-- Attachment #1.1: Type: text/plain, Size: 3733 bytes --]

On Fri, Mar 03, 2023 at 07:06:45PM +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>
> ---

> +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;
> +
> + /*
> + * Revision in table header is the FADT Major revision, and there
> + * is a minor revision of FADT.

What is the point of this part of the comment? Isn't it obvious from the
below code that you expect a major and minor revision?
If feel like you're trying to make a point in it, but the point has been
lost :/

> + *
> + * 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;
> +}

> 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();

Thanks for removing the ifdeffery :)
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]