| Message ID | 20140417143249.889774979@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/17/2014 09:40 AM, Thomas Gleixner wrote: > Reading destination and source is pointless. In DEV_TO_MEM transfers > we are only interested in the destination, in MEM_TO_DEV we care about > the source. In MEM_TO_MEM it really does not matter which one you > read. > > Remove the extra pointer and select dest/source via a bool. > > Reading the src/dst data from an active parameter set in the EDMA > parameter RAM is not reliable, as there might be a concurrent update > from the controller. > > But experimentation showed, that a double readout with comparing the > results and a limited loop works nicely. I've actually never found a > case where the loop limit triggered, but we have it there for sanity > reasons. In case it triggers we return -EBUSY and let the caller deal > with it. > > Remove the export of this function while at it. The only potential > user is the dmaengine and that's always builtin. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > arch/arm/common/edma.c | 48 ++++++++++++++++++++++++++----------- > include/linux/platform_data/edma.h | 2 - > 2 files changed, 35 insertions(+), 15 deletions(-) > > Index: linux/arch/arm/common/edma.c > =================================================================== > --- linux.orig/arch/arm/common/edma.c > +++ linux/arch/arm/common/edma.c > @@ -994,29 +994,49 @@ void edma_set_dest(unsigned slot, dma_ad > EXPORT_SYMBOL(edma_set_dest); > > /** > - * edma_get_position - returns the current transfer points > + * edma_get_position - returns the current transfer point > * @slot: parameter RAM slot being examined > - * @src: pointer to source port position > - * @dst: pointer to destination port position > + * @pos: where to store the data > + * @dst: true selects the dest position, false the source > * > - * Returns current source and destination addresses for a particular > - * parameter RAM slot. Its channel should not be active when this is called. > + * Return 0 indicates a stable readout. -EBUSY indicates that the > + * readout failed due to concurrent updates. > + * > + * Call this on active channels with care. For inactive channels this > + * never fails. > */ > -void edma_get_position(unsigned slot, dma_addr_t *src, dma_addr_t *dst) > +int edma_get_position(unsigned slot, dma_addr_t *pos, bool dst) > { > - struct edmacc_param temp; > - unsigned ctlr; > + u32 dat, ctlr, offs; > + int i; > > ctlr = EDMA_CTLR(slot); > slot = EDMA_CHAN_SLOT(slot); > > - edma_read_slot(EDMA_CTLR_CHAN(ctlr, slot), &temp); > - if (src != NULL) > - *src = temp.src; > - if (dst != NULL) > - *dst = temp.dst; > + if (slot >= edma_cc[ctlr]->num_slots) > + return -EINVAL; > + > + offs = PARM_OFFSET(slot); > + offs += dst ? PARM_DST : PARM_SRC; > + > + /* > + * If the channel is active, we need to double read as we > + * might see half updated data. We limit this to 5 > + * attempts. If that fails we return -EBUSY and let the caller > + * deal with it. > + */ > + dat = edma_read(ctlr, offs); > + for (i = 0; i < 5; i++) { > + u32 tmp = edma_read(ctlr, offs); > + > + if (tmp == dat) { > + *pos = dat; > + return 0; > + } > + dat = tmp; > + } > + return -EBUSY; > } > -EXPORT_SYMBOL(edma_get_position); > > /** The access is synchronized though and you shouldn't see unreliable results, unless you _are_ seeing unreliable results in which case it could be a silicon issue. The original code also doesn't have the double read so I would just drop that, unless you are seeing reliability issues. Here's a thread that discusses it and the end conclusion is that it should be fine (Kyle Kastile is one of the EDMA hardware designers). http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx thanks, -Joel
On 04/17/2014 07:47 PM, Joel Fernandes wrote: > On 04/17/2014 09:40 AM, Thomas Gleixner wrote: >> Reading destination and source is pointless. In DEV_TO_MEM transfers >> we are only interested in the destination, in MEM_TO_DEV we care about >> the source. In MEM_TO_MEM it really does not matter which one you >> read. >> >> Remove the extra pointer and select dest/source via a bool. >> >> Reading the src/dst data from an active parameter set in the EDMA >> parameter RAM is not reliable, as there might be a concurrent update >> from the controller. >> >> But experimentation showed, that a double readout with comparing the >> results and a limited loop works nicely. I've actually never found a >> case where the loop limit triggered, but we have it there for sanity >> reasons. In case it triggers we return -EBUSY and let the caller deal >> with it. >> >> Remove the export of this function while at it. The only potential >> user is the dmaengine and that's always builtin. >> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >> --- >> arch/arm/common/edma.c | 48 ++++++++++++++++++++++++++----------- >> include/linux/platform_data/edma.h | 2 - >> 2 files changed, 35 insertions(+), 15 deletions(-) >> >> Index: linux/arch/arm/common/edma.c >> =================================================================== >> --- linux.orig/arch/arm/common/edma.c >> +++ linux/arch/arm/common/edma.c >> @@ -994,29 +994,49 @@ void edma_set_dest(unsigned slot, dma_ad >> EXPORT_SYMBOL(edma_set_dest); >> >> /** >> - * edma_get_position - returns the current transfer points >> + * edma_get_position - returns the current transfer point >> * @slot: parameter RAM slot being examined >> - * @src: pointer to source port position >> - * @dst: pointer to destination port position >> + * @pos: where to store the data >> + * @dst: true selects the dest position, false the source >> * >> - * Returns current source and destination addresses for a particular >> - * parameter RAM slot. Its channel should not be active when this is called. >> + * Return 0 indicates a stable readout. -EBUSY indicates that the >> + * readout failed due to concurrent updates. >> + * >> + * Call this on active channels with care. For inactive channels this >> + * never fails. >> */ >> -void edma_get_position(unsigned slot, dma_addr_t *src, dma_addr_t *dst) >> +int edma_get_position(unsigned slot, dma_addr_t *pos, bool dst) >> { >> - struct edmacc_param temp; >> - unsigned ctlr; >> + u32 dat, ctlr, offs; >> + int i; >> >> ctlr = EDMA_CTLR(slot); >> slot = EDMA_CHAN_SLOT(slot); >> >> - edma_read_slot(EDMA_CTLR_CHAN(ctlr, slot), &temp); >> - if (src != NULL) >> - *src = temp.src; >> - if (dst != NULL) >> - *dst = temp.dst; >> + if (slot >= edma_cc[ctlr]->num_slots) >> + return -EINVAL; >> + >> + offs = PARM_OFFSET(slot); >> + offs += dst ? PARM_DST : PARM_SRC; >> + >> + /* >> + * If the channel is active, we need to double read as we >> + * might see half updated data. We limit this to 5 >> + * attempts. If that fails we return -EBUSY and let the caller >> + * deal with it. >> + */ >> + dat = edma_read(ctlr, offs); >> + for (i = 0; i < 5; i++) { >> + u32 tmp = edma_read(ctlr, offs); >> + >> + if (tmp == dat) { >> + *pos = dat; >> + return 0; >> + } >> + dat = tmp; >> + } >> + return -EBUSY; >> } >> -EXPORT_SYMBOL(edma_get_position); >> >> /** > > The access is synchronized though and you shouldn't see unreliable > results, unless you _are_ seeing unreliable results in which case it > could be a silicon issue. > > The original code also doesn't have the double read so I would just drop > that, unless you are seeing reliability issues. > > Here's a thread that discusses it and the end conclusion is that it > should be fine (Kyle Kastile is one of the EDMA hardware designers). > http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx I'm sorry to have messed up Kyle's name, its Kyle Castille ;-) thanks, -Joel
On Thu, 17 Apr 2014, Joel Fernandes wrote: > On 04/17/2014 09:40 AM, Thomas Gleixner wrote: > > + /* > > + * If the channel is active, we need to double read as we > > + * might see half updated data. We limit this to 5 > > + * attempts. If that fails we return -EBUSY and let the caller > > + * deal with it. > > + */ > > + dat = edma_read(ctlr, offs); > > + for (i = 0; i < 5; i++) { > > + u32 tmp = edma_read(ctlr, offs); > > + > > + if (tmp == dat) { > > + *pos = dat; > > + return 0; > > + } > > + dat = tmp; > > + } > > + return -EBUSY; > > } > > -EXPORT_SYMBOL(edma_get_position); > > > > /** > > The access is synchronized though and you shouldn't see unreliable > results, unless you _are_ seeing unreliable results in which case it > could be a silicon issue. > > The original code also doesn't have the double read so I would just drop > that, unless you are seeing reliability issues. > > Here's a thread that discusses it and the end conclusion is that it > should be fine (Kyle Kastile is one of the EDMA hardware designers). > http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx Gah, right I saw the issues with the original code, which does a memcpy_fromio which copies byte by byte. Did not think about retrying with the edma_read() based one. Thanks, tglx
On Fri, 18 Apr 2014, Thomas Gleixner wrote: > On Thu, 17 Apr 2014, Joel Fernandes wrote: > > Here's a thread that discusses it and the end conclusion is that it > > should be fine (Kyle Kastile is one of the EDMA hardware designers). > > http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx > > Gah, right I saw the issues with the original code, which does a > memcpy_fromio which copies byte by byte. Did not think about retrying > with the edma_read() based one. Actually I did expect memcpy_fromio to be more clever. But you are right, using the 32bit wide read works like a charm. Thanks, tglx
On 04/18/2014 04:24 AM, Thomas Gleixner wrote: > On Fri, 18 Apr 2014, Thomas Gleixner wrote: >> On Thu, 17 Apr 2014, Joel Fernandes wrote: >>> Here's a thread that discusses it and the end conclusion is that it >>> should be fine (Kyle Kastile is one of the EDMA hardware designers). >>> http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/99/t/66011.aspx >> >> Gah, right I saw the issues with the original code, which does a >> memcpy_fromio which copies byte by byte. Did not think about retrying >> with the edma_read() based one. > > Actually I did expect memcpy_fromio to be more clever. But you are > right, using the 32bit wide read works like a charm. Cool, glad it does :) regards, -Joel
Index: linux/arch/arm/common/edma.c =================================================================== --- linux.orig/arch/arm/common/edma.c +++ linux/arch/arm/common/edma.c @@ -994,29 +994,49 @@ void edma_set_dest(unsigned slot, dma_ad EXPORT_SYMBOL(edma_set_dest); /** - * edma_get_position - returns the current transfer points + * edma_get_position - returns the current transfer point * @slot: parameter RAM slot being examined - * @src: pointer to source port position - * @dst: pointer to destination port position + * @pos: where to store the data + * @dst: true selects the dest position, false the source * - * Returns current source and destination addresses for a particular - * parameter RAM slot. Its channel should not be active when this is called. + * Return 0 indicates a stable readout. -EBUSY indicates that the + * readout failed due to concurrent updates. + * + * Call this on active channels with care. For inactive channels this + * never fails. */ -void edma_get_position(unsigned slot, dma_addr_t *src, dma_addr_t *dst) +int edma_get_position(unsigned slot, dma_addr_t *pos, bool dst) { - struct edmacc_param temp; - unsigned ctlr; + u32 dat, ctlr, offs; + int i; ctlr = EDMA_CTLR(slot); slot = EDMA_CHAN_SLOT(slot); - edma_read_slot(EDMA_CTLR_CHAN(ctlr, slot), &temp); - if (src != NULL) - *src = temp.src; - if (dst != NULL) - *dst = temp.dst; + if (slot >= edma_cc[ctlr]->num_slots) + return -EINVAL; + + offs = PARM_OFFSET(slot); + offs += dst ? PARM_DST : PARM_SRC; + + /* + * If the channel is active, we need to double read as we + * might see half updated data. We limit this to 5 + * attempts. If that fails we return -EBUSY and let the caller + * deal with it. + */ + dat = edma_read(ctlr, offs); + for (i = 0; i < 5; i++) { + u32 tmp = edma_read(ctlr, offs); + + if (tmp == dat) { + *pos = dat; + return 0; + } + dat = tmp; + } + return -EBUSY; } -EXPORT_SYMBOL(edma_get_position); /** * edma_set_src_index - configure DMA source address indexing Index: linux/include/linux/platform_data/edma.h =================================================================== --- linux.orig/include/linux/platform_data/edma.h +++ linux/include/linux/platform_data/edma.h @@ -130,7 +130,7 @@ void edma_set_src(unsigned slot, dma_add enum address_mode mode, enum fifo_width); void edma_set_dest(unsigned slot, dma_addr_t dest_port, enum address_mode mode, enum fifo_width); -void edma_get_position(unsigned slot, dma_addr_t *src, dma_addr_t *dst); +int __must_check edma_get_position(unsigned slot, dma_addr_t *pos, bool dst); void edma_set_src_index(unsigned slot, s16 src_bidx, s16 src_cidx); void edma_set_dest_index(unsigned slot, s16 dest_bidx, s16 dest_cidx); void edma_set_transfer_params(unsigned slot, u16 acnt, u16 bcnt, u16 ccnt,
Reading destination and source is pointless. In DEV_TO_MEM transfers we are only interested in the destination, in MEM_TO_DEV we care about the source. In MEM_TO_MEM it really does not matter which one you read. Remove the extra pointer and select dest/source via a bool. Reading the src/dst data from an active parameter set in the EDMA parameter RAM is not reliable, as there might be a concurrent update from the controller. But experimentation showed, that a double readout with comparing the results and a limited loop works nicely. I've actually never found a case where the loop limit triggered, but we have it there for sanity reasons. In case it triggers we return -EBUSY and let the caller deal with it. Remove the export of this function while at it. The only potential user is the dmaengine and that's always builtin. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/arm/common/edma.c | 48 ++++++++++++++++++++++++++----------- include/linux/platform_data/edma.h | 2 - 2 files changed, 35 insertions(+), 15 deletions(-)