[PATCH 0/6] bug fix for mv_udc_core.c

[PATCH 0/6] bug fix for mv_udc_core.c

[Neil Zhang]

This patch set is mainly for bug fix.

Neil Zhang (6):
  usb: gadget: mv_udc: remove redundant pull up in udc_start
  usb: gadget: mv_udc: disable HW zlt for ep0
  usb: gadget: mv_udc: clear corresponding interrupt when flush fifo
  usb: gadget: mv_udc: check endpoint before queue dtd
  USB: gadget: mv_udc: workaroud for missing dTD
  USB: gadget: mv_udc: fix corner case for missiong dTD

 drivers/usb/gadget/mv_udc_core.c |   47 ++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[PATCH 1/6] usb: gadget: mv_udc: remove redundant pull up in udc_start

[Neil Zhang]

Romove redundant pull up in udc_start since function udc_bind_to_driver
in udc-core.c will do it for us.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 drivers/usb/gadget/mv_udc_core.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index fcff3a5..ebc0dfd 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -1365,9 +1365,6 @@ static int mv_udc_start(struct usb_gadget *gadget,
 		}
 	}
 
-	/* pullup is always on */
-	mv_udc_pullup(&udc->gadget, 1);
-
 	/* When boot with cable attached, there will be no vbus irq occurred */
 	if (udc->qwork)
 		queue_work(udc->qwork, &udc->vbus_work);
-- 
1.7.9.5

[PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Neil Zhang]

Hardware zlt will try to send the zero length packet automatically
when the data transferd is multiple times of max packet, this will
cause issues on Windows.
So let's disable HW zlt by default.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 drivers/usb/gadget/mv_udc_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index ebc0dfd..657ac5c 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -89,7 +89,7 @@ static void ep0_reset(struct mv_udc *udc)
 		/* configure ep0 endpoint capabilities in dQH */
 		ep->dqh->max_packet_length =
 			(EP0_MAX_PKT_SIZE << EP_QUEUE_HEAD_MAX_PKT_LEN_POS)
-			| EP_QUEUE_HEAD_IOS;
+			| EP_QUEUE_HEAD_IOS | EP_QUEUE_HEAD_ZLT_SEL;
 
 		ep->dqh->next_dtd_ptr = EP_QUEUE_HEAD_NEXT_TERMINATE;
 
-- 
1.7.9.5

[PATCH 3/6] usb: gadget: mv_udc: clear corresponding interrupt when flush fifo

[Neil Zhang]

The interruts are useless when this endpoint is going to be flushed.
Especially in the enumeration phase, let's take get device description
for example.
1. Device doesn't ACK the status package from host in time due to irq is
disabled by some module.
2. Host find no ACK in time, and send another request.
3. Device gets the chance to handle the interrupt, the sequence is as
following.
	a. Flush pending requests in ep0.
	b. Queue a request in ep0 according to host's request and change
	   the ep0 state to DATA_STATE_XMIT.
	c. Handle the pending interrupt for the last status package. But
	   actually it will check the new added request instead of the status
	   package and because of wront ep0 state, device will request an
	   unexpected status package from host.
	d. The ep0 state is going mad.

The solution is that we need clear the pending corresponding interrupt
as well as flush
endpoint.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 drivers/usb/gadget/mv_udc_core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index 657ac5c..d5a9bdf 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -692,6 +692,8 @@ static void mv_ep_fifo_flush(struct usb_ep *_ep)
 		}
 		loops--;
 	} while (readl(&udc->op_regs->epstatus) & bit_pos);
+
+	writel(bit_pos, &udc->op_regs->epcomplete);
 }
 
 /* queues (submits) an I/O request to an endpoint */
-- 
1.7.9.5

[PATCH 4/6] usb: gadget: mv_udc: check endpoint before queue dtd

[Neil Zhang]

There is a corner case that endpoint is disabled by system shutdown
between check ep->desc and hold spin lock in mv_ep_queue. In this
case ep->ep.desc will be NULL and occur kernel panic when access
it in build_dtd.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 drivers/usb/gadget/mv_udc_core.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index d5a9bdf..a620cff 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -734,6 +734,14 @@ mv_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
 
 	spin_lock_irqsave(&udc->lock, flags);
 
+	if (!ep->ep.desc) {
+		spin_unlock_irqrestore(&udc->lock, flags);
+		dev_info(&udc->dev->dev,
+			"%s is already disabled!\n", ep->name);
+		retval = -EINVAL;
+		goto err_unmap_dma;
+	}
+
 	/* build dtds and push them to device queue */
 	if (!req_to_dtd(req)) {
 		retval = queue_dtd(ep, req);
-- 
1.7.9.5

[PATCH 5/6] USB: gadget: mv_udc: workaroud for missing dTD

[Neil Zhang]

There is an issue with the add dTD tripwire semaphore (ATDTW bit in
USBCMD register) that can cause the controller to ignore a dTD that is
added to a primed endpoint. When this happens, the software can read
the tripwire bit and the status bit at '1' even though the endpoint
is unprimed.

After executing a dTD, the device controller endpoint state machine
executes a final read of the dTD terminate bit to check if the
application added a dTD to the linked list at the last moment. This read
is done in the finpkt_read_latest_next_td (44) state. After the read is
performed, if the terminate bit is still set, the state machine moves to
unprime the endpoint. The decision to unprime the endpoint is done in
the checkqh_decision (59) state, based on the value of the terminate bit.

Before reaching the checkqh_decision state, the state machine traverses
the writeqhtd_status (57), writeqh_status (56), and release_prime_mask
(42) states. As shown in the waveform, the ep_addtd_tripwire_clr signal
is not set to clear the tripwire bit in these states.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 drivers/usb/gadget/mv_udc_core.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index a620cff..8df8606 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -196,7 +196,27 @@ static int process_ep_req(struct mv_udc *udc, int index,
 			while (readl(&udc->op_regs->epstatus) & bit_pos)
 				udelay(1);
 			break;
+		} else {
+			if (!(readl(&udc->op_regs->epstatus) & bit_pos)) {
+				/* The DMA engine thinks there is no more dTD */
+				curr_dqh->next_dtd_ptr = curr_dtd->dtd_next
+					& EP_QUEUE_HEAD_NEXT_POINTER_MASK;
+
+				/* clear active and halt bit */
+				curr_dqh->size_ioc_int_sts &=
+						~(DTD_STATUS_ACTIVE
+						| DTD_STATUS_HALTED);
+
+				/* Do prime again */
+				wmb();
+				writel(bit_pos, &udc->op_regs->epprime);
+				while (readl(&udc->op_regs->epprime) & bit_pos)
+					cpu_relax();
+
+				break;
+			}
 		}
+
 		udelay(1);
 	}
 
