[Patch v2 1/7] spi: Transform the FSL QuadSPI driver to use the SPI MEM API

[Kuldeep Singh <kuldeep.singh@nxp.com>]

Hi Priyanka,

> -----Original Message-----
> From: Priyanka Jain <priyanka.jain@nxp.com>
> Sent: Tuesday, December 17, 2019 10:38 AM
> To: Kuldeep Singh <kuldeep.singh@nxp.com>; u-boot at lists.denx.de
> Cc: jagan at amarulasolutions.com; sr at denx.de; Ashish Kumar
> <ashish.kumar@nxp.com>; frieder.schrempf at kontron.de; Kuldeep Singh
> <kuldeep.singh@nxp.com>; Ashish Kumar <ashish.kumar@nxp.com>
> Subject: RE: [Patch v2 1/7] spi: Transform the FSL QuadSPI driver to use the SPI
> MEM API
> 
> 
> 
> >-----Original Message-----
> >From: Kuldeep Singh <kuldeep.singh@nxp.com>
> >Sent: Monday, December 16, 2019 5:19 PM
> >To: u-boot at lists.denx.de
> >Cc: jagan at amarulasolutions.com; Priyanka Jain <priyanka.jain@nxp.com>;
> >sr at denx.de; Ashish Kumar <ashish.kumar@nxp.com>;
> >frieder.schrempf at kontron.de; Kuldeep Singh <kuldeep.singh@nxp.com>;
> >Ashish Kumar <ashish.kumar@nxp.com>
> >Subject: [Patch v2 1/7] spi: Transform the FSL QuadSPI driver to use
> >the SPI MEM API
> >
> >To support the SPI MEM API, instead of modifying the existing U-Boot
> >driver, this patch adds a port of the existing Linux driver.
> >This also has the advantage that porting changes and fixes from Linux
> >will be easier.
> >Porting of driver left most of the functions unchanged while few of the
> >changes are:
> >-Remove lock(mutexes) and irq handler as uboot is a single core execution.
> >-Remove clock support as the changing spi speed is not supported in
> >uboot and nor in linux.
> >
> >Currently tested on LS1088ARDB, LS1012ARDB, LS1046ARDB, LS1046AFRWY,
> >LS1043AQDS, LS1021ATWR, LS2080ARDB
> >
> >Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >Signed-off-by: Ashish Kumar <Ashish.Kumar@nxp.com>
> >Signed-off-by: Kuldeep Singh <kuldeep.singh@nxp.com>
> >---
> > drivers/spi/fsl_qspi.c | 1562 +++++++++++++++-------------------------
> > drivers/spi/fsl_qspi.h |  145 ----
> > 2 files changed, 593 insertions(+), 1114 deletions(-)  delete mode
> >100644 drivers/spi/fsl_qspi.h
> >
> >diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
> >8e2a09df36..96178c06bc 100644
> >--- a/drivers/spi/fsl_qspi.c
> >+++ b/drivers/spi/fsl_qspi.c
> >@@ -1,1142 +1,766 @@
> > // SPDX-License-Identifier: GPL-2.0+
> >+
> > /*
> >- * Copyright 2013-2015 Freescale Semiconductor, Inc.
> >+ * Freescale QuadSPI driver.
> >+ *
> >+ * Copyright (C) 2013 Freescale Semiconductor, Inc.
> >+ * Copyright (C) 2018 Bootlin
> >+ * Copyright (C) 2018 exceet electronics GmbH
> >+ * Copyright (C) 2018 Kontron Electronics GmbH
> >+ * Copyright (C) 2019-2020 NXP
> Please check copyright format
> It seems the changes have been brought from Linux driver.
> If yes, it would be good to mention that in File header

Ok. I will mention that information on top.

> >  *
> >- * Freescale Quad Serial Peripheral Interface (QSPI) driver
> >+ * Based on the original fsl-quadspi.c spi-nor driver.
> >+ * Transition to spi-mem in spi-fsl-qspi.c
> >  */
> >
> > #include <common.h>
> >-#include <malloc.h>
> >-#include <spi.h>
> > #include <asm/io.h>
> >-#include <linux/sizes.h>
> >-#include <linux/iopoll.h>
> > #include <dm.h>
> >-#include <errno.h>
> >-#include <watchdog.h>
> >-#include <wait_bit.h>
> >-#include "fsl_qspi.h"
> >+#include <linux/iopoll.h>
> >+#include <linux/sizes.h>
> >+#include <spi.h>
> >+#include <spi-mem.h>
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> >-#define OFFSET_BITS_MASK	GENMASK(23, 0)
> >-
> >-#define FLASH_STATUS_WEL	0x02
> >-
> >-/* SEQID */
> >-#define SEQID_WREN		1
> >-#define SEQID_FAST_READ		2
> >-#define SEQID_RDSR		3
> >-#define SEQID_SE		4
> >-#define SEQID_CHIP_ERASE	5
> >-#define SEQID_PP		6
> >-#define SEQID_RDID		7
> >-#define SEQID_BE_4K		8
> >-#ifdef CONFIG_SPI_FLASH_BAR
> >-#define SEQID_BRRD		9
> >-#define SEQID_BRWR		10
> >-#define SEQID_RDEAR		11
> >-#define SEQID_WREAR		12
> >-#endif
> >-#define SEQID_WRAR		13
> >-#define SEQID_RDAR		14
> >-
> >-/* QSPI CMD */
> >-#define QSPI_CMD_PP		0x02	/* Page program (up to 256
> bytes) */
> >-#define QSPI_CMD_RDSR		0x05	/* Read status register */
> >-#define QSPI_CMD_WREN		0x06	/* Write enable */
> >-#define QSPI_CMD_FAST_READ	0x0b	/* Read data bytes (high
> >frequency) */
> >-#define QSPI_CMD_BE_4K		0x20    /* 4K erase */
> >-#define QSPI_CMD_CHIP_ERASE	0xc7	/* Erase whole flash chip */
> >-#define QSPI_CMD_SE		0xd8	/* Sector erase (usually 64KiB)
> */
> >-#define QSPI_CMD_RDID		0x9f	/* Read JEDEC ID */
> >-
> >-/* Used for Micron, winbond and Macronix flashes */
> >-#define	QSPI_CMD_WREAR		0xc5	/* EAR register write
> */
> >-#define	QSPI_CMD_RDEAR		0xc8	/* EAR reigster read
> */
> >-
> >-/* Used for Spansion flashes only. */
> >-#define	QSPI_CMD_BRRD		0x16	/* Bank register read
> >*/
> >-#define	QSPI_CMD_BRWR		0x17	/* Bank register
> write
> >*/
> >-
> >-/* Used for Spansion S25FS-S family flash only. */
> >-#define QSPI_CMD_RDAR		0x65	/* Read any device register
> */
> >-#define QSPI_CMD_WRAR		0x71	/* Write any device register
> */
> >-
> >-/* 4-byte address QSPI CMD - used on Spansion and some Macronix flashes
> */
> >-#define QSPI_CMD_FAST_READ_4B	0x0c    /* Read data bytes (high
> >frequency) */
> >-#define QSPI_CMD_PP_4B		0x12    /* Page program (up to 256
> >bytes) */
> >-#define QSPI_CMD_SE_4B		0xdc    /* Sector erase (usually 64KiB)
> >*/
> >-
> >-/* fsl_qspi_platdata flags */
> >-#define QSPI_FLAG_REGMAP_ENDIAN_BIG	BIT(0)
> >-
> >-/* default SCK frequency, unit: HZ */
> >-#define FSL_QSPI_DEFAULT_SCK_FREQ	50000000
> >-
> >-/* QSPI max chipselect signals number */
> >-#define FSL_QSPI_MAX_CHIPSELECT_NUM     4
> >-
> >-/* Controller needs driver to swap endian */
> >+/*
> >+ * The driver only uses one single LUT entry, that is updated on
> >+ * each call of exec_op(). Index 0 is preset at boot with a basic
> >+ * read operation, so let's use the last entry (15).
> >+ */
> >+#define	SEQID_LUT			15
> >+
> >+/* Registers used by the driver */
> >+#define QUADSPI_MCR			0x00
> >+#define QUADSPI_MCR_RESERVED_MASK	GENMASK(19, 16)
> >+#define QUADSPI_MCR_MDIS_MASK		BIT(14)
> >+#define QUADSPI_MCR_CLR_TXF_MASK	BIT(11)
> >+#define QUADSPI_MCR_CLR_RXF_MASK	BIT(10)
> >+#define QUADSPI_MCR_DDR_EN_MASK		BIT(7)
> >+#define QUADSPI_MCR_END_CFG_MASK	GENMASK(3, 2)
> >+#define QUADSPI_MCR_SWRSTHD_MASK	BIT(1)
> >+#define QUADSPI_MCR_SWRSTSD_MASK	BIT(0)
> >+
> >+#define QUADSPI_IPCR			0x08
> >+#define QUADSPI_IPCR_SEQID(x)		((x) << 24)
> >+#define QUADSPI_FLSHCR			0x0c
> >+#define QUADSPI_FLSHCR_TCSS_MASK	GENMASK(3, 0)
> >+#define QUADSPI_FLSHCR_TCSH_MASK	GENMASK(11, 8)
> >+#define QUADSPI_FLSHCR_TDH_MASK		GENMASK(17, 16)
> >+
> >+#define QUADSPI_BUF3CR			0x1c
> >+#define QUADSPI_BUF3CR_ALLMST_MASK	BIT(31)
> >+#define QUADSPI_BUF3CR_ADATSZ(x)	((x) << 8)
> >+#define QUADSPI_BUF3CR_ADATSZ_MASK	GENMASK(15, 8)
> >+
> >+#define QUADSPI_BFGENCR			0x20
> >+#define QUADSPI_BFGENCR_SEQID(x)	((x) << 12)
> >+
> >+#define QUADSPI_BUF0IND			0x30
> >+#define QUADSPI_BUF1IND			0x34
> >+#define QUADSPI_BUF2IND			0x38
> >+#define QUADSPI_SFAR			0x100
> >+
> >+#define QUADSPI_SMPR			0x108
> >+#define QUADSPI_SMPR_DDRSMP_MASK	GENMASK(18, 16)
> >+#define QUADSPI_SMPR_FSDLY_MASK		BIT(6)
> >+#define QUADSPI_SMPR_FSPHS_MASK		BIT(5)
> >+#define QUADSPI_SMPR_HSENA_MASK		BIT(0)
> >+
> >+#define QUADSPI_RBCT			0x110
> >+#define QUADSPI_RBCT_WMRK_MASK		GENMASK(4, 0)
> >+#define QUADSPI_RBCT_RXBRD_USEIPS	BIT(8)
> >+
> >+#define QUADSPI_TBDR			0x154
> >+
> >+#define QUADSPI_SR			0x15c
> >+#define QUADSPI_SR_IP_ACC_MASK		BIT(1)
> >+#define QUADSPI_SR_AHB_ACC_MASK		BIT(2)
> >+
> >+#define QUADSPI_FR			0x160
> >+#define QUADSPI_FR_TFF_MASK		BIT(0)
> >+
> >+#define QUADSPI_RSER			0x164
> >+#define QUADSPI_RSER_TFIE		BIT(0)
> >+
> >+#define QUADSPI_SPTRCLR			0x16c
> >+#define QUADSPI_SPTRCLR_IPPTRC		BIT(8)
> >+#define QUADSPI_SPTRCLR_BFPTRC		BIT(0)
> >+
> >+#define QUADSPI_SFA1AD			0x180
> >+#define QUADSPI_SFA2AD			0x184
> >+#define QUADSPI_SFB1AD			0x188
> >+#define QUADSPI_SFB2AD			0x18c
> >+#define QUADSPI_RBDR(x)			(0x200 + ((x) * 4))
> >+
> >+#define QUADSPI_LUTKEY			0x300
> >+#define QUADSPI_LUTKEY_VALUE		0x5AF05AF0
> >+
> >+#define QUADSPI_LCKCR			0x304
> >+#define QUADSPI_LCKER_LOCK		BIT(0)
> >+#define QUADSPI_LCKER_UNLOCK		BIT(1)
> >+
> >+#define QUADSPI_LUT_BASE		0x310
> >+#define QUADSPI_LUT_OFFSET		(SEQID_LUT * 4 * 4)
> >+#define QUADSPI_LUT_REG(idx) \
> >+	(QUADSPI_LUT_BASE + QUADSPI_LUT_OFFSET + (idx) * 4)
> >+
> >+/* Instruction set for the LUT register */
> >+#define LUT_STOP		0
> >+#define LUT_CMD			1
> >+#define LUT_ADDR		2
> >+#define LUT_DUMMY		3
> >+#define LUT_MODE		4
> >+#define LUT_MODE2		5
> >+#define LUT_MODE4		6
> >+#define LUT_FSL_READ		7
> >+#define LUT_FSL_WRITE		8
> >+#define LUT_JMP_ON_CS		9
> >+#define LUT_ADDR_DDR		10
> >+#define LUT_MODE_DDR		11
> >+#define LUT_MODE2_DDR		12
> >+#define LUT_MODE4_DDR		13
> >+#define LUT_FSL_READ_DDR	14
> >+#define LUT_FSL_WRITE_DDR	15
> >+#define LUT_DATA_LEARN		16
> >+
> >+/*
> >+ * The PAD definitions for LUT register.
> >+ *
> >+ * The pad stands for the number of IO lines [0:3].
> >+ * For example, the quad read needs four IO lines,
> >+ * so you should use LUT_PAD(4).
> >+ */
> >+#define LUT_PAD(x) (fls(x) - 1)
> >+
> >+/*
> >+ * Macro for constructing the LUT entries with the following
> >+ * register layout:
> >+ *
> >+ *  ---------------------------------------------------
> >+ *  | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
> >+ *  ---------------------------------------------------
> >+ */
> >+#define LUT_DEF(idx, ins, pad, opr)					\
> >+	((((ins) << 10) | ((pad) << 8) | (opr)) << (((idx) % 2) * 16))
> >+
> >+/* Controller needs driver to swap endianness */
> > #define QUADSPI_QUIRK_SWAP_ENDIAN	BIT(0)
> >
> >-enum fsl_qspi_devtype {
> >-	FSL_QUADSPI_VYBRID,
> >-	FSL_QUADSPI_IMX6SX,
> >-	FSL_QUADSPI_IMX6UL_7D,
> >-	FSL_QUADSPI_IMX7ULP,
> >-};
> >+/* Controller needs 4x internal clock */
> >+#define QUADSPI_QUIRK_4X_INT_CLK	BIT(1)
> >
> >-struct fsl_qspi_devtype_data {
> >-	enum fsl_qspi_devtype devtype;
> >-	u32 rxfifo;
> >-	u32 txfifo;
> >-	u32 ahb_buf_size;
> >-	u32 driver_data;
> >-};
> >+/*
> >+ * TKT253890, the controller needs the driver to fill the txfifo with
> >+ * 16 bytes at least to trigger a data transfer, even though the extra
> >+ * data won't be transferred.
> >+ */
> >+#define QUADSPI_QUIRK_TKT253890		BIT(2)
> >
> >-/**
> >- * struct fsl_qspi_platdata - platform data for Freescale QSPI
> >- *
> >- * @flags: Flags for QSPI QSPI_FLAG_...
> >- * @speed_hz: Default SCK frequency
> >- * @reg_base: Base address of QSPI registers
> >- * @amba_base: Base address of QSPI memory mapping
> >- * @amba_total_size: size of QSPI memory mapping
> >- * @flash_num: Number of active slave devices
> >- * @num_chipselect: Number of QSPI chipselect signals
> >+/* TKT245618, the controller cannot wake up from wait mode */
> >+#define QUADSPI_QUIRK_TKT245618		BIT(3)
> >+
> >+/*
> >+ * Controller adds QSPI_AMBA_BASE (base address of the mapped memory)
> >+ * internally. No need to add it when setting SFXXAD and SFAR
> >+registers
> >  */
> >-struct fsl_qspi_platdata {
> >-	u32 flags;
> >-	u32 speed_hz;
> >-	fdt_addr_t reg_base;
> >-	fdt_addr_t amba_base;
> >-	fdt_size_t amba_total_size;
> >-	u32 flash_num;
> >-	u32 num_chipselect;
> >-};
> >+#define QUADSPI_QUIRK_BASE_INTERNAL	BIT(4)
> >
> >-/**
> >- * struct fsl_qspi_priv - private data for Freescale QSPI
> >- *
> >- * @flags: Flags for QSPI QSPI_FLAG_...
> >- * @bus_clk: QSPI input clk frequency
> >- * @speed_hz: Default SCK frequency
> >- * @cur_seqid: current LUT table sequence id
> >- * @sf_addr: flash access offset
> >- * @amba_base: Base address of QSPI memory mapping of every CS
> >- * @amba_total_size: size of QSPI memory mapping
> >- * @cur_amba_base: Base address of QSPI memory mapping of current CS
> >- * @flash_num: Number of active slave devices
> >- * @num_chipselect: Number of QSPI chipselect signals
> >- * @regs: Point to QSPI register structure for I/O access
> >+/*
> >+ * Controller uses TDH bits in register QUADSPI_FLSHCR.
> >+ * They need to be set in accordance with the DDR/SDR mode.
> >  */
> >-struct fsl_qspi_priv {
> >-	u32 flags;
> >-	u32 bus_clk;
> >-	u32 speed_hz;
> >-	u32 cur_seqid;
> >-	u32 sf_addr;
> >-	u32 amba_base[FSL_QSPI_MAX_CHIPSELECT_NUM];
> >-	u32 amba_total_size;
> >-	u32 cur_amba_base;
> >-	u32 flash_num;
> >-	u32 num_chipselect;
> >-	struct fsl_qspi_regs *regs;
> >-	struct fsl_qspi_devtype_data *devtype_data;
> >+#define QUADSPI_QUIRK_USE_TDH_SETTING	BIT(5)
> >+
> >+struct fsl_qspi_devtype_data {
> >+	unsigned int rxfifo;
> >+	unsigned int txfifo;
> >+	unsigned int ahb_buf_size;
> >+	unsigned int quirks;
> >+	bool little_endian;
> > };
> >
> > static const struct fsl_qspi_devtype_data vybrid_data = {
> >-	.devtype = FSL_QUADSPI_VYBRID,
> >-	.rxfifo = 128,
> >-	.txfifo = 64,
> >-	.ahb_buf_size = 1024,
> >-	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
> >+	.rxfifo = SZ_128,
> >+	.txfifo = SZ_64,
> >+	.ahb_buf_size = SZ_1K,
> >+	.quirks = QUADSPI_QUIRK_SWAP_ENDIAN,
> >+	.little_endian = true,
> > };
> >
> > static const struct fsl_qspi_devtype_data imx6sx_data = {
> >-	.devtype = FSL_QUADSPI_IMX6SX,
> >-	.rxfifo = 128,
> >-	.txfifo = 512,
> >-	.ahb_buf_size = 1024,
> >-	.driver_data = 0,
> >+	.rxfifo = SZ_128,
> >+	.txfifo = SZ_512,
> >+	.ahb_buf_size = SZ_1K,
> >+	.quirks = QUADSPI_QUIRK_4X_INT_CLK | QUADSPI_QUIRK_TKT245618,
> >+	.little_endian = true,
> >+};
> >+
> >+static const struct fsl_qspi_devtype_data imx7d_data = {
> >+	.rxfifo = SZ_128,
> >+	.txfifo = SZ_512,
> >+	.ahb_buf_size = SZ_1K,
> >+	.quirks = QUADSPI_QUIRK_TKT253890 | QUADSPI_QUIRK_4X_INT_CLK
> |
> >+		  QUADSPI_QUIRK_USE_TDH_SETTING,
> >+	.little_endian = true,
> >+};
> >+
> >+static const struct fsl_qspi_devtype_data imx6ul_data = {
> >+	.rxfifo = SZ_128,
> >+	.txfifo = SZ_512,
> >+	.ahb_buf_size = SZ_1K,
> >+	.quirks = QUADSPI_QUIRK_TKT253890 | QUADSPI_QUIRK_4X_INT_CLK
> |
> >+		  QUADSPI_QUIRK_USE_TDH_SETTING,
> >+	.little_endian = true,
> > };
> >
> >-static const struct fsl_qspi_devtype_data imx6ul_7d_data = {
> >-	.devtype = FSL_QUADSPI_IMX6UL_7D,
> >-	.rxfifo = 128,
> >-	.txfifo = 512,
> >-	.ahb_buf_size = 1024,
> >-	.driver_data = 0,
> >+static const struct fsl_qspi_devtype_data ls1021a_data = {
> >+	.rxfifo = SZ_128,
> >+	.txfifo = SZ_64,
> >+	.ahb_buf_size = SZ_1K,
> >+	.quirks = 0,
> >+	.little_endian = false,
> > };
> >
> >-static const struct fsl_qspi_devtype_data imx7ulp_data = {
> >-	.devtype = FSL_QUADSPI_IMX7ULP,
> >-	.rxfifo = 64,
> >-	.txfifo = 64,
> >-	.ahb_buf_size = 128,
> >-	.driver_data = 0,
> >+static const struct fsl_qspi_devtype_data ls1088a_data = {
> >+	.rxfifo = SZ_128,
> >+	.txfifo = SZ_128,
> >+	.ahb_buf_size = SZ_1K,
> >+	.quirks = QUADSPI_QUIRK_TKT253890,
> >+	.little_endian = true,
> > };
> >
> >-static u32 qspi_read32(u32 flags, u32 *addr)
> >+static const struct fsl_qspi_devtype_data ls2080a_data = {
> >+	.rxfifo = SZ_128,
> >+	.txfifo = SZ_64,
> >+	.ahb_buf_size = SZ_1K,
> >+	.quirks = QUADSPI_QUIRK_TKT253890 |
> >QUADSPI_QUIRK_BASE_INTERNAL,
> >+	.little_endian = true,
> >+};
> >+
> >+struct fsl_qspi {
> >+	void __iomem *iobase;
> >+	void __iomem *ahb_addr;
> >+	u32 memmap_phy;
> >+	const struct fsl_qspi_devtype_data *devtype_data;
> >+	int selected;
> >+};
> >+
> It would be better to move all declarations to header file.

Current Linux driver has declarations in the same file and that's why the same idea was applied to u-boot as well.

Thanks
Kuldeep

> <snip>
> -Priyanka