[PATCH 0/2] CQSPI: Add direct mode support

[PATCH 0/2] CQSPI: Add direct mode support

[Vignesh R]

This patch series enables use Direct access controller on Cadence QSPI
which helps in accessing QSPI flash in memory mapped mode.

On TI platforms, this mode has higher throughput compared to indirect
access mode.

Tested on TI's 66AK2G GP EVM.

It would be great if this patch series could be tested SoCFPGA as well.
Although, this patch should have no effect on SoCFPGA platforms as driver
continues to use indirect mode when direct access memory window is less
than size of connected flash.

Vignesh R (2):
  mtd: spi-nor: cadence-quadspi: Refactor indirect read/write sequence.
  mtd: spi-nor: cadence-quadspi: Add support for direct access mode

 drivers/mtd/spi-nor/cadence-quadspi.c | 75 ++++++++++++++++++++++++++++-------
 1 file changed, 60 insertions(+), 15 deletions(-)

-- 
2.15.1

[PATCH 0/2] CQSPI: Add direct mode support

[Vignesh R]

This patch series enables use Direct access controller on Cadence QSPI
which helps in accessing QSPI flash in memory mapped mode.

On TI platforms, this mode has higher throughput compared to indirect
access mode.

Tested on TI's 66AK2G GP EVM.

It would be great if this patch series could be tested SoCFPGA as well.
Although, this patch should have no effect on SoCFPGA platforms as driver
continues to use indirect mode when direct access memory window is less
than size of connected flash.

Vignesh R (2):
  mtd: spi-nor: cadence-quadspi: Refactor indirect read/write sequence.
  mtd: spi-nor: cadence-quadspi: Add support for direct access mode

 drivers/mtd/spi-nor/cadence-quadspi.c | 75 ++++++++++++++++++++++++++++-------
 1 file changed, 60 insertions(+), 15 deletions(-)

-- 
2.15.1

[PATCH 1/2] mtd: spi-nor: cadence-quadspi: Refactor indirect read/write sequence.

[Vignesh R]

Move configuring of indirect read/write start address to
cqspi_indirect_*_execute() function and rename cqspi_indirect_*_setup()
function. This will help to reuse cqspi_indirect_*_setup() function for
supporting direct access mode.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 75a2bc447a99..becc7d714ab8 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -450,8 +450,7 @@ static int cqspi_command_write_addr(struct spi_nor *nor,
 	return cqspi_exec_flash_cmd(cqspi, reg);
 }
 
-static int cqspi_indirect_read_setup(struct spi_nor *nor,
-				     const unsigned int from_addr)
+static int cqspi_read_setup(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
@@ -459,7 +458,6 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
 	unsigned int dummy_clk = 0;
 	unsigned int reg;
 
-	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
 
 	reg = nor->read_opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
 	reg |= cqspi_calc_rdreg(nor, nor->read_opcode);
@@ -493,8 +491,8 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
 	return 0;
 }
 
-static int cqspi_indirect_read_execute(struct spi_nor *nor,
-				       u8 *rxbuf, const unsigned n_rx)
+static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
+				       loff_t from_addr, const size_t n_rx)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
@@ -504,6 +502,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
 	unsigned int bytes_to_read = 0;
 	int ret = 0;
 
+	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
 	writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
 
 	/* Clear all interrupts. */
@@ -570,8 +569,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
 	return ret;
 }
 
-static int cqspi_indirect_write_setup(struct spi_nor *nor,
-				      const unsigned int to_addr)
+static int cqspi_write_setup(struct spi_nor *nor)
 {
 	unsigned int reg;
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
@@ -584,8 +582,6 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
 	reg = cqspi_calc_rdreg(nor, nor->program_opcode);
 	writel(reg, reg_base + CQSPI_REG_RD_INSTR);
 
-	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
-
 	reg = readl(reg_base + CQSPI_REG_SIZE);
 	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
 	reg |= (nor->addr_width - 1);
@@ -593,8 +589,8 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
 	return 0;
 }
 
-static int cqspi_indirect_write_execute(struct spi_nor *nor,
-					const u8 *txbuf, const unsigned n_tx)
+static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
+					const u8 *txbuf, const size_t n_tx)
 {
 	const unsigned int page_size = nor->page_size;
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
@@ -604,6 +600,7 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
 	unsigned int write_bytes;
 	int ret;
 
+	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
 	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
 
 	/* Clear all interrupts. */
@@ -900,11 +897,11 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_write_setup(nor, to);
+	ret = cqspi_write_setup(nor);
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_write_execute(nor, buf, len);
+	ret = cqspi_indirect_write_execute(nor, to, buf, len);
 	if (ret)
 		return ret;
 
@@ -920,11 +917,11 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_read_setup(nor, from);
+	ret = cqspi_read_setup(nor);
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_read_execute(nor, buf, len);
+	ret = cqspi_indirect_read_execute(nor, buf, from, len);
 	if (ret)
 		return ret;
 
-- 
2.15.1

[PATCH 1/2] mtd: spi-nor: cadence-quadspi: Refactor indirect read/write sequence.

[Vignesh R]

Move configuring of indirect read/write start address to
cqspi_indirect_*_execute() function and rename cqspi_indirect_*_setup()
function. This will help to reuse cqspi_indirect_*_setup() function for
supporting direct access mode.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 75a2bc447a99..becc7d714ab8 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -450,8 +450,7 @@ static int cqspi_command_write_addr(struct spi_nor *nor,
 	return cqspi_exec_flash_cmd(cqspi, reg);
 }
 
-static int cqspi_indirect_read_setup(struct spi_nor *nor,
-				     const unsigned int from_addr)
+static int cqspi_read_setup(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
@@ -459,7 +458,6 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
 	unsigned int dummy_clk = 0;
 	unsigned int reg;
 
-	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
 
 	reg = nor->read_opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
 	reg |= cqspi_calc_rdreg(nor, nor->read_opcode);
@@ -493,8 +491,8 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
 	return 0;
 }
 
-static int cqspi_indirect_read_execute(struct spi_nor *nor,
-				       u8 *rxbuf, const unsigned n_rx)
+static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
+				       loff_t from_addr, const size_t n_rx)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
@@ -504,6 +502,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
 	unsigned int bytes_to_read = 0;
 	int ret = 0;
 
