Re: [PATCH v10 02/37] acpi: x86: Write FADT in common code

On 10/23/24 15:19, Patrick Rudolph wrote:

From: Maximilian Brune maximilian.brune@9elements.com

Write the FADT in common code since it's used on all architectures. Since the FADT is mandatory all SoCs or mainboards must implement the introduced function acpi_fill_fadt() and properly update the FADT.

Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-by: Simon Glass sjg@chromium.org Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com

This merged patch breaks

make qemu_riscv64_defconfig acpi.config make

You are implementing function acpi_fill_fadt() only for x86 and call it in code that is used by all architectures.

ACPI_WRITER(5fadt, "FADT", acpi_write_fadt, 0); makes no sense for CONFIG_QFW=y because QEMU is providing the table.

@Tom: After getting this fixed we should add a build with acpi.config into the CI.

further comments below:

---

Changelog v4:

• Drop __weak attribute

---

arch/sandbox/lib/acpi_table.c | 6 +++++ arch/x86/cpu/apollolake/acpi.c | 20 ++++------------ arch/x86/cpu/baytrail/acpi.c | 17 +------------- arch/x86/cpu/quark/acpi.c | 19 +--------------- arch/x86/cpu/tangier/acpi.c | 25 ++------------------ arch/x86/include/asm/acpi_table.h | 12 ---------- arch/x86/lib/acpi_table.c | 23 ------------------- include/acpi/acpi_table.h | 9 ++++++++ lib/acpi/acpi_table.c | 38 +++++++++++++++++++++++++++++++ 9 files changed, 61 insertions(+), 108 deletions(-) create mode 100644 arch/sandbox/lib/acpi_table.c

diff --git a/arch/sandbox/lib/acpi_table.c b/arch/sandbox/lib/acpi_table.c new file mode 100644 index 0000000000..10b7ce441a --- /dev/null +++ b/arch/sandbox/lib/acpi_table.c @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0+ +#include <acpi/acpi_table.h>

+void acpi_fill_fadt(struct acpi_fadt *fadt) +{ +} \ No newline at end of file diff --git a/arch/x86/cpu/apollolake/acpi.c b/arch/x86/cpu/apollolake/acpi.c index 76230aea83..93040e7bb3 100644 --- a/arch/x86/cpu/apollolake/acpi.c +++ b/arch/x86/cpu/apollolake/acpi.c @@ -128,8 +128,10 @@ int arch_madt_sci_irq_polarity(int sci) return MP_IRQ_POLARITY_LOW; }

-void fill_fadt(struct acpi_fadt *fadt) +void acpi_fill_fadt(struct acpi_fadt *fadt) {

• intel_acpi_fill_fadt(fadt);

• fadt->pm_tmr_blk = IOMAP_ACPI_BASE + PM1_TMR;

  fadt->p_lvl2_lat = ACPI_FADT_C2_NOT_SUPPORTED;

@@ -143,23 +145,9 @@ void fill_fadt(struct acpi_fadt *fadt) fadt->x_pm_tmr_blk.space_id = 1; fadt->x_pm_tmr_blk.bit_width = fadt->pm_tmr_len * 8; fadt->x_pm_tmr_blk.addrl = IOMAP_ACPI_BASE + PM1_TMR; -}

-static int apl_write_fadt(struct acpi_ctx *ctx, const struct acpi_writer *entry) -{

• struct acpi_table_header *header;

• struct acpi_fadt *fadt;

• 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);

• return acpi_add_fadt(ctx, fadt);

• fadt->preferred_pm_profile = ACPI_PM_MOBILE; }

-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 7821964f1f..7e1c2de3d3 100644 --- a/arch/x86/cpu/baytrail/acpi.c +++ b/arch/x86/cpu/baytrail/acpi.c @@ -15,20 +15,13 @@ #include <asm/arch/iomap.h> #include <dm/uclass-internal.h>

-static int baytrail_write_fadt(struct acpi_ctx *ctx,

• 	       const struct acpi_writer *entry)
  

