Re: [PATCH 05/25] dmaengine: dw-edma: Convert ll/dt phys-address to PCIe bus/DMA address

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

On Thu, Mar 24, 2022 at 07:23:47PM +0000, Frank Li wrote:
> 
> 
> > -----Original Message-----
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Sent: Wednesday, March 23, 2022 8:48 PM
> > To: Gustavo Pimentel <gustavo.pimentel@synopsys.com>; Vinod Koul
> > <vkoul@kernel.org>; Jingoo Han <jingoohan1@gmail.com>; Bjorn Helgaas
> > <bhelgaas@google.com>; Frank Li <frank.li@nxp.com>; Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org>
> > Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>; Serge Semin
> > <fancer.lancer@gmail.com>; Alexey Malahov
> > <Alexey.Malahov@baikalelectronics.ru>; Pavel Parkhomenko
> > <Pavel.Parkhomenko@baikalelectronics.ru>; Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com>; Rob Herring <robh@kernel.org>; Krzysztof
> > Wilczyński <kw@linux.com>; linux-pci@vger.kernel.org;
> > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; Gustavo Pimentel
> > <Gustavo.Pimentel@synopsys.com>
> > Subject: [EXT] [PATCH 05/25] dmaengine: dw-edma: Convert ll/dt phys-address
> > to PCIe bus/DMA address
> > 
> > Caution: EXT Email
> > 
> > In accordance with the dw_edma_region.paddr field semantics it is supposed
> > to be initialized with a memory base address visible by the DW eDMA
> > controller. If the DMA engine is embedded into the DW PCIe Host/EP
> > controller, then the address should belong to the Local CPU/Application
> > memory. If eDMA is remotely accessible across the PCIe bus via the PCIe
> > memory IOs, then the address needs to be a part of the PCIe bus memory
> > space. The later case hasn't been well covered in the corresponding
> > glue-driver. Since in general the PCIe memory space doesn't have to match
> > the CPU memory space and the pci_dev.resource[] arrays contain the
> > resources defined in the CPU memory space, a proper conversion needs to be
> > performed, otherwise either the driver won't properly work or much worse
> > the memory corruption will happen. The conversion can be done by means of
> > the pci_bus_address() method. Let's use it to retrieve the LL, DT and CSRs
> > PCIe memory ranges.
> 

> Actually, This driver is dead driver. And no one use it and don't know
> How to test it. 

My considerations were based on the commit logs, the databooks info and the
code logic.

> 
> I think pci_bus_address can't resolve this problem.
> 
> If remote side using iATU map bar 0 into 0x7000_0000
> 
> I supposed DMA link should be use 0x7000_0000, how host side pci_bus_address
> know such iATU mapping information of EP side?

pci_dev.resource[] entries contain the CPU-physical addresses of the
corresponding BARs. In general the CPU and PCIe bus address spaces may
mismatch. That is for instance the 0xF000_0000 CPU address can get to
be mapped to the PCIe address 0x7000_0000. The later address in its
turn can be used to initialize the eDMA BARx register. Since in case
of the remote eDMA setup the application SAR/DAR address is supposed
to be set with the value within the BARx range, we need to have the
BARx PCIe address, not the CPU one. In a cross-platform way the real
BAR' address range can be retrieved by means of the pci_bus_address()
method. That's what this patch is about.

-Sergey

> 
> Best regards
> Frank Li
> 
> 
> > 
> > Note in addition to that we need to extend the dw_edma_region.paddr field
> > size. The field normally contains a memory range base address to be set in
> > the DW eDMA Linked-List pointer register or as a base address of the
> > Linked-List data buffer. In accordance with [1] the LL range is supposed
> > to be created in the Local CPU/Application memory, but depending on the DW
> > eDMA utilization the memory can be created as a part of the PCIe bus
> > address space (as in the case of the DW PCIe EP prototype kit). Thus in
> > the former case the dw_edma_region.paddr field should have the dma_addr_t
> > type, while in the later one - pci_bus_addr_t. Seeing the corresponding
> > CSRs are always 64-bits wide let's convert the dw_edma_region.paddr field
> > type to be u64 and let the client code logic to make sure it has a valid
> > address visible by the DW eDMA controller. For instance the DW eDMA PCIe
> > glue-driver initializes the field with the addresses from the PCIe bus
> > memory space.
> > 
> > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
> >     v.5.40a, March 2019, p.1103
> > 
> > Fixes: 41aaff2a2ac0 ("dmaengine: Add Synopsys eDMA IP PCIe glue-logic")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/dma/dw-edma/dw-edma-pcie.c | 8 ++++----
> >  include/linux/dma/edma.h           | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-
> > edma-pcie.c
> > index d6b5e2463884..04c95cba1244 100644
> > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > @@ -231,7 +231,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >                         return -ENOMEM;
> > 
> >                 ll_region->vaddr += ll_block->off;
> > -               ll_region->paddr = pdev->resource[ll_block->bar].start;
> > +               ll_region->paddr = pci_bus_address(pdev, ll_block->bar);
> >                 ll_region->paddr += ll_block->off;
> >                 ll_region->sz = ll_block->sz;
> > 
> > @@ -240,7 +240,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >                         return -ENOMEM;
> > 
> >                 dt_region->vaddr += dt_block->off;
> > -               dt_region->paddr = pdev->resource[dt_block->bar].start;
> > +               dt_region->paddr = pci_bus_address(pdev, dt_block->bar);
> >                 dt_region->paddr += dt_block->off;
> >                 dt_region->sz = dt_block->sz;
> >         }
> > @@ -256,7 +256,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >                         return -ENOMEM;
> > 
> >                 ll_region->vaddr += ll_block->off;
> > -               ll_region->paddr = pdev->resource[ll_block->bar].start;
> > +               ll_region->paddr = pci_bus_address(pdev, ll_block->bar);
> >                 ll_region->paddr += ll_block->off;
> >                 ll_region->sz = ll_block->sz;
> > 
> > @@ -265,7 +265,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >                         return -ENOMEM;
> > 
> >                 dt_region->vaddr += dt_block->off;
> > -               dt_region->paddr = pdev->resource[dt_block->bar].start;
> > +               dt_region->paddr = pci_bus_address(pdev, dt_block->bar);
> >                 dt_region->paddr += dt_block->off;
> >                 dt_region->sz = dt_block->sz;
> >         }
> > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> > index 8897f8a79b52..5abac9640a4e 100644
> > --- a/include/linux/dma/edma.h
> > +++ b/include/linux/dma/edma.h
> > @@ -18,7 +18,7 @@
> >  struct dw_edma;
> > 
> >  struct dw_edma_region {
> > -       phys_addr_t     paddr;
> > +       u64             paddr;
> >         void __iomem    *vaddr;
> >         size_t          sz;
> >  };
> > --
> > 2.35.1
>