[PATCH v2 0/4] usb: renesas_usbhs: add support for USB-DMAC

[PATCH v2 0/4] usb: renesas_usbhs: add support for USB-DMAC

[Yoshihiro Shimoda]

This patch set is based on Felipe's usb.git / testing/next branch.
(commit id = 6461d69d12508fe166be5c6c31d5a9da02a4dfb5)

To use the USB-DMAC, we have to add some device nodes for USB-DMAC
in dts file. If we don't have such device nodes, this driver will
use PIO.

Changes from v1:
 - Change local_irq_save/restore() area in patch 1.
 - Modify the commit log in patch 1.
 - fix some comments in patch 4.

Yoshihiro Shimoda (4):
  usb: renesas_usbhs: fix spinlock suspected in a gadget complete
    function
  usb: renesas_usbhs: add the channel number in dma-names
  usb: renesas_usbhs: fix the sequence in xfer_work()
  usb: renesas_usbhs: add support for USB-DMAC

 .../devicetree/bindings/usb/renesas_usbhs.txt      |    5 +-
 drivers/usb/renesas_usbhs/common.c                 |   19 ++
 drivers/usb/renesas_usbhs/common.h                 |    7 +
 drivers/usb/renesas_usbhs/fifo.c                   |  189 ++++++++++++++++++--
 drivers/usb/renesas_usbhs/fifo.h                   |    1 +
 drivers/usb/renesas_usbhs/mod_gadget.c             |   11 ++
 drivers/usb/renesas_usbhs/pipe.c                   |   39 ++++
 drivers/usb/renesas_usbhs/pipe.h                   |    1 +
 include/linux/usb/renesas_usbhs.h                  |    2 +
 9 files changed, 258 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[PATCH v2 0/4] usb: renesas_usbhs: add support for USB-DMAC

[Yoshihiro Shimoda]

This patch set is based on Felipe's usb.git / testing/next branch.
(commit id = 6461d69d12508fe166be5c6c31d5a9da02a4dfb5)

To use the USB-DMAC, we have to add some device nodes for USB-DMAC
in dts file. If we don't have such device nodes, this driver will
use PIO.

Changes from v1:
 - Change local_irq_save/restore() area in patch 1.
 - Modify the commit log in patch 1.
 - fix some comments in patch 4.

Yoshihiro Shimoda (4):
  usb: renesas_usbhs: fix spinlock suspected in a gadget complete
    function
  usb: renesas_usbhs: add the channel number in dma-names
  usb: renesas_usbhs: fix the sequence in xfer_work()
  usb: renesas_usbhs: add support for USB-DMAC

 .../devicetree/bindings/usb/renesas_usbhs.txt      |    5 +-
 drivers/usb/renesas_usbhs/common.c                 |   19 ++
 drivers/usb/renesas_usbhs/common.h                 |    7 +
 drivers/usb/renesas_usbhs/fifo.c                   |  189 ++++++++++++++++++--
 drivers/usb/renesas_usbhs/fifo.h                   |    1 +
 drivers/usb/renesas_usbhs/mod_gadget.c             |   11 ++
 drivers/usb/renesas_usbhs/pipe.c                   |   39 ++++
 drivers/usb/renesas_usbhs/pipe.h                   |    1 +
 include/linux/usb/renesas_usbhs.h                  |    2 +
 9 files changed, 258 insertions(+), 16 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] usb: renesas_usbhs: add the channel number in dma-names

[Yoshihiro Shimoda]

To connect the channel of USB-DMAC to USBHS DnFIFO number, this patch
adds this channel/FIFO number in dma-names. Otherwise, this driver
needs to add analysis code for device tree.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../devicetree/bindings/usb/renesas_usbhs.txt      |    5 ++++-
 drivers/usb/renesas_usbhs/fifo.c                   |   20 +++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
index 61b045b..dc2a18f 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
@@ -15,7 +15,10 @@ Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
   - dmas: Must contain a list of references to DMA specifiers.
-  - dma-names : Must contain a list of DMA names, "tx" or "rx".
+  - dma-names : Must contain a list of DMA names:
+   - tx0 ... tx<n>
+   - rx0 ... rx<n>
+    - This <n> means DnFIFO in USBHS module.
 
 Example:
 	usbhs: usb@e6590000 {
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index d891bff..28d10eb 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -1069,23 +1069,29 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo)
 					    &fifo->rx_slave);
 }
 
-static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo)
+static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo,
+			       int channel)
 {
-	fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx");
+	char name[16];
+
+	snprintf(name, sizeof(name), "tx%d", channel);
+	fifo->tx_chan = dma_request_slave_channel_reason(dev, name);
 	if (IS_ERR(fifo->tx_chan))
 		fifo->tx_chan = NULL;
-	fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx");
+
+	snprintf(name, sizeof(name), "rx%d", channel);
+	fifo->rx_chan = dma_request_slave_channel_reason(dev, name);
 	if (IS_ERR(fifo->rx_chan))
 		fifo->rx_chan = NULL;
 }
 
-static void usbhsf_dma_init(struct usbhs_priv *priv,
-			    struct usbhs_fifo *fifo)
+static void usbhsf_dma_init(struct usbhs_priv *priv, struct usbhs_fifo *fifo,
+			    int channel)
 {
 	struct device *dev = usbhs_priv_to_dev(priv);
 
 	if (dev->of_node)
-		usbhsf_dma_init_dt(dev, fifo);
+		usbhsf_dma_init_dt(dev, fifo, channel);
 	else
 		usbhsf_dma_init_pdev(fifo);
 
@@ -1231,7 +1237,7 @@ do {									\
 			usbhs_get_dparam(priv, d##channel##_tx_id);	\
 	fifo->rx_slave.shdma_slave.slave_id =				\
 			usbhs_get_dparam(priv, d##channel##_rx_id);	\
-	usbhsf_dma_init(priv, fifo);					\
+	usbhsf_dma_init(priv, fifo, channel);				\
 } while (0)
 
 #define USBHS_DFIFO_INIT(priv, fifo, channel)				\
-- 
1.7.9.5

[PATCH v2 2/4] usb: renesas_usbhs: add the channel number in dma-names

[Yoshihiro Shimoda]

To connect the channel of USB-DMAC to USBHS DnFIFO number, this patch
adds this channel/FIFO number in dma-names. Otherwise, this driver
needs to add analysis code for device tree.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../devicetree/bindings/usb/renesas_usbhs.txt      |    5 ++++-
 drivers/usb/renesas_usbhs/fifo.c                   |   20 +++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
index 61b045b..dc2a18f 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
@@ -15,7 +15,10 @@ Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
   - dmas: Must contain a list of references to DMA specifiers.
-  - dma-names : Must contain a list of DMA names, "tx" or "rx".
+  - dma-names : Must contain a list of DMA names:
+   - tx0 ... tx<n>
+   - rx0 ... rx<n>
+    - This <n> means DnFIFO in USBHS module.
 
 Example:
 	usbhs: usb@e6590000 {
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index d891bff..28d10eb 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -1069,23 +1069,29 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo)
 					    &fifo->rx_slave);
 }
 
-static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo)
+static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo,
+			       int channel)
 {
-	fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx");
+	char name[16];
+
+	snprintf(name, sizeof(name), "tx%d", channel);
+	fifo->tx_chan = dma_request_slave_channel_reason(dev, name);
 	if (IS_ERR(fifo->tx_chan))
 		fifo->tx_chan = NULL;
-	fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx");
+
+	snprintf(name, sizeof(name), "rx%d", channel);
+	fifo->rx_chan = dma_request_slave_channel_reason(dev, name);
 	if (IS_ERR(fifo->rx_chan))
 		fifo->rx_chan = NULL;
 }
 
-static void usbhsf_dma_init(struct usbhs_priv *priv,
-			    struct usbhs_fifo *fifo)
+static void usbhsf_dma_init(struct usbhs_priv *priv, struct usbhs_fifo *fifo,
+			    int channel)
 {
 	struct device *dev = usbhs_priv_to_dev(priv);
 
 	if (dev->of_node)
-		usbhsf_dma_init_dt(dev, fifo);
+		usbhsf_dma_init_dt(dev, fifo, channel);
 	else
 		usbhsf_dma_init_pdev(fifo);
 
@@ -1231,7 +1237,7 @@ do {									\
 			usbhs_get_dparam(priv, d##channel##_tx_id);	\
 	fifo->rx_slave.shdma_slave.slave_id =				\
 			usbhs_get_dparam(priv, d##channel##_rx_id);	\
-	usbhsf_dma_init(priv, fifo);					\
+	usbhsf_dma_init(priv, fifo, channel);				\
 } while (0)
 
 #define USBHS_DFIFO_INIT(priv, fifo, channel)				\
-- 
1.7.9.5

[PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Yoshihiro Shimoda]

According to the gadget.h, a "complete" function will always be called
with interrupts disabled. However, sometimes usbhsg_queue_pop() function
is called with interrupts enabled. So, this function should calls
local_irq_save() before this calls the usb_gadget_giveback_request().
Otherwise, there is possible to cause a spinlock suspected in a gadget
complete function.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/renesas_usbhs/mod_gadget.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index e0384af..104bddf 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -126,11 +126,22 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
 	struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
 	struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep);
 	struct device *dev = usbhsg_gpriv_to_dev(gpriv);
+	unsigned long flags;
 
 	dev_dbg(dev, "pipe %d : queue pop\n", usbhs_pipe_number(pipe));
 
 	ureq->req.status = status;
+	/*
+	 * According to the gadget.h, a "complete" function will always be
+	 * called with interrupts disabled. However, sometimes this function
+	 * is called with interrupts enabled. (e.g. complete a DMAC transfer or
+	 * write data and done is set immediately when PIO.) So, this function
+	 * should calls local_irq_save() before this calls the
+	 * usb_gadget_giveback_request().
+	 */
+	local_irq_save(flags);
 	usb_gadget_giveback_request(&uep->ep, &ureq->req);
+	local_irq_restore(flags);
 }
 
 static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
-- 
1.7.9.5

[PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Yoshihiro Shimoda]

According to the gadget.h, a "complete" function will always be called
with interrupts disabled. However, sometimes usbhsg_queue_pop() function
is called with interrupts enabled. So, this function should calls
local_irq_save() before this calls the usb_gadget_giveback_request().
Otherwise, there is possible to cause a spinlock suspected in a gadget
complete function.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/renesas_usbhs/mod_gadget.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index e0384af..104bddf 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -126,11 +126,22 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
 	struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
 	struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep);
 	struct device *dev = usbhsg_gpriv_to_dev(gpriv);
+	unsigned long flags;
 
 	dev_dbg(dev, "pipe %d : queue pop\n", usbhs_pipe_number(pipe));
 
 	ureq->req.status = status;
+	/*
+	 * According to the gadget.h, a "complete" function will always be
+	 * called with interrupts disabled. However, sometimes this function
+	 * is called with interrupts enabled. (e.g. complete a DMAC transfer or
+	 * write data and done is set immediately when PIO.) So, this function
+	 * should calls local_irq_save() before this calls the
+	 * usb_gadget_giveback_request().
+	 */
+	local_irq_save(flags);
 	usb_gadget_giveback_request(&uep->ep, &ureq->req);
+	local_irq_restore(flags);
 }
 
 static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> According to the gadget.h, a "complete" function will always be called
