Hi Mattijs, (Sorry for the delay in response)
On Fri, Jul 24, 2026, Mattijs Korpershoek wrote: > Hi Thinh, > > On Thu, Jul 23, 2026 at 20:40, Thinh Nguyen <[email protected]> wrote: > > > Hi Mattijs, > > > > On Thu, Jul 23, 2026, Mattijs Korpershoek wrote: > >> Hi Thinh, > >> > >> Thank you for the patch. > >> > >> On Thu, Jul 09, 2026 at 18:19, Thinh Nguyen <[email protected]> > >> wrote: > >> > >> > From: Dan Tran <[email protected]> > >> > > >> > Add SS bulk endpoint descriptors (1024-byte MPS, bMaxBurst=15) to > >> > storage_common.c to support SuperSpeed connections. Extend fsg_ep_desc() > >> > to select them when operating at SuperSpeed, and wire up ss_descriptors > >> > in fsg_bind(). Free ss_descriptors in fsg_unbind() to match. > >> > > >> > Signed-off-by: Dan Tran <[email protected]> > >> > Signed-off-by: Thinh Nguyen <[email protected]> > >> > --- > >> > Changes in v2: > >> > - Removed internal Reviewed-by tags > >> > > >> > > >> > drivers/usb/gadget/f_mass_storage.c | 25 ++++++++++++-- > >> > drivers/usb/gadget/storage_common.c | 52 ++++++++++++++++++++++++++++- > >> > 2 files changed, 74 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/usb/gadget/f_mass_storage.c > >> > b/drivers/usb/gadget/f_mass_storage.c > >> > index 87ed25e8bb3a..a2f34c100482 100644 > >> > --- a/drivers/usb/gadget/f_mass_storage.c > >> > +++ b/drivers/usb/gadget/f_mass_storage.c > >> > @@ -2225,14 +2225,16 @@ reset: > >> > > >> > /* Enable the endpoints */ > >> > d = fsg_ep_desc(common->gadget, > >> > - &fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc); > >> > + &fsg_fs_bulk_in_desc, &fsg_hs_bulk_in_desc, > >> > + &fsg_ss_bulk_in_desc); > >> > rc = enable_endpoint(common, fsg->bulk_in, d); > >> > if (rc) > >> > goto reset; > >> > fsg->bulk_in_enabled = 1; > >> > > >> > d = fsg_ep_desc(common->gadget, > >> > - &fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc); > >> > + &fsg_fs_bulk_out_desc, &fsg_hs_bulk_out_desc, > >> > + &fsg_ss_bulk_out_desc); > >> > rc = enable_endpoint(common, fsg->bulk_out, d); > >> > if (rc) > >> > goto reset; > >> > @@ -2653,6 +2655,7 @@ static void fsg_unbind(struct usb_configuration > >> > *c, struct usb_function *f) > >> > fsg_common_release(fsg->common); > >> > free(fsg->function.descriptors); > >> > free(fsg->function.hs_descriptors); > >> > + free(fsg->function.ss_descriptors); > >> > kfree(fsg); > >> > } > >> > > >> > @@ -2701,6 +2704,24 @@ static int fsg_bind(struct usb_configuration *c, > >> > struct usb_function *f) > >> > return -ENOMEM; > >> > } > >> > } > >> > + > >> > + if (gadget_is_superspeed(gadget)) { > >> > + unsigned int max_burst = min_t(unsigned int, FSG_BUFLEN > >> > / 1024, 15); > >> > >> I can see that this looks similar to what we have in Linux with > >> commit 4bb99b7c82ba ("usb: gadget: storage: add superspeed support") > >> > >> However, the Linux patch added a comment as well: > >> > >> + /* Calculate bMaxBurst, we know packet size is 1024 */ > >> + max_burst = min_t(unsigned, FSG_BUFLEN / 1024, 15); > >> > >> Why can't we do the same here? > > > > The comment doesn't add much beyond what the code already expresses. The > > variable name max_burst and the division by 1024 make the intent clear. > > > > That said, the Linux comment also has a minor inaccuracy: it says > > "packet size" when it should say "max packet size". If a comment is > > warranted, I'd prefer to add a corrected one. We can add it if you > > really think it helps with readability. > > Overall, my rule of thumb is "keep the code as close as possible to the > Linux driver to ease maintenance in U-Boot". > > Please consider adding the corrected comment for v3. I'll add the comment in v3. > > > > >> > >> > + > >> > + fsg_ss_bulk_in_desc.bEndpointAddress = > >> > + fsg_fs_bulk_in_desc.bEndpointAddress; > >> > + fsg_ss_bulk_in_comp_desc.bMaxBurst = max_burst; > >> > + fsg_ss_bulk_out_desc.bEndpointAddress = > >> > + fsg_fs_bulk_out_desc.bEndpointAddress; > >> > + fsg_ss_bulk_out_comp_desc.bMaxBurst = max_burst; > >> > + f->ss_descriptors = > >> > usb_copy_descriptors(fsg_ss_function); > >> > + if (unlikely(!f->ss_descriptors)) { > >> > + free(f->hs_descriptors); > >> > + free(f->descriptors); > >> > + return -ENOMEM; > >> > + } > >> > + } > >> > + > >> > return 0; > >> > > >> > autoconf_fail: > >> > diff --git a/drivers/usb/gadget/storage_common.c > >> > b/drivers/usb/gadget/storage_common.c > > [...] > > >> > + (struct usb_descriptor_header *)&fsg_ss_bulk_out_comp_desc, > >> > + NULL, > >> > +}; > >> > + > >> > /* Maxpacket and other transfer characteristics vary by speed. */ > >> > static struct usb_endpoint_descriptor * > >> > fsg_ep_desc(struct usb_gadget *g, struct usb_endpoint_descriptor *fs, > >> > - struct usb_endpoint_descriptor *hs) > >> > + struct usb_endpoint_descriptor *hs, > >> > + struct usb_endpoint_descriptor *ss) > >> > { > >> > + if (g->speed >= USB_SPEED_SUPER) > >> > + return ss; > >> > >> I can see that this looks similar to what we have in Linux with > >> commit 4bb99b7c82ba ("usb: gadget: storage: add superspeed support") > >> > >> However, Linux uses the following diff instead: > >> + if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER) > >> + return ss; > >> > >> Is there a reason for not doing the same here? > > > > The gadget_is_superspeed() is a hardware capability check. It's > > redundant when we're already checking g->speed for connected speed. > > So does that mean that the dualspeed conditional just below is doing a > redundant check as well? No, uboot's version of gadget_is_dualspeed() is a compile-time CONFIG_USB_GADGET_DUALSPEED check. So it's not the same. > > As for the previous comment, I'd prefer if we can stay closer to the > Linux code. > > If we can't, I'd like to see a strong justification for not doing so. > Fair point, no strong justification to diverge. I'll match Linux in v3. Thanks, Thinh
