diff mbox series

[2/2] spi: fsl_qspi: Support to use full AHB space on i.MX

Message ID 1591689546-64136-2-git-send-email-ye.li@nxp.com
State Accepted
Commit def88bce094e95939abf1e5d02a9b413b3627f6f
Headers show
Series [1/2] spi: fsl_qspi: Add support for i.MX7ULP | expand

Commit Message

Ye Li June 9, 2020, 7:59 a.m. UTC 
i.MX platforms provide large AHB mapped space for QSPI, each
controller has 256MB. However, current driver only maps small
size (AHB buffer size) of AHB space, this implementation
causes i.MX failed to boot M4 with QSPI XIP image.

Add config CONFIG_FSL_QSPI_AHB_FULL_MAP (default enabled for i.MX)
to address above problem.

When the config is set:
1. Full AHB space is divided to each CS.
2. A dedicated LUT entry is used for AHB read only.
3. The MODE instruction in LUT is replaced to standard ADDR instruction
4. The address in spi_mem_op is used to SFAR and AHB read

Signed-off-by: Ye Li <ye.li at nxp.com>
Reviewed-by: Ashish Kumar <Ashish.Kumar at nxp.com>
Reviewed-by: Kuldeep Singh <kuldeep.singh at nxp.com>
---
 drivers/spi/Kconfig    |   8 ++++
 drivers/spi/fsl_qspi.c | 114 ++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 98 insertions(+), 24 deletions(-)

Comments

Frieder Schrempf Nov. 9, 2022, 9:43 a.m. UTC | #1 
Hi,

On 09.06.20 09:59, Ye Li wrote:
> i.MX platforms provide large AHB mapped space for QSPI, each
> controller has 256MB. However, current driver only maps small
> size (AHB buffer size) of AHB space, this implementation
> causes i.MX failed to boot M4 with QSPI XIP image.
> 
> Add config CONFIG_FSL_QSPI_AHB_FULL_MAP (default enabled for i.MX)
> to address above problem.
> 
> When the config is set:
> 1. Full AHB space is divided to each CS.
> 2. A dedicated LUT entry is used for AHB read only.
> 3. The MODE instruction in LUT is replaced to standard ADDR instruction
> 4. The address in spi_mem_op is used to SFAR and AHB read
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Reviewed-by: Ashish Kumar <Ashish.Kumar@nxp.com>
> Reviewed-by: Kuldeep Singh <kuldeep.singh@nxp.com>

I know I'm very late, but I seem to have missed this back in the days
when it was posted. Unfortunately there are some problems with this change.

1. The way the memory mapping of the attached flash (QSPI NOR?) is
implemented here is messy. In Linux we have a dirmap API for this kind
of things and it has recently been ported to U-Boot [1][2]. It would be
better to use this, similar to what was done here [3].

2. This change breaks reading from the SPI NAND flash used on the
Kontron SL i.MX6UL/ULL boards. The read operation returns garbage
instead of the actual data on the flash. When I revert this commit or set

	# CONFIG_FSL_QSPI_AHB_FULL_MAP is not set

in the defconfig, it works.

Ye, can you come up with a solution for point 1 or if not can we change
the default of CONFIG_FSL_QSPI_AHB_FULL_MAP from enabled to disabled,
please?

For now I will disable CONFIG_FSL_QSPI_AHB_FULL_MAP in the boards
defconfig, but we should fix this globally.

What do you think?

Thanks
Frieder

[1]
https://source.denx.de/u-boot/u-boot/-/commit/f7e1de4c6a43beec438bce04571469145086efed
[2]
https://source.denx.de/u-boot/u-boot/-/commit/463cdf66632a0d67bf824cb43b6c1b41782d0765
[3]
https://source.denx.de/u-boot/u-boot/-/commit/992d02ea737895dc67246bbf7065084bb2721d6b
diff mbox series

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 5941520..dc8a958 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -149,6 +149,14 @@  config FSL_QSPI
 	  used to access the SPI NOR flash on platforms embedding this
 	  Freescale IP core.
 
+config FSL_QSPI_AHB_FULL_MAP
+	bool "Use full AHB memory map space"
+	depends on FSL_QSPI
+	default y if ARCH_MX6 || ARCH_MX7 || ARCH_MX7ULP || ARCH_IMX8M
+	help
+	  Enable the Freescale QSPI driver to use full AHB memory map space for
+	  flash access.
+
 config ICH_SPI
 	bool "Intel ICH SPI driver"
 	help
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index d9f531a..eec968e 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -46,6 +46,7 @@  DECLARE_GLOBAL_DATA_PTR;
  * read operation, so let's use the last entry (15).
  */
 #define	SEQID_LUT			15
+#define	SEQID_LUT_AHB		14
 
 /* Registers used by the driver */
 #define QUADSPI_MCR			0x00
@@ -122,6 +123,10 @@  DECLARE_GLOBAL_DATA_PTR;
 #define QUADSPI_LUT_REG(idx) \
 	(QUADSPI_LUT_BASE + QUADSPI_LUT_OFFSET + (idx) * 4)
 
