Hi Thinh, On Thu, Sep 03, 2026 at 02:37, Thinh Nguyen <[email protected]> wrote:
> Hi Mattijs, > > (Sorry for the delay in response) No worries, thanks for following up! > > 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. Ack, understood. > >> >> 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. Thank you. Looking forward for v3. > > Thanks, > Thinh
