[U-Boot] [PATCH 0/2] usb: gadget: fotg210: workaround & new hardware support

[U-Boot] [PATCH 0/2] usb: gadget: fotg210: workaround & new hardware support

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

1. It's known that EP0 fifo empty indication is not reliable, an extra delay
   is necessary to avoid data corruption while handling packets with size
   greater than 64 bytes.

2. Since hardware revision 1.11.0, some fields of interrupt status registers
   are now write-1-clear.

Kuo-Jung Su (2):
  usb: gadget: fotg210: add w1c interrupt status support
  usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

 drivers/usb/gadget/fotg210.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

--
1.7.9.5

[U-Boot] [PATCH 1/2] usb: gadget: fotg210: add w1c interrupt status support

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Since hardware revision 1.11.0, the following interrupt status registers
are now write-1-clear (w1c):

1. Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
2. Interrupt Source Group 2 Register (0x14C) (All bits)

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
CC: Marek Vasut <marex@denx.de>
---
 drivers/usb/gadget/fotg210.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
index 6e19db1..e3a61cc 100644
--- a/drivers/usb/gadget/fotg210.c
+++ b/drivers/usb/gadget/fotg210.c
@@ -847,8 +847,11 @@ int usb_gadget_handle_interrupts(void)
 	/* CX interrupts */
 	if (gisr & GISR_GRP0) {
 		st = readl(&regs->gisr0);
+#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C
+		writel(st & GISR0_CXABORT, &regs->gisr0);
+#else
 		writel(0, &regs->gisr0);
-
+#endif
 		if (st & GISR0_CXERR)
 			printf("fotg210: cmd error\n");
 
@@ -873,8 +876,11 @@ int usb_gadget_handle_interrupts(void)
 	/* Device Status Interrupts */
 	if (gisr & GISR_GRP2) {
 		st = readl(&regs->gisr2);
+#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C
+		writel(st, &regs->gisr2);
+#else
 		writel(0, &regs->gisr2);
-
+#endif
 		if (st & GISR2_RESET)
 			printf("fotg210: reset by host\n");
 		else if (st & GISR2_SUSPEND)
-- 
1.7.9.5

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Because the EP0 fifo empty indication is non-reliable,
an extra delay is necessary to avoid data corruption while
handling packets with size greater than 64 bytes.

This workaround should be applied to all hardware revisions.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
CC: Marek Vasut <marex@denx.de>
---
 drivers/usb/gadget/fotg210.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
index e3a61cc..14bfec6 100644
--- a/drivers/usb/gadget/fotg210.c
+++ b/drivers/usb/gadget/fotg210.c
@@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep, struct fotg210_request *req)
 		if (ep->id == 0) {
 			/* Wait until cx/ep0 fifo empty */
 			fotg210_cxwait(chip, CXFIFO_CXFIFOE);
+			udelay_masked(1);
 			writel(DMAFIFO_CX, &regs->dma_fifo);
 		} else {
 			/* Wait until epx fifo empty */
-- 
1.7.9.5

[U-Boot] [PATCH 1/2] usb: gadget: fotg210: add w1c interrupt status support

[Marek Vasut]

On Wednesday, December 18, 2013 at 08:24:48 AM, Kuo-Jung Su wrote:
> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> 
> Since hardware revision 1.11.0, the following interrupt status registers
> are now write-1-clear (w1c):

What did they look like before ?

> 1. Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
> 2. Interrupt Source Group 2 Register (0x14C) (All bits)
> 
> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> CC: Marek Vasut <marex@denx.de>
> ---
>  drivers/usb/gadget/fotg210.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
> index 6e19db1..e3a61cc 100644
> --- a/drivers/usb/gadget/fotg210.c
> +++ b/drivers/usb/gadget/fotg210.c
> @@ -847,8 +847,11 @@ int usb_gadget_handle_interrupts(void)
>  	/* CX interrupts */
>  	if (gisr & GISR_GRP0) {
>  		st = readl(&regs->gisr0);
> +#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C

Can we not get rid of this ifdef somehow please ? Like detect the revision on-
the-fly and handle the bit accordingly or such ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Marek Vasut]

On Wednesday, December 18, 2013 at 08:24:49 AM, Kuo-Jung Su wrote:
> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> 
> Because the EP0 fifo empty indication is non-reliable,
> an extra delay is necessary to avoid data corruption while
> handling packets with size greater than 64 bytes.
> 
> This workaround should be applied to all hardware revisions.
> 
> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> CC: Marek Vasut <marex@denx.de>
> ---
>  drivers/usb/gadget/fotg210.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
> index e3a61cc..14bfec6 100644
> --- a/drivers/usb/gadget/fotg210.c
> +++ b/drivers/usb/gadget/fotg210.c
> @@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep, struct
> fotg210_request *req) if (ep->id == 0) {
>  			/* Wait until cx/ep0 fifo empty */
>  			fotg210_cxwait(chip, CXFIFO_CXFIFOE);
> +			udelay_masked(1);

Why don't you use regular udelay() here please ? Also, how exactly does the 
delay help solving the unreliability problem please?

>  			writel(DMAFIFO_CX, &regs->dma_fifo);
>  		} else {
>  			/* Wait until epx fifo empty */

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Kuo-Jung Su]

2013/12/18 Marek Vasut <marex@denx.de>:
> On Wednesday, December 18, 2013 at 08:24:49 AM, Kuo-Jung Su wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>
>> Because the EP0 fifo empty indication is non-reliable,
>> an extra delay is necessary to avoid data corruption while
>> handling packets with size greater than 64 bytes.
>>
>> This workaround should be applied to all hardware revisions.
>>
>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> CC: Marek Vasut <marex@denx.de>
>> ---
>>  drivers/usb/gadget/fotg210.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
>> index e3a61cc..14bfec6 100644
>> --- a/drivers/usb/gadget/fotg210.c
>> +++ b/drivers/usb/gadget/fotg210.c
>> @@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep, struct
>> fotg210_request *req) if (ep->id == 0) {
>>                       /* Wait until cx/ep0 fifo empty */
>>                       fotg210_cxwait(chip, CXFIFO_CXFIFOE);
>> +                     udelay_masked(1);
>
> Why don't you use regular udelay() here please ? Also, how exactly does the
> delay help solving the unreliability problem please?
>

1. No specific reason at all, I'll use regular udelay() in next version. :)

2. The fifo size of ep0 is 64 bytes, and my driver is supposed to make
sure the fifo empty
    before filling up the fifo. However there is a hardware bug that
the fifo empty indication is somehow
    a bit earlier than fifo reset. So if I don't add an extra delay
here, the data might be corrupted (i.e., 1 byte missing.)
    And after a couple of tests, it looks like that 1 usec is good
enough for this.

>>                       writel(DMAFIFO_CX, &regs->dma_fifo);
>>               } else {
>>                       /* Wait until epx fifo empty */
>
> Best regards,
> Marek Vasut



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH 1/2] usb: gadget: fotg210: add w1c interrupt status support

