dmaengine: sirf: add dmaengine_prep_slave_single/sg support

PATCH New Barry Song 3 participants 13 messages Aug 25, 2013 – Sep 3, 2013 dmaengine

Share & tools

Barry Song

author

2013-08-25 12:57 UTC #0 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.

the dma engine of sirfsoc doesn't support hardware s/g, so here we are using the list of desc nodes to do SW s/g. when dma operations finish, driver will re-start the next desc automatically.

Signed-off-by Barry Song <[email protected]>

---
 drivers/dma/sirf-dma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

drivers/dma/sirf-dma.c +57 -0

index 6aec3ad..c226c79 100644
@@ -577,6 +577,62 @@ err_dir:
 }
 
 static struct dma_async_tx_descriptor *
+sirfsoc_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+	unsigned int sg_len, enum dma_transfer_direction direction,
+	unsigned long flags, void *context)
+{
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+	struct sirfsoc_dma_desc *first_sdesc;
+	struct sirfsoc_dma_desc *sdesc;
+	struct list_head *l;
+	unsigned long iflags;
+	struct scatterlist *sg;
+	int desc_cnt = 0;
+	int i;
+	int ret;
+
+	/*
+	 * the hardware doesn't support sg, here we use software list
+	 * to simulate sg, so make sure we have enough desc nodes
+	 */
+	spin_lock_irqsave(&schan->lock, iflags);
+	list_for_each(l, &schan->free)
+		desc_cnt++;
+	if (desc_cnt < sg_len) {
+		spin_unlock_irqrestore(&schan->lock, iflags);
+		pr_err("sirfsoc DMA channel busy\n");
+		ret = -EBUSY;
+		goto err;
+	}
+
+	first_sdesc = list_first_entry(&schan->free, struct sirfsoc_dma_desc,
+			node);
+
+	for_each_sg(sgl, sg, sg_len, i) {
+		dma_addr_t addr = sg_dma_address(sg);
+		unsigned int len = sg_dma_len(sg);
+
+		sdesc = list_first_entry(&schan->free, struct sirfsoc_dma_desc,
+			node);
+		list_del(&sdesc->node);
+
+		sdesc->addr = addr;
+		sdesc->dir = (direction == DMA_MEM_TO_DEV ? 1 : 0);
+		sdesc->cyclic = 0;
+		sdesc->xlen = len / SIRFSOC_DMA_WORD_LEN;
+		sdesc->width = sdesc->xlen;
+		sdesc->ylen = 0;
+
+		list_add_tail(&sdesc->node, &schan->prepared);
+	}
+	spin_unlock_irqrestore(&schan->lock, iflags);
+
+	return &first_sdesc->desc;
+err:
+	return ERR_PTR(ret);
+}
+
+static struct dma_async_tx_descriptor *
 sirfsoc_dma_prep_cyclic(struct dma_chan *chan, dma_addr_t addr,
 	size_t buf_len, size_t period_len,
 	enum dma_transfer_direction direction, unsigned long flags, void *context)

@@ -711,6 +767,7 @@ static int sirfsoc_dma_probe(struct platform_device *op)
 	dma->device_control = sirfsoc_dma_control;
 	dma->device_tx_status = sirfsoc_dma_tx_status;
 	dma->device_prep_interleaved_dma = sirfsoc_dma_prep_interleaved;
+	dma->device_prep_slave_sg = sirfsoc_dma_prep_slave_sg;
 	dma->device_prep_dma_cyclic = sirfsoc_dma_prep_cyclic;
 
 	INIT_LIST_HEAD(&dma->channels);

Signature · 1 line

1.8.2.3

Vinod Koul

2013-08-26 08:56 UTC #1 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

Quoted 5 lines · Barry Song

the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.

Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes

Quoted 28 lines

the dma engine of sirfsoc doesn't support hardware s/g, so here we are using the list of desc nodes to do SW s/g. when dma operations finish, driver will re-start the next desc automatically.

Signed-off-by Barry Song <[email protected]>

---
 drivers/dma/sirf-dma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

drivers/dma/sirf-dma.c +11 -0

index 6aec3ad..c226c79 100644
@@ -577,6 +577,62 @@ err_dir:
 }
 
 static struct dma_async_tx_descriptor *
