CPM6 Channel Separation Support

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
 drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
 1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
 #define PCI_DEVICE_ID_XILINX_B054		0xb054
 #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
 #define DW_BLOCK(a, b, c) \
 	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 	u16 vsec;
 	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
 	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
 	if (!vsec)
 		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
 	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
 	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
 	if (map != EDMA_MF_HDMA_NATIVE)
 		return;
 
 	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
 	pci_read_config_dword(pdev, vsec + 0xc, &val);
 	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
 	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
 	pci_read_config_dword(pdev, vsec + 0x14, &val);
 	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 	pdata->rg.off = off;
 
 	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
 	if (!vsec)
 		return;
 
 	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
 			      &val);
 	off = val;
 	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
 			      &val);
 	off <<= 32;
 	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		 * address use the non-LL mode or simple mode supported by
 		 * the HDMA IP.
 		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
 			non_ll = true;
 
 		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		 */
 		if (!non_ll)
 			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
 	}
 
 	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
  drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
  1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
  #define PCI_DEVICE_ID_XILINX_B054		0xb054
  #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Frank

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
  #define DW_BLOCK(a, b, c) \
  	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
  	u16 vsec;
  	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
  	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
  	if (!vsec)
  		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
  	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
  	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
  	if (map != EDMA_MF_HDMA_NATIVE)
  		return;
 
  	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
  	pci_read_config_dword(pdev, vsec + 0xc, &val);
  	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
  	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
  	pci_read_config_dword(pdev, vsec + 0x14, &val);
  	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
  	pdata->rg.off = off;
 
  	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
  	if (!vsec)
  		return;
 
  	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
  			      &val);
  	off = val;
  	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
  			      &val);
  	off <<= 32;
  	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
  		 * address use the non-LL mode or simple mode supported by
  		 * the HDMA IP.
  		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
  			non_ll = true;
 
  		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
  		 */
  		if (!non_ll)
  			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
  	}
 
  	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 47 lines · Frank Li

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
  drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
  1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
  #define PCI_DEVICE_ID_XILINX_B054		0xb054
  #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Quoted 2 lines

Frank

I followed the suggestion you provided at the given link at the end. Talking of Xilinx based devices, the channel separation is fixed for MDB but not for CPM6. So, I have have renamed the specific functionality specific to CPM6 in 2/3 of the same patch series.

This one cleans up the MDB references and makes the functionality generic for all the Xilinx devices.

Suggestion: https://lore.kernel.org/all/amI09ZcfhX4SEPIH@SMW015318/

CPM6 specific declaration:
https://lore.kernel.org/all/[email protected]/

- Devendra

Quoted 96 lines

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
  #define DW_BLOCK(a, b, c) \
  	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
  	u16 vsec;
  	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
  	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
  	if (!vsec)
  		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
  	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
  	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
  	if (map != EDMA_MF_HDMA_NATIVE)
  		return;
 
  	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
  	pci_read_config_dword(pdev, vsec + 0xc, &val);
  	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
  	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
  	pci_read_config_dword(pdev, vsec + 0x14, &val);
  	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
  	pdata->rg.off = off;
 
  	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
  	if (!vsec)
  		return;
 
  	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
  			      &val);
  	off = val;
  	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
  			      &val);
  	off <<= 32;
  	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
  		 * address use the non-LL mode or simple mode supported by
  		 * the HDMA IP.
  		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
  			non_ll = true;
 
  		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
  		 */
  		if (!non_ll)
  			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
  	}
 
  	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 58 lines · Devendra

Quoted 47 lines · Frank Li

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
   drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
   1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
   #define PCI_DEVICE_ID_XILINX_B054		0xb054
   #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Quoted 2 lines

Frank

I followed the suggestion you provided at the given link at the end. Talking of Xilinx based devices, the channel separation is fixed for MDB but not for CPM6. So, I have have renamed the specific functionality specific to CPM6 in 2/3 of the same patch series.

Quoted 4 lines

This one cleans up the MDB references and makes the functionality generic for all the Xilinx devices.

Suggestion: https://lore.kernel.org/all/amI09ZcfhX4SEPIH@SMW015318/

Maybe I have not said cleanly

Actually you only need add one macro to for CPM6, other use MDB macro can work well.

#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP GENMASK(18, 16)

These rename are totally unnecessary. It is quite popular that new chip reuse old chip's register name.

We rename it utils we really can't handle these without rename.

Frank

Quoted 103 lines

CPM6 specific declaration:
https://lore.kernel.org/all/[email protected]/

- Devendra

Quoted 96 lines

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
   #define DW_BLOCK(a, b, c) \
   	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
   	u16 vsec;
   	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
   	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
   	if (!vsec)
   		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
   	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
   	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
   	if (map != EDMA_MF_HDMA_NATIVE)
   		return;
 
   	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
   	pci_read_config_dword(pdev, vsec + 0xc, &val);
   	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
   	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
   	pci_read_config_dword(pdev, vsec + 0x14, &val);
   	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
   	pdata->rg.off = off;
 
   	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
   	if (!vsec)
   		return;
 
   	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
   			      &val);
   	off = val;
   	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
   			      &val);
   	off <<= 32;
   	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
   		 * address use the non-LL mode or simple mode supported by
   		 * the HDMA IP.
   		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
   			non_ll = true;
 
   		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
   		 */
   		if (!non_ll)
   			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
   	}
 
   	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 74 lines · Frank Li

Quoted 58 lines · Devendra

Quoted 47 lines · Frank Li

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
   drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
   1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
   #define PCI_DEVICE_ID_XILINX_B054		0xb054
   #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Quoted 2 lines

Frank

I followed the suggestion you provided at the given link at the end. Talking of Xilinx based devices, the channel separation is fixed for MDB but not for CPM6. So, I have have renamed the specific functionality specific to CPM6 in 2/3 of the same patch series.

Quoted 4 lines

This one cleans up the MDB references and makes the functionality generic for all the Xilinx devices.

Suggestion: https://lore.kernel.org/all/amI09ZcfhX4SEPIH@SMW015318/

Maybe I have not said cleanly

Actually you only need add one macro to for CPM6, other use MDB macro can work well.

#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP GENMASK(18, 16)

These rename are totally unnecessary. It is quite popular that new chip reuse old chip's register name.

Quoted 1 line

We rename it utils we really can't handle these without rename.

