[PATCH v7 0/5] dwc3: omap: fixes and dual-role preparation

[PATCH v7 0/5] dwc3: omap: fixes and dual-role preparation

[Roger Quadros]

Hi Felipe,

I've removed the patches that add dual role support. These are just
some fixes can go in before any actual dual role support.

About the OTG software mailbox, I tried checking internally in TI but
nobody was aware of any constraints. The events can be set as they are
obtained from the PMIC OTG comparators. My gut feeling is that we didn't have
VBUS and ID events working properly back then and so resulting
in a hackish software OTG mailbox implementation.

I tested this on OMAP5, DRA7 and didn't observe any regressions.

Based on balbi/next commit 2a58f9c12bb360f38fb39e470bb5ff94014356e6.

v7:
- remove patches adding dual-role support.
- split out shared irq conversion from threaded irq conversion patch.
- added a new patch about not touching POWERPRESENT bit.

v6:
- use just otg irq to get otg events and don't depend on extcon at all.
- follow OTG flow in TRM strictly.
- use tracepoints instead of dev_dbg().
- match IRQ flags in dwc3_omap and core.c for shared otg interrupt.

v5: Internal revision. Not sent to mailing list.

v4: first version that was reviewed.

cheers,
-roger

Roger Quadros (5):
  usb: dwc3: omap: use request_threaded_irq()
  usb: dwc3: omap: Mark the interrupt handler as shared
  usb: dwc3: omap: Don't set POWERPRESENT
  usb: dwc3: omap: Pass VBUS and ID events transparently
  usb: dwc3: core: cleanup IRQ resources

 drivers/usb/dwc3/core.c      | 26 ++++++++++++++++----------
 drivers/usb/dwc3/core.h      |  5 +++++
 drivers/usb/dwc3/dwc3-omap.c | 33 +++++++++++++++++----------------
 drivers/usb/dwc3/gadget.c    | 19 ++++++++++++++++++-
 drivers/usb/dwc3/host.c      | 19 +++++++++++++++++++
 5 files changed, 75 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH v7 0/5] dwc3: omap: fixes and dual-role preparation

[Roger Quadros]

Hi Felipe,

I've removed the patches that add dual role support. These are just
some fixes can go in before any actual dual role support.

About the OTG software mailbox, I tried checking internally in TI but
nobody was aware of any constraints. The events can be set as they are
obtained from the PMIC OTG comparators. My gut feeling is that we didn't have
VBUS and ID events working properly back then and so resulting
in a hackish software OTG mailbox implementation.

I tested this on OMAP5, DRA7 and didn't observe any regressions.

Based on balbi/next commit 2a58f9c12bb360f38fb39e470bb5ff94014356e6.

v7:
- remove patches adding dual-role support.
- split out shared irq conversion from threaded irq conversion patch.
- added a new patch about not touching POWERPRESENT bit.

v6:
- use just otg irq to get otg events and don't depend on extcon at all.
- follow OTG flow in TRM strictly.
- use tracepoints instead of dev_dbg().
- match IRQ flags in dwc3_omap and core.c for shared otg interrupt.

v5: Internal revision. Not sent to mailing list.

v4: first version that was reviewed.

cheers,
-roger

Roger Quadros (5):
  usb: dwc3: omap: use request_threaded_irq()
  usb: dwc3: omap: Mark the interrupt handler as shared
  usb: dwc3: omap: Don't set POWERPRESENT
  usb: dwc3: omap: Pass VBUS and ID events transparently
  usb: dwc3: core: cleanup IRQ resources

 drivers/usb/dwc3/core.c      | 26 ++++++++++++++++----------
 drivers/usb/dwc3/core.h      |  5 +++++
 drivers/usb/dwc3/dwc3-omap.c | 33 +++++++++++++++++----------------
 drivers/usb/dwc3/gadget.c    | 19 ++++++++++++++++++-
 drivers/usb/dwc3/host.c      | 19 +++++++++++++++++++
 5 files changed, 75 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

We intend to share this interrupt with the OTG driver an to ensure
that irqflags match for the shared interrupt handlers we use
request_threaded_irq()

If we don't use request_treaded_irq() then forced threaded irq will
set IRQF_ONESHOT and this won't match with the OTG IRQ handler's
IRQ flags.

NOTE: OTG IRQ handler is yet to be added. This is a preparatory step.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 59ea35f..6504e94 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -272,16 +272,22 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
 {
 	struct dwc3_omap	*omap = _omap;
 	u32			reg;
+	int			ret = IRQ_NONE;
 
 	reg = dwc3_omap_read_irqmisc_status(omap);
 
+	if (reg)
+		ret = IRQ_HANDLED;
+
 	dwc3_omap_write_irqmisc_status(omap, reg);
 
 	reg = dwc3_omap_read_irq0_status(omap);
+	if (reg)
+		ret = IRQ_HANDLED;
 
 	dwc3_omap_write_irq0_status(omap, reg);
 
-	return IRQ_HANDLED;
+	return ret;
 }
 
 static void dwc3_omap_enable_irqs(struct dwc3_omap *omap)
@@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	/* check the DMA Status */
 	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
 