+sirfsoc_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+	unsigned int sg_len, enum dma_transfer_direction direction,
+	unsigned long flags, void *context)
+{
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+	struct sirfsoc_dma_desc *first_sdesc;
+	struct sirfsoc_dma_desc *sdesc;
+	struct list_head *l;
+	unsigned long iflags;
+	struct scatterlist *sg;
+	int desc_cnt = 0;

drivers/dma/sirf-dma.c err_dir: · line 600

Quoted 10 lines

drivers/dma/sirf-dma.c +10 -0

+	int i;
+	int ret;
+
+	/*
+	 * the hardware doesn't support sg, here we use software list
+	 * to simulate sg, so make sure we have enough desc nodes
+	 */
+	spin_lock_irqsave(&schan->lock, iflags);
+	list_for_each(l, &schan->free)
+		desc_cnt++;

why dont you allocate descriptors here. That will remove this limitation..

~Vinod

Barry Song

author

2013-09-01 13:02 UTC #2 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

2013/8/26 Vinod Koul <[email protected]>:

Quoted 8 lines · Barry Song

Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes

Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?

the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.

Quoted 32 lines

the dma engine of sirfsoc doesn't support hardware s/g, so here we are using the list of desc nodes to do SW s/g. when dma operations finish, driver will re-start the next desc automatically.

Signed-off-by Barry Song <[email protected]>

---
 drivers/dma/sirf-dma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

drivers/dma/sirf-dma.c +14 -0

index 6aec3ad..c226c79 100644
@@ -577,6 +577,62 @@ err_dir:
 }
 
 static struct dma_async_tx_descriptor *