You have pointed out the issue in the last line. For the upcoming support we anticipated that it would be messy to use the MDB related stuff for CPM6 and that's why wanted to clean it before introducing functionality for CPM6.

The MDB works, no denying, but mixing the two creates confusion and requires the understanding of MDB first and CPM6. For that reason the clean approach taken is:
- Add the generic functionality supported by MDB and CPM6 under the name Xilinx
- Add the MDB and CPM6 specific functionality with their own associated names, keeping it clean.

- Devendra>

Quoted 105 lines

Frank

Quoted 103 lines

CPM6 specific declaration:
https://lore.kernel.org/all/[email protected]/

- Devendra

Quoted 96 lines

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
   #define DW_BLOCK(a, b, c) \
   	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
   	u16 vsec;
   	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
   	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
   	if (!vsec)
   		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
   	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
   	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
   	if (map != EDMA_MF_HDMA_NATIVE)
   		return;
 
   	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
   	pci_read_config_dword(pdev, vsec + 0xc, &val);
   	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
   	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
   	pci_read_config_dword(pdev, vsec + 0x14, &val);
   	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
   	pdata->rg.off = off;
 
   	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
   	if (!vsec)
   		return;
 
   	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
   			      &val);
   	off = val;
   	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
   			      &val);
   	off <<= 32;
   	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
   		 * address use the non-LL mode or simple mode supported by
   		 * the HDMA IP.
   		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
   			non_ll = true;
 
   		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
   		 */
   		if (!non_ll)
   			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
   	}
 
   	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 78 lines · Devendra

Quoted 75 lines · Frank Li

Quoted 58 lines · Devendra

Quoted 47 lines · Frank Li

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
    drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
    1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
    #define PCI_DEVICE_ID_XILINX_B054		0xb054
    #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Quoted 2 lines

Frank

I followed the suggestion you provided at the given link at the end. Talking of Xilinx based devices, the channel separation is fixed for MDB but not for CPM6. So, I have have renamed the specific functionality specific to CPM6 in 2/3 of the same patch series.

Quoted 4 lines

This one cleans up the MDB references and makes the functionality generic for all the Xilinx devices.

Suggestion: https://lore.kernel.org/all/amI09ZcfhX4SEPIH@SMW015318/

Maybe I have not said cleanly

Actually you only need add one macro to for CPM6, other use MDB macro can work well.

#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP GENMASK(18, 16)

These rename are totally unnecessary. It is quite popular that new chip reuse old chip's register name.

We rename it utils we really can't handle these without rename.

Quoted 4 lines

You have pointed out the issue in the last line. For the upcoming support we anticipated that it would be messy to use the MDB related stuff for CPM6 and that's why wanted to clean it before introducing functionality for CPM6.

Do it only when have to do that. Now only add one macro DW_PCIE_XILINX_CPM6_VSEC_CH_SEP to support CPM6.

The prefix "DW_PCIE_XILINX_MDB" and "DW_PCIE_XILINX_VSEC" have not bring much information,

The most important part following part after it.

Frank

Quoted 116 lines

The MDB works, no denying, but mixing the two creates confusion and requires the understanding of MDB first and CPM6. For that reason the clean approach taken is:
- Add the generic functionality supported by MDB and CPM6 under the name Xilinx
- Add the MDB and CPM6 specific functionality with their own associated names, keeping it clean.

- Devendra>

Quoted 105 lines

Frank

Quoted 103 lines

CPM6 specific declaration:
https://lore.kernel.org/all/[email protected]/

- Devendra

Quoted 96 lines

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
    #define DW_BLOCK(a, b, c) \
    	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
    	u16 vsec;
    	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
    	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
    	if (!vsec)
    		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
    	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
    	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
    	if (map != EDMA_MF_HDMA_NATIVE)
    		return;
 
    	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
    	pci_read_config_dword(pdev, vsec + 0xc, &val);
    	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
    	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
    	pci_read_config_dword(pdev, vsec + 0x14, &val);
    	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
    	pdata->rg.off = off;
 
    	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
    	if (!vsec)
    		return;
 
    	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
    			      &val);
    	off = val;
    	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
    			      &val);
    	off <<= 32;
    	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
    		 * address use the non-LL mode or simple mode supported by
    		 * the HDMA IP.
    		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
    			non_ll = true;
 
    		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
    		 */
    		if (!non_ll)
    			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
    	}
 
    	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 84 lines · Frank Li

Quoted 82 lines · Devendra

Quoted 69 lines · Frank Li

Quoted 58 lines · Devendra

Quoted 47 lines · Frank Li

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
    drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
    1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
    #define PCI_DEVICE_ID_XILINX_B054		0xb054
    #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Quoted 2 lines

Frank

I followed the suggestion you provided at the given link at the end. Talking of Xilinx based devices, the channel separation is fixed for MDB but not for CPM6. So, I have have renamed the specific functionality specific to CPM6 in 2/3 of the same patch series.

Quoted 4 lines

This one cleans up the MDB references and makes the functionality generic for all the Xilinx devices.

Suggestion: https://lore.kernel.org/all/amI09ZcfhX4SEPIH@SMW015318/

Maybe I have not said cleanly

Actually you only need add one macro to for CPM6, other use MDB macro can work well.

Quoted 6 lines

#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP GENMASK(18, 16)

These rename are totally unnecessary. It is quite popular that new chip reuse old chip's register name.

We rename it utils we really can't handle these without rename.

You have pointed out the issue in the last line. For the upcoming support we anticipated that it would be messy to use the MDB related stuff for CPM6 and that's why wanted to clean it before introducing functionality for CPM6.

Quoted 3 lines

Do it only when have to do that. Now only add one macro DW_PCIE_XILINX_CPM6_VSEC_CH_SEP to support CPM6.

Here also we realized that using MDB would create the confusion later on when more features related to CPM6 will come out.

We are trying to contain the issue early, that is why putting up the effort of segregating the macro names.

As you have already suggested that the names can be changed but when the need arises. We see the need and handling it early.
Request to consider the approach:
- All the Xilinx specific macros, providing common functionality, applicable to MDB and CPM6 are made name agnostic. The macros will go by the Xilinx name similar to Synopsys declarations.

- Macros used specifically for MDB will have MDB to specify the association with the IP and similar approach for CPM6.

Also, as of now, as we are aware of both MDB and CPM6, they look related but there would be end users of these IPs who would not be knowing the MDB if working on CPM6 and vice-versa. This distinction helps in easy understanding rather than creating confusion due to naming dependance on MDB.

