Change in coreboot[master]: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions
Name of user not set #1002895 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions
The NEC uPD720200A USB 3.0 controller on the T420s is actually connected to PCIe root port #5 on the PCH, not #7. Enabled RP#5, disabled RP#7 and updated comments accordingly.
Change-Id: I21ac72fd5632e552bdcdbd573cf92b433ed545ff Signed-off-by: Jake Mannens jakem_5@hotmail.com --- M src/mainboard/lenovo/t420s/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/40281/1
diff --git a/src/mainboard/lenovo/t420s/devicetree.cb b/src/mainboard/lenovo/t420s/devicetree.cb old mode 100644 new mode 100755 index 2fde40f..1f794c1 --- a/src/mainboard/lenovo/t420s/devicetree.cb +++ b/src/mainboard/lenovo/t420s/devicetree.cb @@ -84,9 +84,9 @@ device pci 1c.3 on smbios_slot_desc "7" "3" "ExpressCard Slot" "8" end # PCIe Port #4 ExpressCard - device pci 1c.4 off end # PCIe Port #5 + device pci 1c.4 on end # PCIe Port #5 NEC Corporation uPD720200A USB 3.0 Host Controller device pci 1c.5 off end # PCIe Port #6 Intel Gigabit Ethernet PHY (not PCIe) - device pci 1c.6 on end # PCIe Port #7 NEC Corporation uPD720200A USB 3.0 Host Controller + device pci 1c.6 off end # PCIe Port #7 device pci 1c.7 off end # PCIe Port #8 device pci 1d.0 on end # USB Enhanced Host Controller #1 device pci 1e.0 off end # PCI bridge
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 1:
(4 comments)
Very nice. Welcome to coreboot!
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@7 PS1, Line 7: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions I’d use:
mb/lenovo/t420s/devicetree: Enable USB 3.0 controller
Fix PCIe port definitions to …
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@9 PS1, Line 9: The NEC uPD720200A USB 3.0 controller on the T420s is actually connected to Please wrap lines after 72/75 characters.
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@10 PS1, Line 10: Enabled RP#5, disabled RP#7 and : updated comments accordingly. So, enable RP#5, disable RP#7, and update comments accordingly.
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@12 PS1, Line 12: Test=USB 3.0 controller shows up in lspci.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 1:
It looks like you changed the file permissions. From Jenkins:
File src/mainboard/lenovo/t420s/devicetree.cb has one or more executable bits set in the file permissions.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 1:
The mailing list [1] and IRC discussions also said something about the FireWire device. Is that fixed too?
[1]: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/2ABSL... "ThinkPad T420 and FireWire chip on Linux"
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 1: Code-Review+1
Patch Set 1:
The mailing list [1] and IRC discussions also said something about the FireWire device. Is that fixed too?
T420 and T420S are different boards and use different code, so this thread about the T420 doesn't apply to the T420S.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 1:
Patch Set 1:
It looks like you changed the file permissions. From Jenkins:
File src/mainboard/lenovo/t420s/devicetree.cb has one or more executable bits set in the file permissions.
That usually happens when the coreboot repo is on a filesystem that doesn't support all the permission bits (e. g. Windows machines)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@7 PS1, Line 7: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions
I’d use: […]
Uh, this change doesn't directly enable USB 3.0. It fixes the PCIe root port settings and comments. As a result, the USB 3.0 controller behind a previously-disabled root port is now detected and works.
So I would keep the original phrase, but still shorten the path:
mb/lenovo/t420s/devicetree: Fix PCIe port definitions
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@10 PS1, Line 10: Enabled RP#5, disabled RP#7 and : updated comments accordingly.
So, enable RP#5, disable RP#7, and update comments accordingly.
(To make it clear, the suggestion is to use "timeless present" for the changes that the commit does)
Hello build bot (Jenkins), Angel Pons, Alexander Couzens, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40281
to look at the new patch set (#2).
Change subject: mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions
The NEC uPD720200A USB 3.0 controller on the T420s is actually connected to PCIe root port #5 on the PCH, not #7. Enable RP#5, disable RP#7 and update comments accordingly. Test=USB 3.0 controller shows in `lspci`
Change-Id: I21ac72fd5632e552bdcdbd573cf92b433ed545ff Signed-off-by: Jake Mannens jakem_5@hotmail.com --- M src/mainboard/lenovo/t420s/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/40281/2
Name of user not set #1002895 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1:
It looks like you changed the file permissions. From Jenkins:
File src/mainboard/lenovo/t420s/devicetree.cb has one or more executable bits set in the file permissions.That usually happens when the coreboot repo is on a filesystem that doesn't support all the permission bits (e. g. Windows machines)
I actually used two Linux boxes, but copied the file over SMB at one stage and didn't notice the change, though I normally check :)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 2: Code-Review+2
(4 comments)
Patch Set 2:
Patch Set 1:
Patch Set 1:
It looks like you changed the file permissions. From Jenkins:
File src/mainboard/lenovo/t420s/devicetree.cb has one or more executable bits set in the file permissions.That usually happens when the coreboot repo is on a filesystem that doesn't support all the permission bits (e. g. Windows machines)
I actually used two Linux boxes, but copied the file over SMB at one stage and didn't notice the change, though I normally check :)
Ah yes, SMB does that 😄
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@7 PS1, Line 7: src/mainboard/lenovo/t420s/devicetree.cb: Fix PCIe port definitions
Uh, this change doesn't directly enable USB 3.0. It fixes the PCIe root port settings and comments. […]
Done
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@9 PS1, Line 9: The NEC uPD720200A USB 3.0 controller on the T420s is actually connected to
Please wrap lines after 72/75 characters.
Done (fwiw, the limit is 72)
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@10 PS1, Line 10: Enabled RP#5, disabled RP#7 and : updated comments accordingly.
(To make it clear, the suggestion is to use "timeless present" for the changes that the commit does)
Done
https://review.coreboot.org/c/coreboot/+/40281/1//COMMIT_MSG@12 PS1, Line 12:
Test=USB 3.0 controller shows up in lspci.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions
The NEC uPD720200A USB 3.0 controller on the T420s is actually connected to PCIe root port #5 on the PCH, not #7. Enable RP#5, disable RP#7 and update comments accordingly. Test=USB 3.0 controller shows in `lspci`
Change-Id: I21ac72fd5632e552bdcdbd573cf92b433ed545ff Signed-off-by: Jake Mannens jakem_5@hotmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40281 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/mainboard/lenovo/t420s/devicetree.cb 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/lenovo/t420s/devicetree.cb b/src/mainboard/lenovo/t420s/devicetree.cb index 2fde40f..1f794c1 100644 --- a/src/mainboard/lenovo/t420s/devicetree.cb +++ b/src/mainboard/lenovo/t420s/devicetree.cb @@ -84,9 +84,9 @@ device pci 1c.3 on smbios_slot_desc "7" "3" "ExpressCard Slot" "8" end # PCIe Port #4 ExpressCard - device pci 1c.4 off end # PCIe Port #5 + device pci 1c.4 on end # PCIe Port #5 NEC Corporation uPD720200A USB 3.0 Host Controller device pci 1c.5 off end # PCIe Port #6 Intel Gigabit Ethernet PHY (not PCIe) - device pci 1c.6 on end # PCIe Port #7 NEC Corporation uPD720200A USB 3.0 Host Controller + device pci 1c.6 off end # PCIe Port #7 device pci 1c.7 off end # PCIe Port #8 device pci 1d.0 on end # USB Enhanced Host Controller #1 device pci 1e.0 off end # PCI bridge
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40281 )
Change subject: mb/lenovo/t420s/devicetree.cb: Fix PCIe port definitions ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2254 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2253 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2252
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Name of user not set (Code Review)
-
Paul Menzel (Code Review)