[PATCH 0/2] usb: dwc3: gadget: Fix erratic interrupts and delayed enumeration

[PATCH 0/2] usb: dwc3: gadget: Fix erratic interrupts and delayed enumeration

[Roger Quadros]

Hi,

The existing workaround (for STAR#9000525659) of forcing
DEVSPD to SUPER_SPEED for HIGH_SPEED ports is causing
another side effect which causes erratic interrupts and delayed gadget
enumeration of upto 2 seconds.

Work around the run/stop issue by detecting if
it happened using debug LTSSM state and issuing
soft reset to the device controller when changing RUN_STOP
from 0 to 1.

We apply the workaround only if PRTCAP is DEVICE mode
as we don't yet support this workaround in OTG mode.

Use USB RESET event as exit condition for workaround.

cheers,
-roger

Roger Quadros (2):
  usb: dwc3: core: Introduce dwc3_device_reinit()
  usb: dwc3: gadget: usb: dwc3: run/stop metastability workaround

 drivers/usb/dwc3/core.c   | 142 +++++++++++++++++++++++++------------
 drivers/usb/dwc3/core.h   |   4 ++
 drivers/usb/dwc3/gadget.c | 175 +++++++++++++++++++++++++++++++++++++---------
 3 files changed, 246 insertions(+), 75 deletions(-)

-- 
2.5.0

[PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Roger Quadros]

We will need this function for a workaround.
The function issues a softreset only to the device
controller and performs minimal re-initialization
so that the device controller can be usable.

As some code is similar to dwc3_core_init() take out
common code into dwc3_get_gctl_quirks().

We add a new member (prtcap_mode) to struct dwc3 to
keep track of the current mode in the PRTCAPDIR register.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c | 142 +++++++++++++++++++++++++++++++++---------------
 drivers/usb/dwc3/core.h |   3 +
 2 files changed, 102 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 17fd8144..3b09494 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -58,6 +58,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
 	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
 	reg |= DWC3_GCTL_PRTCAPDIR(mode);
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+	dwc->prtcap_mode = mode;
 }
 
 /**
@@ -525,53 +526,16 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 }
 
 /**
- * dwc3_core_init - Low-level initialization of DWC3 Core
+ * dwc3_get_gctl_quirks - Prepare GCTL register content with quirks
+ * and workarounds.
  * @dwc: Pointer to our controller context structure
  *
- * Returns 0 on success otherwise negative errno.
+ * Returns 32-bit content that must be written into GCTL by caller.
  */
-static int dwc3_core_init(struct dwc3 *dwc)
+static u32 dwc3_get_gctl_quirks(struct dwc3 *dwc)
 {
-	u32			hwparams4 = dwc->hwparams.hwparams4;
-	u32			reg;
-	int			ret;
-
-	reg = dwc3_readl(dwc->regs, DWC3_GSNPSID);
-	/* This should read as U3 followed by revision number */
-	if ((reg & DWC3_GSNPSID_MASK) == 0x55330000) {
-		/* Detected DWC_usb3 IP */
-		dwc->revision = reg;
-	} else if ((reg & DWC3_GSNPSID_MASK) == 0x33310000) {
-		/* Detected DWC_usb31 IP */
-		dwc->revision = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
-		dwc->revision |= DWC3_REVISION_IS_DWC31;
-	} else {
-		dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
-		ret = -ENODEV;
-		goto err0;
-	}
-
-	/*
-	 * Write Linux Version Code to our GUID register so it's easy to figure
-	 * out which kernel version a bug was found.
-	 */
-	dwc3_writel(dwc->regs, DWC3_GUID, LINUX_VERSION_CODE);
-
-	/* Handle USB2.0-only core configuration */
-	if (DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3) ==
-			DWC3_GHWPARAMS3_SSPHY_IFC_DIS) {
-		if (dwc->maximum_speed == USB_SPEED_SUPER)
-			dwc->maximum_speed = USB_SPEED_HIGH;
-	}
-
-	/* issue device SoftReset too */
-	ret = dwc3_soft_reset(dwc);
-	if (ret)
-		goto err0;
-
-	ret = dwc3_core_soft_reset(dwc);
-	if (ret)
-		goto err0;
+	u32 reg;
+	u32 hwparams4 = dwc->hwparams.hwparams4;
 
 	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
 	reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
@@ -639,6 +603,59 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	if (dwc->revision < DWC3_REVISION_190A)
 		reg |= DWC3_GCTL_U2RSTECN;
 
+	return reg;
+}
+
+/**
+ * dwc3_core_init - Low-level initialization of DWC3 Core
+ * @dwc: Pointer to our controller context structure
+ *
+ * Returns 0 on success otherwise negative errno.
+ */
+static int dwc3_core_init(struct dwc3 *dwc)
+{
+	u32			reg;
+	int			ret;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GSNPSID);
+	/* This should read as U3 followed by revision number */
+	if ((reg & DWC3_GSNPSID_MASK) == 0x55330000) {
+		/* Detected DWC_usb3 IP */
+		dwc->revision = reg;
+	} else if ((reg & DWC3_GSNPSID_MASK) == 0x33310000) {
+		/* Detected DWC_usb31 IP */
+		dwc->revision = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
+		dwc->revision |= DWC3_REVISION_IS_DWC31;
+	} else {
+		dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
+		ret = -ENODEV;
+		goto err0;
+	}
+
+	/*
+	 * Write Linux Version Code to our GUID register so it's easy to figure
+	 * out which kernel version a bug was found.
+	 */
+	dwc3_writel(dwc->regs, DWC3_GUID, LINUX_VERSION_CODE);
+
+	/* Handle USB2.0-only core configuration */
+	if (DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3) ==
+			DWC3_GHWPARAMS3_SSPHY_IFC_DIS) {
+		if (dwc->maximum_speed == USB_SPEED_SUPER)
+			dwc->maximum_speed = USB_SPEED_HIGH;
+	}
+
+	/* issue device SoftReset too */
+	ret = dwc3_soft_reset(dwc);
+	if (ret)
+		goto err0;
+
+	ret = dwc3_core_soft_reset(dwc);
+	if (ret)
+		goto err0;
+
+	reg = dwc3_get_gctl_quirks(dwc);
+
 	dwc3_core_num_eps(dwc);
 
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
@@ -666,6 +683,45 @@ err0:
 	return ret;
 }
 
