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
