spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

v2:
- Use bounce buffer
- Link to v1: https://patchwork.ozlabs.org/patch/693069/

 drivers/spi/cadence_qspi_apb.c   | 26 ++++++++++++++++++++------
 include/configs/k2g_evm.h        |  1 +
 include/configs/socfpga_common.h |  1 +
 include/configs/stv0991.h        |  1 +
 4 files changed, 23 insertions(+), 6 deletions(-)

drivers/spi/cadence_qspi_apb.c +20 -6

index e285d3c1e761..6ce98acf747d 100644
@@ -30,6 +30,7 @@
 #include <linux/errno.h>
 #include <wait_bit.h>
 #include <spi.h>
+#include <bouncebuf.h>
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US			(1) /* 1us */

@@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 	unsigned int remaining = n_tx;
 	unsigned int write_bytes;
 	int ret;
+	struct bounce_buffer bb;
+	u8 *bb_txbuf;
+
+	/*
+	 * Handle non-4-byte aligned accesses via bounce buffer to
+	 * avoid data abort.
+	 */
+	ret = bounce_buffer_start(&bb, (void *)txbuf, n_tx, GEN_BB_READ);
+	if (ret)
+		return ret;
+	bb_txbuf = bb.bounce_buffer;
 
 	/* Configure the indirect read transfer bytes */
 	writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);

@@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
 	while (remaining > 0) {
 		write_bytes = remaining > page_size ? page_size : remaining;
-		/* Handle non-4-byte aligned access to avoid data abort. */
-		if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
-			writesb(plat->ahbbase, txbuf, write_bytes);
-		else
-			writesl(plat->ahbbase, txbuf, write_bytes >> 2);
+		writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
+		if (write_bytes % 4)
+			writesb(plat->ahbbase,
+				bb_txbuf + rounddown(write_bytes, 4),
+				write_bytes % 4);
 
 		ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
 				   CQSPI_REG_SDRAMLEVEL_WR_MASK <<

@@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 			goto failwr;
 		}
 
-		txbuf += write_bytes;
+		bb_txbuf += write_bytes;
 		remaining -= write_bytes;
 	}
 

@@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 		printf("Indirect write completion error (%i)\n", ret);
 		goto failwr;
 	}
+	bounce_buffer_stop(&bb);
 
 	/* Clear indirect completion status */
 	writel(CQSPI_REG_INDIRECTWR_DONE_MASK,

@@ -786,6 +799,7 @@ failwr:
 	/* Cancel the indirect write */
 	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
+	bounce_buffer_stop(&bb);
 	return ret;
 }
 

include/configs/k2g_evm.h +1 -0

index a14544526c71..1d603e0c002f 100644
@@ -79,6 +79,7 @@
 #define CONFIG_CADENCE_QSPI
 #define CONFIG_CQSPI_REF_CLK 384000000
 #define CONFIG_CQSPI_DECODER 0x0
+#define CONFIG_BOUNCE_BUFFER
 #endif
 
 #endif /* __CONFIG_K2G_EVM_H */

include/configs/socfpga_common.h +1 -0

index d37e5958b586..2de57b063334 100644
@@ -208,6 +208,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
 #define CONFIG_CQSPI_REF_CLK		cm_get_qspi_controller_clk_hz()
 #endif
 #define CONFIG_CQSPI_DECODER		0
+#define CONFIG_BOUNCE_BUFFER
 
 /*
  * Designware SPI support

include/configs/stv0991.h +1 -0

index bfd1bd719285..09a3064bd6d6 100644
@@ -74,6 +74,7 @@
 #ifdef CONFIG_OF_CONTROL		/* QSPI is controlled via DT */
 #define CONFIG_CQSPI_DECODER		0
 #define CONFIG_CQSPI_REF_CLK		((30/4)/2)*1000*1000
+#define CONFIG_BOUNCE_BUFFER
 
 #endif
 

Quoted 12 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Quoted 2 lines

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

Quoted 16 lines · Marek Vasut

Quoted 14 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Quoted 6 lines

Reviewed-by Marek Vasut <[email protected]>

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

I don't see any issue with READ due to non word size accesses ATM,