-	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
-			"dwc3-omap", omap);
+	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
+					NULL, 0, "dwc3-omap", omap);
 	if (ret) {
 		dev_err(dev, "failed to request IRQ #%d --> %d\n",
 				omap->irq, ret);
-- 
2.7.4

[PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

We intend to share this interrupt with the OTG driver an to ensure
that irqflags match for the shared interrupt handlers we use
request_threaded_irq()

If we don't use request_treaded_irq() then forced threaded irq will
set IRQF_ONESHOT and this won't match with the OTG IRQ handler's
IRQ flags.

NOTE: OTG IRQ handler is yet to be added. This is a preparatory step.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 59ea35f..6504e94 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -272,16 +272,22 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
 {
 	struct dwc3_omap	*omap = _omap;
 	u32			reg;
+	int			ret = IRQ_NONE;
 
 	reg = dwc3_omap_read_irqmisc_status(omap);
 
+	if (reg)
+		ret = IRQ_HANDLED;
+
 	dwc3_omap_write_irqmisc_status(omap, reg);
 
 	reg = dwc3_omap_read_irq0_status(omap);
+	if (reg)
+		ret = IRQ_HANDLED;
 
 	dwc3_omap_write_irq0_status(omap, reg);
 
-	return IRQ_HANDLED;
+	return ret;
 }
 
 static void dwc3_omap_enable_irqs(struct dwc3_omap *omap)
@@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	/* check the DMA Status */
 	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
 
-	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
-			"dwc3-omap", omap);
+	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
+					NULL, 0, "dwc3-omap", omap);
 	if (ret) {
 		dev_err(dev, "failed to request IRQ #%d --> %d\n",
 				omap->irq, ret);
-- 
2.7.4

[PATCH v7 2/5] usb: dwc3: omap: Mark the interrupt handler as shared

[Roger Quadros]

On OMAPs, OTG events come on the same IRQ so we need to share
this IRQ with the OTG device driver.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 6504e94..a30944d 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -504,7 +504,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
 
 	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
-					NULL, 0, "dwc3-omap", omap);
+					NULL, IRQF_SHARED, "dwc3-omap", omap);
 	if (ret) {
 		dev_err(dev, "failed to request IRQ #%d --> %d\n",
 				omap->irq, ret);
-- 
2.7.4

[PATCH v7 2/5] usb: dwc3: omap: Mark the interrupt handler as shared

[Roger Quadros]

On OMAPs, OTG events come on the same IRQ so we need to share
this IRQ with the OTG device driver.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 6504e94..a30944d 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -504,7 +504,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
 
 	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
-					NULL, 0, "dwc3-omap", omap);
+					NULL, IRQF_SHARED, "dwc3-omap", omap);
 	if (ret) {
 		dev_err(dev, "failed to request IRQ #%d --> %d\n",
 				omap->irq, ret);
-- 
2.7.4

[PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

TRM [1] recommends that POWERPRESENT bit must not be
set and left at it's default value of 0.

[1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
Section 23.11.4.5.1 Mailbox VBUS/ID Management

"Because PIPE powerpresent has a different meaning in host and in device mode,
and because of the redundancy with the UTMI signals, the controller ORes
together the appropriate PIPE and UTMI inputs to create its internal
VBUS status. For that reason, it is recommended to leave field
USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
fill in the USB2 VBUS status fields in the same register."

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index a30944d..1f7259d 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -234,8 +234,7 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 		val &= ~(USBOTGSS_UTMI_OTG_CTRL_IDDIG
 				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_SESSEND);
-		val |= USBOTGSS_UTMI_OTG_CTRL_SESSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT;
+		val |= USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
@@ -244,8 +243,7 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 		val &= ~USBOTGSS_UTMI_OTG_CTRL_SESSEND;
 		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG
 				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_SESSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT;
+				| USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
@@ -256,8 +254,7 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 	case OMAP_DWC3_VBUS_OFF:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~(USBOTGSS_UTMI_OTG_CTRL_SESSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT);
+				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID);
 		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND
 				| USBOTGSS_UTMI_OTG_CTRL_IDDIG;
 		dwc3_omap_write_utmi_ctrl(omap, val);
-- 
2.7.4

[PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

TRM [1] recommends that POWERPRESENT bit must not be
set and left at it's default value of 0.

[1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
Section 23.11.4.5.1 Mailbox VBUS/ID Management

"Because PIPE powerpresent has a different meaning in host and in device mode,
and because of the redundancy with the UTMI signals, the controller ORes
together the appropriate PIPE and UTMI inputs to create its internal
VBUS status. For that reason, it is recommended to leave field
USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
fill in the USB2 VBUS status fields in the same register."

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index a30944d..1f7259d 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -234,8 +234,7 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 		val &= ~(USBOTGSS_UTMI_OTG_CTRL_IDDIG
 				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_SESSEND);
-		val |= USBOTGSS_UTMI_OTG_CTRL_SESSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT;
+		val |= USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
@@ -244,8 +243,7 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 		val &= ~USBOTGSS_UTMI_OTG_CTRL_SESSEND;
 		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG
 				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_SESSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT;
+				| USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
@@ -256,8 +254,7 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 	case OMAP_DWC3_VBUS_OFF:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~(USBOTGSS_UTMI_OTG_CTRL_SESSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_POWERPRESENT);
+				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID);
 		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND
 				| USBOTGSS_UTMI_OTG_CTRL_IDDIG;
 		dwc3_omap_write_utmi_ctrl(omap, val);
-- 
2.7.4

[PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Roger Quadros]

Don't make any decisions regarding VBUS session based on ID
status. That is best left to the OTG core.

Pass ID and VBUS events independent of each other so that OTG
core knows exactly what to do.

This makes dual-role with extcon work with OTG irq on OMAP platforms.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 1f7259d..c40d301 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -231,18 +231,14 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 		}
 
 		val = dwc3_omap_read_utmi_ctrl(omap);
-		val &= ~(USBOTGSS_UTMI_OTG_CTRL_IDDIG
-				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_SESSEND);
-		val |= USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
+		val &= ~USBOTGSS_UTMI_OTG_CTRL_IDDIG;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
 	case OMAP_DWC3_VBUS_VALID:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~USBOTGSS_UTMI_OTG_CTRL_SESSEND;
-		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG
-				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
+		val |= USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
@@ -250,13 +246,15 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 	case OMAP_DWC3_ID_FLOAT:
 		if (omap->vbus_reg)
 			regulator_disable(omap->vbus_reg);
+		val = dwc3_omap_read_utmi_ctrl(omap);
+		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG;
+		dwc3_omap_write_utmi_ctrl(omap, val);
 
 	case OMAP_DWC3_VBUS_OFF:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~(USBOTGSS_UTMI_OTG_CTRL_SESSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID);
-		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND
-				| USBOTGSS_UTMI_OTG_CTRL_IDDIG;
+		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
-- 
2.7.4

[PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Roger Quadros]

Don't make any decisions regarding VBUS session based on ID
status. That is best left to the OTG core.

Pass ID and VBUS events independent of each other so that OTG
core knows exactly what to do.

This makes dual-role with extcon work with OTG irq on OMAP platforms.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 1f7259d..c40d301 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -231,18 +231,14 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 		}
 
 		val = dwc3_omap_read_utmi_ctrl(omap);
-		val &= ~(USBOTGSS_UTMI_OTG_CTRL_IDDIG
-				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
-				| USBOTGSS_UTMI_OTG_CTRL_SESSEND);
-		val |= USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
+		val &= ~USBOTGSS_UTMI_OTG_CTRL_IDDIG;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
 	case OMAP_DWC3_VBUS_VALID:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~USBOTGSS_UTMI_OTG_CTRL_SESSEND;
-		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG
-				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
+		val |= USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
@@ -250,13 +246,15 @@ static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
 	case OMAP_DWC3_ID_FLOAT:
 		if (omap->vbus_reg)
 			regulator_disable(omap->vbus_reg);
+		val = dwc3_omap_read_utmi_ctrl(omap);
+		val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG;
+		dwc3_omap_write_utmi_ctrl(omap, val);
 
 	case OMAP_DWC3_VBUS_OFF:
 		val = dwc3_omap_read_utmi_ctrl(omap);
 		val &= ~(USBOTGSS_UTMI_OTG_CTRL_SESSVALID
 				| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID);
-		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND
-				| USBOTGSS_UTMI_OTG_CTRL_IDDIG;
+		val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND;
 		dwc3_omap_write_utmi_ctrl(omap, val);
 		break;
 
-- 
2.7.4

[PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros]

Implementations might use different IRQs for
host, gadget and OTG so use named interrupt resources
to allow Device tree to specify the 3 interrupts.

Following are the interrupt names

Peripheral Interrupt - peripheral
HOST Interrupt - host
OTG Interrupt - otg

We still maintain backward compatibility for a single named
interrupt for all 3 interrupts (e.g. for dwc3-pci) and
single unnamed interrupt for all 3 interrupts (e.g. old DT).

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c   | 26 ++++++++++++++++----------
 drivers/usb/dwc3/core.h   |  5 +++++
 drivers/usb/dwc3/gadget.c | 19 ++++++++++++++++++-
 drivers/usb/dwc3/host.c   | 19 +++++++++++++++++++
 4 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c050a88..5139003 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -729,6 +729,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
 	int ret;
+	struct resource *res;
+	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
@@ -748,6 +750,20 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
+		dwc->otg_irq = platform_get_irq_byname(dwc3_pdev, "otg");
+		if (dwc->otg_irq <= 0) {
+			dwc->otg_irq = platform_get_irq_byname(dwc3_pdev,
+							       "dwc_usb3");
+			if (dwc->otg_irq <= 0) {
+				res = platform_get_resource(dwc3_pdev,
+							    IORESOURCE_IRQ, 0);
+				if (!res) {
+					dev_err(dwc->dev, "missing otg IRQ\n");
+					return -ENODEV;
+				}
+				dwc->otg_irq = res->start;
+			}
+		}
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
 		ret = dwc3_host_init(dwc);
 		if (ret) {
@@ -814,16 +830,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->mem = mem;
 	dwc->dev = dev;
 
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res) {
-		dev_err(dev, "missing IRQ\n");
-		return -ENODEV;
-	}
-	dwc->xhci_resources[1].start = res->start;
-	dwc->xhci_resources[1].end = res->end;
-	dwc->xhci_resources[1].flags = res->flags;
-	dwc->xhci_resources[1].name = res->name;
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "missing memory resource\n");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 186a886..2e20892 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @gadget_irq: IRQ number for Peripheral IRQs
+ * @otg_irq: IRQ number for OTG IRQs
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -817,6 +819,9 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 
+	int			gadget_irq;
+	int			otg_irq;
+
 	/* used for suspend/resume */
 	u32			dcfg;
 	u32			gctl;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c3b0d01..8db8d13 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1605,7 +1605,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	int			irq;
 	u32			reg;
 
-	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
+	irq = dwc->gadget_irq;
 	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
 			IRQF_SHARED, "dwc3", dwc->ev_buf);
 	if (ret) {
@@ -2781,6 +2781,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
 int dwc3_gadget_init(struct dwc3 *dwc)
 {
 	int					ret;
+	struct resource *res;
+	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
+
+	dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev, "peripheral");
+	if (dwc->gadget_irq <= 0) {
+		dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev,
+							  "dwc_usb3");
+		if (dwc->gadget_irq <= 0) {
+			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
+						    0);
+			if (!res) {
+				dev_err(dwc->dev, "missing peripheral IRQ\n");
+				return -ENODEV;
+			}
+			dwc->gadget_irq = res->start;
+		}
+	}
 
 	dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
 			&dwc->ctrl_req_addr, GFP_KERNEL);
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index c679f63..f2b60a4 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -25,6 +25,25 @@ int dwc3_host_init(struct dwc3 *dwc)
 	struct platform_device	*xhci;
 	struct usb_xhci_pdata	pdata;
 	int			ret;
+	struct resource		*res;
+	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
+
+	res = platform_get_resource_byname(dwc3_pdev, IORESOURCE_IRQ, "host");
+	if (!res) {
+		res = platform_get_resource_byname(dwc3_pdev, IORESOURCE_IRQ,
+						   "dwc_usb3");
+		if (!res) {
+			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
+						    0);
+			if (!res)
+				return -ENOMEM;
+		}
+	}
+
+	dwc->xhci_resources[1].start = res->start;
+	dwc->xhci_resources[1].end = res->end;
+	dwc->xhci_resources[1].flags = res->flags;
+	dwc->xhci_resources[1].name = res->name;
 
 	xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
 	if (!xhci) {
-- 
2.7.4

[PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros]

Implementations might use different IRQs for
host, gadget and OTG so use named interrupt resources
to allow Device tree to specify the 3 interrupts.

Following are the interrupt names

Peripheral Interrupt - peripheral
HOST Interrupt - host
OTG Interrupt - otg

We still maintain backward compatibility for a single named
interrupt for all 3 interrupts (e.g. for dwc3-pci) and
single unnamed interrupt for all 3 interrupts (e.g. old DT).

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c   | 26 ++++++++++++++++----------
 drivers/usb/dwc3/core.h   |  5 +++++
 drivers/usb/dwc3/gadget.c | 19 ++++++++++++++++++-
 drivers/usb/dwc3/host.c   | 19 +++++++++++++++++++
 4 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c050a88..5139003 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -729,6 +729,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
 	int ret;
+	struct resource *res;
+	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
@@ -748,6 +750,20 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
+		dwc->otg_irq = platform_get_irq_byname(dwc3_pdev, "otg");
+		if (dwc->otg_irq <= 0) {
+			dwc->otg_irq = platform_get_irq_byname(dwc3_pdev,
+							       "dwc_usb3");
+			if (dwc->otg_irq <= 0) {
+				res = platform_get_resource(dwc3_pdev,
+							    IORESOURCE_IRQ, 0);
+				if (!res) {
+					dev_err(dwc->dev, "missing otg IRQ\n");
+					return -ENODEV;
+				}
+				dwc->otg_irq = res->start;
+			}
+		}
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
 		ret = dwc3_host_init(dwc);
 		if (ret) {
@@ -814,16 +830,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->mem = mem;
 	dwc->dev = dev;
 
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res) {
-		dev_err(dev, "missing IRQ\n");
-		return -ENODEV;
-	}
-	dwc->xhci_resources[1].start = res->start;
-	dwc->xhci_resources[1].end = res->end;
-	dwc->xhci_resources[1].flags = res->flags;
-	dwc->xhci_resources[1].name = res->name;
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "missing memory resource\n");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 186a886..2e20892 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @gadget_irq: IRQ number for Peripheral IRQs
+ * @otg_irq: IRQ number for OTG IRQs
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -817,6 +819,9 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 
+	int			gadget_irq;
+	int			otg_irq;
+
 	/* used for suspend/resume */
 	u32			dcfg;
 	u32			gctl;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c3b0d01..8db8d13 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1605,7 +1605,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	int			irq;
 	u32			reg;
 
-	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
+	irq = dwc->gadget_irq;
 	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
 			IRQF_SHARED, "dwc3", dwc->ev_buf);
 	if (ret) {
@@ -2781,6 +2781,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
 int dwc3_gadget_init(struct dwc3 *dwc)
 {
 	int					ret;
+	struct resource *res;
+	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
+
+	dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev, "peripheral");
+	if (dwc->gadget_irq <= 0) {
+		dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev,
+							  "dwc_usb3");
+		if (dwc->gadget_irq <= 0) {
+			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
+						    0);
+			if (!res) {
+				dev_err(dwc->dev, "missing peripheral IRQ\n");
+				return -ENODEV;
+			}
+			dwc->gadget_irq = res->start;
+		}
+	}
 
 	dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
 			&dwc->ctrl_req_addr, GFP_KERNEL);
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index c679f63..f2b60a4 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -25,6 +25,25 @@ int dwc3_host_init(struct dwc3 *dwc)
 	struct platform_device	*xhci;
 	struct usb_xhci_pdata	pdata;
 	int			ret;
