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: Wed, 23 Feb 2022 15:35:58 +0200	[thread overview]
Message-ID: <YhY4PqqOgYTLgpKr@smile.fi.intel.com> (raw)
In-Reply-To: <20220222103437.194779-6-miquel.raynal@bootlin.com>

On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Pausing a partial transfer only causes data to be written to memory that
> is a multiple of the memory width setting.
> 
> However, when a DMA client driver finishes DMA early, e.g. due to UART
> char timeout interrupt, all data read from the device must be written to
> memory.
> 
> Therefore, allow the slave to limit the memory width to ensure all data
> read from the device is written to memory when DMA is paused.

(I have only 2.17d and 2.21a datasheets, so below based on the latter)

It seems you are referring to the chapter 7.7 "Disabling a Channel Prior
to Transfer Completion" of the data sheet where it stays that it does not
guarantee to have last burst to be completed in case of
SRC_TR_WIDTH < DST_TR_WIDTH and the CH_SUSP bit is high, when the FIFO_EMPTY
is asserted.

Okay, in iDMA 32-bit we have a specific bit (seems like a fix) that drains
FIFO, but still it doesn't drain the FIFO fully (in case of misalignment)
and the last piece of data (which is less than TR width) is lost when channel
gets disabled.

Now, if we look at the implementation of the serial8250_rx_dma_flush() we
may see that it does
 1. Pause channel without draining FIFO
 2. Moves data to TTY buffer
 3. Terminates channel.

During termination it does pause channel again (with draining enabled),
followed by disabling channel and resuming it again.

According to the 7.7 the resuming channel allows to finish the transfer
normally.

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.

-- 
With Best Regards,
Andy Shevchenko