anyways I am working on patch to use BB for READ. Will post that separately.

Thanks for the review!

Quoted 21 lines · Vignesh R

Quoted 18 lines · Marek Vasut

Quoted 7 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

Quoted 7 lines

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

Quoted 6 lines

Quoted 4 lines

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

I don't see any issue with READ due to non word size accesses ATM,

Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?

Quoted 27 lines · Marek Vasut

Quoted 26 lines · Vignesh R

Quoted 22 lines · Marek Vasut

Quoted 7 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

Quoted 7 lines

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

Quoted 3 lines

Quoted 1 line

I don't see any issue with READ due to non word size accesses ATM,

Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?

No issues with that either. The above limitation seems to be only wrt sf write (indirect write).

Quoted 30 lines · Vignesh R

Quoted 27 lines · Marek Vasut

Quoted 26 lines · Vignesh R

Quoted 22 lines · Marek Vasut

Quoted 7 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

Quoted 7 lines

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I don't see any issue with READ due to non word size accesses ATM,

Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?

No issues with that either. The above limitation seems to be only wrt sf write (indirect write).

Because indirect read already uses readsb to handle the alignment , right ?

Quoted 31 lines · Marek Vasut

Quoted 30 lines · Vignesh R

Quoted 27 lines · Marek Vasut

Quoted 26 lines · Vignesh R

Quoted 22 lines · Marek Vasut

Quoted 7 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

Quoted 7 lines

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I don't see any issue with READ due to non word size accesses ATM,

Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?

No issues with that either. The above limitation seems to be only wrt sf write (indirect write).

Because indirect read already uses readsb to handle the alignment , right ?

Alignment is not the problem here, even indirect write uses writesb to handle alignment. But the problem is, driver uses readsb/writesb (which perform byte wise accesses) for reading/writing, if txbuf/rxbuf is unaligned or data length is not a multiple of word size. As per the TI K2G SoC TRM, external master is not permitted to do non 32 bit indirect read/write accesses except for last read/write.
So, if the driver writes say 25 bytes, one byte at a time using writesb, then some bytes are do not get written to flash correctly (instead 0x0 is written).

What my patch is doing is to use bounce buffer so that txbuf is always aligned and writesl can be used instead of writesb. And also make sure that writesb is only to right last trailing bytes in case data length is not a multiple of word size.

But wrt indirect read, I don't see any such data corruption or other issues if driver reads, say 25 bytes, one byte at a time using readsb (though the TRM advises against this)

Quoted 16 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I don't see any issue with READ due to non word size accesses ATM,

Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?

No issues with that either. The above limitation seems to be only wrt sf write (indirect write).

Because indirect read already uses readsb to handle the alignment , right ?

Quoted 9 lines

Alignment is not the problem here, even indirect write uses writesb to handle alignment. But the problem is, driver uses readsb/writesb (which perform byte wise accesses) for reading/writing, if txbuf/rxbuf is unaligned or data length is not a multiple of word size. As per the TI K2G SoC TRM, external master is not permitted to do non 32 bit indirect read/write accesses except for last read/write.
So, if the driver writes say 25 bytes, one byte at a time using writesb, then some bytes are do not get written to flash correctly (instead 0x0 is written).

Well ok, then we have a problem on READ as well.

Quoted 4 lines

What my patch is doing is to use bounce buffer so that txbuf is always aligned and writesl can be used instead of writesb. And also make sure that writesb is only to right last trailing bytes in case data length is not a multiple of word size.

Right.

Quoted 3 lines

But wrt indirect read, I don't see any such data corruption or other issues if driver reads, say 25 bytes, one byte at a time using readsb (though the TRM advises against this)

Correct, so fix the READ path too to be extra sure.

Quoted 16 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
TRM SPRUHY8D[1], the external master is only permitted to issue 32- bit
data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure
that QSPI indirect writes are 32 bit sized except for the last write. If
the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
for all boards that use Cadence QSPI driver.

Quoted 4 lines

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

Quoted 136 lines

v2:
?- Use bounce buffer
?- Link to v1: https://patchwork.ozlabs.org/patch/693069/