-- 
1.7.9.5

[PATCH 6/6] USB: gadget: mv_udc: fix corner case for missiong dTD

[Neil Zhang]

When the missing dTD issue is triggerred, queue_dtd may prime the new
request instead of the missing dTD.
We can just add the request to the queue end and jump out if there are
more than one request in the queue already.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 drivers/usb/gadget/mv_udc_core.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index 8df8606..918b603 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -295,13 +295,23 @@ static int queue_dtd(struct mv_ep *ep, struct mv_req *req)
 
 	/* check if the pipe is empty */
 	if (!(list_empty(&ep->queue))) {
-		struct mv_req *lastreq;
+		struct mv_req *firstreq, *lastreq;
+		firstreq = list_entry(ep->queue.next, struct mv_req, queue);
 		lastreq = list_entry(ep->queue.prev, struct mv_req, queue);
 		lastreq->tail->dtd_next =
 			req->head->td_dma & EP_QUEUE_HEAD_NEXT_POINTER_MASK;
 
 		wmb();
 
+		/*
+		 * If there are more than one reqs in the queue,
+		 * then it's safe to just add it to the list end.
+		 * It should be able to handle the missing DTD issue.
+		 * And suppose it can improve the performance too.
+		 */
+		if (firstreq != lastreq)
+			goto done;
+
 		if (readl(&udc->op_regs->epprime) & bit_pos)
 			goto done;
 
-- 
1.7.9.5

Re: [PATCH 0/6] bug fix for mv_udc_core.c

[Matthieu CASTET]

Le Mon, 24 Feb 2014 16:03:10 +0800,
Neil Zhang <zhangwm@marvell.com> a écrit :

> This patch set is mainly for bug fix.
> 
> Neil Zhang (6):
>   usb: gadget: mv_udc: remove redundant pull up in udc_start
>   usb: gadget: mv_udc: disable HW zlt for ep0
>   usb: gadget: mv_udc: clear corresponding interrupt when flush fifo
>   usb: gadget: mv_udc: check endpoint before queue dtd
>   USB: gadget: mv_udc: workaroud for missing dTD
>   USB: gadget: mv_udc: fix corner case for missiong dTD
> 
>  drivers/usb/gadget/mv_udc_core.c |   47 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 5 deletions(-)
> 

Do you have plan to move to the chipidea udc driver ?

You could have some bug fix for free or share your bug fix with others.


Matthieu

RE: [PATCH 0/6] bug fix for mv_udc_core.c

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1332 bytes --]


> -----Original Message-----
> From: Matthieu CASTET [mailto:matthieu.castet@parrot.com]
> Sent: 2014Äê2ÔÂ24ÈÕ 18:32
> To: Neil Zhang
> Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/6] bug fix for mv_udc_core.c
> 
> Le Mon, 24 Feb 2014 16:03:10 +0800,
> Neil Zhang <zhangwm@marvell.com> a ¨¦crit :
> 
> > This patch set is mainly for bug fix.
> >
> > Neil Zhang (6):
> >   usb: gadget: mv_udc: remove redundant pull up in udc_start
> >   usb: gadget: mv_udc: disable HW zlt for ep0
> >   usb: gadget: mv_udc: clear corresponding interrupt when flush fifo
> >   usb: gadget: mv_udc: check endpoint before queue dtd
> >   USB: gadget: mv_udc: workaroud for missing dTD
> >   USB: gadget: mv_udc: fix corner case for missiong dTD
> >
> >  drivers/usb/gadget/mv_udc_core.c |   47
> ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 42 insertions(+), 5 deletions(-)
> >
> 
> Do you have plan to move to the chipidea udc driver ?
> 
> You could have some bug fix for free or share your bug fix with others.
> 


Not yet.
Will estimate it later.
> 
> Matthieu

Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Peter Chen]

On Mon, Feb 24, 2014 at 04:03:12PM +0800, Neil Zhang wrote:
> Hardware zlt will try to send the zero length packet automatically
> when the data transferd is multiple times of max packet, this will
> cause issues on Windows.
> So let's disable HW zlt by default.

Would you have description that what kinds of issue on Windows
if zlt is is selected?

Peter

> 
> Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> ---
>  drivers/usb/gadget/mv_udc_core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
> index ebc0dfd..657ac5c 100644
> --- a/drivers/usb/gadget/mv_udc_core.c
> +++ b/drivers/usb/gadget/mv_udc_core.c
> @@ -89,7 +89,7 @@ static void ep0_reset(struct mv_udc *udc)
>  		/* configure ep0 endpoint capabilities in dQH */
>  		ep->dqh->max_packet_length =
>  			(EP0_MAX_PKT_SIZE << EP_QUEUE_HEAD_MAX_PKT_LEN_POS)
> -			| EP_QUEUE_HEAD_IOS;
> +			| EP_QUEUE_HEAD_IOS | EP_QUEUE_HEAD_ZLT_SEL;
>  
>  		ep->dqh->next_dtd_ptr = EP_QUEUE_HEAD_NEXT_TERMINATE;
>  
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH 3/6] usb: gadget: mv_udc: clear corresponding interrupt when flush fifo

