Re: [PATCH 7/7] spi: bcm2835: Speed up FIFO access if fill level is known

[Stefan Wahren <stefan.wahren@xxxxxxxx>]

> kernel@xxxxxxxxxxxxxxxx hat am 10. November 2018 um 11:03 geschrieben:
> 
> 
> 
> > On 08.11.2018, at 08:06, Lukas Wunner <lukas@xxxxxxxxx> wrote:
> > 
> > The RX and TX FIFO of the BCM2835 SPI master each accommodate 64 bytes
> > (16 32-bit dwords).  The CS register provides hints on their fill level:
> > 
> >   "Bit 19  RXR - RX FIFO needs Reading ([¾] full)
> >    0 = RX FIFO is less than [¾] full (or not active TA = 0).
> >    1 = RX FIFO is [¾] or more full. Cleared by reading sufficient
> >        data from the RX FIFO or setting TA to 0."
> > 
> >   "Bit 16  DONE - Transfer Done
> >    0 = Transfer is in progress (or not active TA = 0).
> >    1 = Transfer is complete. Cleared by writing more data to the
> >        TX FIFO or setting TA to 0."
> > 
> >   "If DONE is set [...], write up to 16 [dwords] to SPI_FIFO. [...]
> >    If RXR is set read 12 [dwords] data from SPI_FIFO."
> > 
> >   [Source: Pages 153, 154 and 158 of
> >    https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
> >    Note: The spec is missing the "¾" character, presumably due to
> >    copy-pasting from a different charset.  It also incorrectly
> >    refers to 16 and 12 "bytes" instead of 32-bit dwords.]
> > 
> > In short, the RXR bit indicates that 48 bytes can be read and the DONE
> > bit indicates 64 bytes can be written.  Leverage this knowledge to read
> > or write bytes blindly to the FIFO, without polling whether data can be
> > read or free space is available to write.  Moreover, when a transfer is
> > starting, the TX FIFO is known to be empty, likewise allowing a blind
> > write of 64 bytes.
> > 
> > This cuts the number of bus accesses in half if the fill level is known.
> > Also, the (posted) write accesses can be pipelined on the AXI bus since
> > they are no longer interleaved with (non-posted) reads.
> > 
> > bcm2835_spi_transfer_one_poll() previously switched to interrupt mode
> > when exceeding a time limit by calling bcm2835_spi_transfer_one_irq().
> > Because the latter now assumes an empty FIFO, it can no longer be called
> > directly.  Modify the CS register instead to achieve the same result.
> > 
> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> > Cc: Mathias Duckeck <m.duckeck@xxxxxxxxx>
> > Cc: Frank Pavlic <f.pavlic@xxxxxxxxx>
> > Cc: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> > Cc: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > ---
> > drivers/spi/spi-bcm2835.c | 66 ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 62 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> > index 36719d2cc12d..fd9a73963f8c 100644
> > --- a/drivers/spi/spi-bcm2835.c
> > +++ b/drivers/spi/spi-bcm2835.c
> > @@ -72,6 +72,8 @@
> > #define BCM2835_SPI_CS_CS_10		0x00000002
> > #define BCM2835_SPI_CS_CS_01		0x00000001
> > 
> > +#define BCM2835_SPI_FIFO_SIZE		64
> > +#define BCM2835_SPI_FIFO_SIZE_3_4	48
> Not sure if this should not be a DT parameter describing the HW block
> and not being hardcoded in the driver.

I see no reason for this. AFAIK all variants of the BCM2835 have the same FIFO length, so i'm fine with the defines. I only have doubts about the naming FIFO_SIZE_3_4 because it describe a fill level not a size.

Stefan