[PATCH v1 22/43] x86: Add support for building up an NHLT structure

[Simon Glass]

Hi Wolfgang,

On Thu, 2 Jul 2020 at 02:11, Wolfgang Wallner
<wolfgang.wallner at 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 at 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 at 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