2018-04-10 07:54:11

by Baolin Wang

Subject: [PATCH 1/5] dmaengine: sprd: Define the DMA transfer step type

From: Eric Long <[email protected]>

Define the DMA transfer step type to make code more readable.

Signed-off-by: Eric Long <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/dma/sprd-dma.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index b106e8a..af9c156 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -169,6 +169,22 @@ enum sprd_dma_int_type {
SPRD_DMA_CFGERR_INT,
};

+/*
+ * enum sprd_dma_step: define the DMA transfer step type
+ * @SPRD_DMA_NONE_STEP: transfer do not need step
+ * @SPRD_DMA_BYTE_STEP: transfer step is one byte
+ * @SPRD_DMA_SHORT_STEP: transfer step is two bytes
+ * @SPRD_DMA_WORD_STEP: transfer step is one word
+ * @SPRD_DMA_DWORD_STEP: transfer step is double word
+ */
+enum sprd_dma_step {
+ SPRD_DMA_NONE_STEP,
+ SPRD_DMA_BYTE_STEP = BIT(0),
+ SPRD_DMA_SHORT_STEP = BIT(1),
+ SPRD_DMA_WORD_STEP = BIT(2),
+ SPRD_DMA_DWORD_STEP = BIT(3),
+};
+
/* dma channel hardware configuration */
struct sprd_dma_chn_hw {
u32 pause;
@@ -598,16 +614,16 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,

if (IS_ALIGNED(len, 4)) {
datawidth = 2;
- src_step = 4;
- des_step = 4;
+ src_step = SPRD_DMA_WORD_STEP;
+ des_step = SPRD_DMA_WORD_STEP;
} else if (IS_ALIGNED(len, 2)) {
datawidth = 1;
- src_step = 2;
- des_step = 2;
+ src_step = SPRD_DMA_SHORT_STEP;
+ des_step = SPRD_DMA_SHORT_STEP;
} else {
datawidth = 0;
- src_step = 1;
- des_step = 1;
+ src_step = SPRD_DMA_BYTE_STEP;
+ des_step = SPRD_DMA_BYTE_STEP;
}

fragment_len = SPRD_DMA_MEMCPY_MIN_SIZE;
--
1.7.9.5

2018-04-10 07:50:15

by Baolin Wang

Subject: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

There are some Spreadtrum special configuration for DMA controller,
thus this patch adds one 'struct sprd_dma_config' structure for users
to configure.

Moreover this patch did some optimization for sprd_dma_config() and
sprd_dma_prep_dma_memcpy() to prepare to configure DMA from users.

Signed-off-by: Eric Long <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/dma/sprd-dma.c | 262 ++++++++++++++++++++++++++++++++++--------
include/linux/dma/sprd-dma.h | 25 ++++
2 files changed, 238 insertions(+), 49 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 5c26fde..f8038de 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
@@ -173,6 +175,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;
@@ -561,52 +564,162 @@ 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:
+ return SPRD_DMA_DATAWIDTH_1_BYTE;
+
+ case DMA_SLAVE_BUSWIDTH_2_BYTES:
+ return SPRD_DMA_DATAWIDTH_2_BYTES;
+
+ case DMA_SLAVE_BUSWIDTH_4_BYTES:
+ return SPRD_DMA_DATAWIDTH_4_BYTES;
+
+ case DMA_SLAVE_BUSWIDTH_8_BYTES:
+ return SPRD_DMA_DATAWIDTH_8_BYTES;
+
+ default:
+ return SPRD_DMA_DATAWIDTH_4_BYTES;
+ }
+}
+
+static int sprd_dma_get_step(enum dma_slave_buswidth buswidth,
+ enum dma_transfer_direction dir,
+ enum sprd_dma_step *src_step,
+ enum sprd_dma_step *dst_step)
+{
+ switch (dir) {
+ case DMA_MEM_TO_MEM:
+ switch (buswidth) {
+ case DMA_SLAVE_BUSWIDTH_1_BYTE:
+ *src_step = SPRD_DMA_BYTE_STEP;
+ *dst_step = SPRD_DMA_BYTE_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_2_BYTES:
+ *src_step = SPRD_DMA_SHORT_STEP;
+ *dst_step = SPRD_DMA_SHORT_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_4_BYTES:
+ *src_step = SPRD_DMA_WORD_STEP;
+ *dst_step = SPRD_DMA_WORD_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_8_BYTES:
+ *src_step = SPRD_DMA_DWORD_STEP;
+ *dst_step = SPRD_DMA_DWORD_STEP;
+ break;
+
+ default:
+ *src_step = SPRD_DMA_WORD_STEP;
+ *dst_step = SPRD_DMA_WORD_STEP;
+ break;
+ }
+ break;
+
+ case DMA_MEM_TO_DEV:
+ switch (buswidth) {
+ case DMA_SLAVE_BUSWIDTH_1_BYTE:
+ *src_step = SPRD_DMA_BYTE_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_2_BYTES:
+ *src_step = SPRD_DMA_SHORT_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_4_BYTES:
+ *src_step = SPRD_DMA_WORD_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_8_BYTES:
+ *src_step = SPRD_DMA_DWORD_STEP;
+ break;
+
+ default:
+ *src_step = SPRD_DMA_WORD_STEP;
+ break;
+ }
+
+ *dst_step = SPRD_DMA_NONE_STEP;
+ break;
+
+ case DMA_DEV_TO_MEM:
+ switch (buswidth) {
+ case DMA_SLAVE_BUSWIDTH_1_BYTE:
+ *dst_step = SPRD_DMA_BYTE_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_2_BYTES:
+ *dst_step = SPRD_DMA_SHORT_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_4_BYTES:
+ *dst_step = SPRD_DMA_WORD_STEP;
+ break;
+
+ case DMA_SLAVE_BUSWIDTH_8_BYTES:
+ *dst_step = SPRD_DMA_DWORD_STEP;
+ break;
+
+ default:
+ *dst_step = SPRD_DMA_WORD_STEP;
+ break;
+ }
+
+ *src_step = SPRD_DMA_NONE_STEP;
+ break;
+
+ case DMA_DEV_TO_DEV:
+ *src_step = SPRD_DMA_NONE_STEP;
+ *dst_step = SPRD_DMA_NONE_STEP;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
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_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 datawidth, src_step, des_step, fragment_len;
- u32 block_len, req_mode, irq_mode, transcation_len;
- u32 fix_mode = 0, fix_en = 0;
+ u32 fix_mode = 0, fix_en = 0, wrap_en = 0, wrap_mode = 0;
+ enum sprd_dma_step src_step, dst_step;
+ enum sprd_dma_datawidth src_datawidth, dst_datawidth;
+ int ret;

- 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;
+ ret = sprd_dma_get_step(slave_cfg->config.src_addr_width,
+ slave_cfg->config.direction,
+ &src_step, &dst_step);
+ if (ret) {
+ dev_err(sdev->dma_dev.dev, "invalid step values\n");
+ return ret;
}

- 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;
- }
+ if (slave_cfg->config.slave_id)
+ schan->dev_id = slave_cfg->config.slave_id;

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) {
+ hw->wrap_ptr = (u32)((slave_cfg->wrap_ptr & SPRD_DMA_LOW_ADDR_MASK) |
+ ((slave_cfg->config.src_addr >> SPRD_DMA_HIGH_ADDR_OFFSET) &
+ SPRD_DMA_HIGH_ADDR_MASK));
+ hw->wrap_to = (u32)((slave_cfg->wrap_to & SPRD_DMA_LOW_ADDR_MASK) |
+ ((slave_cfg->config.dst_addr >> SPRD_DMA_HIGH_ADDR_OFFSET) &
+ SPRD_DMA_HIGH_ADDR_MASK));
+
+ hw->src_addr =
+ (u32)(slave_cfg->config.src_addr & SPRD_DMA_LOW_ADDR_MASK);
+ hw->des_addr =
+ (u32)(slave_cfg->config.dst_addr & SPRD_DMA_LOW_ADDR_MASK);
+
+ if ((src_step != 0 && dst_step != 0) || (src_step | dst_step) == 0) {
fix_en = 0;
} else {
fix_en = 1;
@@ -616,17 +729,37 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
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 |
+ if (slave_cfg->wrap_ptr && slave_cfg->wrap_to) {
+ wrap_en = 1;
+ if (slave_cfg->wrap_to == slave_cfg->config.src_addr) {
+ wrap_mode = 0;
+ } else if (slave_cfg->wrap_to == slave_cfg->config.dst_addr) {
+ wrap_mode = 1;
+ } else {
+ dev_err(sdev->dma_dev.dev, "invalid wrap mode\n");
+ return -EINVAL;
+ }
+ }
+
+ src_datawidth =
+ sprd_dma_get_datawidth(slave_cfg->config.src_addr_width);
+ dst_datawidth =
+ sprd_dma_get_datawidth(slave_cfg->config.dst_addr_width);
+
+ hw->frg_len = src_datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET |
+ dst_datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET |
+ slave_cfg->req_mode << SPRD_DMA_REQ_MODE_OFFSET |
+ wrap_mode << SPRD_DMA_WRAP_SEL_OFFSET |
+ wrap_en << SPRD_DMA_WRAP_EN_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;
+ (slave_cfg->fragment_len & SPRD_DMA_FRG_LEN_MASK);
+
+ hw->blk_len = slave_cfg->block_len & SPRD_DMA_BLK_LEN_MASK;

hw->intc = SPRD_DMA_CFG_ERR_INT_EN;

- switch (irq_mode) {
+ switch (slave_cfg->int_mode) {
case SPRD_DMA_NO_INT:
break;

@@ -667,12 +800,13 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
return -EINVAL;
}

- if (transcation_len == 0)
- hw->trsc_len = block_len & SPRD_DMA_TRSC_LEN_MASK;
+ if (slave_cfg->transcation_len == 0)
+ hw->trsc_len = slave_cfg->block_len & SPRD_DMA_TRSC_LEN_MASK;
else
- hw->trsc_len = transcation_len & SPRD_DMA_TRSC_LEN_MASK;
+ hw->trsc_len =
+ slave_cfg->transcation_len & SPRD_DMA_TRSC_LEN_MASK;

- hw->trsf_step = (des_step & SPRD_DMA_TRSF_STEP_MASK) <<
+ hw->trsf_step = (dst_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;
@@ -680,7 +814,6 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
hw->frg_step = 0;
hw->src_blk_step = 0;
hw->des_blk_step = 0;
- hw->src_blk_step = 0;
return 0;
}

@@ -689,6 +822,7 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
size_t len, unsigned long flags)
{
struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
+ struct sprd_dma_config *slave_cfg = &schan->slave_cfg;
struct sprd_dma_desc *sdesc;
int ret;

@@ -696,7 +830,37 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
if (!sdesc)
return NULL;

- ret = sprd_dma_config(chan, sdesc, dest, src, len);
+ memset(slave_cfg, 0, sizeof(*slave_cfg));
+
+ slave_cfg->config.src_addr = src;
+ slave_cfg->config.dst_addr = dest;
+ slave_cfg->config.direction = DMA_MEM_TO_MEM;
+
+ if (IS_ALIGNED(len, 4)) {
+ slave_cfg->config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ slave_cfg->config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ } else if (IS_ALIGNED(len, 2)) {
+ slave_cfg->config.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ slave_cfg->config.dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ } else {
+ slave_cfg->config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ slave_cfg->config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ }
+
+ slave_cfg->fragment_len = SPRD_DMA_MEMCPY_MIN_SIZE;
+ if (len <= SPRD_DMA_BLK_LEN_MASK) {
+ slave_cfg->block_len = len;
+ slave_cfg->transcation_len = 0;
+ slave_cfg->req_mode = SPRD_DMA_BLK_REQ;
+ slave_cfg->int_mode = SPRD_DMA_BLK_INT;
+ } else {
+ slave_cfg->block_len = SPRD_DMA_MEMCPY_MIN_SIZE;
+ slave_cfg->transcation_len = len;
+ slave_cfg->req_mode = SPRD_DMA_TRANS_REQ;
+ slave_cfg->int_mode = SPRD_DMA_TRANS_INT;
+ }
+
+ ret = sprd_dma_config(chan, sdesc, slave_cfg);
if (ret) {
kfree(sdesc);
return NULL;
diff --git a/include/linux/dma/sprd-dma.h b/include/linux/dma/sprd-dma.h
index c545162..8bda7d7 100644
--- a/include/linux/dma/sprd-dma.h
+++ b/include/linux/dma/sprd-dma.h
@@ -3,6 +3,8 @@
#ifndef _SPRD_DMA_H_
#define _SPRD_DMA_H_

+#include <linux/dmaengine.h>
+
/*
* enum sprd_dma_req_mode: define the DMA request mode
* @SPRD_DMA_FRAG_REQ: fragment request mode
@@ -54,4 +56,27 @@ enum sprd_dma_int_type {
SPRD_DMA_CFGERR_INT,
};

+/*
+ * struct sprd_dma_config - DMA configuration structure
+ * @config: dma slave channel config
+ * @fragment_len: specify one fragment transfer length
+ * @block_len: specify one block transfer length
+ * @transcation_len: specify one transcation transfer length
+ * @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 config;
+ u32 fragment_len;
+ u32 block_len;
+ u32 transcation_len;
+ phys_addr_t wrap_ptr;
+ phys_addr_t wrap_to;
+ enum sprd_dma_req_mode req_mode;
+ enum sprd_dma_int_type int_mode;
+};
+
#endif
--
1.7.9.5

2018-04-10 07:50:36

by Baolin Wang

Subject: [PATCH 5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces

This patch adds the 'device_config' and 'device_prep_slave_sg' interfaces
for users to configure DMA.

Signed-off-by: Baolin Wang <[email protected]>
---
drivers/dma/sprd-dma.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index f8038de..c923fb0 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -869,6 +869,52 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
return vchan_tx_prep(&schan->vc, &sdesc->vd, flags);
}

+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;
+ 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(slave_cfg->config.direction) || sglen > 1)
+ return NULL;
+
+ sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
+ if (!sdesc)
+ return NULL;
+
+ for_each_sg(sgl, sg, sglen, i) {
+ if (slave_cfg->config.direction == DMA_MEM_TO_DEV)
+ slave_cfg->config.src_addr = sg_dma_address(sg);
+ else
+ slave_cfg->config.dst_addr = sg_dma_address(sg);
+ }
+
+ ret = sprd_dma_config(chan, sdesc, 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 =
+ container_of(config, struct sprd_dma_config, config);
+
+ memcpy(&schan->slave_cfg, slave_cfg, sizeof(*slave_cfg));
+ return 0;
+}
+
static int sprd_dma_pause(struct dma_chan *chan)
{
struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
@@ -996,6 +1042,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;
--
1.7.9.5

2018-04-10 07:53:23

by Baolin Wang

Subject: [PATCH 3/5] dmaengine: sprd: Move DMA request mode and interrupt type into head file

From: Eric Long <[email protected]>

This patch will move the Spreadtrum DMA request mode and interrupt type
into one head file for user to configure. And other special SPRD DMA
configurations will be added in the following patches.

Signed-off-by: Eric Long <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/dma/sprd-dma.c | 52 +-------------------------------------
include/linux/dma/sprd-dma.h | 57 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 51 deletions(-)
create mode 100644 include/linux/dma/sprd-dma.h

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 9f44d6f..5c26fde 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -6,6 +6,7 @@

#include <linux/clk.h>
#include <linux/dma-mapping.h>
+#include <linux/dma/sprd-dma.h>
#include <linux/errno.h>
#include <linux/init.h>
#include <linux/interrupt.h>
@@ -119,57 +120,6 @@
#define SPRD_DMA_SOFTWARE_UID 0

/*
- * enum sprd_dma_req_mode: define the DMA request mode
- * @SPRD_DMA_FRAG_REQ: fragment request mode
- * @SPRD_DMA_BLK_REQ: block request mode
- * @SPRD_DMA_TRANS_REQ: transaction request mode
- * @SPRD_DMA_LIST_REQ: link-list request mode
- *
- * We have 4 types request mode: fragment mode, block mode, transaction mode
- * and linklist mode. One transaction can contain several blocks, one block can
- * contain several fragments. Link-list mode means we can save several DMA
- * configuration into one reserved memory, then DMA can fetch each DMA
- * configuration automatically to start transfer.
- */
-enum sprd_dma_req_mode {
- SPRD_DMA_FRAG_REQ,
- SPRD_DMA_BLK_REQ,
- SPRD_DMA_TRANS_REQ,
- SPRD_DMA_LIST_REQ,
-};
-
-/*
- * enum sprd_dma_int_type: define the DMA interrupt type
- * @SPRD_DMA_NO_INT: do not need generate DMA interrupts.
- * @SPRD_DMA_FRAG_INT: fragment done interrupt when one fragment request
- * is done.
- * @SPRD_DMA_BLK_INT: block done interrupt when one block request is done.
- * @SPRD_DMA_BLK_FRAG_INT: block and fragment interrupt when one fragment
- * or one block request is done.
- * @SPRD_DMA_TRANS_INT: tansaction done interrupt when one transaction
- * request is done.
- * @SPRD_DMA_TRANS_FRAG_INT: transaction and fragment interrupt when one
- * transaction request or fragment request is done.
- * @SPRD_DMA_TRANS_BLK_INT: transaction and block interrupt when one
- * transaction request or block request is done.
- * @SPRD_DMA_LIST_INT: link-list done interrupt when one link-list request
- * is done.
- * @SPRD_DMA_CFGERR_INT: configure error interrupt when configuration is
- * incorrect.
- */
-enum sprd_dma_int_type {
- SPRD_DMA_NO_INT,
- SPRD_DMA_FRAG_INT,
- SPRD_DMA_BLK_INT,
- SPRD_DMA_BLK_FRAG_INT,
- SPRD_DMA_TRANS_INT,
- SPRD_DMA_TRANS_FRAG_INT,
- SPRD_DMA_TRANS_BLK_INT,
- SPRD_DMA_LIST_INT,
- SPRD_DMA_CFGERR_INT,
-};
-
-/*
* enum sprd_dma_step: define the DMA transfer step type
* @SPRD_DMA_NONE_STEP: transfer do not need step
* @SPRD_DMA_BYTE_STEP: transfer step is one byte
diff --git a/include/linux/dma/sprd-dma.h b/include/linux/dma/sprd-dma.h
new file mode 100644
index 0000000..c545162
--- /dev/null
+++ b/include/linux/dma/sprd-dma.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _SPRD_DMA_H_
+#define _SPRD_DMA_H_
+
+/*
+ * enum sprd_dma_req_mode: define the DMA request mode
+ * @SPRD_DMA_FRAG_REQ: fragment request mode
+ * @SPRD_DMA_BLK_REQ: block request mode
+ * @SPRD_DMA_TRANS_REQ: transaction request mode
+ * @SPRD_DMA_LIST_REQ: link-list request mode
+ *
+ * We have 4 types request mode: fragment mode, block mode, transaction mode
+ * and linklist mode. One transaction can contain several blocks, one block can
+ * contain several fragments. Link-list mode means we can save several DMA
+ * configuration into one reserved memory, then DMA can fetch each DMA
+ * configuration automatically to start transfer.
+ */
+enum sprd_dma_req_mode {
+ SPRD_DMA_FRAG_REQ,
+ SPRD_DMA_BLK_REQ,
+ SPRD_DMA_TRANS_REQ,
+ SPRD_DMA_LIST_REQ,
+};
+
+/*
+ * enum sprd_dma_int_type: define the DMA interrupt type
+ * @SPRD_DMA_NO_INT: do not need generate DMA interrupts.
+ * @SPRD_DMA_FRAG_INT: fragment done interrupt when one fragment request
+ * is done.
+ * @SPRD_DMA_BLK_INT: block done interrupt when one block request is done.
+ * @SPRD_DMA_BLK_FRAG_INT: block and fragment interrupt when one fragment
+ * or one block request is done.
+ * @SPRD_DMA_TRANS_INT: tansaction done interrupt when one transaction
+ * request is done.
+ * @SPRD_DMA_TRANS_FRAG_INT: transaction and fragment interrupt when one
+ * transaction request or fragment request is done.
+ * @SPRD_DMA_TRANS_BLK_INT: transaction and block interrupt when one
+ * transaction request or block request is done.
+ * @SPRD_DMA_LIST_INT: link-list done interrupt when one link-list request
+ * is done.
+ * @SPRD_DMA_CFGERR_INT: configure error interrupt when configuration is
+ * incorrect.
+ */
+enum sprd_dma_int_type {
+ SPRD_DMA_NO_INT,
+ SPRD_DMA_FRAG_INT,
+ SPRD_DMA_BLK_INT,
+ SPRD_DMA_BLK_FRAG_INT,
+ SPRD_DMA_TRANS_INT,
+ SPRD_DMA_TRANS_FRAG_INT,
+ SPRD_DMA_TRANS_BLK_INT,
+ SPRD_DMA_LIST_INT,
+ SPRD_DMA_CFGERR_INT,
+};
+
+#endif
--
1.7.9.5

2018-04-10 07:53:23

by Baolin Wang

Subject: [PATCH 2/5] dmaengine: sprd: Define the DMA data width type

Define the DMA data width type to make code more readable.

Signed-off-by: Baolin Wang <[email protected]>
---
drivers/dma/sprd-dma.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index af9c156..9f44d6f 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -185,6 +185,14 @@ enum sprd_dma_step {
SPRD_DMA_DWORD_STEP = BIT(3),
};

+/* dma data width values */
+enum sprd_dma_datawidth {
+ SPRD_DMA_DATAWIDTH_1_BYTE,
+ SPRD_DMA_DATAWIDTH_2_BYTES,
+ SPRD_DMA_DATAWIDTH_4_BYTES,
+ SPRD_DMA_DATAWIDTH_8_BYTES,
+};
+
/* dma channel hardware configuration */
struct sprd_dma_chn_hw {
u32 pause;
@@ -613,15 +621,15 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
u32 fix_mode = 0, fix_en = 0;

if (IS_ALIGNED(len, 4)) {
- datawidth = 2;
+ 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 = 1;
+ datawidth = SPRD_DMA_DATAWIDTH_2_BYTES;
src_step = SPRD_DMA_SHORT_STEP;
des_step = SPRD_DMA_SHORT_STEP;
} else {
- datawidth = 0;
+ datawidth = SPRD_DMA_DATAWIDTH_1_BYTE;
src_step = SPRD_DMA_BYTE_STEP;
des_step = SPRD_DMA_BYTE_STEP;
}
--
1.7.9.5

2018-04-11 09:23:53

by Vinod Koul

Subject: Re: [PATCH 1/5] dmaengine: sprd: Define the DMA transfer step type

On Tue, Apr 10, 2018 at 03:46:03PM +0800, Baolin Wang wrote:
> From: Eric Long <[email protected]>
>
> Define the DMA transfer step type to make code more readable.
>
> Signed-off-by: Eric Long <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> drivers/dma/sprd-dma.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index b106e8a..af9c156 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -169,6 +169,22 @@ enum sprd_dma_int_type {
> SPRD_DMA_CFGERR_INT,
> };
>
> +/*
> + * enum sprd_dma_step: define the DMA transfer step type
> + * @SPRD_DMA_NONE_STEP: transfer do not need step
> + * @SPRD_DMA_BYTE_STEP: transfer step is one byte
> + * @SPRD_DMA_SHORT_STEP: transfer step is two bytes
> + * @SPRD_DMA_WORD_STEP: transfer step is one word
> + * @SPRD_DMA_DWORD_STEP: transfer step is double word
> + */
> +enum sprd_dma_step {

looking at bit defines, does it need to be enum? a #define would just be fine.

> + SPRD_DMA_NONE_STEP,
> + SPRD_DMA_BYTE_STEP = BIT(0),
> + SPRD_DMA_SHORT_STEP = BIT(1),
> + SPRD_DMA_WORD_STEP = BIT(2),
> + SPRD_DMA_DWORD_STEP = BIT(3),
> +};
> +
> /* dma channel hardware configuration */
> struct sprd_dma_chn_hw {
> u32 pause;
> @@ -598,16 +614,16 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
>
> if (IS_ALIGNED(len, 4)) {
> datawidth = 2;
> - src_step = 4;
> - des_step = 4;
> + src_step = SPRD_DMA_WORD_STEP;
> + des_step = SPRD_DMA_WORD_STEP;
> } else if (IS_ALIGNED(len, 2)) {
> datawidth = 1;
> - src_step = 2;
> - des_step = 2;
> + src_step = SPRD_DMA_SHORT_STEP;
> + des_step = SPRD_DMA_SHORT_STEP;
> } else {
> datawidth = 0;
> - src_step = 1;
> - des_step = 1;
> + src_step = SPRD_DMA_BYTE_STEP;
> + des_step = SPRD_DMA_BYTE_STEP;
> }
>
> fragment_len = SPRD_DMA_MEMCPY_MIN_SIZE;
> --
> 1.7.9.5
>

--
~Vinod

2018-04-11 09:38:55

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Tue, Apr 10, 2018 at 03:46:06PM +0800, Baolin Wang wrote:

> +/*
> + * struct sprd_dma_config - DMA configuration structure
> + * @config: dma slave channel config
> + * @fragment_len: specify one fragment transfer length
> + * @block_len: specify one block transfer length
> + * @transcation_len: specify one transcation transfer length
> + * @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 config;
> + u32 fragment_len;

why not use _maxburst?

> + u32 block_len;
> + u32 transcation_len;

what does block and transaction len refer to here

> + phys_addr_t wrap_ptr;
> + phys_addr_t wrap_to;

this sound sg_list to me, why are we not using that here

> + enum sprd_dma_req_mode req_mode;

Looking at definition of request mode we have frag, block, transaction list
etc.. That should depend upon dma request. If you have been asked to
transfer a list, you shall configure list mode. if it is a single
transaction then it should be transaction mode!

> + enum sprd_dma_int_type int_mode;

Here again I think driver needs to take a call based on dma_ctrl_flags.

Okay I dont think we are proceeding in right direction on this one. This
seems to be fairly generic dma controller and in line with other IP blocks
and you should take reference from those one. I dont think we need this
configuration and can do with generic api and configuration provided.

Thanks
--
~Vinod

2018-04-11 09:39:32

by Vinod Koul

Subject: Re: [PATCH 5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces

On Tue, Apr 10, 2018 at 03:46:07PM +0800, Baolin Wang wrote:
> This patch adds the 'device_config' and 'device_prep_slave_sg' interfaces
> for users to configure DMA.
>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> drivers/dma/sprd-dma.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index f8038de..c923fb0 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -869,6 +869,52 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
> return vchan_tx_prep(&schan->vc, &sdesc->vd, flags);
> }
>
> +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;
> + 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(slave_cfg->config.direction) || sglen > 1)

the slave direction check seems wrong to me. .device_config shall give you
dma_slave_config. You should check here if dir passed is slave or not. If
you want to check parameters in slave_config then please use .device_config

> + return NULL;
> +
> + sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> + if (!sdesc)
> + return NULL;
> +
> + for_each_sg(sgl, sg, sglen, i) {
> + if (slave_cfg->config.direction == DMA_MEM_TO_DEV)
> + slave_cfg->config.src_addr = sg_dma_address(sg);

Nope slave_config specifies peripheral address and not memory one passed here

--
~Vinod

2018-04-11 10:54:11

by Baolin Wang

Subject: Re: [PATCH 1/5] dmaengine: sprd: Define the DMA transfer step type

Hi Vinod,

On 11 April 2018 at 17:24, Vinod Koul <[email protected]> wrote:
> On Tue, Apr 10, 2018 at 03:46:03PM +0800, Baolin Wang wrote:
>> From: Eric Long <[email protected]>
>>
>> Define the DMA transfer step type to make code more readable.
>>
>> Signed-off-by: Eric Long <[email protected]>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> drivers/dma/sprd-dma.c | 28 ++++++++++++++++++++++------
>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
>> index b106e8a..af9c156 100644
>> --- a/drivers/dma/sprd-dma.c
>> +++ b/drivers/dma/sprd-dma.c
>> @@ -169,6 +169,22 @@ enum sprd_dma_int_type {
>> SPRD_DMA_CFGERR_INT,
>> };
>>
>> +/*
>> + * enum sprd_dma_step: define the DMA transfer step type
>> + * @SPRD_DMA_NONE_STEP: transfer do not need step
>> + * @SPRD_DMA_BYTE_STEP: transfer step is one byte
>> + * @SPRD_DMA_SHORT_STEP: transfer step is two bytes
>> + * @SPRD_DMA_WORD_STEP: transfer step is one word
>> + * @SPRD_DMA_DWORD_STEP: transfer step is double word
>> + */
>> +enum sprd_dma_step {
>
> looking at bit defines, does it need to be enum? a #define would just be fine.

Right. It seems more clear if we combined them into one enum
structure, but I can convert to #define if you have strong preference.
Thanks.

--
Baolin.wang
Best Regards

2018-04-11 10:55:11

by Baolin Wang

Subject: Re: [PATCH 5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces

Hi Vinod,

On 11 April 2018 at 17:40, Vinod Koul <[email protected]> wrote:
> On Tue, Apr 10, 2018 at 03:46:07PM +0800, Baolin Wang wrote:
>> This patch adds the 'device_config' and 'device_prep_slave_sg' interfaces
>> for users to configure DMA.
>>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> drivers/dma/sprd-dma.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
>> index f8038de..c923fb0 100644
>> --- a/drivers/dma/sprd-dma.c
>> +++ b/drivers/dma/sprd-dma.c
>> @@ -869,6 +869,52 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc,
>> return vchan_tx_prep(&schan->vc, &sdesc->vd, flags);
>> }
>>
>> +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;
>> + 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(slave_cfg->config.direction) || sglen > 1)
>
> the slave direction check seems wrong to me. .device_config shall give you
> dma_slave_config. You should check here if dir passed is slave or not. If
> you want to check parameters in slave_config then please use .device_config

Correct. Sorry I missed this and I will fix it in next version.

>
>> + return NULL;
>> +
>> + sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
>> + if (!sdesc)
>> + return NULL;
>> +
>> + for_each_sg(sgl, sg, sglen, i) {
>> + if (slave_cfg->config.direction == DMA_MEM_TO_DEV)
>> + slave_cfg->config.src_addr = sg_dma_address(sg);
>
> Nope slave_config specifies peripheral address and not memory one passed here

OK. Thanks for your comments.

--
Baolin.wang
Best Regards

2018-04-11 12:20:09

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

Hi Vinod,

On 11 April 2018 at 17:36, Vinod Koul <[email protected]> wrote:
> On Tue, Apr 10, 2018 at 03:46:06PM +0800, Baolin Wang wrote:
>
>> +/*
>> + * struct sprd_dma_config - DMA configuration structure
>> + * @config: dma slave channel config
>> + * @fragment_len: specify one fragment transfer length
>> + * @block_len: specify one block transfer length
>> + * @transcation_len: specify one transcation transfer length
>> + * @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 config;
>> + u32 fragment_len;
>
> why not use _maxburst?

Yes, I can use maxburst.

>
>> + u32 block_len;
>> + u32 transcation_len;
>
> what does block and transaction len refer to here

Our DMA has 3 transfer mode: transaction transfer, block transfer and
fragment transfer. One transaction transfer can contain several blocks
transfer, and each block can be set proper block step. One block can
contain several fragments transfer with proper fragment step. It can
generate interrupts when one transaction transfer or block transfer or
fragment transfer is completed if user set the interrupt type. So here
we should set the length for transaction transfer, block transfer and
fragment transfer.

>
>> + phys_addr_t wrap_ptr;
>> + phys_addr_t wrap_to;
>
> this sound sg_list to me, why are we not using that here

It is similar to sg list, but it is not one software action, we have
hardware registers to help to jump one specified address.

>
>> + enum sprd_dma_req_mode req_mode;
>
> Looking at definition of request mode we have frag, block, transaction list
> etc.. That should depend upon dma request. If you have been asked to
> transfer a list, you shall configure list mode. if it is a single
> transaction then it should be transaction mode!

If I understand your points correctly, you mean we can specify the
request mode when requesting one slave channel by
'dma_request_slave_channel()'. But we need change the request mode
dynamically following different transfer task for this channel, so I
am afraid we can not specify the request mode of this channel at
requesting time.

>
>> + enum sprd_dma_int_type int_mode;
>
> Here again I think driver needs to take a call based on dma_ctrl_flags.

The 'dma_ctrl_flags' defines DMA_PREP_INTERRUPT flag to indicate if a
interrupt is needed after transfer, but it can not distinguish
Spreadtrum special interrupt type of DMA. So can I pass the interrupt
type as one parameter for 'device_prep_slave_sg' interface?

Very appreciated for your useful comments.

--
Baolin.wang
Best Regards

2018-04-12 09:36:35

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Wed, Apr 11, 2018 at 08:13:28PM +0800, Baolin Wang wrote:
> Hi Vinod,
>
> On 11 April 2018 at 17:36, Vinod Koul <[email protected]> wrote:
> > On Tue, Apr 10, 2018 at 03:46:06PM +0800, Baolin Wang wrote:
> >
> >> +/*
> >> + * struct sprd_dma_config - DMA configuration structure
> >> + * @config: dma slave channel config
> >> + * @fragment_len: specify one fragment transfer length
> >> + * @block_len: specify one block transfer length
> >> + * @transcation_len: specify one transcation transfer length
> >> + * @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 config;
> >> + u32 fragment_len;
> >
> > why not use _maxburst?
>
> Yes, I can use maxburst.
>
> >
> >> + u32 block_len;
> >> + u32 transcation_len;
> >
> > what does block and transaction len refer to here
>
> Our DMA has 3 transfer mode: transaction transfer, block transfer and
> fragment transfer. One transaction transfer can contain several blocks
> transfer, and each block can be set proper block step. One block can
> contain several fragments transfer with proper fragment step. It can
> generate interrupts when one transaction transfer or block transfer or
> fragment transfer is completed if user set the interrupt type. So here
> we should set the length for transaction transfer, block transfer and
> fragment transfer.

what are the max size these types support?

>
> >
> >> + phys_addr_t wrap_ptr;
> >> + phys_addr_t wrap_to;
> >
> > this sound sg_list to me, why are we not using that here
>
> It is similar to sg list, but it is not one software action, we have
> hardware registers to help to jump one specified address.
>
> >
> >> + enum sprd_dma_req_mode req_mode;
> >
> > Looking at definition of request mode we have frag, block, transaction list
> > etc.. That should depend upon dma request. If you have been asked to
> > transfer a list, you shall configure list mode. if it is a single
> > transaction then it should be transaction mode!
>
> If I understand your points correctly, you mean we can specify the
> request mode when requesting one slave channel by
> 'dma_request_slave_channel()'. But we need change the request mode
> dynamically following different transfer task for this channel, so I
> am afraid we can not specify the request mode of this channel at
> requesting time.

Nope a channel has nothing to do with request type. You request and grab a
channel. Then you prepare a descriptor for a dma transaction. Based on
transaction requested you should intelligently break it down and create a
descriptor which uses transaction/block/fragment so that DMA throughput is
efficient. If prepare has sg list then you should use link list mode.
Further if you support max length, say 16KB and request is for 20KB you may
break it down for link list with two segments.

Each prep call has flags associated, that can help you configure interrupt
behaviour.

Btw other dma controllers have similar capabilities and driver manages these
:)

--
~Vinod

2018-04-12 11:33:36

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

Hi Vinod,

On 12 April 2018 at 17:37, Vinod Koul <[email protected]> wrote:
> On Wed, Apr 11, 2018 at 08:13:28PM +0800, Baolin Wang wrote:
>> Hi Vinod,
>>
>> On 11 April 2018 at 17:36, Vinod Koul <[email protected]> wrote:
>> > On Tue, Apr 10, 2018 at 03:46:06PM +0800, Baolin Wang wrote:
>> >
>> >> +/*
>> >> + * struct sprd_dma_config - DMA configuration structure
>> >> + * @config: dma slave channel config
>> >> + * @fragment_len: specify one fragment transfer length
>> >> + * @block_len: specify one block transfer length
>> >> + * @transcation_len: specify one transcation transfer length
>> >> + * @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 config;
>> >> + u32 fragment_len;
>> >
>> > why not use _maxburst?
>>
>> Yes, I can use maxburst.
>>
>> >
>> >> + u32 block_len;
>> >> + u32 transcation_len;
>> >
>> > what does block and transaction len refer to here
>>
>> Our DMA has 3 transfer mode: transaction transfer, block transfer and
>> fragment transfer. One transaction transfer can contain several blocks
>> transfer, and each block can be set proper block step. One block can
>> contain several fragments transfer with proper fragment step. It can
>> generate interrupts when one transaction transfer or block transfer or
>> fragment transfer is completed if user set the interrupt type. So here
>> we should set the length for transaction transfer, block transfer and
>> fragment transfer.
>
> what are the max size these types support?

These types max size definition:

#define SPRD_DMA_FRG_LEN_MASK GENMASK(16, 0)

#define SPRD_DMA_BLK_LEN_MASK GENMASK(16, 0)

#define SPRD_DMA_TRSC_LEN_MASK GENMASK(27, 0)

>>
>> >
>> >> + phys_addr_t wrap_ptr;
>> >> + phys_addr_t wrap_to;
>> >
>> > this sound sg_list to me, why are we not using that here
>>
>> It is similar to sg list, but it is not one software action, we have
>> hardware registers to help to jump one specified address.
>>
>> >
>> >> + enum sprd_dma_req_mode req_mode;
>> >
>> > Looking at definition of request mode we have frag, block, transaction list
>> > etc.. That should depend upon dma request. If you have been asked to
>> > transfer a list, you shall configure list mode. if it is a single
>> > transaction then it should be transaction mode!
>>
>> If I understand your points correctly, you mean we can specify the
>> request mode when requesting one slave channel by
>> 'dma_request_slave_channel()'. But we need change the request mode
>> dynamically following different transfer task for this channel, so I
>> am afraid we can not specify the request mode of this channel at
>> requesting time.
>
> Nope a channel has nothing to do with request type. You request and grab a
> channel. Then you prepare a descriptor for a dma transaction. Based on
> transaction requested you should intelligently break it down and create a
> descriptor which uses transaction/block/fragment so that DMA throughput is
> efficient. If prepare has sg list then you should use link list mode.
> Further if you support max length, say 16KB and request is for 20KB you may
> break it down for link list with two segments.

OK. So I can add one more cell to specify the request mode for this channel.

dmas = <&apdma 11 SPRD_DMA_BLK_REQ>

>
> Each prep call has flags associated, that can help you configure interrupt
> behaviour.

Sounds reasonable. Thanks for your comments.

--
Baolin.wang
Best Regards

2018-04-12 11:40:18

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On 12 April 2018 at 19:30, Baolin Wang <[email protected]> wrote:
> Hi Vinod,
>
> On 12 April 2018 at 17:37, Vinod Koul <[email protected]> wrote:
>> On Wed, Apr 11, 2018 at 08:13:28PM +0800, Baolin Wang wrote:
>>> Hi Vinod,
>>>
>>> On 11 April 2018 at 17:36, Vinod Koul <[email protected]> wrote:
>>> > On Tue, Apr 10, 2018 at 03:46:06PM +0800, Baolin Wang wrote:
>>> >
>>> >> +/*
>>> >> + * struct sprd_dma_config - DMA configuration structure
>>> >> + * @config: dma slave channel config
>>> >> + * @fragment_len: specify one fragment transfer length
>>> >> + * @block_len: specify one block transfer length
>>> >> + * @transcation_len: specify one transcation transfer length
>>> >> + * @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 config;
>>> >> + u32 fragment_len;
>>> >
>>> > why not use _maxburst?
>>>
>>> Yes, I can use maxburst.
>>>
>>> >
>>> >> + u32 block_len;
>>> >> + u32 transcation_len;
>>> >
>>> > what does block and transaction len refer to here
>>>
>>> Our DMA has 3 transfer mode: transaction transfer, block transfer and
>>> fragment transfer. One transaction transfer can contain several blocks
>>> transfer, and each block can be set proper block step. One block can
>>> contain several fragments transfer with proper fragment step. It can
>>> generate interrupts when one transaction transfer or block transfer or
>>> fragment transfer is completed if user set the interrupt type. So here
>>> we should set the length for transaction transfer, block transfer and
>>> fragment transfer.
>>
>> what are the max size these types support?
>
> These types max size definition:
>
> #define SPRD_DMA_FRG_LEN_MASK GENMASK(16, 0)
>
> #define SPRD_DMA_BLK_LEN_MASK GENMASK(16, 0)
>
> #define SPRD_DMA_TRSC_LEN_MASK GENMASK(27, 0)
>
>>>
>>> >
>>> >> + phys_addr_t wrap_ptr;
>>> >> + phys_addr_t wrap_to;
>>> >
>>> > this sound sg_list to me, why are we not using that here
>>>
>>> It is similar to sg list, but it is not one software action, we have
>>> hardware registers to help to jump one specified address.
>>>
>>> >
>>> >> + enum sprd_dma_req_mode req_mode;
>>> >
>>> > Looking at definition of request mode we have frag, block, transaction list
>>> > etc.. That should depend upon dma request. If you have been asked to
>>> > transfer a list, you shall configure list mode. if it is a single
>>> > transaction then it should be transaction mode!
>>>
>>> If I understand your points correctly, you mean we can specify the
>>> request mode when requesting one slave channel by
>>> 'dma_request_slave_channel()'. But we need change the request mode
>>> dynamically following different transfer task for this channel, so I
>>> am afraid we can not specify the request mode of this channel at
>>> requesting time.
>>
>> Nope a channel has nothing to do with request type. You request and grab a
>> channel. Then you prepare a descriptor for a dma transaction. Based on
>> transaction requested you should intelligently break it down and create a
>> descriptor which uses transaction/block/fragment so that DMA throughput is
>> efficient. If prepare has sg list then you should use link list mode.
>> Further if you support max length, say 16KB and request is for 20KB you may
>> break it down for link list with two segments.
>
> OK. So I can add one more cell to specify the request mode for this channel.
>
> dmas = <&apdma 11 SPRD_DMA_BLK_REQ>
>
>>
>> Each prep call has flags associated, that can help you configure interrupt
>> behaviour.

After more thinking, I think this will be not useful for users, since
users need configure one DMA channel through different 3 places,
specify the request mode in dts, specify the interrupt type through
prep call flags, and other configuration need to be configured through
'struct sprd_dma_config'. That is not one good design for users. What
do you think? Thanks.

--
Baolin.wang
Best Regards

2018-04-13 03:44:41

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Thu, Apr 12, 2018 at 07:30:01PM +0800, Baolin Wang wrote:

> >> > what does block and transaction len refer to here
> >>
> >> Our DMA has 3 transfer mode: transaction transfer, block transfer and
> >> fragment transfer. One transaction transfer can contain several blocks
> >> transfer, and each block can be set proper block step. One block can
> >> contain several fragments transfer with proper fragment step. It can
> >> generate interrupts when one transaction transfer or block transfer or
> >> fragment transfer is completed if user set the interrupt type. So here
> >> we should set the length for transaction transfer, block transfer and
> >> fragment transfer.
> >
> > what are the max size these types support?
>
> These types max size definition:
>
> #define SPRD_DMA_FRG_LEN_MASK GENMASK(16, 0)
>
> #define SPRD_DMA_BLK_LEN_MASK GENMASK(16, 0)
>
> #define SPRD_DMA_TRSC_LEN_MASK GENMASK(27, 0)

They are register defines. How many items or bytes do each type of txn
support?

--
~Vinod

2018-04-13 03:49:33

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Thu, Apr 12, 2018 at 07:36:34PM +0800, Baolin Wang wrote:
> >>> >> +/*
> >>> >> + * struct sprd_dma_config - DMA configuration structure
> >>> >> + * @config: dma slave channel config
> >>> >> + * @fragment_len: specify one fragment transfer length
> >>> >> + * @block_len: specify one block transfer length
> >>> >> + * @transcation_len: specify one transcation transfer length
> >>> >> + * @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 config;
> >>> >> + u32 fragment_len;
> >>> >
> >>> > why not use _maxburst?
> >>>
> >>> Yes, I can use maxburst.
> >>>
> >>> >
> >>> >> + u32 block_len;
> >>> >> + u32 transcation_len;
> >>> >
> >>> > what does block and transaction len refer to here
> >>>
> >>> Our DMA has 3 transfer mode: transaction transfer, block transfer and
> >>> fragment transfer. One transaction transfer can contain several blocks
> >>> transfer, and each block can be set proper block step. One block can
> >>> contain several fragments transfer with proper fragment step. It can
> >>> generate interrupts when one transaction transfer or block transfer or
> >>> fragment transfer is completed if user set the interrupt type. So here
> >>> we should set the length for transaction transfer, block transfer and
> >>> fragment transfer.
> >>
> >> what are the max size these types support?
> >
> > These types max size definition:
> >
> > #define SPRD_DMA_FRG_LEN_MASK GENMASK(16, 0)
> >
> > #define SPRD_DMA_BLK_LEN_MASK GENMASK(16, 0)
> >
> > #define SPRD_DMA_TRSC_LEN_MASK GENMASK(27, 0)
> >
> >>>
> >>> >
> >>> >> + phys_addr_t wrap_ptr;
> >>> >> + phys_addr_t wrap_to;
> >>> >
> >>> > this sound sg_list to me, why are we not using that here
> >>>
> >>> It is similar to sg list, but it is not one software action, we have
> >>> hardware registers to help to jump one specified address.
> >>>
> >>> >
> >>> >> + enum sprd_dma_req_mode req_mode;
> >>> >
> >>> > Looking at definition of request mode we have frag, block, transaction list
> >>> > etc.. That should depend upon dma request. If you have been asked to
> >>> > transfer a list, you shall configure list mode. if it is a single
> >>> > transaction then it should be transaction mode!
> >>>
> >>> If I understand your points correctly, you mean we can specify the
> >>> request mode when requesting one slave channel by
> >>> 'dma_request_slave_channel()'. But we need change the request mode
> >>> dynamically following different transfer task for this channel, so I
> >>> am afraid we can not specify the request mode of this channel at
> >>> requesting time.
> >>
> >> Nope a channel has nothing to do with request type. You request and grab a
> >> channel. Then you prepare a descriptor for a dma transaction. Based on
> >> transaction requested you should intelligently break it down and create a
> >> descriptor which uses transaction/block/fragment so that DMA throughput is
> >> efficient. If prepare has sg list then you should use link list mode.
> >> Further if you support max length, say 16KB and request is for 20KB you may
> >> break it down for link list with two segments.
> >
> > OK. So I can add one more cell to specify the request mode for this channel.
> >
> > dmas = <&apdma 11 SPRD_DMA_BLK_REQ>
> >
> >>
> >> Each prep call has flags associated, that can help you configure interrupt
> >> behaviour.
>
> After more thinking, I think this will be not useful for users, since
> users need configure one DMA channel through different 3 places,
> specify the request mode in dts, specify the interrupt type through
> prep call flags, and other configuration need to be configured through
> 'struct sprd_dma_config'. That is not one good design for users. What
> do you think? Thanks.

Agreed, users only care about grabbing a channel, setting a descriptor and
submitting that.

I think you need to go back and think about this a bit, please do go thru
dmaengine documentation and see other driver examples.

We don't typically expose these to users, they give us a transfer and we set
that up in hardware for efficient. Its DMA so people expect us to use fastest
mechanism available.

I would say setup as Link list for sg transfers and use one of them modes
(again think efficiency) for single transfers.

Also use all the parameters in dma_slave_config and also setup capabilities if
not done.

--
~Vinod

2018-04-13 05:47:50

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On 13 April 2018 at 11:39, Vinod Koul <[email protected]> wrote:
> On Thu, Apr 12, 2018 at 07:30:01PM +0800, Baolin Wang wrote:
>
>> >> > what does block and transaction len refer to here
>> >>
>> >> Our DMA has 3 transfer mode: transaction transfer, block transfer and
>> >> fragment transfer. One transaction transfer can contain several blocks
>> >> transfer, and each block can be set proper block step. One block can
>> >> contain several fragments transfer with proper fragment step. It can
>> >> generate interrupts when one transaction transfer or block transfer or
>> >> fragment transfer is completed if user set the interrupt type. So here
>> >> we should set the length for transaction transfer, block transfer and
>> >> fragment transfer.
>> >
>> > what are the max size these types support?
>>
>> These types max size definition:
>>
>> #define SPRD_DMA_FRG_LEN_MASK GENMASK(16, 0)
>>
>> #define SPRD_DMA_BLK_LEN_MASK GENMASK(16, 0)
>>
>> #define SPRD_DMA_TRSC_LEN_MASK GENMASK(27, 0)
>
> They are register defines. How many items or bytes do each type of txn
> support?

These macros are the max size definitions, for example one fragment
length can support to 0x1ffff bytes, one transaction transfer can
support to 0xfffffff.

--
Baolin.wang
Best Regards

2018-04-13 06:31:15

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On 13 April 2018 at 11:43, Vinod Koul <[email protected]> wrote:
> On Thu, Apr 12, 2018 at 07:36:34PM +0800, Baolin Wang wrote:
>> >>> >> +/*
>> >>> >> + * struct sprd_dma_config - DMA configuration structure
>> >>> >> + * @config: dma slave channel config
>> >>> >> + * @fragment_len: specify one fragment transfer length
>> >>> >> + * @block_len: specify one block transfer length
>> >>> >> + * @transcation_len: specify one transcation transfer length
>> >>> >> + * @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 config;
>> >>> >> + u32 fragment_len;
>> >>> >
>> >>> > why not use _maxburst?
>> >>>
>> >>> Yes, I can use maxburst.
>> >>>
>> >>> >
>> >>> >> + u32 block_len;
>> >>> >> + u32 transcation_len;
>> >>> >
>> >>> > what does block and transaction len refer to here
>> >>>
>> >>> Our DMA has 3 transfer mode: transaction transfer, block transfer and
>> >>> fragment transfer. One transaction transfer can contain several blocks
>> >>> transfer, and each block can be set proper block step. One block can
>> >>> contain several fragments transfer with proper fragment step. It can
>> >>> generate interrupts when one transaction transfer or block transfer or
>> >>> fragment transfer is completed if user set the interrupt type. So here
>> >>> we should set the length for transaction transfer, block transfer and
>> >>> fragment transfer.
>> >>
>> >> what are the max size these types support?
>> >
>> > These types max size definition:
>> >
>> > #define SPRD_DMA_FRG_LEN_MASK GENMASK(16, 0)
>> >
>> > #define SPRD_DMA_BLK_LEN_MASK GENMASK(16, 0)
>> >
>> > #define SPRD_DMA_TRSC_LEN_MASK GENMASK(27, 0)
>> >
>> >>>
>> >>> >
>> >>> >> + phys_addr_t wrap_ptr;
>> >>> >> + phys_addr_t wrap_to;
>> >>> >
>> >>> > this sound sg_list to me, why are we not using that here
>> >>>
>> >>> It is similar to sg list, but it is not one software action, we have
>> >>> hardware registers to help to jump one specified address.
>> >>>
>> >>> >
>> >>> >> + enum sprd_dma_req_mode req_mode;
>> >>> >
>> >>> > Looking at definition of request mode we have frag, block, transaction list
>> >>> > etc.. That should depend upon dma request. If you have been asked to
>> >>> > transfer a list, you shall configure list mode. if it is a single
>> >>> > transaction then it should be transaction mode!
>> >>>
>> >>> If I understand your points correctly, you mean we can specify the
>> >>> request mode when requesting one slave channel by
>> >>> 'dma_request_slave_channel()'. But we need change the request mode
>> >>> dynamically following different transfer task for this channel, so I
>> >>> am afraid we can not specify the request mode of this channel at
>> >>> requesting time.
>> >>
>> >> Nope a channel has nothing to do with request type. You request and grab a
>> >> channel. Then you prepare a descriptor for a dma transaction. Based on
>> >> transaction requested you should intelligently break it down and create a
>> >> descriptor which uses transaction/block/fragment so that DMA throughput is
>> >> efficient. If prepare has sg list then you should use link list mode.
>> >> Further if you support max length, say 16KB and request is for 20KB you may
>> >> break it down for link list with two segments.
>> >
>> > OK. So I can add one more cell to specify the request mode for this channel.
>> >
>> > dmas = <&apdma 11 SPRD_DMA_BLK_REQ>
>> >
>> >>
>> >> Each prep call has flags associated, that can help you configure interrupt
>> >> behaviour.
>>
>> After more thinking, I think this will be not useful for users, since
>> users need configure one DMA channel through different 3 places,
>> specify the request mode in dts, specify the interrupt type through
>> prep call flags, and other configuration need to be configured through
>> 'struct sprd_dma_config'. That is not one good design for users. What
>> do you think? Thanks.
>
> Agreed, users only care about grabbing a channel, setting a descriptor and
> submitting that.
>
> I think you need to go back and think about this a bit, please do go thru
> dmaengine documentation and see other driver examples.
>
> We don't typically expose these to users, they give us a transfer and we set
> that up in hardware for efficient. Its DMA so people expect us to use fastest
> mechanism available.

But there are some configuration are really special for Spreadtrum
DMA, and must need user to specify how to configure, especially some
scenarios of audio. So I wander if we can add one pointer for
'dma_slave_config' to expand some special DMA configuration
requirements, like:

struct dma_slave_config {
......
unsigned int slave_id;
void *platform_data;
};

So if some DMA has some special configuration (such as Spreadtrum
DMA), they can user this platform_data pointer. Like xilinx DMA, they
also have some special configuration.

>
> I would say setup as Link list for sg transfers and use one of them modes
> (again think efficiency) for single transfers.
>
> Also use all the parameters in dma_slave_config and also setup capabilities if
> not done.

--
Baolin.wang
Best Regards

2018-04-13 06:44:26

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:

> > Agreed, users only care about grabbing a channel, setting a descriptor and
> > submitting that.
> >
> > I think you need to go back and think about this a bit, please do go thru
> > dmaengine documentation and see other driver examples.
> >
> > We don't typically expose these to users, they give us a transfer and we set
> > that up in hardware for efficient. Its DMA so people expect us to use fastest
> > mechanism available.
>
> But there are some configuration are really special for Spreadtrum
> DMA, and must need user to specify how to configure, especially some
> scenarios of audio. So I wander if we can add one pointer for
> 'dma_slave_config' to expand some special DMA configuration
> requirements, like:
>
> struct dma_slave_config {
> ......
> unsigned int slave_id;
> void *platform_data;
> };
>
> So if some DMA has some special configuration (such as Spreadtrum
> DMA), they can user this platform_data pointer. Like xilinx DMA, they
> also have some special configuration.

Well we all think our HW is special and needs some additional stuff, most of
the cases turns out not to be the case.

Can you explain how audio in this case additional configuration...

--
~Vinod

2018-04-13 06:44:52

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On 13 April 2018 at 14:36, Vinod Koul <[email protected]> wrote:
> On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
>
>> > Agreed, users only care about grabbing a channel, setting a descriptor and
>> > submitting that.
>> >
>> > I think you need to go back and think about this a bit, please do go thru
>> > dmaengine documentation and see other driver examples.
>> >
>> > We don't typically expose these to users, they give us a transfer and we set
>> > that up in hardware for efficient. Its DMA so people expect us to use fastest
>> > mechanism available.
>>
>> But there are some configuration are really special for Spreadtrum
>> DMA, and must need user to specify how to configure, especially some
>> scenarios of audio. So I wander if we can add one pointer for
>> 'dma_slave_config' to expand some special DMA configuration
>> requirements, like:
>>
>> struct dma_slave_config {
>> ......
>> unsigned int slave_id;
>> void *platform_data;
>> };
>>
>> So if some DMA has some special configuration (such as Spreadtrum
>> DMA), they can user this platform_data pointer. Like xilinx DMA, they
>> also have some special configuration.
>
> Well we all think our HW is special and needs some additional stuff, most of
> the cases turns out not to be the case.
>
> Can you explain how audio in this case additional configuration...
>

Beside the general configuration, our audio driver will configure the
fragment length, block length, maybe transaction length, and they must
specify the request type and interrupt type, these are what we want to
export for users.

--
Baolin.wang
Best Regards

2018-04-13 10:08:58

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
> On 13 April 2018 at 14:36, Vinod Koul <[email protected]> wrote:
> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
> >
> >> > Agreed, users only care about grabbing a channel, setting a descriptor and
> >> > submitting that.
> >> >
> >> > I think you need to go back and think about this a bit, please do go thru
> >> > dmaengine documentation and see other driver examples.
> >> >
> >> > We don't typically expose these to users, they give us a transfer and we set
> >> > that up in hardware for efficient. Its DMA so people expect us to use fastest
> >> > mechanism available.
> >>
> >> But there are some configuration are really special for Spreadtrum
> >> DMA, and must need user to specify how to configure, especially some
> >> scenarios of audio. So I wander if we can add one pointer for
> >> 'dma_slave_config' to expand some special DMA configuration
> >> requirements, like:
> >>
> >> struct dma_slave_config {
> >> ......
> >> unsigned int slave_id;
> >> void *platform_data;
> >> };
> >>
> >> So if some DMA has some special configuration (such as Spreadtrum
> >> DMA), they can user this platform_data pointer. Like xilinx DMA, they
> >> also have some special configuration.
> >
> > Well we all think our HW is special and needs some additional stuff, most of
> > the cases turns out not to be the case.
> >
> > Can you explain how audio in this case additional configuration...
> >
>
> Beside the general configuration, our audio driver will configure the
> fragment length, block length, maybe transaction length, and they must
> specify the request type and interrupt type, these are what we want to
> export for users.

First doesn't it use sound dmaengine library, it should :)

Second, I think you should calculate the lengths based on given input. Audio
is circular buffer so you shall create a circular linked list and submit.
See how other driver implement circular prepare callback

--
~Vinod

2018-04-13 10:58:28

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On 13 April 2018 at 18:11, Vinod Koul <[email protected]> wrote:
> On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
>> On 13 April 2018 at 14:36, Vinod Koul <[email protected]> wrote:
>> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
>> >
>> >> > Agreed, users only care about grabbing a channel, setting a descriptor and
>> >> > submitting that.
>> >> >
>> >> > I think you need to go back and think about this a bit, please do go thru
>> >> > dmaengine documentation and see other driver examples.
>> >> >
>> >> > We don't typically expose these to users, they give us a transfer and we set
>> >> > that up in hardware for efficient. Its DMA so people expect us to use fastest
>> >> > mechanism available.
>> >>
>> >> But there are some configuration are really special for Spreadtrum
>> >> DMA, and must need user to specify how to configure, especially some
>> >> scenarios of audio. So I wander if we can add one pointer for
>> >> 'dma_slave_config' to expand some special DMA configuration
>> >> requirements, like:
>> >>
>> >> struct dma_slave_config {
>> >> ......
>> >> unsigned int slave_id;
>> >> void *platform_data;
>> >> };
>> >>
>> >> So if some DMA has some special configuration (such as Spreadtrum
>> >> DMA), they can user this platform_data pointer. Like xilinx DMA, they
>> >> also have some special configuration.
>> >
>> > Well we all think our HW is special and needs some additional stuff, most of
>> > the cases turns out not to be the case.
>> >
>> > Can you explain how audio in this case additional configuration...
>> >
>>
>> Beside the general configuration, our audio driver will configure the
>> fragment length, block length, maybe transaction length, and they must
>> specify the request type and interrupt type, these are what we want to
>> export for users.
>
> First doesn't it use sound dmaengine library, it should :)

I do not think so. That's the DMA configuration need to set before
audio use it. Not only audio, but also other drivers using DMA need to
configure these configuration what we exported in sprd_dma_config.

>
> Second, I think you should calculate the lengths based on given input. Audio
> is circular buffer so you shall create a circular linked list and submit.
> See how other driver implement circular prepare callback

Yes, we have not implemented the link list mode for audio now, but in
our plan. So firstly we want to export these necessary configuration
for users to configure.

--
Baolin.wang
Best Regards

2018-04-16 03:56:40

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
> On 13 April 2018 at 14:36, Vinod Koul <[email protected]> wrote:
> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
> >
> >> > Agreed, users only care about grabbing a channel, setting a descriptor and
> >> > submitting that.
> >> >
> >> > I think you need to go back and think about this a bit, please do go thru
> >> > dmaengine documentation and see other driver examples.
> >> >
> >> > We don't typically expose these to users, they give us a transfer and we set
> >> > that up in hardware for efficient. Its DMA so people expect us to use fastest
> >> > mechanism available.
> >>
> >> But there are some configuration are really special for Spreadtrum
> >> DMA, and must need user to specify how to configure, especially some
> >> scenarios of audio. So I wander if we can add one pointer for
> >> 'dma_slave_config' to expand some special DMA configuration
> >> requirements, like:
> >>
> >> struct dma_slave_config {
> >> ......
> >> unsigned int slave_id;
> >> void *platform_data;
> >> };
> >>
> >> So if some DMA has some special configuration (such as Spreadtrum
> >> DMA), they can user this platform_data pointer. Like xilinx DMA, they
> >> also have some special configuration.
> >
> > Well we all think our HW is special and needs some additional stuff, most of
> > the cases turns out not to be the case.
> >
> > Can you explain how audio in this case additional configuration...
>
> Beside the general configuration, our audio driver will configure the
> fragment length, block length, maybe transaction length, and they must
> specify the request type and interrupt type, these are what we want to
> export for users.

As I said before, you need to derive fragment, block or transaction from
given prep_cyclic values. Interrupt type needs to be derived with the flags
passed. Please do see how other drivers make use of it.

--
~Vinod

2018-04-16 06:33:50

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On 16 April 2018 at 11:58, Vinod Koul <[email protected]> wrote:
> On Fri, Apr 13, 2018 at 02:41:48PM +0800, Baolin Wang wrote:
>> On 13 April 2018 at 14:36, Vinod Koul <[email protected]> wrote:
>> > On Fri, Apr 13, 2018 at 02:17:34PM +0800, Baolin Wang wrote:
>> >
>> >> > Agreed, users only care about grabbing a channel, setting a descriptor and
>> >> > submitting that.
>> >> >
>> >> > I think you need to go back and think about this a bit, please do go thru
>> >> > dmaengine documentation and see other driver examples.
>> >> >
>> >> > We don't typically expose these to users, they give us a transfer and we set
>> >> > that up in hardware for efficient. Its DMA so people expect us to use fastest
>> >> > mechanism available.
>> >>
>> >> But there are some configuration are really special for Spreadtrum
>> >> DMA, and must need user to specify how to configure, especially some
>> >> scenarios of audio. So I wander if we can add one pointer for
>> >> 'dma_slave_config' to expand some special DMA configuration
>> >> requirements, like:
>> >>
>> >> struct dma_slave_config {
>> >> ......
>> >> unsigned int slave_id;
>> >> void *platform_data;
>> >> };
>> >>
>> >> So if some DMA has some special configuration (such as Spreadtrum
>> >> DMA), they can user this platform_data pointer. Like xilinx DMA, they
>> >> also have some special configuration.
>> >
>> > Well we all think our HW is special and needs some additional stuff, most of
>> > the cases turns out not to be the case.
>> >
>> > Can you explain how audio in this case additional configuration...
>>
>> Beside the general configuration, our audio driver will configure the
>> fragment length, block length, maybe transaction length, and they must
>> specify the request type and interrupt type, these are what we want to
>> export for users.
>
> As I said before, you need to derive fragment, block or transaction from

I am sorry I did not make things clear here. What I mean is not only
link list mode(prep_cyclic), but also other modes (prep_slave,
prep_interleaved ...), users still need to configure the fragment
length, block length or transaction length according to their
requirements.

> given prep_cyclic values. Interrupt type needs to be derived with the flags
> passed. Please do see how other drivers make use of it.

Fine. We configure the Interrupt type through the flags passed.

--
Baolin.wang
Best Regards

2018-04-16 15:32:44

by Vinod Koul

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On Mon, Apr 16, 2018 at 02:32:05PM +0800, Baolin Wang wrote:

> >> Beside the general configuration, our audio driver will configure the
> >> fragment length, block length, maybe transaction length, and they must
> >> specify the request type and interrupt type, these are what we want to
> >> export for users.
> >
> > As I said before, you need to derive fragment, block or transaction from
>
> I am sorry I did not make things clear here. What I mean is not only
> link list mode(prep_cyclic), but also other modes (prep_slave,
> prep_interleaved ...), users still need to configure the fragment
> length, block length or transaction length according to their
> requirements.

Other modes are similar, they also use the parameters programmed from
dma_slave_config. Pls see other driver examples. IIRC dw dma has
similar capabilities but not all are exposed to user and driver configures.

--
~Vinod

2018-04-17 06:08:18

by Baolin Wang

Subject: Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

On 16 April 2018 at 23:35, Vinod Koul <[email protected]> wrote:
> On Mon, Apr 16, 2018 at 02:32:05PM +0800, Baolin Wang wrote:
>
>> >> Beside the general configuration, our audio driver will configure the
>> >> fragment length, block length, maybe transaction length, and they must
>> >> specify the request type and interrupt type, these are what we want to
>> >> export for users.
>> >
>> > As I said before, you need to derive fragment, block or transaction from
>>
>> I am sorry I did not make things clear here. What I mean is not only
>> link list mode(prep_cyclic), but also other modes (prep_slave,
>> prep_interleaved ...), users still need to configure the fragment
>> length, block length or transaction length according to their
>> requirements.
>
> Other modes are similar, they also use the parameters programmed from
> dma_slave_config. Pls see other driver examples. IIRC dw dma has
> similar capabilities but not all are exposed to user and driver configures.

I'll check the dw dma. Really thanks for your help :)

--
Baolin.wang
Best Regards

2018-04-17 10:46:57

by Lars-Peter Clausen

Subject: Re: [PATCH 5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces

On 04/10/2018 09:46 AM, Baolin Wang wrote:
[...]
> +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 =
> + container_of(config, struct sprd_dma_config, config);
> +

Please do not overload standard API with custom semantics. This makes the
driver incompatible to the API and negates the whole idea of having a common
API. E.g. this will crash when somebody passes a normal dma_slave_config
struct to this function.

> + memcpy(&schan->slave_cfg, slave_cfg, sizeof(*slave_cfg));
> + return 0;
> +}

2018-04-18 05:42:21

by Baolin Wang

Subject: Re: [PATCH 5/5] dmaengine: sprd: Add 'device_config' and 'device_prep_slave_sg' interfaces

On 17 April 2018 at 18:45, Lars-Peter Clausen <[email protected]> wrote:
> On 04/10/2018 09:46 AM, Baolin Wang wrote:
> [...]
>> +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 =
>> + container_of(config, struct sprd_dma_config, config);
>> +
>
> Please do not overload standard API with custom semantics. This makes the
> driver incompatible to the API and negates the whole idea of having a common
> API. E.g. this will crash when somebody passes a normal dma_slave_config
> struct to this function.
>

Yes, we have discussed with Vinod how to use 'dma_slave_config' to
reach our requirements. Thanks for your comments.

--
Baolin.wang
Best Regards