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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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