Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend

[<Nicolas.Ferre@microchip.com>]

On 27/04/2019 at 07:01, Jonas Bonn wrote:
> External E-Mail
> 
> 
> Ping.  Any feedback on this at all?  It's been over two months without a
> single comment.

Jonas,

We are working on the case that you describe internally and associated 
behavior on our SoC. We didn't come to a conclusion yet and that is why 
we didn't come back to you. We wanted to understand the situation 
completely before giving you a comment on your patch series.

Sorry for any misunderstanding it could have created.
Cristian will come back to you a little later: but be reassured, your 
patches are absolutely not forgotten.

Best regards,
   Nicolas

> On 20/02/2019 13:20, Jonas Bonn wrote:
>> This patch adds support for USB suspend to the Atmel UDC.
>>
>> When suspended, the UDC clock can be stopped, resulting in some power
>> savings.  The "wake up" interrupt will fire irregardless of whether the
>> clock is running or not, allowing the UDC clock to be restarted when the
>> USB master wants to wake the device again.
>>
>> The IRQ state of this device is somewhat fiddly.  The "wake up" IRQ
>> seems to actually be a "bus activity" indicator; the IRQ is almost
>> continuously asserted so enabling this IRQ should only be done after a
>> suspend when the wake IRQ becomes relevant.  Similarly, the "suspend"
>> IRQ detects "bus inactivity" and may therefore fire together with a
>> "wake" if the two types of activity coincide during the period between
>> two IRQ handler invocations; therefore, it's important to ignore the
>> "suspend" IRQ while waiting for a wake-up.
>>
>> This has been tested on a SAMA5D2 board.
>>
>> Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
>> CC: Cristian Birsan <cristian.birsan@microchip.com>
>> CC: Felipe Balbi <balbi@kernel.org>
>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> CC: Nicolas Ferre <nicolas.ferre@microchip.com>
>> CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> CC: Ludovic Desroches <ludovic.desroches@microchip.com>
>> CC: linux-arm-kernel@lists.infradead.org
>> CC: linux-usb@vger.kernel.org
>> ---
>>    drivers/usb/gadget/udc/atmel_usba_udc.c | 55 ++++++++++++++++++++++---
>>    drivers/usb/gadget/udc/atmel_usba_udc.h |  1 +
>>    2 files changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index 9d18fdddd9b2..740cb9308a86 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -1703,6 +1703,9 @@ 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;
>> @@ -1720,10 +1723,13 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>    	DBG(DBG_INT, "irq, status=%#08x\n", status);
>>    
>>    	if (status & USBA_DET_SUSPEND) {
>> -		toggle_bias(udc, 0);
>> -		usba_writel(udc, INT_CLR, 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);
>>    		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) {
>> @@ -1734,14 +1740,17 @@ 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
>> @@ -1756,6 +1765,8 @@ 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]);
>> @@ -1765,6 +1776,8 @@ 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]))
>> @@ -1778,7 +1791,9 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>    		struct usba_ep *ep0, *ep;
>>    		int i, n;
>>    
>> -		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>> +		usba_writel(udc, INT_CLR,
>> +			USBA_END_OF_RESET|USBA_END_OF_RESUME
>> +			|USBA_DET_SUSPEND|USBA_WAKE_UP);
>>    		generate_bias_pulse(udc);
>>    		reset_all_endpoints(udc);
>>    
>> @@ -1805,6 +1820,11 @@ 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);
>>    
>> @@ -1872,9 +1892,19 @@ 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);
>>    
>> @@ -1885,6 +1915,9 @@ 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);
>> @@ -1912,6 +1945,7 @@ static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
>>    		if (vbus) {
>>    			usba_start(udc);
>>    		} else {
>> +			udc->suspended = false;
>>    			usba_stop(udc);
>>    
>>    			if (udc->driver->disconnect)
>> @@ -1975,6 +2009,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>>    	if (fifo_mode == 0)
>>    		udc->configured_ep = 1;
>>    
>> +	udc->suspended = false;
>>    	usba_stop(udc);
>>    
>>    	udc->driver = NULL;
>> @@ -2288,6 +2323,7 @@ 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;
>>    	}
>> @@ -2297,10 +2333,13 @@ 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;
>> @@ -2314,8 +2353,12 @@ static int usba_udc_resume(struct device *dev)
>>    	if (!udc->driver)
>>    		return 0;
>>    
>> -	if (device_may_wakeup(dev) && udc->vbus_pin)
>> -		disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
>> +	if (device_may_wakeup(dev)) {
>> +		if (udc->vbus_pin)
>> +			disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
>> +
>> +		disable_irq_wake(udc->irq);
>> +	}
>>    
>>    	/* 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 58c96730e32e..24e6fbd3bb99 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
>> @@ -331,6 +331,7 @@ struct usba_udc {
>>    	struct usba_ep *usba_ep;
>>    	bool bias_pulse_needed;
>>    	bool clocked;
>> +	bool suspended;
>>    
>>    	u16 devstatus;
>>    
>>


-- 
Nicolas Ferre