[PATCH 1/9] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Marek Szyprowski]

This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt to correctly notify gadget subsystem about unplugged usb
cable.

Signed-off-by: Marek Szyprowski <m.szyprowski-***@public.gmane.org>
---
drivers/usb/dwc2/gadget.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856fadd93..119c8a3effc2 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2279,6 +2279,12 @@ irq_retry:
dev_info(hsotg->dev, "OTGInt: %08x\n", otgint);

writel(otgint, hsotg->regs + GOTGINT);
+
+ if (otgint & GOTGINT_SES_END_DET) {
+ if (hsotg->gadget.speed != USB_SPEED_UNKNOWN)
+ s3c_hsotg_disconnect(hsotg);
+ hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+ }
}

if (gintsts & GINTSTS_SESSREQINT) {

[Marek Szyprowski]

This patch moves PHY enable and disable calls from pullup method to
udc_start/stop functions and adds calls to recently introduces soft
disconnect mode in pullup method. This improves dwc2 gadget driver
compatibility with gadget API requirements (now pullup method really
forces soft disconnect mode instead of shutting down the whole hardware)
and as a side effect also solves the issue related to limited caller
context for PHY related functions (they cannot be called from
non-sleeping context).

Signed-off-by: Marek Szyprowski <***@samsung.com>
---
drivers/usb/dwc2/gadget.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index d039334967d7..cdf417a7ae63 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2883,6 +2883,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)
{
struct s3c_hsotg *hsotg = to_hsotg(gadget);
+ unsigned long flags;
int ret;

if (!hsotg) {
@@ -2919,7 +2920,15 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
goto err;
}

+ s3c_hsotg_phy_enable(hsotg);
+
+ spin_lock_irqsave(&hsotg->lock, flags);
+ s3c_hsotg_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+
dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
+
return 0;

err:
@@ -2957,6 +2966,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,

spin_unlock_irqrestore(&hsotg->lock, flags);

+ s3c_hsotg_phy_disable(hsotg);
+
regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);

clk_disable(hsotg->clk);
@@ -2990,14 +3001,13 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);

spin_lock_irqsave(&hsotg->lock, flags);
+
if (is_on) {
- s3c_hsotg_phy_enable(hsotg);
clk_enable(hsotg->clk);
- s3c_hsotg_core_init_disconnected(hsotg);
s3c_hsotg_core_connect(hsotg);
} else {
+ s3c_hsotg_core_disconnect(hsotg);
clk_disable(hsotg->clk);
- s3c_hsotg_phy_disable(hsotg);
}

hsotg->gadget.speed = USB_SPEED_UNKNOWN;

[Felipe Balbi]

Hi,

your subject line is wrong. ->pullup() is already implemented, you're
moving unnecessary code from ->pullup() to other places.

Post by Marek Szyprowski
This patch moves PHY enable and disable calls from pullup method to
udc_start/stop functions and adds calls to recently introduces soft
disconnect mode in pullup method. This improves dwc2 gadget driver
compatibility with gadget API requirements (now pullup method really
forces soft disconnect mode instead of shutting down the whole hardware)
and as a side effect also solves the issue related to limited caller
context for PHY related functions (they cannot be called from
non-sleeping context).

you're doing two things in one patch. See below

Post by Marek Szyprowski
---
drivers/usb/dwc2/gadget.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index d039334967d7..cdf417a7ae63 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2883,6 +2883,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)
{
struct s3c_hsotg *hsotg = to_hsotg(gadget);
+ unsigned long flags;
int ret;
if (!hsotg) {
@@ -2919,7 +2920,15 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
goto err;
}
+ s3c_hsotg_phy_enable(hsotg);

you moved phy_enable to udc_start/udc_stop (one patch)

Post by Marek Szyprowski
+
+ spin_lock_irqsave(&hsotg->lock, flags);
+ s3c_hsotg_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);

you moved core init here (another patch).