-Devendra

Quoted 123 lines

The prefix "DW_PCIE_XILINX_MDB" and "DW_PCIE_XILINX_VSEC" have not bring much information,

The most important part following part after it.

Frank

Quoted 116 lines

The MDB works, no denying, but mixing the two creates confusion and requires the understanding of MDB first and CPM6. For that reason the clean approach taken is:
- Add the generic functionality supported by MDB and CPM6 under the name Xilinx
- Add the MDB and CPM6 specific functionality with their own associated names, keeping it clean.

- Devendra>

Quoted 105 lines

Frank

Quoted 103 lines

CPM6 specific declaration:
https://lore.kernel.org/all/[email protected]/

- Devendra

Quoted 96 lines

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
    #define DW_BLOCK(a, b, c) \
    	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
    	u16 vsec;
    	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
    	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
    	if (!vsec)
    		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
    	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
    	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
    	if (map != EDMA_MF_HDMA_NATIVE)
    		return;
 
    	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
    	pci_read_config_dword(pdev, vsec + 0xc, &val);
    	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
    	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
    	pci_read_config_dword(pdev, vsec + 0x14, &val);
    	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
    	pdata->rg.off = off;
 
    	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
    	if (!vsec)
    		return;
 
    	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
    			      &val);
    	off = val;
    	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
    			      &val);
    	off <<= 32;
    	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
    		 * address use the non-LL mode or simple mode supported by
    		 * the HDMA IP.
    		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
    			non_ll = true;
 
    		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
    		 */
    		if (!non_ll)
    			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
    	}
 
    	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 89 lines · Devendra

Quoted 87 lines · Frank Li

Quoted 76 lines · Devendra

Quoted 74 lines · Frank Li

Quoted 58 lines · Devendra

Quoted 47 lines · Frank Li

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
     drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
     1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
     #define PCI_DEVICE_ID_XILINX_B054		0xb054
     #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Quoted 2 lines

Frank

I followed the suggestion you provided at the given link at the end. Talking of Xilinx based devices, the channel separation is fixed for MDB but not for CPM6. So, I have have renamed the specific functionality specific to CPM6 in 2/3 of the same patch series.

Quoted 4 lines

This one cleans up the MDB references and makes the functionality generic for all the Xilinx devices.

Suggestion: https://lore.kernel.org/all/amI09ZcfhX4SEPIH@SMW015318/

Maybe I have not said cleanly

Actually you only need add one macro to for CPM6, other use MDB macro can work well.

#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP GENMASK(18, 16)

These rename are totally unnecessary. It is quite popular that new chip reuse old chip's register name.

Quoted 6 lines

Quoted 1 line

We rename it utils we really can't handle these without rename.

You have pointed out the issue in the last line. For the upcoming support we anticipated that it would be messy to use the MDB related stuff for CPM6 and that's why wanted to clean it before introducing functionality for CPM6.

Do it only when have to do that. Now only add one macro DW_PCIE_XILINX_CPM6_VSEC_CH_SEP to support CPM6.

Quoted 2 lines

Here also we realized that using MDB would create the confusion later on when more features related to CPM6 will come out.

Do it when you post new features. We will make decistion at that time.

Now only one DW_PCIE_XILINX_CPM6_VSEC_CH_SEP is NOT enough to support rename now.

Unnecessary rename will cause meanless conflict when back port some fix patches to stable tree in future.

Frank

Quoted 144 lines

We are trying to contain the issue early, that is why putting up the effort of segregating the macro names.

As you have already suggested that the names can be changed but when the need arises. We see the need and handling it early.
Request to consider the approach:
- All the Xilinx specific macros, providing common functionality, applicable to MDB and CPM6 are made name agnostic. The macros will go by the Xilinx name similar to Synopsys declarations.

- Macros used specifically for MDB will have MDB to specify the association with the IP and similar approach for CPM6.

Also, as of now, as we are aware of both MDB and CPM6, they look related but there would be end users of these IPs who would not be knowing the MDB if working on CPM6 and vice-versa. This distinction helps in easy understanding rather than creating confusion due to naming dependance on MDB.

-Devendra

Quoted 123 lines

The prefix "DW_PCIE_XILINX_MDB" and "DW_PCIE_XILINX_VSEC" have not bring much information,

The most important part following part after it.

Frank

Quoted 116 lines

The MDB works, no denying, but mixing the two creates confusion and requires the understanding of MDB first and CPM6. For that reason the clean approach taken is:
- Add the generic functionality supported by MDB and CPM6 under the name Xilinx
- Add the MDB and CPM6 specific functionality with their own associated names, keeping it clean.

- Devendra>

Quoted 105 lines

Frank

Quoted 103 lines

CPM6 specific declaration:
https://lore.kernel.org/all/[email protected]/

- Devendra

Quoted 96 lines

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
     #define DW_BLOCK(a, b, c) \
     	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
     	u16 vsec;
     	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
     	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
     	if (!vsec)
     		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
     	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
     	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
     	if (map != EDMA_MF_HDMA_NATIVE)
     		return;
 
     	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
     	pci_read_config_dword(pdev, vsec + 0xc, &val);
     	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
     	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
     	pci_read_config_dword(pdev, vsec + 0x14, &val);
     	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
     	pdata->rg.off = off;
 
     	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
     	if (!vsec)
     		return;
 
     	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
     			      &val);
     	off = val;
     	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
     			      &val);
     	off <<= 32;
     	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
     		 * address use the non-LL mode or simple mode supported by
     		 * the HDMA IP.
     		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
     			non_ll = true;
 
     		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
     		 */
     		if (!non_ll)
     			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
     	}
 
     	/* Mapping PCI BAR regions */
--

2.43.0

Quoted 93 lines · Frank Li

Quoted 91 lines · Devendra

Quoted 79 lines · Frank Li

Quoted 78 lines · Devendra

Quoted 75 lines · Frank Li

Quoted 58 lines · Devendra

Quoted 47 lines · Frank Li

Quoted 33 lines · Devendra K Verma

Xilinx specific macros for MDB device can be reused for the Xilinx supported other similar IP such as CPM6.
Renamed the Xilinx specific macros in a way that can be reused for Xilinx supported upcoming IP, CPM6.
Naming is in accordance with the naming done for Synopsys macros.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o No change

---
     drivers/dma/dw-edma/dw-edma-pcie.c | 58 +++++++++++++++---------------
     1 file changed, 29 insertions(+), 29 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +0 -11

