Re: [PATCH v3] usb: dwc3: core: Do core softreset when switch mode

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> (Issue history:
>> https://urldefense.com/v3/__https://github.com/andy-shev/linux/issues/31__;!!A4F2R9G_pg!Oa6XGH3IqY3wwG5KK4FwPuNA0m3q5bRj7N6vdP-y4sAY6mya-96J90NJ0tJnXNc7KgAw$
>> )
>> 
>> On the PC side this resulted to:
>> 
>> apr 17 18:17:44 delfion kernel: usb 1-5: new high-speed USB device
>> number 12 using xhci_hcd
>> apr 17 18:17:44 delfion kernel: usb 1-5: New USB device found,
>> idVendor=1d6b, idProduct=0104, bcdDevice= 1.00
>> apr 17 18:17:44 delfion kernel: usb 1-5: New USB device strings: Mfr=1,
>> Product=2, SerialNumber=3
>> apr 17 18:17:44 delfion kernel: usb 1-5: Product: USBArmory Gadget
>> apr 17 18:17:44 delfion kernel: usb 1-5: Manufacturer: USBArmory
>> apr 17 18:17:44 delfion kernel: usb 1-5: SerialNumber: 0123456789abcdef
>> apr 17 18:17:49 delfion kernel: usb 1-5: can't set config #1, error -110
>> 
>> 
>> Thanks for all your help!
>> 
>
> Looks like it's LPM related again. To confirm, try this:
> Disable LPM with this property "snps,usb2-gadget-lpm-disable"
> (Note that it's not the same as "snps,dis_enblslpm_quirk")
>
> Make sure that your testing kernel has this patch [1]
> 475e8be53d04 ("usb: dwc3: gadget: Check for disabled LPM quirk")
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=475e8be53d0496f9bc6159f4abb3ff5f9b90e8de
>
>
> The failure you saw was probably due the gadget function attempting
> to start a delayed status stage of the SET_CONFIGURATION request.
> By this time, the host already put the device in low power.
>
> The START_TRANSFER command needs to be executed while the device
> is on "ON" state (or U0 if eSS). We shouldn't use dwc->link_state
> to check for link state because we only enable link state change
> interrupt for some controller versions.
>
> Once you confirms disabling LPM works, try this fix:
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6227641f2d31..06cdec79244e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -309,10 +309,14 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>  
>         if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
>                 int             needs_wakeup;
> +               u8              link_state;
>  
> -               needs_wakeup = (dwc->link_state == DWC3_LINK_STATE_U1 ||
> -                               dwc->link_state == DWC3_LINK_STATE_U2 ||
> -                               dwc->link_state == DWC3_LINK_STATE_U3);
> +               reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> +               link_state = DWC3_DSTS_USBLNKST(reg);
> +
> +               needs_wakeup = (link_state == DWC3_LINK_STATE_U1 ||
> +                               link_state == DWC3_LINK_STATE_U2 ||
> +                               link_state == DWC3_LINK_STATE_U3);

this makes sense. We used to track state using the state change
interrupts, but that's long since being disabled. I think, either way,
we need this fix.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]