+	struct resource		*res;
+	struct platform_device	*dwc3_pdev = to_platform_device(dwc->dev);
+
+	res = platform_get_resource_byname(dwc3_pdev, IORESOURCE_IRQ, "host");
+	if (!res) {
+		res = platform_get_resource_byname(dwc3_pdev, IORESOURCE_IRQ,
+						   "dwc_usb3");
+		if (!res) {
+			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
+						    0);
+			if (!res)
+				return -ENOMEM;
+		}
+	}
+
+	dwc->xhci_resources[1].start = res->start;
+	dwc->xhci_resources[1].end = res->end;
+	dwc->xhci_resources[1].flags = res->flags;
+	dwc->xhci_resources[1].name = res->name;
 
 	xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
 	if (!xhci) {
-- 
2.7.4

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> TRM [1] recommends that POWERPRESENT bit must not be
> set and left at it's default value of 0.
>
> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>
> "Because PIPE powerpresent has a different meaning in host and in device mode,
> and because of the redundancy with the UTMI signals, the controller ORes
> together the appropriate PIPE and UTMI inputs to create its internal
> VBUS status. For that reason, it is recommended to leave field
> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
> fill in the USB2 VBUS status fields in the same register."
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

to make sure we avoid regressions, do you mind sharing on which
platforms you tested this patch ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
> TRM [1] recommends that POWERPRESENT bit must not be
> set and left at it's default value of 0.
>
> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>
> "Because PIPE powerpresent has a different meaning in host and in device mode,
> and because of the redundancy with the UTMI signals, the controller ORes
> together the appropriate PIPE and UTMI inputs to create its internal
> VBUS status. For that reason, it is recommended to leave field
> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
> fill in the USB2 VBUS status fields in the same register."
>
> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>

to make sure we avoid regressions, do you mind sharing on which
platforms you tested this patch ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> Don't make any decisions regarding VBUS session based on ID
> status. That is best left to the OTG core.
>
> Pass ID and VBUS events independent of each other so that OTG
> core knows exactly what to do.
>
> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

I have a feeling this will regress OMAP5432 uEVM. Did you test with that
board ?

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
> Don't make any decisions regarding VBUS session based on ID
> status. That is best left to the OTG core.
>
> Pass ID and VBUS events independent of each other so that OTG
> core knows exactly what to do.
>
> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>
> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>

I have a feeling this will regress OMAP5432 uEVM. Did you test with that
board ?

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> We intend to share this interrupt with the OTG driver an to ensure
> that irqflags match for the shared interrupt handlers we use
> request_threaded_irq()
>
> If we don't use request_treaded_irq() then forced threaded irq will
> set IRQF_ONESHOT and this won't match with the OTG IRQ handler's
> IRQ flags.
>
> NOTE: OTG IRQ handler is yet to be added. This is a preparatory step.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index 59ea35f..6504e94 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -272,16 +272,22 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>  {
>  	struct dwc3_omap	*omap = _omap;
>  	u32			reg;
> +	int			ret = IRQ_NONE;
>  
>  	reg = dwc3_omap_read_irqmisc_status(omap);
>  
> +	if (reg)
> +		ret = IRQ_HANDLED;
> +
>  	dwc3_omap_write_irqmisc_status(omap, reg);
>  
>  	reg = dwc3_omap_read_irq0_status(omap);
> +	if (reg)
> +		ret = IRQ_HANDLED;
>  
>  	dwc3_omap_write_irq0_status(omap, reg);
>  
> -	return IRQ_HANDLED;
> +	return ret;
>  }
>  
>  static void dwc3_omap_enable_irqs(struct dwc3_omap *omap)
> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>  	/* check the DMA Status */
>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>  
> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
> -			"dwc3-omap", omap);
> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
> +					NULL, 0, "dwc3-omap", omap);

if you're using threaded_irq, it's better to have a NULL top half and
valid bottom half.

In fact, since this will be shared, you could do a proper preparation
and on top half check if $this device generated the IRQ and
conditionally schedule the bottom half. Don't forget to mask device's
interrupts from top half so you can run without IRQF_ONESHOT.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
> We intend to share this interrupt with the OTG driver an to ensure
> that irqflags match for the shared interrupt handlers we use
> request_threaded_irq()
>
> If we don't use request_treaded_irq() then forced threaded irq will
> set IRQF_ONESHOT and this won't match with the OTG IRQ handler's
> IRQ flags.
>
> NOTE: OTG IRQ handler is yet to be added. This is a preparatory step.
>
> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index 59ea35f..6504e94 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -272,16 +272,22 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>  {
>  	struct dwc3_omap	*omap = _omap;
>  	u32			reg;
> +	int			ret = IRQ_NONE;
>  
>  	reg = dwc3_omap_read_irqmisc_status(omap);
>  
> +	if (reg)
> +		ret = IRQ_HANDLED;
> +
>  	dwc3_omap_write_irqmisc_status(omap, reg);
>  
>  	reg = dwc3_omap_read_irq0_status(omap);
> +	if (reg)
> +		ret = IRQ_HANDLED;
>  
>  	dwc3_omap_write_irq0_status(omap, reg);
>  
> -	return IRQ_HANDLED;
> +	return ret;
>  }
>  
>  static void dwc3_omap_enable_irqs(struct dwc3_omap *omap)
> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>  	/* check the DMA Status */
>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>  
> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
> -			"dwc3-omap", omap);
> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
> +					NULL, 0, "dwc3-omap", omap);

if you're using threaded_irq, it's better to have a NULL top half and
valid bottom half.

In fact, since this will be shared, you could do a proper preparation
and on top half check if $this device generated the IRQ and
conditionally schedule the bottom half. Don't forget to mask device's
interrupts from top half so you can run without IRQF_ONESHOT.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

On 10/05/16 12:54, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> TRM [1] recommends that POWERPRESENT bit must not be
>> set and left at it's default value of 0.
>>
>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>
>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>> and because of the redundancy with the UTMI signals, the controller ORes
>> together the appropriate PIPE and UTMI inputs to create its internal
>> VBUS status. For that reason, it is recommended to leave field
>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>> fill in the USB2 VBUS status fields in the same register."
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> to make sure we avoid regressions, do you mind sharing on which
> platforms you tested this patch ?
> 
I tested this on omap5-uevm and dra7-evm.
My am437x board stopped working so couldn't test on that one.

cheers,
-roger

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

On 10/05/16 12:54, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> TRM [1] recommends that POWERPRESENT bit must not be
>> set and left at it's default value of 0.
>>
>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>
>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>> and because of the redundancy with the UTMI signals, the controller ORes
>> together the appropriate PIPE and UTMI inputs to create its internal
>> VBUS status. For that reason, it is recommended to leave field
>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>> fill in the USB2 VBUS status fields in the same register."
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> to make sure we avoid regressions, do you mind sharing on which
> platforms you tested this patch ?
> 
I tested this on omap5-uevm and dra7-evm.
My am437x board stopped working so couldn't test on that one.

cheers,
-roger

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Roger Quadros]

On 10/05/16 12:55, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> Don't make any decisions regarding VBUS session based on ID
>> status. That is best left to the OTG core.
>>
>> Pass ID and VBUS events independent of each other so that OTG
>> core knows exactly what to do.
>>
>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
> board ?
> 

Yes. Any specific test case you would like me to test?
For now I'm just doing enumeration with g_zero.

cheers,
-roger

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Roger Quadros]

On 10/05/16 12:55, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> Don't make any decisions regarding VBUS session based on ID
>> status. That is best left to the OTG core.
>>
>> Pass ID and VBUS events independent of each other so that OTG
>> core knows exactly what to do.
>>
>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
> board ?
> 

Yes. Any specific test case you would like me to test?
For now I'm just doing enumeration with g_zero.

cheers,
-roger

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> Implementations might use different IRQs for
> host, gadget and OTG so use named interrupt resources
> to allow Device tree to specify the 3 interrupts.
>
> Following are the interrupt names
>
> Peripheral Interrupt - peripheral
> HOST Interrupt - host
> OTG Interrupt - otg
>
> We still maintain backward compatibility for a single named
> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
> single unnamed interrupt for all 3 interrupts (e.g. old DT).

cool :-)

> @@ -748,6 +750,20 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> +		dwc->otg_irq = platform_get_irq_byname(dwc3_pdev, "otg");
> +		if (dwc->otg_irq <= 0) {
> +			dwc->otg_irq = platform_get_irq_byname(dwc3_pdev,
> +							       "dwc_usb3");
> +			if (dwc->otg_irq <= 0) {
> +				res = platform_get_resource(dwc3_pdev,
> +							    IORESOURCE_IRQ, 0);
> +				if (!res) {
> +					dev_err(dwc->dev, "missing otg IRQ\n");
> +					return -ENODEV;
> +				}
> +				dwc->otg_irq = res->start;
> +			}
> +		}

I suppose this part can be removed and added only when OTG is
supported. Then dwc3_otg_init() (or whatever) can do this.

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 186a886..2e20892 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>   * @revision: revision register contents
>   * @dr_mode: requested mode of operation
> + * @gadget_irq: IRQ number for Peripheral IRQs
> + * @otg_irq: IRQ number for OTG IRQs
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
>   * @usb2_generic_phy: pointer to USB2 PHY
> @@ -817,6 +819,9 @@ struct dwc3 {
>  
>  	enum usb_dr_mode	dr_mode;
>  
> +	int			gadget_irq;
> +	int			otg_irq;

while at that, let's add host_irq too and do proper changes to dwc3/host.c

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index c3b0d01..8db8d13 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1605,7 +1605,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>  	int			irq;
>  	u32			reg;
>  
> -	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> +	irq = dwc->gadget_irq;
>  	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>  			IRQF_SHARED, "dwc3", dwc->ev_buf);
>  	if (ret) {
> @@ -2781,6 +2781,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
>  int dwc3_gadget_init(struct dwc3 *dwc)
>  {
>  	int					ret;
> +	struct resource *res;
> +	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
> +
> +	dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev, "peripheral");
> +	if (dwc->gadget_irq <= 0) {
> +		dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev,
> +							  "dwc_usb3");
> +		if (dwc->gadget_irq <= 0) {
> +			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
> +						    0);
> +			if (!res) {
> +				dev_err(dwc->dev, "missing peripheral IRQ\n");
> +				return -ENODEV;
> +			}
> +			dwc->gadget_irq = res->start;
> +		}
> +	}
>  
>  	dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
>  			&dwc->ctrl_req_addr, GFP_KERNEL);

