* [PATCH 0/3] cmd: acpi: fix list_fadt()
@ 2023-12-15 16:40 Heinrich Schuchardt
2023-12-15 16:40 ` [PATCH 1/3] acpi: fix FADT table Heinrich Schuchardt
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Heinrich Schuchardt @ 2023-12-15 16:40 UTC (permalink / raw)
To: Simon Glass; +Cc: Bin Meng, Andy Shevchenko, u-boot, 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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] acpi: fix FADT table 2023-12-15 16:40 [PATCH 0/3] cmd: acpi: fix list_fadt() Heinrich Schuchardt @ 2023-12-15 16:40 ` Heinrich Schuchardt 2023-12-15 17:43 ` Andy Shevchenko 2023-12-15 17:48 ` Simon Glass 2023-12-15 16:40 ` [PATCH 2/3] cmd: acpi: fix listing DSDT and FACS Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list Heinrich Schuchardt 2 siblings, 2 replies; 9+ messages in thread From: Heinrich Schuchardt @ 2023-12-15 16:40 UTC (permalink / raw) To: Simon Glass; +Cc: Bin Meng, Andy Shevchenko, u-boot, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] acpi: fix FADT table 2023-12-15 16:40 ` [PATCH 1/3] acpi: fix FADT table Heinrich Schuchardt @ 2023-12-15 17:43 ` Andy Shevchenko 2023-12-15 17:48 ` Simon Glass 1 sibling, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2023-12-15 17:43 UTC (permalink / raw) To: Heinrich Schuchardt; +Cc: Simon Glass, Bin Meng, u-boot 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] acpi: fix FADT table 2023-12-15 16:40 ` [PATCH 1/3] acpi: fix FADT table Heinrich Schuchardt 2023-12-15 17:43 ` Andy Shevchenko @ 2023-12-15 17:48 ` Simon Glass 2023-12-15 19:04 ` Heinrich Schuchardt 1 sibling, 1 reply; 9+ messages in thread From: Simon Glass @ 2023-12-15 17:48 UTC (permalink / raw) To: Heinrich Schuchardt; +Cc: Bin Meng, Andy Shevchenko, u-boot 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] acpi: fix FADT table 2023-12-15 17:48 ` Simon Glass @ 2023-12-15 19:04 ` Heinrich Schuchardt 0 siblings, 0 replies; 9+ messages in thread From: Heinrich Schuchardt @ 2023-12-15 19:04 UTC (permalink / raw) To: Simon Glass; +Cc: Bin Meng, Andy Shevchenko, u-boot 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] cmd: acpi: fix listing DSDT and FACS 2023-12-15 16:40 [PATCH 0/3] cmd: acpi: fix list_fadt() Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 1/3] acpi: fix FADT table Heinrich Schuchardt @ 2023-12-15 16:40 ` Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list Heinrich Schuchardt 2 siblings, 0 replies; 9+ messages in thread From: Heinrich Schuchardt @ 2023-12-15 16:40 UTC (permalink / raw) To: Simon Glass; +Cc: Bin Meng, Andy Shevchenko, u-boot, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list 2023-12-15 16:40 [PATCH 0/3] cmd: acpi: fix list_fadt() Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 1/3] acpi: fix FADT table Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 2/3] cmd: acpi: fix listing DSDT and FACS Heinrich Schuchardt @ 2023-12-15 16:40 ` Heinrich Schuchardt 2023-12-15 17:46 ` Andy Shevchenko 2 siblings, 1 reply; 9+ messages in thread From: Heinrich Schuchardt @ 2023-12-15 16:40 UTC (permalink / raw) To: Simon Glass; +Cc: Bin Meng, Andy Shevchenko, u-boot, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list 2023-12-15 16:40 ` [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list Heinrich Schuchardt @ 2023-12-15 17:46 ` Andy Shevchenko 2023-12-15 19:22 ` Heinrich Schuchardt 0 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2023-12-15 17:46 UTC (permalink / raw) To: Heinrich Schuchardt; +Cc: Simon Glass, Bin Meng, u-boot 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list 2023-12-15 17:46 ` Andy Shevchenko @ 2023-12-15 19:22 ` Heinrich Schuchardt 0 siblings, 0 replies; 9+ messages in thread From: Heinrich Schuchardt @ 2023-12-15 19:22 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Simon Glass, Bin Meng, u-boot 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-12-15 19:22 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-15 16:40 [PATCH 0/3] cmd: acpi: fix list_fadt() Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 1/3] acpi: fix FADT table Heinrich Schuchardt 2023-12-15 17:43 ` Andy Shevchenko 2023-12-15 17:48 ` Simon Glass 2023-12-15 19:04 ` Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 2/3] cmd: acpi: fix listing DSDT and FACS Heinrich Schuchardt 2023-12-15 16:40 ` [PATCH 3/3] cmd: acpi: check HW reduced flag in acpi list Heinrich Schuchardt 2023-12-15 17:46 ` Andy Shevchenko 2023-12-15 19:22 ` Heinrich Schuchardt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.