| Message ID | cover.1610592135.git.Thinh.Nguyen@synopsys.com |
|---|---|
| Headers | show |
| Series | usb: Support USB 3.2 multi-lanes | expand |
On 21-01-13 18:53:14, Thinh Nguyen wrote: > If a gadget supports SuperSpeed Plus, then it may operate in different > sublink speeds. For example, if the gadget supports SuperSpeed Plus > gen2x2, then it can support 2 sublink speeds gen1 and gen2. Inform the > host of these speeds in the BOS descriptor. > Hi Thinh, I read USB 3.2 spec: ch9.6.2.5 SuperSpeedPlus USB Device Capability Symmetric. Rx and Tx Sublinks have the same number of lanes and operate at the same speed. Asymmetric. Rx and Tx Sublink have different number of lanes and/or operate at different speeds. Why your below cases are all for symmetric, at least, the example 3 is asymmetric, it has different speed for sublink pairs? Does your below cases are specification defined or user defined? > Use 1 SSID if the gadget supports up to gen2x1, or not specified: > - SSID 0 for symmetric RX/TX sublink speed of 10 Gbps. > > Use 1 SSID if the gadget supports up to gen1x2: > - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > > Use 2 SSIDs if the gadget supports up to gen2x2: > - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. Why SSID 0 is not 10Gbps? > - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. Besides, would you give me an example what kinds of system design will use below sublink speed? - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. Peter > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > Changes in v6: > - Rebase on Greg's usb-testing branch > - Use gadget->max_ssp_rate instead of all the sublink attribute fields (now > removed) in usb_gadget > - Use the updated macros > - Update commit message > Changes in v5: > - Rebase on Felipe's testing/next branch > - Changed Signed-off-by email to match From: email header > Changes in v4: > - None > Changes in v3: > - None > Changes in v2: > - None > > drivers/usb/gadget/composite.c | 80 +++++++++++++++++++++++----------- > 1 file changed, 54 insertions(+), 26 deletions(-) > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index bc17302a9e85..72a9797dbbae 100644 > --- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -735,49 +735,77 @@ static int bos_desc(struct usb_composite_dev *cdev) > /* The SuperSpeedPlus USB Device Capability descriptor */ > if (gadget_is_superspeed_plus(cdev->gadget)) { > struct usb_ssp_cap_descriptor *ssp_cap; > + u8 ssac = 1; > + u8 ssic; > + int i; > > - ssp_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength); > - bos->bNumDeviceCaps++; > + if (cdev->gadget->max_ssp_rate == USB_SSP_GEN_2x2) > + ssac = 3; > > /* > - * Report typical values. > + * Paired RX and TX sublink speed attributes share > + * the same SSID. > */ > + ssic = (ssac + 1) / 2 - 1; > > - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SSP_CAP_SIZE(1)); > - ssp_cap->bLength = USB_DT_USB_SSP_CAP_SIZE(1); > + ssp_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength); > + bos->bNumDeviceCaps++; > + > + le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SSP_CAP_SIZE(ssac)); > + ssp_cap->bLength = USB_DT_USB_SSP_CAP_SIZE(ssac); > ssp_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY; > ssp_cap->bDevCapabilityType = USB_SSP_CAP_TYPE; > ssp_cap->bReserved = 0; > ssp_cap->wReserved = 0; > > ssp_cap->bmAttributes = > - cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_ATTRIBS, 1) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, 0)); > + cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_ATTRIBS, ssac) | > + FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, ssic)); > > ssp_cap->wFunctionalitySupport = > cpu_to_le16(FIELD_PREP(USB_SSP_MIN_SUBLINK_SPEED_ATTRIBUTE_ID, 0) | > FIELD_PREP(USB_SSP_MIN_RX_LANE_COUNT, 1) | > FIELD_PREP(USB_SSP_MIN_TX_LANE_COUNT, 1)); > > - ssp_cap->bmSublinkSpeedAttr[0] = > - cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, 0) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, > - USB_SSP_SUBLINK_SPEED_LSE_GBPS) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST, > - USB_SSP_SUBLINK_SPEED_ST_SYM_RX) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, > - USB_SSP_SUBLINK_SPEED_LP_SSP) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, 10)); > - > - ssp_cap->bmSublinkSpeedAttr[1] = > - cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, 0) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, > - USB_SSP_SUBLINK_SPEED_LSE_GBPS) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST, > - USB_SSP_SUBLINK_SPEED_ST_SYM_TX) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, > - USB_SSP_SUBLINK_SPEED_LP_SSP) | > - FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, 10)); > + /* > + * Use 1 SSID if the gadget supports up to gen2x1 or not > + * specified: > + * - SSID 0 for symmetric RX/TX sublink speed of 10 Gbps. > + * > + * Use 1 SSID if the gadget supports up to gen1x2: > + * - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > + * > + * Use 2 SSIDs if the gadget supports up to gen2x2: > + * - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > + * - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. > + */ > + for (i = 0; i < ssac + 1; i++) { > + u8 ssid; > + u8 mantissa; > + u8 type; > + > + ssid = i >> 1; > + > + if (cdev->gadget->max_ssp_rate == USB_SSP_GEN_2x1 || > + cdev->gadget->max_ssp_rate == USB_SSP_GEN_UNKNOWN) > + mantissa = 10; > + else > + mantissa = 5 << ssid; > + > + if (i % 2) > + type = USB_SSP_SUBLINK_SPEED_ST_SYM_TX; > + else > + type = USB_SSP_SUBLINK_SPEED_ST_SYM_RX; > + > + ssp_cap->bmSublinkSpeedAttr[i] = > + cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, ssid) | > + FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, > + USB_SSP_SUBLINK_SPEED_LSE_GBPS) | > + FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST, type) | > + FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, > + USB_SSP_SUBLINK_SPEED_LP_SSP) | > + FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, mantissa)); > + } > } > > return le16_to_cpu(bos->wTotalLength); > -- > 2.28.0 >
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > @@ -2476,6 +2506,17 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g, > spin_unlock_irqrestore(&dwc->lock, flags); > } > > +static void dwc3_gadget_set_ssp_rate(struct usb_gadget *g, > + enum usb_ssp_rate rate) > +{ > + struct dwc3 *dwc = gadget_to_dwc(g); > + unsigned long flags; > + > + spin_lock_irqsave(&dwc->lock, flags); > + dwc->gadget_ssp_rate = rate; > + spin_unlock_irqrestore(&dwc->lock, flags); > +} it would be best to make this return a value. If udc_set_ssp_rate() is called with invalid rate, UDC can notify core.
Felipe Balbi wrote: > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > >> In preparation for USB 3.2 dual-lane support, add sublink speed >> attribute macros and enum usb_ssp_rate. A USB device that operates in >> SuperSpeed Plus may operate at different speed and lane count. These >> additional macros and enum values help specifying that. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> Changes in v6: >> - Rebase on Greg's usb-testing branch >> - Convert the sublink speed attribute enum to macros and move it to uapi >> - Remove usb_sublink_speed struct >> - To simplify things, use usb_ssp_rate enum to specify the signaling rate >> generation and lane count >> - Update commit message >> Changes in v5: >> - Rebase on Felipe's testing/next branch >> - Changed Signed-off-by email to match From: email header >> Changes in v4: >> - None >> Changes in v3: >> - None >> Changes in v2: >> - Move to include/linux/usb/ch9.h instead of under uapi >> >> include/linux/usb/ch9.h | 9 +++++++++ >> include/uapi/linux/usb/ch9.h | 13 +++++++++++++ >> 2 files changed, 22 insertions(+) >> >> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h >> index 604c6c514a50..86c50907634e 100644 >> --- a/include/linux/usb/ch9.h >> +++ b/include/linux/usb/ch9.h >> @@ -36,6 +36,15 @@ >> #include <linux/device.h> >> #include <uapi/linux/usb/ch9.h> >> >> +/* USB 3.2 SuperSpeed Plus phy signaling rate generation and lane count */ >> + >> +enum usb_ssp_rate { >> + USB_SSP_GEN_UNKNOWN = 0, >> + USB_SSP_GEN_2x1, >> + USB_SSP_GEN_1x2, >> + USB_SSP_GEN_2x2, >> +}; > note that xHCI has some private definitions for USB 3.2 support. Maybe > add a patch converting xHCI to the generic versions? > Should it be part of this series? I plan to do that after this series is merged to help minimize the review effort. Thanks, Thinh
On 21-01-14 06:16:18, Thinh Nguyen wrote: > Hi Peter, > > Peter Chen wrote: > > On 21-01-13 18:53:14, Thinh Nguyen wrote: > >> If a gadget supports SuperSpeed Plus, then it may operate in different > >> sublink speeds. For example, if the gadget supports SuperSpeed Plus > >> gen2x2, then it can support 2 sublink speeds gen1 and gen2. Inform the > >> host of these speeds in the BOS descriptor. > >> > > Hi Thinh, > > > > I read USB 3.2 spec: ch9.6.2.5 SuperSpeedPlus USB Device Capability > > > > Symmetric. Rx and Tx Sublinks have the same number of lanes and operate > > at the same speed. > > Asymmetric. Rx and Tx Sublink have different number of lanes and/or > > operate at different speeds. > > > > Why your below cases are all for symmetric, at least, the example 3 > > is asymmetric, it has different speed for sublink pairs? > > Does your below cases are specification defined or user defined? > > USB 3.2 spec section 8.5.6.7: > Asymmetric lane types are only for SuperSpeed Interchip (SSIC). IMO, > It's unlikely that SSIC user will use Linux kernel. We can extend and > update the gadget framework if there's any use case for that. > > > > > >> Use 1 SSID if the gadget supports up to gen2x1, or not specified: > >> - SSID 0 for symmetric RX/TX sublink speed of 10 Gbps. > >> > >> Use 1 SSID if the gadget supports up to gen1x2: > >> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > >> > >> Use 2 SSIDs if the gadget supports up to gen2x2: > >> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > > Why SSID 0 is not 10Gbps? > > SSID 0 and 1 are arbitrary, we can do 0 for 10Gbps. There's no > constraint or standard from the USB 3.2 spec. However, you need to set > the descriptor wFunctionalitySupport.SSID to be the minimum lane speed > SSID it supports. Using SSID 0 makes it easier since we don't have to > condition it for multiple SSIDs. > > >> - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. > > Besides, would you give me an example what kinds of system design > > will use below sublink speed? > > - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > > - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. > > > > Peter > > These 2 SSIDs indicate that the device is capable of running in gen1 and > gen2 in SSP. > Hi Thinh, I am puzzled, GEN2 is back compatible for GEN1. Then, what's the usage of this descriptor, what kinds of specific information the host wants to get? I think the host wants to get if two pairs of tx/rx are supported, that is what USB 3.2 adds. -- Thanks, Peter Chen
Peter Chen wrote: > On 21-01-14 06:16:18, Thinh Nguyen wrote: >> Hi Peter, >> >> Peter Chen wrote: >>> On 21-01-13 18:53:14, Thinh Nguyen wrote: >>>> If a gadget supports SuperSpeed Plus, then it may operate in different >>>> sublink speeds. For example, if the gadget supports SuperSpeed Plus >>>> gen2x2, then it can support 2 sublink speeds gen1 and gen2. Inform the >>>> host of these speeds in the BOS descriptor. >>>> >>> Hi Thinh, >>> >>> I read USB 3.2 spec: ch9.6.2.5 SuperSpeedPlus USB Device Capability >>> >>> Symmetric. Rx and Tx Sublinks have the same number of lanes and operate >>> at the same speed. >>> Asymmetric. Rx and Tx Sublink have different number of lanes and/or >>> operate at different speeds. >>> >>> Why your below cases are all for symmetric, at least, the example 3 >>> is asymmetric, it has different speed for sublink pairs? >>> Does your below cases are specification defined or user defined? >> USB 3.2 spec section 8.5.6.7: >> Asymmetric lane types are only for SuperSpeed Interchip (SSIC). IMO, >> It's unlikely that SSIC user will use Linux kernel. We can extend and >> update the gadget framework if there's any use case for that. >> >> >>>> Use 1 SSID if the gadget supports up to gen2x1, or not specified: >>>> - SSID 0 for symmetric RX/TX sublink speed of 10 Gbps. >>>> >>>> Use 1 SSID if the gadget supports up to gen1x2: >>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. >>>> >>>> Use 2 SSIDs if the gadget supports up to gen2x2: >>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. >>> Why SSID 0 is not 10Gbps? >> SSID 0 and 1 are arbitrary, we can do 0 for 10Gbps. There's no >> constraint or standard from the USB 3.2 spec. However, you need to set >> the descriptor wFunctionalitySupport.SSID to be the minimum lane speed >> SSID it supports. Using SSID 0 makes it easier since we don't have to >> condition it for multiple SSIDs. >> >>>> - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. >>> Besides, would you give me an example what kinds of system design >>> will use below sublink speed? >>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. >>> - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. >>> >>> Peter >> These 2 SSIDs indicate that the device is capable of running in gen1 and >> gen2 in SSP. >> > Hi Thinh, > > I am puzzled, GEN2 is back compatible for GEN1. Then, what's the usage > of this descriptor, what kinds of specific information the host wants to get? > I think the host wants to get if two pairs of tx/rx are supported, that > is what USB 3.2 adds. > Hi Peter, Yes, it's backward compatible. However Gen1x2 uses SuperSpeed Plus Link Protocol. Single lane Gen 1 doesn't. This additional RX/TX sublink speed attribute pair is telling the host that the device is capable of running at Gen1x2 and Gen2x2. Host can use this information to know that the device supports Gen 1 as SSP, and it can infer that the device supports dual-lane. Otherwise, the host can only check for dual-lane support after it receives a port status or sublink speed device notification TP. Regardless, the gadget should describe all the sublink speeds the device is capable of in the SSP capability descriptor. BR, Thinh
Thinh Nguyen wrote: > Peter Chen wrote: >> On 21-01-14 06:16:18, Thinh Nguyen wrote: >>> Hi Peter, >>> >>> Peter Chen wrote: >>>> On 21-01-13 18:53:14, Thinh Nguyen wrote: >>>>> If a gadget supports SuperSpeed Plus, then it may operate in different >>>>> sublink speeds. For example, if the gadget supports SuperSpeed Plus >>>>> gen2x2, then it can support 2 sublink speeds gen1 and gen2. Inform the >>>>> host of these speeds in the BOS descriptor. >>>>> >>>> Hi Thinh, >>>> >>>> I read USB 3.2 spec: ch9.6.2.5 SuperSpeedPlus USB Device Capability >>>> >>>> Symmetric. Rx and Tx Sublinks have the same number of lanes and operate >>>> at the same speed. >>>> Asymmetric. Rx and Tx Sublink have different number of lanes and/or >>>> operate at different speeds. >>>> >>>> Why your below cases are all for symmetric, at least, the example 3 >>>> is asymmetric, it has different speed for sublink pairs? >>>> Does your below cases are specification defined or user defined? >>> USB 3.2 spec section 8.5.6.7: >>> Asymmetric lane types are only for SuperSpeed Interchip (SSIC). IMO, >>> It's unlikely that SSIC user will use Linux kernel. We can extend and >>> update the gadget framework if there's any use case for that. >>> >>> >>>>> Use 1 SSID if the gadget supports up to gen2x1, or not specified: >>>>> - SSID 0 for symmetric RX/TX sublink speed of 10 Gbps. >>>>> >>>>> Use 1 SSID if the gadget supports up to gen1x2: >>>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. >>>>> >>>>> Use 2 SSIDs if the gadget supports up to gen2x2: >>>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. >>>> Why SSID 0 is not 10Gbps? >>> SSID 0 and 1 are arbitrary, we can do 0 for 10Gbps. There's no >>> constraint or standard from the USB 3.2 spec. However, you need to set >>> the descriptor wFunctionalitySupport.SSID to be the minimum lane speed >>> SSID it supports. Using SSID 0 makes it easier since we don't have to >>> condition it for multiple SSIDs. >>> >>>>> - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. >>>> Besides, would you give me an example what kinds of system design >>>> will use below sublink speed? >>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. >>>> - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. >>>> >>>> Peter >>> These 2 SSIDs indicate that the device is capable of running in gen1 and >>> gen2 in SSP. >>> >> Hi Thinh, >> >> I am puzzled, GEN2 is back compatible for GEN1. Then, what's the usage >> of this descriptor, what kinds of specific information the host wants to get? >> I think the host wants to get if two pairs of tx/rx are supported, that >> is what USB 3.2 adds. >> > Hi Peter, > > Yes, it's backward compatible. However Gen1x2 uses SuperSpeed Plus Link > Protocol. Single lane Gen 1 doesn't. This additional RX/TX sublink speed > attribute pair is telling the host that the device is capable of running > at Gen1x2 and Gen2x2. Host can use this information to know that the > device supports Gen 1 as SSP, and it can infer that the device supports > dual-lane. Otherwise, the host can only check for dual-lane support > after it receives a port status or sublink speed device notification TP. > Regardless, the gadget should describe all the sublink speeds the device > is capable of in the SSP capability descriptor. > Regarding checking port status and sublink speed device notification, I meant it as "connected" status and not capability. Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > Felipe Balbi wrote: >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> >>> In preparation for USB 3.2 dual-lane support, add sublink speed >>> attribute macros and enum usb_ssp_rate. A USB device that operates in >>> SuperSpeed Plus may operate at different speed and lane count. These >>> additional macros and enum values help specifying that. >>> >>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> --- >>> Changes in v6: >>> - Rebase on Greg's usb-testing branch >>> - Convert the sublink speed attribute enum to macros and move it to uapi >>> - Remove usb_sublink_speed struct >>> - To simplify things, use usb_ssp_rate enum to specify the signaling rate >>> generation and lane count >>> - Update commit message >>> Changes in v5: >>> - Rebase on Felipe's testing/next branch >>> - Changed Signed-off-by email to match From: email header >>> Changes in v4: >>> - None >>> Changes in v3: >>> - None >>> Changes in v2: >>> - Move to include/linux/usb/ch9.h instead of under uapi >>> >>> include/linux/usb/ch9.h | 9 +++++++++ >>> include/uapi/linux/usb/ch9.h | 13 +++++++++++++ >>> 2 files changed, 22 insertions(+) >>> >>> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h >>> index 604c6c514a50..86c50907634e 100644 >>> --- a/include/linux/usb/ch9.h >>> +++ b/include/linux/usb/ch9.h >>> @@ -36,6 +36,15 @@ >>> #include <linux/device.h> >>> #include <uapi/linux/usb/ch9.h> >>> >>> +/* USB 3.2 SuperSpeed Plus phy signaling rate generation and lane count */ >>> + >>> +enum usb_ssp_rate { >>> + USB_SSP_GEN_UNKNOWN = 0, >>> + USB_SSP_GEN_2x1, >>> + USB_SSP_GEN_1x2, >>> + USB_SSP_GEN_2x2, >>> +}; >> note that xHCI has some private definitions for USB 3.2 support. Maybe >> add a patch converting xHCI to the generic versions? >> > > Should it be part of this series? I plan to do that after this series is > merged to help minimize the review effort. As long as it's part of your TODO list, should be good :-) -- balbi
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > Felipe Balbi wrote: >> Hi, >> >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> @@ -2476,6 +2506,17 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g, >>> spin_unlock_irqrestore(&dwc->lock, flags); >>> } >>> >>> +static void dwc3_gadget_set_ssp_rate(struct usb_gadget *g, >>> + enum usb_ssp_rate rate) >>> +{ >>> + struct dwc3 *dwc = gadget_to_dwc(g); >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&dwc->lock, flags); >>> + dwc->gadget_ssp_rate = rate; >>> + spin_unlock_irqrestore(&dwc->lock, flags); >>> +} >> it would be best to make this return a value. If udc_set_ssp_rate() is >> called with invalid rate, UDC can notify core. >> > > The core should know what ssp rate the gadget supports via the > gadget->max_ssp_rate capability field. Any rate beyond that is invalid. > Is it necessary to have a return value here? This uses the same logic as > udc_set_speed() Yeah, I don't know what I had in mind when I made ->udc_set_speed() void. Then again, we know exactly who's calling it and we can guarantee that no invalid values will be passed. There's no way for, for example, userspace (via ffs) to call it with a bogus value. Perhaps it's okay, but something to keep an eye for both ->udc_set_ssp_rate() and ->udc_set_speed(). Thanks for pointing me back at ->udc_set_speed(). -- balbi
Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> Felipe Balbi wrote: >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> >>>> In preparation for USB 3.2 dual-lane support, add sublink speed >>>> attribute macros and enum usb_ssp_rate. A USB device that operates in >>>> SuperSpeed Plus may operate at different speed and lane count. These >>>> additional macros and enum values help specifying that. >>>> >>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>> --- >>>> Changes in v6: >>>> - Rebase on Greg's usb-testing branch >>>> - Convert the sublink speed attribute enum to macros and move it to uapi >>>> - Remove usb_sublink_speed struct >>>> - To simplify things, use usb_ssp_rate enum to specify the signaling rate >>>> generation and lane count >>>> - Update commit message >>>> Changes in v5: >>>> - Rebase on Felipe's testing/next branch >>>> - Changed Signed-off-by email to match From: email header >>>> Changes in v4: >>>> - None >>>> Changes in v3: >>>> - None >>>> Changes in v2: >>>> - Move to include/linux/usb/ch9.h instead of under uapi >>>> >>>> include/linux/usb/ch9.h | 9 +++++++++ >>>> include/uapi/linux/usb/ch9.h | 13 +++++++++++++ >>>> 2 files changed, 22 insertions(+) >>>> >>>> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h >>>> index 604c6c514a50..86c50907634e 100644 >>>> --- a/include/linux/usb/ch9.h >>>> +++ b/include/linux/usb/ch9.h >>>> @@ -36,6 +36,15 @@ >>>> #include <linux/device.h> >>>> #include <uapi/linux/usb/ch9.h> >>>> >>>> +/* USB 3.2 SuperSpeed Plus phy signaling rate generation and lane count */ >>>> + >>>> +enum usb_ssp_rate { >>>> + USB_SSP_GEN_UNKNOWN = 0, >>>> + USB_SSP_GEN_2x1, >>>> + USB_SSP_GEN_1x2, >>>> + USB_SSP_GEN_2x2, >>>> +}; >>> note that xHCI has some private definitions for USB 3.2 support. Maybe >>> add a patch converting xHCI to the generic versions? >>> >> Should it be part of this series? I plan to do that after this series is >> merged to help minimize the review effort. > As long as it's part of your TODO list, should be good :-) > Yeah, it's on my list. Currently Linux xHCI driver doesn't really check for USB 3.2. It's missing the root hub sublink speed capability descriptors for gen1x2 and gen2x2. So it's missing some xHCI defined default port speed ID as SSID for gen1x2 and gen2x2. The Linux xHCI right now may think that the connected dual-lane device may not be a SSP device because it doesn't match the SSID from the extended port status. There are more patches for dwc3 and some for xHCI on queue that hopefully get cleaned up and pushed out eventually. BR, Thinh
On 21-01-15 02:56:37, Thinh Nguyen wrote: > Thinh Nguyen wrote: > > Peter Chen wrote: > >> On 21-01-14 06:16:18, Thinh Nguyen wrote: > >>> Hi Peter, > >>> > >>> Peter Chen wrote: > >>>> On 21-01-13 18:53:14, Thinh Nguyen wrote: > >>>>> If a gadget supports SuperSpeed Plus, then it may operate in different > >>>>> sublink speeds. For example, if the gadget supports SuperSpeed Plus > >>>>> gen2x2, then it can support 2 sublink speeds gen1 and gen2. Inform the > >>>>> host of these speeds in the BOS descriptor. > >>>>> > >>>> Hi Thinh, > >>>> > >>>> I read USB 3.2 spec: ch9.6.2.5 SuperSpeedPlus USB Device Capability > >>>> > >>>> Symmetric. Rx and Tx Sublinks have the same number of lanes and operate > >>>> at the same speed. > >>>> Asymmetric. Rx and Tx Sublink have different number of lanes and/or > >>>> operate at different speeds. > >>>> > >>>> Why your below cases are all for symmetric, at least, the example 3 > >>>> is asymmetric, it has different speed for sublink pairs? > >>>> Does your below cases are specification defined or user defined? > >>> USB 3.2 spec section 8.5.6.7: > >>> Asymmetric lane types are only for SuperSpeed Interchip (SSIC). IMO, > >>> It's unlikely that SSIC user will use Linux kernel. We can extend and > >>> update the gadget framework if there's any use case for that. > >>> > >>> > >>>>> Use 1 SSID if the gadget supports up to gen2x1, or not specified: > >>>>> - SSID 0 for symmetric RX/TX sublink speed of 10 Gbps. > >>>>> > >>>>> Use 1 SSID if the gadget supports up to gen1x2: > >>>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > >>>>> > >>>>> Use 2 SSIDs if the gadget supports up to gen2x2: > >>>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > >>>> Why SSID 0 is not 10Gbps? > >>> SSID 0 and 1 are arbitrary, we can do 0 for 10Gbps. There's no > >>> constraint or standard from the USB 3.2 spec. However, you need to set > >>> the descriptor wFunctionalitySupport.SSID to be the minimum lane speed > >>> SSID it supports. Using SSID 0 makes it easier since we don't have to > >>> condition it for multiple SSIDs. > >>> > >>>>> - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. > >>>> Besides, would you give me an example what kinds of system design > >>>> will use below sublink speed? > >>>> - SSID 0 for symmetric RX/TX sublink speed of 5 Gbps. > >>>> - SSID 1 for symmetric RX/TX sublink speed of 10 Gbps. > >>>> > >>>> Peter > >>> These 2 SSIDs indicate that the device is capable of running in gen1 and > >>> gen2 in SSP. > >>> > >> Hi Thinh, > >> > >> I am puzzled, GEN2 is back compatible for GEN1. Then, what's the usage > >> of this descriptor, what kinds of specific information the host wants to get? > >> I think the host wants to get if two pairs of tx/rx are supported, that > >> is what USB 3.2 adds. > >> > > Hi Peter, > > > > Yes, it's backward compatible. However Gen1x2 uses SuperSpeed Plus Link > > Protocol. Single lane Gen 1 doesn't. This additional RX/TX sublink speed > > attribute pair is telling the host that the device is capable of running > > at Gen1x2 and Gen2x2. Host can use this information to know that the > > device supports Gen 1 as SSP, and it can infer that the device supports > > dual-lane. Otherwise, the host can only check for dual-lane support > > after it receives a port status or sublink speed device notification TP. > > Regardless, the gadget should describe all the sublink speeds the device > > is capable of in the SSP capability descriptor. > > > > Regarding checking port status and sublink speed device notification, I > meant it as "connected" status and not capability. > Thanks for explaining it, Thinh -- Thanks, Peter Chen
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>>> In preparation for USB 3.2 dual-lane support, add sublink speed >>>>> attribute macros and enum usb_ssp_rate. A USB device that operates in >>>>> SuperSpeed Plus may operate at different speed and lane count. These >>>>> additional macros and enum values help specifying that. >>>>> >>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>>> --- >>>>> Changes in v6: >>>>> - Rebase on Greg's usb-testing branch >>>>> - Convert the sublink speed attribute enum to macros and move it to uapi >>>>> - Remove usb_sublink_speed struct >>>>> - To simplify things, use usb_ssp_rate enum to specify the signaling rate >>>>> generation and lane count >>>>> - Update commit message >>>>> Changes in v5: >>>>> - Rebase on Felipe's testing/next branch >>>>> - Changed Signed-off-by email to match From: email header >>>>> Changes in v4: >>>>> - None >>>>> Changes in v3: >>>>> - None >>>>> Changes in v2: >>>>> - Move to include/linux/usb/ch9.h instead of under uapi >>>>> >>>>> include/linux/usb/ch9.h | 9 +++++++++ >>>>> include/uapi/linux/usb/ch9.h | 13 +++++++++++++ >>>>> 2 files changed, 22 insertions(+) >>>>> >>>>> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h >>>>> index 604c6c514a50..86c50907634e 100644 >>>>> --- a/include/linux/usb/ch9.h >>>>> +++ b/include/linux/usb/ch9.h >>>>> @@ -36,6 +36,15 @@ >>>>> #include <linux/device.h> >>>>> #include <uapi/linux/usb/ch9.h> >>>>> >>>>> +/* USB 3.2 SuperSpeed Plus phy signaling rate generation and lane count */ >>>>> + >>>>> +enum usb_ssp_rate { >>>>> + USB_SSP_GEN_UNKNOWN = 0, >>>>> + USB_SSP_GEN_2x1, >>>>> + USB_SSP_GEN_1x2, >>>>> + USB_SSP_GEN_2x2, >>>>> +}; >>>> note that xHCI has some private definitions for USB 3.2 support. Maybe >>>> add a patch converting xHCI to the generic versions? >>>> >>> Should it be part of this series? I plan to do that after this series is >>> merged to help minimize the review effort. >> As long as it's part of your TODO list, should be good :-) >> > > Yeah, it's on my list. Currently Linux xHCI driver doesn't really check > for USB 3.2. It's missing the root hub sublink speed capability > descriptors for gen1x2 and gen2x2. So it's missing some xHCI defined > default port speed ID as SSID for gen1x2 and gen2x2. The Linux xHCI > right now may think that the connected dual-lane device may not be a SSP > device because it doesn't match the SSID from the extended port status. > > There are more patches for dwc3 and some for xHCI on queue that > hopefully get cleaned up and pushed out eventually. Sounds great, Thinh. Thanks for all the effort you've been putting here :-) -- balbi
On Wed, Jan 13, 2021 at 06:52:37PM -0800, Thinh Nguyen wrote: > A USB SuperSpeed Plus device may operate at different speed and lane count > (i.e. gen2x2, gen1x2, or gen2x1). The DWC_usb32 IP supports SuperSpeed Plus > gen2x2. To support this, this series update a few things to the USB gadget > stack and dwc3 driver: > > * Accept and parse new maximum_speed devicetree property strings > * Introduce enum usb_ssp_rate to describe the speed in SuperSpeed Plus genXxY > * Capture the connected and max supported usb_ssp_rate > * Report the device sublink speeds base on the usb_ssp_rate in the BOS > descriptor > * Introduce gadget ops to select SuperSpeed Plus various transfer rate and lane > count > * Update dwc3 driver to support the above changes I've taken the first 5 patches now, I'll wait for the DT maintainers and dwc3 maintainers to review the rest before I can take them. Feel free to rebase and resend the smaller set of patches if that makes it easier for them to review. thanks, greg k-h
Greg Kroah-Hartman wrote: > On Wed, Jan 13, 2021 at 06:52:37PM -0800, Thinh Nguyen wrote: >> A USB SuperSpeed Plus device may operate at different speed and lane count >> (i.e. gen2x2, gen1x2, or gen2x1). The DWC_usb32 IP supports SuperSpeed Plus >> gen2x2. To support this, this series update a few things to the USB gadget >> stack and dwc3 driver: >> >> * Accept and parse new maximum_speed devicetree property strings >> * Introduce enum usb_ssp_rate to describe the speed in SuperSpeed Plus genXxY >> * Capture the connected and max supported usb_ssp_rate >> * Report the device sublink speeds base on the usb_ssp_rate in the BOS >> descriptor >> * Introduce gadget ops to select SuperSpeed Plus various transfer rate and lane >> count >> * Update dwc3 driver to support the above changes > I've taken the first 5 patches now, I'll wait for the DT maintainers and > dwc3 maintainers to review the rest before I can take them. Feel free > to rebase and resend the smaller set of patches if that makes it easier > for them to review. > > thanks, > > greg k-h Just rebased and resent. Thanks! Thinh