[PATCH v1] x86: acpi: Refactor XSDT handling in acpi_add_table()

[Simon Glass]

Hi Andy,

On Sun, 29 Mar 2020 at 15:00, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
>
> On Sun, Mar 29, 2020 at 5:13 AM Simon Glass <sjg at chromium.org> wrote:
> > On Thu, 5 Mar 2020 at 05:17, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > > On Tue, Mar 03, 2020 at 07:47:56PM -0700, Simon Glass wrote:
> > > > On Tue, 3 Mar 2020 at 02:23, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > > > > On Tue, Mar 3, 2020 at 1:36 AM Simon Glass <sjg at chromium.org> wrote:
>
> ...
>
> > > 2/ How this is supposed to be compiled?
> > >         + table_compute_checksum((xsdt, xsdt->header.length);
> > >    ...means this series should go thru bisectability tests (something like
> > >    aiaiai https://lwn.net/Articles/488992/ script provides)
> >
> > I'm not sure what you mean by that?
>
> Isn't above may not be compiled without error?
> aiaiai is a script which allows to check bisectability (compile-time)
> errors like above.
>

OK I see. In U-Boot we use buildman for this. It builds entire
branches and I do that before sending patches.

> > > 3/ This one looks not 64-bit compatible.
> > >         +  printf("RSDP %08lx %06x (v%02d %.6s)\n", (ulong)map_to_sysmem(rsdp),
> > >         +         rsdp->length, rsdp->revision, rsdp->oem_id);
> > >   ...means that types for printing and all explicit casting should be revisited.
> >
> > I don't want to print 64-bit addresses if I can help it. I don't even
> > want to use them as they are a pain to look at! If we start using
> > 64-bit U-Boot I still think we will put the ACPI tables below 2GB.
>
> So, it should be documented then.

OK.

>
> ...
>
> > > Till this one "acpi: Add some tables required by the generation code" looks
> > > okay (in terms of approach).
> > >
> > > This one "acpi: Add generation code for devices" requires quite a good review.
> > > So, I would recommend to split the series (and this patch in particular) to
> > > smaller chunks. So does this "acpi: Add functions to generate ACPI code".
> > > They are unreviewable.
> >
> > OK I can split that first part off as a series and then split the next patches.
>
> Thank you.
>
> ...
>
> > > > > I was thinking myself about some U-Boot framework that actually takes
> > > > > ACPI _HID from the driver. So, when you define in U-Boot device tree a
> > > > > compatible string (for U-Boot use), in the driver it will have in the
> > > > > class structure the callback / field / stubstructure to use when ACPI
> > > > > generate tables is enabled. It will drop duplication of compatible
> > > > > with ACPI _HID in each DTS.
> > > >
> > > > Why are you so opposed to using device tree for this? The GPIO and
> > > > pinctrl drivers are intended to be generic....what a pain to add all
> > > > this stuff into the tables in the driver!
> > >
> > > So, this is a trade off between C code and DTS. I'm okay to use DTS for
> > > the stuff that belongs to it. But then, if we enable DTS for ACPI tables
> > > generation, we have to provide a mean to do it without driver involvement.
> > >
> > > How to generate the table for the device U-Boot has no driver for?
> >
> > We add a driver. The driver might only generate ACPI tables, but it is
> > still a driver.
>
> Hmm... This is interesting concept. So, each time we would like to get
> only tables we will need to create a stub driver.

Yes, if it is not actually used in U-Boot.

>
> ...
>
> > > > > But to the current topic, you put *instance* (not even _HID) to the DT
> > > > > with property called "linux,name". It's inappropriate. NAK for that
> > > > > for sure.
> > > >
> > > > OK, so are you saying the property name (linux-name) should change? We
> > > > have acpi,name elsewhere but I don't think that is the _HID.
> > > >
> > > > Or are you saying that the "INT3452:" should be factored out and it
> > > > should know the 00/01/0203 by its position in the device tree?
> > >
> > > It shouldn't be anywhere in the U-Boot, it's complete OS business.
> > > What you have in U-Boot is ACPI _HID (_UID, etc.), and device path (e.g.
> > > \\_SB_.GPO0), no-one should rely on OS (Linux, Windows, etc) internals.
> > > We have already an issue with GPIO pin numbers on Chromebook with Intel
> > > Cherryview SoC.
> >
> > Right but how can ACPI code refer to the GPIO if it cannot reference it?
>
> What do you mean? Any example where you need *instance* over *_HID*?

I suppose I just don't understand this very well. Does ACPI code
access things via a _HID?

>
> > > This
> > >
> > >         + linux-name = "INT3452:00";
> > >
> > > is wrong in both sides -- left, as a property name, and right,
> > > as an *instance* in some OS we must not rely on ever.
> > >
> > > The question is why do you need it?
> >
> > To generate ACPI code which references that GPIO.
> >
> > See chromeos_acpi_gpio_generate().
>
> I  can't see right now. Do you have web browser thru source code?
> GitLab instance doesn't show recent x86 repository.

You can see u-boot-dm/coral-working for the source code.

See here for what I am talking about:

https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/blob/coral-working/board/google/chromebook_coral/coral.c#L51

>
> > Can you suggest a better property name? Maybe acpi,linux-name? But it
> > isn't really an ACPI name.
>
> ACPI _HID. No instance, please. If you are using instance outside of
> Linux kernel code it points to wrong design and I have strong opinion
> not to support this kind of design.

OK let me know what you think of the above. If provides a way to
access these GPIOs.

>
> > > > > > > > > On top of that, I think we rather need to have a conversion layer than
> > > > > > > > > putting some names inside DT, like \_SB_.GPO0 should be generated
> > > > > > > > > automatically from DT node. That said, I don't like DT being polluted
> > > > > > > > > with non-DT stuff.
> > > > > > > >
> > > > > > > > Well DT is the configuration mechanism for U-Boot.
> > > > > > > >
> > > > > > > > \_SB_.GPO0 is a special case since it actually refers to pinctrl (ACPI
> > > > > > > > seems to make no distinction between pinctrl and GPIO) and this node
> > > > > > > > is inside p2sb:
> > > > > > > >
> > > > > > > > pci {
> > > > > > > >    p2sb at d,0 {
> > > > > > > >       n {
> > > > > > > >          gpio-n {
> > > > > > > >
> > > > > > > > So the automatically generated path would have p2sb in it. The same
> > > > > > > > work-around is in coreboot.
> > > > > > >
> > > > > > > It's not a coreboot, we may do better, right?
> > > > > > > So, generation can strip p2sb (special case) from all p2sb devices.
> > > > > > > However, I'm not sure I understand how p2sb is involved in GPIO
> > > > > > > enumeration,
> > > > > >
> > > > > > Well the only other way to create a path is to work up to the root and
> > > > > > build it node by node. I wonder if we could make p2sb be transparent?
> > > > > > I tried that but hit a problem.
> > > > > >
> > > > > > Coreboot has these really awful (IMO) functions that are repeated for every SoC:
> > > > > >
> > > > > > https://github.com/coreboot/coreboot/blob/master/src/soc/amd/stoneyridge/chip.c
> > > > > >
> > > > > > so I want to avoid that.
> > >
> > > I'm not sure I understood how the mentioned source file related to P2SB case.
> > > In that file PCIe functions and USB ports are described.
> >
> > It covers all devices that need an ACPI name. I would like this name
> > to be kept with the rest of the device data, unless if is the same for
> > all devices of a certain type, in which case I suppose we could use a
> > function (e.g. the root node).
>
> I didn't get what you mean under ACPI name here. ACPI _HID? Yes,
> somewhere we need to keep ACPI _HID for the devices. I agree (DTS or
> board code is not so important -- choose best one in your opinion).

OK, DTS.

Regards,
Simon