+/**
+ * dwc3_device_reinit - Reset device controller and re-initialize.
+ * Can currently be called only if dwc->prtcap_mode == DWC3_GCTL_PRTCAP_DEVICE
+ * @dwc: Pointer to our controller context structure
+ *
+ * Returns 0 on success otherwise negative errno.
+ */
+int dwc3_device_reinit(struct dwc3 *dwc)
+{
+	u32			reg;
+	int			ret;
+
+	if (dwc->prtcap_mode != DWC3_GCTL_PRTCAP_DEVICE) {
+		dev_err(dwc->dev, "%s can't be used for current_mode %d\n",
+			__func__, dwc->prtcap_mode);
+		return -EINVAL;
+	}
+
+	dwc3_free_scratch_buffers(dwc);
+
+	ret = dwc3_soft_reset(dwc);
+	if (ret)
+		return ret;
+
+	reg = dwc3_get_gctl_quirks(dwc);
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	ret = dwc3_event_buffers_setup(dwc);
+	if (ret) {
+		dev_err(dwc->dev, "failed to setup event buffers\n");
+		return ret;
+	}
+
+	/* Set portcap. For now we support device only */
+	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+
+	return ret;
+}
+
 static void dwc3_core_exit(struct dwc3 *dwc)
 {
 	dwc3_free_scratch_buffers(dwc);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 6254b2f..2bea1ac 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -672,6 +672,7 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @prtcap_mode: current mode of operation written to PRTCAPDIR
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -774,6 +775,7 @@ struct dwc3 {
 	size_t			regs_size;
 
 	enum usb_dr_mode	dr_mode;
+	u32			prtcap_mode;
 
 	/* used for suspend/resume */
 	u32			dcfg;
@@ -1026,6 +1028,7 @@ struct dwc3_gadget_ep_cmd_params {
 /* prototypes */
 void dwc3_set_mode(struct dwc3 *dwc, u32 mode);
 int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc);
+int dwc3_device_reinit(struct dwc3 *dwc);
 
 /* check whether we are on the DWC_usb31 core */
 static inline bool dwc3_is_usb31(struct dwc3 *dwc)
-- 
2.5.0

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> [ text/plain ]
> We will need this function for a workaround.
> The function issues a softreset only to the device
> controller and performs minimal re-initialization
> so that the device controller can be usable.
>
> As some code is similar to dwc3_core_init() take out
> common code into dwc3_get_gctl_quirks().
>
> We add a new member (prtcap_mode) to struct dwc3 to
> keep track of the current mode in the PRTCAPDIR register.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

I must say, I don't like this at all :-p There's ONE known silicon which
needs this because of a poor silicon integration which took an IP with a
known erratum where it can't be made to work on lower speeds and STILL
was integrated without a superspeed PHY.

There's a reason why I never tried to push this upstream myself ;-)

I'm really thinking we might be better off adding a quirk flag to skip
the metastability workaround and allow this ONE silicon to set the
controller to lower speed.

John, can you check with your colleagues if we would ever fall into
STAR#9000525659 if we set maximum speed to high speed during driver
probe and never touch it again ? I would assume we don't really fall
into the metastability workaround, right ? We're not doing any sort of
PM for dwc3...

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


heh, +john

Felipe Balbi <balbi@kernel.org> writes:
> [ text/plain ]
>
> Hi,
>
> Roger Quadros <rogerq@ti.com> writes:
>> [ text/plain ]
>> We will need this function for a workaround.
>> The function issues a softreset only to the device
>> controller and performs minimal re-initialization
>> so that the device controller can be usable.
>>
>> As some code is similar to dwc3_core_init() take out
>> common code into dwc3_get_gctl_quirks().
>>
>> We add a new member (prtcap_mode) to struct dwc3 to
>> keep track of the current mode in the PRTCAPDIR register.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>
> I must say, I don't like this at all :-p There's ONE known silicon which
> needs this because of a poor silicon integration which took an IP with a
> known erratum where it can't be made to work on lower speeds and STILL
> was integrated without a superspeed PHY.
>
> There's a reason why I never tried to push this upstream myself ;-)
>
> I'm really thinking we might be better off adding a quirk flag to skip
> the metastability workaround and allow this ONE silicon to set the
> controller to lower speed.
>
> John, can you check with your colleagues if we would ever fall into
> STAR#9000525659 if we set maximum speed to high speed during driver
> probe and never touch it again ? I would assume we don't really fall
> into the metastability workaround, right ? We're not doing any sort of
> PM for dwc3...
>
> -- 
> balbi
> [ signature.asc: application/pgp-signature ]

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[John Youn]

On 3/16/2016 6:56 AM, Felipe Balbi wrote:
> 
> heh, +john
> 
> Felipe Balbi <balbi@kernel.org> writes:
>> [ text/plain ]
>>
>> Hi,
>>
>> Roger Quadros <rogerq@ti.com> writes:
>>> [ text/plain ]
>>> We will need this function for a workaround.
>>> The function issues a softreset only to the device
>>> controller and performs minimal re-initialization
>>> so that the device controller can be usable.
>>>
>>> As some code is similar to dwc3_core_init() take out
>>> common code into dwc3_get_gctl_quirks().
>>>
>>> We add a new member (prtcap_mode) to struct dwc3 to
>>> keep track of the current mode in the PRTCAPDIR register.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>
>> I must say, I don't like this at all :-p There's ONE known silicon which
>> needs this because of a poor silicon integration which took an IP with a
>> known erratum where it can't be made to work on lower speeds and STILL
>> was integrated without a superspeed PHY.
>>
>> There's a reason why I never tried to push this upstream myself ;-)
>>
>> I'm really thinking we might be better off adding a quirk flag to skip
>> the metastability workaround and allow this ONE silicon to set the
>> controller to lower speed.
>>
>> John, can you check with your colleagues if we would ever fall into
>> STAR#9000525659 if we set maximum speed to high speed during driver
>> probe and never touch it again ? I would assume we don't really fall
>> into the metastability workaround, right ? We're not doing any sort of
>> PM for dwc3...
>>

Hi Felipe,

I don't know much about this but I will check with the engineers and
get back to you.

John

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
> [ text/plain ]
> On 3/16/2016 6:56 AM, Felipe Balbi wrote:
>> 
>> heh, +john
>> 
>> Felipe Balbi <balbi@kernel.org> writes:
>>> [ text/plain ]
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> [ text/plain ]
>>>> We will need this function for a workaround.
>>>> The function issues a softreset only to the device
>>>> controller and performs minimal re-initialization
>>>> so that the device controller can be usable.
>>>>
>>>> As some code is similar to dwc3_core_init() take out
>>>> common code into dwc3_get_gctl_quirks().
>>>>
>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>> needs this because of a poor silicon integration which took an IP with a
>>> known erratum where it can't be made to work on lower speeds and STILL
>>> was integrated without a superspeed PHY.
>>>
>>> There's a reason why I never tried to push this upstream myself ;-)
>>>
>>> I'm really thinking we might be better off adding a quirk flag to skip
>>> the metastability workaround and allow this ONE silicon to set the
>>> controller to lower speed.
>>>
>>> John, can you check with your colleagues if we would ever fall into
>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>> probe and never touch it again ? I would assume we don't really fall
>>> into the metastability workaround, right ? We're not doing any sort of
>>> PM for dwc3...
>>>
>
> Hi Felipe,
>
> I don't know much about this but I will check with the engineers and
> get back to you.

Thank you John ;-)

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[John Youn]

On 3/18/2016 12:17 PM, John Youn wrote:
> On 3/16/2016 6:56 AM, Felipe Balbi wrote:
>>
>> heh, +john
>>
>> Felipe Balbi <balbi@kernel.org> writes:
>>> [ text/plain ]
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> [ text/plain ]
>>>> We will need this function for a workaround.
>>>> The function issues a softreset only to the device
>>>> controller and performs minimal re-initialization
>>>> so that the device controller can be usable.
>>>>
>>>> As some code is similar to dwc3_core_init() take out
>>>> common code into dwc3_get_gctl_quirks().
>>>>
>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>> needs this because of a poor silicon integration which took an IP with a
>>> known erratum where it can't be made to work on lower speeds and STILL
>>> was integrated without a superspeed PHY.
>>>
>>> There's a reason why I never tried to push this upstream myself ;-)
>>>
>>> I'm really thinking we might be better off adding a quirk flag to skip
>>> the metastability workaround and allow this ONE silicon to set the
>>> controller to lower speed.
>>>
>>> John, can you check with your colleagues if we would ever fall into
>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>> probe and never touch it again ? I would assume we don't really fall
>>> into the metastability workaround, right ? We're not doing any sort of
>>> PM for dwc3...
>>>

Hi Felipe,

Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
I don't see an issue with that as long as we always ignore
dwc->maximum_speed when programming DCFG.speed for all affected
versions of the core. As long as the DCFG.speed = SS, you should not
hit the STAR.

John

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
> [ text/plain ]
> On 3/18/2016 12:17 PM, John Youn wrote:
>> On 3/16/2016 6:56 AM, Felipe Balbi wrote:
>>>
>>> heh, +john
>>>
>>> Felipe Balbi <balbi@kernel.org> writes:
>>>> [ text/plain ]
>>>>
>>>> Hi,
>>>>
>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>> [ text/plain ]
>>>>> We will need this function for a workaround.
>>>>> The function issues a softreset only to the device
>>>>> controller and performs minimal re-initialization
>>>>> so that the device controller can be usable.
>>>>>
>>>>> As some code is similar to dwc3_core_init() take out
>>>>> common code into dwc3_get_gctl_quirks().
>>>>>
>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>
>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>> needs this because of a poor silicon integration which took an IP with a
>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>> was integrated without a superspeed PHY.
>>>>
>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>
>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>> the metastability workaround and allow this ONE silicon to set the
>>>> controller to lower speed.
>>>>
>>>> John, can you check with your colleagues if we would ever fall into
>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>> probe and never touch it again ? I would assume we don't really fall
>>>> into the metastability workaround, right ? We're not doing any sort of
>>>> PM for dwc3...
>>>>
>
> Hi Felipe,
>
> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
> I don't see an issue with that as long as we always ignore
> dwc->maximum_speed when programming DCFG.speed for all affected
> versions of the core. As long as the DCFG.speed = SS, you should not
> hit the STAR.

I actually mean changing DCFG.speed during driver probe and never
touching it again. Would that still cause problems ?

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[John Youn]

On 3/21/2016 11:40 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn@synopsys.com> writes:
>> [ text/plain ]
>> On 3/18/2016 12:17 PM, John Youn wrote:
>>> On 3/16/2016 6:56 AM, Felipe Balbi wrote:
>>>>
>>>> heh, +john
>>>>
>>>> Felipe Balbi <balbi@kernel.org> writes:
>>>>> [ text/plain ]
>>>>>
>>>>> Hi,
>>>>>
>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>> [ text/plain ]
>>>>>> We will need this function for a workaround.
>>>>>> The function issues a softreset only to the device
>>>>>> controller and performs minimal re-initialization
>>>>>> so that the device controller can be usable.
>>>>>>
>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>
>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>
>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>
>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>> was integrated without a superspeed PHY.
>>>>>
>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>
>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>> controller to lower speed.
>>>>>
>>>>> John, can you check with your colleagues if we would ever fall into
>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>> PM for dwc3...
>>>>>
>>
>> Hi Felipe,
>>
>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>> I don't see an issue with that as long as we always ignore
>> dwc->maximum_speed when programming DCFG.speed for all affected
>> versions of the core. As long as the DCFG.speed = SS, you should not
>> hit the STAR.
> 
> I actually mean changing DCFG.speed during driver probe and never
> touching it again. Would that still cause problems ?
> 

In that case I'm not sure. The engineer who would know is off until
next week so I'll get back to you as soon as I can talk to him about
it.

Regards,
John

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
> [ text/plain ]
> On 3/21/2016 11:40 PM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> John Youn <John.Youn@synopsys.com> writes:
>>> [ text/plain ]
>>> On 3/18/2016 12:17 PM, John Youn wrote:
>>>> On 3/16/2016 6:56 AM, Felipe Balbi wrote:
>>>>>
>>>>> heh, +john
>>>>>
>>>>> Felipe Balbi <balbi@kernel.org> writes:
>>>>>> [ text/plain ]
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>>> [ text/plain ]
>>>>>>> We will need this function for a workaround.
>>>>>>> The function issues a softreset only to the device
>>>>>>> controller and performs minimal re-initialization
>>>>>>> so that the device controller can be usable.
>>>>>>>
>>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>>
>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>>
>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>
>>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>>> was integrated without a superspeed PHY.
>>>>>>
>>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>>
>>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>>> controller to lower speed.
>>>>>>
>>>>>> John, can you check with your colleagues if we would ever fall into
>>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>>> PM for dwc3...
>>>>>>
>>>
>>> Hi Felipe,
>>>
>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>> I don't see an issue with that as long as we always ignore
>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>> versions of the core. As long as the DCFG.speed = SS, you should not
>>> hit the STAR.
>> 
>> I actually mean changing DCFG.speed during driver probe and never
>> touching it again. Would that still cause problems ?
>> 
>
> In that case I'm not sure. The engineer who would know is off until
> next week so I'll get back to you as soon as I can talk to him about
> it.

Thank you :-)

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[John Youn]

On 3/23/2016 11:52 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn@synopsys.com> writes:
>> [ text/plain ]
>> On 3/21/2016 11:40 PM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> John Youn <John.Youn@synopsys.com> writes:
>>>> [ text/plain ]
>>>> On 3/18/2016 12:17 PM, John Youn wrote:
>>>>> On 3/16/2016 6:56 AM, Felipe Balbi wrote:
>>>>>>
>>>>>> heh, +john
>>>>>>
>>>>>> Felipe Balbi <balbi@kernel.org> writes:
>>>>>>> [ text/plain ]
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>>>> [ text/plain ]
>>>>>>>> We will need this function for a workaround.
>>>>>>>> The function issues a softreset only to the device
>>>>>>>> controller and performs minimal re-initialization
>>>>>>>> so that the device controller can be usable.
>>>>>>>>
>>>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>>>
>>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>>>
>>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>>
>>>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>>>> was integrated without a superspeed PHY.
>>>>>>>
>>>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>>>
>>>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>>>> controller to lower speed.
>>>>>>>
>>>>>>> John, can you check with your colleagues if we would ever fall into
>>>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>>>> PM for dwc3...
>>>>>>>
>>>>
>>>> Hi Felipe,
>>>>
>>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>>> I don't see an issue with that as long as we always ignore
>>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>>> versions of the core. As long as the DCFG.speed = SS, you should not
>>>> hit the STAR.
>>>
>>> I actually mean changing DCFG.speed during driver probe and never
>>> touching it again. Would that still cause problems ?
>>>
>>
>> In that case I'm not sure. The engineer who would know is off until
>> next week so I'll get back to you as soon as I can talk to him about
>> it.
> 

