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. > >> >> > + >> > + 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? 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. > > BR, > Thinh > >> >> > if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH) >> > return hs; >> > return fs; >> > -- >> > 2.53.0
