[PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Ajay Kumar Gupta]

Isochronous Tx DMA is getting programmed but never getting started
for CPPI and TUSB DMAs and thus Isochronous Tx doesn't work.

Fixing it by starting DMAs using musb_h_tx_dma_start().

Signed-off-by: Swaminathan S <swami.iyer@ti.com>
Signed-off-by: Babu Ravi <ravibabu@ti.com>
Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>
---
 drivers/usb/musb/musb_host.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index cf94511..e3ab40a 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
 		return;
 	} else	if (usb_pipeisoc(pipe) && dma) {
 		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
-				offset, length))
+				offset, length)) {
+			if (is_cppi_enabled() || tusb_dma_omap())
+				musb_h_tx_dma_start(hw_ep);
 			return;
+		}
 	} else	if (tx_csr & MUSB_TXCSR_DMAENAB) {
 		DBG(1, "not complete, but DMA enabled?\n");
 		return;
-- 
1.6.2.4

Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello.

Ajay Kumar Gupta wrote:

> Isochronous Tx DMA is getting programmed but never getting started
> for CPPI and TUSB DMAs and thus Isochronous Tx doesn't work.
>   

   That's not true.

> Fixing it by starting DMAs using musb_h_tx_dma_start().
>
> Signed-off-by: Swaminathan S <swami.iyer@ti.com>
> Signed-off-by: Babu Ravi <ravibabu@ti.com>
> Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>
>   

   NAK.

> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index cf94511..e3ab40a 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
>  		return;
>  	} else	if (usb_pipeisoc(pipe) && dma) {
>  		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
> -				offset, length))
> +				offset, length)) {
> +			if (is_cppi_enabled() || tusb_dma_omap())
> +				musb_h_tx_dma_start(hw_ep);
>  			return;
>   

   It will have been already started by this moment -- from 
musb_start_urb() which is enough . Otherwise it wouldn't work, and I've 
made sure it *does* work.

WBR, Sergei

Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello, I wrote:

>> Isochronous Tx DMA is getting programmed but never getting started
>> for CPPI and TUSB DMAs and thus Isochronous Tx doesn't work.   
>
>   That's not true.

   Well, this is only true iff URB_ISO_ASAP flag is *not* set for an 
URB. In this case, PIO is also not being started, so you should fix this 
too.

>> Signed-off-by: Swaminathan S <swami.iyer@ti.com>
>> Signed-off-by: Babu Ravi <ravibabu@ti.com>
>> Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>
>>   
> Fixing it by starting DMAs using musb_h_tx_dma_start().
>
>   NAK.

   Still NAK...

WBR, Sergei

RE: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Gupta, Ajay Kumar]

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com]
> Sent: Friday, August 28, 2009 2:53 PM
> To: Gupta, Ajay Kumar
> Cc: linux-usb@vger.kernel.org; linux-omap@vger.kernel.org;
> felipe.balbi@nokia.com; david-b@pacbell.net; Subbrathnam, Swaminathan; B,
> Ravi
> Subject: Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs
> 
> Hello.
> 
> Ajay Kumar Gupta wrote:
> 
> > Isochronous Tx DMA is getting programmed but never getting started
> > for CPPI and TUSB DMAs and thus Isochronous Tx doesn't work.
> >
> 
>    That's not true.

We have tested and it doesn't work with current driver. Have you tested it and was it working for you ?

> 
> > Fixing it by starting DMAs using musb_h_tx_dma_start().
> >
> > Signed-off-by: Swaminathan S <swami.iyer@ti.com>
> > Signed-off-by: Babu Ravi <ravibabu@ti.com>
> > Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>
> >
> 
>    NAK.
> 
> > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> > index cf94511..e3ab40a 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
> >  		return;
> >  	} else	if (usb_pipeisoc(pipe) && dma) {
> >  		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
> > -				offset, length))
> > +				offset, length)) {
> > +			if (is_cppi_enabled() || tusb_dma_omap())
> > +				musb_h_tx_dma_start(hw_ep);
> >  			return;
> >
> 
>    It will have been already started by this moment -- from
> musb_start_urb() which is enough . Otherwise it wouldn't work, and I've
> made sure it *does* work.


This part is being done at musb_host_rx() doing next packet programming within same urb and *not* starting next urb. Thus musb_start_urb() doesn't come into this path. So it wouldn't start the DMAs.

In case of PIO, it does load the FIFO and sets the TXPKTREADY.

-Ajay

> 
> WBR, Sergei
> 
> 

RE: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Gupta, Ajay Kumar]

