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

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

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

On Sun, Mar 08, 2020 at 09:44:36PM -0600, Simon Glass wrote:
> ACPI (Advanced Configuration and Power Interface) is an Intel standard

Not Intel for a long time. Or more precisely, not *only* Intel.

Also this should be corrected (I guess dropping Intel would work) everywhere in
this series.

> for specifying information about a platform.

> It is a little like device
> tree but considerably more complicated and with more backslashes.

For what purpose this passage?
Isn't the same reason why ARM64 choose ACPI to be supported for servers?

> 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.

...

> +/* 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

Do we really need two definitions?

...

> +	/* Intel Advanced Configuration and Power Interface (ACPI) */

Same as above for commit message.

Hi Andy,

On Tue, 10 Mar 2020 at 08:46, Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Sun, Mar 08, 2020 at 09:44:36PM -0600, Simon Glass wrote:
> > ACPI (Advanced Configuration and Power Interface) is an Intel standard
>
> Not Intel for a long time. Or more precisely, not *only* Intel.
>
> Also this should be corrected (I guess dropping Intel would work) everywhere in
> this series.

OK will do.

>
> > for specifying information about a platform.
>
> > It is a little like device
> > tree but considerably more complicated and with more backslashes.
>
> For what purpose this passage?
> Isn't the same reason why ARM64 choose ACPI to be supported for servers?
>
> > 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.
>
> ...
>
> > +/* 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
>
> Do we really need two definitions?

It is annoying to have to use

char name[ACPI_NAME_LEN + 1]

everywhere.

On the other hand, some structs don't need a terminator, so we need
ACPI_NAME_LEN.

I'll change it so one is computed from the other.

>
> ...
>
> > +     /* Intel Advanced Configuration and Power Interface (ACPI) */
>
> Same as above for commit message.
>

Regards,
Simon