Hi Zixun, Thank you for the patch.
On Mon, Jun 02, 2025 at 17:45, Zixun LI <[email protected]> wrote: I'm surprised that checkpatch.pl does not catch this, but the subject (title) is too long (it should be less than 70 chars): https://docs.u-boot.org/en/latest/develop/sending_patches.html#commit-message-conventions > Contrary to the datasheet, setting both DETACH and PULLD_DIS bits to 1 > does not always drive the DP and DM lines to high-impedance. This > prevents the host from reliably detecting a USB disconnect and subsequent > reconnect. > > The symptom is that the first gadget command (e.g., dhcp) succeeds, while > subsequent commands (e.g., nfs) fail. > > Disabling and re-enabling the controller entirely, instead of toggling the > PULLD_DIS bit, reliably generates a disconnect event. > > The Linux driver works correctly because gadget_disconnect/gadget_connect > are always followed by gadget_udc_start/gadget_udc_stop. In U-Boot > pullup() is used solely. > > This behavior has been observed on the SAM9X60-Curiosity and > AT91SAM9G25-EK boards and has been reported to Microchip. I'm not against this, but the driver supports multiple gadget families according to the compatible. I'm wondering if this issue is reproduced on other atmel boards that are supported in U-Boot. Eugen, have you noticed the above limitation on a different board family that you maintain? If this issue is chip specific, I think we should test for compatible in usba_udc_pullup() to only use USBA_ENABLE/USBA_DISABLE for this particular compatible (microchip,sam9x60-udc). > > Signed-off-by: Zixun LI <[email protected]> > --- > drivers/usb/gadget/atmel_usba_udc.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/atmel_usba_udc.c > b/drivers/usb/gadget/atmel_usba_udc.c > index > f9326f0a7e7d913bcf201ba3e4aeca9a099e7be7..a38e70957402fb3ade3de611d73e29b990d00703 > 100644 > --- a/drivers/usb/gadget/atmel_usba_udc.c > +++ b/drivers/usb/gadget/atmel_usba_udc.c > @@ -521,16 +521,11 @@ usba_udc_set_selfpowered(struct usb_gadget *gadget, int > is_selfpowered) > static int usba_udc_pullup(struct usb_gadget *gadget, int is_on) > { > struct usba_udc *udc = to_usba_udc(gadget); > - u32 ctrl; > - > - ctrl = usba_readl(udc, CTRL); > > if (is_on) > - ctrl &= ~USBA_DETACH; > + usba_writel(udc, CTRL, USBA_ENABLE_MASK); Could we add a comment in the function as well? Something similar to what's in the commit message: /* Some chips don't reliably drive DP/DM lines to high impedance when * using the DETACH/PULLD_DIS bits. * To ensure a reliable disconnect, power cycle the controller instead */ Feel free to rephrase your preference. > else > - ctrl |= USBA_DETACH; > - > - usba_writel(udc, CTRL, ctrl); > + usba_writel(udc, CTRL, USBA_DISABLE_MASK); > > return 0; > } > > --- > base-commit: 4d3b5c679bc9d5c6cbbeedcc1e4a186f1cc35541 > change-id: 20250602-pullup-83c5540db5cd > > Best regards, > -- > Zixun LI <[email protected]>
