| Message ID | 20231215164016.137794-2-heinrich.schuchardt@canonical.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | cmd: acpi: fix list_fadt() | expand |
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
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;
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(-)