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@xxxxxxxxxxxxxx> wrote:
> Hi Vinod, Morimoto-san, Yokoyama-san,
>
> On Fri, Oct 20, 2017 at 8:15 AM, Vinod Koul <vinod.koul@xxxxxxxxx> wrote:
>> On Thu, Oct 19, 2017 at 01:15:13AM +0000, Kuninori Morimoto wrote:
>>> From: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxx

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