spi: cadence_qspi: optimize & fix indirect rd-writes

PATCH RESEND Superseded Vikas Manocha 5 participants 34 messages Jun 17, 2015 – Aug 13, 2015

Subsystems (2)

Arm (32-bit) SPI

lore

Share & tools

lore

Vikas Manocha

author

2015-06-17 02:14 UTC #0 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

This patchset:
- removes sram polling while reading/writing from flash.
- fixes trigger base & transfer start address register programming. This fix superseeds the previous patch "spi: cadence_qspi: Fix the indirect ahb trigger address setting"
- adds support to get fifo width from device tree

Vikas Manocha (7):
spi: cadence_qspi: remove sram polling from flash read
spi: cadence_qspi: read can be independent of fifo width
spi: cadence_qspi: remove sram polling from flash write
spi: cadence_qspi: move trigger base configuration in init
spi: cadence_qspi: fix indirect read/write start address
spi: cadence_qspi: fix base trigger address & transfer start address
spi: cadence_qspi: get fifo width from device tree

 arch/arm/dts/socfpga.dtsi      |    2 +
 arch/arm/dts/stv0991.dts       |    4 +-
 drivers/spi/cadence_qspi.c     |   14 ++--
 drivers/spi/cadence_qspi.h     |    6 +-
 drivers/spi/cadence_qspi_apb.c |  140 ++++++++++------------------------------
 5 files changed, 50 insertions(+), 116 deletions(-)

Signature · 1 line

1.7.9.5

Vikas Manocha

author

2015-06-17 02:14 UTC #1 lore raw

PATCH RESEND 1/7 spi: cadence_qspi: remove sram polling from flash read

There is no need to check for sram fill level. If sram is empty, cpu will go in the wait state till the time data is available from flash.

Also Relying on SRAM fill level only for deciding when the data should be fetched from the local SRAM is not most efficient approach, particulary if we are working on high data rates.

It should be noticed that after one SRAM word is collected, the information is forwarded into system domain and then synchronized into register domain (apb). If we are using slow APB and AHB clks, SRAM fill level might not be up-to-dated because of latency cycles needed for synchronization. For example in case we are getting SRAM fill level equal to 10 locations but in reality there were 2 another words completed and actual level is 12 but information may not be synchronized yet because of the synchronization latency on APB domain.

Signed-off-by Vikas Manocha <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c |   45 +++++-----------------------------------
 1 file changed, 5 insertions(+), 40 deletions(-)

drivers/spi/cadence_qspi_apb.c +5 ~~-40~~

index 00a115f..8eeb423 100644
@@ -198,7 +198,7 @@ static unsigned int cadence_qspi_apb_cmd2addr(const unsigned char *addr_buf,
 	return addr;
 }
 
-static void cadence_qspi_apb_read_fifo_data(void *dest,
+static int cadence_qspi_apb_read_fifo_data(void *dest,
 	const void *src_ahb_addr, unsigned int bytes)
 {
 	unsigned int temp;

@@ -219,7 +219,7 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
 		dest_ptr++;
 	}
 
-	return;
+	return 0;
 }
 
 static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,

@@ -246,42 +246,6 @@ static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
 	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;
-}
-
 /* 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)

@@ -759,9 +723,10 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 	/* Start the indirect read transfer */
 	writel(CQSPI_REG_INDIRECTRD_START_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
+	udelay(1);
 
-	if (qspi_read_sram_fifo_poll(plat->regbase, (void *)rxbuf,
-				     (const void *)plat->ahbbase, rxlen))
+	if (cadence_qspi_apb_read_fifo_data((void *)rxbuf,
+				(const void *)plat->ahbbase, rxlen))
 		goto failrd;
 
 	/* Check flash indirect controller */

Signature · 1 line

1.7.9.5

Vikas Manocha

author

2015-06-17 02:14 UTC #2 lore raw

PATCH RESEND 2/7 spi: cadence_qspi: read can be independent of fifo width

In reply to Vikas Manocha

Signed-off-by Vikas Manocha <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c |   18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

drivers/spi/cadence_qspi_apb.c +8 ~~-10~~

index 8eeb423..794d51d 100644
@@ -206,18 +206,16 @@ static int cadence_qspi_apb_read_fifo_data(void *dest,
 	unsigned int *dest_ptr = (unsigned int *)dest;
 	unsigned int *src_ptr = (unsigned int *)src_ahb_addr;
 
-	while (remaining > 0) {
-		if (remaining >= CQSPI_FIFO_WIDTH) {
-			*dest_ptr = readl(src_ptr);
-			remaining -= CQSPI_FIFO_WIDTH;
-		} else {
-			/* dangling bytes */
-			temp = readl(src_ptr);
-			memcpy(dest_ptr, &temp, remaining);
-			break;
-		}
+	while (remaining >= sizeof(dest_ptr)) {
+		*dest_ptr = readl(src_ptr);
+		remaining -= sizeof(dest_ptr);
 		dest_ptr++;
 	}
+	if (remaining) {
+		/* dangling bytes */
+		temp = readl(src_ptr);
+		memcpy(dest_ptr, &temp, remaining);
+	}
 
 	return 0;
 }

Signature · 1 line

1.7.9.5

Vikas Manocha

author

2015-06-17 02:14 UTC #3 lore raw

PATCH RESEND 3/7 spi: cadence_qspi: remove sram polling from flash write

In reply to Vikas Manocha

There is no need to poll sram level before writing to flash, data going to SRAM till sram is full, after that backpressure will take over.

Signed-off-by Vikas Manocha <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c |   59 ++++++++++------------------------------
 1 file changed, 15 insertions(+), 44 deletions(-)

drivers/spi/cadence_qspi_apb.c +15 ~~-44~~

index 794d51d..403230a 100644
@@ -220,63 +220,34 @@ static int cadence_qspi_apb_read_fifo_data(void *dest,
 	return 0;
 }
 
-static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
-	const void *src, unsigned int bytes)
-{
-	unsigned int temp;
-	int remaining = bytes;
-	unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
-	unsigned int *src_ptr = (unsigned int *)src;
-
-	while (remaining > 0) {
-		if (remaining >= CQSPI_FIFO_WIDTH) {
-			writel(*src_ptr, dest_ptr);
-			remaining -= sizeof(unsigned int);
-		} else {
-			/* dangling bytes */
-			memcpy(&temp, src_ptr, remaining);
-			writel(temp, dest_ptr);
-			break;
-		}
-		src_ptr++;
-	}
-
-	return;
-}
-
-/* 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;
+	int i = 0;
+	unsigned int *dest_addr = plat->trigger_base;
 	unsigned int wr_bytes;
-	unsigned char *src = (unsigned char *)src_addr;
+	unsigned int *src_ptr = (unsigned int *)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;
+		while (wr_bytes >= CQSPI_FIFO_WIDTH) {
+			for (i = 0; i < CQSPI_FIFO_WIDTH/sizeof(dest_addr); i++)
+				writel(*(src_ptr+i), dest_addr+i);
+			src_ptr += CQSPI_FIFO_WIDTH/sizeof(dest_addr);
+			wr_bytes -= CQSPI_FIFO_WIDTH;
+		}
+		if (wr_bytes) {
+			/* dangling bytes */
+			i = wr_bytes/sizeof(dest_addr);
+			for (i = wr_bytes/sizeof(dest_addr); i >= 0; i--)
+				writel(*(src_ptr+i), dest_addr+i);
+		}
 	}
 
 	return 0;

Signature · 1 line

1.7.9.5

Vikas Manocha

author

2015-06-17 02:14 UTC #4 lore raw

PATCH RESEND 4/7 spi: cadence_qspi: move trigger base configuration in init

In reply to Vikas Manocha

Signed-off-by Vikas Manocha <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/spi/cadence_qspi_apb.c +4 -7

index 403230a..e1e1315 100644
@@ -471,6 +471,10 @@ void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)
 	/* Configure the remap address register, no remap */
 	writel(0, plat->regbase + CQSPI_REG_REMAP);
 
+	/* Setup the indirect trigger address */
+	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
+		plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
+
 	/* Disable all interrupts */
 	writel(0, plat->regbase + CQSPI_REG_IRQMASK);

@@ -629,10 +633,6 @@ int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
 		/* for normal read (only ramtron as of now) */
 		addr_bytes = cmdlen - 1;
 
-	/* Setup the indirect trigger address */
-	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
-	       plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
-
 	/* Configure SRAM partition for read. */
 	writel(CQSPI_REG_SRAM_PARTITION_RD, plat->regbase +
 	       CQSPI_REG_SRAMPARTITION);

@@ -731,9 +731,6 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
 		       cmdlen, (unsigned int)cmdbuf);
 		return -EINVAL;
 	}
-	/* Setup the indirect trigger address */
-	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
-	       plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
 	writel(CQSPI_REG_SRAM_PARTITION_WR,
 	       plat->regbase + CQSPI_REG_SRAMPARTITION);

Signature · 1 line

1.7.9.5

Vikas Manocha

author

2015-06-17 02:14 UTC #5 lore raw

PATCH RESEND 5/7 spi: cadence_qspi: fix indirect read/write start address

In reply to Vikas Manocha

Indirect read/write start addresses are flash start addresses for indirect read or write transfers. These should be absolute flash addresses instead of offsets.

Signed-off-by Vikas Manocha <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/spi/cadence_qspi_apb.c +4 -2