+#define QUADSPI_AHB_LUT_OFFSET		(SEQID_LUT_AHB * 4 * 4)
+#define QUADSPI_AHB_LUT_REG(idx) \
+	(QUADSPI_LUT_BASE + QUADSPI_AHB_LUT_OFFSET + (idx) * 4)
+
 /* Instruction set for the LUT register */
 #define LUT_STOP		0
 #define LUT_CMD			1
@@ -189,6 +194,11 @@  DECLARE_GLOBAL_DATA_PTR;
  */
 #define QUADSPI_QUIRK_USE_TDH_SETTING	BIT(5)
 
+/*
+ * Controller only has Two CS on flash A, no flash B port
+ */
+#define QUADSPI_QUIRK_SINGLE_BUS		BIT(6)
+
 struct fsl_qspi_devtype_data {
 	unsigned int rxfifo;
 	unsigned int txfifo;
@@ -236,7 +246,7 @@  static const struct fsl_qspi_devtype_data imx7ulp_data = {
 	.txfifo = SZ_64,
 	.ahb_buf_size = SZ_128,
 	.quirks = QUADSPI_QUIRK_TKT253890 | QUADSPI_QUIRK_4X_INT_CLK |
-		  QUADSPI_QUIRK_USE_TDH_SETTING,
+		  QUADSPI_QUIRK_USE_TDH_SETTING | QUADSPI_QUIRK_SINGLE_BUS,
 	.little_endian = true,
 };
 
@@ -269,6 +279,7 @@  struct fsl_qspi {
 	void __iomem *iobase;
 	void __iomem *ahb_addr;
 	u32 memmap_phy;
+	u32 memmap_size;
 	const struct fsl_qspi_devtype_data *devtype_data;
 	int selected;
 };
@@ -303,6 +314,11 @@  static inline int needs_tdh_setting(struct fsl_qspi *q)
 	return q->devtype_data->quirks & QUADSPI_QUIRK_USE_TDH_SETTING;
 }
 
+static inline int needs_single_bus(struct fsl_qspi *q)
+{
+	return q->devtype_data->quirks & QUADSPI_QUIRK_SINGLE_BUS;
+}
+
 /*
  * An IC bug makes it necessary to rearrange the 32-bit data.
  * Later chips, such as IMX6SLX, have fixed this bug.
@@ -405,18 +421,27 @@  static void fsl_qspi_prepare_lut(struct fsl_qspi *q,
 	lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
 			     op->cmd.opcode);
 
-	/*
-	 * For some unknown reason, using LUT_ADDR doesn't work in some
-	 * cases (at least with only one byte long addresses), so
-	 * let's use LUT_MODE to write the address bytes one by one
-	 */
-	for (i = 0; i < op->addr.nbytes; i++) {
-		u8 addrbyte = op->addr.val >> (8 * (op->addr.nbytes - i - 1));
-
-		lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_MODE,
-					      LUT_PAD(op->addr.buswidth),
-					      addrbyte);
-		lutidx++;
+	if (IS_ENABLED(CONFIG_FSL_QSPI_AHB_FULL_MAP)) {
+		if (op->addr.nbytes) {
+			lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_ADDR,
+						      LUT_PAD(op->addr.buswidth),
+						      (op->addr.nbytes == 4) ? 0x20 : 0x18);
+			lutidx++;
+		}
+	} else {
+		/*
+		 * For some unknown reason, using LUT_ADDR doesn't work in some
+		 * cases (at least with only one byte long addresses), so
+		 * let's use LUT_MODE to write the address bytes one by one
+		 */
+		for (i = 0; i < op->addr.nbytes; i++) {
+			u8 addrbyte = op->addr.val >> (8 * (op->addr.nbytes - i - 1));
+
+			lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_MODE,
+						      LUT_PAD(op->addr.buswidth),
+						      addrbyte);
+			lutidx++;
+		}
 	}
 
 	if (op->dummy.nbytes) {
@@ -449,6 +474,14 @@  static void fsl_qspi_prepare_lut(struct fsl_qspi *q,
 	for (i = 0; i < ARRAY_SIZE(lutval); i++)
 		qspi_writel(q, lutval[i], base + QUADSPI_LUT_REG(i));
 
+	if (IS_ENABLED(CONFIG_FSL_QSPI_AHB_FULL_MAP)) {
+		if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_IN &&
+		    op->addr.nbytes) {
+			for (i = 0; i < ARRAY_SIZE(lutval); i++)
+				qspi_writel(q, lutval[i], base + QUADSPI_AHB_LUT_REG(i));
+		}
+	}
+
 	/* lock LUT */
 	qspi_writel(q, QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
 	qspi_writel(q, QUADSPI_LCKER_LOCK, q->iobase + QUADSPI_LCKCR);
@@ -491,10 +524,29 @@  static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_slave *slave)
 	fsl_qspi_invalidate(q);
 }
 