> with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> is called with interrupts enabled. So, this function should calls
> local_irq_save() before this calls the usb_gadget_giveback_request().
> Otherwise, there is possible to cause a spinlock suspected in a gadget
> complete function.

I still feel uneasy about adding the call to local_irq_save(), as the need for
this is usually an indicator of another locking problem.

Unfortunately I know next to nothing about the USB gadget subsystem, so some
help from the USB gadget experts would be appreciated.

I had a look at other drivers, and it seems most drivers actually release
and reacquire a spinlock around the call to usb_gadget_giveback_request(),
i.e. they do:

    spin_unlock(...);
    usb_gadget_giveback_request(...);
    spin_lock();

This means they had already acquired the spinlock (and disabled interrupts!)
before, which looks much healthier to me.

There's only one driver that uses local_irq_save() (pxa27x_udc).

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/usb/renesas_usbhs/mod_gadget.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index e0384af..104bddf 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -126,11 +126,22 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
>         struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
>         struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep);
>         struct device *dev = usbhsg_gpriv_to_dev(gpriv);
> +       unsigned long flags;
>
>         dev_dbg(dev, "pipe %d : queue pop\n", usbhs_pipe_number(pipe));
>
>         ureq->req.status = status;
> +       /*
> +        * According to the gadget.h, a "complete" function will always be
> +        * called with interrupts disabled. However, sometimes this function
> +        * is called with interrupts enabled. (e.g. complete a DMAC transfer or
> +        * write data and done is set immediately when PIO.) So, this function
> +        * should calls local_irq_save() before this calls the
> +        * usb_gadget_giveback_request().
> +        */
> +       local_irq_save(flags);
>         usb_gadget_giveback_request(&uep->ep, &ureq->req);
> +       local_irq_restore(flags);
>  }
>
>  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> According to the gadget.h, a "complete" function will always be called
> with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> is called with interrupts enabled. So, this function should calls
> local_irq_save() before this calls the usb_gadget_giveback_request().
> Otherwise, there is possible to cause a spinlock suspected in a gadget
> complete function.

I still feel uneasy about adding the call to local_irq_save(), as the need for
this is usually an indicator of another locking problem.

Unfortunately I know next to nothing about the USB gadget subsystem, so some
help from the USB gadget experts would be appreciated.

I had a look at other drivers, and it seems most drivers actually release
and reacquire a spinlock around the call to usb_gadget_giveback_request(),
i.e. they do:

    spin_unlock(...);
    usb_gadget_giveback_request(...);
    spin_lock();

This means they had already acquired the spinlock (and disabled interrupts!)
before, which looks much healthier to me.

There's only one driver that uses local_irq_save() (pxa27x_udc).

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/renesas_usbhs/mod_gadget.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index e0384af..104bddf 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -126,11 +126,22 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
>         struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
>         struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep);
>         struct device *dev = usbhsg_gpriv_to_dev(gpriv);
> +       unsigned long flags;
>
>         dev_dbg(dev, "pipe %d : queue pop\n", usbhs_pipe_number(pipe));
>
>         ureq->req.status = status;
> +       /*
> +        * According to the gadget.h, a "complete" function will always be
> +        * called with interrupts disabled. However, sometimes this function
> +        * is called with interrupts enabled. (e.g. complete a DMAC transfer or
> +        * write data and done is set immediately when PIO.) So, this function
> +        * should calls local_irq_save() before this calls the
> +        * usb_gadget_giveback_request().
> +        */
> +       local_irq_save(flags);
>         usb_gadget_giveback_request(&uep->ep, &ureq->req);
> +       local_irq_restore(flags);
>  }
>
>  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

SGkgR2VlcnQtc2FuLA0KDQpUaGFuayB5b3UgZm9yIHRoZSByZXBseSBhZ2FpbiENCg0KPiBIaSBT
aGltb2RhLXNhbiwNCj4gDQo+IE9uIE1vbiwgRmViIDE2LCAyMDE1IGF0IDI6NTIgQU0sIFlvc2hp
aGlybyBTaGltb2RhDQo+IDx5b3NoaWhpcm8uc2hpbW9kYS51aEByZW5lc2FzLmNvbT4gd3JvdGU6
DQo+ID4gQWNjb3JkaW5nIHRvIHRoZSBnYWRnZXQuaCwgYSAiY29tcGxldGUiIGZ1bmN0aW9uIHdp
bGwgYWx3YXlzIGJlIGNhbGxlZA0KPiA+IHdpdGggaW50ZXJydXB0cyBkaXNhYmxlZC4gSG93ZXZl
ciwgc29tZXRpbWVzIHVzYmhzZ19xdWV1ZV9wb3AoKSBmdW5jdGlvbg0KPiA+IGlzIGNhbGxlZCB3
aXRoIGludGVycnVwdHMgZW5hYmxlZC4gU28sIHRoaXMgZnVuY3Rpb24gc2hvdWxkIGNhbGxzDQo+
ID4gbG9jYWxfaXJxX3NhdmUoKSBiZWZvcmUgdGhpcyBjYWxscyB0aGUgdXNiX2dhZGdldF9naXZl
YmFja19yZXF1ZXN0KCkuDQo+ID4gT3RoZXJ3aXNlLCB0aGVyZSBpcyBwb3NzaWJsZSB0byBjYXVz
ZSBhIHNwaW5sb2NrIHN1c3BlY3RlZCBpbiBhIGdhZGdldA0KPiA+IGNvbXBsZXRlIGZ1bmN0aW9u
Lg0KPiANCj4gSSBzdGlsbCBmZWVsIHVuZWFzeSBhYm91dCBhZGRpbmcgdGhlIGNhbGwgdG8gbG9j
YWxfaXJxX3NhdmUoKSwgYXMgdGhlIG5lZWQgZm9yDQo+IHRoaXMgaXMgdXN1YWxseSBhbiBpbmRp
Y2F0b3Igb2YgYW5vdGhlciBsb2NraW5nIHByb2JsZW0uDQoNCkkgYWxzbyB0aGluayB0aGF0IEkg
d291bGQgbGlrZSB0byBhdm9pZCB1c2luZyBsb2NhbF9pcnFfc2F2ZSgpLg0KQnV0LCBJIGhhdmUg
bm8gaWRlYSB0byByZXNvbHZlIHRoaXMgaXNzdWUgZm9yIG5vdy4NCg0KPiBVbmZvcnR1bmF0ZWx5
IEkga25vdyBuZXh0IHRvIG5vdGhpbmcgYWJvdXQgdGhlIFVTQiBnYWRnZXQgc3Vic3lzdGVtLCBz
byBzb21lDQo+IGhlbHAgZnJvbSB0aGUgVVNCIGdhZGdldCBleHBlcnRzIHdvdWxkIGJlIGFwcHJl
Y2lhdGVkLg0KPiANCj4gSSBoYWQgYSBsb29rIGF0IG90aGVyIGRyaXZlcnMsIGFuZCBpdCBzZWVt
cyBtb3N0IGRyaXZlcnMgYWN0dWFsbHkgcmVsZWFzZQ0KPiBhbmQgcmVhY3F1aXJlIGEgc3Bpbmxv
Y2sgYXJvdW5kIHRoZSBjYWxsIHRvIHVzYl9nYWRnZXRfZ2l2ZWJhY2tfcmVxdWVzdCgpLA0KPiBp
LmUuIHRoZXkgZG86DQo+IA0KPiAgICAgc3Bpbl91bmxvY2soLi4uKTsNCj4gICAgIHVzYl9nYWRn
ZXRfZ2l2ZWJhY2tfcmVxdWVzdCguLi4pOw0KPiAgICAgc3Bpbl9sb2NrKCk7DQo+IA0KPiBUaGlz
IG1lYW5zIHRoZXkgaGFkIGFscmVhZHkgYWNxdWlyZWQgdGhlIHNwaW5sb2NrIChhbmQgZGlzYWJs
ZWQgaW50ZXJydXB0cyEpDQo+IGJlZm9yZSwgd2hpY2ggbG9va3MgbXVjaCBoZWFsdGhpZXIgdG8g
bWUuDQo+IA0KPiBUaGVyZSdzIG9ubHkgb25lIGRyaXZlciB0aGF0IHVzZXMgbG9jYWxfaXJxX3Nh
dmUoKSAocHhhMjd4X3VkYykuDQoNCkkgaGFkIGEgbG9vayBhdCB0aGUgbXVzYiBkcml2ZXIuIEl0
IHVzZXMgYSBkbWFlbmdpbmUgZHJpdmVyLg0KQW5kLCBpbiB0aGUgY2FsbGJhY2sgcm91dGluZSBv
ZiB0aGUgbXVzYiBkcml2ZXIsIGl0IGNhbGxzIHNwaW5fbG9ja19pcnFzYXZlKCkgYXQgZmlyc3Qu
DQoNCmNwcGk0MV9kbWFfY2FsbGJhY2sNCi0tPiBzcGluX2xvY2tfaXJxc2F2ZSgmbXVzYi0+bG9j
aywgZmxhZ3MpOw0KLS0+IGNwcGk0MV90cmFuc19kb25lDQogLS0+IG11c2JfZG1hX2NvbXBsZXRp
b24NCiAgLS0+IG11c2JfZ190eCBvciBtdXNiX2dfcngNCiAgIC0tPiBtdXNiX2dfZ2l2ZWJhY2sN
CiAgICAtLT4gc3Bpbl91bmxvY2soJm11c2ItPmxvY2spOyAgICMgVGhpcyBtZWFucyB1bmxvY2tl
ZCB0aGUgc3BpbiwgYnV0IGRpc2FibGVkIGludGVycnVwdHMuDQogICAgLS0+IHVzYl9nYWRnZXRf
Z2l2ZWJhY2tfcmVxdWVzdA0KICAgIC0tPiBzcGluX2xvY2soJm11c2ItPmxvY2spOw0KIDwgc25p
cCA+DQotLT4gc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmbXVzYi0+bG9jaywgZmxhZ3MpOw0KDQpT
bywgYWJvdXQgZGlzYWJsZWQgaW50ZXJydXB0cyBiZWZvcmUgdXNiX2dhZGdldF9naXZlYmFja19y
ZXF1ZXN0KCksDQppdCBpcyBzaW1pbGFyIHdpdGggdGhpcyBwYXRjaC4NCg0KQmVzdCByZWdhcmRz
LA0KWW9zaGloaXJvIFNoaW1vZGENCg0K

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi Geert-san,

Thank you for the reply again!

> Hi Shimoda-san,
> 
> On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > According to the gadget.h, a "complete" function will always be called
> > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > is called with interrupts enabled. So, this function should calls
> > local_irq_save() before this calls the usb_gadget_giveback_request().
> > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > complete function.
> 
> I still feel uneasy about adding the call to local_irq_save(), as the need for
> this is usually an indicator of another locking problem.

I also think that I would like to avoid using local_irq_save().
But, I have no idea to resolve this issue for now.

> Unfortunately I know next to nothing about the USB gadget subsystem, so some
> help from the USB gadget experts would be appreciated.
> 
> I had a look at other drivers, and it seems most drivers actually release
> and reacquire a spinlock around the call to usb_gadget_giveback_request(),
> i.e. they do:
> 
>     spin_unlock(...);
>     usb_gadget_giveback_request(...);
>     spin_lock();
> 
> This means they had already acquired the spinlock (and disabled interrupts!)
> before, which looks much healthier to me.
> 
> There's only one driver that uses local_irq_save() (pxa27x_udc).