+	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
 	writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
 
 	/* Clear all interrupts. */
@@ -570,8 +569,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
 	return ret;
 }
 
-static int cqspi_indirect_write_setup(struct spi_nor *nor,
-				      const unsigned int to_addr)
+static int cqspi_write_setup(struct spi_nor *nor)
 {
 	unsigned int reg;
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
@@ -584,8 +582,6 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
 	reg = cqspi_calc_rdreg(nor, nor->program_opcode);
 	writel(reg, reg_base + CQSPI_REG_RD_INSTR);
 
-	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
-
 	reg = readl(reg_base + CQSPI_REG_SIZE);
 	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
 	reg |= (nor->addr_width - 1);
@@ -593,8 +589,8 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
 	return 0;
 }
 
-static int cqspi_indirect_write_execute(struct spi_nor *nor,
-					const u8 *txbuf, const unsigned n_tx)
+static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
+					const u8 *txbuf, const size_t n_tx)
 {
 	const unsigned int page_size = nor->page_size;
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
@@ -604,6 +600,7 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
 	unsigned int write_bytes;
 	int ret;
 
+	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
 	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
 
 	/* Clear all interrupts. */
@@ -900,11 +897,11 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_write_setup(nor, to);
+	ret = cqspi_write_setup(nor);
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_write_execute(nor, buf, len);
+	ret = cqspi_indirect_write_execute(nor, to, buf, len);
 	if (ret)
 		return ret;
 
@@ -920,11 +917,11 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_read_setup(nor, from);
+	ret = cqspi_read_setup(nor);
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_read_execute(nor, buf, len);
+	ret = cqspi_indirect_read_execute(nor, buf, from, len);
 	if (ret)
 		return ret;
 
-- 
2.15.1

[PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Vignesh R]

Cadence QSPI controller provides direct access mode through which flash
can be accessed in a memory-mapped IO mode. This enables read/write to
flash using memcpy*() functions. This mode provides higher throughput
for both read/write operations when compared to current indirect mode of
operation.

This patch therefore adds support to use QSPI in direct mode. If the
window reserved in SoC's memory map for MMIO access is less that of
flash size(like on most SoCFPGA variants), then the driver falls back
to indirect mode of operation.

On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
switching to direct mode improves read throughput from 3MB/s to 8MB/s.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 52 +++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index becc7d714ab8..f8721ed68bc6 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -58,6 +58,7 @@ struct cqspi_flash_pdata {
 	u8		data_width;
 	u8		cs;
 	bool		registered;
+	bool		use_direct_mode;
 };
 
 struct cqspi_st {
@@ -68,6 +69,7 @@ struct cqspi_st {
 
 	void __iomem		*iobase;
 	void __iomem		*ahb_base;
+	resource_size_t		ahb_size;
 	struct completion	transfer_complete;
 	struct mutex		bus_mutex;
 
@@ -103,6 +105,7 @@ struct cqspi_st {
 /* Register map */
 #define CQSPI_REG_CONFIG			0x00
 #define CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL	BIT(7)
 #define CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
 #define CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
 #define CQSPI_REG_CONFIG_DMA_MASK		BIT(15)
@@ -569,6 +572,21 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
 	return ret;
 }
 
+static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
+				     loff_t from_addr, const size_t len)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	u32 reg;
+
+	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
+	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
+	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
+	memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
+
+	return 0;
+}
+
 static int cqspi_write_setup(struct spi_nor *nor)
 {
 	unsigned int reg;
@@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 	return ret;
 }
 
+static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
+				      const u8 *txbuf, const size_t len)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	u32 reg;
+
+	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
+	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
+	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
+	memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
+
+	return 0;
+}
+
 static void cqspi_chipselect(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
@@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
 static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 			   size_t len, const u_char *buf)
 {
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	int ret;
 
 	ret = cqspi_set_protocol(nor, 0);
@@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_write_execute(nor, to, buf, len);
+	if (f_pdata->use_direct_mode)
+		ret = cqspi_direct_write_execute(nor, to, buf, len);
+	else
+		ret = cqspi_indirect_write_execute(nor, to, buf, len);
 	if (ret)
 		return ret;
 
@@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
 			  size_t len, u_char *buf)
 {
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	int ret;
 
 	ret = cqspi_set_protocol(nor, 1);
@@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_read_execute(nor, buf, from, len);
+	if (f_pdata->use_direct_mode)
+		ret = cqspi_direct_read_execute(nor, buf, from, len);
+	else
+		ret = cqspi_indirect_read_execute(nor, buf, from, len);
 	if (ret)
 		return ret;
 
@@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
 			goto err;
 
 		f_pdata->registered = true;
+
+		if (mtd->size <= cqspi->ahb_size) {
+			f_pdata->use_direct_mode = true;
+			dev_info(nor->dev, "using direct mode for %s\n",
+				 mtd->name);
+		}
 	}
 
 	return 0;
@@ -1212,6 +1259,7 @@ static int cqspi_probe(struct platform_device *pdev)
 		dev_err(dev, "Cannot remap AHB address.\n");
 		return PTR_ERR(cqspi->ahb_base);
 	}
