| Message ID | edb004dff4af6106f6bfcb89e1a96391e96eb857.1564825752.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4c524191c0a21d758b519087c64f84348095e940 |
| Headers | show |
| Series | Raspberry Pi SPI speedups | expand |
Hi Lukas, Am 03.08.19 um 12:10 schrieb Lukas Wunner: > 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. has the issue already reported to Raspberry Pi Trading?
On Sun, Aug 11, 2019 at 09:45:09PM +0200, Stefan Wahren wrote: > Am 03.08.19 um 12:10 schrieb Lukas Wunner: > > 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. > > has the issue already reported to Raspberry Pi Trading? You mean to fix such errata in future revisions? I wouldn't know who to report this to, Roger Thornton or James Adams perhaps? I'm not sure if the SPI controller isn't just an IP block licensed from a third party, that might make it difficult to get errata fixed. Thanks, Lukas
Lukas Wunner <lukas@wunner.de> writes: > On Sun, Aug 11, 2019 at 09:45:09PM +0200, Stefan Wahren wrote: >> Am 03.08.19 um 12:10 schrieb Lukas Wunner: >> > 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. >> >> has the issue already reported to Raspberry Pi Trading? > > You mean to fix such errata in future revisions? > > I wouldn't know who to report this to, Roger Thornton or James Adams perhaps? > > I'm not sure if the SPI controller isn't just an IP block licensed from > a third party, that might make it difficult to get errata fixed. It's Broadcom's.
Am 11.08.19 um 21:57 schrieb Lukas Wunner: > On Sun, Aug 11, 2019 at 09:45:09PM +0200, Stefan Wahren wrote: >> Am 03.08.19 um 12:10 schrieb Lukas Wunner: >>> 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. >> has the issue already reported to Raspberry Pi Trading? > You mean to fix such errata in future revisions? I thought about document this issue in some kind of errata or on their website. > > I wouldn't know who to report this to, Roger Thornton or James Adams perhaps? > > I'm not sure if the SPI controller isn't just an IP block licensed from > a third party, that might make it difficult to get errata fixed. > > Thanks, > > Lukas
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index 2bf725e909fd..f6391c302363 100644 --- a/drivers/spi/spi-bcm2835.c +++ b/drivers/spi/spi-bcm2835.c @@ -317,6 +317,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; @@ -475,7 +482,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]), @@ -496,7 +505,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)) {
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> Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: Martin Sperl <kernel@martin.sperl.org> Cc: Noralf Trønnes <noralf@tronnes.org> --- drivers/spi/spi-bcm2835.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)