Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

On 04/07/2016 06:55 AM, Vignesh R wrote:
> 
> 
> On 04/07/2016 01:00 AM, Marek Vasut wrote:
>> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> I encountered a issue with this driver while testing.
>>
>> Try with the attached patches, I am planning to use them for V11
>> submission. I think you're hitting the problem with missing buslock.
>>
> 
> Thanks for the patches.
> But I am pretty sure that's not the problem at my end, because I have
> only one flash device on QSPI bus.
> 
> The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
> being read(during autodetect of chip), but at that instance,
> nor->page_size and nor->mtd.erasesize are not yet initialized (They are
> initialized only after JEDEC ID is looked up in the table and page_size
> and erasesize are known).
> Therefore if nor->page_size is printed during cqspi_switch_cs() then its
> zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
> after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
> configured only after spi_nor struct is fully populated (i.e after
> spi_nor_scan() has recognized the slave after JEDEC ID read).

Got it and I have a patch for this. Nice find, thanks! It gave me 60%
read performance boost on my machine :-)

I am now caching the page_size, erasesize, addr_width values, so I can
avoid reconfiguring the controller if there is no need for it, but
reconfigure it if there is a need. The patch is attached, but it's quite
big, so I also pushed a git branch with this driver for your convenience
(based on linux-next, expect rebases):

https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi

-- 
Best regards,
Marek Vasut