I had a look at the musb driver. It uses a dmaengine driver.
And, in the callback routine of the musb driver, it calls spin_lock_irqsave() at first.

cppi41_dma_callback
--> spin_lock_irqsave(&musb->lock, flags);
--> cppi41_trans_done
 --> musb_dma_completion
  --> musb_g_tx or musb_g_rx
   --> musb_g_giveback
    --> spin_unlock(&musb->lock);   # This means unlocked the spin, but disabled interrupts.
    --> usb_gadget_giveback_request
    --> spin_lock(&musb->lock);
 < snip >
--> spin_unlock_irqrestore(&musb->lock, flags);

So, about disabled interrupts before usb_gadget_giveback_request(),
it is similar with this patch.

Best regards,
Yoshihiro Shimoda

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

SGkgR2VlcnQtc2FuIGFnYWluLA0KDQo+IEhpIEdlZXJ0LXNhbiwNCj4gDQo+IFRoYW5rIHlvdSBm
b3IgdGhlIHJlcGx5IGFnYWluIQ0KPiANCj4gPiBIaSBTaGltb2RhLXNhbiwNCj4gPg0KPiA+IE9u
IE1vbiwgRmViIDE2LCAyMDE1IGF0IDI6NTIgQU0sIFlvc2hpaGlybyBTaGltb2RhDQo+ID4gPHlv
c2hpaGlyby5zaGltb2RhLnVoQHJlbmVzYXMuY29tPiB3cm90ZToNCj4gPiA+IEFjY29yZGluZyB0
byB0aGUgZ2FkZ2V0LmgsIGEgImNvbXBsZXRlIiBmdW5jdGlvbiB3aWxsIGFsd2F5cyBiZSBjYWxs
ZWQNCj4gPiA+IHdpdGggaW50ZXJydXB0cyBkaXNhYmxlZC4gSG93ZXZlciwgc29tZXRpbWVzIHVz
YmhzZ19xdWV1ZV9wb3AoKSBmdW5jdGlvbg0KPiA+ID4gaXMgY2FsbGVkIHdpdGggaW50ZXJydXB0
cyBlbmFibGVkLiBTbywgdGhpcyBmdW5jdGlvbiBzaG91bGQgY2FsbHMNCj4gPiA+IGxvY2FsX2ly
cV9zYXZlKCkgYmVmb3JlIHRoaXMgY2FsbHMgdGhlIHVzYl9nYWRnZXRfZ2l2ZWJhY2tfcmVxdWVz
dCgpLg0KPiA+ID4gT3RoZXJ3aXNlLCB0aGVyZSBpcyBwb3NzaWJsZSB0byBjYXVzZSBhIHNwaW5s
b2NrIHN1c3BlY3RlZCBpbiBhIGdhZGdldA0KPiA+ID4gY29tcGxldGUgZnVuY3Rpb24uDQo+ID4N
Cj4gPiBJIHN0aWxsIGZlZWwgdW5lYXN5IGFib3V0IGFkZGluZyB0aGUgY2FsbCB0byBsb2NhbF9p
cnFfc2F2ZSgpLCBhcyB0aGUgbmVlZCBmb3INCj4gPiB0aGlzIGlzIHVzdWFsbHkgYW4gaW5kaWNh
dG9yIG9mIGFub3RoZXIgbG9ja2luZyBwcm9ibGVtLg0KPiANCj4gSSBhbHNvIHRoaW5rIHRoYXQg
SSB3b3VsZCBsaWtlIHRvIGF2b2lkIHVzaW5nIGxvY2FsX2lycV9zYXZlKCkuDQo+IEJ1dCwgSSBo
YXZlIG5vIGlkZWEgdG8gcmVzb2x2ZSB0aGlzIGlzc3VlIGZvciBub3cuDQoNCkFmdGVyIEkgbW9k
aWZpZWQgdXNiLWRtYWMgZHJpdmVyIHRvIHVzZSBhIHRhc2tsZXQgaW5zdGVhZCBvZiBhIGt0aHJl
YWQsDQp0aGlzIGlzc3VlIGRpc2FwcGVhcmVkLg0KDQpNeSB1bmRlcnN0YW5kaW5nIGlzIHRoZSBm
b2xsb3dpbmdzOg0KLSBBY2NvcmRpbmcgdG8gdGhlIGJhY2t0cmFjZSBiZWxvdywgZHVyaW5nIHVz
YmhzZl9kbWFfY29tcGxldGUoKSB3YXMgcnVubmluZywNCiBhIHNvZnRpcnEgaGFwcGVuZWQuIEFm
dGVyIG5jbV90eF90YXNrbGV0KCkgd2FzIGNhbGxlZCwgdGhlIGlzc3VlIGhhcHBlbmVkLg0KaHR0
cDovL3RocmVhZC5nbWFuZS5vcmcvZ21hbmUubGludXgudXNiLmdlbmVyYWwvMTIyMDIzL2ZvY3Vz
PTQzNzI5DQoNCi0gVGhpcyBtZWFucyB0aGF0IHVzYmhzZl9kbWFfY29tcGxldGUoKSByYW4gb24g
YSBrdGhyZWFkLiBTbywgbmNtIGRyaXZlciBpcyBhYmxlDQogIHRvIGRvIHRoZSBuY21fdHhfdGFz
a2xldCgpLg0KDQotIEFmdGVyIHRoZSBjdXJyZW50IHVzYi1kbWFjIGRyaXZlciwgc2luY2UgdXNi
aHNmX2RtYV9jb21wbGV0ZSgpIHJ1bnMgb24gYSB0YXNrbGV0LA0KICBuY20gZHJpdmVyIGlzIG5v
dCBhYmxlIHRvIGRvIHRoZSBuY21fdHhfdGFza2xldCBkdXJpbmcgdXNiaHNmX2RtYV9jb21wbGV0
ZSgpIHdhcyBydW5uaW5nLg0KDQoNClNvLCBJIHdvdWxkIGxpa2UgdG8gcmVjYWxsIHRoaXMgcGF0
Y2ggYW5kIEkgd2lsbCByZXN1Ym1pdCB0aGlzIHBhdGNoIHNlcmllcyBhcyB2My4NCg0KQmVzdCBy
ZWdhcmRzLA0KWW9zaGloaXJvIFNoaW1vZGENCg0K

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi Geert-san again,

> Hi Geert-san,
> 
> Thank you for the reply again!
> 
> > Hi Shimoda-san,
> >
> > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > According to the gadget.h, a "complete" function will always be called
> > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > is called with interrupts enabled. So, this function should calls
> > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > complete function.
> >
> > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > this is usually an indicator of another locking problem.
> 
> I also think that I would like to avoid using local_irq_save().
> But, I have no idea to resolve this issue for now.

After I modified usb-dmac driver to use a tasklet instead of a kthread,
this issue disappeared.

My understanding is the followings:
- According to the backtrace below, during usbhsf_dma_complete() was running,
 a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729

- This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
  to do the ncm_tx_tasklet().

- After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
  ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.


So, I would like to recall this patch and I will resubmit this patch series as v3.

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> Hi Geert-san again,
> 
> > Hi Geert-san,
> > 
> > Thank you for the reply again!
> > 
> > > Hi Shimoda-san,
> > >
> > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > According to the gadget.h, a "complete" function will always be called
> > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > is called with interrupts enabled. So, this function should calls
> > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > complete function.
> > >
> > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > this is usually an indicator of another locking problem.
> > 
> > I also think that I would like to avoid using local_irq_save().
> > But, I have no idea to resolve this issue for now.
> 
> After I modified usb-dmac driver to use a tasklet instead of a kthread,
> this issue disappeared.
> 
> My understanding is the followings:
> - According to the backtrace below, during usbhsf_dma_complete() was running,
>  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> 
> - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
>   to do the ncm_tx_tasklet().
> 
> - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
>   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> 
> 
> So, I would like to recall this patch and I will resubmit this patch series as v3.

try something like below:

diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index e0384af77e56..e9d75d85be59 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
 /*
  *		queue push/pop
  */
-static void usbhsg_queue_pop(struct usbhsg_uep *uep,
+static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
 			     struct usbhsg_request *ureq,
 			     int status)
 {
@@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
 	usb_gadget_giveback_request(&uep->ep, &ureq->req);
 }
 
+static void usbhsg_queue_pop(struct usbhsg_uep *uep,
+			     struct usbhsg_request *ureq,
+			     int status)
+{
+	usbhs_lock(priv, flags);
+	__usbhsg_queue_pop(uep, ureq, status);
+	usbhs_unlock(priv, flags);
+}
+
 static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;


then, for cases where lock is already held you call __usbhsg_queue_pop()
and for all other cases, call usbhsg_queue_pop().

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> Hi Geert-san again,
> 
> > Hi Geert-san,
> > 
> > Thank you for the reply again!
> > 
> > > Hi Shimoda-san,
> > >
> > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > According to the gadget.h, a "complete" function will always be called
> > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > is called with interrupts enabled. So, this function should calls
> > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > complete function.
> > >
> > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > this is usually an indicator of another locking problem.
> > 
> > I also think that I would like to avoid using local_irq_save().
> > But, I have no idea to resolve this issue for now.
> 
> After I modified usb-dmac driver to use a tasklet instead of a kthread,
> this issue disappeared.
> 
> My understanding is the followings:
> - According to the backtrace below, during usbhsf_dma_complete() was running,
>  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> 
> - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
>   to do the ncm_tx_tasklet().
> 
> - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
>   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> 
> 
> So, I would like to recall this patch and I will resubmit this patch series as v3.

try something like below:

diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index e0384af77e56..e9d75d85be59 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
 /*
  *		queue push/pop
  */
-static void usbhsg_queue_pop(struct usbhsg_uep *uep,
+static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
 			     struct usbhsg_request *ureq,
 			     int status)
 {
@@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
 	usb_gadget_giveback_request(&uep->ep, &ureq->req);
 }
 
+static void usbhsg_queue_pop(struct usbhsg_uep *uep,
+			     struct usbhsg_request *ureq,
+			     int status)
+{
+	usbhs_lock(priv, flags);
+	__usbhsg_queue_pop(uep, ureq, status);
+	usbhs_unlock(priv, flags);
+}
+
 static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;


