[PATCH 1/3] acpi: fix FADT table
Heinrich Schuchardt
heinrich.schuchardt at canonical.com
Fri Dec 15 20:04:58 CET 2023
On 12/15/23 18:48, Simon Glass wrote:
> Hi Heinrich,
>
> On Fri, 15 Dec 2023 at 09:40, Heinrich Schuchardt
> <heinrich.schuchardt at 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 at 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
More information about the U-Boot
mailing list