| Message ID | 20180205232120.5851-1-boris.brezillon@bootlin.com |
|---|---|
| Headers | show |
| Series | spi: Extend the framework to generically support memory devices | expand |
On Tue, Feb 06, 2018 at 12:21:14AM +0100, Boris Brezillon wrote: > SPI NAND layer): you can register a SPI NOR device directly from the > dedicated SPI memory controller, or it can be registered through the > SPI layer if the SPI controller is a generic SPI controller. While > the generic SPI controller path works fine, the dedicated SPI NOR > controller path brings its own set of issues: > * the SPI bus is not represented in sysfs I'm not sure if this is a big deal or not - at some point it's just an implementation detail of the hardware rather than something we're aware of or interested in. > * because there's no bus, there's no uevent, which means you'll have to > select both the SPI NAND and SPI NOR logic as soon as one driver > supports both interfaces if you don't want to run into loading > dependency issues This is sounding like we want a class (well, virtual bus in the new world) for these devices with a SPI based driver sitting on top of that for use with genuine SPI controllers. If the intention is as the comments in the code suggested that controllers implementing the memory mapping stuff don't use SPI at all then we could have the legacy SPI bus support be just another driver for this class. However when I look at what the drivers are actually doing it seems like that's not the case and the new API is intended to sit alongside normal SPI support, perhaps only implementing certain operations and using regular SPI for others. In that case it makes a lot more sense to have this be bolted on the side of SPI.
On Mon, 19 Feb 2018 16:25:10 +0000 Mark Brown <broonie@kernel.org> wrote: > On Tue, Feb 06, 2018 at 12:21:14AM +0100, Boris Brezillon wrote: > > > SPI NAND layer): you can register a SPI NOR device directly from the > > dedicated SPI memory controller, or it can be registered through the > > SPI layer if the SPI controller is a generic SPI controller. While > > the generic SPI controller path works fine, the dedicated SPI NOR > > controller path brings its own set of issues: > > > * the SPI bus is not represented in sysfs > > I'm not sure if this is a big deal or not - at some point it's just an > implementation detail of the hardware rather than something we're aware > of or interested in. > > > * because there's no bus, there's no uevent, which means you'll have to > > select both the SPI NAND and SPI NOR logic as soon as one driver > > supports both interfaces if you don't want to run into loading > > dependency issues > > This is sounding like we want a class (well, virtual bus in the new > world) for these devices with a SPI based driver sitting on top of that > for use with genuine SPI controllers. If the intention is as the > comments in the code suggested that controllers implementing the memory > mapping stuff don't use SPI at all then we could have the legacy SPI bus > support be just another driver for this class. However when I look at > what the drivers are actually doing it seems like that's not the case > and the new API is intended to sit alongside normal SPI support, perhaps > only implementing certain operations and using regular SPI for others. > In that case it makes a lot more sense to have this be bolted on the > side of SPI. This is a case: most controllers support both regular SPI transfers and memory-like operations (with some optimizations for memory oriented operations).
Hi Mark, Le 19/02/2018 à 17:25, Mark Brown a écrit : > On Tue, Feb 06, 2018 at 12:21:14AM +0100, Boris Brezillon wrote: > >> SPI NAND layer): you can register a SPI NOR device directly from the >> dedicated SPI memory controller, or it can be registered through the >> SPI layer if the SPI controller is a generic SPI controller. While >> the generic SPI controller path works fine, the dedicated SPI NOR >> controller path brings its own set of issues: > >> * the SPI bus is not represented in sysfs > > I'm not sure if this is a big deal or not - at some point it's just an > implementation detail of the hardware rather than something we're aware > of or interested in. > >> * because there's no bus, there's no uevent, which means you'll have to >> select both the SPI NAND and SPI NOR logic as soon as one driver >> supports both interfaces if you don't want to run into loading >> dependency issues > > This is sounding like we want a class (well, virtual bus in the new > world) for these devices with a SPI based driver sitting on top of that > for use with genuine SPI controllers. If the intention is as the > comments in the code suggested that controllers implementing the memory > mapping stuff don't use SPI at all then we could have the legacy SPI bus > support be just another driver for this class. However when I look at > what the drivers are actually doing it seems like that's not the case > and the new API is intended to sit alongside normal SPI support, perhaps > only implementing certain operations and using regular SPI for others. > In that case it makes a lot more sense to have this be bolted on the > side of SPI. > Indeed, if we take the example of the Atmel QSPI controller, the hardware mode supports two modes: - the legacy SPI mode: almost the same API (register layout) as the SPI controller driver in the SPI sub-system (drivers/spi/spi-atmel.c) - the SPI flash mode: using a new set of registers dedicated to SPI flash command support and used by the spi-nor driver (drivers/mtd/spi-nor/atmel_quadspi.c). With the sama5d2 version for the QSPI controller, in SPI flash mode, all SPI flash commands - not only Fast Read but also Page Program, register read/write, ... - must be sent through the AHB memory mapping. That's why the spi_flash_read() API proposed by the SPI sub-system was not suited to the Atmel QSPI controller: this current API only supports (Fast) Read operations but nothing else. With the API proposed by Boris, the Atmel QSPI driver could be moved into the SPI sub-system and the controller could also be used if connected to something else but a SPI flash. This is only an example but I guess other SPI controllers could be connected at the same time to both flash and non-flash SPI devices and handle them as needed from the SPI sub-system, using some dedicated API more suited than the legacy one when dealing with SPI flash devices. We already know that other SPI controllers may want to use the "SPI flash" API only for Fast Read but still rely on the legacy SPI API for all other SPI flash commands. So, as you've noticed, the two APIs are not exclusive but should live together. Then implementing a single SPI controller driver, supporting both APIs, would really help dealing with concurrency issues between SPI device drivers. Best regards, Cyrille
Hello all, I know a lot of you have already tried to address some of the flaws in the SPI NOR framework, and I'm coming with yet another proposal to solve some of the problems I faced recently :-/. The approach I'm proposing here has been motivated by the recent work done on SPI NANDs. Peter has proposed a SPI NAND controller interface (based on my suggestions) where the controller had to implement hooks that the SPI NAND framework could use to execute SPI NAND operations. I thought the approach was acceptable as a first step before we move to a solution where we share even more logic between SPI NOR and SPI NAND frameworks. But I recently changed my mind after working with Frieder to support his SPI NAND (Frieder has a SPI NAND connected to the FSL QSPI controller). And here come the problems: we definitely want one FSL QSPI driver that works for both NANDs and NORs. In the current state, that means forcing the FSL QSPI driver to implement both the SPI NAND and SPI NOR interfaces. When you look at the internal controller logic, if we want to support this without duplicating too much code, we have to add an intermediate representation that abstracts away the NOR/NAND specificities and pass only basic information to the controller: * the opcode * the number of address cycles * the number of dummy cycles * the number of data bytes to receive/transmit And the same problem is likely to hit most of the controller drivers placed in drivers/mtd/spi-nor. Another problem I see with the current approach is the duplication of DT parsing code in almost all drivers, even though most of them follow the generic SPI controller/device bindings. And finally, there's the confusion brought by the 2 entry points exposed by the SPI NOR layer (and that we had planned to mimic in the SPI NAND layer): you can register a SPI NOR device directly from the dedicated SPI memory controller, or it can be registered through the SPI layer if the SPI controller is a generic SPI controller. While the generic SPI controller path works fine, the dedicated SPI NOR controller path brings its own set of issues: * the SPI bus is not represented in sysfs * because there's no bus, there's no uevent, which means you'll have to select both the SPI NAND and SPI NOR logic as soon as one driver supports both interfaces if you don't want to run into loading dependency issues Not to mention that, in order to work properly, the SPI flash ids may have to be exported so that all QSPI controller drivers can use the same database. For all these reasons, I decided to try a new approach: move the generic "SPI memory" logic in the SPI framework (what I call the SPI memory interface here is actually not limited to SPI memories, and I'm pretty sure other QSPI devices use the same kind of protocol). So, basically, all kind of operations that can be done on a SPI memory are encoded with a CMD[+ADDR][+DUMMY][+DATA_{IN,OUT}] sequence, and that's pretty much what I'm adding here: a way to ask the SPI framework to execute a SPI memory operation. If the SPI controller this memory is connected to is a generic SPI controller, then those operations will be converted to generic SPI messages, but, if the controller has an high-level interface to send those operations, then it can expose it and the framework will use it. One last comment: this new interface makes the spi_flash_read() API redundant, because it's able to handle both read/write path and allows the same kind of optimizations you could achieve with the spi_flash_read hooks. To sum-up: the idea behind this series is to converge to a common/generic interface that could be used by any kind of SPI memory driver and still allow per-controller optimizations. Right now, the design is quite simple, and drivers can easily implement the various mem_ops to optimize things. I'm currently working on the conversion of the FSL driver, but I'm pretty sure most of the QSPI drivers lying in drivers/mtd/spi-nor can easily be converted to this new interface. Once, we'll have that done, the SPI NOR layer will declare itself as a regular spi_mem_driver and we'll be done with all the funky things we have there. The SPI NAND patches have already been converted to this approach. If you want to see the big picture, my development branch is here [1] and contains this series + the SPI NAND patches + a hack-ish FSL QSPI ported to this driver which will hopefully support both SPI NANDs and SPI NORs (only tested with a SPI NAND so far). I know it's a big introduction, and I probably have lost most of my readers before this point :-). But I'd really like to have feedback on this series, because I'd hate to have to add yet another layer of insane hacks in existing QSPI drivers to support both NORs and NANDs. Mark, Cyrille, Marek, and all people that have been involved in SPI-NOR development lately, please have a quick look at this RFC, and let me know what you think. Thanks, Boris [1]https://github.com/bbrezillon/linux/tree/spi-mem Boris Brezillon (6): spi: Extend the core to ease integration of SPI memory controllers spi: bcm-qspi: Implement the spi_mem interface spi: bcm53xx: Implement the spi_mem interface spi: ti-qspi: Implement the spi_mem interface mtd: spi-nor: Use the spi_mem_xx() API spi: Get rid of the spi_flash_read() API drivers/mtd/devices/m25p80.c | 240 ++++++++-------------- drivers/spi/spi-bcm-qspi.c | 173 ++++++++-------- drivers/spi/spi-bcm53xx.c | 38 +++- drivers/spi/spi-ti-qspi.c | 86 +++++--- drivers/spi/spi.c | 480 +++++++++++++++++++++++++++++++++++++------ include/linux/spi/spi.h | 263 ++++++++++++++++++++---- 6 files changed, 898 insertions(+), 382 deletions(-)