index 791c46e8ae4c..ec5e057a0f11 100644
@@ -29,21 +29,21 @@
     #define PCI_DEVICE_ID_XILINX_B054		0xb054
     #define PCI_DEVICE_ID_XILINX_B00F		0xb00f
 
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
-#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_BAR		GENMASK(10, 8)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_MAP		GENMASK(2, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH	GENMASK(9, 0)
-#define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH	GENMASK(25, 16)
-
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
-#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8
-#define DW_PCIE_XILINX_MDB_INVALID_ADDR		(~0ULL)
-

drivers/dma/dw-edma/dw-edma-pcie.c line 37

Quoted 10 lines

drivers/dma/dw-edma/dw-edma-pcie.c +6 -4

-#define DW_PCIE_XILINX_MDB_LL_OFF_GAP		0x200000
-#define DW_PCIE_XILINX_MDB_LL_SIZE		0x800
-#define DW_PCIE_XILINX_MDB_DT_OFF_GAP		0x100000
-#define DW_PCIE_XILINX_MDB_DT_SIZE		0x800
+#define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
+#define DW_PCIE_XILINX_VSEC_ID			0x20
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+#define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)

v1, you agree keep use old MDB ?

Quoted 2 lines

Frank

I followed the suggestion you provided at the given link at the end. Talking of Xilinx based devices, the channel separation is fixed for MDB but not for CPM6. So, I have have renamed the specific functionality specific to CPM6 in 2/3 of the same patch series.

Quoted 4 lines

This one cleans up the MDB references and makes the functionality generic for all the Xilinx devices.

Suggestion: https://lore.kernel.org/all/amI09ZcfhX4SEPIH@SMW015318/

Maybe I have not said cleanly

Actually you only need add one macro to for CPM6, other use MDB macro can work well.

#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP GENMASK(18, 16)

These rename are totally unnecessary. It is quite popular that new chip reuse old chip's register name.

We rename it utils we really can't handle these without rename.

Quoted 8 lines

Quoted 4 lines

You have pointed out the issue in the last line. For the upcoming support we anticipated that it would be messy to use the MDB related stuff for CPM6 and that's why wanted to clean it before introducing functionality for CPM6.

Do it only when have to do that. Now only add one macro DW_PCIE_XILINX_CPM6_VSEC_CH_SEP to support CPM6.

Here also we realized that using MDB would create the confusion later on when more features related to CPM6 will come out.

Quoted 5 lines

Do it when you post new features. We will make decistion at that time.

Now only one DW_PCIE_XILINX_CPM6_VSEC_CH_SEP is NOT enough to support rename now.

Could you please tell how many changes are required to have the renaming related patch to be pushed?

Quoted 3 lines

Unnecessary rename will cause meanless conflict when back port some fix patches to stable tree in future.

This issue will occure irrespective of when patches are pushed.

Anyways, I will be changing the code as per recommendation. Here we were trying to reduce the misunderstanding but it is not recommended, for 2/3 of the same patch series, the variable causes misunderstanding despite reasons/context.

I will push the changes, please review them. Thanks!

Quoted 146 lines

Frank

Quoted 144 lines

We are trying to contain the issue early, that is why putting up the effort of segregating the macro names.

As you have already suggested that the names can be changed but when the need arises. We see the need and handling it early.
Request to consider the approach:
- All the Xilinx specific macros, providing common functionality, applicable to MDB and CPM6 are made name agnostic. The macros will go by the Xilinx name similar to Synopsys declarations.

- Macros used specifically for MDB will have MDB to specify the association with the IP and similar approach for CPM6.

Also, as of now, as we are aware of both MDB and CPM6, they look related but there would be end users of these IPs who would not be knowing the MDB if working on CPM6 and vice-versa. This distinction helps in easy understanding rather than creating confusion due to naming dependance on MDB.

-Devendra

Quoted 123 lines

The prefix "DW_PCIE_XILINX_MDB" and "DW_PCIE_XILINX_VSEC" have not bring much information,

The most important part following part after it.

Frank

Quoted 116 lines

The MDB works, no denying, but mixing the two creates confusion and requires the understanding of MDB first and CPM6. For that reason the clean approach taken is:
- Add the generic functionality supported by MDB and CPM6 under the name Xilinx
- Add the MDB and CPM6 specific functionality with their own associated names, keeping it clean.

- Devendra>

Quoted 105 lines

Frank

Quoted 103 lines

CPM6 specific declaration:
https://lore.kernel.org/all/[email protected]/

- Devendra

Quoted 96 lines

Quoted 96 lines

drivers/dma/dw-edma/dw-edma-pcie.c +23 -15

+
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH	0xc
+#define DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW	0x8
+#define DW_PCIE_XILINX_INVALID_ADDR		(~0ULL)
+
+#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
+#define DW_PCIE_XILINX_LL_SIZE			0x800
+#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
+#define DW_PCIE_XILINX_DT_SIZE			0x800
 
     #define DW_BLOCK(a, b, c) \
     	{ \

@@ -258,10 +258,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
     	u16 vsec;
     	u64 off;
 
-	pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
+	pdata->devmem_phys_off = DW_PCIE_XILINX_INVALID_ADDR;
 
     	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
+					DW_PCIE_XILINX_VSEC_DMA_ID);
     	if (!vsec)
     		return;
 

@@ -272,18 +272,18 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 
     	pci_dbg(pdev, "Detected Xilinx PCIe Vendor-Specific Extended Capability DMA\n");
     	pci_read_config_dword(pdev, vsec + 0x8, &val);
-	map = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_MAP, val);
+	map = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_MAP, val);
     	if (map != EDMA_MF_HDMA_NATIVE)
     		return;
 
     	pdata->mf = map;
-	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
+	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
     	pci_read_config_dword(pdev, vsec + 0xc, &val);
     	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));
     	pdata->rd_ch_cnt = min(pdata->rd_ch_cnt,
-			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH, val));
+			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_RD_CH, val));
 
     	pci_read_config_dword(pdev, vsec + 0x14, &val);
     	off = val;

@@ -293,16 +293,16 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
     	pdata->rg.off = off;
 
     	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