Post by Marek Szyprowski
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+
dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
+
return 0;
@@ -2957,6 +2966,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
spin_unlock_irqrestore(&hsotg->lock, flags);
+ s3c_hsotg_phy_disable(hsotg);
+
regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
clk_disable(hsotg->clk);
@@ -2990,14 +3001,13 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on);
spin_lock_irqsave(&hsotg->lock, flags);
+
if (is_on) {
- s3c_hsotg_phy_enable(hsotg);
clk_enable(hsotg->clk);
- s3c_hsotg_core_init_disconnected(hsotg);
s3c_hsotg_core_connect(hsotg);
} else {
+ s3c_hsotg_core_disconnect(hsotg);
clk_disable(hsotg->clk);
- s3c_hsotg_phy_disable(hsotg);
}
hsotg->gadget.speed = USB_SPEED_UNKNOWN;
--
1.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Marek Szyprowski]

Suspend/resume code assumed that the gadget was always enabled and
connected to usb bus. This means that the actual state of the gadget
(soft-enabled/disabled or connected/disconnected) was not correctly
preserved on suspend/resume cycle. This patch fixes this issue.

Signed-off-by: Marek Szyprowski <m.szyprowski-***@public.gmane.org>
---
drivers/usb/dwc2/core.h | 4 +++-
drivers/usb/dwc2/gadget.c | 42 ++++++++++++++++++++++++++----------------
2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index bf015ab3b44c..3648b76a18b4 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -210,7 +210,9 @@ struct s3c_hsotg {
u8 ctrl_buff[8];

struct usb_gadget gadget;
- unsigned int setup;
+ unsigned int setup:1;
+ unsigned int connected:1;
+ unsigned int enabled:1;
unsigned long last_rst;
struct s3c_hsotg_ep *eps;
};
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 052b1a857291..83fe123ede05 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2925,6 +2925,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
spin_lock_irqsave(&hsotg->lock, flags);
s3c_hsotg_init(hsotg);
s3c_hsotg_core_init_disconnected(hsotg);
+ hsotg->enabled = 1;
+ hsotg->connected = 0;
spin_unlock_irqrestore(&hsotg->lock, flags);

dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
@@ -2963,6 +2965,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
hsotg->driver = NULL;

hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+ hsotg->enabled = 0;
+ hsotg->connected = 0;

spin_unlock_irqrestore(&hsotg->lock, flags);

@@ -3004,9 +3008,11 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)

if (is_on) {
clk_enable(hsotg->clk);
+ hsotg->connected = 1;
s3c_hsotg_core_connect(hsotg);
} else {
s3c_hsotg_core_disconnect(hsotg);
+ hsotg->connected = 0;
clk_disable(hsotg->clk);
}

@@ -3655,16 +3661,18 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
dev_info(hsotg->dev, "suspending usb gadget %s\n",
hsotg->driver->driver.name);

- spin_lock_irqsave(&hsotg->lock, flags);
- s3c_hsotg_core_disconnect(hsotg);
- s3c_hsotg_disconnect(hsotg);
- hsotg->gadget.speed = USB_SPEED_UNKNOWN;
- spin_unlock_irqrestore(&hsotg->lock, flags);
+ if (hsotg->enabled) {
+ int ep;

- s3c_hsotg_phy_disable(hsotg);
+ spin_lock_irqsave(&hsotg->lock, flags);
+ if (hsotg->connected)
+ s3c_hsotg_core_disconnect(hsotg);
+ s3c_hsotg_disconnect(hsotg);
+ hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+
+ s3c_hsotg_phy_disable(hsotg);

- if (hsotg->driver) {
- int ep;
for (ep = 0; ep < hsotg->num_of_eps; ep++)
s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);

@@ -3682,21 +3690,23 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
unsigned long flags;
int ret = 0;

- if (hsotg->driver) {
+ if (hsotg->driver)
dev_info(hsotg->dev, "resuming usb gadget %s\n",
hsotg->driver->driver.name);

+ if (hsotg->enabled) {
clk_enable(hsotg->clk);
ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
- hsotg->supplies);
- }
+ hsotg->supplies);

