Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
soc/intel/skylake: Remove misleading BUG: messages
The skylake pch_log_rp_wake_source function causes a "BUG: pch_log_rp_wake_source requests hidden...." message for each root port not listed in the device tree. This is caused by the fact that the function tries to read all root ports possible on the Skylake PCH. This is required because of a flaw in the PCH.
The issue is solved by using the 'pcidev_path_on_root' function instead of the one that is standard for the Skylake SoC. This routine doesn display the "BUG" messages.
BUG=N/A TEST=build
Change-Id: I6bc04e48e97b0a29aef8aa050d3aad116cff1a14 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/elog.c 1 file changed, 59 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38750/1
diff --git a/src/soc/intel/skylake/elog.c b/src/soc/intel/skylake/elog.c index 411b3e9..a3c8fc4 100644 --- a/src/soc/intel/skylake/elog.c +++ b/src/soc/intel/skylake/elog.c @@ -120,6 +120,41 @@ }
#define RP_PME_STS_BIT (1 << 16) + +#if !defined(__SIMPLE_DEVICE__) +//#include <device/device.h> +#define RP_DEV(slot, func) pcidev_path_on_root(_PCH_DEVFN(slot, func)) +#else +#define RP_DEV(slot, func) PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func) +#endif + +#define RP_DEV_PCIE1 RP_DEV(PCIE, 0) +#define RP_DEV_PCIE2 RP_DEV(PCIE, 1) +#define RP_DEV_PCIE3 RP_DEV(PCIE, 2) +#define RP_DEV_PCIE4 RP_DEV(PCIE, 3) +#define RP_DEV_PCIE5 RP_DEV(PCIE, 4) +#define RP_DEV_PCIE6 RP_DEV(PCIE, 5) +#define RP_DEV_PCIE7 RP_DEV(PCIE, 6) +#define RP_DEV_PCIE8 RP_DEV(PCIE, 7) + +#define RP_DEV_PCIE9 RP_DEV(PCIE_1, 0) +#define RP_DEV_PCIE10 RP_DEV(PCIE_1, 1) +#define RP_DEV_PCIE11 RP_DEV(PCIE_1, 2) +#define RP_DEV_PCIE12 RP_DEV(PCIE_1, 3) +#define RP_DEV_PCIE13 RP_DEV(PCIE_1, 4) +#define RP_DEV_PCIE14 RP_DEV(PCIE_1, 5) +#define RP_DEV_PCIE15 RP_DEV(PCIE_1, 6) +#define RP_DEV_PCIE16 RP_DEV(PCIE_1, 7) + +#define RP_DEV_PCIE17 RP_DEV(PCIE_2, 0) +#define RP_DEV_PCIE18 RP_DEV(PCIE_2, 1) +#define RP_DEV_PCIE19 RP_DEV(PCIE_2, 2) +#define RP_DEV_PCIE20 RP_DEV(PCIE_2, 3) +#define RP_DEV_PCIE21 RP_DEV(PCIE_2, 4) +#define RP_DEV_PCIE22 RP_DEV(PCIE_2, 5) +#define RP_DEV_PCIE23 RP_DEV(PCIE_2, 6) +#define RP_DEV_PCIE24 RP_DEV(PCIE_2, 7) + static void pch_log_rp_wake_source(void) { size_t i, maxports; @@ -131,30 +166,30 @@ uint32_t val;
struct pme_status_info pme_status_info[] = { - { PCH_DEV_PCIE1, 0x60, ELOG_WAKE_SOURCE_PME_PCIE1 }, - { PCH_DEV_PCIE2, 0x60, ELOG_WAKE_SOURCE_PME_PCIE2 }, - { PCH_DEV_PCIE3, 0x60, ELOG_WAKE_SOURCE_PME_PCIE3 }, - { PCH_DEV_PCIE4, 0x60, ELOG_WAKE_SOURCE_PME_PCIE4 }, - { PCH_DEV_PCIE5, 0x60, ELOG_WAKE_SOURCE_PME_PCIE5 }, - { PCH_DEV_PCIE6, 0x60, ELOG_WAKE_SOURCE_PME_PCIE6 }, - { PCH_DEV_PCIE7, 0x60, ELOG_WAKE_SOURCE_PME_PCIE7 }, - { PCH_DEV_PCIE8, 0x60, ELOG_WAKE_SOURCE_PME_PCIE8 }, - { PCH_DEV_PCIE9, 0x60, ELOG_WAKE_SOURCE_PME_PCIE9 }, - { PCH_DEV_PCIE10, 0x60, ELOG_WAKE_SOURCE_PME_PCIE10 }, - { PCH_DEV_PCIE11, 0x60, ELOG_WAKE_SOURCE_PME_PCIE11 }, - { PCH_DEV_PCIE12, 0x60, ELOG_WAKE_SOURCE_PME_PCIE12 }, - { PCH_DEV_PCIE13, 0x60, ELOG_WAKE_SOURCE_PME_PCIE13 }, - { PCH_DEV_PCIE14, 0x60, ELOG_WAKE_SOURCE_PME_PCIE14 }, - { PCH_DEV_PCIE15, 0x60, ELOG_WAKE_SOURCE_PME_PCIE15 }, - { PCH_DEV_PCIE16, 0x60, ELOG_WAKE_SOURCE_PME_PCIE16 }, - { PCH_DEV_PCIE17, 0x60, ELOG_WAKE_SOURCE_PME_PCIE17 }, - { PCH_DEV_PCIE18, 0x60, ELOG_WAKE_SOURCE_PME_PCIE18 }, - { PCH_DEV_PCIE19, 0x60, ELOG_WAKE_SOURCE_PME_PCIE19 }, - { PCH_DEV_PCIE20, 0x60, ELOG_WAKE_SOURCE_PME_PCIE20 }, - { PCH_DEV_PCIE21, 0x60, ELOG_WAKE_SOURCE_PME_PCIE21 }, - { PCH_DEV_PCIE22, 0x60, ELOG_WAKE_SOURCE_PME_PCIE22 }, - { PCH_DEV_PCIE23, 0x60, ELOG_WAKE_SOURCE_PME_PCIE23 }, - { PCH_DEV_PCIE24, 0x60, ELOG_WAKE_SOURCE_PME_PCIE24 }, + { RP_DEV_PCIE1, 0x60, ELOG_WAKE_SOURCE_PME_PCIE1 }, + { RP_DEV_PCIE2, 0x60, ELOG_WAKE_SOURCE_PME_PCIE2 }, + { RP_DEV_PCIE3, 0x60, ELOG_WAKE_SOURCE_PME_PCIE3 }, + { RP_DEV_PCIE4, 0x60, ELOG_WAKE_SOURCE_PME_PCIE4 }, + { RP_DEV_PCIE5, 0x60, ELOG_WAKE_SOURCE_PME_PCIE5 }, + { RP_DEV_PCIE6, 0x60, ELOG_WAKE_SOURCE_PME_PCIE6 }, + { RP_DEV_PCIE7, 0x60, ELOG_WAKE_SOURCE_PME_PCIE7 }, + { RP_DEV_PCIE8, 0x60, ELOG_WAKE_SOURCE_PME_PCIE8 }, + { RP_DEV_PCIE9, 0x60, ELOG_WAKE_SOURCE_PME_PCIE9 }, + { RP_DEV_PCIE10, 0x60, ELOG_WAKE_SOURCE_PME_PCIE10 }, + { RP_DEV_PCIE11, 0x60, ELOG_WAKE_SOURCE_PME_PCIE11 }, + { RP_DEV_PCIE12, 0x60, ELOG_WAKE_SOURCE_PME_PCIE12 }, + { RP_DEV_PCIE13, 0x60, ELOG_WAKE_SOURCE_PME_PCIE13 }, + { RP_DEV_PCIE14, 0x60, ELOG_WAKE_SOURCE_PME_PCIE14 }, + { RP_DEV_PCIE15, 0x60, ELOG_WAKE_SOURCE_PME_PCIE15 }, + { RP_DEV_PCIE16, 0x60, ELOG_WAKE_SOURCE_PME_PCIE16 }, + { RP_DEV_PCIE17, 0x60, ELOG_WAKE_SOURCE_PME_PCIE17 }, + { RP_DEV_PCIE18, 0x60, ELOG_WAKE_SOURCE_PME_PCIE18 }, + { RP_DEV_PCIE19, 0x60, ELOG_WAKE_SOURCE_PME_PCIE19 }, + { RP_DEV_PCIE20, 0x60, ELOG_WAKE_SOURCE_PME_PCIE20 }, + { RP_DEV_PCIE21, 0x60, ELOG_WAKE_SOURCE_PME_PCIE21 }, + { RP_DEV_PCIE22, 0x60, ELOG_WAKE_SOURCE_PME_PCIE22 }, + { RP_DEV_PCIE23, 0x60, ELOG_WAKE_SOURCE_PME_PCIE23 }, + { RP_DEV_PCIE24, 0x60, ELOG_WAKE_SOURCE_PME_PCIE24 }, };
maxports = MIN(CONFIG_MAX_ROOT_PORTS, ARRAY_SIZE(pme_status_info));
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38750/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38750/2//COMMIT_MSG@16 PS2, Line 16: of the one that is standard for the Skylake SoC. This routine doesn doesnt
Hello Patrick Rudolph, Frans Hendriks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38750
to look at the new patch set (#3).
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
soc/intel/skylake: Remove misleading BUG: messages
The skylake pch_log_rp_wake_source function causes a "BUG: pch_log_rp_wake_source requests hidden...." message for each root port not listed in the device tree. This is caused by the fact that the function tries to read all root ports possible on the Skylake PCH. This is required because of a flaw in the PCH.
The issue is solved by using the 'pcidev_path_on_root' function instead of the one that is standard for the Skylake SoC. This routine doesn't display the "BUG" messages.
BUG=N/A TEST=build
Change-Id: I6bc04e48e97b0a29aef8aa050d3aad116cff1a14 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/skylake/elog.c 1 file changed, 59 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38750/3
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38750/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38750/2//COMMIT_MSG@16 PS2, Line 16: of the one that is standard for the Skylake SoC. This routine doesn
doesnt
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3: Code-Review-1
In the SMI handler, this might work because of the static PCI_DEV() value (it would access the correct PCI config space and likely run into the 0xffffffff check later).
In ramstage however, where the BUG is printed, it is an actual BUG! There are null-pointer dereferences, or worse, reads from the wrong devices (see comment in pci_dev_path_on_root_debug()). One can't simply check for NULL, because of the mess with these device macros (they are only pointers in the !__SIMPLE_DEVICE__ case). People started to write code common to simple and non-simple device cases, before that was supported by the coreboot infrastructure, now every- thing is chaos...
Generally, all soc code that does a #if defined(__SIMPLE_DEVICE__) can be considered broken. If you want the BUG (messages) to go away, please fix the code. IIRC, Kyösti just abandoned some efforts in that area (because it is too hard to fix while others add new copies of this broken code pattern). If you can help him out, maybe we can fix this topic for good.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
Patch Set 3: Code-Review-1
In the SMI handler, this might work because of the static PCI_DEV() value (it would access the correct PCI config space and likely run into the 0xffffffff check later).
In ramstage however, where the BUG is printed, it is an actual BUG! There are null-pointer dereferences, or worse, reads from the wrong devices (see comment in pci_dev_path_on_root_debug()). One can't simply check for NULL, because of the mess with these device macros (they are only pointers in the !__SIMPLE_DEVICE__ case). People started to write code common to simple and non-simple device cases, before that was supported by the coreboot infrastructure, now every- thing is chaos...
Generally, all soc code that does a #if defined(__SIMPLE_DEVICE__) can be considered broken. If you want the BUG (messages) to go away, please fix the code. IIRC, Kyösti just abandoned some efforts in that area (because it is too hard to fix while others add new copies of this broken code pattern). If you can help him out, maybe we can fix this topic for good.
I had a quick look at the pci_dev_path_on_root_debug(). From that I am not sure if I correctly understand the issue. From this I don't get the impression that pci_dev_path_on_root returns false non NULL results. Is that a correct assumption? Or is the entire device tree structure broken cause random errors?
What I am doing here is check if a possible root port is on the devicetree. If it is a non NULL value is returned is it isn't a NULL value is returned and the PCI device shouldn't be accessed. Previously this returned the bug message which is fine for standard accesses but shouldn't be done if you just want to check if a device is on the tree.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3: Code-Review+1
Sorry, I misread the code here. There is an `if (!dev)` already, that I missed and is technically wrong for the __SIMPLE_DEVICE__ case (but happens to work for the values given).
I won't block this, but don't like it as it adds more `#if defined __SIMPLE_DEVICE__` to clean up later.
Please mention in the commit message that null-pointers are already taken care of.
Patch Set 3: I had a quick look at the pci_dev_path_on_root_debug(). From that I am not sure if I correctly understand the issue. From this I don't get the impression that pci_dev_path_on_root returns false non NULL results. Is that a correct assumption? Or is the entire device tree structure broken cause random errors?
It falls back to dev_find_slot(0, ...) which does not traverse the device-tree topology but acts solely on the `secondary` bus field. This can give false positives, because all `secondary` fields are 0 by default and only adapted during PCI enumeration. If a device in the `devicetree.cb` isn't found during enumeration, the field stays at 0... pci_dev_path_on_root() doesn't have this problem.
What I am doing here is check if a possible root port is on the devicetree. If it is a non NULL value is returned is it isn't a NULL value is returned and the PCI device shouldn't be accessed. Previously this returned the bug message which is fine for standard accesses but shouldn't be done if you just want to check if a device is on the tree.
Sorry, missed that there was a null check already.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/38750/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38750/1//COMMIT_MSG@12 PS1, Line 12: This : is required because of a flaw in the PCH. I don't get this part, what is the flaw?
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
(1 comment)
Patch Set 3: Code-Review+1
Sorry, I misread the code here. There is an `if (!dev)` already, that I missed and is technically wrong for the __SIMPLE_DEVICE__ case (but happens to work for the values given).
I won't block this, but don't like it as it adds more `#if defined __SIMPLE_DEVICE__` to clean up later.
Please mention in the commit message that null-pointers are already taken care of.
Patch Set 3: I had a quick look at the pci_dev_path_on_root_debug(). From that I am not sure if I correctly understand the issue. From this I don't get the impression that pci_dev_path_on_root returns false non NULL results. Is that a correct assumption? Or is the entire device tree structure broken cause random errors?
It falls back to dev_find_slot(0, ...) which does not traverse the device-tree topology but acts solely on the `secondary` bus field. This can give false positives, because all `secondary` fields are 0 by default and only adapted during PCI enumeration. If a device in the `devicetree.cb` isn't found during enumeration, the field stays at 0... pci_dev_path_on_root() doesn't have this problem.
What I am doing here is check if a possible root port is on the devicetree. If it is a non NULL value is returned is it isn't a NULL value is returned and the PCI device shouldn't be accessed. Previously this returned the bug message which is fine for standard accesses but shouldn't be done if you just want to check if a device is on the tree.
Sorry, missed that there was a null check already.
For my understanding? How can I avoid the __SIMPLE_DEVICE__ stuff? The return is different in both cases (or is there an alternative I am not aware of?)
https://review.coreboot.org/c/coreboot/+/38750/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38750/1//COMMIT_MSG@12 PS1, Line 12: This : is required because of a flaw in the PCH.
I don't get this part, what is the flaw?
The PCIEXPWAK_STS bit doesn't get set so all rootports need to be scanned to determine if one of that triggered the wake. This is documented in pch_log_wake_source() just before the call to pch_log_rp_wake_source(). If you look at the other chipsets you will see that this is lacking there.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
(1 comment)
What I am doing here is check if a possible root port is on the devicetree. If it is a non NULL value is returned is it isn't a NULL value is returned and the PCI device shouldn't be accessed. Previously this returned the bug message which is fine for standard accesses but shouldn't be done if you just want to check if a device is on the tree.
Sorry, missed that there was a null check already.
For my understanding? How can I avoid the __SIMPLE_DEVICE__ stuff? The return is different in both cases (or is there an alternative I am not aware of?)
Kyösti worked on making the two cases compatible. I don't know the exact plan. While they are incompatible, one shouldn't use the same code for both cases. Beside the type conflict, they have very different environments. In ramstage, one is supposed to use the devicetree (at least after device enumeration, which is the case here, AFAICS). In SMM, there is no enumeration, hence all possible devices are tried.
If I wanted to implement this properly for ramstage, it would be a loop over bus 0:
for (dev = bus->children; dev; dev = dev->sibling) { if (dev->path.type != PCI) continue; if (PCI_SLOT(dev->path.pci.devfn) != PCH_DEV_SLOT_PCIE && ...) continue;
/* check this dev */ }
https://review.coreboot.org/c/coreboot/+/38750/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38750/1//COMMIT_MSG@12 PS1, Line 12: This : is required because of a flaw in the PCH.
The PCIEXPWAK_STS bit doesn't get set so all rootports need to be scanned to determine if one of tha […]
Right, but that's only about the missing `if` there. I guess the function would still be there, to identify where the event occurred (not if). Note that the other platforms have a TODO there.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
Actually, I'm not sure if the order of events matters much. If not, one could also add the check and event reporting to the .init function of the RP's pci driver. That would be the coreboot-native way, but loses the cohesion of a central `elog.c` (and most code sharing with the SMI handler).
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
oops.. I should have searched before starting a fix on this \o/ See cb:39089 for an alternative fix
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
Patch Set 3:
oops.. I should have searched before starting a fix on this \o/ See CB:39089 for an alternative fix
So which one should be merged?
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
oops.. I should have searched before starting a fix on this \o/ See CB:39089 for an alternative fix
So which one should be merged?
Lets go for the CB:39089. This is simpler. It doesn't check the device tree to skip root ports but that indeed shouldn't be a problem and the code is simplified a lot because of it.
Wim Vervoorn has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38750 )
Change subject: soc/intel/skylake: Remove misleading BUG: messages ......................................................................
Abandoned
Use CB:39089 instead.
-
Frans Hendriks (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Wim Vervoorn (Code Review)