Hello Sudip On Tue, Aug 02, 2022 at 06:57:44PM +0100, Sudip Mukherjee wrote: > Some Synopsys SSI controllers support enhanced SPI which includes > Dual mode, Quad mode and Octal mode. DWC_ssi includes clock stretching > feature in enhanced SPI modes which can be used to prevent FIFO underflow > and overflow conditions while transmitting or receiving the data respectively. > This is only tested on controller version 1.03a. Thank you very much the patchset. As I already said adding new controller features support is always welcome. Yet there are some things which need to be fixed before the series would be fully suitable to be merged in into the kernel. Here is a short summary of ones: 1. The eSPI capability isn't specific for the DW AHB SSI controller only. It can be found on the DW APB SSI controllers of v4.x and newer (though the SPI_FRF field is placed at different offset in CTRL0 CSR). Thus your patches will need to be fixed so the in-driver infrastructure would imply that. 2. The mem ops check procedure provided by you doesn't verify whether the passed cmd, address and dummy data lengths meet the controller constraints or at least the constraints set by your code. You always expect the address and command being 4 and 1 bytes long, which is way not always true. So the implementation provided by you just won't correctly work for the unsupported cases with no any error returned. 3. From what I see WAIT_CYCLES is specific for the Read-operations only (see the controller HW manual, the paragraphs like "Write Operation in Enhanced SPI Modes" or the SPI_CTRL0.WAIT_CYCLES field description). So any dummy-bytes requested for the Tx operations just won't be sent. Even though AFAICS the dummy cycles are specific for the Read SPI NAND/NOR operations it still would be correct to explicitly refuse the non-Rx transactions with non-zero dummy data length. 4. I don't really see a reason of adding the address, command and dummy data length constraints. You can as easily update the command/address/dummy lengths passed in the SPI mem-op structure thus providing wider SPI memory devices range support. 5. The main problem I can see in your implementation is that you try to assimilate the eSPI feature for the current DW SSI EEPROM read/write infrastructure. Note the SPI MEM ops currently available in the driver have been specifically created for the platforms with the native CS'es used to access the EEPROM devices. For such cases I had to use some not that nice solutions like IRQ-less transfers, local IRQs disabling and the outbound data collection in a single buffer in order to bypass the nasty DW SSI controller peculiarities. All of that isn't required in your case. You can implement a very nice and robust algorithm. 6. You said your controller supports the clock stretching on Tx and Rx transfers. This is a very useful feature which can be used to bypass the main DW SSI controller problem connected with the native CS auto-toggling when the Tx FIFO gets empty or data loose due to the Rx FIFO overruns. Thus you won't need to always keep up with the Tx/Rx FIFO levels and implement the IRQ-based SPI MEM transfers. 7. You unconditionally enable the eSPI support for the generic device snps,dwc-ssi-1.03a while this is an optional feature which yet can be disabled for any new controllers (see the SSI_SPI_MODE IP-core synthesize parameter). What you really need is to simply auto-detect the eSPI feature availability by checking whether the SPI_FRF field is writable for the DW APB SSI v4.0a and newer and for any DWC AHB SSI. 8. There is no need in the IP-core version added to the compatible string because it can be retrieved from the SSI_VERSION_ID CSR. I would suggest to add a new generic compatible string "snps,dw-ahb-ssi" for the DW AHB SSI controllers and forget about the compatible strings versioning. 9. Always study the driver coding convention before updating. In this particular case should you need to add new methods, macros, etc please add the vendor-specific prefix as is done for the rest of the driver entities. I've deliberately collected all the generic comments here so you'd be aware of the required changes in total, because I very much doubt all of them could be fixed at once via a single patchset iteration. But as soon as all of them are fixed we'll get a very nice and neat solution for the eSPI feature. I'll give you some more detailed comments right in the corresponding patches, but they won't cover all the issues noted above on this patchset iteration. So feel free to update your series based on your understanding of the issues (you can ask me if you don't fully get what I said above). It may reduce the number of the further series re-submissions. -Sergey > > Ben Dooks (1): > spi: dw-apb-ssi: add generic 1.03a version > > Sudip Mukherjee (10): > spi: dw: define capability for enhanced spi > spi: dw: add check for support of dual/quad/octal > spi: dw: define spi_frf for dual/quad/octal modes > spi: dw: use TMOD_RO to read in enhanced spi modes > spi: dw: define SPI_CTRLR0 register and its fields > spi: dw: update SPI_CTRLR0 register > spi: dw: update NDF while writing in enhanced spi mode > spi: dw: update buffer for enhanced spi mode > spi: dw: prepare the transfer routine for enhanced mode > spi: dw: initialize dwc-ssi-1.03a controller > > .../bindings/spi/snps,dw-apb-ssi.yaml | 1 + > drivers/spi/spi-dw-core.c | 288 ++++++++++++++++-- > drivers/spi/spi-dw-mmio.c | 10 + > drivers/spi/spi-dw.h | 19 ++ > 4 files changed, 291 insertions(+), 27 deletions(-) > > -- > 2.30.2 >
