| Message ID | 1438961765-22554-1-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Aug 07, 2015 at 05:36:05PM +0200, Sebastian Andrzej Siewior wrote: > + /* > + * We do not allow DMA_MEM_TO_DEV transfers to be paused. > + * According to RMK the OMAP hardware might prefetch bytes from > + * memory into its FIFO and not send it to the device due to the > + * pause. The bytes in the FIFO are cleared on pause. It is > + * unspecified by how many bytes the source address is updated > + * if at all. Would you mind rewording the above. Please take the time to read the manuals for the device you are playing with. It's mostly documented in there. See the OMAP4430 ES2.x TRM, 16.4.18 and 16.4.19. 16.4.19 states that: "When a source-synchronized channel is disabled during a transfer... "In all other cases, the channel undergoes an abort." A source-synchronised channel is one where the fetching of data is under control of the device. In other words, a device-to-memory transfer. So, a destination-synchronised channel (which would be a memory-to-device transfer) undergoes an abort if you clear the enable bit. 16.4.20.4.6.2 defines what happens when a channel aborts: "If an abort trigger occurs, the channel aborts immediately after completion of current read/write transactions and then FIFO is cleaned up." It doesn't define what "cleaned up" means, so that's open to some interpretation. That's why I contacted TI about this back in 2012: | What is the behaviour of the OMAP DMA hardware when the DMA4_CCRi[7] | enable bit is cleared and subsequently set without any additional | reprogramming? I'm thinking specifically about memory-to-device DMA | operations, in particular the UART ports. | | Will this allow a transfer to be temporarily paused, and then | subsequently resumed with out loss of data in the DMA hardware, as if | nothing had actually happened? The answer I received was to check that RD_ACTIVE and WR_ACTIVE are both clear _before_ disabling the channel, otherwise data loss will occur. The problem is that if the channel is active, then device activity can result in DMA activity starting between reading those as both clear and the write to DMA_CCR to clear the enable bit hitting the hardware. The explanation went on to say that if the DMA hardware can't drain the data in its FIFO to the destination, then data loss "might" occur. That will occur if we're destination-synchronised (eg) to a UART and the UART is not accepting any further data. Due to this, the OMAP DMA engine driver was submitted with this in the cover note: "For the OMAP DMAengine driver, there's a few short-comings: 1. pause/resume support is not implemented; it's not clear whether the OMAP hardware is capable of supporting this sanely."
* Russell King - ARM Linux | 2015-08-07 17:26:48 [+0100]: >On Fri, Aug 07, 2015 at 05:36:05PM +0200, Sebastian Andrzej Siewior wrote: >> + /* >> + * We do not allow DMA_MEM_TO_DEV transfers to be paused. >> + * According to RMK the OMAP hardware might prefetch bytes from >> + * memory into its FIFO and not send it to the device due to the >> + * pause. The bytes in the FIFO are cleared on pause. It is >> + * unspecified by how many bytes the source address is updated >> + * if at all. > >Would you mind rewording the above. > >Please take the time to read the manuals for the device you are playing >with. It's mostly documented in there. See the OMAP4430 ES2.x TRM, >16.4.18 and 16.4.19. I didn't connect the dots that well. Thank you. … I updated the comment to: /* * We do not allow DMA_MEM_TO_DEV transfers to be paused. * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer: * "When a channel is disabled during a transfer, the channel undergoes * an abort, unless it is hardware-source-synchronized …". * A source-synchronised channel is one where the fetching of data is * under control of the device. In other words, a device-to-memory * transfer. So, a destination-synchronised channel (which would be a * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE * bit is cleared. * From 16.1.4.20.4.6.2 Abort: "If an abort trigger occurs, the channel * aborts immediately after completion of current read/write * transactions and then the FIFO is cleaned up." The term "cleaned up" * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE * are both clear _before_ disabling the channel, otherwise data loss * will occur. * The problem is that if the channel is active, then device activity * can result in DMA activity starting between reading those as both * clear and the write to DMA_CCR to clear the enable bit hitting the * hardware. If the DMA hardware can't drain the data in its FIFO to the * destination, then data loss "might" occur (say if we write to an UART * and the UART is not accepting any further data). */ would that be okay? >Due to this, the OMAP DMA engine driver was submitted with this in the >cover note: > >"For the OMAP DMAengine driver, there's a few short-comings: > >1. pause/resume support is not implemented; it's not clear whether the > OMAP hardware is capable of supporting this sanely." If I google for it, I find it. pause/resume support for cyclic was added later without a note why it is only supported for cyclic. Sebastian -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 07, 2015 at 07:55:48PM +0200, Sebastian Andrzej Siewior wrote: > /* > * We do not allow DMA_MEM_TO_DEV transfers to be paused. > * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer: > * "When a channel is disabled during a transfer, the channel undergoes > * an abort, unless it is hardware-source-synchronized …". > * A source-synchronised channel is one where the fetching of data is > * under control of the device. In other words, a device-to-memory > * transfer. So, a destination-synchronised channel (which would be a > * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE > * bit is cleared. > * From 16.1.4.20.4.6.2 Abort: "If an abort trigger occurs, the channel > * aborts immediately after completion of current read/write > * transactions and then the FIFO is cleaned up." The term "cleaned up" > * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE > * are both clear _before_ disabling the channel, otherwise data loss > * will occur. > * The problem is that if the channel is active, then device activity > * can result in DMA activity starting between reading those as both > * clear and the write to DMA_CCR to clear the enable bit hitting the > * hardware. If the DMA hardware can't drain the data in its FIFO to the > * destination, then data loss "might" occur (say if we write to an UART > * and the UART is not accepting any further data). > */ > > would that be okay? Better, if a tad verbose. I guess no one will miss that. :) > If I google for it, I find it. pause/resume support for cyclic was added > later without a note why it is only supported for cyclic. Try searching for the "[PATCH 11/11] ASoC: omap-pcm: Convert to use dmaengine" thread, which was the initial round of patches converting omap-pcm to DMA engine, and where there was some discussion of how to handle it at that time. The result of that was it was felt that the safest approach was to limit it to cyclic transfers for ASoC, and to use the method which had been well proven over previous years/decade on numerous different OMAP hardware. It's all entirely sensible given that omap-dma has to cope with many different hardware revisions with two major hardware versions and people having limited testing resources for validation. So you will understand, given the data loss issues here, why it was decided that omap-dma will only provide pause/resume support for cases where it has been proven to work sufficiently well and where data loss is not that a major issue (if a few samples of audio get lost over a suspend/resume, it's not going to corrupt your data.)
diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c index 249445c8a4c6..6bbf089d2d7f 100644 --- a/drivers/dma/omap-dma.c +++ b/drivers/dma/omap-dma.c @@ -299,7 +299,30 @@ static void omap_dma_start(struct omap_chan *c, struct omap_desc *d) omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE); } -static void omap_dma_stop(struct omap_chan *c) +static void omap_dma_drain_chan(struct omap_chan *c) +{ + int i; + uint32_t val; + + /* Wait for sDMA FIFO to drain */ + for (i = 0; ; i++) { + val = omap_dma_chan_read(c, CCR); + if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))) + break; + + if (i > 100) + break; + + udelay(5); + } + + if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)) + dev_err(c->vc.chan.device->dev, + "DMA drain did not complete on lch %d\n", + c->dma_ch); +} + +static int omap_dma_stop(struct omap_chan *c) { struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device); uint32_t val; @@ -312,7 +335,6 @@ static void omap_dma_stop(struct omap_chan *c) val = omap_dma_chan_read(c, CCR); if (od->plat->errata & DMA_ERRATA_i541 && val & CCR_TRIGGER_SRC) { uint32_t sysconfig; - unsigned i; sysconfig = omap_dma_glbl_read(od, OCP_SYSCONFIG); val = sysconfig & ~DMA_SYSCONFIG_MIDLEMODE_MASK; @@ -323,27 +345,18 @@ static void omap_dma_stop(struct omap_chan *c) val &= ~CCR_ENABLE; omap_dma_chan_write(c, CCR, val); - /* Wait for sDMA FIFO to drain */ - for (i = 0; ; i++) { - val = omap_dma_chan_read(c, CCR); - if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))) - break; - - if (i > 100) - break; - - udelay(5); - } - - if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)) - dev_err(c->vc.chan.device->dev, - "DMA drain did not complete on lch %d\n", - c->dma_ch); + omap_dma_drain_chan(c); omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig); } else { + + if (!(val & CCR_ENABLE)) + return -EINVAL; + val &= ~CCR_ENABLE; omap_dma_chan_write(c, CCR, val); + + omap_dma_drain_chan(c); } mb(); @@ -358,6 +371,7 @@ static void omap_dma_stop(struct omap_chan *c) omap_dma_chan_write(c, CLNK_CTRL, val); } + return 0; } static void omap_dma_start_sg(struct omap_chan *c, struct omap_desc *d, @@ -728,6 +742,8 @@ static enum dma_status omap_dma_tx_status(struct dma_chan *chan, } else { txstate->residue = 0; } + if (ret == DMA_IN_PROGRESS && c->paused) + ret = DMA_PAUSED; spin_unlock_irqrestore(&c->vc.lock, flags); return ret; @@ -1038,10 +1054,8 @@ static int omap_dma_terminate_all(struct dma_chan *chan) omap_dma_stop(c); } - if (c->cyclic) { - c->cyclic = false; - c->paused = false; - } + c->cyclic = false; + c->paused = false; vchan_get_all_descriptors(&c->vc, &head); spin_unlock_irqrestore(&c->vc.lock, flags); @@ -1053,28 +1067,51 @@ static int omap_dma_terminate_all(struct dma_chan *chan) static int omap_dma_pause(struct dma_chan *chan) { struct omap_chan *c = to_omap_dma_chan(chan); + struct omap_dmadev *od = to_omap_dma_dev(chan->device); + unsigned long flags; + int ret = -EINVAL; + bool can_pause; - /* Pause/Resume only allowed with cyclic mode */ - if (!c->cyclic) - return -EINVAL; + spin_lock_irqsave(&od->irq_lock, flags); - if (!c->paused) { - omap_dma_stop(c); - c->paused = true; + if (!c->desc) + goto out; + + if (c->cyclic) + can_pause = true; + + else if (c->desc->dir == DMA_DEV_TO_MEM) + /* + * We do not allow DMA_MEM_TO_DEV transfers to be paused. + * According to RMK the OMAP hardware might prefetch bytes from + * memory into its FIFO and not send it to the device due to the + * pause. The bytes in the FIFO are cleared on pause. It is + * unspecified by how many bytes the source address is updated + * if at all. + */ + can_pause = true; + + if (can_pause && !c->paused) { + ret = omap_dma_stop(c); + if (!ret) + c->paused = true; } +out: + spin_unlock_irqrestore(&od->irq_lock, flags); - return 0; + return ret; } static int omap_dma_resume(struct dma_chan *chan) { struct omap_chan *c = to_omap_dma_chan(chan); + struct omap_dmadev *od = to_omap_dma_dev(chan->device); + unsigned long flags; + int ret = -EINVAL; - /* Pause/Resume only allowed with cyclic mode */ - if (!c->cyclic) - return -EINVAL; + spin_lock_irqsave(&od->irq_lock, flags); - if (c->paused) { + if (c->paused && c->desc) { mb(); /* Restore channel link register */ @@ -1082,9 +1119,11 @@ static int omap_dma_resume(struct dma_chan *chan) omap_dma_start(c, c->desc); c->paused = false; + ret = 0; } + spin_unlock_irqrestore(&od->irq_lock, flags); - return 0; + return ret; } static int omap_dma_chan_init(struct omap_dmadev *od)
This DMA driver is used by 8250-omap on DRA7-evm. There is one requirement that is to pause a transfer. This is currently used on the RX side. It is possible that the UART HW aborted the RX (UART's RX-timeout) but the DMA controller starts the transfer shortly after. Before we can manually purge the FIFO we need to pause the transfer, check how many bytes it already received and terminate the transfer without it making any progress. From testing on the TX side it seems that it is possible that we invoke pause once the transfer has completed which is indicated by the missing CCR_ENABLE bit but before the interrupt has been noticed. In that case the interrupt will come even after disabling it. The AM572x manual says that we have to wait for the CCR_RD_ACTIVE & CCR_WR_ACTIVE bits to be gone before programming it again here is the drain loop. Also it looks like without the drain the TX-transfer makes sometimes progress. One note: The pause + resume combo is broken because after resume the the complete transfer will be programmed again. That means the already transferred bytes (until the pause event) will be sent again. This is currently not important for my UART user because it does only pause + terminate. v1…v2: - move the drain loop into omap_dma_drain_chan() instead of having it twice. - allow pause only for DMA_DEV_TO_MEM transfers if non-cyclic. Add a comment why DMA_MEM_TO_DEV not allowed. - clear pause on terminate_all. Otherwise pause() + terminate_all() will keep the pause bit set and we can't pause the following transfer. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/dma/omap-dma.c | 107 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 73 insertions(+), 34 deletions(-)