then, for cases where lock is already held you call __usbhsg_queue_pop()
and for all other cases, call usbhsg_queue_pop().

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> > Hi Geert-san again,
> >
> > > Hi Geert-san,
> > >
> > > Thank you for the reply again!
> > >
> > > > Hi Shimoda-san,
> > > >
> > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > According to the gadget.h, a "complete" function will always be called
> > > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > > is called with interrupts enabled. So, this function should calls
> > > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > > complete function.
> > > >
> > > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > > this is usually an indicator of another locking problem.
> > >
> > > I also think that I would like to avoid using local_irq_save().
> > > But, I have no idea to resolve this issue for now.
> >
> > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > this issue disappeared.
> >
> > My understanding is the followings:
> > - According to the backtrace below, during usbhsf_dma_complete() was running,
> >  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> > http://thread.gmane.org/gmane.linux.usb.general/122023/focusC729
> >
> > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
> >   to do the ncm_tx_tasklet().
> >
> > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
> >   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> >
> >
> > So, I would like to recall this patch and I will resubmit this patch series as v3.
> 
> try something like below:
> 
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index e0384af77e56..e9d75d85be59 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
>  /*
>   *		queue push/pop
>   */
> -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
>  			     struct usbhsg_request *ureq,
>  			     int status)
>  {
> @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
>  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
>  }
> 
> +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> +			     struct usbhsg_request *ureq,
> +			     int status)
> +{
> +	usbhs_lock(priv, flags);
> +	__usbhsg_queue_pop(uep, ureq, status);
> +	usbhs_unlock(priv, flags);
> +}
> +
>  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
>  {
>  	struct usbhs_pipe *pipe = pkt->pipe;
> 
> 
> then, for cases where lock is already held you call __usbhsg_queue_pop()
> and for all other cases, call usbhsg_queue_pop().

Thank you for the suggestion. However, we cannot use this usbhsg_queue_pop() because
a gadget driver might call usb_ep_queue() in the "complete" function and
this driver calls usbhs_lock() in the usb_ep_queue().

Perhaps my explanation was bad, but this issue was caused by the following conditions:
 - I use the renesas_usbhs driver as udc.
 - I use an old usb-dmac driver that the callback runs on a kthread.
 - I use the ncm driver. In this environment, the ncm driver might cause a spinlock suspected.

As an old solution, I fixed the renesas_usbhs driver by this patch.
However, if I use a new usb-dmac driver that the callback runs on a tasklet,
I don't need this patch. (This is a new solution.)

Best regards,
Yoshihiro Shimoda

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> > Hi Geert-san again,
> >
> > > Hi Geert-san,
> > >
> > > Thank you for the reply again!
> > >
> > > > Hi Shimoda-san,
> > > >
> > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > According to the gadget.h, a "complete" function will always be called
> > > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > > is called with interrupts enabled. So, this function should calls
> > > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > > complete function.
> > > >
> > > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > > this is usually an indicator of another locking problem.
> > >
> > > I also think that I would like to avoid using local_irq_save().
> > > But, I have no idea to resolve this issue for now.
> >
> > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > this issue disappeared.
> >
> > My understanding is the followings:
> > - According to the backtrace below, during usbhsf_dma_complete() was running,
> >  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> > http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> >
> > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
> >   to do the ncm_tx_tasklet().
> >
> > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
> >   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> >
> >
> > So, I would like to recall this patch and I will resubmit this patch series as v3.
> 
> try something like below:
> 
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index e0384af77e56..e9d75d85be59 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
>  /*
>   *		queue push/pop
>   */
> -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
>  			     struct usbhsg_request *ureq,
>  			     int status)
>  {
> @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
>  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
>  }
> 
> +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> +			     struct usbhsg_request *ureq,
> +			     int status)
> +{
> +	usbhs_lock(priv, flags);
> +	__usbhsg_queue_pop(uep, ureq, status);
> +	usbhs_unlock(priv, flags);
> +}
> +
>  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
>  {
>  	struct usbhs_pipe *pipe = pkt->pipe;
> 
> 
> then, for cases where lock is already held you call __usbhsg_queue_pop()
> and for all other cases, call usbhsg_queue_pop().

Thank you for the suggestion. However, we cannot use this usbhsg_queue_pop() because
a gadget driver might call usb_ep_queue() in the "complete" function and
this driver calls usbhs_lock() in the usb_ep_queue().

Perhaps my explanation was bad, but this issue was caused by the following conditions:
 - I use the renesas_usbhs driver as udc.
 - I use an old usb-dmac driver that the callback runs on a kthread.
 - I use the ncm driver. In this environment, the ncm driver might cause a spinlock suspected.

As an old solution, I fixed the renesas_usbhs driver by this patch.
However, if I use a new usb-dmac driver that the callback runs on a tasklet,
I don't need this patch. (This is a new solution.)

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Thu, Mar 12, 2015 at 05:40:56AM +0000, yoshihiro shimoda wrote:
> Hi,
> 
> > On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> > > Hi Geert-san again,
> > >
> > > > Hi Geert-san,
> > > >
> > > > Thank you for the reply again!
> > > >
> > > > > Hi Shimoda-san,
> > > > >
> > > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > > According to the gadget.h, a "complete" function will always be called
> > > > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > > > is called with interrupts enabled. So, this function should calls
> > > > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > > > complete function.
> > > > >
> > > > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > > > this is usually an indicator of another locking problem.
> > > >
> > > > I also think that I would like to avoid using local_irq_save().
> > > > But, I have no idea to resolve this issue for now.
> > >
> > > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > > this issue disappeared.
> > >
> > > My understanding is the followings:
> > > - According to the backtrace below, during usbhsf_dma_complete() was running,
> > >  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> > > http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> > >
> > > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
> > >   to do the ncm_tx_tasklet().
> > >
> > > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
> > >   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> > >
> > >
> > > So, I would like to recall this patch and I will resubmit this patch series as v3.
> > 
> > try something like below:
> > 
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > index e0384af77e56..e9d75d85be59 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> >  /*
> >   *		queue push/pop
> >   */
> > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> >  			     struct usbhsg_request *ureq,
> >  			     int status)
> >  {
> > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> >  }
> > 
> > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > +			     struct usbhsg_request *ureq,
> > +			     int status)
> > +{
> > +	usbhs_lock(priv, flags);
> > +	__usbhsg_queue_pop(uep, ureq, status);
> > +	usbhs_unlock(priv, flags);
> > +}
> > +
> >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> >  {
> >  	struct usbhs_pipe *pipe = pkt->pipe;
> > 
> > 
> > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > and for all other cases, call usbhsg_queue_pop().
> 
> Thank you for the suggestion. However, we cannot use this
> usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> in the "complete" function and this driver calls usbhs_lock() in the
> usb_ep_queue().

right, in that case just call __usbhs_queue_pop() directly.

> Perhaps my explanation was bad, but this issue was caused by the
> following conditions:
>  - I use the renesas_usbhs driver as udc.
>  - I use an old usb-dmac driver that the callback runs on a kthread.
>  - I use the ncm driver. In this environment, the ncm driver might
>	cause a spinlock suspected.
> 
> As an old solution, I fixed the renesas_usbhs driver by this patch.
> However, if I use a new usb-dmac driver that the callback runs on a
> tasklet, I don't need this patch. (This is a new solution.)

then differentiate based on some revision register or something like
that ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Thu, Mar 12, 2015 at 05:40:56AM +0000, yoshihiro shimoda wrote:
> Hi,
> 
> > On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> > > Hi Geert-san again,
> > >
> > > > Hi Geert-san,
> > > >
> > > > Thank you for the reply again!
> > > >
> > > > > Hi Shimoda-san,
> > > > >
> > > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > > According to the gadget.h, a "complete" function will always be called
> > > > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > > > is called with interrupts enabled. So, this function should calls
> > > > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > > > complete function.
> > > > >
> > > > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > > > this is usually an indicator of another locking problem.
> > > >
> > > > I also think that I would like to avoid using local_irq_save().
> > > > But, I have no idea to resolve this issue for now.
> > >
> > > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > > this issue disappeared.
> > >
> > > My understanding is the followings:
> > > - According to the backtrace below, during usbhsf_dma_complete() was running,
> > >  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> > > http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> > >
> > > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
> > >   to do the ncm_tx_tasklet().
> > >
> > > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
> > >   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> > >
> > >
> > > So, I would like to recall this patch and I will resubmit this patch series as v3.
> > 
> > try something like below:
> > 
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > index e0384af77e56..e9d75d85be59 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> >  /*
> >   *		queue push/pop
> >   */
> > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> >  			     struct usbhsg_request *ureq,
> >  			     int status)
> >  {
> > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> >  }
> > 
> > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > +			     struct usbhsg_request *ureq,
> > +			     int status)
> > +{
> > +	usbhs_lock(priv, flags);
> > +	__usbhsg_queue_pop(uep, ureq, status);
> > +	usbhs_unlock(priv, flags);
> > +}
> > +
> >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> >  {
> >  	struct usbhs_pipe *pipe = pkt->pipe;
> > 
> > 
> > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > and for all other cases, call usbhsg_queue_pop().
> 
> Thank you for the suggestion. However, we cannot use this
> usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> in the "complete" function and this driver calls usbhs_lock() in the
> usb_ep_queue().

right, in that case just call __usbhs_queue_pop() directly.

> Perhaps my explanation was bad, but this issue was caused by the
> following conditions:
>  - I use the renesas_usbhs driver as udc.
>  - I use an old usb-dmac driver that the callback runs on a kthread.
>  - I use the ncm driver. In this environment, the ncm driver might
>	cause a spinlock suspected.
> 
> As an old solution, I fixed the renesas_usbhs driver by this patch.
> However, if I use a new usb-dmac driver that the callback runs on a
> tasklet, I don't need this patch. (This is a new solution.)

then differentiate based on some revision register or something like
that ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> Hi,
> 
> On Thu, Mar 12, 2015 at 05:40:56AM +0000, yoshihiro shimoda wrote:
> > Hi,
> >
< snip >
> > >
> > > try something like below:
> > >
> > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > index e0384af77e56..e9d75d85be59 100644
> > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > >  /*
> > >   *		queue push/pop
> > >   */
> > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > >  			     struct usbhsg_request *ureq,
> > >  			     int status)
> > >  {
> > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > >  }
> > >
> > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > +			     struct usbhsg_request *ureq,
> > > +			     int status)
> > > +{
> > > +	usbhs_lock(priv, flags);
> > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > +	usbhs_unlock(priv, flags);
> > > +}
> > > +
> > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > >  {
> > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > >
> > >
> > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > and for all other cases, call usbhsg_queue_pop().
> >
> > Thank you for the suggestion. However, we cannot use this
> > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > in the "complete" function and this driver calls usbhs_lock() in the
> > usb_ep_queue().
> 
> right, in that case just call __usbhs_queue_pop() directly.

Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
because this driver cannot know that a gadget driver calls usb_ep_queue() or not
in the "complete" function.

> > Perhaps my explanation was bad, but this issue was caused by the
> > following conditions:
> >  - I use the renesas_usbhs driver as udc.
> >  - I use an old usb-dmac driver that the callback runs on a kthread.
> >  - I use the ncm driver. In this environment, the ncm driver might
> >	cause a spinlock suspected.
> >
> > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > However, if I use a new usb-dmac driver that the callback runs on a
> > tasklet, I don't need this patch. (This is a new solution.)
> 
> then differentiate based on some revision register or something like
> that ?

Sorry for the lack information. I am submitting this usb-dmac driver that
the callback runs on a tasklet now. This means that the driver is not
merged yet. So, I think that we don't need to differentiate.

Best regards,
Yoshihiro Shimoda

> --
> balbi

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> Hi,
> 
> On Thu, Mar 12, 2015 at 05:40:56AM +0000, yoshihiro shimoda wrote:
> > Hi,
> >
< snip >
> > >
> > > try something like below:
> > >
> > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > index e0384af77e56..e9d75d85be59 100644
> > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > >  /*
> > >   *		queue push/pop
> > >   */
> > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > >  			     struct usbhsg_request *ureq,
> > >  			     int status)
> > >  {
> > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > >  }
> > >
> > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > +			     struct usbhsg_request *ureq,
> > > +			     int status)
> > > +{
> > > +	usbhs_lock(priv, flags);
> > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > +	usbhs_unlock(priv, flags);
> > > +}
> > > +
> > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > >  {
> > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > >
> > >
> > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > and for all other cases, call usbhsg_queue_pop().
> >
> > Thank you for the suggestion. However, we cannot use this
> > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > in the "complete" function and this driver calls usbhs_lock() in the
> > usb_ep_queue().
> 
> right, in that case just call __usbhs_queue_pop() directly.

Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
because this driver cannot know that a gadget driver calls usb_ep_queue() or not
in the "complete" function.

> > Perhaps my explanation was bad, but this issue was caused by the
> > following conditions:
> >  - I use the renesas_usbhs driver as udc.
> >  - I use an old usb-dmac driver that the callback runs on a kthread.
> >  - I use the ncm driver. In this environment, the ncm driver might
> >	cause a spinlock suspected.
> >
> > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > However, if I use a new usb-dmac driver that the callback runs on a
> > tasklet, I don't need this patch. (This is a new solution.)
> 
> then differentiate based on some revision register or something like
> that ?

Sorry for the lack information. I am submitting this usb-dmac driver that
the callback runs on a tasklet now. This means that the driver is not
merged yet. So, I think that we don't need to differentiate.

Best regards,
Yoshihiro Shimoda

> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Fri, Mar 13, 2015 at 01:14:01AM +0000, yoshihiro shimoda wrote:
> > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > index e0384af77e56..e9d75d85be59 100644
> > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > > >  /*
> > > >   *		queue push/pop
> > > >   */
> > > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  			     struct usbhsg_request *ureq,
> > > >  			     int status)
> > > >  {
> > > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > > >  }
> > > >
> > > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +			     struct usbhsg_request *ureq,
> > > > +			     int status)
> > > > +{
> > > > +	usbhs_lock(priv, flags);
> > > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > > +	usbhs_unlock(priv, flags);
> > > > +}
> > > > +
> > > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > > >  {
> > > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > > >
> > > >
> > > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > > and for all other cases, call usbhsg_queue_pop().
> > >
> > > Thank you for the suggestion. However, we cannot use this
> > > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > > in the "complete" function and this driver calls usbhs_lock() in the
> > > usb_ep_queue().
> > 
> > right, in that case just call __usbhs_queue_pop() directly.
> 
> Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
> because this driver cannot know that a gadget driver calls usb_ep_queue() or not
> in the "complete" function.

but you don't need to know that. All you have to do is spin_unlock()
before calling usb_gadget_giveback_request(), look at
drivers/usb/dwc3/gadget.c::dwc3_gadget_giveback()

> > > Perhaps my explanation was bad, but this issue was caused by the
> > > following conditions:
> > >  - I use the renesas_usbhs driver as udc.
> > >  - I use an old usb-dmac driver that the callback runs on a kthread.
> > >  - I use the ncm driver. In this environment, the ncm driver might
> > >	cause a spinlock suspected.
> > >
> > > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > > However, if I use a new usb-dmac driver that the callback runs on a
> > > tasklet, I don't need this patch. (This is a new solution.)
> > 
> > then differentiate based on some revision register or something like
> > that ?
> 
> Sorry for the lack information. I am submitting this usb-dmac driver that
> the callback runs on a tasklet now. This means that the driver is not
> merged yet. So, I think that we don't need to differentiate.

But as soon as your driver gets merged, you will need to differentiate,
right ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Fri, Mar 13, 2015 at 01:14:01AM +0000, yoshihiro shimoda wrote:
> > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > index e0384af77e56..e9d75d85be59 100644
> > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > > >  /*
> > > >   *		queue push/pop
> > > >   */
> > > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  			     struct usbhsg_request *ureq,
> > > >  			     int status)
> > > >  {
> > > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > > >  }
> > > >
> > > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +			     struct usbhsg_request *ureq,
> > > > +			     int status)
> > > > +{
> > > > +	usbhs_lock(priv, flags);
> > > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > > +	usbhs_unlock(priv, flags);
> > > > +}
> > > > +
> > > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > > >  {
> > > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > > >
> > > >
> > > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > > and for all other cases, call usbhsg_queue_pop().
> > >
> > > Thank you for the suggestion. However, we cannot use this
> > > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > > in the "complete" function and this driver calls usbhs_lock() in the
> > > usb_ep_queue().
> > 
> > right, in that case just call __usbhs_queue_pop() directly.
> 
> Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
> because this driver cannot know that a gadget driver calls usb_ep_queue() or not
> in the "complete" function.

but you don't need to know that. All you have to do is spin_unlock()
before calling usb_gadget_giveback_request(), look at
drivers/usb/dwc3/gadget.c::dwc3_gadget_giveback()

> > > Perhaps my explanation was bad, but this issue was caused by the
> > > following conditions:
> > >  - I use the renesas_usbhs driver as udc.
> > >  - I use an old usb-dmac driver that the callback runs on a kthread.
> > >  - I use the ncm driver. In this environment, the ncm driver might
> > >	cause a spinlock suspected.
> > >
> > > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > > However, if I use a new usb-dmac driver that the callback runs on a
> > > tasklet, I don't need this patch. (This is a new solution.)
> > 
> > then differentiate based on some revision register or something like
> > that ?
> 
> Sorry for the lack information. I am submitting this usb-dmac driver that
> the callback runs on a tasklet now. This means that the driver is not
> merged yet. So, I think that we don't need to differentiate.

But as soon as your driver gets merged, you will need to differentiate,
right ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> Hi,
> 
> On Fri, Mar 13, 2015 at 01:14:01AM +0000, yoshihiro shimoda wrote:
> > > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > index e0384af77e56..e9d75d85be59 100644
> > > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > > > >  /*
> > > > >   *		queue push/pop
> > > > >   */
> > > > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > >  			     struct usbhsg_request *ureq,
> > > > >  			     int status)
> > > > >  {
> > > > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > > > >  }
> > > > >
> > > > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > > +			     struct usbhsg_request *ureq,
> > > > > +			     int status)
> > > > > +{
> > > > > +	usbhs_lock(priv, flags);
> > > > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > > > +	usbhs_unlock(priv, flags);
> > > > > +}
> > > > > +
> > > > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > > > >  {
> > > > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > > > >
> > > > >
> > > > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > > > and for all other cases, call usbhsg_queue_pop().
> > > >
> > > > Thank you for the suggestion. However, we cannot use this
> > > > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > > > in the "complete" function and this driver calls usbhs_lock() in the
> > > > usb_ep_queue().
> > >
> > > right, in that case just call __usbhs_queue_pop() directly.
> >
> > Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
> > because this driver cannot know that a gadget driver calls usb_ep_queue() or not
> > in the "complete" function.
> 
> but you don't need to know that. All you have to do is spin_unlock()
> before calling usb_gadget_giveback_request(), look at
> drivers/usb/dwc3/gadget.c::dwc3_gadget_giveback()

Thank you for the suggestion. I understood it.

> > > > Perhaps my explanation was bad, but this issue was caused by the
> > > > following conditions:
> > > >  - I use the renesas_usbhs driver as udc.
> > > >  - I use an old usb-dmac driver that the callback runs on a kthread.
> > > >  - I use the ncm driver. In this environment, the ncm driver might
> > > >	cause a spinlock suspected.
> > > >
> > > > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > > > However, if I use a new usb-dmac driver that the callback runs on a
> > > > tasklet, I don't need this patch. (This is a new solution.)
> > >
> > > then differentiate based on some revision register or something like
> > > that ?
> >
> > Sorry for the lack information. I am submitting this usb-dmac driver that
> > the callback runs on a tasklet now. This means that the driver is not
> > merged yet. So, I think that we don't need to differentiate.
> 
> But as soon as your driver gets merged, you will need to differentiate,
> right ?

Right. So, I will submit v3 patch soon.

Best regards,
Yoshihiro Shimoda

> --
> balbi

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> Hi,
> 
> On Fri, Mar 13, 2015 at 01:14:01AM +0000, yoshihiro shimoda wrote:
> > > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > index e0384af77e56..e9d75d85be59 100644
> > > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > > > >  /*
> > > > >   *		queue push/pop
> > > > >   */
> > > > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > >  			     struct usbhsg_request *ureq,
> > > > >  			     int status)
> > > > >  {
> > > > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > > > >  }
> > > > >
> > > > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > > +			     struct usbhsg_request *ureq,
> > > > > +			     int status)
> > > > > +{
> > > > > +	usbhs_lock(priv, flags);
> > > > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > > > +	usbhs_unlock(priv, flags);
> > > > > +}
> > > > > +
> > > > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > > > >  {
> > > > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > > > >
> > > > >
> > > > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > > > and for all other cases, call usbhsg_queue_pop().
> > > >
> > > > Thank you for the suggestion. However, we cannot use this
> > > > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > > > in the "complete" function and this driver calls usbhs_lock() in the
> > > > usb_ep_queue().
> > >
> > > right, in that case just call __usbhs_queue_pop() directly.
> >
> > Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
> > because this driver cannot know that a gadget driver calls usb_ep_queue() or not
> > in the "complete" function.
> 
> but you don't need to know that. All you have to do is spin_unlock()
> before calling usb_gadget_giveback_request(), look at
> drivers/usb/dwc3/gadget.c::dwc3_gadget_giveback()

Thank you for the suggestion. I understood it.

> > > > Perhaps my explanation was bad, but this issue was caused by the
> > > > following conditions:
> > > >  - I use the renesas_usbhs driver as udc.
> > > >  - I use an old usb-dmac driver that the callback runs on a kthread.
> > > >  - I use the ncm driver. In this environment, the ncm driver might
> > > >	cause a spinlock suspected.
> > > >
> > > > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > > > However, if I use a new usb-dmac driver that the callback runs on a
> > > > tasklet, I don't need this patch. (This is a new solution.)
> > >
> > > then differentiate based on some revision register or something like
> > > that ?
> >
> > Sorry for the lack information. I am submitting this usb-dmac driver that
> > the callback runs on a tasklet now. This means that the driver is not
> > merged yet. So, I think that we don't need to differentiate.
> 
> But as soon as your driver gets merged, you will need to differentiate,
> right ?

Right. So, I will submit v3 patch soon.

Best regards,
Yoshihiro Shimoda

> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/4] usb: renesas_usbhs: fix the sequence in xfer_work()

