[PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Simon Glass]

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.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
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?

 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       | 25 +++++++++++++++++--------
 arch/x86/include/asm/acpi_table.h |  2 --
 arch/x86/lib/acpi_table.c         | 15 ---------------
 6 files changed, 68 insertions(+), 45 deletions(-)

diff --git a/arch/x86/cpu/apollolake/acpi.c b/arch/x86/cpu/apollolake/acpi.c
index fd21c0b49684..16aaed7238ab 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 07757b88a305..4c526ff27310 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 82b776ff65f6..92fa5bc30c36 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 3ffba3897aad..01b30553818c 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,18 @@ 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_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 72e1873d15dc..226753b65d6a 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 e3b7e9a4bbe8..c5b33dc65de4 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
  *
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 2/2] RFC: x86: acpi: Add some debugging

[Simon Glass]

Enable some debugging to help see what is going on.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/lib/tables.c  | 1 +
 lib/acpi/acpi_writer.c | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/lib/tables.c b/arch/x86/lib/tables.c
index 67bc0a72aebc..7c6b32e6e335 100644
--- a/arch/x86/lib/tables.c
+++ b/arch/x86/lib/tables.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2015, Bin Meng <bmeng.cn@gmail.com>
  */
 
+#define LOG_DEBUG
 #define LOG_CATEGORY LOGC_ACPI
 
 #include <common.h>
diff --git a/lib/acpi/acpi_writer.c b/lib/acpi/acpi_writer.c
index 946f90e8e7b1..b1118b602069 100644
--- a/lib/acpi/acpi_writer.c
+++ b/lib/acpi/acpi_writer.c
@@ -5,6 +5,7 @@
  * Copyright 2021 Google LLC
  */
 
+#define LOG_DEBUG
 #define LOG_CATEGORY LOGC_ACPI
 
 #include <common.h>
@@ -21,8 +22,8 @@ int acpi_write_one(struct acpi_ctx *ctx, const struct acpi_writer *entry)
 {
 	int ret;
 
-	log_debug("%s: writing table '%s'\n", entry->name,
-		  entry->table);
+	log_debug("%s: writing table '%s' at %p\n", entry->name,
+		  entry->table, ctx->current);
 	ctx->tab_start = ctx->current;
 	ret = entry->h_write(ctx, entry);
 	if (ret == -ENOENT) {
@@ -57,11 +58,15 @@ static int acpi_write_all(struct acpi_ctx *ctx)
 	const struct acpi_writer *entry;
 	int ret;
 
+	log_debug("writing acpi tables\n");
+
 	for (entry = writer; entry != writer + n_ents; entry++) {
 		ret = acpi_write_one(ctx, entry);
+		log_debug("- return code %d\n", ret);
 		if (ret && ret != -ENOENT)
 			return log_msg_ret("one", ret);
 	}
+	log_debug("writing acpi tables done ok\n");
 
 	return 0;
 }
-- 
2.42.0.rc1.204.g551eb34607-goog

Re: [PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Andy Shevchenko]

On Thu, Aug 24, 2023 at 12:23:32PM -0600, Simon Glass 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.

No luck, but I have a bit of time to debug more.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Andy Shevchenko]

On Fri, Aug 25, 2023 at 02:06:34PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 24, 2023 at 12:23:32PM -0600, Simon Glass 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.
> 
> No luck, but I have a bit of time to debug more.

U-boot:

