[PATCH] usb/dwc3: Correct DEPCFG for Config Action Modify

2012-08-06 Thread Pratyush Anand 
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
---
 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,
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;
+
return dwc3_send_gadget_ep_cmd(dwc, dep-number,
DWC3_DEPCMD_SETEPCONFIG, params);
 }
@@ -1551,6 +1554,9 @@ static int dwc3_gadget_start(struct usb_gadget *g,
/* Start with SuperSpeed Default */
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
 
+   /* At Start - Attached State */
+   dwc-dev_state = DWC3_ATTACHED_STATE;
+
dep = dwc-eps[0];
ret = __dwc3_gadget_ep_enable(dep, dwc3_gadget_ep0_desc, NULL);
if (ret) {
@@ -1976,6 +1982,9 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 
*dwc)
 
dwc-gadget.speed = USB_SPEED_UNKNOWN;
dwc-setup_packet_pending = false;
+
+   /* At Disconnect - Attached State */
+   dwc-dev_state = DWC3_ATTACHED_STATE;
 }
 
 static void dwc3_gadget_usb3_phy_suspend(struct dwc3 *dwc, int suspend)
-- 
1.7.5.4

--
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

Re: [PATCH] usb/dwc3: Correct DEPCFG for Config Action Modify

2012-08-06 Thread Felipe Balbi 
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

Re: [PATCH] usb/dwc3: Correct DEPCFG for Config Action Modify

2012-08-06 Thread Pratyush Anand 

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,

Re: [PATCH] usb/dwc3: Correct DEPCFG for Config Action Modify

2012-08-06 Thread Felipe Balbi 
Hi,

On Mon, Aug 06, 2012 at 07:22:52PM +0530, Pratyush Anand wrote:
 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.

My understanding is that this is only needed when we want to change any
configuration of any endpoint without disabling it first. And the only
case where this happens is with our physical endpoints 0 and 1. I guess
it tells us to use Connect Done event for ep0/1 because at that time we
already know the speed we're running, so we can reconfigure ep0/1
wMaxPacketSize properly.

For all other endpoints, we only change configuration when the endpoint
is disabled. See how the framework is designed today: when we change an
interface, our endpoints will be disabled by the gadget driver and
re-enabled with different descriptors. This means that, except for
ep0/1, no endpoint has an existing configuration ;-)

-- 
balbi


signature.asc
Description: Digital signature