Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Chen <peter.chen@nxp.com>
To: Pawel Laszczak <pawell@cadence.com>
Cc: "balbi@kernel.org" <balbi@kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>, "rogerq@ti.com" <rogerq@ti.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	Jun Li <jun.li@nxp.com>
Subject: Re: [PATCH 2/2] usb: cdns3: gadget: toggle cycle bit before reset endpoint
Date: Sun, 16 Feb 2020 13:42:54 +0000
Message-ID: <20200216134255.GA12539@b29397-desktop> (raw)
In-Reply-To: <BYAPR07MB47098D0E50FA44A8111E94A2DD150@BYAPR07MB4709.namprd07.prod.outlook.com>

On 20-02-14 07:41:58, Pawel Laszczak wrote:
> >
> >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_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.
> >
> >Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> >Signed-off-by: Peter Chen <peter.chen@nxp.com>
> >---
> > drivers/usb/cdns3/gadget.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> >index 1d8a2af35bb0..7b6bb96b91d1 100644
> >--- a/drivers/usb/cdns3/gadget.c
> >+++ b/drivers/usb/cdns3/gadget.c
> >@@ -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;
> >+	}
> >+
> 
> What about doing simple read/write on ep_traddr register:
> 	u32 ep_traddr; 
> 	ep_traddr = readl(&priv_dev->regs->ep_traddr);
> 
> > 	writel(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

> 
> 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. 
> 
> >
> > 	/* 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
> 

-- 

Thanks,
Peter Chen

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14  7:14 [PATCH 0/2] usb: cdns3: two fixes for gadget Peter Chen
2020-02-14  7:14 ` [PATCH 1/2] usb: cdns3: gadget: link trb should point to next request Peter Chen
2020-02-14  7:14 ` [PATCH 2/2] usb: cdns3: gadget: toggle cycle bit before reset endpoint Peter Chen
2020-02-14  7:41   ` Pawel Laszczak
2020-02-16 13:42     ` Peter Chen [this message]
2020-02-14  8:48   ` Roger Quadros
2020-02-16 13:56     ` Peter Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200216134255.GA12539@b29397-desktop \
    --to=peter.chen@nxp.com \
    --cc=balbi@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jun.li@nxp.com \
    --cc=linux-imx@nxp.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=pawell@cadence.com \
    --cc=rogerq@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git