+	cqspi->ahb_size = resource_size(res_ahb);
 
 	init_completion(&cqspi->transfer_complete);
 
-- 
2.15.1

[PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Vignesh R]

Cadence QSPI controller provides direct access mode through which flash
can be accessed in a memory-mapped IO mode. This enables read/write to
flash using memcpy*() functions. This mode provides higher throughput
for both read/write operations when compared to current indirect mode of
operation.

This patch therefore adds support to use QSPI in direct mode. If the
window reserved in SoC's memory map for MMIO access is less that of
flash size(like on most SoCFPGA variants), then the driver falls back
to indirect mode of operation.

On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
switching to direct mode improves read throughput from 3MB/s to 8MB/s.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 52 +++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index becc7d714ab8..f8721ed68bc6 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -58,6 +58,7 @@ struct cqspi_flash_pdata {
 	u8		data_width;
 	u8		cs;
 	bool		registered;
+	bool		use_direct_mode;
 };
 
 struct cqspi_st {
@@ -68,6 +69,7 @@ struct cqspi_st {
 
 	void __iomem		*iobase;
 	void __iomem		*ahb_base;
+	resource_size_t		ahb_size;
 	struct completion	transfer_complete;
 	struct mutex		bus_mutex;
 
@@ -103,6 +105,7 @@ struct cqspi_st {
 /* Register map */
 #define CQSPI_REG_CONFIG			0x00
 #define CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL	BIT(7)
 #define CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
 #define CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
 #define CQSPI_REG_CONFIG_DMA_MASK		BIT(15)
@@ -569,6 +572,21 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
 	return ret;
 }
 
+static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
+				     loff_t from_addr, const size_t len)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	u32 reg;
+
+	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
+	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
+	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
+	memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
+
+	return 0;
+}
+
 static int cqspi_write_setup(struct spi_nor *nor)
 {
 	unsigned int reg;
@@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 	return ret;
 }
 
+static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
+				      const u8 *txbuf, const size_t len)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	u32 reg;
+
+	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
+	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
+	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
+	memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
+
+	return 0;
+}
+
 static void cqspi_chipselect(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
@@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
 static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 			   size_t len, const u_char *buf)
 {
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	int ret;
 
 	ret = cqspi_set_protocol(nor, 0);
@@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_write_execute(nor, to, buf, len);
+	if (f_pdata->use_direct_mode)
+		ret = cqspi_direct_write_execute(nor, to, buf, len);
+	else
+		ret = cqspi_indirect_write_execute(nor, to, buf, len);
 	if (ret)
 		return ret;
 
@@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
 static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
 			  size_t len, u_char *buf)
 {
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	int ret;
 
 	ret = cqspi_set_protocol(nor, 1);
@@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
 	if (ret)
 		return ret;
 
-	ret = cqspi_indirect_read_execute(nor, buf, from, len);
+	if (f_pdata->use_direct_mode)
+		ret = cqspi_direct_read_execute(nor, buf, from, len);
+	else
+		ret = cqspi_indirect_read_execute(nor, buf, from, len);
 	if (ret)
 		return ret;
 
@@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
 			goto err;
 
 		f_pdata->registered = true;
+
+		if (mtd->size <= cqspi->ahb_size) {
+			f_pdata->use_direct_mode = true;
+			dev_info(nor->dev, "using direct mode for %s\n",
+				 mtd->name);
+		}
 	}
 
 	return 0;
@@ -1212,6 +1259,7 @@ static int cqspi_probe(struct platform_device *pdev)
 		dev_err(dev, "Cannot remap AHB address.\n");
 		return PTR_ERR(cqspi->ahb_base);
 	}
+	cqspi->ahb_size = resource_size(res_ahb);
 
 	init_completion(&cqspi->transfer_complete);
 
-- 
2.15.1

Re: [PATCH 1/2] mtd: spi-nor: cadence-quadspi: Refactor indirect read/write sequence.

[Cyrille Pitchen]

Hi Vignesh,

Le 07/12/2017 à 07:38, Vignesh R a écrit :
> Move configuring of indirect read/write start address to
> cqspi_indirect_*_execute() function and rename cqspi_indirect_*_setup()
> function. This will help to reuse cqspi_indirect_*_setup() function for
> supporting direct access mode.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>

This patch looks good to me.

Best regards,

Cyrille