> This part is being done at musb_host_rx() 

musb_host_tx() ofcourse.

> doing next packet programming
> within same urb and *not* starting next urb. Thus musb_start_urb() doesn't
> come into this path. So it wouldn't start the DMAs.
> 
> In case of PIO, it does load the FIFO and sets the TXPKTREADY.
> 
> -Ajay
> 
> >
> > WBR, Sergei
> >
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello.

Gupta, Ajay Kumar wrote:
>> -----Original Message-----
>> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com]
>> Sent: Friday, August 28, 2009 2:53 PM
>> To: Gupta, Ajay Kumar
>> Cc: linux-usb@vger.kernel.org; linux-omap@vger.kernel.org;
>> felipe.balbi@nokia.com; david-b@pacbell.net; Subbrathnam, Swaminathan; B,
>> Ravi
>> Subject: Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs
>>
>> Hello.
>>
>> Ajay Kumar Gupta wrote:
>>
>>     
>>> Isochronous Tx DMA is getting programmed but never getting started
>>> for CPPI and TUSB DMAs and thus Isochronous Tx doesn't work.
>>>
>>>       
>>    That's not true.
>>     
>
> We have tested and it doesn't work with current driver. Have you tested it and was it working for you ?
>   

   Not with the current driver, I must admit. I'll try it today...

>>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>>> index cf94511..e3ab40a 100644
>>> --- a/drivers/usb/musb/musb_host.c
>>> +++ b/drivers/usb/musb/musb_host.c
>>> @@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
>>>  		return;
>>>  	} else	if (usb_pipeisoc(pipe) && dma) {
>>>  		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
>>> -				offset, length))
>>> +				offset, length)) {
>>> +			if (is_cppi_enabled() || tusb_dma_omap())
>>> +				musb_h_tx_dma_start(hw_ep);
>>>  			return;
>>>
>>>       
>>    It will have been already started by this moment -- from
>> musb_start_urb() which is enough . Otherwise it wouldn't work, and I've
>> made sure it *does* work.
>>     
>
> This part is being done at musb_host_rx()

   You're doing it in musb_host_tx() actually. Although musb_host_rx() 
is also broken WRT the isochronous transfers.

>  doing next packet programming within same urb and *not* starting next urb. Thus musb_start_urb() doesn't come into this path.

   What? Read the code, please -- musb_start_urb() call should always 
precede musb_host_tx() which handles the DMA interrupt. Unless something 
clears DMAReqEnab after musb_start_urb() call, setting it only once 
should work.

> So it wouldn't start the DMAs.
>   

   How musb_host_tx() can be called without musb_start_urb()?

> In case of PIO, it does load the FIFO and sets the TXPKTREADY.
>   

   Only is URB_ISO_ASAP is not set.

WBR, Sergei

> -Ajay

WBR, Sergei

RE: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Subbrathnam, Swaminathan]

Sergei,
	Pl. do the required testing with and without the patch on the current tree for ISO transfers in Tx direction.  As Ajay indicated we have done the same and found it not working and hence the fix.

	ISO Rx is also broken and the patch for fixing the same is on the way.  Since that fix involves some bit of re-structuring it is delayed a bit.

	We can discuss further if need be.

Regards
swami

-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov-hkdhdckH98+B+jHODAdFcQ@public.gmane.org] 
Sent: Friday, August 28, 2009 3:33 PM
To: Gupta, Ajay Kumar
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org; david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org; Subbrathnam, Swaminathan; B, Ravi
Subject: Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

Hello.

Gupta, Ajay Kumar wrote:
>> -----Original Message-----
>> From: Sergei Shtylyov [mailto:sshtylyov-hkdhdckH98+B+jHODAdFcQ@public.gmane.org]
>> Sent: Friday, August 28, 2009 2:53 PM
>> To: Gupta, Ajay Kumar
>> Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>> felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org; david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org; Subbrathnam, Swaminathan; B,
>> Ravi
>> Subject: Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs
>>
>> Hello.
>>
>> Ajay Kumar Gupta wrote:
>>
>>     
>>> Isochronous Tx DMA is getting programmed but never getting started
>>> for CPPI and TUSB DMAs and thus Isochronous Tx doesn't work.
>>>
>>>       
>>    That's not true.
>>     
>
> We have tested and it doesn't work with current driver. Have you tested it and was it working for you ?
>   

   Not with the current driver, I must admit. I'll try it today...

