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

2023-08-31 Thread Andy Shevchenko
On Wed, Aug 30, 2023 at 09:13:34PM -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.
> 
> Signed-off-by: Simon Glass 
> Signed-off-by: Andy Shevchenko 

Not sure this is correct SoB chain. In kernel project we would do

Co-developed-by: Andy Shevchenko 
Signed-off-by: Andy Shevchenko 
Signed-off-by: Simon Glass 

> ---
> (Now fixed - thank you Andy!)

Right,
Tested-by: Andy Shevchenko 

-- 
With Best Regards,
Andy Shevchenko




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

2023-08-30 Thread 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 
Signed-off-by: Andy Shevchenko 
---
(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/yga9z7sbfaev6...@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 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 = >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 = >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 
 #include 
 
-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 = >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;