> ---
>  drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index 75a2bc447a99..becc7d714ab8 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -450,8 +450,7 @@ static int cqspi_command_write_addr(struct spi_nor *nor,
>  	return cqspi_exec_flash_cmd(cqspi, reg);
>  }
>  
> -static int cqspi_indirect_read_setup(struct spi_nor *nor,
> -				     const unsigned int from_addr)
> +static int cqspi_read_setup(struct spi_nor *nor)
>  {
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
> @@ -459,7 +458,6 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
>  	unsigned int dummy_clk = 0;
>  	unsigned int reg;
>  
> -	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
>  
>  	reg = nor->read_opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
>  	reg |= cqspi_calc_rdreg(nor, nor->read_opcode);
> @@ -493,8 +491,8 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
>  	return 0;
>  }
>  
> -static int cqspi_indirect_read_execute(struct spi_nor *nor,
> -				       u8 *rxbuf, const unsigned n_rx)
> +static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
> +				       loff_t from_addr, const size_t n_rx)
>  {
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
> @@ -504,6 +502,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
>  	unsigned int bytes_to_read = 0;
>  	int ret = 0;
>  
> +	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
>  	writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
>  
>  	/* Clear all interrupts. */
> @@ -570,8 +569,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
>  	return ret;
>  }
>  
> -static int cqspi_indirect_write_setup(struct spi_nor *nor,
> -				      const unsigned int to_addr)
> +static int cqspi_write_setup(struct spi_nor *nor)
>  {
>  	unsigned int reg;
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
> @@ -584,8 +582,6 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
>  	reg = cqspi_calc_rdreg(nor, nor->program_opcode);
>  	writel(reg, reg_base + CQSPI_REG_RD_INSTR);
>  
> -	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
> -
>  	reg = readl(reg_base + CQSPI_REG_SIZE);
>  	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
>  	reg |= (nor->addr_width - 1);
> @@ -593,8 +589,8 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
>  	return 0;
>  }
>  
> -static int cqspi_indirect_write_execute(struct spi_nor *nor,
> -					const u8 *txbuf, const unsigned n_tx)
> +static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
> +					const u8 *txbuf, const size_t n_tx)
>  {
>  	const unsigned int page_size = nor->page_size;
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
> @@ -604,6 +600,7 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
>  	unsigned int write_bytes;
>  	int ret;
>  
> +	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
>  	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
>  
>  	/* Clear all interrupts. */
> @@ -900,11 +897,11 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_write_setup(nor, to);
> +	ret = cqspi_write_setup(nor);
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_write_execute(nor, buf, len);
> +	ret = cqspi_indirect_write_execute(nor, to, buf, len);
>  	if (ret)
>  		return ret;
>  
> @@ -920,11 +917,11 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_read_setup(nor, from);
> +	ret = cqspi_read_setup(nor);
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_read_execute(nor, buf, len);
> +	ret = cqspi_indirect_read_execute(nor, buf, from, len);
>  	if (ret)
>  		return ret;
>  
> 

[PATCH 1/2] mtd: spi-nor: cadence-quadspi: Refactor indirect read/write sequence.

[Cyrille Pitchen]

Hi Vignesh,

Le 07/12/2017 ? 07:38, Vignesh R a ?crit?:
> Move configuring of indirect read/write start address to
> cqspi_indirect_*_execute() function and rename cqspi_indirect_*_setup()
> function. This will help to reuse cqspi_indirect_*_setup() function for
> supporting direct access mode.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>

This patch looks good to me.

Best regards,

Cyrille

> ---
>  drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index 75a2bc447a99..becc7d714ab8 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -450,8 +450,7 @@ static int cqspi_command_write_addr(struct spi_nor *nor,
>  	return cqspi_exec_flash_cmd(cqspi, reg);
>  }
>  
> -static int cqspi_indirect_read_setup(struct spi_nor *nor,
> -				     const unsigned int from_addr)
> +static int cqspi_read_setup(struct spi_nor *nor)
>  {
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
> @@ -459,7 +458,6 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
>  	unsigned int dummy_clk = 0;
>  	unsigned int reg;
>  
> -	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
>  
>  	reg = nor->read_opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
>  	reg |= cqspi_calc_rdreg(nor, nor->read_opcode);
> @@ -493,8 +491,8 @@ static int cqspi_indirect_read_setup(struct spi_nor *nor,
>  	return 0;
>  }
>  
> -static int cqspi_indirect_read_execute(struct spi_nor *nor,
> -				       u8 *rxbuf, const unsigned n_rx)
> +static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
> +				       loff_t from_addr, const size_t n_rx)
>  {
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
> @@ -504,6 +502,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
>  	unsigned int bytes_to_read = 0;
>  	int ret = 0;
>  
> +	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
>  	writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
>  
>  	/* Clear all interrupts. */
> @@ -570,8 +569,7 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
>  	return ret;
>  }
>  
> -static int cqspi_indirect_write_setup(struct spi_nor *nor,
> -				      const unsigned int to_addr)
> +static int cqspi_write_setup(struct spi_nor *nor)
>  {
>  	unsigned int reg;
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
> @@ -584,8 +582,6 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
>  	reg = cqspi_calc_rdreg(nor, nor->program_opcode);
>  	writel(reg, reg_base + CQSPI_REG_RD_INSTR);
>  
> -	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
> -
>  	reg = readl(reg_base + CQSPI_REG_SIZE);
>  	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
>  	reg |= (nor->addr_width - 1);
> @@ -593,8 +589,8 @@ static int cqspi_indirect_write_setup(struct spi_nor *nor,
>  	return 0;
>  }
>  
> -static int cqspi_indirect_write_execute(struct spi_nor *nor,
> -					const u8 *txbuf, const unsigned n_tx)
> +static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
> +					const u8 *txbuf, const size_t n_tx)
>  {
>  	const unsigned int page_size = nor->page_size;
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
> @@ -604,6 +600,7 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
>  	unsigned int write_bytes;
>  	int ret;
>  
> +	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
>  	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
>  
>  	/* Clear all interrupts. */
> @@ -900,11 +897,11 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_write_setup(nor, to);
> +	ret = cqspi_write_setup(nor);
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_write_execute(nor, buf, len);
> +	ret = cqspi_indirect_write_execute(nor, to, buf, len);
>  	if (ret)
>  		return ret;
>  
> @@ -920,11 +917,11 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_read_setup(nor, from);
> +	ret = cqspi_read_setup(nor);
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_read_execute(nor, buf, len);
> +	ret = cqspi_indirect_read_execute(nor, buf, from, len);
>  	if (ret)
>  		return ret;
>  
> 

Re: [PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Cyrille Pitchen]

Hi Vignesh,

Le 07/12/2017 à 07:38, Vignesh R a écrit :
> Cadence QSPI controller provides direct access mode through which flash
> can be accessed in a memory-mapped IO mode. This enables read/write to
> flash using memcpy*() functions. This mode provides higher throughput
> for both read/write operations when compared to current indirect mode of
> operation.
> 
> This patch therefore adds support to use QSPI in direct mode. If the
> window reserved in SoC's memory map for MMIO access is less that of
> flash size(like on most SoCFPGA variants), then the driver falls back
> to indirect mode of operation.
> 
> On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
> switching to direct mode improves read throughput from 3MB/s to 8MB/s.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/mtd/spi-nor/cadence-quadspi.c | 52 +++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index becc7d714ab8..f8721ed68bc6 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -58,6 +58,7 @@ struct cqspi_flash_pdata {
>  	u8		data_width;
>  	u8		cs;
>  	bool		registered;
> +	bool		use_direct_mode;
>  };
>  
>  struct cqspi_st {
> @@ -68,6 +69,7 @@ struct cqspi_st {
>  
>  	void __iomem		*iobase;
>  	void __iomem		*ahb_base;
> +	resource_size_t		ahb_size;
>  	struct completion	transfer_complete;
>  	struct mutex		bus_mutex;
>  
> @@ -103,6 +105,7 @@ struct cqspi_st {
>  /* Register map */
>  #define CQSPI_REG_CONFIG			0x00
>  #define CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
> +#define CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL	BIT(7)
>  #define CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
>  #define CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
>  #define CQSPI_REG_CONFIG_DMA_MASK		BIT(15)
> @@ -569,6 +572,21 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
>  	return ret;
>  }
>  
> +static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
> +				     loff_t from_addr, const size_t len)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	u32 reg;
> +
> +	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> +	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> +	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);

I guess setting the ENB_DIR_ACC_CTRL bit could be set once for all when you
set use_direct_mode to true, couldn't it?

It may improve the read performance even more. However not expecting much
difference for Page Program operations.

Then you could call directly call mempcy_fromio() from cqspi_read().
Not mandatory for me, since I also like the symmetry of the 2 functions:
cqspi_direct_read_execute() / cqspi_indirect_read_execute().

So it's up to you :)

> +	memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
> +
> +	return 0;
> +}
> +
>  static int cqspi_write_setup(struct spi_nor *nor)
>  {
>  	unsigned int reg;
> @@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>  	return ret;
>  }
>  
> +static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
> +				      const u8 *txbuf, const size_t len)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	u32 reg;
> +
> +	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> +	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> +	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);