[Kuo-Jung Su]

2013/12/18 Marek Vasut <marex@denx.de>:
> On Wednesday, December 18, 2013 at 08:24:48 AM, Kuo-Jung Su wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>
>> Since hardware revision 1.11.0, the following interrupt status registers
>> are now write-1-clear (w1c):
>
> What did they look like before ?
>

They were r/w registers. i.e., software must write a zero to clear the status.
I'll add the comment in next version.

>> 1. Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
>> 2. Interrupt Source Group 2 Register (0x14C) (All bits)
>>
>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> CC: Marek Vasut <marex@denx.de>
>> ---
>>  drivers/usb/gadget/fotg210.c |   10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
>> index 6e19db1..e3a61cc 100644
>> --- a/drivers/usb/gadget/fotg210.c
>> +++ b/drivers/usb/gadget/fotg210.c
>> @@ -847,8 +847,11 @@ int usb_gadget_handle_interrupts(void)
>>       /* CX interrupts */
>>       if (gisr & GISR_GRP0) {
>>               st = readl(&regs->gisr0);
>> +#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C
>
> Can we not get rid of this ifdef somehow please ? Like detect the revision on-
> the-fly and handle the bit accordingly or such ?
>

Unfortunately there is no revision id register in this hardware, so I
have to do it manually.

> Best regards,
> Marek Vasut



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH 1/2] usb: gadget: fotg210: add w1c interrupt status support

[Marek Vasut]

On Thursday, December 19, 2013 at 01:54:56 AM, Kuo-Jung Su wrote:
> 2013/12/18 Marek Vasut <marex@denx.de>:
> > On Wednesday, December 18, 2013 at 08:24:48 AM, Kuo-Jung Su wrote:
> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> 
> >> Since hardware revision 1.11.0, the following interrupt status registers
> > 
> >> are now write-1-clear (w1c):
> > What did they look like before ?
> 
> They were r/w registers. i.e., software must write a zero to clear the
> status. I'll add the comment in next version.

OK.

> >> 1. Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
> >> 2. Interrupt Source Group 2 Register (0x14C) (All bits)
> >> 
> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> CC: Marek Vasut <marex@denx.de>
> >> ---
> >> 
> >>  drivers/usb/gadget/fotg210.c |   10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
> >> index 6e19db1..e3a61cc 100644
> >> --- a/drivers/usb/gadget/fotg210.c
> >> +++ b/drivers/usb/gadget/fotg210.c
> >> @@ -847,8 +847,11 @@ int usb_gadget_handle_interrupts(void)
> >> 
> >>       /* CX interrupts */
> >>       if (gisr & GISR_GRP0) {
> >>       
> >>               st = readl(&regs->gisr0);
> >> 
> >> +#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C
> > 
> > Can we not get rid of this ifdef somehow please ? Like detect the
> > revision on- the-fly and handle the bit accordingly or such ?
> 
> Unfortunately there is no revision id register in this hardware, so I
> have to do it manually.

So what would happen if you write 1, then write 0 into them ? ;-) Won't that 
handle both cases? Writing one will make sure the clean them on new hardware, 
writing zero afterwards will clean them on old hardware.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Marek Vasut]

