[PATCH v10 02/37] acpi: x86: Write FADT in common code
Patrick Rudolph
patrick.rudolph at 9elements.com
Fri Nov 8 08:10:05 CET 2024
Hi Heinrich,
On Thu, Nov 7, 2024 at 10:19 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 10/23/24 15:19, Patrick Rudolph wrote:
> > From: Maximilian Brune <maximilian.brune at 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 at 9elements.com>
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Bin Meng <bmeng.cn at gmail.com>
>
> This merged patch breaks
>
> make qemu_riscv64_defconfig acpi.config
> make
I'm sorry, since the CI doesn't build that target, I was not aware of
build failures.
>
> You are implementing function acpi_fill_fadt() only for x86 and call it
> in code that is used by all architectures.
>
acpi_fill_fadt() is called on all architectures that are ACPI enabled
and build by the CI.
There might additional ACPI enabled platforms, but since they are not
build tested,
they aren't officially supported, are they?
> ACPI_WRITER(5fadt, "FADT", acpi_write_fadt, 0);
> makes no sense for CONFIG_QFW=y because QEMU is providing the table.
Good catch. I'll send a follow up series.
>
> @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.
>
This quote is out of context. The ACPI 2.0 spec requires the OS to
ignore DSDT, when X_DSDT is filled.
U-Boot doesn't know which version of the ACPI spec is used by the OS,
thus it must support all versions.
> 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.
Since the initial content of DSDT is 0, it's the right thing to do in
order to support ACPI 1.0 OS.
When the OS is ACPI 2.0 compliant it will ignore the DSDT and use
X_DSDT. Does map_to_sysmem() ever
return NULL? It sounds like a bug to me that isn't going to work,
regardless how the pointer is passed to the OS.
>
> 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,
>
More information about the U-Boot
mailing list