dma: mmp_pdma: add support for residue reporting

A channel can accommodate more than one transaction, each consisting of multiple descriptors, the last of which has the DCMD_ENDIRQEN bit set.

In order to report the channel's residue, we hence have to 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. Once it is found, walk forward until we find the descriptor that tells us about the end of a transaction via a set DCMD_ENDIRQEN bit.

Signed-off-by Daniel Mack <[email protected]>

---

Hi Vinod, everyone,

I'd like to get the disussion regarding this patch started again which left off here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/217429.html

I think the biggest issue in the previous discussion was a confusion about the term 'descriptor', as it refers to both the internal pdma implementation detail as well as the handle used in the dma subsystem.

I hope I explained that well enough in the link above.

Many thanks,
Daniel

 drivers/dma/mmp_pdma.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 3 deletions(-)

drivers/dma/mmp_pdma.c +60 -2

index b439679..2eb1c10 100644
@@ -29,8 +29,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	BIT(31)	/* Run Bit (read / write) */

@@ -748,11 +748,92 @@ static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd,
 	return 0;
 }
 
+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.

drivers/dma/mmp_pdma.c +6 -0

+	}
+
+	/* We should only get here in case of cyclic transactions */
+	return residue;
+}
+
 static enum dma_status mmp_pdma_tx_status(struct dma_chan *dchan,

On Mon, Feb 17, 2014 at 12:29:06PM +0100, Daniel Mack wrote:
Sorry for delay, This hit my inboxwhen my vacation started...

Quoted 61 lines

drivers/dma/mmp_pdma.c +61 -0

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

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 818

Quoted 7 lines

drivers/dma/mmp_pdma.c +7 -0

+
+		if (sw->async_tx.cookie == cookie) {
+			return residue;
+		} else {
+			residue = 0;
+			passed = false;
+		}

for cookie in queue, the residue is not 0 but complete length of transaction.

Possibly you should check this in mmp_pdma_tx_status and only invoke current function for current transaction.

Secondly, if you have 3 descriptor in the chain_running, the residue on last will add all lengths till last one, that is not something we wnat.

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 834

Quoted 3 lines

drivers/dma/mmp_pdma.c +3 -0

+	ret = dma_cookie_status(dchan, cookie, txstate);
+	if (likely(ret != DMA_ERROR))
+		dma_set_residue(txstate, mmp_pdma_residue(chan, cookie));

Pls check if resuide is not NULL, then only invoke mmp_pdma_residue()

Quoted 1 line

for cookie in queue, the residue is not 0 but complete length of transaction.

Hmm, the code quoted above simply resets the internal residue counter to 0, and the loop will continue increasing it as it iterates more members of the chain_running list. Maybe the example below clarifies.

Again, the problem here is that we have multiple mmp_pdma_desc_sw members that belong to the same transaction.

Quoted 3 lines

Possibly you should check this in mmp_pdma_tx_status and only invoke current function for current transaction.

I can't, because I don't have a specific pointer that leads me to the current transaction inside the chain_running list. That's why I have to walk the list at all.

Quoted 2 lines

Secondly, if you have 3 descriptor in the chain_running, the residue on last will add all lengths till last one, that is not something we wnat.

Not sure wheter I follow, or if I still have a knot in my brain in understanding how the residue logic is supposed to work. Allow me illustrate this with an example, maybe that'd help either one of the two of us :)

Let's assume we have 3 descriptors (in terms of mmp_pdma_desc_sw) in chain_running, and let's assume there's only one transaction, so all 3 descriptors are nicely chained up and the last one has the cookie we're looking for. Further, let's assume the DMA engine is half way through on the 2nd descriptor, at byte 1500.

So the layout looks something like this:

bytes: 0 1024 2048 3072
descs: |----(1)----|----(2)----|----(3)----|
curr: ^

What will happen with the code above is:

1. We start off with passed=false and residue=0
2. The first descriptor is looked at, /passed/ is false, and (curr >= start && curr <= end) is false, so residue remains 0.
3. The second descriptor is looked at, /passed/ is false, but (curr >= start && curr <= end) is true, and the residue is increased by what's left to do in this descriptor, and /passed/ is latched so we know we've passed /curr/ in the iteration.
4. The third descriptor is looked at, and as we've passed the /curr/ mark, all bytes of the descriptor are still to go, and so we add the entire length of that third descriptor to the residue sum.
5. The cookie comparison in the end simply exists to address the fact that we might have operated on an unreleated descriptor, and we have to start over.

So in short, the logic will return the bytes that are not yet processed for a specific transaction, which is the expected thing to do, right?

Quoted 5 lines

drivers/dma/mmp_pdma.c static int mmp_pdma_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, · line 834

Quoted 4 lines

drivers/dma/mmp_pdma.c +3 -0

+	ret = dma_cookie_status(dchan, cookie, txstate);
+	if (likely(ret != DMA_ERROR))
+		dma_set_residue(txstate, mmp_pdma_residue(chan, cookie));

Pls check if resuide is not NULL, then only invoke mmp_pdma_residue()

Did you mean "Pls check if *txstate* is not NULL"? I can fix that, yes.

Thanks again for your time!
Daniel

Quoted 4 lines · Daniel Mack

5. The cookie comparison in the end simply exists to address the fact that we might have operated on an unreleated descriptor, and we have to start over.

Quoted 2 lines

So in short, the logic will return the bytes that are not yet processed for a specific transaction, which is the expected thing to do, right?

Looks fine then BUT I have another questions.

Assuming that you have two txn submitted and driver split them to 3 descriptors each, then in that case the driver would walk over all 6 descriptors and sum up the value, which would lead to incorrect residue. It will work for single pending txn only, right?

While at it and looking at the code again, I think right solution maybe to update the parent child in the descriptors. So on query you simply walk the list for all child descriptors and continue. But the parent and child are defined under CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH.

So adding Dan (his updated email id), would it be okay if we make these as generic in descriptor and use them to manage larger length transactions in drivers?

Quoted 1 line

Did you mean "Pls check if *txstate* is not NULL"? I can fix that, yes.

Yup!

Hi Vinod,

Quoted 6 lines · Vinod Koul

Quoted 4 lines · Daniel Mack

5. The cookie comparison in the end simply exists to address the fact that we might have operated on an unreleated descriptor, and we have to start over.

Quoted 8 lines

Quoted 2 lines

So in short, the logic will return the bytes that are not yet processed for a specific transaction, which is the expected thing to do, right?

Looks fine then BUT I have another questions.

Assuming that you have two txn submitted and driver split them to 3 descriptors each, then in that case the driver would walk over all 6 descriptors and sum up the value, which would lead to incorrect residue. It will work for single pending txn only, right?

No, because each of the two txn would have a different cookie set, and the residue function is called for one specific cookie so we know which one we're looking for. That's the reason why we start over if at the end of an iterated transaction chain, if we recognize that the cookie doesn't match, we start over.

Quoted 4 lines

While at it and looking at the code again, I think right solution maybe to update the parent child in the descriptors. So on query you simply walk the list for all child descriptors and continue. But the parent and child are defined under CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH.

That's another solution, but it's redundant information after all. Ultimately, it makes the driver more complicated and introduces one more area of potentially inconsistent pointers.

Quoted 3 lines

So adding Dan (his updated email id), would it be okay if we make these as generic in descriptor and use them to manage larger length transactions in drivers?

It might add to the readability of the drivers, but for the current case, I don't think it's really necessary.

Quoted 33 lines · Daniel Mack

Hi Vinod,

Quoted 6 lines · Vinod Koul

Quoted 4 lines · Daniel Mack

5. The cookie comparison in the end simply exists to address the fact that we might have operated on an unreleated descriptor, and we have to start over.

Quoted 8 lines

Quoted 2 lines

So in short, the logic will return the bytes that are not yet processed for a specific transaction, which is the expected thing to do, right?

Looks fine then BUT I have another questions.

Assuming that you have two txn submitted and driver split them to 3 descriptors each, then in that case the driver would walk over all 6 descriptors and sum up the value, which would lead to incorrect residue. It will work for single pending txn only, right?

No, because each of the two txn would have a different cookie set, and the residue function is called for one specific cookie so we know which one we're looking for. That's the reason why we start over if at the end of an iterated transaction chain, if we recognize that the cookie doesn't match, we start over.

Quoted 4 lines

While at it and looking at the code again, I think right solution maybe to update the parent child in the descriptors. So on query you simply walk the list for all child descriptors and continue. But the parent and child are defined under CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH.

That's another solution, but it's redundant information after all. Ultimately, it makes the driver more complicated and introduces one more area of potentially inconsistent pointers.

Quoted 6 lines

Quoted 3 lines

So adding Dan (his updated email id), would it be okay if we make these as generic in descriptor and use them to manage larger length transactions in drivers?

It might add to the readability of the drivers, but for the current case, I don't think it's really necessary.

That is because you are maintaining the current descriptors in chain_running. If we use above method then you dont need to use this, right?

Quoted 4 lines

Quoted 2 lines

It might add to the readability of the drivers, but for the current case, I don't think it's really necessary.

That is because you are maintaining the current descriptors in chain_running. If we use above method then you dont need to use this, right?

Jup, but that would result in a rewrite of larger parts of the code. The concept of hot-linking the two list so there's only one resulting list of currently active descriptors is built-in deeply into the driver's concept.

Quoted 5 lines · Daniel Mack

Quoted 4 lines

Quoted 2 lines

It might add to the readability of the drivers, but for the current case, I don't think it's really necessary.

That is because you are maintaining the current descriptors in chain_running. If we use above method then you dont need to use this, right?

Quoted 4 lines

Jup, but that would result in a rewrite of larger parts of the code. The concept of hot-linking the two list so there's only one resulting list of currently active descriptors is built-in deeply into the driver's concept.

Ah I suspected so :)

