[PATCH v5 0/5] dma: pdma: cyclic, residue, DMA_PRIVATE

[PATCH v5 0/5] dma: pdma: cyclic, residue, DMA_PRIVATE

[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_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(-)

-- 
1.8.3.1

[PATCH v5 1/5] dma: mmp_pdma: only complete one transaction from dma_do_tasklet()

[Daniel Mack]

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.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 579f79a..9929f85 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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);
-- 
1.8.3.1

[PATCH v5 2/5] dma: mmp_pdma: don't clear DCMD_ENDIRQEN at end of pending chain

[Daniel Mack]

In 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.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 9929f85..31e9a71 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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);
 
-- 
1.8.3.1

[PATCH v5 3/5] dma: mmp_pdma: add support for cyclic DMA descriptors

[Daniel Mack]

Provide a callback to prepare cyclic DMA transfers.
This is for instance needed for audio channel transport.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index 31e9a71..d82a4f6 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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;
-- 
1.8.3.1

[PATCH v5 4/5] dma: mmp_pdma: add support for residue reporting

[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.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index d82a4f6..efb583f 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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;
-- 
1.8.3.1

[PATCH v5 4/5] dma: mmp_pdma: add support for residue reporting

[Vinod Koul]

On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote:
> 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.

> 
> +		/*
> +		 * '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.

>  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);
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

[Daniel Mack]

Hi Vinod,

Sorry for the late response, I've been on vacations.

On 25.08.2013 18:33, Vinod Koul wrote:
> On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote:
>> 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.
> 
>>
>> +		/*
>> +		 * '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.

> 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?

>>  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);
> 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

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

[Vinod Koul]

On Tue, Sep 10, 2013 at 05:46:19PM +0200, Daniel Mack wrote:
> Hi Vinod,
> 
> Sorry for the late response, I've been on vacations.
> 
> On 25.08.2013 18:33, Vinod Koul wrote:
> > On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote:
> >> 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.
> > 
> >>
> >> +		/*
> >> +		 * '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.
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.

> > 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

[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 :)

On 09/13/2013 06:51 AM, Vinod Koul wrote:
> On Tue, Sep 10, 2013 at 05:46:19PM +0200, Daniel Mack wrote:
>> Hi Vinod,
>>
>> Sorry for the late response, I've been on vacations.
>>
>> On 25.08.2013 18:33, Vinod Koul wrote:
>>> On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote:
>>>> 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.
>>>
>>>>
>>>> +		/*
>>>> +		 * '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.
> 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

[Vinod Koul]

On Mon, Dec 09, 2013 at 08:00:51PM +0100, Daniel Mack wrote:
> 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 :)
> 
> On 09/13/2013 06:51 AM, Vinod Koul wrote:
> > On Tue, Sep 10, 2013 at 05:46:19PM +0200, Daniel Mack wrote:
> >> Hi Vinod,
> >>
> >> Sorry for the late response, I've been on vacations.
> >>
> >> On 25.08.2013 18:33, Vinod Koul wrote:
> >>> On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote:
> >>>> 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.
> >>>
> >>>>
> >>>> +		/*
> >>>> +		 * '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.
> > 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.
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

> 
> 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 :)
Sure pls do post...

--
~Vinod

[PATCH v5 4/5] dma: mmp_pdma: add support for residue reporting

[Daniel Mack]

Hi,

On 12/10/2013 05:11 PM, Vinod Koul wrote:
> On Mon, Dec 09, 2013 at 08:00:51PM +0100, Daniel Mack wrote:

>> 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.
> 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.

> 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.

> - 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.

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.

>> 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

[Russell King - ARM Linux]

On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote:
> @@ -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

[Daniel Mack]

On 25.08.2013 20:08, Russell King - ARM Linux wrote:
> On Wed, Aug 21, 2013 at 02:08:57PM +0200, Daniel Mack wrote:
>> @@ -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

[Daniel Mack]

As 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().

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/mmp_pdma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index efb583f..3efde2d 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -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;
-- 
1.8.3.1

[PATCH v5 0/5] dma: pdma: cyclic, residue, DMA_PRIVATE

[Vinod Koul]

On Wed, Aug 21, 2013 at 02:08:53PM +0200, Daniel Mack wrote:
> 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.
Applied, 1-3 & 5. The 4th one still needs some work

~Vinod

[PATCH v5 0/5] dma: pdma: cyclic, residue, DMA_PRIVATE

[Daniel Mack]

Vinod Koul <vinod.koul@intel.com> wrote:
>On Wed, Aug 21, 2013 at 02:08:53PM +0200, Daniel Mack wrote:
>> 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.
>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