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

From: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
To: u-boot@lists.denx.de
Subject: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
Date: Wed, 8 Jul 2020 13:05:50 +0200	[thread overview]
Message-ID: <OF0FF11EED.538D1CED-ONC125859F.002CE457-C125859F.003CF57F@br-automation.com> (raw)
In-Reply-To: <CAPnjgZ1wNXzvtoV5EDLK4DNWcFfuojnCv6n87UQFXFWy+0TA3w@mail.gmail.com>

Hi Simon,

-----"Simon Glass" <sjg@chromium.org> schrieb: -----
> Betreff: Re: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
> 
> 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

[snip]

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

If we don't need the implementation of nhlt_serialise(), shouldn't we then
also drop its declaration in the header file?

> 
> [..]
> 

regards, Wolfgang