-					DW_PCIE_XILINX_MDB_VSEC_ID);
+					DW_PCIE_XILINX_VSEC_ID);
     	if (!vsec)
     		return;
 
     	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_HIGH,
     			      &val);
     	off = val;
     	pci_read_config_dword(pdev,
-			      vsec + DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW,
+			      vsec + DW_PCIE_XILINX_DEVMEM_OFF_REG_LOW,
     			      &val);
     	off <<= 32;
     	off |= val;

@@ -360,7 +360,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
     		 * address use the non-LL mode or simple mode supported by
     		 * the HDMA IP.
     		 */
-		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_MDB_INVALID_ADDR)
+		if (vsec_data->devmem_phys_off == DW_PCIE_XILINX_INVALID_ADDR)
     			non_ll = true;
 
     		/*

@@ -370,10 +370,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
     		 */
     		if (!non_ll)
     			dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
-						       DW_PCIE_XILINX_MDB_LL_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_LL_SIZE,
-						       DW_PCIE_XILINX_MDB_DT_OFF_GAP,
-						       DW_PCIE_XILINX_MDB_DT_SIZE);
+						       DW_PCIE_XILINX_LL_OFF_GAP,
+						       DW_PCIE_XILINX_LL_SIZE,
+						       DW_PCIE_XILINX_DT_OFF_GAP,
+						       DW_PCIE_XILINX_DT_SIZE);
     	}
 
     	/* Mapping PCI BAR regions */
--

2.43.0

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
 drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
 #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
 #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
 #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
 #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
 #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

 };
 
 static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
 };
 
 static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
 	.rg.bar				= BAR_0,
 	.rg.off				= SZ_4K,	/*  4 Kbytes */
 	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
 	return pci_irq_vector(to_pci_dev(dev), nr);
 }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
 static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
 	pdata->mf = map;
 	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
 	pci_read_config_dword(pdev, vsec + 0xc, &val);
 	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
 			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

 	if (!pdata)
 		return -ENODEV;
--