On Thursday, December 19, 2013 at 01:50:55 AM, Kuo-Jung Su wrote:
> 2013/12/18 Marek Vasut <marex@denx.de>:
> > On Wednesday, December 18, 2013 at 08:24:49 AM, Kuo-Jung Su wrote:
> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> 
> >> Because the EP0 fifo empty indication is non-reliable,
> >> an extra delay is necessary to avoid data corruption while
> >> handling packets with size greater than 64 bytes.
> >> 
> >> This workaround should be applied to all hardware revisions.
> >> 
> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> CC: Marek Vasut <marex@denx.de>
> >> ---
> >> 
> >>  drivers/usb/gadget/fotg210.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
> >> index e3a61cc..14bfec6 100644
> >> --- a/drivers/usb/gadget/fotg210.c
> >> +++ b/drivers/usb/gadget/fotg210.c
> >> @@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep, struct
> >> fotg210_request *req) if (ep->id == 0) {
> >> 
> >>                       /* Wait until cx/ep0 fifo empty */
> >>                       fotg210_cxwait(chip, CXFIFO_CXFIFOE);
> >> 
> >> +                     udelay_masked(1);
> > 
> > Why don't you use regular udelay() here please ? Also, how exactly does
> > the delay help solving the unreliability problem please?
> 
> 1. No specific reason at all, I'll use regular udelay() in next version. :)
> 
> 2. The fifo size of ep0 is 64 bytes, and my driver is supposed to make
> sure the fifo empty
>     before filling up the fifo. However there is a hardware bug that
> the fifo empty indication is somehow
>     a bit earlier than fifo reset. So if I don't add an extra delay
> here, the data might be corrupted (i.e., 1 byte missing.)
>     And after a couple of tests, it looks like that 1 usec is good
> enough for this.

Ick, but I guess you guys know the IP blocks' sourcecode.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Kuo-Jung Su]

2013/12/19 Marek Vasut <marex@denx.de>:
> On Thursday, December 19, 2013 at 01:50:55 AM, Kuo-Jung Su wrote:
>> 2013/12/18 Marek Vasut <marex@denx.de>:
>> > On Wednesday, December 18, 2013 at 08:24:49 AM, Kuo-Jung Su wrote:
>> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >>
>> >> Because the EP0 fifo empty indication is non-reliable,
>> >> an extra delay is necessary to avoid data corruption while
>> >> handling packets with size greater than 64 bytes.
>> >>
>> >> This workaround should be applied to all hardware revisions.
>> >>
>> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >> CC: Marek Vasut <marex@denx.de>
>> >> ---
>> >>
>> >>  drivers/usb/gadget/fotg210.c |    1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
>> >> index e3a61cc..14bfec6 100644
>> >> --- a/drivers/usb/gadget/fotg210.c
>> >> +++ b/drivers/usb/gadget/fotg210.c
>> >> @@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep, struct
>> >> fotg210_request *req) if (ep->id == 0) {
>> >>
>> >>                       /* Wait until cx/ep0 fifo empty */
>> >>                       fotg210_cxwait(chip, CXFIFO_CXFIFOE);
>> >>
>> >> +                     udelay_masked(1);
>> >
>> > Why don't you use regular udelay() here please ? Also, how exactly does
>> > the delay help solving the unreliability problem please?
>>
>> 1. No specific reason at all, I'll use regular udelay() in next version. :)
>>
>> 2. The fifo size of ep0 is 64 bytes, and my driver is supposed to make
>> sure the fifo empty
>>     before filling up the fifo. However there is a hardware bug that
>> the fifo empty indication is somehow
>>     a bit earlier than fifo reset. So if I don't add an extra delay
>> here, the data might be corrupted (i.e., 1 byte missing.)
>>     And after a couple of tests, it looks like that 1 usec is good
>> enough for this.
>
> Ick, but I guess you guys know the IP blocks' sourcecode.
>

Yes, but I don't have the access permission , and I'm not a member of
the IP verification team......

Anyway I'll try to call the IP owner see if he's willing to do FPGA
verification.

> Best regards,
> Marek Vasut



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH 1/2] usb: gadget: fotg210: add w1c interrupt status support

[Kuo-Jung Su]

