Skip to contentSkip to conversation

mtd: cqspi: Simplify indirect write code

PATCH Superseded Marek Vasut 4 participants 12 messages Apr 27, 2016 – May 4, 2016 SPI

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Marek Vasut

author
#0 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

The indirect write code is buggy pile of nastiness which fails horribly when the system runs fast enough to saturate the controller. The failure results in some pages (256B) not being written to the flash. This can be observed on systems which run with Dcache enabled and L2 cache enabled, like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>
---
 drivers/spi/cadence_qspi_apb.c | 122 +++++++++--------------------------------
 1 file changed, 26 insertions(+), 96 deletions(-)
drivers/spi/cadence_qspi_apb.c +26 -96
index 7786dd6..00a50cb 100644
@@ -28,6 +28,7 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/errno.h>
+#include <wait_bit.h>
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US			(1) /* 1us */

@@ -214,32 +215,6 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
 	return;
 }
 
-static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
-	const void *src, unsigned int bytes)
-{
-	unsigned int temp = 0;
-	int i;
-	int remaining = bytes;
-	unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
-	unsigned int *src_ptr = (unsigned int *)src;
-
-	while (remaining >= CQSPI_FIFO_WIDTH) {
-		for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-		src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
-		remaining -= CQSPI_FIFO_WIDTH;
-	}
-	if (remaining) {
-		/* dangling bytes */
-		i = remaining/sizeof(dest_ptr);
-		memcpy(&temp, src_ptr+i, remaining % sizeof(dest_ptr));
-		writel(temp, dest_ptr+i);
-		for (--i; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-	}
-	return;
-}
-
 /* Read from SRAM FIFO with polling SRAM fill level. */
 static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
 			const void *src_addr,  unsigned int num_bytes)

@@ -276,44 +251,6 @@ static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
 	return 0;
 }
 
-/* Write to SRAM FIFO with polling SRAM fill level. */
-static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
-				const void *src_addr, unsigned int num_bytes)
-{
-	const void *reg_base = plat->regbase;
-	void *dest_addr = plat->ahbbase;
-	unsigned int retry = CQSPI_REG_RETRY;
-	unsigned int sram_level;
-	unsigned int wr_bytes;
-	unsigned char *src = (unsigned char *)src_addr;
-	int remaining = num_bytes;
-	unsigned int page_size = plat->page_size;
-	unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
-
-	while (remaining > 0) {
-		retry = CQSPI_REG_RETRY;
-		while (retry--) {
-			sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
-			if (sram_level <= sram_threshold_words)
-				break;
-		}
-		if (!retry) {
-			printf("QSPI: SRAM fill level (0x%08x) not hit lower expected level (0x%08x)",
-			       sram_level, sram_threshold_words);
-			return -1;
-		}
-		/* Write a page or remaining bytes. */
-		wr_bytes = (remaining > page_size) ?
-					page_size : remaining;
-
-		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
-		src += wr_bytes;
-		remaining -= wr_bytes;
-	}
-
-	return 0;
-}
-
 void cadence_qspi_apb_controller_enable(void *reg_base)
 {
 	unsigned int reg;

@@ -810,48 +747,41 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
 }
 
 int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