[Yoshihiro Shimoda]

This patch fixes the setup sequence in xfer_work(). Otherwise,
sometimes a usb transaction will get stuck.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/renesas_usbhs/fifo.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 28d10eb..b737661 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -822,10 +822,10 @@ static void xfer_work(struct work_struct *work)
 		fifo->name, usbhs_pipe_number(pipe), pkt->length, pkt->zero);
 
 	usbhs_pipe_running(pipe, 1);
-	usbhs_pipe_set_trans_count_if_bulk(pipe, pkt->trans);
-	usbhs_pipe_enable(pipe);
 	usbhsf_dma_start(pipe, fifo);
+	usbhs_pipe_set_trans_count_if_bulk(pipe, pkt->trans);
 	dma_async_issue_pending(chan);
+	usbhs_pipe_enable(pipe);
 }
 
 /*
-- 
1.7.9.5

[PATCH v2 3/4] usb: renesas_usbhs: fix the sequence in xfer_work()

[Yoshihiro Shimoda]

This patch fixes the setup sequence in xfer_work(). Otherwise,
sometimes a usb transaction will get stuck.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/renesas_usbhs/fifo.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 28d10eb..b737661 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -822,10 +822,10 @@ static void xfer_work(struct work_struct *work)
 		fifo->name, usbhs_pipe_number(pipe), pkt->length, pkt->zero);
 
 	usbhs_pipe_running(pipe, 1);
-	usbhs_pipe_set_trans_count_if_bulk(pipe, pkt->trans);
-	usbhs_pipe_enable(pipe);
 	usbhsf_dma_start(pipe, fifo);
+	usbhs_pipe_set_trans_count_if_bulk(pipe, pkt->trans);
 	dma_async_issue_pending(chan);
+	usbhs_pipe_enable(pipe);
 }
 
 /*
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/4] usb: renesas_usbhs: add support for USB-DMAC

[Yoshihiro Shimoda]

Some Renesas SoCs have the USB-DMAC. It is able to terminate transfers
when a short packet is received, even if less bytes than the transfer
counter size have been received. Also, it is able to send a short
packet even if the packet size is not multiples of 8bytes.

Since the previous code has used the interruption of USBHS controller
when receiving packets even if this driver has used a dmac, a lot of
interruptions has happened. This patch will reduce such interruptions.

This patch allows to use the USB-DMAC on R-Car H2 and M2.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/usb/renesas_usbhs/common.c |   19 +++++
 drivers/usb/renesas_usbhs/common.h |    7 ++
 drivers/usb/renesas_usbhs/fifo.c   |  165 ++++++++++++++++++++++++++++++++++--
 drivers/usb/renesas_usbhs/fifo.h   |    1 +
 drivers/usb/renesas_usbhs/pipe.c   |   39 +++++++++
 drivers/usb/renesas_usbhs/pipe.h   |    1 +
 include/linux/usb/renesas_usbhs.h  |    2 +
 7 files changed, 228 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
index 4cf77d3..0f7e850 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -276,6 +276,16 @@ int usbhs_set_device_config(struct usbhs_priv *priv, int devnum,
 }
 
 /*
+ *		interrupt functions
+ */
+void usbhs_xxxsts_clear(struct usbhs_priv *priv, u16 sts_reg, u16 bit)
+{
+	u16 pipe_mask = (u16)GENMASK(usbhs_get_dparam(priv, pipe_size), 0);
+
+	usbhs_write(priv, sts_reg, ~(1 << bit) & pipe_mask);
+}
+
+/*
  *		local functions
  */
 static void usbhsc_set_buswait(struct usbhs_priv *priv)
