[v2,04/10] spi: bcm2835: Work around DONE bit erratum
diff mbox series

Message ID 7ceb98f154cdcf72c577615fa312df41adea5f47.1568187525.git.lukas@wunner.de
State New
Headers show
Series
  • Speed up SPI simplex transfers on Raspberry Pi
Related show

Commit Message

Lukas Wunner Sept. 11, 2019, 10:15 a.m. UTC
Commit 3bd7f6589f67 ("spi: bcm2835: Overcome sglist entry length
limitation") amended the BCM2835 SPI driver with support for DMA
transfers whose buffers are not aligned to 4 bytes and require more than
one sglist entry.

When testing this feature with upcoming commits to speed up TX-only and
RX-only transfers, I noticed that SPI transmission sometimes breaks.
A function introduced by the commit, bcm2835_spi_transfer_prologue(),
performs one or two PIO transmissions as a prologue to the actual DMA
transmission.  It turns out that the breakage goes away if the DONE bit
in the CS register is set when ending such a PIO transmission.

The DONE bit signifies emptiness of the TX FIFO.  According to the spec,
the bit is of type RO, so writing it should never have any effect.
Perhaps the spec is wrong and the bit is actually of type RW1C.
E.g. the I2C controller on the BCM2835 does have an RW1C DONE bit which
needs to be cleared by the driver.  Another, possibly more likely
explanation is that it's a hardware erratum since the issue does not
occur consistently.

Either way, amend bcm2835_spi_transfer_prologue() to always write the
DONE bit.

Usually a transmission is ended by bcm2835_spi_reset_hw().  If the
transmission was successful, the TX FIFO is empty and thus the DONE bit
is set when bcm2835_spi_reset_hw() reads the CS register.  The bit is
then written back to the register, so we happen to do the right thing.

However if DONE is not set, e.g. because transmission is aborted with
a non-empty TX FIFO, the bit won't be written by bcm2835_spi_reset_hw()
and it seems possible that transmission might subsequently break.  To be
on the safe side, likewise amend bcm2835_spi_reset_hw() to always write
the bit.

Tested-by: Nuno Sá <nuno.sa@analog.com>
Tested-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Acked-by: Stefan Wahren <wahrenst@gmx.net>
Acked-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi-bcm2835.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Mark Brown Sept. 11, 2019, 11:25 a.m. UTC | #1
On Wed, Sep 11, 2019 at 12:15:30PM +0200, Lukas Wunner wrote:
> Commit 3bd7f6589f67 ("spi: bcm2835: Overcome sglist entry length
> limitation") amended the BCM2835 SPI driver with support for DMA
> transfers whose buffers are not aligned to 4 bytes and require more than
> one sglist entry.

You said in the cover letter that this was at the start of the
series but it's actually patch 4 (or patch 3 in terms of the
order they were sent), and in any case it was already applied so
I'm not clear why you're resending it.  If there's any difference
from the previous version please send an incremental fix for it
instead.

The entire series has arrived but it looks like this in my inbox:

    749 r T 09/11 Lukas Wunner    (1.6K) [PATCH v2 00/10] Speed up SPI simplex t
    750 N T 09/11 Lukas Wunner    (4.0K) ├─>[PATCH v2 05/10] spi: bcm2835: Drop
    751 N T 09/11 Lukas Wunner    (3.5K) ├─>[PATCH v2 09/10] dmaengine: bcm2835:
    752 N T 09/11 Lukas Wunner    (3.6K) ├─>[PATCH v2 04/10] spi: bcm2835: Work
    753 N T 09/11 Lukas Wunner    ( 17K) ├─>[PATCH v2 07/10] spi: bcm2835: Speed
    754 N T 09/11 Lukas Wunner    (5.2K) ├─>[PATCH v2 06/10] spi: bcm2835: Cache
    755 N T 09/11 Lukas Wunner    (2.1K) ├─>[PATCH v2 02/10] dmaengine: bcm2835:
    756 N T 09/11 Lukas Wunner    (1.3K) ├─>[PATCH v2 01/10] dmaengine: bcm2835:
    757 N T 09/11 Lukas Wunner    (2.6K) ├─>[PATCH v2 03/10] spi: Guarantee cach
    758 N T 09/11 Lukas Wunner    (1.1K) ├─>[PATCH v2 08/10] dmaengine: bcm2835:
    759 N T 09/11 Lukas Wunner    (8.6K) └─>[PATCH v2 10/10] spi: bcm2835: Speed

which is really not good, the random ordering you're using when
you send things means that it's a hassle to even figure out that
I've got the entire series.  Please look into what you're doing
here, other people's patch serieses don't have this problem so
there must be something unusual with your tooling.
Lukas Wunner Sept. 11, 2019, 11:53 a.m. UTC | #2
On Wed, Sep 11, 2019 at 12:25:22PM +0100, Mark Brown wrote:
> On Wed, Sep 11, 2019 at 12:15:30PM +0200, Lukas Wunner wrote:
> > Commit 3bd7f6589f67 ("spi: bcm2835: Overcome sglist entry length
> > limitation") amended the BCM2835 SPI driver with support for DMA
> > transfers whose buffers are not aligned to 4 bytes and require more than
> > one sglist entry.
> 
> You said in the cover letter that this was at the start of the
> series but it's actually patch 4

All preceding patches do not touch spi-bcm2835.c,
that's what I meant with "front of the series".


> and in any case it was already applied so
> I'm not clear why you're resending it.

I based the series on your for-5.4 branch and didn't notice that you had
already applied this one to for-5.3.  This wasn't really meant as a fix
and it's not necessary for it to to be applied to for-5.3, though there's
no harm either as long as it lands in Linus' tree before the other
patches in this series.


> If there's any difference
> from the previous version please send an incremental fix for it
> instead.

There is no difference aside from a Tested-by tag for Noralf Trønnes
which is missing from the commit on your for-5.3 branch.

Thanks,

Lukas

Patch
diff mbox series

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index fbd6d1ae4c5e..f79f04ea42e5 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -321,6 +321,13 @@  static void bcm2835_spi_reset_hw(struct spi_controller *ctlr)
 		BCM2835_SPI_CS_INTD |
 		BCM2835_SPI_CS_DMAEN |
 		BCM2835_SPI_CS_TA);
+	/*
+	 * Transmission sometimes breaks unless the DONE bit is written at the
+	 * end of every transfer.  The spec says it's a RO bit.  Either the
+	 * spec is wrong and the bit is actually of type RW1C, or it's a
+	 * hardware erratum.
+	 */
+	cs |= BCM2835_SPI_CS_DONE;
 	/* and reset RX/TX FIFOS */
 	cs |= BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX;
 
@@ -479,7 +486,9 @@  static void bcm2835_spi_transfer_prologue(struct spi_controller *ctlr,
 		bcm2835_wr_fifo_count(bs, bs->rx_prologue);
 		bcm2835_wait_tx_fifo_empty(bs);
 		bcm2835_rd_fifo_count(bs, bs->rx_prologue);
-		bcm2835_spi_reset_hw(ctlr);
+		bcm2835_wr(bs, BCM2835_SPI_CS, cs | BCM2835_SPI_CS_CLEAR_RX
+						  | BCM2835_SPI_CS_CLEAR_TX
+						  | BCM2835_SPI_CS_DONE);
 
 		dma_sync_single_for_device(ctlr->dma_rx->device->dev,
 					   sg_dma_address(&tfr->rx_sg.sgl[0]),
@@ -500,7 +509,8 @@  static void bcm2835_spi_transfer_prologue(struct spi_controller *ctlr,
 						  | BCM2835_SPI_CS_DMAEN);
 		bcm2835_wr_fifo_count(bs, tx_remaining);
 		bcm2835_wait_tx_fifo_empty(bs);
-		bcm2835_wr(bs, BCM2835_SPI_CS, cs | BCM2835_SPI_CS_CLEAR_TX);
+		bcm2835_wr(bs, BCM2835_SPI_CS, cs | BCM2835_SPI_CS_CLEAR_TX
+						  | BCM2835_SPI_CS_DONE);
 	}
 
 	if (likely(!bs->tx_spillover)) {