?drivers/spi/cadence_qspi_apb.c???| 26 ++++++++++++++++++++------
?include/configs/k2g_evm.h????????|??1 +
?include/configs/socfpga_common.h |??1 +
?include/configs/stv0991.h????????|??1 +

?4 files changed, 23 insertions(+), 6 deletions(-)

drivers/spi/cadence_qspi_apb.c +0 -0

index e285d3c1e761..6ce98acf747d 100644
@@ -30,6 +30,7 @@

?#include <linux/errno.h>
?#include <wait_bit.h>
?#include <spi.h>
+#include <bouncebuf.h>
?#include "cadence_qspi.h"

?#define CQSPI_REG_POLL_US????????????????????(1) /* 1us */

drivers/spi/cadence_qspi_apb.c +0 -0

@@ -741,6 +742,17 @@ int

cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
??????unsigned int remaining = n_tx;
??????unsigned int write_bytes;
??????int ret;
+?????struct bounce_buffer bb;
+?????u8 *bb_txbuf;
+
+?????/*
+??????* Handle non-4-byte aligned accesses via bounce buffer to +??????* avoid data abort.
+??????*/
+?????ret = bounce_buffer_start(&bb, (void *)txbuf, n_tx, GEN_BB_READ);
+?????if (ret)
+?????????????return ret;
+?????bb_txbuf = bb.bounce_buffer;

??????/* Configure the indirect read transfer bytes */ ??????writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);

drivers/spi/cadence_qspi_apb.c +0 -0

@@ -751,11 +763,11 @@ int

cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,

??????while (remaining > 0) {
??????????????write_bytes = remaining > page_size ? page_size :
remaining;
-?????????????/* Handle non-4-byte aligned access to avoid data abort. */
-?????????????if (((uintptr_t)txbuf % 4) || (write_bytes % 4)) -?????????????????????writesb(plat->ahbbase, txbuf, write_bytes); -?????????????else -?????????????????????writesl(plat->ahbbase, txbuf, write_bytes >> 2);
+?????????????writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2); +?????????????if (write_bytes % 4) +?????????????????????writesb(plat->ahbbase, +?????????????????????????????bb_txbuf + rounddown(write_bytes, 4), +?????????????????????????????write_bytes % 4);

??????????????ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL, ?????????????????????????????????CQSPI_REG_SDRAMLEVEL_WR_MASK <<

drivers/spi/cadence_qspi_apb.c +0 -0

@@ -765,7 +777,7 @@ int

cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
??????????????????????goto failwr;
??????????????}

-?????????????txbuf += write_bytes;
+?????????????bb_txbuf += write_bytes;
??????????????remaining -= write_bytes;
??????}

drivers/spi/cadence_qspi_apb.c +0 -0

@@ -776,6 +788,7 @@ int

cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
??????????????printf("Indirect write completion error (%i)\n", ret);
??????????????goto failwr;
??????}
+?????bounce_buffer_stop(&bb);

??????/* Clear indirect completion status */
??????writel(CQSPI_REG_INDIRECTWR_DONE_MASK,

drivers/spi/cadence_qspi_apb.c +0 -0

@@ -786,6 +799,7 @@ failwr:

??????/* Cancel the indirect write */ ??????writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK, ?????????????plat->regbase + CQSPI_REG_INDIRECTWR); +?????bounce_buffer_stop(&bb);
??????return ret;
?}

include/configs/k2g_evm.h +0 -0

index a14544526c71..1d603e0c002f 100644
@@ -79,6 +79,7 @@

?#define CONFIG_CADENCE_QSPI
?#define CONFIG_CQSPI_REF_CLK 384000000
?#define CONFIG_CQSPI_DECODER 0x0
+#define CONFIG_BOUNCE_BUFFER
?#endif

?#endif /* __CONFIG_K2G_EVM_H */

include/configs/socfpga_common.h +0 -0

index d37e5958b586..2de57b063334 100644
@@ -208,6 +208,7 @@ unsigned int
 cm_get_qspi_controller_clk_hz(void);