you're regressing dwc3_gadget_stop().

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> Implementations might use different IRQs for
> host, gadget and OTG so use named interrupt resources
> to allow Device tree to specify the 3 interrupts.
>
> Following are the interrupt names
>
> Peripheral Interrupt - peripheral
> HOST Interrupt - host
> OTG Interrupt - otg
>
> We still maintain backward compatibility for a single named
> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
> single unnamed interrupt for all 3 interrupts (e.g. old DT).

cool :-)

> @@ -748,6 +750,20 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> +		dwc->otg_irq = platform_get_irq_byname(dwc3_pdev, "otg");
> +		if (dwc->otg_irq <= 0) {
> +			dwc->otg_irq = platform_get_irq_byname(dwc3_pdev,
> +							       "dwc_usb3");
> +			if (dwc->otg_irq <= 0) {
> +				res = platform_get_resource(dwc3_pdev,
> +							    IORESOURCE_IRQ, 0);
> +				if (!res) {
> +					dev_err(dwc->dev, "missing otg IRQ\n");
> +					return -ENODEV;
> +				}
> +				dwc->otg_irq = res->start;
> +			}
> +		}

I suppose this part can be removed and added only when OTG is
supported. Then dwc3_otg_init() (or whatever) can do this.

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 186a886..2e20892 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>   * @revision: revision register contents
>   * @dr_mode: requested mode of operation
> + * @gadget_irq: IRQ number for Peripheral IRQs
> + * @otg_irq: IRQ number for OTG IRQs
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
>   * @usb2_generic_phy: pointer to USB2 PHY
> @@ -817,6 +819,9 @@ struct dwc3 {
>  
>  	enum usb_dr_mode	dr_mode;
>  
> +	int			gadget_irq;
> +	int			otg_irq;

while at that, let's add host_irq too and do proper changes to dwc3/host.c

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index c3b0d01..8db8d13 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1605,7 +1605,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>  	int			irq;
>  	u32			reg;
>  
> -	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> +	irq = dwc->gadget_irq;
>  	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>  			IRQF_SHARED, "dwc3", dwc->ev_buf);
>  	if (ret) {
> @@ -2781,6 +2781,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
>  int dwc3_gadget_init(struct dwc3 *dwc)
>  {
>  	int					ret;
> +	struct resource *res;
> +	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
> +
> +	dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev, "peripheral");
> +	if (dwc->gadget_irq <= 0) {
> +		dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev,
> +							  "dwc_usb3");
> +		if (dwc->gadget_irq <= 0) {
> +			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
> +						    0);
> +			if (!res) {
> +				dev_err(dwc->dev, "missing peripheral IRQ\n");
> +				return -ENODEV;
> +			}
> +			dwc->gadget_irq = res->start;
> +		}
> +	}
>  
>  	dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
>  			&dwc->ctrl_req_addr, GFP_KERNEL);

you're regressing dwc3_gadget_stop().

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> On 10/05/16 12:54, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Roger Quadros <rogerq@ti.com> writes:
>>> TRM [1] recommends that POWERPRESENT bit must not be
>>> set and left at it's default value of 0.
>>>
>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>
>>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>>> and because of the redundancy with the UTMI signals, the controller ORes
>>> together the appropriate PIPE and UTMI inputs to create its internal
>>> VBUS status. For that reason, it is recommended to leave field
>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>>> fill in the USB2 VBUS status fields in the same register."
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> 
>> to make sure we avoid regressions, do you mind sharing on which
>> platforms you tested this patch ?
>> 
> I tested this on omap5-uevm and dra7-evm.
> My am437x board stopped working so couldn't test on that one.

would you have a colleague or perhaps an automated test-farm which could
run the test for you ? :-)

I can take the patch, no problem, but if there are any regressions don't
blame me :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 10/05/16 12:58, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> We intend to share this interrupt with the OTG driver an to ensure
>> that irqflags match for the shared interrupt handlers we use
>> request_threaded_irq()
>>
>> If we don't use request_treaded_irq() then forced threaded irq will
>> set IRQF_ONESHOT and this won't match with the OTG IRQ handler's
>> IRQ flags.
>>
>> NOTE: OTG IRQ handler is yet to be added. This is a preparatory step.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
>> index 59ea35f..6504e94 100644
>> --- a/drivers/usb/dwc3/dwc3-omap.c
>> +++ b/drivers/usb/dwc3/dwc3-omap.c
>> @@ -272,16 +272,22 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>>  {
>>  	struct dwc3_omap	*omap = _omap;
>>  	u32			reg;
>> +	int			ret = IRQ_NONE;
>>  
>>  	reg = dwc3_omap_read_irqmisc_status(omap);
>>  
>> +	if (reg)
>> +		ret = IRQ_HANDLED;
>> +
>>  	dwc3_omap_write_irqmisc_status(omap, reg);
>>  
>>  	reg = dwc3_omap_read_irq0_status(omap);
>> +	if (reg)
>> +		ret = IRQ_HANDLED;
>>  
>>  	dwc3_omap_write_irq0_status(omap, reg);
>>  
>> -	return IRQ_HANDLED;
>> +	return ret;
>>  }
>>  
>>  static void dwc3_omap_enable_irqs(struct dwc3_omap *omap)
>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>  	/* check the DMA Status */
>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>  
>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>> -			"dwc3-omap", omap);
>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>> +					NULL, 0, "dwc3-omap", omap);
> 
> if you're using threaded_irq, it's better to have a NULL top half and
> valid bottom half.

But in this case we don't need a bottom half as there is nothing to do :).

> 
> In fact, since this will be shared, you could do a proper preparation
> and on top half check if $this device generated the IRQ and
> conditionally schedule the bottom half. Don't forget to mask device's
> interrupts from top half so you can run without IRQF_ONESHOT.
> 

Why do this at all if there is nothing to do in the bottom half?

cheers,
-roger

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 10/05/16 12:58, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> We intend to share this interrupt with the OTG driver an to ensure
>> that irqflags match for the shared interrupt handlers we use
>> request_threaded_irq()
>>
>> If we don't use request_treaded_irq() then forced threaded irq will
>> set IRQF_ONESHOT and this won't match with the OTG IRQ handler's
>> IRQ flags.
>>
>> NOTE: OTG IRQ handler is yet to be added. This is a preparatory step.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
>> index 59ea35f..6504e94 100644
>> --- a/drivers/usb/dwc3/dwc3-omap.c
>> +++ b/drivers/usb/dwc3/dwc3-omap.c
>> @@ -272,16 +272,22 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>>  {
>>  	struct dwc3_omap	*omap = _omap;
>>  	u32			reg;
>> +	int			ret = IRQ_NONE;
>>  
>>  	reg = dwc3_omap_read_irqmisc_status(omap);
>>  
>> +	if (reg)
>> +		ret = IRQ_HANDLED;
>> +
>>  	dwc3_omap_write_irqmisc_status(omap, reg);
>>  
>>  	reg = dwc3_omap_read_irq0_status(omap);
>> +	if (reg)
>> +		ret = IRQ_HANDLED;
>>  
>>  	dwc3_omap_write_irq0_status(omap, reg);
>>  
>> -	return IRQ_HANDLED;
>> +	return ret;
>>  }
>>  
>>  static void dwc3_omap_enable_irqs(struct dwc3_omap *omap)
>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>  	/* check the DMA Status */
>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>  
>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>> -			"dwc3-omap", omap);
>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>> +					NULL, 0, "dwc3-omap", omap);
> 
> if you're using threaded_irq, it's better to have a NULL top half and
> valid bottom half.

But in this case we don't need a bottom half as there is nothing to do :).

> 
> In fact, since this will be shared, you could do a proper preparation
> and on top half check if $this device generated the IRQ and
> conditionally schedule the bottom half. Don't forget to mask device's
> interrupts from top half so you can run without IRQF_ONESHOT.
> 

Why do this at all if there is nothing to do in the bottom half?

cheers,
-roger

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> On 10/05/16 12:55, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Roger Quadros <rogerq@ti.com> writes:
>>> Don't make any decisions regarding VBUS session based on ID
>>> status. That is best left to the OTG core.
>>>
>>> Pass ID and VBUS events independent of each other so that OTG
>>> core knows exactly what to do.
>>>
>>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> 
>> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
>> board ?
>> 
>
> Yes. Any specific test case you would like me to test?
> For now I'm just doing enumeration with g_zero.

IIRC OMAP5 uEVM didn't have separate VBUS and ID GPIOs. How are you
handling that case ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros]

On 10/05/16 13:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> Implementations might use different IRQs for
>> host, gadget and OTG so use named interrupt resources
>> to allow Device tree to specify the 3 interrupts.
>>
>> Following are the interrupt names
>>
>> Peripheral Interrupt - peripheral
>> HOST Interrupt - host
>> OTG Interrupt - otg
>>
>> We still maintain backward compatibility for a single named
>> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
>> single unnamed interrupt for all 3 interrupts (e.g. old DT).
> 
> cool :-)
> 
>> @@ -748,6 +750,20 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>  		}
>>  		break;
>>  	case USB_DR_MODE_OTG:
>> +		dwc->otg_irq = platform_get_irq_byname(dwc3_pdev, "otg");
>> +		if (dwc->otg_irq <= 0) {
>> +			dwc->otg_irq = platform_get_irq_byname(dwc3_pdev,
>> +							       "dwc_usb3");
>> +			if (dwc->otg_irq <= 0) {
>> +				res = platform_get_resource(dwc3_pdev,
>> +							    IORESOURCE_IRQ, 0);
>> +				if (!res) {
>> +					dev_err(dwc->dev, "missing otg IRQ\n");
>> +					return -ENODEV;
>> +				}
>> +				dwc->otg_irq = res->start;
>> +			}
>> +		}
> 
> I suppose this part can be removed and added only when OTG is
> supported. Then dwc3_otg_init() (or whatever) can do this.

