| Message ID | 1392636546-15541-1-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 17, 2014 at 12:29:06PM +0100, Daniel Mack wrote: > 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 <zonque@gmail.com> > --- > > Hi Vinod, everyone, > > I'd like to get the disussion regarding this patch started again > which left off here: Sorry for delay, This hit my inboxwhen my vacation started... > > 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(-) > > diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c > index b439679..2eb1c10 100644 > --- a/drivers/dma/mmp_pdma.c > +++ b/drivers/dma/mmp_pdma.c > @@ -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. > + */ > + if (cyclic || !(sw->desc.dcmd & DCMD_ENDIRQEN)) > + continue; > + > + 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. Perhpas when you fix above it should be okay > + } > + > + /* 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) > { > - return dma_cookie_status(dchan, cookie, txstate); > + struct mmp_pdma_chan *chan = to_mmp_pdma_chan(dchan); > + enum dma_status ret; > + > + 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()
Hi Vinod, On 03/19/2014 04:13 PM, Vinod Koul wrote: > On Mon, Feb 17, 2014 at 12:29:06PM +0100, Daniel Mack wrote: > Sorry for delay, This hit my inboxwhen my vacation started... No problem :) >> +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; >> + >> + 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. 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. > 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. > 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? >> + 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
On Wed, Apr 09, 2014 at 06:35:15PM +0200, Daniel Mack wrote: > > 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. Okay i missed that point, again. > > > 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. > > > 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? 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? > > >> + 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. Yup! > > > Thanks again for your time! > Daniel >
On Wed, Apr 16, 2014 at 10:28:45AM +0200, Daniel Mack wrote: > Hi Vinod, > > On 04/16/2014 08:45 AM, Vinod Koul wrote: > > On Wed, Apr 09, 2014 at 06:35:15PM +0200, Daniel Mack wrote: > > >> 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? > > 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. > > > 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. > > > 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?
Hi Vinod, On 04/16/2014 08:45 AM, Vinod Koul wrote: > On Wed, Apr 09, 2014 at 06:35:15PM +0200, Daniel Mack wrote: >> 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? > 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. > 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. > 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. >> Did you mean "Pls check if *txstate* is not NULL"? I can fix that, yes. > Yup! Ok, but I'll wait with that until we have agreed on the details above :) Thanks, Daniel
On 04/16/2014 10:23 AM, Vinod Koul wrote: > On Wed, Apr 16, 2014 at 10:28:45AM +0200, Daniel Mack wrote: >> Hi Vinod, >> >> On 04/16/2014 08:45 AM, Vinod Koul wrote: >>> On Wed, Apr 09, 2014 at 06:35:15PM +0200, Daniel Mack wrote: >> >>>> 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? >>> 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. >> >>> 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. >> >>> 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? 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. Thanks, Daniel
On Wed, Apr 16, 2014 at 10:38:10AM +0200, Daniel Mack wrote: > >> 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. Ah I suspected so :) I think for now this is fine you can perhaps upgrade this later :)
On 04/16/2014 11:09 AM, Vinod Koul wrote: > On Wed, Apr 16, 2014 at 10:38:10AM +0200, Daniel Mack wrote: >>>> 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. > 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? Thanks, Daniel
On Wed, Apr 16, 2014 at 04:59:13PM +0200, Daniel Mack wrote: > On 04/16/2014 11:09 AM, Vinod Koul wrote: > > On Wed, Apr 16, 2014 at 10:38:10AM +0200, Daniel Mack wrote: > >>>> 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. > > 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...
On 04/16/2014 06:01 PM, Vinod Koul wrote: > On Wed, Apr 16, 2014 at 04:59:13PM +0200, Daniel Mack wrote: >> On 04/16/2014 11:09 AM, Vinod Koul wrote: >>> On Wed, Apr 16, 2014 at 10:38:10AM +0200, Daniel Mack wrote: >>>>>> 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. >>> 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 :) Thanks, Daniel
Hi Vinod, On 04/16/2014 06:40 PM, Daniel Mack wrote: > On 04/16/2014 06:01 PM, Vinod Koul wrote: >> On Wed, Apr 16, 2014 at 04:59:13PM +0200, Daniel Mack wrote: >>> On 04/16/2014 11:09 AM, Vinod Koul wrote: >>>> On Wed, Apr 16, 2014 at 10:38:10AM +0200, Daniel Mack wrote: >>>>>>> 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. >>>> 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 :) 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? Many thanks, Daniel
On Sat, May 03, 2014 at 12:29:02AM +0200, Daniel Mack wrote: > 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? Yes and now i have applied the patch. Thanks
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c index b439679..2eb1c10 100644 --- a/drivers/dma/mmp_pdma.c +++ b/drivers/dma/mmp_pdma.c @@ -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. + */ + if (cyclic || !(sw->desc.dcmd & DCMD_ENDIRQEN)) + continue; + + if (sw->async_tx.cookie == cookie) { + return residue; + } else { + residue = 0; + passed = false; + } + } + + /* 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) { - return dma_cookie_status(dchan, cookie, txstate); + struct mmp_pdma_chan *chan = to_mmp_pdma_chan(dchan); + enum dma_status ret; + + ret = dma_cookie_status(dchan, cookie, txstate); + if (likely(ret != DMA_ERROR)) + dma_set_residue(txstate, mmp_pdma_residue(chan, cookie)); + + return ret; } /**
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 <zonque@gmail.com> --- 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(-)