| Message ID | 1438977619-15488-4-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote: > -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); Note that the FIFO drain only applies to source synchronized transfers... When the BUFFERING is _not_ disabled - in most of the cases this is true. > + /* > + * 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). I don't know if you have checked it, but probably the TX DMA could be also used when the PRZEFETCH is disabled for the channel? Just a guess > + */ > + else if (c->desc->dir == DMA_DEV_TO_MEM) > + 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 +1134,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) >
On Tue, Aug 11, 2015 at 03:02:44PM +0300, Peter Ujfalusi wrote: > On 08/07/2015 11:00 PM, 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). > > I don't know if you have checked it, but probably the TX DMA could be also > used when the PRZEFETCH is disabled for the channel? Just a guess The docs aren't very clear on that... and iirc Santosh's reply didn't suggest that the prefetch bit had any influence on this behaviour. Given the wording in the documentation which seems to be quite explicit about the conditions, and it omits talking about the prefetch bit, I can only assume that the prefetch bit has no influence over this behaviour. For example, what happens if the DMA to the device has started - the device has raised its DMA request line. The DMA controller has then gone to memory and has fetched some data and incremented the source address. Meanwhile, we've cleared the ENABLE bit. What happens then? Does the DMA controller drain the read data to the device, or does it "clean up" the FIFO by discarding the data? Given that the conditions under which the FIFO is drained to the destination are very specific, and which explicitly excludes destination- synchronised transfers, the only conclusion that's possible without knowing the implementation intimately is that the FIFO is "cleaned up" which suggests that it's discarded rather than drained to the destination. As this DMA controller is in all of the OMAP devices and similar, I don't think we can rely on the behaviour of any one implementation either - we don't know what the differences are between the implementations in different generations of devices without TI providing more detailed documentation in this area across their various devices.
On 08/11/2015 03:30 PM, Russell King - ARM Linux wrote: > On Tue, Aug 11, 2015 at 03:02:44PM +0300, Peter Ujfalusi wrote: >> On 08/07/2015 11:00 PM, 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). >> >> I don't know if you have checked it, but probably the TX DMA could be also >> used when the PRZEFETCH is disabled for the channel? Just a guess > > The docs aren't very clear on that... and iirc Santosh's reply didn't > suggest that the prefetch bit had any influence on this behaviour. Given > the wording in the documentation which seems to be quite explicit about > the conditions, and it omits talking about the prefetch bit, I can only > assume that the prefetch bit has no influence over this behaviour. > > For example, what happens if the DMA to the device has started - the > device has raised its DMA request line. The DMA controller has then gone > to memory and has fetched some data and incremented the source address. > Meanwhile, we've cleared the ENABLE bit. What happens then? Does the > DMA controller drain the read data to the device, or does it "clean up" > the FIFO by discarding the data? Hrm, yes. If we do not have prefetch and the destination is using FIFO - so DMA pushes multiple elemets per DMA request this might be the case. But I think - nothing backs this up - if the transfer is element syncronized than we would not loose data if the prefetch is not enabled. If we have prefetch then the DMA is prefetching data to it's FIFO and an abort will just send the content of the FIFO to /dev/null (or something). > Given that the conditions under which the FIFO is drained to the > destination are very specific, and which explicitly excludes destination- > synchronised transfers, the only conclusion that's possible without > knowing the implementation intimately is that the FIFO is "cleaned up" > which suggests that it's discarded rather than drained to the destination. Yes, the wording is not explicit and this is also my take on the issue - the content of the FIFO is just dropped after it finished the ongoing element. > As this DMA controller is in all of the OMAP devices and similar, I > don't think we can rely on the behaviour of any one implementation > either - we don't know what the differences are between the > implementations in different generations of devices without TI providing > more detailed documentation in this area across their various devices. Yep. Let me try to get more information if I can.
diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c index 249445c8a4c6..cb217fab472e 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,66 @@ 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; + + /* + * 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). + */ + else if (c->desc->dir == DMA_DEV_TO_MEM) + 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 +1134,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. v2…v3: - rephrase the comment based on Russell's information / feedback. 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 | 122 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 88 insertions(+), 34 deletions(-)