Sure.
> 
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 186a886..2e20892 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>   * @revision: revision register contents
>>   * @dr_mode: requested mode of operation
>> + * @gadget_irq: IRQ number for Peripheral IRQs
>> + * @otg_irq: IRQ number for OTG IRQs
>>   * @usb2_phy: pointer to USB2 PHY
>>   * @usb3_phy: pointer to USB3 PHY
>>   * @usb2_generic_phy: pointer to USB2 PHY
>> @@ -817,6 +819,9 @@ struct dwc3 {
>>  
>>  	enum usb_dr_mode	dr_mode;
>>  
>> +	int			gadget_irq;
>> +	int			otg_irq;
> 
> while at that, let's add host_irq too and do proper changes to dwc3/host.c

Sure. So we add host_irq here, and manually create an irq resource
in dwc3_host_init?
> 
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index c3b0d01..8db8d13 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1605,7 +1605,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>>  	int			irq;
>>  	u32			reg;
>>  
>> -	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
>> +	irq = dwc->gadget_irq;
>>  	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>>  			IRQF_SHARED, "dwc3", dwc->ev_buf);
>>  	if (ret) {
>> @@ -2781,6 +2781,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
>>  int dwc3_gadget_init(struct dwc3 *dwc)
>>  {
>>  	int					ret;
>> +	struct resource *res;
>> +	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
>> +
>> +	dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev, "peripheral");
>> +	if (dwc->gadget_irq <= 0) {
>> +		dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev,
>> +							  "dwc_usb3");
>> +		if (dwc->gadget_irq <= 0) {
>> +			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
>> +						    0);
>> +			if (!res) {
>> +				dev_err(dwc->dev, "missing peripheral IRQ\n");
>> +				return -ENODEV;
>> +			}
>> +			dwc->gadget_irq = res->start;
>> +		}
>> +	}
>>  
>>  	dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
>>  			&dwc->ctrl_req_addr, GFP_KERNEL);
> 
> you're regressing dwc3_gadget_stop().
> 
ah, good catch.

cheers,
-roger

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros]

On 10/05/16 13:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> Implementations might use different IRQs for
>> host, gadget and OTG so use named interrupt resources
>> to allow Device tree to specify the 3 interrupts.
>>
>> Following are the interrupt names
>>
>> Peripheral Interrupt - peripheral
>> HOST Interrupt - host
>> OTG Interrupt - otg
>>
>> We still maintain backward compatibility for a single named
>> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
>> single unnamed interrupt for all 3 interrupts (e.g. old DT).
> 
> cool :-)
> 
>> @@ -748,6 +750,20 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>  		}
>>  		break;
>>  	case USB_DR_MODE_OTG:
>> +		dwc->otg_irq = platform_get_irq_byname(dwc3_pdev, "otg");
>> +		if (dwc->otg_irq <= 0) {
>> +			dwc->otg_irq = platform_get_irq_byname(dwc3_pdev,
>> +							       "dwc_usb3");
>> +			if (dwc->otg_irq <= 0) {
>> +				res = platform_get_resource(dwc3_pdev,
>> +							    IORESOURCE_IRQ, 0);
>> +				if (!res) {
>> +					dev_err(dwc->dev, "missing otg IRQ\n");
>> +					return -ENODEV;
>> +				}
>> +				dwc->otg_irq = res->start;
>> +			}
>> +		}
> 
> I suppose this part can be removed and added only when OTG is
> supported. Then dwc3_otg_init() (or whatever) can do this.

Sure.
> 
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 186a886..2e20892 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>   * @revision: revision register contents
>>   * @dr_mode: requested mode of operation
>> + * @gadget_irq: IRQ number for Peripheral IRQs
>> + * @otg_irq: IRQ number for OTG IRQs
>>   * @usb2_phy: pointer to USB2 PHY
>>   * @usb3_phy: pointer to USB3 PHY
>>   * @usb2_generic_phy: pointer to USB2 PHY
>> @@ -817,6 +819,9 @@ struct dwc3 {
>>  
>>  	enum usb_dr_mode	dr_mode;
>>  
>> +	int			gadget_irq;
>> +	int			otg_irq;
> 
> while at that, let's add host_irq too and do proper changes to dwc3/host.c

Sure. So we add host_irq here, and manually create an irq resource
in dwc3_host_init?
> 
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index c3b0d01..8db8d13 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1605,7 +1605,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>>  	int			irq;
>>  	u32			reg;
>>  
>> -	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
>> +	irq = dwc->gadget_irq;
>>  	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>>  			IRQF_SHARED, "dwc3", dwc->ev_buf);
>>  	if (ret) {
>> @@ -2781,6 +2781,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
>>  int dwc3_gadget_init(struct dwc3 *dwc)
>>  {
>>  	int					ret;
>> +	struct resource *res;
>> +	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
>> +
>> +	dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev, "peripheral");
>> +	if (dwc->gadget_irq <= 0) {
>> +		dwc->gadget_irq = platform_get_irq_byname(dwc3_pdev,
>> +							  "dwc_usb3");
>> +		if (dwc->gadget_irq <= 0) {
>> +			res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
>> +						    0);
>> +			if (!res) {
>> +				dev_err(dwc->dev, "missing peripheral IRQ\n");
>> +				return -ENODEV;
>> +			}
>> +			dwc->gadget_irq = res->start;
>> +		}
>> +	}
>>  
>>  	dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
>>  			&dwc->ctrl_req_addr, GFP_KERNEL);
> 
> you're regressing dwc3_gadget_stop().
> 
ah, good catch.

cheers,
-roger

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>  	/* check the DMA Status */
>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>  
>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>> -			"dwc3-omap", omap);
>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>> +					NULL, 0, "dwc3-omap", omap);
>> 
>> if you're using threaded_irq, it's better to have a NULL top half and
>> valid bottom half.
>
> But in this case we don't need a bottom half as there is nothing to do :).
>
>> 
>> In fact, since this will be shared, you could do a proper preparation
>> and on top half check if $this device generated the IRQ and
>> conditionally schedule the bottom half. Don't forget to mask device's
>> interrupts from top half so you can run without IRQF_ONESHOT.
>> 
>
> Why do this at all if there is nothing to do in the bottom half?

oh, but there is :-)

The whole idea of threaded IRQs is that you spend as little time as
possible on top half and the (strong) recommendation is that you *only*
check if $this device generated the interrupt. Note that "checking if
$this device generated the interrupt" will be mandatory as soon as you
mark the IRQ line as shared ;-)

So here's how this should look like:

static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
{
        struct dwc3_omap *omap = _omap;
        u32 reg;

        reg = readl(IRQSTATUS)
        if (reg) {
                mask_interrupts(omap);
        	return IRQ_WAKE_THREAD;
        }

	return IRQ_HANDLED;
}

static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
{
        struct dwc3_omap *omap = _omap;
        u32 reg;

        spin_lock(&omap->lock);
        reg = readl(IRQSTATUS);

        if (reg & BIT0)
        	handle_bit_0(omap);

	if (reg & BIT1)
        	handle_bit_1(omap);

	unmask_interrupts(omap);
	spin_unlock(&omap->lock);

	return IRQ_HANDLED;
}

this will *always* behave well with RT and non-RT kernels. It also
allows for the user to change priorities on these interrupt handlers if
necessary.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Roger Quadros]

On 10/05/16 13:05, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> On 10/05/16 12:55, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> Don't make any decisions regarding VBUS session based on ID
>>>> status. That is best left to the OTG core.
>>>>
>>>> Pass ID and VBUS events independent of each other so that OTG
>>>> core knows exactly what to do.
>>>>
>>>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
>>> board ?
>>>
>>
>> Yes. Any specific test case you would like me to test?
>> For now I'm just doing enumeration with g_zero.
> 
> IIRC OMAP5 uEVM didn't have separate VBUS and ID GPIOs. How are you
> handling that case ?
> 
It comes from the PMIC, extcon-palmas.c.

cheers,
-roger

Re: [PATCH v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Roger Quadros]

On 10/05/16 13:05, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
>> On 10/05/16 12:55, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
>>>> Don't make any decisions regarding VBUS session based on ID
>>>> status. That is best left to the OTG core.
>>>>
>>>> Pass ID and VBUS events independent of each other so that OTG
>>>> core knows exactly what to do.
>>>>
>>>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
>>>
>>> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
>>> board ?
>>>
>>
>> Yes. Any specific test case you would like me to test?
>> For now I'm just doing enumeration with g_zero.
> 
> IIRC OMAP5 uEVM didn't have separate VBUS and ID GPIOs. How are you
> handling that case ?
> 
It comes from the PMIC, extcon-palmas.c.

cheers,
-roger
--
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 v7 4/5] usb: dwc3: omap: Pass VBUS and ID events transparently

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>> Don't make any decisions regarding VBUS session based on ID
>>>>> status. That is best left to the OTG core.
>>>>>
>>>>> Pass ID and VBUS events independent of each other so that OTG
>>>>> core knows exactly what to do.
>>>>>
>>>>> This makes dual-role with extcon work with OTG irq on OMAP platforms.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>
>>>> I have a feeling this will regress OMAP5432 uEVM. Did you test with that
>>>> board ?
>>>>
>>>
>>> Yes. Any specific test case you would like me to test?
>>> For now I'm just doing enumeration with g_zero.
>> 
>> IIRC OMAP5 uEVM didn't have separate VBUS and ID GPIOs. How are you
>> handling that case ?
>> 
> It comes from the PMIC, extcon-palmas.c.