>>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>>> index cf94511..e3ab40a 100644
>>> --- a/drivers/usb/musb/musb_host.c
>>> +++ b/drivers/usb/musb/musb_host.c
>>> @@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
>>>  		return;
>>>  	} else	if (usb_pipeisoc(pipe) && dma) {
>>>  		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
>>> -				offset, length))
>>> +				offset, length)) {
>>> +			if (is_cppi_enabled() || tusb_dma_omap())
>>> +				musb_h_tx_dma_start(hw_ep);
>>>  			return;
>>>
>>>       
>>    It will have been already started by this moment -- from
>> musb_start_urb() which is enough . Otherwise it wouldn't work, and I've
>> made sure it *does* work.
>>     
>
> This part is being done at musb_host_rx()

   You're doing it in musb_host_tx() actually. Although musb_host_rx() 
is also broken WRT the isochronous transfers.

>  doing next packet programming within same urb and *not* starting next urb. Thus musb_start_urb() doesn't come into this path.

   What? Read the code, please -- musb_start_urb() call should always 
precede musb_host_tx() which handles the DMA interrupt. Unless something 
clears DMAReqEnab after musb_start_urb() call, setting it only once 
should work.

> So it wouldn't start the DMAs.
>   

   How musb_host_tx() can be called without musb_start_urb()?

> In case of PIO, it does load the FIFO and sets the TXPKTREADY.
>   

   Only is URB_ISO_ASAP is not set.

WBR, Sergei

> -Ajay

WBR, Sergei



--
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] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello.

Subbrathnam, Swaminathan wrote:

> Sergei,
> 	Pl. do the required testing with and without the patch on the current tree for ISO transfers in Tx direction.  As Ajay indicated we have done the same and found it not working and hence the fix.

    Sigh, I'm now seeing it even witout testing... So, I'm sorry for all the 
noise. It was a result of my 2 patches clashing.

> 	ISO Rx is also broken and the patch for fixing the same is on the way.

    That's good to hear... musb_host_rx() was further broken WRT ISO 
trabnsers while the Mentor DMA case was being fixed.

WBR, Sergei
--
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] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello, I wrote:

>> In case of PIO, it does load the FIFO and sets the TXPKTREADY.
>>   
>
>   Only is URB_ISO_ASAP is not set.

   This should read "only if URB_ISO_ASAP is set". :-/

WBR, Sergei


--
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] musb: fix ISOC Tx programming for CPPI DMAs

[Gupta, Ajay Kumar]

> > We have tested and it doesn't work with current driver. Have you tested
> it and was it working for you ?
> >
> 
>    Not with the current driver, I must admit. I'll try it today...
> 
> >>> diff --git a/drivers/usb/musb/musb_host.c
> b/drivers/usb/musb/musb_host.c
> >>> index cf94511..e3ab40a 100644
> >>> --- a/drivers/usb/musb/musb_host.c
> >>> +++ b/drivers/usb/musb/musb_host.c
> >>> @@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
> >>>  		return;
> >>>  	} else	if (usb_pipeisoc(pipe) && dma) {
> >>>  		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
> >>> -				offset, length))
> >>> +				offset, length)) {
> >>> +			if (is_cppi_enabled() || tusb_dma_omap())
> >>> +				musb_h_tx_dma_start(hw_ep);
> >>>  			return;
> >>>
> >>>
> >>    It will have been already started by this moment -- from
> >> musb_start_urb() which is enough . Otherwise it wouldn't work, and I've
> >> made sure it *does* work.
> >>
> >
> > This part is being done at musb_host_rx()
> 
>    You're doing it in musb_host_tx() actually. Although musb_host_rx()
> is also broken WRT the isochronous transfers.
> 
> >  doing next packet programming within same urb and *not* starting next
> urb. Thus musb_start_urb() doesn't come into this path.
> 
>    What? Read the code, please -- musb_start_urb() call should always
> precede musb_host_tx() which handles the DMA interrupt. Unless something
> clears DMAReqEnab after musb_start_urb() call, setting it only once
> should work.

musb_start_urb() call does precede musb_host_tx() but when urb is *completed*. 

check the 'done' flag and musb_advance_schedule getting called in the path.

if (done) {
                /* set status */
                urb->status = status;
                urb->actual_length = qh->offset;
                musb_advance_schedule(musb, urb, hw_ep, USB_DIR_OUT);
                return;
} else  if (usb_pipeisoc(pipe) && dma) {
         if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
                                offset, length)) {
                if (is_cppi_enabled() || tusb_dma_omap()
                                || is_cppi41_enabled())
                    musb_h_tx_dma_start(hw_ep);
           return;
}


