[PATCH 0/5] Cleanup and fixes for dwc3-omap

[PATCH 0/5] Cleanup and fixes for dwc3-omap

[George Cherian]

The series does some refactoring on dwc3_probe()

Patch 1-3 - reduce the size of dwc3_probe()
Patch 4 - Fix the crash on dwc3_omap removal
Patch 5 - Addresses the issue of  xhci hang while resuming from system sleep.


George Cherian (5):
  usb: dwc3: dwc3-omap: Add dwc3_omap_map_offset function
  usb: dwc3: dwc3-omap: Add dwc3_omap_set_utmi_mode() function
  usb: dwc3: dwc3-omap: Add dwc3_omap_extcon_register function
  usb: dwc3: dwc3-omap: Fix the crash on module removal
  usb: dwc3: dwc3-omap: Disable/Enable core interrupts in Suspend/Resume

 drivers/usb/dwc3/dwc3-omap.c | 211 +++++++++++++++++++++++--------------------
 1 file changed, 111 insertions(+), 100 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] usb: dwc3: dwc3-omap: Add dwc3_omap_map_offset function

[George Cherian]

Calculate the wrapper register offsets in a seperate function.
Improve code readability, decrease the dwc3_probe() size.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 80 ++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 1160ff4..872f065 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -383,6 +383,49 @@ static int dwc3_omap_vbus_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static void dwc3_omap_map_offset(struct dwc3_omap *omap)
+{
+	u32			reg;
+	struct device_node	*node = omap->dev->of_node;
+	int			x_major;
+
+	reg = dwc3_omap_readl(omap->base, USBOTGSS_REVISION);
+	omap->revision = reg;
+	x_major = USBOTGSS_REVISION_XMAJOR(reg);
+
+	/* Differentiate between OMAP5 and AM437x */
+	switch (x_major) {
+	case USBOTGSS_REVISION_XMAJOR1:
+	case USBOTGSS_REVISION_XMAJOR2:
+		omap->irq_eoi_offset = 0;
+		omap->irq0_offset = 0;
+		omap->irqmisc_offset = 0;
+		omap->utmi_otg_offset = 0;
+		omap->debug_offset = 0;
+		break;
+	default:
+		/* Default to the latest revision */
+		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
+		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
+		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
+		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
+		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
+		break;
+	}
+
+	/* For OMAP5(ES2.0) and AM437x x_major is 2 even though there are
+	 * changes in wrapper registers, Using dt compatible for aegis
+	 */
+
+	if (of_device_is_compatible(node, "ti,am437x-dwc3")) {
+		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
+		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
+		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
+		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
+		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
+	}
+}
+
 static int dwc3_omap_probe(struct platform_device *pdev)
 {
 	struct device_node	*node = pdev->dev.of_node;
@@ -397,7 +440,6 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	int			irq;
 
 	int			utmi_mode = 0;
-	int			x_major;
 
 	u32			reg;
 
@@ -448,41 +490,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 		goto err0;
 	}
 
-	reg = dwc3_omap_readl(omap->base, USBOTGSS_REVISION);
-	omap->revision = reg;
-	x_major = USBOTGSS_REVISION_XMAJOR(reg);
-
-	/* Differentiate between OMAP5 and AM437x */
-	switch (x_major) {
-	case USBOTGSS_REVISION_XMAJOR1:
-	case USBOTGSS_REVISION_XMAJOR2:
-		omap->irq_eoi_offset = 0;
-		omap->irq0_offset = 0;
-		omap->irqmisc_offset = 0;
-		omap->utmi_otg_offset = 0;
-		omap->debug_offset = 0;
-		break;
-	default:
-		/* Default to the latest revision */
-		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
-		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
-		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
-		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
-		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
-		break;
-	}
-
-	/* For OMAP5(ES2.0) and AM437x x_major is 2 even though there are
-	 * changes in wrapper registers, Using dt compatible for aegis
-	 */
-
-	if (of_device_is_compatible(node, "ti,am437x-dwc3")) {
-		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
-		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
-		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
-		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
-		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
-	}
+	dwc3_omap_map_offset(omap);
 
 	reg = dwc3_omap_read_utmi_status(omap);
 
-- 
1.8.3.1

[PATCH 2/5] usb: dwc3: dwc3-omap: Add dwc3_omap_set_utmi_mode() function

[George Cherian]