[Peter Chen]

On Mon, Feb 24, 2014 at 04:03:13PM +0800, Neil Zhang wrote:
> The interruts are useless when this endpoint is going to be flushed.
> Especially in the enumeration phase, let's take get device description
> for example.
> 1. Device doesn't ACK the status package from host in time due to irq is
> disabled by some module.
> 2. Host find no ACK in time, and send another request.

Why device does not send NAK at that time?
Besides, you can try to prime status at data stage, it can also
avoid the problem usb 2.0 8.5.3.3 "Error Handling on the Last Data
Transaction" described.

> 3. Device gets the chance to handle the interrupt, the sequence is as
> following.
> 	a. Flush pending requests in ep0.
> 	b. Queue a request in ep0 according to host's request and change
> 	   the ep0 state to DATA_STATE_XMIT.
> 	c. Handle the pending interrupt for the last status package. But
> 	   actually it will check the new added request instead of the status
> 	   package and because of wront ep0 state, device will request an

%s/wront/wrong

Peter

> 	   unexpected status package from host.
> 	d. The ep0 state is going mad.
> 
> The solution is that we need clear the pending corresponding interrupt
> as well as flush
> endpoint.
> 
> Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> ---
>  drivers/usb/gadget/mv_udc_core.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
> index 657ac5c..d5a9bdf 100644
> --- a/drivers/usb/gadget/mv_udc_core.c
> +++ b/drivers/usb/gadget/mv_udc_core.c
> @@ -692,6 +692,8 @@ static void mv_ep_fifo_flush(struct usb_ep *_ep)
>  		}
>  		loops--;
>  	} while (readl(&udc->op_regs->epstatus) & bit_pos);
> +
> +	writel(bit_pos, &udc->op_regs->epcomplete);
>  }
>  
>  /* queues (submits) an I/O request to an endpoint */
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 

Best Regards,
Peter Chen

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1924 bytes --]




> -----Original Message-----
> From: Peter Chen [mailto:peter.chen@freescale.com]
> Sent: 2014Äê2ÔÂ25ÈÕ 9:19
> To: Neil Zhang
> Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0
> 
> On Mon, Feb 24, 2014 at 04:03:12PM +0800, Neil Zhang wrote:
> > Hardware zlt will try to send the zero length packet automatically
> > when the data transferd is multiple times of max packet, this will
> > cause issues on Windows.
> > So let's disable HW zlt by default.
> 
> Would you have description that what kinds of issue on Windows if zlt is is
> selected?
> 

Enumeration will fail.

> Peter
> 
> >
> > Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> > ---
> >  drivers/usb/gadget/mv_udc_core.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/mv_udc_core.c
> > b/drivers/usb/gadget/mv_udc_core.c
> > index ebc0dfd..657ac5c 100644
> > --- a/drivers/usb/gadget/mv_udc_core.c
> > +++ b/drivers/usb/gadget/mv_udc_core.c
> > @@ -89,7 +89,7 @@ static void ep0_reset(struct mv_udc *udc)
> >  		/* configure ep0 endpoint capabilities in dQH */
> >  		ep->dqh->max_packet_length =
> >  			(EP0_MAX_PKT_SIZE << EP_QUEUE_HEAD_MAX_PKT_LEN_POS)
> > -			| EP_QUEUE_HEAD_IOS;
> > +			| EP_QUEUE_HEAD_IOS | EP_QUEUE_HEAD_ZLT_SEL;
> >
> >  		ep->dqh->next_dtd_ptr = EP_QUEUE_HEAD_NEXT_TERMINATE;
> >
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> 
> --
> 
> Best Regards,
> Peter Chen


Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/6] bug fix for mv_udc_core.c

[Peter Chen]

On Mon, Feb 24, 2014 at 04:42:40AM -0800, Neil Zhang wrote:
> 
> > -----Original Message-----
> > From: Matthieu CASTET [mailto:matthieu.castet@parrot.com]
> > Sent: 2014年2月24日 18:32
> > To: Neil Zhang
> > Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 0/6] bug fix for mv_udc_core.c
> > 
> > Le Mon, 24 Feb 2014 16:03:10 +0800,
> > Neil Zhang <zhangwm@marvell.com> a écrit :
> > 
> > > This patch set is mainly for bug fix.
> > >
> > > Neil Zhang (6):
> > >   usb: gadget: mv_udc: remove redundant pull up in udc_start
> > >   usb: gadget: mv_udc: disable HW zlt for ep0
> > >   usb: gadget: mv_udc: clear corresponding interrupt when flush fifo
> > >   usb: gadget: mv_udc: check endpoint before queue dtd
> > >   USB: gadget: mv_udc: workaroud for missing dTD
> > >   USB: gadget: mv_udc: fix corner case for missiong dTD
> > >
> > >  drivers/usb/gadget/mv_udc_core.c |   47
> > ++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 42 insertions(+), 5 deletions(-)
> > >
> > 
> > Do you have plan to move to the chipidea udc driver ?
> > 
> > You could have some bug fix for free or share your bug fix with others.
> > 
> 
> 
> Not yet.
> Will estimate it later.
> > 

Freescale uses mv udc driver at u-boot, so the chipidea IP should be
compatible for two SoCs , both Intel and msm are using chipidea driver
at linux kernel, it should be not difficult for marvell to use it. 

For this patchset, some of them may already be fixed at chipidea driver
(3/6, 4/6), some of them may not (5/6, 6/6).

-- 

Best Regards,
Peter Chen

Re: [PATCH 5/6] USB: gadget: mv_udc: workaroud for missing dTD

[Peter Chen]

