[PATCH 0/3] cmd: acpi: fix list_fadt()
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(-)
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;
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.
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
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
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)); }
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)
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.
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
participants (3)
-
Andy Shevchenko -
Heinrich Schuchardt -
Simon Glass