Writing tables to e4500:
ACPI: Writing ACPI tables at e4500
writing acpi tables
0base: writing table '<NULL>' at 000e4500
- return code 0
1facs: writing table 'FACS' at 000e4740
- return code 0
3dsdt: writing table 'DSDT' at 000e4780
- return code 0
4gnvs: writing table 'GNVS' at 000e5390
- return code 0
5csrt: writing table 'CSRT' at 000e5490
- return code 0
5fadt: writing table 'FADT' at 000e54f0
- return code 0
5mcfg: writing table 'MCFG' at 000e5610
- return code 0
5spcr: writing table 'SPCR' at 000e5650
- return code 0
5tcpa: writing table 'TCPA' at 000e56a0
5tcpa: Omitted due to being empty
- return code 0
5tpm2: writing table 'TPM2' at 000e56a0
5tpm2: Omitted due to being empty
- return code 0
5x86: writing table '<NULL>' at 000e56a0
- return code 0
6ssdt: writing table 'SSDT' at 000e56f0
6ssdt: Omitted due to being empty
- return code 0
8dev: writing table '<NULL>' at 000e56f0
- return code 0
writing acpi tables done ok
ACPI current = e56f0
- wrote 'acpi' to e4500, end e56f0
- wrote 'smbios' to e56f0, end e584f
- done writing tables


Linux:

[    0.003034] ACPI: RSDP 0x00000000000E4500 000024 (v02 U-BOOT)
[    0.003090] ACPI: XSDT 0x00000000000E45E0 000044 (v01 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.003158] ACPI: CSRT 0x00000000000E5490 000058 (v00 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.003219] ACPI: MCFG 0x00000000000E5610 00003C (v01 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.003277] ACPI: SPCR 0x00000000000E5650 000050 (v02 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.003332] ACPI: APIC 0x00000000000E56A0 000048 (v02 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.003382] ACPI: Reserving CSRT table memory at [mem 0xe5490-0xe54e7]
[    0.003403] ACPI: Reserving MCFG table memory at [mem 0xe5610-0xe564b]
[    0.003421] ACPI: Reserving SPCR table memory at [mem 0xe5650-0xe569f]
[    0.003437] ACPI: Reserving APIC table memory at [mem 0xe56a0-0xe56e7]

As you can see a few tables are missing:
FACS, DSDT, GNVS, FADT

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Andy Shevchenko]

On Fri, Aug 25, 2023 at 02:10:05PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 25, 2023 at 02:06:34PM +0300, Andy Shevchenko wrote:
> > On Thu, Aug 24, 2023 at 12:23:32PM -0600, Simon Glass 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.
> > 
> > No luck, but I have a bit of time to debug more.
> 
> U-boot:
> 
> Writing tables to e4500:
> ACPI: Writing ACPI tables at e4500
> writing acpi tables
> 0base: writing table '<NULL>' at 000e4500
> - return code 0
> 1facs: writing table 'FACS' at 000e4740
> - return code 0
> 3dsdt: writing table 'DSDT' at 000e4780
> - return code 0
> 4gnvs: writing table 'GNVS' at 000e5390
> - return code 0
> 5csrt: writing table 'CSRT' at 000e5490
> - return code 0
> 5fadt: writing table 'FADT' at 000e54f0
> - return code 0
> 5mcfg: writing table 'MCFG' at 000e5610
> - return code 0
> 5spcr: writing table 'SPCR' at 000e5650
> - return code 0
> 5tcpa: writing table 'TCPA' at 000e56a0
> 5tcpa: Omitted due to being empty
> - return code 0
> 5tpm2: writing table 'TPM2' at 000e56a0
> 5tpm2: Omitted due to being empty
> - return code 0
> 5x86: writing table '<NULL>' at 000e56a0
> - return code 0
> 6ssdt: writing table 'SSDT' at 000e56f0
> 6ssdt: Omitted due to being empty
> - return code 0
> 8dev: writing table '<NULL>' at 000e56f0
> - return code 0
> writing acpi tables done ok
> ACPI current = e56f0
> - wrote 'acpi' to e4500, end e56f0
> - wrote 'smbios' to e56f0, end e584f
> - done writing tables