On Mon, Feb 24, 2014 at 04:03:15PM +0800, Neil Zhang wrote:
> There is an issue with the add dTD tripwire semaphore (ATDTW bit in
> USBCMD register) that can cause the controller to ignore a dTD that is
> added to a primed endpoint. When this happens, the software can read
> the tripwire bit and the status bit at '1' even though the endpoint
> is unprimed.
> 
> After executing a dTD, the device controller endpoint state machine
> executes a final read of the dTD terminate bit to check if the
> application added a dTD to the linked list at the last moment. This read
> is done in the finpkt_read_latest_next_td (44) state. After the read is
> performed, if the terminate bit is still set, the state machine moves to
> unprime the endpoint. The decision to unprime the endpoint is done in
> the checkqh_decision (59) state, based on the value of the terminate bit.
> 
> Before reaching the checkqh_decision state, the state machine traverses
> the writeqhtd_status (57), writeqh_status (56), and release_prime_mask
> (42) states. As shown in the waveform, the ep_addtd_tripwire_clr signal
> is not set to clear the tripwire bit in these states.
> 
> Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> ---
>  drivers/usb/gadget/mv_udc_core.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
> index a620cff..8df8606 100644
> --- a/drivers/usb/gadget/mv_udc_core.c
> +++ b/drivers/usb/gadget/mv_udc_core.c
> @@ -196,7 +196,27 @@ static int process_ep_req(struct mv_udc *udc, int index,
>  			while (readl(&udc->op_regs->epstatus) & bit_pos)
>  				udelay(1);
>  			break;
> +		} else {
> +			if (!(readl(&udc->op_regs->epstatus) & bit_pos)) {
> +				/* The DMA engine thinks there is no more dTD */
> +				curr_dqh->next_dtd_ptr = curr_dtd->dtd_next
> +					& EP_QUEUE_HEAD_NEXT_POINTER_MASK;
> +
> +				/* clear active and halt bit */
> +				curr_dqh->size_ioc_int_sts &=
> +						~(DTD_STATUS_ACTIVE
> +						| DTD_STATUS_HALTED);
> +
> +				/* Do prime again */
> +				wmb();
> +				writel(bit_pos, &udc->op_regs->epprime);
> +				while (readl(&udc->op_regs->epprime) & bit_pos)
> +					cpu_relax();
> +
> +				break;
> +			}
>  		}
> +
>  		udelay(1);
>  	}
>  

Is it a chipidea IP issue? Any errate number for this issue?
Does we need it for chipidea driver?

-- 

Best Regards,
Peter Chen

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Peter Chen]

 
> > On Mon, Feb 24, 2014 at 04:03:12PM +0800, Neil Zhang wrote:
> > > Hardware zlt will try to send the zero length packet automatically
> > > when the data transferd is multiple times of max packet, this will
> > > cause issues on Windows.
> > > So let's disable HW zlt by default.
> >
> > Would you have description that what kinds of issue on Windows if zlt
> > is is selected?
> >
> 
> Enumeration will fail.
> 

What causes enumeration fail, why it does not occur before?

Peter

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1151 bytes --]


> -----Original Message-----
> From: Peter Chen [mailto:Peter.Chen@freescale.com]
> Sent: 2014Äê2ÔÂ25ÈÕ 13:15
> To: Neil Zhang
> Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0
> 
> 
> 
> 
> > > On Mon, Feb 24, 2014 at 04:03:12PM +0800, Neil Zhang wrote:
> > > > Hardware zlt will try to send the zero length packet automatically
> > > > when the data transferd is multiple times of max packet, this will
> > > > cause issues on Windows.
> > > > So let's disable HW zlt by default.
> > >
> > > Would you have description that what kinds of issue on Windows if
> > > zlt is is selected?
> > >
> >
> > Enumeration will fail.
> >
> 
> What causes enumeration fail, why it does not occur before?
> 
A unexpected zero packet cause enumeration fail.
It's not easy that the descriptor is actually 1024 bytes, so not easy to be found.

> Peter

Best Regards,
Neil Zhang

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 5/6] USB: gadget: mv_udc: workaroud for missing dTD

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3186 bytes --]



> -----Original Message-----
> From: Peter Chen [mailto:peter.chen@freescale.com]
> Sent: 2014Äê2ÔÂ25ÈÕ 12:19
> To: Neil Zhang
> Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/6] USB: gadget: mv_udc: workaroud for missing dTD
> 
> On Mon, Feb 24, 2014 at 04:03:15PM +0800, Neil Zhang wrote:
> > There is an issue with the add dTD tripwire semaphore (ATDTW bit in
> > USBCMD register) that can cause the controller to ignore a dTD that is
> > added to a primed endpoint. When this happens, the software can read
> > the tripwire bit and the status bit at '1' even though the endpoint is
> > unprimed.
> >
> > After executing a dTD, the device controller endpoint state machine
> > executes a final read of the dTD terminate bit to check if the
> > application added a dTD to the linked list at the last moment. This
> > read is done in the finpkt_read_latest_next_td (44) state. After the
> > read is performed, if the terminate bit is still set, the state
> > machine moves to unprime the endpoint. The decision to unprime the
> > endpoint is done in the checkqh_decision (59) state, based on the value of
> the terminate bit.
> >
> > Before reaching the checkqh_decision state, the state machine
> > traverses the writeqhtd_status (57), writeqh_status (56), and
> > release_prime_mask
> > (42) states. As shown in the waveform, the ep_addtd_tripwire_clr
> > signal is not set to clear the tripwire bit in these states.
> >
> > Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> > ---
> >  drivers/usb/gadget/mv_udc_core.c |   20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/mv_udc_core.c
> > b/drivers/usb/gadget/mv_udc_core.c
> > index a620cff..8df8606 100644
> > --- a/drivers/usb/gadget/mv_udc_core.c
> > +++ b/drivers/usb/gadget/mv_udc_core.c
> > @@ -196,7 +196,27 @@ static int process_ep_req(struct mv_udc *udc, int
> index,
> >  			while (readl(&udc->op_regs->epstatus) & bit_pos)
> >  				udelay(1);
> >  			break;
> > +		} else {
> > +			if (!(readl(&udc->op_regs->epstatus) & bit_pos)) {
> > +				/* The DMA engine thinks there is no more dTD */
> > +				curr_dqh->next_dtd_ptr = curr_dtd->dtd_next
> > +					& EP_QUEUE_HEAD_NEXT_POINTER_MASK;
> > +
> > +				/* clear active and halt bit */
> > +				curr_dqh->size_ioc_int_sts &=
> > +						~(DTD_STATUS_ACTIVE
> > +						| DTD_STATUS_HALTED);
> > +
> > +				/* Do prime again */
> > +				wmb();
> > +				writel(bit_pos, &udc->op_regs->epprime);
> > +				while (readl(&udc->op_regs->epprime) & bit_pos)
> > +					cpu_relax();
> > +
> > +				break;
> > +			}
> >  		}
> > +
> >  		udelay(1);
> >  	}
> >
> 
> Is it a chipidea IP issue? Any errate number for this issue?
> Does we need it for chipidea driver?
> 