> 
> > So it wouldn't start the DMAs.
> >
> 
>    How musb_host_tx() can be called without musb_start_urb()?
> 
> > In case of PIO, it does load the FIFO and sets the TXPKTREADY.
> >
> 
>    Only is URB_ISO_ASAP is not set.
> 
> WBR, Sergei
> 
> > -Ajay
> 
> WBR, Sergei
> 
> 

RE: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Gupta, Ajay Kumar]

> > > This part is being done at musb_host_rx()
> >
> >    You're doing it in musb_host_tx() actually. Although musb_host_rx()
> > is also broken WRT the isochronous transfers.
> >
> > >  doing next packet programming within same urb and *not* starting next
> > urb. Thus musb_start_urb() doesn't come into this path.
> >
> >    What? Read the code, please -- musb_start_urb() call should always
> > precede musb_host_tx() which handles the DMA interrupt. Unless something
> > clears DMAReqEnab after musb_start_urb() call, setting it only once
> > should work.
> 
> musb_start_urb() call does precede musb_host_tx() but when urb is
> *completed*.

I think you are aware that there are multiple packets within same isochronous urb and musb_start_urb() programs only for first packet.

============================================================
        case USB_ENDPOINT_XFER_ISOC:
                qh->iso_idx = 0;
                qh->frame = 0;
                offset = urb->iso_frame_desc[0].offset;
                len = urb->iso_frame_desc[0].length;
============================================================


-Ajay

> 
> check the 'done' flag and musb_advance_schedule getting called in the
> path.
> 
> if (done) {
>                 /* set status */
>                 urb->status = status;
>                 urb->actual_length = qh->offset;
>                 musb_advance_schedule(musb, urb, hw_ep, USB_DIR_OUT);
>                 return;
> } else  if (usb_pipeisoc(pipe) && dma) {
>          if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
>                                 offset, length)) {
>                 if (is_cppi_enabled() || tusb_dma_omap()
>                                 || is_cppi41_enabled())
>                     musb_h_tx_dma_start(hw_ep);
>            return;
> }
> 
> 
> >
> > > So it wouldn't start the DMAs.
> > >
> >
> >    How musb_host_tx() can be called without musb_start_urb()?
> >
> > > In case of PIO, it does load the FIFO and sets the TXPKTREADY.
> > >
> >
> >    Only is URB_ISO_ASAP is not set.
> >
> > WBR, Sergei
> >
> > > -Ajay
> >
> > WBR, Sergei
> >
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Gupta, Ajay Kumar wrote:

>>>>This part is being done at musb_host_rx()

>>>   You're doing it in musb_host_tx() actually. Although musb_host_rx()
>>>is also broken WRT the isochronous transfers.

>>>> doing next packet programming within same urb and *not* starting next

>>>urb. Thus musb_start_urb() doesn't come into this path.

>>>   What? Read the code, please -- musb_start_urb() call should always
>>>precede musb_host_tx() which handles the DMA interrupt. Unless something
>>>clears DMAReqEnab after musb_start_urb() call, setting it only once
>>>should work.

>>musb_start_urb() call does precede musb_host_tx() but when urb is
>>*completed*.

> I think you are aware that there are multiple packets within same isochronous urb and musb_start_urb() programs only for first packet.
> 
> ============================================================
>         case USB_ENDPOINT_XFER_ISOC:
>                 qh->iso_idx = 0;
>                 qh->frame = 0;
>                 offset = urb->iso_frame_desc[0].offset;
>                 len = urb->iso_frame_desc[0].length;
> ============================================================

    Sure. What I'm still not aware of is where and how the TXCSR DMA bits 
are cleared after the first fragment. Hopefully, testing will reveal this...

WBR, Sergei

Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello, I wrote:

>>>>> This part is being done at musb_host_rx()

>>>>   You're doing it in musb_host_tx() actually. Although musb_host_rx()
>>>> is also broken WRT the isochronous transfers.

>>>>> doing next packet programming within same urb and *not* starting next

>>>> urb. Thus musb_start_urb() doesn't come into this path.

>>>>   What? Read the code, please -- musb_start_urb() call should always
>>>> precede musb_host_tx() which handles the DMA interrupt. Unless 
>>>> something
>>>> clears DMAReqEnab after musb_start_urb() call, setting it only once
>>>> should work.

