On 8/6/2012 5:28 PM, Felipe Balbi wrote:
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...
I keep track of dwc3 patches, but somehow I missed your this particular
patch :(.
---
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
Ok. That I will take care in V2.
versions of the core, this was called "Ignore Sequence Number bit". And
this I have already patched, see below:
In fact, I had thought of exactly same implementation, but then did not
try because of one doubt. 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);
I was not sure about it.
Databook specifically talks about to use "modify" on connection done.
But, in description of "Config Action: Modify", it says that this action
is used when modifying an existing endpoint configuration.
So, does it not mean that when we issue DEPCFG for the first time for
any endpoint, then only we need to set "Config Action :Initialize", else
"Config Action: Modify".
Regards
Pratyush
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
