[PATCH 0/2] OMAP2+: DMA: fix src/dst position reporting

[PATCH 0/2] OMAP2+: DMA: fix src/dst position reporting

[Peter Ujfalusi]

Hello,

If the user asks for the sDMA current position before the first
data has been transmitted (before the first DMA request has been
generated), the reported position is not valid:
src position: CSAC is uninitialized
dst position: CDAC is 0

The return values in both case considered invalid.
This sitation can be identified by checking if the CDAC register
is 0 (it is initialized to 0 in omap_dam_start call).
In this case return the programmed source/destination address.

The affected omap_get_dma_src_pos/omap_get_dma_dst_pos functions
are used by the audio stack mainly for checking the current position
of the audio stream.

Regards,
Peter
---
Peter Ujfalusi (2):
  OMAP2+: DMA: Workaround for invalid source position
  OMAP2+: DMA: Workaround for invalid destination position

 arch/arm/plat-omap/dma.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

-- 
1.7.7.1

[PATCH 1/2] OMAP2+: DMA: Workaround for invalid source position

[Peter Ujfalusi]

If the DMA source position has been asked before the
first actual data transfer has been done, the CSAC
register does not contain valid information.
We can identify this situation by checking the CDAC
register:
CDAC != 0 indicates that the DMA transfer on the channel has
been started already.
When CDAC == 0 we can not trust the CSAC value since it has
not been updated, and can contain random number.
Return the start address in case the DMA has not jet started.