Move find and set the utmi mode to its own seperate function.
Improve code readability, decrease the dwc3_probe() size.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 872f065..b739a24 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -426,6 +426,30 @@ static void dwc3_omap_map_offset(struct dwc3_omap *omap)
 	}
 }
 
+static void dwc3_omap_set_utmi_mode(struct dwc3_omap *omap)
+{
+	u32			reg;
+	struct device_node	*node = omap->dev->of_node;
+	int			utmi_mode = 0;
+
+	reg = dwc3_omap_read_utmi_status(omap);
+
+	of_property_read_u32(node, "utmi-mode", &utmi_mode);
+
+	switch (utmi_mode) {
+	case DWC3_OMAP_UTMI_MODE_SW:
+		reg |= USBOTGSS_UTMI_OTG_STATUS_SW_MODE;
+		break;
+	case DWC3_OMAP_UTMI_MODE_HW:
+		reg &= ~USBOTGSS_UTMI_OTG_STATUS_SW_MODE;
+		break;
+	default:
+		dev_dbg(omap->dev, "UNKNOWN utmi mode %d\n", utmi_mode);
+	}
+
+	dwc3_omap_write_utmi_status(omap, reg);
+}
+
 static int dwc3_omap_probe(struct platform_device *pdev)
 {
 	struct device_node	*node = pdev->dev.of_node;
@@ -439,8 +463,6 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	int			ret = -ENOMEM;
 	int			irq;
 
-	int			utmi_mode = 0;
-
 	u32			reg;
 
 	void __iomem		*base;
@@ -491,23 +513,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	}
 
 	dwc3_omap_map_offset(omap);
-
-	reg = dwc3_omap_read_utmi_status(omap);
-
-	of_property_read_u32(node, "utmi-mode", &utmi_mode);
-
-	switch (utmi_mode) {
-	case DWC3_OMAP_UTMI_MODE_SW:
-		reg |= USBOTGSS_UTMI_OTG_STATUS_SW_MODE;
-		break;
-	case DWC3_OMAP_UTMI_MODE_HW:
-		reg &= ~USBOTGSS_UTMI_OTG_STATUS_SW_MODE;
-		break;
-	default:
-		dev_dbg(dev, "UNKNOWN utmi mode %d\n", utmi_mode);
-	}
-
-	dwc3_omap_write_utmi_status(omap, reg);
+	dwc3_omap_set_utmi_mode(omap);
 
 	/* check the DMA Status */
 	reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
-- 
1.8.3.1

[PATCH 3/5] usb: dwc3: dwc3-omap: Add dwc3_omap_extcon_register function

[George Cherian]

Move the extcon related code to its own function.
Improve code readability, decrease the dwc3_probe() size.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 65 ++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index b739a24..0b9b1d8 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -450,6 +450,42 @@ static void dwc3_omap_set_utmi_mode(struct dwc3_omap *omap)
 	dwc3_omap_write_utmi_status(omap, reg);
 }
 