Yes, I think so.
9000531823	B2-Medium	Adding a dTD to a Primed Endpoint May Not Get Recognized
Impacted Configuration: All device mode configurations. 

> --
> 
> Best Regards,
> Peter Chen

Best Regards,
Neil Zhang

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 0/6] bug fix for mv_udc_core.c

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2325 bytes --]


> -----Original Message-----
> From: Peter Chen [mailto:peter.chen@freescale.com]
> Sent: 2014年2月25日 12:18
> To: Neil Zhang
> Cc: Matthieu CASTET; balbi@ti.com; gregkh@linuxfoundation.org;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/6] bug fix for mv_udc_core.c
> 
> On Mon, Feb 24, 2014 at 04:42:40AM -0800, Neil Zhang wrote:
> >
> > > -----Original Message-----
> > > From: Matthieu CASTET [mailto:matthieu.castet@parrot.com]
> > > Sent: 2014年2月24日 18:32
> > > To: Neil Zhang
> > > Cc: balbi@ti.com; gregkh@linuxfoundation.org;
> > > linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 0/6] bug fix for mv_udc_core.c
> > >
> > > Le Mon, 24 Feb 2014 16:03:10 +0800,
> > > Neil Zhang <zhangwm@marvell.com> a écrit :
> > >
> > > > This patch set is mainly for bug fix.
> > > >
> > > > Neil Zhang (6):
> > > >   usb: gadget: mv_udc: remove redundant pull up in udc_start
> > > >   usb: gadget: mv_udc: disable HW zlt for ep0
> > > >   usb: gadget: mv_udc: clear corresponding interrupt when flush fifo
> > > >   usb: gadget: mv_udc: check endpoint before queue dtd
> > > >   USB: gadget: mv_udc: workaroud for missing dTD
> > > >   USB: gadget: mv_udc: fix corner case for missiong dTD
> > > >
> > > >  drivers/usb/gadget/mv_udc_core.c |   47
> > > ++++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 42 insertions(+), 5 deletions(-)
> > > >
> > >
> > > Do you have plan to move to the chipidea udc driver ?
> > >
> > > You could have some bug fix for free or share your bug fix with others.
> > >
> >
> >
> > Not yet.
> > Will estimate it later.
> > >
> 
> Freescale uses mv udc driver at u-boot, so the chipidea IP should be
> compatible for two SoCs , both Intel and msm are using chipidea driver at linux
> kernel, it should be not difficult for marvell to use it.
> 
> For this patchset, some of them may already be fixed at chipidea driver (3/6,
> 4/6), some of them may not (5/6, 6/6).
> 
> --

Thanks for the comments.
Since we are using it in our product, so it's not that easy to switch to a new driver.

> 
> Best Regards,
> Peter Chen

Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 5/6] USB: gadget: mv_udc: workaroud for missing dTD

[Peter Chen]

 
> > From: Peter Chen [mailto:peter.chen@freescale.com]
> > Sent: 2014年2月25日 12:19
> > To: Neil Zhang
> > Cc: balbi@ti.com; gregkh@linuxfoundation.org;
> > linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 5/6] USB: gadget: mv_udc: workaroud for missing
> > dTD
> >
> > On Mon, Feb 24, 2014 at 04:03:15PM +0800, Neil Zhang wrote:
> > > There is an issue with the add dTD tripwire semaphore (ATDTW bit in
> > > USBCMD register) that can cause the controller to ignore a dTD that
> > > is added to a primed endpoint. When this happens, the software can
> > > read the tripwire bit and the status bit at '1' even though the
> > > endpoint is unprimed.
> > >
> > > After executing a dTD, the device controller endpoint state machine
> > > executes a final read of the dTD terminate bit to check if the
> > > application added a dTD to the linked list at the last moment. This
> > > read is done in the finpkt_read_latest_next_td (44) state. After the
> > > read is performed, if the terminate bit is still set, the state
> > > machine moves to unprime the endpoint. The decision to unprime the
> > > endpoint is done in the checkqh_decision (59) state, based on the
> > > value of
> > the terminate bit.
> > >
> > > Before reaching the checkqh_decision state, the state machine
> > > traverses the writeqhtd_status (57), writeqh_status (56), and
> > > release_prime_mask
> > > (42) states. As shown in the waveform, the ep_addtd_tripwire_clr
> > > signal is not set to clear the tripwire bit in these states.
> > >
> > > Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> > > ---
> > >  drivers/usb/gadget/mv_udc_core.c |   20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/mv_udc_core.c
> > > b/drivers/usb/gadget/mv_udc_core.c
> > > index a620cff..8df8606 100644
> > > --- a/drivers/usb/gadget/mv_udc_core.c
> > > +++ b/drivers/usb/gadget/mv_udc_core.c
> > > @@ -196,7 +196,27 @@ static int process_ep_req(struct mv_udc *udc,
> > > int
> > index,
> > >  			while (readl(&udc->op_regs->epstatus) & bit_pos)
> > >  				udelay(1);
> > >  			break;
> > > +		} else {
> > > +			if (!(readl(&udc->op_regs->epstatus) & bit_pos)) {
> > > +				/* The DMA engine thinks there is no more dTD */
> > > +				curr_dqh->next_dtd_ptr = curr_dtd->dtd_next
> > > +					& EP_QUEUE_HEAD_NEXT_POINTER_MASK;
> > > +
> > > +				/* clear active and halt bit */
> > > +				curr_dqh->size_ioc_int_sts &=
> > > +						~(DTD_STATUS_ACTIVE
> > > +						| DTD_STATUS_HALTED);
> > > +
> > > +				/* Do prime again */
> > > +				wmb();
> > > +				writel(bit_pos, &udc->op_regs->epprime);
> > > +				while (readl(&udc->op_regs->epprime) & bit_pos)
> > > +					cpu_relax();
> > > +
> > > +				break;
> > > +			}
> > >  		}
> > > +
> > >  		udelay(1);
> > >  	}
> > >
> >
> > Is it a chipidea IP issue? Any errate number for this issue?
> > Does we need it for chipidea driver?
> >
> 
> Yes, I think so.
> 9000531823	B2-Medium	Adding a dTD to a Primed Endpoint May Not Get
> Recognized
> Impacted Configuration: All device mode configurations.
> 
 
