Re: [PATCH v2 2/3] dmaengine: dw-edma: Enable Chan Separation via VSEC

[Frank Li <Frank.li@oss.nxp.com>]

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@amd.com>
> > > > > > > > > ---
> > > > > > > > > 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'

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

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

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

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