[U-Boot] [v2 0/6] spi: cadence_qspi: optimize & fix indirect rd-writes

[U-Boot] [v2 0/6] spi: cadence_qspi: optimize & fix indirect rd-writes

[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

Changes in v2:
- rebased to master.
- removed patch "spi: cadence_qspi: read can be independent of fifo width", it
  was implemented in other patchset, in mainline now.

Vikas Manocha (6):
  spi: cadence_qspi: move trigger base configuration in init
  spi: cadence_qspi: remove sram polling from flash read
  spi: cadence_qspi: remove sram polling from flash write
  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      |    4 +-
 arch/arm/dts/stv0991.dts       |    4 +-
 drivers/spi/cadence_qspi.c     |   15 ++---
 drivers/spi/cadence_qspi.h     |    6 +-
 drivers/spi/cadence_qspi_apb.c |  125 ++++++++--------------------------------
 5 files changed, 43 insertions(+), 111 deletions(-)

-- 
1.7.9.5

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[Vikas Manocha]

Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
---

Changes in v2: Rebased to master

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

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index d053407..1ae7edf 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)
 
 	/* Indirect mode configurations */
 	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
+	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
+				plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
 	/* Disable all interrupts */
 	writel(0, plat->regbase + CQSPI_REG_IRQMASK);
@@ -693,10 +695,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 the opcode */
 	rd_reg = cmdbuf[0] << CQSPI_REG_RD_INSTR_OPCODE_LSB;
 
@@ -790,9 +788,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);
 
 	/* Configure the opcode */
 	reg = cmdbuf[0] << CQSPI_REG_WR_INSTR_OPCODE_LSB;
-- 
1.7.9.5

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Vikas Manocha]

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, particularly
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 <vikas.manocha@st.com>
---

Changes in v2: Rebased to master

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

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index 1ae7edf..487bbef 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -192,7 +192,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;
@@ -211,7 +211,7 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
 		memcpy(dest_ptr, &temp, remaining);
 	}
 
-	return;
+	return 0;
 }
 
 static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
@@ -240,42 +240,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)
@@ -750,9 +714,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 */
-- 
1.7.9.5

[U-Boot] [v2 3/6] spi: cadence_qspi: remove sram polling from flash write

[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 <vikas.manocha@st.com>
---

Changes in v2: Rebased to master

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

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index 487bbef..ad8b79a 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -214,67 +214,36 @@ 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 = 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;
-}
-
 /* 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->ahbbase;
 	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;
 }
 
-- 
1.7.9.5

[U-Boot] [v2 4/6] spi: cadence_qspi: fix indirect read/write start address

[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 <vikas.manocha@st.com>
---

Changes in v2: Rebased to master

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

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index ad8b79a..7313b0c 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -638,7 +638,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;
@@ -729,7 +730,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;
-- 
1.7.9.5

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[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 <vikas.manocha@st.com>
---

Changes in v2: Rebased to master

 arch/arm/dts/socfpga.dtsi      |    3 ++-
 arch/arm/dts/stv0991.dts       |    3 ++-
 drivers/spi/cadence_qspi.c     |   14 +++++++-------
 drivers/spi/cadence_qspi.h     |    5 +++--
 drivers/spi/cadence_qspi_apb.c |   11 +++++------
 5 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index 9b12420..1099a92 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -633,7 +633,8 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0xff705000 0x1000>,
-				<0xffa00000 0x1000>;
+				<0xffa00000 0x1000>,
+				<0x00000000 0x0010>;
 			interrupts = <0 151 4>;
 			clocks = <&qspi_clk>;
 			ext-decoder = <0>;  /* external decoder */
diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
index fa3fd64..e23d4fd 100644
--- a/arch/arm/dts/stv0991.dts
+++ b/arch/arm/dts/stv0991.dts
@@ -30,7 +30,8 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0x80203000 0x100>,
-				<0x40000000 0x1000000>;
+				<0x40000000 0x1000000>,
+				<0x40000000 0x0000010>;
 			clocks = <3750000>;
 			sram-size = <256>;
 			status = "okay";
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 34a0f46..95c9cea 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -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",
@@ -311,10 +312,9 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns", 20);
 	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
 
-	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;
 }
 
diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
index 98e57aa..7341339 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -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;
@@ -30,7 +31,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;
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index 7313b0c..e11f715 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -44,7 +44,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)
@@ -219,7 +218,7 @@ static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
 				const void *src_addr, unsigned int num_bytes)
 {
 	int i = 0;
-	unsigned int *dest_addr = plat->ahbbase;
+	unsigned int *dest_addr = plat->trigger_base;
 	unsigned int wr_bytes;
 	unsigned int *src_ptr = (unsigned int *)src_addr;
 	int remaining = num_bytes;
@@ -467,7 +466,7 @@ void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)
 
 	/* Indirect mode configurations */
 	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
-	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
+	writel((u32)plat->trigger_base,
 				plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
 	/* Disable all interrupts */
@@ -638,7 +637,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. */
@@ -687,7 +686,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 */
@@ -730,7 +729,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);
-- 
1.7.9.5

[U-Boot] [v2 6/6] spi: cadence_qspi: get fifo width from device tree

[Vikas Manocha]

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

Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
---

Changes in v2: Rebased to master

 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(-)

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index 1099a92..ee43762 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -640,6 +640,7 @@
 			ext-decoder = <0>;  /* external decoder */
 			num-cs = <4>;
 			fifo-depth = <128>;
+			fifo-width = <4>;
 			sram-size = <128>;
 			bus-num = <2>;
 			status = "disabled";
diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
index e23d4fd..f0f450b 100644
--- a/arch/arm/dts/stv0991.dts
+++ b/arch/arm/dts/stv0991.dts
@@ -34,6 +34,7 @@
 				<0x40000000 0x0000010>;
 			clocks = <3750000>;
 			sram-size = <256>;
+			fifo-width = <8>;
 			status = "okay";
 
 			flash0: n25q32 at 0 {
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 95c9cea..bfcd39e 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -311,6 +311,7 @@ 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);
 	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
+	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,
diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
index 7341339..91f38f1 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -27,6 +27,7 @@ struct cadence_spi_platdata {
 	u32		tchsh_ns;
 	u32		tslch_ns;
 	u32		sram_size;
+	u32		fifo_width;
 };
 
 struct cadence_spi_priv {
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index e11f715..e5da225 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -34,8 +34,6 @@
 #define CQSPI_REG_RETRY				(10000)
 #define CQSPI_POLL_IDLE_RETRY			(3)
 
-#define CQSPI_FIFO_WIDTH			(4)
-
 #define CQSPI_REG_SRAM_THRESHOLD_WORDS		(50)
 
 /* Transfer mode */
@@ -48,9 +46,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)
  ****************************************************************************/
@@ -230,11 +225,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 */
-- 
1.7.9.5

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[Marek Vasut]

On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:

Commit message is missing.

> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> ---
> 
> Changes in v2: Rebased to master
> 
>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi_apb.c
> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
> cadence_spi_platdata *plat)
> 
>  	/* Indirect mode configurations */
>  	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),

You can drop the parenthesis around the first argument, they're useless.
Also, the indent of the second arg should be fixed, I believe checkpatch
might even complain about it.

> +				plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
> 
>  	/* Disable all interrupts */
>  	writel(0, plat->regbase + CQSPI_REG_IRQMASK);
> @@ -693,10 +695,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 the opcode */
>  	rd_reg = cmdbuf[0] << CQSPI_REG_RD_INSTR_OPCODE_LSB;
> 
> @@ -790,9 +788,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);
> 
>  	/* Configure the opcode */
>  	reg = cmdbuf[0] << CQSPI_REG_WR_INSTR_OPCODE_LSB;

Best regards,
Marek Vasut

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
> 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.


Consider the following scenario:
- CPU core reads some memory area, but there are no data available just yet
  => CPU core goes into wait state
- The data never arrive

Will the CPU be stuck forever ? If we checked the fill level first instead,
we would never enter such stuck-state.

> Also Relying on SRAM fill level only for deciding when the data should be
> fetched from the local SRAM is not most efficient approach, particularly
> 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 <vikas.manocha@st.com>
> ---
[...]
Best regards,
Marek Vasut

[U-Boot] [v2 3/6] spi: cadence_qspi: remove sram polling from flash write

[Marek Vasut]

On Thursday, July 16, 2015 at 04:27:31 AM, Vikas Manocha wrote:
> 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.

Please see the question I posed in 2/6 v2 .

> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> ---
> 
> Changes in v2: Rebased to master
> 
>  drivers/spi/cadence_qspi_apb.c |   61
> ++++++++++------------------------------ 1 file changed, 15 insertions(+),
> 46 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi_apb.c
> b/drivers/spi/cadence_qspi_apb.c index 487bbef..ad8b79a 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -214,67 +214,36 @@ 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 = 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;
> -}
> -
>  /* 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->ahbbase;
>  	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);

Why don't you use memcpy instead , didn't you say you're copying data to/from 
SRAM? Is src_ptr value always aligned ?

> +			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;
>  }

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[Marek Vasut]

On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
> This patch is to separate the base trigger from the read/write transfer
> start addresses.

This patch breaks the QSPI support on SoCFPGA.

> 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 <vikas.manocha@st.com>
> ---
> 
> Changes in v2: Rebased to master
> 
>  arch/arm/dts/socfpga.dtsi      |    3 ++-
>  arch/arm/dts/stv0991.dts       |    3 ++-
>  drivers/spi/cadence_qspi.c     |   14 +++++++-------
>  drivers/spi/cadence_qspi.h     |    5 +++--
>  drivers/spi/cadence_qspi_apb.c |   11 +++++------
>  5 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
> index 9b12420..1099a92 100644
> --- a/arch/arm/dts/socfpga.dtsi
> +++ b/arch/arm/dts/socfpga.dtsi
> @@ -633,7 +633,8 @@
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			reg = <0xff705000 0x1000>,
> -				<0xffa00000 0x1000>;
> +				<0xffa00000 0x1000>,
> +				<0x00000000 0x0010>;

Shouldn't there be a phandle to 

>  			interrupts = <0 151 4>;
>  			clocks = <&qspi_clk>;
>  			ext-decoder = <0>;  /* external decoder */
> diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
> index fa3fd64..e23d4fd 100644
> --- a/arch/arm/dts/stv0991.dts
> +++ b/arch/arm/dts/stv0991.dts
> @@ -30,7 +30,8 @@
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			reg = <0x80203000 0x100>,
> -				<0x40000000 0x1000000>;
> +				<0x40000000 0x1000000>,
> +				<0x40000000 0x0000010>;
>  			clocks = <3750000>;
>  			sram-size = <256>;
>  			status = "okay";
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 34a0f46..95c9cea 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -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",
> @@ -311,10 +312,9 @@ static int cadence_spi_ofdata_to_platdata(struct
> udevice *bus) plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns",
> 20);
>  	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
> 
> -	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,

Please never break formating strings, they are the only exception from the 80 
alignment rule. It is not possible to grep for them if they're broken.

> +		plat->trigger_base, plat->max_hz, plat->page_size);
>  	return 0;
>  }

[...]

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[vikasm]

Hi Marek,

On 08/12/2015 07:07 PM, Marek Vasut wrote:
> On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:
> 
> Commit message is missing.

Actually subject of the mail was sufficient, this patch just moves the register configuration in init.

> 
>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>> ---
>>
>> Changes in v2: Rebased to master
>>
>>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/cadence_qspi_apb.c
>> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
>> cadence_spi_platdata *plat)
>>
>>  	/* Indirect mode configurations */
>>  	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
>> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
> 
> You can drop the parenthesis around the first argument, they're useless.
> Also, the indent of the second arg should be fixed, I believe checkpatch
> might even complain about it.

ok.

Rgds,
Vikas

> 
>> +				plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
>>
>>  	/* Disable all interrupts */
>>  	writel(0, plat->regbase + CQSPI_REG_IRQMASK);
>> @@ -693,10 +695,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 the opcode */
>>  	rd_reg = cmdbuf[0] << CQSPI_REG_RD_INSTR_OPCODE_LSB;
>>
>> @@ -790,9 +788,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);
>>
>>  	/* Configure the opcode */
>>  	reg = cmdbuf[0] << CQSPI_REG_WR_INSTR_OPCODE_LSB;
> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[vikasm]

Hi Marek,

On 08/12/2015 07:09 PM, Marek Vasut wrote:
> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
>> 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.
> 
> 
> Consider the following scenario:
> - CPU core reads some memory area, but there are no data available just yet
>   => CPU core goes into wait state
> - The data never arrive
> 
> Will the CPU be stuck forever ? If we checked the fill level first instead,
> we would never enter such stuck-state.

This indirect mode of reading/writing would be entered when the read/write addresses
are in the programmed valid range of addresses.

Even in case of "data never arrive" scenario, a simple timeout seems better then currently
implemented read sram level with timeout.

Rgds,
Vikas

> 
>> Also Relying on SRAM fill level only for deciding when the data should be
>> fetched from the local SRAM is not most efficient approach, particularly
>> 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 <vikas.manocha@st.com>
>> ---
> [...]
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 3/6] spi: cadence_qspi: remove sram polling from flash write

[vikasm]

Hi Marek,

On 08/12/2015 07:11 PM, Marek Vasut wrote:
> On Thursday, July 16, 2015 at 04:27:31 AM, Vikas Manocha wrote:
>> 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.
> 
> Please see the question I posed in 2/6 v2 .

replied in 2/6 v2.

> 
>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>> ---
>>
>> Changes in v2: Rebased to master
>>
>>  drivers/spi/cadence_qspi_apb.c |   61
>> ++++++++++------------------------------ 1 file changed, 15 insertions(+),
>> 46 deletions(-)
>>
>> diff --git a/drivers/spi/cadence_qspi_apb.c
>> b/drivers/spi/cadence_qspi_apb.c index 487bbef..ad8b79a 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -214,67 +214,36 @@ 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 = 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;
>> -}
>> -
>>  /* 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->ahbbase;
>>  	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);
> 
> Why don't you use memcpy instead , didn't you say you're copying data to/from 
> SRAM? Is src_ptr value always aligned ?

i think you are right, i will check it & fix in the next version.

Rgds,
Vikas

> 
>> +			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;
>>  }
> .
> 

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[vikasm]

Hi Marek,

On 08/12/2015 07:15 PM, Marek Vasut wrote:
> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
>> This patch is to separate the base trigger from the read/write transfer
>> start addresses.
> 
> This patch breaks the QSPI support on SoCFPGA.

ok, can you please try to debug the issue. Logically this patch looks good to me unless there
is something specific to socfpga.

I think latest linux v2 patch from Graham has implemented the same patch.

> 
>> 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 <vikas.manocha@st.com>
>> ---
>>
>> Changes in v2: Rebased to master
>>
>>  arch/arm/dts/socfpga.dtsi      |    3 ++-
>>  arch/arm/dts/stv0991.dts       |    3 ++-
>>  drivers/spi/cadence_qspi.c     |   14 +++++++-------
>>  drivers/spi/cadence_qspi.h     |    5 +++--
>>  drivers/spi/cadence_qspi_apb.c |   11 +++++------
>>  5 files changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
>> index 9b12420..1099a92 100644
>> --- a/arch/arm/dts/socfpga.dtsi
>> +++ b/arch/arm/dts/socfpga.dtsi
>> @@ -633,7 +633,8 @@
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  			reg = <0xff705000 0x1000>,
>> -				<0xffa00000 0x1000>;
>> +				<0xffa00000 0x1000>,
>> +				<0x00000000 0x0010>;
> 
> Shouldn't there be a phandle to
> 
>>  			interrupts = <0 151 4>;
>>  			clocks = <&qspi_clk>;
>>  			ext-decoder = <0>;  /* external decoder */
>> diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
>> index fa3fd64..e23d4fd 100644
>> --- a/arch/arm/dts/stv0991.dts
>> +++ b/arch/arm/dts/stv0991.dts
>> @@ -30,7 +30,8 @@
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  			reg = <0x80203000 0x100>,
>> -				<0x40000000 0x1000000>;
>> +				<0x40000000 0x1000000>,
>> +				<0x40000000 0x0000010>;
>>  			clocks = <3750000>;
>>  			sram-size = <256>;
>>  			status = "okay";
>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>> index 34a0f46..95c9cea 100644
>> --- a/drivers/spi/cadence_qspi.c
>> +++ b/drivers/spi/cadence_qspi.c
>> @@ -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",
>> @@ -311,10 +312,9 @@ static int cadence_spi_ofdata_to_platdata(struct
>> udevice *bus) plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns",
>> 20);
>>  	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
>>
>> -	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,
> 
> Please never break formating strings, they are the only exception from the 80 
> alignment rule. It is not possible to grep for them if they're broken.

ok, i will fix in next version.

Rgds,
Vikas

> 
>> +		plat->trigger_base, plat->max_hz, plat->page_size);
>>  	return 0;
>>  }
> 
> [...]
> .
> 

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Thursday, August 13, 2015 at 06:27:08 PM, vikasm wrote:
> Hi Marek,

Hi vikasm,

(you might want to fix your name in your mailer)

> On 08/12/2015 07:09 PM, Marek Vasut wrote:
> > On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
> >> 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.
> > 
> > Consider the following scenario:
> > - CPU core reads some memory area, but there are no data available just
> > yet
> > 
> >   => CPU core goes into wait state
> > 
> > - The data never arrive
> > 
> > Will the CPU be stuck forever ? If we checked the fill level first
> > instead, we would never enter such stuck-state.
> 
> This indirect mode of reading/writing would be entered when the read/write
> addresses are in the programmed valid range of addresses.
> 
> Even in case of "data never arrive" scenario, a simple timeout seems better
> then currently implemented read sram level with timeout.