So the engineers said that this issue can occur while set to HS and
the run/stop bit is changed so it seems that won't work.

Regards,
John

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Roger Quadros]

On 30/03/16 21:44, John Youn wrote:
> On 3/23/2016 11:52 PM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> John Youn <John.Youn@synopsys.com> writes:
>>> [ text/plain ]
>>> On 3/21/2016 11:40 PM, Felipe Balbi wrote:
>>>>
>>>> Hi,
>>>>
>>>> John Youn <John.Youn@synopsys.com> writes:
>>>>> [ text/plain ]
>>>>> On 3/18/2016 12:17 PM, John Youn wrote:
>>>>>> On 3/16/2016 6:56 AM, Felipe Balbi wrote:
>>>>>>>
>>>>>>> heh, +john
>>>>>>>
>>>>>>> Felipe Balbi <balbi@kernel.org> writes:
>>>>>>>> [ text/plain ]
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>>>>> [ text/plain ]
>>>>>>>>> We will need this function for a workaround.
>>>>>>>>> The function issues a softreset only to the device
>>>>>>>>> controller and performs minimal re-initialization
>>>>>>>>> so that the device controller can be usable.
>>>>>>>>>
>>>>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>>>>
>>>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>>>
>>>>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>>>>> was integrated without a superspeed PHY.
>>>>>>>>
>>>>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>>>>
>>>>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>>>>> controller to lower speed.
>>>>>>>>
>>>>>>>> John, can you check with your colleagues if we would ever fall into
>>>>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>>>>> PM for dwc3...
>>>>>>>>
>>>>>
>>>>> Hi Felipe,
>>>>>
>>>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>>>> I don't see an issue with that as long as we always ignore
>>>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>>>> versions of the core. As long as the DCFG.speed = SS, you should not
>>>>> hit the STAR.
>>>>
>>>> I actually mean changing DCFG.speed during driver probe and never
>>>> touching it again. Would that still cause problems ?
>>>>
>>>
>>> In that case I'm not sure. The engineer who would know is off until
>>> next week so I'll get back to you as soon as I can talk to him about
>>> it.
>>
> 
> So the engineers said that this issue can occur while set to HS and
> the run/stop bit is changed so it seems that won't work.

Thanks John.

Felipe,

any suggestion how we can fix this upstream?

cheers,
-roger

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>>>>>>>> We will need this function for a workaround.
>>>>>>>>>> The function issues a softreset only to the device
>>>>>>>>>> controller and performs minimal re-initialization
>>>>>>>>>> so that the device controller can be usable.
>>>>>>>>>>
>>>>>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>>>>>
>>>>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>>>>
>>>>>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>>>>>> was integrated without a superspeed PHY.
>>>>>>>>>
>>>>>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>>>>>
>>>>>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>>>>>> controller to lower speed.
>>>>>>>>>
>>>>>>>>> John, can you check with your colleagues if we would ever fall into
>>>>>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>>>>>> PM for dwc3...
>>>>>>>>>
>>>>>>
>>>>>> Hi Felipe,
>>>>>>
>>>>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>>>>> I don't see an issue with that as long as we always ignore
>>>>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>>>>> versions of the core. As long as the DCFG.speed = SS, you should not
>>>>>> hit the STAR.
>>>>>
>>>>> I actually mean changing DCFG.speed during driver probe and never
>>>>> touching it again. Would that still cause problems ?
>>>>>
>>>>
>>>> In that case I'm not sure. The engineer who would know is off until
>>>> next week so I'll get back to you as soon as I can talk to him about
>>>> it.
>>>
>> 
>> So the engineers said that this issue can occur while set to HS and
>> the run/stop bit is changed so it seems that won't work.
>
> Thanks John.
>
> Felipe,
>
> any suggestion how we can fix this upstream?

no idea, I don't have a lot of memory about this problem. I really don't
remember the details about this, is there an openly available errata
document which I could read ? /me goes search for it.

I found [1] which tells me, the following:


| i819        | A Device Control Bit Meta-Stability for USB3.0 Controller in USB2.0 Mode   |
|-------------+----------------------------------------------------------------------------|
| Criticality | Medium                                                                     |
|             |                                                                            |
| Descritiion | When USB3.0 controller core is programmed to be a USB 2.0-only device      |
|             | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing the core to     |
|             | attempt high speed as well as SuperSpeed connection or completely miss     |
|             | the attach request.                                                        |
|             |                                                                            |
| Workaround  | If the requirement is to always function in USB 2.0 mode, there is no      |
|             | workaround.                                                                |
|             | Otherwise, you can always program the USB controller core to be SuperSpeed |
|             | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4)                                    |
|             |                                                                            |
| Revisions   | SR 1.1, 2.0                                                                |
| Impacted    |                                                                            |
|-------------+----------------------------------------------------------------------------|

So, TI's own documentation says that there is _no_ workaround. My
question is, then: How are you sure that resetting the device actually
solves the issue ? Did you really hit the metastability problem and
noted that it works after a soft-reset ? How did you verify that
Run/Stop was in a metastable state, considering that Run/Stop signal is
not visible outside the die ?

It seems to me that resetting the IP is just as "dangerous" as setting
the IP to High-speed in the first place. No ?

[1] http://www.ti.com/lit/er/sprz429h/sprz429h.pdf

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Roger Quadros]

+Abhishek, Ravi,

Felipe,

On 31/03/16 17:26, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>>>>>>>>> We will need this function for a workaround.
>>>>>>>>>>> The function issues a softreset only to the device
>>>>>>>>>>> controller and performs minimal re-initialization
>>>>>>>>>>> so that the device controller can be usable.
>>>>>>>>>>>
>>>>>>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>>>>>>
>>>>>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>>>>>
>>>>>>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>>>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>>>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>>>>>>> was integrated without a superspeed PHY.
>>>>>>>>>>
>>>>>>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>>>>>>
>>>>>>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>>>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>>>>>>> controller to lower speed.
>>>>>>>>>>
>>>>>>>>>> John, can you check with your colleagues if we would ever fall into
>>>>>>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>>>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>>>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>>>>>>> PM for dwc3...
>>>>>>>>>>
>>>>>>>
>>>>>>> Hi Felipe,
>>>>>>>
>>>>>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>>>>>> I don't see an issue with that as long as we always ignore
>>>>>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>>>>>> versions of the core. As long as the DCFG.speed = SS, you should not
>>>>>>> hit the STAR.
>>>>>>
>>>>>> I actually mean changing DCFG.speed during driver probe and never
>>>>>> touching it again. Would that still cause problems ?
>>>>>>
>>>>>
>>>>> In that case I'm not sure. The engineer who would know is off until
>>>>> next week so I'll get back to you as soon as I can talk to him about
>>>>> it.
>>>>
>>>
>>> So the engineers said that this issue can occur while set to HS and
>>> the run/stop bit is changed so it seems that won't work.
>>
>> Thanks John.
>>
>> Felipe,
>>
>> any suggestion how we can fix this upstream?
> 
> no idea, I don't have a lot of memory about this problem. I really don't
> remember the details about this, is there an openly available errata
> document which I could read ? /me goes search for it.
> 
> I found [1] which tells me, the following:
> 
> 
> | i819        | A Device Control Bit Meta-Stability for USB3.0 Controller in USB2.0 Mode   |
> |-------------+----------------------------------------------------------------------------|
> | Criticality | Medium                                                                     |
> |             |                                                                            |
> | Descritiion | When USB3.0 controller core is programmed to be a USB 2.0-only device      |
> |             | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing the core to     |
> |             | attempt high speed as well as SuperSpeed connection or completely miss     |
> |             | the attach request.                                                        |
> |             |                                                                            |
> | Workaround  | If the requirement is to always function in USB 2.0 mode, there is no      |
> |             | workaround.                                                                |
> |             | Otherwise, you can always program the USB controller core to be SuperSpeed |
> |             | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4)                                    |
> |             |                                                                            |
> | Revisions   | SR 1.1, 2.0                                                                |
> | Impacted    |                                                                            |
> |-------------+----------------------------------------------------------------------------|
> 
> So, TI's own documentation says that there is _no_ workaround. My

We are working on updating that document. Apparently Synopsis has suggested this workaround.
pasting verbatim

"
-          As last step of device configuration we set the RUNSTOP bit in DCTL.

-          Once we set the RUNSTOP bit, we need to monitor GDBGLTSSM for 100 ms until one of the two below happens:.

	o   We see the GDBGLTSSM.LTDB_LINK_STATE changing from 4

	o   We receive the USB 2.0 reset interrupt.

If none of above happens then we can stop monitoring it.

-          If state change from 4 occurs issue a SoftReset thru DCTL.CSftRst and reconfigure Device. This time it is guaranteed that no metastability will occur so no need to do the 100ms timeout.
"

> question is, then: How are you sure that resetting the device actually
> solves the issue ? Did you really hit the metastability problem and
> noted that it works after a soft-reset ? How did you verify that

I don't know if it solves the issue or not. It was suggested by Synopsis to TI's silicon team.
I never hit the metastability problem detection condition in my overnight tests (i.e. LTDB_LINK_STATE != 4).
I have verified that things work after a soft-reset by faking that the error happens.

> Run/Stop was in a metastable state, considering that Run/Stop signal is
> not visible outside the die ?

LTDB_LINK_STATE != 4 within 100ms or RUNSTOP set is the condition to detect that the issue occurred.

> 
> It seems to me that resetting the IP is just as "dangerous" as setting
> the IP to High-speed in the first place. No ?

The soft-reset is just a recovery mechanism if that error is ever hit.
Putting the controller in reset state means it is in a known state. Why do you say it
would be dangerous?

The original workaround i.e. setting the High-speed instance to Super-speed to avoid this errata
is causing another side effect. i.e. erratic interrupts happen and more than 2 seconds delay
to enumerations. This problem is more serious and so we have to keep the controller in
High-speed and tackle the meta-stability condition if it happens.

cheers,
-roger

