Patch [2/2] DaVinci CPPI TX DMA tasklet

Patch [2/2] DaVinci CPPI TX DMA tasklet

[Swaminathan S]

    Signed-off-by: Swaminathan S <swami.iyer-l0cyMroinI0@public.gmane.org>
    
    This patch implements a tasklet to poll for TX Endpoint FIFO
    empty status before completing a transfer.  This is required
    in the context of DaVinci CPPI DMA Tx transfers.  CPPI
    DMA engine generates completion interrupt before the data
    actually moves out of TX FIFO and the driver needs to save the
    Data toggle after ensuring that the FIFO is empty.

diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
index d443a61..9dd24c3 100644
--- a/drivers/usb/musb/davinci.c
+++ b/drivers/usb/musb/davinci.c
@@ -421,6 +421,12 @@ int __init musb_platform_init(struct musb *musb)
 		musb_readb(tibase, DAVINCI_USB_CTRL_REG));
 
 	musb->isr = davinci_interrupt;
+#ifdef CONFIG_USB_MUSB_HDRC_HCD
+#ifdef CONFIG_ARCH_DAVINCI
+	tasklet_init(&musb->fifo_check, musb_fifo_check_tasklet,
+			(unsigned long)musb);
+#endif
+#endif
 	return 0;
 }
 
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 08e421f..c17681b 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -154,7 +154,8 @@ static inline void cppi_host_txdma_start(struct musb_hw_ep *ep)
 
 	/* NOTE: no locks here; caller should lock and select EP */
 	txcsr = musb_readw(ep->regs, MUSB_TXCSR);
-	txcsr |= MUSB_TXCSR_DMAENAB | MUSB_TXCSR_H_WZC_BITS;
+	txcsr |= MUSB_TXCSR_DMAENAB | MUSB_TXCSR_DMAMODE |
+			MUSB_TXCSR_H_WZC_BITS;
 	musb_writew(ep->regs, MUSB_TXCSR, txcsr);
 }
 
@@ -356,6 +357,7 @@ musb_giveback(struct musb_qh *qh, struct urb *urb, int status)
 	usb_hcd_unlink_urb_from_ep(musb_to_hcd(musb), urb);
 
 	qh->is_ready = 0;
+	qh->num_req--;
 	__musb_giveback(musb, urb, status);
 	qh->is_ready = ready;
 
@@ -688,14 +690,15 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
 					| MUSB_TXCSR_TXPKTRDY
 					);
 			csr |= MUSB_TXCSR_MODE;
-
-			if (usb_gettoggle(urb->dev,
-					qh->epnum, 1))
-				csr |= MUSB_TXCSR_H_WR_DATATOGGLE
-					| MUSB_TXCSR_H_DATATOGGLE;
-			else
-				csr |= MUSB_TXCSR_CLRDATATOG;
-
+			if (hw_ep->tx_reinit) {
+				if (usb_gettoggle(urb->dev,
+						qh->epnum, 1))
+					csr |= MUSB_TXCSR_H_WR_DATATOGGLE
+						| MUSB_TXCSR_H_DATATOGGLE;
+				else
+					csr |= MUSB_TXCSR_CLRDATATOG;
+					hw_ep->tx_reinit = 0;
+			}
 			/* twice in case of double packet buffering */
 			musb_writew(epio, MUSB_TXCSR, csr);
 			/* REVISIT may need to clear FLUSHFIFO ... */
@@ -1235,6 +1238,9 @@ void musb_host_tx(struct musb *musb, u8 epnum)
 		/* REVISIT may need to clear FLUSHFIFO ... */
 		musb_writew(epio, MUSB_TXCSR, tx_csr);
 		musb_writeb(epio, MUSB_TXINTERVAL, 0);
+#ifdef CONFIG_ARCH_DAVINCI
+		hw_ep->fifo_flush_check = 0;
+#endif
 
 		done = true;
 	}
@@ -1298,8 +1304,20 @@ void musb_host_tx(struct musb *musb, u8 epnum)
 		/* set status */
 		urb->status = status;
 		urb->actual_length = qh->offset;
