[U-Boot] [PATCH 0/2] usb: gadget: fotg210: workaround & new hardware support
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
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(®s->gisr0); +#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C + writel(st & GISR0_CXABORT, ®s->gisr0); +#else writel(0, ®s->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(®s->gisr2); +#ifdef CONFIG_USB_GADGET_FOTG210_ISRW1C + writel(st, ®s->gisr2); +#else writel(0, ®s->gisr2); - +#endif if (st & GISR2_RESET) printf("fotg210: reset by host\n"); else if (st & GISR2_SUSPEND)
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 ?
- Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
- 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(®s->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
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.
- Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
- 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(®s->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
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.
- Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
- 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(®s->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
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.
- Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
- 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(®s->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
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.
- Interrupt Source Group 0 Register (0x144) (EP0 Abort: BIT5)
- 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(®s->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
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, ®s->dma_fifo); } else { /* Wait until epx fifo empty */
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, ®s->dma_fifo); } else { /* Wait until epx fifo empty */
Best regards, Marek Vasut
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, ®s->dma_fifo); } else { /* Wait until epx fifo empty */Best regards, 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?
No specific reason at all, I'll use regular udelay() in next version. :)
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
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?
No specific reason at all, I'll use regular udelay() in next version. :)
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
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?
- No specific reason at all, I'll use regular udelay() in next version.
:)
- 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 thatthe fifo empty indication is somehow
a bit earlier than fifo reset. So if I don't add an extra delayhere, the data might be corrupted (i.e., 1 byte missing.)
And after a couple of tests, it looks like that 1 usec is goodenough 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
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?
- No specific reason at all, I'll use regular udelay() in next version.
:)
- 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 thatthe fifo empty indication is somehow
a bit earlier than fifo reset. So if I don't add an extra delayhere, the data might be corrupted (i.e., 1 byte missing.)
And after a couple of tests, it looks like that 1 usec is goodenough 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.
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?
- No specific reason at all, I'll use regular udelay() in next
version.
:)
- 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 thatthe fifo empty indication is somehow
a bit earlier than fifo reset. So if I don't add an extra delayhere, the data might be corrupted (i.e., 1 byte missing.)
And after a couple of tests, it looks like that 1 usec is goodenough 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,
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
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(®s->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, ®s->gisr0); writel(0, ®s->gisr0);
if (st & GISR0_CXERR) @@ -873,6 +880,13 @@ int usb_gadget_handle_interrupts(void) /* Device Status Interrupts */ if (gisr & GISR_GRP2) { st = readl(®s->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, ®s->gisr2); writel(0, ®s->gisr2);
if (st & GISR2_RESET) -- 1.7.9.5
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, ®s->dma_fifo); } else { /* Wait until epx fifo empty */ -- 1.7.9.5
On Friday, December 20, 2013 at 05:32:58 AM, Kuo-Jung Su wrote:
From: Kuo-Jung Su dantesu@faraday-tech.com
- 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.
- 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
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
- 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.
- 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.
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
- 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.
- 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
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
- 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.
- 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
participants (2)
-
Kuo-Jung Su -
Marek Vasut