Re: [PATCH v4 5/8] dmaengine: dw-edma: Fix programming the source & dest addresses for ep

[Serge Semin <fancer.lancer@gmail.com>]

On Wed, Mar 09, 2022 at 03:12:01PM -0600, Frank Li wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> When eDMA is controlled by the Endpoint (EP), the current logic incorrectly
> programs the source and destination addresses for read and write. Since the
> Root complex and Endpoint uses the opposite channels for read/write, fix the
> issue by finding out the read operation first and program the eDMA accordingly.
> 
> Cc: stable@vger.kernel.org
> Fixes: bd96f1b2f43a ("dmaengine: dw-edma: support local dma device transfer semantics")
> Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> No change between v1 to v4
> 
>  drivers/dma/dw-edma/dw-edma-core.c | 32 +++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 66dc650577919..507f08db1aad3 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -334,6 +334,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
>  	struct dw_edma_chunk *chunk;
>  	struct dw_edma_burst *burst;
>  	struct dw_edma_desc *desc;
> +	bool read = false;
>  	u32 cnt = 0;
>  	int i;
>  
> @@ -424,7 +425,36 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
>  		chunk->ll_region.sz += burst->sz;
>  		desc->alloc_sz += burst->sz;
>  
> -		if (chan->dir == EDMA_DIR_WRITE) {
> +		/****************************************************************
> +		 *

> +		 *        Root Complex                           Endpoint
> +		 * +-----------------------+             +----------------------+
> +		 * |                       |    TX CH    |                      |
> +		 * |                       |             |                      |
> +		 * |      DEV_TO_MEM       <-------------+     MEM_TO_DEV       |
> +		 * |                       |             |                      |
> +		 * |                       |             |                      |
> +		 * |      MEM_TO_DEV       +------------->     DEV_TO_MEM       |
> +		 * |                       |             |                      |
> +		 * |                       |    RX CH    |                      |
> +		 * +-----------------------+             +----------------------+
> +		 *
> +		 * If eDMA is controlled by the Root complex, TX channel
> +		 * (EDMA_DIR_WRITE) is used for memory read (DEV_TO_MEM) and RX
> +		 * channel (EDMA_DIR_READ) is used for memory write (MEM_TO_DEV).
> +		 *
> +		 * If eDMA is controlled by the endpoint, RX channel
> +		 * (EDMA_DIR_READ) is used for memory read (DEV_TO_MEM) and TX
> +		 * channel (EDMA_DIR_WRITE) is used for memory write (MEM_TO_DEV).

Either I have some wrong notion about this issue, or something wrong
with the explanation above and with this fix below.

From my understanding of the possible DW eDMA IP-core setups the
scatch above and the text below it are incorrect. Here is the way the
DW eDMA can be used:
1) Embedded into the DW PCIe Host/EP controller. In this case
CPU/Application Memory is the memory of the CPU attached to the
host/EP controller, while the remote (link partner) memory is the PCIe
bus memory. In this case MEM_TO_DEV operation is supposed to be
performed by the Tx/Write channels, while the DEV_TO_MEM operation -
by the Rx/Read channels.

Note it's applicable for both Host and End-point case, when Linux is
running on the CPU-side of the eDMA controller. So if it's DW PCIe
end-point, then MEM_TO_DEV means copying data from the local CPU
memory into the remote memory. In general the remote memory can be
either some PCIe device on the bus or the Root Complex' CPU memory,
each of which is some remote device anyway from the Local CPU
perspective.

2) Embedded into the PCIe EP. This case is implemented in the
drivers/dma/dw-edma/dw-edma-pcie.c driver. AFAICS from the commits log
and from the driver code, that device is a Synopsys PCIe EndPoint IP
prototype kit. It is a normal PCIe peripheral device with eDMA
embedded, which CPU/Application interface is connected to some
embedded SRAM while remote (link partner) interface is directed
towards the PCIe bus. At the same time the device is setup and handled
by the code running on a CPU connected to the PCIe Host controller.  I
think that in order to preserve the normal DMA operations semantics we
still need to consider the MEM_TO_DEV/DEV_TO_MEM operations from the
host CPU perspective, since that's the side the DMA controller is
supposed to be setup from.  In this MEM_TO_DEV is supposed to be used
to copy data from the host CPU memory into the remote device memory.
It means to allocate Rx/Read channel on the eDMA controller, so one
would be read data from the Local CPU memory and copied it to the PCIe
device SRAM. The logic of the DEV_TO_MEM direction would be just
flipped. The eDMA PCIe device shall use Tx/Write channel to copy data
from it's SRAM into the Host CPU memory.

Please note as I understand the case 2) describes the Synopsys PCIe
EndPoint IP prototype kit, which is based on some FPGA code. It's just
a test setup with no real application, while the case 1) is a real setup
available on our SoC and I guess on yours.

So what I suggest in the framework of this patch is just to implement
the case 1) only. While the case 2) as it's an artificial one can be
manually handled by the DMA client drivers. BTW There aren't ones available
in the kernel anyway. The only exception is an old-time attempt to get
an eDMA IP test-driver mainlined into the kernel:
https://patchwork.kernel.org/project/linux-pci/patch/cc195ac53839b318764c8f6502002cd6d933a923.1547230339.git.gustavo.pimentel@synopsys.com/
But it was long time ago. So it's unlikely to be accepted at all.

What do you think?

-Sergey

> +		 *
> +		 ****************************************************************/
> +

> +		if ((dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ) ||
> +		    (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE))
> +			read = true;

Seeing the driver support only two directions DMA_DEV_TO_MEM/DMA_DEV_TO_MEM
and EDMA_DIR_READ/EDMA_DIR_WRITE, this conditional statement seems
redundant.

> +
> +		/* Program the source and destination addresses for DMA read/write */
> +		if (read) {
>  			burst->sar = src_addr;
>  			if (xfer->type == EDMA_XFER_CYCLIC) {
>  				burst->dar = xfer->xfer.cyclic.paddr;
> -- 
> 2.24.0.rc1
>