Hi Heinrich, On Fri, 15 Dec 2023 at 09:40, Heinrich Schuchardt <heinrich.schucha...@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.schucha...@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