Re: [PATCH] usb: dwc2: gadget: On disconnect reset device address to zero

[Felipe Balbi <balbi@kernel.org>]

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


Hi again,

Felipe Balbi <balbi@kernel.org> writes:
> Hi,
>
> Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:
>> USB CV driver stack doesn't perform USB RESET after device disconnect/
>> connect, so need to reset to zero DEVADDR field in DCFG to pass
>> enumeration again.
>>
>> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
>> ---
>>  drivers/usb/dwc2/gadget.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 98a4a79e7f6e..deb3d901b99d 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3174,6 +3174,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>  		return;
>>  
>>  	hsotg->connected = 0;
>> +	/* On disconnect reset device address to zero */
>> +	__bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK);
>> +
>
> Which of the tests are you talking about? Which particular USB CV are
> you running?
>
> I don't remember ever seeing this with dwc3. How should I go about
> triggering this problem? If this was really the case, then we would have
> this on *all* UDCs.
>
> I just did a fresh install of USB 3 Gen X CV (that I just download from
> usb.org). Ran Chapter 9 tests against a HS dwc3 board I have around and
> I can't see the problem you're talking about.
>
> Here are all my non-endpoint interrupt events in order. Test passes
> fine. If disconnect, reconnect and run the tests again, then Reset will
> be driven on the bus which will cause address to be reset.
>
> Can you share more details about the problem you're facing?

I've been looking at dwc2 for a while and I think this is a bug in
dwc2_hsotg_irq() on the branch for GINTSTS_USBRST. I don't have docs for
dwc2.

Nowhere in that function is driver making sure DCFG_DEVADDR is cleared
to 0. In fact, there nowhere at all in the driver where you're making
sure to reset device address:

$ git --no-pager grep --color -nH -e DCFG_DEVADDR_ -- drivers/usb/dwc2/
drivers/usb/dwc2/gadget.c:1837:			dcfg &= ~DCFG_DEVADDR_MASK;
drivers/usb/dwc2/gadget.c:1839:				 DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK;
drivers/usb/dwc2/hw.h:426:#define DCFG_DEVADDR_MASK		(0x7f << 4)
drivers/usb/dwc2/hw.h:427:#define DCFG_DEVADDR_SHIFT		4
drivers/usb/dwc2/hw.h:428:#define DCFG_DEVADDR_LIMIT		0x7f

If we look into gadget.c on those lines, here's what we have:

static void dwc2_hsotg_process_control(struct dwc2_hsotg *hsotg,
				       struct usb_ctrlrequest *ctrl)
{

[...]

	struct dwc2_hsotg_ep *ep0 = hsotg->eps_out[0];
	int ret = 0;
	u32 dcfg;

	dev_dbg(hsotg->dev,
		"ctrl Type=%02x, Req=%02x, V=%04x, I=%04x, L=%04x\n",
		ctrl->bRequestType, ctrl->bRequest, ctrl->wValue,
		ctrl->wIndex, ctrl->wLength);

	if (ctrl->wLength == 0) {
		ep0->dir_in = 1;
		hsotg->ep0_state = DWC2_EP0_STATUS_IN;
	} else if (ctrl->bRequestType & USB_DIR_IN) {
		ep0->dir_in = 1;
		hsotg->ep0_state = DWC2_EP0_DATA_IN;
	} else {
		ep0->dir_in = 0;
		hsotg->ep0_state = DWC2_EP0_DATA_OUT;
	}

	if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
		switch (ctrl->bRequest) {
		case USB_REQ_SET_ADDRESS:
			hsotg->connected = 1;
			dcfg = dwc2_readl(hsotg->regs + DCFG);
			dcfg &= ~DCFG_DEVADDR_MASK;
			dcfg |= (le16_to_cpu(ctrl->wValue) <<
				 DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK;
			dwc2_writel(dcfg, hsotg->regs + DCFG);

			dev_info(hsotg->dev, "new address %d\n", ctrl->wValue);

			ret = dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0);
			return;

[...]
}

So you only touch DEVADDR from SetAddress. That's clearly a bug
elsewhere in the driver and is *NOT* related to USB CV at all. Please
fix this driver properly instead of hacking around unrelated functions.

Disconnect IRQ is *NOT* supposed to clear device address. That's a job
for Reset IRQ.

-- 
pbalbi

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