> Linux:
> 
> [    0.003034] ACPI: RSDP 0x00000000000E4500 000024 (v02 U-BOOT)
> [    0.003090] ACPI: XSDT 0x00000000000E45E0 000044 (v01 U-BOOT U-BOOTBL 20231001 INTL 00000000)
> [    0.003158] ACPI: CSRT 0x00000000000E5490 000058 (v00 U-BOOT U-BOOTBL 20231001 INTL 00000000)
> [    0.003219] ACPI: MCFG 0x00000000000E5610 00003C (v01 U-BOOT U-BOOTBL 20231001 INTL 00000000)
> [    0.003277] ACPI: SPCR 0x00000000000E5650 000050 (v02 U-BOOT U-BOOTBL 20231001 INTL 00000000)
> [    0.003332] ACPI: APIC 0x00000000000E56A0 000048 (v02 U-BOOT U-BOOTBL 20231001 INTL 00000000)
> [    0.003382] ACPI: Reserving CSRT table memory at [mem 0xe5490-0xe54e7]
> [    0.003403] ACPI: Reserving MCFG table memory at [mem 0xe5610-0xe564b]
> [    0.003421] ACPI: Reserving SPCR table memory at [mem 0xe5650-0xe569f]
> [    0.003437] ACPI: Reserving APIC table memory at [mem 0xe56a0-0xe56e7]
> 
> As you can see a few tables are missing:
> FACS, DSDT, GNVS, FADT

I added a debug to the ACPI writer, so with your patch

Writing tables to e4500:
ACPI: Writing ACPI tables at e4500
writing acpi tables
0base: writing table '<NULL>' at 000e4500
* other: Added type 3, 000e4500, size 240
- return code 0
1facs: writing table 'FACS' at 000e4740
* other: Added type 3, 000e4740, size 40
- return code 0
3dsdt: writing table 'DSDT' at 000e4780
Writing DSDT tables
Writing DSDT finished, err=0
Failed to find ordering, leaving as is
* other: Added type 3, 000e4780, size c10
- return code 0
4gnvs: writing table 'GNVS' at 000e5390
* other: Added type 3, 000e5390, size 100
- return code 0
5csrt: writing table 'CSRT' at 000e5490
* other: Added type 3, 000e5490, size 60
- return code 0
5fadt: writing table 'FADT' at 000e54f0
* other: Added type 3, 000e54f0, size 120
- return code 0
5mcfg: writing table 'MCFG' at 000e5610
* other: Added type 3, 000e5610, size 40
- return code 0
5spcr: writing table 'SPCR' at 000e5650
* other: Added type 3, 000e5650, size 50
- return code 0
5tcpa: writing table 'TCPA' at 000e56a0
5tcpa: Omitted due to being empty
- return code 0
5tpm2: writing table 'TPM2' at 000e56a0
5tpm2: Omitted due to being empty
- return code 0
5x86: writing table '<NULL>' at 000e56a0
* other: Added type 3, 000e56a0, size 50
- return code 0
6ssdt: writing table 'SSDT' at 000e56f0
Writing SSDT tables
Writing SSDT finished, err=0
Failed to find ordering, leaving as is
6ssdt: Omitted due to being empty
- return code 0
8dev: writing table '<NULL>' at 000e56f0
Writing device tables
Writing finished, err=0
- return code 0
writing acpi tables done ok
ACPI current = e56f0
- wrote 'acpi' to e4500, end e56f0
- wrote 'smbios' to e56f0, end e584f
- done writing tables

Without your patch (to save your time:
there is no single character difference 100% copy):