How do you implement a "simple timeout" if the CPU core is stuck and does
not execute instructions ? If you mean interrupt, then forget it, U-Boot
does not do interrupts ;-)

Best regards,
Marek Vasut

[U-Boot] [v2 3/6] spi: cadence_qspi: remove sram polling from flash write

[Marek Vasut]

On Thursday, August 13, 2015 at 06:30:37 PM, vikasm wrote:
> Hi Marek,
> 
> On 08/12/2015 07:11 PM, Marek Vasut wrote:
> > On Thursday, July 16, 2015 at 04:27:31 AM, Vikas Manocha wrote:
> >> 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.
> > 
> > Please see the question I posed in 2/6 v2 .
> 
> replied in 2/6 v2.

[...]

> >>  /* 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->ahbbase;
> >> 
> >>  	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);
> > 
> > Why don't you use memcpy instead , didn't you say you're copying data
> > to/from SRAM? Is src_ptr value always aligned ?
> 
> i think you are right, i will check it & fix in the next version.

But is memcpy() really correct ? It does byte access and doesn't enforce 
ordering in any way.

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[Marek Vasut]

On Thursday, August 13, 2015 at 05:50:18 PM, vikasm wrote:
> Hi Marek,

Hi!

> On 08/12/2015 07:07 PM, Marek Vasut wrote:
> > On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:
> > 
> > Commit message is missing.
> 
> Actually subject of the mail was sufficient, this patch just moves the
> register configuration in init.

NAK, fix the commit message.

> >> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> >> ---
> >> 
> >> Changes in v2: Rebased to master
> >> 
> >>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
> >>  1 file changed, 2 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/drivers/spi/cadence_qspi_apb.c
> >> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
> >> --- a/drivers/spi/cadence_qspi_apb.c
> >> +++ b/drivers/spi/cadence_qspi_apb.c
> >> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
> >> cadence_spi_platdata *plat)
> >> 
> >>  	/* Indirect mode configurations */
> >>  	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
> >> 
> >> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
> > 
> > You can drop the parenthesis around the first argument, they're useless.
> > Also, the indent of the second arg should be fixed, I believe checkpatch
> > might even complain about it.
> 
> ok.

Thanks :)

Best regards,
Marek Vasut

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[vikasm]

Hi,

On 08/13/2015 10:35 AM, Marek Vasut wrote:
> On Thursday, August 13, 2015 at 05:50:18 PM, vikasm wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 08/12/2015 07:07 PM, Marek Vasut wrote:
>>> On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:
>>>
>>> Commit message is missing.
>>
>> Actually subject of the mail was sufficient, this patch just moves the
>> register configuration in init.
> 
> NAK, fix the commit message.

ok.

Rgds,
Vikas

> 
>>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>>>> ---
>>>>
>>>> Changes in v2: Rebased to master
>>>>
>>>>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/cadence_qspi_apb.c
>>>> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
>>>> cadence_spi_platdata *plat)
>>>>
>>>>  	/* Indirect mode configurations */
>>>>  	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
>>>>
>>>> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
>>>
>>> You can drop the parenthesis around the first argument, they're useless.
>>> Also, the indent of the second arg should be fixed, I believe checkpatch
>>> might even complain about it.
>>
>> ok.
> 
> Thanks :)
> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[vikas]

Hi Marek,


On 08/13/2015 10:33 AM, Marek Vasut wrote:
> On Thursday, August 13, 2015 at 06:27:08 PM, vikasm wrote:
>> Hi Marek,
> 
> Hi vikasm,
> 
> (you might want to fix your name in your mailer)

ok :-)

> 
>> On 08/12/2015 07:09 PM, Marek Vasut wrote:
>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
>>>> 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.
>>>
>>> Consider the following scenario:
>>> - CPU core reads some memory area, but there are no data available just
>>> yet
>>>
>>>   => CPU core goes into wait state
>>>
>>> - The data never arrive
>>>
>>> Will the CPU be stuck forever ? If we checked the fill level first
>>> instead, we would never enter such stuck-state.
>>
>> This indirect mode of reading/writing would be entered when the read/write
>> addresses are in the programmed valid range of addresses.
>>
>> Even in case of "data never arrive" scenario, a simple timeout seems better
>> then currently implemented read sram level with timeout.
> 
> How do you implement a "simple timeout" if the CPU core is stuck and does
> not execute instructions ? If you mean interrupt, then forget it, U-Boot
> does not do interrupts ;-)

Oh yes, you are right.

Rgds,
Vikas

> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote:

Hi!

> >> On 08/12/2015 07:09 PM, Marek Vasut wrote:
> >>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
> >>>> 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.
> >>> 
> >>> Consider the following scenario:
> >>> - CPU core reads some memory area, but there are no data available just
> >>> yet
> >>> 
> >>>   => CPU core goes into wait state
> >>> 
> >>> - The data never arrive
> >>> 
> >>> Will the CPU be stuck forever ? If we checked the fill level first
> >>> instead, we would never enter such stuck-state.
> >> 
> >> This indirect mode of reading/writing would be entered when the
> >> read/write addresses are in the programmed valid range of addresses.
> >> 
> >> Even in case of "data never arrive" scenario, a simple timeout seems
> >> better then currently implemented read sram level with timeout.
> > 
> > How do you implement a "simple timeout" if the CPU core is stuck and does
> > not execute instructions ? If you mean interrupt, then forget it, U-Boot
> > does not do interrupts ;-)
> 
> Oh yes, you are right.

So shall we keep the SRAM piece ?

Best regards,
Marek Vasut

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[vikas]

Hi Marek,

On 08/13/2015 01:35 PM, Marek Vasut wrote:
> On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote:
> 
> Hi!
> 
>>>> On 08/12/2015 07:09 PM, Marek Vasut wrote:
>>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
>>>>>> 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.
>>>>>
>>>>> Consider the following scenario:
>>>>> - CPU core reads some memory area, but there are no data available just
>>>>> yet
>>>>>
>>>>>   => CPU core goes into wait state
>>>>>
>>>>> - The data never arrive
>>>>>
>>>>> Will the CPU be stuck forever ? If we checked the fill level first
>>>>> instead, we would never enter such stuck-state.
>>>>
>>>> This indirect mode of reading/writing would be entered when the
>>>> read/write addresses are in the programmed valid range of addresses.
>>>>
>>>> Even in case of "data never arrive" scenario, a simple timeout seems
>>>> better then currently implemented read sram level with timeout.
>>>
>>> How do you implement a "simple timeout" if the CPU core is stuck and does
>>> not execute instructions ? If you mean interrupt, then forget it, U-Boot
>>> does not do interrupts ;-)
>>
>> Oh yes, you are right.
> 
> So shall we keep the SRAM piece ?

Although in this case the better solution would be to have watermark interrupt/status check based on sram
fill level, let us keep the existing piece of SRAM.

Can we make it configurable (SRAM Level test or not) like from DT or #define ? 

Rgds,
Vikas

> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[vikas]

Hi Marek,

On 08/13/2015 09:42 AM, vikasm wrote:
> Hi Marek,
> 
> On 08/12/2015 07:15 PM, Marek Vasut wrote:
>> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
>>> This patch is to separate the base trigger from the read/write transfer
>>> start addresses.
>>
>> This patch breaks the QSPI support on SoCFPGA.
> 
> ok, can you please try to debug the issue. Logically this patch looks good to me unless there
> is something specific to socfpga.

One quick check, can you test if it works on reverting the trigger base in arch/arm/dts/socfpga.dtsi from
<0x00000000 0x0010> to <0xffa00000 0x1000>.

Rgds,
Vikas