+static int dwc3_omap_extcon_register(struct dwc3_omap *omap)
+{
+	u32			ret;
+	struct device_node	*node = omap->dev->of_node;
+	struct extcon_dev	*edev;
+
+	if (of_property_read_bool(node, "extcon")) {
+		edev = extcon_get_edev_by_phandle(omap->dev, 0);
+		if (IS_ERR(edev)) {
+			dev_vdbg(omap->dev, "couldn't get extcon device\n");
+			return -EPROBE_DEFER;
+		}
+
+		omap->vbus_nb.notifier_call = dwc3_omap_vbus_notifier;
+		ret = extcon_register_interest(&omap->extcon_vbus_dev,
+					       edev->name, "USB",
+					       &omap->vbus_nb);
+		if (ret < 0)
+			dev_vdbg(omap->dev, "failed to register notifier for USB\n");
+
+		omap->id_nb.notifier_call = dwc3_omap_id_notifier;
+		ret = extcon_register_interest(&omap->extcon_id_dev,
+					       edev->name, "USB-HOST",
+					       &omap->id_nb);
+		if (ret < 0)
+			dev_vdbg(omap->dev, "failed to register notifier for USB-HOST\n");
+
+		if (extcon_get_cable_state(edev, "USB") == true)
+			dwc3_omap_set_mailbox(omap, OMAP_DWC3_VBUS_VALID);
+		if (extcon_get_cable_state(edev, "USB-HOST") == true)
+			dwc3_omap_set_mailbox(omap, OMAP_DWC3_ID_GROUND);
+	}
+
+	return 0;
+}
+
 static int dwc3_omap_probe(struct platform_device *pdev)
 {
 	struct device_node	*node = pdev->dev.of_node;
@@ -457,7 +493,6 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	struct dwc3_omap	*omap;
 	struct resource		*res;
 	struct device		*dev = &pdev->dev;
-	struct extcon_dev	*edev;
 	struct regulator	*vbus_reg = NULL;
 
 	int			ret = -ENOMEM;
@@ -529,31 +564,9 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 
 	dwc3_omap_enable_irqs(omap);
 
-	if (of_property_read_bool(node, "extcon")) {
-		edev = extcon_get_edev_by_phandle(dev, 0);
-		if (IS_ERR(edev)) {
-			dev_vdbg(dev, "couldn't get extcon device\n");
-			ret = -EPROBE_DEFER;
-			goto err2;
-		}
-
-		omap->vbus_nb.notifier_call = dwc3_omap_vbus_notifier;
-		ret = extcon_register_interest(&omap->extcon_vbus_dev,
-			edev->name, "USB", &omap->vbus_nb);
-		if (ret < 0)
-			dev_vdbg(dev, "failed to register notifier for USB\n");
-		omap->id_nb.notifier_call = dwc3_omap_id_notifier;
-		ret = extcon_register_interest(&omap->extcon_id_dev, edev->name,
-					 "USB-HOST", &omap->id_nb);
-		if (ret < 0)
-			dev_vdbg(dev,
-				"failed to register notifier for USB-HOST\n");
-
-		if (extcon_get_cable_state(edev, "USB") == true)
-			dwc3_omap_set_mailbox(omap, OMAP_DWC3_VBUS_VALID);
-		if (extcon_get_cable_state(edev, "USB-HOST") == true)
-			dwc3_omap_set_mailbox(omap, OMAP_DWC3_ID_GROUND);
-	}
+	ret = dwc3_omap_extcon_register(omap);
+	if (ret < 0)
+		goto err2;
 
 	ret = of_platform_populate(node, NULL, NULL, dev);
 	if (ret) {
-- 
1.8.3.1

[PATCH 4/5] usb: dwc3: dwc3-omap: Fix the crash on module removal

[George Cherian]

Following crash is seen on dwc3_omap removal
Unable to handle kernel NULL pointer dereference at virtual address 00000018
pgd = ec098000
[00000018] *pgd=ad1f9831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] SMP ARM
Modules linked in: usb_f_ss_lb g_zero usb_f_acm u_serial usb_f_ecm u_ether libcomposite configfs snd_usb_audio snd_usbmidi_lib snd_rawmidi snd_hwdep snd_soc_omap snd_pcm_dmaengine snd_soc_core snd_compress snd_pcm snd_tim]
CPU: 0 PID: 1296 Comm: rmmod Tainted: G        W     3.15.0-rc4-02716-g95c4e18-dirty #10
task: ed05a080 ti: ec368000 task.ti: ec368000
PC is at release_resource+0x14/0x7c
LR is at release_resource+0x10/0x7c
pc : [<c0044724>]    lr : [<c0044720>]    psr: 60000013
sp : ec369ec0  ip : 60000013  fp : 00021008
r10: 00000000  r9 : ec368000  r8 : c000e7a4
r7 : 00000081  r6 : bf0062c0  r5 : ed7cd000  r4 : ed7d85c0
r3 : 00000000  r2 : 00000000  r1 : 00000011  r0 : c086d08c
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 10c5387d  Table: ac098059  DAC: 00000015
Process rmmod (pid: 1296, stack limit = 0xec368248)
Stack: (0xec369ec0 to 0xec36a000)
9ec0: 00000000 00000001 ed7cd000 c034de94 ed7cd010 ed7cd000 00000000 c034e194
9ee0: 00000000 bf0062cc ed7cd010 c03490b0 ed154cc0 ed4c2570 ed2b8410 ed156810
ed156810 bf006d24 c034db9c c034db84 c034c518
9f20: bf006d24 ed156810 bf006d24 c034cd2c bf006d24 bf006d68 00000800 c034c340
9f40: 00000000 c00a9e5c 00000020 00000000 bf006d68 00000800 ec369f4c 33637764
9f60: 616d6f5f 00000070 00000001 ec368000 ed05a080 c000e670 00000001 c0084010
9f80: 00021088 00000800 00021088 00000081 80000010 0000e6f4 00021088 00000800
9fa0: 00021088 c000e5e0 00021088 00000800 000210b8 00000800 e04f6d00 e04f6d00
9fc0: 00021088 00000800 00021088 00000081 00000001 00000000 be91de08 00021008
9fe0: 4d768880 be91dbb4 b6fc5984 4d76888c 80000010 000210b8 00000000 00000000
[<c0044724>] (release_resource) from [<c034de94>] (platform_device_del+0x6c/0x9c)
[<c034de94>] (platform_device_del) from [<c034e194>] (platform_device_unregister+0xc/0x18)
[<c034e194>] (platform_device_unregister) from [<bf0062cc>] (dwc3_omap_remove_core+0xc/0x14 [dwc3_omap])
[<bf0062cc>] (dwc3_omap_remove_core [dwc3_omap]) from [<c03490b0>] (device_for_each_child+0x34/0x74)
[<c03490b0>] (device_for_each_child) from [<bf0062b4>] (dwc3_omap_remove+0x6c/0x78 [dwc3_omap])
[<bf0062b4>] (dwc3_omap_remove [dwc3_omap]) from [<c034db9c>] (platform_drv_remove+0x18/0x1c)
[<c034db9c>] (platform_drv_remove) from [<c034c518>] (__device_release_driver+0x70/0xc8)
[<c034c518>] (__device_release_driver) from [<c034cd2c>] (driver_detach+0xb4/0xb8)
[<c034cd2c>] (driver_detach) from [<c034c340>] (bus_remove_driver+0x4c/0x90)
[<c034c340>] (bus_remove_driver) from [<c00a9e5c>] (SyS_delete_module+0x10c/0x198)
[<c00a9e5c>] (SyS_delete_module) from [<c000e5e0>] (ret_fast_syscall+0x0/0x48)
Code: e1a04000 e59f0068 eb14505e e5943010 (e5932018)
---[ end trace 7e2a8746ff4fc811 ]---
Segmentation fault