-	unsigned int txlen, const u8 *txbuf)
+	unsigned int n_tx, const u8 *txbuf)
 {
-	unsigned int reg = 0;
-	unsigned int retry;
+	unsigned int page_size = plat->page_size;
+	unsigned int remaining = n_tx;
+	unsigned int write_bytes;
+	int ret;
 
 	/* Configure the indirect read transfer bytes */
-	writel(txlen, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
+	writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
 
 	/* Start the indirect write transfer */
 	writel(CQSPI_REG_INDIRECTWR_START_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 
-	if (qpsi_write_sram_fifo_push(plat, (const void *)txbuf, txlen))
-		goto failwr;
-
-	/* Wait until last write is completed (FIFO empty) */
-	retry = CQSPI_REG_RETRY;
-	while (retry--) {
-		reg = CQSPI_GET_WR_SRAM_LEVEL(plat->regbase);
-		if (reg == 0)
-			break;
-
-		udelay(1);
-	}
-
-	if (reg != 0) {
-		printf("QSPI: timeout for indirect write\n");
-		goto failwr;
-	}
+	while (remaining > 0) {
+		write_bytes = remaining > page_size ? page_size : remaining;
+		writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+		ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
+				   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
+				   CQSPI_REG_SDRAMLEVEL_WR_LSB, 0, 10, 0);
+		if (ret) {
+			printf("Indirect write timed out (%i)\n", ret);
+			goto failwr;
+		}
 
-	/* Check flash indirect controller status */
-	retry = CQSPI_REG_RETRY;
-	while (retry--) {
-		reg = readl(plat->regbase + CQSPI_REG_INDIRECTWR);
-		if (reg & CQSPI_REG_INDIRECTWR_DONE_MASK)
-			break;
-		udelay(1);
+		txbuf += write_bytes;
+		remaining -= write_bytes;
 	}
 
-	if (!(reg & CQSPI_REG_INDIRECTWR_DONE_MASK)) {
-		printf("QSPI: indirect completion status error with reg 0x%08x\n",
-		       reg);
+	/* Check indirect done status */
+	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTWR,
+			   CQSPI_REG_INDIRECTWR_DONE_MASK, 1, 10, 0);
+	if (ret) {
+		printf("Indirect write completion error (%i)\n", ret);
 		goto failwr;
 	}
 

@@ -864,7 +794,7 @@ failwr:
 	/* Cancel the indirect write */
 	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
-	return -1;
+	return ret;
 }
 
 void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy)
Signature · 1 line
2.7.0
author
#1 lore raw

PATCH 2/2 mtd: cqspi: Simplify indirect read code

The indirect read code is a pile of nastiness. This patch replaces the whole unmaintainable indirect read implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. All the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing. There is no speed change of the read operation.

Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>
---
 drivers/spi/cadence_qspi_apb.c | 124 ++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 71 deletions(-)
drivers/spi/cadence_qspi_apb.c +53 -71
index 00a50cb..ac47c6f 100644
@@ -193,64 +193,6 @@ static unsigned int cadence_qspi_apb_cmd2addr(const unsigned char *addr_buf,
 	return addr;
 }
 
-static void cadence_qspi_apb_read_fifo_data(void *dest,
-	const void *src_ahb_addr, unsigned int bytes)
-{
-	unsigned int temp;
-	int remaining = bytes;
-	unsigned int *dest_ptr = (unsigned int *)dest;
-	unsigned int *src_ptr = (unsigned int *)src_ahb_addr;
-
-	while (remaining >= sizeof(dest_ptr)) {
-		*dest_ptr = readl(src_ptr);
-		remaining -= sizeof(src_ptr);
-		dest_ptr++;
-	}
-	if (remaining) {
-		/* dangling bytes */
-		temp = readl(src_ptr);
-		memcpy(dest_ptr, &temp, remaining);
-	}
-
-	return;
-}
-
-/* Read from SRAM FIFO with polling SRAM fill level. */
-static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
-			const void *src_addr,  unsigned int num_bytes)
-{
-	unsigned int remaining = num_bytes;
-	unsigned int retry;
-	unsigned int sram_level = 0;
-	unsigned char *dest = (unsigned char *)dest_addr;
-
-	while (remaining > 0) {
-		retry = CQSPI_REG_RETRY;
-		while (retry--) {
-			sram_level = CQSPI_GET_RD_SRAM_LEVEL(reg_base);
-			if (sram_level)
-				break;
-			udelay(1);
-		}
-
-		if (!retry) {
-			printf("QSPI: No receive data after polling for %d times\n",
-			       CQSPI_REG_RETRY);
-			return -1;
-		}
-
-		sram_level *= CQSPI_FIFO_WIDTH;
-		sram_level = sram_level > remaining ? remaining : sram_level;
-
-		/* Read data from FIFO. */
-		cadence_qspi_apb_read_fifo_data(dest, src_addr, sram_level);
-		dest += sram_level;
-		remaining -= sram_level;
-		udelay(1);
-	}
-	return 0;
-}
-
 void cadence_qspi_apb_controller_enable(void *reg_base)
 {
 	unsigned int reg;

@@ -679,40 +621,80 @@ int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
 	return 0;
 }
 
+static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_platdata *plat)
+{
+	u32 reg = readl(plat->regbase + CQSPI_REG_SDRAMLEVEL);
+	reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
+	return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
+}
+
+static int cadence_qspi_wait_for_data(struct cadence_spi_platdata *plat)
+{
+	unsigned int timeout = 10000;
+	u32 reg;
+
+	while (timeout--) {
+		reg = cadence_qspi_get_rd_sram_level(plat);
+		if (reg)
+			return reg;
+		udelay(1);
+	}
+
+	return -ETIMEDOUT;
+}
+
 int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
