| From 9e528c799d17a4ac37d788c81440b50377dd592d Mon Sep 17 00:00:00 2001 |
| From: Lukas Wunner <lukas@wunner.de> |
| Date: Wed, 23 Jan 2019 09:26:00 +0100 |
| Subject: dmaengine: bcm2835: Fix abort of transactions |
| |
| From: Lukas Wunner <lukas@wunner.de> |
| |
| commit 9e528c799d17a4ac37d788c81440b50377dd592d upstream. |
| |
| There are multiple issues with bcm2835_dma_abort() (which is called on |
| termination of a transaction): |
| |
| * The algorithm to abort the transaction first pauses the channel by |
| clearing the ACTIVE flag in the CS register, then waits for the PAUSED |
| flag to clear. Page 49 of the spec documents the latter as follows: |
| |
| "Indicates if the DMA is currently paused and not transferring data. |
| This will occur if the active bit has been cleared [...]" |
| https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf |
| |
| So the function is entering an infinite loop because it is waiting for |
| PAUSED to clear which is always set due to the function having cleared |
| the ACTIVE flag. The only thing that's saving it from itself is the |
| upper bound of 10000 loop iterations. |
| |
| The code comment says that the intention is to "wait for any current |
| AXI transfer to complete", so the author probably wanted to check the |
| WAITING_FOR_OUTSTANDING_WRITES flag instead. Amend the function |
| accordingly. |
| |
| * The CS register is only read at the beginning of the function. It |
| needs to be read again after pausing the channel and before checking |
| for outstanding writes, otherwise writes which were issued between |
| the register read at the beginning of the function and pausing the |
| channel may not be waited for. |
| |
| * The function seeks to abort the transfer by writing 0 to the NEXTCONBK |
| register and setting the ABORT and ACTIVE flags. Thereby, the 0 in |
| NEXTCONBK is sought to be loaded into the CONBLK_AD register. However |
| experimentation has shown this approach to not work: The CONBLK_AD |
| register remains the same as before and the CS register contains |
| 0x00000030 (PAUSED | DREQ_STOPS_DMA). In other words, the control |
| block is not aborted but merely paused and it will be resumed once the |
| next DMA transaction is started. That is absolutely not the desired |
| behavior. |
| |
| A simpler approach is to set the channel's RESET flag instead. This |
| reliably zeroes the NEXTCONBK as well as the CS register. It requires |
| less code and only a single MMIO write. This is also what popular |
| user space DMA drivers do, e.g.: |
| https://github.com/metachris/RPIO/blob/master/source/c_pwm/pwm.c |
| |
| Note that the spec is contradictory whether the NEXTCONBK register |
| is writeable at all. On the one hand, page 41 claims: |
| |
| "The value loaded into the NEXTCONBK register can be overwritten so |
| that the linked list of Control Block data structures can be |
| dynamically altered. However it is only safe to do this when the DMA |
| is paused." |
| |
| On the other hand, page 40 specifies: |
| |
| "Only three registers in each channel's register set are directly |
| writeable (CS, CONBLK_AD and DEBUG). The other registers (TI, |
| SOURCE_AD, DEST_AD, TXFR_LEN, STRIDE & NEXTCONBK), are automatically |
| loaded from a Control Block data structure held in external memory." |
| |
| Fixes: 96286b576690 ("dmaengine: Add support for BCM2835") |
| Signed-off-by: Lukas Wunner <lukas@wunner.de> |
| Cc: stable@vger.kernel.org # v3.14+ |
| Cc: Frank Pavlic <f.pavlic@kunbus.de> |
| Cc: Martin Sperl <kernel@martin.sperl.org> |
| Cc: Florian Meier <florian.meier@koalo.de> |
| Cc: Clive Messer <clive.m.messer@gmail.com> |
| Cc: Matthias Reichl <hias@horus.com> |
| Tested-by: Stefan Wahren <stefan.wahren@i2se.com> |
| Acked-by: Florian Kauer <florian.kauer@koalo.de> |
| Signed-off-by: Vinod Koul <vkoul@kernel.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/dma/bcm2835-dma.c | 41 +++++++++-------------------------------- |
| 1 file changed, 9 insertions(+), 32 deletions(-) |
| |
| --- a/drivers/dma/bcm2835-dma.c |
| +++ b/drivers/dma/bcm2835-dma.c |
| @@ -415,13 +415,11 @@ static void bcm2835_dma_fill_cb_chain_wi |
| } |
| } |
| |
| -static int bcm2835_dma_abort(void __iomem *chan_base) |
| +static int bcm2835_dma_abort(struct bcm2835_chan *c) |
| { |
| - unsigned long cs; |
| + void __iomem *chan_base = c->chan_base; |
| long int timeout = 10000; |
| |
| - cs = readl(chan_base + BCM2835_DMA_CS); |
| - |
| /* |
| * A zero control block address means the channel is idle. |
| * (The ACTIVE flag in the CS register is not a reliable indicator.) |
| @@ -433,25 +431,16 @@ static int bcm2835_dma_abort(void __iome |
| writel(0, chan_base + BCM2835_DMA_CS); |
| |
| /* Wait for any current AXI transfer to complete */ |
| - while ((cs & BCM2835_DMA_ISPAUSED) && --timeout) { |
| + while ((readl(chan_base + BCM2835_DMA_CS) & |
| + BCM2835_DMA_WAITING_FOR_WRITES) && --timeout) |
| cpu_relax(); |
| - cs = readl(chan_base + BCM2835_DMA_CS); |
| - } |
| |
| - /* We'll un-pause when we set of our next DMA */ |
| + /* Peripheral might be stuck and fail to signal AXI write responses */ |
| if (!timeout) |
| - return -ETIMEDOUT; |
| - |
| - if (!(cs & BCM2835_DMA_ACTIVE)) |
| - return 0; |
| - |
| - /* Terminate the control block chain */ |
| - writel(0, chan_base + BCM2835_DMA_NEXTCB); |
| - |
| - /* Abort the whole DMA */ |
| - writel(BCM2835_DMA_ABORT | BCM2835_DMA_ACTIVE, |
| - chan_base + BCM2835_DMA_CS); |
| + dev_err(c->vc.chan.device->dev, |
| + "failed to complete outstanding writes\n"); |
| |
| + writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS); |
| return 0; |
| } |
| |
| @@ -804,7 +793,6 @@ static int bcm2835_dma_terminate_all(str |
| struct bcm2835_chan *c = to_bcm2835_dma_chan(chan); |
| struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device); |
| unsigned long flags; |
| - int timeout = 10000; |
| LIST_HEAD(head); |
| |
| spin_lock_irqsave(&c->vc.lock, flags); |
| @@ -818,18 +806,7 @@ static int bcm2835_dma_terminate_all(str |
| if (c->desc) { |
| bcm2835_dma_desc_free(&c->desc->vd); |
| c->desc = NULL; |
| - bcm2835_dma_abort(c->chan_base); |
| - |
| - /* Wait for stopping */ |
| - while (--timeout) { |
| - if (!readl(c->chan_base + BCM2835_DMA_ADDR)) |
| - break; |
| - |
| - cpu_relax(); |
| - } |
| - |
| - if (!timeout) |
| - dev_err(d->ddev.dev, "DMA transfer could not be terminated\n"); |
| + bcm2835_dma_abort(c); |
| } |
| |
| vchan_get_all_descriptors(&c->vc, &head); |