RE: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller
Hi Boris, > -Original Message- > From: Boris Brezillon [mailto:bbrezil...@kernel.org] > Sent: Monday, January 14, 2019 2:08 PM > To: Schrempf Frieder > Cc: Yogesh Narayan Gaur ; linux- > m...@lists.infradead.org; boris.brezil...@bootlin.com; marek.va...@gmail.com; > broo...@kernel.org; linux-...@vger.kernel.org; devicet...@vger.kernel.org; > mark.rutl...@arm.com; r...@kernel.org; linux-kernel@vger.kernel.org; > computersforpe...@gmail.com; shawn...@kernel.org; linux-arm- > ker...@lists.infradead.org > Subject: Re: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI > controller > > On Thu, 10 Jan 2019 17:28:57 + > Schrempf Frieder wrote: > > > Hi Yogesh, > > > > my comments below are mainly about things I already mentioned in my > > review for v5 and about removing or simplifying some unnecessary or > > complex code. > > [...] > > > > Once you've addressed all of Frieder's comments you can add > > Reviewed-by: Boris Brezillon > Thanks. I have send next version, v7, of the patch series having added code changes as per Frieder's comments. [1] Based on the feedback from Frieder, would add yours r-o-b tag. -- Regards Yogesh Gaur [1] https://patchwork.ozlabs.org/project/linux-mtd/list/?series=86130 > Regards, > > Boris
Re: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller
On Thu, 10 Jan 2019 17:28:57 +
Schrempf Frieder wrote:
> Hi Yogesh,
>
> my comments below are mainly about things I already mentioned in my
> review for v5 and about removing or simplifying some unnecessary or
> complex code.
>
> Also as I gathered from your conversation with Boris, there's still a
> check for the length of the requested memory missing.
>
> On 08.01.19 10:24, Yogesh Narayan Gaur wrote:
> [...]
> > +
> > +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 instructions needed for the op, needs
> > +* to fit into a single LUT entry.
> > +*/
> > + if (op->addr.nbytes +
> > + (op->dummy.nbytes ? 1:0) +
> > + (op->data.nbytes ? 1:0) > 6)
> > + return false;
>
> Actually this check was only needed in the QSPI driver, as we were using
> LUT_MODE and there we needed one instruction for each address byte.
> Here the number of instructions will always fit into one LUT entry.
>
> Instead of this, a check for op->addr.nbytes > 4 (as already suggested
> in my comments for v5) would make more sense. So we can make sure that
> the address length passed in is supported (between 1 and 4 bytes).
>
> > +
> > + /* 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_select_mem(struct nxp_fspi *f, struct spi_device *spi)
> > +{
> > + unsigned long rate = spi->max_speed_hz;
> > + int ret;
> > + uint64_t size_kb;
> > +
> > + /*
> > +* Return, if previously selected slave device is same as current
> > +* requested slave device.
> > +*/
> > + if (f->selected == spi->chip_select)
> > + return;
> > +
> > + /* Reset FLSHxxCR0 registers */
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHA1CR0);
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHA2CR0);
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHB1CR0);
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHB2CR0);
> > +
> > + /* Assign controller memory mapped space as size, KBytes, of flash. */
> > + size_kb = FSPI_FLSHXCR0_SZ(f->memmap_phy_size);
> > +
> > + switch (spi->chip_select) {
> > + case 0:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0);
> > + break;
> > + case 1:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA2CR0);
> > + break;
> > + case 2:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB1CR0);
> > + break;
> > + case 3:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB2CR0);
> > + break;
> > + }
>
> The switch statement above can be replaced by:
>
> fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0 +
> 4 * spi->chip_select);
>
> > +
> > + dev_dbg(f->dev, "Slave device [CS:%x] selected\n", spi->chip_select);
> > +
> > + nxp_fspi_clk_disable_unprep(f);
> > +
> > + ret = clk_set_rate(f->clk, rate);
> > + if (ret)
> > + return;
> > +
> > + ret = nxp_fspi_clk_prep_enable(f);
> > + if (ret)
> > + return;
> > +
> > + f->selected = spi->chip_select;
> > +}
> > +
> > +static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op
> > *op)
> > +{
> > + u32 len = op->data.nbytes;
> > +
> > + /* Read out the data directly from the AHB buffer. */
> > + memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len);
> > +}
> > +
> > +static void nxp_fspi_fill_txfifo(struct nxp_fspi *f,
> > +const struct spi_mem_op *op)
> > +{
> > + void __iomem *base = f->iobase;
> > + int i, j, ret;
> > + int size, tmp, wm_size;
> > + u32 data = 0;
> > + u32 *txbuf = (u32 *) op->data.buf.out;
>
> You can cast the u8 buffer to u32 and increment it by 1 in each cycle
> below, or you can just use the u8
Re: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller
Hi Yogesh,
my comments below are mainly about things I already mentioned in my
review for v5 and about removing or simplifying some unnecessary or
complex code.
Also as I gathered from your conversation with Boris, there's still a
check for the length of the requested memory missing.
On 08.01.19 10:24, Yogesh Narayan Gaur wrote:
[...]
> +
> +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 instructions needed for the op, needs
> + * to fit into a single LUT entry.
> + */
> + if (op->addr.nbytes +
> +(op->dummy.nbytes ? 1:0) +
> +(op->data.nbytes ? 1:0) > 6)
> + return false;
Actually this check was only needed in the QSPI driver, as we were using
LUT_MODE and there we needed one instruction for each address byte.
Here the number of instructions will always fit into one LUT entry.
Instead of this, a check for op->addr.nbytes > 4 (as already suggested
in my comments for v5) would make more sense. So we can make sure that
the address length passed in is supported (between 1 and 4 bytes).
> +
> + /* 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_select_mem(struct nxp_fspi *f, struct spi_device *spi)
> +{
> + unsigned long rate = spi->max_speed_hz;
> + int ret;
> + uint64_t size_kb;
> +
> + /*
> + * Return, if previously selected slave device is same as current
> + * requested slave device.
> + */
> + if (f->selected == spi->chip_select)
> + return;
> +
> + /* Reset FLSHxxCR0 registers */
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHA1CR0);
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHA2CR0);
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHB1CR0);
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHB2CR0);
> +
> + /* Assign controller memory mapped space as size, KBytes, of flash. */
> + size_kb = FSPI_FLSHXCR0_SZ(f->memmap_phy_size);
> +
> + switch (spi->chip_select) {
> + case 0:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0);
> + break;
> + case 1:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA2CR0);
> + break;
> + case 2:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB1CR0);
> + break;
> + case 3:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB2CR0);
> + break;
> + }
The switch statement above can be replaced by:
fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0 +
4 * spi->chip_select);
> +
> + dev_dbg(f->dev, "Slave device [CS:%x] selected\n", spi->chip_select);
> +
> + nxp_fspi_clk_disable_unprep(f);
> +
> + ret = clk_set_rate(f->clk, rate);
> + if (ret)
> + return;
> +
> + ret = nxp_fspi_clk_prep_enable(f);
> + if (ret)
> + return;
> +
> + f->selected = spi->chip_select;
> +}
> +
> +static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op
> *op)
> +{
> + u32 len = op->data.nbytes;
> +
> + /* Read out the data directly from the AHB buffer. */
> + memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len);
> +}
> +
> +static void nxp_fspi_fill_txfifo(struct nxp_fspi *f,
> + const struct spi_mem_op *op)
> +{
> + void __iomem *base = f->iobase;
> + int i, j, ret;
> + int size, tmp, wm_size;
> + u32 data = 0;
> + u32 *txbuf = (u32 *) op->data.buf.out;
You can cast the u8 buffer to u32 and increment it by 1 in each cycle
below, or you can just use the u8 buffer and align and increment by 8 as
I did in my proposal for v5.
I still like my version better as it seems simpler and easier to
understand ;)
> +
> + /* clear the TX FIFO. */
> +
[PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller
- 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 --- 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 | 1095 3 files changed, 1106 insertions(+) create mode 100644 drivers/spi/spi-nxp-fspi.c diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index dc67eda1788a..fc4cc7a65c33 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -279,6 +279,16 @@ config SPI_FSL_QUADSPI This controller does not support generic SPI messages. It only supports the high-level SPI memory interface. +config SPI_NXP_FLEXSPI + tristate "NXP Flex SPI controller" + depends on ARCH_LAYERSCAPE || HAS_IOMEM + help + This enables support for the Flex SPI controller in master mode. + Up to four slave devices can be connected on two buses with two + chipselects each. + This controller does not support generic SPI messages and only + supports the high-level SPI memory interface. + config SPI_GPIO tristate "GPIO-based bitbanging SPI Master" depends on GPIOLIB || COMPILE_TEST diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 2a857cb9aa81..5c5af4676279 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -64,6 +64,7 @@ obj-$(CONFIG_SPI_MXIC)+= spi-mxic.o obj-$(CONFIG_SPI_MXS) += spi-mxs.o obj-$(CONFIG_SPI_NPCM_PSPI)+= spi-npcm-pspi.o obj-$(CONFIG_SPI_NUC900) += spi-nuc900.o +obj-$(CONFIG_SPI_NXP_FLEXSPI) += spi-nxp-fspi.o obj-$(CONFIG_SPI_OC_TINY) += spi-oc-tiny.o spi-octeon-objs:= spi-cavium.o spi-cavium-octeon.o obj-$(CONFIG_SPI_OCTEON) += spi-octeon.o diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c new file mode 100644 index ..b271afaed4bc --- /dev/null +++ b/drivers/spi/spi-nxp-fspi.c @@ -0,0 +1,1095 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/* + * NXP FlexSPI(FSPI) controller driver. + * + * Copyright 2019 NXP. + * + * 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). + * + * FlexSPI controller is driven by the LUT(Look-up Table) registers + * LUT registers are a look-up-table for sequences of instructions. + * A valid sequence consists of four LUT registers. + * Maximum 32 LUT sequences can be programmed simultaneously. + * + * LUTs are being created at run-time based on the commands passed + * from the spi-mem framework, thus using single LUT index. + * + * Software triggered Flash read/write access by IP Bus. + * + * Memory mapped read access by AHB Bus. + * + * Based on SPI MEM interface and spi-fsl-qspi.c driver. + * + *