Same comment here.

> +	memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
> +
> +	return 0;
> +}
> +
>  static void cqspi_chipselect(struct spi_nor *nor)
>  {
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
> @@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
>  static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  			   size_t len, const u_char *buf)
>  {
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	int ret;
>  
>  	ret = cqspi_set_protocol(nor, 0);
> @@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_write_execute(nor, to, buf, len);
> +	if (f_pdata->use_direct_mode)
> +		ret = cqspi_direct_write_execute(nor, to, buf, len);
> +	else
> +		ret = cqspi_indirect_write_execute(nor, to, buf, len);
>  	if (ret)
>  		return ret;
>  
> @@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>  			  size_t len, u_char *buf)
>  {
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	int ret;
>  
>  	ret = cqspi_set_protocol(nor, 1);
> @@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_read_execute(nor, buf, from, len);
> +	if (f_pdata->use_direct_mode)
> +		ret = cqspi_direct_read_execute(nor, buf, from, len);
> +	else
> +		ret = cqspi_indirect_read_execute(nor, buf, from, len);
>  	if (ret)
>  		return ret;
>  
> @@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>  			goto err;
>  
>  		f_pdata->registered = true;
> +
> +		if (mtd->size <= cqspi->ahb_size) {
> +			f_pdata->use_direct_mode = true;
> +			dev_info(nor->dev, "using direct mode for %s\n",
> +				 mtd->name);

Please use dev_dbg() here insted of dev_info(). IMHO, this kind of output
is not really needed by regular users.

Otherwise, the series looks great!

Best regards,

Cyrille

> +		}
>  	}
>  
>  	return 0;
> @@ -1212,6 +1259,7 @@ static int cqspi_probe(struct platform_device *pdev)
>  		dev_err(dev, "Cannot remap AHB address.\n");
>  		return PTR_ERR(cqspi->ahb_base);
>  	}
> +	cqspi->ahb_size = resource_size(res_ahb);
>  
>  	init_completion(&cqspi->transfer_complete);
>  
> 

[PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Cyrille Pitchen]

Hi Vignesh,

Le 07/12/2017 ? 07:38, Vignesh R a ?crit :
> Cadence QSPI controller provides direct access mode through which flash
> can be accessed in a memory-mapped IO mode. This enables read/write to
> flash using memcpy*() functions. This mode provides higher throughput
> for both read/write operations when compared to current indirect mode of
> operation.
> 
> This patch therefore adds support to use QSPI in direct mode. If the
> window reserved in SoC's memory map for MMIO access is less that of
> flash size(like on most SoCFPGA variants), then the driver falls back
> to indirect mode of operation.
> 
> On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
> switching to direct mode improves read throughput from 3MB/s to 8MB/s.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/mtd/spi-nor/cadence-quadspi.c | 52 +++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index becc7d714ab8..f8721ed68bc6 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -58,6 +58,7 @@ struct cqspi_flash_pdata {
>  	u8		data_width;
>  	u8		cs;
>  	bool		registered;
> +	bool		use_direct_mode;
>  };
>  
>  struct cqspi_st {
> @@ -68,6 +69,7 @@ struct cqspi_st {
>  
>  	void __iomem		*iobase;
>  	void __iomem		*ahb_base;
> +	resource_size_t		ahb_size;
>  	struct completion	transfer_complete;
>  	struct mutex		bus_mutex;
>  
> @@ -103,6 +105,7 @@ struct cqspi_st {
>  /* Register map */
>  #define CQSPI_REG_CONFIG			0x00
>  #define CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
> +#define CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL	BIT(7)
>  #define CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
>  #define CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
>  #define CQSPI_REG_CONFIG_DMA_MASK		BIT(15)
> @@ -569,6 +572,21 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
>  	return ret;
>  }
>  
> +static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
> +				     loff_t from_addr, const size_t len)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	u32 reg;
> +
> +	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> +	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> +	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);