- s3c_hsotg_phy_enable(hsotg);
+ s3c_hsotg_phy_enable(hsotg);

- spin_lock_irqsave(&hsotg->lock, flags);
- s3c_hsotg_core_init_disconnected(hsotg);
- s3c_hsotg_core_connect(hsotg);
- spin_unlock_irqrestore(&hsotg->lock, flags);
+ spin_lock_irqsave(&hsotg->lock, flags);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ if (hsotg->connected)
+ s3c_hsotg_core_connect(hsotg);
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+ }

return ret;
}

[Marek Szyprowski]

This patch removes duplicated code and sets last_rst variable in the
function which does the hardware reset.

Signed-off-by: Marek Szyprowski <***@samsung.com>
---
drivers/usb/dwc2/gadget.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 178a6ea5eef8..1ba0682fb252 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2247,6 +2247,8 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
/* must be at-least 3ms to allow bus to see disconnect */
mdelay(3);

+ hsotg->last_rst = jiffies;
+
/* remove the soft-disconnect and let's go */
__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
}
@@ -2347,7 +2349,6 @@ irq_retry:
-ECONNRESET, true);

s3c_hsotg_core_init(hsotg);
- hsotg->last_rst = jiffies;
}
}
}
@@ -2908,7 +2909,6 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
goto err;
}

- hsotg->last_rst = jiffies;
dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
return 0;

@@ -3669,7 +3669,6 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
}

spin_lock_irqsave(&hsotg->lock, flags);
- hsotg->last_rst = jiffies;
s3c_hsotg_phy_enable(hsotg);
s3c_hsotg_core_init(hsotg);
spin_unlock_irqrestore(&hsotg->lock, flags);

[Marek Szyprowski]

This patch fixes probe function to match the pattern used elsewhere in
the driver, where power regulators are turned off as the last element in
the device shutdown procedure.

Signed-off-by: Marek Szyprowski <m.szyprowski-***@public.gmane.org>
---
drivers/usb/dwc2/gadget.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 37fda4c03397..178a6ea5eef8 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3573,6 +3573,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
s3c_hsotg_initep(hsotg, &hsotg->eps[epnum], epnum);

/* disable power and clock */
+ s3c_hsotg_phy_disable(hsotg);

ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
hsotg->supplies);
@@ -3581,8 +3582,6 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
goto err_ep_mem;
}

- s3c_hsotg_phy_disable(hsotg);

[Marek Szyprowski]

Excessive debug messages might cause timing issues that prevent correct
usb enumeration. This patch hides information about USB bus reset to let
driver enumerate fast enough to avoid making host angry. This fixes
endless enumeration and usb reset loop observed with some Linux hosts.

Signed-off-by: Marek Szyprowski <***@samsung.com>
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 119c8a3effc2..8870e38c1d82 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2333,7 +2333,7 @@ irq_retry:

u32 usb_status = readl(hsotg->regs + GOTGCTL);

- dev_info(hsotg->dev, "%s: USBRst\n", __func__);
+ dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
readl(hsotg->regs + GNPTXSTS));

[Felipe Balbi]

Post by Marek Szyprowski
Excessive debug messages might cause timing issues that prevent correct
usb enumeration. This patch hides information about USB bus reset to let
driver enumerate fast enough to avoid making host angry. This fixes
endless enumeration and usb reset loop observed with some Linux hosts.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 119c8a3effc2..8870e38c1d82 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
u32 usb_status = readl(hsotg->regs + GOTGCTL);
- dev_info(hsotg->dev, "%s: USBRst\n", __func__);
+ dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);

considering this is inside an IRQ handler, I'd rather use dev_vdbg() but
no strong feelings:

Reviewed-by: Felipe Balbi <***@ti.com>

