LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 1/2] dmaengine: sprd: Optimize the sprd_dma_prep_dma_memcpy()
@ 2018-05-11 13:06 Baolin Wang
  2018-05-11 13:06 ` [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration Baolin Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Baolin Wang @ 2018-05-11 13:06 UTC (permalink / raw)
  To: dan.j.williams, vkoul
  Cc: eric.long, broonie, baolin.wang, dmaengine, linux-kernel

From: Eric Long <eric.long@spreadtrum.com>

This is one preparation patch, we can use default DMA configuration to
implement the device_prep_dma_memcpy() interface instead of issuing
sprd_dma_config().

We will implement one new sprd_dma_config() function with introducing
device_prep_slave_sg() interface in following patch. So we can remove
the obsolete sprd_dma_config() firstly.

Signed-off-by: Eric Long <eric.long@spreadtrum.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v2:
 - Change logic to make code more readable.

Changes since v1:
 - No updates.
---
 drivers/dma/sprd-dma.c |  167 +++++++++++-------------------------------------
 1 file changed, 39 insertions(+), 128 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index e715d07..924ada4 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -552,147 +552,58 @@ static void sprd_dma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&schan->vc.lock, flags);
 }
 
-static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
-			   dma_addr_t dest, dma_addr_t src, size_t len)
-{
-	struct sprd_dma_dev *sdev = to_sprd_dma_dev(chan);
-	struct sprd_dma_chn_hw *hw = &sdesc->chn_hw;
-	u32 datawidth, src_step, des_step, fragment_len;
-	u32 block_len, req_mode, irq_mode, transcation_len;
-	u32 fix_mode = 0, fix_en = 0;
-
-	if (IS_ALIGNED(len, 4)) {
-		datawidth = SPRD_DMA_DATAWIDTH_4_BYTES;
-		src_step = SPRD_DMA_WORD_STEP;
-		des_step = SPRD_DMA_WORD_STEP;
-	} else if (IS_ALIGNED(len, 2)) {
-		datawidth = SPRD_DMA_DATAWIDTH_2_BYTES;
-		src_step = SPRD_DMA_SHORT_STEP;
-		des_step = SPRD_DMA_SHORT_STEP;
-	} else {
-		datawidth = SPRD_DMA_DATAWIDTH_1_BYTE;
-		src_step = SPRD_DMA_BYTE_STEP;
-		des_step = SPRD_DMA_BYTE_STEP;
-	}
-
-	fragment_len = SPRD_DMA_MEMCPY_MIN_SIZE;
-	if (len <= SPRD_DMA_BLK_LEN_MASK) {
-		block_len = len;
-		transcation_len = 0;
-		req_mode = SPRD_DMA_BLK_REQ;
-		irq_mode = SPRD_DMA_BLK_INT;
-	} else {
-		block_len = SPRD_DMA_MEMCPY_MIN_SIZE;
-		transcation_len = len;
-		req_mode = SPRD_DMA_TRANS_REQ;
-		irq_mode = SPRD_DMA_TRANS_INT;
-	}
-
-	hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
-	hw->wrap_ptr = (u32)((src >> SPRD_DMA_HIGH_ADDR_OFFSET) &
-			     SPRD_DMA_HIGH_ADDR_MASK);
-	hw->wrap_to = (u32)((dest >> SPRD_DMA_HIGH_ADDR_OFFSET) &
-			    SPRD_DMA_HIGH_ADDR_MASK);
-
-	hw->src_addr = (u32)(src & SPRD_DMA_LOW_ADDR_MASK);
-	hw->des_addr = (u32)(dest & SPRD_DMA_LOW_ADDR_MASK);
-
-	if ((src_step != 0 && des_step != 0) || (src_step | des_step) == 0) {
-		fix_en = 0;
-	} else {
-		fix_en = 1;
-		if (src_step)
-			fix_mode = 1;
-		else
-			fix_mode = 0;
-	}
-
-	hw->frg_len = datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET |
-		datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET |
-		req_mode << SPRD_DMA_REQ_MODE_OFFSET |
-		fix_mode << SPRD_DMA_FIX_SEL_OFFSET |
-		fix_en << SPRD_DMA_FIX_EN_OFFSET |
-		(fragment_len & SPRD_DMA_FRG_LEN_MASK);
-	hw->blk_len = block_len & SPRD_DMA_BLK_LEN_MASK;
-
-	hw->intc = SPRD_DMA_CFG_ERR_INT_EN;
-
-	switch (irq_mode) {
-	case SPRD_DMA_NO_INT:
-		break;
-
-	case SPRD_DMA_FRAG_INT:
-		hw->intc |= SPRD_DMA_FRAG_INT_EN;
-		break;
-
-	case SPRD_DMA_BLK_INT:
-		hw->intc |= SPRD_DMA_BLK_INT_EN;
-		break;
-
-	case SPRD_DMA_BLK_FRAG_INT:
-		hw->intc |= SPRD_DMA_BLK_INT_EN | SPRD_DMA_FRAG_INT_EN;
-		break;
-
-	case SPRD_DMA_TRANS_INT:
-		hw->intc |= SPRD_DMA_TRANS_INT_EN;
-		break;
-
-	case SPRD_DMA_TRANS_FRAG_INT:
-		hw->intc |= SPRD_DMA_TRANS_INT_EN | SPRD_DMA_FRAG_INT_EN;
-		break;
-
-	case SPRD_DMA_TRANS_BLK_INT:
-		hw->intc |= SPRD_DMA_TRANS_INT_EN | SPRD_DMA_BLK_INT_EN;
-		break;
-
-	case SPRD_DMA_LIST_INT:
-		hw->intc |= SPRD_DMA_LIST_INT_EN;
-		break;
-
-	case SPRD_DMA_CFGERR_INT:
-		hw->intc |= SPRD_DMA_CFG_ERR_INT_EN;
-		break;
-
-	default:
-		dev_err(sdev->dma_dev.dev, "invalid irq mode\n");
-		return -EINVAL;
-	}
-
-	if (transcation_len == 0)
-		hw->trsc_len = block_len & SPRD_DMA_TRSC_LEN_MASK;
-	else
-		hw->trsc_len = transcation_len & SPRD_DMA_TRSC_LEN_MASK;
-
-	hw->trsf_step = (des_step & SPRD_DMA_TRSF_STEP_MASK) <<
-			SPRD_DMA_DEST_TRSF_STEP_OFFSET |
-			(src_step & SPRD_DMA_TRSF_STEP_MASK) <<
-			SPRD_DMA_SRC_TRSF_STEP_OFFSET;
-
-	hw->frg_step = 0;
-	hw->src_blk_step = 0;
-	hw->des_blk_step = 0;
-	hw->src_blk_step = 0;
-	return 0;
-}
-
 static struct dma_async_tx_descriptor *
 sprd_dma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 			 size_t len, unsigned long flags)
 {
 	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
 	struct sprd_dma_desc *sdesc;
-	int ret;
+	struct sprd_dma_chn_hw *hw;
+	enum sprd_dma_datawidth datawidth;
+	u32 step, temp;
 
 	sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
 	if (!sdesc)
 		return NULL;
 
-	ret = sprd_dma_config(chan, sdesc, dest, src, len);
-	if (ret) {
-		kfree(sdesc);
-		return NULL;
+	hw = &sdesc->chn_hw;
+
+	hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
+	hw->intc = SPRD_DMA_TRANS_INT | SPRD_DMA_CFG_ERR_INT_EN;
+	hw->src_addr = src & SPRD_DMA_LOW_ADDR_MASK;
+	hw->des_addr = dest & SPRD_DMA_LOW_ADDR_MASK;
+	hw->wrap_ptr = (src >> SPRD_DMA_HIGH_ADDR_OFFSET) &
+		SPRD_DMA_HIGH_ADDR_MASK;
+	hw->wrap_to = (dest >> SPRD_DMA_HIGH_ADDR_OFFSET) &
+		SPRD_DMA_HIGH_ADDR_MASK;
+
+	if (IS_ALIGNED(len, 8)) {
+		datawidth = SPRD_DMA_DATAWIDTH_8_BYTES;
+		step = SPRD_DMA_DWORD_STEP;
+	} else if (IS_ALIGNED(len, 4)) {
+		datawidth = SPRD_DMA_DATAWIDTH_4_BYTES;
+		step = SPRD_DMA_WORD_STEP;
+	} else if (IS_ALIGNED(len, 2)) {
+		datawidth = SPRD_DMA_DATAWIDTH_2_BYTES;
+		step = SPRD_DMA_SHORT_STEP;
+	} else {
+		datawidth = SPRD_DMA_DATAWIDTH_1_BYTE;
+		step = SPRD_DMA_BYTE_STEP;
 	}
 
+	temp = datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET;
+	temp |= datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET;
+	temp |= SPRD_DMA_TRANS_REQ << SPRD_DMA_REQ_MODE_OFFSET;
+	temp |= len & SPRD_DMA_FRG_LEN_MASK;
+	hw->frg_len = temp;
+
+	hw->blk_len = len & SPRD_DMA_BLK_LEN_MASK;
+	hw->trsc_len = len & SPRD_DMA_TRSC_LEN_MASK;
+
+	temp = (step & SPRD_DMA_TRSF_STEP_MASK) << SPRD_DMA_DEST_TRSF_STEP_OFFSET;
+	temp |= (step & SPRD_DMA_TRSF_STEP_MASK) << SPRD_DMA_SRC_TRSF_STEP_OFFSET;
+	hw->trsf_step = temp;
+
 	return vchan_tx_prep(&schan->vc, &sdesc->vd, flags);
 }
 
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration
  2018-05-11 13:06 [PATCH v3 1/2] dmaengine: sprd: Optimize the sprd_dma_prep_dma_memcpy() Baolin Wang
@ 2018-05-11 13:06 ` Baolin Wang
  2018-05-17  5:14   ` Vinod
  0 siblings, 1 reply; 6+ messages in thread
From: Baolin Wang @ 2018-05-11 13:06 UTC (permalink / raw)
  To: dan.j.williams, vkoul
  Cc: eric.long, broonie, baolin.wang, dmaengine, linux-kernel

From: Eric Long <eric.long@spreadtrum.com>

This patch adds the 'device_config' and 'device_prep_slave_sg' interfaces
for users to configure DMA, as well as adding one 'struct sprd_dma_config'
structure to save Spreadtrum DMA configuration for each DMA channel.

Signed-off-by: Eric Long <eric.long@spreadtrum.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v2:
 - Remove src/dst from struct sprd_dma_config.
 - Simplify sprd_dma_get_datawidth()/sprd_dma_get_step().
 - Change some logic to make code more readable.
 - Other optimization.

Changes since v1:
 - Fix the incorrect parameter type of sprd_dma_get_step().
---
 drivers/dma/sprd-dma.c       |  213 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma/sprd-dma.h |    4 +
 2 files changed, 217 insertions(+)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 924ada4..00fcec4 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -100,6 +100,8 @@
 #define SPRD_DMA_DES_DATAWIDTH_OFFSET	28
 #define SPRD_DMA_SWT_MODE_OFFSET	26
 #define SPRD_DMA_REQ_MODE_OFFSET	24
+#define SPRD_DMA_WRAP_SEL_OFFSET	23
+#define SPRD_DMA_WRAP_EN_OFFSET		22
 #define SPRD_DMA_REQ_MODE_MASK		GENMASK(1, 0)
 #define SPRD_DMA_FIX_SEL_OFFSET		21
 #define SPRD_DMA_FIX_EN_OFFSET		20
@@ -154,6 +156,31 @@ struct sprd_dma_chn_hw {
 	u32 des_blk_step;
 };
 
+/*
+ * struct sprd_dma_config - DMA configuration structure
+ * @cfg: dma slave channel runtime config
+ * @block_len: specify one block transfer length
+ * @transcation_len: specify one transcation transfer length
+ * @src_step: source transfer step
+ * @dst_step: destination transfer step
+ * @wrap_ptr: wrap pointer address, once the transfer address reaches the
+ * 'wrap_ptr', the next transfer address will jump to the 'wrap_to' address.
+ * @wrap_to: wrap jump to address
+ * @req_mode: specify the DMA request mode
+ * @int_mode: specify the DMA interrupt type
+ */
+struct sprd_dma_config {
+	struct dma_slave_config cfg;
+	u32 block_len;
+	u32 transcation_len;
+	u32 src_step;
+	u32 dst_step;
+	phys_addr_t wrap_ptr;
+	phys_addr_t wrap_to;
+	enum sprd_dma_req_mode req_mode;
+	enum sprd_dma_int_type int_mode;
+};
+
 /* dma request description */
 struct sprd_dma_desc {
 	struct virt_dma_desc	vd;
@@ -164,6 +191,7 @@ struct sprd_dma_desc {
 struct sprd_dma_chn {
 	struct virt_dma_chan	vc;
 	void __iomem		*chn_base;
+	struct sprd_dma_config	slave_cfg;
 	u32			chn_num;
 	u32			dev_id;
 	struct sprd_dma_desc	*cur_desc;
@@ -552,6 +580,113 @@ static void sprd_dma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&schan->vc.lock, flags);
 }
 
+static enum sprd_dma_datawidth
+sprd_dma_get_datawidth(enum dma_slave_buswidth buswidth)
+{
+	switch (buswidth) {
+	case DMA_SLAVE_BUSWIDTH_1_BYTE:
+	case DMA_SLAVE_BUSWIDTH_2_BYTES:
+	case DMA_SLAVE_BUSWIDTH_4_BYTES:
+	case DMA_SLAVE_BUSWIDTH_8_BYTES:
+		return ffs(buswidth) - 1;
+	default:
+		return SPRD_DMA_DATAWIDTH_4_BYTES;
+	}
+}
+
+static u32 sprd_dma_get_step(enum dma_slave_buswidth buswidth)
+{
+	switch (buswidth) {
+	case DMA_SLAVE_BUSWIDTH_1_BYTE:
+	case DMA_SLAVE_BUSWIDTH_2_BYTES:
+	case DMA_SLAVE_BUSWIDTH_4_BYTES:
+	case DMA_SLAVE_BUSWIDTH_8_BYTES:
+		return buswidth;
+
+	default:
+		return SPRD_DMA_DWORD_STEP;
+	}
+}
+
+static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
+			   dma_addr_t src, dma_addr_t dst,
+			   struct sprd_dma_config *slave_cfg)
+{
+	struct sprd_dma_dev *sdev = to_sprd_dma_dev(chan);
+	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
+	struct sprd_dma_chn_hw *hw = &sdesc->chn_hw;
+	u32 fix_mode = 0, fix_en = 0, wrap_en = 0, wrap_mode = 0;
+	u32 src_datawidth, dst_datawidth, temp;
+
+	if (slave_cfg->cfg.slave_id)
+		schan->dev_id = slave_cfg->cfg.slave_id;
+
+	hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
+
+	temp = slave_cfg->wrap_ptr & SPRD_DMA_LOW_ADDR_MASK;
+	temp |= (src >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
+	hw->wrap_ptr = temp;
+
+	temp = slave_cfg->wrap_to & SPRD_DMA_LOW_ADDR_MASK;
+	temp |= (dst >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
+	hw->wrap_to = temp;
+
+	hw->src_addr = src & SPRD_DMA_LOW_ADDR_MASK;
+	hw->des_addr = dst & SPRD_DMA_LOW_ADDR_MASK;
+
+	if ((slave_cfg->src_step != 0 && slave_cfg->dst_step != 0)
+	    || (slave_cfg->src_step | slave_cfg->dst_step) == 0) {
+		fix_en = 0;
+	} else {
+		fix_en = 1;
+		if (slave_cfg->src_step)
+			fix_mode = 1;
+		else
+			fix_mode = 0;
+	}
+
+	if (slave_cfg->wrap_ptr && slave_cfg->wrap_to) {
+		wrap_en = 1;
+		if (slave_cfg->wrap_to == src) {
+			wrap_mode = 0;
+		} else if (slave_cfg->wrap_to == dst) {
+			wrap_mode = 1;
+		} else {
+			dev_err(sdev->dma_dev.dev, "invalid wrap mode\n");
+			return -EINVAL;
+		}
+	}
+
+	hw->intc = slave_cfg->int_mode | SPRD_DMA_CFG_ERR_INT_EN;
+
+	src_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.src_addr_width);
+	dst_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.dst_addr_width);
+
+	temp = src_datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET;
+	temp |= dst_datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET;
+	temp |= slave_cfg->req_mode << SPRD_DMA_REQ_MODE_OFFSET;
+	temp |= wrap_mode << SPRD_DMA_WRAP_SEL_OFFSET;
+	temp |= wrap_en << SPRD_DMA_WRAP_EN_OFFSET;
+	temp |= fix_mode << SPRD_DMA_FIX_SEL_OFFSET;
+	temp |= fix_en << SPRD_DMA_FIX_EN_OFFSET;
+	temp |= slave_cfg->cfg.src_maxburst & SPRD_DMA_FRG_LEN_MASK;
+	hw->frg_len = temp;
+
+	hw->blk_len = slave_cfg->block_len & SPRD_DMA_BLK_LEN_MASK;
+	hw->trsc_len = slave_cfg->transcation_len & SPRD_DMA_TRSC_LEN_MASK;
+
+	temp = (slave_cfg->dst_step & SPRD_DMA_TRSF_STEP_MASK) <<
+		SPRD_DMA_DEST_TRSF_STEP_OFFSET;
+	temp |= (slave_cfg->src_step & SPRD_DMA_TRSF_STEP_MASK) <<
+		SPRD_DMA_SRC_TRSF_STEP_OFFSET;
+	hw->trsf_step = temp;
+
+	hw->frg_step = 0;
+	hw->src_blk_step = 0;
+	hw->des_blk_step = 0;
+	return 0;
+}
+
 static struct dma_async_tx_descriptor *
 sprd_dma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 			 size_t len, unsigned long flags)
@@ -607,6 +742,82 @@ static void sprd_dma_issue_pending(struct dma_chan *chan)
 	return vchan_tx_prep(&schan->vc, &sdesc->vd, flags);
 }
 
+static void sprd_dma_fill_config(struct sprd_dma_config *slave_cfg,
+				 u32 src_step, u32 dst_step, u32 len,
+				 unsigned long flags)
+{
+	slave_cfg->src_step = src_step;
+	slave_cfg->dst_step = dst_step;
+	slave_cfg->block_len = len;
+	slave_cfg->transcation_len = len;
+	slave_cfg->req_mode =
+		(flags >> SPRD_DMA_REQ_SHIFT) & SPRD_DMA_REQ_MODE_MASK;
+	slave_cfg->int_mode = flags & SPRD_DMA_INT_MASK;
+}
+
+static struct dma_async_tx_descriptor *
+sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+		       unsigned int sglen, enum dma_transfer_direction dir,
+		       unsigned long flags, void *context)
+{
+	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
+	struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
+	u32 src_step = 0, dst_step = 0, len = 0;
+	dma_addr_t src = 0, dst = 0;
+	struct sprd_dma_desc *sdesc;
+	struct scatterlist *sg;
+	int ret, i;
+
+	/* TODO: now we only support one sg for each DMA configuration. */
+	if (!is_slave_direction(dir) || sglen > 1)
+		return NULL;
+
+	sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
+	if (!sdesc)
+		return NULL;
+
+	for_each_sg(sgl, sg, sglen, i) {
+		len = sg_dma_len(sg);
+
+		if (dir == DMA_MEM_TO_DEV) {
+			src = sg_dma_address(sg);
+			dst = slave_cfg->cfg.dst_addr;
+			src_step = sprd_dma_get_step(slave_cfg->cfg.src_addr_width);
+			dst_step = SPRD_DMA_NONE_STEP;
+		} else {
+			src = slave_cfg->cfg.src_addr;
+			dst = sg_dma_address(sg);
+			src_step = SPRD_DMA_NONE_STEP;
+			dst_step = sprd_dma_get_step(slave_cfg->cfg.dst_addr_width);
+		}
+	}
+
+	sprd_dma_fill_config(slave_cfg, src_step, dst_step, len, flags);
+
+	ret = sprd_dma_config(chan, sdesc, src, dst, slave_cfg);
+	if (ret) {
+		kfree(sdesc);
+		return NULL;
+	}
+
+	return vchan_tx_prep(&schan->vc, &sdesc->vd, flags);
+}
+
+static int sprd_dma_slave_config(struct dma_chan *chan,
+				 struct dma_slave_config *config)
+{
+	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
+	struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
+
+	if (!is_slave_direction(config->direction))
+		return -EINVAL;
+
+	memset(slave_cfg, 0, sizeof(*slave_cfg));
+	memcpy(&slave_cfg->cfg, config, sizeof(*config));
+
+	return 0;
+}
+
 static int sprd_dma_pause(struct dma_chan *chan)
 {
 	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
@@ -733,6 +944,8 @@ static int sprd_dma_probe(struct platform_device *pdev)
 	sdev->dma_dev.device_tx_status = sprd_dma_tx_status;
 	sdev->dma_dev.device_issue_pending = sprd_dma_issue_pending;
 	sdev->dma_dev.device_prep_dma_memcpy = sprd_dma_prep_dma_memcpy;
+	sdev->dma_dev.device_prep_slave_sg = sprd_dma_prep_slave_sg;
+	sdev->dma_dev.device_config = sprd_dma_slave_config;
 	sdev->dma_dev.device_pause = sprd_dma_pause;
 	sdev->dma_dev.device_resume = sprd_dma_resume;
 	sdev->dma_dev.device_terminate_all = sprd_dma_terminate_all;
diff --git a/include/linux/dma/sprd-dma.h b/include/linux/dma/sprd-dma.h
index c545162..b0115e3 100644
--- a/include/linux/dma/sprd-dma.h
+++ b/include/linux/dma/sprd-dma.h
@@ -3,6 +3,10 @@
 #ifndef _SPRD_DMA_H_
 #define _SPRD_DMA_H_
 
+#define SPRD_DMA_REQ_SHIFT 16
+#define SPRD_DMA_FLAGS(req_mode, int_type) \
+	((req_mode) << SPRD_DMA_REQ_SHIFT | (int_type))
+
 /*
  * enum sprd_dma_req_mode: define the DMA request mode
  * @SPRD_DMA_FRAG_REQ: fragment request mode
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration
  2018-05-11 13:06 ` [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration Baolin Wang
@ 2018-05-17  5:14   ` Vinod
  2018-05-17  6:02     ` Baolin Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Vinod @ 2018-05-17  5:14 UTC (permalink / raw)
  To: Baolin Wang; +Cc: dan.j.williams, eric.long, broonie, dmaengine, linux-kernel

On 11-05-18, 21:06, Baolin Wang wrote:
> +struct sprd_dma_config {
> +	struct dma_slave_config cfg;
> +	u32 block_len;
> +	u32 transcation_len;

/s/transcation/transaction

now in code I see block_len and this filled by len which is sg_dma_len()?
So why two varibales when you are using only one.

Second, you are storing parameters programmed thru _prep call into
_dma_config. That is not correct.

We use these to store channel parameters and NOT transaction parameters which
would differ with each txn. No wonder you can only support only 1 txn :)

Lastly, if these are same values why not use src/dstn_max_burst?

> +static enum sprd_dma_datawidth
> +sprd_dma_get_datawidth(enum dma_slave_buswidth buswidth)
> +{
> +	switch (buswidth) {
> +	case DMA_SLAVE_BUSWIDTH_1_BYTE:
> +	case DMA_SLAVE_BUSWIDTH_2_BYTES:
> +	case DMA_SLAVE_BUSWIDTH_4_BYTES:
> +	case DMA_SLAVE_BUSWIDTH_8_BYTES:
> +		return ffs(buswidth) - 1;
> +	default:
> +		return SPRD_DMA_DATAWIDTH_4_BYTES;

Does default make sense, should we error out?

> +static u32 sprd_dma_get_step(enum dma_slave_buswidth buswidth)
> +{
> +	switch (buswidth) {
> +	case DMA_SLAVE_BUSWIDTH_1_BYTE:
> +	case DMA_SLAVE_BUSWIDTH_2_BYTES:
> +	case DMA_SLAVE_BUSWIDTH_4_BYTES:
> +	case DMA_SLAVE_BUSWIDTH_8_BYTES:
> +		return buswidth;
> +
> +	default:
> +		return SPRD_DMA_DWORD_STEP;

Does default make sense, should we error out?

> +static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
> +			   dma_addr_t src, dma_addr_t dst,
> +			   struct sprd_dma_config *slave_cfg)
> +{
> +	struct sprd_dma_dev *sdev = to_sprd_dma_dev(chan);
> +	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> +	struct sprd_dma_chn_hw *hw = &sdesc->chn_hw;
> +	u32 fix_mode = 0, fix_en = 0, wrap_en = 0, wrap_mode = 0;
> +	u32 src_datawidth, dst_datawidth, temp;
> +
> +	if (slave_cfg->cfg.slave_id)
> +		schan->dev_id = slave_cfg->cfg.slave_id;
> +
> +	hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
> +
> +	temp = slave_cfg->wrap_ptr & SPRD_DMA_LOW_ADDR_MASK;
> +	temp |= (src >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
> +	hw->wrap_ptr = temp;
> +
> +	temp = slave_cfg->wrap_to & SPRD_DMA_LOW_ADDR_MASK;
> +	temp |= (dst >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
> +	hw->wrap_to = temp;
> +
> +	hw->src_addr = src & SPRD_DMA_LOW_ADDR_MASK;
> +	hw->des_addr = dst & SPRD_DMA_LOW_ADDR_MASK;
> +
> +	if ((slave_cfg->src_step != 0 && slave_cfg->dst_step != 0)
> +	    || (slave_cfg->src_step | slave_cfg->dst_step) == 0) {

this check doesnt seem right to me, what are you trying here?

> +		fix_en = 0;
> +	} else {
> +		fix_en = 1;
> +		if (slave_cfg->src_step)
> +			fix_mode = 1;
> +		else
> +			fix_mode = 0;
> +	}
> +
> +	if (slave_cfg->wrap_ptr && slave_cfg->wrap_to) {
> +		wrap_en = 1;
> +		if (slave_cfg->wrap_to == src) {
> +			wrap_mode = 0;
> +		} else if (slave_cfg->wrap_to == dst) {
> +			wrap_mode = 1;
> +		} else {
> +			dev_err(sdev->dma_dev.dev, "invalid wrap mode\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	hw->intc = slave_cfg->int_mode | SPRD_DMA_CFG_ERR_INT_EN;
> +
> +	src_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.src_addr_width);
> +	dst_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.dst_addr_width);
> +
> +	temp = src_datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET;
> +	temp |= dst_datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET;
> +	temp |= slave_cfg->req_mode << SPRD_DMA_REQ_MODE_OFFSET;
> +	temp |= wrap_mode << SPRD_DMA_WRAP_SEL_OFFSET;
> +	temp |= wrap_en << SPRD_DMA_WRAP_EN_OFFSET;
> +	temp |= fix_mode << SPRD_DMA_FIX_SEL_OFFSET;
> +	temp |= fix_en << SPRD_DMA_FIX_EN_OFFSET;
> +	temp |= slave_cfg->cfg.src_maxburst & SPRD_DMA_FRG_LEN_MASK;
> +	hw->frg_len = temp;
> +
> +	hw->blk_len = slave_cfg->block_len & SPRD_DMA_BLK_LEN_MASK;
> +	hw->trsc_len = slave_cfg->transcation_len & SPRD_DMA_TRSC_LEN_MASK;
> +
> +	temp = (slave_cfg->dst_step & SPRD_DMA_TRSF_STEP_MASK) <<
> +		SPRD_DMA_DEST_TRSF_STEP_OFFSET;
> +	temp |= (slave_cfg->src_step & SPRD_DMA_TRSF_STEP_MASK) <<
> +		SPRD_DMA_SRC_TRSF_STEP_OFFSET;
> +	hw->trsf_step = temp;
> +
> +	hw->frg_step = 0;
> +	hw->src_blk_step = 0;
> +	hw->des_blk_step = 0;
> +	return 0;
> +}
> +static struct dma_async_tx_descriptor *
> +sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +		       unsigned int sglen, enum dma_transfer_direction dir,
> +		       unsigned long flags, void *context)
> +{
> +	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> +	struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
> +	u32 src_step = 0, dst_step = 0, len = 0;
> +	dma_addr_t src = 0, dst = 0;
> +	struct sprd_dma_desc *sdesc;
> +	struct scatterlist *sg;
> +	int ret, i;
> +
> +	/* TODO: now we only support one sg for each DMA configuration. */
> +	if (!is_slave_direction(dir) || sglen > 1)
> +		return NULL;
> +
> +	sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> +	if (!sdesc)
> +		return NULL;
> +
> +	for_each_sg(sgl, sg, sglen, i) {
> +		len = sg_dma_len(sg);
> +
> +		if (dir == DMA_MEM_TO_DEV) {
> +			src = sg_dma_address(sg);
> +			dst = slave_cfg->cfg.dst_addr;
> +			src_step = sprd_dma_get_step(slave_cfg->cfg.src_addr_width);
> +			dst_step = SPRD_DMA_NONE_STEP;
> +		} else {
> +			src = slave_cfg->cfg.src_addr;
> +			dst = sg_dma_address(sg);
> +			src_step = SPRD_DMA_NONE_STEP;
> +			dst_step = sprd_dma_get_step(slave_cfg->cfg.dst_addr_width);
> +		}
> +	}
> +
> +	sprd_dma_fill_config(slave_cfg, src_step, dst_step, len, flags);
> +
> +	ret = sprd_dma_config(chan, sdesc, src, dst, slave_cfg);
> +	if (ret) {
> +		kfree(sdesc);
> +		return NULL;
> +	}

Am more intrigued here, the above call fills you config struct but you do not
use it.. so what is the use of that. 

-- 
~Vinod

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration
  2018-05-17  5:14   ` Vinod
@ 2018-05-17  6:02     ` Baolin Wang
  2018-05-17  6:11       ` Vinod
  0 siblings, 1 reply; 6+ messages in thread
From: Baolin Wang @ 2018-05-17  6:02 UTC (permalink / raw)
  To: Vinod; +Cc: Dan Williams, Eric Long, Mark Brown, dmaengine, LKML

Hi Vinod,

On 17 May 2018 at 13:14, Vinod <vkoul@kernel.org> wrote:
> On 11-05-18, 21:06, Baolin Wang wrote:
>> +struct sprd_dma_config {
>> +     struct dma_slave_config cfg;
>> +     u32 block_len;
>> +     u32 transcation_len;
>
> /s/transcation/transaction

OK.

>
> now in code I see block_len and this filled by len which is sg_dma_len()?
> So why two varibales when you are using only one.

Like I explained before, we have transaction transfer, block transfer
and fragment transfer, and we should set the length for each transfer.
In future, we we will use these two variables in cyclic mode, but for
prep_sg, we just make them equal to

>
> Second, you are storing parameters programmed thru _prep call into
> _dma_config. That is not correct.
>
> We use these to store channel parameters and NOT transaction parameters which
> would differ with each txn. No wonder you can only support only 1 txn :)

Fine, we can remove block_len/transcation_len from 'struct
sprd_dma_config'. Any other issues for the 'struct sprd_dma_config'?

>
> Lastly, if these are same values why not use src/dstn_max_burst?

The fragment length will be filled by src/dstn_max_burst,so we can not
use it again.

>
>> +static enum sprd_dma_datawidth
>> +sprd_dma_get_datawidth(enum dma_slave_buswidth buswidth)
>> +{
>> +     switch (buswidth) {
>> +     case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> +     case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> +     case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> +     case DMA_SLAVE_BUSWIDTH_8_BYTES:
>> +             return ffs(buswidth) - 1;
>> +     default:
>> +             return SPRD_DMA_DATAWIDTH_4_BYTES;
>
> Does default make sense, should we error out?

Not one error, maybe we can give some warning information.

>
>> +static u32 sprd_dma_get_step(enum dma_slave_buswidth buswidth)
>> +{
>> +     switch (buswidth) {
>> +     case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> +     case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> +     case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> +     case DMA_SLAVE_BUSWIDTH_8_BYTES:
>> +             return buswidth;
>> +
>> +     default:
>> +             return SPRD_DMA_DWORD_STEP;
>
> Does default make sense, should we error out?

Ditto.

>
>> +static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
>> +                        dma_addr_t src, dma_addr_t dst,
>> +                        struct sprd_dma_config *slave_cfg)
>> +{
>> +     struct sprd_dma_dev *sdev = to_sprd_dma_dev(chan);
>> +     struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
>> +     struct sprd_dma_chn_hw *hw = &sdesc->chn_hw;
>> +     u32 fix_mode = 0, fix_en = 0, wrap_en = 0, wrap_mode = 0;
>> +     u32 src_datawidth, dst_datawidth, temp;
>> +
>> +     if (slave_cfg->cfg.slave_id)
>> +             schan->dev_id = slave_cfg->cfg.slave_id;
>> +
>> +     hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
>> +
>> +     temp = slave_cfg->wrap_ptr & SPRD_DMA_LOW_ADDR_MASK;
>> +     temp |= (src >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
>> +     hw->wrap_ptr = temp;
>> +
>> +     temp = slave_cfg->wrap_to & SPRD_DMA_LOW_ADDR_MASK;
>> +     temp |= (dst >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
>> +     hw->wrap_to = temp;
>> +
>> +     hw->src_addr = src & SPRD_DMA_LOW_ADDR_MASK;
>> +     hw->des_addr = dst & SPRD_DMA_LOW_ADDR_MASK;
>> +
>> +     if ((slave_cfg->src_step != 0 && slave_cfg->dst_step != 0)
>> +         || (slave_cfg->src_step | slave_cfg->dst_step) == 0) {
>
> this check doesnt seem right to me, what are you trying here?

This is SPRD DMA internal feature: address-fix transfer. If the src
step and dst step both are 0 or both are not 0, that means we can not
enable the fix mode.
If one is 0 and another one is not, we can enable the fix mode.

>
>> +             fix_en = 0;
>> +     } else {
>> +             fix_en = 1;
>> +             if (slave_cfg->src_step)
>> +                     fix_mode = 1;
>> +             else
>> +                     fix_mode = 0;
>> +     }
>> +
>> +     if (slave_cfg->wrap_ptr && slave_cfg->wrap_to) {
>> +             wrap_en = 1;
>> +             if (slave_cfg->wrap_to == src) {
>> +                     wrap_mode = 0;
>> +             } else if (slave_cfg->wrap_to == dst) {
>> +                     wrap_mode = 1;
>> +             } else {
>> +                     dev_err(sdev->dma_dev.dev, "invalid wrap mode\n");
>> +                     return -EINVAL;
>> +             }
>> +     }
>> +
>> +     hw->intc = slave_cfg->int_mode | SPRD_DMA_CFG_ERR_INT_EN;
>> +
>> +     src_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.src_addr_width);
>> +     dst_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.dst_addr_width);
>> +
>> +     temp = src_datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET;
>> +     temp |= dst_datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET;
>> +     temp |= slave_cfg->req_mode << SPRD_DMA_REQ_MODE_OFFSET;
>> +     temp |= wrap_mode << SPRD_DMA_WRAP_SEL_OFFSET;
>> +     temp |= wrap_en << SPRD_DMA_WRAP_EN_OFFSET;
>> +     temp |= fix_mode << SPRD_DMA_FIX_SEL_OFFSET;
>> +     temp |= fix_en << SPRD_DMA_FIX_EN_OFFSET;
>> +     temp |= slave_cfg->cfg.src_maxburst & SPRD_DMA_FRG_LEN_MASK;
>> +     hw->frg_len = temp;
>> +
>> +     hw->blk_len = slave_cfg->block_len & SPRD_DMA_BLK_LEN_MASK;
>> +     hw->trsc_len = slave_cfg->transcation_len & SPRD_DMA_TRSC_LEN_MASK;
>> +
>> +     temp = (slave_cfg->dst_step & SPRD_DMA_TRSF_STEP_MASK) <<
>> +             SPRD_DMA_DEST_TRSF_STEP_OFFSET;
>> +     temp |= (slave_cfg->src_step & SPRD_DMA_TRSF_STEP_MASK) <<
>> +             SPRD_DMA_SRC_TRSF_STEP_OFFSET;
>> +     hw->trsf_step = temp;
>> +
>> +     hw->frg_step = 0;
>> +     hw->src_blk_step = 0;
>> +     hw->des_blk_step = 0;
>> +     return 0;
>> +}
>> +static struct dma_async_tx_descriptor *
>> +sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>> +                    unsigned int sglen, enum dma_transfer_direction dir,
>> +                    unsigned long flags, void *context)
>> +{
>> +     struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
>> +     struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
>> +     u32 src_step = 0, dst_step = 0, len = 0;
>> +     dma_addr_t src = 0, dst = 0;
>> +     struct sprd_dma_desc *sdesc;
>> +     struct scatterlist *sg;
>> +     int ret, i;
>> +
>> +     /* TODO: now we only support one sg for each DMA configuration. */
>> +     if (!is_slave_direction(dir) || sglen > 1)
>> +             return NULL;
>> +
>> +     sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
>> +     if (!sdesc)
>> +             return NULL;
>> +
>> +     for_each_sg(sgl, sg, sglen, i) {
>> +             len = sg_dma_len(sg);
>> +
>> +             if (dir == DMA_MEM_TO_DEV) {
>> +                     src = sg_dma_address(sg);
>> +                     dst = slave_cfg->cfg.dst_addr;
>> +                     src_step = sprd_dma_get_step(slave_cfg->cfg.src_addr_width);
>> +                     dst_step = SPRD_DMA_NONE_STEP;
>> +             } else {
>> +                     src = slave_cfg->cfg.src_addr;
>> +                     dst = sg_dma_address(sg);
>> +                     src_step = SPRD_DMA_NONE_STEP;
>> +                     dst_step = sprd_dma_get_step(slave_cfg->cfg.dst_addr_width);
>> +             }
>> +     }
>> +
>> +     sprd_dma_fill_config(slave_cfg, src_step, dst_step, len, flags);
>> +
>> +     ret = sprd_dma_config(chan, sdesc, src, dst, slave_cfg);
>> +     if (ret) {
>> +             kfree(sdesc);
>> +             return NULL;
>> +     }
>
> Am more intrigued here, the above call fills you config struct but you do not
> use it.. so what is the use of that.

I did not get you here. We have passed the slave_cfg to
sprd_dma_config() to configure DMA hardware channel. Thanks.

-- 
Baolin.wang
Best Regards

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration
  2018-05-17  6:02     ` Baolin Wang
@ 2018-05-17  6:11       ` Vinod
  2018-05-17  6:28         ` Baolin Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Vinod @ 2018-05-17  6:11 UTC (permalink / raw)
  To: Baolin Wang; +Cc: Dan Williams, Eric Long, Mark Brown, dmaengine, LKML

On 17-05-18, 14:02, Baolin Wang wrote:
> Hi Vinod,
> 
> On 17 May 2018 at 13:14, Vinod <vkoul@kernel.org> wrote:
> > On 11-05-18, 21:06, Baolin Wang wrote:
> >> +struct sprd_dma_config {
> >> +     struct dma_slave_config cfg;
> >> +     u32 block_len;
> >> +     u32 transcation_len;
> >
> > /s/transcation/transaction
> 
> OK.
> 
> >
> > now in code I see block_len and this filled by len which is sg_dma_len()?
> > So why two varibales when you are using only one.
> 
> Like I explained before, we have transaction transfer, block transfer
> and fragment transfer, and we should set the length for each transfer.
> In future, we we will use these two variables in cyclic mode, but for
> prep_sg, we just make them equal to

Please add them when you have use for it. Sorry linux kernel is not a place for
dumping future use work

> 
> >
> > Second, you are storing parameters programmed thru _prep call into
> > _dma_config. That is not correct.
> >
> > We use these to store channel parameters and NOT transaction parameters which
> > would differ with each txn. No wonder you can only support only 1 txn :)
> 
> Fine, we can remove block_len/transcation_len from 'struct
> sprd_dma_config'. Any other issues for the 'struct sprd_dma_config'?

Yes see the comments on prep_

> 
> >
> > Lastly, if these are same values why not use src/dstn_max_burst?
> 
> The fragment length will be filled by src/dstn_max_burst,so we can not
> use it again.
> 
> >
> >> +static enum sprd_dma_datawidth
> >> +sprd_dma_get_datawidth(enum dma_slave_buswidth buswidth)
> >> +{
> >> +     switch (buswidth) {
> >> +     case DMA_SLAVE_BUSWIDTH_1_BYTE:
> >> +     case DMA_SLAVE_BUSWIDTH_2_BYTES:
> >> +     case DMA_SLAVE_BUSWIDTH_4_BYTES:
> >> +     case DMA_SLAVE_BUSWIDTH_8_BYTES:
> >> +             return ffs(buswidth) - 1;
> >> +     default:
> >> +             return SPRD_DMA_DATAWIDTH_4_BYTES;
> >
> > Does default make sense, should we error out?
> 
> Not one error, maybe we can give some warning information.

well client programmed a wrong value, so I expect this to error out.

> 
> >
> >> +static u32 sprd_dma_get_step(enum dma_slave_buswidth buswidth)
> >> +{
> >> +     switch (buswidth) {
> >> +     case DMA_SLAVE_BUSWIDTH_1_BYTE:
> >> +     case DMA_SLAVE_BUSWIDTH_2_BYTES:
> >> +     case DMA_SLAVE_BUSWIDTH_4_BYTES:
> >> +     case DMA_SLAVE_BUSWIDTH_8_BYTES:
> >> +             return buswidth;
> >> +
> >> +     default:
> >> +             return SPRD_DMA_DWORD_STEP;
> >
> > Does default make sense, should we error out?
> 
> Ditto.
> 
> >
> >> +static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
> >> +                        dma_addr_t src, dma_addr_t dst,
> >> +                        struct sprd_dma_config *slave_cfg)
> >> +{
> >> +     struct sprd_dma_dev *sdev = to_sprd_dma_dev(chan);
> >> +     struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> >> +     struct sprd_dma_chn_hw *hw = &sdesc->chn_hw;
> >> +     u32 fix_mode = 0, fix_en = 0, wrap_en = 0, wrap_mode = 0;
> >> +     u32 src_datawidth, dst_datawidth, temp;
> >> +
> >> +     if (slave_cfg->cfg.slave_id)
> >> +             schan->dev_id = slave_cfg->cfg.slave_id;
> >> +
> >> +     hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
> >> +
> >> +     temp = slave_cfg->wrap_ptr & SPRD_DMA_LOW_ADDR_MASK;
> >> +     temp |= (src >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
> >> +     hw->wrap_ptr = temp;
> >> +
> >> +     temp = slave_cfg->wrap_to & SPRD_DMA_LOW_ADDR_MASK;
> >> +     temp |= (dst >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
> >> +     hw->wrap_to = temp;
> >> +
> >> +     hw->src_addr = src & SPRD_DMA_LOW_ADDR_MASK;
> >> +     hw->des_addr = dst & SPRD_DMA_LOW_ADDR_MASK;
> >> +
> >> +     if ((slave_cfg->src_step != 0 && slave_cfg->dst_step != 0)
> >> +         || (slave_cfg->src_step | slave_cfg->dst_step) == 0) {
> >
> > this check doesnt seem right to me, what are you trying here?
> 
> This is SPRD DMA internal feature: address-fix transfer. If the src
> step and dst step both are 0 or both are not 0, that means we can not
> enable the fix mode.
> If one is 0 and another one is not, we can enable the fix mode.

A comment here would help explain this :)

> 
> >
> >> +             fix_en = 0;
> >> +     } else {
> >> +             fix_en = 1;
> >> +             if (slave_cfg->src_step)
> >> +                     fix_mode = 1;
> >> +             else
> >> +                     fix_mode = 0;
> >> +     }
> >> +
> >> +     if (slave_cfg->wrap_ptr && slave_cfg->wrap_to) {
> >> +             wrap_en = 1;
> >> +             if (slave_cfg->wrap_to == src) {
> >> +                     wrap_mode = 0;
> >> +             } else if (slave_cfg->wrap_to == dst) {
> >> +                     wrap_mode = 1;
> >> +             } else {
> >> +                     dev_err(sdev->dma_dev.dev, "invalid wrap mode\n");
> >> +                     return -EINVAL;
> >> +             }
> >> +     }
> >> +
> >> +     hw->intc = slave_cfg->int_mode | SPRD_DMA_CFG_ERR_INT_EN;
> >> +
> >> +     src_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.src_addr_width);
> >> +     dst_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.dst_addr_width);
> >> +
> >> +     temp = src_datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET;
> >> +     temp |= dst_datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET;
> >> +     temp |= slave_cfg->req_mode << SPRD_DMA_REQ_MODE_OFFSET;
> >> +     temp |= wrap_mode << SPRD_DMA_WRAP_SEL_OFFSET;
> >> +     temp |= wrap_en << SPRD_DMA_WRAP_EN_OFFSET;
> >> +     temp |= fix_mode << SPRD_DMA_FIX_SEL_OFFSET;
> >> +     temp |= fix_en << SPRD_DMA_FIX_EN_OFFSET;
> >> +     temp |= slave_cfg->cfg.src_maxburst & SPRD_DMA_FRG_LEN_MASK;
> >> +     hw->frg_len = temp;
> >> +
> >> +     hw->blk_len = slave_cfg->block_len & SPRD_DMA_BLK_LEN_MASK;
> >> +     hw->trsc_len = slave_cfg->transcation_len & SPRD_DMA_TRSC_LEN_MASK;
> >> +
> >> +     temp = (slave_cfg->dst_step & SPRD_DMA_TRSF_STEP_MASK) <<
> >> +             SPRD_DMA_DEST_TRSF_STEP_OFFSET;
> >> +     temp |= (slave_cfg->src_step & SPRD_DMA_TRSF_STEP_MASK) <<
> >> +             SPRD_DMA_SRC_TRSF_STEP_OFFSET;
> >> +     hw->trsf_step = temp;
> >> +
> >> +     hw->frg_step = 0;
> >> +     hw->src_blk_step = 0;
> >> +     hw->des_blk_step = 0;
> >> +     return 0;
> >> +}
> >> +static struct dma_async_tx_descriptor *
> >> +sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> >> +                    unsigned int sglen, enum dma_transfer_direction dir,
> >> +                    unsigned long flags, void *context)
> >> +{
> >> +     struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> >> +     struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
> >> +     u32 src_step = 0, dst_step = 0, len = 0;
> >> +     dma_addr_t src = 0, dst = 0;
> >> +     struct sprd_dma_desc *sdesc;
> >> +     struct scatterlist *sg;
> >> +     int ret, i;
> >> +
> >> +     /* TODO: now we only support one sg for each DMA configuration. */
> >> +     if (!is_slave_direction(dir) || sglen > 1)
> >> +             return NULL;
> >> +
> >> +     sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> >> +     if (!sdesc)
> >> +             return NULL;
> >> +
> >> +     for_each_sg(sgl, sg, sglen, i) {
> >> +             len = sg_dma_len(sg);
> >> +
> >> +             if (dir == DMA_MEM_TO_DEV) {
> >> +                     src = sg_dma_address(sg);
> >> +                     dst = slave_cfg->cfg.dst_addr;
> >> +                     src_step = sprd_dma_get_step(slave_cfg->cfg.src_addr_width);
> >> +                     dst_step = SPRD_DMA_NONE_STEP;
> >> +             } else {
> >> +                     src = slave_cfg->cfg.src_addr;
> >> +                     dst = sg_dma_address(sg);
> >> +                     src_step = SPRD_DMA_NONE_STEP;
> >> +                     dst_step = sprd_dma_get_step(slave_cfg->cfg.dst_addr_width);
> >> +             }
> >> +     }
> >> +
> >> +     sprd_dma_fill_config(slave_cfg, src_step, dst_step, len, flags);
> >> +
> >> +     ret = sprd_dma_config(chan, sdesc, src, dst, slave_cfg);
> >> +     if (ret) {
> >> +             kfree(sdesc);
> >> +             return NULL;
> >> +     }
> >
> > Am more intrigued here, the above call fills you config struct but you do not
> > use it.. so what is the use of that.
> 
> I did not get you here. We have passed the slave_cfg to
> sprd_dma_config() to configure DMA hardware channel. Thanks.

But you are not using that... So why bother configuring!

Again this is a channel specific data structure and not transaction specific.
The values should go into descriptor and not something which is channel specific

-- 
~Vinod

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration
  2018-05-17  6:11       ` Vinod
@ 2018-05-17  6:28         ` Baolin Wang
  0 siblings, 0 replies; 6+ messages in thread
From: Baolin Wang @ 2018-05-17  6:28 UTC (permalink / raw)
  To: Vinod; +Cc: Dan Williams, Eric Long, Mark Brown, dmaengine, LKML

On 17 May 2018 at 14:11, Vinod <vkoul@kernel.org> wrote:
> On 17-05-18, 14:02, Baolin Wang wrote:
>> Hi Vinod,
>>
>> On 17 May 2018 at 13:14, Vinod <vkoul@kernel.org> wrote:
>> > On 11-05-18, 21:06, Baolin Wang wrote:
>> >> +struct sprd_dma_config {
>> >> +     struct dma_slave_config cfg;
>> >> +     u32 block_len;
>> >> +     u32 transcation_len;
>> >
>> > /s/transcation/transaction
>>
>> OK.
>>
>> >
>> > now in code I see block_len and this filled by len which is sg_dma_len()?
>> > So why two varibales when you are using only one.
>>
>> Like I explained before, we have transaction transfer, block transfer
>> and fragment transfer, and we should set the length for each transfer.
>> In future, we we will use these two variables in cyclic mode, but for
>> prep_sg, we just make them equal to
>
> Please add them when you have use for it. Sorry linux kernel is not a place for
> dumping future use work

We now use them, since we have registers to configure, and we just set
the same values for them now.

>>
>> >
>> > Second, you are storing parameters programmed thru _prep call into
>> > _dma_config. That is not correct.
>> >
>> > We use these to store channel parameters and NOT transaction parameters which
>> > would differ with each txn. No wonder you can only support only 1 txn :)
>>
>> Fine, we can remove block_len/transcation_len from 'struct
>> sprd_dma_config'. Any other issues for the 'struct sprd_dma_config'?
>
> Yes see the comments on prep_

OK.

>>
>> >
>> > Lastly, if these are same values why not use src/dstn_max_burst?
>>
>> The fragment length will be filled by src/dstn_max_burst,so we can not
>> use it again.
>>
>> >
>> >> +static enum sprd_dma_datawidth
>> >> +sprd_dma_get_datawidth(enum dma_slave_buswidth buswidth)
>> >> +{
>> >> +     switch (buswidth) {
>> >> +     case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> >> +     case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> >> +     case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> >> +     case DMA_SLAVE_BUSWIDTH_8_BYTES:
>> >> +             return ffs(buswidth) - 1;
>> >> +     default:
>> >> +             return SPRD_DMA_DATAWIDTH_4_BYTES;
>> >
>> > Does default make sense, should we error out?
>>
>> Not one error, maybe we can give some warning information.
>
> well client programmed a wrong value, so I expect this to error out.

OK.

>
>>
>> >
>> >> +static u32 sprd_dma_get_step(enum dma_slave_buswidth buswidth)
>> >> +{
>> >> +     switch (buswidth) {
>> >> +     case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> >> +     case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> >> +     case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> >> +     case DMA_SLAVE_BUSWIDTH_8_BYTES:
>> >> +             return buswidth;
>> >> +
>> >> +     default:
>> >> +             return SPRD_DMA_DWORD_STEP;
>> >
>> > Does default make sense, should we error out?
>>
>> Ditto.
>>
>> >
>> >> +static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
>> >> +                        dma_addr_t src, dma_addr_t dst,
>> >> +                        struct sprd_dma_config *slave_cfg)
>> >> +{
>> >> +     struct sprd_dma_dev *sdev = to_sprd_dma_dev(chan);
>> >> +     struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
>> >> +     struct sprd_dma_chn_hw *hw = &sdesc->chn_hw;
>> >> +     u32 fix_mode = 0, fix_en = 0, wrap_en = 0, wrap_mode = 0;
>> >> +     u32 src_datawidth, dst_datawidth, temp;
>> >> +
>> >> +     if (slave_cfg->cfg.slave_id)
>> >> +             schan->dev_id = slave_cfg->cfg.slave_id;
>> >> +
>> >> +     hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
>> >> +
>> >> +     temp = slave_cfg->wrap_ptr & SPRD_DMA_LOW_ADDR_MASK;
>> >> +     temp |= (src >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
>> >> +     hw->wrap_ptr = temp;
>> >> +
>> >> +     temp = slave_cfg->wrap_to & SPRD_DMA_LOW_ADDR_MASK;
>> >> +     temp |= (dst >> SPRD_DMA_HIGH_ADDR_OFFSET) & SPRD_DMA_HIGH_ADDR_MASK;
>> >> +     hw->wrap_to = temp;
>> >> +
>> >> +     hw->src_addr = src & SPRD_DMA_LOW_ADDR_MASK;
>> >> +     hw->des_addr = dst & SPRD_DMA_LOW_ADDR_MASK;
>> >> +
>> >> +     if ((slave_cfg->src_step != 0 && slave_cfg->dst_step != 0)
>> >> +         || (slave_cfg->src_step | slave_cfg->dst_step) == 0) {
>> >
>> > this check doesnt seem right to me, what are you trying here?
>>
>> This is SPRD DMA internal feature: address-fix transfer. If the src
>> step and dst step both are 0 or both are not 0, that means we can not
>> enable the fix mode.
>> If one is 0 and another one is not, we can enable the fix mode.
>
> A comment here would help explain this :)

Sure.

>>
>> >
>> >> +             fix_en = 0;
>> >> +     } else {
>> >> +             fix_en = 1;
>> >> +             if (slave_cfg->src_step)
>> >> +                     fix_mode = 1;
>> >> +             else
>> >> +                     fix_mode = 0;
>> >> +     }
>> >> +
>> >> +     if (slave_cfg->wrap_ptr && slave_cfg->wrap_to) {
>> >> +             wrap_en = 1;
>> >> +             if (slave_cfg->wrap_to == src) {
>> >> +                     wrap_mode = 0;
>> >> +             } else if (slave_cfg->wrap_to == dst) {
>> >> +                     wrap_mode = 1;
>> >> +             } else {
>> >> +                     dev_err(sdev->dma_dev.dev, "invalid wrap mode\n");
>> >> +                     return -EINVAL;
>> >> +             }
>> >> +     }
>> >> +
>> >> +     hw->intc = slave_cfg->int_mode | SPRD_DMA_CFG_ERR_INT_EN;
>> >> +
>> >> +     src_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.src_addr_width);
>> >> +     dst_datawidth = sprd_dma_get_datawidth(slave_cfg->cfg.dst_addr_width);
>> >> +
>> >> +     temp = src_datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET;
>> >> +     temp |= dst_datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET;
>> >> +     temp |= slave_cfg->req_mode << SPRD_DMA_REQ_MODE_OFFSET;
>> >> +     temp |= wrap_mode << SPRD_DMA_WRAP_SEL_OFFSET;
>> >> +     temp |= wrap_en << SPRD_DMA_WRAP_EN_OFFSET;
>> >> +     temp |= fix_mode << SPRD_DMA_FIX_SEL_OFFSET;
>> >> +     temp |= fix_en << SPRD_DMA_FIX_EN_OFFSET;
>> >> +     temp |= slave_cfg->cfg.src_maxburst & SPRD_DMA_FRG_LEN_MASK;
>> >> +     hw->frg_len = temp;
>> >> +
>> >> +     hw->blk_len = slave_cfg->block_len & SPRD_DMA_BLK_LEN_MASK;
>> >> +     hw->trsc_len = slave_cfg->transcation_len & SPRD_DMA_TRSC_LEN_MASK;
>> >> +
>> >> +     temp = (slave_cfg->dst_step & SPRD_DMA_TRSF_STEP_MASK) <<
>> >> +             SPRD_DMA_DEST_TRSF_STEP_OFFSET;
>> >> +     temp |= (slave_cfg->src_step & SPRD_DMA_TRSF_STEP_MASK) <<
>> >> +             SPRD_DMA_SRC_TRSF_STEP_OFFSET;
>> >> +     hw->trsf_step = temp;
>> >> +
>> >> +     hw->frg_step = 0;
>> >> +     hw->src_blk_step = 0;
>> >> +     hw->des_blk_step = 0;
>> >> +     return 0;
>> >> +}
>> >> +static struct dma_async_tx_descriptor *
>> >> +sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>> >> +                    unsigned int sglen, enum dma_transfer_direction dir,
>> >> +                    unsigned long flags, void *context)
>> >> +{
>> >> +     struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
>> >> +     struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
>> >> +     u32 src_step = 0, dst_step = 0, len = 0;
>> >> +     dma_addr_t src = 0, dst = 0;
>> >> +     struct sprd_dma_desc *sdesc;
>> >> +     struct scatterlist *sg;
>> >> +     int ret, i;
>> >> +
>> >> +     /* TODO: now we only support one sg for each DMA configuration. */
>> >> +     if (!is_slave_direction(dir) || sglen > 1)
>> >> +             return NULL;
>> >> +
>> >> +     sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
>> >> +     if (!sdesc)
>> >> +             return NULL;
>> >> +
>> >> +     for_each_sg(sgl, sg, sglen, i) {
>> >> +             len = sg_dma_len(sg);
>> >> +
>> >> +             if (dir == DMA_MEM_TO_DEV) {
>> >> +                     src = sg_dma_address(sg);
>> >> +                     dst = slave_cfg->cfg.dst_addr;
>> >> +                     src_step = sprd_dma_get_step(slave_cfg->cfg.src_addr_width);
>> >> +                     dst_step = SPRD_DMA_NONE_STEP;
>> >> +             } else {
>> >> +                     src = slave_cfg->cfg.src_addr;
>> >> +                     dst = sg_dma_address(sg);
>> >> +                     src_step = SPRD_DMA_NONE_STEP;
>> >> +                     dst_step = sprd_dma_get_step(slave_cfg->cfg.dst_addr_width);
>> >> +             }
>> >> +     }
>> >> +
>> >> +     sprd_dma_fill_config(slave_cfg, src_step, dst_step, len, flags);
>> >> +
>> >> +     ret = sprd_dma_config(chan, sdesc, src, dst, slave_cfg);
>> >> +     if (ret) {
>> >> +             kfree(sdesc);
>> >> +             return NULL;
>> >> +     }
>> >
>> > Am more intrigued here, the above call fills you config struct but you do not
>> > use it.. so what is the use of that.
>>
>> I did not get you here. We have passed the slave_cfg to
>> sprd_dma_config() to configure DMA hardware channel. Thanks.
>
> But you are not using that... So why bother configuring!

No, we used it. We use some configuration (src_maxburst, addr_width
and slave id) from slave_cfg to configure our DMA hardware channel.

>
> Again this is a channel specific data structure and not transaction specific.
> The values should go into descriptor and not something which is channel specific

Make sense.

-- 
Baolin.wang
Best Regards

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-17  6:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-11 13:06 [PATCH v3 1/2] dmaengine: sprd: Optimize the sprd_dma_prep_dma_memcpy() Baolin Wang
2018-05-11 13:06 ` [PATCH v3 2/2] dmaengine: sprd: Add Spreadtrum DMA configuration Baolin Wang
2018-05-17  5:14   ` Vinod
2018-05-17  6:02     ` Baolin Wang
2018-05-17  6:11       ` Vinod
2018-05-17  6:28         ` Baolin Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).