I guess setting the ENB_DIR_ACC_CTRL bit could be set once for all when you
set use_direct_mode to true, couldn't it?

It may improve the read performance even more. However not expecting much
difference for Page Program operations.

Then you could call directly call mempcy_fromio() from cqspi_read().
Not mandatory for me, since I also like the symmetry of the 2 functions:
cqspi_direct_read_execute() / cqspi_indirect_read_execute().

So it's up to you :)

> +	memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
> +
> +	return 0;
> +}
> +
>  static int cqspi_write_setup(struct spi_nor *nor)
>  {
>  	unsigned int reg;
> @@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>  	return ret;
>  }
>  
> +static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
> +				      const u8 *txbuf, const size_t len)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	u32 reg;
> +
> +	reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> +	reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
> +	writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);

Same comment here.

> +	memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
> +
> +	return 0;
> +}
> +
>  static void cqspi_chipselect(struct spi_nor *nor)
>  {
>  	struct cqspi_flash_pdata *f_pdata = nor->priv;
> @@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
>  static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  			   size_t len, const u_char *buf)
>  {
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	int ret;
>  
>  	ret = cqspi_set_protocol(nor, 0);
> @@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_write_execute(nor, to, buf, len);
> +	if (f_pdata->use_direct_mode)
> +		ret = cqspi_direct_write_execute(nor, to, buf, len);
> +	else
> +		ret = cqspi_indirect_write_execute(nor, to, buf, len);
>  	if (ret)
>  		return ret;
>  
> @@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>  static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>  			  size_t len, u_char *buf)
>  {
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>  	int ret;
>  
>  	ret = cqspi_set_protocol(nor, 1);
> @@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>  	if (ret)
>  		return ret;
>  
> -	ret = cqspi_indirect_read_execute(nor, buf, from, len);
> +	if (f_pdata->use_direct_mode)
> +		ret = cqspi_direct_read_execute(nor, buf, from, len);
> +	else
> +		ret = cqspi_indirect_read_execute(nor, buf, from, len);
>  	if (ret)
>  		return ret;
>  
> @@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>  			goto err;
>  
>  		f_pdata->registered = true;
> +
> +		if (mtd->size <= cqspi->ahb_size) {
> +			f_pdata->use_direct_mode = true;
> +			dev_info(nor->dev, "using direct mode for %s\n",
> +				 mtd->name);

Please use dev_dbg() here insted of dev_info(). IMHO, this kind of output
is not really needed by regular users.

Otherwise, the series looks great!

Best regards,

Cyrille

> +		}
>  	}
>  
>  	return 0;
> @@ -1212,6 +1259,7 @@ static int cqspi_probe(struct platform_device *pdev)
>  		dev_err(dev, "Cannot remap AHB address.\n");
>  		return PTR_ERR(cqspi->ahb_base);
>  	}
> +	cqspi->ahb_size = resource_size(res_ahb);
>  
>  	init_completion(&cqspi->transfer_complete);
>  
> 

Re: [PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Vignesh R]

Hi Cyrille

On Wednesday 20 December 2017 08:43 PM, Cyrille Pitchen wrote:
> Hi Vignesh,
> 
> Le 07/12/2017 à 07:38, Vignesh R a écrit :
>> Cadence QSPI controller provides direct access mode through which flash
>> can be accessed in a memory-mapped IO mode. This enables read/write to
>> flash using memcpy*() functions. This mode provides higher throughput
>> for both read/write operations when compared to current indirect mode of
>> operation.
>> 
>> This patch therefore adds support to use QSPI in direct mode. If the
>> window reserved in SoC's memory map for MMIO access is less that of
>> flash size(like on most SoCFPGA variants), then the driver falls back
>> to indirect mode of operation.
>> 
>> On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
>> switching to direct mode improves read throughput from 3MB/s to 8MB/s.
>> 
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---

[...]