2013/12/19 Marek Vasut <marex@denx.de>:
> On Thursday, December 19, 2013 at 01:54:56 AM, Kuo-Jung Su wrote:
>> 2013/12/18 Marek Vasut <marex@denx.de>:
>> > On Wednesday, December 18, 2013 at 08:24:48 AM, Kuo-Jung Su wrote:
>> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >>
>> >> Since hardware revision 1.11.0, the following interrupt status registers
>> >
>> >> are now write-1-clear (w1c):
>> > What did they look like before ?
>>
>> They were r/w registers. i.e., software must write a zero to clear the
>> status. I'll add the comment in next version.
>
> OK.
>
>> >> 1. Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
>> >> 2. Interrupt Source Group 2 Register (0x14C) (All bits)
>> >>
>> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >> CC: Marek Vasut <marex@denx.de>
>> >> ---
>> >>
>> >>  drivers/usb/gadget/fotg210.c |   10 ++++++++--
>> >>  1 file changed, 8 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
>> >> index 6e19db1..e3a61cc 100644
>> >> --- a/drivers/usb/gadget/fotg210.c
>> >> +++ b/drivers/usb/gadget/fotg210.c
>> >> @@ -847,8 +847,11 @@ int usb_gadget_handle_interrupts(void)
>> >>
>> >>       /* CX interrupts */
>> >>       if (gisr & GISR_GRP0) {
>> >>
>> >>               st = readl(&regs->gisr0);
>> >>
>> >> +#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C
>> >
>> > Can we not get rid of this ifdef somehow please ? Like detect the
>> > revision on- the-fly and handle the bit accordingly or such ?
>>
>> Unfortunately there is no revision id register in this hardware, so I
>> have to do it manually.
>
> So what would happen if you write 1, then write 0 into them ? ;-) Won't that
> handle both cases? Writing one will make sure the clean them on new hardware,
> writing zero afterwards will clean them on old hardware.
>

Good idea! I think it should work just fine.
I'll make sure if does work, and then prepare for the new patch.

> Best regards,
> Marek Vasut



-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH 1/2] usb: gadget: fotg210: add w1c interrupt status support

[Marek Vasut]

On Thursday, December 19, 2013 at 08:10:05 AM, Kuo-Jung Su wrote:
> 2013/12/19 Marek Vasut <marex@denx.de>:
> > On Thursday, December 19, 2013 at 01:54:56 AM, Kuo-Jung Su wrote:
> >> 2013/12/18 Marek Vasut <marex@denx.de>:
> >> > On Wednesday, December 18, 2013 at 08:24:48 AM, Kuo-Jung Su wrote:
> >> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> >> 
> >> >> Since hardware revision 1.11.0, the following interrupt status
> >> >> registers
> >> > 
> >> >> are now write-1-clear (w1c):
> >> > What did they look like before ?
> >> 
> >> They were r/w registers. i.e., software must write a zero to clear the
> >> status. I'll add the comment in next version.
> > 
> > OK.
> > 
> >> >> 1. Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
> >> >> 2. Interrupt Source Group 2 Register (0x14C) (All bits)
> >> >> 
> >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> >> CC: Marek Vasut <marex@denx.de>
> >> >> ---
> >> >> 
> >> >>  drivers/usb/gadget/fotg210.c |   10 ++++++++--
> >> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/usb/gadget/fotg210.c
> >> >> b/drivers/usb/gadget/fotg210.c index 6e19db1..e3a61cc 100644
> >> >> --- a/drivers/usb/gadget/fotg210.c
> >> >> +++ b/drivers/usb/gadget/fotg210.c
> >> >> @@ -847,8 +847,11 @@ int usb_gadget_handle_interrupts(void)
> >> >> 
> >> >>       /* CX interrupts */
> >> >>       if (gisr & GISR_GRP0) {
> >> >>       
> >> >>               st = readl(&regs->gisr0);
> >> >> 
> >> >> +#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C
> >> > 
> >> > Can we not get rid of this ifdef somehow please ? Like detect the
> >> > revision on- the-fly and handle the bit accordingly or such ?
> >> 
> >> Unfortunately there is no revision id register in this hardware, so I
> >> have to do it manually.
> > 
> > So what would happen if you write 1, then write 0 into them ? ;-) Won't
> > that handle both cases? Writing one will make sure the clean them on new
> > hardware, writing zero afterwards will clean them on old hardware.
> 
> Good idea! I think it should work just fine.
> I'll make sure if does work, and then prepare for the new patch.

Thanks :)

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Marek Vasut]

On Thursday, December 19, 2013 at 08:07:00 AM, Kuo-Jung Su wrote:
> 2013/12/19 Marek Vasut <marex@denx.de>:
> > On Thursday, December 19, 2013 at 01:50:55 AM, Kuo-Jung Su wrote:
> >> 2013/12/18 Marek Vasut <marex@denx.de>:
> >> > On Wednesday, December 18, 2013 at 08:24:49 AM, Kuo-Jung Su wrote:
> >> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> >> 
> >> >> Because the EP0 fifo empty indication is non-reliable,
> >> >> an extra delay is necessary to avoid data corruption while
> >> >> handling packets with size greater than 64 bytes.
> >> >> 
> >> >> This workaround should be applied to all hardware revisions.
> >> >> 
> >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> >> CC: Marek Vasut <marex@denx.de>
> >> >> ---
> >> >> 
> >> >>  drivers/usb/gadget/fotg210.c |    1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >> 
> >> >> diff --git a/drivers/usb/gadget/fotg210.c
> >> >> b/drivers/usb/gadget/fotg210.c index e3a61cc..14bfec6 100644
> >> >> --- a/drivers/usb/gadget/fotg210.c
> >> >> +++ b/drivers/usb/gadget/fotg210.c
> >> >> @@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep,
> >> >> struct fotg210_request *req) if (ep->id == 0) {
> >> >> 
> >> >>                       /* Wait until cx/ep0 fifo empty */
> >> >>                       fotg210_cxwait(chip, CXFIFO_CXFIFOE);
> >> >> 
> >> >> +                     udelay_masked(1);
> >> > 
> >> > Why don't you use regular udelay() here please ? Also, how exactly
> >> > does the delay help solving the unreliability problem please?
> >> 
> >> 1. No specific reason at all, I'll use regular udelay() in next version.
> >> :)
> >> 
> >> 2. The fifo size of ep0 is 64 bytes, and my driver is supposed to make
> >> sure the fifo empty
> >> 
> >>     before filling up the fifo. However there is a hardware bug that
> >> 
> >> the fifo empty indication is somehow
> >> 
> >>     a bit earlier than fifo reset. So if I don't add an extra delay
> >> 
> >> here, the data might be corrupted (i.e., 1 byte missing.)
> >> 
> >>     And after a couple of tests, it looks like that 1 usec is good
> >> 
> >> enough for this.
> > 
> > Ick, but I guess you guys know the IP blocks' sourcecode.
> 
> Yes, but I don't have the access permission , and I'm not a member of
> the IP verification team......
> 
> Anyway I'll try to call the IP owner see if he's willing to do FPGA
> verification.

