Re: [PATCH v7 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller

[Schrempf Frieder <frieder.schrempf@xxxxxxxxxx>]

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 <yogeshnarayan.gaur@xxxxxxx>
> ---
> 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 <frieder.schrempf@xxxxxxxxxx>

Thanks,
Frieder