2014-02-24 08:05:55

by Neil Zhang

[permalink] [raw]
Subject: [PATCH 0/6] bug fix for mv_udc_core.c

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


2014-02-24 08:03:27

by Neil Zhang

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

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 <[email protected]>
---
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

2014-02-24 08:03:37

by Neil Zhang

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

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 <[email protected]>
---
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

2014-02-24 08:03:34

by Neil Zhang

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

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 <[email protected]>
---
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

2014-02-24 08:03:31

by Neil Zhang

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

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 <[email protected]>
---
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

2014-02-24 08:04:43

by Neil Zhang

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

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 <[email protected]>
---
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

2014-02-24 08:05:02

by Neil Zhang

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

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 <[email protected]>
---
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

2014-02-24 10:32:02

by Matthieu CASTET

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

Le Mon, 24 Feb 2014 16:03:10 +0800,
Neil Zhang <[email protected]> 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

2014-02-24 12:42:49

by Neil Zhang

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


> -----Original Message-----
> From: Matthieu CASTET [mailto:[email protected]]
> Sent: 2014??2??24?? 18:32
> To: Neil Zhang
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 0/6] bug fix for mv_udc_core.c
>
> Le Mon, 24 Feb 2014 16:03:10 +0800,
> Neil Zhang <[email protected]> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-25 02:13:45

by Peter Chen

[permalink] [raw]
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?

Peter

>
> Signed-off-by: Neil Zhang <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>

--

Best Regards,
Peter Chen

2014-02-25 02:54:25

by Peter Chen

[permalink] [raw]
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

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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>

--

Best Regards,
Peter Chen

2014-02-25 04:14:18

by Neil Zhang

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




> -----Original Message-----
> From: Peter Chen [mailto:[email protected]]
> Sent: 2014??2??25?? 9:19
> To: Neil Zhang
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> > ---
> > 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 [email protected] More majordomo
> > info at http://vger.kernel.org/majordomo-info.html
> >
> >
>
> --
>
> Best Regards,
> Peter Chen


Best Regards,
Neil Zhang
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-25 05:12:39

by Peter Chen

[permalink] [raw]
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:[email protected]]
> > Sent: 2014年2月24日 18:32
> > To: Neil Zhang
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH 0/6] bug fix for mv_udc_core.c
> >
> > Le Mon, 24 Feb 2014 16:03:10 +0800,
> > Neil Zhang <[email protected]> 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

2014-02-25 05:14:24

by Peter Chen

[permalink] [raw]
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 <[email protected]>
> ---
> 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

2014-02-25 05:15:25

by Peter Chen

[permalink] [raw]
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?

Peter

2014-02-25 07:11:46

by Neil Zhang

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


> -----Original Message-----
> From: Peter Chen [mailto:[email protected]]
> Sent: 2014??2??25?? 13:15
> To: Neil Zhang
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-25 07:16:56

by Neil Zhang

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



> -----Original Message-----
> From: Peter Chen [mailto:[email protected]]
> Sent: 2014??2??25?? 12:19
> To: Neil Zhang
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> > ---
> > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-25 07:18:51

by Neil Zhang

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


> -----Original Message-----
> From: Peter Chen [mailto:[email protected]]
> Sent: 2014年2月25日 12:18
> To: Neil Zhang
> Cc: Matthieu CASTET; [email protected]; [email protected];
> [email protected]; [email protected]
> 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:[email protected]]
> > > Sent: 2014年2月24日 18:32
> > > To: Neil Zhang
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH 0/6] bug fix for mv_udc_core.c
> > >
> > > Le Mon, 24 Feb 2014 16:03:10 +0800,
> > > Neil Zhang <[email protected]> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-25 07:23:21

by Peter Chen

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




> > From: Peter Chen [mailto:[email protected]]
> > Sent: 2014$BG/(B2$B7n(B25$BF|(B 12:19
> > To: Neil Zhang
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]
> > 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 <[email protected]>
> > > ---
> > > 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

2014-02-25 07:26:37

by Neil Zhang

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



> -----Original Message-----
> From: Peter Chen [mailto:[email protected]]
> Sent: 2014??2??25?? 9:59
> To: Neil Zhang
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> > ---
> > 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 [email protected] More majordomo
> > info at http://vger.kernel.org/majordomo-info.html
> >
> >
>

Best Regards,
Neil Zhang

> --
>
> Best Regards,
> Peter Chen

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-25 07:46:33

by Peter Chen

[permalink] [raw]
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.
>

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

Peter

2014-02-25 18:14:06

by Felipe Balbi

[permalink] [raw]
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.

--
balbi


Attachments:
(No filename) (1.11 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-02-26 01:48:25

by Neil Zhang

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


> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: 2014??2??26?? 2:13
> To: Peter Chen
> Cc: Neil Zhang; [email protected]; [email protected];
> [email protected]; [email protected]; 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-26 02:36:25

by Peter Chen

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


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

2014-02-26 15:20:47

by Felipe Balbi

[permalink] [raw]
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.

--
balbi


Attachments:
(No filename) (1.73 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-02-26 15:21:31

by Felipe Balbi

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

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


Attachments:
(No filename) (1.07 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-02-26 15:25:09

by Felipe Balbi

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

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


Attachments:
(No filename) (954.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-02-27 03:30:09

by Peter Chen

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


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

2014-02-27 09:12:28

by Neil Zhang

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


> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: 2014??2??26?? 23:19
> To: Neil Zhang
> Cc: [email protected]; Peter Chen; [email protected];
> [email protected]; [email protected]; 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-02-27 16:53:56

by Felipe Balbi

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

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


Attachments:
(No filename) (1.18 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-02-27 18:05:20

by Fabio Estevam

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

On Thu, Feb 27, 2014 at 12:30 AM, Peter Chen <[email protected]> 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.

2014-02-27 19:05:50

by Felipe Balbi

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

On Thu, Feb 27, 2014 at 03:05:14PM -0300, Fabio Estevam wrote:
> On Thu, Feb 27, 2014 at 12:30 AM, Peter Chen <[email protected]> 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


Attachments:
(No filename) (555.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments