Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver

[Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>]

On Fri, May 08, 2020 at 11:22:10AM +0100, Mark Brown wrote:
> On Fri, May 08, 2020 at 01:15:41PM +0300, Serge Semin wrote:
> > On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote:
> 
> > > > slave device. Taking into account the peculiarities of the controller
> > > > registers and physically mapped SPI flash access, very limited resources
> > > > and seeing the normal usecase of the controller is to access an external
> > > > SPI-nor flash, we decided to create a dedicated SPI driver for it.
> 
> > > It seems a lot of code.
> > > Why can't you use spi-dw-mmio.c et al.?
> 
> > I said above why. Even though the registers set is similar It's too specific
> > to be integrated into the generic DW SSI driver.
> 
> Can you be more specific about the issues?  From what you wrote it
> sounded like the main thing was chip select handling.

I thought it would be obvious from the patch itself. I've thoroughly described all
the issues there. Here in cover-letter it's a summary of the main ones.
Anyway here are all collected at once:

1) Registers mapping. The DW SSI registers are shifted by 0x100 with
respect to the MMIO region start. The lowest 0x100 registers are
responsible for the Baikal-T1 Boot Controller settings. There aren't much
of them there though. Our code is interested only in a flag, which switches
an accessibility of the DW APB SSI registers and direct SPI flash mapping.
And this switchability is a reason of another peculiarity (see the next
item for details).

2) SPI flash direct mapping. SPI flash direct mapping and DW APB SSI registers
are mutual exclusive, so only one of them can be enabled at a time. In
order to use the dirmap we have to switch the RDA bit off in the Boot
Controller setup register. If DW APB SSI registers need to be accessed the
RDA bit should be set. For this reason we have to make sure that dirmap
operations, SPI operations and SPI-mem-ops operations are exclusive, since
some of them need to interact with the DW APB SSI registers, while another
the directly mapped SPI flash MMIO (currently ctlr->io_mutex is responsible
for this).

3) A specific access to MMIO (concerning directly mapped SPI flash MMIO).
The SoC interconnect is designed in a way so we can't use any instruction to
read/write from/to the MMIO space. It has to be done by lw/sw with
dword-aligned address passed. Though in this driver we only use a read
operation from the directly mapped SPI flash memory.

4) No direct handling of the CS. Though this is an issue of all DW SSI
controllers, here with very small FIFO and no DMA/IRQ supported it mandates to
workaround any preemption/interruption during a non-GPIO-CS-based transfer.
For the same reason the driver doesn't support normal spi-messages based
interface if no GPIO-CS supplied. In addition since FIFO is too small and most
of our platforms don't have GPIO-CS support we had to create the SPI-mem-ops
instead of generic SPI-callback.

5) MMIO access race condition. As I described in the in-code comment it's a
very tricky race happening during concurrent access from different cores to the
APB bus. Due to this if SPI interface is working high frequency like
12.5 - 25 MHz and there is some another code working with APB bus on another
core, the SPI data pushing function might not keep up with filling small FIFO
(8 bytes) on time, since the APB bus has got just 50 MHz frequency. Due to
this I had artificially limit the SPI bus frequency if there is more than one
CPU in the system.

6) A single CS. It's normally connected to an external SPI flash so the
driver is equipped with mem-ops and dirmap out of the box. BTW normal
SPI-operations are in fact unneeded on all the platforms currently
equipped with Baikal-T1 because all of them have got SPI flash attached to
this interface, and most likely it will be like in new platforms too.

7) No interrupts. Yes, this is another peculiarity. Since this DW APB core
is a part of the boot controller it just don't need IRQs. Boot controller
is responsible for the code loading from SPI flash. It has got a dedicated
RTL which provide a transparent access to the flash just by reading from a
corresponding MMIO range (see the SPI flash direct mapping described above).

8) No DMA. Yeah, and DMA isn't supported for the same reason.

I am pretty sure I have forgotten something. Anyway it has been much easier
to create a new driver instead of integrating all of these into a generic
one. Integrating something like this in the current DW APB SSI driver would
mean to have it completely overwritten (refactored if you want) which would
bring us to a new driver anyway. I don't think it would be good to
complicate the generic driver with so many peculiar things scattered around
the code with various hooks or ifdef, especially seeing the current code has
already become a bit messy.

> 
> > > > The driver provides callbacks for native messages-based SPI interface,
> > > > SPI-memory and direct mapping read operations. Due to not having any
> > > > asynchronous signaling interface provided by the core we have no choice
> 
> What do you mean by "asynchronous signaling interface provided by the
> core" here?

By asynchronous signalling I meant DMA and IRQ. Non of them provided by the
controller.

-Sergey