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

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

-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/
>>
>>>
>>>>    };
>>>>
>>>>    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
>>>>
>>