Thanks for your information.

Peter

RE: [PATCH 3/6] usb: gadget: mv_udc: clear corresponding interrupt when flush fifo

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2753 bytes --]



> -----Original Message-----
> From: Peter Chen [mailto:peter.chen@freescale.com]
> Sent: 2014Äê2ÔÂ25ÈÕ 9:59
> To: Neil Zhang
> Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/6] usb: gadget: mv_udc: clear corresponding interrupt
> when flush fifo
> 
> On Mon, Feb 24, 2014 at 04:03:13PM +0800, Neil Zhang wrote:
> > The interruts are useless when this endpoint is going to be flushed.
> > Especially in the enumeration phase, let's take get device description
> > for example.
> > 1. Device doesn't ACK the status package from host in time due to irq
> > is disabled by some module.
> > 2. Host find no ACK in time, and send another request.
> 
> Why device does not send NAK at that time?
> Besides, you can try to prime status at data stage, it can also avoid the
> problem usb 2.0 8.5.3.3 "Error Handling on the Last Data Transaction"
> described.
> 
> > 3. Device gets the chance to handle the interrupt, the sequence is as
> > following.
> > 	a. Flush pending requests in ep0.
> > 	b. Queue a request in ep0 according to host's request and change
> > 	   the ep0 state to DATA_STATE_XMIT.
> > 	c. Handle the pending interrupt for the last status package. But
> > 	   actually it will check the new added request instead of the status
> > 	   package and because of wront ep0 state, device will request an
> 
> %s/wront/wrong


Thanks for the catch.
> 
> Peter
> 
> > 	   unexpected status package from host.
> > 	d. The ep0 state is going mad.
> >
> > The solution is that we need clear the pending corresponding interrupt
> > as well as flush endpoint.
> >
> > Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> > ---
> >  drivers/usb/gadget/mv_udc_core.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/mv_udc_core.c
> > b/drivers/usb/gadget/mv_udc_core.c
> > index 657ac5c..d5a9bdf 100644
> > --- a/drivers/usb/gadget/mv_udc_core.c
> > +++ b/drivers/usb/gadget/mv_udc_core.c
> > @@ -692,6 +692,8 @@ static void mv_ep_fifo_flush(struct usb_ep *_ep)
> >  		}
> >  		loops--;
> >  	} while (readl(&udc->op_regs->epstatus) & bit_pos);
> > +
> > +	writel(bit_pos, &udc->op_regs->epcomplete);
> >  }
> >
> >  /* queues (submits) an I/O request to an endpoint */
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> 

Best Regards,
Neil Zhang

> --
> 
> Best Regards,
> Peter Chen

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Peter Chen]

 
> >
> >
> >
> >
> > > > On Mon, Feb 24, 2014 at 04:03:12PM +0800, Neil Zhang wrote:
> > > > > Hardware zlt will try to send the zero length packet
> > > > > automatically when the data transferd is multiple times of max
> > > > > packet, this will cause issues on Windows.
> > > > > So let's disable HW zlt by default.
> > > >
> > > > Would you have description that what kinds of issue on Windows if
> > > > zlt is is selected?
> > > >
> > >
> > > Enumeration will fail.
> > >
> >
> > What causes enumeration fail, why it does not occur before?
> >
> A unexpected zero packet cause enumeration fail.
> It's not easy that the descriptor is actually 1024 bytes, so not easy to
> be found.
> 

Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data Stage, USB 2.0 spec?

Peter

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Felipe Balbi]

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

Hi,

On Tue, Feb 25, 2014 at 07:46:08AM +0000, Peter Chen wrote:
> > > > > > Hardware zlt will try to send the zero length packet
> > > > > > automatically when the data transferd is multiple times of max
> > > > > > packet, this will cause issues on Windows.
> > > > > > So let's disable HW zlt by default.
> > > > >
> > > > > Would you have description that what kinds of issue on Windows if
> > > > > zlt is is selected?
> > > > >
> > > >
> > > > Enumeration will fail.
> > > >
> > >
> > > What causes enumeration fail, why it does not occur before?
> > >
> > A unexpected zero packet cause enumeration fail.
> > It's not easy that the descriptor is actually 1024 bytes, so not easy to
> > be found.
> > 
> 
> Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data Stage, USB 2.0 spec?

wait, this is a chipidea core ? Why aren't you guys using the chipidea
driver yet ? You need to switch over to that driver dude, we can't have
duplicated code in the tree.

I'm sorry, but I won't be taking this series, please use chipidea
driver, it should be very simple to add a glue layer for your core to
the chipidea driver.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1906 bytes --]


> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: 2014Äê2ÔÂ26ÈÕ 2:13
> To: Peter Chen
> Cc: Neil Zhang; balbi@ti.com; gregkh@linuxfoundation.org;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Alexander Shishkin
> Subject: Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0
> 
> Hi,
> 
> On Tue, Feb 25, 2014 at 07:46:08AM +0000, Peter Chen wrote:
> > > > > > > Hardware zlt will try to send the zero length packet
> > > > > > > automatically when the data transferd is multiple times of
> > > > > > > max packet, this will cause issues on Windows.
> > > > > > > So let's disable HW zlt by default.
> > > > > >
> > > > > > Would you have description that what kinds of issue on Windows
> > > > > > if zlt is is selected?
> > > > > >
> > > > >
> > > > > Enumeration will fail.
> > > > >
> > > >
> > > > What causes enumeration fail, why it does not occur before?
> > > >
> > > A unexpected zero packet cause enumeration fail.
> > > It's not easy that the descriptor is actually 1024 bytes, so not
> > > easy to be found.
> > >
> >
> > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data Stage, USB
> 2.0 spec?
> 
> wait, this is a chipidea core ? Why aren't you guys using the chipidea driver
> yet ? You need to switch over to that driver dude, we can't have duplicated
> code in the tree.
> 
> I'm sorry, but I won't be taking this series, please use chipidea driver, it should
> be very simple to add a glue layer for your core to the chipidea driver.
> 

Yes, it use chipidea IP.
But the driver is earlier than the chipidea one and we use it for our products.
So it may be not that easy to switch to chipidea driver due to the stability.

> --
> Balbi

Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Peter Chen]

 
> > > > easy to be found.
> > > >
> > >
> > > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data
> > > Stage, USB
> > 2.0 spec?
> >
> > wait, this is a chipidea core ? Why aren't you guys using the chipidea
> > driver yet ? You need to switch over to that driver dude, we can't
> > have duplicated code in the tree.
> >
> > I'm sorry, but I won't be taking this series, please use chipidea
> > driver, it should be very simple to add a glue layer for your core to
> the chipidea driver.
> >
> 
> Yes, it use chipidea IP.
> But the driver is earlier than the chipidea one and we use it for our
> products.
> So it may be not that easy to switch to chipidea driver due to the
> stability.
> 

Freescale i.mx SoC used fsl_udc_core.c before which was the one of the
oldest chipidea drivers, now, all i.mx SoC uses chipidea driver including
old hardware.

Peter

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Felipe Balbi]

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

Hi,

On Tue, Feb 25, 2014 at 05:48:17PM -0800, Neil Zhang wrote:
> > > > > > > > Hardware zlt will try to send the zero length packet
> > > > > > > > automatically when the data transferd is multiple times of
> > > > > > > > max packet, this will cause issues on Windows.
> > > > > > > > So let's disable HW zlt by default.
> > > > > > >
> > > > > > > Would you have description that what kinds of issue on Windows
> > > > > > > if zlt is is selected?
> > > > > > >
> > > > > >
> > > > > > Enumeration will fail.
> > > > > >
> > > > >
> > > > > What causes enumeration fail, why it does not occur before?
> > > > >
> > > > A unexpected zero packet cause enumeration fail.
> > > > It's not easy that the descriptor is actually 1024 bytes, so not
> > > > easy to be found.
> > > >
> > >
> > > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data Stage, USB
> > 2.0 spec?
> > 
> > wait, this is a chipidea core ? Why aren't you guys using the chipidea driver
> > yet ? You need to switch over to that driver dude, we can't have duplicated
> > code in the tree.
> > 
> > I'm sorry, but I won't be taking this series, please use chipidea driver, it should
> > be very simple to add a glue layer for your core to the chipidea driver.
> > 
> 
> Yes, it use chipidea IP.
> But the driver is earlier than the chipidea one and we use it for our
> products.
> So it may be not that easy to switch to chipidea driver due to the
> stability.

that's nonsense, the average chipidea glue layer is ~80 LOCs. You can
write that in less than 2 hours and give it a try. We cannot have
duplicate drivers in the tree and development effort *must* be shared.

If you guys use the same IP, why wouldn't you use the same chipidea
driver ?

sorry, you didn't convince me.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Felipe Balbi]

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

On Wed, Feb 26, 2014 at 02:36:19AM +0000, Peter Chen wrote:
>  
> > > > > easy to be found.
> > > > >
> > > >
> > > > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data
> > > > Stage, USB
> > > 2.0 spec?
> > >
> > > wait, this is a chipidea core ? Why aren't you guys using the chipidea
> > > driver yet ? You need to switch over to that driver dude, we can't
> > > have duplicated code in the tree.
> > >
> > > I'm sorry, but I won't be taking this series, please use chipidea
> > > driver, it should be very simple to add a glue layer for your core to
> > the chipidea driver.
> > >
> > 
> > Yes, it use chipidea IP.
> > But the driver is earlier than the chipidea one and we use it for our
> > products.
> > So it may be not that easy to switch to chipidea driver due to the
> > stability.
> > 
> 
> Freescale i.mx SoC used fsl_udc_core.c before which was the one of the
> oldest chipidea drivers, now, all i.mx SoC uses chipidea driver including
> old hardware.

Exactly, Freescale and Intel folks have shown that chipidea driver is
pretty good and ready for production.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Felipe Balbi]

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

On Wed, Feb 26, 2014 at 02:36:19AM +0000, Peter Chen wrote:
>  
> > > > > easy to be found.
> > > > >
> > > >
> > > > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data
> > > > Stage, USB
> > > 2.0 spec?
> > >
> > > wait, this is a chipidea core ? Why aren't you guys using the chipidea
> > > driver yet ? You need to switch over to that driver dude, we can't
> > > have duplicated code in the tree.
> > >
> > > I'm sorry, but I won't be taking this series, please use chipidea
> > > driver, it should be very simple to add a glue layer for your core to
> > the chipidea driver.
> > >
> > 
> > Yes, it use chipidea IP.
> > But the driver is earlier than the chipidea one and we use it for our
> > products.
> > So it may be not that easy to switch to chipidea driver due to the
> > stability.
> > 
> 
> Freescale i.mx SoC used fsl_udc_core.c before which was the one of the

