| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Jan 2019 10:35:35 +0000
From: Schrempf Frieder <frieder.schrempf@...tron.de>
To: Yogesh Narayan Gaur <yogeshnarayan.gaur@....com>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
"boris.brezillon@...tlin.com" <boris.brezillon@...tlin.com>,
"marek.vasut@...il.com" <marek.vasut@...il.com>,
"broonie@...nel.org" <broonie@...nel.org>,
"linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC: "robh@...nel.org" <robh@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"computersforpeace@...il.com" <computersforpeace@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 1/5] spi: spi-mem: Add driver for NXP FlexSPI
controller
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@....com>
> ---
> 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@...tron.de>
Thanks,
Frieder
Powered by blists - more mailing lists