Hi Yogesh,

On 15.01.19 10:50, Yogesh Narayan Gaur wrote:

- Add driver for NXP FlexSPI host controller

(0) What is the FlexSPI controller?
  FlexSPI is a flexsible SPI host controller which supports two SPI
  channels and up to 4 external devices. Each channel supports
  Single/Dual/Quad/Octal mode data transfer (1/2/4/8 bidirectional
  data lines) i.e. FlexSPI acts as an interface to external devices,
  maximum 4, each with up to 8 bidirectional data lines.

  It uses new SPI memory interface of the SPI framework to issue
  flash memory operations to up to four connected flash
  devices (2 buses with 2 CS each).

(1) Tested this driver with the mtd_debug and JFFS2 filesystem utility
  on NXP LX2160ARDB and LX2160AQDS targets.
  LX2160ARDB is having two NOR slave device connected on single bus A
  i.e. A0 and A1 (CS0 and CS1).
  LX2160AQDS is having two NOR slave device connected on separate buses
  one flash on A0 and second on B1 i.e. (CS0 and CS3).
  Verified this driver on following SPI NOR flashes:
     Micron, mt35xu512ab, [Read - 1 bit mode]
     Cypress, s25fl512s, [Read - 1/2/4 bit mode]

Signed-off-by: Yogesh Narayan Gaur <redacted>
---
Changes for v7:
- Add func pointer for '.get_name' for struct spi_controller_mem_ops
- Add input address range check as per controller memory mapped space
- Update _fill_txfifo/_read_rxfifo funcs as per Frieder review comments
Changes for v6:
- Rebase on top of v5.0-rc1
- Updated as per Frieder review comments and perform code cleanup
- Updated _fill_txfifo/_read_rxfifo func write/read logic
Changes for v5:
- Rebase on top of v4.20-rc2
- Modified fspi_readl_poll_tout() as per review comments
- Arrange header file in alphabetical order
- Removed usage of read()/write() function callback pointer
- Add support for 1 and 2 byte address length
- Change Frieder e-mail to new e-mail address
Changes for v4:
- Incorporate Boris review comments
   * Use readl_poll_timeout() instead of busy looping.
   * Re-define register masking as per comment.
   * Drop fspi_devtype enum.
Changes for v3:
- Added endianness flag in platform specific structure instead of DTS.
- Modified nxp_fspi_read_ahb(), removed remapping code.
- Added Boris and Frieder as Author and provided reference of spi-fsl-qspi.c
Changes for v2:
- Incorporated Boris review comments.
- Remove dependency of driver over connected flash device size.
- Modified the logic to select requested CS.
- Remove SPI-Octal Macros.

  drivers/spi/Kconfig        |   10 +
  drivers/spi/Makefile       |    1 +
  drivers/spi/spi-nxp-fspi.c | 1105 ++++++++++++++++++++++++++++++++++++
  3 files changed, 1116 insertions(+)
  create mode 100644 drivers/spi/spi-nxp-fspi.c

[...]

+
+static bool nxp_fspi_supports_op(struct spi_mem *mem,
+				 const struct spi_mem_op *op)
+{
+	struct nxp_fspi *f = spi_controller_get_devdata(mem->spi->master);
+	int ret;
+
+	ret = nxp_fspi_check_buswidth(f, op->cmd.buswidth);
+
+	if (op->addr.nbytes)
+		ret |= nxp_fspi_check_buswidth(f, op->addr.buswidth);
+
+	if (op->dummy.nbytes)
+		ret |= nxp_fspi_check_buswidth(f, op->dummy.buswidth);
+
+	if (op->data.nbytes)
+		ret |= nxp_fspi_check_buswidth(f, op->data.buswidth);
+
+	if (ret)
+		return false;
+
+	/*
+	 * The number of address bytes should be equal to and less than 4bytes.

Nitpick: use "or" instead of "and", add a space between "4" and "bytes".

+	 */
+	if (op->addr.nbytes > 4)
+		return false;
+
+	/*
+	 * If requested address value is greater than controller assigned
+	 * memory mapped space, return error as it didn't fit in the range
+	 * of assigned address space.
+	 */
+	if (op->addr.val >= f->memmap_phy_size)
+		return false;
+
+	/* Max 64 dummy clock cycles supported */
+	if (op->dummy.buswidth &&
+	    (op->dummy.nbytes * 8 / op->dummy.buswidth > 64))
+		return false;
+
+	/* Max data length, check controller limits and alignment */
+	if (op->data.dir == SPI_MEM_DATA_IN &&
+	    (op->data.nbytes > f->devtype_data->ahb_buf_size ||
+	     (op->data.nbytes > f->devtype_data->rxfifo - 4 &&
+	      !IS_ALIGNED(op->data.nbytes, 8))))
+		return false;
+
+	if (op->data.dir == SPI_MEM_DATA_OUT &&
+	    op->data.nbytes > f->devtype_data->txfifo)
+		return false;
+
+	return true;
+}

