Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
sb/intel/lynxpoint: Enable/disable AER via Kconfig
Semi-recent changes to the Linux kernel now enable AER for many devices for which it was previously disabled. This, coupled with the SB enabling AER for all PCIe devices, has resulted in a large amount of AER timeout errors in the kernel log for devices which do not support AER. To mitigate this, guard AER enablement via Kconfig, select it by default (as to maintain current default behavior), and allow boards which need to disable it to do so.
This implementation is identical to/copied from soc/intel/broadwell.
Test: build/boot google/beltino variants with AER disabled, verify dmesg log free of AER errors.
Change-Id: Ia03ef0d111335892c65122954c1248191ded7cb8 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/southbridge/intel/lynxpoint/Kconfig M src/southbridge/intel/lynxpoint/pcie.c 2 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/46133/1
diff --git a/src/southbridge/intel/lynxpoint/Kconfig b/src/southbridge/intel/lynxpoint/Kconfig index c104cbb..43f8bce 100644 --- a/src/southbridge/intel/lynxpoint/Kconfig +++ b/src/southbridge/intel/lynxpoint/Kconfig @@ -58,4 +58,8 @@ If you set this option to y, the USB ports will be routed to the XHCI controller during the finalize SMM callback.
+config PCIEXP_AER + bool + default y + endif diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 35ce5c47..42e002b 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -670,8 +670,13 @@ /* Set EOI forwarding disable. */ pci_or_config32(dev, 0xd4, 1 << 1);
- /* Set something involving advanced error reporting. */ - pci_update_config32(dev, 0x100, ~((1 << 20) - 1), 0x10001); + /* Set AER Extended Cap ID to 01h and Next Cap Pointer to 200h. */ + if (CONFIG(PCIEXP_AER)) + pci_update_config32(dev, 0x100, ~(1 << 29) & ~0xfffff, + (1 << 29) | 0x10001); + else + pci_update_config32(dev, 0x100, ~(1 << 29) & ~0xfffff, + (1 << 29));
if (is_lp) pci_or_config32(dev, 0x100, 1 << 29);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 675: ~(1 << 29) This doesn't matter because the or-value will always set it. I think it can be dropped for clarity.
Same on the other branch
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 676: (1 << 29) | 0x10001); nit: fits in 96 characters
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/Kconfig:
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 61: config PCIEXP_AER I saw the follow-ups and I wonder if it would be better to disable AER by default.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 675: ~(1 << 29)
This doesn't matter because the or-value will always set it. I think it can be dropped for clarity. […]
so just 'pci_update_config32(dev, 0x100, ~0xfffff, (1 << 29) | 0x10001);' then?
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 676: (1 << 29) | 0x10001);
nit: fits in 96 characters
Ack
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/Kconfig:
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 61: config PCIEXP_AER
I saw the follow-ups and I wonder if it would be better to disable AER by default.
I didn't want to change the default behavior as part of another change, but can do so separately if that's deemed a better solution vs disabling per board
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46133/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46133/1//COMMIT_MSG@9 PS1, Line 9: Semi-recent changes to the Linux kernel It’d be great, if you referenced the Linux commit.
https://review.coreboot.org/c/coreboot/+/46133/1//COMMIT_MSG@20 PS1, Line 20: dmesg log free of AER errors. Do these errors just spam the logs, or does it cause “real problems”?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 675: ~(1 << 29)
so just 'pci_update_config32(dev, 0x100, ~0xfffff, (1 << 29) | 0x10001);' then?
Yes
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/Kconfig:
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 61: config PCIEXP_AER
I didn't want to change the default behavior as part of another change, but can do so separately if […]
Sounds good.
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46133
to look at the new patch set (#2).
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
sb/intel/lynxpoint: Enable/disable AER via Kconfig
Semi-recent changes to the Linux kernel now enable AER for many devices for which it was previously disabled. This, coupled with the SB enabling AER for all PCIe devices, has resulted in a large amount of AER timeout errors in the kernel log for devices which do not support AER. To mitigate this, guard AER enablement via Kconfig, select it by default (as to maintain current default behavior), and allow boards which need to disable it to do so.
This implementation is identical to/copied from soc/intel/broadwell.
Test: build/boot google/beltino variants with AER disabled, verify dmesg log free of AER errors.
Change-Id: Ia03ef0d111335892c65122954c1248191ded7cb8 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/southbridge/intel/lynxpoint/Kconfig M src/southbridge/intel/lynxpoint/pcie.c 2 files changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/46133/2
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46133
to look at the new patch set (#3).
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
sb/intel/lynxpoint: Enable/disable AER via Kconfig
Several changes[1][2] to the Linux kernel now enable ASPM/AER for the rt8169 network driver, for which it was previously disabled. This, coupled with the southbridge enabling AER for all PCIe devices, has resulted in a large amount of AER timeout errors in the kernel log for boards which utilize the rt8169 for on-board Ethernet (e.g., google/beltino). While performance is not impacted, the errors do accumulate.
To mitigate this, guard AER enablement via Kconfig, select it by default (as to maintain current default behavior), and allow boards which need to disable it to do so (implemented in subsequent commits).
This implementation is derived from that in soc/intel/broadwell.
Test: build/boot google/beltino variants with AER disabled (CB:46136), verify dmesg log free of AER timeout errors.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... [2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
Change-Id: Ia03ef0d111335892c65122954c1248191ded7cb8 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/southbridge/intel/lynxpoint/Kconfig M src/southbridge/intel/lynxpoint/pcie.c 2 files changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/46133/3
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46133/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46133/1//COMMIT_MSG@9 PS1, Line 9: Semi-recent changes to the Linux kernel
It’d be great, if you referenced the Linux commit.
Done
https://review.coreboot.org/c/coreboot/+/46133/1//COMMIT_MSG@20 PS1, Line 20: dmesg log free of AER errors.
Do these errors just spam the logs, or does it cause “real problems”?
Done
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 675: ~(1 << 29)
Yes
Done
https://review.coreboot.org/c/coreboot/+/46133/1/src/southbridge/intel/lynxp... PS1, Line 676: (1 << 29) | 0x10001);
Ack
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46133 )
Change subject: sb/intel/lynxpoint: Enable/disable AER via Kconfig ......................................................................
sb/intel/lynxpoint: Enable/disable AER via Kconfig
Several changes[1][2] to the Linux kernel now enable ASPM/AER for the rt8169 network driver, for which it was previously disabled. This, coupled with the southbridge enabling AER for all PCIe devices, has resulted in a large amount of AER timeout errors in the kernel log for boards which utilize the rt8169 for on-board Ethernet (e.g., google/beltino). While performance is not impacted, the errors do accumulate.
To mitigate this, guard AER enablement via Kconfig, select it by default (as to maintain current default behavior), and allow boards which need to disable it to do so (implemented in subsequent commits).
This implementation is derived from that in soc/intel/broadwell.
Test: build/boot google/beltino variants with AER disabled (CB:46136), verify dmesg log free of AER timeout errors.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... [2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=...
Change-Id: Ia03ef0d111335892c65122954c1248191ded7cb8 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46133 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/intel/lynxpoint/Kconfig M src/southbridge/intel/lynxpoint/pcie.c 2 files changed, 9 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/southbridge/intel/lynxpoint/Kconfig b/src/southbridge/intel/lynxpoint/Kconfig index c104cbb..43f8bce 100644 --- a/src/southbridge/intel/lynxpoint/Kconfig +++ b/src/southbridge/intel/lynxpoint/Kconfig @@ -58,4 +58,8 @@ If you set this option to y, the USB ports will be routed to the XHCI controller during the finalize SMM callback.
+config PCIEXP_AER + bool + default y + endif diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 35ce5c47..077dcd6 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -670,8 +670,11 @@ /* Set EOI forwarding disable. */ pci_or_config32(dev, 0xd4, 1 << 1);
- /* Set something involving advanced error reporting. */ - pci_update_config32(dev, 0x100, ~((1 << 20) - 1), 0x10001); + /* Set AER Extended Cap ID to 01h and Next Cap Pointer to 200h. */ + if (CONFIG(PCIEXP_AER)) + pci_update_config32(dev, 0x100, ~0xfffff, (1 << 29) | 0x10001); + else + pci_update_config32(dev, 0x100, ~0xfffff, (1 << 29));
if (is_lp) pci_or_config32(dev, 0x100, 1 << 29);
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)