+#ifdef CONFIG_ARCH_DAVINCI
+		/* Check for FIFO empty status.  If not wait for the same
+		 * before completing the urb.  This ensures that the toggle
+		 * status is correctly preserved and data will not be lost.
+		 */
+		if (((tx_csr & MUSB_TXCSR_FIFONOTEMPTY) ||
+			(tx_csr & MUSB_TXCSR_TXPKTRDY)) &&
+			(qh->num_req == 1)) {
+			hw_ep->fifo_flush_check = 1;
+			tasklet_schedule(&musb->fifo_check);
+			goto finish;
+		}
+#endif
 		musb_advance_schedule(musb, urb, hw_ep, USB_DIR_OUT);
-
 	} else if (!(tx_csr & MUSB_TXCSR_DMAENAB)) {
 		/* WARN_ON(!buf); */
 
@@ -1776,6 +1794,7 @@ static int musb_urb_enqueue(
 	 */
 	if (qh) {
 		urb->hcpriv = qh;
+		qh->num_req++;
 		return 0;
 	}
 
@@ -1891,6 +1910,7 @@ static int musb_urb_enqueue(
 
 	if (ret == 0) {
 		urb->hcpriv = qh;
+		qh->num_req = 1;
 		/* FIXME set urb->start_frame for iso/intr, it's tested in
 		 * musb_start_urb(), but otherwise only konicawc cares ...
 		 */
@@ -1959,6 +1979,9 @@ static int musb_cleanup_urb(struct urb *urb, struct musb_qh *qh, int is_in)
 		musb_writew(epio, MUSB_TXCSR, csr);
 		/* flush cpu writebuffer */
 		csr = musb_readw(epio, MUSB_TXCSR);
+#ifdef CONFIG_ARCH_DAVINCI
+		ep->fifo_flush_check = 0;
+#endif
 	}
 	if (status == 0)
 		musb_advance_schedule(ep->musb, urb, ep, is_in);
@@ -2132,6 +2155,46 @@ static int musb_bus_suspend(struct usb_hcd *hcd)
 		return 0;
 }
 
+#ifdef CONFIG_ARCH_DAVINCI
+/* Tasklet routine to handle the completion request. Check for Fifo status
+ * before completing the request. Avoids false completions when data is still
+ * in the fifo
+ */
+void musb_fifo_check_tasklet(unsigned long data)
+{
+	struct musb		 *musb = (struct musb *)data;
+	u8			 epnum = 1, sch_tsklt = 0;
+	struct musb_hw_ep	 *hw_ep = NULL;
+	unsigned long		 flags;
+	u16			 csr;
+	struct musb_qh		*qh;
+
+	do {
+		hw_ep = &(musb->endpoints[epnum++]);
+		spin_lock_irqsave(&musb->lock, flags);
+		if (hw_ep->fifo_flush_check) {
+			csr = musb_readw(hw_ep->regs, MUSB_TXCSR);
+			if (((csr & MUSB_TXCSR_FIFONOTEMPTY) ||
+				(csr & MUSB_TXCSR_TXPKTRDY)))
+				sch_tsklt = 1;
+			else {
+				hw_ep->fifo_flush_check = 0;
+				qh = hw_ep->out_qh;
+				musb_advance_schedule(musb, next_urb(qh),
+							hw_ep, USB_DIR_OUT);
+				DBG(6, "Completed Tasklet %d\n", hw_ep->epnum);
+			}
+		}
+
+		spin_unlock_irqrestore(&musb->lock, flags);
+	} while (epnum < MUSB_C_NUM_EPS);
+
+	if (sch_tsklt)
+		tasklet_schedule(&musb->fifo_check);
+}
+#endif
+
+
 static int musb_bus_resume(struct usb_hcd *hcd)
 {
 	/* resuming child port does the work */
@@ -2164,3 +2227,4 @@ const struct hc_driver musb_hc_driver = {
 	/* .start_port_reset	= NULL, */
 	/* .hub_irq_enable	= NULL, */
 };
+
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

[Felipe Balbi]

On Thu, Aug 21, 2008 at 07:28:42PM +0530, ext Swaminathan S wrote:
> 
>     Signed-off-by: Swaminathan S <swami.iyer@ti.com>
>     
>     This patch implements a tasklet to poll for TX Endpoint FIFO
>     empty status before completing a transfer.  This is required
>     in the context of DaVinci CPPI DMA Tx transfers.  CPPI
>     DMA engine generates completion interrupt before the data
>     actually moves out of TX FIFO and the driver needs to save the
>     Data toggle after ensuring that the FIFO is empty.

same comments from before.

> diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
> index d443a61..9dd24c3 100644
> --- a/drivers/usb/musb/davinci.c
> +++ b/drivers/usb/musb/davinci.c
> @@ -421,6 +421,12 @@ int __init musb_platform_init(struct musb *musb)
>  		musb_readb(tibase, DAVINCI_USB_CTRL_REG));
>  
>  	musb->isr = davinci_interrupt;
> +#ifdef CONFIG_USB_MUSB_HDRC_HCD

use if (is_host_capable()) instead.

> +		if (hw_ep->fifo_flush_check) {
> +			csr = musb_readw(hw_ep->regs, MUSB_TXCSR);
> +			if (((csr & MUSB_TXCSR_FIFONOTEMPTY) ||
> +				(csr & MUSB_TXCSR_TXPKTRDY)))
> +				sch_tsklt = 1;
> +			else {

put {} on both branches.

> +

this new line is unnecessary

this was just a primary overview. I wanna test more this patch since it
changes a bit musb_ep_program() which would affect all archs using musb.

I'll leave my n810 testing during this weekend and get back to you later
on.

-- 
balbi

Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

[Dmitry Krivoschekov]

Hi Swami,

Swaminathan S wrote:
>     Signed-off-by: Swaminathan S <swami.iyer-l0cyMroinI0@public.gmane.org>
>     
>     This patch implements a tasklet to poll for TX Endpoint FIFO
>     empty status before completing a transfer.  This is required
>     in the context of DaVinci CPPI DMA Tx transfers.  CPPI
>     DMA engine generates completion interrupt before the data
>     actually moves out of TX FIFO 

I guess the DMA interrupt is generated after the data have been moved
from a s/w buffer to TX FIFO.
> and the driver needs to save the
>     Data toggle after ensuring that the FIFO is empty.

why don't rely on an endpoint interrupt which is generated after a USB
transaction is complete?


Thanks,
Dmitry



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

[David Brownell]

On Thursday 21 August 2008, Dmitry Krivoschekov wrote:
> why don't rely on an endpoint interrupt which is generated
> after a USB transaction is complete?

That's what I thought *was* working a couple years ago...

I take it that something broke since then?  Or is this
a corner case which didn't always work right?

I'm not sure this would be appropriate for mainline
until the DaVinci code can run there.  There are a
bunch of arch/arm/mach-davinci updates needed first.
I know Kevin was looking at some of this, but just
now I think enabling MUSB on a 2.6.27-rc4 DaVinci
config means a broken build.

- Dave

RE: Patch [2/2] DaVinci CPPI TX DMA tasklet

[Subbrathnam, Swaminathan]

Without these changes DMA on DaVinci has never worked traditionally.
We fixed this issue pretty early in DaVinci program ~2006 on the 2.6.10
treee.  We are now in the process of forward porting the musb patches
from that tree to the git tree.

We did some performance enhancements/bug fixes on that tree and hope to make
it available to the general community w.r.t MUSB.  This (specific to DaVinci)
 and the following patches (musb generic) are in that direction.

I discussed with Kevin on the DaVinci Git tree and Filipe here and they recommended that
I take the changes to linux-usb, linux-omap tree (w.r.t musb changes) to get a wider 
audience for review and acceptance.

Kevin would then pull in the changes as part of his regular synch ups.

This patch (+ CPPI DMA changes) in a different form has already been 
tested in the DaVinci git community and it resolved the USB DMA issues and
improved performance considerably as communicated by the community
members like Neuros.

I will be submitting the CPPI DMA changes as a seperate patch as it is big
and based on preliminary review needs to be broken down significantly to
enable better review and acceptance.

Thanks for your comments.

regards
swami

________________________________________
From: David Brownell [david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org]
Sent: Friday, August 22, 2008 12:45 AM
To: Dmitry Krivoschekov; Subbrathnam, Swaminathan
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org; Kevin Hilman
Subject: Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

On Thursday 21 August 2008, Dmitry Krivoschekov wrote:
> why don't rely on an endpoint interrupt which is generated
> after a USB transaction is complete?

That's what I thought *was* working a couple years ago...

I take it that something broke since then?  Or is this
a corner case which didn't always work right?

I'm not sure this would be appropriate for mainline
until the DaVinci code can run there.  There are a
bunch of arch/arm/mach-davinci updates needed first.
I know Kevin was looking at some of this, but just
now I think enabling MUSB on a 2.6.27-rc4 DaVinci
config means a broken build.

- Dave--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

[David Brownell]

On Thursday 21 August 2008, Subbrathnam, Swaminathan wrote:
> Without these changes DMA on DaVinci has never worked traditionally.
> We fixed this issue pretty early in DaVinci program ~2006 on the 2.6.10
> treee. 

I remember a recommendation not to use DMA, but that was primarily
because it seemed to hurt performance (unless it was configured to
provide incorrect semantics).  If there were also correctness issues,
2006 is too long ago for me to remember.  :)


> We are now in the process of forward porting the musb patches 
> from that tree to the git tree.
> 
> We did some performance enhancements/bug fixes on that tree and hope to make
> it available to the general community w.r.t MUSB.  This (specific to DaVinci)
> and the following patches (musb generic) are in that direction.

Great.  Seems to me those could be ready for the 2.6.28 merge window,
given some updates to the DaVinci platform support.

They'll need to be in nicely reviewable chunks ... I remember seeing
some DaVinci patches in late 2006 which broke on TUSB6010 silicon,
for example.

DMA in particular could really stand some cleanup.  Having four
different chunks of DMA code -- RX/TX vs Host/Peripheral -- with
ifdeffery for multiple DMA engines is ... chaotic.


> I discussed with Kevin on the DaVinci Git tree and Filipe here and they recommended that
> I take the changes to linux-usb, linux-omap tree (w.r.t musb changes) to get a wider 
> audience for review and acceptance.
> 
> Kevin would then pull in the changes as part of his regular synch ups.

I think the plan should be to have various SOC-specific trees (DaVinci,
OMAP, Blackfin, etc) stop hosting MUSB-specific patches.  They should
be pushed up to mainline ASAP ... the linux-omap tree should stop being
the place where the latest MUSB code sits.


> This patch (+ CPPI DMA changes) in a different form has already been 
> tested in the DaVinci git community

It has???   It's not git://source.mvista.com/git/linux-davinci-2.6 ...


>	 and it resolved the USB DMA issues and 
> improved performance considerably as communicated by the community
> members like Neuros.
> 
> I will be submitting the CPPI DMA changes as a seperate patch as it is big
> and based on preliminary review needs to be broken down significantly to
> enable better review and acceptance.

I'm quite willing to believe the CPPI stuff can be improved.  :)

- Dave


> Thanks for your comments.
> 
> regards
> swami
> 
> ________________________________________
> From: David Brownell [david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org]
> Sent: Friday, August 22, 2008 12:45 AM
> To: Dmitry Krivoschekov; Subbrathnam, Swaminathan
> Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org; Kevin Hilman
> Subject: Re: Patch [2/2] DaVinci CPPI TX DMA tasklet
> 
> On Thursday 21 August 2008, Dmitry Krivoschekov wrote:
> > why don't rely on an endpoint interrupt which is generated
> > after a USB transaction is complete?
> 
> That's what I thought *was* working a couple years ago...
> 
> I take it that something broke since then?  Or is this
> a corner case which didn't always work right?
> 
> I'm not sure this would be appropriate for mainline
> until the DaVinci code can run there.  There are a
> bunch of arch/arm/mach-davinci updates needed first.
> I know Kevin was looking at some of this, but just
> now I think enabling MUSB on a 2.6.27-rc4 DaVinci
> config means a broken build.
> 
> - Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

[Felipe Balbi]

On Thu, Aug 21, 2008 at 08:24:11PM -0700, David Brownell wrote:
> They'll need to be in nicely reviewable chunks ... I remember seeing
> some DaVinci patches in late 2006 which broke on TUSB6010 silicon,
> for example.
> 
> DMA in particular could really stand some cleanup.  Having four
> different chunks of DMA code -- RX/TX vs Host/Peripheral -- with
> ifdeffery for multiple DMA engines is ... chaotic.

Yeah, after the debugfs changes I plan to clean up those. Might take a
while due to internal tasks

> > I discussed with Kevin on the DaVinci Git tree and Filipe here and they recommended that
> > I take the changes to linux-usb, linux-omap tree (w.r.t musb changes) to get a wider 
> > audience for review and acceptance.
> > 
> > Kevin would then pull in the changes as part of his regular synch ups.
> 
> I think the plan should be to have various SOC-specific trees (DaVinci,
> OMAP, Blackfin, etc) stop hosting MUSB-specific patches.  They should
> be pushed up to mainline ASAP ... the linux-omap tree should stop being
> the place where the latest MUSB code sits.

It's not anymore, we Cc that list so interested people might also take a
look on the patches, since we pushed musb code via Greg's queue,
linux-usb is the place for musb discussion.

-- 
balbi

RE: Patch [2/2] DaVinci CPPI TX DMA tasklet

[Subbrathnam, Swaminathan]

Hi Dmitry,

Comments inlined.

swami


________________________________________
From: Dmitry Krivoschekov [dmitry.krivoschekov@gmail.com]
Sent: Thursday, August 21, 2008 8:39 PM
To: Subbrathnam, Swaminathan
Cc: linux-omap@vger.kernel.org; linux-usb@vger.kernel.org; felipe.balbi@nokia.com
Subject: Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

Hi Swami,

Swaminathan S wrote:
>     Signed-off-by: Swaminathan S <swami.iyer@ti.com>
>
>     This patch implements a tasklet to poll for TX Endpoint FIFO
>     empty status before completing a transfer.  This is required
>     in the context of DaVinci CPPI DMA Tx transfers.  CPPI
>     DMA engine generates completion interrupt before the data
>     actually moves out of TX FIFO

I guess the DMA interrupt is generated after the data have been moved
from a s/w buffer to TX FIFO.

<Swami> Yes.  This being an external DMA (unlike Mentor DMA) that was
the designed behavior.

> and the driver needs to save the
>     Data toggle after ensuring that the FIFO is empty.

why don't rely on an endpoint interrupt which is generated after a USB
transaction is complete?

<Swami> When in DMA mode (mode 1) Endpoint interrupt does not get
generated (only DMA completion interrupt) and hence cannot rely on that.

Thanks,
Dmitry

Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

[Felipe Balbi]

On Fri, Aug 22, 2008 at 07:01:26AM +0530, ext Subbrathnam, Swaminathan wrote:
> <Swami> When in DMA mode (mode 1) Endpoint interrupt does not get
> generated (only DMA completion interrupt) and hence cannot rely on that.

afaict, endpoint interrupt is generated case we get a short packet. Then
dma engine won't send that last short packet and an enpoint interrupt
will be generated, from that point you decide if you wanna use PIO or
reprogram the dma for mode 0.

-- 
balbi

RE: Patch [2/2] DaVinci CPPI TX DMA tasklet

[Subbrathnam, Swaminathan]

Felipe,
< afaict, endpoint interrupt is generated case we get a short packet. Then
< dma engine won't send that last short packet and an enpoint interrupt
< will be generated, from that point you decide if you wanna use PIO or
< reprogram the dma for mode 0.
	
DaVinci CPPI TX DMA requires operating in Mode 1 and only DMA interrupts are raised and endpoint interrupts are not raised (excepting under error conditions).


Regards

Swami

(Type "pspproducts" in you web browser for PSP info)

http://dbdwss01.india.ti.com/pspproducts/

PSP downloads at : http://software.ti.com/swcoe/intranet/reports/pds/PSP_releases.php

Office : +91-80-25048629

-----Original Message-----
From: Felipe Balbi [mailto:felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org] 
Sent: Friday, August 22, 2008 12:53 PM
To: Subbrathnam, Swaminathan
Cc: Dmitry Krivoschekov; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org
Subject: Re: Patch [2/2] DaVinci CPPI TX DMA tasklet

On Fri, Aug 22, 2008 at 07:01:26AM +0530, ext Subbrathnam, Swaminathan wrote:
> <Swami> When in DMA mode (mode 1) Endpoint interrupt does not get
> generated (only DMA completion interrupt) and hence cannot rely on that.

afaict, endpoint interrupt is generated case we get a short packet. Then
dma engine won't send that last short packet and an enpoint interrupt
will be generated, from that point you decide if you wanna use PIO or
reprogram the dma for mode 0.

-- 
balbi

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html