Would be nice, but I dont mind picking it even without such confirmation. 
Thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Kuo-Jung Su]

2013/12/19 Marek Vasut <marex@denx.de>:
> On Thursday, December 19, 2013 at 08:07:00 AM, Kuo-Jung Su wrote:
>> 2013/12/19 Marek Vasut <marex@denx.de>:
>> > On Thursday, December 19, 2013 at 01:50:55 AM, Kuo-Jung Su wrote:
>> >> 2013/12/18 Marek Vasut <marex@denx.de>:
>> >> > On Wednesday, December 18, 2013 at 08:24:49 AM, Kuo-Jung Su wrote:
>> >> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >> >>
>> >> >> Because the EP0 fifo empty indication is non-reliable,
>> >> >> an extra delay is necessary to avoid data corruption while
>> >> >> handling packets with size greater than 64 bytes.
>> >> >>
>> >> >> This workaround should be applied to all hardware revisions.
>> >> >>
>> >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >> >> CC: Marek Vasut <marex@denx.de>
>> >> >> ---
>> >> >>
>> >> >>  drivers/usb/gadget/fotg210.c |    1 +
>> >> >>  1 file changed, 1 insertion(+)
>> >> >>
>> >> >> diff --git a/drivers/usb/gadget/fotg210.c
>> >> >> b/drivers/usb/gadget/fotg210.c index e3a61cc..14bfec6 100644
>> >> >> --- a/drivers/usb/gadget/fotg210.c
>> >> >> +++ b/drivers/usb/gadget/fotg210.c
>> >> >> @@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep,
>> >> >> struct fotg210_request *req) if (ep->id == 0) {
>> >> >>
>> >> >>                       /* Wait until cx/ep0 fifo empty */
>> >> >>                       fotg210_cxwait(chip, CXFIFO_CXFIFOE);
>> >> >>
>> >> >> +                     udelay_masked(1);
>> >> >
>> >> > Why don't you use regular udelay() here please ? Also, how exactly
>> >> > does the delay help solving the unreliability problem please?
>> >>
>> >> 1. No specific reason at all, I'll use regular udelay() in next version.
>> >> :)
>> >>
>> >> 2. The fifo size of ep0 is 64 bytes, and my driver is supposed to make
>> >> sure the fifo empty
>> >>
>> >>     before filling up the fifo. However there is a hardware bug that
>> >>
>> >> the fifo empty indication is somehow
>> >>
>> >>     a bit earlier than fifo reset. So if I don't add an extra delay
>> >>
>> >> here, the data might be corrupted (i.e., 1 byte missing.)
>> >>
>> >>     And after a couple of tests, it looks like that 1 usec is good
>> >>
>> >> enough for this.
>> >
>> > Ick, but I guess you guys know the IP blocks' sourcecode.
>>
>> Yes, but I don't have the access permission , and I'm not a member of
>> the IP verification team......
>>
>> Anyway I'll try to call the IP owner see if he's willing to do FPGA
>> verification.
>
> Would be nice, but I dont mind picking it even without such confirmation.
> Thanks!
>
> Best regards,
> Marek Vasut

Sorry, this bug is schedule as a lowest priority.
Because I'm the only one who have this issue in Faraday.
i.e., I'm the only one to configure the FOTG210 as an Ethernet gadget,
while others always configure it as an bulk-only mass storage gadget.
And only under Ethernet gadget mode, we'll have to handle EP0 packets
with size greater than 64Bytes.

So I can't clearly figure out the root cause of the buggy Faraday FOTG210.
-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH v2 0/2] usb: gadget: fotg210: workaround & new hardware support

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

1. It's known that EP0 fifo empty indication is not reliable, an extra delay
   is necessary to avoid data corruption while handling packets with size
   greater than 64 bytes.

2. Since hardware revision 1.11.0, some fields of interrupt status registers
   are now write-1-clear.

