[PATCH 1/5] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally

[PATCH 1/5] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally

[Frank Li]

API dw_edma_probe(struct dw_edma_chip *chip) defined at include/linux/dma,
but struct dw_edma_chip have a hidden internal struct dw_edma *dw,
supposed dw should be allocated in API dw_edma_probe().

	@dw: struct dw_edma that is filed by dw_edma_probe()

but dw need allocate and fill chip related information  before call dw_edma_probe()
in current code. See ref
	drivers/dma/dw-edma/dw-edma-pci.c

Move chip related information from dw-edma-core.h to edma.h
allocate memory inside dw_edma_probe()

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/dw-edma/dw-edma-core.c       | 31 ++++++++++++---
 drivers/dma/dw-edma/dw-edma-core.h       | 28 +++-----------
 drivers/dma/dw-edma/dw-edma-v0-core.c    |  2 +-
 drivers/dma/dw-edma/dw-edma-v0-debugfs.c |  2 +-
 include/linux/dma/edma.h                 | 48 +++++++++++++++++++++++-
 5 files changed, 78 insertions(+), 33 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 53289927dd0d6..029085c035067 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -905,19 +905,32 @@ int dw_edma_probe(struct dw_edma_chip *chip)
 	if (!dev)
 		return -EINVAL;
 
-	dw = chip->dw;
-	if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector)
+	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
+	if (!dw)
+		return -ENOMEM;
+
+	chip->dw = dw;
+
+	if (!chip->nr_irqs || !chip->ops)
 		return -EINVAL;
 
 	raw_spin_lock_init(&dw->lock);
 
-	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt,
+	dw->rg_region = &chip->rg_region;
+	dw->ll_region_wr = chip->ll_region_wr;
+	dw->ll_region_rd = chip->ll_region_rd;
+	dw->dt_region_wr = chip->dt_region_wr;
+	dw->dt_region_rd = chip->dt_region_rd;
+
+	dw->mf = chip->mf;
+
+	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt,
 			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE));
-	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH);
+	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt, EDMA_MAX_WR_CH);
 
-	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt,
+	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt,
 			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ));
-	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH);
+	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt, EDMA_MAX_RD_CH);
 
 	if (!dw->wr_ch_cnt && !dw->rd_ch_cnt)
 		return -EINVAL;
@@ -936,6 +949,12 @@ int dw_edma_probe(struct dw_edma_chip *chip)
 	/* Disable eDMA, only to establish the ideal initial conditions */
 	dw_edma_v0_core_off(dw);
 
+	dw->nr_irqs = chip->nr_irqs;
+	dw->ops = chip->ops;
+	dw->irq = devm_kcalloc(dev, dw->nr_irqs, sizeof(*dw->irq), GFP_KERNEL);
+	if (!dw->irq)
+		return -ENOMEM;
+
 	/* Request IRQs */
 	err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc);
 	if (err)
diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
index 60316d408c3e0..8ca195814a878 100644
--- a/drivers/dma/dw-edma/dw-edma-core.h
+++ b/drivers/dma/dw-edma/dw-edma-core.h
@@ -15,20 +15,12 @@
 #include "../virt-dma.h"
 
 #define EDMA_LL_SZ					24
-#define EDMA_MAX_WR_CH					8
-#define EDMA_MAX_RD_CH					8
 
 enum dw_edma_dir {
 	EDMA_DIR_WRITE = 0,
 	EDMA_DIR_READ
 };
 
-enum dw_edma_map_format {
-	EDMA_MF_EDMA_LEGACY = 0x0,
-	EDMA_MF_EDMA_UNROLL = 0x1,
-	EDMA_MF_HDMA_COMPAT = 0x5
-};
-
 enum dw_edma_request {
 	EDMA_REQ_NONE = 0,
 	EDMA_REQ_STOP,
@@ -57,12 +49,6 @@ struct dw_edma_burst {
 	u32				sz;
 };
 
-struct dw_edma_region {
-	phys_addr_t			paddr;
-	void				__iomem *vaddr;
-	size_t				sz;
-};
-
 struct dw_edma_chunk {
 	struct list_head		list;
 	struct dw_edma_chan		*chan;
@@ -109,10 +95,6 @@ struct dw_edma_irq {
 	struct dw_edma			*dw;
 };
 
-struct dw_edma_core_ops {
-	int	(*irq_vector)(struct device *dev, unsigned int nr);
-};
-
 struct dw_edma {
 	char				name[20];
 
@@ -122,11 +104,11 @@ struct dw_edma {
 	struct dma_device		rd_edma;
 	u16				rd_ch_cnt;
 
-	struct dw_edma_region		rg_region;	/* Registers */
-	struct dw_edma_region		ll_region_wr[EDMA_MAX_WR_CH];
-	struct dw_edma_region		ll_region_rd[EDMA_MAX_RD_CH];
-	struct dw_edma_region		dt_region_wr[EDMA_MAX_WR_CH];
-	struct dw_edma_region		dt_region_rd[EDMA_MAX_RD_CH];
+	struct dw_edma_region		*rg_region;	/* Registers */
+	struct dw_edma_region		*ll_region_wr;
+	struct dw_edma_region		*ll_region_rd;
+	struct dw_edma_region		*dt_region_wr;
+	struct dw_edma_region		*dt_region_rd;
 
 	struct dw_edma_irq		*irq;
 	int				nr_irqs;
diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index 329fc2e57b703..884ba55fbd530 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
@@ -25,7 +25,7 @@ enum dw_edma_control {
 
 static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw)
 {
-	return dw->rg_region.vaddr;
+	return dw->rg_region->vaddr;
 }
 
 #define SET_32(dw, name, value)				\
diff --git a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
index 4b3bcffd15ef1..a42047791e727 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
@@ -288,7 +288,7 @@ void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip)
 	if (!dw)
 		return;
 
-	regs = dw->rg_region.vaddr;
+	regs = dw->rg_region->vaddr;
 	if (!regs)
 		return;
 
diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
index cab6e18773dad..fdb19c717aa09 100644
--- a/include/linux/dma/edma.h
+++ b/include/linux/dma/edma.h
@@ -12,19 +12,63 @@
 #include <linux/device.h>
 #include <linux/dmaengine.h>
 
+#define EDMA_MAX_WR_CH                                  8
+#define EDMA_MAX_RD_CH                                  8
+
 struct dw_edma;
 
+struct dw_edma_region {
+	phys_addr_t	paddr;
+	void __iomem	*vaddr;
+	size_t		sz;
+};
+
+struct dw_edma_core_ops {
+	int (*irq_vector)(struct device *dev, unsigned int nr);
+};
+
+enum dw_edma_map_format {
+	EDMA_MF_EDMA_LEGACY = 0x0,
+	EDMA_MF_EDMA_UNROLL = 0x1,
+	EDMA_MF_HDMA_COMPAT = 0x5
+};
+
 /**
  * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
  * @dev:		 struct device of the eDMA controller
  * @id:			 instance ID
- * @irq:		 irq line
+ * @nr_irqs:		 total dma irq number
+ * reg64bit		 if support 64bit write to register
+ * @ops			 DMA channel to IRQ number mapping
+ * @wr_ch_cnt		 DMA write channel number
+ * @rd_ch_cnt		 DMA read channel number
+ * @rg_region		 DMA register region
+ * @ll_region_wr	 DMA descriptor link list memory for write channel
+ * @ll_region_rd	 DMA descriptor link list memory for read channel
+ * @mf			 DMA register map format
  * @dw:			 struct dw_edma that is filed by dw_edma_probe()
  */
 struct dw_edma_chip {
 	struct device		*dev;
 	int			id;
-	int			irq;
+	int			nr_irqs;
+	const struct dw_edma_core_ops   *ops;
+
+	u16			wr_ch_cnt;
+	u16			rd_ch_cnt;
+
+	struct dw_edma_region	rg_region;      /* Registers */
+
+	/* link list address */
+	struct dw_edma_region	ll_region_wr[EDMA_MAX_WR_CH];
+	struct dw_edma_region	ll_region_rd[EDMA_MAX_RD_CH];
+
+	/* data region */
+	struct dw_edma_region	dt_region_wr[EDMA_MAX_WR_CH];
+	struct dw_edma_region	dt_region_rd[EDMA_MAX_RD_CH];
+
+	enum dw_edma_map_format	mf;
+
 	struct dw_edma		*dw;
 };
 
-- 
2.24.0.rc1

[PATCH 2/5] dmaengine: dw-edma-pcie: don't touch internal struct dw_edma

[Frank Li]

dw_edma_probe() defined at include/linux/dma/edma.h

	@dw: struct dw_edma that is filed by dw_edma_probe()

dw_edma_pcie_probe() shouldn't access dw_edma, only filed
struct dw_edma_chip before call dw_edma_probe()

change all dw(struct dw_edma*)  to chip(struct dw_edma_chip*)

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/dw-edma/dw-edma-pcie.c | 83 +++++++++++++-----------------
 1 file changed, 36 insertions(+), 47 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
index 44f6e09bdb531..2262b1c196978 100644
--- a/drivers/dma/dw-edma/dw-edma-pcie.c
+++ b/drivers/dma/dw-edma/dw-edma-pcie.c
@@ -148,7 +148,6 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 	struct dw_edma_pcie_data vsec_data;
 	struct device *dev = &pdev->dev;
 	struct dw_edma_chip *chip;
-	struct dw_edma *dw;
 	int err, nr_irqs;
 	int i, mask;
 
@@ -214,10 +213,6 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 	if (!chip)
 		return -ENOMEM;
 
-	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
-	if (!dw)
-		return -ENOMEM;
-
 	/* IRQs allocation */
 	nr_irqs = pci_alloc_irq_vectors(pdev, 1, vsec_data.irqs,
 					PCI_IRQ_MSI | PCI_IRQ_MSIX);
@@ -228,29 +223,27 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 	}
 
 	/* Data structure initialization */
-	chip->dw = dw;
 	chip->dev = dev;
 	chip->id = pdev->devfn;
-	chip->irq = pdev->irq;
 
-	dw->mf = vsec_data.mf;
-	dw->nr_irqs = nr_irqs;
-	dw->ops = &dw_edma_pcie_core_ops;
-	dw->wr_ch_cnt = vsec_data.wr_ch_cnt;
-	dw->rd_ch_cnt = vsec_data.rd_ch_cnt;
+	chip->mf = vsec_data.mf;
+	chip->nr_irqs = nr_irqs;
+	chip->ops = &dw_edma_pcie_core_ops;
+	chip->wr_ch_cnt = vsec_data.wr_ch_cnt;
+	chip->rd_ch_cnt = vsec_data.rd_ch_cnt;
 
-	dw->rg_region.vaddr = pcim_iomap_table(pdev)[vsec_data.rg.bar];
-	if (!dw->rg_region.vaddr)
+	chip->rg_region.vaddr = pcim_iomap_table(pdev)[vsec_data.rg.bar];
+	if (!chip->rg_region.vaddr)
 		return -ENOMEM;
 
-	dw->rg_region.vaddr += vsec_data.rg.off;
-	dw->rg_region.paddr = pdev->resource[vsec_data.rg.bar].start;
-	dw->rg_region.paddr += vsec_data.rg.off;
-	dw->rg_region.sz = vsec_data.rg.sz;
+	chip->rg_region.vaddr += vsec_data.rg.off;
+	chip->rg_region.paddr = pdev->resource[vsec_data.rg.bar].start;
+	chip->rg_region.paddr += vsec_data.rg.off;
+	chip->rg_region.sz = vsec_data.rg.sz;
 
-	for (i = 0; i < dw->wr_ch_cnt; i++) {
-		struct dw_edma_region *ll_region = &dw->ll_region_wr[i];
-		struct dw_edma_region *dt_region = &dw->dt_region_wr[i];
+	for (i = 0; i < chip->wr_ch_cnt; i++) {
+		struct dw_edma_region *ll_region = &chip->ll_region_wr[i];
+		struct dw_edma_region *dt_region = &chip->dt_region_wr[i];
 		struct dw_edma_block *ll_block = &vsec_data.ll_wr[i];
 		struct dw_edma_block *dt_block = &vsec_data.dt_wr[i];
 
@@ -273,9 +266,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		dt_region->sz = dt_block->sz;
 	}
 
-	for (i = 0; i < dw->rd_ch_cnt; i++) {
-		struct dw_edma_region *ll_region = &dw->ll_region_rd[i];
-		struct dw_edma_region *dt_region = &dw->dt_region_rd[i];
+	for (i = 0; i < chip->rd_ch_cnt; i++) {
+		struct dw_edma_region *ll_region = &chip->ll_region_rd[i];
+		struct dw_edma_region *dt_region = &chip->dt_region_rd[i];
 		struct dw_edma_block *ll_block = &vsec_data.ll_rd[i];
 		struct dw_edma_block *dt_block = &vsec_data.dt_rd[i];
 
@@ -299,45 +292,45 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 	}
 
 	/* Debug info */
-	if (dw->mf == EDMA_MF_EDMA_LEGACY)
-		pci_dbg(pdev, "Version:\teDMA Port Logic (0x%x)\n", dw->mf);
-	else if (dw->mf == EDMA_MF_EDMA_UNROLL)
-		pci_dbg(pdev, "Version:\teDMA Unroll (0x%x)\n", dw->mf);
-	else if (dw->mf == EDMA_MF_HDMA_COMPAT)
-		pci_dbg(pdev, "Version:\tHDMA Compatible (0x%x)\n", dw->mf);
+	if (chip->mf == EDMA_MF_EDMA_LEGACY)
+		pci_dbg(pdev, "Version:\teDMA Port Logic (0x%x)\n", chip->mf);
+	else if (chip->mf == EDMA_MF_EDMA_UNROLL)
+		pci_dbg(pdev, "Version:\teDMA Unroll (0x%x)\n", chip->mf);
+	else if (chip->mf == EDMA_MF_HDMA_COMPAT)
+		pci_dbg(pdev, "Version:\tHDMA Compatible (0x%x)\n", chip->mf);
 	else
-		pci_dbg(pdev, "Version:\tUnknown (0x%x)\n", dw->mf);
+		pci_dbg(pdev, "Version:\tUnknown (0x%x)\n", chip->mf);
 
 	pci_dbg(pdev, "Registers:\tBAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
 		vsec_data.rg.bar, vsec_data.rg.off, vsec_data.rg.sz,
-		dw->rg_region.vaddr, &dw->rg_region.paddr);
+		chip->rg_region.vaddr, &chip->rg_region.paddr);
 
 
-	for (i = 0; i < dw->wr_ch_cnt; i++) {
+	for (i = 0; i < chip->wr_ch_cnt; i++) {
 		pci_dbg(pdev, "L. List:\tWRITE CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
 			i, vsec_data.ll_wr[i].bar,
-			vsec_data.ll_wr[i].off, dw->ll_region_wr[i].sz,
-			dw->ll_region_wr[i].vaddr, &dw->ll_region_wr[i].paddr);
+			vsec_data.ll_wr[i].off, chip->ll_region_wr[i].sz,
+			chip->ll_region_wr[i].vaddr, &chip->ll_region_wr[i].paddr);
 
 		pci_dbg(pdev, "Data:\tWRITE CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
 			i, vsec_data.dt_wr[i].bar,
-			vsec_data.dt_wr[i].off, dw->dt_region_wr[i].sz,
-			dw->dt_region_wr[i].vaddr, &dw->dt_region_wr[i].paddr);
+			vsec_data.dt_wr[i].off, chip->dt_region_wr[i].sz,
+			chip->dt_region_wr[i].vaddr, &chip->dt_region_wr[i].paddr);
 	}
 
-	for (i = 0; i < dw->rd_ch_cnt; i++) {
+	for (i = 0; i < chip->rd_ch_cnt; i++) {
 		pci_dbg(pdev, "L. List:\tREAD CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
 			i, vsec_data.ll_rd[i].bar,
-			vsec_data.ll_rd[i].off, dw->ll_region_rd[i].sz,
-			dw->ll_region_rd[i].vaddr, &dw->ll_region_rd[i].paddr);
+			vsec_data.ll_rd[i].off, chip->ll_region_rd[i].sz,
+			chip->ll_region_rd[i].vaddr, &chip->ll_region_rd[i].paddr);
 
 		pci_dbg(pdev, "Data:\tREAD CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
 			i, vsec_data.dt_rd[i].bar,
-			vsec_data.dt_rd[i].off, dw->dt_region_rd[i].sz,
-			dw->dt_region_rd[i].vaddr, &dw->dt_region_rd[i].paddr);
+			vsec_data.dt_rd[i].off, chip->dt_region_rd[i].sz,
+			chip->dt_region_rd[i].vaddr, &chip->dt_region_rd[i].paddr);
 	}
 
-	pci_dbg(pdev, "Nr. IRQs:\t%u\n", dw->nr_irqs);
+	pci_dbg(pdev, "Nr. IRQs:\t%u\n", chip->nr_irqs);
 
 	/* Validating if PCI interrupts were enabled */
 	if (!pci_dev_msi_enabled(pdev)) {
@@ -345,10 +338,6 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		return -EPERM;
 	}
 
-	dw->irq = devm_kcalloc(dev, nr_irqs, sizeof(*dw->irq), GFP_KERNEL);
-	if (!dw->irq)
-		return -ENOMEM;
-
 	/* Starting eDMA driver */
 	err = dw_edma_probe(chip);
 	if (err) {
-- 
2.24.0.rc1

Re: [PATCH 2/5] dmaengine: dw-edma-pcie: don't touch internal struct dw_edma

[Manivannan Sadhasivam]

On Tue, Mar 01, 2022 at 09:26:43PM -0600, Frank Li wrote:
> dw_edma_probe() defined at include/linux/dma/edma.h
> 
> 	@dw: struct dw_edma that is filed by dw_edma_probe()
> 
> dw_edma_pcie_probe() shouldn't access dw_edma, only filed
> struct dw_edma_chip before call dw_edma_probe()
> 
> change all dw(struct dw_edma*)  to chip(struct dw_edma_chip*)
> 

"struct dw_edma" is an internal structure of the eDMA core. This should not be
used by the eDMA controllers like "dw-edma-pcie" for passing the controller
specific information to the core.

Instead, use the fields local to the "struct dw_edma_chip" for passing the
controller specific info to the core.

Thanks,
Mani

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/dma/dw-edma/dw-edma-pcie.c | 83 +++++++++++++-----------------
>  1 file changed, 36 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index 44f6e09bdb531..2262b1c196978 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> @@ -148,7 +148,6 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	struct dw_edma_pcie_data vsec_data;
>  	struct device *dev = &pdev->dev;
>  	struct dw_edma_chip *chip;
> -	struct dw_edma *dw;
>  	int err, nr_irqs;
>  	int i, mask;
>  
> @@ -214,10 +213,6 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	if (!chip)
>  		return -ENOMEM;
>  
> -	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
> -	if (!dw)
> -		return -ENOMEM;
> -
>  	/* IRQs allocation */
>  	nr_irqs = pci_alloc_irq_vectors(pdev, 1, vsec_data.irqs,
>  					PCI_IRQ_MSI | PCI_IRQ_MSIX);
> @@ -228,29 +223,27 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	}
>  
>  	/* Data structure initialization */
> -	chip->dw = dw;
>  	chip->dev = dev;
>  	chip->id = pdev->devfn;
> -	chip->irq = pdev->irq;
>  
> -	dw->mf = vsec_data.mf;
> -	dw->nr_irqs = nr_irqs;
> -	dw->ops = &dw_edma_pcie_core_ops;
> -	dw->wr_ch_cnt = vsec_data.wr_ch_cnt;
> -	dw->rd_ch_cnt = vsec_data.rd_ch_cnt;
> +	chip->mf = vsec_data.mf;
> +	chip->nr_irqs = nr_irqs;
> +	chip->ops = &dw_edma_pcie_core_ops;
> +	chip->wr_ch_cnt = vsec_data.wr_ch_cnt;
> +	chip->rd_ch_cnt = vsec_data.rd_ch_cnt;
>  
> -	dw->rg_region.vaddr = pcim_iomap_table(pdev)[vsec_data.rg.bar];
> -	if (!dw->rg_region.vaddr)
> +	chip->rg_region.vaddr = pcim_iomap_table(pdev)[vsec_data.rg.bar];
> +	if (!chip->rg_region.vaddr)
>  		return -ENOMEM;
>  
> -	dw->rg_region.vaddr += vsec_data.rg.off;
> -	dw->rg_region.paddr = pdev->resource[vsec_data.rg.bar].start;
> -	dw->rg_region.paddr += vsec_data.rg.off;
> -	dw->rg_region.sz = vsec_data.rg.sz;
> +	chip->rg_region.vaddr += vsec_data.rg.off;
> +	chip->rg_region.paddr = pdev->resource[vsec_data.rg.bar].start;
> +	chip->rg_region.paddr += vsec_data.rg.off;
> +	chip->rg_region.sz = vsec_data.rg.sz;
>  
> -	for (i = 0; i < dw->wr_ch_cnt; i++) {
> -		struct dw_edma_region *ll_region = &dw->ll_region_wr[i];
> -		struct dw_edma_region *dt_region = &dw->dt_region_wr[i];
> +	for (i = 0; i < chip->wr_ch_cnt; i++) {
> +		struct dw_edma_region *ll_region = &chip->ll_region_wr[i];
> +		struct dw_edma_region *dt_region = &chip->dt_region_wr[i];
>  		struct dw_edma_block *ll_block = &vsec_data.ll_wr[i];
>  		struct dw_edma_block *dt_block = &vsec_data.dt_wr[i];
>  
> @@ -273,9 +266,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  		dt_region->sz = dt_block->sz;
>  	}
>  
> -	for (i = 0; i < dw->rd_ch_cnt; i++) {
> -		struct dw_edma_region *ll_region = &dw->ll_region_rd[i];
> -		struct dw_edma_region *dt_region = &dw->dt_region_rd[i];
> +	for (i = 0; i < chip->rd_ch_cnt; i++) {
> +		struct dw_edma_region *ll_region = &chip->ll_region_rd[i];
> +		struct dw_edma_region *dt_region = &chip->dt_region_rd[i];
>  		struct dw_edma_block *ll_block = &vsec_data.ll_rd[i];
>  		struct dw_edma_block *dt_block = &vsec_data.dt_rd[i];
>  
> @@ -299,45 +292,45 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	}
>  
>  	/* Debug info */
> -	if (dw->mf == EDMA_MF_EDMA_LEGACY)
> -		pci_dbg(pdev, "Version:\teDMA Port Logic (0x%x)\n", dw->mf);
> -	else if (dw->mf == EDMA_MF_EDMA_UNROLL)
> -		pci_dbg(pdev, "Version:\teDMA Unroll (0x%x)\n", dw->mf);
> -	else if (dw->mf == EDMA_MF_HDMA_COMPAT)
> -		pci_dbg(pdev, "Version:\tHDMA Compatible (0x%x)\n", dw->mf);
> +	if (chip->mf == EDMA_MF_EDMA_LEGACY)
> +		pci_dbg(pdev, "Version:\teDMA Port Logic (0x%x)\n", chip->mf);
> +	else if (chip->mf == EDMA_MF_EDMA_UNROLL)
> +		pci_dbg(pdev, "Version:\teDMA Unroll (0x%x)\n", chip->mf);
> +	else if (chip->mf == EDMA_MF_HDMA_COMPAT)
> +		pci_dbg(pdev, "Version:\tHDMA Compatible (0x%x)\n", chip->mf);
>  	else
> -		pci_dbg(pdev, "Version:\tUnknown (0x%x)\n", dw->mf);
> +		pci_dbg(pdev, "Version:\tUnknown (0x%x)\n", chip->mf);
>  
>  	pci_dbg(pdev, "Registers:\tBAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
>  		vsec_data.rg.bar, vsec_data.rg.off, vsec_data.rg.sz,
> -		dw->rg_region.vaddr, &dw->rg_region.paddr);
> +		chip->rg_region.vaddr, &chip->rg_region.paddr);
>  
>  
> -	for (i = 0; i < dw->wr_ch_cnt; i++) {
> +	for (i = 0; i < chip->wr_ch_cnt; i++) {
>  		pci_dbg(pdev, "L. List:\tWRITE CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
>  			i, vsec_data.ll_wr[i].bar,
> -			vsec_data.ll_wr[i].off, dw->ll_region_wr[i].sz,
> -			dw->ll_region_wr[i].vaddr, &dw->ll_region_wr[i].paddr);
> +			vsec_data.ll_wr[i].off, chip->ll_region_wr[i].sz,
> +			chip->ll_region_wr[i].vaddr, &chip->ll_region_wr[i].paddr);
>  
>  		pci_dbg(pdev, "Data:\tWRITE CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
>  			i, vsec_data.dt_wr[i].bar,
> -			vsec_data.dt_wr[i].off, dw->dt_region_wr[i].sz,
> -			dw->dt_region_wr[i].vaddr, &dw->dt_region_wr[i].paddr);
> +			vsec_data.dt_wr[i].off, chip->dt_region_wr[i].sz,
> +			chip->dt_region_wr[i].vaddr, &chip->dt_region_wr[i].paddr);
>  	}
>  
> -	for (i = 0; i < dw->rd_ch_cnt; i++) {
> +	for (i = 0; i < chip->rd_ch_cnt; i++) {
>  		pci_dbg(pdev, "L. List:\tREAD CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
>  			i, vsec_data.ll_rd[i].bar,
> -			vsec_data.ll_rd[i].off, dw->ll_region_rd[i].sz,
> -			dw->ll_region_rd[i].vaddr, &dw->ll_region_rd[i].paddr);
> +			vsec_data.ll_rd[i].off, chip->ll_region_rd[i].sz,
> +			chip->ll_region_rd[i].vaddr, &chip->ll_region_rd[i].paddr);
>  
>  		pci_dbg(pdev, "Data:\tREAD CH%.2u, BAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%p, p=%pa)\n",
>  			i, vsec_data.dt_rd[i].bar,
> -			vsec_data.dt_rd[i].off, dw->dt_region_rd[i].sz,
> -			dw->dt_region_rd[i].vaddr, &dw->dt_region_rd[i].paddr);
> +			vsec_data.dt_rd[i].off, chip->dt_region_rd[i].sz,
> +			chip->dt_region_rd[i].vaddr, &chip->dt_region_rd[i].paddr);
>  	}
>  
> -	pci_dbg(pdev, "Nr. IRQs:\t%u\n", dw->nr_irqs);
> +	pci_dbg(pdev, "Nr. IRQs:\t%u\n", chip->nr_irqs);
>  
>  	/* Validating if PCI interrupts were enabled */
>  	if (!pci_dev_msi_enabled(pdev)) {
> @@ -345,10 +338,6 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  		return -EPERM;
>  	}
>  
> -	dw->irq = devm_kcalloc(dev, nr_irqs, sizeof(*dw->irq), GFP_KERNEL);
> -	if (!dw->irq)
> -		return -ENOMEM;
> -
>  	/* Starting eDMA driver */
>  	err = dw_edma_probe(chip);
>  	if (err) {
> -- 
> 2.24.0.rc1
> 

[PATCH 3/5] dmaengine: dw-edma: add flags at struct dw_edma_chip

[Frank Li]

Allow user don't enable remote MSI irq.
PCI ep probe dma locally, don't want to raise irq
to remote PCI host.

Add option allow force 32bit register access even at
64bit system. i.MX8 hardware only allowed 32bit register
access.

Add option allow EP side probe dma. remote side dma is
continue physical memory, local memory is scatter list.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/dma/dw-edma/dw-edma-core.c    |  7 ++++++-
 drivers/dma/dw-edma/dw-edma-v0-core.c | 15 +++++++++++----
 include/linux/dma/edma.h              |  7 +++++++
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 029085c035067..8134909a46fed 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -336,6 +336,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
 	struct dw_edma_desc *desc;
 	u32 cnt = 0;
 	int i;
+	bool b;
 
 	if (!chan->configured)
 		return NULL;
@@ -424,7 +425,11 @@ 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) {
+		b = (chan->dir == EDMA_DIR_WRITE);
+		if (chan->chip->flags & DW_EDMA_CHIP_LOCAL_EP)
+			b = !b;
+
+		if (b) {
 			burst->sar = src_addr;
 			if (xfer->type == EDMA_XFER_CYCLIC) {
 				burst->dar = xfer->xfer.cyclic.paddr;
diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index 884ba55fbd530..322f1db7226c6 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
@@ -301,13 +301,18 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma *dw, enum dw_edma_dir dir)
 static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
 {
 	struct dw_edma_burst *child;
+	struct dw_edma_chan *chan = chunk->chan;
 	struct dw_edma_v0_lli __iomem *lli;
 	struct dw_edma_v0_llp __iomem *llp;
 	u32 control = 0, i = 0;
+	u32 rie = 0;
 	int j;
 
 	lli = chunk->ll_region.vaddr;
 
+	if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI))
+		rie = DW_EDMA_V0_RIE;
+
 	if (chunk->cb)
 		control = DW_EDMA_V0_CB;
 
@@ -315,7 +320,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
 	list_for_each_entry(child, &chunk->burst->list, list) {
 		j--;
 		if (!j)
-			control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE);
+			control |= (DW_EDMA_V0_LIE | rie);
 
 		/* Channel control */
 		SET_LL_32(&lli[i].control, control);
@@ -414,15 +419,17 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 		SET_CH_32(dw, chan->dir, chan->id, ch_control1,
 			  (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
 		/* Linked list */
-		#ifdef CONFIG_64BIT
+
+		if (!(chan->chip->flags & DW_EDMA_CHIP_REG32BIT) &&
+		    IS_ENABLED(CONFIG_64BIT)) {
 			SET_CH_64(dw, chan->dir, chan->id, llp.reg,
 				  chunk->ll_region.paddr);
-		#else /* CONFIG_64BIT */
+		} else {
 			SET_CH_32(dw, chan->dir, chan->id, llp.lsb,
 				  lower_32_bits(chunk->ll_region.paddr));
 			SET_CH_32(dw, chan->dir, chan->id, llp.msb,
 				  upper_32_bits(chunk->ll_region.paddr));
-		#endif /* CONFIG_64BIT */
+		}
 	}
 	/* Doorbell */
 	SET_RW_32(dw, chan->dir, doorbell,
diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
index fdb19c717aa09..a7b9898e8d356 100644
--- a/include/linux/dma/edma.h
+++ b/include/linux/dma/edma.h
@@ -33,6 +33,10 @@ enum dw_edma_map_format {
 	EDMA_MF_HDMA_COMPAT = 0x5
 };
 
+#define DW_EDMA_CHIP_NO_MSI	BIT(0)
+#define DW_EDMA_CHIP_REG32BIT	BIT(1)
+#define DW_EDMA_CHIP_LOCAL_EP	BIT(2)
+
 /**
  * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
  * @dev:		 struct device of the eDMA controller
@@ -40,6 +44,8 @@ enum dw_edma_map_format {
  * @nr_irqs:		 total dma irq number
  * reg64bit		 if support 64bit write to register
  * @ops			 DMA channel to IRQ number mapping
+ * @flags		 - DW_EDMA_CHIP_NO_MSI can't generate remote MSI irq
+ *			 - DW_EDMA_CHIP_REG32BIT only support 32bit register write
  * @wr_ch_cnt		 DMA write channel number
  * @rd_ch_cnt		 DMA read channel number
  * @rg_region		 DMA register region
@@ -53,6 +59,7 @@ struct dw_edma_chip {
 	int			id;
 	int			nr_irqs;
 	const struct dw_edma_core_ops   *ops;
+	u32			flags;
 
 	u16			wr_ch_cnt;
 	u16			rd_ch_cnt;
-- 
2.24.0.rc1

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

[Frank Li]

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(+)

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:
-- 
2.24.0.rc1

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

[Bjorn Helgaas]

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

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

[Zhi Li]

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

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

[Bjorn Helgaas]

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;

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

[Zhi Li]

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;

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

[Bjorn Helgaas]

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

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

[Zhi Li]

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

[PATCH 5/5] PCI: endpoint: functions/pci-epf-test: Support PCI controller DMA

[Frank Li]

Designware provided DMA support in controller. This enabled use
this DMA controller to transfer data.

The whole flow align with standard DMA usage module

1. Using dma_request_channel() and filter function to find correct
RX and TX Channel.
2. dmaengine_slave_config() config remote side physcial address.
3. using dmaengine_prep_slave_single() create transfer descriptor
4. tx_submit();
5. dma_async_issue_pending();

Tested at i.MX8DXL platform.

root@imx8qmmek:~# /usr/bin/pcitest -d -w
WRITE ( 102400 bytes):          OKAY
root@imx8qmmek:~# /usr/bin/pcitest -d -r
READ ( 102400 bytes):           OKAY

WRITE => Size: 102400 bytes DMA: YES  Time: 0.000180145 seconds Rate: 555108 KB/s
READ => Size: 102400 bytes  DMA: YES  Time: 0.000194397 seconds Rate: 514411 KB/s

READ => Size: 102400 bytes  DMA: NO   Time: 0.013532597 seconds Rate: 7389 KB/s
WRITE => Size: 102400 bytes DMA: NO   Time: 0.000857090 seconds Rate: 116673 KB/s

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 106 ++++++++++++++++--
 1 file changed, 96 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 90d84d3bc868f..22ae420c30693 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -52,9 +52,11 @@ struct pci_epf_test {
 	enum pci_barno		test_reg_bar;
 	size_t			msix_table_offset;
 	struct delayed_work	cmd_handler;
-	struct dma_chan		*dma_chan;
+	struct dma_chan		*dma_chan_tx;
+	struct dma_chan		*dma_chan_rx;
 	struct completion	transfer_complete;
 	bool			dma_supported;
+	bool			dma_private;
 	const struct pci_epc_features *epc_features;
 };
 
@@ -105,14 +107,17 @@ static void pci_epf_test_dma_callback(void *param)
  */
 static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 				      dma_addr_t dma_dst, dma_addr_t dma_src,
-				      size_t len)
+				      size_t len, dma_addr_t remote,
+				      enum dma_transfer_direction dir)
 {
 	enum dma_ctrl_flags flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
-	struct dma_chan *chan = epf_test->dma_chan;
+	struct dma_chan *chan = (dir == DMA_DEV_TO_MEM) ? epf_test->dma_chan_tx : epf_test->dma_chan_rx;
 	struct pci_epf *epf = epf_test->epf;
 	struct dma_async_tx_descriptor *tx;
 	struct device *dev = &epf->dev;
 	dma_cookie_t cookie;
+	struct dma_slave_config	sconf;
+	dma_addr_t local = (dir == DMA_MEM_TO_DEV) ? dma_src : dma_dst;
 	int ret;
 
 	if (IS_ERR_OR_NULL(chan)) {
@@ -120,7 +125,20 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 		return -EINVAL;
 	}
 
-	tx = dmaengine_prep_dma_memcpy(chan, dma_dst, dma_src, len, flags);
+	if (epf_test->dma_private) {
+		memset(&sconf, 0, sizeof(sconf));
+		sconf.direction = dir;
+		if (dir == DMA_MEM_TO_DEV)
+			sconf.dst_addr = remote;
+		else
+			sconf.src_addr = remote;
+
+		dmaengine_slave_config(chan, &sconf);
+		tx = dmaengine_prep_slave_single(chan, local, len, dir, flags);
+	} else {
+		tx = dmaengine_prep_dma_memcpy(chan, dma_dst, dma_src, len, flags);
+	}
+
 	if (!tx) {
 		dev_err(dev, "Failed to prepare DMA memcpy\n");
 		return -EIO;
@@ -148,6 +166,23 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 	return 0;
 }
 
+struct epf_dma_filter {
+	struct device *dev;
+	u32 dma_mask;
+};
+
+static bool epf_dma_filter_fn(struct dma_chan *chan, void *node)
+{
+	struct epf_dma_filter *filter = node;
+	struct dma_slave_caps caps;
+
+	memset(&caps, 0, sizeof(caps));
+	dma_get_slave_caps(chan, &caps);
+
+	return chan->device->dev == filter->dev
+		&& (filter->dma_mask & caps.directions);
+}
+
 /**
  * pci_epf_test_init_dma_chan() - Function to initialize EPF test DMA channel
  * @epf_test: the EPF test device that performs data transfer operation
@@ -160,8 +195,42 @@ static int pci_epf_test_init_dma_chan(struct pci_epf_test *epf_test)
 	struct device *dev = &epf->dev;
 	struct dma_chan *dma_chan;
 	dma_cap_mask_t mask;
+	struct epf_dma_filter filter;
 	int ret;
 
+	filter.dev = epf->epc->dev.parent;
+	filter.dma_mask = BIT(DMA_DEV_TO_MEM);
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+	dma_chan = dma_request_channel(mask, epf_dma_filter_fn, &filter);
+	if (IS_ERR(dma_chan)) {
+		dev_info(dev, "Failure get built-in DMA channel, fail back to try allocate general DMA channel\n");
+		goto fail_back_tx;
+	}
+
+	epf_test->dma_chan_rx = dma_chan;
+
+	filter.dma_mask = BIT(DMA_MEM_TO_DEV);
+	dma_chan = dma_request_channel(mask, epf_dma_filter_fn, &filter);
+
+	if (IS_ERR(dma_chan)) {
+		dev_info(dev, "Failure get built-in DMA channel, fail back to try allocate general DMA channel\n");
+		goto fail_back_rx;
+	}
+
+	epf_test->dma_chan_tx = dma_chan;
+	epf_test->dma_private = true;
+
+	init_completion(&epf_test->transfer_complete);
+
+	return 0;
+
+fail_back_rx:
+	dma_release_channel(epf_test->dma_chan_rx);
+	epf_test->dma_chan_tx = NULL;
+
+fail_back_tx:
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_MEMCPY, mask);
 
@@ -174,7 +243,7 @@ static int pci_epf_test_init_dma_chan(struct pci_epf_test *epf_test)
 	}
 	init_completion(&epf_test->transfer_complete);
 
-	epf_test->dma_chan = dma_chan;
+	epf_test->dma_chan_tx = epf_test->dma_chan_rx = dma_chan;
 
 	return 0;
 }
@@ -190,8 +259,17 @@ static void pci_epf_test_clean_dma_chan(struct pci_epf_test *epf_test)
 	if (!epf_test->dma_supported)
 		return;
 
-	dma_release_channel(epf_test->dma_chan);
-	epf_test->dma_chan = NULL;
+	dma_release_channel(epf_test->dma_chan_tx);
+	if (epf_test->dma_chan_tx == epf_test->dma_chan_rx) {
+		epf_test->dma_chan_tx = NULL;
+		epf_test->dma_chan_rx = NULL;
+		return;
+	}
+
+	dma_release_channel(epf_test->dma_chan_rx);
+	epf_test->dma_chan_rx = NULL;
+
+	return;
 }
 
 static void pci_epf_test_print_rate(const char *ops, u64 size,
@@ -280,8 +358,14 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 			goto err_map_addr;
 		}
 
+		if (epf_test->dma_private) {
+			dev_err(dev, "Cannot transfer data using DMA\n");
+			ret = -EINVAL;
+			goto err_map_addr;
+		}
+
 		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
-						 src_phys_addr, reg->size);
+						 src_phys_addr, reg->size, 0, DMA_MEM_TO_MEM);
 		if (ret)
 			dev_err(dev, "Data transfer failed\n");
 	} else {
@@ -363,7 +447,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
 
 		ktime_get_ts64(&start);
 		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
-						 phys_addr, reg->size);
+						 phys_addr, reg->size,
+						 reg->src_addr, DMA_DEV_TO_MEM);
 		if (ret)
 			dev_err(dev, "Data transfer failed\n");
 		ktime_get_ts64(&end);
@@ -453,8 +538,9 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test)
 		}
 
 		ktime_get_ts64(&start);
+
 		ret = pci_epf_test_data_transfer(epf_test, phys_addr,
-						 src_phys_addr, reg->size);
+						 src_phys_addr, reg->size, reg->dst_addr, DMA_MEM_TO_DEV);
 		if (ret)
 			dev_err(dev, "Data transfer failed\n");
 		ktime_get_ts64(&end);
-- 
2.24.0.rc1

Re: [PATCH 1/5] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally

[Manivannan Sadhasivam]

On Tue, Mar 01, 2022 at 09:26:42PM -0600, Frank Li wrote:
> API dw_edma_probe(struct dw_edma_chip *chip) defined at include/linux/dma,
> but struct dw_edma_chip have a hidden internal struct dw_edma *dw,
> supposed dw should be allocated in API dw_edma_probe().
> 
> 	@dw: struct dw_edma that is filed by dw_edma_probe()
> 
> but dw need allocate and fill chip related information  before call dw_edma_probe()
> in current code. See ref
> 	drivers/dma/dw-edma/dw-edma-pci.c
> 
> Move chip related information from dw-edma-core.h to edma.h
> allocate memory inside dw_edma_probe()

Commit message could be reworded a bit:

"struct dw_edma_chip" contains an internal structure "struct dw_edma" that is
used by the eDMA core internally. This structure should not be touched
by the eDMA controller drivers themselves. But currently, the eDMA
controller drivers like "dw-edma-pci" allocates and populates this
internal structure then passes it on to eDMA core. The eDMA core further
populates the structure and uses it. This is wrong!

Hence, move all the "struct dw_edma" specifics from controller drivers
to the eDMA core.

> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/dma/dw-edma/dw-edma-core.c       | 31 ++++++++++++---
>  drivers/dma/dw-edma/dw-edma-core.h       | 28 +++-----------
>  drivers/dma/dw-edma/dw-edma-v0-core.c    |  2 +-
>  drivers/dma/dw-edma/dw-edma-v0-debugfs.c |  2 +-
>  include/linux/dma/edma.h                 | 48 +++++++++++++++++++++++-
>  5 files changed, 78 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 53289927dd0d6..029085c035067 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -905,19 +905,32 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	if (!dev)
>  		return -EINVAL;
>  
> -	dw = chip->dw;
> -	if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector)
> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;
> +
> +	chip->dw = dw;
> +
> +	if (!chip->nr_irqs || !chip->ops)
>  		return -EINVAL;
>  
>  	raw_spin_lock_init(&dw->lock);
>  
> -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt,
> +	dw->rg_region = &chip->rg_region;
> +	dw->ll_region_wr = chip->ll_region_wr;
> +	dw->ll_region_rd = chip->ll_region_rd;
> +	dw->dt_region_wr = chip->dt_region_wr;
> +	dw->dt_region_rd = chip->dt_region_rd;
> +
> +	dw->mf = chip->mf;
> +

The eDMA core should be able to reuse the existing fields from "dw_edma_chip".
I don't think the duplication is needed here.

> +	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt,
>  			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE));

Eventhough this is an old code, I don't get the logic. Can the controller driver
pass channel count that is beyond the range hardware actually supports?

I think we should let the eDMA core to read the registers and populate
these fields instead of relying on the controller drivers.

> -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH);
> +	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt, EDMA_MAX_WR_CH);
>  
> -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt,
> +	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt,
>  			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ));
> -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH);
> +	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt, EDMA_MAX_RD_CH);
>  
>  	if (!dw->wr_ch_cnt && !dw->rd_ch_cnt)
>  		return -EINVAL;
> @@ -936,6 +949,12 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	/* Disable eDMA, only to establish the ideal initial conditions */
>  	dw_edma_v0_core_off(dw);
>  
> +	dw->nr_irqs = chip->nr_irqs;
> +	dw->ops = chip->ops;
> +	dw->irq = devm_kcalloc(dev, dw->nr_irqs, sizeof(*dw->irq), GFP_KERNEL);
> +	if (!dw->irq)
> +		return -ENOMEM;
> +
>  	/* Request IRQs */
>  	err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc);
>  	if (err)
> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index 60316d408c3e0..8ca195814a878 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h
> @@ -15,20 +15,12 @@
>  #include "../virt-dma.h"
>  
>  #define EDMA_LL_SZ					24
> -#define EDMA_MAX_WR_CH					8
> -#define EDMA_MAX_RD_CH					8
>  
>  enum dw_edma_dir {
>  	EDMA_DIR_WRITE = 0,
>  	EDMA_DIR_READ
>  };
>  
> -enum dw_edma_map_format {
> -	EDMA_MF_EDMA_LEGACY = 0x0,
> -	EDMA_MF_EDMA_UNROLL = 0x1,
> -	EDMA_MF_HDMA_COMPAT = 0x5
> -};
> -
>  enum dw_edma_request {
>  	EDMA_REQ_NONE = 0,
>  	EDMA_REQ_STOP,
> @@ -57,12 +49,6 @@ struct dw_edma_burst {
>  	u32				sz;
>  };
>  
> -struct dw_edma_region {
> -	phys_addr_t			paddr;
> -	void				__iomem *vaddr;
> -	size_t				sz;
> -};
> -
>  struct dw_edma_chunk {
>  	struct list_head		list;
>  	struct dw_edma_chan		*chan;
> @@ -109,10 +95,6 @@ struct dw_edma_irq {
>  	struct dw_edma			*dw;
>  };
>  
> -struct dw_edma_core_ops {
> -	int	(*irq_vector)(struct device *dev, unsigned int nr);
> -};
> -
>  struct dw_edma {
>  	char				name[20];
>  
> @@ -122,11 +104,11 @@ struct dw_edma {
>  	struct dma_device		rd_edma;
>  	u16				rd_ch_cnt;
>  
> -	struct dw_edma_region		rg_region;	/* Registers */
> -	struct dw_edma_region		ll_region_wr[EDMA_MAX_WR_CH];
> -	struct dw_edma_region		ll_region_rd[EDMA_MAX_RD_CH];
> -	struct dw_edma_region		dt_region_wr[EDMA_MAX_WR_CH];
> -	struct dw_edma_region		dt_region_rd[EDMA_MAX_RD_CH];
> +	struct dw_edma_region		*rg_region;	/* Registers */
> +	struct dw_edma_region		*ll_region_wr;
> +	struct dw_edma_region		*ll_region_rd;
> +	struct dw_edma_region		*dt_region_wr;
> +	struct dw_edma_region		*dt_region_rd;
>  

As I said above, can we just reuse the existing fields from "dw_edma_chip"?

Thanks,
Mani

Re: [PATCH 1/5] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally

[Manivannan Sadhasivam]

Hi Frank,

+ Dmitry

On Tue, Mar 01, 2022 at 09:26:42PM -0600, Frank Li wrote:
> API dw_edma_probe(struct dw_edma_chip *chip) defined at include/linux/dma,
> but struct dw_edma_chip have a hidden internal struct dw_edma *dw,
> supposed dw should be allocated in API dw_edma_probe().
> 
> 	@dw: struct dw_edma that is filed by dw_edma_probe()
> 
> but dw need allocate and fill chip related information  before call dw_edma_probe()
> in current code. See ref
> 	drivers/dma/dw-edma/dw-edma-pci.c
> 
> Move chip related information from dw-edma-core.h to edma.h
> allocate memory inside dw_edma_probe()
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

I started reviewing your eDMA patches because me and my colleague Dmitry are
also working on the eDMA support for Qualcomm platforms. By looking at the eDMA
driver in mainline, it looks like there is a bit of cleanup required. So for
avoiding the duplication of efforts, let's work together for fixing the eDMA
driver and add support for respective platforms.

Please CC us for all of your future patches as well. We will also share the
patches so that all can be combined into a single series.

Thanks,
Mani

> ---
>  drivers/dma/dw-edma/dw-edma-core.c       | 31 ++++++++++++---
>  drivers/dma/dw-edma/dw-edma-core.h       | 28 +++-----------
>  drivers/dma/dw-edma/dw-edma-v0-core.c    |  2 +-
>  drivers/dma/dw-edma/dw-edma-v0-debugfs.c |  2 +-
>  include/linux/dma/edma.h                 | 48 +++++++++++++++++++++++-
>  5 files changed, 78 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 53289927dd0d6..029085c035067 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -905,19 +905,32 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	if (!dev)
>  		return -EINVAL;
>  
> -	dw = chip->dw;
> -	if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector)
> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;
> +
> +	chip->dw = dw;
> +
> +	if (!chip->nr_irqs || !chip->ops)
>  		return -EINVAL;
>  
>  	raw_spin_lock_init(&dw->lock);
>  
> -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt,
> +	dw->rg_region = &chip->rg_region;
> +	dw->ll_region_wr = chip->ll_region_wr;
> +	dw->ll_region_rd = chip->ll_region_rd;
> +	dw->dt_region_wr = chip->dt_region_wr;
> +	dw->dt_region_rd = chip->dt_region_rd;
> +
> +	dw->mf = chip->mf;
> +
> +	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt,
>  			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE));
> -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH);
> +	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt, EDMA_MAX_WR_CH);
>  
> -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt,
> +	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt,
>  			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ));
> -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH);
> +	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt, EDMA_MAX_RD_CH);
>  
>  	if (!dw->wr_ch_cnt && !dw->rd_ch_cnt)
>  		return -EINVAL;
> @@ -936,6 +949,12 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	/* Disable eDMA, only to establish the ideal initial conditions */
>  	dw_edma_v0_core_off(dw);
>  
> +	dw->nr_irqs = chip->nr_irqs;
> +	dw->ops = chip->ops;
> +	dw->irq = devm_kcalloc(dev, dw->nr_irqs, sizeof(*dw->irq), GFP_KERNEL);
> +	if (!dw->irq)
> +		return -ENOMEM;
> +
>  	/* Request IRQs */
>  	err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc);
>  	if (err)
> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index 60316d408c3e0..8ca195814a878 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h
> @@ -15,20 +15,12 @@
>  #include "../virt-dma.h"
>  
>  #define EDMA_LL_SZ					24
> -#define EDMA_MAX_WR_CH					8
> -#define EDMA_MAX_RD_CH					8
>  
>  enum dw_edma_dir {
>  	EDMA_DIR_WRITE = 0,
>  	EDMA_DIR_READ
>  };
>  
> -enum dw_edma_map_format {
> -	EDMA_MF_EDMA_LEGACY = 0x0,
> -	EDMA_MF_EDMA_UNROLL = 0x1,
> -	EDMA_MF_HDMA_COMPAT = 0x5
> -};
> -
>  enum dw_edma_request {
>  	EDMA_REQ_NONE = 0,
>  	EDMA_REQ_STOP,
> @@ -57,12 +49,6 @@ struct dw_edma_burst {
>  	u32				sz;
>  };
>  
> -struct dw_edma_region {
> -	phys_addr_t			paddr;
> -	void				__iomem *vaddr;
> -	size_t				sz;
> -};
> -
>  struct dw_edma_chunk {
>  	struct list_head		list;
>  	struct dw_edma_chan		*chan;
> @@ -109,10 +95,6 @@ struct dw_edma_irq {
>  	struct dw_edma			*dw;
>  };
>  
> -struct dw_edma_core_ops {
> -	int	(*irq_vector)(struct device *dev, unsigned int nr);
> -};
> -
>  struct dw_edma {
>  	char				name[20];
>  
> @@ -122,11 +104,11 @@ struct dw_edma {
>  	struct dma_device		rd_edma;
>  	u16				rd_ch_cnt;
>  
> -	struct dw_edma_region		rg_region;	/* Registers */
> -	struct dw_edma_region		ll_region_wr[EDMA_MAX_WR_CH];
> -	struct dw_edma_region		ll_region_rd[EDMA_MAX_RD_CH];
> -	struct dw_edma_region		dt_region_wr[EDMA_MAX_WR_CH];
> -	struct dw_edma_region		dt_region_rd[EDMA_MAX_RD_CH];
> +	struct dw_edma_region		*rg_region;	/* Registers */
> +	struct dw_edma_region		*ll_region_wr;
> +	struct dw_edma_region		*ll_region_rd;
> +	struct dw_edma_region		*dt_region_wr;
> +	struct dw_edma_region		*dt_region_rd;
>  
>  	struct dw_edma_irq		*irq;
>  	int				nr_irqs;
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 329fc2e57b703..884ba55fbd530 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -25,7 +25,7 @@ enum dw_edma_control {
>  
>  static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw)
>  {
> -	return dw->rg_region.vaddr;
> +	return dw->rg_region->vaddr;
>  }
>  
>  #define SET_32(dw, name, value)				\
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
> index 4b3bcffd15ef1..a42047791e727 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
> @@ -288,7 +288,7 @@ void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip)
>  	if (!dw)
>  		return;
>  
> -	regs = dw->rg_region.vaddr;
> +	regs = dw->rg_region->vaddr;
>  	if (!regs)
>  		return;
>  
> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> index cab6e18773dad..fdb19c717aa09 100644
> --- a/include/linux/dma/edma.h
> +++ b/include/linux/dma/edma.h
> @@ -12,19 +12,63 @@
>  #include <linux/device.h>
>  #include <linux/dmaengine.h>
>  
> +#define EDMA_MAX_WR_CH                                  8
> +#define EDMA_MAX_RD_CH                                  8
> +
>  struct dw_edma;
>  
> +struct dw_edma_region {
> +	phys_addr_t	paddr;
> +	void __iomem	*vaddr;
> +	size_t		sz;
> +};
> +
> +struct dw_edma_core_ops {
> +	int (*irq_vector)(struct device *dev, unsigned int nr);
> +};
> +
> +enum dw_edma_map_format {
> +	EDMA_MF_EDMA_LEGACY = 0x0,
> +	EDMA_MF_EDMA_UNROLL = 0x1,
> +	EDMA_MF_HDMA_COMPAT = 0x5
> +};
> +
>  /**
>   * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
>   * @dev:		 struct device of the eDMA controller
>   * @id:			 instance ID
> - * @irq:		 irq line
> + * @nr_irqs:		 total dma irq number
> + * reg64bit		 if support 64bit write to register
> + * @ops			 DMA channel to IRQ number mapping
> + * @wr_ch_cnt		 DMA write channel number
> + * @rd_ch_cnt		 DMA read channel number
> + * @rg_region		 DMA register region
> + * @ll_region_wr	 DMA descriptor link list memory for write channel
> + * @ll_region_rd	 DMA descriptor link list memory for read channel
> + * @mf			 DMA register map format
>   * @dw:			 struct dw_edma that is filed by dw_edma_probe()
>   */
>  struct dw_edma_chip {
>  	struct device		*dev;
>  	int			id;
> -	int			irq;
> +	int			nr_irqs;
> +	const struct dw_edma_core_ops   *ops;
> +
> +	u16			wr_ch_cnt;
> +	u16			rd_ch_cnt;
> +
> +	struct dw_edma_region	rg_region;      /* Registers */
> +
> +	/* link list address */
> +	struct dw_edma_region	ll_region_wr[EDMA_MAX_WR_CH];
> +	struct dw_edma_region	ll_region_rd[EDMA_MAX_RD_CH];
> +
> +	/* data region */
> +	struct dw_edma_region	dt_region_wr[EDMA_MAX_WR_CH];
> +	struct dw_edma_region	dt_region_rd[EDMA_MAX_RD_CH];
> +
> +	enum dw_edma_map_format	mf;
> +
>  	struct dw_edma		*dw;
>  };
>  
> -- 
> 2.24.0.rc1
>