Antwort: [PATCH v2 12/39] dm: core: Add basic ACPI support

[Wolfgang Wallner]

Hi Simon,

-----"Simon Glass" <sjg at chromium.org> schrieb: -----
> 
> ACPI (Advanced Configuration and Power Interface) is an Intel standard
> for specifying information about a platform. It is a little like device
> tree but considerably more complicated and with more backslashes. A
> primary difference is that it supports an interpreted bytecode language.
> 
> Driver model does not use ACPI for U-Boot's configuration, but it is
> convenient to have it support generation of ACPI tables for passing to
> Linux, etc.
> 
> As a starting point, add an optional set of ACPI operations to each
> device. Initially only a single operation is available, to obtain the
> ACPI name for the device. More operations are added later.
> 
> Enable ACPI for sandbox to ensure build coverage and so that we can add
> tests.
> 
> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
> Changes in v2:
> - Move LOGC_ACPI definition to this patch
> 
>  drivers/core/Kconfig  |  9 ++++++
>  drivers/core/Makefile |  1 +
>  drivers/core/acpi.c   | 32 +++++++++++++++++++
>  include/dm/acpi.h     | 73 +++++++++++++++++++++++++++++++++++++++++++
>  include/dm/device.h   |  5 +++
>  include/log.h         |  2 ++
>  6 files changed, 122 insertions(+)
>  create mode 100644 drivers/core/acpi.c
>  create mode 100644 include/dm/acpi.h
> 
> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> index 3b95b5387b..a3b0399342 100644
> --- a/drivers/core/Kconfig
> +++ b/drivers/core/Kconfig
> @@ -261,4 +261,13 @@ config DM_DEV_READ_INLINE
>  	bool
>  	default y if !OF_LIVE
>  
> +config ACPIGEN
> +	bool "Support ACPI table generation in driver model"
> +	default y if SANDBOX || GENERATE_ACPI_TABLE
> +	help
> +	  This option enables generation of ACPI tables using driver-model
> +	  devices. It adds a new operation struct to each driver, to support
> +	  things like generating device-specific tables and returning the ACPI
> +	  name of a device.
> +
>  endmenu
> diff --git a/drivers/core/Makefile b/drivers/core/Makefile
> index bce7467da1..c707026a3a 100644
> --- a/drivers/core/Makefile
> +++ b/drivers/core/Makefile
> @@ -3,6 +3,7 @@
>  # Copyright (c) 2013 Google, Inc
>  
>  obj-y	+= device.o fdtaddr.o lists.o root.o uclass.o util.o
> +obj-$(CONFIG_$(SPL_TPL_)ACPIGEN) += acpi.o
>  obj-$(CONFIG_DEVRES) += devres.o
>  obj-$(CONFIG_$(SPL_)DM_DEVICE_REMOVE)	+= device-remove.o
>  obj-$(CONFIG_$(SPL_)SIMPLE_BUS)	+= simple-bus.o
> diff --git a/drivers/core/acpi.c b/drivers/core/acpi.c
> new file mode 100644
> index 0000000000..45542199f5
> --- /dev/null
> +++ b/drivers/core/acpi.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Core driver model support for ACPI table generation
> + *
> + * Copyright 2019 Google LLC
> + * Written by Simon Glass <sjg at chromium.org>
> + */
> +
> +#define LOG_CATEOGRY	LOGC_ACPI
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <dm/acpi.h>
> +#include <dm/root.h>
> +
> +int acpi_return_name(char *out_name, const char *name)

A few nits:

* IMHO acpi_copy_name() would be a more fitting name. The function does not
  return a name, it copies it.
* Could we use strncpy + explicit null-termination instead of strcpy?