index e1e1315..7be67a9 100644
@@ -647,7 +647,8 @@ int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
 
 	/* Get address */
 	addr_value = cadence_qspi_apb_cmd2addr(&cmdbuf[1], addr_bytes);
-	writel(addr_value, plat->regbase + CQSPI_REG_INDIRECTRDSTARTADDR);
+	writel((u32)plat->ahbbase + addr_value,
+			plat->regbase + CQSPI_REG_INDIRECTRDSTARTADDR);
 
 	/* The remaining lenght is dummy bytes. */
 	dummy_bytes = cmdlen - addr_bytes - 1;

@@ -741,7 +742,8 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
 
 	/* Setup write address. */
 	reg = cadence_qspi_apb_cmd2addr(&cmdbuf[1], addr_bytes);
-	writel(reg, plat->regbase + CQSPI_REG_INDIRECTWRSTARTADDR);
+	writel((u32)plat->ahbbase + reg,
+			plat->regbase + CQSPI_REG_INDIRECTWRSTARTADDR);
 
 	reg = readl(plat->regbase + CQSPI_REG_SIZE);
 	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;

Signature · 1 line

1.7.9.5

Vikas Manocha

author

2015-06-17 02:14 UTC #6 lore raw

PATCH RESEND 6/7 spi: cadence_qspi: fix base trigger address & transfer start address

In reply to Vikas Manocha

This patch is to separate the base trigger from the read/write transfer start addresses.

Base trigger register address (0x1c register) corresponds to the address which should be put on AHB bus to handle indirect transfer triggered before.
To handle indirect transfer we need to issue addresses from (value of 0x1c) to (value of 0x1c) + 15*4 ("4" corresponds to size of SRAM location). There are no obstacles in issuing const address just equal to 0x1c. Important thing to note is that indirect trigger address has nothing in common with your physical or mapped NOR Flash address.

Transfer read/write start addresses (offset 0x68/0x78)should be programmed with the absolute flash address to be read/written.

plat->ahbbase has been renamed to plat->flashbase for clarity. plat->triggerbase is added in device tree for mapped spi flash address.

Signed-off-by Vikas Manocha <[email protected]>

---
 arch/arm/dts/socfpga.dtsi      |    1 +
 arch/arm/dts/stv0991.dts       |    3 ++-
 drivers/spi/cadence_qspi.c     |   13 +++++++------
 drivers/spi/cadence_qspi.h     |    5 +++--
 drivers/spi/cadence_qspi_apb.c |   12 +++++-------
 5 files changed, 18 insertions(+), 16 deletions(-)

arch/arm/dts/socfpga.dtsi +1 -0

index bf791c5..d89c974 100644
@@ -634,6 +634,7 @@
 			#size-cells = <0>;
 			reg = <0xff705000 0x1000>,
 				<0xffa00000 0x1000>;
+				<0x00000000 0x0010>;
 			interrupts = <0 151 4>;
 			clocks = <&qspi_clk>;
 			ext-decoder = <0>;  /* external decoder */

arch/arm/dts/stv0991.dts +2 -1

index 3b1efca..72399ff 100644
@@ -30,7 +30,8 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0x80203000 0x100>,
-				<0x40000000 0x1000000>;
+				<0x40000000 0x1000000>,
+				<0x40000000 0x0000010>;
 			clocks = <3750000>;
 			ext-decoder = <0>; /* external decoder */
 			num-cs = <4>;

drivers/spi/cadence_qspi.c +7 -6

index a75fc46..b23461d 100644
@@ -150,7 +150,7 @@ static int cadence_spi_probe(struct udevice *bus)
 	struct cadence_spi_priv *priv = dev_get_priv(bus);
 
 	priv->regbase = plat->regbase;
-	priv->ahbbase = plat->ahbbase;
+	priv->flashbase = plat->flashbase;
 
 	if (!priv->qspi_is_init) {
 		cadence_qspi_apb_controller_init(plat);

@@ -278,7 +278,7 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	const void *blob = gd->fdt_blob;
 	int node = bus->of_offset;
 	int subnode;
-	u32 data[4];
+	u32 data[6];
 	int ret;
 
 	/* 2 base addresses are needed, lets get them from the DT */

@@ -289,7 +289,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	}
 
 	plat->regbase = (void *)data[0];
-	plat->ahbbase = (void *)data[2];
+	plat->flashbase = (void *)data[2];
+	plat->trigger_base = (void *)data[4];
 
 	/* Use 500KHz as a suitable default */
 	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",

@@ -310,9 +311,9 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	plat->tchsh_ns = fdtdec_get_int(blob, subnode, "tchsh-ns", 20);
 	plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns", 20);
 
-	debug("%s: regbase=%p ahbbase=%p max-frequency=%d page-size=%d\n",
-	      __func__, plat->regbase, plat->ahbbase, plat->max_hz,
-	      plat->page_size);
+	debug("%s: regbase=%p flashbase=%p trigger_base=%p max-frequency=%d \
+		page-size=%d\n", __func__, plat->regbase, plat->flashbase,
+		plat->trigger_base, plat->max_hz, plat->page_size);
 
 	return 0;
 }

drivers/spi/cadence_qspi.h +3 -2