alright, then :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 186a886..2e20892 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>>   * @revision: revision register contents
>>>   * @dr_mode: requested mode of operation
>>> + * @gadget_irq: IRQ number for Peripheral IRQs
>>> + * @otg_irq: IRQ number for OTG IRQs
>>>   * @usb2_phy: pointer to USB2 PHY
>>>   * @usb3_phy: pointer to USB3 PHY
>>>   * @usb2_generic_phy: pointer to USB2 PHY
>>> @@ -817,6 +819,9 @@ struct dwc3 {
>>>  
>>>  	enum usb_dr_mode	dr_mode;
>>>  
>>> +	int			gadget_irq;
>>> +	int			otg_irq;
>> 
>> while at that, let's add host_irq too and do proper changes to dwc3/host.c
>
> Sure. So we add host_irq here, and manually create an irq resource
> in dwc3_host_init?

right :-) Then the code looks similar for otg, peripheral and host parts ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 10/05/16 13:12, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>>  	/* check the DMA Status */
>>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>  
>>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>> -			"dwc3-omap", omap);
>>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>> +					NULL, 0, "dwc3-omap", omap);
>>>
>>> if you're using threaded_irq, it's better to have a NULL top half and
>>> valid bottom half.
>>
>> But in this case we don't need a bottom half as there is nothing to do :).
>>
>>>
>>> In fact, since this will be shared, you could do a proper preparation
>>> and on top half check if $this device generated the IRQ and
>>> conditionally schedule the bottom half. Don't forget to mask device's
>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>
>>
>> Why do this at all if there is nothing to do in the bottom half?
> 
> oh, but there is :-)
> 
> The whole idea of threaded IRQs is that you spend as little time as
> possible on top half and the (strong) recommendation is that you *only*
> check if $this device generated the interrupt. Note that "checking if
> $this device generated the interrupt" will be mandatory as soon as you
> mark the IRQ line as shared ;-)
> 
> So here's how this should look like:
> 
> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         reg = readl(IRQSTATUS)
>         if (reg) {
>                 mask_interrupts(omap);
>         	return IRQ_WAKE_THREAD;
>         }
> 
> 	return IRQ_HANDLED;
> }
> 
> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         spin_lock(&omap->lock);
>         reg = readl(IRQSTATUS);
> 
>         if (reg & BIT0)
>         	handle_bit_0(omap);
> 
> 	if (reg & BIT1)
>         	handle_bit_1(omap);
> 
> 	unmask_interrupts(omap);
> 	spin_unlock(&omap->lock);
> 
> 	return IRQ_HANDLED;
> }
> 
> this will *always* behave well with RT and non-RT kernels. It also
> allows for the user to change priorities on these interrupt handlers if
> necessary.
> 

No problem, I can implement a bottom half. We are not handling anything
there at the moment so it is a bit of an overkill :)
It might help in the future if someone wants to handle something.

cheers,
-roge

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 10/05/16 13:12, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>>  	/* check the DMA Status */
>>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>  
>>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>> -			"dwc3-omap", omap);
>>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>> +					NULL, 0, "dwc3-omap", omap);
>>>
>>> if you're using threaded_irq, it's better to have a NULL top half and
>>> valid bottom half.
>>
>> But in this case we don't need a bottom half as there is nothing to do :).
>>
>>>
>>> In fact, since this will be shared, you could do a proper preparation
>>> and on top half check if $this device generated the IRQ and
>>> conditionally schedule the bottom half. Don't forget to mask device's
>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>
>>
>> Why do this at all if there is nothing to do in the bottom half?
> 
> oh, but there is :-)
> 
> The whole idea of threaded IRQs is that you spend as little time as
> possible on top half and the (strong) recommendation is that you *only*
> check if $this device generated the interrupt. Note that "checking if
> $this device generated the interrupt" will be mandatory as soon as you
> mark the IRQ line as shared ;-)
> 
> So here's how this should look like:
> 
> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         reg = readl(IRQSTATUS)
>         if (reg) {
>                 mask_interrupts(omap);
>         	return IRQ_WAKE_THREAD;
>         }
> 
> 	return IRQ_HANDLED;
> }
> 
> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         spin_lock(&omap->lock);
>         reg = readl(IRQSTATUS);
> 
>         if (reg & BIT0)
>         	handle_bit_0(omap);
> 
> 	if (reg & BIT1)
>         	handle_bit_1(omap);
> 
> 	unmask_interrupts(omap);
> 	spin_unlock(&omap->lock);
> 
> 	return IRQ_HANDLED;
> }
> 
> this will *always* behave well with RT and non-RT kernels. It also
> allows for the user to change priorities on these interrupt handlers if
> necessary.
> 

No problem, I can implement a bottom half. We are not handling anything
there at the moment so it is a bit of an overkill :)
It might help in the future if someone wants to handle something.

cheers,
-roge
--
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 v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

On 10/05/16 13:04, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> On 10/05/16 12:54, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> TRM [1] recommends that POWERPRESENT bit must not be
>>>> set and left at it's default value of 0.
>>>>
>>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>>
>>>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>>>> and because of the redundancy with the UTMI signals, the controller ORes
>>>> together the appropriate PIPE and UTMI inputs to create its internal
>>>> VBUS status. For that reason, it is recommended to leave field
>>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>>>> fill in the USB2 VBUS status fields in the same register."
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> to make sure we avoid regressions, do you mind sharing on which
>>> platforms you tested this patch ?
>>>
>> I tested this on omap5-uevm and dra7-evm.
>> My am437x board stopped working so couldn't test on that one.
> 
> would you have a colleague or perhaps an automated test-farm which could
> run the test for you ? :-)
> 
> I can take the patch, no problem, but if there are any regressions don't
> blame me :-)
> 
Don't worry, blame is on the TRM then :).

cheers,
-roger

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

On 10/05/16 13:04, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> On 10/05/16 12:54, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> TRM [1] recommends that POWERPRESENT bit must not be
>>>> set and left at it's default value of 0.
>>>>
>>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>>
>>>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>>>> and because of the redundancy with the UTMI signals, the controller ORes
>>>> together the appropriate PIPE and UTMI inputs to create its internal
>>>> VBUS status. For that reason, it is recommended to leave field
>>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>>>> fill in the USB2 VBUS status fields in the same register."
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> to make sure we avoid regressions, do you mind sharing on which
>>> platforms you tested this patch ?
>>>
>> I tested this on omap5-uevm and dra7-evm.
>> My am437x board stopped working so couldn't test on that one.
> 
> would you have a colleague or perhaps an automated test-farm which could
> run the test for you ? :-)
> 
> I can take the patch, no problem, but if there are any regressions don't
> blame me :-)
> 
Don't worry, blame is on the TRM then :).

cheers,
-roger

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

On 10/05/16 13:04, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> On 10/05/16 12:54, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> TRM [1] recommends that POWERPRESENT bit must not be
>>>> set and left at it's default value of 0.
>>>>
>>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>>
>>>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>>>> and because of the redundancy with the UTMI signals, the controller ORes
>>>> together the appropriate PIPE and UTMI inputs to create its internal
>>>> VBUS status. For that reason, it is recommended to leave field
>>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>>>> fill in the USB2 VBUS status fields in the same register."
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> to make sure we avoid regressions, do you mind sharing on which
>>> platforms you tested this patch ?
>>>
>> I tested this on omap5-uevm and dra7-evm.
>> My am437x board stopped working so couldn't test on that one.
> 
> would you have a colleague or perhaps an automated test-farm which could
> run the test for you ? :-)

Bin, do you mind testing this and giving your blessings please?

cheers,
-roger

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Roger Quadros]

On 10/05/16 13:04, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> On 10/05/16 12:54, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> TRM [1] recommends that POWERPRESENT bit must not be
>>>> set and left at it's default value of 0.
>>>>
>>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>>
>>>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>>>> and because of the redundancy with the UTMI signals, the controller ORes
>>>> together the appropriate PIPE and UTMI inputs to create its internal
>>>> VBUS status. For that reason, it is recommended to leave field
>>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>>>> fill in the USB2 VBUS status fields in the same register."
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> to make sure we avoid regressions, do you mind sharing on which
>>> platforms you tested this patch ?
>>>
>> I tested this on omap5-uevm and dra7-evm.
>> My am437x board stopped working so couldn't test on that one.
> 
> would you have a colleague or perhaps an automated test-farm which could
> run the test for you ? :-)

Bin, do you mind testing this and giving your blessings please?

cheers,
-roger

Re: [PATCH v7 3/5] usb: dwc3: omap: Don't set POWERPRESENT

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> On 10/05/16 13:04, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Roger Quadros <rogerq@ti.com> writes:
>>> On 10/05/16 12:54, Felipe Balbi wrote:
>>>>
>>>> Hi,
>>>>
>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>> TRM [1] recommends that POWERPRESENT bit must not be
>>>>> set and left at it's default value of 0.
>>>>>
>>>>> [1] OMAP542x TRM - http://www.ti.com/lit/pdf/swpu249
>>>>> Section 23.11.4.5.1 Mailbox VBUS/ID Management
>>>>>
>>>>> "Because PIPE powerpresent has a different meaning in host and in device mode,
>>>>> and because of the redundancy with the UTMI signals, the controller ORes
>>>>> together the appropriate PIPE and UTMI inputs to create its internal
>>>>> VBUS status. For that reason, it is recommended to leave field
>>>>> USBOTGSS_UTMI_OTG_STATUS[9] POWERPRESENT at its default value (=0), and only to
>>>>> fill in the USB2 VBUS status fields in the same register."
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>
>>>> to make sure we avoid regressions, do you mind sharing on which
>>>> platforms you tested this patch ?
>>>>
>>> I tested this on omap5-uevm and dra7-evm.
>>> My am437x board stopped working so couldn't test on that one.
>> 
>> would you have a colleague or perhaps an automated test-farm which could
>> run the test for you ? :-)
>> 
>> I can take the patch, no problem, but if there are any regressions don't
>> blame me :-)
>> 
> Don't worry, blame is on the TRM then :).

alright then, so seems like v8 will be queued. Just remember we're
pretty close to opening the merge window and I have already sent a pull
request to Greg. On the bright side, your patches will sit in linux-next
for quite some time :-) (well, until -rc1 is tagged)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros]

On 10/05/16 13:14, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 186a886..2e20892 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>>>   * @revision: revision register contents
>>>>   * @dr_mode: requested mode of operation
>>>> + * @gadget_irq: IRQ number for Peripheral IRQs
>>>> + * @otg_irq: IRQ number for OTG IRQs
>>>>   * @usb2_phy: pointer to USB2 PHY
>>>>   * @usb3_phy: pointer to USB3 PHY
>>>>   * @usb2_generic_phy: pointer to USB2 PHY
>>>> @@ -817,6 +819,9 @@ struct dwc3 {
>>>>  
>>>>  	enum usb_dr_mode	dr_mode;
>>>>  
>>>> +	int			gadget_irq;
>>>> +	int			otg_irq;
>>>
>>> while at that, let's add host_irq too and do proper changes to dwc3/host.c
>>
>> Sure. So we add host_irq here, and manually create an irq resource
>> in dwc3_host_init?
> 
> right :-) Then the code looks similar for otg, peripheral and host parts ;-)
> 
Just saw that host_irq is not used anywhere other than creating the XHCI platform
device. So I don't see why we need host_irq in struct dwc3.
It is obtained in dwc3_host_init() and consumed there itself.

