[PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Marcelo Roberto Jimenez]

The regression occurs on the second time a self powered gadget is
connected to the host. In this situation, the gadget no loger accepts
an address in the USB enumeration phase.

The regression has been introduced in
commit 70a7f8be8598 ("usb: gadget: atmel: support USB suspend")

Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>
---

Hi,

I have been using an ACME Arietta board (Microchip AT91SAM9G25 MPU, ARM9@400Mhz) for some years, and from time to time I need to do a kernel upgrade to fix issues or introduce new required features.

I have recently noticed a regression in the ethernet over USB Gadget. The system boots fine and when the device is first connected to a host it is recognized. But upon disconnection, the second time the device is connected to the host, it is no longer recognized. Of course, the gadget is self powered.

I did a "git bisect" and found that the patch introducing the regression is this:

commit 70a7f8be85986a3c2ffc7274c41b89552dfe2ad0
Author: Jonas Bonn <jonas@norrbonn.se>
Date:   Wed Feb 20 13:20:00 2019 +0100
    usb: gadget: atmel: support USB suspend
    This patch adds support for USB suspend to the Atmel UDC.

I understand that supporting USB suspend state is important for power consumption, but something in this patch has broken the hardware behavior, so my suggestion is to revert it until we can figure out what is going wrong.

I have tested and confirm that reverting that patch makes the issue go away.

On the host side, I can see some log messages that indicate that the USB gadget is not accepting a new address in the enumeration phase. The following log shows the successful initial connection and disconnection, and then the failed second connection attempt:

usb 1-4.3: new high-speed USB device number 85 using xhci_hcd
usb 1-4.3: New USB device found, idVendor=0525, idProduct=a4a2, bcdDevice= 5.10
usb 1-4.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
usb 1-4.3: Product: RNDIS/Ethernet Gadget
usb 1-4.3: Manufacturer: Linux 5.10.80-linux4microchip-2021.10-rc2+ with atmel_usba_udc
cdc_subset: probe of 1-4.3:1.0 failed with error -22
cdc_ether 1-4.3:1.0 usb0: register 'cdc_ether' at usb-0000:00:14.0-4.3, CDC Ethernet Device, 4a:85:ff:68:fa:82
cdc_ether 1-4.3:1.0 enp0s20f0u4u3: renamed from usb0
usb 1-4.3: USB disconnect, device number 85
cdc_ether 1-4.3:1.0 enp0s20f0u4u3: unregister 'cdc_ether' usb-0000:00:14.0-4.3, CDC Ethernet Device
usb 1-4.3: new high-speed USB device number 86 using xhci_hcd
usb 1-4.3: device descriptor read/64, error -71
usb 1-4.3: device descriptor read/64, error -71
usb 1-4.3: new high-speed USB device number 87 using xhci_hcd
usb 1-4.3: device descriptor read/64, error -71
usb 1-4.3: device descriptor read/64, error -71
usb 1-4-port3: attempt power cycle
usb 1-4.3: new high-speed USB device number 88 using xhci_hcd
usb 1-4.3: Device not responding to setup address.
usb 1-4.3: Device not responding to setup address.
usb 1-4.3: device not accepting address 88, error -71
usb 1-4.3: new high-speed USB device number 89 using xhci_hcd
usb 1-4.3: Device not responding to setup address.
usb 1-4.3: Device not responding to setup address.
usb 1-4.3: device not accepting address 89, error -71
usb 1-4-port3: unable to enumerate USB device

The steps to reproduce are very easy:
1. Have both the host and the gadget turned on.
2. Connect the gadget to an USB port.
3. Disconnect the gadget from the USB port.
4. Reconnect the gadget to the USB port.
5. The gadget is no longer recognized.

On a side note, the ACME Arietta board DTD does not use VBUS sensing by default, although the hardware diagram shows a provision to do that using the PB16 GPIO. I have tried to enable that via DTD, but there seems to be a hardware design flaw (assuming that the intention was to provide VBUS sensing) due to a connection with the processor SHDN pin, which is from the processor shutdown controller, and the 3.3 volts voltage regulator enable pin. The problem is that even when the USB is disconnected, the voltage never gets near enough zero to trigger the GPIO interrupt, which I could verify that would indeed happen by a temporary short of the (unconnected) USB VBUS pin to the ground. That was only to say that even without VBUS sensing, the device has always worked, and that enabling VBUS sensing is not a viable solution in the current project.

Best regards,
Marcelo.


 drivers/usb/gadget/udc/atmel_usba_udc.c | 55 +++----------------------
 drivers/usb/gadget/udc/atmel_usba_udc.h |  1 -
 2 files changed, 6 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 2b893bceea45..54cdea5cc31c 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1705,9 +1705,6 @@ static void usba_dma_irq(struct usba_udc *udc, struct usba_ep *ep)
 	}
 }
 