> 
> [1] http://www.ti.com/lit/er/sprz429h/sprz429h.pdf
> 

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>>>>>>>>>> We will need this function for a workaround.
>>>>>>>>>>>> The function issues a softreset only to the device
>>>>>>>>>>>> controller and performs minimal re-initialization
>>>>>>>>>>>> so that the device controller can be usable.
>>>>>>>>>>>>
>>>>>>>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>>>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>>>>>>>
>>>>>>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>>>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>>>>>>
>>>>>>>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>>>>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>>>>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>>>>>>>> was integrated without a superspeed PHY.
>>>>>>>>>>>
>>>>>>>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>>>>>>>
>>>>>>>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>>>>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>>>>>>>> controller to lower speed.
>>>>>>>>>>>
>>>>>>>>>>> John, can you check with your colleagues if we would ever fall into
>>>>>>>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>>>>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>>>>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>>>>>>>> PM for dwc3...
>>>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Felipe,
>>>>>>>>
>>>>>>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>>>>>>> I don't see an issue with that as long as we always ignore
>>>>>>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>>>>>>> versions of the core. As long as the DCFG.speed = SS, you should not
>>>>>>>> hit the STAR.
>>>>>>>
>>>>>>> I actually mean changing DCFG.speed during driver probe and never
>>>>>>> touching it again. Would that still cause problems ?
>>>>>>>
>>>>>>
>>>>>> In that case I'm not sure. The engineer who would know is off until
>>>>>> next week so I'll get back to you as soon as I can talk to him about
>>>>>> it.
>>>>>
>>>>
>>>> So the engineers said that this issue can occur while set to HS and
>>>> the run/stop bit is changed so it seems that won't work.
>>>
>>> Thanks John.
>>>
>>> Felipe,
>>>
>>> any suggestion how we can fix this upstream?
>> 
>> no idea, I don't have a lot of memory about this problem. I really don't
>> remember the details about this, is there an openly available errata
>> document which I could read ? /me goes search for it.
>> 
>> I found [1] which tells me, the following:
>> 
>> 
>> | i819        | A Device Control Bit Meta-Stability for USB3.0 Controller in USB2.0 Mode   |
>> |-------------+----------------------------------------------------------------------------|
>> | Criticality | Medium                                                                     |
>> |             |                                                                            |
>> | Descritiion | When USB3.0 controller core is programmed to be a USB 2.0-only device      |
>> |             | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing the core to     |
>> |             | attempt high speed as well as SuperSpeed connection or completely miss     |
>> |             | the attach request.                                                        |
>> |             |                                                                            |
>> | Workaround  | If the requirement is to always function in USB 2.0 mode, there is no      |
>> |             | workaround.                                                                |
>> |             | Otherwise, you can always program the USB controller core to be SuperSpeed |
>> |             | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4)                                    |
>> |             |                                                                            |
>> | Revisions   | SR 1.1, 2.0                                                                |
>> | Impacted    |                                                                            |
>> |-------------+----------------------------------------------------------------------------|
>> 
>> So, TI's own documentation says that there is _no_ workaround. My
>
> We are working on updating that document. Apparently Synopsis has suggested this workaround.
> pasting verbatim
>
> "
> -          As last step of device configuration we set the RUNSTOP bit in DCTL.
>
> -          Once we set the RUNSTOP bit, we need to monitor GDBGLTSSM for 100 ms until one of the two below happens:.
>
> 	o   We see the GDBGLTSSM.LTDB_LINK_STATE changing from 4
>
> 	o   We receive the USB 2.0 reset interrupt.
>
> If none of above happens then we can stop monitoring it.
>
> -          If state change from 4 occurs issue a SoftReset thru DCTL.CSftRst and reconfigure Device. This time it is guaranteed that no metastability will occur so no need to do the 100ms timeout.
> "

I don't have this text anywhere so I don't know. Is this something TI
came up with or Synopsys ? Unless I can see a document (preferrably from
Synopsys) stating this, I can't really accept $subject.

Another question is: if all it takes is an extra SoftReset, why don't we
just reset it during probe() if max_speed < SUPER and we're running on
rev < 2.20a ? BTW, which revision of the IP is on AM57x/DRA7x ?

>> question is, then: How are you sure that resetting the device actually
>> solves the issue ? Did you really hit the metastability problem and
>> noted that it works after a soft-reset ? How did you verify that
>
> I don't know if it solves the issue or not. It was suggested by
> Synopsis to TI's silicon team.

now that's a bummer ;-)

> I never hit the metastability problem detection condition in my
> overnight tests (i.e. LTDB_LINK_STATE != 4).

overnight is not enough. You need to keep this running for weeks.

> I have verified that things work after a soft-reset by faking that the
> error happens.

but if you never hit the actual failure, you have verified that it works
_without_ the workaround. I mean, if you can't be sure RUN/STOP went
metastable, you can't really be sure you're working around the Erratum.

>> Run/Stop was in a metastable state, considering that Run/Stop signal is
>> not visible outside the die ?
>
> LTDB_LINK_STATE != 4 within 100ms or RUNSTOP set is the condition to
> detect that the issue occurred.

this doesn't prove anything. This just means that your 100ms timer
expired. Unless you can verify that Run/Stop is in metastability, you
cannot be sure this workaround works.

Did anybody run silicon simulation to verify this case ? It's really the
only way to be sure.

>> It seems to me that resetting the IP is just as "dangerous" as setting
>> the IP to High-speed in the first place. No ?
>
> The soft-reset is just a recovery mechanism if that error is ever hit.

but you don't know if that's a *proper* recovery mechanism because you
never even *hit* the error.

> Putting the controller in reset state means it is in a known
> state. Why do you say it would be dangerous?

Because you can't predict the systems' behavior. If the flip-flop didn't
have time to settle into 0 or 1 state, you don't know what the
combinatorial part of the IP will do with that bogus value. It's truly
unpredictable. You also cannot know, for sure, that a SoftReset will be
enough to bring that flip-flop out of metastability.

> The original workaround i.e. setting the High-speed instance to
> Super-speed to avoid this errata is causing another side
> effect. i.e. erratic interrupts happen and more than 2 seconds delay

this should have been an expected side-effect when you design a
SuperSpeed controller without a SuperSpeed PHY and don't properly
terminate inputs. What you have is a floating PIPE3 interface not
properly terminated and capturing random noise (basically acting as a
very poor antenna inside your die). Of course the IP will go bonkers and
give you "erratic error" interrupts. It has no idea what the hell this
"PHY" on the PIPE3 interface is doing.

> to enumerations. This problem is more serious and so we have to keep
> the controller in High-speed and tackle the meta-stability condition
> if it happens.

you have to tackle the meta-stability, sure, but we need guarantee that
$subject IS indeed tackling that problem. Unless there's proof that this
really solves the meta-stability issue, I won't take $subject. Sorry
dude, but I don't want regressions elsewhere because of a badly
validated patch.

Bottomline, it's not enough to _state_ that it solves the problem. You
need to first *trigger* the issue without the workaround, then apply
workaround and trigger it again. Then, and only then, you can be certain
you're fixing the problem.

After that, we will look into how to make sure this has no impact to
other users.

-- 
balbi

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

RE: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Shankar, Abhishek]

Roger
I am sorry but this mail chain is unreadable!! I am not able to filter out the >>>...
Please send me a clean version of the mail if possible...i will be unable to respond without that..

---Abhishek

-----Original Message-----
From: Felipe Balbi [mailto:balbi@kernel.org] 
Sent: Monday, April 04, 2016 3:11 AM
To: Quadros, Roger; John Youn
Cc: Nori, Sekhar; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Shankar, Abhishek; B, Ravi
Subject: Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>>>>>>>>>> We will need this function for a workaround.
>>>>>>>>>>>> The function issues a softreset only to the device 
>>>>>>>>>>>> controller and performs minimal re-initialization so that 
>>>>>>>>>>>> the device controller can be usable.
>>>>>>>>>>>>
>>>>>>>>>>>> As some code is similar to dwc3_core_init() take out common 
>>>>>>>>>>>> code into dwc3_get_gctl_quirks().
>>>>>>>>>>>>
>>>>>>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to keep 
>>>>>>>>>>>> track of the current mode in the PRTCAPDIR register.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>>>>>>
>>>>>>>>>>> I must say, I don't like this at all :-p There's ONE known 
>>>>>>>>>>> silicon which needs this because of a poor silicon 
>>>>>>>>>>> integration which took an IP with a known erratum where it 
>>>>>>>>>>> can't be made to work on lower speeds and STILL was integrated without a superspeed PHY.
>>>>>>>>>>>
>>>>>>>>>>> There's a reason why I never tried to push this upstream 
>>>>>>>>>>> myself ;-)
>>>>>>>>>>>
>>>>>>>>>>> I'm really thinking we might be better off adding a quirk 
>>>>>>>>>>> flag to skip the metastability workaround and allow this ONE 
>>>>>>>>>>> silicon to set the controller to lower speed.
>>>>>>>>>>>
>>>>>>>>>>> John, can you check with your colleagues if we would ever 
>>>>>>>>>>> fall into
>>>>>>>>>>> STAR#9000525659 if we set maximum speed to high speed during 
>>>>>>>>>>> driver probe and never touch it again ? I would assume we 
>>>>>>>>>>> don't really fall into the metastability workaround, right ? 
>>>>>>>>>>> We're not doing any sort of PM for dwc3...
>>>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Felipe,
>>>>>>>>
>>>>>>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>>>>>>> I don't see an issue with that as long as we always ignore
>>>>>>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>>>>>>> versions of the core. As long as the DCFG.speed = SS, you 
>>>>>>>> should not hit the STAR.
>>>>>>>
>>>>>>> I actually mean changing DCFG.speed during driver probe and 
>>>>>>> never touching it again. Would that still cause problems ?
>>>>>>>
>>>>>>
>>>>>> In that case I'm not sure. The engineer who would know is off 
>>>>>> until next week so I'll get back to you as soon as I can talk to 
>>>>>> him about it.
>>>>>
>>>>
>>>> So the engineers said that this issue can occur while set to HS and 
>>>> the run/stop bit is changed so it seems that won't work.
>>>
>>> Thanks John.
>>>
>>> Felipe,
>>>
>>> any suggestion how we can fix this upstream?
>> 
>> no idea, I don't have a lot of memory about this problem. I really 
>> don't remember the details about this, is there an openly available 
>> errata document which I could read ? /me goes search for it.
>> 
>> I found [1] which tells me, the following:
>> 
>> 
>> | i819        | A Device Control Bit Meta-Stability for USB3.0 Controller in USB2.0 Mode   |
>> |-------------+----------------------------------------------------------------------------|
>> | Criticality | Medium                                                                     |
>> |             |                                                                            |
>> | Descritiion | When USB3.0 controller core is programmed to be a USB 2.0-only device      |
>> |             | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing the core to     |
>> |             | attempt high speed as well as SuperSpeed connection or completely miss     |
>> |             | the attach request.                                                        |
>> |             |                                                                            |
>> | Workaround  | If the requirement is to always function in USB 2.0 mode, there is no      |
>> |             | workaround.                                                                |
>> |             | Otherwise, you can always program the USB controller core to be SuperSpeed |
>> |             | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4)                                    |
>> |             |                                                                            |
>> | Revisions   | SR 1.1, 2.0                                                                |
>> | Impacted    |                                                                            |
>> |-------------+----------------------------------------------------------------------------|
>> 
>> So, TI's own documentation says that there is _no_ workaround. My
>
> We are working on updating that document. Apparently Synopsis has suggested this workaround.
> pasting verbatim
>
> "
> -          As last step of device configuration we set the RUNSTOP bit in DCTL.
>
> -          Once we set the RUNSTOP bit, we need to monitor GDBGLTSSM for 100 ms until one of the two below happens:.
>
> 	o   We see the GDBGLTSSM.LTDB_LINK_STATE changing from 4
>
> 	o   We receive the USB 2.0 reset interrupt.
>
> If none of above happens then we can stop monitoring it.
>
> -          If state change from 4 occurs issue a SoftReset thru DCTL.CSftRst and reconfigure Device. This time it is guaranteed that no metastability will occur so no need to do the 100ms timeout.
> "

