| Message ID | 20220302032646.3793-4-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally | expand |
[+cc Jingoo for DesignWare generic question] 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. On Tue, Mar 01, 2022 at 09:26:45PM -0600, Frank Li wrote: > Designware PCIe control have embedded DMA controller. > This enable the DMA controller support. Maybe: Add support for the DMA controller in the DesignWare PCIe core. If this DMA controller is in the DesignWare core, is everything in this patch specific to imx6? Or could some of it be shared with other dwc-based drivers? > 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. > Prepare struct dw_edma_chip and call dw_edma_probe "dw_edma_probe()" so it's obvious this is a function. > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 61 +++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index efa8b81711090..a588b848a1650 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -38,6 +38,7 @@ > #include "../../pci.h" > > #include "pcie-designware.h" > +#include "linux/dma/edma.h" > > #define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7c > #define IMX8MQ_PCIE_LINK_CAP_L1EL_64US GENMASK(18, 17) > @@ -164,6 +165,8 @@ struct imx6_pcie { > const struct imx6_pcie_drvdata *drvdata; > struct regulator *epdev_on; > struct phy *phy; > + > + struct dw_edma_chip dma_chip; > }; > > /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ > @@ -2031,6 +2034,61 @@ static const struct dw_pcie_ep_ops pcie_ep_ops = { > .get_features = imx_pcie_ep_get_features, > }; > > +static int imx_dma_irq_vector(struct device *dev, unsigned int nr) Function names should match existing style in this driver, i.e., they should start with "imx6", not "imx". > +{ > + struct platform_device *pdev = to_platform_device(dev); > + > + return platform_get_irq_byname(pdev, "dma"); > +} > + > +static struct dw_edma_core_ops dma_ops = { > + .irq_vector = imx_dma_irq_vector, > +}; > + > +static int imx_add_pcie_dma(struct imx6_pcie *imx6_pcie, > + struct platform_device *pdev, You don't use "pdev" in this function, so no need to pass it in. > + 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. > +{ > + 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; > + > + dma->dev = dev; > + > + dma->rg_region.paddr = pbase; > + dma->rg_region.vaddr = vbase; > + dma->rg_region.sz = 0x424; > + > + dma->wr_ch_cnt = dma->rd_ch_cnt = 1; > + > + dma->ops = &dma_ops; > + dma->nr_irqs = 1; > + > + dma->flags = DW_EDMA_CHIP_NO_MSI | DW_EDMA_CHIP_REG32BIT | DW_EDMA_CHIP_LOCAL_EP; > + > + dma->ll_region_wr[0].sz = sz; > + dma->ll_region_wr[0].vaddr = dmam_alloc_coherent(dev, sz, > + &dma->ll_region_wr[i].paddr, > + GFP_KERNEL); > + > + dma->ll_region_rd[0].sz = sz; > + dma->ll_region_rd[0].vaddr = dmam_alloc_coherent(dev, sz, > + &dma->ll_region_rd[i].paddr, > + GFP_KERNEL); > + > + return dw_edma_probe(dma); > +} > + > static int imx_add_pcie_ep(struct imx6_pcie *imx6_pcie, > struct platform_device *pdev) > { > @@ -2694,6 +2752,9 @@ static int imx6_pcie_probe(struct platform_device *pdev) > goto err_ret; > } > > + if (imx_add_pcie_dma(imx6_pcie, pdev, dbi_base)) > + dev_info(dev, "pci edma probe failure\n"); > + > return 0; > > err_ret: > -- > 2.24.0.rc1 >
On Wed, Mar 2, 2022 at 2:15 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Jingoo for DesignWare generic question] > > 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. > > On Tue, Mar 01, 2022 at 09:26:45PM -0600, Frank Li wrote: > > Designware PCIe control have embedded DMA controller. > > This enable the DMA controller support. > > Maybe: > > Add support for the DMA controller in the DesignWare PCIe core. > > If this DMA controller is in the DesignWare core, is everything in > this patch specific to imx6? Or could some of it be shared with other > dwc-based drivers? The DMA register base address, Irq number. Can't support 64bit register access. > > > 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. > > > Prepare struct dw_edma_chip and call dw_edma_probe > > "dw_edma_probe()" so it's obvious this is a function. > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 61 +++++++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index efa8b81711090..a588b848a1650 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -38,6 +38,7 @@ > > #include "../../pci.h" > > > > #include "pcie-designware.h" > > +#include "linux/dma/edma.h" > > > > #define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7c > > #define IMX8MQ_PCIE_LINK_CAP_L1EL_64US GENMASK(18, 17) > > @@ -164,6 +165,8 @@ struct imx6_pcie { > > const struct imx6_pcie_drvdata *drvdata; > > struct regulator *epdev_on; > > struct phy *phy; > > + > > + struct dw_edma_chip dma_chip; > > }; > > > > /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ > > @@ -2031,6 +2034,61 @@ static const struct dw_pcie_ep_ops pcie_ep_ops = { > > .get_features = imx_pcie_ep_get_features, > > }; > > > > +static int imx_dma_irq_vector(struct device *dev, unsigned int nr) > > Function names should match existing style in this driver, i.e., they > should start with "imx6", not "imx". > > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + > > + return platform_get_irq_byname(pdev, "dma"); > > +} > > + > > +static struct dw_edma_core_ops dma_ops = { > > + .irq_vector = imx_dma_irq_vector, > > +}; > > + > > +static int imx_add_pcie_dma(struct imx6_pcie *imx6_pcie, > > + struct platform_device *pdev, > > You don't use "pdev" in this function, so no need to pass it in. > > > + 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. > > > +{ > > + 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; > > + > > + dma->dev = dev; > > + > > + dma->rg_region.paddr = pbase; > > + dma->rg_region.vaddr = vbase; > > + dma->rg_region.sz = 0x424; > > + > > + dma->wr_ch_cnt = dma->rd_ch_cnt = 1; > > + > > + dma->ops = &dma_ops; > > + dma->nr_irqs = 1; > > + > > + dma->flags = DW_EDMA_CHIP_NO_MSI | DW_EDMA_CHIP_REG32BIT | DW_EDMA_CHIP_LOCAL_EP; > > + > > + dma->ll_region_wr[0].sz = sz; > > + dma->ll_region_wr[0].vaddr = dmam_alloc_coherent(dev, sz, > > + &dma->ll_region_wr[i].paddr, > > + GFP_KERNEL); > > + > > + dma->ll_region_rd[0].sz = sz; > > + dma->ll_region_rd[0].vaddr = dmam_alloc_coherent(dev, sz, > > + &dma->ll_region_rd[i].paddr, > > + GFP_KERNEL); > > + > > + return dw_edma_probe(dma); > > +} > > + > > static int imx_add_pcie_ep(struct imx6_pcie *imx6_pcie, > > struct platform_device *pdev) > > { > > @@ -2694,6 +2752,9 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > goto err_ret; > > } > > > > + if (imx_add_pcie_dma(imx6_pcie, pdev, dbi_base)) > > + dev_info(dev, "pci edma probe failure\n"); > > + > > return 0; > > > > err_ret: > > -- > > 2.24.0.rc1 > >
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? > > > +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. > > > +{ > > > + 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;
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;
On Wed, Mar 02, 2022 at 03:28:48PM -0600, Zhi Li wrote: > 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: > > > > 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! I guess that means the DMA controller is functionally and logically sort of a separate device from the PCI host bridge? Sounds like the DMA controller doesn't receive or generate PCI transactions? Bjorn
On Thu, Mar 3, 2022 at 11:48 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Mar 02, 2022 at 03:28:48PM -0600, Zhi Li wrote: > > 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: > > > > > 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! > > I guess that means the DMA controller is functionally and logically > sort of a separate device from the PCI host bridge? Sounds like the > DMA controller doesn't receive or generate PCI transactions? It is not a separated DMA controller physically. LOCAL BUS -> [ DMA, PCI controller (EP/RC) ]. -> PCI Bus. DMA Read data from local bus, then convert to PCI TLP transaction sent out to PCI bus. It is mainly used for EP mode. It also works for RC mode, but not commonly. > > Bjorn
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index efa8b81711090..a588b848a1650 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -38,6 +38,7 @@ #include "../../pci.h" #include "pcie-designware.h" +#include "linux/dma/edma.h" #define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7c #define IMX8MQ_PCIE_LINK_CAP_L1EL_64US GENMASK(18, 17) @@ -164,6 +165,8 @@ struct imx6_pcie { const struct imx6_pcie_drvdata *drvdata; struct regulator *epdev_on; struct phy *phy; + + struct dw_edma_chip dma_chip; }; /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@ -2031,6 +2034,61 @@ static const struct dw_pcie_ep_ops pcie_ep_ops = { .get_features = imx_pcie_ep_get_features, }; +static int imx_dma_irq_vector(struct device *dev, unsigned int nr) +{ + struct platform_device *pdev = to_platform_device(dev); + + return platform_get_irq_byname(pdev, "dma"); +} + +static struct dw_edma_core_ops dma_ops = { + .irq_vector = imx_dma_irq_vector, +}; + +static int imx_add_pcie_dma(struct imx6_pcie *imx6_pcie, + struct platform_device *pdev, + struct resource *dbi_base) +{ + 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; + + dma->dev = dev; + + dma->rg_region.paddr = pbase; + dma->rg_region.vaddr = vbase; + dma->rg_region.sz = 0x424; + + dma->wr_ch_cnt = dma->rd_ch_cnt = 1; + + dma->ops = &dma_ops; + dma->nr_irqs = 1; + + dma->flags = DW_EDMA_CHIP_NO_MSI | DW_EDMA_CHIP_REG32BIT | DW_EDMA_CHIP_LOCAL_EP; + + dma->ll_region_wr[0].sz = sz; + dma->ll_region_wr[0].vaddr = dmam_alloc_coherent(dev, sz, + &dma->ll_region_wr[i].paddr, + GFP_KERNEL); + + dma->ll_region_rd[0].sz = sz; + dma->ll_region_rd[0].vaddr = dmam_alloc_coherent(dev, sz, + &dma->ll_region_rd[i].paddr, + GFP_KERNEL); + + return dw_edma_probe(dma); +} + static int imx_add_pcie_ep(struct imx6_pcie *imx6_pcie, struct platform_device *pdev) { @@ -2694,6 +2752,9 @@ static int imx6_pcie_probe(struct platform_device *pdev) goto err_ret; } + if (imx_add_pcie_dma(imx6_pcie, pdev, dbi_base)) + dev_info(dev, "pci edma probe failure\n"); + return 0; err_ret:
Designware PCIe control have embedded DMA controller. This enable the DMA controller support. The DMA can transfer data to any remote address location regardless PCI address space size. Prepare struct dw_edma_chip and call dw_edma_probe Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/pci/controller/dwc/pci-imx6.c | 61 +++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)