[PATCH 0/3] cmd: acpi: fix list_fadt()

[PATCH 0/3] cmd: acpi: fix list_fadt()

[Heinrich Schuchardt]

Fields X_FIRMWARE and X_DSDT in the FADT table must be 64bit.
Fix the definition in our include.

The 64bit fields X_FIRMWARE and X_DSDT take precedence over the respective
32bit fields.

Don't fill unused fields FIRMWAE and DSDT.

Write an error if the hardware reduce flag is not set for non-x86 systems.

Heinrich Schuchardt (3):
  acpi: fix FADT table
  cmd: acpi: fix listing DSDT and FACS
  cmd: acpi: check HW reduced flag in acpi list

 arch/x86/cpu/baytrail/acpi.c |  8 ++------
 arch/x86/cpu/quark/acpi.c    |  8 ++------
 arch/x86/cpu/tangier/acpi.c  |  8 ++------
 arch/x86/lib/acpi_table.c    |  9 ++-------
 cmd/acpi.c                   | 12 ++++++++++--
 include/acpi/acpi_table.h    |  6 ++----
 6 files changed, 20 insertions(+), 31 deletions(-)

-- 
2.40.1

[PATCH 1/3] acpi: fix FADT table

[Heinrich Schuchardt]

Fields X_FIRMWAE_CTRL and X_DSDT must be 64bit wide. Convert pointers to
to uintptr_t to fill these.

If field X_FIRMWARE_CTRL is filled, field FIRMWARE must be ignored. If
field X_DSDT is filled, field DSDT must be ignored. We should not fill
unused fields.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 arch/x86/cpu/baytrail/acpi.c | 8 ++------
 arch/x86/cpu/quark/acpi.c    | 8 ++------
 arch/x86/cpu/tangier/acpi.c  | 8 ++------
 arch/x86/lib/acpi_table.c    | 9 ++-------
 include/acpi/acpi_table.h    | 6 ++----
 5 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/arch/x86/cpu/baytrail/acpi.c b/arch/x86/cpu/baytrail/acpi.c
index 4378846f8b..5c28c4d260 100644
--- a/arch/x86/cpu/baytrail/acpi.c
+++ b/arch/x86/cpu/baytrail/acpi.c
@@ -31,8 +31,6 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx,
 	header->length = sizeof(struct acpi_fadt);
 	header->revision = 4;
 
-	fadt->firmware_ctrl = (u32)ctx->facs;
-	fadt->dsdt = (u32)ctx->dsdt;
 	fadt->preferred_pm_profile = ACPI_PM_MOBILE;
 	fadt->sci_int = 9;
 	fadt->smi_cmd = 0;
@@ -79,10 +77,8 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx,
 	fadt->reset_reg.addrh = 0;
 	fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
 
-	fadt->x_firmware_ctl_l = (u32)ctx->facs;
-	fadt->x_firmware_ctl_h = 0;
-	fadt->x_dsdt_l = (u32)ctx->dsdt;
-	fadt->x_dsdt_h = 0;
+	fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
+	fadt->x_dsdt = (uintptr_t)ctx->dsdt;
 
 	fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO;
 	fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
