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

[Zhi Li <lznuaa@gmail.com>]

On Thu, Mar 10, 2022 at 10:32 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> 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.

I think yes. But Remote EP also is a one kind of usage module. Just no one
writes an EP functional driver for it yet.  Even pci-epf-test was just
a test function.
I previously sent vNTB patches to implement a virtual network between
RC and EP,
you can look if you have interest.

>
> 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
> >