+void acpi_fill_fadt(struct acpi_fadt *fadt) { struct acpi_table_header *header;

• struct acpi_fadt *fadt;

• fadt = ctx->current; header = &fadt->header; u16 pmbase = ACPI_BASE_ADDRESS;

• memset(fadt, '\0', sizeof(struct acpi_fadt));

• acpi_fill_header(header, "FACP");

• header->length = sizeof(struct acpi_fadt); header->revision = 4;

  fadt->preferred_pm_profile = ACPI_PM_MOBILE;

@@ -77,9 +70,6 @@ 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_ctrl = map_to_sysmem(ctx->facs);
• fadt->x_dsdt = map_to_sysmem(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; fadt->x_pm1a_evt_blk.bit_offset = 0;

@@ -135,12 +125,7 @@ static int baytrail_write_fadt(struct acpi_ctx *ctx, fadt->x_gpe1_blk.access_size = 0; fadt->x_gpe1_blk.addrl = 0x0; fadt->x_gpe1_blk.addrh = 0x0;

• header->checksum = table_compute_checksum(fadt, header->length);
• return acpi_add_fadt(ctx, fadt); }

-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 80e94600fc..0fe5f2bafb 100644 --- a/arch/x86/cpu/quark/acpi.c +++ b/arch/x86/cpu/quark/acpi.c @@ -11,23 +11,14 @@ #include <asm/arch/iomap.h> #include <linux/string.h>

-static int quark_write_fadt(struct acpi_ctx *ctx,

• 	    const struct acpi_writer *entry)
  

+void acpi_fill_fadt(struct acpi_fadt *fadt) { u16 pmbase = ACPI_PM1_BASE_ADDRESS; struct acpi_table_header *header;

• struct acpi_fadt *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->preferred_pm_profile = ACPI_PM_UNSPECIFIED; fadt->sci_int = 9; fadt->smi_cmd = 0; fadt->acpi_enable = 0;

@@ -73,9 +64,6 @@ 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_ctrl = map_to_sysmem(ctx->facs);
• fadt->x_dsdt = map_to_sysmem(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; fadt->x_pm1a_evt_blk.bit_offset = 0;

@@ -131,12 +119,7 @@ static int quark_write_fadt(struct acpi_ctx *ctx, fadt->x_gpe1_blk.access_size = 0; fadt->x_gpe1_blk.addrl = 0x0; fadt->x_gpe1_blk.addrh = 0x0;

• header->checksum = table_compute_checksum(fadt, header->length);
• return acpi_add_fadt(ctx, fadt); }

-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 d4d0ef6f85..1c73a9dbfe 100644 --- a/arch/x86/cpu/tangier/acpi.c +++ b/arch/x86/cpu/tangier/acpi.c @@ -16,21 +16,8 @@ #include <asm/arch/iomap.h> #include <dm/uclass-internal.h>

-static int tangier_write_fadt(struct acpi_ctx *ctx,

• 	      const struct acpi_writer *entry)
  

+void acpi_fill_fadt(struct acpi_fadt *fadt) {

• struct acpi_table_header *header;

• struct acpi_fadt *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->preferred_pm_profile = ACPI_PM_UNSPECIFIED;

  fadt->iapc_boot_arch = ACPI_FADT_VGA_NOT_PRESENT |

@@ -40,17 +27,9 @@ static int tangier_write_fadt(struct acpi_ctx *ctx, ACPI_FADT_POWER_BUTTON | ACPI_FADT_SLEEP_BUTTON | ACPI_FADT_SEALED_CASE | ACPI_FADT_HEADLESS | ACPI_FADT_HW_REDUCED_ACPI;

• fadt->header.revision = 6; fadt->minor_revision = 2;

• fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs);
• fadt->x_dsdt = map_to_sysmem(ctx->dsdt);
• header->checksum = table_compute_checksum(fadt, header->length);
• return acpi_add_fadt(ctx, fadt); }

-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 e617524617..3988898f66 100644 --- a/arch/x86/include/asm/acpi_table.h +++ b/arch/x86/include/asm/acpi_table.h @@ -168,18 +168,6 @@ int acpi_create_dmar_ds_ioapic(struct acpi_ctx *ctx, uint enumeration_id, int acpi_create_dmar_ds_msi_hpet(struct acpi_ctx *ctx, uint enumeration_id, pci_dev_t bdf);

-/**

• • acpi_fadt_common() - Handle common parts of filling out an FADT
• • This sets up the Fixed ACPI Description Table
• • @fadt: Pointer to place to put FADT
• • @facs: Pointer to the FACS
• • @dsdt: Pointer to the DSDT
• */

-void acpi_fadt_common(struct acpi_fadt *fadt, struct acpi_facs *facs,

•       void *dsdt);
  

• /**
  • intel_acpi_fill_fadt() - Set up the contents of the FADT

diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c index 08fd1e54ce..ff02ce80d1 100644 --- a/arch/x86/lib/acpi_table.c +++ b/arch/x86/lib/acpi_table.c @@ -381,29 +381,6 @@ int acpi_write_hpet(struct acpi_ctx *ctx) return 0; }

-void acpi_fadt_common(struct acpi_fadt *fadt, struct acpi_facs *facs,

•       void *dsdt)
  

-{

• struct acpi_table_header *header = &fadt->header;
• memset((void *)fadt, '\0', sizeof(struct acpi_fadt));
• acpi_fill_header(header, "FACP");
• header->length = sizeof(struct acpi_fadt);
• header->revision = 4;
• memcpy(header->oem_id, OEM_ID, 6);
• memcpy(header->oem_table_id, OEM_TABLE_ID, 8);
• memcpy(header->creator_id, ASLC_ID, 4);
• fadt->x_firmware_ctrl = map_to_sysmem(facs);
• fadt->x_dsdt = map_to_sysmem(dsdt);
• fadt->preferred_pm_profile = ACPI_PM_MOBILE;
• /* Use ACPI 3.0 revision */
• fadt->header.revision = 4;

-}

• void acpi_create_dmar_drhd(struct acpi_ctx *ctx, uint flags, uint segment, u64 bar) {

diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h index a372435492..19cf0fb43c 100644 --- a/include/acpi/acpi_table.h +++ b/include/acpi/acpi_table.h @@ -954,6 +954,15 @@ void acpi_fill_header(struct acpi_table_header *header, char *signature); */ int acpi_fill_csrt(struct acpi_ctx *ctx);

+/**

• • acpi_fill_fadt() - Fill out the body of the FADT
• • Must be implemented in SoC specific code or in mainboard code.
• • @fadt: Pointer to FADT to update
• */

+void acpi_fill_fadt(struct acpi_fadt *fadt);

• /**
  • acpi_get_rsdp_addr() - get ACPI RSDP table address

diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c index c9ddcca8cb..9eb0b507a0 100644 --- a/lib/acpi/acpi_table.c +++ b/lib/acpi/acpi_table.c @@ -202,6 +202,44 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table) return 0; }

+int acpi_write_fadt(struct acpi_ctx *ctx, const struct acpi_writer *entry) +{

• struct acpi_table_header *header;
• struct acpi_fadt *fadt;
• fadt = ctx->current;
• header = &fadt->header;
• memset((void *)fadt, '\0', sizeof(struct acpi_fadt));
• acpi_fill_header(header, "FACP");
• header->length = sizeof(struct acpi_fadt);
• header->revision = acpi_get_table_revision(ACPITAB_FADT);
• memcpy(header->oem_id, OEM_ID, 6);
• memcpy(header->oem_table_id, OEM_TABLE_ID, 8);
• memcpy(header->creator_id, ASLC_ID, 4);
• header->creator_revision = 1;
• fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs);
• fadt->x_dsdt = map_to_sysmem(ctx->dsdt);
• if (fadt->x_firmware_ctrl < 0x100000000ULL)

• fadt->firmware_ctrl = fadt->x_firmware_ctrl;
  

The ACPI spec requires fadt->firmware_ctrl to be ignored if fadt->x_firmware_ctrl is filled.

• if (fadt->x_dsdt < 0x100000000ULL)

• fadt->dsdt = fadt->x_dsdt;
  

The ACPI spec requires fadt->dsdt to be ignored if fadt->x_dsdt is filled.

Overwriting the 32-bit fields if the 64-bit fields are 0 is the wrong thing to do. If the 64-bit fields are non-zero it is superfluous.

Please, remove these lines.

Best regards

Heinrich

• fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
• acpi_fill_fadt(fadt);
• header->checksum = table_compute_checksum(fadt, header->length);
• return acpi_add_fadt(ctx, fadt);

+}

+ACPI_WRITER(5fadt, "FADT", acpi_write_fadt, 0);

• void acpi_create_dbg2(struct acpi_dbg2_header *dbg2, int port_type, int port_subtype, struct acpi_gen_regaddr *address, u32 address_size,