[PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

SYS/RT/Audio DMAC have both TCR/TCRB.
Its difference is transfer counter value of read (= TCR)
or write (= TCRB). The relationship is like below.

         TCR       TCRB
 [SOURCE] -> [DMAC] -> [DESTINATION]

Thus, for residue calculation, we want to read
TCRB when MEM_TO_MEM/DEV_TO_MEM
TCR  when MEM_TO_DEV
Otherwise, Sound Capture has noise after PluseAudio support
(= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate"))

Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
[Kuninori: add detail info on log, care direction]
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - fixup Subject
 - care direction
 - From Kuninori Morimoto

 drivers/dma/sh/rcar-dmac.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 2b2c7db..f5d7bb7 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1246,6 +1246,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
 	enum dma_status status;
 	unsigned int residue = 0;
 	unsigned int dptr = 0;
+	u32 reg;
 
 	if (!desc)
 		return 0;
@@ -1309,8 +1310,17 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
 		residue += chunk->size;
 	}
 
-	/* Add the residue for the current chunk. */
-	residue += rcar_dmac_chan_read(chan, RCAR_DMATCR) << desc->xfer_shift;
+	/*
+	 * Add the residue for the current chunk
+	 *
+	 *         TCR       TCRB
+	 * [SOURCE] -> [DMAC] -> [DESTINATION]
+	 *
+	 * MEM_TO_xxx : TCR
+	 * xxx_TO_MEM : TCRB
+	 */
+	reg = (desc->direction == DMA_MEM_TO_DEV) ? RCAR_DMATCR : RCAR_DMATCRB;
+	residue += rcar_dmac_chan_read(chan, reg) << desc->xfer_shift;
 
 	return residue;
 }
-- 
1.9.1

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

(By the way the subject line should have mentioned v2)

On Monday, 16 October 2017 10:28:35 EEST Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> SYS/RT/Audio DMAC have both TCR/TCRB.
> Its difference is transfer counter value of read (= TCR)
> or write (= TCRB). The relationship is like below.
> 
>          TCR       TCRB
>  [SOURCE] -> [DMAC] -> [DESTINATION]
> 
> Thus, for residue calculation, we want to read
> TCRB when MEM_TO_MEM/DEV_TO_MEM
> TCR  when MEM_TO_DEV
> Otherwise, Sound Capture has noise after PluseAudio support
> (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate"))

In the MEM_TO_DEV direction, what really matters is how much data has been 
written to memory or to the device. If the DMA is interrupted between read and 
write, the data read but not written doesn't matter. It doesn't end up in the 
destination, so shouldn't be counted. TCRB is thus the register we should use 
in this cases.

In the DEV_TO_MEM direction, the situation is more complex. Both the read and 
write side are important. What matters from a data consumer point of view is 
how much data has been written to memory. On the other hand, if the transfer 
is interrupted between read and write, we'll end up losing data (read from the 
device but never written to memory), which can also be important to report.

In practice, however, I see why DMA could be interrupted between read and 
write in the MEM_TO_DEV case if the device doesn't acknowledge a write for 
whatever reason. Interruptions in the DEV_TO_MEM case would surprise me, as 
the write side shouldn't prevent data from reaching memory (or we'd have much 
worse problems than DMA). Both TCR and TCRB should thus be equivalent in this 
case.

Similarly the MEM_TO_MEM case should be fine with both TCR and TCRB.

There could be problems I'm not aware of, so the explanation above might not 
be correct, but if it is you could use TCRB unconditionally as you did in v1. 
In any case, this patch uses TCR for MEM_TO_DEV, while I think the correct 
register in that case is TCRB.

I believe it would be worth capturing the above explanation in the commit 
message and/or comment.

> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> [Kuninori: add detail info on log, care direction]
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v1 -> v2
> 
>  - fixup Subject
>  - care direction
>  - From Kuninori Morimoto
> 
>  drivers/dma/sh/rcar-dmac.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 2b2c7db..f5d7bb7 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1246,6 +1246,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct
> rcar_dmac_chan *chan, enum dma_status status;
>  	unsigned int residue = 0;
>  	unsigned int dptr = 0;
> +	u32 reg;
> 
>  	if (!desc)
>  		return 0;
> @@ -1309,8 +1310,17 @@ static unsigned int rcar_dmac_chan_get_residue(struct
> rcar_dmac_chan *chan, residue += chunk->size;
>  	}
> 
> -	/* Add the residue for the current chunk. */
> -	residue += rcar_dmac_chan_read(chan, RCAR_DMATCR) << desc->xfer_shift;
> +	/*
> +	 * Add the residue for the current chunk
> +	 *
> +	 *         TCR       TCRB
> +	 * [SOURCE] -> [DMAC] -> [DESTINATION]
> +	 *
> +	 * MEM_TO_xxx : TCR
> +	 * xxx_TO_MEM : TCRB
> +	 */
> +	reg = (desc->direction == DMA_MEM_TO_DEV) ? RCAR_DMATCR : RCAR_DMATCRB;
> +	residue += rcar_dmac_chan_read(chan, reg) << desc->xfer_shift;
> 
>  	return residue;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Kuninori Morimoto]

Hi Laurent

Thank you for your review

> (By the way the subject line should have mentioned v2)

Yeah. I didn't because I exchanged title...

> >          TCR       TCRB
> >  [SOURCE] -> [DMAC] -> [DESTINATION]
(snip)
> In the MEM_TO_DEV direction, what really matters is how much data has been 
> written to memory or to the device. If the DMA is interrupted between read and 
> write, the data read but not written doesn't matter. It doesn't end up in the 
> destination, so shouldn't be counted. TCRB is thus the register we should use 
> in this cases.
> 
> In the DEV_TO_MEM direction, the situation is more complex. Both the read and 
> write side are important. What matters from a data consumer point of view is 
> how much data has been written to memory. On the other hand, if the transfer 
> is interrupted between read and write, we'll end up losing data (read from the 
> device but never written to memory), which can also be important to report.
> 
> In practice, however, I see why DMA could be interrupted between read and 
> write in the MEM_TO_DEV case if the device doesn't acknowledge a write for 
> whatever reason. Interruptions in the DEV_TO_MEM case would surprise me, as 
> the write side shouldn't prevent data from reaching memory (or we'd have much 
> worse problems than DMA). Both TCR and TCRB should thus be equivalent in this 
> case.
> 
> Similarly the MEM_TO_MEM case should be fine with both TCR and TCRB.
> 
> There could be problems I'm not aware of, so the explanation above might not 
> be correct, but if it is you could use TCRB unconditionally as you did in v1. 
> In any case, this patch uses TCR for MEM_TO_DEV, while I think the correct 
> register in that case is TCRB.
> 
> I believe it would be worth capturing the above explanation in the commit 
> message and/or comment.

Thank you for your explanation.
My 1st patch focused to "transfer completed" count (= TCRB) for all case.
In any case, "completed" information should be used.
But in MEM_TO_DEV case, I thought if is OK if data was read from MEM
(= the data will be send to DEV automatically, I didn't care about interruption)
But yes, your opinion is correct I think.

I think MEM_TO_MEM should use TCRB.
I think logic is same as your MEM_TO_DEV explanation ?

Anyway, in all case I can use TCRB in v3 patch,
and it needs abouve explanation. 

Best regards
---
Kuninori Morimoto

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Kuninori Morimoto]

Hi

> Thank you for your explanation.
> My 1st patch focused to "transfer completed" count (= TCRB) for all case.
> In any case, "completed" information should be used.
> But in MEM_TO_DEV case, I thought if is OK if data was read from MEM
> (= the data will be send to DEV automatically, I didn't care about interruption)
> But yes, your opinion is correct I think.
> 
> I think MEM_TO_MEM should use TCRB.
> I think logic is same as your MEM_TO_DEV explanation ?
> 
> Anyway, in all case I can use TCRB in v3 patch,
> and it needs abouve explanation. 

If so, I think v1 is enough... ?
"transfer completed count is important for all case" is no doubt... ?

Best regards
---
Kuninori Morimoto

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Laurent Pinchart]

Hi Morimoto-san,

On Tuesday, 17 October 2017 03:18:49 EEST Kuninori Morimoto wrote:
> Hi
> 
> > Thank you for your explanation.
> > My 1st patch focused to "transfer completed" count (= TCRB) for all case.
> > In any case, "completed" information should be used.
> > But in MEM_TO_DEV case, I thought if is OK if data was read from MEM
> > (= the data will be send to DEV automatically, I didn't care about
> > interruption) But yes, your opinion is correct I think.
> > 
> > I think MEM_TO_MEM should use TCRB.
> > I think logic is same as your MEM_TO_DEV explanation ?