Note: The CDAC register has been initialized to 0 at dma_start
time.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/plat-omap/dma.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index c22217c..38b0d44 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -1024,12 +1024,23 @@ EXPORT_SYMBOL(omap_set_dma_callback);
  */
 dma_addr_t omap_get_dma_src_pos(int lch)
 {
+	u32 cdac;
 	dma_addr_t offset = 0;
 
 	if (cpu_is_omap15xx())
 		offset = p->dma_read(CPC, lch);
-	else
-		offset = p->dma_read(CSAC, lch);
+	else {
+		/*
+		 * CDAC == 0 indicates that the DMA transfer on the channel has
+		 * not been started (no data has been transferred so far).
+		 * Return the programmed source start address in this case.
+		 */
+		cdac = p->dma_read(CDAC, lch);
+		if (likely(cdac))
+			offset = p->dma_read(CSAC, lch);
+		else
+			offset = p->dma_read(CSSA, lch);
+	}
 
 	if (IS_DMA_ERRATA(DMA_ERRATA_3_3) && offset == 0)
 		offset = p->dma_read(CSAC, lch);
-- 
1.7.7.1

[PATCH 2/2] OMAP2+: DMA: Workaround for invalid destination position

[Peter Ujfalusi]

If the DMA destination position has been asked before the
first actual data transfer has been done, the CDAC
register still contains 0 (it is initialized to 0 at
omsp_dma_start).
If CDAC == 0, return the programmed start address.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/plat-omap/dma.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index 38b0d44..9186491 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -1066,8 +1066,16 @@ dma_addr_t omap_get_dma_dst_pos(int lch)
 
 	if (cpu_is_omap15xx())
 		offset = p->dma_read(CPC, lch);
-	else
+	else {
 		offset = p->dma_read(CDAC, lch);
+		/*
+		 * CDAC == 0 indicates that the DMA transfer on the channel has
+		 * not been started (no data has been transferred so far).
+		 * Return the programmed destination start address in this case.
+		 */
+		if (unlikely(!offset))
+			offset = p->dma_read(CDSA, lch);
+	}
 
 	/*
 	 * omap 3.2/3.3 erratum: sometimes 0 is returned if CSAC/CDAC is
-- 
1.7.7.1

Re: [PATCH 0/2] OMAP2+: DMA: fix src/dst position reporting

[Jarkko Nikula]

On 10/31/2011 04:20 PM, Peter Ujfalusi wrote:
> Hello,
>
> If the user asks for the sDMA current position before the first
> data has been transmitted (before the first DMA request has been
> generated), the reported position is not valid:
> src position: CSAC is uninitialized
> dst position: CDAC is 0
>
> The return values in both case considered invalid.
> This sitation can be identified by checking if the CDAC register
> is 0 (it is initialized to 0 in omap_dam_start call).
> In this case return the programmed source/destination address.
>
> The affected omap_get_dma_src_pos/omap_get_dma_dst_pos functions
> are used by the audio stack mainly for checking the current position
> of the audio stream.
>
> Regards,
> Peter
> ---
> Peter Ujfalusi (2):
>    OMAP2+: DMA: Workaround for invalid source position
>    OMAP2+: DMA: Workaround for invalid destination position
>
>   arch/arm/plat-omap/dma.c |   25 ++++++++++++++++++++++---
>   1 files changed, 22 insertions(+), 3 deletions(-)
>
Both,

Reviewed-by: Jarkko Nikula <jarkko.nikula@bitmer.com>

Re: [PATCH 1/2] OMAP2+: DMA: Workaround for invalid source position

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [111031 06:46]:
> If the DMA source position has been asked before the
> first actual data transfer has been done, the CSAC
> register does not contain valid information.
> We can identify this situation by checking the CDAC
> register:
> CDAC != 0 indicates that the DMA transfer on the channel has
> been started already.
> When CDAC == 0 we can not trust the CSAC value since it has
> not been updated, and can contain random number.
> Return the start address in case the DMA has not jet started.
> 
> Note: The CDAC register has been initialized to 0 at dma_start
> time.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  arch/arm/plat-omap/dma.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> index c22217c..38b0d44 100644
> --- a/arch/arm/plat-omap/dma.c
> +++ b/arch/arm/plat-omap/dma.c
> @@ -1024,12 +1024,23 @@ EXPORT_SYMBOL(omap_set_dma_callback);
>   */
>  dma_addr_t omap_get_dma_src_pos(int lch)
>  {
> +	u32 cdac;
>  	dma_addr_t offset = 0;
>  
>  	if (cpu_is_omap15xx())
>  		offset = p->dma_read(CPC, lch);
> -	else
> -		offset = p->dma_read(CSAC, lch);
> +	else {
> +		/*
> +		 * CDAC == 0 indicates that the DMA transfer on the channel has
> +		 * not been started (no data has been transferred so far).
> +		 * Return the programmed source start address in this case.
> +		 */
> +		cdac = p->dma_read(CDAC, lch);
> +		if (likely(cdac))
> +			offset = p->dma_read(CSAC, lch);
> +		else
> +			offset = p->dma_read(CSSA, lch);
> +	}
>  
>  	if (IS_DMA_ERRATA(DMA_ERRATA_3_3) && offset == 0)
>  		offset = p->dma_read(CSAC, lch);

Should these tests be done only after the errata re-read for both
src and dst patches? Otherwise the errata will not be handled?

Tony

Re: [PATCH 1/2] OMAP2+: DMA: Workaround for invalid source position

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [111103 13:53]:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [111031 06:46]:
> > If the DMA source position has been asked before the
> > first actual data transfer has been done, the CSAC
> > register does not contain valid information.
> > We can identify this situation by checking the CDAC
> > register:
> > CDAC != 0 indicates that the DMA transfer on the channel has
> > been started already.
> > When CDAC == 0 we can not trust the CSAC value since it has
> > not been updated, and can contain random number.
> > Return the start address in case the DMA has not jet started.
> > 
> > Note: The CDAC register has been initialized to 0 at dma_start
> > time.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > ---
> >  arch/arm/plat-omap/dma.c |   15 +++++++++++++--
> >  1 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> > index c22217c..38b0d44 100644
> > --- a/arch/arm/plat-omap/dma.c
> > +++ b/arch/arm/plat-omap/dma.c
> > @@ -1024,12 +1024,23 @@ EXPORT_SYMBOL(omap_set_dma_callback);
> >   */
> >  dma_addr_t omap_get_dma_src_pos(int lch)
> >  {
> > +	u32 cdac;
> >  	dma_addr_t offset = 0;
> >  
> >  	if (cpu_is_omap15xx())
> >  		offset = p->dma_read(CPC, lch);
> > -	else
> > -		offset = p->dma_read(CSAC, lch);
> > +	else {
> > +		/*
> > +		 * CDAC == 0 indicates that the DMA transfer on the channel has
> > +		 * not been started (no data has been transferred so far).
> > +		 * Return the programmed source start address in this case.
> > +		 */
> > +		cdac = p->dma_read(CDAC, lch);
> > +		if (likely(cdac))
> > +			offset = p->dma_read(CSAC, lch);
> > +		else
> > +			offset = p->dma_read(CSSA, lch);
> > +	}
> >  
> >  	if (IS_DMA_ERRATA(DMA_ERRATA_3_3) && offset == 0)
> >  		offset = p->dma_read(CSAC, lch);
> 
> Should these tests be done only after the errata re-read for both
> src and dst patches? Otherwise the errata will not be handled?

Also, please Cc linux-arm-kernel list too so I don't have to repost :)

Tony

Re: Re: [PATCH 1/2] OMAP2+: DMA: Workaround for invalid source position

[Péter Ujfalusi]

On Thursday 03 November 2011 14:27:56 Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [111031 06:46]:
> > If the DMA source position has been asked before the
> > first actual data transfer has been done, the CSAC
> > register does not contain valid information.
> > We can identify this situation by checking the CDAC
> > register:
> > CDAC != 0 indicates that the DMA transfer on the channel has
> > been started already.
> > When CDAC == 0 we can not trust the CSAC value since it has
> > not been updated, and can contain random number.
> > Return the start address in case the DMA has not jet started.
> > 
> > Note: The CDAC register has been initialized to 0 at dma_start
> > time.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > ---
> > 
> >  arch/arm/plat-omap/dma.c |   15 +++++++++++++--
> >  1 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> > index c22217c..38b0d44 100644
> > --- a/arch/arm/plat-omap/dma.c
> > +++ b/arch/arm/plat-omap/dma.c
> > @@ -1024,12 +1024,23 @@ EXPORT_SYMBOL(omap_set_dma_callback);
> > 
> >   */
> >  
> >  dma_addr_t omap_get_dma_src_pos(int lch)
> >  {
> > 
> > +	u32 cdac;
> > 
> >  	dma_addr_t offset = 0;
> >  	
> >  	if (cpu_is_omap15xx())
> >  	
> >  		offset = p->dma_read(CPC, lch);
> > 
> > -	else
> > -		offset = p->dma_read(CSAC, lch);
> > +	else {
> > +		/*
> > +		 * CDAC == 0 indicates that the DMA transfer on the channel has
> > +		 * not been started (no data has been transferred so far).
> > +		 * Return the programmed source start address in this case.
> > +		 */
> > +		cdac = p->dma_read(CDAC, lch);
> > +		if (likely(cdac))
> > +			offset = p->dma_read(CSAC, lch);
> > +		else
> > +			offset = p->dma_read(CSSA, lch);
> > +	}
> > 
> >  	if (IS_DMA_ERRATA(DMA_ERRATA_3_3) && offset == 0)
> >  	
> >  		offset = p->dma_read(CSAC, lch);
> 
> Should these tests be done only after the errata re-read for both
> src and dst patches? Otherwise the errata will not be handled?

Yes that might be a good idea.
I was trying to locate the original errata description for DMA_ERRATA_3_3, but 
it does not exist, or at least I can not find it. This only been mentioned in 
the kernel's comment.
I'm not sure when this DMA_ERRATA_3_3 would have been in force. My guess would 
be that if someone wants to read the src/dst position right after the channel 
is disabled, but what would we expect it to return when the channel has been 
disabled?
I mean what is the reasonable src/dst for a disabled channel? 0 is as good as 
any other number, probably the programmed start of the DMA transfer would be 
my best bet.
I think the errata description for DMA_ERRATA_3_3 is not correct, and it is in 
fact to handle the case I'm also handling: Before the first DMA request the 
CDAC is 0 (since we configured it to be), the CSAC contains _something_ (most 
of the time 0, but can be random number).
In some situation re-reading the src/dst position will give enough time to 
receive the first DMA request, which will update CDAC/CSAC registers.

What do you think?

--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH 1/2] OMAP2+: DMA: Workaround for invalid source position

[Tony Lindgren]

* Péter Ujfalusi <peter.ujfalusi@ti.com> [111104 02:00]:
> On Thursday 03 November 2011 14:27:56 Tony Lindgren wrote:
> > 
> > Should these tests be done only after the errata re-read for both
> > src and dst patches? Otherwise the errata will not be handled?
> 
> Yes that might be a good idea.
> I was trying to locate the original errata description for DMA_ERRATA_3_3, but 
> it does not exist, or at least I can not find it. This only been mentioned in 
> the kernel's comment.

Seems it got added with commit d3c9be2f.

> I'm not sure when this DMA_ERRATA_3_3 would have been in force. My guess would 
> be that if someone wants to read the src/dst position right after the channel 
> is disabled, but what would we expect it to return when the channel has been 
> disabled?

No idea.. Or read it after stopping it?

> I mean what is the reasonable src/dst for a disabled channel? 0 is as good as 
> any other number, probably the programmed start of the DMA transfer would be 
> my best bet.
> I think the errata description for DMA_ERRATA_3_3 is not correct, and it is in 
> fact to handle the case I'm also handling: Before the first DMA request the 
> CDAC is 0 (since we configured it to be), the CSAC contains _something_ (most 
> of the time 0, but can be random number).
> In some situation re-reading the src/dst position will give enough time to 
> receive the first DMA request, which will update CDAC/CSAC registers.

Yeah could be since the handling is just an immediate re-read of the register..
 
> What do you think?

Probably the safest thing to do is move your check after the re-read test
in case there's more to the ERRATA_3_3.

Regards,

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