> 
> I think latest linux v2 patch from Graham has implemented the same patch.
> 
>>
>>> 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 <vikas.manocha@st.com>
>>> ---
>>>
>>> Changes in v2: Rebased to master
>>>
>>>  arch/arm/dts/socfpga.dtsi      |    3 ++-
>>>  arch/arm/dts/stv0991.dts       |    3 ++-
>>>  drivers/spi/cadence_qspi.c     |   14 +++++++-------
>>>  drivers/spi/cadence_qspi.h     |    5 +++--
>>>  drivers/spi/cadence_qspi_apb.c |   11 +++++------
>>>  5 files changed, 19 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
>>> index 9b12420..1099a92 100644
>>> --- a/arch/arm/dts/socfpga.dtsi
>>> +++ b/arch/arm/dts/socfpga.dtsi
>>> @@ -633,7 +633,8 @@
>>>  			#address-cells = <1>;
>>>  			#size-cells = <0>;
>>>  			reg = <0xff705000 0x1000>,
>>> -				<0xffa00000 0x1000>;
>>> +				<0xffa00000 0x1000>,
>>> +				<0x00000000 0x0010>;
>>
>> Shouldn't there be a phandle to
>>
>>>  			interrupts = <0 151 4>;
>>>  			clocks = <&qspi_clk>;
>>>  			ext-decoder = <0>;  /* external decoder */
>>> diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
>>> index fa3fd64..e23d4fd 100644
>>> --- a/arch/arm/dts/stv0991.dts
>>> +++ b/arch/arm/dts/stv0991.dts
>>> @@ -30,7 +30,8 @@
>>>  			#address-cells = <1>;
>>>  			#size-cells = <0>;
>>>  			reg = <0x80203000 0x100>,
>>> -				<0x40000000 0x1000000>;
>>> +				<0x40000000 0x1000000>,
>>> +				<0x40000000 0x0000010>;
>>>  			clocks = <3750000>;
>>>  			sram-size = <256>;
>>>  			status = "okay";
>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>> index 34a0f46..95c9cea 100644
>>> --- a/drivers/spi/cadence_qspi.c
>>> +++ b/drivers/spi/cadence_qspi.c
>>> @@ -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",
>>> @@ -311,10 +312,9 @@ static int cadence_spi_ofdata_to_platdata(struct
>>> udevice *bus) plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns",
>>> 20);
>>>  	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
>>>
>>> -	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,
>>
>> Please never break formating strings, they are the only exception from the 80 
>> alignment rule. It is not possible to grep for them if they're broken.
> 
> ok, i will fix in next version.
> 
> Rgds,
> Vikas
> 
>>
>>> +		plat->trigger_base, plat->max_hz, plat->page_size);
>>>  	return 0;
>>>  }
>>
>> [...]
>> .
>>

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote:
> Hi Marek,

Hi!

> On 08/13/2015 01:35 PM, Marek Vasut wrote:
> > On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote:
> > 
> > Hi!
> > 
> >>>> On 08/12/2015 07:09 PM, Marek Vasut wrote:
> >>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
> >>>>>> 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.
> >>>>> 
> >>>>> Consider the following scenario:
> >>>>> - CPU core reads some memory area, but there are no data available
> >>>>> just yet
> >>>>> 
> >>>>>   => CPU core goes into wait state
> >>>>> 
> >>>>> - The data never arrive
> >>>>> 
> >>>>> Will the CPU be stuck forever ? If we checked the fill level first
> >>>>> instead, we would never enter such stuck-state.
> >>>> 
> >>>> This indirect mode of reading/writing would be entered when the
> >>>> read/write addresses are in the programmed valid range of addresses.
> >>>> 
> >>>> Even in case of "data never arrive" scenario, a simple timeout seems
> >>>> better then currently implemented read sram level with timeout.
> >>> 
> >>> How do you implement a "simple timeout" if the CPU core is stuck and
> >>> does not execute instructions ? If you mean interrupt, then forget it,
> >>> U-Boot does not do interrupts ;-)
> >> 
> >> Oh yes, you are right.
> > 
> > So shall we keep the SRAM piece ?
> 
> Although in this case the better solution would be to have watermark
> interrupt/status check based on sram fill level, let us keep the existing
> piece of SRAM.

Good.

> Can we make it configurable (SRAM Level test or not) like from DT or
> #define ?

How would you call such an option ? Something like CONFIG_SYS_YOLO (to indicate
that life is too short to use correct, but slower code) ? :-)

I don't want to have two different codepaths in the codebase, one of which is
buggy. So no, I disagree we should add this option. I also don't think it would
be such a performance improvement, so I only see downsides in such a code.

Best regards,
Marek Vasut

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[Marek Vasut]

On Thursday, August 13, 2015 at 11:36:31 PM, vikas wrote:
> Hi Marek,

Hi!

> On 08/13/2015 09:42 AM, vikasm wrote:
> > Hi Marek,
> > 
> > On 08/12/2015 07:15 PM, Marek Vasut wrote:
> >> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
> >>> This patch is to separate the base trigger from the read/write transfer
> >>> start addresses.
> >> 
> >> This patch breaks the QSPI support on SoCFPGA.
> > 
> > ok, can you please try to debug the issue. Logically this patch looks
> > good to me unless there is something specific to socfpga.
> 
> One quick check, can you test if it works on reverting the trigger base in
> arch/arm/dts/socfpga.dtsi from <0x00000000 0x0010> to <0xffa00000 0x1000>.

Can you please spin V3 of the patchset first, one which omits the buggy bits,
so I can test it on a more final form of the patches ? Please keep me on CC,
I'll test it then.

Thanks!

Best regards,
Marek Vasut

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[vikas]

Hi Marek,

On 08/13/2015 03:47 PM, Marek Vasut wrote:
> On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 08/13/2015 01:35 PM, Marek Vasut wrote:
>>> On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote:
>>>
>>> Hi!
>>>
>>>>>> On 08/12/2015 07:09 PM, Marek Vasut wrote:
>>>>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> Consider the following scenario:
>>>>>>> - CPU core reads some memory area, but there are no data available
>>>>>>> just yet
>>>>>>>
>>>>>>>   => CPU core goes into wait state
>>>>>>>
>>>>>>> - The data never arrive
>>>>>>>
>>>>>>> Will the CPU be stuck forever ? If we checked the fill level first
>>>>>>> instead, we would never enter such stuck-state.
>>>>>>
>>>>>> This indirect mode of reading/writing would be entered when the
>>>>>> read/write addresses are in the programmed valid range of addresses.
>>>>>>
>>>>>> Even in case of "data never arrive" scenario, a simple timeout seems
>>>>>> better then currently implemented read sram level with timeout.
>>>>>
>>>>> How do you implement a "simple timeout" if the CPU core is stuck and
>>>>> does not execute instructions ? If you mean interrupt, then forget it,
>>>>> U-Boot does not do interrupts ;-)
>>>>
>>>> Oh yes, you are right.
>>>
>>> So shall we keep the SRAM piece ?
>>
>> Although in this case the better solution would be to have watermark
>> interrupt/status check based on sram fill level, let us keep the existing
>> piece of SRAM.
> 
> Good.
> 
>> Can we make it configurable (SRAM Level test or not) like from DT or
>> #define ?
> 
> How would you call such an option ? Something like CONFIG_SYS_YOLO (to indicate
> that life is too short to use correct, but slower code) ? :-)
> 
> I don't want to have two different codepaths in the codebase, one of which is
> buggy. So no, I disagree we should add this option. I also don't think it would
> be such a performance improvement, so I only see downsides in such a code.

I expected the same answer :-) & agree also.
ok, the issue is SRAM Fill Level register is not being updated on my SOC, seems like design issue.
Any idea how can i add this fix (to avoid sram level polling) to my soc in u-boot mainline.

Rgds,
Vikas

> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Friday, August 14, 2015 at 01:18:59 AM, vikas wrote:
> Hi Marek,

Hi!

> On 08/13/2015 03:47 PM, Marek Vasut wrote:
> > On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote:
> >> Hi Marek,
> > 
> > Hi!
> > 
> >> On 08/13/2015 01:35 PM, Marek Vasut wrote:
> >>> On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote:
> >>> 
> >>> Hi!
> >>> 
> >>>>>> On 08/12/2015 07:09 PM, Marek Vasut wrote:
> >>>>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
> >>>>>>>> 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.
> >>>>>>> 
> >>>>>>> Consider the following scenario:
> >>>>>>> - CPU core reads some memory area, but there are no data available
> >>>>>>> just yet
> >>>>>>> 
> >>>>>>>   => CPU core goes into wait state
> >>>>>>> 
> >>>>>>> - The data never arrive
> >>>>>>> 
> >>>>>>> Will the CPU be stuck forever ? If we checked the fill level first
> >>>>>>> instead, we would never enter such stuck-state.
> >>>>>> 
> >>>>>> This indirect mode of reading/writing would be entered when the
> >>>>>> read/write addresses are in the programmed valid range of addresses.
> >>>>>> 
> >>>>>> Even in case of "data never arrive" scenario, a simple timeout seems
> >>>>>> better then currently implemented read sram level with timeout.
> >>>>> 
> >>>>> How do you implement a "simple timeout" if the CPU core is stuck and
> >>>>> does not execute instructions ? If you mean interrupt, then forget
> >>>>> it, U-Boot does not do interrupts ;-)
> >>>> 
> >>>> Oh yes, you are right.
> >>> 
> >>> So shall we keep the SRAM piece ?
> >> 
> >> Although in this case the better solution would be to have watermark
> >> interrupt/status check based on sram fill level, let us keep the
> >> existing piece of SRAM.
> > 
> > Good.
> > 
> >> Can we make it configurable (SRAM Level test or not) like from DT or
> >> #define ?
> > 
> > How would you call such an option ? Something like CONFIG_SYS_YOLO (to
> > indicate that life is too short to use correct, but slower code) ? :-)
> > 
> > I don't want to have two different codepaths in the codebase, one of
> > which is buggy. So no, I disagree we should add this option. I also
> > don't think it would be such a performance improvement, so I only see
> > downsides in such a code.
> 
> I expected the same answer :-) & agree also.