diff --git a/arch/x86/cpu/quark/acpi.c b/arch/x86/cpu/quark/acpi.c
index 9a2d682451..583d4583c0 100644
--- a/arch/x86/cpu/quark/acpi.c
+++ b/arch/x86/cpu/quark/acpi.c
@@ -26,8 +26,6 @@ static int quark_write_fadt(struct acpi_ctx *ctx,
 	header->length = sizeof(struct acpi_fadt);
 	header->revision = 4;
 
-	fadt->firmware_ctrl = (u32)ctx->facs;
-	fadt->dsdt = (u32)ctx->dsdt;
 	fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
 	fadt->sci_int = 9;
 	fadt->smi_cmd = 0;
@@ -74,10 +72,8 @@ static int quark_write_fadt(struct acpi_ctx *ctx,
 	fadt->reset_reg.addrh = 0;
 	fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
 
-	fadt->x_firmware_ctl_l = (u32)ctx->facs;
-	fadt->x_firmware_ctl_h = 0;
-	fadt->x_dsdt_l = (u32)ctx->dsdt;
-	fadt->x_dsdt_h = 0;
+	fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
+	fadt->x_dsdt = (uintptr_t)ctx->dsdt;
 
 	fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO;
 	fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
index 1c667c7d56..19ec6e3390 100644
--- a/arch/x86/cpu/tangier/acpi.c
+++ b/arch/x86/cpu/tangier/acpi.c
@@ -31,8 +31,6 @@ static int tangier_write_fadt(struct acpi_ctx *ctx,
 	header->length = sizeof(struct acpi_fadt);
 	header->revision = 6;
 
-	fadt->firmware_ctrl = (u32)ctx->facs;
-	fadt->dsdt = (u32)ctx->dsdt;
 	fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
 
 	fadt->iapc_boot_arch = ACPI_FADT_VGA_NOT_PRESENT |
@@ -45,10 +43,8 @@ static int tangier_write_fadt(struct acpi_ctx *ctx,
 
 	fadt->minor_revision = 2;
 
-	fadt->x_firmware_ctl_l = (u32)ctx->facs;
-	fadt->x_firmware_ctl_h = 0;
-	fadt->x_dsdt_l = (u32)ctx->dsdt;
-	fadt->x_dsdt_h = 0;
+	fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
+	fadt->x_dsdt = (uintptr_t)ctx->dsdt;
 
 	header->checksum = table_compute_checksum(fadt, header->length);
 
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index c5b33dc65d..f49b6933ab 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -572,13 +572,8 @@ void acpi_fadt_common(struct acpi_fadt *fadt, struct acpi_facs *facs,
 	memcpy(header->aslc_id, ASLC_ID, 4);
 	header->aslc_revision = 1;
 
-	fadt->firmware_ctrl = (unsigned long)facs;
-	fadt->dsdt = (unsigned long)dsdt;
-
-	fadt->x_firmware_ctl_l = (unsigned long)facs;
-	fadt->x_firmware_ctl_h = 0;
-	fadt->x_dsdt_l = (unsigned long)dsdt;
-	fadt->x_dsdt_h = 0;
+	fadt->x_firmware_ctrl = (uintptr_t)facs;
+	fadt->x_dsdt = (uintptr_t)dsdt;
 
 	fadt->preferred_pm_profile = ACPI_PM_MOBILE;
 
diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
index 20ac3b51ba..e67562ef65 100644
--- a/include/acpi/acpi_table.h
+++ b/include/acpi/acpi_table.h
@@ -228,10 +228,8 @@ struct __packed acpi_fadt {
 	u8 reset_value;
 	u16 arm_boot_arch;
 	u8 minor_revision;
-	u32 x_firmware_ctl_l;
-	u32 x_firmware_ctl_h;
-	u32 x_dsdt_l;
-	u32 x_dsdt_h;
+	u64 x_firmware_ctrl;
+	u64 x_dsdt;
 	struct acpi_gen_regaddr x_pm1a_evt_blk;
 	struct acpi_gen_regaddr x_pm1b_evt_blk;
 	struct acpi_gen_regaddr x_pm1a_cnt_blk;
-- 
2.40.1

[PATCH 2/3] cmd: acpi: fix listing DSDT and FACS

[Heinrich Schuchardt]

If field X_FIRMWARE_CTRL is filled, field FIRMWARE must be ignored. If
field X_DSDT is filled, field DSDT must be ignored.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 cmd/acpi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/cmd/acpi.c b/cmd/acpi.c
index 0c14409242..24910c150b 100644
--- a/cmd/acpi.c
+++ b/cmd/acpi.c
@@ -53,9 +53,13 @@ static int dump_table_name(const char *sig)
 
 static void list_fadt(struct acpi_fadt *fadt)
 {
-	if (fadt->dsdt)
+	if (fadt->x_dsdt)
+		dump_hdr(map_sysmem(fadt->x_dsdt, 0));
+	else if (fadt->dsdt)
 		dump_hdr(map_sysmem(fadt->dsdt, 0));
-	if (fadt->firmware_ctrl)
+	if (fadt->x_firmware_ctrl)
+		dump_hdr(map_sysmem(fadt->x_firmware_ctrl, 0));
+	else if (fadt->firmware_ctrl)
 		dump_hdr(map_sysmem(fadt->firmware_ctrl, 0));
 }
 
-- 
2.40.1

[PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list

[Heinrich Schuchardt]

On non x86 platforms the hardware reduce flag must be set in the FADT
table. Write an error message if the flag is missing.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 cmd/acpi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cmd/acpi.c b/cmd/acpi.c
index 24910c150b..2e9a333ffa 100644
--- a/cmd/acpi.c
+++ b/cmd/acpi.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <command.h>
 #include <display_options.h>
+#include <log.h>
 #include <mapmem.h>
 #include <acpi/acpi_table.h>
 #include <asm/acpi_table.h>
@@ -57,6 +58,9 @@ static void list_fadt(struct acpi_fadt *fadt)
 		dump_hdr(map_sysmem(fadt->x_dsdt, 0));
 	else if (fadt->dsdt)
 		dump_hdr(map_sysmem(fadt->dsdt, 0));
+	if (!IS_ENABLED(CONFIG_X86) &&
+	    !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
+		log_err("FADT not ACPI hardware reduced compliant\n");
 	if (fadt->x_firmware_ctrl)
 		dump_hdr(map_sysmem(fadt->x_firmware_ctrl, 0));
 	else if (fadt->firmware_ctrl)
-- 
2.40.1

Re: [PATCH 1/3] acpi: fix FADT table

[Andy Shevchenko]

On Fri, Dec 15, 2023 at 05:40:14PM +0100, Heinrich Schuchardt wrote:
> Fields X_FIRMWAE_CTRL and X_DSDT must be 64bit wide. Convert pointers to
> to uintptr_t to fill these.
> 
> If field X_FIRMWARE_CTRL is filled, field FIRMWARE must be ignored. If
> field X_DSDT is filled, field DSDT must be ignored. We should not fill
> unused fields.

Is it dictated by the specifications?
Please, put the reference here in the commit message (section X.Y.Z "Goo bar").

...


From code perspective LGTM.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list

[Andy Shevchenko]

On Fri, Dec 15, 2023 at 05:40:16PM +0100, Heinrich Schuchardt wrote:
> On non x86 platforms the hardware reduce flag must be set in the FADT
> table. Write an error message if the flag is missing.

...

> +	if (!IS_ENABLED(CONFIG_X86) &&
> +	    !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
> +		log_err("FADT not ACPI hardware reduced compliant\n");

I guess it's half baked solution as this, HW reduced, flag adds more
limitations even on x86. If you want a good validation it should be done
in a separate function taking others aspects into account.

But since it doesn't affect my area of interest in U-Boot, I won't prevent
you from doing this way, it's up to the U-Boot maintainers how to proceed
with it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] acpi: fix FADT table

[Simon Glass]

Hi Heinrich,

On Fri, 15 Dec 2023 at 09:40, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Fields X_FIRMWAE_CTRL and X_DSDT must be 64bit wide. Convert pointers to
> to uintptr_t to fill these.
>
> If field X_FIRMWARE_CTRL is filled, field FIRMWARE must be ignored. If
> field X_DSDT is filled, field DSDT must be ignored. We should not fill
> unused fields.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  arch/x86/cpu/baytrail/acpi.c | 8 ++------
>  arch/x86/cpu/quark/acpi.c    | 8 ++------
>  arch/x86/cpu/tangier/acpi.c  | 8 ++------
>  arch/x86/lib/acpi_table.c    | 9 ++-------
>  include/acpi/acpi_table.h    | 6 ++----
>  5 files changed, 10 insertions(+), 29 deletions(-)
>

Just as a general comment, the word 'fix' does not really describe
things very well. There may be many fixes to a piece of code or
feature. I would suggest something a bit more specific.  Perhaps
something like 'Support 64-bit addresses in FADT table'

Also please use the U-Boot standard of ulong for addresses, not uintptr_t

For casts, it is better to use map_sysmem() and map_to_sysmem() so
that we can use the code in unit tests (although of course we
currently have no way of running x86-specific code in sandbox).

> diff --git a/arch/x86/cpu/baytrail/acpi.c b/arch/x86/cpu/baytrail/acpi.c
> index 4378846f8b..5c28c4d260 100644
> --- a/arch/x86/cpu/baytrail/acpi.c
> +++ b/arch/x86/cpu/baytrail/acpi.c
> @@ -31,8 +31,6 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx,
>         header->length = sizeof(struct acpi_fadt);
>         header->revision = 4;
>
> -       fadt->firmware_ctrl = (u32)ctx->facs;
> -       fadt->dsdt = (u32)ctx->dsdt;
>         fadt->preferred_pm_profile = ACPI_PM_MOBILE;
>         fadt->sci_int = 9;
>         fadt->smi_cmd = 0;
> @@ -79,10 +77,8 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx,
>         fadt->reset_reg.addrh = 0;
>         fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
>
> -       fadt->x_firmware_ctl_l = (u32)ctx->facs;
> -       fadt->x_firmware_ctl_h = 0;
> -       fadt->x_dsdt_l = (u32)ctx->dsdt;
> -       fadt->x_dsdt_h = 0;
> +       fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
> +       fadt->x_dsdt = (uintptr_t)ctx->dsdt;
>
>         fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO;
>         fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
> diff --git a/arch/x86/cpu/quark/acpi.c b/arch/x86/cpu/quark/acpi.c
> index 9a2d682451..583d4583c0 100644
> --- a/arch/x86/cpu/quark/acpi.c
> +++ b/arch/x86/cpu/quark/acpi.c
> @@ -26,8 +26,6 @@ static int quark_write_fadt(struct acpi_ctx *ctx,
>         header->length = sizeof(struct acpi_fadt);
>         header->revision = 4;
>
> -       fadt->firmware_ctrl = (u32)ctx->facs;
> -       fadt->dsdt = (u32)ctx->dsdt;
>         fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
>         fadt->sci_int = 9;
>         fadt->smi_cmd = 0;
> @@ -74,10 +72,8 @@ static int quark_write_fadt(struct acpi_ctx *ctx,
>         fadt->reset_reg.addrh = 0;
>         fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
>
> -       fadt->x_firmware_ctl_l = (u32)ctx->facs;
> -       fadt->x_firmware_ctl_h = 0;
> -       fadt->x_dsdt_l = (u32)ctx->dsdt;
> -       fadt->x_dsdt_h = 0;
> +       fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
> +       fadt->x_dsdt = (uintptr_t)ctx->dsdt;
>
>         fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO;
>         fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
> diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
> index 1c667c7d56..19ec6e3390 100644
> --- a/arch/x86/cpu/tangier/acpi.c
> +++ b/arch/x86/cpu/tangier/acpi.c
> @@ -31,8 +31,6 @@ static int tangier_write_fadt(struct acpi_ctx *ctx,
>         header->length = sizeof(struct acpi_fadt);
>         header->revision = 6;
>
> -       fadt->firmware_ctrl = (u32)ctx->facs;
> -       fadt->dsdt = (u32)ctx->dsdt;
>         fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
>
>         fadt->iapc_boot_arch = ACPI_FADT_VGA_NOT_PRESENT |
> @@ -45,10 +43,8 @@ static int tangier_write_fadt(struct acpi_ctx *ctx,
>
>         fadt->minor_revision = 2;
>
> -       fadt->x_firmware_ctl_l = (u32)ctx->facs;
> -       fadt->x_firmware_ctl_h = 0;
> -       fadt->x_dsdt_l = (u32)ctx->dsdt;
> -       fadt->x_dsdt_h = 0;
> +       fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
> +       fadt->x_dsdt = (uintptr_t)ctx->dsdt;
>
>         header->checksum = table_compute_checksum(fadt, header->length);
>
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index c5b33dc65d..f49b6933ab 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -572,13 +572,8 @@ void acpi_fadt_common(struct acpi_fadt *fadt, struct acpi_facs *facs,
>         memcpy(header->aslc_id, ASLC_ID, 4);
>         header->aslc_revision = 1;
>
> -       fadt->firmware_ctrl = (unsigned long)facs;
> -       fadt->dsdt = (unsigned long)dsdt;
> -
> -       fadt->x_firmware_ctl_l = (unsigned long)facs;
> -       fadt->x_firmware_ctl_h = 0;
> -       fadt->x_dsdt_l = (unsigned long)dsdt;
> -       fadt->x_dsdt_h = 0;
> +       fadt->x_firmware_ctrl = (uintptr_t)facs;
> +       fadt->x_dsdt = (uintptr_t)dsdt;
>
>         fadt->preferred_pm_profile = ACPI_PM_MOBILE;
>
> diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
> index 20ac3b51ba..e67562ef65 100644
> --- a/include/acpi/acpi_table.h
> +++ b/include/acpi/acpi_table.h
> @@ -228,10 +228,8 @@ struct __packed acpi_fadt {
>         u8 reset_value;
>         u16 arm_boot_arch;
>         u8 minor_revision;
> -       u32 x_firmware_ctl_l;
> -       u32 x_firmware_ctl_h;
> -       u32 x_dsdt_l;
> -       u32 x_dsdt_h;
> +       u64 x_firmware_ctrl;
> +       u64 x_dsdt;
>         struct acpi_gen_regaddr x_pm1a_evt_blk;
>         struct acpi_gen_regaddr x_pm1b_evt_blk;
>         struct acpi_gen_regaddr x_pm1a_cnt_blk;
> --
> 2.40.1
>

Regards,
Simon

Regards,
Simon

Re: [PATCH 1/3] acpi: fix FADT table

[Heinrich Schuchardt]

On 12/15/23 18:48, Simon Glass wrote:
> Hi Heinrich,
> 
> On Fri, 15 Dec 2023 at 09:40, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Fields X_FIRMWAE_CTRL and X_DSDT must be 64bit wide. Convert pointers to
>> to uintptr_t to fill these.
>>
>> If field X_FIRMWARE_CTRL is filled, field FIRMWARE must be ignored. If
>> field X_DSDT is filled, field DSDT must be ignored. We should not fill
>> unused fields.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   arch/x86/cpu/baytrail/acpi.c | 8 ++------
>>   arch/x86/cpu/quark/acpi.c    | 8 ++------
>>   arch/x86/cpu/tangier/acpi.c  | 8 ++------
>>   arch/x86/lib/acpi_table.c    | 9 ++-------
>>   include/acpi/acpi_table.h    | 6 ++----
>>   5 files changed, 10 insertions(+), 29 deletions(-)
>>
> 
> Just as a general comment, the word 'fix' does not really describe
> things very well. There may be many fixes to a piece of code or
> feature. I would suggest something a bit more specific.  Perhaps
> something like ':'
> 
> Also please use the U-Boot standard of ulong for addresses, not uintptr_t

You can have pointers that are shorter or longer than long depending on 
compiler invocation parametes. uintptr_t is the only type guaranteed to 
take a pointer. Using unsigned long here would be a non-portable 
programming style and we should avoid such sin.

> 
> For casts, it is better to use map_sysmem() and map_to_sysmem() so
> that we can use the code in unit tests (although of course we
> currently have no way of running x86-specific code in sandbox).

Please, keep the sandbox buildable on other architectues.

Using map_to_sysmem() makes sense here as we use the same fields as 
virtual sandbox addresses.

Best regards

Heinrich

> 
>> diff --git a/arch/x86/cpu/baytrail/acpi.c b/arch/x86/cpu/baytrail/acpi.c
>> index 4378846f8b..5c28c4d260 100644
>> --- a/arch/x86/cpu/baytrail/acpi.c
>> +++ b/arch/x86/cpu/baytrail/acpi.c
>> @@ -31,8 +31,6 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx,
>>          header->length = sizeof(struct acpi_fadt);
>>          header->revision = 4;
>>
>> -       fadt->firmware_ctrl = (u32)ctx->facs;
>> -       fadt->dsdt = (u32)ctx->dsdt;
>>          fadt->preferred_pm_profile = ACPI_PM_MOBILE;
>>          fadt->sci_int = 9;
>>          fadt->smi_cmd = 0;
>> @@ -79,10 +77,8 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx,
>>          fadt->reset_reg.addrh = 0;
>>          fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
>>
>> -       fadt->x_firmware_ctl_l = (u32)ctx->facs;
>> -       fadt->x_firmware_ctl_h = 0;
>> -       fadt->x_dsdt_l = (u32)ctx->dsdt;
>> -       fadt->x_dsdt_h = 0;
>> +       fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
>> +       fadt->x_dsdt = (uintptr_t)ctx->dsdt;
>>
>>          fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO;
>>          fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
>> diff --git a/arch/x86/cpu/quark/acpi.c b/arch/x86/cpu/quark/acpi.c
>> index 9a2d682451..583d4583c0 100644
>> --- a/arch/x86/cpu/quark/acpi.c
>> +++ b/arch/x86/cpu/quark/acpi.c
>> @@ -26,8 +26,6 @@ static int quark_write_fadt(struct acpi_ctx *ctx,
>>          header->length = sizeof(struct acpi_fadt);
>>          header->revision = 4;
>>
>> -       fadt->firmware_ctrl = (u32)ctx->facs;
>> -       fadt->dsdt = (u32)ctx->dsdt;
>>          fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
>>          fadt->sci_int = 9;
>>          fadt->smi_cmd = 0;
>> @@ -74,10 +72,8 @@ static int quark_write_fadt(struct acpi_ctx *ctx,
>>          fadt->reset_reg.addrh = 0;
>>          fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
>>
>> -       fadt->x_firmware_ctl_l = (u32)ctx->facs;
>> -       fadt->x_firmware_ctl_h = 0;
>> -       fadt->x_dsdt_l = (u32)ctx->dsdt;
>> -       fadt->x_dsdt_h = 0;
>> +       fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
>> +       fadt->x_dsdt = (uintptr_t)ctx->dsdt;
>>
>>          fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO;
>>          fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
>> diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
>> index 1c667c7d56..19ec6e3390 100644
>> --- a/arch/x86/cpu/tangier/acpi.c
>> +++ b/arch/x86/cpu/tangier/acpi.c
>> @@ -31,8 +31,6 @@ static int tangier_write_fadt(struct acpi_ctx *ctx,
>>          header->length = sizeof(struct acpi_fadt);
>>          header->revision = 6;
>>
>> -       fadt->firmware_ctrl = (u32)ctx->facs;
>> -       fadt->dsdt = (u32)ctx->dsdt;
>>          fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
>>
>>          fadt->iapc_boot_arch = ACPI_FADT_VGA_NOT_PRESENT |
>> @@ -45,10 +43,8 @@ static int tangier_write_fadt(struct acpi_ctx *ctx,
>>
>>          fadt->minor_revision = 2;
>>
>> -       fadt->x_firmware_ctl_l = (u32)ctx->facs;
>> -       fadt->x_firmware_ctl_h = 0;
>> -       fadt->x_dsdt_l = (u32)ctx->dsdt;
>> -       fadt->x_dsdt_h = 0;
>> +       fadt->x_firmware_ctrl = (uintptr_t)ctx->facs;
>> +       fadt->x_dsdt = (uintptr_t)ctx->dsdt;
>>
>>          header->checksum = table_compute_checksum(fadt, header->length);
>>
>> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
>> index c5b33dc65d..f49b6933ab 100644
>> --- a/arch/x86/lib/acpi_table.c
>> +++ b/arch/x86/lib/acpi_table.c
>> @@ -572,13 +572,8 @@ void acpi_fadt_common(struct acpi_fadt *fadt, struct acpi_facs *facs,
>>          memcpy(header->aslc_id, ASLC_ID, 4);
>>          header->aslc_revision = 1;
>>
>> -       fadt->firmware_ctrl = (unsigned long)facs;
>> -       fadt->dsdt = (unsigned long)dsdt;
>> -
>> -       fadt->x_firmware_ctl_l = (unsigned long)facs;
>> -       fadt->x_firmware_ctl_h = 0;
>> -       fadt->x_dsdt_l = (unsigned long)dsdt;
>> -       fadt->x_dsdt_h = 0;
>> +       fadt->x_firmware_ctrl = (uintptr_t)facs;
>> +       fadt->x_dsdt = (uintptr_t)dsdt;
>>
>>          fadt->preferred_pm_profile = ACPI_PM_MOBILE;
>>
>> diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
>> index 20ac3b51ba..e67562ef65 100644
>> --- a/include/acpi/acpi_table.h
>> +++ b/include/acpi/acpi_table.h
>> @@ -228,10 +228,8 @@ struct __packed acpi_fadt {
>>          u8 reset_value;
>>          u16 arm_boot_arch;
>>          u8 minor_revision;
>> -       u32 x_firmware_ctl_l;
>> -       u32 x_firmware_ctl_h;
>> -       u32 x_dsdt_l;
>> -       u32 x_dsdt_h;
>> +       u64 x_firmware_ctrl;
>> +       u64 x_dsdt;
>>          struct acpi_gen_regaddr x_pm1a_evt_blk;
>>          struct acpi_gen_regaddr x_pm1b_evt_blk;
>>          struct acpi_gen_regaddr x_pm1a_cnt_blk;
>> --
>> 2.40.1
>>
> 
> Regards,
> Simon
> 
> Regards,
> Simon

Re: [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list

[Heinrich Schuchardt]

On 12/15/23 18:46, Andy Shevchenko wrote:
> On Fri, Dec 15, 2023 at 05:40:16PM +0100, Heinrich Schuchardt wrote:
>> On non x86 platforms the hardware reduce flag must be set in the FADT
>> table. Write an error message if the flag is missing.
> 
> ...
> 
>> +	if (!IS_ENABLED(CONFIG_X86) &&
>> +	    !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI))
>> +		log_err("FADT not ACPI hardware reduced compliant\n");
> 
> I guess it's half baked solution as this, HW reduced, flag adds more
> limitations even on x86. If you want a good validation it should be done
> in a separate function taking others aspects into account.
> 
> But since it doesn't affect my area of interest in U-Boot, I won't prevent
> you from doing this way, it's up to the U-Boot maintainers how to proceed
> with it.
> 

This flag is checked by Linux. It won't boot at all if the flag is not 
set on arm64 or riscv64. See arch/arm64/kernel/acpi.c and 
arch/riscv/kernel/acpi.c, function acpi_fadt_sanity_check().

Once you have booted you can use the Firmware Test Suite (FWTS) to check 
the rest. I don't plan rewriting such a validation in U-Boot.

Best regards

Heinrich