?#define CONFIG_CQSPI_REF_CLK?????????cm_get_qspi_controller_clk_hz() ?#endif
?#define CONFIG_CQSPI_DECODER?????????0
+#define CONFIG_BOUNCE_BUFFER

?/*
? * Designware SPI support

include/configs/stv0991.h +0 -0

index bfd1bd719285..09a3064bd6d6 100644
@@ -74,6 +74,7 @@

?#ifdef CONFIG_OF_CONTROL?????????????/* QSPI is controlled via DT */
?#define CONFIG_CQSPI_DECODER?????????0
?#define CONFIG_CQSPI_REF_CLK?????????((30/4)/2)*1000*1000 +#define CONFIG_BOUNCE_BUFFER

Quoted 3 lines

?#endif

--
Best regards,
Marek Vasut

________________________________

Confidentiality Notice.
This message may contain information that is confidential or otherwise protected from disclosure. If you are not the intended recipient, you are hereby notified that any use, disclosure, dissemination, distribution, or copying of this message, or any attachments, is strictly prohibited. If you have received this message in error, please advise the sender by reply e-mail, and delete the message and any attachments. Thank you.

Quoted 23 lines · Marek Vasut

Quoted 12 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
TRM SPRUHY8D[1], the external master is only permitted to issue 32- bit
data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure
that QSPI indirect writes are 32 bit sized except for the last write. If
the txbuf is unaligned then use bounce buffer to avoid data aborts.

Quoted 8 lines

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

Quoted 1 line

I'd like to have at least Dinh's/Chin's ack on this.

THanks Marek

Hmmm... From 11.15.4.1.1, the data slave port should able to accept only byte, half-word and word access. This should not create any data abort but probably bad performance. But it should insignificant as access time for the flash is longer than the data port access itself.

Quoted 32 lines · Chin Liang

Quoted 23 lines · Marek Vasut

Quoted 21 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G
SoC
TRM SPRUHY8D[1], the external master is only permitted to issue 32-
bit
data interface writes until the last word of an indirect transfer
otherwise indirect writes is known to fails sometimes. So, make sure
that QSPI indirect writes are 32 bit sized except for the last write. If
the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
for all boards that use Cadence QSPI driver.

Quoted 7 lines

Quoted 4 lines

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

Reviewed-by Marek Vasut <[email protected]>

I'd like to have at least Dinh's/Chin's ack on this.

Quoted 9 lines

THanks Marek

Hmmm... From 11.15.4.1.1, the data slave port should able to accept only byte, half-word and word access. This should not create any data
abort but probably bad performance. But it should insignificant as access time for the flash is longer than the data port access itself.

Data slave port does accept byte, half-word and word access, there are
no data aborts. But indirect write controller seems to have limitation(as documented in section 11.15.4.9.2) couping with non 32- bit
data writes on TI platform. For example with current driver if I try:

fatload mmc 0 0x82000000 zImage
sf erase 0x0 0x500000
sf write 0x82000000 0x0 0x35
sf read 0xA0000000 0x0 0x35

md.b 0xA0000000
a0000000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
a0000030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00 md.b 0x82000000
82000000: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000010: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000020: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00 82000030: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1

As you can see, every fourth byte turn out to be 0x00. Therefore this patch is required.

Quoted 33 lines

Quoted 8 lines

Hmmm... From 11.15.4.1.1, the data slave port should able to accept only byte, half-word and word access. This should not create any data
abort but probably bad performance. But it should insignificant as access time for the flash is longer than the data port access itself.

Data slave port does accept byte, half-word and word access, there are
no data aborts. But indirect write controller seems to have limitation(as documented in section 11.15.4.9.2) couping with non 32- bit
data writes on TI platform. For example with current driver if I try:

fatload mmc 0 0x82000000 zImage
sf erase 0x0 0x500000
sf write 0x82000000 0x0 0x35
sf read 0xA0000000 0x0 0x35

md.b 0xA0000000
a0000000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
a0000030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00 md.b 0x82000000
82000000: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000010: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000020: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00 82000030: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1

Quoted 2 lines

As you can see, every fourth byte turn out to be 0x00. Therefore this patch is required.

Thanks Vignesh