[Marek Szyprowski]

This patch moves calls to phy enable/disable out of spinlock protected
blocks in device suspend/resume to fix incorrect caller context. Phy
related functions must not be called from atomic context.

Signed-off-by: Marek Szyprowski <m.szyprowski-***@public.gmane.org>
---
drivers/usb/dwc2/gadget.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index cdf417a7ae63..052b1a857291 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3656,11 +3656,13 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
hsotg->driver->driver.name);

spin_lock_irqsave(&hsotg->lock, flags);
+ s3c_hsotg_core_disconnect(hsotg);
s3c_hsotg_disconnect(hsotg);
- s3c_hsotg_phy_disable(hsotg);
hsotg->gadget.speed = USB_SPEED_UNKNOWN;
spin_unlock_irqrestore(&hsotg->lock, flags);

+ s3c_hsotg_phy_disable(hsotg);
+
if (hsotg->driver) {
int ep;
for (ep = 0; ep < hsotg->num_of_eps; ep++)
@@ -3689,9 +3691,10 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
hsotg->supplies);
}

- spin_lock_irqsave(&hsotg->lock, flags);
s3c_hsotg_phy_enable(hsotg);
- s3c_hsotg_core_init_disconnect(hsotg);
+
+ spin_lock_irqsave(&hsotg->lock, flags);
+ s3c_hsotg_core_init_disconnected(hsotg);
s3c_hsotg_core_connect(hsotg);
spin_unlock_irqrestore(&hsotg->lock, flags);

[Felipe Balbi]

Hi,

Post by Marek Szyprowski
This patch moves calls to phy enable/disable out of spinlock protected
blocks in device suspend/resume to fix incorrect caller context. Phy
related functions must not be called from atomic context.
---
drivers/usb/dwc2/gadget.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index cdf417a7ae63..052b1a857291 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3656,11 +3656,13 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
hsotg->driver->driver.name);
spin_lock_irqsave(&hsotg->lock, flags);
+ s3c_hsotg_core_disconnect(hsotg);
s3c_hsotg_disconnect(hsotg);
- s3c_hsotg_phy_disable(hsotg);
hsotg->gadget.speed = USB_SPEED_UNKNOWN;
spin_unlock_irqrestore(&hsotg->lock, flags);
+ s3c_hsotg_phy_disable(hsotg);

this is aching to have a locked version as well as an unlocked version.
Look at what you do here. There's a minor race when you release that
spinlock. By the time ->suspend() is called, IRQs are not yet disabled.

[Marek Szyprowski]

Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
This patch moves calls to phy enable/disable out of spinlock protected
blocks in device suspend/resume to fix incorrect caller context. Phy
related functions must not be called from atomic context.
---
drivers/usb/dwc2/gadget.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index cdf417a7ae63..052b1a857291 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3656,11 +3656,13 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t state)
hsotg->driver->driver.name);
spin_lock_irqsave(&hsotg->lock, flags);
+ s3c_hsotg_core_disconnect(hsotg);
s3c_hsotg_disconnect(hsotg);
- s3c_hsotg_phy_disable(hsotg);
hsotg->gadget.speed = USB_SPEED_UNKNOWN;
spin_unlock_irqrestore(&hsotg->lock, flags);
+ s3c_hsotg_phy_disable(hsotg);

this is aching to have a locked version as well as an unlocked version.
Look at what you do here. There's a minor race when you release that
spinlock. By the time ->suspend() is called, IRQs are not yet disabled.

s3c_hsotg_core_disconnect() disconnects the udc hardware from the usb bus, so even
if the irq comes before s3c_hsotg_phy_disable(), nothing wrong happens, because the
driver state is already set to disconnected.

Best regards

[Marek Szyprowski]

This patch changes s3c_hsotg_core_init function to leave hardware in
soft disconnect mode, so the actual moment of coupling the hardware to
the usb bus can be later controlled by the driver in the more accurate
way. For this purpose, separate functions for enabling and disabling
soft disconnect mode have been added.

