Hi Wolfgang,
On Thu, 2 Jul 2020 at 02:11, Wolfgang Wallner wolfgang.wallner@br-automation.com wrote:
Hi Simon,
I dont know NHLT well enough to actually review the code, but I did compare the files in this patch to the version in coreboot. Most of the changes are obvious (coding style, spelling, ...), but some things are not, see the remarks below.
-----"Simon Glass" sjg@chromium.org schrieb: -----
Betreff: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
The Intel Non-High-Definition-Audio Link Table (NHLT) table describes the audio codecs and connections in a system. Various devices can contribute information to produce the table.
Add functions to allow adding to the structure that is eventually written to the ACPI tables. Also add the device-tree bindings.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v1:
- Add a new patch to support building up an NHLT structure
arch/x86/include/asm/acpi_nhlt.h | 314 ++++++++++++++++++++ arch/x86/lib/Makefile | 1 + arch/x86/lib/acpi_nhlt.c | 482 +++++++++++++++++++++++++++++++ 3 files changed, 797 insertions(+) create mode 100644 arch/x86/include/asm/acpi_nhlt.h create mode 100644 arch/x86/lib/acpi_nhlt.c
diff --git a/arch/x86/include/asm/acpi_nhlt.h b/arch/x86/include/asm/acpi_nhlt.h new file mode 100644 index 0000000000..4d2573d5ff --- /dev/null +++ b/arch/x86/include/asm/acpi_nhlt.h @@ -0,0 +1,314 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Copyright 2020 Google LLC
- Modified from coreboot nhlt.h
- */
+#ifndef _NHLT_H_ +#define _NHLT_H_
+struct acpi_ctx; +struct nhlt; +struct nhlt_endpoint; +struct nhlt_format; +struct nhlt_format_config;
+/*
- Non HD Audio ACPI support. This table is typically used for Intel Smart
- Sound Technology DSP. It provides a way to encode opaque settings in
- the ACPI tables.
- While the structure fields of the NHLT structs are exposed below
- the SoC/chipset code should be the only other user manipulating the
- fields directly aside from the library itself.
- The NHLT table consists of endpoints which in turn contain different
- supporting stream formats. Each endpoint may contain a device specific
- configuration payload as well as each stream format.
- Most code should use the SoC variants of the functions because
- there is required logic needed to be performed by the SoC. The SoC
- code should be abstracting the inner details of these functions that
- specically apply to NHLT objects for that SoC.
- An example sequence:
- nhlt = nhlt_init()
- ep = nhlt_add_endpoint()
- nhlt_endpoint_append_config(ep)
- nhlt_endpoint_add_formats(ep)
- nhlt_soc_serialise()
- */
+/* Obtain an nhlt object for adding endpoints. Returns NULL on error. */ +struct nhlt *nhlt_init(void);
+/* Return the size of the NHLT table including ACPI header. */ +size_t nhlt_current_size(struct nhlt *nhlt);
+/*
- Helper functions for adding NHLT devices utilizing an nhlt_endp_descriptor
- to drive the logic.
- */
+struct nhlt_endp_descriptor {
/* NHLT endpoint types. */int link;int device;int direction;u16 vid;u16 did;/* Optional endpoint specific configuration data. */const void *cfg;size_t cfg_size;/* Formats supported for endpoint. */const struct nhlt_format_config *formats;size_t num_formats;+};
+/*
- Add the number of endpoints described by each descriptor. The virtual bus
- id for each descriptor is the default value of 0.
- Returns < 0 on error, 0 on success.
- */
+int nhlt_add_endpoints(struct nhlt *nhlt,
const struct nhlt_endp_descriptor *epds,size_t num_epds);+/*
- Add the number of endpoints associated with a single NHLT SSP instance id.
- Each endpoint described in the endpoint descriptor array uses the provided
- virtual bus id. Returns < 0 on error, 0 on success.
- */
+int nhlt_add_ssp_endpoints(struct nhlt *nhlt, int virtual_bus_id,
const struct nhlt_endp_descriptor *epds,size_t num_epds);+/*
- Add endpoint to NHLT object. Returns NULL on error.
- generic nhlt_add_endpoint() is called by the SoC code to provide
- the specific assumptions/uses for NHLT for that platform. All fields
- are the NHLT enumerations found within this header file.
- */
+struct nhlt_endpoint *nhlt_add_endpoint(struct nhlt *nhlt, int link_type,
int device_type, int dir,u16 vid, u16 did);+/*
- Append blob of configuration to the endpoint proper. Returns 0 on
- success, < 0 on error. A copy of the configuration is made so any
- resources pointed to by config can be freed after the call.
- */
+int nhlt_endpoint_append_config(struct nhlt_endpoint *endpoint,
const void *config, size_t config_sz);+/* Add a format type to the provided endpoint. Returns NULL on error. */ +struct nhlt_format *nhlt_add_format(struct nhlt_endpoint *endpoint,
int num_channels, int sample_freq_khz,int container_bits_per_sample,int valid_bits_per_sample,u32 speaker_mask);+/*
- Append blob of configuration to the format proper. Returns 0 on
- success, < 0 on error. A copy of the configuration is made so any
- resources pointed to by config can be freed after the call.
- */
+int nhlt_format_append_config(struct nhlt_format *format, const void *config,
size_t config_sz);+/*
- Add num_formats described by formats to the endpoint. This function
- effectively wraps nhlt_add_format() and nhlt_format_config() using the
- data found in each nhlt_format_config object. Returns 0 on success, < 0
- on error.
- */
+int nhlt_endpoint_add_formats(struct nhlt_endpoint *endpoint,
const struct nhlt_format_config *formats,size_t num_formats);+/*
- Increment the instance id for a given link type. This function is
- used for marking a device being completely added to the NHLT object.
- Subsequent endpoints added to the nhlt object with the same link type
- will use incremented instance id.
- */
+void nhlt_next_instance(struct nhlt *nhlt, int link_type);
+/*
- Serialize NHLT object to ACPI table. Take in the beginning address of where
- the table will reside and return the address of the next ACPI table. On
- error 0 will be returned. The NHLT object is no longer valid after this
- function is called.
- */
+uintptr_t nhlt_serialise(struct nhlt *nhlt, uintptr_t acpi_addr);
The implementation for this functions seems to be missing in this patch. In coreboot it looks like this:
uintptr_t nhlt_serialize(struct nhlt *nhlt, uintptr_t acpi_addr) { return nhlt_serialize_oem_overrides(nhlt, acpi_addr, NULL, NULL, 0); }
Yes we don't need this at present. If we do we would likely put this in the device tree. Since coreboot doesn't have a proper device tree it uses code to call override functions to get the data, which IMO makes it quite hard to work out the config for a particular board
[..]
+/*
- Channel mask for an endpoint. While they are prefixed with 'SPEAKER' the
- channel masks are also used for capture devices
- */
+enum {
SPEAKER_FRONT_LEFT = 1 << 0,Nit: BIT(0) ?
Yes will fix thanks.
[..]
+static int append_specific_config(struct nhlt_specific_config *spec_cfg,
const void *config, size_t config_sz)+{
size_t new_sz;void *new_cfg;In coreboot this function starts with a check of config and config_sz:
if (config == NULL || config_sz == 0) return 0;
Why is this check left out in this implementation?
We don't allow the config to be NULL since that is pointless. Same with the size.
new_sz = spec_cfg->size + config_sz;new_cfg = malloc(new_sz);if (!new_cfg)return -ENOMEM;
Regards, SImon