Heh, thanks :-)

> ok, the issue is SRAM Fill Level register is not being updated on my SOC,
> seems like design issue. Any idea how can i add this fix (to avoid sram
> level polling) to my soc in u-boot mainline.

Mask all interrupts in the controller, set watermark to N bytes (depending
on the length of your transfer) and poll the interrupt status register until
the watermark is reached ? Is this possible ?

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[vikas]

Hi,

On 08/13/2015 04:46 PM, Marek Vasut wrote:
> On Friday, August 14, 2015 at 01:18:59 AM, vikas wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 08/13/2015 03:47 PM, Marek Vasut wrote:
>>> On Thursday, August 13, 2015 at 11:04:59 PM, vikas wrote:
>>>> Hi Marek,
>>>
>>> Hi!
>>>
>>>> On 08/13/2015 01:35 PM, Marek Vasut wrote:
>>>>> On Thursday, August 13, 2015 at 09:49:49 PM, vikas wrote:
>>>>>
>>>>> Hi!
>>>>>
>>>>>>>> On 08/12/2015 07:09 PM, Marek Vasut wrote:
>>>>>>>>> On Thursday, July 16, 2015 at 04:27:30 AM, Vikas Manocha wrote:
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> Consider the following scenario:
>>>>>>>>> - CPU core reads some memory area, but there are no data available
>>>>>>>>> just yet
>>>>>>>>>
>>>>>>>>>   => CPU core goes into wait state
>>>>>>>>>
>>>>>>>>> - The data never arrive
>>>>>>>>>
>>>>>>>>> Will the CPU be stuck forever ? If we checked the fill level first
>>>>>>>>> instead, we would never enter such stuck-state.
>>>>>>>>
>>>>>>>> This indirect mode of reading/writing would be entered when the
>>>>>>>> read/write addresses are in the programmed valid range of addresses.
>>>>>>>>
>>>>>>>> Even in case of "data never arrive" scenario, a simple timeout seems
>>>>>>>> better then currently implemented read sram level with timeout.
>>>>>>>
>>>>>>> How do you implement a "simple timeout" if the CPU core is stuck and
>>>>>>> does not execute instructions ? If you mean interrupt, then forget
>>>>>>> it, U-Boot does not do interrupts ;-)
>>>>>>
>>>>>> Oh yes, you are right.
>>>>>
>>>>> So shall we keep the SRAM piece ?
>>>>
>>>> Although in this case the better solution would be to have watermark
>>>> interrupt/status check based on sram fill level, let us keep the
>>>> existing piece of SRAM.
>>>
>>> Good.
>>>
>>>> Can we make it configurable (SRAM Level test or not) like from DT or
>>>> #define ?
>>>
>>> How would you call such an option ? Something like CONFIG_SYS_YOLO (to
>>> indicate that life is too short to use correct, but slower code) ? :-)
>>>
>>> I don't want to have two different codepaths in the codebase, one of
>>> which is buggy. So no, I disagree we should add this option. I also
>>> don't think it would be such a performance improvement, so I only see
>>> downsides in such a code.
>>
>> I expected the same answer :-) & agree also.
> 
> Heh, thanks :-)
> 
>> ok, the issue is SRAM Fill Level register is not being updated on my SOC,
>> seems like design issue. Any idea how can i add this fix (to avoid sram
>> level polling) to my soc in u-boot mainline.
> 
> Mask all interrupts in the controller, set watermark to N bytes (depending
> on the length of your transfer) and poll the interrupt status register until
> the watermark is reached ? Is this possible ?

Watermark is for sram level and might not work as well.
In general also for such soc/platform issues for any driver, any idea how to apply fixes.

Rgds,
Vikas

> .
> 

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[vikas]

Hi,

On 08/13/2015 03:48 PM, Marek Vasut wrote:
> On Thursday, August 13, 2015 at 11:36:31 PM, vikas wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 08/13/2015 09:42 AM, vikasm wrote:
>>> Hi Marek,
>>>
>>> On 08/12/2015 07:15 PM, Marek Vasut wrote:
>>>> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
>>>>> This patch is to separate the base trigger from the read/write transfer
>>>>> start addresses.
>>>>
>>>> This patch breaks the QSPI support on SoCFPGA.
>>>
>>> ok, can you please try to debug the issue. Logically this patch looks
>>> good to me unless there is something specific to socfpga.
>>
>> One quick check, can you test if it works on reverting the trigger base in
>> arch/arm/dts/socfpga.dtsi from <0x00000000 0x0010> to <0xffa00000 0x1000>.
> 
> Can you please spin V3 of the patchset first, one which omits the buggy bits,
> so I can test it on a more final form of the patches ? Please keep me on CC,
> I'll test it then.


Once i add sram level test patches, i would not be able to test it on my platform.
This particular issue is pending for long time. I suggest to debug (at least with above mentioned quick test)
this particular issue/patch with current version v2. There is only minor change (format string) in this patch for v3.

Rgds,
Vikas

> 
> Thanks!
> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Friday, August 14, 2015 at 02:26:02 AM, vikas wrote:

Hi!

> >>>>>>>>>> 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.
> >>>>>>>>> 
> >>>>>>>>> Consider the following scenario:
> >>>>>>>>> - CPU core reads some memory area, but there are no data
> >>>>>>>>> available just yet
> >>>>>>>>> 
> >>>>>>>>>   => CPU core goes into wait state
> >>>>>>>>> 
> >>>>>>>>> - The data never arrive
> >>>>>>>>> 
> >>>>>>>>> Will the CPU be stuck forever ? If we checked the fill level
> >>>>>>>>> first instead, we would never enter such stuck-state.
> >>>>>>>> 
> >>>>>>>> This indirect mode of reading/writing would be entered when the
> >>>>>>>> read/write addresses are in the programmed valid range of
> >>>>>>>> addresses.
> >>>>>>>> 
> >>>>>>>> Even in case of "data never arrive" scenario, a simple timeout
> >>>>>>>> seems better then currently implemented read sram level with
> >>>>>>>> timeout.
> >>>>>>> 
> >>>>>>> How do you implement a "simple timeout" if the CPU core is stuck
> >>>>>>> and does not execute instructions ? If you mean interrupt, then
> >>>>>>> forget it, U-Boot does not do interrupts ;-)
> >>>>>> 
> >>>>>> Oh yes, you are right.
> >>>>> 
> >>>>> So shall we keep the SRAM piece ?
> >>>> 
> >>>> Although in this case the better solution would be to have watermark
> >>>> interrupt/status check based on sram fill level, let us keep the
> >>>> existing piece of SRAM.
> >>> 
> >>> Good.
> >>> 
> >>>> Can we make it configurable (SRAM Level test or not) like from DT or
> >>>> #define ?
> >>> 
> >>> How would you call such an option ? Something like CONFIG_SYS_YOLO (to
> >>> indicate that life is too short to use correct, but slower code) ? :-)
> >>> 
> >>> I don't want to have two different codepaths in the codebase, one of
> >>> which is buggy. So no, I disagree we should add this option. I also
> >>> don't think it would be such a performance improvement, so I only see
> >>> downsides in such a code.
> >> 
> >> I expected the same answer :-) & agree also.
> > 
> > Heh, thanks :-)
> > 
> >> ok, the issue is SRAM Fill Level register is not being updated on my
> >> SOC, seems like design issue. Any idea how can i add this fix (to avoid
> >> sram level polling) to my soc in u-boot mainline.
> > 
> > Mask all interrupts in the controller, set watermark to N bytes
> > (depending on the length of your transfer) and poll the interrupt status
> > register until the watermark is reached ? Is this possible ?
> 
> Watermark is for sram level and might not work as well.
> In general also for such soc/platform issues for any driver, any idea how
> to apply fixes.

That's a good question . Is there any way on the ST chip to avoid such direct
blocking read which can stall the CPU core indefinitelly? Can you check with
the chip designers ?

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[vikas]

Hi,