I don't have this text anywhere so I don't know. Is this something TI came up with or Synopsys ? Unless I can see a document (preferrably from
Synopsys) stating this, I can't really accept $subject.

Another question is: if all it takes is an extra SoftReset, why don't we just reset it during probe() if max_speed < SUPER and we're running on rev < 2.20a ? BTW, which revision of the IP is on AM57x/DRA7x ?

>> question is, then: How are you sure that resetting the device 
>> actually solves the issue ? Did you really hit the metastability 
>> problem and noted that it works after a soft-reset ? How did you 
>> verify that
>
> I don't know if it solves the issue or not. It was suggested by 
> Synopsis to TI's silicon team.

now that's a bummer ;-)

> I never hit the metastability problem detection condition in my 
> overnight tests (i.e. LTDB_LINK_STATE != 4).

overnight is not enough. You need to keep this running for weeks.

> I have verified that things work after a soft-reset by faking that the 
> error happens.

but if you never hit the actual failure, you have verified that it works _without_ the workaround. I mean, if you can't be sure RUN/STOP went metastable, you can't really be sure you're working around the Erratum.

>> Run/Stop was in a metastable state, considering that Run/Stop signal 
>> is not visible outside the die ?
>
> LTDB_LINK_STATE != 4 within 100ms or RUNSTOP set is the condition to 
> detect that the issue occurred.

this doesn't prove anything. This just means that your 100ms timer expired. Unless you can verify that Run/Stop is in metastability, you cannot be sure this workaround works.

Did anybody run silicon simulation to verify this case ? It's really the only way to be sure.

>> It seems to me that resetting the IP is just as "dangerous" as 
>> setting the IP to High-speed in the first place. No ?
>
> The soft-reset is just a recovery mechanism if that error is ever hit.

but you don't know if that's a *proper* recovery mechanism because you never even *hit* the error.

> Putting the controller in reset state means it is in a known state. 
> Why do you say it would be dangerous?

Because you can't predict the systems' behavior. If the flip-flop didn't have time to settle into 0 or 1 state, you don't know what the combinatorial part of the IP will do with that bogus value. It's truly unpredictable. You also cannot know, for sure, that a SoftReset will be enough to bring that flip-flop out of metastability.

> The original workaround i.e. setting the High-speed instance to 
> Super-speed to avoid this errata is causing another side effect. i.e. 
> erratic interrupts happen and more than 2 seconds delay

this should have been an expected side-effect when you design a SuperSpeed controller without a SuperSpeed PHY and don't properly terminate inputs. What you have is a floating PIPE3 interface not properly terminated and capturing random noise (basically acting as a very poor antenna inside your die). Of course the IP will go bonkers and give you "erratic error" interrupts. It has no idea what the hell this "PHY" on the PIPE3 interface is doing.

> to enumerations. This problem is more serious and so we have to keep 
> the controller in High-speed and tackle the meta-stability condition 
> if it happens.

you have to tackle the meta-stability, sure, but we need guarantee that $subject IS indeed tackling that problem. Unless there's proof that this really solves the meta-stability issue, I won't take $subject. Sorry dude, but I don't want regressions elsewhere because of a badly validated patch.

Bottomline, it's not enough to _state_ that it solves the problem. You need to first *trigger* the issue without the workaround, then apply workaround and trigger it again. Then, and only then, you can be certain you're fixing the problem.

After that, we will look into how to make sure this has no impact to other users.

--
balbi

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Roger Quadros]

On 04/04/16 11:10, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>>>>>>>>>>> We will need this function for a workaround.
>>>>>>>>>>>>> The function issues a softreset only to the device
>>>>>>>>>>>>> controller and performs minimal re-initialization
>>>>>>>>>>>>> so that the device controller can be usable.
>>>>>>>>>>>>>
>>>>>>>>>>>>> As some code is similar to dwc3_core_init() take out
>>>>>>>>>>>>> common code into dwc3_get_gctl_quirks().
>>>>>>>>>>>>>
>>>>>>>>>>>>> We add a new member (prtcap_mode) to struct dwc3 to
>>>>>>>>>>>>> keep track of the current mode in the PRTCAPDIR register.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>>>>>>>
>>>>>>>>>>>> I must say, I don't like this at all :-p There's ONE known silicon which
>>>>>>>>>>>> needs this because of a poor silicon integration which took an IP with a
>>>>>>>>>>>> known erratum where it can't be made to work on lower speeds and STILL
>>>>>>>>>>>> was integrated without a superspeed PHY.
>>>>>>>>>>>>
>>>>>>>>>>>> There's a reason why I never tried to push this upstream myself ;-)
>>>>>>>>>>>>
>>>>>>>>>>>> I'm really thinking we might be better off adding a quirk flag to skip
>>>>>>>>>>>> the metastability workaround and allow this ONE silicon to set the
>>>>>>>>>>>> controller to lower speed.
>>>>>>>>>>>>
>>>>>>>>>>>> John, can you check with your colleagues if we would ever fall into
>>>>>>>>>>>> STAR#9000525659 if we set maximum speed to high speed during driver
>>>>>>>>>>>> probe and never touch it again ? I would assume we don't really fall
>>>>>>>>>>>> into the metastability workaround, right ? We're not doing any sort of
>>>>>>>>>>>> PM for dwc3...
>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Felipe,
>>>>>>>>>
>>>>>>>>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS?
>>>>>>>>> I don't see an issue with that as long as we always ignore
>>>>>>>>> dwc->maximum_speed when programming DCFG.speed for all affected
>>>>>>>>> versions of the core. As long as the DCFG.speed = SS, you should not
>>>>>>>>> hit the STAR.
>>>>>>>>
>>>>>>>> I actually mean changing DCFG.speed during driver probe and never
>>>>>>>> touching it again. Would that still cause problems ?
>>>>>>>>
>>>>>>>
>>>>>>> In that case I'm not sure. The engineer who would know is off until
>>>>>>> next week so I'll get back to you as soon as I can talk to him about
>>>>>>> it.
>>>>>>
>>>>>
>>>>> So the engineers said that this issue can occur while set to HS and
>>>>> the run/stop bit is changed so it seems that won't work.
>>>>
>>>> Thanks John.
>>>>
>>>> Felipe,
>>>>
>>>> any suggestion how we can fix this upstream?
>>>
>>> no idea, I don't have a lot of memory about this problem. I really don't
>>> remember the details about this, is there an openly available errata
>>> document which I could read ? /me goes search for it.
>>>
>>> I found [1] which tells me, the following:
>>>
>>>
>>> | i819        | A Device Control Bit Meta-Stability for USB3.0 Controller in USB2.0 Mode   |
>>> |-------------+----------------------------------------------------------------------------|
>>> | Criticality | Medium                                                                     |
>>> |             |                                                                            |
>>> | Descritiion | When USB3.0 controller core is programmed to be a USB 2.0-only device      |
>>> |             | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing the core to     |
>>> |             | attempt high speed as well as SuperSpeed connection or completely miss     |
>>> |             | the attach request.                                                        |
>>> |             |                                                                            |
>>> | Workaround  | If the requirement is to always function in USB 2.0 mode, there is no      |
>>> |             | workaround.                                                                |
>>> |             | Otherwise, you can always program the USB controller core to be SuperSpeed |
>>> |             | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4)                                    |
>>> |             |                                                                            |
>>> | Revisions   | SR 1.1, 2.0                                                                |
>>> | Impacted    |                                                                            |
>>> |-------------+----------------------------------------------------------------------------|
>>>
>>> So, TI's own documentation says that there is _no_ workaround. My
>>
>> We are working on updating that document. Apparently Synopsis has suggested this workaround.
>> pasting verbatim
>>
>> "
>> -          As last step of device configuration we set the RUNSTOP bit in DCTL.
>>
>> -          Once we set the RUNSTOP bit, we need to monitor GDBGLTSSM for 100 ms until one of the two below happens:.
>>
>> 	o   We see the GDBGLTSSM.LTDB_LINK_STATE changing from 4
>>
>> 	o   We receive the USB 2.0 reset interrupt.
>>
>> If none of above happens then we can stop monitoring it.
>>
>> -          If state change from 4 occurs issue a SoftReset thru DCTL.CSftRst and reconfigure Device. This time it is guaranteed that no metastability will occur so no need to do the 100ms timeout.
>> "
> 
> I don't have this text anywhere so I don't know. Is this something TI
> came up with or Synopsys ? Unless I can see a document (preferrably from
> Synopsys) stating this, I can't really accept $subject.

OK. I'll try to find out if there is an official document about this.

> 
> Another question is: if all it takes is an extra SoftReset, why don't we
> just reset it during probe() if max_speed < SUPER and we're running on
> rev < 2.20a ? BTW, which revision of the IP is on AM57x/DRA7x ?

The issue might happen on any Run/Stop transition so not sure if doing it
SoftReset just at probe fixes anything.

On DRA7x it is rev 2.02a.