I think for now this is fine you can perhaps upgrade this later :)

Quoted 10 lines · Vinod Koul

Quoted 9 lines · Daniel Mack

Quoted 4 lines

Quoted 2 lines

It might add to the readability of the drivers, but for the current case, I don't think it's really necessary.

That is because you are maintaining the current descriptors in chain_running. If we use above method then you dont need to use this, right?

Jup, but that would result in a rewrite of larger parts of the code. The concept of hot-linking the two list so there's only one resulting list of currently active descriptors is built-in deeply into the driver's concept.

Quoted 3 lines

Ah I suspected so :)

I think for now this is fine you can perhaps upgrade this later :)

So, do you want me to resend with the minor dma_set_residue() change?

Quoted 11 lines · Daniel Mack

Quoted 10 lines · Vinod Koul

Quoted 9 lines · Daniel Mack

Quoted 4 lines

Quoted 2 lines

It might add to the readability of the drivers, but for the current case, I don't think it's really necessary.

That is because you are maintaining the current descriptors in chain_running. If we use above method then you dont need to use this, right?

Jup, but that would result in a rewrite of larger parts of the code. The concept of hot-linking the two list so there's only one resulting list of currently active descriptors is built-in deeply into the driver's concept.

Quoted 5 lines

Quoted 3 lines