>> +static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
>> +                                  loff_t from_addr, const size_t len)
>> +{
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +     struct cqspi_st *cqspi = f_pdata->cqspi;
>> +     u32 reg;
>> +
>> +     reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +     reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +     writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> I guess setting the ENB_DIR_ACC_CTRL bit could be set once for all when you
> set use_direct_mode to true, couldn't it?
> 
> It may improve the read performance even more. However not expecting much
> difference for Page Program operations.
> 
> Then you could call directly call mempcy_fromio() from cqspi_read().
> Not mandatory for me, since I also like the symmetry of the 2 functions:
> cqspi_direct_read_execute() / cqspi_indirect_read_execute().
> 

Right, actually I can unconditionally set ENB_DIR_ACC_CTRL once in
cqspi_controller_init(). Indirect accesses will still be forwarded to
indirect access controller even if direct access controller is kept
enabled. So, indirect ops users are not affected.
I will make the changes in v2.

> So it's up to you :)
> 
>> +     memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
>> +
>> +     return 0;
>> +}
>> +
>>  static int cqspi_write_setup(struct spi_nor *nor)
>>  {
>>        unsigned int reg;
>> @@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>>        return ret;
>>  }
>>  
>> +static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
>> +                                   const u8 *txbuf, const size_t len)
>> +{
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +     struct cqspi_st *cqspi = f_pdata->cqspi;
>> +     u32 reg;
>> +
>> +     reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +     reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +     writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> Same comment here.
> 
>> +     memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
>> +
>> +     return 0;
>> +}
>> +
>>  static void cqspi_chipselect(struct spi_nor *nor)
>>  {
>>        struct cqspi_flash_pdata *f_pdata = nor->priv;
>> @@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
>>  static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>                           size_t len, const u_char *buf)
>>  {
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>>        int ret;
>>  
>>        ret = cqspi_set_protocol(nor, 0);
>> @@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>        if (ret)
>>                return ret;
>>  
>> -     ret = cqspi_indirect_write_execute(nor, to, buf, len);
>> +     if (f_pdata->use_direct_mode)
>> +             ret = cqspi_direct_write_execute(nor, to, buf, len);
>> +     else
>> +             ret = cqspi_indirect_write_execute(nor, to, buf, len);
>>        if (ret)
>>                return ret;
>>  
>> @@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>  static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>                          size_t len, u_char *buf)
>>  {
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>>        int ret;
>>  
>>        ret = cqspi_set_protocol(nor, 1);
>> @@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>        if (ret)
>>                return ret;
>>  
>> -     ret = cqspi_indirect_read_execute(nor, buf, from, len);
>> +     if (f_pdata->use_direct_mode)
>> +             ret = cqspi_direct_read_execute(nor, buf, from, len);
>> +     else
>> +             ret = cqspi_indirect_read_execute(nor, buf, from, len);
>>        if (ret)
>>                return ret;
>>  
>> @@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>>                        goto err;
>>  
>>                f_pdata->registered = true;
>> +
>> +             if (mtd->size <= cqspi->ahb_size) {
>> +                     f_pdata->use_direct_mode = true;
>> +                     dev_info(nor->dev, "using direct mode for %s\n",
>> +                              mtd->name);
> 
> Please use dev_dbg() here insted of dev_info(). IMHO, this kind of output
> is not really needed by regular users.
> 

Agreed, will update that.

> Otherwise, the series looks great!
> 

Thanks for the review! Will submit v2 shortly.


-- 
Regards
Vignesh

Re: [PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Vignesh R]

Hi Cyrille

On Wednesday 20 December 2017 08:43 PM, Cyrille Pitchen wrote:
> Hi Vignesh,
> 
> Le 07/12/2017 à 07:38, Vignesh R a écrit :
>> Cadence QSPI controller provides direct access mode through which flash
>> can be accessed in a memory-mapped IO mode. This enables read/write to
>> flash using memcpy*() functions. This mode provides higher throughput
>> for both read/write operations when compared to current indirect mode of
>> operation.
>> 
>> This patch therefore adds support to use QSPI in direct mode. If the
>> window reserved in SoC's memory map for MMIO access is less that of
>> flash size(like on most SoCFPGA variants), then the driver falls back
>> to indirect mode of operation.
>> 
>> On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
>> switching to direct mode improves read throughput from 3MB/s to 8MB/s.
>> 
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---

[...]

>> +static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
>> +                                  loff_t from_addr, const size_t len)
>> +{
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +     struct cqspi_st *cqspi = f_pdata->cqspi;
>> +     u32 reg;
>> +
>> +     reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +     reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +     writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> I guess setting the ENB_DIR_ACC_CTRL bit could be set once for all when you
> set use_direct_mode to true, couldn't it?
> 
> It may improve the read performance even more. However not expecting much
> difference for Page Program operations.
> 
> Then you could call directly call mempcy_fromio() from cqspi_read().
> Not mandatory for me, since I also like the symmetry of the 2 functions:
> cqspi_direct_read_execute() / cqspi_indirect_read_execute().
> 

Right, actually I can unconditionally set ENB_DIR_ACC_CTRL once in
cqspi_controller_init(). Indirect accesses will still be forwarded to
indirect access controller even if direct access controller is kept
enabled. So, indirect ops users are not affected.
I will make the changes in v2.

> So it's up to you :)
> 
>> +     memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
>> +
>> +     return 0;
>> +}
>> +
>>  static int cqspi_write_setup(struct spi_nor *nor)
>>  {
>>        unsigned int reg;
>> @@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>>        return ret;
>>  }
>>  
>> +static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
>> +                                   const u8 *txbuf, const size_t len)
>> +{
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +     struct cqspi_st *cqspi = f_pdata->cqspi;
>> +     u32 reg;
>> +
>> +     reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +     reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +     writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> Same comment here.
> 
>> +     memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
>> +
>> +     return 0;
>> +}
>> +
>>  static void cqspi_chipselect(struct spi_nor *nor)
>>  {
>>        struct cqspi_flash_pdata *f_pdata = nor->priv;
>> @@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
>>  static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>                           size_t len, const u_char *buf)
>>  {
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>>        int ret;
>>  
>>        ret = cqspi_set_protocol(nor, 0);
>> @@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>        if (ret)
>>                return ret;
>>  
>> -     ret = cqspi_indirect_write_execute(nor, to, buf, len);
>> +     if (f_pdata->use_direct_mode)
>> +             ret = cqspi_direct_write_execute(nor, to, buf, len);
>> +     else
>> +             ret = cqspi_indirect_write_execute(nor, to, buf, len);
>>        if (ret)
>>                return ret;
>>  
>> @@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>  static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>                          size_t len, u_char *buf)
>>  {
>> +     struct cqspi_flash_pdata *f_pdata = nor->priv;
>>        int ret;
>>  
>>        ret = cqspi_set_protocol(nor, 1);
>> @@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>        if (ret)
>>                return ret;
>>  
>> -     ret = cqspi_indirect_read_execute(nor, buf, from, len);
>> +     if (f_pdata->use_direct_mode)
>> +             ret = cqspi_direct_read_execute(nor, buf, from, len);
>> +     else
>> +             ret = cqspi_indirect_read_execute(nor, buf, from, len);
>>        if (ret)
>>                return ret;
>>  
>> @@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>>                        goto err;
>>  
>>                f_pdata->registered = true;
>> +
>> +             if (mtd->size <= cqspi->ahb_size) {
>> +                     f_pdata->use_direct_mode = true;
>> +                     dev_info(nor->dev, "using direct mode for %s\n",
>> +                              mtd->name);
> 
> Please use dev_dbg() here insted of dev_info(). IMHO, this kind of output
> is not really needed by regular users.
> 

Agreed, will update that.

> Otherwise, the series looks great!
> 

Thanks for the review! Will submit v2 shortly.


-- 
Regards
Vignesh

[PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Vignesh R]

Hi Cyrille

On Wednesday 20 December 2017 08:43 PM, Cyrille Pitchen wrote:
> Hi Vignesh,
> 
> Le 07/12/2017 ? 07:38, Vignesh R a ?crit :
>> Cadence QSPI controller provides direct access mode through which flash
>> can be accessed in a memory-mapped IO mode. This enables read/write to
>> flash using memcpy*() functions. This mode provides higher throughput
>> for both read/write operations when compared to current indirect mode of
>> operation.
>> 
>> This patch therefore adds support to use QSPI in direct mode. If the
>> window reserved in SoC's memory map for MMIO access is less that of
>> flash size(like on most SoCFPGA variants), then the driver falls back
>> to indirect mode of operation.
>> 
>> On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
>> switching to direct mode improves read throughput from 3MB/s to 8MB/s.
>> 
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---

[...]

>> +static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
>> +????????????????????????????????? loff_t from_addr, const size_t len)
>> +{
>> +???? struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +???? struct cqspi_st *cqspi = f_pdata->cqspi;
>> +???? u32 reg;
>> +
>> +???? reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +???? reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +???? writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> I guess setting the ENB_DIR_ACC_CTRL bit could be set once for all when you
> set use_direct_mode to true, couldn't it?
> 
> It may improve the read performance even more. However not expecting much
> difference for Page Program operations.
> 
> Then you could call directly call mempcy_fromio() from cqspi_read().
> Not mandatory for me, since I also like the symmetry of the 2 functions:
> cqspi_direct_read_execute() / cqspi_indirect_read_execute().
> 

Right, actually I can unconditionally set ENB_DIR_ACC_CTRL once in
cqspi_controller_init(). Indirect accesses will still be forwarded to
indirect access controller even if direct access controller is kept
enabled. So, indirect ops users are not affected.
I will make the changes in v2.

> So it's up to you :)
> 
>> +???? memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
>> +
>> +???? return 0;
>> +}
>> +
>>? static int cqspi_write_setup(struct spi_nor *nor)
>>? {
>>??????? unsigned int reg;
>> @@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>>??????? return ret;
>>? }
>>? 
>> +static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
>> +?????????????????????????????????? const u8 *txbuf, const size_t len)
>> +{
>> +???? struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +???? struct cqspi_st *cqspi = f_pdata->cqspi;
>> +???? u32 reg;
>> +
>> +???? reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +???? reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +???? writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> Same comment here.
> 
>> +???? memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
>> +
>> +???? return 0;
>> +}
>> +
>>? static void cqspi_chipselect(struct spi_nor *nor)
>>? {
>>??????? struct cqspi_flash_pdata *f_pdata = nor->priv;
>> @@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
>>? static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>?????????????????????????? size_t len, const u_char *buf)
>>? {
>> +???? struct cqspi_flash_pdata *f_pdata = nor->priv;
>>??????? int ret;
>>? 
>>??????? ret = cqspi_set_protocol(nor, 0);
>> @@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>??????? if (ret)
>>??????????????? return ret;
>>? 
>> -???? ret = cqspi_indirect_write_execute(nor, to, buf, len);
>> +???? if (f_pdata->use_direct_mode)
>> +???????????? ret = cqspi_direct_write_execute(nor, to, buf, len);
>> +???? else
>> +???????????? ret = cqspi_indirect_write_execute(nor, to, buf, len);
>>??????? if (ret)
>>??????????????? return ret;
>>? 
>> @@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>? static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>????????????????????????? size_t len, u_char *buf)
>>? {
>> +???? struct cqspi_flash_pdata *f_pdata = nor->priv;
>>??????? int ret;
>>? 
>>??????? ret = cqspi_set_protocol(nor, 1);
>> @@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>??????? if (ret)
>>??????????????? return ret;
>>? 
>> -???? ret = cqspi_indirect_read_execute(nor, buf, from, len);
>> +???? if (f_pdata->use_direct_mode)
>> +???????????? ret = cqspi_direct_read_execute(nor, buf, from, len);
>> +???? else
>> +???????????? ret = cqspi_indirect_read_execute(nor, buf, from, len);
>>??????? if (ret)
>>??????????????? return ret;
>>? 
>> @@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>>??????????????????????? goto err;
>>? 
>>??????????????? f_pdata->registered = true;
>> +
>> +???????????? if (mtd->size <= cqspi->ahb_size) {
>> +???????????????????? f_pdata->use_direct_mode = true;
>> +???????????????????? dev_info(nor->dev, "using direct mode for %s\n",
>> +????????????????????????????? mtd->name);
> 
> Please use dev_dbg() here insted of dev_info(). IMHO, this kind of output
> is not really needed by regular users.
> 

Agreed, will update that.

> Otherwise, the series looks great!
> 

Thanks for the review! Will submit v2 shortly.


-- 
Regards
Vignesh