Signed-off-by: Marek Szyprowski <m.szyprowski-***@public.gmane.org>
---
drivers/usb/dwc2/gadget.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 1ba0682fb252..d039334967d7 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
*
* Issue a soft reset to the core, and await the core finishing it.
*/
-static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
+static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
{
s3c_hsotg_corereset(hsotg);

@@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
readl(hsotg->regs + DOEPCTL0));

/* clear global NAKs */
- writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
+ writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
hsotg->regs + DCTL);

/* must be at-least 3ms to allow bus to see disconnect */
mdelay(3);

hsotg->last_rst = jiffies;
+}
+
+static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
+{
+ /* set the soft-disconnect bit */
+ __orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
+}

+static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
+{
/* remove the soft-disconnect and let's go */
__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
}
@@ -2348,7 +2357,8 @@ irq_retry:
kill_all_requests(hsotg, &hsotg->eps[0],
-ECONNRESET, true);

- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ s3c_hsotg_core_connect(hsotg);
}
}
}
@@ -2983,7 +2993,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
if (is_on) {
s3c_hsotg_phy_enable(hsotg);
clk_enable(hsotg->clk);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ s3c_hsotg_core_connect(hsotg);
} else {
clk_disable(hsotg->clk);
s3c_hsotg_phy_disable(hsotg);
@@ -3670,7 +3681,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)

spin_lock_irqsave(&hsotg->lock, flags);
s3c_hsotg_phy_enable(hsotg);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnect(hsotg);
+ s3c_hsotg_core_connect(hsotg);
spin_unlock_irqrestore(&hsotg->lock, flags);

return ret;

[Felipe Balbi]

Post by Marek Szyprowski
This patch changes s3c_hsotg_core_init function to leave hardware in
soft disconnect mode, so the actual moment of coupling the hardware to
the usb bus can be later controlled by the driver in the more accurate

what is this "more accurate way" you talk about ? Why is it more
accurate ? Perhaps you have failed some USB Certification test ? Which
test id was that ? Why did it fail ? and why does this patch solve the
issue ?

Post by Marek Szyprowski
way. For this purpose, separate functions for enabling and disabling
soft disconnect mode have been added.
---
drivers/usb/dwc2/gadget.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 1ba0682fb252..d039334967d7 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
*
* Issue a soft reset to the core, and await the core finishing it.
*/
-static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
+static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
{
s3c_hsotg_corereset(hsotg);
@@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
readl(hsotg->regs + DOEPCTL0));
/* clear global NAKs */
- writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
+ writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
hsotg->regs + DCTL);
/* must be at-least 3ms to allow bus to see disconnect */
mdelay(3);
hsotg->last_rst = jiffies;
+}
+
+static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
+{
+ /* set the soft-disconnect bit */
+ __orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
+}
+static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
+{
/* remove the soft-disconnect and let's go */
__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
}
kill_all_requests(hsotg, &hsotg->eps[0],
-ECONNRESET, true);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ s3c_hsotg_core_connect(hsotg);
}
}
}
@@ -2983,7 +2993,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
if (is_on) {
s3c_hsotg_phy_enable(hsotg);
clk_enable(hsotg->clk);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ s3c_hsotg_core_connect(hsotg);
} else {
clk_disable(hsotg->clk);
s3c_hsotg_phy_disable(hsotg);
@@ -3670,7 +3681,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
spin_lock_irqsave(&hsotg->lock, flags);
s3c_hsotg_phy_enable(hsotg);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnect(hsotg);
+ s3c_hsotg_core_connect(hsotg);
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
--
1.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Marek Szyprowski]

Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
This patch changes s3c_hsotg_core_init function to leave hardware in
soft disconnect mode, so the actual moment of coupling the hardware to
the usb bus can be later controlled by the driver in the more accurate