Ah I suspected so :)

I think for now this is fine you can perhaps upgrade this later :)

So, do you want me to resend with the minor dma_set_residue() change?

Yes please...

Quoted 12 lines · Vinod Koul

Quoted 11 lines · Daniel Mack

Quoted 10 lines · Vinod Koul

Quoted 9 lines · Daniel Mack

Quoted 4 lines

Quoted 2 lines

It might add to the readability of the drivers, but for the current case, I don't think it's really necessary.

That is because you are maintaining the current descriptors in chain_running. If we use above method then you dont need to use this, right?

Jup, but that would result in a rewrite of larger parts of the code. The concept of hot-linking the two list so there's only one resulting list of currently active descriptors is built-in deeply into the driver's concept.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Ah I suspected so :)

I think for now this is fine you can perhaps upgrade this later :)

So, do you want me to resend with the minor dma_set_residue() change?

Yes please...

Ah, just checked again and my call to

	dma_set_residue(txstate, mmp_pdma_residue(chan, cookie));

resolves to ...

static inline void dma_set_residue(struct dma_tx_state *state, u32 residue)

{
        if (state)
                state->residue = residue;
}

So there's no need to check for txstate != NULL on the caller side :)

Hi Vinod,

On 04/16/2014 06:40 PM, Daniel Mack wrote:

Just in case you missed that over the other mmp_pdma patches: as pointed out above, dma_set_residue() already does the NULL pointer check you asked for, so there's nothing to fix up here. IIRC, we were eventually on the same page regarding all the other details, right?