Re: [PATCH v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

Re: [PATCH v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Geert Uytterhoeven]

On Thu, Oct 19, 2017 at 3:15 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> From: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
>
> SYS/RT/Audio DMAC includes independent data buffers for reading
> and writing. Therefore, the read transfer counter and write transfer
> counter have different values.
> TCR indicates read counter, and TCRB indicates write counter.
> The relationship is like below.
>
>         TCR       TCRB
> [SOURCE] -> [DMAC] -> [SINK]
>
> In the MEM_TO_DEV direction, what really matters is how much data has
> been written to the device. If the DMA is interrupted between read and
> write, then, the data 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. It can also be important to report.
>
> In the MEM_TO_MEM direction, what matters is of course how much data
> has been written to memory from data consumer point of view.
> Here, because read and write have independent data buffers, it will
> take a while for TCR and TCRB to become equal. Thus we should check
> TCRB in this case, too.
>
> Thus, all cases we should check TCRB instead of TCR.
>
> Without this patch, Sound Capture has noise after PluseAudio support
> (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate")), because
> the recorder will use wrong residue counter which indicates transferred
> from sound device, but in reality the data was not yet put to memory
> and recorder will record it.
>
> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> [Kuninori: added detail information in log]
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v2 -> v3
>
>  - Code is back to same as v1
>  - log has more detail explanation
>  - From: is back to Yokoyama-san again

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Geert Uytterhoeven]

CC linux-renesas-soc

On Tue, Oct 31, 2017 at 11:45 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Vinod, Morimoto-san, Yokoyama-san,
>
> On Fri, Oct 20, 2017 at 8:15 AM, Vinod Koul <vinod.koul@intel.com> wrote:
>> On Thu, Oct 19, 2017 at 01:15:13AM +0000, Kuninori Morimoto wrote:
>>> From: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
>>>
>>> SYS/RT/Audio DMAC includes independent data buffers for reading
>>> and writing. Therefore, the read transfer counter and write transfer
>>> counter have different values.
>>> TCR indicates read counter, and TCRB indicates write counter.
>>> The relationship is like below.
>>>
>>>         TCR       TCRB
>>> [SOURCE] -> [DMAC] -> [SINK]
>>>
>>> In the MEM_TO_DEV direction, what really matters is how much data has
>>> been written to the device. If the DMA is interrupted between read and
>>> write, then, the data 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. It can also be important to report.
>>>
>>> In the MEM_TO_MEM direction, what matters is of course how much data
>>> has been written to memory from data consumer point of view.
>>> Here, because read and write have independent data buffers, it will
>>> take a while for TCR and TCRB to become equal. Thus we should check
>>> TCRB in this case, too.
>>>
>>> Thus, all cases we should check TCRB instead of TCR.
>>>
>>> Without this patch, Sound Capture has noise after PluseAudio support
>>> (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate")), because
>>> the recorder will use wrong residue counter which indicates transferred
>>> from sound device, but in reality the data was not yet put to memory
>>> and recorder will record it.
>>
>> Applied, thanks.
>
> This is now commit 847449f23dcbff68 ("dmaengine: rcar-dmac: use TCRB
> instead of TCR for residue") in slave-dma/next, and breaks serial console
> input on koelsch (shmobile_defconfig) and salvator-x (renesas_defconfig).
> Reverting that commit fixes the issue for me.
>
> Large serial console input (copy and pasting long lines) works, as that uses
> DMA. Small serial console input (typing) doesn't work.
>
> Apparently for the serial port, TCR contains the value we need (< 0x20),
> while TCRB always contains 0x20.
> Perhaps the code should use the minimum of both registers instead?
>
> For reference, below is the (gmail-whitespace-damaged) patch I used for
> debugging (note that you cannot print from rcar_dmac_chan_get_residue()!):
>
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 50c4950050bea876..c2683294d1c735aa 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1237,6 +1237,10 @@ static int rcar_dmac_chan_terminate_all(struct
> dma_chan *chan)
>         return 0;
>  }
>
> +unsigned int different, same;
> +unsigned int diff_cnt;
> +u32 diff_tcr[100], diff_tcrb[100];
> +
>  static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
>                                                dma_cookie_t cookie)
>  {
> @@ -1310,7 +1314,21 @@ static unsigned int
> rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
>         }
>
>         /* Add the residue for the current chunk. */
> -       residue += rcar_dmac_chan_read(chan, RCAR_DMATCRB) << desc->xfer_shift;
> +{
> +u32 tcr = rcar_dmac_chan_read(chan, RCAR_DMATCR);
> +u32 tcrb = rcar_dmac_chan_read(chan, RCAR_DMATCRB);
> +if (tcr == tcrb) {
> +       same++;
> +} else {
> +       different++;
> +       if (diff_cnt < ARRAY_SIZE(diff_tcr)) {
> +               diff_tcr[diff_cnt] = tcr;
> +               diff_tcrb[diff_cnt] = tcrb;
> +               diff_cnt++;
> +       }
> +}
> +       residue += tcrb << desc->xfer_shift;
> +}
>
>         return residue;
>  }
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index d5714deaaf928b3d..25df0288c85be9e0 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1389,6 +1389,10 @@ static void work_fn_tx(struct work_struct *work)
>         dma_async_issue_pending(chan);
>  }
>
> +extern unsigned int different, same;
> +extern unsigned int diff_cnt;
> +extern u32 diff_tcr[100], diff_tcrb[100];
> +
>  static void rx_timer_fn(unsigned long arg)
>  {
>         struct sci_port *s = (struct sci_port *)arg;
> @@ -1402,6 +1406,13 @@ static void rx_timer_fn(unsigned long arg)
>         u16 scr;
>
>         dev_dbg(port->dev, "DMA Rx timed out\n");
> +if (diff_cnt) {
> +       unsigned int i;
> +
> +       for (i = 0; i <  diff_cnt; i++)
> +               pr_info("tcr[%u] = 0x%x != tcrb[%u] = 0x%x (same %u,
> different %u)\n", i, diff_tcr[i], i, diff_tcrb[i], same, different);
> +       diff_cnt = 0;
> +}
>
>         spin_lock_irqsave(&port->lock, flags);
>

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 v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Vinod Koul]