what is this "more accurate way" you talk about ? Why is it more
accurate ? Perhaps you have failed some USB Certification test ? Which
test id was that ? Why did it fail ? and why does this patch solve the
issue ?

This patch is just a preparation for the next patches, which introduces
usage
of soft-disconnect feature in pullup() method.

Post by Felipe Balbi

Post by Marek Szyprowski
way. For this purpose, separate functions for enabling and disabling
soft disconnect mode have been added.
---
drivers/usb/dwc2/gadget.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 1ba0682fb252..d039334967d7 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
*
* Issue a soft reset to the core, and await the core finishing it.
*/
-static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
+static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
{
s3c_hsotg_corereset(hsotg);
@@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
readl(hsotg->regs + DOEPCTL0));
/* clear global NAKs */
- writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
+ writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
hsotg->regs + DCTL);
/* must be at-least 3ms to allow bus to see disconnect */
mdelay(3);
hsotg->last_rst = jiffies;
+}
+
+static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
+{
+ /* set the soft-disconnect bit */
+ __orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
+}
+static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
+{
/* remove the soft-disconnect and let's go */
__bic32(hsotg->regs + DCTL, DCTL_SFTDISCON);
}
kill_all_requests(hsotg, &hsotg->eps[0],
-ECONNRESET, true);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ s3c_hsotg_core_connect(hsotg);
}
}
}
@@ -2983,7 +2993,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
if (is_on) {
s3c_hsotg_phy_enable(hsotg);
clk_enable(hsotg->clk);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnected(hsotg);
+ s3c_hsotg_core_connect(hsotg);
} else {
clk_disable(hsotg->clk);
s3c_hsotg_phy_disable(hsotg);
@@ -3670,7 +3681,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
spin_lock_irqsave(&hsotg->lock, flags);
s3c_hsotg_phy_enable(hsotg);
- s3c_hsotg_core_init(hsotg);
+ s3c_hsotg_core_init_disconnect(hsotg);
+ s3c_hsotg_core_connect(hsotg);
spin_unlock_irqrestore(&hsotg->lock, flags);
return ret;
--
1.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

Best regards

[Marek Szyprowski]

Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.

Signed-off-by: Marek Szyprowski <***@samsung.com>
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}

- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

driver->driver.bus = NULL;
hsotg->driver = driver;

[Felipe Balbi]

Hi,

Post by Marek Szyprowski
Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}
- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

[Marek Szyprowski]

Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}
- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
drivers. I was a bit confused by the fact that udc core passes driver to
udc_stop(), when called from soft_connect and NULL on gadget removal.

Best regards

[Felipe Balbi]

Post by Marek Szyprowski
Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}
- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
drivers. I was a bit confused by the fact that udc core passes driver to
udc_stop(), when called from soft_connect and NULL on gadget removal.

That can probably be cleaned up, I'll go have a look on all UDCs and
make sure I won't break anything.

[Felipe Balbi]

Hi,

Post by Felipe Balbi

Post by Marek Szyprowski
Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}
- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
drivers. I was a bit confused by the fact that udc core passes driver to
udc_stop(), when called from soft_connect and NULL on gadget removal.

That can probably be cleaned up, I'll go have a look on all UDCs and
make sure I won't break anything.

looks like chipidea is the only one still using that argument, if you
make your patch look like below:

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..ac14328 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2934,8 +2934,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,

spin_lock_irqsave(&hsotg->lock, flags);

- if (!driver)
- hsotg->driver = NULL;
+ hsotg->driver = NULL;

hsotg->gadget.speed = USB_SPEED_UNKNOWN;


I'll remove the argument.

[Felipe Balbi]

Post by Felipe Balbi
Hi,

Post by Felipe Balbi

Post by Marek Szyprowski
Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}
- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
drivers. I was a bit confused by the fact that udc core passes driver to
udc_stop(), when called from soft_connect and NULL on gadget removal.

That can probably be cleaned up, I'll go have a look on all UDCs and
make sure I won't break anything.

