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

[Eugen Hristev]

On 6/4/25 11:20, Mattijs Korpershoek wrote:
> Hi Zixun,
> 
> Thank you for the patch.
> 
> On Mon, Jun 02, 2025 at 17:45, Zixun LI <admin at 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?

I have not seen this before, or don't remember about it.

Adding Mihai from Microchip team to see if we can get more info, whether
this is a general gadget thing or it's specific to these chipsets. If it
was reported to Microchip it would be interesting to see the response.

Eugen

> 
> 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 at 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 at hifiphile.com>