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

Thu Mar 5 13:17:28 CET 2020

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:
> > > On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg at chromium.org> wrote:
> > > > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > > > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko
> > > > > > > <andriy.shevchenko at linux.intel.com> wrote:
> > > > > >
> > > > > > > Could you take a look at the ACPI series?
> > > > > > >
> > > > > > > It was sent out about a month ago and has a refactor to this function.
> > > > > > >
> > > > > > > u-boot-dm/coral-working
> > > > > >
> > > > > > There are tons of changes. Care to point what changes are more
> > > > > > important (generic to all x86)?
> > > > >
> > > > > I'm not quite sure about that...but x86 patches have an x86: tag, so
> > > > > perhaps that helps?
> > > >
> > > > Okay, some like 50 of them or even more? I really don't want to spend
> > > > time on the board related patches like "x86: apl:".
> > >
> > > Well that's why I add the tags, so you can see what they relate to.
> > > This is probably a good one to review:
> > >
> > >  dm: core: Add basic ACPI support
> >
> > Okay, I will try to find a time to look at it first.

I started looking at them from the above mentioned patch.

1/ Can we do include/acpi/ folder for ACPI related headers?
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)
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.

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.

Perhaps first pile some generalization that ARM people may start their work...

> > > > > > P.S. Briefly looking at the last ~30 patches I can say that the idea
> > > > > > looks good, implementation needs more work. For example, there is
> > > > > > 'linux,name' property. Shouldn't be referred at all. Linux names and
> > > > > > other type of enumerations is utterly opaque to the outside world.
> > > > >
> > > > > How do we add the required linux,name ACPI property into the ACPI
> > > > > tables for a device?
> > > >
> > > > There must not be Linux device names or anything Linux related (like
> > > > hardcoded GPIO numbers) in the ACPI table.
> > >
> > > Apparently the Intel GPIO driver requires that name. See for example here:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/intel/pinctrl-broxton.c#L999
> > >
> > > static const struct acpi_device_id bxt_pinctrl_acpi_match[] = {
> > > { "INT3452", (kernel_ulong_t)apl_pinctrl_soc_data },
> > > { "INT34D1", (kernel_ulong_t)bxt_pinctrl_soc_data },
> > > { }
> > > };
> > >
> > > So we have to put INT3452 in the ACPI table.
> >
> > Wait, this is not a *name*, this is ACPI _HID. ACPI _HID, of course,
> > should be somewhere in board code.
> >
> > 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?

> When other platforms use APL we can move some .dts nodes over into a
> intel-apl.dtsi file (or similar) to deal with any duplication. Of
> course we don't want duplication.
> 
> Re the thread that Wolfgang references, I'm going to have a close look
> at that and hopefully simplify things. We still need quite a bit more
> patches to be reviewed before it is worth sending again, I think.

Yes, please. My main point here is to avoid data duplication because it's
simpler to pollute DTS with it.

> > 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.

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?

> > > > > > 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.

-- 
With Best Regards,
Andy Shevchenko