looks like chipidea is the only one still using that argument, if you

meant dwc2

[Felipe Balbi]

Post by Felipe Balbi

Post by Felipe Balbi
Hi,

Post by Felipe Balbi

Post by Marek Szyprowski
Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}
- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
drivers. I was a bit confused by the fact that udc core passes driver to
udc_stop(), when called from soft_connect and NULL on gadget removal.

That can probably be cleaned up, I'll go have a look on all UDCs and
make sure I won't break anything.

looks like chipidea is the only one still using that argument, if you

meant dwc2

Hmm, there are still a few other gadgets relying on that argument. It'll
take a little more effort to remove it.

[Felipe Balbi]

Post by Felipe Balbi

Post by Felipe Balbi

Post by Felipe Balbi
Hi,

Post by Felipe Balbi

Post by Marek Szyprowski
Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
Enabling and disabling usb gadget by writing to
/sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
functions with the same usb gadget driver, so the driver should not WARN
about such case.
---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8870e38c1d82..37fda4c03397 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
return -EINVAL;
}
- WARN_ON(hsotg->driver);
+ WARN_ON(hsotg->driver && hsotg->driver != driver);

the bug is in your ->udc_stop(). You should clear hsotg->driver to NULL
there.

Ok, I will change udc_stop() to always zero hsotg->driver, like other udc
drivers. I was a bit confused by the fact that udc core passes driver to
udc_stop(), when called from soft_connect and NULL on gadget removal.

That can probably be cleaned up, I'll go have a look on all UDCs and
make sure I won't break anything.

looks like chipidea is the only one still using that argument, if you

meant dwc2

Hmm, there are still a few other gadgets relying on that argument. It'll
take a little more effort to remove it.

Alright, just pending your bug fix. I have fixed all other drivers. Will
send out shortly.

[Felipe Balbi]

Post by Marek Szyprowski
This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt to correctly notify gadget subsystem about unplugged usb
cable.
---
drivers/usb/dwc2/gadget.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856fadd93..119c8a3effc2 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
dev_info(hsotg->dev, "OTGInt: %08x\n", otgint);
writel(otgint, hsotg->regs + GOTGINT);
+
+ if (otgint & GOTGINT_SES_END_DET) {

looks like this should be done for GINTSTS_DISCONNINT.

[Marek Szyprowski]

Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt to correctly notify gadget subsystem about unplugged usb
cable.
---
drivers/usb/dwc2/gadget.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856fadd93..119c8a3effc2 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
dev_info(hsotg->dev, "OTGInt: %08x\n", otgint);
writel(otgint, hsotg->regs + GOTGINT);
+
+ if (otgint & GOTGINT_SES_END_DET) {

looks like this should be done for GINTSTS_DISCONNINT.

I also would like to report it from that interrupt, but according to
DWC2 databook
(version 2.81a) and my observations on Samsung Exynos SoCs, DISCONNINT
interrupt
is asserted only in host mode, so in device mode we need to use
something else.

Best regards

[Felipe Balbi]

Post by Marek Szyprowski
Hello,

Post by Felipe Balbi

Post by Marek Szyprowski
This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt to correctly notify gadget subsystem about unplugged usb
cable.
---
drivers/usb/dwc2/gadget.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856fadd93..119c8a3effc2 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
dev_info(hsotg->dev, "OTGInt: %08x\n", otgint);
writel(otgint, hsotg->regs + GOTGINT);
+
+ if (otgint & GOTGINT_SES_END_DET) {

looks like this should be done for GINTSTS_DISCONNINT.

I also would like to report it from that interrupt, but according to
DWC2 databook (version 2.81a) and my observations on Samsung Exynos
SoCs, DISCONNINT interrupt is asserted only in host mode, so in device
mode we need to use something else.

If that's the case, then I withdraw my comments. I don't have access to
HW or docs about this IP, it just looked suspicious :-)