Re: [PATCH] usb: gadget: atmel: reliably generate disconnect by disabling controller instead of toggling PULLD_DIS

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Zixun,

Thank you for the patch.

On Mon, Jun 02, 2025 at 17:45, Zixun LI <admin@hifiphile.com> 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 <admin@hifiphile.com>
> ---
>  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 <admin@hifiphile.com>