* [PATCH 0/3] usb: gadget: atmel: support USB suspend/resume @ 2019-02-20 12:19 Jonas Bonn 2019-02-20 12:19 ` [1/3] " Jonas Bonn ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:19 UTC (permalink / raw) To: linux-kernel, linux-usb; +Cc: Jonas Bonn This patch series hooks up proper support for USB suspend and resume to the Atmel UDC. Jonas Bonn (3): usb: gadget: atmel_usba_udc: simplify setting of interrupt-enabled mask usb: gadget: atmel: support USB suspend usb: gadget: atmel: tie wake lock to running clock drivers/usb/gadget/udc/atmel_usba_udc.c | 84 ++++++++++++++++++++----- drivers/usb/gadget/udc/atmel_usba_udc.h | 1 + 2 files changed, 71 insertions(+), 14 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] usb: gadget: atmel_usba_udc: simplify setting of interrupt-enabled mask @ 2019-02-20 12:19 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:19 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Jonas Bonn, Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel This patch adds set and clear functions for enabling/disabling interrupts. This simplifies the implementation a bit as the masking of previously set bits doesn't need to be so explicit. 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 | 29 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index 51a2b9232baa..9d18fdddd9b2 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -358,8 +358,20 @@ static inline u32 usba_int_enb_get(struct usba_udc *udc) return udc->int_enb_cache; } -static inline void usba_int_enb_set(struct usba_udc *udc, u32 val) +static inline void usba_int_enb_set(struct usba_udc *udc, u32 mask) { + u32 val; + + val = udc->int_enb_cache | mask; + usba_writel(udc, INT_ENB, val); + udc->int_enb_cache = val; +} + +static inline void usba_int_enb_clear(struct usba_udc *udc, u32 mask) +{ + u32 val; + + val = udc->int_enb_cache & ~mask; usba_writel(udc, INT_ENB, val); udc->int_enb_cache = val; } @@ -629,14 +641,12 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc) if (ep->can_dma) { u32 ctrl; - usba_int_enb_set(udc, usba_int_enb_get(udc) | - USBA_BF(EPT_INT, 1 << ep->index) | + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1 << ep->index) | USBA_BF(DMA_INT, 1 << ep->index)); ctrl = USBA_AUTO_VALID | USBA_INTDIS_DMA; usba_ep_writel(ep, CTL_ENB, ctrl); } else { - usba_int_enb_set(udc, usba_int_enb_get(udc) | - USBA_BF(EPT_INT, 1 << ep->index)); + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1 << ep->index)); } spin_unlock_irqrestore(&udc->lock, flags); @@ -680,8 +690,7 @@ static int usba_ep_disable(struct usb_ep *_ep) usba_dma_readl(ep, STATUS); } usba_ep_writel(ep, CTL_DIS, USBA_EPT_ENABLE); - usba_int_enb_set(udc, usba_int_enb_get(udc) & - ~USBA_BF(EPT_INT, 1 << ep->index)); + usba_int_enb_clear(udc, USBA_BF(EPT_INT, 1 << ep->index)); request_complete_list(ep, &req_list, -ESHUTDOWN); @@ -1713,7 +1722,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid) if (status & USBA_DET_SUSPEND) { toggle_bias(udc, 0); usba_writel(udc, INT_CLR, USBA_DET_SUSPEND); - usba_int_enb_set(udc, int_enb | USBA_WAKE_UP); + usba_int_enb_set(udc, USBA_WAKE_UP); udc->bias_pulse_needed = true; DBG(DBG_BUS, "Suspend detected\n"); if (udc->gadget.speed != USB_SPEED_UNKNOWN @@ -1727,7 +1736,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid) if (status & USBA_WAKE_UP) { toggle_bias(udc, 1); usba_writel(udc, INT_CLR, USBA_WAKE_UP); - usba_int_enb_set(udc, int_enb & ~USBA_WAKE_UP); + usba_int_enb_clear(udc, USBA_WAKE_UP); DBG(DBG_BUS, "Wake Up CPU detected\n"); } @@ -1796,7 +1805,7 @@ 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); - usba_int_enb_set(udc, int_enb | USBA_BF(EPT_INT, 1) | + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1) | USBA_DET_SUSPEND | USBA_END_OF_RESUME); /* -- 2.19.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/3] usb: gadget: atmel_usba_udc: simplify setting of interrupt-enabled mask @ 2019-02-20 12:19 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:19 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Felipe Balbi, Alexandre Belloni, Greg Kroah-Hartman, Jonas Bonn, Ludovic Desroches, linux-arm-kernel, Cristian Birsan This patch adds set and clear functions for enabling/disabling interrupts. This simplifies the implementation a bit as the masking of previously set bits doesn't need to be so explicit. 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 | 29 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index 51a2b9232baa..9d18fdddd9b2 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -358,8 +358,20 @@ static inline u32 usba_int_enb_get(struct usba_udc *udc) return udc->int_enb_cache; } -static inline void usba_int_enb_set(struct usba_udc *udc, u32 val) +static inline void usba_int_enb_set(struct usba_udc *udc, u32 mask) { + u32 val; + + val = udc->int_enb_cache | mask; + usba_writel(udc, INT_ENB, val); + udc->int_enb_cache = val; +} + +static inline void usba_int_enb_clear(struct usba_udc *udc, u32 mask) +{ + u32 val; + + val = udc->int_enb_cache & ~mask; usba_writel(udc, INT_ENB, val); udc->int_enb_cache = val; } @@ -629,14 +641,12 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc) if (ep->can_dma) { u32 ctrl; - usba_int_enb_set(udc, usba_int_enb_get(udc) | - USBA_BF(EPT_INT, 1 << ep->index) | + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1 << ep->index) | USBA_BF(DMA_INT, 1 << ep->index)); ctrl = USBA_AUTO_VALID | USBA_INTDIS_DMA; usba_ep_writel(ep, CTL_ENB, ctrl); } else { - usba_int_enb_set(udc, usba_int_enb_get(udc) | - USBA_BF(EPT_INT, 1 << ep->index)); + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1 << ep->index)); } spin_unlock_irqrestore(&udc->lock, flags); @@ -680,8 +690,7 @@ static int usba_ep_disable(struct usb_ep *_ep) usba_dma_readl(ep, STATUS); } usba_ep_writel(ep, CTL_DIS, USBA_EPT_ENABLE); - usba_int_enb_set(udc, usba_int_enb_get(udc) & - ~USBA_BF(EPT_INT, 1 << ep->index)); + usba_int_enb_clear(udc, USBA_BF(EPT_INT, 1 << ep->index)); request_complete_list(ep, &req_list, -ESHUTDOWN); @@ -1713,7 +1722,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid) if (status & USBA_DET_SUSPEND) { toggle_bias(udc, 0); usba_writel(udc, INT_CLR, USBA_DET_SUSPEND); - usba_int_enb_set(udc, int_enb | USBA_WAKE_UP); + usba_int_enb_set(udc, USBA_WAKE_UP); udc->bias_pulse_needed = true; DBG(DBG_BUS, "Suspend detected\n"); if (udc->gadget.speed != USB_SPEED_UNKNOWN @@ -1727,7 +1736,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid) if (status & USBA_WAKE_UP) { toggle_bias(udc, 1); usba_writel(udc, INT_CLR, USBA_WAKE_UP); - usba_int_enb_set(udc, int_enb & ~USBA_WAKE_UP); + usba_int_enb_clear(udc, USBA_WAKE_UP); DBG(DBG_BUS, "Wake Up CPU detected\n"); } @@ -1796,7 +1805,7 @@ 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); - usba_int_enb_set(udc, int_enb | USBA_BF(EPT_INT, 1) | + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1) | USBA_DET_SUSPEND | USBA_END_OF_RESUME); /* -- 2.19.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [1/3] usb: gadget: atmel_usba_udc: simplify setting of interrupt-enabled mask @ 2019-02-20 12:19 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:19 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Jonas Bonn, Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel This patch adds set and clear functions for enabling/disabling interrupts. This simplifies the implementation a bit as the masking of previously set bits doesn't need to be so explicit. 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 | 29 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index 51a2b9232baa..9d18fdddd9b2 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -358,8 +358,20 @@ static inline u32 usba_int_enb_get(struct usba_udc *udc) return udc->int_enb_cache; } -static inline void usba_int_enb_set(struct usba_udc *udc, u32 val) +static inline void usba_int_enb_set(struct usba_udc *udc, u32 mask) { + u32 val; + + val = udc->int_enb_cache | mask; + usba_writel(udc, INT_ENB, val); + udc->int_enb_cache = val; +} + +static inline void usba_int_enb_clear(struct usba_udc *udc, u32 mask) +{ + u32 val; + + val = udc->int_enb_cache & ~mask; usba_writel(udc, INT_ENB, val); udc->int_enb_cache = val; } @@ -629,14 +641,12 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc) if (ep->can_dma) { u32 ctrl; - usba_int_enb_set(udc, usba_int_enb_get(udc) | - USBA_BF(EPT_INT, 1 << ep->index) | + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1 << ep->index) | USBA_BF(DMA_INT, 1 << ep->index)); ctrl = USBA_AUTO_VALID | USBA_INTDIS_DMA; usba_ep_writel(ep, CTL_ENB, ctrl); } else { - usba_int_enb_set(udc, usba_int_enb_get(udc) | - USBA_BF(EPT_INT, 1 << ep->index)); + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1 << ep->index)); } spin_unlock_irqrestore(&udc->lock, flags); @@ -680,8 +690,7 @@ static int usba_ep_disable(struct usb_ep *_ep) usba_dma_readl(ep, STATUS); } usba_ep_writel(ep, CTL_DIS, USBA_EPT_ENABLE); - usba_int_enb_set(udc, usba_int_enb_get(udc) & - ~USBA_BF(EPT_INT, 1 << ep->index)); + usba_int_enb_clear(udc, USBA_BF(EPT_INT, 1 << ep->index)); request_complete_list(ep, &req_list, -ESHUTDOWN); @@ -1713,7 +1722,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid) if (status & USBA_DET_SUSPEND) { toggle_bias(udc, 0); usba_writel(udc, INT_CLR, USBA_DET_SUSPEND); - usba_int_enb_set(udc, int_enb | USBA_WAKE_UP); + usba_int_enb_set(udc, USBA_WAKE_UP); udc->bias_pulse_needed = true; DBG(DBG_BUS, "Suspend detected\n"); if (udc->gadget.speed != USB_SPEED_UNKNOWN @@ -1727,7 +1736,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid) if (status & USBA_WAKE_UP) { toggle_bias(udc, 1); usba_writel(udc, INT_CLR, USBA_WAKE_UP); - usba_int_enb_set(udc, int_enb & ~USBA_WAKE_UP); + usba_int_enb_clear(udc, USBA_WAKE_UP); DBG(DBG_BUS, "Wake Up CPU detected\n"); } @@ -1796,7 +1805,7 @@ 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); - usba_int_enb_set(udc, int_enb | USBA_BF(EPT_INT, 1) | + usba_int_enb_set(udc, USBA_BF(EPT_INT, 1) | USBA_DET_SUSPEND | USBA_END_OF_RESUME); /* ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] usb: gadget: atmel: support USB suspend @ 2019-02-20 12:20 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:20 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Jonas Bonn, Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel 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; -- 2.19.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] usb: gadget: atmel: support USB suspend @ 2019-02-20 12:20 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:20 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Felipe Balbi, Alexandre Belloni, Greg Kroah-Hartman, Jonas Bonn, Ludovic Desroches, linux-arm-kernel, Cristian Birsan 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; -- 2.19.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [2/3] usb: gadget: atmel: support USB suspend @ 2019-02-20 12:20 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:20 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Jonas Bonn, Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel 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; ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend @ 2019-04-27 5:01 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-04-27 5:01 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel Ping. Any feedback on this at all? It's been over two months without a single comment. Thanks, Jonas 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; > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend @ 2019-04-27 5:01 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-04-27 5:01 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Felipe Balbi, Alexandre Belloni, Greg Kroah-Hartman, Ludovic Desroches, linux-arm-kernel, Cristian Birsan Ping. Any feedback on this at all? It's been over two months without a single comment. Thanks, Jonas 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; > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [2/3] usb: gadget: atmel: support USB suspend @ 2019-04-27 5:01 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-04-27 5:01 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel Ping. Any feedback on this at all? It's been over two months without a single comment. Thanks, Jonas 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; > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend @ 2019-04-29 8:42 ` Nicolas Ferre 0 siblings, 0 replies; 16+ messages in thread From: Nicolas.Ferre @ 2019-04-29 8:42 UTC (permalink / raw) To: jonas, linux-kernel, linux-usb, Cristian.Birsan Cc: balbi, gregkh, alexandre.belloni, Ludovic.Desroches, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend @ 2019-04-29 8:42 ` Nicolas Ferre 0 siblings, 0 replies; 16+ messages in thread From: Nicolas.Ferre @ 2019-04-29 8:42 UTC (permalink / raw) To: jonas, linux-kernel, linux-usb, Cristian.Birsan Cc: balbi, alexandre.belloni, Ludovic.Desroches, linux-arm-kernel, gregkh 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [2/3] usb: gadget: atmel: support USB suspend @ 2019-04-29 8:42 ` Nicolas Ferre 0 siblings, 0 replies; 16+ messages in thread From: Nicolas Ferre @ 2019-04-29 8:42 UTC (permalink / raw) To: jonas, linux-kernel, linux-usb, Cristian.Birsan Cc: balbi, gregkh, alexandre.belloni, Ludovic.Desroches, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] usb: gadget: atmel: tie wake lock to running clock @ 2019-02-20 12:20 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:20 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Jonas Bonn, Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel If the USB device is connected to a host, the CPU cannot be suspended or else the USB device appears to be disconnected from the host's point of view. Only after a "USB suspend" state has been entered (as set by the host) or the host is disconnected can the system safely be suspended: in both these states, the clock is stopped. As such, this patch associates a "wake lock" with the running clock of the UDC to keep the system awake as long as the host maintains the USB connection active. 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 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index 740cb9308a86..864d03c3c9db 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -1859,6 +1859,8 @@ static int start_clock(struct usba_udc *udc) if (udc->clocked) return 0; + pm_stay_awake(&udc->pdev->dev); + ret = clk_prepare_enable(udc->pclk); if (ret) return ret; @@ -1881,6 +1883,8 @@ static void stop_clock(struct usba_udc *udc) clk_disable_unprepare(udc->pclk); udc->clocked = false; + + pm_relax(&udc->pdev->dev); } static int usba_start(struct usba_udc *udc) -- 2.19.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] usb: gadget: atmel: tie wake lock to running clock @ 2019-02-20 12:20 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:20 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Felipe Balbi, Alexandre Belloni, Greg Kroah-Hartman, Jonas Bonn, Ludovic Desroches, linux-arm-kernel, Cristian Birsan If the USB device is connected to a host, the CPU cannot be suspended or else the USB device appears to be disconnected from the host's point of view. Only after a "USB suspend" state has been entered (as set by the host) or the host is disconnected can the system safely be suspended: in both these states, the clock is stopped. As such, this patch associates a "wake lock" with the running clock of the UDC to keep the system awake as long as the host maintains the USB connection active. 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 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index 740cb9308a86..864d03c3c9db 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -1859,6 +1859,8 @@ static int start_clock(struct usba_udc *udc) if (udc->clocked) return 0; + pm_stay_awake(&udc->pdev->dev); + ret = clk_prepare_enable(udc->pclk); if (ret) return ret; @@ -1881,6 +1883,8 @@ static void stop_clock(struct usba_udc *udc) clk_disable_unprepare(udc->pclk); udc->clocked = false; + + pm_relax(&udc->pdev->dev); } static int usba_start(struct usba_udc *udc) -- 2.19.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [3/3] usb: gadget: atmel: tie wake lock to running clock @ 2019-02-20 12:20 ` Jonas Bonn 0 siblings, 0 replies; 16+ messages in thread From: Jonas Bonn @ 2019-02-20 12:20 UTC (permalink / raw) To: linux-kernel, linux-usb Cc: Jonas Bonn, Cristian Birsan, Felipe Balbi, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel If the USB device is connected to a host, the CPU cannot be suspended or else the USB device appears to be disconnected from the host's point of view. Only after a "USB suspend" state has been entered (as set by the host) or the host is disconnected can the system safely be suspended: in both these states, the clock is stopped. As such, this patch associates a "wake lock" with the running clock of the UDC to keep the system awake as long as the host maintains the USB connection active. 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 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index 740cb9308a86..864d03c3c9db 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -1859,6 +1859,8 @@ static int start_clock(struct usba_udc *udc) if (udc->clocked) return 0; + pm_stay_awake(&udc->pdev->dev); + ret = clk_prepare_enable(udc->pclk); if (ret) return ret; @@ -1881,6 +1883,8 @@ static void stop_clock(struct usba_udc *udc) clk_disable_unprepare(udc->pclk); udc->clocked = false; + + pm_relax(&udc->pdev->dev); } static int usba_start(struct usba_udc *udc) ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-04-29 8:42 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-20 12:19 [PATCH 0/3] usb: gadget: atmel: support USB suspend/resume Jonas Bonn 2019-02-20 12:19 ` [PATCH 1/3] usb: gadget: atmel_usba_udc: simplify setting of interrupt-enabled mask Jonas Bonn 2019-02-20 12:19 ` Jonas Bonn 2019-02-20 12:19 ` [1/3] " Jonas Bonn 2019-02-20 12:20 ` [PATCH 2/3] usb: gadget: atmel: support USB suspend Jonas Bonn 2019-02-20 12:20 ` Jonas Bonn 2019-02-20 12:20 ` [2/3] " Jonas Bonn 2019-04-27 5:01 ` [PATCH 2/3] " Jonas Bonn 2019-04-27 5:01 ` Jonas Bonn 2019-04-27 5:01 ` [2/3] " Jonas Bonn 2019-04-29 8:42 ` [PATCH 2/3] " Nicolas.Ferre 2019-04-29 8:42 ` Nicolas.Ferre 2019-04-29 8:42 ` [2/3] " Nicolas Ferre 2019-02-20 12:20 ` [PATCH 3/3] usb: gadget: atmel: tie wake lock to running clock Jonas Bonn 2019-02-20 12:20 ` Jonas Bonn 2019-02-20 12:20 ` [3/3] " Jonas Bonn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.