Antwort: [PATCH v2 05/35] acpi: Support generation of ACPI code

[Wolfgang Wallner]

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.

> +	*(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()?

> +}
> +
> +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?
	
> +	free(*ctxp);
> +	*ctxp = NULL;
> +}
> +
> +/* Test emitting simple types and acpigen_get_current() */
> +static int dm_test_acpi_emit_simple(struct unit_test_state *uts)
> +{
> +	struct acpi_ctx *ctx;
> +	u8 *ptr;
> +
> +	ut_assertok(alloc_context(&ctx));
> +
> +	ptr = acpigen_get_current(ctx);
> +	acpigen_emit_byte(ctx, 0x23);
> +	ut_asserteq(1, acpigen_get_current(ctx) - ptr);
> +	ut_asserteq(0x23, *(u8 *)ptr);
> +
> +	acpigen_emit_word(ctx, 0x1234);
> +	ut_asserteq(3, acpigen_get_current(ctx) - ptr);
> +	ut_asserteq(0x1234, get_unaligned((u16 *)(ptr + 1)));
> +
> +	acpigen_emit_dword(ctx, 0x87654321);
> +	ut_asserteq(7, acpigen_get_current(ctx) - ptr);
> +	ut_asserteq(0x87654321, get_unaligned((u32 *)(ptr + 3)));
> +
> +	free_context(&ctx);
> +
> +	return 0;
> +}
> +DM_TEST(dm_test_acpi_emit_simple, 0);
> -- 
> 2.26.2.645.ge9eca65c58-goog

regards, Wolfgang