TCRB is better for MEM_TO_MEM too in my opinion. When reporting residue 
information we should indicate how much data has been transferred, and that 
includes both read from source and written to destination.

> > Anyway, in all case I can use TCRB in v3 patch,
> > and it needs abouve explanation.
> 
> If so, I think v1 is enough... ?
> "transfer completed count is important for all case" is no doubt... ?

That's correct, but I don't think the explanation was detailed and clear 
enough. If it was Geert wouldn't have asked for a v2, and you wouldn't have 
agreed to his request :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Kuninori Morimoto]

Hi Vinod, Laurent

> > > Anyway, in all case I can use TCRB in v3 patch,
> > > and it needs abouve explanation.
> > 
> > If so, I think v1 is enough... ?
> > "transfer completed count is important for all case" is no doubt... ?
> 
> That's correct, but I don't think the explanation was detailed and clear 
> enough. If it was Geert wouldn't have asked for a v2, and you wouldn't have 
> agreed to his request :-)

OK. Let's follow Vinod's decision.

Vinod, I'm happy if you are OK on v1.
And I'm happy to create v3 patch which includes detail reason
which is explained by Laurent if you want.

Best regards
---
Kuninori Morimoto

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Laurent Pinchart]

Hi Morimoto-san,

On Wednesday, 18 October 2017 03:01:28 EEST Kuninori Morimoto wrote:
> Hi Vinod, Laurent
> 
> >>> Anyway, in all case I can use TCRB in v3 patch,
> >>> and it needs abouve explanation.
> >> 
> >> If so, I think v1 is enough... ?
> >> "transfer completed count is important for all case" is no doubt... ?
> > 
> > That's correct, but I don't think the explanation was detailed and clear
> > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't
> > have
> > agreed to his request :-)
> 
> OK. Let's follow Vinod's decision.
> 
> Vinod, I'm happy if you are OK on v1.
> And I'm happy to create v3 patch which includes detail reason
> which is explained by Laurent if you want.

I'd be happier with v3 :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Geert Uytterhoeven]

On Wed, Oct 18, 2017 at 3:46 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday, 18 October 2017 03:01:28 EEST Kuninori Morimoto wrote:
>> >>> Anyway, in all case I can use TCRB in v3 patch,
>> >>> and it needs abouve explanation.
>> >> If so, I think v1 is enough... ?
>> >> "transfer completed count is important for all case" is no doubt... ?
>> >
>> > That's correct, but I don't think the explanation was detailed and clear
>> > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't
>> > have
>> > agreed to his request :-)
>>
>> OK. Let's follow Vinod's decision.
>>
>> Vinod, I'm happy if you are OK on v1.
>> And I'm happy to create v3 patch which includes detail reason
>> which is explained by Laurent if you want.
>
> I'd be happier with v3 :-)

+1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Kuninori Morimoto]

Hi Laurent, Geert

> >> > That's correct, but I don't think the explanation was detailed and clear
> >> > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't
> >> > have
> >> > agreed to his request :-)
> >>
> >> OK. Let's follow Vinod's decision.
> >>
> >> Vinod, I'm happy if you are OK on v1.
> >> And I'm happy to create v3 patch which includes detail reason
> >> which is explained by Laurent if you want.
> >
> > I'd be happier with v3 :-)

Oops, I didn't explain this.
This DMAC has buffer. thus it will take a while for TCR and TCRB to
become equal. I will add this to log in v3

Best regards
---
Kuninori Morimoto

Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

[Vinod Koul]

On Wed, Oct 18, 2017 at 12:01:28AM +0000, Kuninori Morimoto wrote:
> 
> Hi Vinod, Laurent
> 
> > > > Anyway, in all case I can use TCRB in v3 patch,
> > > > and it needs abouve explanation.
> > > 
> > > If so, I think v1 is enough... ?
> > > "transfer completed count is important for all case" is no doubt... ?
> > 
> > That's correct, but I don't think the explanation was detailed and clear 
> > enough. If it was Geert wouldn't have asked for a v2, and you wouldn't have 
> > agreed to his request :-)
> 
> OK. Let's follow Vinod's decision.
> 
> Vinod, I'm happy if you are OK on v1.
> And I'm happy to create v3 patch which includes detail reason
> which is explained by Laurent if you want.

Yes that is correct assumption. Only completed data matters :)

-- 
~Vinod