index c9a6142..5ea4581 100644
@@ -17,7 +17,8 @@
 struct cadence_spi_platdata {
 	unsigned int	max_hz;
 	void		*regbase;
-	void		*ahbbase;
+	void		*flashbase;
+	void		*trigger_base;
 
 	u32		page_size;
 	u32		block_size;

@@ -29,7 +30,7 @@ struct cadence_spi_platdata {
 
 struct cadence_spi_priv {
 	void		*regbase;
-	void		*ahbbase;
+	void		*flashbase;
 	size_t		cmd_len;
 	u8		cmd_buf[32];
 	size_t		data_len;

drivers/spi/cadence_qspi_apb.c +5 -7

index 7be67a9..2994158 100644
@@ -50,7 +50,6 @@
 #define CQSPI_INST_TYPE_QUAD			(2)
 
 #define CQSPI_STIG_DATA_LEN_MAX			(8)
-#define CQSPI_INDIRECTTRIGGER_ADDR_MASK		(0xFFFFF)
 
 #define CQSPI_DUMMY_CLKS_PER_BYTE		(8)
 #define CQSPI_DUMMY_BYTES_MAX			(4)

@@ -471,9 +470,8 @@ void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)
 	/* Configure the remap address register, no remap */
 	writel(0, plat->regbase + CQSPI_REG_REMAP);
 
-	/* Setup the indirect trigger address */
-	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
-		plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
+	writel((u32)plat->trigger_base,
+			plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
 	/* Disable all interrupts */
 	writel(0, plat->regbase + CQSPI_REG_IRQMASK);

@@ -647,7 +645,7 @@ int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
 
 	/* Get address */
 	addr_value = cadence_qspi_apb_cmd2addr(&cmdbuf[1], addr_bytes);
-	writel((u32)plat->ahbbase + addr_value,
+	writel((u32)plat->flashbase + addr_value,
 			plat->regbase + CQSPI_REG_INDIRECTRDSTARTADDR);
 
 	/* The remaining lenght is dummy bytes. */

@@ -696,7 +694,7 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 	udelay(1);
 
 	if (cadence_qspi_apb_read_fifo_data((void *)rxbuf,
-				(const void *)plat->ahbbase, rxlen))
+				(const void *)plat->trigger_base, rxlen))
 		goto failrd;
 
 	/* Check flash indirect controller */

@@ -742,7 +740,7 @@ int cadence_qspi_apb_indirect_write_setup(struct cadence_spi_platdata *plat,
 
 	/* Setup write address. */
 	reg = cadence_qspi_apb_cmd2addr(&cmdbuf[1], addr_bytes);
-	writel((u32)plat->ahbbase + reg,
+	writel((u32)plat->flashbase + reg,
 			plat->regbase + CQSPI_REG_INDIRECTWRSTARTADDR);
 
 	reg = readl(plat->regbase + CQSPI_REG_SIZE);

Signature · 1 line

1.7.9.5

Vikas Manocha

author

2015-06-17 02:14 UTC #7 lore raw

PATCH RESEND 7/7 spi: cadence_qspi: get fifo width from device tree

In reply to Vikas Manocha

Fifo width could be different on different socs, e.g. stv0991 & altera soc have different fifo width.

Signed-off-by Vikas Manocha <[email protected]>

---
 arch/arm/dts/socfpga.dtsi      |    1 +
 arch/arm/dts/stv0991.dts       |    1 +
 drivers/spi/cadence_qspi.c     |    1 +
 drivers/spi/cadence_qspi.h     |    1 +
 drivers/spi/cadence_qspi_apb.c |   13 ++++---------
 5 files changed, 8 insertions(+), 9 deletions(-)

arch/arm/dts/socfpga.dtsi +1 -0

index d89c974..a2a2029 100644
@@ -640,6 +640,7 @@
 			ext-decoder = <0>;  /* external decoder */
 			num-cs = <4>;
 			fifo-depth = <128>;
+			fifo-width = <4>;
 			bus-num = <2>;
 			status = "disabled";
 		};

arch/arm/dts/stv0991.dts +1 -0

index 72399ff..433fcd1 100644
@@ -36,6 +36,7 @@
 			ext-decoder = <0>; /* external decoder */
 			num-cs = <4>;
 			fifo-depth = <256>;
+			fifo-width = <8>;
 			bus-num = <0>;
 			status = "okay";

drivers/spi/cadence_qspi.c +1 -0

index b23461d..dfdd9e3 100644
@@ -310,6 +310,7 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	plat->tsd2d_ns = fdtdec_get_int(blob, subnode, "tsd2d-ns", 255);
 	plat->tchsh_ns = fdtdec_get_int(blob, subnode, "tchsh-ns", 20);
 	plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns", 20);
+	plat->fifo_width = fdtdec_get_int(blob, node, "fifo-width", 4);
 
 	debug("%s: regbase=%p flashbase=%p trigger_base=%p max-frequency=%d \
 		page-size=%d\n", __func__, plat->regbase, plat->flashbase,

drivers/spi/cadence_qspi.h +1 -0

index 5ea4581..2be1b43 100644
@@ -26,6 +26,7 @@ struct cadence_spi_platdata {
 	u32		tsd2d_ns;
 	u32		tchsh_ns;
 	u32		tslch_ns;
+	u32		fifo_width;
 };
 
 struct cadence_spi_priv {

drivers/spi/cadence_qspi_apb.c +4 -9

index 2994158..61fc95a 100644
@@ -34,8 +34,6 @@
 #define CQSPI_REG_RETRY				(10000)
 #define CQSPI_POLL_IDLE_RETRY			(3)
 
-#define CQSPI_FIFO_WIDTH			(4)
-
 /* Controller sram size in word */
 #define CQSPI_REG_SRAM_SIZE_WORD		(128)
 #define CQSPI_REG_SRAM_RESV_WORDS		(2)

@@ -54,9 +52,6 @@
 #define CQSPI_DUMMY_CLKS_PER_BYTE		(8)
 #define CQSPI_DUMMY_BYTES_MAX			(4)
 
-
-#define CQSPI_REG_SRAM_FILL_THRESHOLD	\
-	((CQSPI_REG_SRAM_SIZE_WORD / 2) * CQSPI_FIFO_WIDTH)
 /****************************************************************************
  * Controller's configuration and status register (offset from QSPI_BASE)
  ****************************************************************************/

@@ -235,11 +230,11 @@ static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
 					page_size : remaining;
 
 		remaining -= wr_bytes;
-		while (wr_bytes >= CQSPI_FIFO_WIDTH) {
-			for (i = 0; i < CQSPI_FIFO_WIDTH/sizeof(dest_addr); i++)
+		while (wr_bytes >= plat->fifo_width) {
+			for (i = 0; i < plat->fifo_width/sizeof(dest_addr); i++)
 				writel(*(src_ptr+i), dest_addr+i);
-			src_ptr += CQSPI_FIFO_WIDTH/sizeof(dest_addr);
-			wr_bytes -= CQSPI_FIFO_WIDTH;
+			src_ptr += plat->fifo_width/sizeof(dest_addr);
+			wr_bytes -= plat->fifo_width;
 		}
 		if (wr_bytes) {
 			/* dangling bytes */

Signature · 1 line

1.7.9.5

Stefan Roese

2015-06-18 12:02 UTC #8 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

In reply to Vikas Manocha

Hi Vikas,

I finally got to testing your latest patchset. And have some comments / problems:

Quoted 16 lines · Vikas Manocha

This patchset:
- removes sram polling while reading/writing from flash.
- fixes trigger base & transfer start address register programming. This fix superseeds the previous patch "spi: cadence_qspi: Fix the indirect ahb trigger address setting"
- adds support to get fifo width from device tree
Vikas Manocha (7):
spi: cadence_qspi: remove sram polling from flash read
spi: cadence_qspi: read can be independent of fifo width
spi: cadence_qspi: remove sram polling from flash write
spi: cadence_qspi: move trigger base configuration in init
spi: cadence_qspi: fix indirect read/write start address
spi: cadence_qspi: fix base trigger address & transfer start address
spi: cadence_qspi: get fifo width from device tree

Quoted 6 lines

  arch/arm/dts/socfpga.dtsi      |    2 +
  arch/arm/dts/stv0991.dts       |    4 +-
  drivers/spi/cadence_qspi.c     |   14 ++--
  drivers/spi/cadence_qspi.h     |    6 +-
  drivers/spi/cadence_qspi_apb.c |  140 ++++++++++------------------------------
  5 files changed, 50 insertions(+), 116 deletions(-)

With these patches applied, I see this compilation error:

$ make -s -j10
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error
FATAL ERROR: Unable to parse input tree
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [arch/arm/dts/socfpga_arria5_socdk.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socdk.dtb] Error 1
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socrates.dtb] Error 1

The socfpga.dtsi has incorrect syntax. Here a quick fix for this - please add this to your next version. And please also compile-test for e.g. socrates.

$ gd

arch/arm/dts/socfpga.dtsi +2 -2

index a2a2029..448870e 100644
@@ -633,8 +633,8 @@
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <0xff705000 0x1000>,
-                               <0xffa00000 0x1000>;
-                               <0x00000000 0x0010>;
+                             <0xffa00000 0x1000>,
+                             <0x00000000 0x0010>;
                        interrupts = <0 151 4>;
                        clocks = <&qspi_clk>;
                        ext-decoder = <0>;  /* external decoder */

Okay. After installing the resulting image on the SoCrates, I get the following error while reading from SD-card:

=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR => sf read 100000 0 100000
QSPI: indirect completion status error with reg 0x0000000c
SF: 1048576 bytes @ 0x0 Read: ERROR

So there seems to be something breaking the SoCFPGA Cadence QSPI support. Any idea whats going wrong here?

Thanks,
Stefan

Vikas MANOCHA

author

2015-06-18 18:05 UTC #9 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Thanks Stefan,

Quoted 53 lines

-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Thursday, June 18, 2015 5:02 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
I finally got to testing your latest patchset. And have some comments / problems:
Quoted 16 lines · Vikas Manocha
This patchset:
- removes sram polling while reading/writing from flash.
- fixes trigger base & transfer start address register programming. This fix superseeds the previous patch "spi: cadence_qspi: Fix the indirect ahb trigger address setting"
- adds support to get fifo width from device tree
Vikas Manocha (7):
spi: cadence_qspi: remove sram polling from flash read
spi: cadence_qspi: read can be independent of fifo width
spi: cadence_qspi: remove sram polling from flash write
spi: cadence_qspi: move trigger base configuration in init
spi: cadence_qspi: fix indirect read/write start address
spi: cadence_qspi: fix base trigger address & transfer start address
spi: cadence_qspi: get fifo width from device tree
Quoted 5 lines
  arch/arm/dts/socfpga.dtsi      |    2 +
  arch/arm/dts/stv0991.dts       |    4 +-
  drivers/spi/cadence_qspi.c     |   14 ++--
  drivers/spi/cadence_qspi.h     |    6 +-
  drivers/spi/cadence_qspi_apb.c |  140 ++++++++++---------------------------
---
Quoted 1 line
  5 files changed, 50 insertions(+), 116 deletions(-)
With these patches applied, I see this compilation error:
$ make -s -j10
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR: Unable
to parse input tree
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR: Unable
to parse input tree
make[2]: *** [arch/arm/dts/socfpga_arria5_socdk.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socdk.dtb] Error 1
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR: Unable
to parse input tree
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socrates.dtb] Error 1

Quoted 2 lines

The socfpga.dtsi has incorrect syntax. Here a quick fix for this - please add this to your next version. And please also compile-test for e.g. socrates.

You are right, semicolon has to be replaced with comma. I will fix it in next version & do the compile-test also.

Quoted 29 lines

$ gd
arch/arm/dts/socfpga.dtsi index renamed from arch/arm/dts/socfpga.dtsi +0 -0
a2a2029..448870e 100644
arch/arm/dts/socfpga.dtsi +2 -2
@@ -633,8 +633,8 @@
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <0xff705000 0x1000>,
-                               <0xffa00000 0x1000>;
-                               <0x00000000 0x0010>;
+                             <0xffa00000 0x1000>,
+                             <0x00000000 0x0010>;
                        interrupts = <0 151 4>;
                        clocks = <&qspi_clk>;
                        ext-decoder = <0>;  /* external decoder */
Okay. After installing the resulting image on the SoCrates, I get the following error while reading from SD-card:
=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR => sf read 100000 0 100000
QSPI: indirect completion status error with reg 0x0000000c
SF: 1048576 bytes @ 0x0 Read: ERROR

Quoted 2 lines

So there seems to be something breaking the SoCFPGA Cadence QSPI support. Any idea whats going wrong here?

It means indirect read was not successful. Can you please:

- please check if "sf write" is also causing some error or is working fine.
- git bisect or cherry-pick to find out which patch is breaking the read functionality.

Rgds,
Vikas

Quoted 3 lines

Thanks,
Stefan

Stefan Roese

2015-06-19 06:16 UTC #10 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Vikas,

On 18.06.2015 20:05, Vikas MANOCHA wrote:

<snip>

Quoted 12 lines

$ make -s -j10
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR: Unable
to parse input tree
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR: Unable
to parse input tree
make[2]: *** [arch/arm/dts/socfpga_arria5_socdk.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socdk.dtb] Error 1
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR: Unable
to parse input tree
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socrates.dtb] Error 1

Quoted 5 lines

Quoted 2 lines
The socfpga.dtsi has incorrect syntax. Here a quick fix for this - please add this to your next version. And please also compile-test for e.g. socrates.
You are right, semicolon has to be replaced with comma. I will fix it in next version & do the compile-test also.

And please also take care of the correct indentation.

Quoted 29 lines

Quoted 29 lines
$ gd
arch/arm/dts/socfpga.dtsi index renamed from arch/arm/dts/socfpga.dtsi +0 -0
a2a2029..448870e 100644
arch/arm/dts/socfpga.dtsi +2 -2
@@ -633,8 +633,8 @@
                         #address-cells = <1>;
                         #size-cells = <0>;
                         reg = <0xff705000 0x1000>,
-                               <0xffa00000 0x1000>;
-                               <0x00000000 0x0010>;
+                             <0xffa00000 0x1000>,
+                             <0x00000000 0x0010>;
                         interrupts = <0 151 4>;
                         clocks = <&qspi_clk>;
                         ext-decoder = <0>;  /* external decoder */
Okay. After installing the resulting image on the SoCrates, I get the following error while reading from SD-card:
=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR => sf read 100000 0 100000
QSPI: indirect completion status error with reg 0x0000000c
SF: 1048576 bytes @ 0x0 Read: ERROR

Quoted 7 lines

Quoted 2 lines
So there seems to be something breaking the SoCFPGA Cadence QSPI support. Any idea whats going wrong here?
It means indirect read was not successful. Can you please:
- please check if "sf write" is also causing some error or is working fine.

Same error.

Quoted 2 lines

- git bisect or cherry-pick to find out which patch is breaking the read functionality.

This one is the first introducing this breakage:

spi: cadence_qspi: fix base trigger address & transfer start address

Here the output from the complete patchset with DEBUG enabled:

=> sf probe
cadence_spi_ofdata_to_platdata: regbase=ff705000 flashbase=ffa00000 trigger_base=00000000 max-frequency=500000 page-size=256
cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 1000000Hz Div 0xf cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 1000000Hz Div 0xf cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 500000Hz Div 0xf
SF: Read data capture delay calibrated to 7 (0 - 15)
cadence_spi_set_speed: speed=1000000
cadence_spi_xfer: len=1 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
cadence_spi_xfer: len=5 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 1000000Hz Div 0xf
cadence_spi_set_speed: speed=1000000
=> sf read 200000 100000 10000
cadence_spi_xfer: len=5 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
cadence_spi_xfer: len=65536 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
QSPI: indirect completion status error with reg 0x0000000c
SF: 65536 bytes @ 0x100000 Read: ERROR

HTP.

Thanks,
Stefan

Vikas MANOCHA

author

2015-06-19 21:38 UTC #11 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Thanks Stefan,

Quoted 74 lines

-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Thursday, June 18, 2015 11:16 PM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
On 18.06.2015 20:05, Vikas MANOCHA wrote:
<snip>
Quoted 12 lines
Quoted 12 lines
$ make -s -j10
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR:
Unable to parse input tree
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR:
Unable to parse input tree
make[2]: *** [arch/arm/dts/socfpga_arria5_socdk.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socdk.dtb] Error 1
Error: arch/arm/dts/socfpga.dtsi:637.5-6 syntax error FATAL ERROR:
Unable to parse input tree
make[2]: *** [arch/arm/dts/socfpga_cyclone5_socrates.dtb] Error 1
Quoted 2 lines
Quoted 2 lines
The socfpga.dtsi has incorrect syntax. Here a quick fix for this - please add this to your next version. And please also compile-test for e.g.
socrates.
Quoted 3 lines
You are right, semicolon has to be replaced with comma. I will fix it in next version & do the compile-test also.
And please also take care of the correct indentation.
Quoted 30 lines
Quoted 30 lines
$ gd
arch/arm/dts/socfpga.dtsi +2 -2
index a2a2029..448870e 100644
@@ -633,8 +633,8 @@
                         #address-cells = <1>;
                         #size-cells = <0>;
                         reg = <0xff705000 0x1000>,
-                               <0xffa00000 0x1000>;
-                               <0x00000000 0x0010>;
+                             <0xffa00000 0x1000>,
+                             <0x00000000 0x0010>;
                         interrupts = <0 151 4>;
                         clocks = <&qspi_clk>;
                         ext-decoder = <0>;  /* external decoder */
Okay. After installing the resulting image on the SoCrates, I get the following error while reading from SD-card:
=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR => sf read 100000 0 100000
QSPI: indirect completion status error with reg 0x0000000c
SF: 1048576 bytes @ 0x0 Read: ERROR
Quoted 7 lines
Quoted 2 lines
So there seems to be something breaking the SoCFPGA Cadence QSPI support. Any idea whats going wrong here?
It means indirect read was not successful. Can you please:
- please check if "sf write" is also causing some error or is working fine.

Quoted 9 lines

Same error.
Quoted 2 lines
- git bisect or cherry-pick to find out which patch is breaking the read functionality.
This one is the first introducing this breakage:
spi: cadence_qspi: fix base trigger address & transfer start address

Ok, can you confirm applying patches upto (& including) "spi: cadence_qspi: fix indirect read/write start address" , read/write
to flash are working fine.

The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should
also work fine.
Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.

Rgds,
Vikas

Quoted 35 lines

Here the output from the complete patchset with DEBUG enabled:
=> sf probe
cadence_spi_ofdata_to_platdata: regbase=ff705000 flashbase=ffa00000
trigger_base=00000000 max-frequency=500000 page-size=256
cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 1000000Hz Div 0xf
cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 1000000Hz Div 0xf
cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 500000Hz Div 0xf
SF: Read data capture delay calibrated to 7 (0 - 15)
cadence_spi_set_speed: speed=1000000
cadence_spi_xfer: len=1 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
cadence_spi_xfer: len=5 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR
cadence_qspi_apb_config_baudrate_div: ref_clk 400000000Hz sclk 1000000Hz Div 0xf
cadence_spi_set_speed: speed=1000000
=> sf read 200000 100000 10000
cadence_spi_xfer: len=5 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
cadence_spi_xfer: len=65536 [bytes]
cadence_qspi_apb_chipselect : chipselect 0 decode 0
QSPI: indirect completion status error with reg 0x0000000c
SF: 65536 bytes @ 0x100000 Read: ERROR
HTP.
Thanks,
Stefan

Stefan Roese

2015-06-22 08:34 UTC #12 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Vikas,

Quoted 11 lines · Vikas MANOCHA

Quoted 7 lines
Quoted 2 lines
- git bisect or cherry-pick to find out which patch is breaking the read functionality.
This one is the first introducing this breakage:
spi: cadence_qspi: fix base trigger address & transfer start address
Ok, can you confirm applying patches upto (& including) "spi: cadence_qspi: fix indirect read/write start address" , read/write
to flash are working fine.

Please note that with this patch the code does not compile:

drivers/spi/cadence_qspi_apb.c: In function 'qpsi_write_sram_fifo_push':
drivers/spi/cadence_qspi_apb.c:227:32: error: 'struct cadence_spi_platdata' has no member named 'trigger_base'
unsigned int *dest_addr = plat->trigger_base;

I've manually fixed this trivial change (trigger_base -> ahbbase) and tests with these patches applied show some problems with "sf" stability (bit-flips):

=> sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 34.196s, speed 31395 B/s => sf read 500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK
=> cmp.b 400000 500000 100000
byte at 0x0040001e (0x9f) != byte at 0x0050001e (0x8f) Total of 30 byte(s) were the same

This is new - removing all your patches seems to solve this issue again. So there seems to be something wrong with the previous patches as well. here the output with the patches reverted:

=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR
=> sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 35.872s, speed 29929 B/s => sf read 500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK
=> cmp.b 400000 500000 100000
Total of 1048576 byte(s) were the same

Quoted 6 lines

The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should
also work fine.
Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.

No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.

Thanks,
Stefan

Vikas MANOCHA

author

2015-06-22 23:31 UTC #13 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Thanks Stefan,

Quoted 26 lines

-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Monday, June 22, 2015 1:35 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
Quoted 11 lines · Vikas MANOCHA
Quoted 7 lines
Quoted 2 lines
- git bisect or cherry-pick to find out which patch is breaking the read functionality.
This one is the first introducing this breakage:
spi: cadence_qspi: fix base trigger address & transfer start address
Ok, can you confirm applying patches upto (& including) "spi: cadence_qspi: fix indirect read/write start address" , read/write
to flash are working fine.
Please note that with this patch the code does not compile:

Quoted 8 lines

drivers/spi/cadence_qspi_apb.c: In function 'qpsi_write_sram_fifo_push':
drivers/spi/cadence_qspi_apb.c:227:32: error: 'struct cadence_spi_platdata' has no member named 'trigger_base'
unsigned int *dest_addr = plat->trigger_base;
I've manually fixed this trivial change (trigger_base -> ahbbase) and tests with these patches applied show some problems with "sf" stability (bit-flips):

Sorry for this dumb mistake which happened during rebase :-(, I will correct in next patch series.

Quoted 10 lines

=> sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 34.196s, speed 31395 B/s => sf read 500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK
=> cmp.b 400000 500000 100000
byte at 0x0040001e (0x9f) != byte at 0x0050001e (0x8f) Total of 30 byte(s) were the same
This is new - removing all your patches seems to solve this issue again.

Thanks again Stefan for these tests. It is not easy for me to figure out this instability without the board but I don 't see any logical error in the patches. Off-course code is more optimized & fast with these patches. Access timing in the log provided by you also confirms it.

I can suggest following checks on the hardware:

- Figure out the issue is with read or write, which means comparing the flash with ram after read/write.
- Put some random microsecond delays in the read/write to confirm it is timing issue.

Quoted 21 lines

So there seems to be something wrong with the previous patches as well. here the output with the patches reverted:
=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR => sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 35.872s, speed 29929 B/s => sf read 500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK
=> cmp.b 400000 500000 100000
Total of 1048576 byte(s) were the same
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.

Quoted 2 lines

No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.

Ok, I don't know why socfpga is not working with trigger_base to be 0xFFA0_0000.
Normally it should work, Graham also thinks the same, Let's wait for his discussion with the Altera designers.

Rgds,
Vikas

Quoted 3 lines

Thanks,
Stefan

Graham Moore

2015-06-23 14:36 UTC #14 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

On 06/22/2015 06:31 PM, Vikas MANOCHA wrote:
...

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.

Quoted 6 lines

Quoted 2 lines
No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.
Ok, I don't know why socfpga is not working with trigger_base to be 0xFFA0_0000.
Normally it should work, Graham also thinks the same, Let's wait for his discussion with the Altera designers.

Wait a minute, on SoCFPGA, the flashbase is 0xffa00000, and the trigger base is 0x00000000. The point of having a different address was that they needed to be different on SoCFPGA, right?

As for why they're different, the Altera Cyclone5 SoCFPGA has a complex multilevel interconnect where the QSPI is three levels down, and is the only slave of an AHB master. At that level of the interconnect, the base address has long been stripped off, it was used to select down to the final master. The QSPI is the only thing on that AHB bus, so its address is zero. Or at least that's how I understand it.

-Graham

Vikas MANOCHA

author

2015-06-23 14:51 UTC #15 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Graham,

Quoted 32 lines

-----Original Message-----
From: Graham Moore [mailto:grmoore at opensource.altera.com]
Sent: Tuesday, June 23, 2015 7:37 AM
To: Vikas MANOCHA
Cc: Stefan Roese; u-boot at lists.denx.de; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
On 06/22/2015 06:31 PM, Vikas MANOCHA wrote:
...
Quoted 11 lines
Quoted 9 lines
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.
No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.
Ok, I don't know why socfpga is not working with trigger_base to be
0xFFA0_0000.
Quoted 1 line
Normally it should work, Graham also thinks the same, Let's wait for his
discussion with the Altera designers.
Quoted 1 line
Wait a minute, on SoCFPGA, the flashbase is 0xffa00000, and the trigger base is 0x00000000. The point of having a different address was that they needed to be different on SoCFPGA, right?

Quoted 6 lines

As for why they're different, the Altera Cyclone5 SoCFPGA has a complex multilevel interconnect where the QSPI is three levels down, and is the only slave of an AHB master. At that level of the interconnect, the base address has long been stripped off, it was used to select down to the final master. The QSPI is the only thing on that AHB bus, so its address is zero. Or at least that's how I understand it.

Hmm ok, let us keep trigger base as 0x0 for SofFPGA cyclone5.

Rgds,
Vikas

Quoted 2 lines

-Graham

Vikas MANOCHA

author

2015-07-02 17:50 UTC #16 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

In reply to Graham Moore

Hi Graham,

Quoted 32 lines

-----Original Message-----
From: Graham Moore [mailto:grmoore at opensource.altera.com]
Sent: Tuesday, June 23, 2015 7:37 AM
To: Vikas MANOCHA
Cc: Stefan Roese; u-boot at lists.denx.de; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
On 06/22/2015 06:31 PM, Vikas MANOCHA wrote:
...
Quoted 11 lines
Quoted 9 lines
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.
No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.
Ok, I don't know why socfpga is not working with trigger_base to be
0xFFA0_0000.
Quoted 1 line
Normally it should work, Graham also thinks the same, Let's wait for his
discussion with the Altera designers.
Quoted 1 line
Wait a minute, on SoCFPGA, the flashbase is 0xffa00000, and the trigger base is 0x00000000. The point of having a different address was that they needed to be different on SoCFPGA, right?

Quoted 6 lines

As for why they're different, the Altera Cyclone5 SoCFPGA has a complex multilevel interconnect where the QSPI is three levels down, and is the only slave of an AHB master. At that level of the interconnect, the base address has long been stripped off, it was used to select down to the final master. The QSPI is the only thing on that AHB bus, so its address is zero. Or at least that's how I understand it.

If we check the code for reading/writing to the FIFO/SRAM of the controller, complete base address is being used.
Which means CPU is using address 0xFFA0_0000, so does not seems like base address is stripped off. It is only for the trigger address which needs programming to 0x0 ?

Regardless, it is something specific to socfpga architecture & should not be part of cadence qspi driver.

Rgds,
Vikas

Quoted 2 lines

-Graham

Graham Moore

2015-07-06 17:56 UTC #17 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Quoted 41 lines · Vikas MANOCHA

Hi Graham,
Quoted 32 lines
-----Original Message-----
From: Graham Moore [mailto:grmoore at opensource.altera.com]
Sent: Tuesday, June 23, 2015 7:37 AM
To: Vikas MANOCHA
Cc: Stefan Roese; u-boot at lists.denx.de; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
On 06/22/2015 06:31 PM, Vikas MANOCHA wrote:
...
Quoted 11 lines
Quoted 9 lines
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.
No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.
Ok, I don't know why socfpga is not working with trigger_base to be
0xFFA0_0000.
Quoted 1 line
Normally it should work, Graham also thinks the same, Let's wait for his
discussion with the Altera designers.
Quoted 1 line
Wait a minute, on SoCFPGA, the flashbase is 0xffa00000, and the trigger base is 0x00000000. The point of having a different address was that they needed to be different on SoCFPGA, right?
Quoted 6 lines
As for why they're different, the Altera Cyclone5 SoCFPGA has a complex multilevel interconnect where the QSPI is three levels down, and is the only slave of an AHB master. At that level of the interconnect, the base address has long been stripped off, it was used to select down to the final master. The QSPI is the only thing on that AHB bus, so its address is zero. Or at least that's how I understand it.

Quoted 5 lines

If we check the code for reading/writing to the FIFO/SRAM of the controller, complete base address is being used.
Which means CPU is using address 0xFFA0_0000, so does not seems like base address is stripped off. It is only for the trigger address which needs programming to 0x0 ?
Regardless, it is something specific to socfpga architecture & should not be part of cadence qspi driver.

I'm sorry, I don't understand your comment. What is 'it' that should not be part of the cadence qspi driver?

-Graham

Quoted 5 lines

Rgds,
Vikas
Quoted 2 lines
-Graham

Vikas MANOCHA

author

2015-07-06 18:19 UTC #18 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Graham,

Quoted 60 lines

-----Original Message-----
From: Graham Moore [mailto:grmoore at opensource.altera.com]
Sent: Monday, July 06, 2015 10:57 AM
To: Vikas MANOCHA
Cc: Stefan Roese; u-boot at lists.denx.de; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Quoted 20 lines · Vikas MANOCHA
Hi Graham,
Quoted 18 lines
-----Original Message-----
From: Graham Moore [mailto:grmoore at opensource.altera.com]
Sent: Tuesday, June 23, 2015 7:37 AM
To: Vikas MANOCHA
Cc: Stefan Roese; u-boot at lists.denx.de;
dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
On 06/22/2015 06:31 PM, Vikas MANOCHA wrote:
...
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving
same.
Quoted 11 lines
Quoted 11 lines
Quoted 5 lines
Quoted 3 lines
No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.
Ok, I don't know why socfpga is not working with trigger_base to be
0xFFA0_0000.
Quoted 2 lines
Normally it should work, Graham also thinks the same, Let's wait for his
discussion with the Altera designers.
Quoted 1 line
Quoted 8 lines
Quoted 8 lines
Wait a minute, on SoCFPGA, the flashbase is 0xffa00000, and the trigger base is 0x00000000. The point of having a different address was that they needed to be different on SoCFPGA, right?
As for why they're different, the Altera Cyclone5 SoCFPGA has a complex multilevel interconnect where the QSPI is three levels down, and is the only slave of an AHB master. At that level of the interconnect, the base address has long been stripped off, it was used to
select down to the final master.
Quoted 4 lines
Quoted 2 lines
The QSPI is the only thing on that AHB bus, so its address is zero. Or at least that's how I understand it.
If we check the code for reading/writing to the FIFO/SRAM of the
controller, complete base address is being used.
Quoted 1 line
Which means CPU is using address 0xFFA0_0000, so does not seems like
base address is stripped off. It is only for the trigger address which needs programming to 0x0 ?
Quoted 1 line

Quoted 6 lines

Quoted 1 line
Regardless, it is something specific to socfpga architecture & should not be
part of cadence qspi driver.
Quoted 1 line
I'm sorry, I don't understand your comment. What is 'it' that should not be part of the cadence qspi driver?

I meant : Trigger base address programming to one address (which is 0x0 in case of socfpga) & then reading/writing from
another locations (NOR location 0xFFA0_0000 in case of socfpga) : It shouldn't be like this in the driver as per the definition of trigger base address.

Trigger base address is the location which should be used to read/write in indirect mode. It actually makes it like reading sram & hence called indirect mode.

It is what was explained & implemented in attached patch. Stefan tested it & it didn't work on socfpga platform, there seems something specific to arch, to be debugged.

Rgds,
Vikas

Quoted 8 lines

-Graham
Quoted 5 lines
Rgds,
Vikas
Quoted 2 lines
-Graham

-------------- next part --------------
An embedded message was scrubbed...
From: Vikas MANOCHA <[email protected]>
Subject: [PATCH RESEND 6/7] spi: cadence_qspi: fix base trigger address & transfer start address
Date: Wed, 17 Jun 2015 04:14:24 +0200
Size: 8985
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150706/5710eca7/attachment.mht>

Vikas MANOCHA

author

2015-07-01 16:24 UTC #19 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

In reply to Stefan Roese

Hi Stefan,

Quoted 69 lines

-----Original Message-----
From: Vikas MANOCHA
Sent: Monday, June 22, 2015 4:31 PM
To: 'Stefan Roese'
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Thanks Stefan,
Quoted 21 lines
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Monday, June 22, 2015 1:35 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
Quoted 9 lines · Vikas MANOCHA
Quoted 8 lines
Quoted 2 lines
- git bisect or cherry-pick to find out which patch is breaking the read functionality.
This one is the first introducing this breakage:
spi: cadence_qspi: fix base trigger address & transfer start address
Quoted 8 lines
Quoted 3 lines
Ok, can you confirm applying patches upto (& including) "spi: cadence_qspi: fix indirect read/write start address" , read/write
to flash are working fine.
Please note that with this patch the code does not compile:
drivers/spi/cadence_qspi_apb.c: In function 'qpsi_write_sram_fifo_push':
drivers/spi/cadence_qspi_apb.c:227:32: error: 'struct
cadence_spi_platdata'
Quoted 7 lines
has no member named 'trigger_base'
unsigned int *dest_addr = plat->trigger_base;
I've manually fixed this trivial change (trigger_base -> ahbbase) and tests with these patches applied show some problems with "sf" stability
(bit-flips):
Sorry for this dumb mistake which happened during rebase :-(, I will correct in next patch series.
Quoted 11 lines
=> sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 34.196s, speed 31395 B/s => sf read
500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK
=> cmp.b 400000 500000 100000
byte at 0x0040001e (0x9f) != byte at 0x0050001e (0x8f) Total of 30 byte(s) were the same
This is new - removing all your patches seems to solve this issue again.
Thanks again Stefan for these tests. It is not easy for me to figure out this instability without the board but I don 't see any logical error in the patches. Off-course code is more optimized & fast with these patches. Access timing in the log provided by you also confirms it.

Quoted 6 lines

I can suggest following checks on the hardware:
- Figure out the issue is with read or write, which means comparing the flash with ram after read/write.
- Put some random microsecond delays in the read/write to confirm it is timing issue.

Any update on it ?

Rgds,
Vikas

Quoted 37 lines

Quoted 23 lines
So there seems to be something wrong with the previous patches as well. here the output with the patches reverted:
=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total 32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR => sf update 400000 100000 100000 1048576 bytes written, 0 bytes skipped in 35.872s, speed 29929 B/s => sf read
500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK
=> cmp.b 400000 500000 100000
Total of 1048576 byte(s) were the same
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.
Quoted 2 lines
No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.
Ok, I don't know why socfpga is not working with trigger_base to be 0xFFA0_0000.
Normally it should work, Graham also thinks the same, Let's wait for his discussion with the Altera designers.
Rgds,
Vikas
Quoted 3 lines
Thanks,
Stefan

Vikas MANOCHA

author

2015-07-09 01:29 UTC #20 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

In reply to Stefan Roese

Hi Stefan,

Quoted 81 lines

-----Original Message-----
From: Vikas MANOCHA
Sent: Wednesday, July 01, 2015 9:25 AM
To: 'Stefan Roese'
Cc: 'u-boot at lists.denx.de'; 'grmoore at opensource.altera.com'; 'dinguyen at opensource.altera.com'; 'jteki at openedev.com'
Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Stefan,
Quoted 9 lines
-----Original Message-----
From: Vikas MANOCHA
Sent: Monday, June 22, 2015 4:31 PM
To: 'Stefan Roese'
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Quoted 30 lines
Thanks Stefan,
Quoted 28 lines
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Monday, June 22, 2015 1:35 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
Quoted 5 lines · Vikas MANOCHA
Quoted 5 lines
Quoted 2 lines
- git bisect or cherry-pick to find out which patch is breaking the read functionality.
This one is the first introducing this breakage:
Quoted 7 lines
Quoted 3 lines
spi: cadence_qspi: fix base trigger address & transfer start address
Ok, can you confirm applying patches upto (& including) "spi: cadence_qspi: fix indirect read/write start address" , read/write
to flash are working fine.
Please note that with this patch the code does not compile:
drivers/spi/cadence_qspi_apb.c: In function
'qpsi_write_sram_fifo_push':
Quoted 22 lines
Quoted 1 line
drivers/spi/cadence_qspi_apb.c:227:32: error: 'struct
cadence_spi_platdata'
Quoted 7 lines
has no member named 'trigger_base'
unsigned int *dest_addr = plat->trigger_base;
I've manually fixed this trivial change (trigger_base -> ahbbase) and tests with these patches applied show some problems with "sf" stability
(bit-flips):
Sorry for this dumb mistake which happened during rebase :-(, I will correct in next patch series.
Quoted 9 lines
=> sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 34.196s, speed 31395 B/s => sf read
500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK => cmp.b 400000 500000 100000 byte at 0x0040001e (0x9f) != byte at 0x0050001e (0x8f) Total of 30 byte(s) were the same
Quoted 4 lines
Quoted 1 line
This is new - removing all your patches seems to solve this issue again.
Thanks again Stefan for these tests. It is not easy for me to figure out this instability without the board but I don 't see any logical error in the
patches.
Quoted 3 lines
Off-course code is more optimized & fast with these patches. Access timing in the log provided by you also confirms it.

Quoted 8 lines

Quoted 6 lines
I can suggest following checks on the hardware:
- Figure out the issue is with read or write, which means comparing the flash with ram after read/write.
- Put some random microsecond delays in the read/write to confirm it is timing issue.
Any update on it ?

In addition can you please check the patch causing this instability on socfpga. I don't like to bug you but to close this patchset, this info & tests mentioned above seems to be required.

I am ready to check it but would need the hardware platform.

Rgds,
Vikas

Quoted 42 lines

Rgds,
Vikas
Quoted 2 lines
Quoted 2 lines
So there seems to be something wrong with the previous patches as
well.
Quoted 35 lines
Quoted 21 lines
here the output with the patches reverted:
=> sf probe
SF: Detected N25Q256 with page size 256 Bytes, erase size 4 KiB, total
32 MiB
SF: Warning - Only lower 16MiB accessible, Full access #define CONFIG_SPI_FLASH_BAR => sf update 400000 100000 100000 1048576 bytes written, 0 bytes skipped in 35.872s, speed 29929 B/s => sf read
500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK => cmp.b 400000 500000 100000 Total of 1048576 byte(s) were the same
Quoted 6 lines
The point is if after applying above mentioned patch (...: fix indirect read/write start address), Read/write are working fine, then trigger_base value of 0xFFA00_0000 should also work fine. Can you please modify the trigger_base value from 0x0 to 0xFFA0_0000 in Socfpga.dtsi & check.
If it works, it would mean both (socfpga & stv0991) are behaving same.
Quoted 2 lines
No. With this change, the "sf read" command crashes / hangs on the SoCFPGA board.
Ok, I don't know why socfpga is not working with trigger_base to be 0xFFA0_0000.
Normally it should work, Graham also thinks the same, Let's wait for his discussion with the Altera designers.
Rgds,
Vikas
Quoted 3 lines
Thanks,
Stefan

Stefan Roese

2015-07-13 09:00 UTC #21 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Vikas,

Quoted 88 lines · Vikas MANOCHA

Quoted 88 lines
-----Original Message-----
From: Vikas MANOCHA
Sent: Wednesday, July 01, 2015 9:25 AM
To: 'Stefan Roese'
Cc: 'u-boot at lists.denx.de'; 'grmoore at opensource.altera.com'; 'dinguyen at opensource.altera.com'; 'jteki at openedev.com'
Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Stefan,
Quoted 9 lines
-----Original Message-----
From: Vikas MANOCHA
Sent: Monday, June 22, 2015 4:31 PM
To: 'Stefan Roese'
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Quoted 30 lines
Thanks Stefan,
Quoted 28 lines
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Monday, June 22, 2015 1:35 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
Quoted 5 lines · Vikas MANOCHA
Quoted 5 lines
Quoted 2 lines
- git bisect or cherry-pick to find out which patch is breaking the read functionality.
This one is the first introducing this breakage:
Quoted 7 lines
Quoted 3 lines
spi: cadence_qspi: fix base trigger address & transfer start address
Ok, can you confirm applying patches upto (& including) "spi: cadence_qspi: fix indirect read/write start address" , read/write
to flash are working fine.
Please note that with this patch the code does not compile:
drivers/spi/cadence_qspi_apb.c: In function
'qpsi_write_sram_fifo_push':
Quoted 22 lines
Quoted 1 line
drivers/spi/cadence_qspi_apb.c:227:32: error: 'struct
cadence_spi_platdata'
Quoted 7 lines
has no member named 'trigger_base'
    unsigned int *dest_addr = plat->trigger_base;
I've manually fixed this trivial change (trigger_base -> ahbbase) and tests with these patches applied show some problems with "sf" stability
(bit-flips):
Sorry for this dumb mistake which happened during rebase :-(, I will correct in next patch series.
Quoted 9 lines
=> sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 34.196s, speed 31395 B/s => sf read
500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK => cmp.b 400000 500000 100000 byte at 0x0040001e (0x9f) != byte at 0x0050001e (0x8f) Total of 30 byte(s) were the same
Quoted 4 lines
Quoted 1 line
This is new - removing all your patches seems to solve this issue again.
Thanks again Stefan for these tests. It is not easy for me to figure out this instability without the board but I don 't see any logical error in the
patches.
Quoted 9 lines
Off-course code is more optimized & fast with these patches. Access timing in the log provided by you also confirms it.
I can suggest following checks on the hardware:
- Figure out the issue is with read or write, which means comparing the flash with ram after read/write.
- Put some random microsecond delays in the read/write to confirm it is timing issue.

Quoted 6 lines

Quoted 1 line
Any update on it ?
In addition can you please check the patch causing this instability on socfpga. I don't like to bug you but
to close this patchset, this info & tests mentioned above seems to be required.

Okay. I'll try to find some time this week to do some testing here. It seems that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into mainline yet. To make it easier for me, could you perhaps publish a git repository that is based on current mainline. And has the mentioned above patch series included. And all the patches in the latest version that are currently causing these problems on SoCFPGA?

Quoted 1 line

I am ready to check it but would need the hardware platform.

Understood.

Thanks,
Stefan

Vikas MANOCHA

author

2015-07-15 21:14 UTC #22 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Stefan,

Quoted 108 lines

-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Monday, July 13, 2015 2:01 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
Quoted 65 lines · Vikas MANOCHA
Quoted 65 lines
-----Original Message-----
From: Vikas MANOCHA
Sent: Wednesday, July 01, 2015 9:25 AM
To: 'Stefan Roese'
Cc: 'u-boot at lists.denx.de'; 'grmoore at opensource.altera.com'; 'dinguyen at opensource.altera.com'; 'jteki at openedev.com'
Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Stefan,
Quoted 9 lines
-----Original Message-----
From: Vikas MANOCHA
Sent: Monday, June 22, 2015 4:31 PM
To: 'Stefan Roese'
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: RE: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Quoted 30 lines
Thanks Stefan,
Quoted 28 lines
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: Monday, June 22, 2015 1:35 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; grmoore at opensource.altera.com; dinguyen at opensource.altera.com; jteki at openedev.com
Subject: Re: [PATCH RESEND 0/7] spi: cadence_qspi: optimize & fix indirect rd-writes
Hi Vikas,
Quoted 5 lines · Vikas MANOCHA
Quoted 5 lines
Quoted 2 lines
- git bisect or cherry-pick to find out which patch is breaking the read functionality.
This one is the first introducing this breakage:
Quoted 7 lines
Quoted 3 lines
spi: cadence_qspi: fix base trigger address & transfer start address
Ok, can you confirm applying patches upto (& including) "spi: cadence_qspi: fix indirect read/write start address" , read/write
to flash are working fine.
Please note that with this patch the code does not compile:
drivers/spi/cadence_qspi_apb.c: In function
'qpsi_write_sram_fifo_push':
Quoted 14 lines
Quoted 1 line
drivers/spi/cadence_qspi_apb.c:227:32: error: 'struct
cadence_spi_platdata'
Quoted 7 lines
has no member named 'trigger_base'
    unsigned int *dest_addr = plat->trigger_base;
I've manually fixed this trivial change (trigger_base -> ahbbase) and tests with these patches applied show some problems with "sf" stability
(bit-flips):
Sorry for this dumb mistake which happened during rebase :-(, I will correct in next patch series.
Quoted 1 line
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
=> sf update 400000 100000 100000
1048576 bytes written, 0 bytes skipped in 34.196s, speed 31395 B/s => sf read
500000 100000 100000
SF: 1048576 bytes @ 0x100000 Read: OK => cmp.b 400000 500000
100000
Quoted 19 lines
Quoted 19 lines
Quoted 8 lines
Quoted 4 lines
byte at 0x0040001e (0x9f) != byte at 0x0050001e (0x8f) Total of 30 byte(s) were the same
This is new - removing all your patches seems to solve this issue again.
Thanks again Stefan for these tests. It is not easy for me to figure out this instability without the board but I don 't see any logical error in the
patches.
Quoted 9 lines
Off-course code is more optimized & fast with these patches. Access timing in the log provided by you also confirms it.
I can suggest following checks on the hardware:
- Figure out the issue is with read or write, which means comparing the flash with ram after read/write.
- Put some random microsecond delays in the read/write to confirm it is timing issue.
Quoted 5 lines
Quoted 1 line
Any update on it ?
In addition can you please check the patch causing this instability on socfpga. I don't like to bug you but to close this patchset, this info & tests mentioned above seems to be required.

Quoted 7 lines

Okay. I'll try to find some time this week to do some testing here. It seems that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into mainline yet. To make it easier for me, could you perhaps publish a git repository that is based on current mainline. And has the mentioned above patch series included. And all the patches in the latest version that are currently causing these problems on SoCFPGA?

The patchset was in u-boot-spi repository, yesterday pulled by Tom in mainline. I will rebase the patchset in discussion (spi: cadence_qspi: optimize & fix indirect rd-writes) on mainline master & send the V2.

Let me know if it is ok.

Rgds,
Vikas

Quoted 7 lines

Quoted 2 lines
I am ready to check it but would need the hardware platform.
Understood.
Thanks,
Stefan

Stefan Roese

2015-07-16 06:46 UTC #23 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Vikas,

On 15.07.2015 23:14, Vikas MANOCHA wrote:

<snip>

Quoted 12 lines

Quoted 11 lines
Quoted 3 lines
In addition can you please check the patch causing this instability on socfpga. I don't like to bug you but to close this patchset, this info & tests mentioned above seems to be required.
Okay. I'll try to find some time this week to do some testing here. It seems that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into mainline yet. To make it easier for me, could you perhaps publish a git repository that is based on current mainline. And has the mentioned above patch series included. And all the patches in the latest version that are currently causing these problems on SoCFPGA?

Quoted 4 lines

The patchset was in u-boot-spi repository, yesterday pulled by Tom in mainline. I will rebase the patchset in discussion (spi: cadence_qspi: optimize & fix indirect rd-writes) on mainline master & send the V2.
Let me know if it is ok.

Okay, I'll try to find some time later this week or next week for some tests.

Thanks,
Stefan

Stefan Roese

2015-07-23 12:22 UTC #24 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Vikas,

Quoted 21 lines · Stefan Roese

Quoted 21 lines
Quoted 4 lines
Quoted 3 lines
In addition can you please check the patch causing this instability on socfpga. I don't like to bug you but to close this patchset, this info & tests mentioned above seems to be required.
Quoted 11 lines
Okay. I'll try to find some time this week to do some testing here. It seems
that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into
mainline yet. To make it easier for me, could you perhaps publish a git repository that is based on current mainline. And has the mentioned above
patch series included. And all the patches in the latest version that are
currently causing these problems on SoCFPGA?
The patchset was in u-boot-spi repository, yesterday pulled by Tom in mainline. I will rebase the patchset in discussion
(spi: cadence_qspi: optimize & fix indirect rd-writes) on mainline master & send the V2.

Quoted 4 lines

Quoted 1 line
Let me know if it is ok.
Okay, I'll try to find some time later this week or next week for some tests.

I couldn't find the required time to test these patches again yet. Sorry. And I'll leave for a short vacation tomorrow. So I won't be able to get to this issue before week 32.

Perhaps somebody else finds some time to look at this QSPI driver issue in the meantime

Thanks,
Stefan

vikasm

author

2015-08-11 21:19 UTC #25 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Stefan,

Quoted 29 lines · Stefan Roese

Hi Vikas,
Quoted 21 lines · Stefan Roese
Quoted 21 lines
Quoted 4 lines
Quoted 3 lines
In addition can you please check the patch causing this instability on socfpga. I don't like to bug you but to close this patchset, this info & tests mentioned above seems to be required.
Quoted 11 lines
Okay. I'll try to find some time this week to do some testing here. It seems
that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into
mainline yet. To make it easier for me, could you perhaps publish a git repository that is based on current mainline. And has the mentioned above
patch series included. And all the patches in the latest version that are
currently causing these problems on SoCFPGA?
The patchset was in u-boot-spi repository, yesterday pulled by Tom in mainline. I will rebase the patchset in discussion
(spi: cadence_qspi: optimize & fix indirect rd-writes) on mainline master & send the V2.
Quoted 4 lines
Quoted 1 line
Let me know if it is ok.
Okay, I'll try to find some time later this week or next week for some tests.

Quoted 6 lines

I couldn't find the required time to test these patches again yet. Sorry. And I'll leave for a short vacation tomorrow. So I won't be able to get to this issue before week 32.
Perhaps somebody else finds some time to look at this QSPI driver issue in the meantime

Any update ?

Cheers,
Vikas

Quoted 6 lines

Thanks,
Stefan
.

Stefan Roese

2015-08-12 11:36 UTC #26 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Vikas,

(added Marek to Cc)

Quoted 30 lines · vikasm

Quoted 29 lines · Stefan Roese
Hi Vikas,
Quoted 25 lines · Stefan Roese
Quoted 16 lines
Quoted 4 lines
Quoted 3 lines
In addition can you please check the patch causing this instability on socfpga. I don't like to bug you but to close this patchset, this info & tests mentioned above seems to be required.
Quoted 11 lines
Okay. I'll try to find some time this week to do some testing here. It seems
that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into
mainline yet. To make it easier for me, could you perhaps publish a git repository that is based on current mainline. And has the mentioned above
patch series included. And all the patches in the latest version that are
currently causing these problems on SoCFPGA?
Quoted 6 lines
The patchset was in u-boot-spi repository, yesterday pulled by Tom in mainline. I will rebase the patchset in discussion
(spi: cadence_qspi: optimize & fix indirect rd-writes) on mainline master & send the V2.
Let me know if it is ok.
Okay, I'll try to find some time later this week or next week for some tests.

Quoted 8 lines

Quoted 6 lines
I couldn't find the required time to test these patches again yet. Sorry. And I'll leave for a short vacation tomorrow. So I won't be able to get to this issue before week 32.
Perhaps somebody else finds some time to look at this QSPI driver issue in the meantime
Any update ?

Not from me, sorry. And I really won't be able to dig into this in the next weeks.

Again, perhaps someone else finds the time to look at this QSPI driver issue in the meantime?

Thanks,
Stefan

Jagan Teki

2015-08-12 12:01 UTC #27 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Vikas,

Did you verified on board, can you just verified with 'sf update' before and after.. I just wanted to see if you get any performance improvement with these
optimization fixes.

Will get back again for my comments.

Quoted 54 lines · Stefan Roese

Hi Vikas,
(added Marek to Cc)
Quoted 48 lines · vikasm
Quoted 44 lines · Stefan Roese
Hi Vikas,
Quoted 27 lines · Stefan Roese
Quoted 27 lines
Quoted 8 lines
Quoted 6 lines
In addition can you please check the patch causing this instability on
socfpga. I don't like to bug you but to close this patchset, this info
& tests mentioned above seems to be required.
Quoted 12 lines
Okay. I'll try to find some time this week to do some testing here. It seems
that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into
mainline yet. To make it easier for me, could you perhaps publish a git
repository that is based on current mainline. And has the mentioned above
patch series included. And all the patches in the latest version that are
currently causing these problems on SoCFPGA?
The patchset was in u-boot-spi repository, yesterday pulled by Tom in mainline. I will rebase the patchset in discussion
(spi: cadence_qspi: optimize & fix indirect rd-writes) on mainline master & send the V2.
Quoted 5 lines
Quoted 1 line
Let me know if it is ok.
Okay, I'll try to find some time later this week or next week for some tests.
I couldn't find the required time to test these patches again yet. Sorry. And I'll leave for a short vacation tomorrow. So I won't be able to get to this issue before week 32.
Perhaps somebody else finds some time to look at this QSPI driver issue in the meantime

Quoted 8 lines

Quoted 1 line
Any update ?
Not from me, sorry. And I really won't be able to dig into this in the next weeks.
Again, perhaps someone else finds the time to look at this QSPI driver issue in the meantime?

thanks!

Signature · 1 line

Jagan | openedev.

vikasm

author

2015-08-12 17:52 UTC #28 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Thanks Jagan,

Quoted 6 lines · Jagan Teki

Vikas,
Did you verified on board, can you just verified with 'sf update' before and after.. I just wanted to see if you get any performance improvement with these
optimization fixes.

The main point is someone needs to debug the patchset on socfpga platform. It works on stv0991 platform. But which patch is really creating issue on socfpga & debug that patch.

Rgds,
Vikas

Quoted 69 lines

Will get back again for my comments.
Quoted 54 lines · Stefan Roese
Hi Vikas,
(added Marek to Cc)
Quoted 48 lines · vikasm
Quoted 44 lines · Stefan Roese
Hi Vikas,
Quoted 27 lines · Stefan Roese
Quoted 27 lines
Quoted 8 lines
Quoted 6 lines
In addition can you please check the patch causing this instability on
socfpga. I don't like to bug you but to close this patchset, this info
& tests mentioned above seems to be required.
Quoted 12 lines
Okay. I'll try to find some time this week to do some testing here. It seems
that the other cadence patchset from you ([v4 00/10] spi:
cadence_qspi: sram depth from DT & fix for FIFO width) is not pulled into
mainline yet. To make it easier for me, could you perhaps publish a git
repository that is based on current mainline. And has the mentioned above
patch series included. And all the patches in the latest version that are
currently causing these problems on SoCFPGA?
The patchset was in u-boot-spi repository, yesterday pulled by Tom in mainline. I will rebase the patchset in discussion
(spi: cadence_qspi: optimize & fix indirect rd-writes) on mainline master & send the V2.
Quoted 5 lines
Quoted 1 line
Let me know if it is ok.
Okay, I'll try to find some time later this week or next week for some tests.
I couldn't find the required time to test these patches again yet. Sorry. And I'll leave for a short vacation tomorrow. So I won't be able to get to this issue before week 32.
Perhaps somebody else finds some time to look at this QSPI driver issue in the meantime
Quoted 8 lines
Quoted 1 line
Any update ?
Not from me, sorry. And I really won't be able to dig into this in the next weeks.
Again, perhaps someone else finds the time to look at this QSPI driver issue in the meantime?
thanks!

Marek Vasut

2015-08-12 20:22 UTC #29 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Quoted 5 lines · vikasm

Thanks Jagan,
Quoted 2 lines · Jagan Teki
Vikas,

Quoted 8 lines

Quoted 4 lines
Did you verified on board, can you just verified with 'sf update' before and after.. I just wanted to see if you get any performance improvement with these
optimization fixes.
The main point is someone needs to debug the patchset on socfpga platform. It works on stv0991 platform. But which patch is really creating issue on socfpga & debug that patch.

I'll happily help, can you please rebase it on top of u-boot/master and repost? I can't seem to be able to apply it. Also, please keep me in Cc.

Thanks

Best regards,
Marek Vasut

vikasm

author

2015-08-13 00:16 UTC #30 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Marek,

Quoted 11 lines · Marek Vasut

Quoted 10 lines · vikasm
Thanks Jagan,
Quoted 6 lines · Jagan Teki
Vikas,
Did you verified on board, can you just verified with 'sf update' before and after.. I just wanted to see if you get any performance improvement with these
optimization fixes.

Quoted 6 lines

Quoted 3 lines
The main point is someone needs to debug the patchset on socfpga platform. It works on stv0991 platform. But which patch is really creating issue on socfpga & debug that patch.
I'll happily help, can you please rebase it on top of u-boot/master and repost? I can't seem to be able to apply it. Also, please keep me in Cc.

There is v2 of the patchset rebased on master, please use that.

Cheers,
Vikas

Quoted 6 lines

Thanks
Best regards,
Marek Vasut

Marek Vasut

2015-08-13 00:26 UTC #31 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Quoted 14 lines · vikasm

Hi Marek,
Quoted 11 lines · Marek Vasut
Quoted 10 lines · vikasm
Thanks Jagan,
Quoted 6 lines · Jagan Teki
Vikas,
Did you verified on board, can you just verified with 'sf update' before and after.. I just wanted to see if you get any performance improvement with these
optimization fixes.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
The main point is someone needs to debug the patchset on socfpga platform. It works on stv0991 platform. But which patch is really creating issue on socfpga & debug that patch.
I'll happily help, can you please rebase it on top of u-boot/master and repost? I can't seem to be able to apply it. Also, please keep me in Cc.
There is v2 of the patchset rebased on master, please use that.

I only see this stuff in the mailing list ; did the patches get renamed or something ? Can you please provide patchwork links or at least some ML archive link , that'd really help.

Best regards,
Marek Vasut

vikasm

author

2015-08-13 00:36 UTC #32 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Hi Marek,

Quoted 19 lines · Marek Vasut

Quoted 18 lines · vikasm
Hi Marek,
Quoted 15 lines · Marek Vasut
Quoted 13 lines · vikasm
Thanks Jagan,
Quoted 6 lines · Jagan Teki
Vikas,
Did you verified on board, can you just verified with 'sf update' before and after.. I just wanted to see if you get any performance improvement with these
optimization fixes.
The main point is someone needs to debug the patchset on socfpga platform. It works on stv0991 platform. But which patch is really creating issue on socfpga & debug that patch.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
I'll happily help, can you please rebase it on top of u-boot/master and repost? I can't seem to be able to apply it. Also, please keep me in Cc.
There is v2 of the patchset rebased on master, please use that.
I only see this stuff in the mailing list ; did the patches get renamed or something ? Can you please provide patchwork links or at least some ML archive link , that'd really help.

I just forwarded you one of the v2 patch & here is the patchwork link:
https://patchwork.ozlabs.org/patch/496526/

Rgds,
Vikas

Quoted 5 lines

Best regards,
Marek Vasut
.

Marek Vasut

2015-08-13 02:15 UTC #33 lore raw

cover spi: cadence_qspi: optimize & fix indirect rd-writes

Quoted 1 line · vikasm

Hi Marek,

Hi!

Quoted 24 lines · Marek Vasut

Quoted 23 lines · vikasm
Quoted 22 lines
Hi Marek,
Quoted 18 lines · Marek Vasut
Quoted 5 lines · vikasm
Thanks Jagan,
Quoted 2 lines · Jagan Teki
Vikas,
Quoted 8 lines
Quoted 4 lines
Did you verified on board, can you just verified with 'sf update' before and after.. I just wanted to see if you get any performance improvement with these
optimization fixes.
The main point is someone needs to debug the patchset on socfpga platform. It works on stv0991 platform. But which patch is really creating issue on socfpga & debug that patch.
I'll happily help, can you please rebase it on top of u-boot/master and repost? I can't seem to be able to apply it. Also, please keep me in Cc.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
There is v2 of the patchset rebased on master, please use that.
I only see this stuff in the mailing list ; did the patches get renamed or something ? Can you please provide patchwork links or at least some ML archive link , that'd really help.
I just forwarded you one of the v2 patch & here is the patchwork link:
https://patchwork.ozlabs.org/patch/496526/

I took a brief look and commented on the patches.

Best regards,
Marek Vasut