Interesting to know that the newer version of controller has this constrain. Let me pull out my board to ensure this patch doesn't break the SOCFPGA

Quoted 28 lines

Quoted 27 lines

Data slave port does accept byte, half-word and word access, there are
no data aborts. But indirect write controller seems to have limitation(as documented in section 11.15.4.9.2) couping with non 32- bit
data writes on TI platform. For example with current driver if I try:

fatload mmc 0 0x82000000 zImage
sf erase 0x0 0x500000
sf write 0x82000000 0x0 0x35
sf read 0xA0000000 0x0 0x35

md.b 0xA0000000
a0000000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
a0000030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00 md.b 0x82000000
82000000: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000010: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000020: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00 82000030: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1

As you can see, every fourth byte turn out to be 0x00. Therefore this patch is required.

Quoted 6 lines

Thanks Vignesh

Interesting to know that the newer version of controller has this constrain. Let me pull out my board to ensure this patch doesn't break the SOCFPGA

Hi,

On Thursday 01 December 2016 09:41 AM, Vignesh R wrote:
[...]

Quoted 37 lines

Quoted 37 lines

Quoted 37 lines

Data slave port does accept byte, half-word and word access, there
are
no data aborts. But indirect write controller seems to have limitation(as documented in section 11.15.4.9.2) couping with non 32-
bit
data writes on TI platform. For example with current driver if I try:

fatload mmc 0 0x82000000 zImage
sf erase 0x0 0x500000
sf write 0x82000000 0x0 0x35
sf read 0xA0000000 0x0 0x35

md.b 0xA0000000
a0000000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a0000020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
a0000030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00 md.b 0x82000000
82000000: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000010: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 82000020: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00 82000030: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1

As you can see, every fourth byte turn out to be 0x00. Therefore this
patch is required.

Quoted 7 lines

Quoted 7 lines

Thanks Vignesh

Interesting to know that the newer version of controller has this constrain. Let me pull out my board to ensure this patch doesn't break
the SOCFPGA

Did you get a chance to test this patch? There is also a similar patch
for indirect read as well[1], it would be great if you could give your
Tested-by for both the patches. Thanks!

[1]https://patchwork.ozlabs.org/patch/700990/

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Did you get a chance to test this patch? There is also a similar patch
for indirect read as well[1], it would be great if you could give your
Tested-by for both the patches. Thanks!

[1]https://patchwork.ozlabs.org/patch/700990/

Actually I was bumping into sf probe error. This is happen at mainline even without your patch. Let me continue the troubelshooting at my side

and test out your patch asap.

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

v2:
- Use bounce buffer
- Link to v1: https://patchwork.ozlabs.org/patch/693069/

 drivers/spi/cadence_qspi_apb.c   | 26 ++++++++++++++++++++------
 include/configs/k2g_evm.h        |  1 +
 include/configs/socfpga_common.h |  1 +
 include/configs/stv0991.h        |  1 +
 4 files changed, 23 insertions(+), 6 deletions(-)

drivers/spi/cadence_qspi_apb.c +20 -6

index e285d3c1e761..6ce98acf747d 100644
@@ -30,6 +30,7 @@
 #include <linux/errno.h>
 #include <wait_bit.h>
 #include <spi.h>
+#include <bouncebuf.h>
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US                      (1) /* 1us */

@@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
        unsigned int remaining = n_tx;
        unsigned int write_bytes;
        int ret;
+       struct bounce_buffer bb;
+       u8 *bb_txbuf;
+
+       /*
+        * Handle non-4-byte aligned accesses via bounce buffer to
+        * avoid data abort.
+        */
+       ret = bounce_buffer_start(&bb, (void *)txbuf, n_tx, GEN_BB_READ);
+       if (ret)
+               return ret;
+       bb_txbuf = bb.bounce_buffer;
 
        /* Configure the indirect read transfer bytes */
        writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);

@@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
        while (remaining > 0) {
                write_bytes = remaining > page_size ? page_size : remaining;
-               /* Handle non-4-byte aligned access to avoid data abort. */
-               if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
-                       writesb(plat->ahbbase, txbuf, write_bytes);
-               else
-                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
+               writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
+               if (write_bytes % 4)
+                       writesb(plat->ahbbase,
+                               bb_txbuf + rounddown(write_bytes, 4),
+                               write_bytes % 4);
 
                ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
                                   CQSPI_REG_SDRAMLEVEL_WR_MASK <<

@@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
                        goto failwr;
                }
 
