Re: [PATCH v2 5/8] dma: dw: Avoid partial transfers

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Vinod Koul <vkoul@kernel.org>,
	dmaengine@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	Magnus Damm <magnus.damm@gmail.com>,
	Gareth Williams <gareth.williams.jx@renesas.com>,
	Phil Edworthy <phil.edworthy@renesas.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Milan Stevanovic <milan.stevanovic@se.com>,
	Jimmy Lalande <jimmy.lalande@se.com>,
	Pascal Eberhard <pascal.eberhard@se.com>
Subject: Re: [PATCH v2 5/8] dma: dw: Avoid partial transfers
Date: Fri, 25 Feb 2022 22:30:44 +0200	[thread overview]
Message-ID: <Yhk8dAUuQ1OuNkqX@smile.fi.intel.com> (raw)
In-Reply-To: <20220224173009.0d37c12e@xps13>

On Thu, Feb 24, 2022 at 05:30:09PM +0100, Miquel Raynal wrote:
> andriy.shevchenko@linux.intel.com wrote on Wed, 23 Feb 2022 15:35:58
> +0200:
> > On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote:

...

> > It seems the logic in the ->terminate_all() is broken and we actually need
> > to resume channel first (possibly conditionally, if it was suspended), then
> > pause it and disable and resume again.
> > 
> > The problem with ->terminate_all() is that it has no knowledge if it has
> > been called on paused channel (that's why it has to pause channel itself).
> > The pause on termination is required due to some issues in early steppings
> > of iDMA 32-bit hardware implementations.
> > 
> > If my theory is correct, the above change should fix the issues you see.
> 
> I don't have access to these datasheets so I will believe your words
> and try to apply Andy's solution. I ended up with the following fix,
> hopefully I got it right:
> 
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index 48cdefe997f1..59822664d8ec 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -865,6 +865,10 @@ static int dwc_terminate_all(struct dma_chan *chan)
>  
>         clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
>  
> +       /* Ensure the last byte(s) are drained before disabling the channel */
> +       if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags))
> +               dwc_chan_resume(dwc, true);
> +
>         dwc_chan_pause(dwc, true);
>  
>         dwc_chan_disable(dw, dwc);

Yes, this is good enough PoC. Needs to be tested, thanks!

> Phil, I know it's been 3 years since you investigated this issue, but
> do you still have access to the script reproducing the issue? Even
> better, do you still have the hardware to test?

-- 
With Best Regards,
Andy Shevchenko