On 08/13/2015 05:44 PM, Marek Vasut wrote:
> On Friday, August 14, 2015 at 02:26:02 AM, vikas wrote:
> 
> Hi!
> 
>>>>>>>>>>>> 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.
>>>>>>>>>>>
>>>>>>>>>>> Consider the following scenario:
>>>>>>>>>>> - CPU core reads some memory area, but there are no data
>>>>>>>>>>> available just yet
>>>>>>>>>>>
>>>>>>>>>>>   => CPU core goes into wait state
>>>>>>>>>>>
>>>>>>>>>>> - The data never arrive
>>>>>>>>>>>
>>>>>>>>>>> Will the CPU be stuck forever ? If we checked the fill level
>>>>>>>>>>> first instead, we would never enter such stuck-state.
>>>>>>>>>>
>>>>>>>>>> This indirect mode of reading/writing would be entered when the
>>>>>>>>>> read/write addresses are in the programmed valid range of
>>>>>>>>>> addresses.
>>>>>>>>>>
>>>>>>>>>> Even in case of "data never arrive" scenario, a simple timeout
>>>>>>>>>> seems better then currently implemented read sram level with
>>>>>>>>>> timeout.
>>>>>>>>>
>>>>>>>>> How do you implement a "simple timeout" if the CPU core is stuck
>>>>>>>>> and does not execute instructions ? If you mean interrupt, then
>>>>>>>>> forget it, U-Boot does not do interrupts ;-)
>>>>>>>>
>>>>>>>> Oh yes, you are right.
>>>>>>>
>>>>>>> So shall we keep the SRAM piece ?
>>>>>>
>>>>>> Although in this case the better solution would be to have watermark
>>>>>> interrupt/status check based on sram fill level, let us keep the
>>>>>> existing piece of SRAM.
>>>>>
>>>>> Good.
>>>>>
>>>>>> Can we make it configurable (SRAM Level test or not) like from DT or
>>>>>> #define ?
>>>>>
>>>>> How would you call such an option ? Something like CONFIG_SYS_YOLO (to
>>>>> indicate that life is too short to use correct, but slower code) ? :-)
>>>>>
>>>>> I don't want to have two different codepaths in the codebase, one of
>>>>> which is buggy. So no, I disagree we should add this option. I also
>>>>> don't think it would be such a performance improvement, so I only see
>>>>> downsides in such a code.
>>>>
>>>> I expected the same answer :-) & agree also.
>>>
>>> Heh, thanks :-)
>>>
>>>> ok, the issue is SRAM Fill Level register is not being updated on my
>>>> SOC, seems like design issue. Any idea how can i add this fix (to avoid
>>>> sram level polling) to my soc in u-boot mainline.
>>>
>>> Mask all interrupts in the controller, set watermark to N bytes
>>> (depending on the length of your transfer) and poll the interrupt status
>>> register until the watermark is reached ? Is this possible ?
>>
>> Watermark is for sram level and might not work as well.
>> In general also for such soc/platform issues for any driver, any idea how
>> to apply fixes.
> 
> That's a good question . Is there any way on the ST chip to avoid such direct
> blocking read which can stall the CPU core indefinitelly? Can you check with
> the chip designers ?

I already checked & it does not exist in the hardware. Solution to that are timeout interrupts
in indirect mode or use only direct mode. In case of direct mode, the u-boot driver can't be used.

But again, can we add some fix for stv0991 arch to avoid sram level. Without that we will not be
able to use the driver at all for stv0991.

In general, again it seems not a unique problem. Any idea how to apply fixes for such peripheral design bugs.

Rgds,
Vikas

> 
> .
> 

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Friday, August 14, 2015 at 02:46:20 AM, vikas wrote:
> Hi,

Hi,

[...]

> >>>>> I don't want to have two different codepaths in the codebase, one of
> >>>>> which is buggy. So no, I disagree we should add this option. I also
> >>>>> don't think it would be such a performance improvement, so I only see
> >>>>> downsides in such a code.
> >>>> 
> >>>> I expected the same answer :-) & agree also.
> >>> 
> >>> Heh, thanks :-)
> >>> 
> >>>> ok, the issue is SRAM Fill Level register is not being updated on my
> >>>> SOC, seems like design issue. Any idea how can i add this fix (to
> >>>> avoid sram level polling) to my soc in u-boot mainline.
> >>> 
> >>> Mask all interrupts in the controller, set watermark to N bytes
> >>> (depending on the length of your transfer) and poll the interrupt
> >>> status register until the watermark is reached ? Is this possible ?
> >> 
> >> Watermark is for sram level and might not work as well.
> >> In general also for such soc/platform issues for any driver, any idea
> >> how to apply fixes.
> > 
> > That's a good question . Is there any way on the ST chip to avoid such
> > direct blocking read which can stall the CPU core indefinitelly? Can you
> > check with the chip designers ?
> 
> I already checked & it does not exist in the hardware. Solution to that are
> timeout interrupts in indirect mode or use only direct mode. In case of
> direct mode, the u-boot driver can't be used.

Why can't you use direct mode ? Wouldn't that solve your problem (with some
performance penalty probably) ?

> But again, can we add some fix for stv0991 arch to avoid sram level.
> Without that we will not be able to use the driver at all for stv0991.
> 
> In general, again it seems not a unique problem. Any idea how to apply
> fixes for such peripheral design bugs.

What do you mean it's not unique please ?

One option would be to pull out the SRAM level check into a separate function
and in that function check whether the controller is the one on ST chip (via
compatible string) and if it is, return fake "full" SRAM level. You'd then
fall back to the blocking read. I don't like the idea , but if there is no
other way ...

Best regards,
Marek Vasut

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[Marek Vasut]

On Friday, August 14, 2015 at 02:37:53 AM, vikas wrote:
> Hi,
> 
> On 08/13/2015 03:48 PM, Marek Vasut wrote:
> > On Thursday, August 13, 2015 at 11:36:31 PM, vikas wrote:
> >> Hi Marek,
> > 
> > Hi!
> > 
> >> On 08/13/2015 09:42 AM, vikasm wrote:
> >>> Hi Marek,
> >>> 
> >>> On 08/12/2015 07:15 PM, Marek Vasut wrote:
> >>>> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
> >>>>> This patch is to separate the base trigger from the read/write
> >>>>> transfer start addresses.
> >>>> 
> >>>> This patch breaks the QSPI support on SoCFPGA.
> >>> 
> >>> ok, can you please try to debug the issue. Logically this patch looks
> >>> good to me unless there is something specific to socfpga.
> >> 
> >> One quick check, can you test if it works on reverting the trigger base
> >> in arch/arm/dts/socfpga.dtsi from <0x00000000 0x0010> to <0xffa00000
> >> 0x1000>.
> > 
> > Can you please spin V3 of the patchset first, one which omits the buggy
> > bits, so I can test it on a more final form of the patches ? Please keep
> > me on CC, I'll test it then.
> 
> Once i add sram level test patches, i would not be able to test it on my
> platform. This particular issue is pending for long time. I suggest to
> debug (at least with above mentioned quick test) this particular
> issue/patch with current version v2. There is only minor change (format
> string) in this patch for v3.

That's fine, just send the V3 as RFT and I'll check it.

Best regards,
Marek Vasut

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[vikas]

Hi,

On 08/13/2015 06:03 PM, Marek Vasut wrote:
> On Friday, August 14, 2015 at 02:46:20 AM, vikas wrote:
>> Hi,
> 
> Hi,
> 
> [...]
> 
>>>>>>> I don't want to have two different codepaths in the codebase, one of
>>>>>>> which is buggy. So no, I disagree we should add this option. I also
>>>>>>> don't think it would be such a performance improvement, so I only see
>>>>>>> downsides in such a code.
>>>>>>
>>>>>> I expected the same answer :-) & agree also.
>>>>>
>>>>> Heh, thanks :-)
>>>>>
>>>>>> ok, the issue is SRAM Fill Level register is not being updated on my
>>>>>> SOC, seems like design issue. Any idea how can i add this fix (to
>>>>>> avoid sram level polling) to my soc in u-boot mainline.
>>>>>
>>>>> Mask all interrupts in the controller, set watermark to N bytes
>>>>> (depending on the length of your transfer) and poll the interrupt
>>>>> status register until the watermark is reached ? Is this possible ?
>>>>
>>>> Watermark is for sram level and might not work as well.
>>>> In general also for such soc/platform issues for any driver, any idea
>>>> how to apply fixes.
>>>
>>> That's a good question . Is there any way on the ST chip to avoid such
>>> direct blocking read which can stall the CPU core indefinitelly? Can you
>>> check with the chip designers ?
>>
>> I already checked & it does not exist in the hardware. Solution to that are
>> timeout interrupts in indirect mode or use only direct mode. In case of
>> direct mode, the u-boot driver can't be used.
> 
> Why can't you use direct mode ? Wouldn't that solve your problem (with some
> performance penalty probably) ?

u-boot driver does not support direct mode.

> 
>> But again, can we add some fix for stv0991 arch to avoid sram level.
>> Without that we will not be able to use the driver at all for stv0991.
>>
>> In general, again it seems not a unique problem. Any idea how to apply
>> fixes for such peripheral design bugs.
> 
> What do you mean it's not unique please ?

SOCs have one or the other design bugs often & there should be some way to apply fixes/workrounds.

> 
> One option would be to pull out the SRAM level check into a separate function
> and in that function check whether the controller is the one on ST chip (via
> compatible string) and if it is, return fake "full" SRAM level. You'd then
> fall back to the blocking read. I don't like the idea , but if there is no
> other way ...

It seems ok to me, I can send the patch for the same. Please let me know.

Rgds,
Vikas

> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[vikas]

Hi Marek,

