[PATCH v2 05/35] acpi: Support generation of ACPI code
Wolfgang Wallner
wolfgang.wallner at br-automation.com
Wed Jun 10 09:33:24 CEST 2020
Hi Simon,
-----"Simon Glass" <sjg at chromium.org> schrieb: -----
> Betreff: Re: [PATCH v2 05/35] acpi: Support generation of ACPI code
>
> Hi Wolfgang,
>
> On Thu, 14 May 2020 at 02:32, Wolfgang Wallner
> <wolfgang.wallner at br-automation.com> wrote:
> >
> > Hi Simon,
> >
> > -----"Simon Glass" <sjg at chromium.org> schrieb: -----
> > > Betreff: [PATCH v2 05/35] acpi: Support generation of ACPI code
> > >
> > > Add a new file to handle generating ACPI code programatically. This is
> > > used when information must be dynamically added to the tables, e.g. the
> > > SSDT.
> > >
> > > Initial support is just for writing simple values.
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > ---
> > >
> > > Changes in v2: None
> > > Changes in v1: None
> > >
> > > include/acpi/acpigen.h | 49 +++++++++++++++++++++++++++++++
> > > lib/acpi/Makefile | 1 +
> > > lib/acpi/acpigen.c | 38 ++++++++++++++++++++++++
> > > test/dm/Makefile | 1 +
> > > test/dm/acpigen.c | 65 ++++++++++++++++++++++++++++++++++++++++++
> > > 5 files changed, 154 insertions(+)
> > > create mode 100644 include/acpi/acpigen.h
> > > create mode 100644 lib/acpi/acpigen.c
> > > create mode 100644 test/dm/acpigen.c
> > >
> > > diff --git a/include/acpi/acpigen.h b/include/acpi/acpigen.h
> > > new file mode 100644
> > > index 0000000000..8809cdb4e1
> > > --- /dev/null
> > > +++ b/include/acpi/acpigen.h
> > > @@ -0,0 +1,49 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Core ACPI (Advanced Configuration and Power Interface) support
> > > + *
> > > + * Copyright 2019 Google LLC
> > > + *
> > > + * Modified from coreboot file acpigen.h
> > > + */
> > > +
> > > +#ifndef __ACPI_ACPIGEN_H
> > > +#define __ACPI_ACPIGEN_H
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +struct acpi_ctx;
> > > +
> > > +/**
> > > + * acpigen_get_current() - Get the current ACPI code output pointer
> > > + *
> > > + * @ctx: ACPI context pointer
> > > + * @return output pointer
> > > + */
> > > +u8 *acpigen_get_current(struct acpi_ctx *ctx);
> > > +
> > > +/**
> > > + * acpigen_emit_byte() - Emit a byte to the ACPI code
> > > + *
> > > + * @ctx: ACPI context pointer
> > > + * @data: Value to output
> > > + */
> > > +void acpigen_emit_byte(struct acpi_ctx *ctx, uint data);
> > > +
> > > +/**
> > > + * acpigen_emit_word() - Emit a 16-bit word to the ACPI code
> > > + *
> > > + * @ctx: ACPI context pointer
> > > + * @data: Value to output
> > > + */
> > > +void acpigen_emit_word(struct acpi_ctx *ctx, uint data);
> > > +
> > > +/**
> > > + * acpigen_emit_dword() - Emit a 32-bit 'double word' to the ACPI code
> > > + *
> > > + * @ctx: ACPI context pointer
> > > + * @data: Value to output
> > > + */
> > > +void acpigen_emit_dword(struct acpi_ctx *ctx, uint data);
> > > +
> > > +#endif
> > > diff --git a/lib/acpi/Makefile b/lib/acpi/Makefile
> > > index caae6c01bd..85a1f774ad 100644
> > > --- a/lib/acpi/Makefile
> > > +++ b/lib/acpi/Makefile
> > > @@ -1,5 +1,6 @@
> > > # SPDX-License-Identifier: GPL-2.0+
> > > #
> > >
> > > +obj-y += acpigen.o
> > > obj-y += acpi_device.o
> > > obj-y += acpi_table.o
> > > diff --git a/lib/acpi/acpigen.c b/lib/acpi/acpigen.c
> > > new file mode 100644
> > > index 0000000000..59bd3af0b7
> > > --- /dev/null
> > > +++ b/lib/acpi/acpigen.c
> > > @@ -0,0 +1,38 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Generation of ACPI (Advanced Configuration and Power Interface) tables
> > > + *
> > > + * Copyright 2019 Google LLC
> > > + * Mostly taken from coreboot
> > > + */
> > > +
> > > +#define LOG_CATEGORY LOGC_ACPI
> > > +
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <acpi/acpigen.h>
> > > +#include <dm/acpi.h>
> > > +
> > > +u8 *acpigen_get_current(struct acpi_ctx *ctx)
> > > +{
> > > + return ctx->current;
> > > +}
> > > +
> > > +void acpigen_emit_byte(struct acpi_ctx *ctx, uint data)
> > > +{
> >
> > As we expect exactly a byte, could data be of type uint8_t ?
> > Similar for the functions below.
>
> It could but I really try to avoid that. It means that the compiler
> has to mask the values coming in in a register wihch adds to code
> size.
Ok, thanks for the explanation.
>
> >
> > > + *(u8 *)ctx->current++ = data;
> > > +}
> > > +
> > > +void acpigen_emit_word(struct acpi_ctx *ctx, uint data)
> > > +{
> > > + acpigen_emit_byte(ctx, data & 0xff);
> > > + acpigen_emit_byte(ctx, (data >> 8) & 0xff);
> >
> > This function assumes little-endian host endianess. This works under
> > x86 and probably most of ARM, and I'm not aware of other architectures
> > using ACPI.
> >
> > Should it be made more portable anyway e.g. by using cpu_to_le16()?
>
> Do you mean that ACPI can store the bytes in the other order?
No, I did not mean that.
> I
> thought it was only big-endian but actually I don't know that. Do you
> have any details?
You are right that ACPI only supports a single endianess, but it is
little-endian, not big-endian.
Section 5.2 "ACPI System Description Tables" of ACPI 6.3 states:
"All numeric values in ACPI-defined tables, blocks, and structures are always
encoded in little endian format."
and section 19.3.5 "ASL Data Types" tells us that an integer is "An n-bit
little-endian unsigned integer".
The only exception to the everything-is-little-endian-rule that I could find
is given in Section "5.2.6 System Description Table Header":
"Tables defined outside of the ACPI specification may define data value
encodings in either little endian or big endian format. For the purpose of
clarity, external table definition documents should include the endian-ness
of their data value encodings."
> Or are you asking about the shift? I believe that on big-endian
> machines, shifting right still divides by 256, and so does what is
> expected.
Yes, that is what I was worried about, but I got it wrong. As long as we only
write, we only have to take care about the endianess of the target (ACPI).
The could above should work on both little-endian and big-endian architectures,
so please ignore my comment.
> >
> > > +}
> > > +
> > > +void acpigen_emit_dword(struct acpi_ctx *ctx, uint data)
> > > +{
> > > + acpigen_emit_byte(ctx, data & 0xff);
> > > + acpigen_emit_byte(ctx, (data >> 8) & 0xff);
> > > + acpigen_emit_byte(ctx, (data >> 16) & 0xff);
> > > + acpigen_emit_byte(ctx, (data >> 24) & 0xff);
> > > +}
> > > diff --git a/test/dm/Makefile b/test/dm/Makefile
> > > index 6c18fd04ce..e3e0cccf01 100644
> > > --- a/test/dm/Makefile
> > > +++ b/test/dm/Makefile
> > > @@ -14,6 +14,7 @@ obj-$(CONFIG_UT_DM) += test-uclass.o
> > > obj-$(CONFIG_UT_DM) += core.o
> > > ifneq ($(CONFIG_SANDBOX),)
> > > obj-$(CONFIG_ACPIGEN) += acpi.o
> > > +obj-$(CONFIG_ACPIGEN) += acpigen.o
> > > obj-$(CONFIG_SOUND) += audio.o
> > > obj-$(CONFIG_BLK) += blk.o
> > > obj-$(CONFIG_BOARD) += board.o
> > > diff --git a/test/dm/acpigen.c b/test/dm/acpigen.c
> > > new file mode 100644
> > > index 0000000000..68f2b73132
> > > --- /dev/null
> > > +++ b/test/dm/acpigen.c
> > > @@ -0,0 +1,65 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Tests for ACPI code generation
> > > + *
> > > + * Copyright 2019 Google LLC
> > > + * Written by Simon Glass <sjg at chromium.org>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <malloc.h>
> > > +#include <acpi/acpigen.h>
> > > +#include <asm/unaligned.h>
> > > +#include <dm/acpi.h>
> > > +#include <dm/test.h>
> > > +#include <test/ut.h>
> > > +
> > > +static int alloc_context(struct acpi_ctx **ctxp)
> > > +{
> > > + struct acpi_ctx *ctx;
> > > +
> > > + *ctxp = NULL;
> > > + ctx = malloc(sizeof(*ctx));
> > > + if (!ctx)
> > > + return -ENOMEM;
> > > + ctx->current = malloc(150);
> > > + if (!ctx->current)
> >
> > free(ctx)
> >
> > > + return -ENOMEM;
> > > + *ctxp = ctx;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void free_context(struct acpi_ctx **ctxp)
> > > +{
> >
> > Should the memory that was initally allocated to ctx->current also be
> > released or stay allocted?
>
> It is normally allocated by the code that uses it, and generally just
> points to memory (e.g. 0xf0000) rather than using malloc(). But for
> the benefit of tests I can add a base value, and free it.
>
> Regards,
> Simon
regards, Wolfgang
More information about the U-Boot
mailing list