>>> musb_start_urb() call does precede musb_host_tx() but when urb is
>>> *completed*.

>> I think you are aware that there are multiple packets within same 
>> isochronous urb and musb_start_urb() programs only for first packet.

>> ============================================================
>>         case USB_ENDPOINT_XFER_ISOC:
>>                 qh->iso_idx = 0;
>>                 qh->frame = 0;
>>                 offset = urb->iso_frame_desc[0].offset;
>>                 len = urb->iso_frame_desc[0].length;
>> ============================================================

>    Sure. What I'm still not aware of is where and how the TXCSR DMA bits 
> are cleared after the first fragment. Hopefully, testing will reveal 
> this...

    I really should have stared at the code a bit more myself. Now that I 
have the sad truth has dawned on me... :-/
    My patch "USB: musb: bugfixes for multi-packet TXDMA support" (commit 
c7bbc056a92476b3b3d70a8df7cc746ac5d56de7) inevitably causes the DMA bits to 
be cleared because we're using DMA mode 1 regardless of whether this is 
isochronous transfer or no.  (We could really use mode 0 for isochronous 
transfer and save the hassle when filtering out DMA completion interrupt in 
musb_host_tx().)
    So, I now have to ACK your patch (which could use a better description 
though) and strew my head with ashes. All this also must mean that I have 
managed to break ISO Tx DMA in the internal tree, where I added the 
abovementioned patch after the one that fixed it (the order of the patches 
in the community tree is reverse). I probably did not retest USB audio back 
then...

WBR, Sergei

--
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] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello, I wrote:

>>>>>   You're doing it in musb_host_tx() actually. Although musb_host_rx()
>>>>> is also broken WRT the isochronous transfers.

>>>>>> doing next packet programming within same urb and *not* starting next

>>>>> urb. Thus musb_start_urb() doesn't come into this path.

>>>>>   What? Read the code, please -- musb_start_urb() call should always
>>>>> precede musb_host_tx() which handles the DMA interrupt. Unless 
>>>>> something
>>>>> clears DMAReqEnab after musb_start_urb() call, setting it only once
>>>>> should work.

>>>> musb_start_urb() call does precede musb_host_tx() but when urb is
>>>> *completed*.

>>> I think you are aware that there are multiple packets within same 
>>> isochronous urb and musb_start_urb() programs only for first packet.

>>> ============================================================
>>>         case USB_ENDPOINT_XFER_ISOC:
>>>                 qh->iso_idx = 0;
>>>                 qh->frame = 0;
>>>                 offset = urb->iso_frame_desc[0].offset;
>>>                 len = urb->iso_frame_desc[0].length;
>>> ============================================================

>>    Sure. What I'm still not aware of is where and how the TXCSR DMA 
>> bits are cleared after the first fragment. Hopefully, testing will 
>> reveal this...

>    I really should have stared at the code a bit more myself. Now that I 
> have the sad truth has dawned on me... :-/
>    My patch "USB: musb: bugfixes for multi-packet TXDMA support" (commit 
> c7bbc056a92476b3b3d70a8df7cc746ac5d56de7) inevitably causes the DMA bits 

    You can also refer to this patch'es brokenness in your patch description.
Looks like I now have the complete picture of what happened back then when 
the patches were (re)submitted. The DaVinci case was competely broken when I 
recast the patch "USB: musb: fix isochronous TXDMA (take 2)" to fix the 
failure to transfer the whole ISO URB in DMA mode for the Mentor's own DMA 
case; before that, DaVinci case could (likewise) complete the URB transfer 
in PIO mode after transferring the first fragment via DMA...

>    So, I now have to ACK your patch (which could use a better description though) and strew my head with ashes. All this also must mean that I have managed to break ISO Tx DMA in the internal tree, where I added the abovementioned patch after the one that fixed it (the order of the patches in the community tree is reverse). I probably did not retest USB audio back then...

    No, looks like in the internal tree transfer is completed in PIO mode 
since the internal version of patch lacks that Mentor DMA fix (luckily :-). 
Should still check to make sure...

WBR, Sergei

Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Hello.

Gupta, Ajay Kumar wrote:

