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

Hi Frank

Could you check and provide suggestion on the comments
inline?

regards,
Devendra

On 14-Aug-26 10:33, Verma, Devendra wrote:
> 
> On 13-Aug-26 22:01, Frank Li wrote:
>> On Thu, Aug 13, 2026 at 05:26:51PM +0530, Verma, Devendra wrote:
>>> On 10-Aug-26 22:44, Frank Li wrote:
>>>> On Mon, Aug 10, 2026 at 03:40:01PM +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 v2:
>>>>>      o Replaced 'ch_sep_sz' with 'ch_space_sz' wherever
>>>>>        applicable upon reviewer recommendation.
>>>>>      o Dropped 1/3 patch of this series, after review,
>>>>>        now the 2/3 is 1/2 of the current patch series.
>>>>>
>>>>> 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.
>>>>> ---
>>>>>    drivers/dma/dw-edma/dw-edma-pcie.c | 20 +++++++++++++++++---
>>>>>    1 file changed, 17 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw- 
>>>>> edma/dw-edma-pcie.c
>>>>> index 791c46e8ae4c..5d7507a64056 100644
>>>>> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
>>>>> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
>>>>> @@ -31,8 +31,10 @@
>>>>>
>>>>>    #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_BAR        GENMASK(10, 8)
>>>>
>>>> Don't mix code cleanup or reorg into this improvement patches.
>>>>
>>>
>>> It was recommended by you in the v1 of the same patch series that is
>>> why pushed it.
>>>
>>> Ref: https://lore.kernel.org/all/amDbGHRbObX-lt5F@SMW015318/
>>
>> You can fix the order by using prep patch, which just fix order, typo
>> or some cleanup.
>>
>> But not mixed into new feature patches.
>>
>>>
>>>>> +/* AMD CPM6 (Xilinx) supported cap */
>>>>> +#define DW_PCIE_XILINX_CPM6_VSEC_CH_SEP        GENMASK(18, 16)
>>>>>    #define DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH    GENMASK(9, 0)
>>>>>    #define DW_PCIE_XILINX_MDB_VSEC_DMA_RD_CH    GENMASK(25, 16)
>>>>>
>>>>> @@ -73,6 +75,7 @@ struct dw_edma_pcie_data {
>>>>>        u16                wr_ch_cnt;
>>>>>        u16                rd_ch_cnt;
>>>>>        u64                devmem_phys_off;
>>>>> +    u32                ch_space_sz;
>>>>>    };
>>>>>
>>>>>    static const struct dw_edma_pcie_data snps_edda_data = {
>>>>> @@ -127,7 +130,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 */
>>>>
>>>> This typo fix have not mixed this patch.
>>>
>>> This is subtle change. Since the focus of this patch series is CPM6, the
>>> namings and comments should align with the appropriate name.
>>>
> 
> Can we take up this change along with this patch series as it is related
> to CPM6 only?
> 
>>>>
>>>>>        .rg.bar                = BAR_0,
>>>>>        .rg.off                = SZ_4K,    /*  4 Kbytes */
>>>>>        .rg.sz                = SZ_8K,    /*  8 Kbytes */
>>>>> @@ -189,6 +192,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_space_sz(u32 val)
>>>>> +{
>>>>> +    if (val > 0 && val <= 7)
>>>>> +        return 256 << 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 +289,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_MDB_VSEC_DMA_BAR, 
>>>>> val);
>>>>>
>>>>> +    if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
>>>>> +        pdata->ch_space_sz = dw_edma_get_ch_space_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_MDB_VSEC_DMA_WR_CH, 
>>>>> val));
>>>>> @@ -324,9 +338,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;
>>>>
>>>> unnecssary change here
>>>>
>>>
>>> Yeah, this change was part of some patch series I pushed.
>>> This change was breaking the inverted X-mas format that's why
>>> made this change.
>>
>> But not related this patch. Use a small sperate patch to fix these small
>> problem.
>>
>> Use a small patch to do clean up, for example
>>
>> reorder register bit defination
>> fix commends about CPM6
>> fix inverted X-mas format.
>>
> The 1st and 3rd can be taken in a separate patch series but the 2nd
> is related to CPM6 only so having it is beneficial with this patch
> series, removes the misleading comment.
> 
> I will push the changes after your response.
> 

Any update on the above suggestion?

> -Devendra
> 
>  > > Frank
>>
>>>
>>> -Devendra
>>>
>>>> Frank
>>>>>
>>>>>        if (!pdata)
>>>>>            return -ENODEV;
>>>>> -- 
>>>>> 2.43.0
>>>>>
>>>
>