Changes for v2:
	- usb: gadget: fotg210: add w1c interrupt status support:
	  By writting 1 then 0 to get rid of the use of
	  CONFIG_USB_GADGET_FOTG210_ISRW1C.
	  Thanks for Marek's comments.
	- usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
	  udelay_masked() -> udelay(), and patch comment updates.

Kuo-Jung Su (2):
  usb: gadget: fotg210: add w1c interrupt status support
  usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

 drivers/usb/gadget/fotg210.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

--
1.7.9.5

[U-Boot] [PATCH v2 1/2] usb: gadget: fotg210: add w1c interrupt status support

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

Since hardware revision 1.11.0, the following interrupt status
registers are now W1C (i.e., write 1 clear):

1. Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
2. Interrupt Source Group 2 Register (0x14C) (All bits)

And before revision 1.11.0, these registers are all R/W.
Which means software must write a 0 to clear the status.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
CC: Marek Vasut <marex@denx.de>
---
Changes for v2:
	- By writting 1 then 0 to get rid of the use of
	  CONFIG_USB_GADGET_FOTG210_ISRW1C.
	  Thanks for Marek's comments.

 drivers/usb/gadget/fotg210.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
index 6e19db1..cc5c507 100644
--- a/drivers/usb/gadget/fotg210.c
+++ b/drivers/usb/gadget/fotg210.c
@@ -847,6 +847,13 @@ int usb_gadget_handle_interrupts(void)
 	/* CX interrupts */
 	if (gisr & GISR_GRP0) {
 		st = readl(&regs->gisr0);
+		/*
+		 * Write 1 and then 0 works for both W1C & RW.
+		 *
+		 * HW v1.11.0+: It's a W1C register (write 1 clear)
+		 * HW v1.10.0-: It's a R/W register (write 0 clear)
+		 */
+		writel(st & GISR0_CXABORT, &regs->gisr0);
 		writel(0, &regs->gisr0);

 		if (st & GISR0_CXERR)
@@ -873,6 +880,13 @@ int usb_gadget_handle_interrupts(void)
 	/* Device Status Interrupts */
 	if (gisr & GISR_GRP2) {
 		st = readl(&regs->gisr2);
+		/*
+		 * Write 1 and then 0 works for both W1C & RW.
+		 *
+		 * HW v1.11.0+: It's a W1C register (write 1 clear)
+		 * HW v1.10.0-: It's a R/W register (write 0 clear)
+		 */
+		writel(st, &regs->gisr2);
 		writel(0, &regs->gisr2);

 		if (st & GISR2_RESET)
--
1.7.9.5

[U-Boot] [PATCH v2 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Kuo-Jung Su]

From: Kuo-Jung Su <dantesu@faraday-tech.com>

The fifo size of ep0 is 64 bytes, and if the packet size grater than
64 bytes, the driver would have to fill up the fifo multiple times,
and before filling up the fifo, the driver should make sure the fifo
is empty by checking fifo empty indication.

However there is a hardware bug that the fifo empty indication is
somehow a bit earlier than fifo reset. So if I don't add an extra
delay here, the data might be corrupted. (i.e., 1 byte missing)

After a couple of tests, it truns out that 1 usec is good enough.

This workaround should be applied to all hardware revisions.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
CC: Marek Vasut <marex@denx.de>
---
Changes for v2:
	- udelay_masked() -> udelay(), and patch comment updates.

 drivers/usb/gadget/fotg210.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
index cc5c507..3acf6a1 100644
--- a/drivers/usb/gadget/fotg210.c
+++ b/drivers/usb/gadget/fotg210.c
@@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep, struct fotg210_request *req)
 		if (ep->id == 0) {
 			/* Wait until cx/ep0 fifo empty */
 			fotg210_cxwait(chip, CXFIFO_CXFIFOE);
+			udelay(1);
 			writel(DMAFIFO_CX, &regs->dma_fifo);
 		} else {
 			/* Wait until epx fifo empty */
--
1.7.9.5

[U-Boot] [PATCH 2/2] usb: gadget: fotg210: EP0 fifo empty indication is non-reliable

[Marek Vasut]

On Friday, December 20, 2013 at 04:45:39 AM, Kuo-Jung Su wrote:
> 2013/12/19 Marek Vasut <marex@denx.de>:
> > On Thursday, December 19, 2013 at 08:07:00 AM, Kuo-Jung Su wrote:
> >> 2013/12/19 Marek Vasut <marex@denx.de>:
> >> > On Thursday, December 19, 2013 at 01:50:55 AM, Kuo-Jung Su wrote:
> >> >> 2013/12/18 Marek Vasut <marex@denx.de>:
> >> >> > On Wednesday, December 18, 2013 at 08:24:49 AM, Kuo-Jung Su wrote:
> >> >> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> >> >> 
> >> >> >> Because the EP0 fifo empty indication is non-reliable,
> >> >> >> an extra delay is necessary to avoid data corruption while
> >> >> >> handling packets with size greater than 64 bytes.
> >> >> >> 
> >> >> >> This workaround should be applied to all hardware revisions.
> >> >> >> 
> >> >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> >> >> CC: Marek Vasut <marex@denx.de>
> >> >> >> ---
> >> >> >> 
> >> >> >>  drivers/usb/gadget/fotg210.c |    1 +
> >> >> >>  1 file changed, 1 insertion(+)
> >> >> >> 
> >> >> >> diff --git a/drivers/usb/gadget/fotg210.c
> >> >> >> b/drivers/usb/gadget/fotg210.c index e3a61cc..14bfec6 100644
> >> >> >> --- a/drivers/usb/gadget/fotg210.c
> >> >> >> +++ b/drivers/usb/gadget/fotg210.c
> >> >> >> @@ -245,6 +245,7 @@ static int fotg210_dma(struct fotg210_ep *ep,
> >> >> >> struct fotg210_request *req) if (ep->id == 0) {
> >> >> >> 
> >> >> >>                       /* Wait until cx/ep0 fifo empty */
> >> >> >>                       fotg210_cxwait(chip, CXFIFO_CXFIFOE);
> >> >> >> 
> >> >> >> +                     udelay_masked(1);
> >> >> > 
> >> >> > Why don't you use regular udelay() here please ? Also, how exactly
> >> >> > does the delay help solving the unreliability problem please?
> >> >> 
> >> >> 1. No specific reason at all, I'll use regular udelay() in next
> >> >> version.
> >> >> 
> >> >> :)
> >> >> 
> >> >> 2. The fifo size of ep0 is 64 bytes, and my driver is supposed to
> >> >> make sure the fifo empty
> >> >> 
> >> >>     before filling up the fifo. However there is a hardware bug that
> >> >> 
> >> >> the fifo empty indication is somehow
> >> >> 
> >> >>     a bit earlier than fifo reset. So if I don't add an extra delay
> >> >> 
> >> >> here, the data might be corrupted (i.e., 1 byte missing.)
> >> >> 
> >> >>     And after a couple of tests, it looks like that 1 usec is good
> >> >> 
> >> >> enough for this.
> >> > 
> >> > Ick, but I guess you guys know the IP blocks' sourcecode.
> >> 
> >> Yes, but I don't have the access permission , and I'm not a member of
> >> the IP verification team......
> >> 
> >> Anyway I'll try to call the IP owner see if he's willing to do FPGA
> >> verification.
> > 
> > Would be nice, but I dont mind picking it even without such confirmation.
> > Thanks!
> > 
> > Best regards,
> > Marek Vasut
> 
> Sorry, this bug is schedule as a lowest priority.
> Because I'm the only one who have this issue in Faraday.
> i.e., I'm the only one to configure the FOTG210 as an Ethernet gadget,
> while others always configure it as an bulk-only mass storage gadget.
> And only under Ethernet gadget mode, we'll have to handle EP0 packets
> with size greater than 64Bytes.
> 
> So I can't clearly figure out the root cause of the buggy Faraday FOTG210.

OK, don't worry about it. Thanks for checking!

Best regards,

[U-Boot] [PATCH v2 0/2] usb: gadget: fotg210: workaround & new hardware support

[Marek Vasut]

On Friday, December 20, 2013 at 05:32:58 AM, Kuo-Jung Su wrote:
> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> 
> 1. It's known that EP0 fifo empty indication is not reliable, an extra
> delay is necessary to avoid data corruption while handling packets with
> size greater than 64 bytes.
> 
> 2. Since hardware revision 1.11.0, some fields of interrupt status
> registers are now write-1-clear.
> 
> Changes for v2:
> 	- usb: gadget: fotg210: add w1c interrupt status support:
> 	  By writting 1 then 0 to get rid of the use of
> 	  CONFIG_USB_GADGET_FOTG210_ISRW1C.
> 	  Thanks for Marek's comments.
> 	- usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
> 	  udelay_masked() -> udelay(), and patch comment updates.
> 
> Kuo-Jung Su (2):
>   usb: gadget: fotg210: add w1c interrupt status support
>   usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
> 
>  drivers/usb/gadget/fotg210.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Applied both , thanks!

btw. is this FOTG210 stuff used by any platform or is this just a dead code ?

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 0/2] usb: gadget: fotg210: workaround & new hardware support

[Kuo-Jung Su]

2013/12/20 Marek Vasut <marex@denx.de>:
> On Friday, December 20, 2013 at 05:32:58 AM, Kuo-Jung Su wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>
>> 1. It's known that EP0 fifo empty indication is not reliable, an extra
>> delay is necessary to avoid data corruption while handling packets with
>> size greater than 64 bytes.
>>
>> 2. Since hardware revision 1.11.0, some fields of interrupt status
>> registers are now write-1-clear.
>>
>> Changes for v2:
>>       - usb: gadget: fotg210: add w1c interrupt status support:
>>         By writting 1 then 0 to get rid of the use of
>>         CONFIG_USB_GADGET_FOTG210_ISRW1C.
>>         Thanks for Marek's comments.
>>       - usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
>>         udelay_masked() -> udelay(), and patch comment updates.
>>
>> Kuo-Jung Su (2):
>>   usb: gadget: fotg210: add w1c interrupt status support
>>   usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
>>
>>  drivers/usb/gadget/fotg210.c |   15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>
> Applied both , thanks!
>
> btw. is this FOTG210 stuff used by any platform or is this just a dead code ?
>

It's still an active IP, and the only USB 2.0 (host, device, otg)
controller we have currently in Faraday.

It's used in upcoming Faraday A369 SoC patch set (I'm still waiting
for the dependant NAND driver: ftnandc021 to be commited),

