Re: [PATCH v2 2/3] dmaengine: dw-edma: Enable Chan Separation via VSEC
From: Verma, Devendra
Date: Thu Aug 06 2026 - 09:08:07 EST
On 05-Aug-26 04:03, Frank Li wrote:
On Mon, Aug 03, 2026 at 06:01:39PM +0530, Verma, Devendra wrote:
On 01-Aug-26 00:35, Frank Li wrote:
On Fri, Jul 31, 2026 at 10:03:10PM +0530, Verma, Devendra wrote:
On 31-Jul-26 21:04, Frank Li wrote:
On Fri, Jul 31, 2026 at 04:22:22PM +0530, Verma, Devendra wrote:
On 28-Jul-26 21:03, Frank Li wrote:
On Tue, Jul 28, 2026 at 02:47:43PM +0530, Devendra K Verma wrote:
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 <devverma@xxxxxxx>
---
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(-)
diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
index ec5e057a0f11..d0f209082878 100644
--- a/drivers/dma/dw-edma/dw-edma-pcie.c
+++ b/drivers/dma/dw-edma/dw-edma-pcie.c
@@ -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)
@@ -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
which version? I only find "Register Wire Channel seperation" at 6.21a
Feb 2025.
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
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.
-Devendra
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/
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.
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
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
-Devendra
};
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));
@@ -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
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.
if (!pdata)
return -ENODEV;
--
2.43.0