Signed-off-by: George Cherian <george.cherian@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 0b9b1d8..82b20d8f 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -322,7 +322,7 @@ static int dwc3_omap_remove_core(struct device *dev, void *c)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 
-	platform_device_unregister(pdev);
+	of_device_unregister(pdev);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 5/5] usb: dwc3: dwc3-omap: Disable/Enable core interrupts in Suspend/Resume

[George Cherian]

Enabling the core interrupts in complete is too late for XHCI, and stops
xhci from proper operation. So remove prepare and complete and disable/enable
interrupts in suspend/resume

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 82b20d8f..0916c4b 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -622,27 +622,12 @@ static const struct of_device_id of_dwc3_match[] = {
 MODULE_DEVICE_TABLE(of, of_dwc3_match);
 
 #ifdef CONFIG_PM_SLEEP
-static int dwc3_omap_prepare(struct device *dev)
-{
-	struct dwc3_omap	*omap = dev_get_drvdata(dev);
-
-	dwc3_omap_disable_irqs(omap);
-
-	return 0;
-}
-
-static void dwc3_omap_complete(struct device *dev)
-{
-	struct dwc3_omap	*omap = dev_get_drvdata(dev);
-
-	dwc3_omap_enable_irqs(omap);
-}
-
 static int dwc3_omap_suspend(struct device *dev)
 {
 	struct dwc3_omap	*omap = dev_get_drvdata(dev);
 
 	omap->utmi_otg_status = dwc3_omap_read_utmi_status(omap);
+	dwc3_omap_disable_irqs(omap);
 
 	return 0;
 }
@@ -652,6 +637,7 @@ static int dwc3_omap_resume(struct device *dev)
 	struct dwc3_omap	*omap = dev_get_drvdata(dev);
 
 	dwc3_omap_write_utmi_status(omap, omap->utmi_otg_status);
+	dwc3_omap_enable_irqs(omap);
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_active(dev);
@@ -661,8 +647,6 @@ static int dwc3_omap_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dwc3_omap_dev_pm_ops = {
-	.prepare	= dwc3_omap_prepare,
-	.complete	= dwc3_omap_complete,
 
 	SET_SYSTEM_SLEEP_PM_OPS(dwc3_omap_suspend, dwc3_omap_resume)
 };
-- 
1.8.3.1

Re: [PATCH 5/5] usb: dwc3: dwc3-omap: Disable/Enable core interrupts in Suspend/Resume

[Felipe Balbi]

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

Hi,

On Thu, May 08, 2014 at 03:03:07PM +0530, George Cherian wrote:
> Enabling the core interrupts in complete is too late for XHCI, and stops
> xhci from proper operation. So remove prepare and complete and disable/enable

isn't this a bug in xhci ? I mean the driver should make no assumption
as to when IRQs are enabled, why do we need to enable IRQs earlier when
the device is only considered "ready for use" after ->complete()
finishes executing ?

From documentation we have:

107  * @complete: Undo the changes made by @prepare().  This method is executed for
108  *      all kinds of resume transitions, following one of the resume callbacks:
109  *      @resume(), @thaw(), @restore().  Also called if the state transition
110  *      fails before the driver's suspend callback: @suspend(), @freeze() or
111  *      @poweroff(), can be executed (e.g. if the suspend callback fails for one
112  *      of the other devices that the PM core has unsuccessfully attempted to
113  *      suspend earlier).
114  *      The PM core executes subsystem-level @complete() after it has executed
115  *      the appropriate resume callbacks for all devices.

which tells me that using ->complete() to reenable IRQs is ok here.
Specially when you consider that the role of ->prepare() is to prevent
new children from being created and, for a USB host, that means we
should prevent hub port changes.

cheers

-- 
balbi

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

Re: [PATCH 1/5] usb: dwc3: dwc3-omap: Add dwc3_omap_map_offset function

[Felipe Balbi]

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

Hi,

On Thu, May 08, 2014 at 03:03:03PM +0530, George Cherian wrote:
> Calculate the wrapper register offsets in a seperate function.
> Improve code readability, decrease the dwc3_probe() size.
> 
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>  drivers/usb/dwc3/dwc3-omap.c | 80 ++++++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index 1160ff4..872f065 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -383,6 +383,49 @@ static int dwc3_omap_vbus_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> +static void dwc3_omap_map_offset(struct dwc3_omap *omap)
> +{
> +	u32			reg;
> +	struct device_node	*node = omap->dev->of_node;
> +	int			x_major;
> +
> +	reg = dwc3_omap_readl(omap->base, USBOTGSS_REVISION);
> +	omap->revision = reg;
> +	x_major = USBOTGSS_REVISION_XMAJOR(reg);
> +
> +	/* Differentiate between OMAP5 and AM437x */
> +	switch (x_major) {
> +	case USBOTGSS_REVISION_XMAJOR1:
> +	case USBOTGSS_REVISION_XMAJOR2:
> +		omap->irq_eoi_offset = 0;
> +		omap->irq0_offset = 0;
> +		omap->irqmisc_offset = 0;
> +		omap->utmi_otg_offset = 0;
> +		omap->debug_offset = 0;
> +		break;
> +	default:
> +		/* Default to the latest revision */
> +		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
> +		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
> +		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
> +		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
> +		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
> +		break;
> +	}
> +
> +	/* For OMAP5(ES2.0) and AM437x x_major is 2 even though there are
> +	 * changes in wrapper registers, Using dt compatible for aegis
> +	 */
> +
> +	if (of_device_is_compatible(node, "ti,am437x-dwc3")) {
> +		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
> +		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
> +		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
> +		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
> +		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
> +	}

can you add a patch before $subject which gets rid of the switch
statement above since it's pretty much useless now that we use
compatible strings to differentiate omap5 and am437x ?

-- 
balbi

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

Re: [PATCH 1/5] usb: dwc3: dwc3-omap: Add dwc3_omap_map_offset function

[George Cherian]

On 5/13/2014 9:32 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, May 08, 2014 at 03:03:03PM +0530, George Cherian wrote:
>> Calculate the wrapper register offsets in a seperate function.
>> Improve code readability, decrease the dwc3_probe() size.
>>
>> Signed-off-by: George Cherian <george.cherian@ti.com>
>> ---
>>   drivers/usb/dwc3/dwc3-omap.c | 80 ++++++++++++++++++++++++--------------------
>>   1 file changed, 44 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
>> index 1160ff4..872f065 100644
>> --- a/drivers/usb/dwc3/dwc3-omap.c
>> +++ b/drivers/usb/dwc3/dwc3-omap.c
>> @@ -383,6 +383,49 @@ static int dwc3_omap_vbus_notifier(struct notifier_block *nb,
>>   	return NOTIFY_DONE;
>>   }
>>   
>> +static void dwc3_omap_map_offset(struct dwc3_omap *omap)
>> +{
>> +	u32			reg;
>> +	struct device_node	*node = omap->dev->of_node;
>> +	int			x_major;
>> +
>> +	reg = dwc3_omap_readl(omap->base, USBOTGSS_REVISION);
>> +	omap->revision = reg;
>> +	x_major = USBOTGSS_REVISION_XMAJOR(reg);
>> +
>> +	/* Differentiate between OMAP5 and AM437x */
>> +	switch (x_major) {
>> +	case USBOTGSS_REVISION_XMAJOR1:
>> +	case USBOTGSS_REVISION_XMAJOR2:
>> +		omap->irq_eoi_offset = 0;
>> +		omap->irq0_offset = 0;
>> +		omap->irqmisc_offset = 0;
>> +		omap->utmi_otg_offset = 0;
>> +		omap->debug_offset = 0;
>> +		break;
>> +	default:
>> +		/* Default to the latest revision */
>> +		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
>> +		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
>> +		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
>> +		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
>> +		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
>> +		break;
>> +	}
>> +
>> +	/* For OMAP5(ES2.0) and AM437x x_major is 2 even though there are
>> +	 * changes in wrapper registers, Using dt compatible for aegis
>> +	 */
>> +
>> +	if (of_device_is_compatible(node, "ti,am437x-dwc3")) {
>> +		omap->irq_eoi_offset = USBOTGSS_EOI_OFFSET;
>> +		omap->irq0_offset = USBOTGSS_IRQ0_OFFSET;
>> +		omap->irqmisc_offset = USBOTGSS_IRQMISC_OFFSET;
>> +		omap->utmi_otg_offset = USBOTGSS_UTMI_OTG_OFFSET;
>> +		omap->debug_offset = USBOTGSS_DEBUG_OFFSET;
>> +	}
> can you add a patch before $subject which gets rid of the switch
> statement above since it's pretty much useless now that we use
> compatible strings to differentiate omap5 and am437x ?'
okay will do in v2.
>


-- 
-George

Re: [PATCH 5/5] usb: dwc3: dwc3-omap: Disable/Enable core interrupts in Suspend/Resume

[George Cherian]

On 5/13/2014 9:20 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, May 08, 2014 at 03:03:07PM +0530, George Cherian wrote:
>> Enabling the core interrupts in complete is too late for XHCI, and stops
>> xhci from proper operation. So remove prepare and complete and disable/enable
> isn't this a bug in xhci ? I mean the driver should make no assumption
> as to when IRQs are enabled, why do we need to enable IRQs earlier when
> the device is only considered "ready for use" after ->complete()
> finishes executing ?
I dont think its a bug in xhci. In case of xhci-pci driver it actually 
does an
hcd->driver->pci_suspend (xhci_suspend) followed by synchronize_irq()
and the does a pci_disable_device().  In resume path it calls 
pci_enable_device()
followed by hcd->driver->pci_resume(xhci_resume).

In case of dwc3-omap we do have a wrapper register which can still 
disable the XHCI IRQs
even though the xhci driver enables the interrupts internally.

Now dwc3-omap wrapper driver should not actually fiddle with the core 
Interrupt
enable/disable except in probe/remove.

>  From documentation we have:
>
> 107  * @complete: Undo the changes made by @prepare().  This method is executed for
> 108  *      all kinds of resume transitions, following one of the resume callbacks:
> 109  *      @resume(), @thaw(), @restore().  Also called if the state transition
> 110  *      fails before the driver's suspend callback: @suspend(), @freeze() or
> 111  *      @poweroff(), can be executed (e.g. if the suspend callback fails for one
> 112  *      of the other devices that the PM core has unsuccessfully attempted to
> 113  *      suspend earlier).
> 114  *      The PM core executes subsystem-level @complete() after it has executed
> 115  *      the appropriate resume callbacks for all devices.
>
> which tells me that using ->complete() to reenable IRQs is ok here.
> Specially when you consider that the role of ->prepare() is to prevent
> new children from being created and, for a USB host, that means we
> should prevent hub port changes.
Probably the patch should have been to still keep the complete/prepare 
in place
but not disable the core interrupts, rather enable/disable only the 
wrapper interrupt.
> cheers
>


-- 
-George