and the new SoC platforms (For politics reason,  they won't be
released by myself.) in year 2013.

-- 
Best wishes,
Kuo-Jung Su

[U-Boot] [PATCH v2 0/2] usb: gadget: fotg210: workaround & new hardware support

[Marek Vasut]

On Monday, December 23, 2013 at 01:50:36 AM, Kuo-Jung Su wrote:
> 2013/12/20 Marek Vasut <marex@denx.de>:
> > On Friday, December 20, 2013 at 05:32:58 AM, Kuo-Jung Su wrote:
> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
> >> 
> >> 1. It's known that EP0 fifo empty indication is not reliable, an extra
> >> delay is necessary to avoid data corruption while handling packets with
> >> size greater than 64 bytes.
> >> 
> >> 2. Since hardware revision 1.11.0, some fields of interrupt status
> >> registers are now write-1-clear.
> >> 
> >> Changes for v2:
> >>       - usb: gadget: fotg210: add w1c interrupt status support:
> >>         By writting 1 then 0 to get rid of the use of
> >>         CONFIG_USB_GADGET_FOTG210_ISRW1C.
> >>         Thanks for Marek's comments.
> >>       
> >>       - usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
> >>       
> >>         udelay_masked() -> udelay(), and patch comment updates.
> >> 
> >> Kuo-Jung Su (2):
> >>   usb: gadget: fotg210: add w1c interrupt status support
> >>   usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
> >>  
> >>  drivers/usb/gadget/fotg210.c |   15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> > 
> > Applied both , thanks!
> > 
> > btw. is this FOTG210 stuff used by any platform or is this just a dead
> > code ?
> 
> It's still an active IP, and the only USB 2.0 (host, device, otg)
> controller we have currently in Faraday.
> 
> It's used in upcoming Faraday A369 SoC patch set (I'm still waiting
> for the dependant NAND driver: ftnandc021 to be commited),
> 
> and the new SoC platforms (For politics reason,  they won't be
> released by myself.) in year 2013.