+sirfsoc_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+     unsigned int sg_len, enum dma_transfer_direction direction,
+     unsigned long flags, void *context)
+{
+     struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+     struct sirfsoc_dma_desc *first_sdesc;
+     struct sirfsoc_dma_desc *sdesc;
+     struct list_head *l;
+     unsigned long iflags;
+     struct scatterlist *sg;
+     int desc_cnt = 0;
+     int i;
+     int ret;
+

Quoted 8 lines

drivers/dma/sirf-dma.c err_dir: · line 600
Quoted 7 lines
drivers/dma/sirf-dma.c +7 -0
+     /*
+      * the hardware doesn't support sg, here we use software list
+      * to simulate sg, so make sure we have enough desc nodes
+      */
+     spin_lock_irqsave(&schan->lock, iflags);
+     list_for_each(l, &schan->free)
+             desc_cnt++;
why dont you allocate descriptors here. That will remove this limitation..

i understand. here sirf user scenerios will never be over the limitation of SIRFSOC_DMA_DESCRIPTORS = 16. so i don't want to make it too complex.

Quoted 2 lines

~Vinod

-barry

Vinod Koul

2013-09-02 06:25 UTC #3 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

Quoted 10 lines · Barry Song

2013/8/26 Vinod Koul <[email protected]>:
Quoted 8 lines · Barry Song
Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes

Quoted 6 lines

Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?
the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.

The whole premise of doing the generic interleaved api was to ensure we can use any of the usuages as a case of interleaved api. Can you explain how it would be difficult?

Quoted 8 lines

Quoted 4 lines
drivers/dma/sirf-dma.c err_dir: · line 600
Quoted 3 lines
drivers/dma/sirf-dma.c +3 -0
+     spin_lock_irqsave(&schan->lock, iflags);
+     list_for_each(l, &schan->free)
+             desc_cnt++;
why dont you allocate descriptors here. That will remove this limitation..
i understand. here sirf user scenerios will never be over the limitation of SIRFSOC_DMA_DESCRIPTORS = 16. so i don't want to make it too complex.

I think you can make code simler by dynamically allocating and freeing descriptors...

~Vinod

Barry Song

author

2013-09-03 10:06 UTC #4 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

2013/9/2 Vinod Koul <[email protected]>:

Quoted 14 lines · Barry Song

Quoted 13 lines
2013/8/26 Vinod Koul <[email protected]>:
Quoted 8 lines · Barry Song
Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes
Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?

Quoted 6 lines

Quoted 3 lines
the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.
The whole premise of doing the generic interleaved api was to ensure we can use any of the usuages as a case of interleaved api. Can you explain how it would be difficult?

for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval

the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.

Quoted 11 lines

Quoted 9 lines
Quoted 5 lines
drivers/dma/sirf-dma.c err_dir: · line 600
Quoted 4 lines
drivers/dma/sirf-dma.c +3 -0
+     spin_lock_irqsave(&schan->lock, iflags);
+     list_for_each(l, &schan->free)
+             desc_cnt++;
why dont you allocate descriptors here. That will remove this limitation..
i understand. here sirf user scenerios will never be over the limitation of SIRFSOC_DMA_DESCRIPTORS = 16. so i don't want to make it too complex.
I think you can make code simler by dynamically allocating and freeing descriptors...

when do you think is the right time to free? while the whole sg finished? i always think the code will be ugly.

Quoted 3 lines

~Vinod
--

-barry

Vinod Koul

2013-09-03 11:39 UTC #5 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

Quoted 36 lines · Barry Song

2013/9/2 Vinod Koul <[email protected]>:
Quoted 14 lines · Barry Song
Quoted 13 lines
2013/8/26 Vinod Koul <[email protected]>:
Quoted 8 lines · Barry Song
Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes
Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?
Quoted 6 lines
Quoted 3 lines
the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.
The whole premise of doing the generic interleaved api was to ensure we can use any of the usuages as a case of interleaved api. Can you explain how it would be difficult?
for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval

Quoted 2 lines

the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.

Why not:

Okay there are two ways to this
1. Advertize your capablity and let client use that to break. You cna do so using dma_set/get_max_seg_size().

2. in DMAC, nothing stops you from breaking the given sg_list into smaller chunks. So if you get a buffer exceeding what you do, you cna internally split to multiple descriptors and chain those to parent one.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/dma/sirf-dma.c err_dir: · line 600
Quoted 5 lines
drivers/dma/sirf-dma.c +3 -0
+     spin_lock_irqsave(&schan->lock, iflags);
+     list_for_each(l, &schan->free)
+             desc_cnt++;
why dont you allocate descriptors here. That will remove this limitation..

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
i understand. here sirf user scenerios will never be over the limitation of SIRFSOC_DMA_DESCRIPTORS = 16. so i don't want to make it too complex.
I think you can make code simler by dynamically allocating and freeing descriptors...
when do you think is the right time to free? while the whole sg finished? i always think the code will be ugly.

when the descriptor has been completed. You allocate in prepare. Bunch of driver already do so... I dont think its so complicated

~Vinod

Barry Song

author

2013-09-03 12:42 UTC #6 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

2013/9/3 Vinod Koul <[email protected]>:

Quoted 37 lines · Barry Song

Quoted 36 lines
2013/9/2 Vinod Koul <[email protected]>:
Quoted 14 lines · Barry Song
Quoted 13 lines
2013/8/26 Vinod Koul <[email protected]>:
Quoted 8 lines · Barry Song
Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes
Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?
Quoted 6 lines
Quoted 3 lines
the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.
The whole premise of doing the generic interleaved api was to ensure we can use any of the usuages as a case of interleaved api. Can you explain how it would be difficult?
for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval

Quoted 8 lines

Quoted 2 lines
the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.
Why not:
Okay there are two ways to this
1. Advertize your capablity and let client use that to break. You cna do so using dma_set/get_max_seg_size().

this way is ok. does max_seg also match with interleaved mode?

Quoted 3 lines

2. in DMAC, nothing stops you from breaking the given sg_list into smaller chunks. So if you get a buffer exceeding what you do, you cna internally split to multiple descriptors and chain those to parent one.

this way still need hacking in dmac driver.

Quoted 11 lines

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 7 lines
drivers/dma/sirf-dma.c err_dir: · line 600
Quoted 6 lines
drivers/dma/sirf-dma.c +3 -0
+     spin_lock_irqsave(&schan->lock, iflags);
+     list_for_each(l, &schan->free)
+             desc_cnt++;
why dont you allocate descriptors here. That will remove this limitation..
i understand. here sirf user scenerios will never be over the limitation of SIRFSOC_DMA_DESCRIPTORS = 16. so i don't want to make it too complex.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
I think you can make code simler by dynamically allocating and freeing descriptors...
when do you think is the right time to free? while the whole sg finished? i always think the code will be ugly.
when the descriptor has been completed. You allocate in prepare. Bunch of driver already do so... I dont think its so complicated

here sirf dmac alloc all 16 desc in alloc_resoures and free all in free_resources. if we can move the way you said, it is fine. but it seems it means another separate patch for that.

Quoted 3 lines

~Vinod
--

-barry

Vinod Koul

2013-09-03 12:12 UTC #7 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

Quoted 8 lines · Barry Song

Quoted 6 lines
Why not:
Okay there are two ways to this
1. Advertize your capablity and let client use that to break. You cna do so using dma_set/get_max_seg_size().
this way is ok. does max_seg also match with interleaved mode?

i dont see a reason not to..

Quoted 7 lines

Quoted 5 lines
2. in DMAC, nothing stops you from breaking the given sg_list into smaller chunks. So if you get a buffer exceeding what you do, you cna internally split to multiple descriptors and chain those to parent one.
this way still need hacking in dmac driver.

Yup but you get a driver which cna handle any buffers any lists, which is very nice from users POV

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
Quoted 8 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+     spin_lock_irqsave(&schan->lock, iflags);
+     list_for_each(l, &schan->free)
+             desc_cnt++;
why dont you allocate descriptors here. That will remove this limitation..
i understand. here sirf user scenerios will never be over the limitation of SIRFSOC_DMA_DESCRIPTORS = 16. so i don't want to make it too complex.
I think you can make code simler by dynamically allocating and freeing descriptors...

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
when do you think is the right time to free? while the whole sg finished? i always think the code will be ugly.
when the descriptor has been completed. You allocate in prepare. Bunch of driver already do so... I dont think its so complicated
here sirf dmac alloc all 16 desc in alloc_resoures and free all in free_resources. if we can move the way you said, it is fine. but it seems it means another separate patch for that.

Sure...

~Vinod

Jassi Brar

2013-09-03 11:55 UTC #8 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

In reply to Barry Song

Quoted 36 lines · Barry Song

2013/9/2 Vinod Koul <[email protected]>:
Quoted 14 lines · Barry Song
Quoted 13 lines
2013/8/26 Vinod Koul <[email protected]>:
Quoted 8 lines · Barry Song
Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes
Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?
Quoted 6 lines
Quoted 3 lines
the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.
The whole premise of doing the generic interleaved api was to ensure we can use any of the usuages as a case of interleaved api. Can you explain how it would be difficult?
for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval

Quoted 3 lines

the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.

A client is only interested in transferring the total amount data and if/when it wants notified. How the dma controller divides the big transfer, shouldn't matter to the client. In fact it would be bad for a client to care about the working of a dma controller. IOW, the 16KB transfer could be divided into 4 parts by the dmac that supports max 4KB transfers, into 8 parts if max is 2KB and so on... the client shouldn't care.

-jassi

Vinod Koul

2013-09-03 12:08 UTC #9 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

Quoted 17 lines · Jassi Brar

Quoted 14 lines
for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval
Quoted 3 lines
the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.

Quoted 7 lines

A client is only interested in transferring the total amount data and if/when it wants notified. How the dma controller divides the big transfer, shouldn't matter to the client. In fact it would be bad for a client to care about the working of a dma controller. IOW, the 16KB transfer could be divided into 4 parts by the dmac that supports max 4KB transfers, into 8 parts if max is 2KB and so on... the client shouldn't care.

That is also a correct approach. I agree that dma driver should be able to handle any lengths and split that to multiple descriptors.

But I think in interleaved API, clients should know the capablity otherwise dma driver will have redo the whole list again...

~Vinod

Jassi Brar

2013-09-03 13:15 UTC #10 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

Quoted 25 lines · Vinod Koul

Quoted 17 lines · Jassi Brar
Quoted 14 lines
for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval
Quoted 3 lines
the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.
Quoted 7 lines
A client is only interested in transferring the total amount data and if/when it wants notified. How the dma controller divides the big transfer, shouldn't matter to the client. In fact it would be bad for a client to care about the working of a dma controller. IOW, the 16KB transfer could be divided into 4 parts by the dmac that supports max 4KB transfers, into 8 parts if max is 2KB and so on... the client shouldn't care.

Quoted 6 lines

That is also a correct approach. I agree that dma driver should be able to handle any lengths and split that to multiple descriptors.
But I think in interleaved API, clients should know the capablity otherwise dma driver will have redo the whole list again...

I am not sure if the clients should do the mapping from arbitrary data pattern onto how the dmac is to be programmed. The client is no more generic (it just passes dmac specific data in a generic way), and the approach also entails yet another api for dmacs to express their capability.

As we know, the interleaved api was meant for clients to express arbitrary data arrangement in memory. It's not for clients passing transfer info in dmac specific format.

-jassi

Barry Song

author

2013-09-03 12:38 UTC #11 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

In reply to Jassi Brar

2013/9/3 Jassi Brar <[email protected]>:

Quoted 40 lines · Barry Song

Quoted 36 lines
2013/9/2 Vinod Koul <[email protected]>:
Quoted 14 lines · Barry Song
Quoted 13 lines
2013/8/26 Vinod Koul <[email protected]>:
Quoted 8 lines · Barry Song
Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes
Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?
Quoted 6 lines
Quoted 3 lines
the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.
The whole premise of doing the generic interleaved api was to ensure we can use any of the usuages as a case of interleaved api. Can you explain how it would be difficult?
for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval
Quoted 3 lines
the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.

Quoted 7 lines

A client is only interested in transferring the total amount data and if/when it wants notified. How the dma controller divides the big transfer, shouldn't matter to the client. In fact it would be bad for a client to care about the working of a dma controller. IOW, the 16KB transfer could be divided into 4 parts by the dmac that supports max 4KB transfers, into 8 parts if max is 2KB and so on... the client shouldn't care.

here my point is that we don't expose dma transferring hardware details to clients. i actually wanted to hide that in dma driver not dma client.
the only difference between vinod and me is that Vinod's point is hiding that in dma core, my way is hiding it in dma driver.

btw, interleaved_dma clients do care about how the interleaved transfer will happen. but single_dma client doesn't care.

Quoted 2 lines

-jassi

-barry

Jassi Brar

2013-09-03 12:54 UTC #12 lore raw

PATCH 1/1 dmaengine: sirf: add dmaengine_prep_slave_single/sg support

Quoted 49 lines · Barry Song

2013/9/3 Jassi Brar <[email protected]>:
Quoted 40 lines · Barry Song
Quoted 36 lines
2013/9/2 Vinod Koul <[email protected]>:
Quoted 14 lines · Barry Song
Quoted 13 lines
2013/8/26 Vinod Koul <[email protected]>:
Quoted 8 lines · Barry Song
Quoted 5 lines
the dma engine of sirfsoc supports interleaved mode, but if we set xlen=width instead xlen<width, it will work as non-interleaved. as most clients of sirf dma driver still don't need interleaved mode, so here we still need to implement prep_slave_sg entry so that users like uart, spi can use these APIs instead of interleaved API.
Well in that case why dont you just create a wrapper on top of interleaved API to make this work without driver changes
Vinod, do you mean using interleaved API to provide sg/single API if specific drivers implement interleaved_dma but not implement sg_dma?
Quoted 6 lines
Quoted 3 lines
the problem is that is difficult to set right legal sgl[i].size, sgl[i].icg and numf for all dmaengines as that depends on specific dma hardware limitation.
The whole premise of doing the generic interleaved api was to ensure we can use any of the usuages as a case of interleaved api. Can you explain how it would be difficult?
for example, if someone wants to do a 16Kbytes single transfer. how will interleaved dma set transfer length and interval between every row?
if it sets the transfer length to 16KB directly, the dma hardware might not support such long a transfer at all.
so it might want to set as two 8KB transfer without interval between them.
0~8KB empty interval
8KB-16KB empty interval
or four 4KB transfer as
0~4KB empty interval
4KB-8KB empty interval
8KB-12KB empty interval
12KB-16KB empty interval
Quoted 3 lines
the problem is we don't know the hardware limitation of every dma controllers for transferring length of each row.
Quoted 7 lines
A client is only interested in transferring the total amount data and if/when it wants notified. How the dma controller divides the big transfer, shouldn't matter to the client. In fact it would be bad for a client to care about the working of a dma controller. IOW, the 16KB transfer could be divided into 4 parts by the dmac that supports max 4KB transfers, into 8 parts if max is 2KB and so on... the client shouldn't care.

Quoted 6 lines

here my point is that we don't expose dma transferring hardware details to clients. i actually wanted to hide that in dma driver not dma client.
the only difference between vinod and me is that Vinod's point is hiding that in dma core, my way is hiding it in dma driver.

How big is the effort to convert the clients to use interleaved api?

Quoted 3 lines

btw, interleaved_dma clients do care about how the interleaved transfer will happen

No, there is difference. Interleaved api only tells how the data, to be transferred, is laid out in memory. It doesn't not specify how the dma controller actually does the transfer.

-jassi