-               txbuf += write_bytes;
+               bb_txbuf += write_bytes;
                remaining -= write_bytes;
        }
 

@@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
                printf("Indirect write completion error (%i)\n", ret);
                goto failwr;
        }
+       bounce_buffer_stop(&bb);
 
        /* Clear indirect completion status */
        writel(CQSPI_REG_INDIRECTWR_DONE_MASK,

@@ -786,6 +799,7 @@ failwr:
        /* Cancel the indirect write */
        writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
               plat->regbase + CQSPI_REG_INDIRECTWR);
+       bounce_buffer_stop(&bb);
        return ret;
 }
 

include/configs/k2g_evm.h +0 -0

index a14544526c71..1d603e0c002f 100644

Quoted 97 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

v2:
- Use bounce buffer
- Link to v1: https://patchwork.ozlabs.org/patch/693069/

 drivers/spi/cadence_qspi_apb.c   | 26 ++++++++++++++++++++------
 include/configs/k2g_evm.h        |  1 +
 include/configs/socfpga_common.h |  1 +
 include/configs/stv0991.h        |  1 +
 4 files changed, 23 insertions(+), 6 deletions(-)

drivers/spi/cadence_qspi_apb.c +20 -6

index e285d3c1e761..6ce98acf747d 100644
@@ -30,6 +30,7 @@
 #include <linux/errno.h>
 #include <wait_bit.h>
 #include <spi.h>
+#include <bouncebuf.h>
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US                      (1) /* 1us */

@@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
        unsigned int remaining = n_tx;
        unsigned int write_bytes;
        int ret;
+       struct bounce_buffer bb;
+       u8 *bb_txbuf;
+
+       /*
+        * Handle non-4-byte aligned accesses via bounce buffer to
+        * avoid data abort.
+        */
+       ret = bounce_buffer_start(&bb, (void *)txbuf, n_tx, GEN_BB_READ);
+       if (ret)
+               return ret;
+       bb_txbuf = bb.bounce_buffer;
 
        /* Configure the indirect read transfer bytes */
        writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);

@@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
        while (remaining > 0) {
                write_bytes = remaining > page_size ? page_size : remaining;
-               /* Handle non-4-byte aligned access to avoid data abort. */
-               if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
-                       writesb(plat->ahbbase, txbuf, write_bytes);
-               else
-                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
+               writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
+               if (write_bytes % 4)
+                       writesb(plat->ahbbase,
+                               bb_txbuf + rounddown(write_bytes, 4),
+                               write_bytes % 4);
 
                ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
                                   CQSPI_REG_SDRAMLEVEL_WR_MASK <<

@@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
                        goto failwr;
                }
 
-               txbuf += write_bytes;
+               bb_txbuf += write_bytes;
                remaining -= write_bytes;
        }
 

@@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
                printf("Indirect write completion error (%i)\n", ret);
                goto failwr;
        }
+       bounce_buffer_stop(&bb);
 
        /* Clear indirect completion status */
        writel(CQSPI_REG_INDIRECTWR_DONE_MASK,

@@ -786,6 +799,7 @@ failwr:
        /* Cancel the indirect write */
        writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
               plat->regbase + CQSPI_REG_INDIRECTWR);
+       bounce_buffer_stop(&bb);
        return ret;
 }
 

include/configs/k2g_evm.h line 82

Quoted 9 lines

include/configs/k2g_evm.h +1 -0

index a14544526c71..1d603e0c002f 100644
@@ -79,6 +79,7 @@
 #define CONFIG_CADENCE_QSPI
 #define CONFIG_CQSPI_REF_CLK 384000000
 #define CONFIG_CQSPI_DECODER 0x0
+#define CONFIG_BOUNCE_BUFFER

Better define this on Kconfig and add it on defconfig.

Quoted 98 lines · Jagan Teki

