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

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

[Peter Ujfalusi]

Hello,

Changes since v1:
- Move the check after the ERRATA_3_3 re-read test

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 |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

-- 
1.7.7.2

[PATCH v2 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 |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index c22217c..a9983b6 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -1034,6 +1034,18 @@ dma_addr_t omap_get_dma_src_pos(int lch)
 	if (IS_DMA_ERRATA(DMA_ERRATA_3_3) && offset == 0)
 		offset = p->dma_read(CSAC, lch);
 
+	if (!cpu_is_omap15xx()) {
+		/*
+		 * 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.
+		 */
+		if (likely(p->dma_read(CDAC, lch)))
+			offset = p->dma_read(CSAC, lch);
+		else
+			offset = p->dma_read(CSSA, lch);
+	}
+
 	if (cpu_class_is_omap1())
 		offset |= (p->dma_read(CSSA, lch) & 0xFFFF0000);
 
-- 
1.7.7.2

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

[Jarkko Nikula]

On 11/07/2011 11:33 AM, Peter Ujfalusi wrote:
> 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 |   12 ++++++++++++
>   1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> index c22217c..a9983b6 100644
> --- a/arch/arm/plat-omap/dma.c
> +++ b/arch/arm/plat-omap/dma.c
> @@ -1034,6 +1034,18 @@ dma_addr_t omap_get_dma_src_pos(int lch)
>   	if (IS_DMA_ERRATA(DMA_ERRATA_3_3)&&  offset == 0)
>   		offset = p->dma_read(CSAC, lch);
>
> +	if (!cpu_is_omap15xx()) {
> +		/*
> +		 * 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.
> +		 */
> +		if (likely(p->dma_read(CDAC, lch)))
> +			offset = p->dma_read(CSAC, lch);
> +		else
> +			offset = p->dma_read(CSSA, lch);
> +	}
> +

I think this is enough:

if (unlikely(p->dma_read(CDAC, lch) == 0))
	offset = p->dma_read(CSSA, lch);

I suppose offset is ok for normal case as it is already read (twise) above.

-- 
Jarkko

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

[Jarkko Nikula]

On 11/10/2011 02:46 PM, Jarkko Nikula wrote:
> On 11/07/2011 11:33 AM, Peter Ujfalusi wrote:
>> 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 | 12 ++++++++++++
>> 1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
>> index c22217c..a9983b6 100644
>> --- a/arch/arm/plat-omap/dma.c
>> +++ b/arch/arm/plat-omap/dma.c
>> @@ -1034,6 +1034,18 @@ dma_addr_t omap_get_dma_src_pos(int lch)
>> if (IS_DMA_ERRATA(DMA_ERRATA_3_3)&& offset == 0)
>> offset = p->dma_read(CSAC, lch);
>>
>> + if (!cpu_is_omap15xx()) {
>> + /*
>> + * 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.
>> + */
>> + if (likely(p->dma_read(CDAC, lch)))
>> + offset = p->dma_read(CSAC, lch);
>> + else
>> + offset = p->dma_read(CSSA, lch);
>> + }
>> +
>
> I think this is enough:
>
> if (unlikely(p->dma_read(CDAC, lch) == 0))
> offset = p->dma_read(CSSA, lch);
>
> I suppose offset is ok for normal case as it is already read (twise) above.
>
Or actually my proposal could have a race if CDAC changes between CSAC 
read and CDAC read. In that case it's better to re-read CSAC as your 
patch does after CDAC test and give to both:

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

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

[Péter Ujfalusi]

On Thursday 10 November 2011 15:02:04 Jarkko Nikula wrote:
> On 11/10/2011 02:46 PM, Jarkko Nikula wrote:
> > On 11/07/2011 11:33 AM, Peter Ujfalusi wrote:
> >> 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 | 12 ++++++++++++
> >> 1 files changed, 12 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> >> index c22217c..a9983b6 100644
> >> --- a/arch/arm/plat-omap/dma.c
> >> +++ b/arch/arm/plat-omap/dma.c
> >> @@ -1034,6 +1034,18 @@ dma_addr_t omap_get_dma_src_pos(int lch)
> >> if (IS_DMA_ERRATA(DMA_ERRATA_3_3)&& offset == 0)
> >> offset = p->dma_read(CSAC, lch);
> >> 
> >> + if (!cpu_is_omap15xx()) {
> >> + /*
> >> + * 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.
> >> + */
> >> + if (likely(p->dma_read(CDAC, lch)))
> >> + offset = p->dma_read(CSAC, lch);
> >> + else
> >> + offset = p->dma_read(CSSA, lch);
> >> + }
> >> +
> > 
> > I think this is enough:
> > 
> > if (unlikely(p->dma_read(CDAC, lch) == 0))
> > offset = p->dma_read(CSSA, lch);
> > 
> > I suppose offset is ok for normal case as it is already read (twise)
> > above.
> Or actually my proposal could have a race if CDAC changes between CSAC
> read and CDAC read. In that case it's better to re-read CSAC as your
> patch does after CDAC test and give to both:
> 
> Reviewed-by: Jarkko Nikula <jarkko.nikula@bitmer.com>

Tony, have you taken this patch? I failed to find it in the l-o tree...

--
P?ter

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

[Tony Lindgren]

* P?ter Ujfalusi <peter.ujfalusi@ti.com> [111129 04:26]:
> On Thursday 10 November 2011 15:02:04 Jarkko Nikula wrote:
> > On 11/10/2011 02:46 PM, Jarkko Nikula wrote:
> > > On 11/07/2011 11:33 AM, Peter Ujfalusi wrote:
> > > 
> > > I think this is enough:
> > > 
> > > if (unlikely(p->dma_read(CDAC, lch) == 0))
> > > offset = p->dma_read(CSSA, lch);
> > > 
> > > I suppose offset is ok for normal case as it is already read (twise)
> > > above.
> > Or actually my proposal could have a race if CDAC changes between CSAC
> > read and CDAC read. In that case it's better to re-read CSAC as your
> > patch does after CDAC test and give to both:
> > 
> > Reviewed-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> 
> Tony, have you taken this patch? I failed to find it in the l-o tree...

Sorry for the delay, applying both into fixes-non-critical.

Tony

[PATCH v2 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 a9983b6..002fb4d 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -1074,8 +1074,16 @@ dma_addr_t omap_get_dma_dst_pos(int lch)
 	 * omap 3.2/3.3 erratum: sometimes 0 is returned if CSAC/CDAC is
 	 * read before the DMA controller finished disabling the channel.
 	 */
-	if (!cpu_is_omap15xx() && offset == 0)
+	if (!cpu_is_omap15xx() && offset == 0) {
 		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);
+	}
 
 	if (cpu_class_is_omap1())
 		offset |= (p->dma_read(CDSA, lch) & 0xFFFF0000);
-- 
1.7.7.2