Re: [PATCH v2 2/3] dmaengine: dw-edma: Enable Chan Separation via VSEC
From: Verma, Devendra
Date: Fri Aug 07 2026 - 10:46:16 EST
On 06-Aug-26 20:50, Frank Li wrote:
On Thu, Aug 06, 2026 at 06:36:26PM +0530, Verma, Devendra wrote:
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
git grep -E 'space_sz|space_size'
The discussion started over the following, which is not so common as per
the suggestion.
>>> 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.
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.
Other vendors will also reference through the same RM for the same IP
only when need arises to study the use of the variable. It is neither specific to AMD nor reused by AMD. Simple rules, 1) correlate with RM, 2) useful to distinguish, are applied.
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.
May be it is looks easy now to judge for ll case, but please check the
conversation that followed for it. If it was that easy to push every one would have accepted it but I am glad that it was reasoned upon and
accepted which does not look like the case as this one.
Ref: https://lore.kernel.org/all/20251223162842.GA4022246@bhelgaas/
This is one way thought process, 'space' is a generic term and
ch_space_sz could also mean the space/BAR space it may require overall.
The misunderstanding may arise for 'space' as the misunderstanding
arose by 'sep'.
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.
The namings shall not only be consistent with usage but also with the
context it brings in.
Nevertheless, I got it, it is one way when it comes to the namings,
adding context to variables is not important the wide usage is the
policy, will be using it for further pushes.
Since so much discussion already happened, requesting to @mani,
@vkaul and @bhelgaas to provide inputs / comments about the general
practice being followed as you also are the maintainers for these files,
that way the patches can be aligned with the thought process and it can
go smoothly after review from the respectable maintainers. Thanks!
I will change the name. EoD.
-Devendra
drvdata is high frequency change data when new hardware appear. We need
make it easy understand and self documented.
Frank
-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