Change in coreboot[master]: libpayload/drivers/usb/xhci: Allowing xHCI v1.2 in libpayload
Dossym Nurmukhanov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allowing xHCI v1.2 in libpayload ......................................................................
libpayload/drivers/usb/xhci: Allowing xHCI v1.2 in libpayload
The latest Intel FSP requires xHCI v1.2 support in libpayload, so updated the supported protocols. This was sufficient for the tests that I performed.
BUG=b:155315876 TEST=booting from multiple USB sticks with the latest Intel FSP that requires xHCI v1.2
Change-Id: I236fed9beef86ff5e1bf7962d882fdae5817a1ff Signed-off-by: Dossym Nurmukhanov dossym@google.com --- M payloads/libpayload/drivers/usb/xhci.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/41039/1
diff --git a/payloads/libpayload/drivers/usb/xhci.c b/payloads/libpayload/drivers/usb/xhci.c index 08a81ef..53dd782 100644 --- a/payloads/libpayload/drivers/usb/xhci.c +++ b/payloads/libpayload/drivers/usb/xhci.c @@ -198,7 +198,7 @@ xhci_debug("hciversion: %"PRIx8".%"PRIx8"\n", CAP_GET(CAPVER_HI, xhci->capreg), CAP_GET(CAPVER_LO, xhci->capreg)); if ((CAP_GET(CAPVER, xhci->capreg) < 0x96) || - (CAP_GET(CAPVER, xhci->capreg) > 0x110)) { + (CAP_GET(CAPVER, xhci->capreg) > 0x120)) { xhci_debug("Unsupported xHCI version\n"); goto _free_xhci; }
Dossym Nurmukhanov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allowing xHCI v1.2 in libpayload ......................................................................
Patch Set 1:
This change is ready for review.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allowing xHCI v1.2 in libpayload ......................................................................
Patch Set 1: Code-Review+2
There's a pretty good change log at the end of the XHCI 1.2 spec (Appendix I). From a quick look I didn't see anything that should affect our code. Then again switching to 1.1 brought us CL:291681 so who knows what weird issues this one will bring...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allowing xHCI v1.2 in libpayload ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/41039/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41039/1//COMMIT_MSG@7 PS1, Line 7: Allowing Allow
https://review.coreboot.org/c/coreboot/+/41039/1//COMMIT_MSG@10 PS1, Line 10: updated update
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Angel Pons, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41039
to look at the new patch set (#2).
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload
The latest Intel FSP requires xHCI v1.2 support in libpayload, so update the supported protocols. This was sufficient for the tests that I performed.
BUG=b:155315876 TEST=booting from multiple USB sticks with the latest Intel FSP that requires xHCI v1.2
Change-Id: I236fed9beef86ff5e1bf7962d882fdae5817a1ff Signed-off-by: Dossym Nurmukhanov dossym@google.com --- M payloads/libpayload/drivers/usb/xhci.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/41039/2
Dossym Nurmukhanov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 2:
(2 comments)
Thanks!
https://review.coreboot.org/c/coreboot/+/41039/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41039/1//COMMIT_MSG@7 PS1, Line 7: Allowing
Allow
Done
https://review.coreboot.org/c/coreboot/+/41039/1//COMMIT_MSG@10 PS1, Line 10: updated
update
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG@9 PS2, Line 9: Intel FSP requires xHCI v1.2 support in libpayload So the issue is that FSP makes the chipset advertise 1.2 support?
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG@10 PS2, Line 10: This was sufficient for the : tests that I performed. I think we now have three people who looked through Appendix I of the xhci 1.2 spec without noticing anything that requires more changes (you, I and Julius)? May be worth mentioning, too.
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG@15 PS2, Line 15: requires advertises?
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Angel Pons, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41039
to look at the new patch set (#3).
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload
The latest Intel FSP advertises xHCI v1.2 chipset support, so update libpayload to include that version. No critical changes were identified in review of the xHCI v1.2 spec, and booting from USB works with the included change as expected.
BUG=b:155315876 TEST=booting from multiple USB sticks/hubs with the latest Intel FSP that advertises xHCI v1.2
Change-Id: I236fed9beef86ff5e1bf7962d882fdae5817a1ff Signed-off-by: Dossym Nurmukhanov dossym@google.com --- M payloads/libpayload/drivers/usb/xhci.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/41039/3
Dossym Nurmukhanov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 3:
(3 comments)
Appreciate the comments.
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG@9 PS2, Line 9: Intel FSP requires xHCI v1.2 support in libpayload
So the issue is that FSP makes the chipset advertise 1. […]
Correct
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG@10 PS2, Line 10: This was sufficient for the : tests that I performed.
I think we now have three people who looked through Appendix I of the xhci 1. […]
Updated the comment. Would "Reviewed-by" in the CL be enough to identify the reviewers or you think I should explicit call them out in the description?
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG@15 PS2, Line 15: requires
advertises?
Done
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 3: Code-Review+2
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41039/2//COMMIT_MSG@9 PS2, Line 9: Intel FSP requires xHCI v1.2 support in libpayload
Correct
Confirmed that new FSP set version info based on USB controller IP version.
Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload
The latest Intel FSP advertises xHCI v1.2 chipset support, so update libpayload to include that version. No critical changes were identified in review of the xHCI v1.2 spec, and booting from USB works with the included change as expected.
BUG=b:155315876 TEST=booting from multiple USB sticks/hubs with the latest Intel FSP that advertises xHCI v1.2
Change-Id: I236fed9beef86ff5e1bf7962d882fdae5817a1ff Signed-off-by: Dossym Nurmukhanov dossym@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41039 Reviewed-by: Wonkyu Kim wonkyu.kim@intel.com Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M payloads/libpayload/drivers/usb/xhci.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Wonkyu Kim: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/usb/xhci.c b/payloads/libpayload/drivers/usb/xhci.c index 08a81ef..53dd782 100644 --- a/payloads/libpayload/drivers/usb/xhci.c +++ b/payloads/libpayload/drivers/usb/xhci.c @@ -198,7 +198,7 @@ xhci_debug("hciversion: %"PRIx8".%"PRIx8"\n", CAP_GET(CAPVER_HI, xhci->capreg), CAP_GET(CAPVER_LO, xhci->capreg)); if ((CAP_GET(CAPVER, xhci->capreg) < 0x96) || - (CAP_GET(CAPVER, xhci->capreg) > 0x110)) { + (CAP_GET(CAPVER, xhci->capreg) > 0x120)) { xhci_debug("Unsupported xHCI version\n"); goto _free_xhci; }
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3203 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3202 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3201 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3200
Please note: This test is under development and might not be accurate at all!
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 4:
24h, not 2.4h. Would have been nice to have the changes that might affect libpayload mentioned in the commit message.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 4: Code-Review+2
Patch Set 4:
24h, not 2.4h. Would have been nice to have the changes that might affect libpayload mentioned in the commit message.
Oh, yeah, sorry. The third +2 was implicit (adding it for reference now), and this patch was fairly trivial.
Let non-trivial patches sit in a review state for at least 24 hours before submission. Remember that there are coreboot developers in timezones all over the world, and everyone should have a chance to contribute. Trivial patches would be things like whitespace changes or spelling fixes, in general those that don’t impact the final binary output. The 24-hour period would start at submission, and would be restarted at any update which significantly changes any part of the patch. Patches can be ‘Fast-tracked’ and submitted in under 24 hours with the agreement of at least 3 +2 votes.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 4:
Patch Set 4: Code-Review+2
Patch Set 4:
24h, not 2.4h. Would have been nice to have the changes that might affect libpayload mentioned in the commit message.
Oh, yeah, sorry. The third +2 was implicit (adding it for reference now), and this patch was fairly trivial.
Let non-trivial patches sit in a review state for at least 24 hours before submission. Remember that there are coreboot developers in timezones all over the world, and everyone should have a chance to contribute. Trivial patches would be things like whitespace changes or spelling fixes, in general those that don’t impact the final binary output. The 24-hour period would start at submission, and would be restarted at any update which significantly changes any part of the patch. Patches can be ‘Fast-tracked’ and submitted in under 24 hours with the agreement of at least 3 +2 votes.
I think we should keep those fast track submissions for fixes to build breakage and the like. I'm also not sure if this qualifies as "trivial", given the example Julius dug up earlier about how the change to signal xHCI 1.1 support needed fixing in the code.
OTOH mistakes happen, this commit is easy to revert, and I'm not sure how big the impact really is, given that it requires the device to advertise xHCI 1.2 support first. Dossym, which platforms are affected by this?
Dossym Nurmukhanov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41039 )
Change subject: libpayload/drivers/usb/xhci: Allow xHCI v1.2 in libpayload ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4: Code-Review+2
Patch Set 4:
24h, not 2.4h. Would have been nice to have the changes that might affect libpayload mentioned in the commit message.
Oh, yeah, sorry. The third +2 was implicit (adding it for reference now), and this patch was fairly trivial.
Let non-trivial patches sit in a review state for at least 24 hours before submission. Remember that there are coreboot developers in timezones all over the world, and everyone should have a chance to contribute. Trivial patches would be things like whitespace changes or spelling fixes, in general those that don’t impact the final binary output. The 24-hour period would start at submission, and would be restarted at any update which significantly changes any part of the patch. Patches can be ‘Fast-tracked’ and submitted in under 24 hours with the agreement of at least 3 +2 votes.
I think we should keep those fast track submissions for fixes to build breakage and the like. I'm also not sure if this qualifies as "trivial", given the example Julius dug up earlier about how the change to signal xHCI 1.1 support needed fixing in the code.
OTOH mistakes happen, this commit is easy to revert, and I'm not sure how big the impact really is, given that it requires the device to advertise xHCI 1.2 support first. Dossym, which platforms are affected by this?
This change was for Tiger Lake platforms
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Dossym Nurmukhanov (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)
-
Wonkyu Kim (Code Review)