Writing tables to e4500:
ACPI: Writing ACPI tables at e4500
writing acpi tables
0base: writing table '<NULL>' at 000e4500
* other: Added type 3, 000e4500, size 240
- return code 0
1facs: writing table 'FACS' at 000e4740
* other: Added type 3, 000e4740, size 40
- return code 0
3dsdt: writing table 'DSDT' at 000e4780
Writing DSDT tables
Writing DSDT finished, err=0
Failed to find ordering, leaving as is
* other: Added type 3, 000e4780, size c10
- return code 0
4gnvs: writing table 'GNVS' at 000e5390
* other: Added type 3, 000e5390, size 100
- return code 0
5csrt: writing table 'CSRT' at 000e5490
* other: Added type 3, 000e5490, size 60
- return code 0
5fact: writing table 'FADT' at 000e54f0
* other: Added type 3, 000e54f0, size 120
- return code 0
5mcfg: writing table 'MCFG' at 000e5610
* other: Added type 3, 000e5610, size 40
- return code 0
5spcr: writing table 'SPCR' at 000e5650
* other: Added type 3, 000e5650, size 50
- return code 0
5tcpa: writing table 'TCPA' at 000e56a0
5tcpa: Omitted due to being empty
- return code 0
5tpm2: writing table 'TPM2' at 000e56a0
5tpm2: Omitted due to being empty
- return code 0
5x86: writing table '<NULL>' at 000e56a0
* other: Added type 3, 000e56a0, size 50
- return code 0
6ssdt: writing table 'SSDT' at 000e56f0
Writing SSDT tables
Writing SSDT finished, err=0
Failed to find ordering, leaving as is
6ssdt: Omitted due to being empty
- return code 0
8dev: writing table '<NULL>' at 000e56f0
Writing device tables
Writing finished, err=0
- return code 0
writing acpi tables done ok
ACPI current = e56f0
- wrote 'acpi' to e4500, end e56f0
- wrote 'smbios' to e56f0, end e584f
- done writing tables

But here you see the difference. So, your patch damages something there.
Since we lost a few tables, it might be that list of the table pointers
is corrupted with your new approach. And current debug is poor enough to
not catch that...

Linux:

[    0.002557] ACPI: Early table checksum verification disabled
[    0.002582] ACPI: RSDP 0x00000000000E4500 000024 (v02 U-BOOT)
[    0.002637] ACPI: XSDT 0x00000000000E45E0 00004C (v01 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.002704] ACPI: CSRT 0x00000000000E5490 000058 (v00 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.002765] ACPI: FACP 0x00000000000E54F0 000114 (v06 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.002830] ACPI: DSDT 0x00000000000E4780 000C06 (v02 U-BOOT U-BOOTBL 00010000 INTL 20200925)
[    0.002889] ACPI: MCFG 0x00000000000E5610 00003C (v01 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.002945] ACPI: SPCR 0x00000000000E5650 000050 (v02 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.003000] ACPI: APIC 0x00000000000E56A0 000048 (v02 U-BOOT U-BOOTBL 20231001 INTL 00000000)
[    0.003049] ACPI: Reserving CSRT table memory at [mem 0xe5490-0xe54e7]
[    0.003070] ACPI: Reserving FACP table memory at [mem 0xe54f0-0xe5603]
[    0.003087] ACPI: Reserving DSDT table memory at [mem 0xe4780-0xe5385]
[    0.003103] ACPI: Reserving MCFG table memory at [mem 0xe5610-0xe564b]
[    0.003119] ACPI: Reserving SPCR table memory at [mem 0xe5650-0xe569f]
[    0.003135] ACPI: Reserving APIC table memory at [mem 0xe56a0-0xe56e7]

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Andy Shevchenko]

On Fri, Aug 25, 2023 at 02:06:34PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 24, 2023 at 12:23:32PM -0600, Simon Glass 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.
> 
> No luck, but I have a bit of time to debug more.

Okay, after your patch even U-Boot can't see those tables

=> acpi list
Name      Base   Size  Detail
----  --------  -----  ------
RSDP  000e4500     24  v02 U-BOOT
RSDT  000e4530     34  v01 U-BOOT U-BOOTBL 20231001 INTL 0
XSDT  000e45e0     44  v01 U-BOOT U-BOOTBL 20231001 INTL 0
CSRT  000e5490     58  v00 U-BOOT U-BOOTBL 20231001 INTL 0
MCFG  000e5610     3c  v01 U-BOOT U-BOOTBL 20231001 INTL 0
SPCR  000e5650     50  v02 U-BOOT U-BOOTBL 20231001 INTL 0
APIC  000e56a0     48  v02 U-BOOT U-BOOTBL 20231001 INTL 0

