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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

On Wed, Jun 04, 2025 at 16:10, Zixun LI <admin@hifiphile.com> wrote:

> Hi all,
>
> The case has been reported with No.01589331.
>
> In brief line high-impedance is not observed when both PULLD_DIS &
> DETACH bits are 1:
> - In unenumerated state (no cable attached) DP is still pulled up
> - In enumerated state, disconnection is not detected by host [1] [2]
> only by [3].
>
> The test has been done in 3 ways on SAM9X60-Curiosity and AT91SAM9G25-EK boards:
> - Modify UDPHS_CTRL on u-boot-at91 with mw command
> - Modify UDPHS_CTRL on
> linux4sam-poky-sam9x60_curiosity-headless-2024.10 with devmem2 utils
> - On linux-mchp 6.6, comment out gadget_udc_start/stop in
> soft_connect_store() of drivers/usb/gadget/udc/core.c,
> leaving only gadget_connect/disconnect. Then run echo disconnect >
> /sys/class/udc/500000.gadget/soft_connect
>
> Host used:
> 1. AMD Ryzen 5800H native port, Linux 6.14
> 2. Intel i7-8750H native port, Windows 10
> 3. Genesys GL3523 hub attached to [1]
>
> Zixun LI
>
> On Jun 4, 2025, at 11:51, Eugen Hristev <eugen.hristev@linaro.org> wrote:
>>
>>
>>
>> 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@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.

Mihai, Zixun, do you have any update on case No.01589331 ?

I'd like to pick this up and some point so please keep me informed.

Thank you
Mattijs

>>
>> 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@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>
>>
>>