Re: [PATCH 4/5] PCI: imx6: add PCIe embedded DMA support

[Zhi Li <lznuaa@gmail.com>]

On Wed, Mar 2, 2022 at 3:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Mar 02, 2022 at 02:49:45PM -0600, Zhi Li wrote:
> > On Wed, Mar 2, 2022 at 2:15 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > In subject:
> > >
> > >   PCI: imx6: Add embedded DMA support
> > >
> > > to match existing style.  "PCIe" seems superfluous here since we
> > > already mentioned it earlier in the subject.
> >
> > Sorry, it is PCI when git log to check old history.
>
> I don't understand.  But maybe this would be better?
>
>   PCI: imx6: Add embedded DMA controller support
>
> > > On Tue, Mar 01, 2022 at 09:26:45PM -0600, Frank Li wrote:
> > > > ...
>
> > > > The DMA can transfer data to any remote address location
> > > > regardless PCI address space size.
> > >
> > > What is this sentence telling us?  Is it merely that the DMA "inbound
> > > address space" may be larger than the MMIO "outbound address space"?
> > > I think there's no necessary connection between them, and there's no
> > > need to call it out as though it's something special.
> >
> > There are outbound address windows. such as 256M, but RC sides have more
> > than 256M ddr memory, such as 16GB. If CPU or external DMA controller,
> > only can access 256M
> > address space.
> >
> > But if using an embedded DMA controller,  it can access the whole RC's
> > 16G address without
> > changing iAtu mapping.
> >
> > I want to say why I need enable embedded DMA for EP.
>
> OK, so if IIUC, the DMA controller is embedded in the imx6 host bridge
> (of course; that's obvious from what you're doing here).  And unlike
> DMA from devices *below* the host bridge, DMAs from the embedded
> controller don't go through the iATU, so they are not subject to any
> of the iATU limitations.  Right?

Yes!

>
> > > > +static int imx_add_pcie_dma(struct imx6_pcie *imx6_pcie,
> > > > +                         struct platform_device *pdev,
> > > > +                         struct resource *dbi_base)
> > >
> > > IIUC this is already in pci->dbi_base, so why not use that instead of
> > > passing it in?  Passing both a struct and the contents of a member of
> > > the struct is an opportunity for a mistake.
> >
> > pci->dbi_base just provides a virtual address.
> > I can change dbi_base as dbi_res.
>
> Ah, I missed that you use the CPU physical address from the struct
> resource.
>
> Strictly speaking, what you need is not the CPU physical address, but
> the DMA address that appears on the PCI bus.  In your case, these
> likely have identical values, but the logical PCI architecture, which
> allows things like IOMMUs, does not guarantee this.

I think dw_edma driver may not use this physical address.
But dw_edma_probe() requested fill in this data.

>
> > > > +{
> > > > +     unsigned int pcie_dma_offset;
> > > > +     struct dw_pcie *pci = imx6_pcie->pci;
> > > > +     struct device *dev = pci->dev;
> > > > +     struct dw_edma_chip *dma = &imx6_pcie->dma_chip;
> > > > +     int i = 0;
> > > > +     u64 pbase;
> > > > +     void *vbase;
> > > > +     int sz = PAGE_SIZE;
> > > > +
> > > > +     pcie_dma_offset = 0x970;
> > > > +
> > > > +     pbase = dbi_base->start + pcie_dma_offset;
> > > > +     vbase = pci->dbi_base + pcie_dma_offset;