@@ -487,6 +497,15 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
 	if (gpio > 0)
 		dparam->enable_gpio = gpio;
 
+	switch (dparam->type) {
+	case USBHS_TYPE_R8A7790:
+	case USBHS_TYPE_R8A7791:
+		dparam->has_usb_dmac = 1;
+		break;
+	default:
+		break;
+	}
+
 	return info;
 }
 
diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
index fc96e92..8c5fc12 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -193,6 +193,7 @@ struct usbhs_priv;
 #define TYPE_BULK	(1 << 14)
 #define TYPE_INT	(2 << 14)
 #define TYPE_ISO	(3 << 14)
+#define BFRE		(1 << 10)	/* BRDY Interrupt Operation Spec. */
 #define DBLB		(1 << 9)	/* Double Buffer Mode */
 #define SHTNAK		(1 << 7)	/* Pipe Disable in Transfer End */
 #define DIR_OUT		(1 << 4)	/* Transfer Direction */
@@ -216,6 +217,7 @@ struct usbhs_priv;
 #define	ACLRM		(1 << 9)	/* Buffer Auto-Clear Mode */
 #define SQCLR		(1 << 8)	/* Toggle Bit Clear */
 #define SQSET		(1 << 7)	/* Toggle Bit Set */
+#define SQMON		(1 << 6)	/* Toggle Bit Check */
 #define PBUSY		(1 << 5)	/* Pipe Busy */
 #define PID_MASK	(0x3)		/* Response PID */
 #define  PID_NAK	0
@@ -324,6 +326,11 @@ int usbhs_set_device_config(struct usbhs_priv *priv, int devnum, u16 upphub,
 			   u16 hubport, u16 speed);
 
 /*
+ * interrupt functions
+ */
+void usbhs_xxxsts_clear(struct usbhs_priv *priv, u16 sts_reg, u16 bit);
+
+/*
  * data
  */
 struct usbhs_priv *usbhs_pdev_to_priv(struct platform_device *pdev);
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index b737661..8597cf9 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -813,7 +813,8 @@ static void xfer_work(struct work_struct *work)
 	desc->callback		= usbhsf_dma_complete;
 	desc->callback_param	= pipe;
 
-	if (dmaengine_submit(desc) < 0) {
+	pkt->cookie = dmaengine_submit(desc);
+	if (pkt->cookie < 0) {
 		dev_err(dev, "Failed to submit dma descriptor\n");
 		return;
 	}
@@ -838,6 +839,7 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)
 	struct usbhs_fifo *fifo;
 	int len = pkt->length - pkt->actual;
 	int ret;
+	uintptr_t align_mask;
 
 	if (usbhs_pipe_is_busy(pipe))
 		return 0;
@@ -847,10 +849,14 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)
 	    usbhs_pipe_is_dcp(pipe))
 		goto usbhsf_pio_prepare_push;
 
-	if (len & 0x7) /* 8byte alignment */
+	/* check data length if this driver don't use USB-DMAC */
+	if (!usbhs_get_dparam(priv, has_usb_dmac) && len & 0x7)
 		goto usbhsf_pio_prepare_push;
 
-	if ((uintptr_t)(pkt->buf + pkt->actual) & 0x7) /* 8byte alignment */
+	/* check buffer alignment */
+	align_mask = usbhs_get_dparam(priv, has_usb_dmac) ?
+					USBHS_USB_DMAC_XFER_SIZE - 1 : 0x7;
+	if ((uintptr_t)(pkt->buf + pkt->actual) & align_mask)
 		goto usbhsf_pio_prepare_push;
 
 	/* return at this time if the pipe is running */
@@ -924,7 +930,85 @@ struct usbhs_pkt_handle usbhs_fifo_dma_push_handler = {
 /*
  *		DMA pop handler
  */
-static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)
+
+static int usbhsf_dma_prepare_pop_with_rx_irq(struct usbhs_pkt *pkt,
+					      int *is_done)
+{
+	return usbhsf_prepare_pop(pkt, is_done);
+}
+
+static int usbhsf_dma_prepare_pop_with_usb_dmac(struct usbhs_pkt *pkt,
+						int *is_done)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+	struct usbhs_fifo *fifo;
+	int ret;
+
+	if (usbhs_pipe_is_busy(pipe))
+		return 0;
+
+	/* use PIO if packet is less than pio_dma_border or pipe is DCP */
+	if ((pkt->length < usbhs_get_dparam(priv, pio_dma_border)) ||
+	    usbhs_pipe_is_dcp(pipe))
+		goto usbhsf_pio_prepare_pop;
+
+	fifo = usbhsf_get_dma_fifo(priv, pkt);
+	if (!fifo)
+		goto usbhsf_pio_prepare_pop;
+
+	if ((uintptr_t)pkt->buf & (USBHS_USB_DMAC_XFER_SIZE - 1))
+		goto usbhsf_pio_prepare_pop;
+
+	usbhs_pipe_config_change_bfre(pipe, 1);
+
+	ret = usbhsf_fifo_select(pipe, fifo, 0);
+	if (ret < 0)
+		goto usbhsf_pio_prepare_pop;
+
+	if (usbhsf_dma_map(pkt) < 0)
+		goto usbhsf_pio_prepare_pop_unselect;
+
+	/* DMA */
+
+	/*
+	 * usbhs_fifo_dma_pop_handler :: prepare
+	 * enabled irq to come here.
+	 * but it is no longer needed for DMA. disable it.
+	 */
+	usbhsf_rx_irq_ctrl(pipe, 0);
+
+	pkt->trans = pkt->length;
+
+	INIT_WORK(&pkt->work, xfer_work);
+	schedule_work(&pkt->work);
+
+	return 0;
+
+usbhsf_pio_prepare_pop_unselect:
+	usbhsf_fifo_unselect(pipe, fifo);
+usbhsf_pio_prepare_pop:
+
+	/*
+	 * change handler to PIO
+	 */
+	pkt->handler = &usbhs_fifo_pio_pop_handler;
+	usbhs_pipe_config_change_bfre(pipe, 0);
+
+	return pkt->handler->prepare(pkt, is_done);
+}
+
+static int usbhsf_dma_prepare_pop(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	if (usbhs_get_dparam(priv, has_usb_dmac))
+		return usbhsf_dma_prepare_pop_with_usb_dmac(pkt, is_done);
+	else
+		return usbhsf_dma_prepare_pop_with_rx_irq(pkt, is_done);
+}
+
+static int usbhsf_dma_try_pop_with_rx_irq(struct usbhs_pkt *pkt, int *is_done)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;
 	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
@@ -993,7 +1077,16 @@ usbhsf_pio_prepare_pop:
 	return pkt->handler->try_run(pkt, is_done);
 }
 
-static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
+static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	BUG_ON(usbhs_get_dparam(priv, has_usb_dmac));
+
+	return usbhsf_dma_try_pop_with_rx_irq(pkt, is_done);
+}
+
+static int usbhsf_dma_pop_done_with_rx_irq(struct usbhs_pkt *pkt, int *is_done)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;
 	int maxp = usbhs_pipe_get_maxpacket(pipe);
@@ -1017,8 +1110,68 @@ static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
 	return 0;
 }
 
+static size_t usbhs_dma_calc_received_size(struct usbhs_pkt *pkt,
+					   struct dma_chan *chan, int dtln)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct dma_tx_state state;
+	size_t received_size;
+	int maxp = usbhs_pipe_get_maxpacket(pipe);
+
+	dmaengine_tx_status(chan, pkt->cookie, &state);
+	received_size = pkt->length - state.residue;
+
+	if (dtln) {
+		received_size -= USBHS_USB_DMAC_XFER_SIZE;
+		received_size &= ~(maxp - 1);
+		received_size += dtln;
+	}
+
+	return received_size;
+}
+
+static int usbhsf_dma_pop_done_with_usb_dmac(struct usbhs_pkt *pkt,
+					     int *is_done)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+	struct usbhs_fifo *fifo = usbhs_pipe_to_fifo(pipe);
+	struct dma_chan *chan = usbhsf_dma_chan_get(fifo, pkt);
+	int rcv_len;
+
+	/*
+	 * Since the driver disables rx_irq in DMA mode, the interrupt handler
+	 * cannot the BRDYSTS. So, the function clears it here because the
+	 * driver may use PIO mode next time.
+	 */
+	usbhs_xxxsts_clear(priv, BRDYSTS, usbhs_pipe_number(pipe));
+
+	rcv_len = usbhsf_fifo_rcv_len(priv, fifo);
+	usbhsf_fifo_clear(pipe, fifo);
+	pkt->actual = usbhs_dma_calc_received_size(pkt, chan, rcv_len);
+
+	usbhsf_dma_stop(pipe, fifo);
+	usbhsf_dma_unmap(pkt);
+	usbhsf_fifo_unselect(pipe, pipe->fifo);
+
+	/* The driver can assume the rx transaction is always "done" */
+	*is_done = 1;
+
+	return 0;
+}
+
+static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	if (usbhs_get_dparam(priv, has_usb_dmac))
+		return usbhsf_dma_pop_done_with_usb_dmac(pkt, is_done);
+	else
+		return usbhsf_dma_pop_done_with_rx_irq(pkt, is_done);
+}
+
 struct usbhs_pkt_handle usbhs_fifo_dma_pop_handler = {
-	.prepare	= usbhsf_prepare_pop,
+	.prepare	= usbhsf_dma_prepare_pop,
 	.try_run	= usbhsf_dma_try_pop,
 	.dma_done	= usbhsf_dma_pop_done
 };