-static int start_clock(struct usba_udc *udc);
-static void stop_clock(struct usba_udc *udc);
-
 static irqreturn_t usba_udc_irq(int irq, void *devid)
 {
 	struct usba_udc *udc = devid;
@@ -1722,13 +1719,10 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	DBG(DBG_INT, "irq, status=%#08x\n", status);
 
 	if (status & USBA_DET_SUSPEND) {
-		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND|USBA_WAKE_UP);
-		usba_int_enb_set(udc, USBA_WAKE_UP);
-		usba_int_enb_clear(udc, USBA_DET_SUSPEND);
-		udc->suspended = true;
 		toggle_bias(udc, 0);
+		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
+		usba_int_enb_set(udc, USBA_WAKE_UP);
 		udc->bias_pulse_needed = true;
-		stop_clock(udc);
 		DBG(DBG_BUS, "Suspend detected\n");
 		if (udc->gadget.speed != USB_SPEED_UNKNOWN
 				&& udc->driver && udc->driver->suspend) {
@@ -1739,17 +1733,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	}
 
 	if (status & USBA_WAKE_UP) {
-		start_clock(udc);
 		toggle_bias(udc, 1);
 		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
+		usba_int_enb_clear(udc, USBA_WAKE_UP);
 		DBG(DBG_BUS, "Wake Up CPU detected\n");
 	}
 
 	if (status & USBA_END_OF_RESUME) {
-		udc->suspended = false;
 		usba_writel(udc, INT_CLR, USBA_END_OF_RESUME);
-		usba_int_enb_clear(udc, USBA_WAKE_UP);
-		usba_int_enb_set(udc, USBA_DET_SUSPEND);
 		generate_bias_pulse(udc);
 		DBG(DBG_BUS, "Resume detected\n");
 		if (udc->gadget.speed != USB_SPEED_UNKNOWN
@@ -1764,8 +1755,6 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	if (dma_status) {
 		int i;
 
-		usba_int_enb_set(udc, USBA_DET_SUSPEND);
-
 		for (i = 1; i <= USBA_NR_DMAS; i++)
 			if (dma_status & (1 << i))
 				usba_dma_irq(udc, &udc->usba_ep[i]);
@@ -1775,8 +1764,6 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	if (ep_status) {
 		int i;
 
-		usba_int_enb_set(udc, USBA_DET_SUSPEND);
-
 		for (i = 0; i < udc->num_ep; i++)
 			if (ep_status & (1 << i)) {
 				if (ep_is_control(&udc->usba_ep[i]))
@@ -1790,9 +1777,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 		struct usba_ep *ep0, *ep;
 		int i;
 
-		usba_writel(udc, INT_CLR,
-			USBA_END_OF_RESET|USBA_END_OF_RESUME
-			|USBA_DET_SUSPEND|USBA_WAKE_UP);
+		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
 		generate_bias_pulse(udc);
 		reset_all_endpoints(udc);
 
@@ -1819,11 +1804,6 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 				| USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE)));
 		usba_ep_writel(ep0, CTL_ENB,
 				USBA_EPT_ENABLE | USBA_RX_SETUP);
-
-		/* If we get reset while suspended... */
-		udc->suspended = false;
-		usba_int_enb_clear(udc, USBA_WAKE_UP);
-
 		usba_int_enb_set(udc, USBA_BF(EPT_INT, 1) |
 				      USBA_DET_SUSPEND | USBA_END_OF_RESUME);
 
@@ -1896,19 +1876,9 @@ static int usba_start(struct usba_udc *udc)
 	if (ret)
 		return ret;
 
-	if (udc->suspended)
-		return 0;
-
 	spin_lock_irqsave(&udc->lock, flags);
 	toggle_bias(udc, 1);
 	usba_writel(udc, CTRL, USBA_ENABLE_MASK);
-	/* Clear all requested and pending interrupts... */
-	usba_writel(udc, INT_ENB, 0);
-	udc->int_enb_cache = 0;
-	usba_writel(udc, INT_CLR,
-		USBA_END_OF_RESET|USBA_END_OF_RESUME
-		|USBA_DET_SUSPEND|USBA_WAKE_UP);
-	/* ...and enable just 'reset' IRQ to get us started */
 	usba_int_enb_set(udc, USBA_END_OF_RESET);
 	spin_unlock_irqrestore(&udc->lock, flags);
 
@@ -1919,9 +1889,6 @@ static void usba_stop(struct usba_udc *udc)
 {
 	unsigned long flags;
 
-	if (udc->suspended)
-		return;
-
 	spin_lock_irqsave(&udc->lock, flags);
 	udc->gadget.speed = USB_SPEED_UNKNOWN;
 	reset_all_endpoints(udc);
@@ -1949,7 +1916,6 @@ static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
 		if (vbus) {
 			usba_start(udc);
 		} else {
-			udc->suspended = false;
 			if (udc->driver->disconnect)
 				udc->driver->disconnect(&udc->gadget);
 
@@ -2028,7 +1994,6 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
 	if (udc->vbus_pin)
 		disable_irq(gpiod_to_irq(udc->vbus_pin));
 
-	udc->suspended = false;
 	usba_stop(udc);
 
 	udc->driver = NULL;
@@ -2393,7 +2358,6 @@ static int usba_udc_suspend(struct device *dev)
 	mutex_lock(&udc->vbus_mutex);
 
 	if (!device_may_wakeup(dev)) {
-		udc->suspended = false;
 		usba_stop(udc);
 		goto out;
 	}
@@ -2403,13 +2367,10 @@ static int usba_udc_suspend(struct device *dev)
 	 * to request vbus irq, assuming always on.
 	 */
 	if (udc->vbus_pin) {
-		/* FIXME: right to stop here...??? */
 		usba_stop(udc);
 		enable_irq_wake(gpiod_to_irq(udc->vbus_pin));
 	}
 
-	enable_irq_wake(udc->irq);
-
 out:
 	mutex_unlock(&udc->vbus_mutex);
 	return 0;
@@ -2423,12 +2384,8 @@ static int usba_udc_resume(struct device *dev)
 	if (!udc->driver)
 		return 0;
 
-	if (device_may_wakeup(dev)) {
-		if (udc->vbus_pin)
-			disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
-
-		disable_irq_wake(udc->irq);
-	}
+	if (device_may_wakeup(dev) && udc->vbus_pin)
+		disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
 
 	/* If Vbus is present, enable the controller and wait for reset */
 	mutex_lock(&udc->vbus_mutex);
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index 620472f218bc..b44873148393 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -343,7 +343,6 @@ struct usba_udc {
 	struct usba_ep *usba_ep;
 	bool bias_pulse_needed;
 	bool clocked;
-	bool suspended;
 	bool ep_prealloc;
 
 	u16 devstatus;
-- 
2.30.2

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Thorsten Leemhuis]

[TLDR: adding this regression to regzbot; most of this mail is compiled
from a few templates paragraphs some of you might have seen already.]

Hi, this is your Linux kernel regression tracker speaking.

Top-posting for once, to make this easy accessible to everyone.

Thanks for the report. BTW, if anyone like me wonders what the included
patch is about: it afaics is the revert of the patch causing the
problem. Anyway:

To be sure this issue doesn't fall through the cracks unnoticed, I'm
adding it to regzbot, my Linux kernel regression tracking bot:

#regzbot ^introduced 70a7f8be8598
#regzbot title usb: gadget: atmel: ethernet over USB Gadget not
recognized anymore after disconnect and reconnect
#regzbot ignore-activity

Reminder: when fixing the issue, please add a 'Link:' tag with the URL
to the report (the parent of this mail), then regzbot will automatically
mark the regression as resolved once the fix lands in the appropriate
tree. For more details about regzbot see footer.

Sending this to everyone that got the initial report, to make all aware
of the tracking. I also hope that messages like this motivate people to
directly get at least the regression mailing list and ideally even
regzbot involved when dealing with regressions, as messages like this
wouldn't be needed then.

Don't worry, I'll send further messages wrt to this regression just to
the lists (with a tag in the subject so people can filter them away), as
long as they are intended just for regzbot. With a bit of luck no such
messages will be needed anyway.

Ciao, Thorsten

P.S.: As a Linux kernel regression tracker I'm getting a lot of reports
on my table. I can only look briefly into most of them. Unfortunately
therefore I sometimes will get things wrong or miss something important.
I hope that's not the case here; if you think it is, don't hesitate to
tell me about it in a public reply. That's in everyone's interest, as
what I wrote above might be misleading to everyone reading this; any
suggestion I gave thus might sent someone reading this down the wrong
rabbit hole, which none of us wants.

BTW, I have no personal interest in this issue, which is tracked using
regzbot, my Linux kernel regression tracking bot
(https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting
this mail to get things rolling again and hence don't need to be CC on
all further activities wrt to this regression.



On 11.12.21 19:36, Marcelo Roberto Jimenez wrote:
> The regression occurs on the second time a self powered gadget is
> connected to the host. In this situation, the gadget no loger accepts
> an address in the USB enumeration phase.
> 
> The regression has been introduced in
> commit 70a7f8be8598 ("usb: gadget: atmel: support USB suspend")
> 
> Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>
> ---
> 
> Hi,
> 
> I have been using an ACME Arietta board (Microchip AT91SAM9G25 MPU, ARM9@400Mhz) for some years, and from time to time I need to do a kernel upgrade to fix issues or introduce new required features.
> 
> I have recently noticed a regression in the ethernet over USB Gadget. The system boots fine and when the device is first connected to a host it is recognized. But upon disconnection, the second time the device is connected to the host, it is no longer recognized. Of course, the gadget is self powered.
> 
> I did a "git bisect" and found that the patch introducing the regression is this:
> 
> commit 70a7f8be85986a3c2ffc7274c41b89552dfe2ad0
> Author: Jonas Bonn <jonas@norrbonn.se>
> Date:   Wed Feb 20 13:20:00 2019 +0100
>     usb: gadget: atmel: support USB suspend
>     This patch adds support for USB suspend to the Atmel UDC.
> 
> I understand that supporting USB suspend state is important for power consumption, but something in this patch has broken the hardware behavior, so my suggestion is to revert it until we can figure out what is going wrong.
> 
> I have tested and confirm that reverting that patch makes the issue go away.
> 
> On the host side, I can see some log messages that indicate that the USB gadget is not accepting a new address in the enumeration phase. The following log shows the successful initial connection and disconnection, and then the failed second connection attempt:
> 
> usb 1-4.3: new high-speed USB device number 85 using xhci_hcd
> usb 1-4.3: New USB device found, idVendor=0525, idProduct=a4a2, bcdDevice= 5.10
> usb 1-4.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> usb 1-4.3: Product: RNDIS/Ethernet Gadget
> usb 1-4.3: Manufacturer: Linux 5.10.80-linux4microchip-2021.10-rc2+ with atmel_usba_udc
> cdc_subset: probe of 1-4.3:1.0 failed with error -22
> cdc_ether 1-4.3:1.0 usb0: register 'cdc_ether' at usb-0000:00:14.0-4.3, CDC Ethernet Device, 4a:85:ff:68:fa:82
> cdc_ether 1-4.3:1.0 enp0s20f0u4u3: renamed from usb0
> usb 1-4.3: USB disconnect, device number 85
> cdc_ether 1-4.3:1.0 enp0s20f0u4u3: unregister 'cdc_ether' usb-0000:00:14.0-4.3, CDC Ethernet Device
> usb 1-4.3: new high-speed USB device number 86 using xhci_hcd
> usb 1-4.3: device descriptor read/64, error -71
> usb 1-4.3: device descriptor read/64, error -71
> usb 1-4.3: new high-speed USB device number 87 using xhci_hcd
> usb 1-4.3: device descriptor read/64, error -71
> usb 1-4.3: device descriptor read/64, error -71
> usb 1-4-port3: attempt power cycle
> usb 1-4.3: new high-speed USB device number 88 using xhci_hcd
> usb 1-4.3: Device not responding to setup address.
> usb 1-4.3: Device not responding to setup address.
> usb 1-4.3: device not accepting address 88, error -71
> usb 1-4.3: new high-speed USB device number 89 using xhci_hcd
> usb 1-4.3: Device not responding to setup address.
> usb 1-4.3: Device not responding to setup address.
> usb 1-4.3: device not accepting address 89, error -71
> usb 1-4-port3: unable to enumerate USB device
> 
> The steps to reproduce are very easy:
> 1. Have both the host and the gadget turned on.
> 2. Connect the gadget to an USB port.
> 3. Disconnect the gadget from the USB port.
> 4. Reconnect the gadget to the USB port.
> 5. The gadget is no longer recognized.
> 
> On a side note, the ACME Arietta board DTD does not use VBUS sensing by default, although the hardware diagram shows a provision to do that using the PB16 GPIO. I have tried to enable that via DTD, but there seems to be a hardware design flaw (assuming that the intention was to provide VBUS sensing) due to a connection with the processor SHDN pin, which is from the processor shutdown controller, and the 3.3 volts voltage regulator enable pin. The problem is that even when the USB is disconnected, the voltage never gets near enough zero to trigger the GPIO interrupt, which I could verify that would indeed happen by a temporary short of the (unconnected) USB VBUS pin to the ground. That was only to say that even without VBUS sensing, the device has always worked, and that enabling VBUS sensing is not a viable solution in the current project.
> 
> Best regards,
> Marcelo.
> 
> 
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 55 +++----------------------
>  drivers/usb/gadget/udc/atmel_usba_udc.h |  1 -
>  2 files changed, 6 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 2b893bceea45..54cdea5cc31c 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1705,9 +1705,6 @@ static void usba_dma_irq(struct usba_udc *udc, struct usba_ep *ep)
>  	}
>  }
>  
> -static int start_clock(struct usba_udc *udc);
> -static void stop_clock(struct usba_udc *udc);
> -
>  static irqreturn_t usba_udc_irq(int irq, void *devid)
>  {
>  	struct usba_udc *udc = devid;
> @@ -1722,13 +1719,10 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  	DBG(DBG_INT, "irq, status=%#08x\n", status);
>  
>  	if (status & USBA_DET_SUSPEND) {
> -		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND|USBA_WAKE_UP);
> -		usba_int_enb_set(udc, USBA_WAKE_UP);
> -		usba_int_enb_clear(udc, USBA_DET_SUSPEND);
> -		udc->suspended = true;
>  		toggle_bias(udc, 0);
> +		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
> +		usba_int_enb_set(udc, USBA_WAKE_UP);
>  		udc->bias_pulse_needed = true;
> -		stop_clock(udc);
>  		DBG(DBG_BUS, "Suspend detected\n");
>  		if (udc->gadget.speed != USB_SPEED_UNKNOWN
>  				&& udc->driver && udc->driver->suspend) {
> @@ -1739,17 +1733,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  	}
>  
>  	if (status & USBA_WAKE_UP) {
> -		start_clock(udc);
>  		toggle_bias(udc, 1);
>  		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
> +		usba_int_enb_clear(udc, USBA_WAKE_UP);
>  		DBG(DBG_BUS, "Wake Up CPU detected\n");
>  	}
>  
>  	if (status & USBA_END_OF_RESUME) {
> -		udc->suspended = false;
>  		usba_writel(udc, INT_CLR, USBA_END_OF_RESUME);
> -		usba_int_enb_clear(udc, USBA_WAKE_UP);
> -		usba_int_enb_set(udc, USBA_DET_SUSPEND);
>  		generate_bias_pulse(udc);
>  		DBG(DBG_BUS, "Resume detected\n");
>  		if (udc->gadget.speed != USB_SPEED_UNKNOWN
> @@ -1764,8 +1755,6 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  	if (dma_status) {
>  		int i;
>  
> -		usba_int_enb_set(udc, USBA_DET_SUSPEND);
> -
>  		for (i = 1; i <= USBA_NR_DMAS; i++)
>  			if (dma_status & (1 << i))
>  				usba_dma_irq(udc, &udc->usba_ep[i]);
> @@ -1775,8 +1764,6 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  	if (ep_status) {
>  		int i;
>  
> -		usba_int_enb_set(udc, USBA_DET_SUSPEND);
> -
>  		for (i = 0; i < udc->num_ep; i++)
>  			if (ep_status & (1 << i)) {
>  				if (ep_is_control(&udc->usba_ep[i]))
> @@ -1790,9 +1777,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  		struct usba_ep *ep0, *ep;
>  		int i;
>  
> -		usba_writel(udc, INT_CLR,
> -			USBA_END_OF_RESET|USBA_END_OF_RESUME
> -			|USBA_DET_SUSPEND|USBA_WAKE_UP);
> +		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>  		generate_bias_pulse(udc);
>  		reset_all_endpoints(udc);
>  
> @@ -1819,11 +1804,6 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  				| USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE)));
>  		usba_ep_writel(ep0, CTL_ENB,
>  				USBA_EPT_ENABLE | USBA_RX_SETUP);
> -
> -		/* If we get reset while suspended... */
> -		udc->suspended = false;
> -		usba_int_enb_clear(udc, USBA_WAKE_UP);
> -
>  		usba_int_enb_set(udc, USBA_BF(EPT_INT, 1) |
>  				      USBA_DET_SUSPEND | USBA_END_OF_RESUME);
>  
> @@ -1896,19 +1876,9 @@ static int usba_start(struct usba_udc *udc)
>  	if (ret)
>  		return ret;
>  
> -	if (udc->suspended)
> -		return 0;
> -
>  	spin_lock_irqsave(&udc->lock, flags);
>  	toggle_bias(udc, 1);
>  	usba_writel(udc, CTRL, USBA_ENABLE_MASK);
> -	/* Clear all requested and pending interrupts... */
> -	usba_writel(udc, INT_ENB, 0);
> -	udc->int_enb_cache = 0;
> -	usba_writel(udc, INT_CLR,
> -		USBA_END_OF_RESET|USBA_END_OF_RESUME
> -		|USBA_DET_SUSPEND|USBA_WAKE_UP);
> -	/* ...and enable just 'reset' IRQ to get us started */
>  	usba_int_enb_set(udc, USBA_END_OF_RESET);
>  	spin_unlock_irqrestore(&udc->lock, flags);
>  
> @@ -1919,9 +1889,6 @@ static void usba_stop(struct usba_udc *udc)
>  {
>  	unsigned long flags;
>  
> -	if (udc->suspended)
> -		return;
> -
>  	spin_lock_irqsave(&udc->lock, flags);
>  	udc->gadget.speed = USB_SPEED_UNKNOWN;
>  	reset_all_endpoints(udc);
> @@ -1949,7 +1916,6 @@ static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
>  		if (vbus) {
>  			usba_start(udc);
>  		} else {
> -			udc->suspended = false;
>  			if (udc->driver->disconnect)
>  				udc->driver->disconnect(&udc->gadget);
>  
> @@ -2028,7 +1994,6 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>  	if (udc->vbus_pin)
>  		disable_irq(gpiod_to_irq(udc->vbus_pin));
>  
> -	udc->suspended = false;
>  	usba_stop(udc);
>  
>  	udc->driver = NULL;
> @@ -2393,7 +2358,6 @@ static int usba_udc_suspend(struct device *dev)
>  	mutex_lock(&udc->vbus_mutex);
>  
>  	if (!device_may_wakeup(dev)) {
> -		udc->suspended = false;
>  		usba_stop(udc);
>  		goto out;
>  	}
> @@ -2403,13 +2367,10 @@ static int usba_udc_suspend(struct device *dev)
>  	 * to request vbus irq, assuming always on.
>  	 */
>  	if (udc->vbus_pin) {
> -		/* FIXME: right to stop here...??? */
>  		usba_stop(udc);
>  		enable_irq_wake(gpiod_to_irq(udc->vbus_pin));
>  	}
>  
> -	enable_irq_wake(udc->irq);
> -
>  out:
>  	mutex_unlock(&udc->vbus_mutex);
>  	return 0;
> @@ -2423,12 +2384,8 @@ static int usba_udc_resume(struct device *dev)
>  	if (!udc->driver)
>  		return 0;
>  
> -	if (device_may_wakeup(dev)) {
> -		if (udc->vbus_pin)
> -			disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
> -
> -		disable_irq_wake(udc->irq);
> -	}
> +	if (device_may_wakeup(dev) && udc->vbus_pin)
> +		disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
>  
>  	/* If Vbus is present, enable the controller and wait for reset */
>  	mutex_lock(&udc->vbus_mutex);
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
> index 620472f218bc..b44873148393 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
> @@ -343,7 +343,6 @@ struct usba_udc {
>  	struct usba_ep *usba_ep;
>  	bool bias_pulse_needed;
>  	bool clocked;
> -	bool suspended;
>  	bool ep_prealloc;
>  
>  	u16 devstatus;
> 

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Greg Kroah-Hartman]

On Sun, Dec 12, 2021 at 10:48:14AM +0100, Thorsten Leemhuis wrote:
> [TLDR: adding this regression to regzbot; most of this mail is compiled
> from a few templates paragraphs some of you might have seen already.]
> 
> Hi, this is your Linux kernel regression tracker speaking.
> 
> Top-posting for once, to make this easy accessible to everyone.
> 
> Thanks for the report. BTW, if anyone like me wonders what the included
> patch is about: it afaics is the revert of the patch causing the
> problem. Anyway:
> 
> To be sure this issue doesn't fall through the cracks unnoticed, I'm
> adding it to regzbot, my Linux kernel regression tracking bot:
> 
> #regzbot ^introduced 70a7f8be8598
> #regzbot title usb: gadget: atmel: ethernet over USB Gadget not
> recognized anymore after disconnect and reconnect
> #regzbot ignore-activity

How do I say "this isn't a regression, it's a broken board
configuration" and get regzbot to stop tracking this?

thanks,

greg k-h

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Thorsten Leemhuis]

On 26.01.22 13:20, Greg Kroah-Hartman wrote:
> On Sun, Dec 12, 2021 at 10:48:14AM +0100, Thorsten Leemhuis wrote:
>> [TLDR: adding this regression to regzbot; most of this mail is compiled
>> from a few templates paragraphs some of you might have seen already.]
>>
>> Hi, this is your Linux kernel regression tracker speaking.
>>
>> Top-posting for once, to make this easy accessible to everyone.
>>
>> Thanks for the report. BTW, if anyone like me wonders what the included
>> patch is about: it afaics is the revert of the patch causing the
>> problem. Anyway:
>>
>> To be sure this issue doesn't fall through the cracks unnoticed, I'm
>> adding it to regzbot, my Linux kernel regression tracking bot:
>>
>> #regzbot ^introduced 70a7f8be8598
>> #regzbot title usb: gadget: atmel: ethernet over USB Gadget not
>> recognized anymore after disconnect and reconnect
>> #regzbot ignore-activity
> 
> How do I say "this isn't a regression, it's a broken board
> configuration" and get regzbot to stop tracking this?

Like this:

#regzbot invalid: not a regression, it's a broken board configuration

And it's hereby done. :-D

TWIMC, this documents explains all regzbot commands:
https://gitlab.com/knurd42/regzbot/-/blob/main/docs/reference.md

Ciao, Thorsten

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Marcelo Roberto Jimenez]

Hi Greg,

On Wed, Jan 26, 2022 at 9:20 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Dec 12, 2021 at 10:48:14AM +0100, Thorsten Leemhuis wrote:
> > [TLDR: adding this regression to regzbot; most of this mail is compiled
> > from a few templates paragraphs some of you might have seen already.]
> >
> > Hi, this is your Linux kernel regression tracker speaking.
> >
> > Top-posting for once, to make this easy accessible to everyone.
> >
> > Thanks for the report. BTW, if anyone like me wonders what the included
> > patch is about: it afaics is the revert of the patch causing the
> > problem. Anyway:
> >
> > To be sure this issue doesn't fall through the cracks unnoticed, I'm
> > adding it to regzbot, my Linux kernel regression tracking bot:
> >
> > #regzbot ^introduced 70a7f8be8598
> > #regzbot title usb: gadget: atmel: ethernet over USB Gadget not
> > recognized anymore after disconnect and reconnect
> > #regzbot ignore-activity
>
> How do I say "this isn't a regression, it's a broken board
> configuration" and get regzbot to stop tracking this?

I sent the logs to Cristian on Jan, 14, but I don't know if they were
helpful. The problem is pretty much reproducible here. I understand
that from the previously quoted specification sentence, the hardware
must perform VBUS sensing, and this is something I can confirm the
hardware is not capable of. But if Cristian cannot reproduce it in a
different SOC configured without a VBUS sensing pin, I can only hope
Microchip/Atmel will look into the issue. On my side, it seems like I
will have to maintain a patched kernel for this hardware.

So, let's move on.

> thanks,
>
> greg k-h

Best regards,
Marcelo.

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Jonas Bonn]

Hi Marcelo,

On 11/12/2021 19:36, Marcelo Roberto Jimenez wrote:
> The regression occurs on the second time a self powered gadget is
> connected to the host. In this situation, the gadget no loger accepts
> an address in the USB enumeration phase.
> 
> The regression has been introduced in
> commit 70a7f8be8598 ("usb: gadget: atmel: support USB suspend")
> 
> Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>
> ---
> 
> Hi,
> 
> I have been using an ACME Arietta board (Microchip AT91SAM9G25 MPU, ARM9@400Mhz) for some years, and from time to time I need to do a kernel upgrade to fix issues or introduce new required features.
> 
> I have recently noticed a regression in the ethernet over USB Gadget. The system boots fine and when the device is first connected to a host it is recognized. But upon disconnection, the second time the device is connected to the host, it is no longer recognized. Of course, the gadget is self powered.
> 
> I did a "git bisect" and found that the patch introducing the regression is this:
> 
> commit 70a7f8be85986a3c2ffc7274c41b89552dfe2ad0
> Author: Jonas Bonn <jonas@norrbonn.se>
> Date:   Wed Feb 20 13:20:00 2019 +0100
>      usb: gadget: atmel: support USB suspend
>      This patch adds support for USB suspend to the Atmel UDC.

Given that the patch that you want to revert is almost 3 years old, it's 
a bit of a stretch to call this a regression.  I suspect that a cleaner 
way forward is to introduce some kind of quirk for your board.

I have a self-powered board where the USB suspend sequence works and 
device add/remove works fine.  So fundamentally, I suspect that the 
driver is ok.

With all that said, I'm not immediately in a position to run tests on my 
SAMA5D2 board right now and the kernel on that board is 5.2.  I'm not 
sure when we we would notice that USB suspend stopped working because 
there is no kernel maintenance planned for that board, as far as I know; 
someday, however, that work might happen and the lack of working USB 
suspend will be a regression in and of itself.

Thanks,
Jonas

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Cristian.Birsan]

Hi Marcelo, Jonas,

On 12/13/21 4:31 PM, Marcelo Roberto Jimenez wrote:
> 	
> Some people who received this message don't often get email from marcelo.jimenez@gmail.com. Learn why this is important <http://aka.ms/LearnAboutSenderIdentification>
> 	
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> Hi Jonas,
> 
> Thank you for the quick response, I really appreciate it.
> 
> On Mon, Dec 13, 2021 at 7:02 AM Jonas Bonn <jonas@norrbonn.se <mailto:jonas@norrbonn.se>> wrote:
> 
> 
>     Given that the patch that you want to revert is almost 3 years old, it's
>     a bit of a stretch to call this a regression.  I suspect that a cleaner
>     way forward is to introduce some kind of quirk for your board.
> 
> 
> Well, the board is basically the MPU, so if there is a hardware problem it would mean that it is a problem with the on chip peripheral.
>  
> 
>     I have a self-powered board where the USB suspend sequence works and
>     device add/remove works fine.  So fundamentally, I suspect that the
>     driver is ok.
> 
> 
> Maybe you have VBUS sensing enabled in your board. As I reported before, the VBUS sensing is not an option in this board. Nonetheless, the code in the kernel suggests that VBUS sensing is optional, since the presence of a VBUS sensing pin is explicitly tested in it.

Is it possible to add a wire that connects VBUS to the right pin on the MPU ? Just to see if this has an impact or not ?

> 
> I have not read the full USB spec, but if this is a fundamentally not resolvable problem, maybe we could have a configuration option, runtime or compile time, to enable or disable SUSPEND state, assuming that there is no problem with the original patch.
> 
> In my particular application, it is more important to detect the disconnection, so that we can reconnect, than to enter SUSPEND. Whenever USB is not necessary, it will not be connected, so the energy saved will be very little in my case.
> 
> My intention with this patch is also to provide a quick fix for anyone facing this problem, reverting is not necessarily the best long term solution, especially if the code is found to be correct. But assuming the chip USB controller has no design flaws, and assuming it should be possible to do without VBUS sensing, then the present code should be missing some detail.
>  

I would prefer to have a clear understanding of what is causing the issue before making any changes to the patch sent by Jonas in the upstream kernel.
Marcelo, can you point where the driver hangs ? One can enable debug messages in the driver by using #define DEBUG_LEVEL DBG_ALL in atmel_usba_udc.h.

> 
>     With all that said, I'm not immediately in a position to run tests on my
>     SAMA5D2 board right now and the kernel on that board is 5.2.  I'm not
>     sure when we we would notice that USB suspend stopped working because
>     there is no kernel maintenance planned for that board, as far as I know;
>     someday, however, that work might happen and the lack of working USB
>     suspend will be a regression in and of itself.

> 
> 
> I can test it with the AT91SAM9G25 processor and I can also get a SAMA5D27 board to test with.

I was able to run the tests on the kernel version pointed by Marcelo (5.10) on SAMA5D2 Xplained and on SAM9x60-EK (the USB IP version on this one should be closer to AT91SAM9G25).
It works on both boards with no issues. Both our boards use VBUS sensing.  Unfortunately, currently I do not have access to a board with AT91SAM9G25 or to ACME Arietta to check / debug on it.

>  
> 
>     Thanks,
>     Jonas
> 
> 
> Best regards,
> Marcelo.
>

Regards,
Cristian

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Marcelo Roberto Jimenez]

Hi Cristian,

On Wed, Dec 15, 2021 at 10:04 AM <Cristian.Birsan@microchip.com> wrote:
>
> Hi Marcelo, Jonas,
>
> On 12/13/21 4:31 PM, Marcelo Roberto Jimenez wrote:
> >
> > Some people who received this message don't often get email from marcelo.jimenez@gmail.com. Learn why this is important <http://aka.ms/LearnAboutSenderIdentification>

Hum, shame on me.

> > Hi Jonas,
> >
> > Thank you for the quick response, I really appreciate it.
> >
> > On Mon, Dec 13, 2021 at 7:02 AM Jonas Bonn <jonas@norrbonn.se <mailto:jonas@norrbonn.se>> wrote:
> >
> >
> >     Given that the patch that you want to revert is almost 3 years old, it's
> >     a bit of a stretch to call this a regression.  I suspect that a cleaner
> >     way forward is to introduce some kind of quirk for your board.
> >
> >
> > Well, the board is basically the MPU, so if there is a hardware problem it would mean that it is a problem with the on chip peripheral.
> >
> >
> >     I have a self-powered board where the USB suspend sequence works and
> >     device add/remove works fine.  So fundamentally, I suspect that the
> >     driver is ok.
> >
> >
> > Maybe you have VBUS sensing enabled in your board. As I reported before, the VBUS sensing is not an option in this board. Nonetheless, the code in the kernel suggests that VBUS sensing is optional, since the presence of a VBUS sensing pin is explicitly tested in it.
>
> Is it possible to add a wire that connects VBUS to the right pin on the MPU ? Just to see if this has an impact or not ?

Yes. Maybe I was not clear about that issue, so let me try to clarify.
The board _seems_ to have a provision for VBUS sensing, but it is not
working. I was not involved in this board's project and I found no
other documentation on the ACME Systems site, all I am saying here is
from reading the circuit diagram, so it is all my personal
interpretation. For hardware reasons, the aforementioned VBUS sensing
pin does not reach zero volts when the USB connector is removed, which
makes VBUS sensing ineffective. But I have tested it anyway and to
make it work, the first step is to prepare the board as specified here
https://www.acmesystems.it/arietta_power_supply in the section "Supply
power at 3.3 volt". The key here is to remove the R36 resistor, so
that the board can be fed by an external 3.3V voltage and become self
powered. Then, you add a line "atmel,vbus-gpio = <&pioB 16 0>;" below
the "usb2:" line in the Arietta DTD. After recompiling the kernel and
installing, it still does not work by just unplugging the USB cable.
You need to manually and carefully (!) short circuit the +5 USB line
to the ground when the cable is not connected. Only then VBUS sensing
will work and the device will accept enumeration again.

In short, the VBUS sensing code in the kernel is ok. But that is not
my point. My point is that the kernel code implies that it is possible
to do without a VBUS sensing pin. The file
Documentation/devicetree/bindings/usb/atmel-usb.txt states that
"atmel,vbus-gpio" is an optional property. So, it seems to me like the
code should work without it, and indeed it worked. That is why I have
called this a regression.

> >
> > I have not read the full USB spec, but if this is a fundamentally not resolvable problem, maybe we could have a configuration option, runtime or compile time, to enable or disable SUSPEND state, assuming that there is no problem with the original patch.
> >
> > In my particular application, it is more important to detect the disconnection, so that we can reconnect, than to enter SUSPEND. Whenever USB is not necessary, it will not be connected, so the energy saved will be very little in my case.
> >
> > My intention with this patch is also to provide a quick fix for anyone facing this problem, reverting is not necessarily the best long term solution, especially if the code is found to be correct. But assuming the chip USB controller has no design flaws, and assuming it should be possible to do without VBUS sensing, then the present code should be missing some detail.
> >
>
> I would prefer to have a clear understanding of what is causing the issue before making any changes to the patch sent by Jonas in the upstream kernel.

Indeed. We must understand properly how the code is changing the state
of the USB state machine. I do not have the knowledge of this
protocol/state machine inner workings and what are the code
assumptions in this regard.

> Marcelo, can you point where the driver hangs ? One can enable debug messages in the driver by using #define DEBUG_LEVEL DBG_ALL in atmel_usba_udc.h.

As far as I know, the driver does not hang. I do not have an USB
analyzer to check the protocol, but from the logs on the host side,
the gadget code seems to be rejecting enumeration the second time it
is plugged, and my assumption is that this is due to not having
detected the disconnection.

> >
> >     With all that said, I'm not immediately in a position to run tests on my
> >     SAMA5D2 board right now and the kernel on that board is 5.2.  I'm not
> >     sure when we we would notice that USB suspend stopped working because
> >     there is no kernel maintenance planned for that board, as far as I know;
> >     someday, however, that work might happen and the lack of working USB
> >     suspend will be a regression in and of itself.
>
> >
> >
> > I can test it with the AT91SAM9G25 processor and I can also get a SAMA5D27 board to test with.
>
> I was able to run the tests on the kernel version pointed by Marcelo (5.10) on SAMA5D2 Xplained and on SAM9x60-EK (the USB IP version on this one should be closer to AT91SAM9G25).
> It works on both boards with no issues. Both our boards use VBUS sensing.  Unfortunately, currently I do not have access to a board with AT91SAM9G25 or to ACME Arietta to check / debug on it.

From what I read in atmel_usba_udc.c, it should be possible to emulate
the absence of VBUS sensing by eliminating the "atmel,vbus" or
"atmel,vbus-gpio" line from the DTD file. So in principle you can test
it with your boards.

> >     Thanks,
> >     Jonas
> >
> >
> > Best regards,
> > Marcelo.
> >
>
> Regards,
> Cristian

Best regards,
Marcelo.

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Greg Kroah-Hartman]

On Wed, Dec 15, 2021 at 12:54:49PM -0300, Marcelo Roberto Jimenez wrote:
> Hi Cristian,
> 
> On Wed, Dec 15, 2021 at 10:04 AM <Cristian.Birsan@microchip.com> wrote:
> >
> > Hi Marcelo, Jonas,
> >
> > On 12/13/21 4:31 PM, Marcelo Roberto Jimenez wrote:
> > >
> > > Some people who received this message don't often get email from marcelo.jimenez@gmail.com. Learn why this is important <http://aka.ms/LearnAboutSenderIdentification>
> 
> Hum, shame on me.
> 
> > > Hi Jonas,
> > >
> > > Thank you for the quick response, I really appreciate it.
> > >
> > > On Mon, Dec 13, 2021 at 7:02 AM Jonas Bonn <jonas@norrbonn.se <mailto:jonas@norrbonn.se>> wrote:
> > >
> > >
> > >     Given that the patch that you want to revert is almost 3 years old, it's
> > >     a bit of a stretch to call this a regression.  I suspect that a cleaner
> > >     way forward is to introduce some kind of quirk for your board.
> > >
> > >
> > > Well, the board is basically the MPU, so if there is a hardware problem it would mean that it is a problem with the on chip peripheral.
> > >
> > >
> > >     I have a self-powered board where the USB suspend sequence works and
> > >     device add/remove works fine.  So fundamentally, I suspect that the
> > >     driver is ok.
> > >
> > >
> > > Maybe you have VBUS sensing enabled in your board. As I reported before, the VBUS sensing is not an option in this board. Nonetheless, the code in the kernel suggests that VBUS sensing is optional, since the presence of a VBUS sensing pin is explicitly tested in it.
> >
> > Is it possible to add a wire that connects VBUS to the right pin on the MPU ? Just to see if this has an impact or not ?
> 
> Yes. Maybe I was not clear about that issue, so let me try to clarify.
> The board _seems_ to have a provision for VBUS sensing, but it is not
> working. I was not involved in this board's project and I found no
> other documentation on the ACME Systems site, all I am saying here is
> from reading the circuit diagram, so it is all my personal
> interpretation. For hardware reasons, the aforementioned VBUS sensing
> pin does not reach zero volts when the USB connector is removed, which
> makes VBUS sensing ineffective. But I have tested it anyway and to
> make it work, the first step is to prepare the board as specified here
> https://www.acmesystems.it/arietta_power_supply in the section "Supply
> power at 3.3 volt". The key here is to remove the R36 resistor, so
> that the board can be fed by an external 3.3V voltage and become self
> powered. Then, you add a line "atmel,vbus-gpio = <&pioB 16 0>;" below
> the "usb2:" line in the Arietta DTD. After recompiling the kernel and
> installing, it still does not work by just unplugging the USB cable.
> You need to manually and carefully (!) short circuit the +5 USB line
> to the ground when the cable is not connected. Only then VBUS sensing
> will work and the device will accept enumeration again.
> 
> In short, the VBUS sensing code in the kernel is ok. But that is not
> my point. My point is that the kernel code implies that it is possible
> to do without a VBUS sensing pin. The file
> Documentation/devicetree/bindings/usb/atmel-usb.txt states that
> "atmel,vbus-gpio" is an optional property. So, it seems to me like the
> code should work without it, and indeed it worked. That is why I have
> called this a regression.

Yes, hardware that used to work, and now does not, is a regression.

So, should I revert the offending patch here?  Adding new features is
not a good reason to keep things that break systems.  Or is there a
potential fix in this thread that I missed?

thanks,

greg k-h

Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[Cristian.Birsan]

Hi,

On 12/20/21 4:50 PM, Greg Kroah-Hartman wrote:
> 
> On Wed, Dec 15, 2021 at 12:54:49PM -0300, Marcelo Roberto Jimenez wrote:
>> Hi Cristian,
>>
>> On Wed, Dec 15, 2021 at 10:04 AM <Cristian.Birsan@microchip.com> wrote:
>>>
>>> Hi Marcelo, Jonas,
>>>
>>> On 12/13/21 4:31 PM, Marcelo Roberto Jimenez wrote:
>>>>
>>>> Some people who received this message don't often get email from marcelo.jimenez@gmail.com. Learn why this is important <http://aka.ms/LearnAboutSenderIdentification>
>>
>> Hum, shame on me.
>>
>>>> Hi Jonas,
>>>>
>>>> Thank you for the quick response, I really appreciate it.
>>>>
>>>> On Mon, Dec 13, 2021 at 7:02 AM Jonas Bonn <jonas@norrbonn.se <mailto:jonas@norrbonn.se>> wrote:
>>>>
>>>>
>>>>     Given that the patch that you want to revert is almost 3 years old, it's
>>>>     a bit of a stretch to call this a regression.  I suspect that a cleaner
>>>>     way forward is to introduce some kind of quirk for your board.
>>>>
>>>>
>>>> Well, the board is basically the MPU, so if there is a hardware problem it would mean that it is a problem with the on chip peripheral.
>>>>
>>>>
>>>>     I have a self-powered board where the USB suspend sequence works and
>>>>     device add/remove works fine.  So fundamentally, I suspect that the
>>>>     driver is ok.
>>>>
>>>>
>>>> Maybe you have VBUS sensing enabled in your board. As I reported before, the VBUS sensing is not an option in this board. Nonetheless, the code in the kernel suggests that VBUS sensing is optional, since the presence of a VBUS sensing pin is explicitly tested in it.
>>>
>>> Is it possible to add a wire that connects VBUS to the right pin on the MPU ? Just to see if this has an impact or not ?
>>
>> Yes. Maybe I was not clear about that issue, so let me try to clarify.
>> The board _seems_ to have a provision for VBUS sensing, but it is not
>> working. I was not involved in this board's project and I found no
>> other documentation on the ACME Systems site, all I am saying here is
>> from reading the circuit diagram, so it is all my personal
>> interpretation. For hardware reasons, the aforementioned VBUS sensing
>> pin does not reach zero volts when the USB connector is removed, which
>> makes VBUS sensing ineffective. But I have tested it anyway and to
>> make it work, the first step is to prepare the board as specified here
>> https://www.acmesystems.it/arietta_power_supply in the section "Supply
>> power at 3.3 volt". The key here is to remove the R36 resistor, so
>> that the board can be fed by an external 3.3V voltage and become self
>> powered. Then, you add a line "atmel,vbus-gpio = <&pioB 16 0>;" below
>> the "usb2:" line in the Arietta DTD. After recompiling the kernel and
>> installing, it still does not work by just unplugging the USB cable.
>> You need to manually and carefully (!) short circuit the +5 USB line
>> to the ground when the cable is not connected. Only then VBUS sensing
>> will work and the device will accept enumeration again.
>>
>> In short, the VBUS sensing code in the kernel is ok. But that is not
>> my point. My point is that the kernel code implies that it is possible
>> to do without a VBUS sensing pin. The file
>> Documentation/devicetree/bindings/usb/atmel-usb.txt states that
>> "atmel,vbus-gpio" is an optional property. So, it seems to me like the
>> code should work without it, and indeed it worked. That is why I have
>> called this a regression.

In USB 2.0 specification (Chapter 9.1.1.2 Powered) there is the following
sentence: “Although self-powered devices may already be powered before they
are attached to the USB, they are not considered to be in the Powered state
until they are attached to the USB and VBUS is applied to the device.” This
makes VBUS sensing mandatory for self-powered devices and non mandatory for
bus-powered devices. This is how, in my opinion, “atmel,vbus-gpio” should
be used (optional for bus-powered devices only).

Yesterday I removed the “atmel,vbus-gpio”  from SAM9x60-EK device tree to
create a setup close to the one used by Marcelo and performed a test. After
plugging / unplugging the usb device cable the Ethernet gadget is able to
re-enumerate. This makes me think the issue is specific to Arietta board or
to SAM9G25 SoC. I retrieved a SAM9x5 from office and I need to prepare it
before I’m able to run some tests on it. This can tell us if the issue is
board or SoC related.

By reverting this patch I’m afraid we encourage board designers to not
implement VBUS detection because the driver used to just work without it
and I think is not correct.

Currently I’m not able to replicated the issue to debug it. I’m willing to
continue to investigate this and propose a patch but will need some help in
reproducing it on one of our boards. Another alternative would be to enable
debug information in the driver on Arietta board 
(by using #define DEBUG_LEVEL DBG_ALL in atmel_usba_udc.h) and send the logs
to my email.

Regards,
Cristian

> 
> Yes, hardware that used to work, and now does not, is a regression.
> 
> So, should I revert the offending patch here?  Adding new features is
> not a good reason to keep things that break systems.  Or is there a
> potential fix in this thread that I missed?
> 
> thanks,
> 
> greg k-h
>