-	unsigned int rxlen, u8 *rxbuf)
+	unsigned int n_rx, u8 *rxbuf)
 {
-	unsigned int reg;
+	unsigned int remaining = n_rx;
+	unsigned int bytes_to_read = 0;
+	int ret;
 
-	writel(rxlen, plat->regbase + CQSPI_REG_INDIRECTRDBYTES);
+	writel(n_rx, plat->regbase + CQSPI_REG_INDIRECTRDBYTES);
 
 	/* Start the indirect read transfer */
 	writel(CQSPI_REG_INDIRECTRD_START_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 
-	if (qspi_read_sram_fifo_poll(plat->regbase, (void *)rxbuf,
-				     (const void *)plat->ahbbase, rxlen))
-		goto failrd;
+	while (remaining > 0) {
+		ret = cadence_qspi_wait_for_data(plat);
+		if (ret < 0) {
+			printf("Indirect write timed out (%i)\n", ret);
+			goto failrd;
+		}
 
-	/* Check flash indirect controller */
-	reg = readl(plat->regbase + CQSPI_REG_INDIRECTRD);
-	if (!(reg & CQSPI_REG_INDIRECTRD_DONE_MASK)) {
-		reg = readl(plat->regbase + CQSPI_REG_INDIRECTRD);
-		printf("QSPI: indirect completion status error with reg 0x%08x\n",
-		       reg);
+		bytes_to_read = ret;
+
+		while (bytes_to_read != 0) {
+			bytes_to_read *= CQSPI_FIFO_WIDTH;
+			bytes_to_read = bytes_to_read > remaining ?
+					remaining : bytes_to_read;
+			readsl(plat->ahbbase, rxbuf, DIV_ROUND_UP(bytes_to_read, 4));
+			rxbuf += bytes_to_read;
+			remaining -= bytes_to_read;
+			bytes_to_read = cadence_qspi_get_rd_sram_level(plat);
+		}
+	}
+
+	/* Check indirect done status */
+	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTRD,
+			   CQSPI_REG_INDIRECTRD_DONE_MASK, 1, 10, 0);
+	if (ret) {
+		printf("Indirect read completion error (%i)\n", ret);
 		goto failrd;
 	}
 
 	/* Clear indirect completion status */
 	writel(CQSPI_REG_INDIRECTRD_DONE_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
+
 	return 0;
 
 failrd:
 	/* Cancel the indirect read */
 	writel(CQSPI_REG_INDIRECTRD_CANCEL_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
-	return -1;
+	return ret;
 }
 
 /* Opcode + Address (3/4 bytes) */
Signature · 1 line
2.7.0
#2 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

In reply to Marek Vasut
Quoted 22 lines · Marek Vasut

The indirect write code is buggy pile of nastiness which fails horribly when the system runs fast enough to saturate the controller. The failure results in some pages (256B) not being written to the flash. This can be observed on systems which run with Dcache enabled and L2 cache enabled, like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>
Quoted 1 line

---

Tested on K2G EVM that has Cadence QSPI controller and s25fl512s Spansion flash
For both the patches:

Tested-by Vignesh R <[email protected]>
Signature · 2 lines
Regards
Vignesh
#3 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

In reply to Marek Vasut

Hi Marek,

Quoted 14 lines · Marek Vasut

The indirect write code is buggy pile of nastiness which fails horribly when the system runs fast enough to saturate the controller. The failure results in some pages (256B) not being written to the flash. This can be observed on systems which run with Dcache enabled and L2 cache enabled, like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>

I've applied both patches and tested them on SR1500 (SPI-NOR used for booting and redundant environment). This is what I get upon "saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB Erasing SPI flash...Writing to SPI flash...data abort

pc : [<3ff8368a>]          lr : [<3ff8301b>]

reloc pc : [<010216ca>] lr : [<0102105b>]
sp : 3bf54eb8 ip : 3ff82f69 fp : 00000002
r10: 00000000 r9 : 3bf5dee8 r8 : ffff0000
r7 : 00000001 r6 : 3bf54f9b r5 : 00000001 r4 : 3bf5e520
r3 : 00000000 r2 : 3bf54f9b r1 : 00000001 r0 : ffa00000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-00009-ge1bf9b8 (Apr 29 2016 - 11:25:46)

Any idea, what might be going wrong here?

Thanks,
Stefan

author
#4 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

Quoted 43 lines · Stefan Roese

Hi Marek,

Quoted 14 lines · Marek Vasut

The indirect write code is buggy pile of nastiness which fails horribly when the system runs fast enough to saturate the controller. The failure results in some pages (256B) not being written to the flash. This can be observed on systems which run with Dcache enabled and L2 cache enabled, like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>

I've applied both patches and tested them on SR1500 (SPI-NOR used for booting and redundant environment). This is what I get upon "saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB Erasing SPI flash...Writing to SPI flash...data abort

pc : [<3ff8368a>]          lr : [<3ff8301b>]

reloc pc : [<010216ca>] lr : [<0102105b>]
sp : 3bf54eb8 ip : 3ff82f69 fp : 00000002
r10: 00000000 r9 : 3bf5dee8 r8 : ffff0000
r7 : 00000001 r6 : 3bf54f9b r5 : 00000001 r4 : 3bf5e520
r3 : 00000000 r2 : 3bf54f9b r1 : 00000001 r0 : ffa00000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

Quoted 6 lines

resetting ...

U-Boot SPL 2016.05-rc3-00009-ge1bf9b8 (Apr 29 2016 - 11:25:46)

Any idea, what might be going wrong here?

Does it work without the patch ? Where does your PC point to at the time of the crash ,which function is it ?

Quoted 3 lines

Thanks,
Stefan

Signature · 2 lines
Best regards,
Marek Vasut
#5 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

Quoted 41 lines · Marek Vasut
Quoted 41 lines · Marek Vasut
Quoted 14 lines

The indirect write code is buggy pile of nastiness which fails horribly when the system runs fast enough to saturate the controller. The failure results in some pages (256B) not being written to the flash. This can be observed on systems which run with Dcache enabled and L2 cache enabled, like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>

I've applied both patches and tested them on SR1500 (SPI-NOR used for booting and redundant environment). This is what I get upon "saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB Erasing SPI flash...Writing to SPI flash...data abort

pc : [<3ff8368a>]          lr : [<3ff8301b>]

reloc pc : [<010216ca>] lr : [<0102105b>]
sp : 3bf54eb8 ip : 3ff82f69 fp : 00000002
r10: 00000000 r9 : 3bf5dee8 r8 : ffff0000
r7 : 00000001 r6 : 3bf54f9b r5 : 00000001 r4 : 3bf5e520
r3 : 00000000 r2 : 3bf54f9b r1 : 00000001 r0 : ffa00000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

Quoted 8 lines
Quoted 6 lines

resetting ...

U-Boot SPL 2016.05-rc3-00009-ge1bf9b8 (Apr 29 2016 - 11:25:46)

Any idea, what might be going wrong here?

Does it work without the patch ?

Yes, of course. I wouldn't have posted as a reply to this patch if this is not the root cause. The board is using SPI NOR for env storage from the beginning.

Where does your PC point to at the time

Quoted 1 line

of the crash ,which function is it ?

Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte aligned txbuf. This causes the data-abort here. Here my small patch that fixes the problem:

drivers/spi/cadence_qspi_apb.c +9 -1
index ac47c6f..021a3e8 100644
@@ -745,7 +745,15 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
        while (remaining > 0) {
                write_bytes = remaining > page_size ? page_size : remaining;
-               writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+               /*
+                * Handle non 4-byte aligned access differently to avoid
+                * data-aborts
+                */
+               if (((u32)txbuf % 4) || (write_bytes % 4))
+                       writesb(plat->ahbbase, txbuf, write_bytes);
+               else
+                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
 
                ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
                                   CQSPI_REG_SDRAMLEVEL_WR_MASK <<

Please fell free to use this patch as-is and squash it into your patches or enhance it while doing this. The read function is also missing this unaligned handling. And of course the Linux driver version as well.

Thanks,
Stefan

author
#6 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

Quoted 47 lines · Stefan Roese
Quoted 46 lines · Marek Vasut
Quoted 46 lines · Marek Vasut
Quoted 14 lines

The indirect write code is buggy pile of nastiness which fails horribly when the system runs fast enough to saturate the controller. The failure results in some pages (256B) not being written to the flash. This can be observed on systems which run with Dcache enabled and L2 cache enabled, like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>

I've applied both patches and tested them on SR1500 (SPI-NOR used for booting and redundant environment). This is what I get upon "saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB Erasing SPI flash...Writing to SPI flash...data abort

pc : [<3ff8368a>]          lr : [<3ff8301b>]

reloc pc : [<010216ca>] lr : [<0102105b>]
sp : 3bf54eb8 ip : 3ff82f69 fp : 00000002
r10: 00000000 r9 : 3bf5dee8 r8 : ffff0000
r7 : 00000001 r6 : 3bf54f9b r5 : 00000001 r4 : 3bf5e520
r3 : 00000000 r2 : 3bf54f9b r1 : 00000001 r0 : ffa00000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-00009-ge1bf9b8 (Apr 29 2016 - 11:25:46)

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Any idea, what might be going wrong here?

Does it work without the patch ?

Yes, of course. I wouldn't have posted as a reply to this patch if this is not the root cause.

*grumble*

Quoted 1 line

The board is using SPI NOR for env storage from the beginning.

It only happens if you use redundant env in SPI NOR.

Quoted 11 lines

Where does your PC point to at the time

Quoted 1 line

of the crash ,which function is it ?

Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte aligned txbuf. This causes the data-abort here. Here my small patch that fixes the problem:

Thanks, see below.

Quoted 23 lines
drivers/spi/cadence_qspi_apb.c +9 -1
index ac47c6f..021a3e8 100644
@@ -745,7 +745,15 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
        while (remaining > 0) {
                write_bytes = remaining > page_size ? page_size : remaining;
-               writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+               /*
+                * Handle non 4-byte aligned access differently to avoid
+                * data-aborts
+                */
+               if (((u32)txbuf % 4) || (write_bytes % 4))
+                       writesb(plat->ahbbase, txbuf, write_bytes);
+               else
+                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
 
                ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
                                   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
Quoted 3 lines

Please fell free to use this patch as-is and squash it into your patches or enhance it while doing this. The read function is also missing this unaligned handling.

Im afraid of the performance hit that we can suffer if we use byte-level access for every unaligned buffer. What do you think
about using a bounce-buffer instead ?

Quoted 1 line

And of course the Linux driver version as well.

Does linux use unaligned buffers internally ?

Quoted 3 lines

Thanks,
Stefan

Signature · 2 lines
Best regards,
Marek Vasut
#7 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

Quoted 48 lines · Marek Vasut
Quoted 47 lines · Stefan Roese
Quoted 46 lines · Marek Vasut
Quoted 46 lines · Marek Vasut
Quoted 14 lines

The indirect write code is buggy pile of nastiness which fails horribly when the system runs fast enough to saturate the controller. The failure results in some pages (256B) not being written to the flash. This can be observed on systems which run with Dcache enabled and L2 cache enabled, like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>

I've applied both patches and tested them on SR1500 (SPI-NOR used for booting and redundant environment). This is what I get upon "saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB Erasing SPI flash...Writing to SPI flash...data abort

pc : [<3ff8368a>]          lr : [<3ff8301b>]

reloc pc : [<010216ca>] lr : [<0102105b>]
sp : 3bf54eb8 ip : 3ff82f69 fp : 00000002
r10: 00000000 r9 : 3bf5dee8 r8 : ffff0000
r7 : 00000001 r6 : 3bf54f9b r5 : 00000001 r4 : 3bf5e520
r3 : 00000000 r2 : 3bf54f9b r1 : 00000001 r0 : ffa00000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-00009-ge1bf9b8 (Apr 29 2016 - 11:25:46)

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Any idea, what might be going wrong here?

Does it work without the patch ?

Yes, of course. I wouldn't have posted as a reply to this patch if this is not the root cause.

*grumble*

?

Quoted 41 lines
Quoted 1 line

The board is using SPI NOR for env storage from the beginning.

It only happens if you use redundant env in SPI NOR.

Quoted 11 lines

Where does your PC point to at the time

Quoted 1 line

of the crash ,which function is it ?

Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte aligned txbuf. This causes the data-abort here. Here my small patch that fixes the problem:

Thanks, see below.

Quoted 23 lines
drivers/spi/cadence_qspi_apb.c +9 -1
index ac47c6f..021a3e8 100644
@@ -745,7 +745,15 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
         while (remaining > 0) {
                 write_bytes = remaining > page_size ? page_size : remaining;
-               writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+               /*
+                * Handle non 4-byte aligned access differently to avoid
+                * data-aborts
+                */
+               if (((u32)txbuf % 4) || (write_bytes % 4))
+                       writesb(plat->ahbbase, txbuf, write_bytes);
+               else
+                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
 
                 ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
                                    CQSPI_REG_SDRAMLEVEL_WR_MASK <<
Quoted 6 lines
Quoted 3 lines

Please fell free to use this patch as-is and squash it into your patches or enhance it while doing this. The read function is also missing this unaligned handling.

Im afraid of the performance hit that we can suffer if we use byte-level access for every unaligned buffer.

This is why is wrote: "or enhance it while doing this". You might want to change the code to first write the (optionally) unaligned bytes, then the aligned bytes via writesl() and last the (optionally) unaligned bytes.

Quoted 2 lines

What do you think
about using a bounce-buffer instead ?

I would prefer the simple solution I've drafted above.

Quoted 3 lines
Quoted 1 line

And of course the Linux driver version as well.

Does linux use unaligned buffers internally ?

Frankly, I don't know for sure. But I suspect, that you can also see unaligned buffers (and sizes!!!) in Linux as well. And you can't just write a different amount of data to the SPI NOR, which happens when you use DIV_ROUND_UP on an unaligned size.

Thanks,
Stefan

author
#8 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

Quoted 62 lines · Stefan Roese
Quoted 61 lines · Marek Vasut
Quoted 60 lines · Stefan Roese
Quoted 59 lines · Marek Vasut
Quoted 58 lines · Marek Vasut
Quoted 24 lines

The indirect write code is buggy pile of nastiness which fails horribly
when the system runs fast enough to saturate the controller. The failure
results in some pages (256B) not being written to the flash. This can be
observed on systems which run with Dcache enabled and L2 cache enabled,
like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation
with the one from upcoming Linux CQSPI driver, which went through multiple
rounds of thorough review and testing. While this makes the patch look
terrifying and violates all best-practices of software development, all
the patch does is it plucks out duplicate ad-hoc code distributed across
the driver and replaces it with more compact code doing exactly the same
thing.

Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>

I've applied both patches and tested them on SR1500 (SPI-NOR used for booting and redundant environment). This is what I get upon "saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB
Erasing SPI flash...Writing to SPI flash...data abort

pc : [<3ff8368a>]          lr : [<3ff8301b>]

reloc pc : [<010216ca>] lr : [<0102105b>]
sp : 3bf54eb8 ip : 3ff82f69 fp : 00000002
r10: 00000000 r9 : 3bf5dee8 r8 : ffff0000
r7 : 00000001 r6 : 3bf54f9b r5 : 00000001 r4 : 3bf5e520
r3 : 00000000 r2 : 3bf54f9b r1 : 00000001 r0 : ffa00000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-00009-ge1bf9b8 (Apr 29 2016 - 11:25:46)

Any idea, what might be going wrong here?

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Does it work without the patch ?

Yes, of course. I wouldn't have posted as a reply to this patch if this is not the root cause.

*grumble*

?

I just sensed slight USB subtext here ;-)

Quoted 59 lines
Quoted 47 lines
Quoted 1 line

The board is using SPI NOR for env storage from the beginning.

It only happens if you use redundant env in SPI NOR.

Quoted 11 lines

Where does your PC point to at the time

Quoted 1 line

of the crash ,which function is it ?

Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte aligned txbuf. This causes the data-abort here. Here my small patch that fixes the problem:

Thanks, see below.

Quoted 29 lines
drivers/spi/cadence_qspi_apb.c +0 -0
index ac47c6f..021a3e8 100644
@@ -745,7 +745,15 @@ int

cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,

while (remaining > 0) {
write_bytes = remaining > page_size ? page_size :
remaining;
- writesl(plat->ahbbase, txbuf,
DIV_ROUND_UP(write_bytes, 4));

drivers/spi/cadence_qspi_apb.c +9 -0
+
+               /*
+                * Handle non 4-byte aligned access differently to avoid
+                * data-aborts
+                */
+               if (((u32)txbuf % 4) || (write_bytes % 4))
+                       writesb(plat->ahbbase, txbuf, write_bytes);
+               else
+                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
 
                 ret = wait_for_bit("QSPI", plat->regbase +

CQSPI_REG_SDRAMLEVEL,
CQSPI_REG_SDRAMLEVEL_WR_MASK <<

Quoted 6 lines
Quoted 3 lines

Please fell free to use this patch as-is and squash it into your patches or enhance it while doing this. The read function is also missing this unaligned handling.

Im afraid of the performance hit that we can suffer if we use byte-level access for every unaligned buffer.

This is why is wrote: "or enhance it while doing this". You might want to change the code to first write the (optionally) unaligned bytes, then the aligned bytes via writesl() and last the (optionally) unaligned bytes.

Quoted 4 lines
Quoted 2 lines

What do you think
about using a bounce-buffer instead ?

I would prefer the simple solution I've drafted above.

OK, I checked how many aligned and unaligned transfers happens and I guess it's really not worth going through the bounce buffer for 1 byte which only happens once during saveenv.

I'll squash it and pick via socfpga tree, so we can get this in 2016.05 and have working QSPI NOR support there.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

And of course the Linux driver version as well.

Does linux use unaligned buffers internally ?

Frankly, I don't know for sure. But I suspect, that you can also see unaligned buffers (and sizes!!!) in Linux as well. And you can't just write a different amount of data to the SPI NOR, which happens when you use DIV_ROUND_UP on an unaligned size.

You can write different amount of data into the FIFO, the amount which is transferred is controlled by INDIRECTRDBYTES / INDIRECTWRBYTES register.

Quoted 2 lines

Thanks,
Stefan

Signature · 2 lines
Best regards,
Marek Vasut
#9 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

Quoted 136 lines · Marek Vasut
Quoted 62 lines · Stefan Roese
Quoted 61 lines · Marek Vasut
Quoted 60 lines · Stefan Roese
Quoted 59 lines · Marek Vasut
Quoted 58 lines · Marek Vasut
Quoted 24 lines

The indirect write code is buggy pile of nastiness which fails horribly
when the system runs fast enough to saturate the controller. The failure
results in some pages (256B) not being written to the flash. This can be
observed on systems which run with Dcache enabled and L2 cache enabled,
like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation
with the one from upcoming Linux CQSPI driver, which went through multiple
rounds of thorough review and testing. While this makes the patch look
terrifying and violates all best-practices of software development, all
the patch does is it plucks out duplicate ad-hoc code distributed across
the driver and replaces it with more compact code doing exactly the same
thing.

Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Jagan Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vignesh R <[email protected]>

I've applied both patches and tested them on SR1500 (SPI-NOR used for booting and redundant environment). This is what I get upon "saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB
Erasing SPI flash...Writing to SPI flash...data abort

pc : [<3ff8368a>]          lr : [<3ff8301b>]

reloc pc : [<010216ca>] lr : [<0102105b>]
sp : 3bf54eb8 ip : 3ff82f69 fp : 00000002
r10: 00000000 r9 : 3bf5dee8 r8 : ffff0000
r7 : 00000001 r6 : 3bf54f9b r5 : 00000001 r4 : 3bf5e520
r3 : 00000000 r2 : 3bf54f9b r1 : 00000001 r0 : ffa00000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-00009-ge1bf9b8 (Apr 29 2016 - 11:25:46)

Any idea, what might be going wrong here?

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Does it work without the patch ?

Yes, of course. I wouldn't have posted as a reply to this patch if this is not the root cause.

*grumble*

?

I just sensed slight USB subtext here ;-)

Quoted 62 lines
Quoted 47 lines
Quoted 1 line

The board is using SPI NOR for env storage from the beginning.

It only happens if you use redundant env in SPI NOR.

Quoted 11 lines

Where does your PC point to at the time

Quoted 1 line

of the crash ,which function is it ?

Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte aligned txbuf. This causes the data-abort here. Here my small patch that fixes the problem:

Thanks, see below.

Quoted 29 lines
drivers/spi/cadence_qspi_apb.c +0 -0
index ac47c6f..021a3e8 100644
@@ -745,7 +745,15 @@ int

cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,

while (remaining > 0) {
write_bytes = remaining > page_size ? page_size :
remaining;
- writesl(plat->ahbbase, txbuf,
DIV_ROUND_UP(write_bytes, 4));

drivers/spi/cadence_qspi_apb.c +9 -0
+
+               /*
+                * Handle non 4-byte aligned access differently to avoid
+                * data-aborts
+                */
+               if (((u32)txbuf % 4) || (write_bytes % 4))
+                       writesb(plat->ahbbase, txbuf, write_bytes);
+               else
+                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
 
                  ret = wait_for_bit("QSPI", plat->regbase +

CQSPI_REG_SDRAMLEVEL,
CQSPI_REG_SDRAMLEVEL_WR_MASK <<

Quoted 6 lines
Quoted 3 lines

Please fell free to use this patch as-is and squash it into your patches or enhance it while doing this. The read function is also missing this unaligned handling.

Im afraid of the performance hit that we can suffer if we use byte-level access for every unaligned buffer.

This is why is wrote: "or enhance it while doing this". You might want to change the code to first write the (optionally) unaligned bytes, then the aligned bytes via writesl() and last the (optionally) unaligned bytes.

Quoted 2 lines

What do you think
about using a bounce-buffer instead ?

Quoted 8 lines
Quoted 1 line

I would prefer the simple solution I've drafted above.

OK, I checked how many aligned and unaligned transfers happens and I guess it's really not worth going through the bounce buffer for 1 byte which only happens once during saveenv.

I'll squash it and pick via socfpga tree, so we can get this in 2016.05 and have working QSPI NOR support there.

Good, thanks!

Quoted 11 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

And of course the Linux driver version as well.

Does linux use unaligned buffers internally ?

Frankly, I don't know for sure. But I suspect, that you can also see unaligned buffers (and sizes!!!) in Linux as well. And you can't just write a different amount of data to the SPI NOR, which happens when you use DIV_ROUND_UP on an unaligned size.

You can write different amount of data into the FIFO, the amount which is transferred is controlled by INDIRECTRDBYTES / INDIRECTWRBYTES register.

Okay. But won't those additional (non-written) bytes not be stuck in the FIFO until the next transfer? And sent first before the new data that is written to the FIFO?

But that's a mood discussion as its not a problem any more with the v2 of the patches AFAICT.

Thanks,
Stefan

#10 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

In reply to Marek Vasut

Hi!

Quoted 5 lines

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all

Could we get something less terifying and less violating? :-).

Quoted 3 lines

the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

So this one is just a cleanup, and no behaviour change yet?

Pavel

Quoted 176 lines
---
 drivers/spi/cadence_qspi_apb.c | 122 +++++++++--------------------------------
 1 file changed, 26 insertions(+), 96 deletions(-)
drivers/spi/cadence_qspi_apb.c +26 -96
index 7786dd6..00a50cb 100644
@@ -28,6 +28,7 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/errno.h>
+#include <wait_bit.h>
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US			(1) /* 1us */

@@ -214,32 +215,6 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
 	return;
 }
 
-static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
-	const void *src, unsigned int bytes)
-{
-	unsigned int temp = 0;
-	int i;
-	int remaining = bytes;
-	unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
-	unsigned int *src_ptr = (unsigned int *)src;
-
-	while (remaining >= CQSPI_FIFO_WIDTH) {
-		for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-		src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
-		remaining -= CQSPI_FIFO_WIDTH;
-	}
-	if (remaining) {
-		/* dangling bytes */
-		i = remaining/sizeof(dest_ptr);
-		memcpy(&temp, src_ptr+i, remaining % sizeof(dest_ptr));
-		writel(temp, dest_ptr+i);
-		for (--i; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-	}
-	return;
-}
-
 /* Read from SRAM FIFO with polling SRAM fill level. */
 static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
 			const void *src_addr,  unsigned int num_bytes)

@@ -276,44 +251,6 @@ static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
 	return 0;
 }
 
-/* Write to SRAM FIFO with polling SRAM fill level. */
-static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
-				const void *src_addr, unsigned int num_bytes)
-{
-	const void *reg_base = plat->regbase;
-	void *dest_addr = plat->ahbbase;
-	unsigned int retry = CQSPI_REG_RETRY;
-	unsigned int sram_level;
-	unsigned int wr_bytes;
-	unsigned char *src = (unsigned char *)src_addr;
-	int remaining = num_bytes;
-	unsigned int page_size = plat->page_size;
-	unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
-
-	while (remaining > 0) {
-		retry = CQSPI_REG_RETRY;
-		while (retry--) {
-			sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
-			if (sram_level <= sram_threshold_words)
-				break;
-		}
-		if (!retry) {
-			printf("QSPI: SRAM fill level (0x%08x) not hit lower expected level (0x%08x)",
-			       sram_level, sram_threshold_words);
-			return -1;
-		}
-		/* Write a page or remaining bytes. */
-		wr_bytes = (remaining > page_size) ?
-					page_size : remaining;
-
-		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
-		src += wr_bytes;
-		remaining -= wr_bytes;
-	}
-
-	return 0;
-}
-
 void cadence_qspi_apb_controller_enable(void *reg_base)
 {
 	unsigned int reg;

@@ -810,48 +747,41 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
 }
 
 int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
-	unsigned int txlen, const u8 *txbuf)
+	unsigned int n_tx, const u8 *txbuf)
 {
-	unsigned int reg = 0;
-	unsigned int retry;
+	unsigned int page_size = plat->page_size;
+	unsigned int remaining = n_tx;
+	unsigned int write_bytes;
+	int ret;
 
 	/* Configure the indirect read transfer bytes */
-	writel(txlen, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
+	writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
 
 	/* Start the indirect write transfer */
 	writel(CQSPI_REG_INDIRECTWR_START_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 
-	if (qpsi_write_sram_fifo_push(plat, (const void *)txbuf, txlen))
-		goto failwr;
-
-	/* Wait until last write is completed (FIFO empty) */
-	retry = CQSPI_REG_RETRY;
-	while (retry--) {
-		reg = CQSPI_GET_WR_SRAM_LEVEL(plat->regbase);
-		if (reg == 0)
-			break;
-
-		udelay(1);
-	}
-
-	if (reg != 0) {
-		printf("QSPI: timeout for indirect write\n");
-		goto failwr;
-	}
+	while (remaining > 0) {
+		write_bytes = remaining > page_size ? page_size : remaining;
+		writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+		ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
+				   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
+				   CQSPI_REG_SDRAMLEVEL_WR_LSB, 0, 10, 0);
+		if (ret) {
+			printf("Indirect write timed out (%i)\n", ret);
+			goto failwr;
+		}
 
-	/* Check flash indirect controller status */
-	retry = CQSPI_REG_RETRY;
-	while (retry--) {
-		reg = readl(plat->regbase + CQSPI_REG_INDIRECTWR);
-		if (reg & CQSPI_REG_INDIRECTWR_DONE_MASK)
-			break;
-		udelay(1);
+		txbuf += write_bytes;
+		remaining -= write_bytes;
 	}
 
-	if (!(reg & CQSPI_REG_INDIRECTWR_DONE_MASK)) {
-		printf("QSPI: indirect completion status error with reg 0x%08x\n",
-		       reg);
+	/* Check indirect done status */
+	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTWR,
+			   CQSPI_REG_INDIRECTWR_DONE_MASK, 1, 10, 0);
+	if (ret) {
+		printf("Indirect write completion error (%i)\n", ret);
 		goto failwr;
 	}
 

@@ -864,7 +794,7 @@ failwr:
 	/* Cancel the indirect write */
 	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
-	return -1;
+	return ret;
 }
 
 void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy)
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#11 lore raw

PATCH 1/2 mtd: cqspi: Simplify indirect write code

Quoted 8 lines · Pavel Machek

Hi!

Quoted 5 lines

This patch replaces the whole unmaintainable indirect write implementation with the one from upcoming Linux CQSPI driver, which went through multiple rounds of thorough review and testing. While this makes the patch look terrifying and violates all best-practices of software development, all

Quoted 7 lines

Could we get something less terifying and less violating? :-).

Quoted 3 lines

the patch does is it plucks out duplicate ad-hoc code distributed across the driver and replaces it with more compact code doing exactly the same thing.

So this one is just a cleanup, and no behaviour change yet?

Apparently Stefan discovered one behavior change, which I need to look into. Apparently, u-boot can pass unaligned buffer to the driver, which can cause problems.

Quoted 179 lines

Pavel

Quoted 176 lines
---
 drivers/spi/cadence_qspi_apb.c | 122 +++++++++--------------------------------
 1 file changed, 26 insertions(+), 96 deletions(-)
drivers/spi/cadence_qspi_apb.c +26 -96
index 7786dd6..00a50cb 100644
@@ -28,6 +28,7 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/errno.h>
+#include <wait_bit.h>
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US			(1) /* 1us */

@@ -214,32 +215,6 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
 	return;
 }
 
