dma: pdma: cyclic, residue, DMA_PRIVATE
v5 cover dma: pdma: cyclic, residue, DMA_PRIVATE
This is v5 of my series to teach the pdma driver more required functions.
In contrast to v4, it has yet again grown by one more patch (2/5), which removes code that clears DCMD_ENDIRQEN at the end of the pending chain. We want to leave of the DCMD_ENDIRQEN intact in order to identify where a transaction ends.
Thanks to Xiang Wang and Andy Shevchenko for their reviews.
Daniel
Daniel Mack (5):
dma: mmp_pdma: only complete one transaction from dma_do_tasklet()
dma: mmp_pdma: don't clear DCMD_ENDIRQEN at end of pending chain
dma: mmp_pdma: add support for cyclic DMA descriptors
dma: mmp_pdma: add support for residue reporting
dma: mmp_pdma: set DMA_PRIVATE
drivers/dma/mmp_pdma.c | 247 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 210 insertions(+), 37 deletions(-)
Signature · 1 line
1.8.3.1
PATCH v5 1/5 dma: mmp_pdma: only complete one transaction from dma_do_tasklet()
Currently, when an interrupt has occured for a channel, the tasklet worker code will only look at the very last entry in the running list and complete its cookie, and then dispose the entire running chain. Hence, the first transaction's cookie will never complete.
In fact, the interrupt we should handle will be the one related to the first descriptor in the chain with the ENDIRQEN bit set, so complete the second transaction that is in fact still running.
As a result, the driver can't currently handle multiple transactions on one chanel, and it's likely that no drivers exist that rely on this feature.
Fix this by walking the running_chain and look for the first descriptor that has the interrupt-enable bit set. Only queue descriptors up to that point for completion handling, while leaving the rest intact. Also, only make the channel idle if the list is completely empty after such a cycle.
--- drivers/dma/mmp_pdma.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)
drivers/dma/mmp_pdma.c +21 -14
@@ -710,25 +710,32 @@ static void dma_do_tasklet(unsigned long data)spin_lock_irqsave (&chan->desc_lock, flags); -/* update the cookie if we have some descriptors to cleanup */ -if (!list_empty (&chan->chain_running)) { -dma_cookie_t cookie; - - desc =to_mmp_pdma_desc (chan->chain_running.prev); - cookie = desc->async_tx.cookie; -dma_cookie_complete (&desc->async_tx); +list_for_each_entry_safe (desc, _desc, &chan->chain_running, node) { +/* +* move the descriptors to a temporary list so we can drop +* the lock during the entire cleanup operation +*/ +list_del (&desc->node); +list_add (&desc->node, &chain_cleanup); -dev_dbg (chan->dev,"completed_cookie=%d\n" , cookie); +/* +* Look for the first list entry which has the ENDIRQEN flag +* set. That is the descriptor we got an interrupt for, so +* complete that transaction and its cookie. +*/ +if (desc->desc.dcmd &DCMD_ENDIRQEN ) { +dma_cookie_t cookie = desc->async_tx.cookie; +dma_cookie_complete (&desc->async_tx); +dev_dbg (chan->dev,"completed_cookie=%d\n" , cookie); +break ; + } }/* -* move the descriptors to a temporary list so we can drop the lock -* during the entire cleanup operation +* The hardware is idle and ready for more when the +* chain_running list is empty. */ -list_splice_tail_init (&chan->chain_running, &chain_cleanup); - -/* the hardware is now idle and ready for more */ - chan->idle = true; + chan->idle =list_empty (&chan->chain_running);/* Start any pending transactions automatically */ start_pending_queue (chan);
Signature · 1 line
1.8.3.1
PATCH v5 2/5 dma: mmp_pdma: don't clear DCMD_ENDIRQEN at end of pending chain
In reply to Daniel MackIn order to fully support multiple transactions per channel, we need to assure we get an interrupt for each completed transaction. That flags bit is also our only way to tell at which descriptor a transaction ends.
So, remove the manual clearing of that bit, and then inline the only remaining command that is left in append_pending_queue() for better readability.
--- drivers/dma/mmp_pdma.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-)
drivers/dma/mmp_pdma.c +2 -20
@@ -279,25 +279,6 @@ static void mmp_pdma_free_phy(struct mmp_pdma_chan *pchan)spin_unlock_irqrestore (&pdev->phy_lock, flags); } -/* desc->tx_list ==> pending list */ -static void append_pending_queue (struct mmp_pdma_chan *chan, -struct mmp_pdma_desc_sw *desc) -{ -struct mmp_pdma_desc_sw *tail = -to_mmp_pdma_desc (chan->chain_pending.prev); - -if (list_empty (&chan->chain_pending)) - goto out_splice; - -/* one irq per queue, even appended */ - tail->desc.ddadr = desc->async_tx.phys; - tail->desc.dcmd &= ~DCMD_ENDIRQEN ; - -/* softly link to pending list */ -out_splice: -list_splice_tail_init (&desc->tx_list, &chan->chain_pending); -} -/** * start_pending_queue - transfer any pending transactions * pending list ==> running list
@@ -360,7 +341,8 @@ static dma_cookie_t mmp_pdma_tx_submit(struct dma_async_tx_descriptor *tx) cookie =dma_cookie_assign (&child->async_tx); } -append_pending_queue (chan, desc); +/* softly link to pending list - desc->tx_list ==> pending list */ +list_splice_tail_init (&desc->tx_list, &chan->chain_pending);spin_unlock_irqrestore (&chan->desc_lock, flags);
Signature · 1 line
1.8.3.1
PATCH v5 3/5 dma: mmp_pdma: add support for cyclic DMA descriptors
In reply to Daniel MackProvide a callback to prepare cyclic DMA transfers. This is for instance needed for audio channel transport.
--- drivers/dma/mmp_pdma.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 111 insertions(+), 1 deletion(-)
drivers/dma/mmp_pdma.c +111 -1
@@ -98,6 +98,9 @@ struct mmp_pdma_chan {struct mmp_pdma_phy *phy;enum dma_transfer_direction dir; +struct mmp_pdma_desc_sw *cyclic_first;/* first desc_sw if channel +* is in cyclic mode */ +/* channel's basic info */ struct tasklet_struct tasklet;u32 dcmd;
@@ -499,6 +502,8 @@ mmp_pdma_prep_memcpy(struct dma_chan *dchan, new->desc.ddadr =DDADR_STOP ; new->desc.dcmd |=DCMD_ENDIRQEN ; + chan->cyclic_first =NULL ; +return &first->async_tx; fail:
@@ -574,6 +579,94 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl, new->desc.ddadr =DDADR_STOP ; new->desc.dcmd |=DCMD_ENDIRQEN ; + chan->dir = dir; + chan->cyclic_first =NULL ; + +return &first->async_tx; + +fail: +if (first) +mmp_pdma_free_desc_list (chan, &first->tx_list); +return NULL ; +} + +static struct dma_async_tx_descriptor *mmp_pdma_prep_dma_cyclic ( +struct dma_chan *dchan,dma_addr_t buf_addr,size_t len, +size_t period_len,enum dma_transfer_direction direction, +unsigned long flags,void *context) +{ +struct mmp_pdma_chan *chan; +struct mmp_pdma_desc_sw *first =NULL , *prev =NULL , *new; +dma_addr_t dma_src, dma_dst; + +if (!dchan || !len || !period_len) +return NULL ; + +/* the buffer length must be a multiple of period_len */ +if (len % period_len !=0 ) +return NULL ; + +if (period_len >PDMA_MAX_DESC_BYTES ) +return NULL ; + + chan =to_mmp_pdma_chan (dchan); + +switch (direction) { +case DMA_MEM_TO_DEV : + dma_src = buf_addr; + dma_dst = chan->dev_addr; +break ; +case DMA_DEV_TO_MEM : + dma_dst = buf_addr; + dma_src = chan->dev_addr; +break ; +default : +dev_err (chan->dev,"Unsupported direction for cyclic DMA\n" ); +return NULL ; + } + + chan->dir = direction; + +do { +/* Allocate the link descriptor from DMA pool */ + new =mmp_pdma_alloc_descriptor (chan); +if (!new) { +dev_err (chan->dev,"no memory for desc\n" ); + goto fail; + } + + new->desc.dcmd = chan->dcmd |DCMD_ENDIRQEN | + (DCMD_LENGTH & period_len); + new->desc.dsadr = dma_src; + new->desc.dtadr = dma_dst; + +if (!first) + first = new; +else + prev->desc.ddadr = new->async_tx.phys; + + new->async_tx.cookie =0 ; +async_tx_ack (&new->async_tx); + + prev = new; + len -= period_len; + +if (chan->dir ==DMA_MEM_TO_DEV ) + dma_src += period_len; +else + dma_dst += period_len; + +/* Insert the link descriptor to the LD ring */ +list_add_tail (&new->node, &first->tx_list); + }while (len); + + first->async_tx.flags = flags;/* client is in control of this ack */ + first->async_tx.cookie = -EBUSY ; + +/* make the cyclic link */ + new->desc.ddadr = first->async_tx.phys; + chan->cyclic_first = first; +return &first->async_tx; fail:
@@ -688,8 +781,23 @@ static void dma_do_tasklet(unsigned long data)LIST_HEAD (chain_cleanup);unsigned long flags; -/* submit pending list; callback for each desc; free desc */ +if (chan->cyclic_first) { +dma_async_tx_callback cb =NULL ; +void *cb_data =NULL ; +spin_lock_irqsave (&chan->desc_lock, flags); + desc = chan->cyclic_first; + cb = desc->async_tx.callback; + cb_data = desc->async_tx.callback_param; +spin_unlock_irqrestore (&chan->desc_lock, flags); + +if (cb) +cb (cb_data); + +return ; + } + +/* submit pending list; callback for each desc; free desc */ spin_lock_irqsave (&chan->desc_lock, flags);list_for_each_entry_safe (desc, _desc, &chan->chain_running, node) {
@@ -886,12 +994,14 @@ static int mmp_pdma_probe(struct platform_device *op)dma_cap_set (DMA_SLAVE , pdev->device.cap_mask);dma_cap_set (DMA_MEMCPY , pdev->device.cap_mask); +dma_cap_set (DMA_CYCLIC , pdev->device.cap_mask); pdev->device.dev = &op->dev; pdev->device.device_alloc_chan_resources = mmp_pdma_alloc_chan_resources; pdev->device.device_free_chan_resources = mmp_pdma_free_chan_resources; pdev->device.device_tx_status = mmp_pdma_tx_status; pdev->device.device_prep_dma_memcpy = mmp_pdma_prep_memcpy; pdev->device.device_prep_slave_sg = mmp_pdma_prep_slave_sg; + pdev->device.device_prep_dma_cyclic = mmp_pdma_prep_dma_cyclic; pdev->device.device_issue_pending = mmp_pdma_issue_pending; pdev->device.device_control = mmp_pdma_control; pdev->device.copy_align =PDMA_ALIGNMENT ;
Signature · 1 line
1.8.3.1
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
In reply to Daniel MackIn order to report the channel's residue, we walk the list of running descriptors, look for those which match the cookie, and then try to find the descriptor which defines upper and lower boundaries that embrace the current transport pointer.
--- drivers/dma/mmp_pdma.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 2 deletions(-)
drivers/dma/mmp_pdma.c +75 -2
@@ -28,8 +28,8 @@#define DALGN 0x00a0#define DINT 0x00f0#define DDADR 0x0200 -#define DSADR 0x0204 -#define DTADR 0x0208 +#define DSADR (n) (0x0204 + ((n) << 4)) +#define DTADR (n) (0x0208 + ((n) << 4))#define DCMD 0x020c#define DCSR_RUN (1 << 31)/* Run Bit (read / write) */
@@ -741,6 +741,78 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,return ret; } +static unsigned int mmp_pdma_residue (struct mmp_pdma_chan *chan, +dma_cookie_t cookie) +{ +struct mmp_pdma_desc_sw *sw; +u32 curr, residue =0 ; +bool passed = false; +bool cyclic = chan->cyclic_first !=NULL ; + +/* +* If the channel does not have a phy pointer anymore, it has already +* been completed. Therefore, its residue is 0. +*/ +if (!chan->phy) +return 0 ; + +if (chan->dir ==DMA_DEV_TO_MEM ) + curr =readl (chan->phy->base +DTADR (chan->phy->idx)); +else + curr =readl (chan->phy->base +DSADR (chan->phy->idx)); + +list_for_each_entry (sw, &chan->chain_running, node) { +u32 start, end, len; + +if (chan->dir ==DMA_DEV_TO_MEM ) + start = sw->desc.dtadr; +else + start = sw->desc.dsadr; + + len = sw->desc.dcmd &DCMD_LENGTH ; + end = start + len; + +/* +* 'passed' will be latched once we found the descriptor which +* lies inside the boundaries of the curr pointer. All +* descriptors that occur in the list _after_ we found that +* partially handled descriptor are still to be processed and +* are hence added to the residual bytes counter. +*/ +if (passed) { + residue += len; + }else if (curr >= start && curr <= end) { + residue += end - curr; + passed = true; + } + +/* +* Descriptors that have the ENDIRQEN bit set mark the end of a +* transaction chain, and the cookie assigned with it has been +* returned previously from mmp_pdma_tx_submit(). +* +* In case we have multiple transactions in the running chain, +* and the cookie does not match the one the user asked us +* about, reset the state variables and start over. +* +* This logic does not apply to cyclic transactions, where all +* descriptors have the ENDIRQEN bit set, and for which we +* can't have multiple transactions on one channel anyway. +*/ +if (!cyclic && (sw->desc.dcmd &DCMD_ENDIRQEN )) { +if (sw->async_tx.cookie != cookie) { + residue =0 ; + passed = false; + }else { +return residue; + } + } + } + +/* We should only get here in case of cyclic transactions */ +return residue; +} +static enum dma_status mmp_pdma_tx_status (struct dma_chan *dchan,dma_cookie_t cookie,struct dma_tx_state *txstate) {
@@ -750,6 +822,7 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,spin_lock_irqsave (&chan->desc_lock, flags); ret =dma_cookie_status (dchan, cookie, txstate); + txstate->residue =mmp_pdma_residue (chan, cookie);spin_unlock_irqrestore (&chan->desc_lock, flags);return ret;
Signature · 1 line
1.8.3.1
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 781
Quoted 13 lines · Daniel Mack
In order to report the channel's residue, we walk the list of running descriptors, look for those which match the cookie, and then try to find the descriptor which defines upper and lower boundaries that embrace the current transport pointer.
drivers/dma/mmp_pdma.c +7
-0+/* +* 'passed' will be latched once we found the descriptor which +* lies inside the boundaries of the curr pointer. All +* descriptors that occur in the list _after_ we found that +* partially handled descriptor are still to be processed and +* are hence added to the residual bytes counter. +*/
do you have multiple descriptors for one transaction? Should be No.
The cookie is assigned to a transaction when it is submitted. so when you see descriptor cookie is less than completed one, then it already completed and should not be in pending list.
drivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825
Quoted 8 lines
static enum
dma_statusmmp_pdma_tx_status(structdma_chan*dchan,drivers/dma/mmp_pdma.c +1
-0dma_cookie_t cookie, struct dma_tx_state *txstate) {@@ -750,6 +822,7 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,spin_lock_irqsave (&chan->desc_lock, flags); ret =dma_cookie_status (dchan, cookie, txstate); + txstate->residue =mmp_pdma_residue (chan, cookie);
here you check ret value first, if descriptor is completed then you will get DMA_SUCCESS and just return that, no need to check the residue. If it is pending, then also just return size of transaction. Only when it is progress you need to calculate and check
~Vinod
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
Hi Vinod,
Sorry for the late response, I've been on vacations.
Quoted 7 lines · Vinod Koul
Quoted 6 lines · Daniel Mack
In order to report the channel's residue, we walk the list of running descriptors, look for those which match the cookie, and then try to find the descriptor which defines upper and lower boundaries that embrace the current transport pointer.
Quoted 8 lines
drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 781
Quoted 7 lines
drivers/dma/mmp_pdma.c +7
-0+/* +* 'passed' will be latched once we found the descriptor which +* lies inside the boundaries of the curr pointer. All +* descriptors that occur in the list _after_ we found that +* partially handled descriptor are still to be processed and +* are hence added to the residual bytes counter. +*/ do you have multiple descriptors for one transaction? Should be No.
Sure, that can be the case. One transaction could span across multiple descriptors, especially if its overall length exceeds the maximum length of one descriptor.
Quoted 3 lines
The cookie is assigned to a transaction when it is submitted. so when you see descriptor cookie is less than completed one, then it already completed and should not be in pending list.
Hmm, what about an integer overrun?
Quoted 5 lines
Quoted 5 lines
static enum
dma_statusmmp_pdma_tx_status(structdma_chan*dchan,drivers/dma/mmp_pdma.c +0
-0dma_cookie_t cookie, struct dma_tx_state *txstate) {@@ -750,6 +822,7 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,
Quoted 7 lines
drivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825
Quoted 3 lines
drivers/dma/mmp_pdma.c +1
-0spin_lock_irqsave (&chan->desc_lock, flags); ret =dma_cookie_status (dchan, cookie, txstate); + txstate->residue =mmp_pdma_residue (chan, cookie);here you check ret value first, if descriptor is completed then you will get
DMA_SUCCESSand just return that, no need to check the residue. If it is pending, then also just return size of transaction. Only when it is progress you need to calculate and check
Because in case of success, the residue is always 0. Alright, will change that.
Thanks for the feedback!
Daniel
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
Quoted 21 lines · Daniel Mack
Hi Vinod,
Sorry for the late response, I've been on vacations.
Quoted 7 lines · Vinod Koul
Quoted 6 lines · Daniel Mack
In order to report the channel's residue, we walk the list of running descriptors, look for those which match the cookie, and then try to find the descriptor which defines upper and lower boundaries that embrace the current transport pointer.
Quoted 8 lines
drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 781
Quoted 7 lines
drivers/dma/mmp_pdma.c +7
-0+/* +* 'passed' will be latched once we found the descriptor which +* lies inside the boundaries of the curr pointer. All +* descriptors that occur in the list _after_ we found that +* partially handled descriptor are still to be processed and +* are hence added to the residual bytes counter. +*/ do you have multiple descriptors for one transaction? Should be No.
Quoted 3 lines
Sure, that can be the case. One transaction could span across multiple descriptors, especially if its overall length exceeds the maximum length of one descriptor.
Yes and these addiional descriptor for a transacation should be child descriptors.
The child descriptors should have ->parent point to parent descriptor and ->next to next in the chain of children.
Quoted 5 lines
Quoted 3 lines
The cookie is assigned to a transaction when it is submitted. so when you see descriptor cookie is less than completed one, then it already completed and should not be in pending list.
Hmm, what about an integer overrun?
The cookie needs to wrapped to 1. If you use virtual dma code then it would be done. Since the variable is 32bit, i dont think you will have so many eonding trasaction so i havent seen any concerns on that.
~Vinod
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
Hi Vinod,
Very sorry for such a long delay. I only get to pick up on this topic now. Hope you can remember the context of the discussion :)
Quoted 22 lines · Vinod Koul
Quoted 21 lines · Daniel Mack
Hi Vinod,
Sorry for the late response, I've been on vacations.
Quoted 15 lines · Vinod Koul
drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 781
Quoted 13 lines · Daniel Mack
In order to report the channel's residue, we walk the list of running descriptors, look for those which match the cookie, and then try to find the descriptor which defines upper and lower boundaries that embrace the current transport pointer.
drivers/dma/mmp_pdma.c +7
-0+/* +* 'passed' will be latched once we found the descriptor which +* lies inside the boundaries of the curr pointer. All +* descriptors that occur in the list _after_ we found that +* partially handled descriptor are still to be processed and +* are hence added to the residual bytes counter. +*/ do you have multiple descriptors for one transaction? Should be No.
Quoted 7 lines
Quoted 3 lines
Sure, that can be the case. One transaction could span across multiple descriptors, especially if its overall length exceeds the maximum length of one descriptor.
Yes and these addiional descriptor for a transacation should be child descriptors.
The child descriptors should have ->parent point to parent descriptor and ->next to next in the chain of children.
I looked into your idea and implemented it, but I really feel having an explicit chain pointer for each descriptor is redundant information.
Hence, let me summarize again how the driver currently works:
* Any of the prep_* function will allocate a number of descriptors to accommodate the payload, and link them all together via the 'node' list_head, forming a transaction.
* The last descriptor in a transaction has the DCMD_ENDIRQEN flag set.
* When tx_submit() is called, each descriptor in the linked list will be assigned a cookie, and then entire list of the transaction is appended to the chain_pending list. At this point, the information of where a transaction ends is no longer visible in the 'node' list head.
* start_pending_queue() moves the chain_pending entries to chain_running.
When determining the channel's residue, we need to find the transaction currently in progress, and then count upwards until we reach the end of the transaction chain.
So, while I could add a ->next pointer to the descriptors, and only use that to link up descriptors of one transaction in the prep_*() functions, that extra information doesn't actually buy us anything, as the same information is already stored in the DCMD_ENDIRQEN flag.
I rebased the residue patch on top of Joe's cleanup work, and I can resubmit if you want me to. Maybe that serves as a better foundation for the ongoing discussion :)
Many thanks,
Daniel
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
Quoted 52 lines · Daniel Mack
Hi Vinod,
Very sorry for such a long delay. I only get to pick up on this topic now. Hope you can remember the context of the discussion :)
Quoted 22 lines · Vinod Koul
Quoted 21 lines · Daniel Mack
Hi Vinod,
Sorry for the late response, I've been on vacations.
Quoted 15 lines · Vinod Koul
drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 781
Quoted 13 lines · Daniel Mack
In order to report the channel's residue, we walk the list of running descriptors, look for those which match the cookie, and then try to find the descriptor which defines upper and lower boundaries that embrace the current transport pointer.
drivers/dma/mmp_pdma.c +7
-0+/* +* 'passed' will be latched once we found the descriptor which +* lies inside the boundaries of the curr pointer. All +* descriptors that occur in the list _after_ we found that +* partially handled descriptor are still to be processed and +* are hence added to the residual bytes counter. +*/ do you have multiple descriptors for one transaction? Should be No.
Quoted 7 lines
Quoted 3 lines
Sure, that can be the case. One transaction could span across multiple descriptors, especially if its overall length exceeds the maximum length of one descriptor.
Yes and these addiional descriptor for a transacation should be child descriptors.
The child descriptors should have ->parent point to parent descriptor and ->next to next in the chain of children.I looked into your idea and implemented it, but I really feel having an explicit chain pointer for each descriptor is redundant information.
Hence, let me summarize again how the driver currently works:
* Any of the prep_* function will allocate a number of descriptors to accommodate the payload, and link them all together via the 'node'
list_head, forming a transaction.* The last descriptor in a transaction has the
DCMD_ENDIRQENflag set.* When
tx_submit()is called, each descriptor in the linked list will be assigned a cookie, and then entire list of the transaction is appended to thechain_pendinglist. At this point, the information of where a transaction ends is no longer visible in the 'node' list head.
Quoted 5 lines
*
start_pending_queue()moves thechain_pendingentries tochain_running.When determining the channel's residue, we need to find the transaction currently in progress, and then count upwards until we reach the end of the transaction chain.
ah here is the catch! You dont get reside for a channel. You get for the respetive transaction represented by the descriptor!
So lets establish few rules:
- Assume you have transactions A, B, C and D in a list
- on sumbmit (assume serial), you get descriptor 1, 2, 3 and 4 respectively
- Now residue is for a descriptor, not a series of descriptors!, so
- If DMA is started and currently 1 is being transferred, then
- residue on 1 will give remaining bytes of 1
- residue on 2 will give full length
At client, you can sum up and decide how much is remianing for your point of interest.
Yes things will be a bit different if your submit a transaction and dma driver splits to multiple descriptors and the client driver doesnt know about them. But since split is done by DMA driver, it know how to manage
Quoted 10 lines
So, while I could add a ->next pointer to the descriptors, and only use that to link up descriptors of one transaction in the prep_*() functions, that extra information doesn't actually buy us anything, as the same information is already stored in the
DCMD_ENDIRQENflag.I rebased the residue patch on top of Joe's cleanup work, and I can resubmit if you want me to. Maybe that serves as a better foundation for the ongoing discussion :)
Sure pls do post...
--
~Vinod
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
Hi,
Quoted 18 lines · Vinod Koul
Quoted 16 lines · Daniel Mack
I looked into your idea and implemented it, but I really feel having an explicit chain pointer for each descriptor is redundant information.
Hence, let me summarize again how the driver currently works:
* Any of the prep_* function will allocate a number of descriptors to accommodate the payload, and link them all together via the 'node'
list_head, forming a transaction.* The last descriptor in a transaction has the
DCMD_ENDIRQENflag set.* When
tx_submit()is called, each descriptor in the linked list will be assigned a cookie, and then entire list of the transaction is appended to thechain_pendinglist. At this point, the information of where a transaction ends is no longer visible in the 'node' list head.
Quoted 7 lines
Quoted 5 lines
*
start_pending_queue()moves thechain_pendingentries tochain_running.When determining the channel's residue, we need to find the transaction currently in progress, and then count upwards until we reach the end of the transaction chain.
ah here is the catch! You dont get reside for a channel. You get for the respetive transaction represented by the descriptor!
Yes, exactly. I think part of the reason why we're not on the same page is a misleading wording on my side. Let me try and explain.
Quoted 3 lines
So lets establish few rules:
- Assume you have transactions A, B, C and D in a list
- on sumbmit (assume serial), you get descriptor 1, 2, 3 and 4 respectively
You're referring to dma_async_tx_descriptor, while I was talking about mmp_pdma_desc_sw. IOW: what's handed out by the driver from any of its prep_* functions are dma_async_tx_descriptors, which consist of N chained mmp_pdma_desc_sw descriptors internally.
Another specialty of this particular driver is that each mmp_pdma_desc_sw is assigned a dma_cookie_t, but only the last one in a chained list is handed out to the user.
Following your example, the transaction-cookie matching could look like this, for example:
A: 1, 2, 3, 4 B: 5 C: 6, 7, 8 D: 9, 10
So the only cookies the user 'knows' about and query the residue for, are 4, 5, 8 and 10.
Quoted 7 lines
- Now residue is for a descriptor, not a series of descriptors!, so
- If DMA is started and currently 1 is being transferred, then
- residue on 1 will give remaining bytes of 1
- residue on 2 will give full lengthAt client, you can sum up and decide how much is remianing for your point of interest.
Absolutely. I understand that concept :)
In this driver, however, I have to deal with multiple mmp_pdma_desc_sw descriptors that are all chained up in the channel's running list. Hence, my implementation goes as follows:
* Walk the list of all mmp_pdma_desc_sw until we find the end of a
transaction chain; IOW: find the end of one dma_async_tx_descriptor. This end entry has the DCMD_ENDIRQEN flag set.
* Check if that mmp_pdma_desc_sw corresponds with the cookie the user
wants to know the residue for, and if it does, return the residue that was calculated during the iteration.
* Otherwise, reset the internal state and carry on.
Quoted 4 lines
Quoted 3 lines
I rebased the residue patch on top of Joe's cleanup work, and I can resubmit if you want me to. Maybe that serves as a better foundation for the ongoing discussion :)
Sure pls do post...
Will do.
Thanks,
Daniel
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
In reply to Daniel Mackdrivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825
Quoted 5 lines · Daniel Mack
drivers/dma/mmp_pdma.c +1
-0@@ -750,6 +822,7 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,spin_lock_irqsave (&chan->desc_lock, flags); ret =dma_cookie_status (dchan, cookie, txstate); + txstate->residue =mmp_pdma_residue (chan, cookie);
Please consider using dma_set_residue() here - it is legal to call the tx status function with a NULL txstate.
PATCH v5 4/5 dma: mmp_pdma: add support for residue reporting
Quoted 10 lines · Russell King - ARM Linux
drivers/dma/mmp_pdma.c static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan, · line 825
Quoted 5 lines · Daniel Mack
drivers/dma/mmp_pdma.c +1
-0@@ -750,6 +822,7 @@ static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,spin_lock_irqsave (&chan->desc_lock, flags); ret =dma_cookie_status (dchan, cookie, txstate); + txstate->residue =mmp_pdma_residue (chan, cookie);Please consider using
dma_set_residue()here - it is legal to call the tx status function with a NULL txstate.
Will do. Thanks for the hint.
Daniel
PATCH v5 5/5 dma: mmp_pdma: set DMA_PRIVATE
In reply to Daniel MackAs the driver now has its own xlate function and makes use of the dma_get_slave_channel(), we need to manually set the DMA_PRIVATE flags.
Drivers which rely on of_dma_simple_xlate() do implicitly the same by going through __dma_request_channel().
--- drivers/dma/mmp_pdma.c | 1 + 1 file changed, 1 insertion(+)
drivers/dma/mmp_pdma.c +1 -0
@@ -1068,6 +1068,7 @@ static int mmp_pdma_probe(struct platform_device *op)dma_cap_set (DMA_SLAVE , pdev->device.cap_mask);dma_cap_set (DMA_MEMCPY , pdev->device.cap_mask);dma_cap_set (DMA_CYCLIC , pdev->device.cap_mask); +dma_cap_set (DMA_PRIVATE , pdev->device.cap_mask); pdev->device.dev = &op->dev; pdev->device.device_alloc_chan_resources = mmp_pdma_alloc_chan_resources; pdev->device.device_free_chan_resources = mmp_pdma_free_chan_resources;
Signature · 1 line
1.8.3.1
Vinod Koul
Appliedv5 cover dma: pdma: cyclic, residue, DMA_PRIVATE
In reply to Daniel MackQuoted 6 lines · Daniel Mack
This is v5 of my series to teach the pdma driver more required functions.
In contrast to v4, it has yet again grown by one more patch (2/5), which removes code that clears
DCMD_ENDIRQENat the end of the pending chain. We want to leave of theDCMD_ENDIRQENintact in order to identify where a transaction ends.
Applied, 1-3 & 5. The 4th one still needs some work
~Vinod
v5 cover dma: pdma: cyclic, residue, DMA_PRIVATE
Quoted 10 lines · Vinod Koul
Quoted 1 line · Daniel Mack
This is v5 of my series to teach the pdma driver more required
functions.
Quoted 3 lines
In contrast to v4, it has yet again grown by one more patch (2/5), which removes code that clears
DCMD_ENDIRQENat the end of thepending
Quoted 2 lines
chain. We want to leave of the
DCMD_ENDIRQENintact in order to identify where a transaction ends.Applied, 1-3 & 5. The 4th one still needs some work
Thanks. I'm currently travelling and AFK. Will get back to this issue once I'm back, but that will most certainly be after the merge window has opened.
Daniel