From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 3 Mar 2020 19:47:56 -0700 Subject: [PATCH v1] x86: acpi: Refactor XSDT handling in acpi_add_table() In-Reply-To: References: <20200227140051.75072-1-andriy.shevchenko@linux.intel.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Andy, On Tue, 3 Mar 2020 at 02:23, Andy Shevchenko wrote: > > On Tue, Mar 3, 2020 at 1:36 AM Simon Glass wrote: > > On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko wrote: > > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass wrote: > > > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko wrote: > > > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass wrote: > > > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > > > > 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. > > > > > > 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! 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. > > 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? > > > > > > 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. > > Good that we aware. Yes. > > > > > 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 > > Yes, that what I'm talking about. So we are on the same page here. OK good. Regards, Simpon