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

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

[Vikas Manocha]

This patchset:
- 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 v7:
- added info for renaming the parameter from ahbbase to flashbase
- created separate patch to remove the unused macro

Changes in v6:
- fixed binding for trigger-address.

Changes in v5:
- fixed compilation warnings.

Changes in v4:
- fifo-width & trigger address alligned to linux device tree binding.
- removed un-necessary casting.
- renaming of one parameter splitted to separate patch.
- trigger address of socfpga reverted back to 0x0.
- code formatting done to avoid checkpatch CHECKS.

Changes in v3:
- removed two patches which were bypassing the sram level check.
- format string in patch corrected 3/4
- added commit message in patch 1/4

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: fix indirect read/write start address
  spi: cadence_qspi: fix base trigger address & transfer start address
  spi: cadence_qspi: rename ahbbase to flashbase for clarity
  spi: cadence_qspi: remove unused macro
  spi: cadence_qspi: get fifo width from device tree

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

-- 
1.7.9.5

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

[Vikas Manocha]

No need to configure indirect trigger address for every read/write.

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

Changes in v7: None
Changes in v6: None
Changes in v5: fixed type cast compilation warnings.
Changes in v4: removed extra type casts.
Changes in v3: added commit message & removed extra bracket.
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..d377ad1 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] [PATCH v7 2/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 v7: none
Changes in v6: none
Changes in v5: fixed type cast compilation warnings.
Changes in v4: removed extra type casts.
Changes in v3: none
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 d377ad1..c5b14c5 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -705,7 +705,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;
@@ -795,7 +796,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] [PATCH v7 3/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->triggerbase is added in device tree for mapped spi flash address.

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

Changes in v7: None.
Changes in v6: fixed binding for trigger-address.
Changes in v5: None.
Changes in v4:
- fifo-width & trigger address alligned to linux device tree binding.
- renaming of one parameter moved to separate patch.
- trigger address of socfpga reverted back to 0x0.

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

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index 9b12420..9756544 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -639,6 +639,7 @@
 			ext-decoder = <0>;  /* external decoder */
 			num-cs = <4>;
 			fifo-depth = <128>;
+			cdns,trigger-address = <0x00000000>;
 			sram-size = <128>;
 			bus-num = <2>;
 			status = "disabled";
diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
index fa3fd64..6bc5372 100644
--- a/arch/arm/dts/stv0991.dts
+++ b/arch/arm/dts/stv0991.dts
@@ -33,6 +33,7 @@
 				<0x40000000 0x1000000>;
 			clocks = <3750000>;
 			sram-size = <256>;
+			cdns,trigger-address = <0x40000000>;
 			status = "okay";
 
 			flash0: n25q32 at 0 {
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 34a0f46..0d1abc8 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -290,6 +290,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 
 	plat->regbase = (void *)data[0];
 	plat->ahbbase = (void *)data[2];
+	plat->trigger_base = (u32 *)fdtdec_get_addr(blob, node,
+						    "cdns,trigger-address");
 
 	/* Use 500KHz as a suitable default */
 	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
@@ -311,10 +313,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 ahbbase=%p trigger_base=%p max-frequency=%d page-size=%d\n",
+	      __func__, plat->regbase, plat->ahbbase, 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..2f1bd92 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -18,6 +18,7 @@ struct cadence_spi_platdata {
 	unsigned int	max_hz;
 	void		*regbase;
 	void		*ahbbase;
+	void		*trigger_base;
 
 	u32		page_size;
 	u32		block_size;
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index c5b14c5..8156b2b 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)
@@ -281,7 +280,7 @@ 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;
+	void *dest_addr = plat->trigger_base;
 	unsigned int retry = CQSPI_REG_RETRY;
 	unsigned int sram_level;
 	unsigned int wr_bytes;
@@ -534,7 +533,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 */
@@ -753,7 +752,7 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 
 	if (qspi_read_sram_fifo_poll(plat->regbase, (void *)rxbuf,
-				     (const void *)plat->ahbbase, rxlen))
+				     (const void *)plat->trigger_base, rxlen))
 		goto failrd;
 
 	/* Check flash indirect controller */
