Skip to contentSkip to conversation

dmaengine: dw-axi-dmac: Support src_maxburst and dst_maxburst

PATCH Superseded Tan En De 2 participants 3 messages Sep 13, 2023 – Nov 6, 2023

Subsystems (2)
dmaengine Synopsys DesignWare AXI DMAC driver

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Tan En De

author
#0 lore raw

PATCH v2 1/1 dmaengine: dw-axi-dmac: Support src_maxburst and dst_maxburst

Current implementation has hardcoded CHx_CTL.SRC_MSIZE and CHx_CTL.DST_MSIZE with a constant, namely DWAXIDMAC_BURST_TRANS_LEN_4.

However, to support hardware with different source/destination burst transaction length, the aforementioned values shall be configurable based on dma_slave_config set by client driver.

So, this commit is to allow client driver to configure
- CHx_CTL.SRC_MSIZE via dma_slave_config.src_maxburst
- CHx_CTL.DST_MSIZE via dma_slave_config.dst_maxburst

Signed-off-by Tan En De <[email protected]>

---
v1 -> v2:
- Fixed typo (replaced return -EINVAL with goto err_desc_get in dma_chan_prep_dma_memcpy()).

---
 .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    | 38 +++++++++++++++----
 drivers/dma/dw-axi-dmac/dw-axi-dmac.h         |  3 +-
 2 files changed, 33 insertions(+), 8 deletions(-)
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +31 -7
index dd02f84e404d..f234097dffb9 100644
@@ -610,7 +610,7 @@ static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 	size_t axi_block_ts;
 	size_t block_ts;
 	u32 ctllo, ctlhi;
-	u32 burst_len;
+	u32 burst_len, src_burst_trans_len, dst_burst_trans_len;
 
 	axi_block_ts = chan->chip->dw->hdata->block_size[chan->id];
 

@@ -674,8 +674,20 @@ static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 
 	hw_desc->lli->block_ts_lo = cpu_to_le32(block_ts - 1);
 
