From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 7 Jul 2020 21:32:57 -0600 Subject: [PATCH v1 22/43] x86: Add support for building up an NHLT structure In-Reply-To: References: <20200615035738.248710-1-sjg@chromium.org> <20200615035738.248710-12-sjg@chromium.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Wolfgang, On Thu, 2 Jul 2020 at 02:11, Wolfgang Wallner 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" 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 > > --- > > > > 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