Quoted 97 lines · Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit data interface writes until the last word of an indirect transfer otherwise indirect writes is known to fails sometimes. So, make sure that QSPI indirect writes are 32 bit sized except for the last write. If the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by Vignesh R <[email protected]>

---

v2:
- Use bounce buffer
- Link to v1: https://patchwork.ozlabs.org/patch/693069/

 drivers/spi/cadence_qspi_apb.c   | 26 ++++++++++++++++++++------
 include/configs/k2g_evm.h        |  1 +
 include/configs/socfpga_common.h |  1 +
 include/configs/stv0991.h        |  1 +
 4 files changed, 23 insertions(+), 6 deletions(-)

drivers/spi/cadence_qspi_apb.c +20 -6

index e285d3c1e761..6ce98acf747d 100644
@@ -30,6 +30,7 @@
 #include <linux/errno.h>
 #include <wait_bit.h>
 #include <spi.h>
+#include <bouncebuf.h>
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US                      (1) /* 1us */

@@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
        unsigned int remaining = n_tx;
        unsigned int write_bytes;
        int ret;
+       struct bounce_buffer bb;
+       u8 *bb_txbuf;
+
+       /*
+        * Handle non-4-byte aligned accesses via bounce buffer to
+        * avoid data abort.
+        */
+       ret = bounce_buffer_start(&bb, (void *)txbuf, n_tx, GEN_BB_READ);
+       if (ret)
+               return ret;
+       bb_txbuf = bb.bounce_buffer;
 
        /* Configure the indirect read transfer bytes */
        writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);

@@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
        while (remaining > 0) {
                write_bytes = remaining > page_size ? page_size : remaining;
-               /* Handle non-4-byte aligned access to avoid data abort. */
-               if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
-                       writesb(plat->ahbbase, txbuf, write_bytes);
-               else
-                       writesl(plat->ahbbase, txbuf, write_bytes >> 2);
+               writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
+               if (write_bytes % 4)
+                       writesb(plat->ahbbase,
+                               bb_txbuf + rounddown(write_bytes, 4),
+                               write_bytes % 4);
 
                ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
                                   CQSPI_REG_SDRAMLEVEL_WR_MASK <<

@@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
                        goto failwr;
                }
 
-               txbuf += write_bytes;
+               bb_txbuf += write_bytes;
                remaining -= write_bytes;
        }
 

@@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
                printf("Indirect write completion error (%i)\n", ret);
                goto failwr;
        }
+       bounce_buffer_stop(&bb);
 
        /* Clear indirect completion status */
        writel(CQSPI_REG_INDIRECTWR_DONE_MASK,

@@ -786,6 +799,7 @@ failwr:
        /* Cancel the indirect write */
        writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
               plat->regbase + CQSPI_REG_INDIRECTWR);
+       bounce_buffer_stop(&bb);
        return ret;
 }
 

Quoted 11 lines

include/configs/k2g_evm.h line 82

Quoted 9 lines

include/configs/k2g_evm.h +1 -0

index a14544526c71..1d603e0c002f 100644
@@ -79,6 +79,7 @@
 #define CONFIG_CADENCE_QSPI
 #define CONFIG_CQSPI_REF_CLK 384000000
 #define CONFIG_CQSPI_DECODER 0x0
+#define CONFIG_BOUNCE_BUFFER

Better define this on Kconfig and add it on defconfig.

Such change is unrelated to this patch and should be fixed in a separate/subsequent one.

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 11 lines

Quoted 9 lines

include/configs/k2g_evm.h line 82

Quoted 9 lines

include/configs/k2g_evm.h +1 -0

index a14544526c71..1d603e0c002f 100644
@@ -79,6 +79,7 @@
 #define CONFIG_CADENCE_QSPI
 #define CONFIG_CQSPI_REF_CLK 384000000
 #define CONFIG_CQSPI_DECODER 0x0
+#define CONFIG_BOUNCE_BUFFER

Better define this on Kconfig and add it on defconfig.

Quoted 2 lines

Quoted 2 lines

Such change is unrelated to this patch and should be fixed in a separate/subsequent one.

Agreed it should be a separate patch.