Hi, On Mon, Aug 06, 2012 at 03:33:16PM +0530, Pratyush Anand wrote: > From: Pratyush ANAND <pratyush.an...@st.com> > > We need to issue DEPCFG (with Config Action Set to "Modify") as per > specification. Even if we do not respect this , it works in normal > cases. But, I have observed one failure in very peculiar situation. > > If cpu clock is very slow and execution of connection done ISR takes > long time (may be around 150 mS), then one will never be able to > complete even first setup request. Setup bytes is received, but IN data > for get device descriptor is never observed on wire. dwc3 always keeps > on waiting for first data transfer complete event. > > It can easily be reproduced even when working with high cpu clock by > deliberately putting delay around 150-200 mS at the start of connection > done handler. > > Current patch corrects the error. > > Signed-off-by: Pratyush Anand <pratyush.an...@st.com>
this is wrong, you implement it with weird heuristics based on the
endpoint state. All you needed to do was pass another argument to the
set_ep_config() function like I did...
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/gadget.c | 9 +++++++++
> 2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 133ed5a..e5b6496 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -489,6 +489,7 @@ enum dwc3_link_state {
> };
>
> enum dwc3_device_state {
> + DWC3_ATTACHED_STATE,
this is not a USB-defined state, so I don't want to use it.
> DWC3_DEFAULT_STATE,
> DWC3_ADDRESS_STATE,
> DWC3_CONFIGURED_STATE,
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f6ba644..3926095 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -473,6 +473,9 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc,
> struct dwc3_ep *dep,
> dep->interval = 1 << (desc->bInterval - 1);
> }
>
> + if (dwc->dev_state != DWC3_ATTACHED_STATE)
> + params.param0 |= DWC3_DEPCFG_ACTION_MODIFY;
the modify action is not available on all versions of the core. On older
versions of the core, this was called "Ignore Sequence Number bit". And
this I have already patched, see below:
commit 4b345c9a3c7452340fb477868d8db475f05978b1
Author: Felipe Balbi <ba...@ti.com>
Date: Mon Jul 16 14:08:16 2012 +0300
usb: dwc3: gadget: set Ignore Sequence Number bit from ConnectDone Event
Databook says we should set Ignore Sequence Number bit
from ConnectDone Event, so let's do so.
Signed-off-by: Felipe Balbi <ba...@ti.com>
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 58fdfad..283c0cb 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -431,7 +431,8 @@ static int dwc3_gadget_start_config(struct dwc3 *dwc,
struct dwc3_ep *dep)
static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep,
const struct usb_endpoint_descriptor *desc,
- const struct usb_ss_ep_comp_descriptor *comp_desc)
+ const struct usb_ss_ep_comp_descriptor *comp_desc,
+ bool ignore)
{
struct dwc3_gadget_ep_cmd_params params;
@@ -441,6 +442,9 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc,
struct dwc3_ep *dep,
| DWC3_DEPCFG_MAX_PACKET_SIZE(usb_endpoint_maxp(desc))
| DWC3_DEPCFG_BURST_SIZE(dep->endpoint.maxburst - 1);
+ if (ignore)
+ params.param0 |= DWC3_DEPCFG_IGN_SEQ_NUM;
+
params.param1 = DWC3_DEPCFG_XFER_COMPLETE_EN
| DWC3_DEPCFG_XFER_NOT_READY_EN;
@@ -498,7 +502,8 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc,
struct dwc3_ep *dep)
*/
static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
const struct usb_endpoint_descriptor *desc,
- const struct usb_ss_ep_comp_descriptor *comp_desc)
+ const struct usb_ss_ep_comp_descriptor *comp_desc,
+ bool ignore)
{
struct dwc3 *dwc = dep->dwc;
u32 reg;
@@ -510,7 +515,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
return ret;
}
- ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc);
+ ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc, ignore);
if (ret)
return ret;
@@ -683,7 +688,7 @@ static int dwc3_gadget_ep_enable(struct usb_ep *ep,
dev_vdbg(dwc->dev, "Enabling %s\n", dep->name);
spin_lock_irqsave(&dwc->lock, flags);
- ret = __dwc3_gadget_ep_enable(dep, desc, ep->comp_desc);
+ ret = __dwc3_gadget_ep_enable(dep, desc, ep->comp_desc, false);
spin_unlock_irqrestore(&dwc->lock, flags);
return ret;
@@ -1518,14 +1523,14 @@ static int dwc3_gadget_start(struct usb_gadget *g,
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
dep = dwc->eps[0];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL);
+ ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
goto err0;
}
dep = dwc->eps[1];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL);
+ ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
goto err1;
@@ -2141,14 +2146,14 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3
*dwc)
}
dep = dwc->eps[0];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL);
+ ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, true);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
return;
}
dep = dwc->eps[1];
- ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL);
+ ret = __dwc3_gadget_ep_enable(dep, &dwc3_gadget_ep0_desc, NULL, true);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
return;
I have sent this patch to linux-usb a while ago [1]
[1] http://marc.info/?l=linux-usb&m=134303785528433&w=2
--
balbi
signature.asc
Description: Digital signature
