Re: [PATCH] usb: gadget: f_ncm: allow using NCM in SuperSpeed Plus gadgets.

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

Lorenzo Colitti <lorenzo@google.com> writes:

> Currently, using f_ncm in a SuperSpeed Plus gadget results in
> an oops in config_ep_by_speed because ncm_set_alt passes in NULL
> ssp_descriptors. Fix this by defining new descriptors for
> SuperSpeed Plus. (We cannot re-use the existing definitions for
> the SuperSpeed descriptors, even though they are mostly the same,
> because they are not fixed initializers).
>
> Also fix reported bandwidth to match bandwidth reported for
> SuperSpeed. This calculation is already incorrect, because it
> returns 851 Mbps and NCM can already reach speeds in excess of
> 1.7 Gbps on a 5 Gbps port. But it's better to return 851 Mbps
> than 9 Mbps for SuperSpeed Plus.
>
> Tested: enabled f_ncm on a dwc3 gadget and 10Gbps link, ran iperf
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> ---
>  drivers/usb/gadget/function/f_ncm.c | 79 ++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index 1d900081b1..91f87165e7 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -85,7 +85,8 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
>  /* peak (theoretical) bulk transfer rate in bits-per-second */
>  static inline unsigned ncm_bitrate(struct usb_gadget *g)
>  {
> -	if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
> +	if ((gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER) ||
> +	    (gadget_is_superspeed_plus(g) && g->speed == USB_SPEED_SUPER_PLUS))
>  		return 13 * 1024 * 8 * 1000 * 8;

shouldn't this be 16 * 1024 * 8 * 1000 * 8? I mean, assuming we have
bMaxBurst set to 15. If I remember how this calculation was done it was:

	13 packets / uFrame
        1024 bytes per packet
        8000 uFrames / second
        8    bits / byte

But for SS and SSP what controls the number of packets in a uFrame is
bMaxBurst, which can go up to 16 (15 + 1). Seems like we should also
update ss_ncm_bulk_comp_desc to set a bMaxBurst of 15:

@@ -381,8 +381,8 @@ static struct usb_ss_ep_comp_descriptor ss_ncm_bulk_comp_desc = {
 	.bLength =		sizeof(ss_ncm_bulk_comp_desc),
 	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
 
-	/* the following 2 values can be tweaked if necessary */
-	/* .bMaxBurst =		0, */
+	.bMaxBurst =		15,
+	/* the following value can be tweaked if necessary */
 	/* .bmAttributes =	0, */
 };
 
But anyway, if we change 13 to 16, then we 1Gbps. How did you get
1.7Gbps? I think we should really update ncm_bitrate() to contain the
correct equations for bitrate on different speeds.

> @@ -400,6 +401,75 @@ static struct usb_descriptor_header *ncm_ss_function[] = {
>  	NULL,
>  };
>  
> +/* super speed plus support: */
> +
> +static struct usb_endpoint_descriptor ssp_ncm_notify_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT,
> +
> +	.bEndpointAddress =	USB_DIR_IN,
> +	.bmAttributes =		USB_ENDPOINT_XFER_INT,
> +	.wMaxPacketSize =	cpu_to_le16(NCM_STATUS_BYTECOUNT),
> +	.bInterval =		USB_MS_TO_HS_INTERVAL(NCM_STATUS_INTERVAL_MS)
> +};
> +
> +static struct usb_ss_ep_comp_descriptor ssp_ncm_notify_comp_desc = {
> +	.bLength =		sizeof(ssp_ncm_notify_comp_desc),
> +	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
> +
> +	/* the following 3 values can be tweaked if necessary */
> +	/* .bMaxBurst =		0, */
> +	/* .bmAttributes =	0, */
> +	.wBytesPerInterval =	cpu_to_le16(NCM_STATUS_BYTECOUNT),
> +};
> +
> +static struct usb_endpoint_descriptor ssp_ncm_in_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT,
> +
> +	.bEndpointAddress =	USB_DIR_IN,
> +	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
> +	.wMaxPacketSize =	cpu_to_le16(1024),
> +};
> +
> +static struct usb_endpoint_descriptor ssp_ncm_out_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT,
> +
> +	.bEndpointAddress =	USB_DIR_OUT,
> +	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
> +	.wMaxPacketSize =	cpu_to_le16(1024),
> +};
> +
> +static struct usb_ss_ep_comp_descriptor ssp_ncm_bulk_comp_desc = {
> +	.bLength =		sizeof(ssp_ncm_bulk_comp_desc),
> +	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
> +
> +	/* the following 2 values can be tweaked if necessary */
> +	/* .bMaxBurst =		0, */

should you add bMaxBurst = 15 here?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]