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

From: Nicolas.Ferre
Date: Mon Apr 29 2019 - 04:42:35 EST

Next message: Mao Han: "Re: [PATCH 2/3] riscv: Add support for perf registers sampling"
Previous message: Hans Verkuil: "Re: [PATCH v4] media: docs-rst: Document m2m stateless video decoder interface"
In reply to: Jonas Bonn: "Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
>> CC: Cristian Birsan <cristian.birsan@xxxxxxxxxxxxx>
>> CC: Felipe Balbi <balbi@xxxxxxxxxx>
>> CC: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> CC: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
>> CC: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
>> CC: Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx>
>> CC: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> CC: linux-usb@xxxxxxxxxxxxxxx
>> ---
>> 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

Next message: Mao Han: "Re: [PATCH 2/3] riscv: Add support for perf registers sampling"
Previous message: Hans Verkuil: "Re: [PATCH v4] media: docs-rst: Document m2m stateless video decoder interface"
In reply to: Jonas Bonn: "Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]