Because the base is badly corrupted:

-00000030: 52 53 44 54 38 00 00 00 01 95 55 2d 42 4f 4f 54  RSDT8.....U-BOOT
+00000030: 52 53 44 54 34 00 00 00 01 eb 55 2d 42 4f 4f 54  RSDT4.....U-BOOT

-00000050: 00 00 00 00 90 54 0e 00 f0 54 0e 00 10 56 0e 00  .....T...T...V..
+00000050: 00 00 00 00 90 54 0e 00 10 56 0e 00 50 56 0e 00  .....T...V..PV..

-00000060: 50 56 0e 00 a0 56 0e 00 00 00 00 00 00 00 00 00  PV...V..........
+00000060: a0 56 0e 00 00 00 00 00 00 00 00 00 00 00 00 00  .V..............

-000000e0: 58 53 44 54 4c 00 00 00 01 7b 55 2d 42 4f 4f 54  XSDTL....{U-BOOT
+000000e0: 58 53 44 54 44 00 00 00 01 d5 55 2d 42 4f 4f 54  XSDTD.....U-BOOT

-00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 f0 54 0e 00  .....T.......T..
+00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 10 56 0e 00  .....T.......V..

-00000110: 00 00 00 00 10 56 0e 00 00 00 00 00 50 56 0e 00  .....V......PV..
+00000110: 00 00 00 00 50 56 0e 00 00 00 00 00 a0 56 0e 00  ....PV.......V..

-00000120: 00 00 00 00 a0 56 0e 00 00 00 00 00 00 00 00 00  .....V..........
+00000120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Andy Shevchenko]

On Fri, Aug 25, 2023 at 04:17:58PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 25, 2023 at 02:06:34PM +0300, Andy Shevchenko wrote:
> > On Thu, Aug 24, 2023 at 12:23:32PM -0600, Simon Glass 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.
> > 
> > No luck, but I have a bit of time to debug more.
> 
> Okay, after your patch even U-Boot can't see those tables
> 
> => acpi list
> Name      Base   Size  Detail
> ----  --------  -----  ------
> RSDP  000e4500     24  v02 U-BOOT
> RSDT  000e4530     34  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> XSDT  000e45e0     44  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> CSRT  000e5490     58  v00 U-BOOT U-BOOTBL 20231001 INTL 0
> MCFG  000e5610     3c  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> SPCR  000e5650     50  v02 U-BOOT U-BOOTBL 20231001 INTL 0
> APIC  000e56a0     48  v02 U-BOOT U-BOOTBL 20231001 INTL 0
> 
> Because the base is badly corrupted:
> 
> -00000030: 52 53 44 54 38 00 00 00 01 95 55 2d 42 4f 4f 54  RSDT8.....U-BOOT
> +00000030: 52 53 44 54 34 00 00 00 01 eb 55 2d 42 4f 4f 54  RSDT4.....U-BOOT

Length -4 bytes, another checksum as a result.

> -00000050: 00 00 00 00 90 54 0e 00 f0 54 0e 00 10 56 0e 00  .....T...T...V..
> +00000050: 00 00 00 00 90 54 0e 00 10 56 0e 00 50 56 0e 00  .....T...V..PV..

Missing pointer to 0x0e54f0 which is... FADT!

> -00000060: 50 56 0e 00 a0 56 0e 00 00 00 00 00 00 00 00 00  PV...V..........
> +00000060: a0 56 0e 00 00 00 00 00 00 00 00 00 00 00 00 00  .V..............
> 
> -000000e0: 58 53 44 54 4c 00 00 00 01 7b 55 2d 42 4f 4f 54  XSDTL....{U-BOOT
> +000000e0: 58 53 44 54 44 00 00 00 01 d5 55 2d 42 4f 4f 54  XSDTD.....U-BOOT

Wrong length, another checksum.

> -00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 f0 54 0e 00  .....T.......T..
> +00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 10 56 0e 00  .....T.......V..
> 
> -00000110: 00 00 00 00 10 56 0e 00 00 00 00 00 50 56 0e 00  .....V......PV..
> +00000110: 00 00 00 00 50 56 0e 00 00 00 00 00 a0 56 0e 00  ....PV.......V..
> 
> -00000120: 00 00 00 00 a0 56 0e 00 00 00 00 00 00 00 00 00  .....V..........
> +00000120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................

Same reason. So, your code forgot to link FADT to the main tables.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Andy Shevchenko]

