[PATCH v3 1/2] Reland "x86: Move FACP table into separate functions""
Each board has its own way of creating this table. Rather than calling the acpi_create_fadt() function for each one from a common acpi_write_fadt() function, just move the writer into the board-specific code.
Co-developed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Simon Glass sjg@chromium.org Tested-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- (Now fixed - thank you Andy!)
Make another attempt to land this. It apparently breaks Edison but I cannot test that board, nor can I work out what is wrong with the code.
Previous test report is here:
https://lore.kernel.org/all/Yga9Z7sBFAeV64FV@smile.fi.intel.com/
Looking at the image, the method is definitely present:
$ nm /tmp/b/edison/u-boot |grep acpi_writer 0115fde0 D _u_boot_list_2_acpi_writer_2_0base 0115fdf0 D _u_boot_list_2_acpi_writer_2_1facs 0115fe00 D _u_boot_list_2_acpi_writer_2_3dsdt 0115fe10 D _u_boot_list_2_acpi_writer_2_4gnvs 0115fe20 D _u_boot_list_2_acpi_writer_2_5csrt 0115fe30 D _u_boot_list_2_acpi_writer_2_5fadt 0115fe40 D _u_boot_list_2_acpi_writer_2_5mcfg 0115fe50 D _u_boot_list_2_acpi_writer_2_5spcr 0115fe60 D _u_boot_list_2_acpi_writer_2_5tcpa 0115fe70 D _u_boot_list_2_acpi_writer_2_5tpm2 0115fe80 D _u_boot_list_2_acpi_writer_2_5x86 0115fe90 D _u_boot_list_2_acpi_writer_2_6ssdt 0115fea0 D _u_boot_list_2_acpi_writer_2_8dev
I wonder if the code in quark_write_fadt() is not being called?
'acpi list' shows what tables are installed. On minnowmax there is no difference with or without this patch. Perhaps someone with sharper eyes than me can figure this out?
The mechanism is that the functions to be called are put in a linker list, each element being declared using ACPI_WRITER(function_name).
Then acpi_write_all() is called to write each one. Debugging could be added to that function, perhaps?
Changes in v3: - None (just added the tags since patchwork doesn't do Co-developed-by)
Changes in v2: - Squash in the fix from Andy Shevchenko
arch/x86/cpu/apollolake/acpi.c | 17 +++++++++++++---- arch/x86/cpu/baytrail/acpi.c | 27 +++++++++++++++++++-------- arch/x86/cpu/quark/acpi.c | 27 +++++++++++++++++++-------- arch/x86/cpu/tangier/acpi.c | 27 +++++++++++++++++++-------- arch/x86/include/asm/acpi_table.h | 2 -- arch/x86/lib/acpi_table.c | 15 --------------- 6 files changed, 70 insertions(+), 45 deletions(-)
diff --git a/arch/x86/cpu/apollolake/acpi.c b/arch/x86/cpu/apollolake/acpi.c index fd21c0b4968..16aaed7238a 100644 --- a/arch/x86/cpu/apollolake/acpi.c +++ b/arch/x86/cpu/apollolake/acpi.c @@ -146,16 +146,25 @@ void fill_fadt(struct acpi_fadt *fadt) fadt->x_pm_tmr_blk.addrl = IOMAP_ACPI_BASE + PM1_TMR; }
-void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, - void *dsdt) +static int apl_write_fadt(struct acpi_ctx *ctx, const struct acpi_writer *entry) { - struct acpi_table_header *header = &fadt->header; + struct acpi_table_header *header; + struct acpi_fadt *fadt;
- acpi_fadt_common(fadt, facs, dsdt); + fadt = ctx->current; + acpi_fadt_common(fadt, ctx->facs, ctx->dsdt); intel_acpi_fill_fadt(fadt); fill_fadt(fadt); + header = &fadt->header; header->checksum = table_compute_checksum(fadt, header->length); + + acpi_add_table(ctx, fadt); + + acpi_inc(ctx, sizeof(struct acpi_fadt)); + + return 0; } +ACPI_WRITER(5fadt, "FADT", apl_write_fadt, 0);
int apl_acpi_fill_dmar(struct acpi_ctx *ctx) { diff --git a/arch/x86/cpu/baytrail/acpi.c b/arch/x86/cpu/baytrail/acpi.c index 07757b88a30..4c526ff2731 100644 --- a/arch/x86/cpu/baytrail/acpi.c +++ b/arch/x86/cpu/baytrail/acpi.c @@ -15,20 +15,24 @@ #include <asm/arch/iomap.h> #include <dm/uclass-internal.h>
-void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, - void *dsdt) +static int baytrail_write_fadt(struct acpi_ctx *ctx, + const struct acpi_writer *entry) { - struct acpi_table_header *header = &(fadt->header); + struct acpi_table_header *header; + struct acpi_fadt *fadt; + + fadt = ctx->current; + header = &fadt->header; u16 pmbase = ACPI_BASE_ADDRESS;
- memset((void *)fadt, 0, sizeof(struct acpi_fadt)); + memset(fadt, '\0', sizeof(struct acpi_fadt));
acpi_fill_header(header, "FACP"); header->length = sizeof(struct acpi_fadt); header->revision = 4;
- fadt->firmware_ctrl = (u32)facs; - fadt->dsdt = (u32)dsdt; + 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; @@ -75,9 +79,9 @@ void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, fadt->reset_reg.addrh = 0; fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
- fadt->x_firmware_ctl_l = (u32)facs; + fadt->x_firmware_ctl_l = (u32)ctx->facs; fadt->x_firmware_ctl_h = 0; - fadt->x_dsdt_l = (u32)dsdt; + fadt->x_dsdt_l = (u32)ctx->dsdt; fadt->x_dsdt_h = 0;
fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO; @@ -137,7 +141,14 @@ void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, fadt->x_gpe1_blk.addrh = 0x0;
header->checksum = table_compute_checksum(fadt, header->length); + + acpi_add_table(ctx, fadt); + + acpi_inc(ctx, sizeof(struct acpi_fadt)); + + return 0; } +ACPI_WRITER(5fadt, "FADT", baytrail_write_fadt, 0);
int acpi_create_gnvs(struct acpi_global_nvs *gnvs) { diff --git a/arch/x86/cpu/quark/acpi.c b/arch/x86/cpu/quark/acpi.c index 82b776ff65f..92fa5bc30c3 100644 --- a/arch/x86/cpu/quark/acpi.c +++ b/arch/x86/cpu/quark/acpi.c @@ -10,20 +10,24 @@ #include <asm/arch/global_nvs.h> #include <asm/arch/iomap.h>
-void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, - void *dsdt) +static int quark_write_fadt(struct acpi_ctx *ctx, + const struct acpi_writer *entry) { - struct acpi_table_header *header = &(fadt->header); u16 pmbase = ACPI_PM1_BASE_ADDRESS; + struct acpi_table_header *header; + struct acpi_fadt *fadt;
- memset((void *)fadt, 0, sizeof(struct acpi_fadt)); + fadt = ctx->current; + header = &fadt->header; + + memset(fadt, '\0', sizeof(struct acpi_fadt));
acpi_fill_header(header, "FACP"); header->length = sizeof(struct acpi_fadt); header->revision = 4;
- fadt->firmware_ctrl = (u32)facs; - fadt->dsdt = (u32)dsdt; + 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; @@ -70,9 +74,9 @@ void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, fadt->reset_reg.addrh = 0; fadt->reset_value = SYS_RST | RST_CPU | FULL_RST;
- fadt->x_firmware_ctl_l = (u32)facs; + fadt->x_firmware_ctl_l = (u32)ctx->facs; fadt->x_firmware_ctl_h = 0; - fadt->x_dsdt_l = (u32)dsdt; + fadt->x_dsdt_l = (u32)ctx->dsdt; fadt->x_dsdt_h = 0;
fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO; @@ -132,7 +136,14 @@ void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, fadt->x_gpe1_blk.addrh = 0x0;
header->checksum = table_compute_checksum(fadt, header->length); + + acpi_add_table(ctx, fadt); + + acpi_inc(ctx, sizeof(struct acpi_fadt)); + + return 0; } +ACPI_WRITER(5fadt, "FADT", quark_write_fadt, 0);
int acpi_create_gnvs(struct acpi_global_nvs *gnvs) { diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c index 3ffba3897aa..ffaa56ab6f8 100644 --- a/arch/x86/cpu/tangier/acpi.c +++ b/arch/x86/cpu/tangier/acpi.c @@ -16,19 +16,23 @@ #include <asm/arch/iomap.h> #include <dm/uclass-internal.h>
-void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, - void *dsdt) +static int tangier_write_fadt(struct acpi_ctx *ctx, + const struct acpi_writer *entry) { - struct acpi_table_header *header = &(fadt->header); + struct acpi_table_header *header; + struct acpi_fadt *fadt;
- memset((void *)fadt, 0, sizeof(struct acpi_fadt)); + fadt = ctx->current; + header = &fadt->header; + + memset(fadt, '\0', sizeof(struct acpi_fadt));
acpi_fill_header(header, "FACP"); header->length = sizeof(struct acpi_fadt); header->revision = 6;
- fadt->firmware_ctrl = (u32)facs; - fadt->dsdt = (u32)dsdt; + 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 | @@ -41,13 +45,20 @@ void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs,
fadt->minor_revision = 2;
- fadt->x_firmware_ctl_l = (u32)facs; + fadt->x_firmware_ctl_l = (u32)ctx->facs; fadt->x_firmware_ctl_h = 0; - fadt->x_dsdt_l = (u32)dsdt; + fadt->x_dsdt_l = (u32)ctx->dsdt; fadt->x_dsdt_h = 0;
header->checksum = table_compute_checksum(fadt, header->length); + + acpi_add_table(ctx, fadt); + + acpi_inc(ctx, sizeof(struct acpi_fadt)); + + return 0; } +ACPI_WRITER(5fadt, "FADT", tangier_write_fadt, 0);
u32 acpi_fill_madt(u32 current) { diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h index 72e1873d15d..226753b65d6 100644 --- a/arch/x86/include/asm/acpi_table.h +++ b/arch/x86/include/asm/acpi_table.h @@ -24,8 +24,6 @@ struct acpi_table_header;
/* These can be used by the target port */
-void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs, - void *dsdt); int acpi_create_madt_lapics(u32 current); int acpi_create_madt_ioapic(struct acpi_madt_ioapic *ioapic, u8 id, u32 addr, u32 gsi_base); diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c index e3b7e9a4bbe..c5b33dc65de 100644 --- a/arch/x86/lib/acpi_table.c +++ b/arch/x86/lib/acpi_table.c @@ -458,21 +458,6 @@ int acpi_write_gnvs(struct acpi_ctx *ctx, const struct acpi_writer *entry) } ACPI_WRITER(4gnvs, "GNVS", acpi_write_gnvs, 0);
-static int acpi_write_fadt(struct acpi_ctx *ctx, - const struct acpi_writer *entry) -{ - struct acpi_fadt *fadt; - - fadt = ctx->current; - acpi_create_fadt(fadt, ctx->facs, ctx->dsdt); - acpi_add_table(ctx, fadt); - - acpi_inc(ctx, sizeof(struct acpi_fadt)); - - return 0; -} -ACPI_WRITER(5fact, "FADT", acpi_write_fadt, 0); - /** * acpi_write_hpet() - Write out a HPET table *
From: Andy Shevchenko andriy.shevchenko@linux.intel.com
Recent approach with FADT writer shows that there is a room for subtle errors. Prevent this from happening again by introducing acpi_add_fadt() helper.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
arch/x86/cpu/apollolake/acpi.c | 6 +----- arch/x86/cpu/baytrail/acpi.c | 6 +----- arch/x86/cpu/quark/acpi.c | 6 +----- arch/x86/cpu/tangier/acpi.c | 6 +----- include/acpi/acpi_table.h | 7 +++++++ 5 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/arch/x86/cpu/apollolake/acpi.c b/arch/x86/cpu/apollolake/acpi.c index 16aaed7238a..c610a7f4477 100644 --- a/arch/x86/cpu/apollolake/acpi.c +++ b/arch/x86/cpu/apollolake/acpi.c @@ -158,11 +158,7 @@ static int apl_write_fadt(struct acpi_ctx *ctx, const struct acpi_writer *entry) header = &fadt->header; header->checksum = table_compute_checksum(fadt, header->length);
- acpi_add_table(ctx, fadt); - - acpi_inc(ctx, sizeof(struct acpi_fadt)); - - return 0; + return acpi_add_fadt(ctx, fadt); } ACPI_WRITER(5fadt, "FADT", apl_write_fadt, 0);
diff --git a/arch/x86/cpu/baytrail/acpi.c b/arch/x86/cpu/baytrail/acpi.c index 4c526ff2731..4378846f8b0 100644 --- a/arch/x86/cpu/baytrail/acpi.c +++ b/arch/x86/cpu/baytrail/acpi.c @@ -142,11 +142,7 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx,
header->checksum = table_compute_checksum(fadt, header->length);
- acpi_add_table(ctx, fadt); - - acpi_inc(ctx, sizeof(struct acpi_fadt)); - - return 0; + return acpi_add_fadt(ctx, fadt); } ACPI_WRITER(5fadt, "FADT", baytrail_write_fadt, 0);
diff --git a/arch/x86/cpu/quark/acpi.c b/arch/x86/cpu/quark/acpi.c index 92fa5bc30c3..9a2d682451b 100644 --- a/arch/x86/cpu/quark/acpi.c +++ b/arch/x86/cpu/quark/acpi.c @@ -137,11 +137,7 @@ static int quark_write_fadt(struct acpi_ctx *ctx,
header->checksum = table_compute_checksum(fadt, header->length);
- acpi_add_table(ctx, fadt); - - acpi_inc(ctx, sizeof(struct acpi_fadt)); - - return 0; + return acpi_add_fadt(ctx, fadt); } ACPI_WRITER(5fadt, "FADT", quark_write_fadt, 0);
diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c index ffaa56ab6f8..1c667c7d569 100644 --- a/arch/x86/cpu/tangier/acpi.c +++ b/arch/x86/cpu/tangier/acpi.c @@ -52,11 +52,7 @@ static int tangier_write_fadt(struct acpi_ctx *ctx,
header->checksum = table_compute_checksum(fadt, header->length);
- acpi_add_table(ctx, fadt); - - acpi_inc(ctx, sizeof(struct acpi_fadt)); - - return 0; + return acpi_add_fadt(ctx, fadt); } ACPI_WRITER(5fadt, "FADT", tangier_write_fadt, 0);
diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h index 7ed0443c821..1f85de091d3 100644 --- a/include/acpi/acpi_table.h +++ b/include/acpi/acpi_table.h @@ -883,6 +883,13 @@ void acpi_inc_align(struct acpi_ctx *ctx, uint amount); */ int acpi_add_table(struct acpi_ctx *ctx, void *table);
+static inline int acpi_add_fadt(struct acpi_ctx *ctx, struct acpi_fadt *fadt) +{ + acpi_add_table(ctx, fadt); + acpi_inc(ctx, sizeof(struct acpi_fadt)); + return 0; +} + /** * acpi_write_rsdp() - Write out an RSDP indicating where the ACPI tables are *
On Sat, Sep 2, 2023 at 1:27 AM Simon Glass sjg@chromium.org wrote:
From: Andy Shevchenko andriy.shevchenko@linux.intel.com
Recent approach with FADT writer shows that there is a room for subtle errors. Prevent this from happening again by introducing acpi_add_fadt() helper.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
arch/x86/cpu/apollolake/acpi.c | 6 +----- arch/x86/cpu/baytrail/acpi.c | 6 +----- arch/x86/cpu/quark/acpi.c | 6 +----- arch/x86/cpu/tangier/acpi.c | 6 +----- include/acpi/acpi_table.h | 7 +++++++ 5 files changed, 11 insertions(+), 20 deletions(-)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On Sat, Sep 2, 2023 at 1:27 AM Simon Glass sjg@chromium.org wrote:
Each board has its own way of creating this table. Rather than calling the acpi_create_fadt() function for each one from a common acpi_write_fadt() function, just move the writer into the board-specific code.
Co-developed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Simon Glass sjg@chromium.org Tested-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
(Now fixed - thank you Andy!)
Make another attempt to land this. It apparently breaks Edison but I cannot test that board, nor can I work out what is wrong with the code.
Previous test report is here:
https://lore.kernel.org/all/Yga9Z7sBFAeV64FV@smile.fi.intel.com/
Looking at the image, the method is definitely present:
$ nm /tmp/b/edison/u-boot |grep acpi_writer 0115fde0 D _u_boot_list_2_acpi_writer_2_0base 0115fdf0 D _u_boot_list_2_acpi_writer_2_1facs 0115fe00 D _u_boot_list_2_acpi_writer_2_3dsdt 0115fe10 D _u_boot_list_2_acpi_writer_2_4gnvs 0115fe20 D _u_boot_list_2_acpi_writer_2_5csrt 0115fe30 D _u_boot_list_2_acpi_writer_2_5fadt 0115fe40 D _u_boot_list_2_acpi_writer_2_5mcfg 0115fe50 D _u_boot_list_2_acpi_writer_2_5spcr 0115fe60 D _u_boot_list_2_acpi_writer_2_5tcpa 0115fe70 D _u_boot_list_2_acpi_writer_2_5tpm2 0115fe80 D _u_boot_list_2_acpi_writer_2_5x86 0115fe90 D _u_boot_list_2_acpi_writer_2_6ssdt 0115fea0 D _u_boot_list_2_acpi_writer_2_8dev
I wonder if the code in quark_write_fadt() is not being called?
'acpi list' shows what tables are installed. On minnowmax there is no difference with or without this patch. Perhaps someone with sharper eyes than me can figure this out?
The mechanism is that the functions to be called are put in a linker list, each element being declared using ACPI_WRITER(function_name).
Then acpi_write_all() is called to write each one. Debugging could be added to that function, perhaps?
Changes in v3:
- None (just added the tags since patchwork doesn't do Co-developed-by)
Changes in v2:
- Squash in the fix from Andy Shevchenko
arch/x86/cpu/apollolake/acpi.c | 17 +++++++++++++---- arch/x86/cpu/baytrail/acpi.c | 27 +++++++++++++++++++-------- arch/x86/cpu/quark/acpi.c | 27 +++++++++++++++++++-------- arch/x86/cpu/tangier/acpi.c | 27 +++++++++++++++++++-------- arch/x86/include/asm/acpi_table.h | 2 -- arch/x86/lib/acpi_table.c | 15 --------------- 6 files changed, 70 insertions(+), 45 deletions(-)
series applied to u-boot-x86/next, thanks!
participants (2)
-
Bin Meng -
Simon Glass