-- 
1.7.9.5

[U-Boot] [PATCH v7 4/6] spi: cadence_qspi: rename ahbbase to flashbase for clarity

[Vikas Manocha]

plat->ahbbase renamed to plat->flashbase for better clarity.
With ahbbase it was not clear which base address it is, flashbase makes
it clear that it is mapped flash memory base address.

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

Changes in v7: added info for renaming the parameter from ahbbase to flashbase
Changes in v6: none
Changes in v5: none
Changes in v4: new

 drivers/spi/cadence_qspi.c     |    8 ++++----
 drivers/spi/cadence_qspi.h     |    4 ++--
 drivers/spi/cadence_qspi_apb.c |    4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 0d1abc8..c63f583 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);
@@ -289,7 +289,7 @@ 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 = (u32 *)fdtdec_get_addr(blob, node,
 						    "cdns,trigger-address");
 
@@ -313,8 +313,8 @@ 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 trigger_base=%p max-frequency=%d page-size=%d\n",
-	      __func__, plat->regbase, plat->ahbbase, plat->trigger_base,
+	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 2f1bd92..7341339 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -17,7 +17,7 @@
 struct cadence_spi_platdata {
 	unsigned int	max_hz;
 	void		*regbase;
-	void		*ahbbase;
+	void		*flashbase;
 	void		*trigger_base;
 
 	u32		page_size;
@@ -31,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 8156b2b..2638f00 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -704,7 +704,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. */
@@ -795,7 +795,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] [PATCH v7 5/6] spi: cadence_qspi: remove unused macro

[Vikas Manocha]

This macro is not being used anywhere in the code.

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

Changes in v7: new.

 drivers/spi/cadence_qspi_apb.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index 2638f00..c0116e3 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -48,9 +48,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)
  ****************************************************************************/
-- 
1.7.9.5

[U-Boot] [PATCH v7 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 v7: unused macro moved to separate patch.
Changes in v6: none
Changes in v5: none
Changes in v4: alligned to linux device tree binding.
Changes in v3: none
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 |   21 ++++++++++-----------
 5 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index 9756544..5f0b0fa 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -640,6 +640,7 @@
 			num-cs = <4>;
 			fifo-depth = <128>;
 			cdns,trigger-address = <0x00000000>;
+			cdns,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 6bc5372..0a88b69 100644
--- a/arch/arm/dts/stv0991.dts
+++ b/arch/arm/dts/stv0991.dts
@@ -34,6 +34,7 @@
 			clocks = <3750000>;
 			sram-size = <256>;
 			cdns,trigger-address = <0x40000000>;
+			cdns,fifo-width = <8>;
 			status = "okay";
 
 			flash0: n25q32 at 0 {
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index c63f583..0430218 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -312,6 +312,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, "cdns,fifo-width", 4);
 
 	debug("%s: regbase=%p flashbase=%p trigger_base=%p max-frequency=%d page-size=%d\n",
 	      __func__, plat->regbase, plat->flashbase, plat->trigger_base,
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 c0116e3..478b5a0 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 */
@@ -211,7 +209,7 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
 }
 
 static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
-	const void *src, unsigned int bytes)
+	const void *src, unsigned int fifo_width, unsigned int bytes)
 {
 	unsigned int temp = 0;
 	int i;
@@ -219,11 +217,11 @@ static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
 	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--)