-static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
-	const void *src, unsigned int bytes)
-{
-	unsigned int temp = 0;
-	int i;
-	int remaining = bytes;
-	unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
-	unsigned int *src_ptr = (unsigned int *)src;
-
-	while (remaining >= CQSPI_FIFO_WIDTH) {
-		for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-		src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
-		remaining -= CQSPI_FIFO_WIDTH;
-	}
-	if (remaining) {
-		/* dangling bytes */
-		i = remaining/sizeof(dest_ptr);
-		memcpy(&temp, src_ptr+i, remaining % sizeof(dest_ptr));
-		writel(temp, dest_ptr+i);
-		for (--i; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-	}
-	return;
-}
-
 /* Read from SRAM FIFO with polling SRAM fill level. */
 static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
 			const void *src_addr,  unsigned int num_bytes)

@@ -276,44 +251,6 @@ static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
 	return 0;
 }
 
-/* Write to SRAM FIFO with polling SRAM fill level. */
-static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
-				const void *src_addr, unsigned int num_bytes)
-{
-	const void *reg_base = plat->regbase;
-	void *dest_addr = plat->ahbbase;
-	unsigned int retry = CQSPI_REG_RETRY;
-	unsigned int sram_level;
-	unsigned int wr_bytes;
-	unsigned char *src = (unsigned char *)src_addr;
-	int remaining = num_bytes;
-	unsigned int page_size = plat->page_size;
-	unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
-
-	while (remaining > 0) {
-		retry = CQSPI_REG_RETRY;
-		while (retry--) {
-			sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
-			if (sram_level <= sram_threshold_words)
-				break;
-		}
-		if (!retry) {
-			printf("QSPI: SRAM fill level (0x%08x) not hit lower expected level (0x%08x)",
-			       sram_level, sram_threshold_words);
-			return -1;
-		}
-		/* Write a page or remaining bytes. */
-		wr_bytes = (remaining > page_size) ?
-					page_size : remaining;
-
-		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
-		src += wr_bytes;
-		remaining -= wr_bytes;
-	}
-
-	return 0;
-}
-
 void cadence_qspi_apb_controller_enable(void *reg_base)
 {
 	unsigned int reg;

@@ -810,48 +747,41 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
 }
 
 int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
