EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
libpayload/usb: add USB 3.1 GEN2 support
USB 3.1 GEN2 report speed type 4, add into speed enum.
BUG=b:139787920 BRANCH=N/A TEST=Build libpayload and depthcharge on sarien and boot with USB GEN2 HUB with USB disk.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia0ef12b2f0d91bf0d0db766bbc9019de1614a4f4 --- M payloads/libpayload/drivers/usb/usb.c M payloads/libpayload/drivers/usb/usbhub.c M payloads/libpayload/drivers/usb/xhci_devconf.c M payloads/libpayload/include/usb/usb.h 4 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/35023/1
diff --git a/payloads/libpayload/drivers/usb/usb.c b/payloads/libpayload/drivers/usb/usb.c index 9c9854d..51b84f1 100644 --- a/payloads/libpayload/drivers/usb/usb.c +++ b/payloads/libpayload/drivers/usb/usb.c @@ -265,6 +265,7 @@ } return bMaxPacketSize0; case SUPER_SPEED: + case SUPER_SPEED_PLUS: if (bMaxPacketSize0 != 9) { usb_debug("Invalid MPS0: 0x%02x\n", bMaxPacketSize0); bMaxPacketSize0 = 9; @@ -284,6 +285,7 @@ case HIGH_SPEED: return 64; case SUPER_SPEED: + case SUPER_SPEED_PLUS: default: return 512; } @@ -319,6 +321,7 @@ return LOG2(bInterval); } case SUPER_SPEED: + case SUPER_SPEED_PLUS: switch (type) { case ISOCHRONOUS: case INTERRUPT: return bInterval - 1; diff --git a/payloads/libpayload/drivers/usb/usbhub.c b/payloads/libpayload/drivers/usb/usbhub.c index 340e47a..0482da8 100644 --- a/payloads/libpayload/drivers/usb/usbhub.c +++ b/payloads/libpayload/drivers/usb/usbhub.c @@ -98,6 +98,8 @@ /* SuperSpeed hubs can only have SuperSpeed devices. */ if (dev->speed == SUPER_SPEED) return SUPER_SPEED; + if (dev->speed == SUPER_SPEED_PLUS) + return SUPER_SPEED_PLUS;
/*[bit] 10 9 (USB 2.0 port status word) * 0 0 full speed @@ -176,7 +178,7 @@ void usb_hub_init(usbdev_t *const dev) { - int type = dev->speed == SUPER_SPEED ? 0x2a : 0x29; /* similar enough */ + int type = dev->speed > SUPER_SPEED ? 0x2a : 0x29; /* similar enough */ hub_descriptor_t desc; /* won't fit the whole thing, we don't care */ if (get_descriptor(dev, gen_bmRequestType(device_to_host, class_type, dev_recp), type, 0, &desc, sizeof(desc)) != sizeof(desc)) { @@ -185,7 +187,7 @@ return; }
- if (dev->speed == SUPER_SPEED) + if (dev->speed > SUPER_SPEED) usb_hub_set_hub_depth(dev); if (generic_hub_init(dev, desc.bNbrPorts, &usb_hub_ops) < 0) return; diff --git a/payloads/libpayload/drivers/usb/xhci_devconf.c b/payloads/libpayload/drivers/usb/xhci_devconf.c index 99e3037..b53c8ef 100644 --- a/payloads/libpayload/drivers/usb/xhci_devconf.c +++ b/payloads/libpayload/drivers/usb/xhci_devconf.c @@ -267,7 +267,7 @@ static int xhci_finish_hub_config(usbdev_t *const dev, inputctx_t *const ic) { - int type = dev->speed == SUPER_SPEED ? 0x2a : 0x29; /* similar enough */ + int type = dev->speed > SUPER_SPEED ? 0x2a : 0x29; /* similar enough */ hub_descriptor_t desc;
if (get_descriptor(dev, gen_bmRequestType(device_to_host, class_type, diff --git a/payloads/libpayload/include/usb/usb.h b/payloads/libpayload/include/usb/usb.h index db7ec57..ee767ad 100644 --- a/payloads/libpayload/include/usb/usb.h +++ b/payloads/libpayload/include/usb/usb.h @@ -210,6 +210,7 @@ LOW_SPEED = 1, HIGH_SPEED = 2, SUPER_SPEED = 3, + SUPER_SPEED_PLUS = 4, } usb_speed;
struct usbdev {
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 1:
Please help review this. I am not sure any else need to modify as well. I suppose GEN2 use the same setting as GEN1.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usbhub.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 181: > This should either be >= or better to add a helper: is_usb_hub_enhanced_ss() which checks if speed is SUPER_SPEED or SUPER_SPEED_PLUS and returns true.
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 190: > Same comment as above.
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/xhci_devconf.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 270: > same comment as other file.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usbhub.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 181: >
This should either be >= or better to add a helper: is_usb_hub_enhanced_ss() which checks if speed i […]
SGTM, update the patch soon.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 663: speeds This will have to be updated too.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 663: speeds
This will have to be updated too.
This is quite interesting. When I debugging, I am not see this message. I will check this as well.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usbhub.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 99: if (dev->speed == SUPER_SPEED) : return SUPER_SPEED; : if (dev->speed == SUPER_SPEED_PLUS) : return SUPER_SPEED_PLUS; Maybe if (dev->speed >= SUPER_SPEED) return dev->speed; ?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usbhub.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 99: if (dev->speed == SUPER_SPEED) : return SUPER_SPEED; : if (dev->speed == SUPER_SPEED_PLUS) : return SUPER_SPEED_PLUS;
Maybe […]
This could work thanks. I will modify as well.
Hello Julius Werner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35023
to look at the new patch set (#2).
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
libpayload/usb: add USB 3.1 GEN2 support
USB 3.1 GEN2 report speed type 4, add into speed enum.
BUG=b:139787920 BRANCH=N/A TEST=Build libpayload and depthcharge on sarien and boot with USB GEN2 HUB with USB disk. Check ultra speed device in cbmem log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia0ef12b2f0d91bf0d0db766bbc9019de1614a4f4 --- M payloads/libpayload/drivers/usb/usb.c M payloads/libpayload/drivers/usb/usbhub.c M payloads/libpayload/drivers/usb/xhci_devconf.c M payloads/libpayload/include/usb/usb.h 4 files changed, 21 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/35023/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 663: static const char *speeds[] = { "full", "low", "high", "super", "ultra" }; static const char * array should probably be static const char * const
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 663: speeds
This is quite interesting. When I debugging, I am not see this message. I will check this as well.
Done
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 663: static const char *speeds[] = { "full", "low", "high", "super", "ultra" };
static const char * array should probably be static const char * const
Ack
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usbhub.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 99: if (dev->speed == SUPER_SPEED) : return SUPER_SPEED; : if (dev->speed == SUPER_SPEED_PLUS) : return SUPER_SPEED_PLUS;
This could work thanks. I will modify as well.
Done
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 181: >
SGTM, update the patch soon.
Done
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 190: >
Same comment as above.
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 663: static const char *speeds[] = { "full", "low", "high", "super", "ultra" };
Ack
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/xhci_devconf.c:
https://review.coreboot.org/c/coreboot/+/35023/1/payloads/libpayload/drivers... PS1, Line 270: >
same comment as other file.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 701: int is_usb_speed_ss(usb_speed speed) : { : if (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS) : return 1; : return 0; : } Sorry to be a pain, but you can use a bool here. and then return (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS);
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... File payloads/libpayload/include/usb/usb.h:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... PS2, Line 297: int bool, see above
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... File payloads/libpayload/include/usb/usb.h:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... PS2, Line 297: int
bool, see above
I tried but it needs _Bool, and true and false bot not define. If we need bool, I can check how to use it.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 701: int is_usb_speed_ss(usb_speed speed) : { : if (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS) : return 1; : return 0; : }
Sorry to be a pain, but you can use a bool here. […]
I want this be a little flexible. If we have GNE3 or 4 in the future? I am okay with your comment as well. Any idea?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 701: int is_usb_speed_ss(usb_speed speed) : { : if (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS) : return 1; : return 0; : }
I want this be a little flexible. […]
Sure, then you can change it to: return (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS || speed == SUPER_SPEED_MEGA) or whatever it's called next :)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... File payloads/libpayload/include/usb/usb.h:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... PS2, Line 297: int
I tried but it needs _Bool, and true and false bot not define. […]
It looks like libpayload has a <stdbool.h> that should reference 'bool', 'true', and 'false'.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... File payloads/libpayload/include/usb/usb.h:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... PS2, Line 297: int
It looks like libpayload has a <stdbool.h> that should reference 'bool', 'true', and 'false'.
You are right, thanks. I will work on it next week.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 701: int is_usb_speed_ss(usb_speed speed) : { : if (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS) : return 1; : return 0; : }
Sure, then you can change it to: […]
return (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS)? ture : false; This is more readable, what do you think?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 701: int is_usb_speed_ss(usb_speed speed) : { : if (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS) : return 1; : return 0; : }
return (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS)? ture : false; This is more readable, wha […]
That's fine
Hello Julius Werner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35023
to look at the new patch set (#3).
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
libpayload/usb: add USB 3.1 GEN2 support
USB 3.1 GEN2 report speed type 4, add into speed enum.
BUG=b:139787920 BRANCH=N/A TEST=Build libpayload and depthcharge on sarien and boot with USB GEN2 HUB with USB disk. Check ultra speed device in cbmem log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia0ef12b2f0d91bf0d0db766bbc9019de1614a4f4 --- M payloads/libpayload/drivers/usb/usb.c M payloads/libpayload/drivers/usb/usbhub.c M payloads/libpayload/drivers/usb/xhci_devconf.c M payloads/libpayload/include/usb/usb.h 4 files changed, 19 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/35023/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 663: static const char *speeds[] = { "full", "low", "high", "super", "ultra" }; static const char * array should probably be static const char * const
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/drivers... PS2, Line 701: int is_usb_speed_ss(usb_speed speed) : { : if (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS) : return 1; : return 0; : }
That's fine
There is no stdbool.h, I use _Bool instead
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... File payloads/libpayload/include/usb/usb.h:
https://review.coreboot.org/c/coreboot/+/35023/2/payloads/libpayload/include... PS2, Line 297: int
You are right, thanks. I will work on it next week.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 3:
(3 comments)
If you wouldn't mind adding those comments, the rest LGTM
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 267: SUPER_SPEED A comment to indicate that the fallthrough from one case to the other would be very helpful. /* Intentional fallthrough */ or something like that?
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 287: case SUPER_SPEED: A comment to indicate that the fallthrough from one case to the other would be very helpful. /* Intentional fallthrough */ or something like that?
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 323: SUPER_SPEED A comment to indicate that the fallthrough from one case to the other would be very helpful. /* Intentional fallthrough */ or something like that?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 3:
(1 comment)
Patch Set 3:
(3 comments)
If you wouldn't mind adding those comments, the rest LGTM
I can check the GEN1 and GEN2 setting and add this. I assume they use the same setting. I will add comment on this.
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 663: static const char *speeds[] = { "full", "low", "high", "super", "ultra" };
static const char * array should probably be static const char * const
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 3:
Patch Set 3:
(3 comments)
If you wouldn't mind adding those comments, the rest LGTM
I would like check linux kernel setting then update here.
Hello Julius Werner, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35023
to look at the new patch set (#4).
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
libpayload/usb: add USB 3.1 GEN2 support
USB 3.1 GEN2 report speed type 4, add into speed enum.
BUG=b:139787920 BRANCH=N/A TEST=Build libpayload and depthcharge on sarien and boot with USB GEN2 HUB with USB disk. Check ultra speed device in cbmem log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia0ef12b2f0d91bf0d0db766bbc9019de1614a4f4 --- M payloads/libpayload/drivers/usb/usb.c M payloads/libpayload/drivers/usb/usbhub.c M payloads/libpayload/drivers/usb/xhci_devconf.c M payloads/libpayload/include/usb/usb.h 4 files changed, 22 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/35023/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/4/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/4/payloads/libpayload/drivers... PS4, Line 666: static const char *speeds[] = { "full", "low", "high", "super", "ultra" }; static const char * array should probably be static const char * const
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 4:
(3 comments)
Just check kernel driver, no comment in the xhci driver. BTW, I found kernel support USB 3.2 as well.
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 267: SUPER_SPEED
A comment to indicate that the fallthrough from one case to the other would be very helpful. […]
Done
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 287: case SUPER_SPEED:
A comment to indicate that the fallthrough from one case to the other would be very helpful. […]
Done
https://review.coreboot.org/c/coreboot/+/35023/3/payloads/libpayload/drivers... PS3, Line 323: SUPER_SPEED
A comment to indicate that the fallthrough from one case to the other would be very helpful. […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
libpayload/usb: add USB 3.1 GEN2 support
USB 3.1 GEN2 report speed type 4, add into speed enum.
BUG=b:139787920 BRANCH=N/A TEST=Build libpayload and depthcharge on sarien and boot with USB GEN2 HUB with USB disk. Check ultra speed device in cbmem log.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia0ef12b2f0d91bf0d0db766bbc9019de1614a4f4 Reviewed-on: https://review.coreboot.org/c/coreboot/+/35023 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M payloads/libpayload/drivers/usb/usb.c M payloads/libpayload/drivers/usb/usbhub.c M payloads/libpayload/drivers/usb/xhci_devconf.c M payloads/libpayload/include/usb/usb.h 4 files changed, 22 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/usb/usb.c b/payloads/libpayload/drivers/usb/usb.c index 9c9854d..4004def 100644 --- a/payloads/libpayload/drivers/usb/usb.c +++ b/payloads/libpayload/drivers/usb/usb.c @@ -265,6 +265,8 @@ } return bMaxPacketSize0; case SUPER_SPEED: + /* Intentional fallthrough */ + case SUPER_SPEED_PLUS: if (bMaxPacketSize0 != 9) { usb_debug("Invalid MPS0: 0x%02x\n", bMaxPacketSize0); bMaxPacketSize0 = 9; @@ -284,6 +286,8 @@ case HIGH_SPEED: return 64; case SUPER_SPEED: + /* Intentional fallthrough */ + case SUPER_SPEED_PLUS: default: return 512; } @@ -319,6 +323,8 @@ return LOG2(bInterval); } case SUPER_SPEED: + /* Intentional fallthrough */ + case SUPER_SPEED_PLUS: switch (type) { case ISOCHRONOUS: case INTERRUPT: return bInterval - 1; @@ -657,7 +663,7 @@ int usb_attach_device(hci_t *controller, int hubaddress, int port, usb_speed speed) { - static const char* speeds[] = { "full", "low", "high", "super" }; + static const char *speeds[] = { "full", "low", "high", "super", "ultra" }; usb_debug ("%sspeed device\n", (speed < sizeof(speeds) / sizeof(char*)) ? speeds[speed] : "invalid value - no"); int newdev = set_address (controller, speed, port, hubaddress); @@ -693,6 +699,14 @@ }
/* + * returns the speed is above SUPER_SPEED or not + */ +_Bool is_usb_speed_ss(usb_speed speed) +{ + return (speed == SUPER_SPEED || speed == SUPER_SPEED_PLUS); +} + +/* * returns the address of the closest USB2.0 hub, which is responsible for * split transactions, along with the number of the used downstream port */ diff --git a/payloads/libpayload/drivers/usb/usbhub.c b/payloads/libpayload/drivers/usb/usbhub.c index 340e47a..78643c0 100644 --- a/payloads/libpayload/drivers/usb/usbhub.c +++ b/payloads/libpayload/drivers/usb/usbhub.c @@ -96,8 +96,8 @@ int ret = get_status (dev, port, DR_PORT, sizeof(buf), buf); if (ret >= 0 && (buf[0] & PORT_ENABLE)) { /* SuperSpeed hubs can only have SuperSpeed devices. */ - if (dev->speed == SUPER_SPEED) - return SUPER_SPEED; + if (is_usb_speed_ss(dev->speed)) + return dev->speed;
/*[bit] 10 9 (USB 2.0 port status word) * 0 0 full speed @@ -176,7 +176,7 @@ void usb_hub_init(usbdev_t *const dev) { - int type = dev->speed == SUPER_SPEED ? 0x2a : 0x29; /* similar enough */ + int type = is_usb_speed_ss(dev->speed) ? 0x2a : 0x29; /* similar enough */ hub_descriptor_t desc; /* won't fit the whole thing, we don't care */ if (get_descriptor(dev, gen_bmRequestType(device_to_host, class_type, dev_recp), type, 0, &desc, sizeof(desc)) != sizeof(desc)) { @@ -185,7 +185,7 @@ return; }
- if (dev->speed == SUPER_SPEED) + if (is_usb_speed_ss(dev->speed)) usb_hub_set_hub_depth(dev); if (generic_hub_init(dev, desc.bNbrPorts, &usb_hub_ops) < 0) return; diff --git a/payloads/libpayload/drivers/usb/xhci_devconf.c b/payloads/libpayload/drivers/usb/xhci_devconf.c index 99e3037..3f50caa 100644 --- a/payloads/libpayload/drivers/usb/xhci_devconf.c +++ b/payloads/libpayload/drivers/usb/xhci_devconf.c @@ -267,7 +267,7 @@ static int xhci_finish_hub_config(usbdev_t *const dev, inputctx_t *const ic) { - int type = dev->speed == SUPER_SPEED ? 0x2a : 0x29; /* similar enough */ + int type = is_usb_speed_ss(dev->speed) ? 0x2a : 0x29; /* similar enough */ hub_descriptor_t desc;
if (get_descriptor(dev, gen_bmRequestType(device_to_host, class_type, diff --git a/payloads/libpayload/include/usb/usb.h b/payloads/libpayload/include/usb/usb.h index db7ec57..8505c4f 100644 --- a/payloads/libpayload/include/usb/usb.h +++ b/payloads/libpayload/include/usb/usb.h @@ -210,6 +210,7 @@ LOW_SPEED = 1, HIGH_SPEED = 2, SUPER_SPEED = 3, + SUPER_SPEED_PLUS = 4, } usb_speed;
struct usbdev { @@ -293,6 +294,7 @@ int set_configuration (usbdev_t *dev); int clear_feature (usbdev_t *dev, int endp, int feature, int rtype); int clear_stall (endpoint_t *ep); +_Bool is_usb_speed_ss(usb_speed speed);
void usb_nop_init (usbdev_t *dev); void usb_hub_init (usbdev_t *dev);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/5/payloads/libpayload/drivers... PS5, Line 702: * returns the speed is above SUPER_SPEED or not Is an *if* missing?
returns if the speed is above …
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/5/payloads/libpayload/drivers... PS5, Line 666: ultra Sorry for being too late with my comments... but what is "ultra"? Is that an actual term used in the USB specs somewhere? We should be using the official names here (e.g. "super-plus-" or "super+" or something), not make something up. (Technically it's only super+x1, but it doesn't seem like any system really uses the x2 ones so I guess we can ignore them.)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35023 )
Change subject: libpayload/usb: add USB 3.1 GEN2 support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35023/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/usb.c:
https://review.coreboot.org/c/coreboot/+/35023/5/payloads/libpayload/drivers... PS5, Line 666: ultra
Sorry for being too late with my comments... […]
We can change this. Because this is use "%s"Speed, I just think a name looks not weird which is mentioned on usb disk product. Please feel free to add a patch to modify this.
-
build bot (Jenkins) (Code Review) -
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)