[-- Attachment #2: qspi.patch --]
[-- Type: text/x-patch, Size: 10714 bytes --]

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 7e61fba..331e7d1 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -66,6 +66,9 @@ struct cqspi_st {
 	struct mutex		bus_mutex;
 
 	int			current_cs;
+	int			current_page_size;
+	int			current_erase_size;
+	int			current_addr_width;
 	unsigned long		master_ref_clk_hz;
 	bool			is_decoded_cs;
 	u32			fifo_depth;
@@ -654,109 +657,60 @@ failwr:
 	return ret;
 }
 
-static int cqspi_set_protocol(struct spi_nor *nor, enum spi_nor_protocol proto)
+static void cqspi_chipselect(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int chip_select = f_pdata->cs;
+	unsigned int reg;
 
-	switch (proto) {
-	case SNOR_PROTO_1_1_1:
-	case SNOR_PROTO_1_1_2:
-	case SNOR_PROTO_1_1_4:
-	case SNOR_PROTO_1_2_2:
-	case SNOR_PROTO_1_4_4:
-		f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
-		break;
-	case SNOR_PROTO_2_2_2:
-		f_pdata->inst_width = CQSPI_INST_TYPE_DUAL;
-		break;
-	case SNOR_PROTO_4_4_4:
-		f_pdata->inst_width = CQSPI_INST_TYPE_QUAD;
-		break;
-	default:
-		return -EINVAL;
-	}
+	reg = readl(reg_base + CQSPI_REG_CONFIG);
+	if (cqspi->is_decoded_cs) {
+		reg |= CQSPI_REG_CONFIG_DECODE_MASK;
+	} else {
+		reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
 
-	switch (proto) {
-	case SNOR_PROTO_1_1_1:
-	case SNOR_PROTO_1_1_2:
-	case SNOR_PROTO_1_1_4:
-		f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
-		break;
-	case SNOR_PROTO_1_2_2:
-	case SNOR_PROTO_2_2_2:
-		f_pdata->addr_width = CQSPI_INST_TYPE_DUAL;
-		break;
-	case SNOR_PROTO_1_4_4:
-	case SNOR_PROTO_4_4_4:
-		f_pdata->addr_width = CQSPI_INST_TYPE_QUAD;
-		break;
-	default:
-		return -EINVAL;
+		/* Convert CS if without decoder.
+		 * CS0 to 4b'1110
+		 * CS1 to 4b'1101
+		 * CS2 to 4b'1011
+		 * CS3 to 4b'0111
+		 */
+		chip_select = 0xF & ~(1 << chip_select);
 	}
 
-	return 0;
-}
-
-static void cqspi_write(struct spi_nor *nor, loff_t to,
-			size_t len, size_t *retlen, const u_char *buf)
-{
-	int ret;
-
-	ret = cqspi_set_protocol(nor, nor->write_proto);
-	if (ret)
-		return;
-
-	ret = cqspi_indirect_write_setup(nor, to);
-	if (ret)
-		return;
-
-	ret = cqspi_indirect_write_execute(nor, buf, len);
-	if (ret)
-		return;
-
-	*retlen += len;
-}
-
-static int cqspi_read(struct spi_nor *nor, loff_t from,
-		      size_t len, size_t *retlen, u_char *buf)
-{
-	int ret;
-
-	ret = cqspi_set_protocol(nor, nor->read_proto);
-	if (ret)
-		return ret;
-
-	ret = cqspi_indirect_read_setup(nor, from);
-	if (ret)
-		return ret;
-
-	ret = cqspi_indirect_read_execute(nor, buf, len);
-	if (ret)
-		return ret;
-
-	*retlen += len;
-	return ret;
+	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
+		 << CQSPI_REG_CONFIG_CHIPSELECT_LSB);
+	reg |= (chip_select & CQSPI_REG_CONFIG_CHIPSELECT_MASK)
+	    << CQSPI_REG_CONFIG_CHIPSELECT_LSB;
+	writel(reg, reg_base + CQSPI_REG_CONFIG);
 }
 
-static int cqspi_erase(struct spi_nor *nor, loff_t offs)
+static void cqspi_configure_cs_and_sizes(struct spi_nor *nor)
 {
-	int ret;
-
-	ret = cqspi_set_protocol(nor, nor->erase_proto);
-	if (ret)
-		return ret;
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *iobase = cqspi->iobase;
+	unsigned int reg;
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
-	if (ret)
-		return ret;
+	/* configure page size and block size. */
+	reg = readl(iobase + CQSPI_REG_SIZE);
+	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
+	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
+	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
+	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
+	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
+	reg |= (nor->addr_width - 1);
+	writel(reg, iobase + CQSPI_REG_SIZE);
 
-	/* Set up command buffer. */
-	ret = cqspi_command_write_addr(nor, nor->erase_opcode, offs);
-	if (ret)
-		return ret;
+	/* configure the chip select */
+	cqspi_chipselect(nor);
 
-	return 0;
+	/* Store the new configuration of the controller */
+	cqspi->current_page_size = nor->page_size;
+	cqspi->current_erase_size = nor->mtd.erasesize;
+	cqspi->current_addr_width = nor->addr_width;
 }
 
 static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz,
@@ -854,36 +808,6 @@ static void cqspi_readdata_capture(struct cqspi_st *cqspi,
 	writel(reg, reg_base + CQSPI_REG_READCAPTURE);
 }
 
-static void cqspi_chipselect(struct spi_nor *nor)
-{
-	struct cqspi_flash_pdata *f_pdata = nor->priv;
-	struct cqspi_st *cqspi = f_pdata->cqspi;
-	void __iomem *reg_base = cqspi->iobase;
-	unsigned int chip_select = f_pdata->cs;
-	unsigned int reg;
-
-	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	if (cqspi->is_decoded_cs) {
-		reg |= CQSPI_REG_CONFIG_DECODE_MASK;
-	} else {
-		reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
-
-		/* Convert CS if without decoder.
-		 * CS0 to 4b'1110
-		 * CS1 to 4b'1101
-		 * CS2 to 4b'1011
-		 * CS3 to 4b'0111
-		 */
-		chip_select = 0xF & ~(1 << chip_select);
-	}
-
-	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
-		 << CQSPI_REG_CONFIG_CHIPSELECT_LSB);
-	reg |= (chip_select & CQSPI_REG_CONFIG_CHIPSELECT_MASK)
-	    << CQSPI_REG_CONFIG_CHIPSELECT_LSB;
-	writel(reg, reg_base + CQSPI_REG_CONFIG);
-}
-
 static void cqspi_controller_enable(struct cqspi_st *cqspi, bool enable)
 {
 	void __iomem *reg_base = cqspi->iobase;
@@ -899,34 +823,18 @@ static void cqspi_controller_enable(struct cqspi_st *cqspi, bool enable)
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 }
 