+static u32 fsl_qspi_memsize_per_cs(struct fsl_qspi *q)
+{
+	if (IS_ENABLED(CONFIG_FSL_QSPI_AHB_FULL_MAP)) {
+		if (needs_single_bus(q))
+			return q->memmap_size / 2;
+		else
+			return q->memmap_size / 4;
+	} else {
+		return ALIGN(q->devtype_data->ahb_buf_size, 0x400);
+	}
+}
+
 static void fsl_qspi_read_ahb(struct fsl_qspi *q, const struct spi_mem_op *op)
 {
+	void __iomem *ahb_read_addr = q->ahb_addr;
+
+	if (IS_ENABLED(CONFIG_FSL_QSPI_AHB_FULL_MAP)) {
+		if (op->addr.nbytes)
+			ahb_read_addr += op->addr.val;
+	}
+
 	memcpy_fromio(op->data.buf.in,
-		      q->ahb_addr + q->selected * ALIGN(q->devtype_data->ahb_buf_size, 0x400),
+		      ahb_read_addr + q->selected * fsl_qspi_memsize_per_cs(q),
 		      op->data.nbytes);
 }
 
@@ -597,8 +649,13 @@  static int fsl_qspi_exec_op(struct spi_slave *slave,
 	if (needs_amba_base_offset(q))
 		addr_offset = q->memmap_phy;
 
+	if (IS_ENABLED(CONFIG_FSL_QSPI_AHB_FULL_MAP)) {
+		if (op->addr.nbytes)
+			addr_offset += op->addr.val;
+	}
+
 	qspi_writel(q,
-		    q->selected * ALIGN(q->devtype_data->ahb_buf_size, 0x400) + addr_offset,
+		    q->selected * fsl_qspi_memsize_per_cs(q) + addr_offset,
 		    base + QUADSPI_SFAR);
 
 	qspi_writel(q, qspi_readl(q, base + QUADSPI_MCR) |
@@ -655,7 +712,7 @@  static int fsl_qspi_adjust_op_size(struct spi_slave *slave,
 static int fsl_qspi_default_setup(struct fsl_qspi *q)
 {
 	void __iomem *base = q->iobase;
-	u32 reg, addr_offset = 0;
+	u32 reg, addr_offset = 0, memsize_cs;
 
 	/* Reset the module */
 	qspi_writel(q, QUADSPI_MCR_SWRSTSD_MASK | QUADSPI_MCR_SWRSTHD_MASK,
@@ -687,8 +744,13 @@  static int fsl_qspi_default_setup(struct fsl_qspi *q)
 	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
 	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
 
-	qspi_writel(q, QUADSPI_BFGENCR_SEQID(SEQID_LUT),
-		    q->iobase + QUADSPI_BFGENCR);
+	if (IS_ENABLED(CONFIG_FSL_QSPI_AHB_FULL_MAP))
+		qspi_writel(q, QUADSPI_BFGENCR_SEQID(SEQID_LUT_AHB),
+			    q->iobase + QUADSPI_BFGENCR);
+	else
+		qspi_writel(q, QUADSPI_BFGENCR_SEQID(SEQID_LUT),
+			    q->iobase + QUADSPI_BFGENCR);
+
 	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, base + QUADSPI_RBCT);
 	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
 		    QUADSPI_BUF3CR_ADATSZ(q->devtype_data->ahb_buf_size / 8),
@@ -704,14 +766,17 @@  static int fsl_qspi_default_setup(struct fsl_qspi *q)
 	 * We use ahb_buf_size for each chip and set SFA1AD, SFA2AD, SFB1AD,
 	 * SFB2AD accordingly.
 	 */
-	qspi_writel(q, ALIGN(q->devtype_data->ahb_buf_size, 0x400) + addr_offset,
+	memsize_cs = fsl_qspi_memsize_per_cs(q);
+	qspi_writel(q, memsize_cs + addr_offset,
 		    base + QUADSPI_SFA1AD);
-	qspi_writel(q, ALIGN(q->devtype_data->ahb_buf_size, 0x400) * 2 + addr_offset,
+	qspi_writel(q, memsize_cs * 2 + addr_offset,
 		    base + QUADSPI_SFA2AD);
-	qspi_writel(q, ALIGN(q->devtype_data->ahb_buf_size, 0x400) * 3 + addr_offset,
-		    base + QUADSPI_SFB1AD);
-	qspi_writel(q, ALIGN(q->devtype_data->ahb_buf_size, 0x400) * 4 + addr_offset,
-		    base + QUADSPI_SFB2AD);
+	if (!needs_single_bus(q)) {
+		qspi_writel(q, memsize_cs * 3 + addr_offset,
+			    base + QUADSPI_SFB1AD);
+		qspi_writel(q, memsize_cs * 4 + addr_offset,
+			    base + QUADSPI_SFB2AD);
+	}
 
 	q->selected = -1;
 
@@ -759,6 +824,7 @@  static int fsl_qspi_probe(struct udevice *bus)
 
 	q->ahb_addr = map_physmem(res.start, res.end - res.start, MAP_NOCACHE);
 	q->memmap_phy = res.start;
+	q->memmap_size = res.end - res.start;
 
 	dm_bus->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
 					66000000);