On Fri, Dec 28, 2018 at 02:20:42PM +0100, Stefan Wahren wrote:
> Lukas Wunner <lukas@xxxxxxxxx> hat am 22. Dezember 2018 um 08:28 geschrieben:
> > drivers/dma/bcm2835-dma.c | 33 +++------------------------------
> > 1 file changed, 3 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> > index e94f41c56975..17bc7304db3a 100644
> > --- a/drivers/dma/bcm2835-dma.c
> > +++ b/drivers/dma/bcm2835-dma.c
> > @@ -419,25 +419,11 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
> > 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 */
> > - if (!timeout)
> > - return -ETIMEDOUT;
>
> i'm only sceptical about silently ignoring the timeout case. I prefer
> to have a comment explaining why we proceed with BCM2835_DMA_RESET in
> this case and some kind of error / debug message like below.
>
> > - if (!timeout)
> > - dev_err(d->ddev.dev, "DMA transfer could not be terminated\n");
The Foundation's downstream tree contains an additional DMA driver
called drivers/dma/bcm2708-dmaengine.c:
https://github.com/raspberrypi/linux/commit/5fa54ef4d495
There's a code comment in that driver which sheds some light on the
abort algorithm's rationale:
"This routine waits for the current AXI transfer to complete before
terminating the current DMA. If the current transfer is hung on a DREQ used
by an uncooperative peripheral the AXI transfer may never complete. In this
case the routine times out and return a non-zero error code."
The author's intention seems to have been that a peripheral may buffer
a write if it has deasserted its DREQ signal and won't send an AXI
write response until it can process the write. And if the peripheral
is somehow stuck, that write response may never occur.
I'm not sure if that can actually happen in the real world or if it's
a purely theoretical issue. But the additional cost (in terms of LoC
and performance) is small, so I left it in.
There's not much we can do if the peripheral doesn't acknowledge writes,
so I just carry on resetting the channel.
I intend to amend the commit like below to address your objection,
let me know if you do not consider this sufficient. Thanks.
-- >8 --
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index ceec110..c3d9a26 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -417,8 +417,9 @@ static void bcm2835_dma_fill_cb_chain_with_sg(
}
}
-static int bcm2835_dma_abort(void __iomem *chan_base)
+static int bcm2835_dma_abort(struct bcm2835_chan *c)
{
+ void __iomem *chan_base = c->chan_base;
unsigned long cs;
long int timeout = 10000;
@@ -434,6 +435,11 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
BCM2835_DMA_WAITING_FOR_WRITES) && --timeout)
cpu_relax();
+ /* Uncooperative peripherals may not signal AXI write responses */
+ if (!timeout)
+ dev_err(c->vc.chan.device->dev,
+ "failed to complete outstanding writes\n");
+
writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS);
return 0;
}
@@ -797,7 +803,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
if (c->desc) {
vchan_terminate_vdesc(&c->desc->vd);
c->desc = NULL;
- bcm2835_dma_abort(c->chan_base);
+ bcm2835_dma_abort(c);
}
vchan_get_all_descriptors(&c->vc, &head);