> 
>>> question is, then: How are you sure that resetting the device actually
>>> solves the issue ? Did you really hit the metastability problem and
>>> noted that it works after a soft-reset ? How did you verify that
>>
>> I don't know if it solves the issue or not. It was suggested by
>> Synopsis to TI's silicon team.
> 
> now that's a bummer ;-)
> 
>> I never hit the metastability problem detection condition in my
>> overnight tests (i.e. LTDB_LINK_STATE != 4).
> 
> overnight is not enough. You need to keep this running for weeks.

how many weeks is acceptable for you? I can run for that long, no problem.
And what if the issue doesn't happen in that time frame, would you still
consider this case?

> 
>> I have verified that things work after a soft-reset by faking that the
>> error happens.
> 
> but if you never hit the actual failure, you have verified that it works
> _without_ the workaround. I mean, if you can't be sure RUN/STOP went
> metastable, you can't really be sure you're working around the Erratum.
> 
>>> Run/Stop was in a metastable state, considering that Run/Stop signal is
>>> not visible outside the die ?
>>
>> LTDB_LINK_STATE != 4 within 100ms or RUNSTOP set is the condition to
>> detect that the issue occurred.
> 
> this doesn't prove anything. This just means that your 100ms timer
> expired. Unless you can verify that Run/Stop is in metastability, you
> cannot be sure this workaround works.
> 
> Did anybody run silicon simulation to verify this case ? It's really the
> only way to be sure.

AFAIK this wasn't reproducible during silicon simulation either.

> 
>>> It seems to me that resetting the IP is just as "dangerous" as setting
>>> the IP to High-speed in the first place. No ?
>>
>> The soft-reset is just a recovery mechanism if that error is ever hit.
> 
> but you don't know if that's a *proper* recovery mechanism because you
> never even *hit* the error.
> 
>> Putting the controller in reset state means it is in a known
>> state. Why do you say it would be dangerous?
> 
> Because you can't predict the systems' behavior. If the flip-flop didn't
> have time to settle into 0 or 1 state, you don't know what the
> combinatorial part of the IP will do with that bogus value. It's truly
> unpredictable. You also cannot know, for sure, that a SoftReset will be
> enough to bring that flip-flop out of metastability.

I'm not an expert in this area and can only follow the advice the Silicon team gives.

> 
>> The original workaround i.e. setting the High-speed instance to
>> Super-speed to avoid this errata is causing another side
>> effect. i.e. erratic interrupts happen and more than 2 seconds delay
> 
> this should have been an expected side-effect when you design a
> SuperSpeed controller without a SuperSpeed PHY and don't properly
> terminate inputs. What you have is a floating PIPE3 interface not
> properly terminated and capturing random noise (basically acting as a
> very poor antenna inside your die). Of course the IP will go bonkers and
> give you "erratic error" interrupts. It has no idea what the hell this
> "PHY" on the PIPE3 interface is doing.

We know that. The damage is already done. :)

> 
>> to enumerations. This problem is more serious and so we have to keep
>> the controller in High-speed and tackle the meta-stability condition
>> if it happens.
> 
> you have to tackle the meta-stability, sure, but we need guarantee that
> $subject IS indeed tackling that problem. Unless there's proof that this
> really solves the meta-stability issue, I won't take $subject. Sorry
> dude, but I don't want regressions elsewhere because of a badly
> validated patch.

I understand. I will see if someone from TI can provide me official documentation
about the workaround.

> 
> Bottomline, it's not enough to _state_ that it solves the problem. You
> need to first *trigger* the issue without the workaround, then apply
> workaround and trigger it again. Then, and only then, you can be certain
> you're fixing the problem.
> 
> After that, we will look into how to make sure this has no impact to
> other users.
> 

OK, Thanks.

cheers,
-roger

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:

<snip>

>> I don't have this text anywhere so I don't know. Is this something TI
>> came up with or Synopsys ? Unless I can see a document (preferrably from
>> Synopsys) stating this, I can't really accept $subject.
>
> OK. I'll try to find out if there is an official document about this.
>
>> 
>> Another question is: if all it takes is an extra SoftReset, why don't we
>> just reset it during probe() if max_speed < SUPER and we're running on
>> rev < 2.20a ? BTW, which revision of the IP is on AM57x/DRA7x ?
>
> The issue might happen on any Run/Stop transition so not sure if doing it
> SoftReset just at probe fixes anything.
>
> On DRA7x it is rev 2.02a.

oh, same block as OMAP5 ES2.0 :-(

>>>> question is, then: How are you sure that resetting the device actually
>>>> solves the issue ? Did you really hit the metastability problem and
>>>> noted that it works after a soft-reset ? How did you verify that
>>>
>>> I don't know if it solves the issue or not. It was suggested by
>>> Synopsis to TI's silicon team.
>> 
>> now that's a bummer ;-)
>> 
>>> I never hit the metastability problem detection condition in my
>>> overnight tests (i.e. LTDB_LINK_STATE != 4).
>> 
>> overnight is not enough. You need to keep this running for weeks.
>
> how many weeks is acceptable for you? I can run for that long, no problem.
> And what if the issue doesn't happen in that time frame, would you still
> consider this case?

Well, there's always the possibility we have never triggered the issue
to start with :-) What happens if you remove the the current workaround,
set maximum-speed to high-speed and constantly toggle run/stop bit
(there's a soft-connect file under the UDC's directory in sysfs). Can
you ever cause the problems ?

>>>> Run/Stop was in a metastable state, considering that Run/Stop signal is
>>>> not visible outside the die ?
>>>
>>> LTDB_LINK_STATE != 4 within 100ms or RUNSTOP set is the condition to
>>> detect that the issue occurred.
>> 
>> this doesn't prove anything. This just means that your 100ms timer
>> expired. Unless you can verify that Run/Stop is in metastability, you
>> cannot be sure this workaround works.
>> 
>> Did anybody run silicon simulation to verify this case ? It's really the
>> only way to be sure.
>
> AFAIK this wasn't reproducible during silicon simulation either.

now this is a big problem. We just don't know if $subject is really
avoiding the problem ;-) Unless we can trigger the problem, we can't be
sure. We are, however, sure that current workaround avoids the problem
completely.

>>>> It seems to me that resetting the IP is just as "dangerous" as setting
>>>> the IP to High-speed in the first place. No ?
>>>
>>> The soft-reset is just a recovery mechanism if that error is ever hit.
>> 
>> but you don't know if that's a *proper* recovery mechanism because you
>> never even *hit* the error.
>> 
>>> Putting the controller in reset state means it is in a known
>>> state. Why do you say it would be dangerous?
>> 
>> Because you can't predict the systems' behavior. If the flip-flop didn't
>> have time to settle into 0 or 1 state, you don't know what the
>> combinatorial part of the IP will do with that bogus value. It's truly
>> unpredictable. You also cannot know, for sure, that a SoftReset will be
>> enough to bring that flip-flop out of metastability.
>
> I'm not an expert in this area and can only follow the advice the
> Silicon team gives.

fair enough. But you must understand we can't just accept anything even
if we never trigger we problems. Unless we're certain about the fix,
without a shadow of a doubt, we might be creating a very, very hard to
debug regression which might end up with sales drop and what not. It's
the kinda thing that we all must be concerned about ;-)

>>> The original workaround i.e. setting the High-speed instance to
>>> Super-speed to avoid this errata is causing another side
>>> effect. i.e. erratic interrupts happen and more than 2 seconds delay
>> 
>> this should have been an expected side-effect when you design a
>> SuperSpeed controller without a SuperSpeed PHY and don't properly
>> terminate inputs. What you have is a floating PIPE3 interface not
>> properly terminated and capturing random noise (basically acting as a
>> very poor antenna inside your die). Of course the IP will go bonkers and
>> give you "erratic error" interrupts. It has no idea what the hell this
>> "PHY" on the PIPE3 interface is doing.
>
> We know that. The damage is already done. :)

right, and I'm trying to avoid further damage caused by a fix that
hasn't been properly validated :)

>>> to enumerations. This problem is more serious and so we have to keep
>>> the controller in High-speed and tackle the meta-stability condition
>>> if it happens.
>> 
>> you have to tackle the meta-stability, sure, but we need guarantee that
>> $subject IS indeed tackling that problem. Unless there's proof that this
>> really solves the meta-stability issue, I won't take $subject. Sorry
>> dude, but I don't want regressions elsewhere because of a badly
>> validated patch.
>
> I understand. I will see if someone from TI can provide me official
> documentation about the workaround.

thank you

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Roger Quadros]

On 11/04/16 15:51, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
> 
> <snip>
> 
>>> I don't have this text anywhere so I don't know. Is this something TI
>>> came up with or Synopsys ? Unless I can see a document (preferrably from
>>> Synopsys) stating this, I can't really accept $subject.
>>
>> OK. I'll try to find out if there is an official document about this.
>>
>>>
>>> Another question is: if all it takes is an extra SoftReset, why don't we
>>> just reset it during probe() if max_speed < SUPER and we're running on
>>> rev < 2.20a ? BTW, which revision of the IP is on AM57x/DRA7x ?
>>
>> The issue might happen on any Run/Stop transition so not sure if doing it
>> SoftReset just at probe fixes anything.
>>
>> On DRA7x it is rev 2.02a.
> 
> oh, same block as OMAP5 ES2.0 :-(
> 
>>>>> question is, then: How are you sure that resetting the device actually
>>>>> solves the issue ? Did you really hit the metastability problem and
>>>>> noted that it works after a soft-reset ? How did you verify that
>>>>
>>>> I don't know if it solves the issue or not. It was suggested by
>>>> Synopsis to TI's silicon team.
>>>
>>> now that's a bummer ;-)
>>>
>>>> I never hit the metastability problem detection condition in my
>>>> overnight tests (i.e. LTDB_LINK_STATE != 4).
>>>
>>> overnight is not enough. You need to keep this running for weeks.
>>
>> how many weeks is acceptable for you? I can run for that long, no problem.
>> And what if the issue doesn't happen in that time frame, would you still
>> consider this case?
> 
> Well, there's always the possibility we have never triggered the issue
> to start with :-) What happens if you remove the the current workaround,
> set maximum-speed to high-speed and constantly toggle run/stop bit
> (there's a soft-connect file under the UDC's directory in sysfs). Can
> you ever cause the problems ?

I had tried a with max-speed set to high-speed and a script that just reloads g_zero.
That should toggle Run/Stop right?
Running this overnight didn't cause any problems.