On Fri, Aug 25, 2023 at 04:25:29PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 25, 2023 at 04:17:58PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 25, 2023 at 02:06:34PM +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 24, 2023 at 12:23:32PM -0600, Simon Glass 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.
> > > 
> > > No luck, but I have a bit of time to debug more.
> > 
> > Okay, after your patch even U-Boot can't see those tables
> > 
> > => acpi list
> > Name      Base   Size  Detail
> > ----  --------  -----  ------
> > RSDP  000e4500     24  v02 U-BOOT
> > RSDT  000e4530     34  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> > XSDT  000e45e0     44  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> > CSRT  000e5490     58  v00 U-BOOT U-BOOTBL 20231001 INTL 0
> > MCFG  000e5610     3c  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> > SPCR  000e5650     50  v02 U-BOOT U-BOOTBL 20231001 INTL 0
> > APIC  000e56a0     48  v02 U-BOOT U-BOOTBL 20231001 INTL 0
> > 
> > Because the base is badly corrupted:
> > 
> > -00000030: 52 53 44 54 38 00 00 00 01 95 55 2d 42 4f 4f 54  RSDT8.....U-BOOT
> > +00000030: 52 53 44 54 34 00 00 00 01 eb 55 2d 42 4f 4f 54  RSDT4.....U-BOOT
> 
> Length -4 bytes, another checksum as a result.
> 
> > -00000050: 00 00 00 00 90 54 0e 00 f0 54 0e 00 10 56 0e 00  .....T...T...V..
> > +00000050: 00 00 00 00 90 54 0e 00 10 56 0e 00 50 56 0e 00  .....T...V..PV..
> 
> Missing pointer to 0x0e54f0 which is... FADT!
> 
> > -00000060: 50 56 0e 00 a0 56 0e 00 00 00 00 00 00 00 00 00  PV...V..........
> > +00000060: a0 56 0e 00 00 00 00 00 00 00 00 00 00 00 00 00  .V..............
> > 
> > -000000e0: 58 53 44 54 4c 00 00 00 01 7b 55 2d 42 4f 4f 54  XSDTL....{U-BOOT
> > +000000e0: 58 53 44 54 44 00 00 00 01 d5 55 2d 42 4f 4f 54  XSDTD.....U-BOOT
> 
> Wrong length, another checksum.
> 
> > -00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 f0 54 0e 00  .....T.......T..
> > +00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 10 56 0e 00  .....T.......V..
> > 
> > -00000110: 00 00 00 00 10 56 0e 00 00 00 00 00 50 56 0e 00  .....V......PV..
> > +00000110: 00 00 00 00 50 56 0e 00 00 00 00 00 a0 56 0e 00  ....PV.......V..
> > 
> > -00000120: 00 00 00 00 a0 56 0e 00 00 00 00 00 00 00 00 00  .....V..........
> > +00000120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> 
> Same reason. So, your code forgot to link FADT to the main tables.

By code analysis you simply missed

+       acpi_add_table(ctx, fadt);

I'm going to check this right now... Stay tuned!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 1/2] TO BE FOLDED: x86: tangier: Add FADT to the chain

[Andy Shevchenko]

The Simon's patch missed the FADT to be added to the chain, hence all
the issues on Intel Tangier.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/cpu/tangier/acpi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
index 01b30553818c..ffaa56ab6f87 100644
--- a/arch/x86/cpu/tangier/acpi.c
+++ b/arch/x86/cpu/tangier/acpi.c
@@ -52,6 +52,8 @@ 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;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/2] x86: Prevent from missing the FADT chaining