On Tue, Oct 31, 2017 at 11:46:36AM +0100, Geert Uytterhoeven wrote:
> CC linux-renesas-soc
> 
> On Tue, Oct 31, 2017 at 11:45 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > Hi Vinod, Morimoto-san, Yokoyama-san,
> >
> > On Fri, Oct 20, 2017 at 8:15 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> >> On Thu, Oct 19, 2017 at 01:15:13AM +0000, Kuninori Morimoto wrote:
> >>> From: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> >>>
> >>> SYS/RT/Audio DMAC includes independent data buffers for reading
> >>> and writing. Therefore, the read transfer counter and write transfer
> >>> counter have different values.
> >>> TCR indicates read counter, and TCRB indicates write counter.
> >>> The relationship is like below.
> >>>
> >>>         TCR       TCRB
> >>> [SOURCE] -> [DMAC] -> [SINK]
> >>>
> >>> In the MEM_TO_DEV direction, what really matters is how much data has
> >>> been written to the device. If the DMA is interrupted between read and
> >>> write, then, the data 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. It can also be important to report.
> >>>
> >>> In the MEM_TO_MEM direction, what matters is of course how much data
> >>> has been written to memory from data consumer point of view.
> >>> Here, because read and write have independent data buffers, it will
> >>> take a while for TCR and TCRB to become equal. Thus we should check
> >>> TCRB in this case, too.
> >>>
> >>> Thus, all cases we should check TCRB instead of TCR.
> >>>
> >>> Without this patch, Sound Capture has noise after PluseAudio support
> >>> (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate")), because
> >>> the recorder will use wrong residue counter which indicates transferred
> >>> from sound device, but in reality the data was not yet put to memory
> >>> and recorder will record it.
> >>
> >> Applied, thanks.
> >
> > This is now commit 847449f23dcbff68 ("dmaengine: rcar-dmac: use TCRB
> > instead of TCR for residue") in slave-dma/next, and breaks serial console
> > input on koelsch (shmobile_defconfig) and salvator-x (renesas_defconfig).
> > Reverting that commit fixes the issue for me.

Okay since we are close to merge window I can drop this patch for now,
unless we identify the fix very quickly

-- 
~Vinod

Re: [PATCH v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Kuninori Morimoto]

Hi Geert, Vinod

Geert, thank you for your report,
Vinod, thank you for your quick help.

> > > This is now commit 847449f23dcbff68 ("dmaengine: rcar-dmac: use TCRB
> > > instead of TCR for residue") in slave-dma/next, and breaks serial console
> > > input on koelsch (shmobile_defconfig) and salvator-x (renesas_defconfig).
> > > Reverting that commit fixes the issue for me.

This patch solved my issue (= sound noise), but it is transferring
large size data. From "transferring data size" point of view,
my sound situation is same as your large serial console input situation?

I will ask this to HW guys.
Thanks

Best regards
---
Kuninori Morimoto

Re: [PATCH v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Kuninori Morimoto]

Hi Geert

> Large serial console input (copy and pasting long lines) works, as that uses
> DMA. Small serial console input (typing) doesn't work.
>
> Apparently for the serial port, TCR contains the value we need (< 0x20),
> while TCRB always contains 0x20.
> Perhaps the code should use the minimum of both registers instead?

TCR(= read) happen 1st, and TCRB (= write) happen next.
"TCRB always contains 0x20" means, DMA didn't transfer data for some reason ?

"use minimum" idea can't guarantee data transfering, same as previous topic I guess.

Best regards
---
Kuninori Morimoto

Re: [PATCH v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Geert Uytterhoeven]

Hi Morimoto-san,

On Mon, Nov 6, 2017 at 2:24 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>> Large serial console input (copy and pasting long lines) works, as that uses
>> DMA. Small serial console input (typing) doesn't work.
>>
>> Apparently for the serial port, TCR contains the value we need (< 0x20),
>> while TCRB always contains 0x20.
>> Perhaps the code should use the minimum of both registers instead?
>
> TCR(= read) happen 1st, and TCRB (= write) happen next.
> "TCRB always contains 0x20" means, DMA didn't transfer data for some reason ?

In the case of the serial console, the individual characters were actually
transfered, as the console did manage to read them.

With your patch, the serial driver doesn't know about them (residue == 0),
and the console doesn't see them.

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 v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Vinod Koul]

On Tue, Oct 31, 2017 at 05:11:13PM +0530, Vinod Koul wrote:
> On Tue, Oct 31, 2017 at 11:46:36AM +0100, Geert Uytterhoeven wrote:
> > >
> > > This is now commit 847449f23dcbff68 ("dmaengine: rcar-dmac: use TCRB
> > > instead of TCR for residue") in slave-dma/next, and breaks serial console
> > > input on koelsch (shmobile_defconfig) and salvator-x (renesas_defconfig).
> > > Reverting that commit fixes the issue for me.
> 
> Okay since we are close to merge window I can drop this patch for now,
> unless we identify the fix very quickly

I have not seen a resolution to this, so reverted it.

-- 
~Vinod

Re: [PATCH v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Kuninori Morimoto]

Hi Vinod

> > > > This is now commit 847449f23dcbff68 ("dmaengine: rcar-dmac: use TCRB
> > > > instead of TCR for residue") in slave-dma/next, and breaks serial console
> > > > input on koelsch (shmobile_defconfig) and salvator-x (renesas_defconfig).
> > > > Reverting that commit fixes the issue for me.
> > 
> > Okay since we are close to merge window I can drop this patch for now,
> > unless we identify the fix very quickly
> 
> I have not seen a resolution to this, so reverted it.

We are sorry, but we still don't have solution about this.

Best regards
---
Kuninori Morimoto

Re: [PATCH v3] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

[Vinod Koul]

On Wed, Nov 08, 2017 at 06:33:33AM +0000, Kuninori Morimoto wrote:
> 
> Hi Vinod
> 
> > > > > This is now commit 847449f23dcbff68 ("dmaengine: rcar-dmac: use TCRB
> > > > > instead of TCR for residue") in slave-dma/next, and breaks serial console
> > > > > input on koelsch (shmobile_defconfig) and salvator-x (renesas_defconfig).
> > > > > Reverting that commit fixes the issue for me.
> > > 
> > > Okay since we are close to merge window I can drop this patch for now,
> > > unless we identify the fix very quickly
> > 
> > I have not seen a resolution to this, so reverted it.
> 
> We are sorry, but we still don't have solution about this.

thats okay, reverted seemed best in this situation. Once we have a solution
we can apply and mark to stable.

-- 
~Vinod