+	while (remaining >= fifo_width) {
+		for (i = 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;
+		src_ptr += fifo_width/sizeof(src_ptr);
+		remaining -= fifo_width;
 	}
 	if (remaining) {
 		/* dangling bytes */
@@ -238,7 +236,7 @@ static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
 
 /* 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)
+	const void *src_addr, unsigned int fifo_width, unsigned int num_bytes)
 {
 	unsigned int remaining = num_bytes;
 	unsigned int retry;
@@ -260,7 +258,7 @@ static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
 			return -1;
 		}
 
-		sram_level *= CQSPI_FIFO_WIDTH;
+		sram_level *= fifo_width;
 		sram_level = sram_level > remaining ? remaining : sram_level;
 
 		/* Read data from FIFO. */
@@ -302,7 +300,8 @@ static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
 		wr_bytes = (remaining > page_size) ?
 					page_size : remaining;
 
-		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
+		cadence_qspi_apb_write_fifo_data(dest_addr, src,
+						 plat->fifo_width, wr_bytes);
 		src += wr_bytes;
 		remaining -= wr_bytes;
 	}
@@ -749,7 +748,7 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 
 	if (qspi_read_sram_fifo_poll(plat->regbase, (void *)rxbuf,
-				     (const void *)plat->trigger_base, rxlen))
+		(const void *)plat->trigger_base, plat->fifo_width, rxlen))
 		goto failrd;
 
 	/* Check flash indirect controller */
-- 
1.7.9.5

[U-Boot] [PATCH v7 2/6] spi: cadence_qspi: fix indirect read/write start address

[Wolfgang Denk]

Dear Vikas,