diff --git a/drivers/usb/renesas_usbhs/fifo.h b/drivers/usb/renesas_usbhs/fifo.h
index f07037c1..04d3f8a 100644
--- a/drivers/usb/renesas_usbhs/fifo.h
+++ b/drivers/usb/renesas_usbhs/fifo.h
@@ -58,6 +58,7 @@ struct usbhs_pkt {
 		     struct usbhs_pkt *pkt);
 	struct work_struct work;
 	dma_addr_t dma;
+	dma_cookie_t cookie;
 	void *buf;
 	int length;
 	int trans;
diff --git a/drivers/usb/renesas_usbhs/pipe.c b/drivers/usb/renesas_usbhs/pipe.c
index 007f45a..4f9c335 100644
--- a/drivers/usb/renesas_usbhs/pipe.c
+++ b/drivers/usb/renesas_usbhs/pipe.c
@@ -84,6 +84,17 @@ static void __usbhsp_pipe_xxx_set(struct usbhs_pipe *pipe,
 		usbhs_bset(priv, pipe_reg, mask, val);
 }
 
+static u16 __usbhsp_pipe_xxx_get(struct usbhs_pipe *pipe,
+				 u16 dcp_reg, u16 pipe_reg)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+
+	if (usbhs_pipe_is_dcp(pipe))
+		return usbhs_read(priv, dcp_reg);
+	else
+		return usbhs_read(priv, pipe_reg);
+}
+
 /*
  *		DCPCFG/PIPECFG functions
  */
@@ -92,6 +103,11 @@ static void usbhsp_pipe_cfg_set(struct usbhs_pipe *pipe, u16 mask, u16 val)
 	__usbhsp_pipe_xxx_set(pipe, DCPCFG, PIPECFG, mask, val);
 }
 
+static u16 usbhsp_pipe_cfg_get(struct usbhs_pipe *pipe)
+{
+	return __usbhsp_pipe_xxx_get(pipe, DCPCFG, PIPECFG);
+}
+
 /*
  *		PIPEnTRN/PIPEnTRE functions
  */
@@ -616,6 +632,11 @@ void usbhs_pipe_data_sequence(struct usbhs_pipe *pipe, int sequence)
 	usbhsp_pipectrl_set(pipe, mask, val);
 }
 
+static int usbhs_pipe_get_data_sequence(struct usbhs_pipe *pipe)
+{
+	return !!(usbhsp_pipectrl_get(pipe) & SQMON);
+}
+
 void usbhs_pipe_clear(struct usbhs_pipe *pipe)
 {
 	if (usbhs_pipe_is_dcp(pipe)) {
@@ -626,6 +647,24 @@ void usbhs_pipe_clear(struct usbhs_pipe *pipe)
 	}
 }
 
+void usbhs_pipe_config_change_bfre(struct usbhs_pipe *pipe, int enable)
+{
+	int sequence;
+
+	if (usbhs_pipe_is_dcp(pipe))
+		return;
+
+	usbhsp_pipe_select(pipe);
+	/* check if the driver needs to change the BFRE value */
+	if (!(enable ^ !!(usbhsp_pipe_cfg_get(pipe) & BFRE)))
+		return;
+
+	sequence = usbhs_pipe_get_data_sequence(pipe);
+	usbhsp_pipe_cfg_set(pipe, BFRE, enable ? BFRE : 0);
+	usbhs_pipe_clear(pipe);
+	usbhs_pipe_data_sequence(pipe, sequence);
+}
+
 static struct usbhs_pipe *usbhsp_get_pipe(struct usbhs_priv *priv, u32 type)
 {
 	struct usbhs_pipe *pos, *pipe;
diff --git a/drivers/usb/renesas_usbhs/pipe.h b/drivers/usb/renesas_usbhs/pipe.h
index d24a059..b0bc7b6 100644
--- a/drivers/usb/renesas_usbhs/pipe.h
+++ b/drivers/usb/renesas_usbhs/pipe.h
@@ -97,6 +97,7 @@ void usbhs_pipe_set_trans_count_if_bulk(struct usbhs_pipe *pipe, int len);
 void usbhs_pipe_select_fifo(struct usbhs_pipe *pipe, struct usbhs_fifo *fifo);
 void usbhs_pipe_config_update(struct usbhs_pipe *pipe, u16 devsel,
 			      u16 epnum, u16 maxp);
+void usbhs_pipe_config_change_bfre(struct usbhs_pipe *pipe, int enable);
 
 #define usbhs_pipe_sequence_data0(pipe)	usbhs_pipe_data_sequence(pipe, 0)
 #define usbhs_pipe_sequence_data1(pipe)	usbhs_pipe_data_sequence(pipe, 1)
diff --git a/include/linux/usb/renesas_usbhs.h b/include/linux/usb/renesas_usbhs.h
index 9fd9e48..f06529c 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/linux/usb/renesas_usbhs.h
@@ -165,6 +165,8 @@ struct renesas_usbhs_driver_param {
 	 */
 	u32 has_otg:1; /* for controlling PWEN/EXTLP */
 	u32 has_sudmac:1; /* for SUDMAC */
+	u32 has_usb_dmac:1; /* for USB-DMAC */
+#define USBHS_USB_DMAC_XFER_SIZE	32	/* hardcode the xfer size */
 };
 
 #define USBHS_TYPE_R8A7790 1
-- 
1.7.9.5

[PATCH v2 4/4] usb: renesas_usbhs: add support for USB-DMAC

[Yoshihiro Shimoda]

Some Renesas SoCs have the USB-DMAC. It is able to terminate transfers
when a short packet is received, even if less bytes than the transfer
counter size have been received. Also, it is able to send a short
packet even if the packet size is not multiples of 8bytes.

Since the previous code has used the interruption of USBHS controller
when receiving packets even if this driver has used a dmac, a lot of
interruptions has happened. This patch will reduce such interruptions.

This patch allows to use the USB-DMAC on R-Car H2 and M2.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/renesas_usbhs/common.c |   19 +++++
 drivers/usb/renesas_usbhs/common.h |    7 ++
 drivers/usb/renesas_usbhs/fifo.c   |  165 ++++++++++++++++++++++++++++++++++--
 drivers/usb/renesas_usbhs/fifo.h   |    1 +
 drivers/usb/renesas_usbhs/pipe.c   |   39 +++++++++
 drivers/usb/renesas_usbhs/pipe.h   |    1 +
 include/linux/usb/renesas_usbhs.h  |    2 +
 7 files changed, 228 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
index 4cf77d3..0f7e850 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -276,6 +276,16 @@ int usbhs_set_device_config(struct usbhs_priv *priv, int devnum,
 }
 
 /*
+ *		interrupt functions
+ */
+void usbhs_xxxsts_clear(struct usbhs_priv *priv, u16 sts_reg, u16 bit)
+{
+	u16 pipe_mask = (u16)GENMASK(usbhs_get_dparam(priv, pipe_size), 0);
+
+	usbhs_write(priv, sts_reg, ~(1 << bit) & pipe_mask);
+}
+
+/*
  *		local functions
  */
 static void usbhsc_set_buswait(struct usbhs_priv *priv)
@@ -487,6 +497,15 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
 	if (gpio > 0)
 		dparam->enable_gpio = gpio;
 
+	switch (dparam->type) {
+	case USBHS_TYPE_R8A7790:
+	case USBHS_TYPE_R8A7791:
+		dparam->has_usb_dmac = 1;
+		break;
+	default:
+		break;
+	}
+
 	return info;
 }
 
diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
index fc96e92..8c5fc12 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -193,6 +193,7 @@ struct usbhs_priv;
 #define TYPE_BULK	(1 << 14)
 #define TYPE_INT	(2 << 14)
 #define TYPE_ISO	(3 << 14)
+#define BFRE		(1 << 10)	/* BRDY Interrupt Operation Spec. */
 #define DBLB		(1 << 9)	/* Double Buffer Mode */
 #define SHTNAK		(1 << 7)	/* Pipe Disable in Transfer End */
 #define DIR_OUT		(1 << 4)	/* Transfer Direction */
@@ -216,6 +217,7 @@ struct usbhs_priv;
 #define	ACLRM		(1 << 9)	/* Buffer Auto-Clear Mode */
 #define SQCLR		(1 << 8)	/* Toggle Bit Clear */
 #define SQSET		(1 << 7)	/* Toggle Bit Set */
+#define SQMON		(1 << 6)	/* Toggle Bit Check */
 #define PBUSY		(1 << 5)	/* Pipe Busy */
 #define PID_MASK	(0x3)		/* Response PID */
 #define  PID_NAK	0
@@ -324,6 +326,11 @@ int usbhs_set_device_config(struct usbhs_priv *priv, int devnum, u16 upphub,
 			   u16 hubport, u16 speed);
 
 /*
+ * interrupt functions
+ */
+void usbhs_xxxsts_clear(struct usbhs_priv *priv, u16 sts_reg, u16 bit);
+
+/*
  * data
  */
 struct usbhs_priv *usbhs_pdev_to_priv(struct platform_device *pdev);
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index b737661..8597cf9 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -813,7 +813,8 @@ static void xfer_work(struct work_struct *work)
 	desc->callback		= usbhsf_dma_complete;
 	desc->callback_param	= pipe;
 
-	if (dmaengine_submit(desc) < 0) {
+	pkt->cookie = dmaengine_submit(desc);
+	if (pkt->cookie < 0) {
 		dev_err(dev, "Failed to submit dma descriptor\n");
 		return;
 	}
@@ -838,6 +839,7 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)
 	struct usbhs_fifo *fifo;
 	int len = pkt->length - pkt->actual;
 	int ret;
+	uintptr_t align_mask;
 
 	if (usbhs_pipe_is_busy(pipe))
 		return 0;
@@ -847,10 +849,14 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)
 	    usbhs_pipe_is_dcp(pipe))
 		goto usbhsf_pio_prepare_push;
 
-	if (len & 0x7) /* 8byte alignment */
+	/* check data length if this driver don't use USB-DMAC */
+	if (!usbhs_get_dparam(priv, has_usb_dmac) && len & 0x7)
 		goto usbhsf_pio_prepare_push;
 
-	if ((uintptr_t)(pkt->buf + pkt->actual) & 0x7) /* 8byte alignment */
+	/* check buffer alignment */
+	align_mask = usbhs_get_dparam(priv, has_usb_dmac) ?
+					USBHS_USB_DMAC_XFER_SIZE - 1 : 0x7;
+	if ((uintptr_t)(pkt->buf + pkt->actual) & align_mask)
 		goto usbhsf_pio_prepare_push;
 
 	/* return at this time if the pipe is running */