OK, glad to see this won't be dead code soon. Next time, I suppose it'd be 
better to commit platform first, drivers afterwards, so we dont have dead code 
for an extended period of time.

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 0/2] usb: gadget: fotg210: workaround & new hardware support

[Kuo-Jung Su]

2013/12/23 Marek Vasut <marex@denx.de>:
> On Monday, December 23, 2013 at 01:50:36 AM, Kuo-Jung Su wrote:
>> 2013/12/20 Marek Vasut <marex@denx.de>:
>> > On Friday, December 20, 2013 at 05:32:58 AM, Kuo-Jung Su wrote:
>> >> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>> >>
>> >> 1. It's known that EP0 fifo empty indication is not reliable, an extra
>> >> delay is necessary to avoid data corruption while handling packets with
>> >> size greater than 64 bytes.
>> >>
>> >> 2. Since hardware revision 1.11.0, some fields of interrupt status
>> >> registers are now write-1-clear.
>> >>
>> >> Changes for v2:
>> >>       - usb: gadget: fotg210: add w1c interrupt status support:
>> >>         By writting 1 then 0 to get rid of the use of
>> >>         CONFIG_USB_GADGET_FOTG210_ISRW1C.
>> >>         Thanks for Marek's comments.
>> >>
>> >>       - usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
>> >>
>> >>         udelay_masked() -> udelay(), and patch comment updates.
>> >>
>> >> Kuo-Jung Su (2):
>> >>   usb: gadget: fotg210: add w1c interrupt status support
>> >>   usb: gadget: fotg210: EP0 fifo empty indication is non-reliable
>> >>
>> >>  drivers/usb/gadget/fotg210.c |   15 +++++++++++++++
>> >>  1 file changed, 15 insertions(+)
>> >
>> > Applied both , thanks!
>> >
>> > btw. is this FOTG210 stuff used by any platform or is this just a dead
>> > code ?
>>
>> It's still an active IP, and the only USB 2.0 (host, device, otg)
>> controller we have currently in Faraday.
>>
>> It's used in upcoming Faraday A369 SoC patch set (I'm still waiting
>> for the dependant NAND driver: ftnandc021 to be commited),
>>
>> and the new SoC platforms (For politics reason,  they won't be
>> released by myself.) in year 2013.
>
> OK, glad to see this won't be dead code soon. Next time, I suppose it'd be
> better to commit platform first, drivers afterwards, so we dont have dead code
> for an extended period of time.
>

Got it, thanks


-- 
Best wishes,
Kuo-Jung Su