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

[Simon Glass]

Hi Andy,

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

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

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

>
> > > Also, I'm not sure how your rework helps ARM (or any other
> > > architecture) people with their approach to ACPI enabling (most of the
> > > files are under x86).
> >
> > I kept x86-specific tables in the x86 directories. Of course I might
> > be wrong about this. But then, people who use ACPI on ARM (ick!)
>
> Haven't you seen the series to introduce ACPI for ARM in U-Boot recently?

Yes, and that author is awaiting us getting this series in so that he
can build on it.

>
> > probably have a better idea on what is needed. The core DM support and
> > tests are there.
>
> I think with a such big rework it's not big deal to simple move it
> outside of arch/x86 to the lib/acpi or so.

My intention was to put generic ACPI things in there though, not
x86-specific stuff. I assume that ARM would have its own stuff in
arch.arm

Bin, what do you think?

Regards,
Simon