[Andy Shevchenko]

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>
---
 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 16aaed7238ab..c610a7f44770 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 4c526ff27310..4378846f8b0c 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 92fa5bc30c36..9a2d682451be 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 ffaa56ab6f87..1c667c7d5693 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 7ed0443c821a..1f85de091d39 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
  *
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 2/2] x86: Prevent from missing the FADT chaining

[Andy Shevchenko]

On Fri, Aug 25, 2023 at 04:51:41PM +0300, Andy Shevchenko wrote:

...

> +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;
> +}

Ideally it should be like

	int ret;

	ret = acpi_add_table(ctx, fadt);
	if (ret)
		return ret;

	acpi_inc(ctx, sizeof(struct acpi_fadt));

	return 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] TO BE FOLDED: x86: tangier: Add FADT to the chain

[Simon Glass]

Hi Andy,

On Fri, 25 Aug 2023 at 07:52, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The Simon's patch missed the FADT to be added to the chain, hence all
> the issues on Intel Tangier.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/cpu/tangier/acpi.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
> index 01b30553818c..ffaa56ab6f87 100644
> --- a/arch/x86/cpu/tangier/acpi.c
> +++ b/arch/x86/cpu/tangier/acpi.c
> @@ -52,6 +52,8 @@ 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;
> --
> 2.40.0.1.gaa8946217a0b
>

Thank you! I really don't know how I missed that, after starting at it
all for an hour...

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH 1/2] Reland "x86: Move FACP table into separate functions""

[Simon Glass]

Hi Andy,