cheers,
-roger

Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>> how many weeks is acceptable for you? I can run for that long, no problem.
>>> And what if the issue doesn't happen in that time frame, would you still
>>> consider this case?
>> 
>> Well, there's always the possibility we have never triggered the issue
>> to start with :-) What happens if you remove the the current workaround,
>> set maximum-speed to high-speed and constantly toggle run/stop bit
>> (there's a soft-connect file under the UDC's directory in sysfs). Can
>> you ever cause the problems ?
>
> I had tried a with max-speed set to high-speed and a script that just
> reloads g_zero.
>
> That should toggle Run/Stop right?

yes it should.

> Running this overnight didn't cause any problems.

as you can see, it's a difficult problem to trigger :-)

-- 
balbi

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

[PATCH 2/2] usb: dwc3: gadget: usb: dwc3: run/stop metastability workaround

[Roger Quadros]

The existing workaround of forcing DEVSPD to SUPER_SPEED
for HIGH_SPEED ports is causing another side effect
which causes erratic interrupts and delayed gadget
enumeration of upto 2 seconds.

Work around the run/stop issue by detecting if
it happened using debug LTSSM state and issuing
soft reset to the device controller when changing RUN_STOP
from 0 to 1.

We apply the workaround only if PRTCAP is DEVICE mode
as we don't yet support this workaround in OTG mode.

Use USB RESET event as exit condition for workaround.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.h   |   1 +
 drivers/usb/dwc3/gadget.c | 175 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 144 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2bea1ac..a724c0d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -762,6 +762,7 @@ struct dwc3 {
 
 	struct usb_gadget	gadget;
 	struct usb_gadget_driver *gadget_driver;
+	struct completion	reset_event; /* used for run/stop workaround */
 
 	struct usb_phy		*usb2_phy;
 	struct usb_phy		*usb3_phy;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3ac170f..03418b8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -35,6 +35,9 @@
 #include "gadget.h"
 #include "io.h"
 
+static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
+static int dwc3_gadget_restart(struct dwc3 *dwc);
+
 /**
  * dwc3_gadget_set_test_mode - Enables USB2 Test Modes
  * @dwc: pointer to our context structure
@@ -1570,13 +1573,100 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	struct dwc3		*dwc = gadget_to_dwc(g);
 	unsigned long		flags;
 	int			ret;
+	int			trys = 0;
 
 	is_on = !!is_on;
 
+	if (is_on)
+		reinit_completion(&dwc->reset_event);
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	ret = dwc3_gadget_run_stop(dwc, is_on, false);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+try:
+	/**
+	 * WORKAROUND: DWC3 revision < 2.20a have an issue
+	 * which would cause metastability state on Run/Stop
+	 * bit if we try to force the IP to USB2-only mode.
+	 *
+	 * Because of that, we check if we hit that issue and
+	 * reset core and retry if we did.
+	 *
+	 * We only attempt this workaround if we are in
+	 * DEVICE mode (i.e. not OTG).
+	 *
+	 * Refers to:
+	 *
+	 * STAR#9000525659: Clock Domain Crossing on DCTL in
+	 * USB 2.0 Mode
+	 */
+	if (is_on && dwc->revision < DWC3_REVISION_220A &&
+	    dwc->prtcap_mode == DWC3_GCTL_PRTCAP_DEVICE) {
+		u32 devspd, ltssm;
+		unsigned long t;
+
+		/* only applicable if devspd != SUPERSPEED */
+		devspd = dwc3_readl(dwc->regs, DWC3_DCFG) & DWC3_DCFG_SPEED_MASK;
+		if (devspd == DWC3_DCFG_SUPERSPEED)
+			goto done;
+
+		/* get link state */
+		ltssm = dwc3_readl(dwc->regs, DWC3_GDBGLTSSM);
+		ltssm = (ltssm >> 22) & 0xf;
+
+		/**
+		 * Need to wait for 100ms and check if ltssm != 4 to detect
+		 * metastability issue. If we got a reset event then we are
+		 * safe and can continue.
+		 */
+		t = wait_for_completion_timeout(&dwc->reset_event,
+						msecs_to_jiffies(100));
+		if (t)
+			goto done;
+
+		/**
+		 * If link state != 4 we've hit the metastability issue, soft reset.
+		 */
+		if (ltssm == 4)
+			goto done;
+
+		dwc3_trace(trace_dwc3_gadget,
+			   "applying metastability workaround\n");
+		trys++;
+		if (trys == 2) {
+			dev_WARN_ONCE(dwc->dev, true,
+				      "metastability workaround failed!\n");
+			return -ETIMEDOUT;
+		}
+
+		spin_lock_irqsave(&dwc->lock, flags);
+		/* stop gadget */
+		dwc3_gadget_disable_irq(dwc);
+		__dwc3_gadget_ep_disable(dwc->eps[0]);
+		__dwc3_gadget_ep_disable(dwc->eps[1]);
+
+		/* soft reset device and restart */
+		ret = dwc3_device_reinit(dwc);
+		if (ret) {
+			dev_err(dwc->dev, "device reinit failed\n");
+			return ret;
+		}
+
+		reinit_completion(&dwc->reset_event);
+		/* restart gadget */
+		ret = dwc3_gadget_restart(dwc);
+		if (ret) {
+			dev_err(dwc->dev, "failed to re-init gadget\n");
+			return ret;
+		}
+
+		spin_unlock_irqrestore(&dwc->lock, flags);
+		goto try;
+	}
+
+done:
+
 	return ret;
 }
 
@@ -1607,37 +1697,12 @@ static void dwc3_gadget_disable_irq(struct dwc3 *dwc)
 static irqreturn_t dwc3_interrupt(int irq, void *_dwc);
 static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc);
 
-static int dwc3_gadget_start(struct usb_gadget *g,
-		struct usb_gadget_driver *driver)
+static int dwc3_gadget_restart(struct dwc3 *dwc)
 {
-	struct dwc3		*dwc = gadget_to_dwc(g);
 	struct dwc3_ep		*dep;
-	unsigned long		flags;
 	int			ret = 0;
-	int			irq;
 	u32			reg;
 
-	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
-	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
-			IRQF_SHARED, "dwc3", dwc);
-	if (ret) {
-		dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
-				irq, ret);
-		goto err0;
-	}
-
-	spin_lock_irqsave(&dwc->lock, flags);
-
-	if (dwc->gadget_driver) {
-		dev_err(dwc->dev, "%s is already bound to %s\n",
-				dwc->gadget.name,
-				dwc->gadget_driver->driver.name);
-		ret = -EBUSY;
-		goto err1;
-	}
-
-	dwc->gadget_driver	= driver;
-
 	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
 	reg &= ~(DWC3_DCFG_SPEED_MASK);
 
@@ -1649,12 +1714,15 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	 * Because of that, we cannot configure the IP to any
 	 * speed other than the SuperSpeed
 	 *
+	 * For non OTG mode we can attempt softreset workaround.
+	 *
 	 * Refers to:
 	 *
 	 * STAR#9000525659: Clock Domain Crossing on DCTL in
 	 * USB 2.0 Mode
 	 */
-	if (dwc->revision < DWC3_REVISION_220A) {
+	if ((dwc->revision < DWC3_REVISION_220A) &&
+	     (dwc->prtcap_mode == DWC3_GCTL_PRTCAP_OTG)) {
 		reg |= DWC3_DCFG_SUPERSPEED;
 	} else {
 		switch (dwc->maximum_speed) {
@@ -1689,7 +1757,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 			false);
 	if (ret) {
 		dev_err(dwc->dev, "failed to enable %s\n", dep->name);
-		goto err2;
+		return ret;
 	}
 
 	dep = dwc->eps[1];
@@ -1697,7 +1765,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 			false);
 	if (ret) {
 		dev_err(dwc->dev, "failed to enable %s\n", dep->name);
-		goto err3;
+		goto err;
 	}
 
 	/* begin to receive SETUP packets */
@@ -1706,12 +1774,50 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 
 	dwc3_gadget_enable_irq(dwc);
 
-	spin_unlock_irqrestore(&dwc->lock, flags);
-
 	return 0;
 
-err3:
+err:
 	__dwc3_gadget_ep_disable(dwc->eps[0]);
+	return ret;
+}
+
+static int dwc3_gadget_start(struct usb_gadget *g,
+		struct usb_gadget_driver *driver)
+{
+	struct dwc3		*dwc = gadget_to_dwc(g);
+	unsigned long		flags;
+	int			ret = 0;
+	int			irq;
+
+	irq = platform_get_irq(to_platform_device(dwc->dev), 0);
+	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
+			IRQF_SHARED, "dwc3", dwc);
+	if (ret) {
+		dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
+				irq, ret);
+		goto err0;
+	}
+
+	spin_lock_irqsave(&dwc->lock, flags);
+
+	if (dwc->gadget_driver) {
+		dev_err(dwc->dev, "%s is already bound to %s\n",
+				dwc->gadget.name,
+				dwc->gadget_driver->driver.name);
+		ret = -EBUSY;
+		goto err1;
+	}
+
+	dwc->gadget_driver	= driver;
+
+	ret = dwc3_gadget_restart(dwc);
+	if (ret) {
+		dev_err(dwc->dev, "gadget start failed\n");
+		goto err2;
+	}
+
+	spin_unlock_irqrestore(&dwc->lock, flags);
+	return 0;
 
 err2:
 	dwc->gadget_driver = NULL;
@@ -2321,6 +2427,9 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
 			dwc3_gadget_disconnect_interrupt(dwc);
 	}
 
+	/* notify run/stop metastability workaround */
+	complete(&dwc->reset_event);
+
 	dwc3_reset_gadget(dwc);
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
@@ -2868,6 +2977,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 	 */
 	dwc->gadget.quirk_ep_out_aligned_size = true;
 
+	init_completion(&dwc->reset_event);
+
 	/*
 	 * REVISIT: Here we should clear all pending IRQs to be
 	 * sure we're starting from a well known location.
-- 
2.5.0

Re: [PATCH 2/2] usb: dwc3: gadget: usb: dwc3: run/stop metastability workaround

[Felipe Balbi]

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

Roger Quadros <rogerq@ti.com> writes:

> [ text/plain ]
> The existing workaround of forcing DEVSPD to SUPER_SPEED
> for HIGH_SPEED ports is causing another side effect
> which causes erratic interrupts and delayed gadget
> enumeration of upto 2 seconds.
>
> Work around the run/stop issue by detecting if
> it happened using debug LTSSM state and issuing
> soft reset to the device controller when changing RUN_STOP
> from 0 to 1.
>
> We apply the workaround only if PRTCAP is DEVICE mode
> as we don't yet support this workaround in OTG mode.
>
> Use USB RESET event as exit condition for workaround.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/core.h   |   1 +
>  drivers/usb/dwc3/gadget.c | 175 +++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 144 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2bea1ac..a724c0d 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -762,6 +762,7 @@ struct dwc3 {
>  
>  	struct usb_gadget	gadget;
>  	struct usb_gadget_driver *gadget_driver;
> +	struct completion	reset_event; /* used for run/stop workaround */

