Change in coreboot[master]: sb/intel/lynxpoint: Consider root ports being disabled by strap
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
sb/intel/lynxpoint: Consider root ports being disabled by strap
PCIe RPC (Root Port Configuration) straps will force-disable some root port functions if some root ports have a width greater than x1. In two cases, this affects the last function. The PCIe init code will never finish configuring the root ports if that is the case: it assumes that the last function will eventually run through the code, but it doesn't.
If PCIe initialization does not complete, pressing the power button will not power off the board, unless it is held for about five seconds. Also, Windows 10 will show a BSOD about MACHINE CHECK EXCEPTION, and lock up instead of rebooting. Depending on the microcode version, the BSOD may not be visible. This happens even when the root port is not populated.
Use the strap fuse configuration value to know which configuration the PCH is strapped to. If needed, update the number of ports accordingly. In addition, print the updated value to ease debugging PCIe init code.
Tested on Asrock B85M Pro4, PCIe initialization completes successfully.
Change-Id: Id6da3a1f45467f00002a5ed41df8650f4a74eeba Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/pcie.c 1 file changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/44155/1
diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index d2950e7..5323e74 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -100,6 +100,36 @@ } }
+static void update_num_ports(void) +{ + /* + * The last visible function depends on the strapped root port width: + * + * +-----+----+----+----+----+ + * | RPC | #5 | #6 | #7 | #8 | + * +-----+----+----+----+----+ + * | 0 | x1 | x1 | x1 | x1 | + * | 1 | x2 | | x1 | x1 | + * | 2 | x2 | | x2 | | + * | 3 | x4 | | | | + * +-----+----+----+----+----+ + */ + switch ((rpc.strpfusecfg2 >> 14) & 0x3) { + case 0: + case 1: + break; + case 2: + rpc.num_ports = MIN(rpc.num_ports, 7); + break; + case 3: + rpc.num_ports = MIN(rpc.num_ports, 5); + break; + } + + printk(BIOS_DEBUG, "Adjusted number of PCIe root ports to %d as per strpfusecfg2\n", + rpc.num_ports); +} + static void root_port_init_config(struct device *dev) { int rp; @@ -137,6 +167,10 @@ case 5: rpc.strpfusecfg2 = pci_read_config32(dev, 0xfc); rpc.b0d28f4_32c = pci_read_config32(dev, 0x32c); + + if (!pch_is_lp()) + update_num_ports(); + break; case 6: rpc.b0d28f5_32c = pci_read_config32(dev, 0x32c);
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Nice work. Thank you. Maybe also something for the release notes.
https://review.coreboot.org/c/coreboot/+/44155/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44155/1//COMMIT_MSG@18 PS1, Line 18: Depending on the microcode version As you documented that elsewhere, could you please reference it or put the “test table” in here?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44155/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44155/1//COMMIT_MSG@18 PS1, Line 18: Depending on the microcode version
As you documented that elsewhere, could you please reference it or put the “test table” in here?
I only did a quick test, and even then it could be different in newer Windows versions. The most obvious sign that there's a problem is the power button not behaving properly.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 2:
Patch Set 1: Code-Review+1
(1 comment)
Nice work. Thank you. Maybe also something for the release notes.
Now that you mention it, there's several things that should go into the release notes. One of these days I'll take a look at the commit history and extract these.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... PS2, Line 108: * +-----+----+----+----+----+ : * | RPC | #5 | #6 | #7 | #8 | : * +-----+----+----+----+----+ : * | 0 | x1 | x1 | x1 | x1 | : * | 1 | x2 | | x1 | x1 | : * | 2 | x2 | | x2 | | : * | 3 | x4 | | | | : * +-----+----+----+----+----+ The datasheet (328904-003) only states something about port 1-4 about this. Maybe add a comment that this applies to port 5-8 too?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... PS2, Line 108: * +-----+----+----+----+----+ : * | RPC | #5 | #6 | #7 | #8 | : * +-----+----+----+----+----+ : * | 0 | x1 | x1 | x1 | x1 | : * | 1 | x2 | | x1 | x1 | : * | 2 | x2 | | x2 | | : * | 3 | x4 | | | | : * +-----+----+----+----+----+
The datasheet (328904-003) only states something about port 1-4 about this. […]
Are you by chance referring to this part of the datasheet? https://imgur.com/QrElUnO.png
It mentions both ports 1-4 and ports 5-8, it's just clumped together though.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... PS2, Line 108: * +-----+----+----+----+----+ : * | RPC | #5 | #6 | #7 | #8 | : * +-----+----+----+----+----+ : * | 0 | x1 | x1 | x1 | x1 | : * | 1 | x2 | | x1 | x1 | : * | 2 | x2 | | x2 | | : * | 3 | x4 | | | | : * +-----+----+----+----+----+
Are you by chance referring to this part of the datasheet? https://imgur.com/QrElUnO.png
It mentions both ports 1-4 and ports 5-8, it's just clumped together though.
No PECR4 (0xfc) bits 14-15.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... PS2, Line 108: * +-----+----+----+----+----+ : * | RPC | #5 | #6 | #7 | #8 | : * +-----+----+----+----+----+ : * | 0 | x1 | x1 | x1 | x1 | : * | 1 | x2 | | x1 | x1 | : * | 2 | x2 | | x2 | | : * | 3 | x4 | | | | : * +-----+----+----+----+----+
Are you by chance referring to this part of the datasheet? https://imgur.com/QrElUnO.png […]
After some deliberation on IRC, it was decided that I should explain this is what existing code assumes and that said assumptions have been propagated.
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Paul Menzel, Arthur Heymans, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44155
to look at the new patch set (#3).
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
sb/intel/lynxpoint: Consider root ports being disabled by strap
PCIe RPC (Root Port Configuration) straps will force-disable some root port functions if some root ports have a width greater than x1. In two cases, this affects the last function. The PCIe init code will never finish configuring the root ports if that is the case: it assumes that the last function will eventually run through the code, but it doesn't.
If PCIe initialization does not complete, pressing the power button will not power off the board, unless it is held for about five seconds. Also, Windows 10 will show a BSOD about MACHINE CHECK EXCEPTION, and lock up instead of rebooting. Depending on the microcode version, the BSOD may not be visible. This happens even when the root port is not populated.
Use the strap fuse configuration value to know which configuration the PCH is strapped to. If needed, update the number of ports accordingly. In addition, print the updated value to ease debugging PCIe init code.
Existing code in coreboot disagrees with public documentation about the root port width straps. Assume existing code is correct and document these assumptions in a table, as an explanation for the added code.
Tested on Asrock B85M Pro4, PCIe initialization completes successfully.
Change-Id: Id6da3a1f45467f00002a5ed41df8650f4a74eeba Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/pcie.c 1 file changed, 36 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/44155/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pcie.c:
https://review.coreboot.org/c/coreboot/+/44155/2/src/southbridge/intel/lynxp... PS2, Line 108: * +-----+----+----+----+----+ : * | RPC | #5 | #6 | #7 | #8 | : * +-----+----+----+----+----+ : * | 0 | x1 | x1 | x1 | x1 | : * | 1 | x2 | | x1 | x1 | : * | 2 | x2 | | x2 | | : * | 3 | x4 | | | | : * +-----+----+----+----+----+
After some deliberation on IRC, it was decided that I should explain this is what existing code assu […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44155/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44155/1//COMMIT_MSG@18 PS1, Line 18: Depending on the microcode version
I only did a quick test, and even then it could be different in newer Windows versions. […]
Ack
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 3: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
Patch Set 6: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44155 )
Change subject: sb/intel/lynxpoint: Consider root ports being disabled by strap ......................................................................
sb/intel/lynxpoint: Consider root ports being disabled by strap
PCIe RPC (Root Port Configuration) straps will force-disable some root port functions if some root ports have a width greater than x1. In two cases, this affects the last function. The PCIe init code will never finish configuring the root ports if that is the case: it assumes that the last function will eventually run through the code, but it doesn't.
If PCIe initialization does not complete, pressing the power button will not power off the board, unless it is held for about five seconds. Also, Windows 10 will show a BSOD about MACHINE CHECK EXCEPTION, and lock up instead of rebooting. Depending on the microcode version, the BSOD may not be visible. This happens even when the root port is not populated.
Use the strap fuse configuration value to know which configuration the PCH is strapped to. If needed, update the number of ports accordingly. In addition, print the updated value to ease debugging PCIe init code.
Existing code in coreboot disagrees with public documentation about the root port width straps. Assume existing code is correct and document these assumptions in a table, as an explanation for the added code.
Tested on Asrock B85M Pro4, PCIe initialization completes successfully.
Change-Id: Id6da3a1f45467f00002a5ed41df8650f4a74eeba Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44155 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/intel/lynxpoint/pcie.c 1 file changed, 36 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index d2950e7..4e94f12 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -100,6 +100,38 @@ } }
+static void update_num_ports(void) +{ + /* + * According to existing code in 'root_port_check_disable()', which does + * not agree with the confusing information on the datasheets, the last + * visible function depends on the strapped root port width as follows: + * + * +-----+----+----+----+----+ + * | RPC | #5 | #6 | #7 | #8 | + * +-----+----+----+----+----+ + * | 0 | x1 | x1 | x1 | x1 | + * | 1 | x2 | | x1 | x1 | + * | 2 | x2 | | x2 | | + * | 3 | x4 | | | | + * +-----+----+----+----+----+ + */ + switch ((rpc.strpfusecfg2 >> 14) & 0x3) { + case 0: + case 1: + break; + case 2: + rpc.num_ports = MIN(rpc.num_ports, 7); + break; + case 3: + rpc.num_ports = MIN(rpc.num_ports, 5); + break; + } + + printk(BIOS_DEBUG, "Adjusted number of PCIe root ports to %d as per strpfusecfg2\n", + rpc.num_ports); +} + static void root_port_init_config(struct device *dev) { int rp; @@ -137,6 +169,10 @@ case 5: rpc.strpfusecfg2 = pci_read_config32(dev, 0xfc); rpc.b0d28f4_32c = pci_read_config32(dev, 0x32c); + + if (!pch_is_lp()) + update_num_ports(); + break; case 6: rpc.b0d28f5_32c = pci_read_config32(dev, 0x32c);
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Paul Menzel (Code Review)