CPM6 Channel Separation Support
Subsystems (5)
v5 cover CPM6 Channel Separation Support
'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.
In order to use the VSEC functionality following changes have been made:
o Utilize the existing VSEC implementation to retrieve the channel separation and translate it to correct size.
o Modify the structs and functions to calculate correct DMA channel register base using the channel, direction and channel separation selected.
Devendra K Verma (2):
dmaengine: dw-edma: Enable Chan Separation via VSEC
dmaengine: dw-edma: Add changes to support Channel Separation
drivers/dma/dw-edma/dw-edma-pcie.c | 18 +++++++++++++++ drivers/dma/dw-edma/dw-hdma-v0-core.c | 23 ++++++++++++-------- drivers/dma/dw-edma/dw-hdma-v0-debugfs.c | 17 +++++---------- drivers/dma/dw-edma/dw-hdma-v0-regs.h | 10 --------- drivers/pci/controller/dwc/pcie-designware.c | 1 + include/linux/dma/edma.h | 1 + 6 files changed, 40 insertions(+), 30 deletions(-)
Signature · 1 line
2.43.0
PATCH RESEND v5 1/2 dmaengine: dw-edma: Enable Chan Separation via VSEC
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.
---
Changes in v4:
o No change
Changes in v3:
o Reverted the trivial changes / typos.
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 | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/dma/dw-edma/dw-edma-pcie.c +14 -0
@@ -33,6 +33,8 @@#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) +/* 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)
@@ -76,6 +78,7 @@ struct dw_edma_pcie_data {u16 rd_ch_cnt;u64 devmem_phys_off;bool cfg_non_ll; +u32 ch_space_sz; };struct dw_edma_pcie_match_data {
@@ -208,6 +211,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);
@@ -298,6 +308,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));
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH RESEND v5 1/2] dmaengine: dw-edma: Enable Chan Separation via VSEC
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 11 lines
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]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH RESEND v5 2/2 dmaengine: dw-edma: Add changes to support Channel Separation
In reply to Devendra K VermaHDMA supports configurable DMA channel register space separation which ranges from 256B to 32KB. Current implementation supports 256B as default for DMA channel register space. CPM6 supports the selection of channel register space via VSEC. The default value for channel register space for CPM6 is 512B.
Updated the functions and methods to calculate the DMA channel registers base address as per the selected channel separation available as part of 'ch_space_sz'. Removed the unused function __dw_regs() and structs, namely dw_hdma_v0_ch and dw_hdma_v0_regs.
---
Changes in v4:
o No change.
Changes in v3:
o No change.
Changes in v2:
o Replace 'ch_sep_sz' with 'ch_space_sz' as per review recommendations.
o As patch 1/3 of previous series dropped after review, this patch becomes 2/2 of the current series.
Changes in v1:
o Updated the description as per the review comment
o Updated ch_sep_sz for EPC driver.
--- drivers/dma/dw-edma/dw-edma-pcie.c | 4 ++++ drivers/dma/dw-edma/dw-hdma-v0-core.c | 23 ++++++++++++-------- drivers/dma/dw-edma/dw-hdma-v0-debugfs.c | 17 +++++---------- drivers/dma/dw-edma/dw-hdma-v0-regs.h | 10 --------- drivers/pci/controller/dwc/pcie-designware.c | 1 + include/linux/dma/edma.h | 1 + 6 files changed, 26 insertions(+), 30 deletions(-)
drivers/dma/dw-edma/dw-edma-pcie.c +4 -0
@@ -133,6 +133,7 @@ static const struct dw_edma_pcie_data snps_edda_data = { .irqs = 1, .wr_ch_cnt = 2, .rd_ch_cnt = 2, + .ch_space_sz = 256, };static const struct dw_edma_pcie_data xilinx_mdb_data = {
@@ -146,6 +147,7 @@ static const struct dw_edma_pcie_data xilinx_mdb_data = { .irqs = 1, .wr_ch_cnt = 8, .rd_ch_cnt = 8, + .ch_space_sz = 256, };static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = {
@@ -159,6 +161,7 @@ static const struct dw_edma_pcie_data xilinx_cpm6_dma_data = { .irqs = 1, .wr_ch_cnt = 8, .rd_ch_cnt = 8, + .ch_space_sz = 512, }; static void dw_edma_set_chan_region_offset(struct dw_edma_pcie_data *pdata,
@@ -491,6 +494,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, chip->nr_irqs = nr_irqs; chip->ops = match->plat_ops; chip->cfg_non_ll = dma_data->cfg_non_ll; + chip->ch_space_sz = dma_data->ch_space_sz; chip->ll_wr_cnt = dma_data->wr_ch_cnt; chip->ll_rd_cnt = dma_data->rd_ch_cnt;
drivers/dma/dw-edma/dw-hdma-v0-core.c +14 -9
@@ -23,18 +23,23 @@ enum dw_hdma_control { DW_HDMA_V0_LLE = BIT(9), }; -static inline struct dw_hdma_v0_regs __iomem *__dw_regs (struct dw_edma *dw) -{ -return dw->chip->reg_base; -} -static inline struct dw_hdma_v0_ch_regs __iomem *__dw_ch_regs (struct dw_edma *dw,enum dw_edma_dir dir,u16 ch) { -if (dir ==EDMA_DIR_WRITE ) -return &(__dw_regs (dw)->ch[ch].wr); -else -return &(__dw_regs (dw)->ch[ch].rd); +u32 ch_base; + +/* +* For Write, the channel register index starts at +* wr_base(ch_idx) = (2 * ch_idx) * ch_space_sz +* +* For Read channel, +* rd_base(ch_idx) = (2 * ch_idx + 1) * ch_space_sz +*/ + ch_base =2 * ch; +if (dir ==EDMA_DIR_READ ) + ch_base +=1 ; + +return dw->chip->reg_base + (ch_base * dw->chip->ch_space_sz); }#define SET_CH_32 (dw, dir, ch, name, value) \
drivers/dma/dw-edma/dw-hdma-v0-debugfs.c +6 -11
@@ -13,22 +13,17 @@#include "dw-hdma-v0-regs.h" #include "dw-edma-core.h" -#define REGS_ADDR (dw, name) \ - ({ \ - struct dw_hdma_v0_regs __iomem *__regs = (dw)->chip->reg_base; \ - \ - (void __iomem *)&__regs->name; \ - }) -#define REGS_CH_ADDR (dw, name, _dir, _ch) \ ({ \ - struct dw_hdma_v0_ch_regs __iomem *__ch_regs; \ + struct dw_hdma_v0_ch_regs __iomem *__ch_regs; \ + off_t __off = (dw)->chip->ch_space_sz; \ \ - if (_dir == EDMA_DIR_READ) \ - __ch_regs = REGS_ADDR(dw, ch[_ch].rd); \ + if ((_dir) == EDMA_DIR_READ) \ + __off *= (2 * (_ch) + 1); \ else \ - __ch_regs = REGS_ADDR(dw, ch[_ch].wr); \ + __off *= (2 * (_ch)); \ \ + __ch_regs = ((dw)->chip->reg_base + __off); \ (void __iomem *)&__ch_regs->name; \ })
drivers/dma/dw-edma/dw-hdma-v0-regs.h +0 -10
@@ -85,16 +85,6 @@ struct dw_hdma_v0_ch_regs { }; } msi_abort;u32 msi_msgdata;/* 0x00a8 */ -u32 padding_2[21 ];/* 0x00ac..0x00fc */ -} __packed; - -struct dw_hdma_v0_ch { -struct dw_hdma_v0_ch_regs wr;/* 0x0000 */ -struct dw_hdma_v0_ch_regs rd;/* 0x0100 */ -} __packed; - -struct dw_hdma_v0_regs { -struct dw_hdma_v0_ch ch[HDMA_V0_MAX_NR_CH ];/* 0x0000..0x0fa8 */ } __packed;struct dw_hdma_v0_lli {
drivers/pci/controller/dwc/pcie-designware.c +1 -0
@@ -1039,6 +1039,7 @@ static void dw_pcie_edma_init_data(struct dw_pcie *pci) pci->edma.ops = &dw_pcie_edma_ops; pci->edma.flags |=DW_EDMA_CHIP_LOCAL ; + pci->edma.ch_space_sz =256 ; }static int dw_pcie_edma_find_mf (struct dw_pcie *pci)
include/linux/dma/edma.h +1 -0
@@ -147,6 +147,7 @@ struct dw_edma_chip {struct dw_edma *dw;bool cfg_non_ll; +u32 ch_space_sz; };/* Export to the platform drivers */
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH RESEND v5 2/2] dmaengine: dw-edma: Add changes to support Channel Separation
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] IRQ vectors allocated by pci_alloc_irq_vectors() are leaked on multiple error paths and during device removal. --
commit ba0a3a6c27ea5625a5bca375aac32fcdc517816e
Author: Devendra K Verma <[email protected]>
dmaengine: dw-edma: Add changes to support Channel Separation
This commit updates the DMA channel register base address calculation to support a configurable channel register space separation. It modifies the probe functions to pass the channel space size to the chip structure.
Quoted 4 lines
drivers/dma/dw-edma/dw-edma-pcie.c +0
-0
[ ... ]
drivers/dma/dw-edma/dw-edma-pcie.c static int dw_edma_pcie_probe(struct pci_dev *pdev, · line 500
Quoted 8 lines
drivers/dma/dw-edma/dw-edma-pcie.c +1
-0@@ -491,6 +494,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, chip->nr_irqs = nr_irqs; chip->ops = match->plat_ops; chip->cfg_non_ll = dma_data->cfg_non_ll; + chip->ch_space_sz = dma_data->ch_space_sz; chip->ll_wr_cnt = dma_data->wr_ch_cnt; chip->ll_rd_cnt = dma_data->rd_ch_cnt;
[Severity: High]
This is a pre-existing issue, but I noticed a potential resource leak in dw_edma_pcie_probe() regarding IRQ vector cleanup.
Does dw_edma_pcie_probe() leak IRQ vectors on error paths and during device removal?
Earlier in dw_edma_pcie_probe(), vectors are allocated using pci_alloc_irq_vectors(). However, multiple error paths after this point return directly without calling pci_free_irq_vectors(). For example, if pcim_iomap_table() fails:
chip->reg_base = pcim_iomap_table(pdev)[dma_data->rg.bar];
if (!chip->reg_base)
return -ENOMEM;Similar leaks appear to happen if allocating the logical link blocks fails, or if dw_edma_probe() fails near the end of the function.
Furthermore, dw_edma_pcie_remove() does not appear to call pci_free_irq_vectors(), which means these IRQ vectors are leaked when the device is unbound or the module is unloaded.
According to PCI subsystem guidelines, every error path after a successful allocation must clean up the vectors to prevent IRQ resource exhaustion. Could we add proper error labels to clean up these vectors?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Quoted 10 lines · Devendra K Verma
'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.
In order to use the VSEC functionality following changes have been made:[...]
Applied, thanks!
[1/2] dmaengine: dw-edma: Enable Chan Separation via VSEC
commit: 62fb4a3a59223abb4d7baa3e088169767a28d551
[2/2] dmaengine: dw-edma: Add changes to support Channel Separation
commit: ff4dc66e80d2c31138a9ca31f1a4340dc4919d50Best regards,
Signature · 1 line
~Vinod