> +{
> +	strcpy(out_name, name);
> +
> +	return 0;
> +}
> +
> +int acpi_get_name(const struct udevice *dev, char *out_name)
> +{
> +	struct acpi_ops *aops;
> +
> +	aops = device_get_acpi_ops(dev);
> +	if (aops && aops->get_name)
> +		return aops->get_name(dev, out_name);
> +
> +	return -ENOSYS;
> +}
> diff --git a/include/dm/acpi.h b/include/dm/acpi.h
> new file mode 100644
> index 0000000000..120576adc0
> --- /dev/null
> +++ b/include/dm/acpi.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Core ACPI (Advanced Configuration and Power Interface) support
> + *
> + * Copyright 2019 Google LLC
> + * Written by Simon Glass <sjg at chromium.org>
> + */
> +
> +#ifndef __DM_ACPI_H__
> +#define __DM_ACPI_H__
> +
> +/* Allow operations to be optional for ACPI */
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> +#define acpi_ops_ptr(_ptr)	.acpi_ops	= _ptr,
> +#else
> +#define acpi_ops_ptr(_ptr)
> +#endif
> +
> +/* Length of an ACPI name string, excluding nul terminator */
> +#define ACPI_NAME_LEN	4
> +
> +/* Length of an ACPI name string including nul terminator */
> +#define ACPI_NAME_MAX	5
> +
> +/**
> + * struct acpi_ops - ACPI operations supported by driver model
> + */
> +struct acpi_ops {
> +	/**
> +	 * get_name() - Obtain the ACPI name of a device
> +	 *
> +	 * @dev: Device to check
> +	 * @out_name: Place to put the name, must hold at least ACPI_NAME_MAX
> +	 *	bytes
> +	 * @return 0 if OK, -ENOENT if no name is available, other -ve value on
> +	 *	other error
> +	 */
> +	int (*get_name)(const struct udevice *dev, char *out_name);
> +};
> +
> +#define device_get_acpi_ops(dev)	((dev)->driver->acpi_ops)
> +
> +/**
> + * acpi_get_name() - Obtain the ACPI name of a device
> + *
> + * @dev: Device to check
> + * @out_name: Place to put the name, must hold at least ACPI_NAME_MAX
> + *	bytes
> + * @return 0 if OK, -ENOENT if no name is available, other -ve value on
> + *	other error
> + */
> +int acpi_get_name(const struct udevice *dev, char *out_name);
> +
> +/**
> + * acpi_return_name() - Copy an ACPI name to an output buffer
> + *
> + * This convenience function can be used to return a literal string as a name
> + * in functions that implement the get_name() method.
> + *
> + * For example:
> + *
> + *	static int mydev_get_name(const struct udevice *dev, char *out_name)
> + *	{
> + *		return acpi_return_name(out_name, "WIBB");
> + *	}
> + *
> + * @out_name: Place to put the name
> + * @name: Name to copy
> + * @return 0 (always)
> + */
> +int acpi_return_name(char *out_name, const char *name);
> +
> +#endif
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 3517fc1926..9e77a0cd7b 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -245,6 +245,8 @@ struct udevice_id {
>   * pointers defined by the driver, to implement driver functions required by
>   * the uclass.
>   * @flags: driver flags - see DM_FLAGS_...
> + * @acpi_ops: Advanced Configuration and Power Interface (ACPI) operations,
> + * allowing the device to add things to the ACPI tables passed to Linux
>   */
>  struct driver {
>  	char *name;
> @@ -264,6 +266,9 @@ struct driver {
>  	int per_child_platdata_auto_alloc_size;
>  	const void *ops;	/* driver-specific operations */
>  	uint32_t flags;
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> +	struct acpi_ops *acpi_ops;
> +#endif
>  };
>  
>  /* Declare a new U-Boot driver */
> diff --git a/include/log.h b/include/log.h
> index 62fb8afbd0..d706a1a464 100644
> --- a/include/log.h
> +++ b/include/log.h
> @@ -51,6 +51,8 @@ enum log_category_t {
>  	LOGC_SANDBOX,	/* Related to the sandbox board */
>  	LOGC_BLOBLIST,	/* Bloblist */
>  	LOGC_DEVRES,	/* Device resources (devres_... functions) */
> +	/* Intel Advanced Configuration and Power Interface (ACPI) */

I would drop the "Intel".

> +	LOGC_ACPI,
>  
>  	LOGC_COUNT,	/* Number of log categories */
>  	LOGC_END,	/* Sentinel value for a list of log categories */
> -- 
> 2.25.1.481.gfbce0eb801-goog
> 

regards, Wolfgang