In message <1443053976-9112-3-git-send-email-vikas.manocha@st.com> you wrote:
> Indirect read/write start addresses are flash start addresses for indirect read
> or write transfers. These should be absolute flash addresses instead of
> offsets.
...
>  	/* 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);
...
>  	/* 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);

As has been pointed out before, adding type casts to silence compiler
warnings is inherently wrong.  The compiler issues a warning here to
tell you that your code is broken, so you should fix your code and not
silence the warning to paper over it.

Please fix.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Voodoo Programming: Things programmers do that  they  know  shouldn't
work  but they try anyway, and which sometimes actually work, such as
recompiling everything.                             - Karl Lehenbauer

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

[Wolfgang Denk]

Dear Vikas,

In message <1443053976-9112-2-git-send-email-vikas.manocha@st.com> you wrote:
> No need to configure indirect trigger address for every read/write.
...
>  	/* 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);

I did not mention this explicitly, so I do it here:

Please fix this type cast issue globally, in all your patches.
Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Marriage is like a cage; one sees the birds outside desperate to get
in, and those inside desperate to get out."               - Montaigne

[U-Boot] [PATCH v7 3/6] spi: cadence_qspi: fix base trigger address & transfer start address

[Wolfgang Denk]

Dear Vikas,

In message <1443053976-9112-4-git-send-email-vikas.manocha@st.com> you wrote:
> 
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -290,6 +290,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
>  
>  	plat->regbase = (void *)data[0];
>  	plat->ahbbase = (void *)data[2];
> +	plat->trigger_base = (u32 *)fdtdec_get_addr(blob, node,
> +						    "cdns,trigger-address");

What happens if fdtdec_get_addr() returns FDT_ADDR_T_NONE?

I think we should implement some error handling here.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The whole world is about three drinks behind."     - Humphrey Bogart

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

[Wolfgang Denk]

Dear Vikas Manocha,

In message <1443053976-9112-7-git-send-email-vikas.manocha@st.com> you wrote:
> Fifo width could be different on different socs, e.g. stv0991 & altera soc
> have different fifo width.
...
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -312,6 +312,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, "cdns,fifo-width", 4);

Is using default values (which are known to be wrong on some
platforms) the right approach here?  It means that some systems will
silently misperform when the respective entries in the DT are missing.

Should we not add proper error handling here?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Nobody trips over mountains. It is the small pebble that  causes  you
to  stumble.  Pass all the pebbles in your path and you will find you
have crossed the mountain.

[U-Boot] [PATCH v7 3/6] spi: cadence_qspi: fix base trigger address & transfer start address

[Wolfgang Denk]

Dear Vikas Manocha,

In message <1443053976-9112-4-git-send-email-vikas.manocha@st.com> you wrote:
> This patch is to separate the base trigger from the read/write transfer start
> addresses.
...
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 34a0f46..0d1abc8 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -290,6 +290,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
>  
>  	plat->regbase = (void *)data[0];
>  	plat->ahbbase = (void *)data[2];

I realize this code is not new, but for the sake of consistency, the
above two regbase and ahbbase should also use fdtdec_get_addr() ...

Can you please fix that [and similar things, in case there any]?

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"He was so narrow minded he could see through  a  keyhole  with  both
eyes ..."

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

[Vikas MANOCHA]

Thanks Wolfgang,

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Thursday, September 24, 2015 12:22 AM
> To: Vikas MANOCHA
> Cc: u-boot at lists.denx.de; sr at denx.de; grmoore at opensource.altera.com;
> jteki at openedev.com; marex at denx.de
> Subject: Re: [U-Boot] [PATCH v7 1/6] spi: cadence_qspi: move trigger base
> configuration in init
> 
> Dear Vikas,
> 
> In message <1443053976-9112-2-git-send-email-vikas.manocha@st.com>
> you wrote:
> > No need to configure indirect trigger address for every read/write.
> ...
> >  	/* 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);
> 
> I did not mention this explicitly, so I do it here:
> 
> Please fix this type cast issue globally, in all your patches.

I agree it should be done but this patchset is not introducing the typecasting, it only moves the statement to another logical location.
e.g. the above code is not new, it was just moved from other location to init function.

This fix to remove typecasting from all variables (triggerbase, flashbase,regbase) is a significant change in many routines in terms of parameters passing/handling & deserve separate patch/set.
I am ready to send a separate patch/set for the same later.  Please let me know if you agree.

Rgds,
Vikas

> Thanks.
> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> "Marriage is like a cage; one sees the birds outside desperate to get
> in, and those inside desperate to get out."               - Montaigne

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

[Wolfgang Denk]

Dear Vikas,

In message <9026814FBF99304F9FA3AC3FB72F3E2F04BFF85EFE@SAFEX1MAIL4.st.com> you wrote:
>
> > CQSPI_INDIRECTTRIGGER_ADDR_MASK,
> > > +	       plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
> > 
> > I did not mention this explicitly, so I do it here:
> > 
> > Please fix this type cast issue globally, in all your patches.
>
> I agree it should be done but this patchset is not introducing the
> typecasting, it only moves the statement to another logical location.
> e.g. the above code is not new, it was just moved from other location
> to init function.

I am fully aware of that.  But I feel if we touch a piece of code, and
notice it has issues, we should fix these while we are at it.
Otherwise there is always the risk that we add more and more such bad
code, and/or forget about cleaning up later.

> This fix to remove typecasting from all variables (triggerbase,
> flashbase,regbase) is a significant change in many routines in terms
> of parameters passing/handling & deserve separate patch/set.

Hm... this makes me wander about the overall code quality.  I have to
admit that I don't have any in-depth understanding of this specific
driver, but it looks.... well, let's state it looks pretty much
dfferent from the corresponding Linux kernel driver code.  Which does
not have such issues.  So if you say it would take _significant_
efforts to clean up this implementation  I'm asking myuself how much
more (or less?) effort would it take to adapt the Linux driver for
U-Boot instead?  Having a common driver code base has been very
beneficial in a number of other areas, too...

> I am ready to send a separate patch/set for the same later. Please
> let me know if you agree.

If we follow strict rules, such a cleanup patch should go in first.

Thanks.

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It seems intuitively obvious to me, which  means  that  it  might  be
wrong.                                                 -- Chris Torek

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

[Jagan Teki]

Hi Wolfgang,

On 25 September 2015 at 00:26, Wolfgang Denk <wd@denx.de> wrote:
> Dear Vikas,
>
> In message <9026814FBF99304F9FA3AC3FB72F3E2F04BFF85EFE@SAFEX1MAIL4.st.com> you wrote:
>>
>> > CQSPI_INDIRECTTRIGGER_ADDR_MASK,
>> > > +        plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
>> >
>> > I did not mention this explicitly, so I do it here:
>> >
>> > Please fix this type cast issue globally, in all your patches.
>>
>> I agree it should be done but this patchset is not introducing the
>> typecasting, it only moves the statement to another logical location.
>> e.g. the above code is not new, it was just moved from other location
>> to init function.
>
> I am fully aware of that.  But I feel if we touch a piece of code, and
> notice it has issues, we should fix these while we are at it.
> Otherwise there is always the risk that we add more and more such bad
> code, and/or forget about cleaning up later.
>
>> This fix to remove typecasting from all variables (triggerbase,
>> flashbase,regbase) is a significant change in many routines in terms
>> of parameters passing/handling & deserve separate patch/set.
>
> Hm... this makes me wander about the overall code quality.  I have to
> admit that I don't have any in-depth understanding of this specific
> driver, but it looks.... well, let's state it looks pretty much
> dfferent from the corresponding Linux kernel driver code.  Which does
> not have such issues.  So if you say it would take _significant_
> efforts to clean up this implementation  I'm asking myuself how much
> more (or less?) effort would it take to adapt the Linux driver for
> U-Boot instead?  Having a common driver code base has been very
> beneficial in a number of other areas, too...
>
>> I am ready to send a separate patch/set for the same later. Please
>> let me know if you agree.
>
> If we follow strict rules, such a cleanup patch should go in first.

Looks like driver code itself has some issues and Vikas made changes
wrt to existing code, cleaning up existing driver and then make Vikas
changes might be reasonable and time consuming task.

What if we move Vikas changes now for this release as he stated in
previous mail "about sending patches later". My idea is someone is
trying to clean it up existing code then give him a chance to move his
code as well because he sent couple times.

thanks!
-- 
Jagan.

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

[Wolfgang Denk]

Dear Jagan,

In message <CAD6G_RSZAK7u=_ArT8SgqMvrdQ6ULi9szmxhF++kAEw-gv4Erw@mail.gmail.com> you wrote:
> 
> What if we move Vikas changes now for this release as he stated in
> previous mail "about sending patches later".

Well, we've been there too many times before.  In to many cases, the
"later" just did not happen.

> My idea is someone is trying to clean it up existing code then give
> him a chance to move his code as well because he sent couple times.

Well, I think it would be better to clean up first, instead of just
reshuffeling the crappy code.

Also, I would like to hear a comment about comparing the efforts (and
benefits) of cleaning up this stuff versus doing a clean port of the
Linux version of that driver.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Eureka!                                                 -- Archimedes

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

[Vikas MANOCHA]

Thanks Wolfgang,

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Thursday, September 24, 2015 3:32 PM
> To: Jagan Teki
> Cc: Vikas MANOCHA; u-boot at lists.denx.de; marex at denx.de; sr at denx.de;
> grmoore at opensource.altera.com
> Subject: Re: [U-Boot] [PATCH v7 1/6] spi: cadence_qspi: move trigger base
> configuration in init
> 
> Dear Jagan,
> 
> In message <CAD6G_RSZAK7u=_ArT8SgqMvrdQ6ULi9szmxhF++kAEw-
> gv4Erw at mail.gmail.com> you wrote:
> >
> > What if we move Vikas changes now for this release as he stated in
> > previous mail "about sending patches later".
> 
> Well, we've been there too many times before.  In to many cases, the "later"
> just did not happen.

To ensure it this time, I can add a patch for the same on top of these patches in next version :-)

> 
> > My idea is someone is trying to clean it up existing code then give
> > him a chance to move his code as well because he sent couple times.
> 
> Well, I think it would be better to clean up first, instead of just reshuffeling
> the crappy code.
> 
> Also, I would like to hear a comment about comparing the efforts (and
> benefits) of cleaning up this stuff versus doing a clean port of the Linux
> version of that driver.

I think it makes sense.
Although both drivers (uboot, linux) have lot of similarities, there are differences also. It would be good to sync up.

Rgds,
Vikas

> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Eureka!                                                 -- Archimedes