diff mbox series

[v3,23/29] acpi: Convert part of acpi_table to use acpi_ctx

Message ID 20200330171226.v3.23.I93e1e33891714417335e1dd517982b18bf9f882f@changeid
State Superseded
Delegated to: Bin Meng
Headers show
Series dm: Add programmatic generation of ACPI tables (part A) | expand

Commit Message

Simon Glass March 30, 2020, 11:12 p.m. UTC 
The current code uses an address but a pointer would result in fewer
casts. Also it repeats the alignment code in a lot of places so this would
be better done in a helper function.

Update write_acpi_tables() to make use of the new acpi_ctx structure,
adding a few helpers to clean things up.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
---

Changes in v3:
- Rename acpi_align_large() to acpi_align64()

Changes in v2: None

 arch/x86/lib/acpi_table.c | 88 +++++++++++++++++++--------------------
 include/acpi/acpi_table.h | 36 ++++++++++++++++
 lib/acpi/acpi_table.c     | 24 ++++++++++-
 test/dm/acpi.c            | 28 +++++++++++++
 4 files changed, 130 insertions(+), 46 deletions(-)

Comments

Andy Shevchenko April 3, 2020, 1:24 p.m. UTC | #1 
On Mon, Mar 30, 2020 at 05:12:59PM -0600, Simon Glass wrote:
> The current code uses an address but a pointer would result in fewer
> casts. Also it repeats the alignment code in a lot of places so this would
> be better done in a helper function.
> 
> Update write_acpi_tables() to make use of the new acpi_ctx structure,
> adding a few helpers to clean things up.

...

> +void acpi_align(struct acpi_ctx *ctx);

> +void acpi_align64(struct acpi_ctx *ctx);

In the code, it will be not understandable the difference.
align without number would be good if the function only single one.

So, align16() much better.

> +void acpi_inc(struct acpi_ctx *ctx, uint amount);

inc with amount is not inc, it's rather add.

> + * acpi_inc_align() - Increment the ACPI output pointer by a bit and align

Align how?

> +void acpi_inc_align(struct acpi_ctx *ctx, uint amount);
Andy Shevchenko April 3, 2020, 1:25 p.m. UTC | #2 
On Fri, Apr 03, 2020 at 04:24:06PM +0300, Andy Shevchenko wrote:
> On Mon, Mar 30, 2020 at 05:12:59PM -0600, Simon Glass wrote:
> > The current code uses an address but a pointer would result in fewer
> > casts. Also it repeats the alignment code in a lot of places so this would
> > be better done in a helper function.
> > 
> > Update write_acpi_tables() to make use of the new acpi_ctx structure,
> > adding a few helpers to clean things up.

...

> > +void acpi_inc(struct acpi_ctx *ctx, uint amount);
> 
> inc with amount is not inc, it's rather add.

...or pad if you wish.

> 
> > + * acpi_inc_align() - Increment the ACPI output pointer by a bit and align
Simon Glass April 8, 2020, 2:57 a.m. UTC | #3 
Hi Andy,

On Fri, 3 Apr 2020 at 07:24, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Mar 30, 2020 at 05:12:59PM -0600, Simon Glass wrote:
> > The current code uses an address but a pointer would result in fewer
> > casts. Also it repeats the alignment code in a lot of places so this would
> > be better done in a helper function.
> >
> > Update write_acpi_tables() to make use of the new acpi_ctx structure,
> > adding a few helpers to clean things up.
>
> ...
>
> > +void acpi_align(struct acpi_ctx *ctx);
>
> > +void acpi_align64(struct acpi_ctx *ctx);
>
> In the code, it will be not understandable the difference.
> align without number would be good if the function only single one.
>
> So, align16() much better.

We had this discussion previously and changed it to what we have now
in v3. I feel it is a matter of preference.

>
> > +void acpi_inc(struct acpi_ctx *ctx, uint amount);
>
> inc with amount is not inc, it's rather add.

Increment by one, or increment by an amount. The word 'add' would be
very confusing here, since we are not adding anything to the ACPI
table.