On 08/13/2015 10:35 AM, Marek Vasut wrote:
> On Thursday, August 13, 2015 at 05:50:18 PM, vikasm wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 08/12/2015 07:07 PM, Marek Vasut wrote:
>>> On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:
>>>
>>> Commit message is missing.
>>
>> Actually subject of the mail was sufficient, this patch just moves the
>> register configuration in init.
> 
> NAK, fix the commit message.
> 
>>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>>>> ---
>>>>
>>>> Changes in v2: Rebased to master
>>>>
>>>>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/cadence_qspi_apb.c
>>>> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
>>>> cadence_spi_platdata *plat)
>>>>
>>>>  	/* Indirect mode configurations */
>>>>  	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
>>>>
>>>> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
>>>
>>> You can drop the parenthesis around the first argument, they're useless.
>>> Also, the indent of the second arg should be fixed, I believe checkpatch
>>> might even complain about it.

ok for first comment about parenthesis but indent of second arg seems ok.
yes, checkpatch warning was "CHECK: Alignment should match open parenthesis" but i ignored it.
To respect 80 column, i had to move second arg in another line. Am i missing something ?

Rgds,
Vikas

>>
>> ok.
> 
> Thanks :)
> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[vikas]

Hi Marek,

On 08/13/2015 06:04 PM, Marek Vasut wrote:
> On Friday, August 14, 2015 at 02:37:53 AM, vikas wrote:
>> Hi,
>>
>> On 08/13/2015 03:48 PM, Marek Vasut wrote:
>>> On Thursday, August 13, 2015 at 11:36:31 PM, vikas wrote:
>>>> Hi Marek,
>>>
>>> Hi!
>>>
>>>> On 08/13/2015 09:42 AM, vikasm wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 08/12/2015 07:15 PM, Marek Vasut wrote:
>>>>>> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
>>>>>>> This patch is to separate the base trigger from the read/write
>>>>>>> transfer start addresses.
>>>>>>
>>>>>> This patch breaks the QSPI support on SoCFPGA.
>>>>>
>>>>> ok, can you please try to debug the issue. Logically this patch looks
>>>>> good to me unless there is something specific to socfpga.
>>>>
>>>> One quick check, can you test if it works on reverting the trigger base
>>>> in arch/arm/dts/socfpga.dtsi from <0x00000000 0x0010> to <0xffa00000
>>>> 0x1000>.
>>>
>>> Can you please spin V3 of the patchset first, one which omits the buggy
>>> bits, so I can test it on a more final form of the patches ? Please keep
>>> me on CC, I'll test it then.
>>
>> Once i add sram level test patches, i would not be able to test it on my
>> platform. This particular issue is pending for long time. I suggest to
>> debug (at least with above mentioned quick test) this particular
>> issue/patch with current version v2. There is only minor change (format
>> string) in this patch for v3.
> 
> That's fine, just send the V3 as RFT and I'll check it.

If this patch is not debugged, there is no major significance of this patchset.
I would appreciate if you do some test with current v2.

Rgds,
Vikas

> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[Marek Vasut]

On Friday, August 14, 2015 at 03:24:10 AM, vikas wrote:
> Hi Marek,

Hi,

> On 08/13/2015 10:35 AM, Marek Vasut wrote:
> > On Thursday, August 13, 2015 at 05:50:18 PM, vikasm wrote:
> >> Hi Marek,
> > 
> > Hi!
> > 
> >> On 08/12/2015 07:07 PM, Marek Vasut wrote:
> >>> On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:
> >>> 
> >>> Commit message is missing.
> >> 
> >> Actually subject of the mail was sufficient, this patch just moves the
> >> register configuration in init.
> > 
> > NAK, fix the commit message.
> > 
> >>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> >>>> ---
> >>>> 
> >>>> Changes in v2: Rebased to master
> >>>> 
> >>>>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
> >>>>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/spi/cadence_qspi_apb.c
> >>>> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
> >>>> --- a/drivers/spi/cadence_qspi_apb.c
> >>>> +++ b/drivers/spi/cadence_qspi_apb.c
> >>>> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
> >>>> cadence_spi_platdata *plat)
> >>>> 
> >>>>  	/* Indirect mode configurations */
> >>>>  	writel((plat->sram_size/2), plat->regbase +
> >>>>  	CQSPI_REG_SRAMPARTITION);
> >>>> 
> >>>> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
> >>> 
> >>> You can drop the parenthesis around the first argument, they're
> >>> useless. Also, the indent of the second arg should be fixed, I believe
> >>> checkpatch might even complain about it.
> 
> ok for first comment about parenthesis but indent of second arg seems ok.
> yes, checkpatch warning was "CHECK: Alignment should match open
> parenthesis" but i ignored it. To respect 80 column, i had to move second
> arg in another line. Am i missing something ?

Just don't ignore the checkpatch warnings next time please ;-)

Best regards,
Marek Vasut

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[vikas]

Hi,

On 08/13/2015 06:43 PM, Marek Vasut wrote:
> On Friday, August 14, 2015 at 03:24:10 AM, vikas wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On 08/13/2015 10:35 AM, Marek Vasut wrote:
>>> On Thursday, August 13, 2015 at 05:50:18 PM, vikasm wrote:
>>>> Hi Marek,
>>>
>>> Hi!
>>>
>>>> On 08/12/2015 07:07 PM, Marek Vasut wrote:
>>>>> On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:
>>>>>
>>>>> Commit message is missing.
>>>>
>>>> Actually subject of the mail was sufficient, this patch just moves the
>>>> register configuration in init.
>>>
>>> NAK, fix the commit message.
>>>
>>>>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2: Rebased to master
>>>>>>
>>>>>>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
>>>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/spi/cadence_qspi_apb.c
>>>>>> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
>>>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>>>> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
>>>>>> cadence_spi_platdata *plat)
>>>>>>
>>>>>>  	/* Indirect mode configurations */
>>>>>>  	writel((plat->sram_size/2), plat->regbase +
>>>>>>  	CQSPI_REG_SRAMPARTITION);
>>>>>>
>>>>>> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
>>>>>
>>>>> You can drop the parenthesis around the first argument, they're
>>>>> useless. Also, the indent of the second arg should be fixed, I believe
>>>>> checkpatch might even complain about it.
>>
>> ok for first comment about parenthesis but indent of second arg seems ok.
>> yes, checkpatch warning was "CHECK: Alignment should match open
>> parenthesis" but i ignored it. To respect 80 column, i had to move second
>> arg in another line. Am i missing something ?
> 
> Just don't ignore the checkpatch warnings next time please ;-)

This CHECK message is gonna come in any case if i move second argument in second line to restrict 80 column rule.
My understanding is to ignore this CHECK message.

Rgds,
Vikas

> 
> Best regards,
> Marek Vasut
> .
> 

[U-Boot] [v2 1/6] spi: cadence_qspi: move trigger base configuration in init

[Marek Vasut]