-static void cqspi_switch_cs(struct spi_nor *nor)
-{
-	struct cqspi_flash_pdata *f_pdata = nor->priv;
-	struct cqspi_st *cqspi = f_pdata->cqspi;
-	void __iomem *iobase = cqspi->iobase;
-	unsigned int reg;
-
-	/* configure page size and block size. */
-	reg = readl(iobase + CQSPI_REG_SIZE);
-	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
-	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
-	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
-	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
-	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
-	reg |= (nor->addr_width - 1);
-	writel(reg, iobase + CQSPI_REG_SIZE);
-
-	/* configure the chip select */
-	cqspi_chipselect(nor);
-}
-
-static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
+static void cqspi_configure(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
 	const unsigned int sclk = f_pdata->clk_rate;
-	const int switch_cs = (cqspi->current_cs != f_pdata->cs);
-	const int switch_ck = (cqspi->sclk != sclk);
+	int switch_cs = (cqspi->current_cs != f_pdata->cs);
+	int switch_ck = (cqspi->sclk != sclk);
+
+	if ((cqspi->current_page_size != nor->page_size) ||
+	    (cqspi->current_erase_size != nor->mtd.erasesize) ||
+	    (cqspi->current_addr_width != nor->addr_width))
+		switch_cs = 1;
 
 	if (switch_cs || switch_ck)
 		cqspi_controller_enable(cqspi, 0);
@@ -934,7 +842,7 @@ static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
 	/* Switch chip select. */
 	if (switch_cs) {
 		cqspi->current_cs = f_pdata->cs;
-		cqspi_switch_cs(nor);
+		cqspi_configure_cs_and_sizes(nor);
 	}
 
 	/* Setup baudrate divisor and delays */
@@ -947,6 +855,111 @@ static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
 
 	if (switch_cs || switch_ck)
 		cqspi_controller_enable(cqspi, 1);
+}
+
+static int cqspi_set_protocol(struct spi_nor *nor, enum spi_nor_protocol proto)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:
+	case SNOR_PROTO_1_1_2:
+	case SNOR_PROTO_1_1_4:
+	case SNOR_PROTO_1_2_2:
+	case SNOR_PROTO_1_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SNOR_PROTO_2_2_2:
+		f_pdata->inst_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SNOR_PROTO_4_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:
+	case SNOR_PROTO_1_1_2:
+	case SNOR_PROTO_1_1_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SNOR_PROTO_1_2_2:
+	case SNOR_PROTO_2_2_2:
+		f_pdata->addr_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SNOR_PROTO_1_4_4:
+	case SNOR_PROTO_4_4_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	cqspi_configure(nor);
+
+	return 0;
+}
+
+static void cqspi_write(struct spi_nor *nor, loff_t to,
+			size_t len, size_t *retlen, const u_char *buf)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->write_proto);
+	if (ret)
+		return;
+
+	ret = cqspi_indirect_write_setup(nor, to);
+	if (ret)
+		return;
+
+	ret = cqspi_indirect_write_execute(nor, buf, len);
+	if (ret)
+		return;
+
+	*retlen += len;
+}
+
+static int cqspi_read(struct spi_nor *nor, loff_t from,
+		      size_t len, size_t *retlen, u_char *buf)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->read_proto);
+	if (ret)
+		return ret;
+
+	ret = cqspi_indirect_read_setup(nor, from);
+	if (ret)
+		return ret;
+
+	ret = cqspi_indirect_read_execute(nor, buf, len);
+	if (ret)
+		return ret;
+
+	*retlen += len;
+	return ret;
+}
+
+static int cqspi_erase(struct spi_nor *nor, loff_t offs)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->erase_proto);
+	if (ret)
+		return ret;
+
+	/* Send write enable, then erase commands. */
+	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
+	if (ret)
+		return ret;
+
+	/* Set up command buffer. */
+	ret = cqspi_command_write_addr(nor, nor->erase_opcode, offs);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -958,7 +971,7 @@ static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
 
 	mutex_lock(&cqspi->bus_mutex);
 
-	return cqspi_prep_unlocked(nor, ops);
+	return 0;
 }
 
 static void cqspi_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
@@ -974,13 +987,9 @@ static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
-	if (ret)
-		goto exit;
-
-	cqspi_prep_unlocked(nor, SPI_NOR_OPS_READ);
+	if (!ret)
+		ret = cqspi_command_read(nor, &opcode, 1, buf, len);
 
-	ret = cqspi_command_read(nor, &opcode, 1, buf, len);
-exit:
 	return ret;
 }
 
@@ -989,13 +998,9 @@ static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
-	if (ret)
-		goto exit;
-
-	cqspi_prep_unlocked(nor, SPI_NOR_OPS_WRITE);
+	if (!ret)
+		ret = cqspi_command_write(nor, opcode, buf, len);
 
-	ret = cqspi_command_write(nor, opcode, buf, len);
-exit:
 	return ret;
 }