>
> > + * acpi_inc_align() - Increment the ACPI output pointer by a bit and align
>
> Align how?

Will update the comment

>
REgardsm

Simon
diff mbox series

Patch

diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index f98d6d17b3e..afa00bc2f80 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -10,6 +10,7 @@ 
 #include <cpu.h>
 #include <dm.h>
 #include <dm/uclass-internal.h>
+#include <mapmem.h>
 #include <serial.h>
 #include <version.h>
 #include <acpi/acpi_table.h>
@@ -19,6 +20,7 @@ 
 #include <asm/mpspec.h>
 #include <asm/tables.h>
 #include <asm/arch/global_nvs.h>
+#include <dm/acpi.h>
 
 /*
  * IASL compiles the dsdt entries and writes the hex values
@@ -468,9 +470,9 @@  static void acpi_create_spcr(struct acpi_spcr *spcr)
 /*
  * QEMU's version of write_acpi_tables is defined in drivers/misc/qfw.c
  */
-ulong write_acpi_tables(ulong start)
+ulong write_acpi_tables(ulong start_addr)
 {
-	u32 current;
+	struct acpi_ctx sctx, *ctx = &sctx;
 	struct acpi_rsdp *rsdp;
 	struct acpi_rsdt *rsdt;
 	struct acpi_xsdt *xsdt;
@@ -481,60 +483,61 @@  ulong write_acpi_tables(ulong start)
 	struct acpi_madt *madt;
 	struct acpi_csrt *csrt;
 	struct acpi_spcr *spcr;
+	void *start;
+	ulong addr;
 	int i;
 
-	current = start;
+	start = map_sysmem(start_addr, 0);
+	ctx->current = start;
 
 	/* Align ACPI tables to 16 byte */
-	current = ALIGN(current, 16);
+	acpi_align(ctx);
 
-	debug("ACPI: Writing ACPI tables at %lx\n", start);
+	debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
 
 	/* We need at least an RSDP and an RSDT Table */
-	rsdp = (struct acpi_rsdp *)current;
-	current += sizeof(struct acpi_rsdp);
-	current = ALIGN(current, 16);
-	rsdt = (struct acpi_rsdt *)current;
-	current += sizeof(struct acpi_rsdt);
-	current = ALIGN(current, 16);
-	xsdt = (struct acpi_xsdt *)current;
-	current += sizeof(struct acpi_xsdt);
+	rsdp = ctx->current;
+	acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
+	rsdt = ctx->current;
+	acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
+	xsdt = ctx->current;
+	acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
 	/*
 	 * Per ACPI spec, the FACS table address must be aligned to a 64 byte
 	 * boundary (Windows checks this, but Linux does not).
 	 */
-	current = ALIGN(current, 64);
+	acpi_align64(ctx);
 
 	/* clear all table memory */
-	memset((void *)start, 0, current - start);
+	memset((void *)start, 0, ctx->current - start);
 
 	acpi_write_rsdp(rsdp, rsdt, xsdt);
 	acpi_write_rsdt(rsdt);
 	acpi_write_xsdt(xsdt);
 
 	debug("ACPI:    * FACS\n");
-	facs = (struct acpi_facs *)current;
-	current += sizeof(struct acpi_facs);
-	current = ALIGN(current, 16);
+	facs = ctx->current;
+	acpi_inc_align(ctx, sizeof(struct acpi_facs));
 
 	acpi_create_facs(facs);
 
 	debug("ACPI:    * DSDT\n");
-	dsdt = (struct acpi_table_header *)current;
+	dsdt = ctx->current;
 	memcpy(dsdt, &AmlCode, sizeof(struct acpi_table_header));
-	current += sizeof(struct acpi_table_header);
-	memcpy((char *)current,
+	acpi_inc(ctx, sizeof(struct acpi_table_header));
+	memcpy(ctx->current,
 	       (char *)&AmlCode + sizeof(struct acpi_table_header),
 	       dsdt->length - sizeof(struct acpi_table_header));
-	current += dsdt->length - sizeof(struct acpi_table_header);
-	current = ALIGN(current, 16);
+	acpi_inc_align(ctx, dsdt->length - sizeof(struct acpi_table_header));
 
 	/* Pack GNVS into the ACPI table area */
 	for (i = 0; i < dsdt->length; i++) {
 		u32 *gnvs = (u32 *)((u32)dsdt + i);
 		if (*gnvs == ACPI_GNVS_ADDR) {
-			debug("Fix up global NVS in DSDT to 0x%08x\n", current);
-			*gnvs = current;
+			ulong addr = (ulong)map_to_sysmem(ctx->current);
+
+			debug("Fix up global NVS in DSDT to %#08lx\n", addr);
+			*gnvs = addr;
 			break;
 		}
 	}
@@ -544,51 +547,46 @@  ulong write_acpi_tables(ulong start)
 	dsdt->checksum = table_compute_checksum((void *)dsdt, dsdt->length);
 
 	/* Fill in platform-specific global NVS variables */
-	acpi_create_gnvs((struct acpi_global_nvs *)current);
-	current += sizeof(struct acpi_global_nvs);
-	current = ALIGN(current, 16);
+	acpi_create_gnvs(ctx->current);
+	acpi_inc_align(ctx, sizeof(struct acpi_global_nvs));
 
 	debug("ACPI:    * FADT\n");
-	fadt = (struct acpi_fadt *)current;
-	current += sizeof(struct acpi_fadt);
-	current = ALIGN(current, 16);
+	fadt = ctx->current;
+	acpi_inc_align(ctx, sizeof(struct acpi_fadt));
 	acpi_create_fadt(fadt, facs, dsdt);
 	acpi_add_table(rsdp, fadt);
 
 	debug("ACPI:    * MADT\n");
-	madt = (struct acpi_madt *)current;
+	madt = ctx->current;
 	acpi_create_madt(madt);
-	current += madt->header.length;
+	acpi_inc_align(ctx, madt->header.length);
 	acpi_add_table(rsdp, madt);
-	current = ALIGN(current, 16);
 
 	debug("ACPI:    * MCFG\n");
-	mcfg = (struct acpi_mcfg *)current;
+	mcfg = ctx->current;
 	acpi_create_mcfg(mcfg);
-	current += mcfg->header.length;
+	acpi_inc_align(ctx, mcfg->header.length);
 	acpi_add_table(rsdp, mcfg);
-	current = ALIGN(current, 16);
 
 	debug("ACPI:    * CSRT\n");
-	csrt = (struct acpi_csrt *)current;
+	csrt = ctx->current;
 	acpi_create_csrt(csrt);
-	current += csrt->header.length;
+	acpi_inc_align(ctx, csrt->header.length);
 	acpi_add_table(rsdp, csrt);
-	current = ALIGN(current, 16);
 
 	debug("ACPI:    * SPCR\n");
-	spcr = (struct acpi_spcr *)current;
+	spcr = ctx->current;
 	acpi_create_spcr(spcr);
-	current += spcr->header.length;
+	acpi_inc_align(ctx, spcr->header.length);
 	acpi_add_table(rsdp, spcr);
-	current = ALIGN(current, 16);
 
-	debug("current = %x\n", current);
+	addr = map_to_sysmem(ctx->current);
+	debug("current = %lx\n", addr);
 
 	acpi_rsdp_addr = (unsigned long)rsdp;
 	debug("ACPI: done\n");
 
-	return current;
+	return addr;
 }
 
 ulong acpi_get_rsdp_addr(void)
diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
index 268e36b67e1..0710324e698 100644
--- a/include/acpi/acpi_table.h
+++ b/include/acpi/acpi_table.h
@@ -26,6 +26,8 @@ 
 
 #if !defined(__ACPI__)
 
+struct acpi_ctx;
+
 /*
  * RSDP (Root System Description Pointer)
  * Note: ACPI 1.0 didn't have length, xsdt_address, and ext_checksum
@@ -519,6 +521,40 @@  int acpi_create_dmar(struct acpi_dmar *dmar, enum dmar_flags flags);
  */
 void acpi_fill_header(struct acpi_table_header *header, char *signature);
 
+/**
+ * acpi_align() - Align the ACPI output pointer to a 16-byte boundary
+ *
+ * @ctx: ACPI context
+ */
+void acpi_align(struct acpi_ctx *ctx);
+
+/**
+ * acpi_align64() - Align the ACPI output pointer to a 64-byte boundary
+ *
+ * @ctx: ACPI context
+ */
+void acpi_align64(struct acpi_ctx *ctx);
+
+/**
+ * acpi_inc() - Increment the ACPI output pointer by a bit
+ *
+ * The pointer is NOT aligned afterwards.
+ *
+ * @ctx: ACPI context
+ * @amount: Amount to increment by
+ */
+void acpi_inc(struct acpi_ctx *ctx, uint amount);
+
+/**
+ * acpi_inc_align() - Increment the ACPI output pointer by a bit and align
+ *
+ * The pointer is aligned afterwards.
+ *
+ * @ctx: ACPI context
+ * @amount: Amount to increment by
+ */
+void acpi_inc_align(struct acpi_ctx *ctx, uint amount);
+
 #endif /* !__ACPI__*/
 
 #include <asm/acpi_table.h>
diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
index 1710cde2c40..35075a608f7 100644
--- a/lib/acpi/acpi_table.c
+++ b/lib/acpi/acpi_table.c
@@ -6,10 +6,11 @@ 
  */
 
 #include <common.h>
-#include <acpi/acpi_table.h>
 #include <dm.h>
 #include <cpu.h>
 #include <version.h>
+#include <acpi/acpi_table.h>
+#include <dm/acpi.h>
 
 int acpi_create_dmar(struct acpi_dmar *dmar, enum dmar_flags flags)
 {
@@ -96,3 +97,24 @@  void acpi_fill_header(struct acpi_table_header *header, char *signature)
 	header->oem_revision = U_BOOT_BUILD_DATE;
 	memcpy(header->aslc_id, ASLC_ID, 4);
 }
+
+void acpi_align(struct acpi_ctx *ctx)
+{
+	ctx->current = (void *)ALIGN((ulong)ctx->current, 16);
+}
+
+void acpi_align64(struct acpi_ctx *ctx)
+{
+	ctx->current = (void *)ALIGN((ulong)ctx->current, 64);
+}
+
+void acpi_inc(struct acpi_ctx *ctx, uint amount)
+{
+	ctx->current += amount;
+}
+
+void acpi_inc_align(struct acpi_ctx *ctx, uint amount)
+{
+	ctx->current += amount;
+	acpi_align(ctx);
+}
diff --git a/test/dm/acpi.c b/test/dm/acpi.c
index 5743530a3a5..17e208ee80e 100644
--- a/test/dm/acpi.c
+++ b/test/dm/acpi.c
@@ -151,3 +151,31 @@  static int dm_test_acpi_write_tables(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_acpi_write_tables, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Test basic ACPI functions */
+static int dm_test_acpi_basic(struct unit_test_state *uts)
+{
+	struct acpi_ctx ctx;
+
+	/* Check align works */
+	ctx.current = (void *)5;
+	acpi_align(&ctx);
+	ut_asserteq_ptr((void *)16, ctx.current);
+
+	/* Check that align does nothing if already aligned */
+	acpi_align(&ctx);
+	ut_asserteq_ptr((void *)16, ctx.current);
+	acpi_align64(&ctx);
+	ut_asserteq_ptr((void *)64, ctx.current);
+	acpi_align64(&ctx);
+	ut_asserteq_ptr((void *)64, ctx.current);
+
+	/* Check incrementing */
+	acpi_inc(&ctx, 3);
+	ut_asserteq_ptr((void *)67, ctx.current);
+	acpi_inc_align(&ctx, 3);
+	ut_asserteq_ptr((void *)80, ctx.current);
+
+	return 0;
+}
+DM_TEST(dm_test_acpi_basic, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);