cheers,
-roger

Re: [PATCH v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros]

On 10/05/16 13:14, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 186a886..2e20892 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>>>   * @revision: revision register contents
>>>>   * @dr_mode: requested mode of operation
>>>> + * @gadget_irq: IRQ number for Peripheral IRQs
>>>> + * @otg_irq: IRQ number for OTG IRQs
>>>>   * @usb2_phy: pointer to USB2 PHY
>>>>   * @usb3_phy: pointer to USB3 PHY
>>>>   * @usb2_generic_phy: pointer to USB2 PHY
>>>> @@ -817,6 +819,9 @@ struct dwc3 {
>>>>  
>>>>  	enum usb_dr_mode	dr_mode;
>>>>  
>>>> +	int			gadget_irq;
>>>> +	int			otg_irq;
>>>
>>> while at that, let's add host_irq too and do proper changes to dwc3/host.c
>>
>> Sure. So we add host_irq here, and manually create an irq resource
>> in dwc3_host_init?
> 
> right :-) Then the code looks similar for otg, peripheral and host parts ;-)
> 
Just saw that host_irq is not used anywhere other than creating the XHCI platform
device. So I don't see why we need host_irq in struct dwc3.
It is obtained in dwc3_host_init() and consumed there itself.

cheers,
-roger
--
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 v7 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:
>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>>> index 186a886..2e20892 100644
>>>>> --- a/drivers/usb/dwc3/core.h
>>>>> +++ b/drivers/usb/dwc3/core.h
>>>>> @@ -716,6 +716,8 @@ struct dwc3_scratchpad_array {
>>>>>   * @maximum_speed: maximum speed requested (mainly for testing purposes)
>>>>>   * @revision: revision register contents
>>>>>   * @dr_mode: requested mode of operation
>>>>> + * @gadget_irq: IRQ number for Peripheral IRQs
>>>>> + * @otg_irq: IRQ number for OTG IRQs
>>>>>   * @usb2_phy: pointer to USB2 PHY
>>>>>   * @usb3_phy: pointer to USB3 PHY
>>>>>   * @usb2_generic_phy: pointer to USB2 PHY
>>>>> @@ -817,6 +819,9 @@ struct dwc3 {
>>>>>  
>>>>>  	enum usb_dr_mode	dr_mode;
>>>>>  
>>>>> +	int			gadget_irq;
>>>>> +	int			otg_irq;
>>>>
>>>> while at that, let's add host_irq too and do proper changes to dwc3/host.c
>>>
>>> Sure. So we add host_irq here, and manually create an irq resource
>>> in dwc3_host_init?
>> 
>> right :-) Then the code looks similar for otg, peripheral and host parts ;-)
>> 
> Just saw that host_irq is not used anywhere other than creating the XHCI platform
> device. So I don't see why we need host_irq in struct dwc3.
> It is obtained in dwc3_host_init() and consumed there itself.

fair enough.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

Felipe,

On 10/05/16 13:12, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>>  	/* check the DMA Status */
>>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>  
>>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>> -			"dwc3-omap", omap);
>>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>> +					NULL, 0, "dwc3-omap", omap);
>>>
>>> if you're using threaded_irq, it's better to have a NULL top half and
>>> valid bottom half.
>>
>> But in this case we don't need a bottom half as there is nothing to do :).
>>
>>>
>>> In fact, since this will be shared, you could do a proper preparation
>>> and on top half check if $this device generated the IRQ and
>>> conditionally schedule the bottom half. Don't forget to mask device's
>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>
>>
>> Why do this at all if there is nothing to do in the bottom half?
> 
> oh, but there is :-)
> 
> The whole idea of threaded IRQs is that you spend as little time as
> possible on top half and the (strong) recommendation is that you *only*
> check if $this device generated the interrupt. Note that "checking if
> $this device generated the interrupt" will be mandatory as soon as you
> mark the IRQ line as shared ;-)
> 
> So here's how this should look like:
> 
> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         reg = readl(IRQSTATUS)
>         if (reg) {
>                 mask_interrupts(omap);
>         	return IRQ_WAKE_THREAD;
>         }
> 
> 	return IRQ_HANDLED;

This should be IRQ_NONE right?

> }
> 
> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         spin_lock(&omap->lock);

Do we really need a spin_lock for the dwc3-omap driver?
Currently we won't be doing anything other than just
clearing the irqstatus and re-enabling the interrupts.


>         reg = readl(IRQSTATUS);
> 
>         if (reg & BIT0)
>         	handle_bit_0(omap);
> 
> 	if (reg & BIT1)
>         	handle_bit_1(omap);
> 
> 	unmask_interrupts(omap);
> 	spin_unlock(&omap->lock);
> 
> 	return IRQ_HANDLED;
> }
> 
> this will *always* behave well with RT and non-RT kernels. It also
> allows for the user to change priorities on these interrupt handlers if
> necessary.
> 

--
cheers,
-roger

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

Felipe,

On 10/05/16 13:12, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> writes:
>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>>  	/* check the DMA Status */
>>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>  
>>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>> -			"dwc3-omap", omap);
>>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>> +					NULL, 0, "dwc3-omap", omap);
>>>
>>> if you're using threaded_irq, it's better to have a NULL top half and
>>> valid bottom half.
>>
>> But in this case we don't need a bottom half as there is nothing to do :).
>>
>>>
>>> In fact, since this will be shared, you could do a proper preparation
>>> and on top half check if $this device generated the IRQ and
>>> conditionally schedule the bottom half. Don't forget to mask device's
>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>
>>
>> Why do this at all if there is nothing to do in the bottom half?
> 
> oh, but there is :-)
> 
> The whole idea of threaded IRQs is that you spend as little time as
> possible on top half and the (strong) recommendation is that you *only*
> check if $this device generated the interrupt. Note that "checking if
> $this device generated the interrupt" will be mandatory as soon as you
> mark the IRQ line as shared ;-)
> 
> So here's how this should look like:
> 
> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         reg = readl(IRQSTATUS)
>         if (reg) {
>                 mask_interrupts(omap);
>         	return IRQ_WAKE_THREAD;
>         }
> 
> 	return IRQ_HANDLED;

This should be IRQ_NONE right?

> }
> 
> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
> {
>         struct dwc3_omap *omap = _omap;
>         u32 reg;
> 
>         spin_lock(&omap->lock);

Do we really need a spin_lock for the dwc3-omap driver?
Currently we won't be doing anything other than just
clearing the irqstatus and re-enabling the interrupts.


>         reg = readl(IRQSTATUS);
> 
>         if (reg & BIT0)
>         	handle_bit_0(omap);
> 
> 	if (reg & BIT1)
>         	handle_bit_1(omap);
> 
> 	unmask_interrupts(omap);
> 	spin_unlock(&omap->lock);
> 
> 	return IRQ_HANDLED;
> }
> 
> this will *always* behave well with RT and non-RT kernels. It also
> allows for the user to change priorities on these interrupt handlers if
> necessary.
> 

--
cheers,
-roger
--
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 v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:
>>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>>>  	/* check the DMA Status */
>>>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>>  
>>>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>>> -			"dwc3-omap", omap);
>>>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>>> +					NULL, 0, "dwc3-omap", omap);
>>>>
>>>> if you're using threaded_irq, it's better to have a NULL top half and
>>>> valid bottom half.
>>>
>>> But in this case we don't need a bottom half as there is nothing to do :).
>>>
>>>>
>>>> In fact, since this will be shared, you could do a proper preparation
>>>> and on top half check if $this device generated the IRQ and
>>>> conditionally schedule the bottom half. Don't forget to mask device's
>>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>>
>>>
>>> Why do this at all if there is nothing to do in the bottom half?
>> 
>> oh, but there is :-)
>> 
>> The whole idea of threaded IRQs is that you spend as little time as
>> possible on top half and the (strong) recommendation is that you *only*
>> check if $this device generated the interrupt. Note that "checking if
>> $this device generated the interrupt" will be mandatory as soon as you
>> mark the IRQ line as shared ;-)
>> 
>> So here's how this should look like:
>> 
>> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>> {
>>         struct dwc3_omap *omap = _omap;
>>         u32 reg;
>> 
>>         reg = readl(IRQSTATUS)
>>         if (reg) {
>>                 mask_interrupts(omap);
>>         	return IRQ_WAKE_THREAD;
>>         }
>> 
>> 	return IRQ_HANDLED;
>
> This should be IRQ_NONE right?

possibly, testing will say ;-)