-	unsigned int txlen, const u8 *txbuf)
+	unsigned int n_tx, const u8 *txbuf)
 {
-	unsigned int reg = 0;
-	unsigned int retry;
+	unsigned int page_size = plat->page_size;
+	unsigned int remaining = n_tx;
+	unsigned int write_bytes;
+	int ret;
 
 	/* Configure the indirect read transfer bytes */
-	writel(txlen, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
+	writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
 
 	/* Start the indirect write transfer */
 	writel(CQSPI_REG_INDIRECTWR_START_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 
-	if (qpsi_write_sram_fifo_push(plat, (const void *)txbuf, txlen))
-		goto failwr;
-
-	/* Wait until last write is completed (FIFO empty) */
-	retry = CQSPI_REG_RETRY;
-	while (retry--) {
-		reg = CQSPI_GET_WR_SRAM_LEVEL(plat->regbase);
-		if (reg == 0)
-			break;
-
-		udelay(1);
-	}
-
-	if (reg != 0) {
-		printf("QSPI: timeout for indirect write\n");
-		goto failwr;
-	}
+	while (remaining > 0) {
+		write_bytes = remaining > page_size ? page_size : remaining;
+		writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+		ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
+				   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
+				   CQSPI_REG_SDRAMLEVEL_WR_LSB, 0, 10, 0);
+		if (ret) {
+			printf("Indirect write timed out (%i)\n", ret);
+			goto failwr;
+		}
 
-	/* Check flash indirect controller status */
-	retry = CQSPI_REG_RETRY;
-	while (retry--) {
-		reg = readl(plat->regbase + CQSPI_REG_INDIRECTWR);
-		if (reg & CQSPI_REG_INDIRECTWR_DONE_MASK)
-			break;
-		udelay(1);
+		txbuf += write_bytes;
+		remaining -= write_bytes;
 	}
 
-	if (!(reg & CQSPI_REG_INDIRECTWR_DONE_MASK)) {
-		printf("QSPI: indirect completion status error with reg 0x%08x\n",
-		       reg);
+	/* Check indirect done status */
+	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTWR,
+			   CQSPI_REG_INDIRECTWR_DONE_MASK, 1, 10, 0);
+	if (ret) {
+		printf("Indirect write completion error (%i)\n", ret);
 		goto failwr;
 	}
 

@@ -864,7 +794,7 @@ failwr:
 	/* Cancel the indirect write */
 	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
-	return -1;
+	return ret;
 }
 
 void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy)
Signature · 2 lines
Best regards,
Marek Vasut