usb: cdns3: two fixes for gadget
Subsystems (3)
cover usb: cdns3: two fixes for gadget
These two issues are found during run "Error Recovery Test" for the latest USB CV MSC test, the TRB doesn't advance correctly after dequeue and clear halt. With these two fixes, the test can be passed.
Peter Chen (2):
usb: cdns3: gadget: link trb should point to next request
usb: cdns3: gadget: toggle cycle bit before reset endpoint
drivers/usb/cdns3/gadget.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/2 usb: cdns3: gadget: link trb should point to next request
It has marked the dequeue trb as link trb, but its next segment pointer is still itself, it causes the transfer can't go on. Fix it by set its pointer as the trb address for the next request.
--- drivers/usb/cdns3/gadget.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/cdns3/gadget.c +1 -1
@@ -2550,7 +2550,7 @@ int cdns3_gadget_ep_dequeue(struct usb_ep *ep,/* Update ring only if removed request is on pending_req_list list */ if (req_on_hw_ring) { link_trb->buffer =TRB_BUFFER (priv_ep->trb_pool_dma + - (priv_req->start_trb *TRB_SIZE )); + ((priv_req->end_trb +1 ) *TRB_SIZE )); link_trb->control = (link_trb->control &TRB_CYCLE ) |TRB_TYPE (TRB_LINK ) |TRB_CHAIN ;
Signature · 1 line
2.17.1
PATCH 2/2 usb: cdns3: gadget: toggle cycle bit before reset endpoint
In reply to Peter ChenIf there are TRBs pending during clear stall and reset endpoint, the DMA will advance after reset operation, but it doesn't be expected, since the data has still not be ready (For OUT, the data has still not received). After the data is ready, there isn't any interrupt since the EP_TRADDR has already points to next TRB entry.
To fix it, it toggles cyclt bit before reset operation, and restore it after reset, it could keep DMA stopping.
--- drivers/usb/cdns3/gadget.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/usb/cdns3/gadget.c +14 -3
@@ -2595,11 +2595,21 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) {struct cdns3_device *priv_dev = priv_ep->cdns3_dev;struct usb_request *request; +struct cdns3_request *priv_req; +struct cdns3_trb *trb =NULL ;int ret;int val;trace_cdns3_halt (priv_ep,0 ,0 ); + request =cdns3_next_request (&priv_ep->pending_req_list); +if (request) { + priv_req =to_cdns3_request (request); + trb = priv_req->trb; +if (trb) + trb->control = trb->control ^1 ; + } +writel (EP_CMD_CSTALL |EP_CMD_EPRST , &priv_dev->regs->ep_cmd);/* wait for EPRST cleared */
@@ -2610,10 +2620,11 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) priv_ep->flags &= ~(EP_STALLED |EP_STALL_PENDING ); - request =cdns3_next_request (&priv_ep->pending_req_list); - -if (request) +if (request) { +if (trb) + trb->control = trb->control ^1 ;cdns3_rearm_transfer (priv_ep,1 ); + }cdns3_start_all_request (priv_dev, priv_ep);return ret;
Signature · 1 line
2.17.1
Pawel Laszczak
maintainerRE: [PATCH 2/2] usb: cdns3: gadget: toggle cycle bit before reset endpoint
Quoted 29 lines
If there are TRBs pending during clear stall and reset endpoint, the DMA will advance after reset operation, but it doesn't be expected, since the data has still not be ready (For OUT, the data has still not received). After the data is ready, there isn't any interrupt since the
EP_TRADDRhas already points to next TRB entry.To fix it, it toggles cyclt bit before reset operation, and restore it after reset, it could keep DMA stopping.
Fixes 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")Signed-off-by Peter Chen <[email protected]>---
drivers/usb/cdns3/gadget.c| 17 ++++++++++++++---1 file changed, 14 insertions(+), 3 deletions(-)drivers/usb/cdns3/gadget.c +2
-0@@ -2595,11 +2595,21 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) {struct cdns3_device *priv_dev = priv_ep->cdns3_dev;struct usb_request *request; +struct cdns3_request *priv_req; +struct cdns3_trb *trb =NULL ;int ret;int val;
drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2612
Quoted 10 lines
drivers/usb/cdns3/gadget.c +8
-0trace_cdns3_halt (priv_ep,0 ,0 ); + request =cdns3_next_request (&priv_ep->pending_req_list); +if (request) { + priv_req =to_cdns3_request (request); + trb = priv_req->trb; +if (trb) + trb->control = trb->control ^1 ; + } +
What about doing simple read/write on ep_traddr register:
u32 ep_traddr; ep_traddr = readl(&priv_dev->regs->ep_traddr);
drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2613
Quoted 1 line
drivers/usb/cdns3/gadget.c +0
-0writel (EP_CMD_CSTALL |EP_CMD_EPRST , &priv_dev->regs->ep_cmd);
writel(&priv_dev->regs->ep_traddr, ep_traddr);
It should work in the same way but is simpler.
And maybe we could add some comment because this code look little strange. Something like:
When endpoint is armed during endpoint reset the controller can advance TRADDR to next TD, so driver need to restore the previous value.
Quoted 19 lines
drivers/usb/cdns3/gadget.c +4
-4/* wait for EPRST cleared */ @@ -2610,10 +2620,11 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) priv_ep->flags &= ~(EP_STALLED |EP_STALL_PENDING ); - request =cdns3_next_request (&priv_ep->pending_req_list); - -if (request) +if (request) { +if (trb) + trb->control = trb->control ^1 ;cdns3_rearm_transfer (priv_ep,1 ); + }cdns3_start_all_request (priv_dev, priv_ep);return ret; --2.17.1
Re: [PATCH 2/2] usb: cdns3: gadget: toggle cycle bit before reset endpoint
Quoted 41 lines · Pawel Laszczak
Quoted 29 lines
If there are TRBs pending during clear stall and reset endpoint, the DMA will advance after reset operation, but it doesn't be expected, since the data has still not be ready (For OUT, the data has still not received). After the data is ready, there isn't any interrupt since the
EP_TRADDRhas already points to next TRB entry.To fix it, it toggles cyclt bit before reset operation, and restore it after reset, it could keep DMA stopping.
Fixes 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")Signed-off-by Peter Chen <[email protected]>---
drivers/usb/cdns3/gadget.c| 17 ++++++++++++++---1 file changed, 14 insertions(+), 3 deletions(-)drivers/usb/cdns3/gadget.c +2
-0@@ -2595,11 +2595,21 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) {struct cdns3_device *priv_dev = priv_ep->cdns3_dev;struct usb_request *request; +struct cdns3_request *priv_req; +struct cdns3_trb *trb =NULL ;int ret;int val;drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2612
Quoted 10 lines
drivers/usb/cdns3/gadget.c +8
-0trace_cdns3_halt (priv_ep,0 ,0 ); + request =cdns3_next_request (&priv_ep->pending_req_list); +if (request) { + priv_req =to_cdns3_request (request); + trb = priv_req->trb; +if (trb) + trb->control = trb->control ^1 ; + } +What about doing simple read/write on
ep_traddrregister:
Quoted 8 lines
u32 ep_traddr; ep_traddr = readl(&priv_dev->regs->ep_traddr);drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2613
Quoted 1 line
drivers/usb/cdns3/gadget.c +0
-0writel (EP_CMD_CSTALL |EP_CMD_EPRST , &priv_dev->regs->ep_cmd);writel(&priv_dev->regs->ep_traddr, ep_traddr);It should work in the same way but is simpler.
No, we can't do thing like this since the controller may change TRB content during the reset. The trb address 0x96003024 is the pending one. At this context, I still not add changes for link trb.
file-storage-4050 [003] d..1 128.390623: cdns3_ep_queue: ep1out: req: ffff8008366ac100, req buff ffff8008378b0000, length: 0/1024 , status: -115, trb: [start:2, end:2: virt addr 0x0000000000000000], flags:0
file-storage-4050 [003] d..1 128.390628: cdns3_log: 5b110000.usb3: WA1 set guard file-storage-4050 [003] d..1 128.390633: cdns3_log: 5b110000.usb3: dorbel 1, dma_index 2, prev_enqueu 3
file-storage-4050 [003] d..1 128.390637: cdns3_log: 5b110000.usb3: WA1: update cycle bit file-storage-4050 [003] d..1 128.390639: cdns3_prepare_trb: ep1out: trb ffff00000a418024, dma buf: 0xfc7a1000, size: 1024, burst: 16 ctrl: 0x00000425 (C=1, T=0, ISP, IOC, Normal)
file-storage-4050 [003] d..1 128.390642: cdns3_log: 5b110000.usb3: Update ep_trbaddr for ep1out to 96003024 file-storage-4050 [003] d..1 128.390647: cdns3_ring:
Ring contents for ep1out:
Ring deq index: 3, trb: ffff00000a418024 (virt), 0x96003024 (dma)
Ring enq index: 4, trb: ffff00000a418030 (virt), 0x96003030 (dma)
free trbs: 28, CCS=1, PCS=1
@0x0000000096003000 fc79f000 0000001f 00000427
@0x000000009600300c fc7a0000 0000001f 00000427
@0x0000000096003018 fc7a0800 10020400 00000425
@0x0000000096003024 fc7a1000 10020400 00000425
@0x0000000096003030 00000000 00000000 00000000
@0x000000009600303c 00000000 00000000 00000000
@0x0000000096003048 00000000 00000000 00000000
@0x0000000096003054 00000000 00000000 00000000
@0x0000000096003060 00000000 00000000 00000000
@0x000000009600306c 00000000 00000000 00000000
@0x0000000096003078 00000000 00000000 00000000
@0x0000000096003084 00000000 00000000 00000000
file-storage-4050 [003] d..1 128.390654: cdns3_doorbell_epx: //Ding Dong ep1out, ep_trbaddr 96003024
irq/42-5b110000-2246 [001] d..1 128.390692: cdns3_ep0_irq: IRQ for ep0OUT: 00010c85 SETUP IOC TRBERR
irq/42-5b110000-2246 [001] d..1 128.390696: cdns3_ctrl_req: Clear Endpoint Feature(Halt ep1out)
irq/42-5b110000-2246 [001] d..1 128.390700: cdns3_log: 5b110000.usb3: Clear Stalled endpoint ep1out irq/42-5b110000-2246 [001] d..1 128.390719: cdns3_log: 5b110000.usb3: Resume transfer for ep1out, ep_sta:0xc00
irq/42-5b110000-2246 [001] d..1 128.390724: cdns3_ring:
Ring contents for ep1out: Ring deq index: 3, trb: ffff00000a418024 (virt), 0x96003024 (dma) Ring enq index: 4, trb: ffff00000a418030 (virt), 0x96003030 (dma) free trbs: 28, CCS=1, PCS=1 @0x0000000096003000 fc79f000 0000001f 00000427 @0x000000009600300c fc7a0000 0000001f 00000427 @0x0000000096003018 fc7a0800 10020400 00000425 @0x0000000096003024 fc7a1000 00000000 00000467 @0x0000000096003030 00000000 00000000 00000000 @0x000000009600303c 00000000 00000000 00000000 @0x0000000096003048 00000000 00000000 00000000 @0x0000000096003054 00000000 00000000 00000000
Peter
Quoted 25 lines
And maybe we could add some comment because this code look little strange. Something like:
When endpoint is armed during endpoint reset the controller can advance TRADDR to next TD, so driver need to restore the previous value.Quoted 19 lines
drivers/usb/cdns3/gadget.c +4
-4/* wait for EPRST cleared */ @@ -2610,10 +2620,11 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) priv_ep->flags &= ~(EP_STALLED |EP_STALL_PENDING ); - request =cdns3_next_request (&priv_ep->pending_req_list); - -if (request) +if (request) { +if (trb) + trb->control = trb->control ^1 ;cdns3_rearm_transfer (priv_ep,1 ); + }cdns3_start_all_request (priv_dev, priv_ep);return ret; --2.17.1
Signature · 2 lines
Thanks, Peter Chen
Re: [PATCH 2/2] usb: cdns3: gadget: toggle cycle bit before reset endpoint
In reply to Peter ChenQuoted 1 line · Peter Chen
If there are TRBs pending during clear stall and reset endpoint, the
s/and/or?
Quoted 1 line
DMA will advance after reset operation, but it doesn't be expected,
s/doesn't/isn't?
Quoted 1 line
since the data has still not be ready (For OUT, the data has still
s/"has still not be"/"is not yet"
(e.g. for OUT, the data is not yet available).
Quoted 1 line
not received). After the data is ready, there isn't any interrupt
s/"there isn't any"/"there won't be any"
Quoted 1 line
since the
EP_TRADDRhas already points to next TRB entry.
remove "has"
Quoted 2 lines
To fix it, it toggles cyclt bit before reset operation, and restore
s/cyclt/cycle
s/restore/restores
Quoted 1 line
it after reset, it could keep DMA stopping.
It prevents DMA from getting stuck up?
Quoted 18 lines
Fixes 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")Signed-off-by Peter Chen <[email protected]>--- drivers/usb/cdns3/gadget.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/usb/cdns3/gadget.c +2
-0@@ -2595,11 +2595,21 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) {struct cdns3_device *priv_dev = priv_ep->cdns3_dev;struct usb_request *request; +struct cdns3_request *priv_req; +struct cdns3_trb *trb =NULL ;int ret;
drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2610
Quoted 10 lines
drivers/usb/cdns3/gadget.c +6
-0int val;trace_cdns3_halt (priv_ep,0 ,0 ); + request =cdns3_next_request (&priv_ep->pending_req_list); +if (request) { + priv_req =to_cdns3_request (request); + trb = priv_req->trb; +if (trb) + trb->control = trb->control ^1 ;
use TRB_CYCLE macro instead of 1.
Is it better to toggle this bit or explicitly set/clear it?
Quoted 6 lines
drivers/usb/cdns3/gadget.c +2
-0+ } +writel (EP_CMD_CSTALL |EP_CMD_EPRST , &priv_dev->regs->ep_cmd);/* wait for EPRST cleared */
drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2625
Quoted 9 lines
drivers/usb/cdns3/gadget.c +3
-3@@ -2610,10 +2620,11 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) priv_ep->flags &= ~(EP_STALLED |EP_STALL_PENDING ); - request =cdns3_next_request (&priv_ep->pending_req_list); - -if (request) +if (request) { +if (trb) + trb->control = trb->control ^1 ;
same here.
Quoted 6 lines
drivers/usb/cdns3/gadget.c +1
-0cdns3_rearm_transfer (priv_ep,1 ); + }cdns3_start_all_request (priv_dev, priv_ep);return ret;
Signature · 5 lines
cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 2/2] usb: cdns3: gadget: toggle cycle bit before reset endpoint
Quoted 6 lines · Roger Quadros
Quoted 1 line · Peter Chen
If there are TRBs pending during clear stall and reset endpoint, the
s/and/or?
I think it is reset operation, just I observe it at these two operations together.
Quoted 17 lines
Quoted 2 lines
DMA will advance after reset operation, but it doesn't be expected,
s/doesn't/isn't?
Quoted 1 line
since the data has still not be ready (For OUT, the data has still
s/"has still not be"/"is not yet"
(e.g. for OUT, the data is not yet available).
Quoted 1 line
not received). After the data is ready, there isn't any interrupt
s/"there isn't any"/"there won't be any"
Quoted 1 line
since the
EP_TRADDRhas already points to next TRB entry.
Quoted 9 lines
remove "has"
Quoted 2 lines
To fix it, it toggles cyclt bit before reset operation, and restore
s/cyclt/cycle
s/restore/restores
Will change above typo.
Quoted 3 lines
Quoted 1 line
it after reset, it could keep DMA stopping.
It prevents DMA from getting stuck up?
It could avoid unexpected DMA advance later due to TRB content has changed during the reset.
Quoted 30 lines
Quoted 17 lines
Fixes 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")Signed-off-by Peter Chen <[email protected]>--- drivers/usb/cdns3/gadget.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/usb/cdns3/gadget.c +1
-0@@ -2595,11 +2595,21 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) {struct cdns3_device *priv_dev = priv_ep->cdns3_dev;struct usb_request *request; +struct cdns3_request *priv_req;drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2608
Quoted 10 lines
drivers/usb/cdns3/gadget.c +7
-0+struct cdns3_trb *trb =NULL ;int ret;int val;trace_cdns3_halt (priv_ep,0 ,0 ); + request =cdns3_next_request (&priv_ep->pending_req_list); +if (request) { + priv_req =to_cdns3_request (request); + trb = priv_req->trb; +if (trb) + trb->control = trb->control ^1 ;use
TRB_CYCLEmacro instead of 1.
Quoted 1 line
Is it better to toggle this bit or explicitly set/clear it?
Since we don't know the value for cycle bit, it is better just toggle it.
Quoted 15 lines
drivers/usb/cdns3/gadget.c int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) · line 2623
Quoted 13 lines
drivers/usb/cdns3/gadget.c +5
-3+ } +writel (EP_CMD_CSTALL |EP_CMD_EPRST , &priv_dev->regs->ep_cmd);/* wait for EPRST cleared */ @@ -2610,10 +2620,11 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep) priv_ep->flags &= ~(EP_STALLED |EP_STALL_PENDING ); - request =cdns3_next_request (&priv_ep->pending_req_list); - -if (request) +if (request) { +if (trb) + trb->control = trb->control ^1 ;same here.
Ok.
Signature · 2 lines
Thanks, Peter Chen