[PATCH v3 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.
v3: fix subject lines of patches 1 and 4 use map_to_sysmem for pointer to address conversion v2: check FADT table revision correct dumping DSDT and FACS
Heinrich Schuchardt (4): acpi: use 64-bit addresses in FADT table cmd: acpi: fix listing DSDT and FACS cmd: acpi: check HW reduced flag in acpi list acpi: support 64bit in acpi_find_table for DSDT and FACS
arch/x86/cpu/baytrail/acpi.c | 9 +++------ arch/x86/cpu/quark/acpi.c | 9 +++------ arch/x86/cpu/tangier/acpi.c | 9 +++------ 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, 47 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.
See the field definitions in chapter "5.2.9 Fixed ACPI Description Table (FADT)" of the ACPI Specification 6.5.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- arch/x86/cpu/baytrail/acpi.c | 9 +++------ arch/x86/cpu/quark/acpi.c | 9 +++------ arch/x86/cpu/tangier/acpi.c | 9 +++------ arch/x86/lib/acpi_table.c | 9 ++------- include/acpi/acpi_table.h | 6 ++---- 5 files changed, 13 insertions(+), 29 deletions(-)
diff --git a/arch/x86/cpu/baytrail/acpi.c b/arch/x86/cpu/baytrail/acpi.c index 4378846f8b..ccc4851b18 100644 --- a/arch/x86/cpu/baytrail/acpi.c +++ b/arch/x86/cpu/baytrail/acpi.c @@ -7,6 +7,7 @@ #include <cpu.h> #include <dm.h> #include <log.h> +#include <mapmem.h> #include <acpi/acpi_s3.h> #include <acpi/acpi_table.h> #include <asm/io.h> @@ -31,8 +32,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 +78,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 = map_to_sysmem(ctx->facs); + fadt->x_dsdt = map_to_sysmem(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..0e18ceab68 100644 --- a/arch/x86/cpu/quark/acpi.c +++ b/arch/x86/cpu/quark/acpi.c @@ -4,6 +4,7 @@ */
#include <common.h> +#include <mapmem.h> #include <acpi/acpi_table.h> #include <asm/processor.h> #include <asm/tables.h> @@ -26,8 +27,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 +73,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 = map_to_sysmem(ctx->facs); + fadt->x_dsdt = map_to_sysmem(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..1d37cc9e2b 100644 --- a/arch/x86/cpu/tangier/acpi.c +++ b/arch/x86/cpu/tangier/acpi.c @@ -8,6 +8,7 @@ #include <common.h> #include <cpu.h> #include <dm.h> +#include <mapmem.h> #include <acpi/acpi_table.h> #include <asm/ioapic.h> #include <asm/mpspec.h> @@ -31,8 +32,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 +44,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 = map_to_sysmem(ctx->facs); + fadt->x_dsdt = map_to_sysmem(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..ef738c02a6 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 = map_to_sysmem(facs); + fadt->x_dsdt = map_to_sysmem(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 Sat, 16 Dec 2023 at 01:12, 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.
See the field definitions in chapter "5.2.9 Fixed ACPI Description Table (FADT)" of the ACPI Specification 6.5.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
arch/x86/cpu/baytrail/acpi.c | 9 +++------ arch/x86/cpu/quark/acpi.c | 9 +++------ arch/x86/cpu/tangier/acpi.c | 9 +++------ arch/x86/lib/acpi_table.c | 9 ++------- include/acpi/acpi_table.h | 6 ++---- 5 files changed, 13 insertions(+), 29 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, 16 Dec 2023 at 01:12, 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.
See the field definitions in chapter "5.2.9 Fixed ACPI Description Table (FADT)" of the ACPI Specification 6.5.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
arch/x86/cpu/baytrail/acpi.c | 9 +++------ arch/x86/cpu/quark/acpi.c | 9 +++------ arch/x86/cpu/tangier/acpi.c | 9 +++------ arch/x86/lib/acpi_table.c | 9 ++------- include/acpi/acpi_table.h | 6 ++---- 5 files changed, 13 insertions(+), 29 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/next, thanks!
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..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 Sat, 16 Dec 2023 at 01:12, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
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(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, 16 Dec 2023 at 01:12, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
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(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/next, thanks!
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 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 Sat, 16 Dec 2023 at 01:12, 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
cmd/acpi.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, 16 Dec 2023 at 01:12, 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
cmd/acpi.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/next, thanks!
Use X_DSDT and X_FIRMWARE_CTRL if available.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- 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 Sat, 16 Dec 2023 at 01:12, 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
lib/acpi/acpi.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
This could really use a test
On 12/16/23 19:45, Simon Glass wrote:
On Sat, 16 Dec 2023 at 01:12, 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
lib/acpi/acpi.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
This could really use a test
I guess we want a test on library level separate from test/dm/acpi.c as this test will not be driver model related?
Best regards
Heinrich
Hi Heinrich,
On Sat, 16 Dec 2023 at 12:21, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 12/16/23 19:45, Simon Glass wrote:
On Sat, 16 Dec 2023 at 01:12, 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
lib/acpi/acpi.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
This could really use a test
I guess we want a test on library level separate from test/dm/acpi.c as this test will not be driver model related?
Yes, but it isn't that important. I have been moving DM tests to only differ in their flags, so I'm not sure there is much difference anymore.
Regards, Simon
Hi Heinrich,
On Sat, 16 Dec 2023 at 12:21, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 12/16/23 19:45, Simon Glass wrote:
On Sat, 16 Dec 2023 at 01:12, 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
lib/acpi/acpi.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
This could really use a test
I guess we want a test on library level separate from test/dm/acpi.c as this test will not be driver model related?
Yes, but it isn't that important. I have been moving DM tests to only differ in their flags, so I'm not sure there is much difference anymore.
Regards, Simon
Applied to u-boot-dm/next, thanks!
participants (2)
-
Heinrich Schuchardt -
Simon Glass