[...]

+static void nxp_fspi_fill_txfifo(struct nxp_fspi *f,
+				 const struct spi_mem_op *op)
+{
+	void __iomem *base = f->iobase;
+	int i, ret;
+
+	/* clear the TX FIFO. */
+	fspi_writel(f, FSPI_IPTXFCR_CLR, base + FSPI_IPTXFCR);
+
+	/*
+	 * Default value of water mark level is 8 bytes, hence in single
+	 * write request controller can write max 8 bytes of data.
+	 */
+
+	for (i = 0; i < ALIGN_DOWN(op->data.nbytes, 8); i += 8) {
+		/* Wait for TXFIFO empty */
+		ret = fspi_readl_poll_tout(f, f->iobase + FSPI_INTR,
+					   FSPI_INTR_IPTXWE, 0,
+					   POLL_TOUT, true);
+		WARN_ON(ret);
+
+		fspi_writel(f, *(u32 *) ((u8 *)op->data.buf.out + i),
+			    base + FSPI_TFDR);
+		fspi_writel(f, *(u32 *) ((u8 *)op->data.buf.out + i + 4),
+			    base + FSPI_TFDR + 4);

Nitpick: Add a "u8 *buf = (u8 *)op->data.buf.out" to the top of the 
function and use it here...

+		fspi_writel(f, FSPI_INTR_IPTXWE, base + FSPI_INTR);
+	}
+
+	if (i < op->data.nbytes) {
+		u32 data = 0;
+		int j;
+		/* Wait for TXFIFO empty */
+		ret = fspi_readl_poll_tout(f, f->iobase + FSPI_INTR,
+					   FSPI_INTR_IPTXWE, 0,
+					   POLL_TOUT, true);
+		WARN_ON(ret);
+
+		for (j = 0; j < ALIGN(op->data.nbytes - i, 4); j += 4) {
+			memcpy(&data, op->data.buf.out + i + j, 4);

...and also here.

+			fspi_writel(f, data, base + FSPI_TFDR + j);
+		}
+		fspi_writel(f, FSPI_INTR_IPTXWE, base + FSPI_INTR);
+	}
+}
+
+static void nxp_fspi_read_rxfifo(struct nxp_fspi *f,
+			  const struct spi_mem_op *op)
+{
+	void __iomem *base = f->iobase;
+	int i, ret;
+	int len = op->data.nbytes;
+	u8 *buf = (u8 *) op->data.buf.in;
+
+	/*
+	 * Default value of water mark level is 8 bytes, hence in single
+	 * read request controller can read max 8 bytes of data.
+	 */
+	for (i = 0; i < ALIGN_DOWN(len, 8); i += 8) {
+		/* Wait for RXFIFO available */
+		ret = fspi_readl_poll_tout(f, f->iobase + FSPI_INTR,
+					   FSPI_INTR_IPRXWA, 0,
+					   POLL_TOUT, true);
+		WARN_ON(ret);
+
+		*(u32 *)(buf + i) = fspi_readl(f, base + FSPI_RFDR);
+		*(u32 *)(buf + i + 4) = fspi_readl(f, base + FSPI_RFDR + 4);
+		/* move the FIFO pointer */
+		fspi_writel(f, FSPI_INTR_IPRXWA, base + FSPI_INTR);
+	}
+
+	if (i < len) {
+		u32 tmp;
+		int size, j;
+
+		buf = op->data.buf.in + i;
+		/* Wait for RXFIFO available */
+		ret = fspi_readl_poll_tout(f, f->iobase + FSPI_INTR,
+					   FSPI_INTR_IPRXWA, 0,
+					   POLL_TOUT, true);
+		WARN_ON(ret);
+
+		len = op->data.nbytes - i;
+		for (j = 0; j < (ALIGN(len, 4))/4; j++, buf += size) {
+			tmp = fspi_readl(f, base + FSPI_RFDR + j * 4);
+			size = min(len, 4);
+			memcpy(buf, &tmp, size);
+		}

In the second iteration of this loop, the calculation of size seems 
wrong, as len has not changed.

Maybe this could work?:

len = op->data.nbytes - i;
for (j = 0; j < op->data.nbytes - i; j += 4) {
	tmp = fspi_readl(f, base + FSPI_RFDR + j);
	size = min(len, 4);
	memcpy(buf + j, &tmp, size);
	len -= size;
}

When this is fixed and tested you can add:

Reviewed-by: Frieder Schrempf <redacted>

Thanks,
Frieder