@@ -924,7 +930,85 @@ struct usbhs_pkt_handle usbhs_fifo_dma_push_handler = {
 /*
  *		DMA pop handler
  */
-static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)
+
+static int usbhsf_dma_prepare_pop_with_rx_irq(struct usbhs_pkt *pkt,
+					      int *is_done)
+{
+	return usbhsf_prepare_pop(pkt, is_done);
+}
+
+static int usbhsf_dma_prepare_pop_with_usb_dmac(struct usbhs_pkt *pkt,
+						int *is_done)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+	struct usbhs_fifo *fifo;
+	int ret;
+
+	if (usbhs_pipe_is_busy(pipe))
+		return 0;
+
+	/* use PIO if packet is less than pio_dma_border or pipe is DCP */
+	if ((pkt->length < usbhs_get_dparam(priv, pio_dma_border)) ||
+	    usbhs_pipe_is_dcp(pipe))
+		goto usbhsf_pio_prepare_pop;
+
+	fifo = usbhsf_get_dma_fifo(priv, pkt);
+	if (!fifo)
+		goto usbhsf_pio_prepare_pop;
+
+	if ((uintptr_t)pkt->buf & (USBHS_USB_DMAC_XFER_SIZE - 1))
+		goto usbhsf_pio_prepare_pop;
+
+	usbhs_pipe_config_change_bfre(pipe, 1);
+
+	ret = usbhsf_fifo_select(pipe, fifo, 0);
+	if (ret < 0)
+		goto usbhsf_pio_prepare_pop;
+
+	if (usbhsf_dma_map(pkt) < 0)
+		goto usbhsf_pio_prepare_pop_unselect;
+
+	/* DMA */
+
+	/*
+	 * usbhs_fifo_dma_pop_handler :: prepare
+	 * enabled irq to come here.
+	 * but it is no longer needed for DMA. disable it.
+	 */
+	usbhsf_rx_irq_ctrl(pipe, 0);
+
+	pkt->trans = pkt->length;
+
+	INIT_WORK(&pkt->work, xfer_work);
+	schedule_work(&pkt->work);
+
+	return 0;
+
+usbhsf_pio_prepare_pop_unselect:
+	usbhsf_fifo_unselect(pipe, fifo);
+usbhsf_pio_prepare_pop:
+
+	/*
+	 * change handler to PIO
+	 */
+	pkt->handler = &usbhs_fifo_pio_pop_handler;
+	usbhs_pipe_config_change_bfre(pipe, 0);
+
+	return pkt->handler->prepare(pkt, is_done);
+}
+
+static int usbhsf_dma_prepare_pop(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	if (usbhs_get_dparam(priv, has_usb_dmac))
+		return usbhsf_dma_prepare_pop_with_usb_dmac(pkt, is_done);
+	else
+		return usbhsf_dma_prepare_pop_with_rx_irq(pkt, is_done);
+}
+
+static int usbhsf_dma_try_pop_with_rx_irq(struct usbhs_pkt *pkt, int *is_done)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;
 	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
@@ -993,7 +1077,16 @@ usbhsf_pio_prepare_pop:
 	return pkt->handler->try_run(pkt, is_done);
 }
 
-static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
+static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	BUG_ON(usbhs_get_dparam(priv, has_usb_dmac));
+
+	return usbhsf_dma_try_pop_with_rx_irq(pkt, is_done);
+}
+
+static int usbhsf_dma_pop_done_with_rx_irq(struct usbhs_pkt *pkt, int *is_done)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;
 	int maxp = usbhs_pipe_get_maxpacket(pipe);
@@ -1017,8 +1110,68 @@ static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
 	return 0;
 }
 
+static size_t usbhs_dma_calc_received_size(struct usbhs_pkt *pkt,
+					   struct dma_chan *chan, int dtln)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct dma_tx_state state;
+	size_t received_size;
+	int maxp = usbhs_pipe_get_maxpacket(pipe);
+
+	dmaengine_tx_status(chan, pkt->cookie, &state);
+	received_size = pkt->length - state.residue;
+
+	if (dtln) {
+		received_size -= USBHS_USB_DMAC_XFER_SIZE;
+		received_size &= ~(maxp - 1);
+		received_size += dtln;
+	}
+
+	return received_size;
+}
+
+static int usbhsf_dma_pop_done_with_usb_dmac(struct usbhs_pkt *pkt,
+					     int *is_done)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+	struct usbhs_fifo *fifo = usbhs_pipe_to_fifo(pipe);
+	struct dma_chan *chan = usbhsf_dma_chan_get(fifo, pkt);
+	int rcv_len;
+
+	/*
+	 * Since the driver disables rx_irq in DMA mode, the interrupt handler
+	 * cannot the BRDYSTS. So, the function clears it here because the
+	 * driver may use PIO mode next time.
+	 */
+	usbhs_xxxsts_clear(priv, BRDYSTS, usbhs_pipe_number(pipe));
+
+	rcv_len = usbhsf_fifo_rcv_len(priv, fifo);
+	usbhsf_fifo_clear(pipe, fifo);
+	pkt->actual = usbhs_dma_calc_received_size(pkt, chan, rcv_len);
+
+	usbhsf_dma_stop(pipe, fifo);
+	usbhsf_dma_unmap(pkt);
+	usbhsf_fifo_unselect(pipe, pipe->fifo);
+
+	/* The driver can assume the rx transaction is always "done" */
+	*is_done = 1;
+
+	return 0;
+}
+
+static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	if (usbhs_get_dparam(priv, has_usb_dmac))
+		return usbhsf_dma_pop_done_with_usb_dmac(pkt, is_done);
+	else
+		return usbhsf_dma_pop_done_with_rx_irq(pkt, is_done);
+}
+
 struct usbhs_pkt_handle usbhs_fifo_dma_pop_handler = {
-	.prepare	= usbhsf_prepare_pop,
+	.prepare	= usbhsf_dma_prepare_pop,
 	.try_run	= usbhsf_dma_try_pop,
 	.dma_done	= usbhsf_dma_pop_done
 };
diff --git a/drivers/usb/renesas_usbhs/fifo.h b/drivers/usb/renesas_usbhs/fifo.h
index f07037c1..04d3f8a 100644
--- a/drivers/usb/renesas_usbhs/fifo.h
+++ b/drivers/usb/renesas_usbhs/fifo.h
@@ -58,6 +58,7 @@ struct usbhs_pkt {
 		     struct usbhs_pkt *pkt);
 	struct work_struct work;
 	dma_addr_t dma;
+	dma_cookie_t cookie;
 	void *buf;
 	int length;
 	int trans;
diff --git a/drivers/usb/renesas_usbhs/pipe.c b/drivers/usb/renesas_usbhs/pipe.c
index 007f45a..4f9c335 100644
--- a/drivers/usb/renesas_usbhs/pipe.c
+++ b/drivers/usb/renesas_usbhs/pipe.c
@@ -84,6 +84,17 @@ static void __usbhsp_pipe_xxx_set(struct usbhs_pipe *pipe,
 		usbhs_bset(priv, pipe_reg, mask, val);
 }
 
+static u16 __usbhsp_pipe_xxx_get(struct usbhs_pipe *pipe,
+				 u16 dcp_reg, u16 pipe_reg)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+
+	if (usbhs_pipe_is_dcp(pipe))
+		return usbhs_read(priv, dcp_reg);
+	else
+		return usbhs_read(priv, pipe_reg);
+}
+
 /*
  *		DCPCFG/PIPECFG functions
  */
@@ -92,6 +103,11 @@ static void usbhsp_pipe_cfg_set(struct usbhs_pipe *pipe, u16 mask, u16 val)
 	__usbhsp_pipe_xxx_set(pipe, DCPCFG, PIPECFG, mask, val);
 }
 
+static u16 usbhsp_pipe_cfg_get(struct usbhs_pipe *pipe)
+{
+	return __usbhsp_pipe_xxx_get(pipe, DCPCFG, PIPECFG);
+}
+
 /*
  *		PIPEnTRN/PIPEnTRE functions
  */
@@ -616,6 +632,11 @@ void usbhs_pipe_data_sequence(struct usbhs_pipe *pipe, int sequence)
 	usbhsp_pipectrl_set(pipe, mask, val);
 }
 
+static int usbhs_pipe_get_data_sequence(struct usbhs_pipe *pipe)
+{
+	return !!(usbhsp_pipectrl_get(pipe) & SQMON);
+}
+
 void usbhs_pipe_clear(struct usbhs_pipe *pipe)
 {
 	if (usbhs_pipe_is_dcp(pipe)) {
@@ -626,6 +647,24 @@ void usbhs_pipe_clear(struct usbhs_pipe *pipe)
 	}
 }
 
+void usbhs_pipe_config_change_bfre(struct usbhs_pipe *pipe, int enable)
+{
+	int sequence;
+
+	if (usbhs_pipe_is_dcp(pipe))
+		return;
+
+	usbhsp_pipe_select(pipe);
+	/* check if the driver needs to change the BFRE value */
+	if (!(enable ^ !!(usbhsp_pipe_cfg_get(pipe) & BFRE)))
+		return;
+
+	sequence = usbhs_pipe_get_data_sequence(pipe);
+	usbhsp_pipe_cfg_set(pipe, BFRE, enable ? BFRE : 0);
+	usbhs_pipe_clear(pipe);
+	usbhs_pipe_data_sequence(pipe, sequence);
+}
+
 static struct usbhs_pipe *usbhsp_get_pipe(struct usbhs_priv *priv, u32 type)
 {
 	struct usbhs_pipe *pos, *pipe;
diff --git a/drivers/usb/renesas_usbhs/pipe.h b/drivers/usb/renesas_usbhs/pipe.h
index d24a059..b0bc7b6 100644
--- a/drivers/usb/renesas_usbhs/pipe.h
+++ b/drivers/usb/renesas_usbhs/pipe.h
@@ -97,6 +97,7 @@ void usbhs_pipe_set_trans_count_if_bulk(struct usbhs_pipe *pipe, int len);
 void usbhs_pipe_select_fifo(struct usbhs_pipe *pipe, struct usbhs_fifo *fifo);
 void usbhs_pipe_config_update(struct usbhs_pipe *pipe, u16 devsel,
 			      u16 epnum, u16 maxp);
+void usbhs_pipe_config_change_bfre(struct usbhs_pipe *pipe, int enable);
 
 #define usbhs_pipe_sequence_data0(pipe)	usbhs_pipe_data_sequence(pipe, 0)
 #define usbhs_pipe_sequence_data1(pipe)	usbhs_pipe_data_sequence(pipe, 1)
diff --git a/include/linux/usb/renesas_usbhs.h b/include/linux/usb/renesas_usbhs.h
index 9fd9e48..f06529c 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/linux/usb/renesas_usbhs.h
@@ -165,6 +165,8 @@ struct renesas_usbhs_driver_param {
 	 */
 	u32 has_otg:1; /* for controlling PWEN/EXTLP */
 	u32 has_sudmac:1; /* for SUDMAC */
+	u32 has_usb_dmac:1; /* for USB-DMAC */
+#define USBHS_USB_DMAC_XFER_SIZE	32	/* hardcode the xfer size */
 };
 
 #define USBHS_TYPE_R8A7790 1
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html