On Fri, 25 Aug 2023 at 07:27, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 25, 2023 at 04:25:29PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 25, 2023 at 04:17:58PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 25, 2023 at 02:06:34PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 24, 2023 at 12:23:32PM -0600, Simon Glass 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.
> > > >
> > > > No luck, but I have a bit of time to debug more.
> > >
> > > Okay, after your patch even U-Boot can't see those tables
> > >
> > > => acpi list
> > > Name      Base   Size  Detail
> > > ----  --------  -----  ------
> > > RSDP  000e4500     24  v02 U-BOOT
> > > RSDT  000e4530     34  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> > > XSDT  000e45e0     44  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> > > CSRT  000e5490     58  v00 U-BOOT U-BOOTBL 20231001 INTL 0
> > > MCFG  000e5610     3c  v01 U-BOOT U-BOOTBL 20231001 INTL 0
> > > SPCR  000e5650     50  v02 U-BOOT U-BOOTBL 20231001 INTL 0
> > > APIC  000e56a0     48  v02 U-BOOT U-BOOTBL 20231001 INTL 0
> > >
> > > Because the base is badly corrupted:
> > >
> > > -00000030: 52 53 44 54 38 00 00 00 01 95 55 2d 42 4f 4f 54  RSDT8.....U-BOOT
> > > +00000030: 52 53 44 54 34 00 00 00 01 eb 55 2d 42 4f 4f 54  RSDT4.....U-BOOT
> >
> > Length -4 bytes, another checksum as a result.
> >
> > > -00000050: 00 00 00 00 90 54 0e 00 f0 54 0e 00 10 56 0e 00  .....T...T...V..
> > > +00000050: 00 00 00 00 90 54 0e 00 10 56 0e 00 50 56 0e 00  .....T...V..PV..
> >
> > Missing pointer to 0x0e54f0 which is... FADT!
> >
> > > -00000060: 50 56 0e 00 a0 56 0e 00 00 00 00 00 00 00 00 00  PV...V..........
> > > +00000060: a0 56 0e 00 00 00 00 00 00 00 00 00 00 00 00 00  .V..............
> > >
> > > -000000e0: 58 53 44 54 4c 00 00 00 01 7b 55 2d 42 4f 4f 54  XSDTL....{U-BOOT
> > > +000000e0: 58 53 44 54 44 00 00 00 01 d5 55 2d 42 4f 4f 54  XSDTD.....U-BOOT
> >
> > Wrong length, another checksum.
> >
> > > -00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 f0 54 0e 00  .....T.......T..
> > > +00000100: 00 00 00 00 90 54 0e 00 00 00 00 00 10 56 0e 00  .....T.......V..
> > >
> > > -00000110: 00 00 00 00 10 56 0e 00 00 00 00 00 50 56 0e 00  .....V......PV..
> > > +00000110: 00 00 00 00 50 56 0e 00 00 00 00 00 a0 56 0e 00  ....PV.......V..
> > >
> > > -00000120: 00 00 00 00 a0 56 0e 00 00 00 00 00 00 00 00 00  .....V..........
> > > +00000120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >
> > Same reason. So, your code forgot to link FADT to the main tables.
>
> By code analysis you simply missed
>
> +       acpi_add_table(ctx, fadt);
>
> I'm going to check this right now... Stay tuned!

Thanks for digging into this, I really appreciate it!


- Simon

Re: [PATCH v1 1/2] TO BE FOLDED: x86: tangier: Add FADT to the chain

[Bin Meng]

Hi Andy,

On Fri, Aug 25, 2023 at 9:52 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The Simon's patch missed the FADT to be added to the chain, hence all
> the issues on Intel Tangier.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/cpu/tangier/acpi.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
> index 01b30553818c..ffaa56ab6f87 100644
> --- a/arch/x86/cpu/tangier/acpi.c
> +++ b/arch/x86/cpu/tangier/acpi.c
> @@ -52,6 +52,8 @@ 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;
> --

Which commit should we squash this patch in?

Regards,
Bin

Re: [PATCH v1 1/2] TO BE FOLDED: x86: tangier: Add FADT to the chain

[Bin Meng]

On Tue, Sep 19, 2023 at 3:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Andy,
>
> On Fri, Aug 25, 2023 at 9:52 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > The Simon's patch missed the FADT to be added to the chain, hence all
> > the issues on Intel Tangier.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  arch/x86/cpu/tangier/acpi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
> > index 01b30553818c..ffaa56ab6f87 100644
> > --- a/arch/x86/cpu/tangier/acpi.c
> > +++ b/arch/x86/cpu/tangier/acpi.c
> > @@ -52,6 +52,8 @@ 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;
> > --
>
> Which commit should we squash this patch in?
>

Ah, I see this patch is already included in Simon's patch:
https://patchwork.ozlabs.org/project/uboot/patch/20230901112707.v3.1.I61008e563b1209243a9a7f2b66073b4737ff82d1@changeid/

I am going to apply Simon's patch unless you guys say I am looking at
the wrong patch :)

Regards,
Bin

Re: [PATCH v1 1/2] TO BE FOLDED: x86: tangier: Add FADT to the chain

[Simon Glass]

Hi Bin,

On Tue, 19 Sept 2023 at 01:56, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Sep 19, 2023 at 3:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Andy,
> >
> > On Fri, Aug 25, 2023 at 9:52 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > The Simon's patch missed the FADT to be added to the chain, hence all
> > > the issues on Intel Tangier.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  arch/x86/cpu/tangier/acpi.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
> > > index 01b30553818c..ffaa56ab6f87 100644
> > > --- a/arch/x86/cpu/tangier/acpi.c
> > > +++ b/arch/x86/cpu/tangier/acpi.c
> > > @@ -52,6 +52,8 @@ 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;
> > > --
> >
> > Which commit should we squash this patch in?
> >
>
> Ah, I see this patch is already included in Simon's patch:
> https://patchwork.ozlabs.org/project/uboot/patch/20230901112707.v3.1.I61008e563b1209243a9a7f2b66073b4737ff82d1@changeid/
>
> I am going to apply Simon's patch unless you guys say I am looking at
> the wrong patch :)

Yes, that's it.

Regards,
Simon