2.43.0

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
  drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
  1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
  #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
  #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
  #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
  #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
  #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
  	u16				wr_ch_cnt;
  	u16				rd_ch_cnt;
  	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

  };
 
  static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
  };
 
  static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
  	.rg.bar				= BAR_0,
  	.rg.off				= SZ_4K,	/*  4 Kbytes */
  	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
  	return pci_irq_vector(to_pci_dev(dev), nr);
  }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
  static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
  {
  	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
  	pdata->mf = map;
  	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
  	pci_read_config_dword(pdev, vsec + 0xc, &val);
  	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
  			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
  	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
  	struct device *dev = &pdev->dev;
  	struct dw_edma_chip *chip;
+	bool non_ll = false;
  	int err, nr_irqs;
  	int i, mask;
-	bool non_ll = false;

unnecesary change

Frank

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

  	if (!pdata)
  		return -ENODEV;
--

2.43.0

Quoted 40 lines · Frank Li

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
  drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
  1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
  #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
  #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
  #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
  #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
  #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
  	u16				wr_ch_cnt;
  	u16				rd_ch_cnt;
  	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

  };
 
  static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
  };
 
  static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
  	.rg.bar				= BAR_0,
  	.rg.off				= SZ_4K,	/*  4 Kbytes */
  	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
  	return pci_irq_vector(to_pci_dev(dev), nr);
  }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
  static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
  {
  	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
  	pdata->mf = map;
  	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
  	pci_read_config_dword(pdev, vsec + 0xc, &val);
  	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
  			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
  	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
  	struct device *dev = &pdev->dev;
  	struct dw_edma_chip *chip;
+	bool non_ll = false;
  	int err, nr_irqs;
  	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

  	if (!pdata)
  		return -ENODEV;
--

2.43.0

Quoted 51 lines · Devendra

Quoted 47 lines · Frank Li

Quoted 40 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
   drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
   1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
   #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
   #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
   #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
   #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
   #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 4 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
   	u16				wr_ch_cnt;
   	u16				rd_ch_cnt;
   	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 2 lines

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

which version? I only find "Register Wire Channel seperation" at 6.21a Feb 2025.

Frank

Quoted 81 lines

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

   };
 
   static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
   };
 
   static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
   	.rg.bar				= BAR_0,
   	.rg.off				= SZ_4K,	/*  4 Kbytes */
   	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
   	return pci_irq_vector(to_pci_dev(dev), nr);
   }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
   static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
   {
   	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
   	pdata->mf = map;
   	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
   	pci_read_config_dword(pdev, vsec + 0xc, &val);
   	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
   			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
   	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
   	struct device *dev = &pdev->dev;
   	struct dw_edma_chip *chip;
+	bool non_ll = false;
   	int err, nr_irqs;
   	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

   	if (!pdata)
   		return -ENODEV;
--

2.43.0

Quoted 58 lines · Frank Li

Quoted 51 lines · Devendra

Quoted 40 lines · Frank Li

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
   drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
   1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
   #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
   #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
   #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
   #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
   #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
   	u16				wr_ch_cnt;
   	u16				rd_ch_cnt;
   	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 2 lines

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

which version? I only find "Register Wire Channel seperation" at 6.21a Feb 2025.

Quoted 1 line

Frank

We have the reference from RM - 6.00a June 2022, following sections:
- Table 1-4
- Sec 3.2.34.3, Table 3-276 (VSECDMA_DEVICE_INFORMATION) refer to channel separation and describe for HDMA

-Devendra

Quoted 81 lines

Quoted 81 lines

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

   };
 
   static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
   };
 
   static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
   	.rg.bar				= BAR_0,
   	.rg.off				= SZ_4K,	/*  4 Kbytes */
   	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
   	return pci_irq_vector(to_pci_dev(dev), nr);
   }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
   static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
   {
   	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
   	pdata->mf = map;
   	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
   	pci_read_config_dword(pdev, vsec + 0xc, &val);
   	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
   			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
   	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
   	struct device *dev = &pdev->dev;
   	struct dw_edma_chip *chip;
+	bool non_ll = false;
   	int err, nr_irqs;
   	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

   	if (!pdata)
   		return -ENODEV;
--

2.43.0

Quoted 59 lines · Devendra

Quoted 58 lines · Frank Li

Quoted 51 lines · Devendra

Quoted 40 lines · Frank Li

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
    drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
    1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
    #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
    #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
    #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
    #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
    #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
    	u16				wr_ch_cnt;
    	u16				rd_ch_cnt;
    	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 2 lines

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

which version? I only find "Register Wire Channel seperation" at 6.21a Feb 2025.

Quoted 6 lines

Quoted 1 line

Frank

We have the reference from RM - 6.00a June 2022, following sections:
- Table 1-4
- Sec 3.2.34.3, Table 3-276 (VSECDMA_DEVICE_INFORMATION) refer to channel separation and describe for HDMA

But this is not register value, the register MACRO defination should follow spec.

"Channel Separation: Address distance between read and write channels."

helper function dw_edma_get_ch_sep_sz() convert register value to this field.

The field should use software easy understand/generic term, so reader can know what's that easily without check spec. Driver need do some abstract, instead direct copy from hardware.

Frank

Quoted 11 lines

-Devendra

Quoted 7 lines

Quoted 7 lines

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Quoted 68 lines

Quoted 67 lines

Quoted 67 lines

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

    };
 
    static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
    };
 
    static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
    	.rg.bar				= BAR_0,
    	.rg.off				= SZ_4K,	/*  4 Kbytes */
    	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
    	return pci_irq_vector(to_pci_dev(dev), nr);
    }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
    static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
    {
    	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
    	pdata->mf = map;
    	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
    	pci_read_config_dword(pdev, vsec + 0xc, &val);
    	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
    			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
    	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
    	struct device *dev = &pdev->dev;
    	struct dw_edma_chip *chip;
+	bool non_ll = false;
    	int err, nr_irqs;
    	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

    	if (!pdata)
    		return -ENODEV;
--

2.43.0

Quoted 72 lines · Frank Li

Quoted 59 lines · Devendra

Quoted 58 lines · Frank Li

Quoted 51 lines · Devendra

Quoted 40 lines · Frank Li

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
    drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
    1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
    #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
    #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
    #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
    #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
    #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
    	u16				wr_ch_cnt;
    	u16				rd_ch_cnt;
    	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 2 lines

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

which version? I only find "Register Wire Channel seperation" at 6.21a Feb 2025.

Quoted 6 lines

Quoted 1 line

Frank

We have the reference from RM - 6.00a June 2022, following sections:
- Table 1-4
- Sec 3.2.34.3, Table 3-276 (VSECDMA_DEVICE_INFORMATION) refer to channel separation and describe for HDMA

But this is not register value, the register MACRO defination should follow spec.

"Channel Separation: Address distance between read and write channels."

Quoted 9 lines

helper function dw_edma_get_ch_sep_sz() convert register value to this field.

The field should use software easy understand/generic term, so reader can know what's that easily without check spec. Driver need do some abstract, instead direct copy from hardware.

Frank

The other macros defined also follow the similar format as mentioned in the RM. I did not deviate from the rule and used the variable which is both understood in software and hardware context alike.
Please follow the discussion below.

Quoted 11 lines

Quoted 11 lines

-Devendra

Quoted 7 lines

Quoted 7 lines

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Also, as pointed out in this thread, it was a learning how naming shall be convenient and relevant to the concept being implemented and that's where the idea of naming the variable close to software and hardware was taken up. It kind of helps in relating with the RM data.

-Devendra

Quoted 68 lines

Quoted 68 lines

Quoted 67 lines

Quoted 67 lines

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

    };
 
    static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
    };
 
    static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
    	.rg.bar				= BAR_0,
    	.rg.off				= SZ_4K,	/*  4 Kbytes */
    	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
    	return pci_irq_vector(to_pci_dev(dev), nr);
    }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
    static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
    {
    	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
    	pdata->mf = map;
    	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
    	pci_read_config_dword(pdev, vsec + 0xc, &val);
    	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
    			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
    	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
    	struct device *dev = &pdev->dev;
    	struct dw_edma_chip *chip;
+	bool non_ll = false;
    	int err, nr_irqs;
    	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

    	if (!pdata)
    		return -ENODEV;
--

2.43.0

Quoted 107 lines · Devendra

Quoted 72 lines · Frank Li

Quoted 59 lines · Devendra

Quoted 58 lines · Frank Li

Quoted 51 lines · Devendra

Quoted 40 lines · Frank Li

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
     drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
     1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
     #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
     #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
     #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
     #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
     #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
     	u16				wr_ch_cnt;
     	u16				rd_ch_cnt;
     	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 2 lines

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

which version? I only find "Register Wire Channel seperation" at 6.21a Feb 2025.

Quoted 6 lines

Quoted 1 line

Frank

We have the reference from RM - 6.00a June 2022, following sections:
- Table 1-4
- Sec 3.2.34.3, Table 3-276 (VSECDMA_DEVICE_INFORMATION) refer to channel separation and describe for HDMA

But this is not register value, the register MACRO defination should follow spec.

"Channel Separation: Address distance between read and write channels."

Quoted 9 lines

helper function dw_edma_get_ch_sep_sz() convert register value to this field.

The field should use software easy understand/generic term, so reader can know what's that easily without check spec. Driver need do some abstract, instead direct copy from hardware.

Frank

The other macros defined also follow the similar format as mentioned in the RM. I did not deviate from the rule and used the variable which is both understood in software and hardware context alike.
Please follow the discussion below.

Quoted 11 lines

Quoted 11 lines

-Devendra

Quoted 7 lines

Quoted 7 lines

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Quoted 5 lines

Also, as pointed out in this thread, it was a learning how naming shall be convenient and relevant to the concept being implemented and that's where the idea of naming the variable close to software and hardware was taken up. It kind of helps in relating with the RM data.

The term "channel space size" (ch_space_sz) is more commonly used in kernel drivers. It is widely seen across various DMA engine drivers and is generally easier for software developers to understand. In contrast, ch_sep_sz appears to be more specific to your hardware implementation.

The other existing fields are self-documenting, clean, and easy to understand.

Additionally, this change introduces a conversion from the register value through an algorithm, even though the calculation itself is relatively simple.

dw_edma_pcie_data is not only used for your hardware.

You register name already align RM data, which is already enough.

Frank

Quoted 72 lines

-Devendra

Quoted 68 lines

Quoted 68 lines

Quoted 67 lines

Quoted 67 lines

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

     };
 
     static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
     };
 
     static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
     	.rg.bar				= BAR_0,
     	.rg.off				= SZ_4K,	/*  4 Kbytes */
     	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
     	return pci_irq_vector(to_pci_dev(dev), nr);
     }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
     static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
     {
     	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
     	pdata->mf = map;
     	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
     	pci_read_config_dword(pdev, vsec + 0xc, &val);
     	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
     			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
     	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
     	struct device *dev = &pdev->dev;
     	struct dw_edma_chip *chip;
+	bool non_ll = false;
     	int err, nr_irqs;
     	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

     	if (!pdata)
     		return -ENODEV;
--

2.43.0

Quoted 114 lines · Frank Li

Quoted 107 lines · Devendra

Quoted 72 lines · Frank Li

Quoted 59 lines · Devendra

Quoted 58 lines · Frank Li

Quoted 51 lines · Devendra

Quoted 40 lines · Frank Li

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
     drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
     1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
     #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
     #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
     #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
     #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
     #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
     	u16				wr_ch_cnt;
     	u16				rd_ch_cnt;
     	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 2 lines

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

which version? I only find "Register Wire Channel seperation" at 6.21a Feb 2025.

Quoted 6 lines

Quoted 1 line

Frank

We have the reference from RM - 6.00a June 2022, following sections:
- Table 1-4
- Sec 3.2.34.3, Table 3-276 (VSECDMA_DEVICE_INFORMATION) refer to channel separation and describe for HDMA

But this is not register value, the register MACRO defination should follow spec.

"Channel Separation: Address distance between read and write channels."

Quoted 9 lines

helper function dw_edma_get_ch_sep_sz() convert register value to this field.

The field should use software easy understand/generic term, so reader can know what's that easily without check spec. Driver need do some abstract, instead direct copy from hardware.

Frank

The other macros defined also follow the similar format as mentioned in the RM. I did not deviate from the rule and used the variable which is both understood in software and hardware context alike.
Please follow the discussion below.

Quoted 11 lines

Quoted 11 lines

-Devendra

Quoted 7 lines

Quoted 7 lines

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Quoted 5 lines

Also, as pointed out in this thread, it was a learning how naming shall be convenient and relevant to the concept being implemented and that's where the idea of naming the variable close to software and hardware was taken up. It kind of helps in relating with the RM data.

Quoted 5 lines

The term "channel space size" (ch_space_sz) is more commonly used in kernel drivers. It is widely seen across various DMA engine drivers and is generally easier for software developers to understand. In contrast, ch_sep_sz appears to be more specific to your hardware implementation.

Hi Frank, could we have a little libral approach about the naming conventions?
I checked the linux/drivers and there is no such name exists ch_space_sz or ch_space_size. The term 'channel space size' may have

been commonly used but that is not getting translated into any variable in the linux/drivers code.
I checked it here: https://elixir.bootlin.com/linux/v7.2-rc5/source
and manually within the source code.

I get the intent that widely used names help in easy understanding but if a variable fits a requirement and has a valid reasoning then it should be allowed instead of having of single point of view. Some variables may have variations, if it does not change the meaning drastically then I guess it should have been allowed if not causing much harm.

The selection of name is based on two such references so it is backed by the evidence as well:
- RM widely mentions it as chsep_256, etc, it can be directly related with the code and vice-versa.
- There was one such previous discussion over the exact similar topic, how the name 'non_ll' was concluded, even though it sounded as negation but fit the requirement, eventually.

Also, it makes developers totally dependent on the reviewers as only the selected name will go, not even the variations are allowed to passthrough.
Please consider the name once, it was well thought about in advance.

-Devendra

Quoted 84 lines

The other existing fields are self-documenting, clean, and easy to understand.

Additionally, this change introduces a conversion from the register value through an algorithm, even though the calculation itself is relatively simple.

dw_edma_pcie_data is not only used for your hardware.

You register name already align RM data, which is already enough.

Frank

Quoted 72 lines

-Devendra

Quoted 68 lines

Quoted 68 lines

Quoted 67 lines

Quoted 67 lines

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

     };
 
     static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
     };
 
     static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
     	.rg.bar				= BAR_0,
     	.rg.off				= SZ_4K,	/*  4 Kbytes */
     	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
     	return pci_irq_vector(to_pci_dev(dev), nr);
     }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
     static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
     {
     	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
     	pdata->mf = map;
     	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
     	pci_read_config_dword(pdev, vsec + 0xc, &val);
     	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
     			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
     	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
     	struct device *dev = &pdev->dev;
     	struct dw_edma_chip *chip;
+	bool non_ll = false;
     	int err, nr_irqs;
     	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

     	if (!pdata)
     		return -ENODEV;
--

2.43.0

Quoted 121 lines · Devendra

Quoted 119 lines · Frank Li

Quoted 107 lines · Devendra

Quoted 72 lines · Frank Li

Quoted 59 lines · Devendra

Quoted 58 lines · Frank Li

Quoted 51 lines · Devendra

Quoted 40 lines · Frank Li

Quoted 39 lines · Devendra K Verma

As per, 'Designware Cores PCI Express DM Controller - Reference Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports the channel separation mechanisms. Basically, the HDMA IP allows the user to configure the separation between DMA channel registers and retrieve it via the VSEC capability mentioned above.

HDMA IP supports the channel register space separation from 256B to 32KB. Default supported size is 256B.

Signed-off-by Devendra K Verma <[email protected]>

---
Changes in v1:
o Modified dw_edma_get_ch_sep_sz() as per review comment. The function now supports ch_sep_sz up to 32KB.
o Updated to description to reflect the supported channel separation sizes.
o Introduced the CPM6 specific macro for VSEC cap.

---
      drivers/dma/dw-edma/dw-edma-pcie.c | 21 ++++++++++++++++++---
      1 file changed, 18 insertions(+), 3 deletions(-)

drivers/dma/dw-edma/dw-edma-pcie.c +4 -1

index ec5e057a0f11..d0f209082878 100644
@@ -31,8 +31,11 @@
 
      #define DW_PCIE_XILINX_VSEC_DMA_ID		0x6
      #define DW_PCIE_XILINX_VSEC_ID			0x20
-#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
      #define DW_PCIE_XILINX_VSEC_DMA_MAP		GENMASK(2, 0)
+#define DW_PCIE_XILINX_VSEC_DMA_BAR		GENMASK(10, 8)
+/* AMD CPM6 (Xilinx) supported cap */
+#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP		GENMASK(18, 16)
+
      #define DW_PCIE_XILINX_VSEC_DMA_WR_CH		GENMASK(9, 0)
      #define DW_PCIE_XILINX_VSEC_DMA_RD_CH		GENMASK(25, 16)
 

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c struct dw_edma_pcie_data { · line 79

Quoted 5 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -0

@@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
      	u16				wr_ch_cnt;
      	u16				rd_ch_cnt;
      	u64				devmem_phys_off;
+	u32				ch_sep_sz;

ch_space_sz?

Quoted 2 lines

In the document, Designware Cores PCI Express controller Reference Manual, it widely referred as channel separation and ch_sep

which version? I only find "Register Wire Channel seperation" at 6.21a Feb 2025.

Quoted 6 lines

Quoted 1 line

Frank

We have the reference from RM - 6.00a June 2022, following sections:
- Table 1-4
- Sec 3.2.34.3, Table 3-276 (VSECDMA_DEVICE_INFORMATION) refer to channel separation and describe for HDMA

But this is not register value, the register MACRO defination should follow spec.

"Channel Separation: Address distance between read and write channels."

Quoted 9 lines

helper function dw_edma_get_ch_sep_sz() convert register value to this field.

The field should use software easy understand/generic term, so reader can know what's that easily without check spec. Driver need do some abstract, instead direct copy from hardware.

Frank

The other macros defined also follow the similar format as mentioned in the RM. I did not deviate from the rule and used the variable which is both understood in software and hardware context alike.
Please follow the discussion below.

Quoted 11 lines

Quoted 11 lines

-Devendra

Quoted 7 lines

Quoted 7 lines

has been used so in order to keep the relevance with the document, I chose it to be ch_sep_sz. The other name used is chaddr_space but it is not used widely as the ch_sep.
With your suggestion, it can be infered to be same but relevance with the document may not be established.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

In the past, a similar discussion we had for choosing the non_ll naming and chose this as it mentioned and has relevance with the document.

Reference
https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/

Quoted 5 lines

Also, as pointed out in this thread, it was a learning how naming shall be convenient and relevant to the concept being implemented and that's where the idea of naming the variable close to software and hardware was taken up. It kind of helps in relating with the RM data.

The term "channel space size" (ch_space_sz) is more commonly used in kernel drivers. It is widely seen across various DMA engine drivers and is generally easier for software developers to understand. In contrast, ch_sep_sz appears to be more specific to your hardware implementation.

Quoted 4 lines

Hi Frank, could we have a little libral approach about the naming conventions?
I checked the linux/drivers and there is no such name exists ch_space_sz or ch_space_size. The term 'channel space size' may have

git grep -E 'space_sz|space_size'

Quoted 9 lines

Quoted 2 lines

The term "channel space size" (ch_space_sz) is more commonly used in kernel drivers.

been commonly used but that is not getting translated into any variable in the linux/drivers code.
I checked it here: https://elixir.bootlin.com/linux/v7.2-rc5/source
and manually within the source code.

Quoted 6 lines

I get the intent that widely used names help in easy understanding but if a variable fits a requirement and has a valid reasoning then it should be allowed instead of having of single point of view. Some variables may have variations, if it does not change the meaning drastically then I guess it should have been allowed if not causing much harm.

Not exactly, field in drvdata will used by other vendor also. Not only for AMD. I checked origin DW design book, ch_sep is hardware signal/config name.

AMD just reuse it. This driver is NOT dedicated for AMD.

Quoted 8 lines

The selection of name is based on two such references so it is backed by the evidence as well:
- RM widely mentions it as chsep_256, etc, it can be directly related with the code and vice-versa.
- There was one such previous discussion over the exact similar topic, how the name 'non_ll' was concluded, even though it sounded as negation but fit the requirement, eventually.

Because "ll" is enough popular as link list. Many dma driver use it. git grep "lli" drivers/dma

non is just prefix, everyone know thats. "sep" is easy to miss understand as seperate token/chars.

naming itself for both non_ll and (has)_ll are accetable.

Quoted 5 lines

Also, it makes developers totally dependent on the reviewers as only the selected name will go, not even the variations are allowed to passthrough.
Please consider the name once, it was well thought about in advance.

One of reviewer/maintainer's reponsible is make API or key name consistent and maintainable in long term.

drvdata is high frequency change data when new hardware appear. We need make it easy understand and self documented.

Frank

Quoted 88 lines

-Devendra

Quoted 84 lines

The other existing fields are self-documenting, clean, and easy to understand.

Additionally, this change introduces a conversion from the register value through an algorithm, even though the calculation itself is relatively simple.

dw_edma_pcie_data is not only used for your hardware.

You register name already align RM data, which is already enough.

Frank

Quoted 72 lines

-Devendra

Quoted 68 lines

Quoted 68 lines

Quoted 67 lines

Quoted 67 lines

Quoted 49 lines

Quoted 39 lines

drivers/dma/dw-edma/dw-edma-pcie.c +12 -1

      };
 
      static const struct dw_edma_pcie_data snps_edda_data = {

@@ -127,7 +131,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = {
      };
 
      static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
-	/* MDB registers location */
+	/* CPM6 registers location */
      	.rg.bar				= BAR_0,
      	.rg.off				= SZ_4K,	/*  4 Kbytes */
      	.rg.sz				= SZ_8K,	/*  8 Kbytes */

@@ -189,6 +193,13 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
      	return pci_irq_vector(to_pci_dev(dev), nr);
      }
 
+static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
+{
+	if (ch_sep_val > 0 && ch_sep_val <= 7)
+		return 256 << ch_sep_val;
+	return 256;
+}
+
      static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr)
      {
      	struct pci_dev *pdev = to_pci_dev(dev);

@@ -279,6 +290,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
      	pdata->mf = map;
      	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_BAR, val);
 
+	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
+		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
+								   val));
+
      	pci_read_config_dword(pdev, vsec + 0xc, &val);
      	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
      			       FIELD_GET(DW_PCIE_XILINX_VSEC_DMA_WR_CH, val));

drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 329

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +1 -1

@@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
      	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
      	struct device *dev = &pdev->dev;
      	struct dw_edma_chip *chip;
+	bool non_ll = false;
      	int err, nr_irqs;
      	int i, mask;
-	bool non_ll = false;

unnecesary change

Quoted 1 line

Frank

Yes, it is an unncessary change. It was introduced by me in some patch series and wanted to maintain the revers x-mas order that's why posted it. The declration order looks in the patch after this change. Please suggest is it OK to include changes like this or it is kind of extra scrutiny for the reviewers? I will make the change accordingly in next patch.

Quoted 7 lines

Quoted 7 lines

drivers/dma/dw-edma/dw-edma-pcie.c +0 -1

      	if (!pdata)
      		return -ENODEV;
--

2.43.0