the fact that you needed a struct completion here already points to the
fact this is really, really bad :-)

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3ac170f..03418b8 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -35,6 +35,9 @@
>  #include "gadget.h"
>  #include "io.h"
>  
> +static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
> +static int dwc3_gadget_restart(struct dwc3 *dwc);
> +
>  /**
>   * dwc3_gadget_set_test_mode - Enables USB2 Test Modes
>   * @dwc: pointer to our context structure
> @@ -1570,13 +1573,100 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	struct dwc3		*dwc = gadget_to_dwc(g);
>  	unsigned long		flags;
>  	int			ret;
> +	int			trys = 0;
>  
>  	is_on = !!is_on;
>  
> +	if (is_on)
> +		reinit_completion(&dwc->reset_event);
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	ret = dwc3_gadget_run_stop(dwc, is_on, false);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +try:
> +	/**
> +	 * WORKAROUND: DWC3 revision < 2.20a have an issue
> +	 * which would cause metastability state on Run/Stop
> +	 * bit if we try to force the IP to USB2-only mode.
> +	 *
> +	 * Because of that, we check if we hit that issue and
> +	 * reset core and retry if we did.
> +	 *
> +	 * We only attempt this workaround if we are in
> +	 * DEVICE mode (i.e. not OTG).
> +	 *
> +	 * Refers to:
> +	 *
> +	 * STAR#9000525659: Clock Domain Crossing on DCTL in
> +	 * USB 2.0 Mode
> +	 */
> +	if (is_on && dwc->revision < DWC3_REVISION_220A &&
> +	    dwc->prtcap_mode == DWC3_GCTL_PRTCAP_DEVICE) {
> +		u32 devspd, ltssm;
> +		unsigned long t;
> +
> +		/* only applicable if devspd != SUPERSPEED */
> +		devspd = dwc3_readl(dwc->regs, DWC3_DCFG) & DWC3_DCFG_SPEED_MASK;
> +		if (devspd == DWC3_DCFG_SUPERSPEED)
> +			goto done;
> +
> +		/* get link state */
> +		ltssm = dwc3_readl(dwc->regs, DWC3_GDBGLTSSM);
> +		ltssm = (ltssm >> 22) & 0xf;
> +
> +		/**
> +		 * Need to wait for 100ms and check if ltssm != 4 to detect
> +		 * metastability issue. If we got a reset event then we are
> +		 * safe and can continue.
> +		 */
> +		t = wait_for_completion_timeout(&dwc->reset_event,
> +						msecs_to_jiffies(100));
> +		if (t)
> +			goto done;
> +
> +		/**
> +		 * If link state != 4 we've hit the metastability issue, soft reset.
> +		 */
> +		if (ltssm == 4)
> +			goto done;
> +
> +		dwc3_trace(trace_dwc3_gadget,
> +			   "applying metastability workaround\n");
> +		trys++;
> +		if (trys == 2) {
> +			dev_WARN_ONCE(dwc->dev, true,
> +				      "metastability workaround failed!\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		/* stop gadget */
> +		dwc3_gadget_disable_irq(dwc);
> +		__dwc3_gadget_ep_disable(dwc->eps[0]);
> +		__dwc3_gadget_ep_disable(dwc->eps[1]);
> +
> +		/* soft reset device and restart */
> +		ret = dwc3_device_reinit(dwc);
> +		if (ret) {
> +			dev_err(dwc->dev, "device reinit failed\n");
> +			return ret;
> +		}
> +
> +		reinit_completion(&dwc->reset_event);
> +		/* restart gadget */
> +		ret = dwc3_gadget_restart(dwc);
> +		if (ret) {
> +			dev_err(dwc->dev, "failed to re-init gadget\n");
> +			return ret;
> +		}
> +
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		goto try;
> +	}

holy crap dude ?!? Now ->pullup() has to reinitialize dwc3 completely ? 

-- 
balbi

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

Re: [PATCH 2/2] usb: dwc3: gadget: usb: dwc3: run/stop metastability workaround

[Roger Quadros]

On 16/03/16 15:14, Felipe Balbi wrote:
> Roger Quadros <rogerq@ti.com> writes:
> 
>> [ text/plain ]
>> The existing workaround of forcing DEVSPD to SUPER_SPEED
>> for HIGH_SPEED ports is causing another side effect
>> which causes erratic interrupts and delayed gadget
>> enumeration of upto 2 seconds.
>>
>> Work around the run/stop issue by detecting if
>> it happened using debug LTSSM state and issuing
>> soft reset to the device controller when changing RUN_STOP
>> from 0 to 1.
>>
>> We apply the workaround only if PRTCAP is DEVICE mode
>> as we don't yet support this workaround in OTG mode.
>>
>> Use USB RESET event as exit condition for workaround.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/dwc3/core.h   |   1 +
>>  drivers/usb/dwc3/gadget.c | 175 +++++++++++++++++++++++++++++++++++++---------
>>  2 files changed, 144 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 2bea1ac..a724c0d 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -762,6 +762,7 @@ struct dwc3 {
>>  
>>  	struct usb_gadget	gadget;
>>  	struct usb_gadget_driver *gadget_driver;
>> +	struct completion	reset_event; /* used for run/stop workaround */
> 
> the fact that you needed a struct completion here already points to the
> fact this is really, really bad :-)
> 
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3ac170f..03418b8 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -35,6 +35,9 @@
>>  #include "gadget.h"
>>  #include "io.h"
>>  
>> +static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
>> +static int dwc3_gadget_restart(struct dwc3 *dwc);
>> +
>>  /**
>>   * dwc3_gadget_set_test_mode - Enables USB2 Test Modes
>>   * @dwc: pointer to our context structure
>> @@ -1570,13 +1573,100 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>  	struct dwc3		*dwc = gadget_to_dwc(g);
>>  	unsigned long		flags;
>>  	int			ret;
>> +	int			trys = 0;
>>  
>>  	is_on = !!is_on;
>>  
>> +	if (is_on)
>> +		reinit_completion(&dwc->reset_event);
>> +
>>  	spin_lock_irqsave(&dwc->lock, flags);
>>  	ret = dwc3_gadget_run_stop(dwc, is_on, false);
>>  	spin_unlock_irqrestore(&dwc->lock, flags);
>>  
>> +try:
>> +	/**
>> +	 * WORKAROUND: DWC3 revision < 2.20a have an issue
>> +	 * which would cause metastability state on Run/Stop
>> +	 * bit if we try to force the IP to USB2-only mode.
>> +	 *
>> +	 * Because of that, we check if we hit that issue and
>> +	 * reset core and retry if we did.
>> +	 *
>> +	 * We only attempt this workaround if we are in
>> +	 * DEVICE mode (i.e. not OTG).
>> +	 *
>> +	 * Refers to:
>> +	 *
>> +	 * STAR#9000525659: Clock Domain Crossing on DCTL in
>> +	 * USB 2.0 Mode
>> +	 */
>> +	if (is_on && dwc->revision < DWC3_REVISION_220A &&
>> +	    dwc->prtcap_mode == DWC3_GCTL_PRTCAP_DEVICE) {
>> +		u32 devspd, ltssm;
>> +		unsigned long t;
>> +
>> +		/* only applicable if devspd != SUPERSPEED */
>> +		devspd = dwc3_readl(dwc->regs, DWC3_DCFG) & DWC3_DCFG_SPEED_MASK;
>> +		if (devspd == DWC3_DCFG_SUPERSPEED)
>> +			goto done;
>> +
>> +		/* get link state */
>> +		ltssm = dwc3_readl(dwc->regs, DWC3_GDBGLTSSM);
>> +		ltssm = (ltssm >> 22) & 0xf;
>> +
>> +		/**
>> +		 * Need to wait for 100ms and check if ltssm != 4 to detect
>> +		 * metastability issue. If we got a reset event then we are
>> +		 * safe and can continue.
>> +		 */
>> +		t = wait_for_completion_timeout(&dwc->reset_event,
>> +						msecs_to_jiffies(100));
>> +		if (t)
>> +			goto done;
>> +
>> +		/**
>> +		 * If link state != 4 we've hit the metastability issue, soft reset.
>> +		 */
>> +		if (ltssm == 4)
>> +			goto done;
>> +
>> +		dwc3_trace(trace_dwc3_gadget,
>> +			   "applying metastability workaround\n");
>> +		trys++;
>> +		if (trys == 2) {
>> +			dev_WARN_ONCE(dwc->dev, true,
>> +				      "metastability workaround failed!\n");
>> +			return -ETIMEDOUT;
>> +		}
>> +
>> +		spin_lock_irqsave(&dwc->lock, flags);
>> +		/* stop gadget */
>> +		dwc3_gadget_disable_irq(dwc);
>> +		__dwc3_gadget_ep_disable(dwc->eps[0]);
>> +		__dwc3_gadget_ep_disable(dwc->eps[1]);
>> +
>> +		/* soft reset device and restart */
>> +		ret = dwc3_device_reinit(dwc);
>> +		if (ret) {
>> +			dev_err(dwc->dev, "device reinit failed\n");
>> +			return ret;
>> +		}
>> +
>> +		reinit_completion(&dwc->reset_event);
>> +		/* restart gadget */
>> +		ret = dwc3_gadget_restart(dwc);
>> +		if (ret) {
>> +			dev_err(dwc->dev, "failed to re-init gadget\n");
>> +			return ret;
>> +		}
>> +
>> +		spin_unlock_irqrestore(&dwc->lock, flags);
>> +		goto try;
>> +	}
> 
> holy crap dude ?!? Now ->pullup() has to reinitialize dwc3 completely ? 
> 

Only if it hits the error state. We're only re-initializing the device controller
not complete dwc3.

cheers,
-roger

Re: [PATCH 0/2] usb: dwc3: gadget: Fix erratic interrupts and delayed enumeration

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:
> The existing workaround (for STAR#9000525659) of forcing
> DEVSPD to SUPER_SPEED for HIGH_SPEED ports is causing
> another side effect which causes erratic interrupts and delayed gadget
> enumeration of upto 2 seconds.

right, but the real problem is with an SoC which wants to force lower
speed on an IP that can't support it ;-)

-- 
balbi

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