| Message ID | 20200227140051.75072-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | ddcccb2b2c1251ee9cd658c340d722a716d75a96 |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [v1] x86: acpi: Refactor XSDT handling in acpi_add_table() | expand |
Hi Andy, On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > There is no need to have an assignment to NULL for XSDT pointer. > Therefore, no need to assign it when rsdt_address is not set. > Because of above changes we may decrease indentation level as well. > > While here, drop unnecessary parentheses. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/x86/lib/acpi_table.c | 37 +++++++++++++++++++------------------ > 1 file changed, 19 insertions(+), 18 deletions(-) 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 Regards, Simon > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c > index efc4edf801..421456fc2e 100644 > --- a/arch/x86/lib/acpi_table.c > +++ b/arch/x86/lib/acpi_table.c > @@ -109,14 +109,11 @@ static void acpi_add_table(struct acpi_rsdp *rsdp, void *table) > { > int i, entries_num; > struct acpi_rsdt *rsdt; > - struct acpi_xsdt *xsdt = NULL; > + struct acpi_xsdt *xsdt; > > /* The RSDT is mandatory while the XSDT is not */ > rsdt = (struct acpi_rsdt *)rsdp->rsdt_address; > > - if (rsdp->xsdt_address) > - xsdt = (struct acpi_xsdt *)((u32)rsdp->xsdt_address); > - > /* This should always be MAX_ACPI_TABLES */ > entries_num = ARRAY_SIZE(rsdt->entry); > > @@ -135,30 +132,34 @@ static void acpi_add_table(struct acpi_rsdp *rsdp, void *table) > > /* Fix RSDT length or the kernel will assume invalid entries */ > rsdt->header.length = sizeof(struct acpi_table_header) + > - (sizeof(u32) * (i + 1)); > + sizeof(u32) * (i + 1); > > /* Re-calculate checksum */ > rsdt->header.checksum = 0; > rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, > rsdt->header.length); > > + /* The RSDT is mandatory while the XSDT is not */ > + if (!rsdp->xsdt_address) > + return; > + > /* > * And now the same thing for the XSDT. We use the same index as for > * now we want the XSDT and RSDT to always be in sync in U-Boot > */ > - if (xsdt) { > - /* Add table to the XSDT */ > - xsdt->entry[i] = (u64)(u32)table; > - > - /* Fix XSDT length */ > - xsdt->header.length = sizeof(struct acpi_table_header) + > - (sizeof(u64) * (i + 1)); > - > - /* Re-calculate checksum */ > - xsdt->header.checksum = 0; > - xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, > - xsdt->header.length); > - } > + xsdt = (struct acpi_xsdt *)((u32)rsdp->xsdt_address); > + > + /* Add table to the XSDT */ > + xsdt->entry[i] = (u64)(u32)table; > + > + /* Fix XSDT length */ > + xsdt->header.length = sizeof(struct acpi_table_header) + > + sizeof(u64) * (i + 1); > + > + /* Re-calculate checksum */ > + xsdt->header.checksum = 0; > + xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, > + xsdt->header.length); > } > > static void acpi_create_facs(struct acpi_facs *facs) > -- > 2.25.0 >
On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > <andriy.shevchenko@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)? 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. 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. 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).
Hi Andy, On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > <andriy.shevchenko@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? > > 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? > > 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@d,0 { n { gpio-n { So the automatically generated path would have p2sb in it. The same work-around is in coreboot. > > 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!) probably have a better idea on what is needed. The core DM support and tests are there. Regards, Simon
On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg@chromium.org> wrote: > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > <andriy.shevchenko@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:". > > 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. > > 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@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, > > 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? > 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.
Hi Andy, On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg@chromium.org> wrote: > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > > <andriy.shevchenko@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@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
On Tue, Mar 3, 2020 at 7:36 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Andy, > > On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg@chromium.org> wrote: > > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > > > <andriy.shevchenko@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@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 > That's what my assumption too. > Bin, what do you think? I will take some time to review the remaining patches though. Regards, Bin
On Tue, Mar 3, 2020 at 1:36 AM Simon Glass <sjg@chromium.org> wrote: > On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg@chromium.org> wrote: > > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > > > <andriy.shevchenko@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. > > > > 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. 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. > > > > 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@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. > > > 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.
Hi Andy, -----"U-Boot" <u-boot-bounces@lists.denx.de> schrieb: ----- > > [snip] > > > > > > 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. There is a related discussion in another thread, here is the link: https://lists.denx.de/pipermail/u-boot/2020-February/398856.html I have brought that up, but I'm no expert in this area, so any feedback would be welcome. That discussion is not only about inferring _HID, but also about the idea of inferring other ACPI properties from device tree (the example discussed is the HID offset). regards, Wolfgang
On Tue, Mar 3, 2020 at 12:00 PM Wolfgang Wallner <wolfgang.wallner@br-automation.com> wrote: ... > > 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. > > There is a related discussion in another thread, here is the link: > https://lists.denx.de/pipermail/u-boot/2020-February/398856.html Thank you for pointing this out. I totally agree with you. I really do not like the idea of polluting DT with ACPI bits. > I have brought that up, but I'm no expert in this area, so any feedback would > be welcome. > > That discussion is not only about inferring _HID, but also about the idea of > inferring other ACPI properties from device tree (the example discussed is the > HID offset). Yes, that's what I'm implying above as well. I'm on the same page with you!
Hi Andy, On Tue, 3 Mar 2020 at 02:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Mar 3, 2020 at 1:36 AM Simon Glass <sjg@chromium.org> wrote: > > On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg@chromium.org> wrote: > > > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > > > > <andriy.shevchenko@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. > > > > > > 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@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
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@gmail.com> wrote: > > On Tue, Mar 3, 2020 at 1:36 AM Simon Glass <sjg@chromium.org> wrote: > > > On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg@chromium.org> wrote: > > > > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > > > > > <andriy.shevchenko@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@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.
On Thu, Feb 27, 2020 at 10:00 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > There is no need to have an assignment to NULL for XSDT pointer. > Therefore, no need to assign it when rsdt_address is not set. > Because of above changes we may decrease indentation level as well. > > While here, drop unnecessary parentheses. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/x86/lib/acpi_table.c | 37 +++++++++++++++++++------------------ > 1 file changed, 19 insertions(+), 18 deletions(-) > This patch looks good to me and I believe we can put this in v2020.04. Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Regards, Bin
On Wed, Mar 25, 2020 at 2:48 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Thu, Feb 27, 2020 at 10:00 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > There is no need to have an assignment to NULL for XSDT pointer. > > Therefore, no need to assign it when rsdt_address is not set. > > Because of above changes we may decrease indentation level as well. > > > > While here, drop unnecessary parentheses. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > arch/x86/lib/acpi_table.c | 37 +++++++++++++++++++------------------ > > 1 file changed, 19 insertions(+), 18 deletions(-) > > > > This patch looks good to me and I believe we can put this in v2020.04. > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > applied to u-boot-x86, thanks!
Hi Andy, On Thu, 5 Mar 2020 at 05:17, Andy Shevchenko <andy.shevchenko@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@gmail.com> wrote: > > > On Tue, Mar 3, 2020 at 1:36 AM Simon Glass <sjg@chromium.org> wrote: > > > > On Mon, 2 Mar 2020 at 13:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Mon, Mar 2, 2020 at 9:47 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > On Fri, 28 Feb 2020 at 01:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > On Fri, Feb 28, 2020 at 1:41 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > On Thu, 27 Feb 2020 at 06:00, Andy Shevchenko > > > > > > > > <andriy.shevchenko@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? OK > 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? > 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. > > > 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. > > 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? We add a driver. The driver might only generate ACPI tables, but it is still a 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. > > Yes, please. My main point here is to avoid data duplication because it's > simpler to pollute DTS with it. OK I have done that in v3. > > > > 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? > > 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(). Can you suggest a better property name? Maybe acpi,linux-name? But it isn't really an ACPI name. > > > > > > > > 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@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). Regards, Simon
On Sun, Mar 29, 2020 at 5:13 AM Simon Glass <sjg@chromium.org> wrote: > On Thu, 5 Mar 2020 at 05:17, Andy Shevchenko <andy.shevchenko@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@gmail.com> wrote: > > > > On Tue, Mar 3, 2020 at 1:36 AM Simon Glass <sjg@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. > > 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. ... > > 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. ... > > > > 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*? > > 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. > 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. > > > > > > > > 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@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).
Hi Andy, On Sun, 29 Mar 2020 at 15:00, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Mar 29, 2020 at 5:13 AM Simon Glass <sjg@chromium.org> wrote: > > On Thu, 5 Mar 2020 at 05:17, Andy Shevchenko <andy.shevchenko@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@gmail.com> wrote: > > > > > On Tue, Mar 3, 2020 at 1:36 AM Simon Glass <sjg@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@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
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c index efc4edf801..421456fc2e 100644 --- a/arch/x86/lib/acpi_table.c +++ b/arch/x86/lib/acpi_table.c @@ -109,14 +109,11 @@ static void acpi_add_table(struct acpi_rsdp *rsdp, void *table) { int i, entries_num; struct acpi_rsdt *rsdt; - struct acpi_xsdt *xsdt = NULL; + struct acpi_xsdt *xsdt; /* The RSDT is mandatory while the XSDT is not */ rsdt = (struct acpi_rsdt *)rsdp->rsdt_address; - if (rsdp->xsdt_address) - xsdt = (struct acpi_xsdt *)((u32)rsdp->xsdt_address); - /* This should always be MAX_ACPI_TABLES */ entries_num = ARRAY_SIZE(rsdt->entry); @@ -135,30 +132,34 @@ static void acpi_add_table(struct acpi_rsdp *rsdp, void *table) /* Fix RSDT length or the kernel will assume invalid entries */ rsdt->header.length = sizeof(struct acpi_table_header) + - (sizeof(u32) * (i + 1)); + sizeof(u32) * (i + 1); /* Re-calculate checksum */ rsdt->header.checksum = 0; rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, rsdt->header.length); + /* The RSDT is mandatory while the XSDT is not */ + if (!rsdp->xsdt_address) + return; + /* * And now the same thing for the XSDT. We use the same index as for * now we want the XSDT and RSDT to always be in sync in U-Boot */ - if (xsdt) { - /* Add table to the XSDT */ - xsdt->entry[i] = (u64)(u32)table; - - /* Fix XSDT length */ - xsdt->header.length = sizeof(struct acpi_table_header) + - (sizeof(u64) * (i + 1)); - - /* Re-calculate checksum */ - xsdt->header.checksum = 0; - xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, - xsdt->header.length); - } + xsdt = (struct acpi_xsdt *)((u32)rsdp->xsdt_address); + + /* Add table to the XSDT */ + xsdt->entry[i] = (u64)(u32)table; + + /* Fix XSDT length */ + xsdt->header.length = sizeof(struct acpi_table_header) + + sizeof(u64) * (i + 1); + + /* Re-calculate checksum */ + xsdt->header.checksum = 0; + xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, + xsdt->header.length); } static void acpi_create_facs(struct acpi_facs *facs)
There is no need to have an assignment to NULL for XSDT pointer. Therefore, no need to assign it when rsdt_address is not set. Because of above changes we may decrease indentation level as well. While here, drop unnecessary parentheses. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/x86/lib/acpi_table.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)