On Friday, August 14, 2015 at 03:44:41 AM, vikas wrote:
> Hi,
> 
> On 08/13/2015 06:43 PM, Marek Vasut wrote:
> > On Friday, August 14, 2015 at 03:24:10 AM, vikas wrote:
> >> Hi Marek,
> > 
> > Hi,
> > 
> >> On 08/13/2015 10:35 AM, Marek Vasut wrote:
> >>> On Thursday, August 13, 2015 at 05:50:18 PM, vikasm wrote:
> >>>> Hi Marek,
> >>> 
> >>> Hi!
> >>> 
> >>>> On 08/12/2015 07:07 PM, Marek Vasut wrote:
> >>>>> On Thursday, July 16, 2015 at 04:27:29 AM, Vikas Manocha wrote:
> >>>>> 
> >>>>> Commit message is missing.
> >>>> 
> >>>> Actually subject of the mail was sufficient, this patch just moves the
> >>>> register configuration in init.
> >>> 
> >>> NAK, fix the commit message.
> >>> 
> >>>>>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> >>>>>> ---
> >>>>>> 
> >>>>>> Changes in v2: Rebased to master
> >>>>>> 
> >>>>>>  drivers/spi/cadence_qspi_apb.c |    9 ++-------
> >>>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/spi/cadence_qspi_apb.c
> >>>>>> b/drivers/spi/cadence_qspi_apb.c index d053407..1ae7edf 100644
> >>>>>> --- a/drivers/spi/cadence_qspi_apb.c
> >>>>>> +++ b/drivers/spi/cadence_qspi_apb.c
> >>>>>> @@ -534,6 +534,8 @@ void cadence_qspi_apb_controller_init(struct
> >>>>>> cadence_spi_platdata *plat)
> >>>>>> 
> >>>>>>  	/* Indirect mode configurations */
> >>>>>>  	writel((plat->sram_size/2), plat->regbase +
> >>>>>>  	CQSPI_REG_SRAMPARTITION);
> >>>>>> 
> >>>>>> +	writel(((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK),
> >>>>> 
> >>>>> You can drop the parenthesis around the first argument, they're
> >>>>> useless. Also, the indent of the second arg should be fixed, I
> >>>>> believe checkpatch might even complain about it.
> >> 
> >> ok for first comment about parenthesis but indent of second arg seems
> >> ok. yes, checkpatch warning was "CHECK: Alignment should match open
> >> parenthesis" but i ignored it. To respect 80 column, i had to move
> >> second arg in another line. Am i missing something ?
> > 
> > Just don't ignore the checkpatch warnings next time please ;-)
> 
> This CHECK message is gonna come in any case if i move second argument in
> second line to restrict 80 column rule. My understanding is to ignore this
> CHECK message.

I'll let others to present their opinion, I stop here.

Best regards,
Marek Vasut

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[Marek Vasut]

On Friday, August 14, 2015 at 03:39:22 AM, vikas wrote:
> Hi Marek,
> 
> On 08/13/2015 06:04 PM, Marek Vasut wrote:
> > On Friday, August 14, 2015 at 02:37:53 AM, vikas wrote:
> >> Hi,
> >> 
> >> On 08/13/2015 03:48 PM, Marek Vasut wrote:
> >>> On Thursday, August 13, 2015 at 11:36:31 PM, vikas wrote:
> >>>> Hi Marek,
> >>> 
> >>> Hi!
> >>> 
> >>>> On 08/13/2015 09:42 AM, vikasm wrote:
> >>>>> Hi Marek,
> >>>>> 
> >>>>> On 08/12/2015 07:15 PM, Marek Vasut wrote:
> >>>>>> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
> >>>>>>> This patch is to separate the base trigger from the read/write
> >>>>>>> transfer start addresses.
> >>>>>> 
> >>>>>> This patch breaks the QSPI support on SoCFPGA.
> >>>>> 
> >>>>> ok, can you please try to debug the issue. Logically this patch looks
> >>>>> good to me unless there is something specific to socfpga.
> >>>> 
> >>>> One quick check, can you test if it works on reverting the trigger
> >>>> base in arch/arm/dts/socfpga.dtsi from <0x00000000 0x0010> to
> >>>> <0xffa00000 0x1000>.
> >>> 
> >>> Can you please spin V3 of the patchset first, one which omits the buggy
> >>> bits, so I can test it on a more final form of the patches ? Please
> >>> keep me on CC, I'll test it then.
> >> 
> >> Once i add sram level test patches, i would not be able to test it on my
> >> platform. This particular issue is pending for long time. I suggest to
> >> debug (at least with above mentioned quick test) this particular
> >> issue/patch with current version v2. There is only minor change (format
> >> string) in this patch for v3.
> > 
> > That's fine, just send the V3 as RFT and I'll check it.
> 
> If this patch is not debugged, there is no major significance of this
> patchset. I would appreciate if you do some test with current v2.

I stop here for today and will get back to this in a few days again.
Please send V3 until then, I don't want to dig in old/broken code.

Best regards,
Marek Vasut

[U-Boot] [v2 5/6] spi: cadence_qspi: fix base trigger address & transfer start address

[Vikas MANOCHA]

Hi,


> On Aug 13, 2015, at 6:57 PM, Marek Vasut <marex@denx.de> wrote:
> 
>> On Friday, August 14, 2015 at 03:39:22 AM, vikas wrote:
>> Hi Marek,
>> 
>>> On 08/13/2015 06:04 PM, Marek Vasut wrote:
>>>> On Friday, August 14, 2015 at 02:37:53 AM, vikas wrote:
>>>> Hi,
>>>> 
>>>>> On 08/13/2015 03:48 PM, Marek Vasut wrote:
>>>>>> On Thursday, August 13, 2015 at 11:36:31 PM, vikas wrote:
>>>>>> Hi Marek,
>>>>> 
>>>>> Hi!
>>>>> 
>>>>>>> On 08/13/2015 09:42 AM, vikasm wrote:
>>>>>>> Hi Marek,
>>>>>>> 
>>>>>>>> On 08/12/2015 07:15 PM, Marek Vasut wrote:
>>>>>>>>> On Thursday, July 16, 2015 at 04:27:33 AM, Vikas Manocha wrote:
>>>>>>>>> This patch is to separate the base trigger from the read/write
>>>>>>>>> transfer start addresses.
>>>>>>>> 
>>>>>>>> This patch breaks the QSPI support on SoCFPGA.
>>>>>>> 
>>>>>>> ok, can you please try to debug the issue. Logically this patch looks
>>>>>>> good to me unless there is something specific to socfpga.
>>>>>> 
>>>>>> One quick check, can you test if it works on reverting the trigger
>>>>>> base in arch/arm/dts/socfpga.dtsi from <0x00000000 0x0010> to
>>>>>> <0xffa00000 0x1000>.
>>>>> 
>>>>> Can you please spin V3 of the patchset first, one which omits the buggy
>>>>> bits, so I can test it on a more final form of the patches ? Please
>>>>> keep me on CC, I'll test it then.
>>>> 
>>>> Once i add sram level test patches, i would not be able to test it on my
>>>> platform. This particular issue is pending for long time. I suggest to
>>>> debug (at least with above mentioned quick test) this particular
>>>> issue/patch with current version v2. There is only minor change (format
>>>> string) in this patch for v3.
>>> 
>>> That's fine, just send the V3 as RFT and I'll check it.
>> 
>> If this patch is not debugged, there is no major significance of this
>> patchset. I would appreciate if you do some test with current v2.
> 
> I stop here for today and will get back to this in a few days again.
> Please send V3 until then, I don't want to dig in old/broken code.

Ok, I will send the v3 tomorrow.

Rgds,
Vikas

> 
> Best regards,
> Marek Vasut

[U-Boot] [v2 2/6] spi: cadence_qspi: remove sram polling from flash read

[Marek Vasut]

On Friday, August 14, 2015 at 03:05:57 AM, vikas wrote:
> Hi,

Hi!

> On 08/13/2015 06:03 PM, Marek Vasut wrote:
> > On Friday, August 14, 2015 at 02:46:20 AM, vikas wrote:
> >> Hi,
> > 
> > Hi,
> > 
> > [...]
> > 
> >>>>>>> I don't want to have two different codepaths in the codebase, one
> >>>>>>> of which is buggy. So no, I disagree we should add this option. I
> >>>>>>> also don't think it would be such a performance improvement, so I
> >>>>>>> only see downsides in such a code.
> >>>>>> 
> >>>>>> I expected the same answer :-) & agree also.
> >>>>> 
> >>>>> Heh, thanks :-)
> >>>>> 
> >>>>>> ok, the issue is SRAM Fill Level register is not being updated on my
> >>>>>> SOC, seems like design issue. Any idea how can i add this fix (to
> >>>>>> avoid sram level polling) to my soc in u-boot mainline.
> >>>>> 
> >>>>> Mask all interrupts in the controller, set watermark to N bytes
> >>>>> (depending on the length of your transfer) and poll the interrupt
> >>>>> status register until the watermark is reached ? Is this possible ?
> >>>> 
> >>>> Watermark is for sram level and might not work as well.
> >>>> In general also for such soc/platform issues for any driver, any idea
> >>>> how to apply fixes.
> >>> 
> >>> That's a good question . Is there any way on the ST chip to avoid such
> >>> direct blocking read which can stall the CPU core indefinitelly? Can
> >>> you check with the chip designers ?
> >> 
> >> I already checked & it does not exist in the hardware. Solution to that
> >> are timeout interrupts in indirect mode or use only direct mode. In
> >> case of direct mode, the u-boot driver can't be used.
> > 
> > Why can't you use direct mode ? Wouldn't that solve your problem (with
> > some performance penalty probably) ?
> 
> u-boot driver does not support direct mode.

What about implementing support for direct mode ? :)

> >> But again, can we add some fix for stv0991 arch to avoid sram level.
> >> Without that we will not be able to use the driver at all for stv0991.
> >> 
> >> In general, again it seems not a unique problem. Any idea how to apply
> >> fixes for such peripheral design bugs.
> > 
> > What do you mean it's not unique please ?
> 
> SOCs have one or the other design bugs often & there should be some way to
> apply fixes/workrounds.

They do, but unless we precisely know what the problem is, we're kept in
the dark. If it's really the case that your SRAM level is not begin updated,
then you might want to add DT prop, something like "cdns,broken-sram-level",
pull out the sram level checking code and if this prop is set, simple report
that the SRAM level is "full".

But I wonder, is your SRAM level counter not updating or is your FIFO just 
overflowing (ie. SRAM level indicator is rolling over) ? What are the real
symptoms ?

> > One option would be to pull out the SRAM level check into a separate
> > function and in that function check whether the controller is the one on
> > ST chip (via compatible string) and if it is, return fake "full" SRAM
> > level. You'd then fall back to the blocking read. I don't like the idea
> > , but if there is no other way ...
> 
> It seems ok to me, I can send the patch for the same. Please let me know.

Thinking about it a bit further, adding a DT prop for this might be a better
idea than the compatible string.

Best regards,