>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>> {
>>         struct dwc3_omap *omap = _omap;
>>         u32 reg;
>> 
>>         spin_lock(&omap->lock);
>
> Do we really need a spin_lock for the dwc3-omap driver?
> Currently we won't be doing anything other than just
> clearing the irqstatus and re-enabling the interrupts.

well, if there's no possibility of races, then no. But only testing will
say for sure, I guess. I didn't really go through the entire thing just
to a write a quick little template :-p

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 11/05/16 12:47, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>>>>  	/* check the DMA Status */
>>>>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>>>  
>>>>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>>>> -			"dwc3-omap", omap);
>>>>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>>>> +					NULL, 0, "dwc3-omap", omap);
>>>>>
>>>>> if you're using threaded_irq, it's better to have a NULL top half and
>>>>> valid bottom half.
>>>>
>>>> But in this case we don't need a bottom half as there is nothing to do :).
>>>>
>>>>>
>>>>> In fact, since this will be shared, you could do a proper preparation
>>>>> and on top half check if $this device generated the IRQ and
>>>>> conditionally schedule the bottom half. Don't forget to mask device's
>>>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>>>
>>>>
>>>> Why do this at all if there is nothing to do in the bottom half?
>>>
>>> oh, but there is :-)
>>>
>>> The whole idea of threaded IRQs is that you spend as little time as
>>> possible on top half and the (strong) recommendation is that you *only*
>>> check if $this device generated the interrupt. Note that "checking if
>>> $this device generated the interrupt" will be mandatory as soon as you
>>> mark the IRQ line as shared ;-)
>>>
>>> So here's how this should look like:
>>>
>>> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>>> {
>>>         struct dwc3_omap *omap = _omap;
>>>         u32 reg;
>>>
>>>         reg = readl(IRQSTATUS)
>>>         if (reg) {
>>>                 mask_interrupts(omap);
>>>         	return IRQ_WAKE_THREAD;
>>>         }
>>>
>>> 	return IRQ_HANDLED;
>>
>> This should be IRQ_NONE right?
> 
> possibly, testing will say ;-)
> 
>>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>>> {
>>>         struct dwc3_omap *omap = _omap;
>>>         u32 reg;
>>>
>>>         spin_lock(&omap->lock);
>>
>> Do we really need a spin_lock for the dwc3-omap driver?
>> Currently we won't be doing anything other than just
>> clearing the irqstatus and re-enabling the interrupts.
> 
> well, if there's no possibility of races, then no. But only testing will
> say for sure, I guess. I didn't really go through the entire thing just
> to a write a quick little template :-p
> 
OK. Another hurdle I have is that how do I mask/unmask the interrupts?
I do not see any masking bits, only enable/disable bits.

I don't think we can use the enable/disable bits to mask as we'd loose
events while the event is disabled.

cheers,
-roger

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 11/05/16 12:47, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>> @@ -497,8 +503,8 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>>>>>>  	/* check the DMA Status */
>>>>>>  	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
>>>>>>  
>>>>>> -	ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>>>>>> -			"dwc3-omap", omap);
>>>>>> +	ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>>>>>> +					NULL, 0, "dwc3-omap", omap);
>>>>>
>>>>> if you're using threaded_irq, it's better to have a NULL top half and
>>>>> valid bottom half.
>>>>
>>>> But in this case we don't need a bottom half as there is nothing to do :).
>>>>
>>>>>
>>>>> In fact, since this will be shared, you could do a proper preparation
>>>>> and on top half check if $this device generated the IRQ and
>>>>> conditionally schedule the bottom half. Don't forget to mask device's
>>>>> interrupts from top half so you can run without IRQF_ONESHOT.
>>>>>
>>>>
>>>> Why do this at all if there is nothing to do in the bottom half?
>>>
>>> oh, but there is :-)
>>>
>>> The whole idea of threaded IRQs is that you spend as little time as
>>> possible on top half and the (strong) recommendation is that you *only*
>>> check if $this device generated the interrupt. Note that "checking if
>>> $this device generated the interrupt" will be mandatory as soon as you
>>> mark the IRQ line as shared ;-)
>>>
>>> So here's how this should look like:
>>>
>>> static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap)
>>> {
>>>         struct dwc3_omap *omap = _omap;
>>>         u32 reg;
>>>
>>>         reg = readl(IRQSTATUS)
>>>         if (reg) {
>>>                 mask_interrupts(omap);
>>>         	return IRQ_WAKE_THREAD;
>>>         }
>>>
>>> 	return IRQ_HANDLED;
>>
>> This should be IRQ_NONE right?
> 
> possibly, testing will say ;-)
> 
>>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>>> {
>>>         struct dwc3_omap *omap = _omap;
>>>         u32 reg;
>>>
>>>         spin_lock(&omap->lock);
>>
>> Do we really need a spin_lock for the dwc3-omap driver?
>> Currently we won't be doing anything other than just
>> clearing the irqstatus and re-enabling the interrupts.
> 
> well, if there's no possibility of races, then no. But only testing will
> say for sure, I guess. I didn't really go through the entire thing just
> to a write a quick little template :-p
> 
OK. Another hurdle I have is that how do I mask/unmask the interrupts?
I do not see any masking bits, only enable/disable bits.

I don't think we can use the enable/disable bits to mask as we'd loose
events while the event is disabled.

cheers,
-roger

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>>>> {
>>>>         struct dwc3_omap *omap = _omap;
>>>>         u32 reg;
>>>>
>>>>         spin_lock(&omap->lock);
>>>
>>> Do we really need a spin_lock for the dwc3-omap driver?
>>> Currently we won't be doing anything other than just
>>> clearing the irqstatus and re-enabling the interrupts.
>> 
>> well, if there's no possibility of races, then no. But only testing will
>> say for sure, I guess. I didn't really go through the entire thing just
>> to a write a quick little template :-p
>> 
> OK. Another hurdle I have is that how do I mask/unmask the interrupts?
> I do not see any masking bits, only enable/disable bits.
>
> I don't think we can use the enable/disable bits to mask as we'd loose
> events while the event is disabled.

I'm pretty sure your TRM discusses the usage of IRQSTATUS_RAW*
registers, doesn't it ? :-)

Those registers should be modified by HW even when interrupts are
disabled/masked. Note that, the IRQSTATUS_SET* and IRQSTATUS_CLR*
registers act more like mask/unmask than enable/disable considering we
can still read IRQ status using *RAW* registers.

See if below works fine for OMAP5, AM4 and AM5 SoCs:

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index af264493bbae..ece2f25ad2c3 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -165,20 +165,20 @@ static void dwc3_omap_write_utmi_ctrl(struct dwc3_omap *omap, u32 value)
 
 static u32 dwc3_omap_read_irq0_status(struct dwc3_omap *omap)
 {
-	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_0 -
+	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
 						omap->irq0_offset);
 }
 
 static void dwc3_omap_write_irq0_status(struct dwc3_omap *omap, u32 value)
 {
-	dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_0 -
+	dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
 						omap->irq0_offset, value);
 
 }
 
 static u32 dwc3_omap_read_irqmisc_status(struct dwc3_omap *omap)
 {
-	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_MISC +
+	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_MISC +
 						omap->irqmisc_offset);
 }
 
-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 11/05/16 15:39, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>>>>> {
>>>>>         struct dwc3_omap *omap = _omap;
>>>>>         u32 reg;
>>>>>
>>>>>         spin_lock(&omap->lock);
>>>>
>>>> Do we really need a spin_lock for the dwc3-omap driver?
>>>> Currently we won't be doing anything other than just
>>>> clearing the irqstatus and re-enabling the interrupts.
>>>
>>> well, if there's no possibility of races, then no. But only testing will
>>> say for sure, I guess. I didn't really go through the entire thing just
>>> to a write a quick little template :-p
>>>
>> OK. Another hurdle I have is that how do I mask/unmask the interrupts?
>> I do not see any masking bits, only enable/disable bits.
>>
>> I don't think we can use the enable/disable bits to mask as we'd loose
>> events while the event is disabled.
> 
> I'm pretty sure your TRM discusses the usage of IRQSTATUS_RAW*
> registers, doesn't it ? :-)

It does and also says that it is mostly for debug. But I don't mind using it
if it solves our problem :).

> 
> Those registers should be modified by HW even when interrupts are
> disabled/masked. Note that, the IRQSTATUS_SET* and IRQSTATUS_CLR*
> registers act more like mask/unmask than enable/disable considering we
> can still read IRQ status using *RAW* registers.
> 
> See if below works fine for OMAP5, AM4 and AM5 SoCs:
> 
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index af264493bbae..ece2f25ad2c3 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -165,20 +165,20 @@ static void dwc3_omap_write_utmi_ctrl(struct dwc3_omap *omap, u32 value)
>  
>  static u32 dwc3_omap_read_irq0_status(struct dwc3_omap *omap)
>  {
> -	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_0 -
> +	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
>  						omap->irq0_offset);
>  }
>  
>  static void dwc3_omap_write_irq0_status(struct dwc3_omap *omap, u32 value)
>  {
> -	dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_0 -
> +	dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
>  						omap->irq0_offset, value);

We shouldn't write to RAW here as it will trigger a software IRQ instead of
clearing the event.
>  
>  }
>  
>  static u32 dwc3_omap_read_irqmisc_status(struct dwc3_omap *omap)
>  {
> -	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_MISC +
> +	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_MISC +
>  						omap->irqmisc_offset);
>  }
>  
> 

The rest is fine. It seemed to work on omap5. I'll do more tests though.
Thanks for the hint :)

cheers,
-roger

Re: [PATCH v7 1/5] usb: dwc3: omap: use request_threaded_irq()

[Roger Quadros]

On 11/05/16 15:39, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>>> static irqreturn_t dwc3_omap_threaded_interrupt(int irq, void *_omap)
>>>>> {
>>>>>         struct dwc3_omap *omap = _omap;
>>>>>         u32 reg;
>>>>>
>>>>>         spin_lock(&omap->lock);
>>>>
>>>> Do we really need a spin_lock for the dwc3-omap driver?
>>>> Currently we won't be doing anything other than just
>>>> clearing the irqstatus and re-enabling the interrupts.
>>>
>>> well, if there's no possibility of races, then no. But only testing will
>>> say for sure, I guess. I didn't really go through the entire thing just
>>> to a write a quick little template :-p
>>>
>> OK. Another hurdle I have is that how do I mask/unmask the interrupts?
>> I do not see any masking bits, only enable/disable bits.
>>
>> I don't think we can use the enable/disable bits to mask as we'd loose
>> events while the event is disabled.
> 
> I'm pretty sure your TRM discusses the usage of IRQSTATUS_RAW*
> registers, doesn't it ? :-)

It does and also says that it is mostly for debug. But I don't mind using it
if it solves our problem :).

> 
> Those registers should be modified by HW even when interrupts are
> disabled/masked. Note that, the IRQSTATUS_SET* and IRQSTATUS_CLR*
> registers act more like mask/unmask than enable/disable considering we
> can still read IRQ status using *RAW* registers.
> 
> See if below works fine for OMAP5, AM4 and AM5 SoCs:
> 
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index af264493bbae..ece2f25ad2c3 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -165,20 +165,20 @@ static void dwc3_omap_write_utmi_ctrl(struct dwc3_omap *omap, u32 value)
>  
>  static u32 dwc3_omap_read_irq0_status(struct dwc3_omap *omap)
>  {
> -	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_0 -
> +	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
>  						omap->irq0_offset);
>  }
>  
>  static void dwc3_omap_write_irq0_status(struct dwc3_omap *omap, u32 value)
>  {
> -	dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_0 -
> +	dwc3_omap_writel(omap->base, USBOTGSS_IRQSTATUS_RAW_0 -
>  						omap->irq0_offset, value);

We shouldn't write to RAW here as it will trigger a software IRQ instead of
clearing the event.
>  
>  }
>  
>  static u32 dwc3_omap_read_irqmisc_status(struct dwc3_omap *omap)
>  {
> -	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_MISC +
> +	return dwc3_omap_readl(omap->base, USBOTGSS_IRQSTATUS_RAW_MISC +
>  						omap->irqmisc_offset);
>  }
>  
> 

The rest is fine. It seemed to work on omap5. I'll do more tests though.
Thanks for the hint :)

cheers,
-roger