-	ctllo |= DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_DST_MSIZE_POS |
-		 DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_SRC_MSIZE_POS;
+	dst_burst_trans_len = chan->config.dst_maxburst ?
+				__ffs(chan->config.dst_maxburst) - 1 :
+				DWAXIDMAC_BURST_TRANS_LEN_4;
+	if (dst_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+		return -EINVAL;
+	ctllo |= dst_burst_trans_len << CH_CTL_L_DST_MSIZE_POS;
+
+	src_burst_trans_len = chan->config.src_maxburst ?
+				__ffs(chan->config.src_maxburst) - 1 :
+				DWAXIDMAC_BURST_TRANS_LEN_4;
+	if (src_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+		return -EINVAL;
+	ctllo |= src_burst_trans_len << CH_CTL_L_SRC_MSIZE_POS;
+
 	hw_desc->lli->ctl_lo = cpu_to_le32(ctllo);
 
 	set_desc_src_master(hw_desc);

@@ -878,7 +890,7 @@ dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst_adr,
 	size_t block_ts, max_block_ts, xfer_len;
 	struct axi_dma_hw_desc *hw_desc = NULL;
 	struct axi_dma_desc *desc = NULL;
-	u32 xfer_width, reg, num;
+	u32 xfer_width, reg, num, src_burst_trans_len, dst_burst_trans_len;
 	u64 llp = 0;
 	u8 lms = 0; /* Select AXI0 master for LLI fetching */
 

@@ -936,9 +948,21 @@ dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst_adr,
 		}
 		hw_desc->lli->ctl_hi = cpu_to_le32(reg);
 
-		reg = (DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_DST_MSIZE_POS |
-		       DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_SRC_MSIZE_POS |
-		       xfer_width << CH_CTL_L_DST_WIDTH_POS |
+		dst_burst_trans_len = chan->config.dst_maxburst ?
+					__ffs(chan->config.dst_maxburst) - 1 :
+					DWAXIDMAC_BURST_TRANS_LEN_4;
+		if (dst_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+			goto err_desc_get;
+		reg |= dst_burst_trans_len << CH_CTL_L_DST_MSIZE_POS;
+
+		src_burst_trans_len = chan->config.src_maxburst ?
+					__ffs(chan->config.src_maxburst) - 1 :
+					DWAXIDMAC_BURST_TRANS_LEN_4;
+		if (src_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+			goto err_desc_get;
+		reg |= src_burst_trans_len << CH_CTL_L_SRC_MSIZE_POS;
+
+		reg = (xfer_width << CH_CTL_L_DST_WIDTH_POS |
 		       xfer_width << CH_CTL_L_SRC_WIDTH_POS |
 		       DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_DST_INC_POS |
 		       DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_SRC_INC_POS);
drivers/dma/dw-axi-dmac/dw-axi-dmac.h +2 -1
index eb267cb24f67..877bff395740 100644
@@ -265,7 +265,8 @@ enum {
 	DWAXIDMAC_BURST_TRANS_LEN_128,
 	DWAXIDMAC_BURST_TRANS_LEN_256,
 	DWAXIDMAC_BURST_TRANS_LEN_512,
-	DWAXIDMAC_BURST_TRANS_LEN_1024
+	DWAXIDMAC_BURST_TRANS_LEN_1024,
+	DWAXIDMAC_BURST_TRANS_LEN_MAX  = DWAXIDMAC_BURST_TRANS_LEN_1024
 };
 
 #define CH_CTL_L_DST_WIDTH_POS		11

base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec

Signature · 1 line
2.34.1
maintainer
#1 lore raw
Quoted 6 lines · Tan En De

Current implementation has hardcoded CHx_CTL.SRC_MSIZE and CHx_CTL.DST_MSIZE with a constant, namely DWAXIDMAC_BURST_TRANS_LEN_4.

However, to support hardware with different source/destination burst transaction length, the aforementioned values shall be configurable based on dma_slave_config set by client driver.

Are client drivers setting this today, will there be regression if not set?

Quoted 69 lines

So, this commit is to allow client driver to configure
- CHx_CTL.SRC_MSIZE via dma_slave_config.src_maxburst
- CHx_CTL.DST_MSIZE via dma_slave_config.dst_maxburst

Signed-off-by Tan En De <[email protected]>

---
v1 -> v2:
- Fixed typo (replaced return -EINVAL with goto err_desc_get in dma_chan_prep_dma_memcpy()).

---
 .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    | 38 +++++++++++++++----
 drivers/dma/dw-axi-dmac/dw-axi-dmac.h         |  3 +-
 2 files changed, 33 insertions(+), 8 deletions(-)
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +19 -7
index dd02f84e404d..f234097dffb9 100644
@@ -610,7 +610,7 @@ static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 	size_t axi_block_ts;
 	size_t block_ts;
 	u32 ctllo, ctlhi;
-	u32 burst_len;
+	u32 burst_len, src_burst_trans_len, dst_burst_trans_len;
 
 	axi_block_ts = chan->chip->dw->hdata->block_size[chan->id];
 

@@ -674,8 +674,20 @@ static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
 
 	hw_desc->lli->block_ts_lo = cpu_to_le32(block_ts - 1);
 
-	ctllo |= DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_DST_MSIZE_POS |
-		 DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_SRC_MSIZE_POS;
+	dst_burst_trans_len = chan->config.dst_maxburst ?
+				__ffs(chan->config.dst_maxburst) - 1 :
+				DWAXIDMAC_BURST_TRANS_LEN_4;
+	if (dst_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+		return -EINVAL;
+	ctllo |= dst_burst_trans_len << CH_CTL_L_DST_MSIZE_POS;
+
+	src_burst_trans_len = chan->config.src_maxburst ?
+				__ffs(chan->config.src_maxburst) - 1 :
+				DWAXIDMAC_BURST_TRANS_LEN_4;
+	if (src_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+		return -EINVAL;
+	ctllo |= src_burst_trans_len << CH_CTL_L_SRC_MSIZE_POS;
+
 	hw_desc->lli->ctl_lo = cpu_to_le32(ctllo);
 
 	set_desc_src_master(hw_desc);

@@ -878,7 +890,7 @@ dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst_adr,
 	size_t block_ts, max_block_ts, xfer_len;
 	struct axi_dma_hw_desc *hw_desc = NULL;
 	struct axi_dma_desc *desc = NULL;
-	u32 xfer_width, reg, num;
+	u32 xfer_width, reg, num, src_burst_trans_len, dst_burst_trans_len;
 	u64 llp = 0;
 	u8 lms = 0; /* Select AXI0 master for LLI fetching */
 

@@ -936,9 +948,21 @@ dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst_adr,
 		}
 		hw_desc->lli->ctl_hi = cpu_to_le32(reg);
 
-		reg = (DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_DST_MSIZE_POS |
-		       DWAXIDMAC_BURST_TRANS_LEN_4 << CH_CTL_L_SRC_MSIZE_POS |
-		       xfer_width << CH_CTL_L_DST_WIDTH_POS |
+		dst_burst_trans_len = chan->config.dst_maxburst ?
+					__ffs(chan->config.dst_maxburst) - 1 :
+					DWAXIDMAC_BURST_TRANS_LEN_4;

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst_adr, · line 963

Quoted 10 lines
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +10 -0
+		if (dst_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+			goto err_desc_get;
+		reg |= dst_burst_trans_len << CH_CTL_L_DST_MSIZE_POS;
+
+		src_burst_trans_len = chan->config.src_maxburst ?
+					__ffs(chan->config.src_maxburst) - 1 :
+					DWAXIDMAC_BURST_TRANS_LEN_4;
+		if (src_burst_trans_len > DWAXIDMAC_BURST_TRANS_LEN_MAX)
+			goto err_desc_get;
+		reg |= src_burst_trans_len << CH_CTL_L_SRC_MSIZE_POS;

this is wrong, mempcy should never use slave config value. These values are for peripheral and not meant for mem-mem transfers

You should try max throughput by aligning the txn to bus width available on this system

Quoted 23 lines
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +2 -0
+
+		reg = (xfer_width << CH_CTL_L_DST_WIDTH_POS |
 		       xfer_width << CH_CTL_L_SRC_WIDTH_POS |
 		       DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_DST_INC_POS |
 		       DWAXIDMAC_CH_CTL_L_INC << CH_CTL_L_SRC_INC_POS);
drivers/dma/dw-axi-dmac/dw-axi-dmac.h +2 -1
index eb267cb24f67..877bff395740 100644
@@ -265,7 +265,8 @@ enum {
 	DWAXIDMAC_BURST_TRANS_LEN_128,
 	DWAXIDMAC_BURST_TRANS_LEN_256,
 	DWAXIDMAC_BURST_TRANS_LEN_512,
-	DWAXIDMAC_BURST_TRANS_LEN_1024
+	DWAXIDMAC_BURST_TRANS_LEN_1024,
+	DWAXIDMAC_BURST_TRANS_LEN_MAX  = DWAXIDMAC_BURST_TRANS_LEN_1024
 };
 
 #define CH_CTL_L_DST_WIDTH_POS		11

base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec

Signature · 1 line
2.34.1
Signature · 1 line
~Vinod
author
#2 lore raw
Quoted 1 line

Are client drivers setting this today, will there be regression if not set?

I don't see any dw-axi-dmac client driver setting this src_maxburt/dst_maxburst explicitly.
This means that all the while, those client drivers have been using the default value of DWAXIDMAC_BURST_TRANS_LEN_4. In the patch, to maintain this backward compatibility, ternary operator is used, for example:

Quoted 3 lines

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan, · line 679

Quoted 3 lines
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +3 -0
+	dst_burst_trans_len = chan->config.dst_maxburst ?
+				__ffs(chan->config.dst_maxburst) - 1 :
+				DWAXIDMAC_BURST_TRANS_LEN_4;

so that if src_maxburst/dst_maxburst is not set, default value of DWAXIDMAC_BURST_TRANS_LEN_4 applies.

Quoted 2 lines

this is wrong, memcpy should never use slave config value. These values are for peripheral and not meant for mem-mem transfers

Agreed, thanks for pointing that out. It shall be removed in patch v3.