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

  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>
---
 .../admin-guide/kernel-parameters.txt         |   8 +-
 arch/riscv/kernel/acpi.c                      | 126 ++++++++++++++++++
 arch/riscv/kernel/setup.c                     |  25 ++--
 3 files changed, 147 insertions(+), 12 deletions(-)
  

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.
  

On Mon, Mar 06, 2023 at 09:17:34PM +0000, Conor Dooley wrote:
> 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 :/
> 
It just highlights that major and minor revision fields are in two
different places. Let me remove this comment since it is part of the
spec anyway.

Thanks,
Sunil
  

On Wed, Mar 08, 2023 at 03:12:18PM +0530, Sunil V L wrote:
> On Mon, Mar 06, 2023 at 09:17:34PM +0000, Conor Dooley wrote:
> > 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 :/
> > 
> It just highlights that major and minor revision fields are in two
> different places. 

I thought that that was what you meant, but only because the code does
it. The comment doesn't actually say so!

Instead of deleting it, something like the following?
/*
 * 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.
 * <snip>