btw, when can I remove fsl_udc_core.c from the tree ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Peter Chen]

 
> > > > >
> > > > > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data
> > > > > Stage, USB
> > > > 2.0 spec?
> > > >
> > > > wait, this is a chipidea core ? Why aren't you guys using the
> > > > chipidea driver yet ? You need to switch over to that driver dude,
> > > > we can't have duplicated code in the tree.
> > > >
> > > > I'm sorry, but I won't be taking this series, please use chipidea
> > > > driver, it should be very simple to add a glue layer for your core
> > > > to
> > > the chipidea driver.
> > > >
> > >
> > > Yes, it use chipidea IP.
> > > But the driver is earlier than the chipidea one and we use it for
> > > our products.
> > > So it may be not that easy to switch to chipidea driver due to the
> > > stability.
> > >
> >
> > Freescale i.mx SoC used fsl_udc_core.c before which was the one of the
> 
> btw, when can I remove fsl_udc_core.c from the tree ?
> 

Freescale has other processor group (PowerPC, etc) has used this code now.

Peter

RE: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2490 bytes --]


> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: 2014Äê2ÔÂ26ÈÕ 23:19
> To: Neil Zhang
> Cc: balbi@ti.com; Peter Chen; gregkh@linuxfoundation.org;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Alexander Shishkin
> Subject: Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0
> 
> Hi,
> 
> On Tue, Feb 25, 2014 at 05:48:17PM -0800, Neil Zhang wrote:
> > > > > > > > > Hardware zlt will try to send the zero length packet
> > > > > > > > > automatically when the data transferd is multiple times
> > > > > > > > > of max packet, this will cause issues on Windows.
> > > > > > > > > So let's disable HW zlt by default.
> > > > > > > >
> > > > > > > > Would you have description that what kinds of issue on
> > > > > > > > Windows if zlt is is selected?
> > > > > > > >
> > > > > > >
> > > > > > > Enumeration will fail.
> > > > > > >
> > > > > >
> > > > > > What causes enumeration fail, why it does not occur before?
> > > > > >
> > > > > A unexpected zero packet cause enumeration fail.
> > > > > It's not easy that the descriptor is actually 1024 bytes, so not
> > > > > easy to be found.
> > > > >
> > > >
> > > > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data
> > > > Stage, USB
> > > 2.0 spec?
> > >
> > > wait, this is a chipidea core ? Why aren't you guys using the
> > > chipidea driver yet ? You need to switch over to that driver dude,
> > > we can't have duplicated code in the tree.
> > >
> > > I'm sorry, but I won't be taking this series, please use chipidea
> > > driver, it should be very simple to add a glue layer for your core to the
> chipidea driver.
> > >
> >
> > Yes, it use chipidea IP.
> > But the driver is earlier than the chipidea one and we use it for our
> > products.
> > So it may be not that easy to switch to chipidea driver due to the
> > stability.
> 
> that's nonsense, the average chipidea glue layer is ~80 LOCs. You can write
> that in less than 2 hours and give it a try. We cannot have duplicate drivers in
> the tree and development effort *must* be shared.
> 
> If you guys use the same IP, why wouldn't you use the same chipidea driver ?
> 
> sorry, you didn't convince me.

It's too sad!
Anyway we will estimate the chipidea driver to see whether it meets our requirement.

> 
> --
> Balbi

Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Felipe Balbi]

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

On Thu, Feb 27, 2014 at 03:30:03AM +0000, Peter Chen wrote:
>  
> > > > > >
> > > > > > Chipidea bug too? Does it follow ch 8.5.3.2 Variable-length Data
> > > > > > Stage, USB
> > > > > 2.0 spec?
> > > > >
> > > > > wait, this is a chipidea core ? Why aren't you guys using the
> > > > > chipidea driver yet ? You need to switch over to that driver dude,
> > > > > we can't have duplicated code in the tree.
> > > > >
> > > > > I'm sorry, but I won't be taking this series, please use chipidea
> > > > > driver, it should be very simple to add a glue layer for your core
> > > > > to
> > > > the chipidea driver.
> > > > >
> > > >
> > > > Yes, it use chipidea IP.
> > > > But the driver is earlier than the chipidea one and we use it for
> > > > our products.
> > > > So it may be not that easy to switch to chipidea driver due to the
> > > > stability.
> > > >
> > >
> > > Freescale i.mx SoC used fsl_udc_core.c before which was the one of the
> > 
> > btw, when can I remove fsl_udc_core.c from the tree ?
> > 
> 
> Freescale has other processor group (PowerPC, etc) has used this code
> now.

Can we move those to chipidea too ? It would be real nice to remove the
duplicated driver by 3.16.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Fabio Estevam]

On Thu, Feb 27, 2014 at 12:30 AM, Peter Chen <Peter.Chen@freescale.com> wrote:

>> btw, when can I remove fsl_udc_core.c from the tree ?
>>
>
> Freescale has other processor group (PowerPC, etc) has used this code now.

Not only PowerPC, but also the imx platforms that have not been
converted to device tree yet.

Re: [PATCH 2/6] usb: gadget: mv_udc: disable HW zlt for ep0

[Felipe Balbi]

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

On Thu, Feb 27, 2014 at 03:05:14PM -0300, Fabio Estevam wrote:
> On Thu, Feb 27, 2014 at 12:30 AM, Peter Chen <Peter.Chen@freescale.com> wrote:
> 
> >> btw, when can I remove fsl_udc_core.c from the tree ?
> >>
> >
> > Freescale has other processor group (PowerPC, etc) has used this code now.
> 
> Not only PowerPC, but also the imx platforms that have not been
> converted to device tree yet.

hmm, but chipidea supports non-DT, right ? I would rather have pdata
added to your chipidea glue layer than maintaining the old driver in the
tree.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]