[PATCH v2 0/4] cmd: acpi: correct handling of DSDT and FACS
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. Consider this in the 'acpi list' and 'acpi dump' commands. The fields only exist for FADT table revision 3 and above.
Don't fill unused fields FIRMWAE and DSDT.
Write an error if the hardware reduce flag is not set for non-x86 systems.
v2: check FADT table revision correct dumping DSDT and FACS
Heinrich Schuchardt (4): acpi: fix FADT table cmd: acpi: fix listing DSDT and FACS cmd: acpi: check HW reduced flag in acpi list aci: fix acpi_find_table() for DSDT and FACS
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 ++---- lib/acpi/acpi.c | 29 ++++++++++++++++++++++++----- 7 files changed, 44 insertions(+), 36 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 --- v2: no change --- 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 07:26:55PM +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.
...
v2: no change
Have you seen my email?
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 --- v2: check FADT table revision --- cmd/acpi.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/cmd/acpi.c b/cmd/acpi.c index 0c14409242..8d9eaf36c9 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->header.revision >= 3 && 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->header.revision >= 3 && 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 --- v2: no change --- cmd/acpi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/cmd/acpi.c b/cmd/acpi.c index 8d9eaf36c9..f6e02ea7a5 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->header.revision >= 3 && fadt->x_firmware_ctrl) dump_hdr(map_sysmem(fadt->x_firmware_ctrl, 0)); else if (fadt->firmware_ctrl)
On Fri, 15 Dec 2023 at 11:27, Heinrich Schuchardt heinrich.schuchardt@canonical.com 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.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
cmd/acpi.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
This was when the mistake (of using ACPI on ARM) was made...
Use X_DSDT and X_FIRMWARE_CTRL if available.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: new patch --- lib/acpi/acpi.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/lib/acpi/acpi.c b/lib/acpi/acpi.c index f21e509461..f80b2176e1 100644 --- a/lib/acpi/acpi.c +++ b/lib/acpi/acpi.c @@ -45,11 +45,30 @@ struct acpi_table_header *acpi_find_table(const char *sig) if (!memcmp(hdr->signature, "FACP", ACPI_NAME_LEN)) { struct acpi_fadt *fadt = (struct acpi_fadt *)hdr;
- if (!memcmp(sig, "DSDT", ACPI_NAME_LEN) && fadt->dsdt) - return map_sysmem(fadt->dsdt, 0); - if (!memcmp(sig, "FACS", ACPI_NAME_LEN) && - fadt->firmware_ctrl) - return map_sysmem(fadt->firmware_ctrl, 0); + if (!memcmp(sig, "DSDT", ACPI_NAME_LEN)) { + void *dsdt; + + if (fadt->header.revision >= 3 && fadt->x_dsdt) + dsdt = map_sysmem(fadt->x_dsdt, 0); + else if (fadt->dsdt) + dsdt = map_sysmem(fadt->dsdt, 0); + else + dsdt = NULL; + return dsdt; + } + + if (!memcmp(sig, "FACS", ACPI_NAME_LEN)) { + void *facs; + + if (fadt->header.revision >= 3 && + fadt->x_firmware_ctrl) + facs = map_sysmem(fadt->x_firmware_ctrl, 0); + else if (fadt->firmware_ctrl) + facs = map_sysmem(fadt->firmware_ctrl, 0); + else + facs = NULL; + return facs; + } } }
On Fri, 15 Dec 2023 at 11:27, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Use X_DSDT and X_FIRMWARE_CTRL if available.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: new patch
lib/acpi/acpi.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Could use a test.
participants (3)
-
Andy Shevchenko -
Heinrich Schuchardt -
Simon Glass