>>>>> diff --git a/drivers/usb/musb/musb_host.c
>>>>>           
>> b/drivers/usb/musb/musb_host.c
>>     
>>>>> index cf94511..e3ab40a 100644
>>>>> --- a/drivers/usb/musb/musb_host.c
>>>>> +++ b/drivers/usb/musb/musb_host.c
>>>>> @@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
>>>>>  		return;
>>>>>  	} else	if (usb_pipeisoc(pipe) && dma) {
>>>>>  		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
>>>>> -				offset, length))
>>>>> +				offset, length)) {
>>>>> +			if (is_cppi_enabled() || tusb_dma_omap())
>>>>> +				musb_h_tx_dma_start(hw_ep);
>>>>>  			return;
>>>>>
>>>>>
>>>>>           
>>>>    It will have been already started by this moment -- from
>>>> musb_start_urb() which is enough . Otherwise it wouldn't work, and I've
>>>> made sure it *does* work.
>>>>
>>>>         
>>> This part is being done at musb_host_rx()
>>>       
>>    You're doing it in musb_host_tx() actually. Although musb_host_rx()
>> is also broken WRT the isochronous transfers.
>>
>>     
>>>  doing next packet programming within same urb and *not* starting next
>>>       
>> urb. Thus musb_start_urb() doesn't come into this path.
>>
>>    What? Read the code, please -- musb_start_urb() call should always
>> precede musb_host_tx() which handles the DMA interrupt. Unless something
>> clears DMAReqEnab after musb_start_urb() call, setting it only once
>> should work.
>>     
>
> musb_start_urb() call does precede musb_host_tx() but when urb is *completed*.
>
> check the 'done' flag and musb_advance_schedule getting called in the path.
>
> if (done) {
>                 /* set status */
>                 urb->status = status;
>                 urb->actual_length = qh->offset;
>                 musb_advance_schedule(musb, urb, hw_ep, USB_DIR_OUT);
>                 return;
> } else  if (usb_pipeisoc(pipe) && dma) {
>          if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
>                                 offset, length)) {
>                 if (is_cppi_enabled() || tusb_dma_omap()
>                                 || is_cppi41_enabled())
>                     musb_h_tx_dma_start(hw_ep);
>            return;
> }
>   

   Sigh, that will be musb_start_urb() for the *next* URB after a 
completed one... Someone must have called it for the *current* URB when 
starting an ISO transfer.
This call to musb_tx_dma_program() is only done for the 2nd and 
subsequent data fragments of an URB -- the call for the 1st fragment 
happens elsewhere, from musb_ep_program().
There are basically two Tx URB starting paths within the driver:

1) musb_urb_enqueue() -> musb_schedule() -> musb_start_urb() -> 
musb_h_tx_dma_start();
2) musb_host_tx() -> musb_advance_schedule() -> musb_start_urb() -> 
musb_h_tx_dma_start().

   Transfer of the 1st fragment is started on either of these patch, 
depending on whether there was URBs already queued at the time of 
submitting the new URB; after that DMAReqMode/DMAReqEnab bits should 
remain set after the first musb_h_tx_dma_start() call, so that calling 
musb_tx_dma_program() should be enough for the subsequent fragments. So 
your statement that "Isochronous Tx DMA is getting programmed but never 
getting started for CPPI and TUSB DMAs" is an overstatement in any case 
-- first fragment must be properly started.

WBR, Sergei

Re: [PATCH] musb: fix ISOC Tx programming for CPPI DMAs

[Sergei Shtylyov]

Ajay Kumar Gupta wrote:

> Isochronous Tx DMA is getting programmed but never getting started
> for CPPI and TUSB DMAs and thus Isochronous Tx doesn't work.

> Fixing it by starting DMAs using musb_h_tx_dma_start().

> Signed-off-by: Swaminathan S <swami.iyer@ti.com>
> Signed-off-by: Babu Ravi <ravibabu@ti.com>
> Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index cf94511..e3ab40a 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -1301,8 +1301,11 @@ void musb_host_tx(struct musb *musb, u8 epnum)
>  		return;
>  	} else	if (usb_pipeisoc(pipe) && dma) {
>  		if (musb_tx_dma_program(musb->dma_controller, hw_ep, qh, urb,
> -				offset, length))
> +				offset, length)) {
> +			if (is_cppi_enabled() || tusb_dma_omap())

    The latter check shouldn't really be needed, as in the TUSB DMA mode 
only DMA mode 0 is used, in which case the DMA interrupt filtering code 
above in this function shouldn't clear the TXCSR.DMAReEnab bit.

> +				musb_h_tx_dma_start(hw_ep);
>  			return;
> +		}
>  	} else	if (tx_csr & MUSB_TXCSR_DMAENAB) {
>  		DBG(1, "not complete, but DMA enabled?\n");
>  		return;

WBR, Sergei