[PATCH v4 0/2] spi: cadence-quadpsi: Add support for the Cadence QSPI controller

From: Ramuthevar,Vadivel MuruganX
Date: Mon Dec 23 2019 - 01:30:14 EST


Add support for the Cadence QSPI controller. This controller is
present in the Intel Lightning Mountain(LGM) SoCs, Altera and TI SoCs.
This driver has been tested on the Intel LGM SoCs.

This driver does not support generic SPI and also the implementation
only supports spi-mem interface to replace the existing driver in
mtd/spi-nor/cadence-quadspi.c, the existing driver only support SPI-NOR
flash memory.

Thank you Vignesh for the valuable review comments and guidance,
sorry for the delay since waitting for the merge window to rebase
and send it now by one-shot.

changes from v3:
spi-cadence-quadspi.c
-- static to all functions wrt to local to the file.
-- Prefix cqspi_ and make the function static
-- cmd_ops, data_ops and dummy_ops dropped
-- addr_ops kept since it is required for address calculation.
-- devm_ used for supported functions , removed legacy API's
-- removed "indirect" name from functions
-- replaced by master->mode_bits = SPI_RX_QUAD | SPI_TX_DUAL | SPI_RX_DUAL | SPI_RX_OCTAL;
as per Vignesh susggestion
-- removed free functions since devm_ handles automatically.
-- dropped all unused Macros

YAML file update:
-- cadence,qspi.yaml file name replace by cdns,qspi-nor.yaml
-- compatible string updated as per Vignesh suggestion
-- for single entry, removed descriptions
-- removed optional parameters
Build Result:
linux$ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml dt_binding_check
CHKDT Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
SCHEMA Documentation/devicetree/bindings/processed-schema.yaml
DTC Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml
CHECK Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dt.yaml

changes from V2:
1. Supports Octal mode as well. So its 1/2/4/8-bit wide

2. Please be consistent with function names. Start all functions with
cadence_qspi_. If need you could have a shorter function prefix like cqspi_

Also, this and few other functions below can be static and they are not
used outside of this module.

3. Could you please keep polling routines from original driver
(drivers/mtd/spi-nor/cadence-quadspi.c). If you really want to improve
these helpers, please do those changes in a separate patch later on in
the series. This will greatly help in reviewing the code.

Also, I see that you have dropped many inline comments in the code.
These comments are quite important and I suggest not to drop them unless
they are no longer applicable

3. s/docoder/decoder

4. Please use cqspi_wait_for_bit() that uses readl_relaxed_poll_timeout()
here. There are couple corner cases that above code does not take care of.

5. Why do we need flash type specific handling here? Does not spi-nand
driver provide appropriate dummy_clk values? Why is the driver always
adding 8 dummy clks always?

6. Nope, no custom handling or interpreting of opcodes in driver.
Just set addrwidth to quad in CQSPI_REG_WR_INSTR when op->addr.buswidth
is 4.

7. This was wait_for_completion_timeout() previously. Have you tested
interrupting the sleep and verified that driver handles this case correctly?

8 rdid_length harcoded values are changed as generic

9. Please get ride of all flash type specific handling... There should be
no need for it. IF there are gaps in then lets discuss and fix it in
spi-nand and spi-nor frameworks and not in the driver.

spi-mem user should just take spi_mem_op template passed by the core and
execute it. No assumptions wrt flash type or opcode should be done.


10. Decision to use STIG was INDAC mode can be done using:

if (op->data.dir == SPI_MEM_DATA_IN && op->data.buf.in) {
if (!op->addr.nbytes)
mode = CQSPI_STIG_READ;
else
mode = IDC_READ_MODE;
} else {
if (!op->addr.nbytes || !op->data.buf.out)
mode = CQSPI_STIG_WRITE;
else
mode = IDC_WRITE_MODE;
}

No need for rdid_length etc.

11. This is a big gap and there is lot of code sharing that can be done b/w
INDAC and DAC mode.

Looks like driver isn't quite close to being ready. I suggest you also
take a look at patches for spi-mem conversion of cadence-quadspi driver
in U-Boot as well:

https://patchwork.ozlabs.org/cover/1176362/


12. Please dont add any new DT properties, there is no need for them. See
how this was handled in existing driver and reuse it

13. Slave DMA was never supported and is not needed.

14. Octal?

15. NACK, should be GPLv2 and no need for boilerplate text below when SPDX
Identifier is present.



Ramuthevar Vadivel Murugan (2):
dt-bindings: spi: Add schema for Cadence QSPI Controller driver
spi: cadence-quadpsi: Add support for the Cadence QSPI controller

.../devicetree/bindings/spi/cdns,qspi-nor.yaml | 147 +++
drivers/spi/Kconfig | 8 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-cadence-quadspi.c | 1172 ++++++++++++++++++++
drivers/spi/spi-cadence-quadspi.h | 245 ++++
5 files changed, 1573 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
create mode 100644 drivers/spi/spi-cadence-quadspi.c
create mode 100644 drivers/spi/spi-cadence-quadspi.h

--
2.11.0