All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM
@ 2019-04-28 20:28 Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 01/19] spi: mpc8xxx: Use short type names Jagan Teki
                   ` (19 more replies)
  0 siblings, 20 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

This would be next version mpc8xxx dm conversion.

No functional changes in v2 apart from rebase, and this would
merge soon.

Changes for v2:
- rebase on master
- patch for SPI MIGRATION status

Any inputs?
Jagan.

Jagan Teki (2):
  spi: mpc8xxx: Convert to DM
  dm: MIGRATION: Update migration status for SPI

Mario Six (17):
  spi: mpc8xxx: Use short type names
  spi: mpc8xxx: Fix comments
  spi: mpc8xxx: Rename camel-case variables
  spi: mpc8xxx: Fix space after cast
  spi: mpc8xxx: Fix function names in strings
  spi: mpc8xxx: Replace defines with enums
  spi: mpc8xxx: Use IO accessors
  spi: mpc8xxx: Simplify if
  spi: mpc8xxx: Get rid of is_read
  spi: mpc8xxx: Simplify logic a bit
  spi: mpc8xxx: Reduce scope of loop variables
  spi: mpc8xxx: Make code more readable
  spi: mpc8xxx: Rename variable
  spi: mpc8xxx: Document LEN setting better
  spi: mpc8xxx: Re-order transfer setup
  spi: mpc8xxx: Fix if check
  spi: mpc8xxx: Use get_timer

 doc/driver-model/MIGRATION.txt |   6 +-
 drivers/spi/Kconfig            |  10 +-
 drivers/spi/mpc8xxx_spi.c      | 279 ++++++++++++++++++++++-----------
 3 files changed, 194 insertions(+), 101 deletions(-)

-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 01/19] spi: mpc8xxx: Use short type names
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 02/19] spi: mpc8xxx: Fix comments Jagan Teki
                   ` (18 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

The function signatures in the driver are quite long as is. Use short
type names (uint etc.) to make them more readable.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 8d6d86d2b0..0c77f95159 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -20,8 +20,7 @@
 
 #define SPI_TIMEOUT	1000
 
-struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
-		unsigned int max_hz, unsigned int mode)
+struct spi_slave *spi_setup_slave(uint bus, uint cs, uint max_hz, uint mode)
 {
 	struct spi_slave *slave;
 
@@ -68,17 +67,16 @@ int spi_claim_bus(struct spi_slave *slave)
 
 void spi_release_bus(struct spi_slave *slave)
 {
-
 }
 
-int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
-		void *din, unsigned long flags)
+int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
+	     ulong flags)
 {
 	volatile spi8xxx_t *spi = &((immap_t *) (CONFIG_SYS_IMMR))->spi;
-	unsigned int tmpdout, tmpdin, event;
+	uint tmpdout, tmpdin, event;
 	int numBlks = DIV_ROUND_UP(bitlen, 32);
 	int tm, isRead = 0;
-	unsigned char charSize = 32;
+	uchar charSize = 32;
 
 	debug("spi_xfer: slave %u:%u dout %08X din %08X bitlen %u\n",
 	      slave->bus, slave->cs, *(uint *) dout, *(uint *) din, bitlen);
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 02/19] spi: mpc8xxx: Fix comments
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 01/19] spi: mpc8xxx: Use short type names Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 03/19] spi: mpc8xxx: Rename camel-case variables Jagan Teki
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

There are some comments on the same line as the code they document. Put
comments above the code lines they document, so the line length is not
unnecessarily increased.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 0c77f95159..3016cfe2ca 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -53,11 +53,14 @@ void spi_init(void)
 	 * some registers
 	 */
 	spi->mode = SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN;
-	spi->mode = (spi->mode & 0xfff0ffff) | BIT(16); /* Use SYSCLK / 8
-							     (16.67MHz typ.) */
-	spi->event = 0xffffffff;	/* Clear all SPI events */
-	spi->mask = 0x00000000;	/* Mask  all SPI interrupts */
-	spi->com = 0;		/* LST bit doesn't do anything, so disregard */
+	/* Use SYSCLK / 8 (16.67MHz typ.) */
+	spi->mode = (spi->mode & 0xfff0ffff) | BIT(16);
+	/* Clear all SPI events */
+	spi->event = 0xffffffff;
+	/* Mask  all SPI interrupts */
+	spi->mask = 0x00000000;
+	/* LST bit doesn't do anything, so disregard */
+	spi->com = 0;
 }
 
 int spi_claim_bus(struct spi_slave *slave)
@@ -84,9 +87,10 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	if (flags & SPI_XFER_BEGIN)
 		spi_cs_activate(slave);
 
-	spi->event = 0xffffffff;	/* Clear all SPI events */
+	/* Clear all SPI events */
+	spi->event = 0xffffffff;
 
-	/* handle data in 32-bit chunks */
+	/* Handle data in 32-bit chunks */
 	while (numBlks--) {
 		tmpdout = 0;
 		charSize = (bitlen >= 32 ? 32 : bitlen);
@@ -120,7 +124,9 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 
 		spi->mode |= SPI_MODE_EN;
 
-		spi->tx = tmpdout;	/* Write the data out */
+		/* Write the data out */
+		spi->tx = tmpdout;
+
 		debug("*** spi_xfer: ... %08x written\n", tmpdout);
 
 		/*
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 03/19] spi: mpc8xxx: Rename camel-case variables
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 01/19] spi: mpc8xxx: Use short type names Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 02/19] spi: mpc8xxx: Fix comments Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 04/19] spi: mpc8xxx: Fix space after cast Jagan Teki
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

There are three variables that have camel-case names, which is not the
preferred naming style.

Give those variables more compliant names instead.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 3016cfe2ca..0393765b6f 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -77,9 +77,9 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 {
 	volatile spi8xxx_t *spi = &((immap_t *) (CONFIG_SYS_IMMR))->spi;
 	uint tmpdout, tmpdin, event;
-	int numBlks = DIV_ROUND_UP(bitlen, 32);
-	int tm, isRead = 0;
-	uchar charSize = 32;
+	int num_blks = DIV_ROUND_UP(bitlen, 32);
+	int tm, is_read = 0;
+	uchar char_size = 32;
 
 	debug("spi_xfer: slave %u:%u dout %08X din %08X bitlen %u\n",
 	      slave->bus, slave->cs, *(uint *) dout, *(uint *) din, bitlen);
@@ -91,12 +91,12 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	spi->event = 0xffffffff;
 
 	/* Handle data in 32-bit chunks */
-	while (numBlks--) {
+	while (num_blks--) {
 		tmpdout = 0;
-		charSize = (bitlen >= 32 ? 32 : bitlen);
+		char_size = (bitlen >= 32 ? 32 : bitlen);
 
 		/* Shift data so it's msb-justified */
-		tmpdout = *(u32 *) dout >> (32 - charSize);
+		tmpdout = *(u32 *) dout >> (32 - char_size);
 
 		/* The LEN field of the SPMODE register is set as follows:
 		 *
@@ -134,15 +134,15 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		 * or time out (1 second = 1000 ms)
 		 * The NE event must be read and cleared first
 		 */
-		for (tm = 0, isRead = 0; tm < SPI_TIMEOUT; ++tm) {
+		for (tm = 0, is_read = 0; tm < SPI_TIMEOUT; ++tm) {
 			event = spi->event;
 			if (event & SPI_EV_NE) {
 				tmpdin = spi->rx;
 				spi->event |= SPI_EV_NE;
-				isRead = 1;
+				is_read = 1;
 
-				*(u32 *) din = (tmpdin << (32 - charSize));
-				if (charSize == 32) {
+				*(u32 *) din = (tmpdin << (32 - char_size));
+				if (char_size == 32) {
 					/* Advance output buffer by 32 bits */
 					din += 4;
 				}
@@ -153,7 +153,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 			 * in the future put an arbitrary delay after writing
 			 * the device.  Arbitrary delays suck, though...
 			 */
-			if (isRead && (event & SPI_EV_NF))
+			if (is_read && (event & SPI_EV_NF))
 				break;
 		}
 		if (tm >= SPI_TIMEOUT)
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 04/19] spi: mpc8xxx: Fix space after cast
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (2 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 03/19] spi: mpc8xxx: Rename camel-case variables Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 05/19] spi: mpc8xxx: Fix function names in strings Jagan Teki
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Fix all "superfluous space after case" style errors.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 0393765b6f..1424e7febe 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -82,7 +82,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	uchar char_size = 32;
 
 	debug("spi_xfer: slave %u:%u dout %08X din %08X bitlen %u\n",
-	      slave->bus, slave->cs, *(uint *) dout, *(uint *) din, bitlen);
+	      slave->bus, slave->cs, *(uint *)dout, *(uint *)din, bitlen);
 
 	if (flags & SPI_XFER_BEGIN)
 		spi_cs_activate(slave);
@@ -96,7 +96,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		char_size = (bitlen >= 32 ? 32 : bitlen);
 
 		/* Shift data so it's msb-justified */
-		tmpdout = *(u32 *) dout >> (32 - char_size);
+		tmpdout = *(u32 *)dout >> (32 - char_size);
 
 		/* The LEN field of the SPMODE register is set as follows:
 		 *
@@ -141,7 +141,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 				spi->event |= SPI_EV_NE;
 				is_read = 1;
 
-				*(u32 *) din = (tmpdin << (32 - char_size));
+				*(u32 *)din = (tmpdin << (32 - char_size));
 				if (char_size == 32) {
 					/* Advance output buffer by 32 bits */
 					din += 4;
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 05/19] spi: mpc8xxx: Fix function names in strings
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (3 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 04/19] spi: mpc8xxx: Fix space after cast Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 06/19] spi: mpc8xxx: Replace defines with enums Jagan Teki
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Replace the function name with a "%s" format string and the __func__
variable in debug statements (as proposed by checkpatch).

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 1424e7febe..91b639f1e6 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -81,7 +81,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	int tm, is_read = 0;
 	uchar char_size = 32;
 
-	debug("spi_xfer: slave %u:%u dout %08X din %08X bitlen %u\n",
+	debug("%s: slave %u:%u dout %08X din %08X bitlen %u\n", __func__,
 	      slave->bus, slave->cs, *(uint *)dout, *(uint *)din, bitlen);
 
 	if (flags & SPI_XFER_BEGIN)
@@ -127,7 +127,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		/* Write the data out */
 		spi->tx = tmpdout;
 
-		debug("*** spi_xfer: ... %08x written\n", tmpdout);
+		debug("*** %s: ... %08x written\n", __func__, tmpdout);
 
 		/*
 		 * Wait for SPI transmit to get out
@@ -157,9 +157,10 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 				break;
 		}
 		if (tm >= SPI_TIMEOUT)
-			puts("*** spi_xfer: Time out during SPI transfer");
+			debug("*** %s: Time out during SPI transfer\n",
+			      __func__);
 
-		debug("*** spi_xfer: transfer ended. Value=%08x\n", tmpdin);
+		debug("*** %s: transfer ended. Value=%08x\n", __func__, tmpdin);
 	}
 
 	if (flags & SPI_XFER_END)
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 06/19] spi: mpc8xxx: Replace defines with enums
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (4 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 05/19] spi: mpc8xxx: Fix function names in strings Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 07/19] spi: mpc8xxx: Use IO accessors Jagan Teki
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Replace pre-processor defines with proper enums, and use the BIT macro
where applicable.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 91b639f1e6..7b2ab1e4af 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -10,13 +10,25 @@
 #include <spi.h>
 #include <asm/mpc8xxx_spi.h>
 
-#define SPI_EV_NE	(0x80000000 >> 22)	/* Receiver Not Empty */
-#define SPI_EV_NF	(0x80000000 >> 23)	/* Transmitter Not Full */
-
-#define SPI_MODE_LOOP	(0x80000000 >> 1)	/* Loopback mode */
-#define SPI_MODE_REV	(0x80000000 >> 5)	/* Reverse mode - MSB first */
-#define SPI_MODE_MS	(0x80000000 >> 6)	/* Always master */
-#define SPI_MODE_EN	(0x80000000 >> 7)	/* Enable interface */
+enum {
+	SPI_EV_NE = BIT(31 - 22),	/* Receiver Not Empty */
+	SPI_EV_NF = BIT(31 - 23),	/* Transmitter Not Full */
+};
+
+enum {
+	SPI_MODE_LOOP  = BIT(31 - 1),	/* Loopback mode */
+	SPI_MODE_CI    = BIT(31 - 2),	/* Clock invert */
+	SPI_MODE_CP    = BIT(31 - 3),	/* Clock phase */
+	SPI_MODE_DIV16 = BIT(31 - 4),	/* Divide clock source by 16 */
+	SPI_MODE_REV   = BIT(31 - 5),	/* Reverse mode - MSB first */
+	SPI_MODE_MS    = BIT(31 - 6),	/* Always master */
+	SPI_MODE_EN    = BIT(31 - 7),	/* Enable interface */
+
+	SPI_MODE_LEN_MASK = 0xf00000,
+	SPI_MODE_PM_MASK = 0xf0000,
+
+	SPI_COM_LST = BIT(31 - 9),
+};
 
 #define SPI_TIMEOUT	1000
 
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 07/19] spi: mpc8xxx: Use IO accessors
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (5 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 06/19] spi: mpc8xxx: Replace defines with enums Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 08/19] spi: mpc8xxx: Simplify if Jagan Teki
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Accesses to the register map are currently done by directly reading and
writing the structure.

Switch to the appropriate IO accessors instead.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 7b2ab1e4af..da9e1e3f98 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -58,21 +58,21 @@ void spi_free_slave(struct spi_slave *slave)
 
 void spi_init(void)
 {
-	volatile spi8xxx_t *spi = &((immap_t *) (CONFIG_SYS_IMMR))->spi;
+	spi8xxx_t *spi = &((immap_t *)(CONFIG_SYS_IMMR))->spi;
 
 	/*
 	 * SPI pins on the MPC83xx are not muxed, so all we do is initialize
 	 * some registers
 	 */
-	spi->mode = SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN;
+	out_be32(&spi->mode, SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN);
 	/* Use SYSCLK / 8 (16.67MHz typ.) */
-	spi->mode = (spi->mode & 0xfff0ffff) | BIT(16);
+	clrsetbits_be32(&spi->mode, 0x000f0000, BIT(16));
 	/* Clear all SPI events */
-	spi->event = 0xffffffff;
+	setbits_be32(&spi->event, 0xffffffff);
 	/* Mask  all SPI interrupts */
-	spi->mask = 0x00000000;
+	clrbits_be32(&spi->mask, 0xffffffff);
 	/* LST bit doesn't do anything, so disregard */
-	spi->com = 0;
+	out_be32(&spi->com, 0);
 }
 
 int spi_claim_bus(struct spi_slave *slave)
@@ -87,7 +87,7 @@ void spi_release_bus(struct spi_slave *slave)
 int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	     ulong flags)
 {
-	volatile spi8xxx_t *spi = &((immap_t *) (CONFIG_SYS_IMMR))->spi;
+	spi8xxx_t *spi = &((immap_t *)(CONFIG_SYS_IMMR))->spi;
 	uint tmpdout, tmpdin, event;
 	int num_blks = DIV_ROUND_UP(bitlen, 32);
 	int tm, is_read = 0;
@@ -100,7 +100,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		spi_cs_activate(slave);
 
 	/* Clear all SPI events */
-	spi->event = 0xffffffff;
+	setbits_be32(&spi->event, 0xffffffff);
 
 	/* Handle data in 32-bit chunks */
 	while (num_blks--) {
@@ -118,26 +118,26 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		 * len > 16               0
 		 */
 
-		spi->mode &= ~SPI_MODE_EN;
+		clrbits_be32(&spi->mode, SPI_MODE_EN);
 
 		if (bitlen <= 16) {
 			if (bitlen <= 4)
-				spi->mode = (spi->mode & 0xff0fffff) |
-					    (3 << 20);
+				clrsetbits_be32(&spi->mode, 0x00f00000,
+						(3 << 20));
 			else
-				spi->mode = (spi->mode & 0xff0fffff) |
-					    ((bitlen - 1) << 20);
+				clrsetbits_be32(&spi->mode, 0x00f00000,
+						((bitlen - 1) << 20));
 		} else {
-			spi->mode = (spi->mode & 0xff0fffff);
+			clrbits_be32(&spi->mode, 0x00f00000);
 			/* Set up the next iteration if sending > 32 bits */
 			bitlen -= 32;
 			dout += 4;
 		}
 
-		spi->mode |= SPI_MODE_EN;
+		setbits_be32(&spi->mode, SPI_MODE_EN);
 
 		/* Write the data out */
-		spi->tx = tmpdout;
+		out_be32(&spi->tx, tmpdout);
 
 		debug("*** %s: ... %08x written\n", __func__, tmpdout);
 
@@ -147,10 +147,10 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		 * The NE event must be read and cleared first
 		 */
 		for (tm = 0, is_read = 0; tm < SPI_TIMEOUT; ++tm) {
-			event = spi->event;
+			event = in_be32(&spi->event);
 			if (event & SPI_EV_NE) {
-				tmpdin = spi->rx;
-				spi->event |= SPI_EV_NE;
+				tmpdin = in_be32(&spi->rx);
+				setbits_be32(&spi->event, SPI_EV_NE);
 				is_read = 1;
 
 				*(u32 *)din = (tmpdin << (32 - char_size));
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 08/19] spi: mpc8xxx: Simplify if
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (6 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 07/19] spi: mpc8xxx: Use IO accessors Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 09/19] spi: mpc8xxx: Get rid of is_read Jagan Teki
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Instead of having a nested if block, just have two branches within the
overarching if block to eliminate one nesting level.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index da9e1e3f98..ca34570901 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -120,13 +120,11 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 
 		clrbits_be32(&spi->mode, SPI_MODE_EN);
 
-		if (bitlen <= 16) {
-			if (bitlen <= 4)
-				clrsetbits_be32(&spi->mode, 0x00f00000,
-						(3 << 20));
-			else
-				clrsetbits_be32(&spi->mode, 0x00f00000,
-						((bitlen - 1) << 20));
+		if (bitlen <= 4) {
+			clrsetbits_be32(&spi->mode, 0x00f00000, (3 << 20));
+		} else if (bitlen <= 16) {
+			clrsetbits_be32(&spi->mode, 0x00f00000,
+					((bitlen - 1) << 20));
 		} else {
 			clrbits_be32(&spi->mode, 0x00f00000);
 			/* Set up the next iteration if sending > 32 bits */
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 09/19] spi: mpc8xxx: Get rid of is_read
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (7 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 08/19] spi: mpc8xxx: Simplify if Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit Jagan Teki
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Get rid of the is_read variable, and just keep the state of the "not
empty" and "not full" events in two boolean variables within the loop
body.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index ca34570901..962ef710f8 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -90,7 +90,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	spi8xxx_t *spi = &((immap_t *)(CONFIG_SYS_IMMR))->spi;
 	uint tmpdout, tmpdin, event;
 	int num_blks = DIV_ROUND_UP(bitlen, 32);
-	int tm, is_read = 0;
+	int tm;
 	uchar char_size = 32;
 
 	debug("%s: slave %u:%u dout %08X din %08X bitlen %u\n", __func__,
@@ -144,12 +144,14 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		 * or time out (1 second = 1000 ms)
 		 * The NE event must be read and cleared first
 		 */
-		for (tm = 0, is_read = 0; tm < SPI_TIMEOUT; ++tm) {
+		for (tm = 0; tm < SPI_TIMEOUT; ++tm) {
 			event = in_be32(&spi->event);
-			if (event & SPI_EV_NE) {
+			bool have_ne = event & SPI_EV_NE;
+			bool have_nf = event & SPI_EV_NF;
+
+			if (have_ne) {
 				tmpdin = in_be32(&spi->rx);
 				setbits_be32(&spi->event, SPI_EV_NE);
-				is_read = 1;
 
 				*(u32 *)din = (tmpdin << (32 - char_size));
 				if (char_size == 32) {
@@ -163,7 +165,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 			 * in the future put an arbitrary delay after writing
 			 * the device.  Arbitrary delays suck, though...
 			 */
-			if (is_read && (event & SPI_EV_NF))
+			if (have_ne && have_nf)
 				break;
 		}
 		if (tm >= SPI_TIMEOUT)
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (8 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 09/19] spi: mpc8xxx: Get rid of is_read Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-29  9:17   ` Joakim Tjernlund
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 11/19] spi: mpc8xxx: Reduce scope of loop variables Jagan Teki
                   ` (9 subsequent siblings)
  19 siblings, 1 reply; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

We do nothing in the loop if the "not empty" event was not detected. To
simplify the logic, check if this is the case, and skip the execution of
the loop early to reduce the nesting level and flag checking.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 962ef710f8..a2e698ea17 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -149,25 +149,28 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 			bool have_ne = event & SPI_EV_NE;
 			bool have_nf = event & SPI_EV_NF;
 
-			if (have_ne) {
-				tmpdin = in_be32(&spi->rx);
-				setbits_be32(&spi->event, SPI_EV_NE);
-
-				*(u32 *)din = (tmpdin << (32 - char_size));
-				if (char_size == 32) {
-					/* Advance output buffer by 32 bits */
-					din += 4;
-				}
+			if (!have_ne)
+				continue;
+
+			tmpdin = in_be32(&spi->rx);
+			setbits_be32(&spi->event, SPI_EV_NE);
+
+			*(u32 *)din = (tmpdin << (32 - char_size));
+			if (char_size == 32) {
+				/* Advance output buffer by 32 bits */
+				din += 4;
 			}
+
 			/*
 			 * Only bail when we've had both NE and NF events.
 			 * This will cause timeouts on RO devices, so maybe
 			 * in the future put an arbitrary delay after writing
 			 * the device.  Arbitrary delays suck, though...
 			 */
-			if (have_ne && have_nf)
+			if (have_nf)
 				break;
 		}
+
 		if (tm >= SPI_TIMEOUT)
 			debug("*** %s: Time out during SPI transfer\n",
 			      __func__);
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 11/19] spi: mpc8xxx: Reduce scope of loop variables
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (9 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 12/19] spi: mpc8xxx: Make code more readable Jagan Teki
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

The transmission loop starts with setting some variables, which are only
used inside the loop. Reduce the scope to the loop to make the
declaration and initialization of these variables coincide.

In the case of char_size this also always initializes the variable
immediately with the final value actually used in the loop (instead of
the placeholder value 32).

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index a2e698ea17..2a0f3cc06a 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -88,10 +88,8 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	     ulong flags)
 {
 	spi8xxx_t *spi = &((immap_t *)(CONFIG_SYS_IMMR))->spi;
-	uint tmpdout, tmpdin, event;
+	u32 tmpdin;
 	int num_blks = DIV_ROUND_UP(bitlen, 32);
-	int tm;
-	uchar char_size = 32;
 
 	debug("%s: slave %u:%u dout %08X din %08X bitlen %u\n", __func__,
 	      slave->bus, slave->cs, *(uint *)dout, *(uint *)din, bitlen);
@@ -104,8 +102,9 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 
 	/* Handle data in 32-bit chunks */
 	while (num_blks--) {
-		tmpdout = 0;
-		char_size = (bitlen >= 32 ? 32 : bitlen);
+		int tm;
+		u32 tmpdout = 0;
+		uchar char_size = (bitlen >= 32 ? 32 : bitlen);
 
 		/* Shift data so it's msb-justified */
 		tmpdout = *(u32 *)dout >> (32 - char_size);
@@ -145,7 +144,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		 * The NE event must be read and cleared first
 		 */
 		for (tm = 0; tm < SPI_TIMEOUT; ++tm) {
-			event = in_be32(&spi->event);
+			u32 event = in_be32(&spi->event);
 			bool have_ne = event & SPI_EV_NE;
 			bool have_nf = event & SPI_EV_NF;
 
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 12/19] spi: mpc8xxx: Make code more readable
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (10 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 11/19] spi: mpc8xxx: Reduce scope of loop variables Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 13/19] spi: mpc8xxx: Rename variable Jagan Teki
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Introduce the to_prescale_mod and set_char_len inline functions to make
the code more readable.

Note that the added "if (bitlen > 16)" check does not change the
semantics of the current code, and hence only preserves the current
error (this will be fixed in a later patch in the series).

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 2a0f3cc06a..83fd8b3cc1 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -30,6 +30,16 @@ enum {
 	SPI_COM_LST = BIT(31 - 9),
 };
 
+static inline u32 to_prescale_mod(u32 val)
+{
+	return (min(val, (u32)15) << 16);
+}
+
+static void set_char_len(spi8xxx_t *spi, u32 val)
+{
+	clrsetbits_be32(&spi->mode, SPI_MODE_LEN_MASK, (val << 20));
+}
+
 #define SPI_TIMEOUT	1000
 
 struct spi_slave *spi_setup_slave(uint bus, uint cs, uint max_hz, uint mode)
@@ -66,7 +76,7 @@ void spi_init(void)
 	 */
 	out_be32(&spi->mode, SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN);
 	/* Use SYSCLK / 8 (16.67MHz typ.) */
-	clrsetbits_be32(&spi->mode, 0x000f0000, BIT(16));
+	clrsetbits_be32(&spi->mode, SPI_MODE_PM_MASK, to_prescale_mod(1));
 	/* Clear all SPI events */
 	setbits_be32(&spi->event, 0xffffffff);
 	/* Mask  all SPI interrupts */
@@ -119,13 +129,14 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 
 		clrbits_be32(&spi->mode, SPI_MODE_EN);
 
-		if (bitlen <= 4) {
-			clrsetbits_be32(&spi->mode, 0x00f00000, (3 << 20));
-		} else if (bitlen <= 16) {
-			clrsetbits_be32(&spi->mode, 0x00f00000,
-					((bitlen - 1) << 20));
-		} else {
-			clrbits_be32(&spi->mode, 0x00f00000);
+		if (bitlen <= 4)
+			set_char_len(spi, 3);
+		else if (bitlen <= 16)
+			set_char_len(spi, bitlen - 1);
+		else
+			set_char_len(spi, 0);
+
+		if (bitlen > 16) {
 			/* Set up the next iteration if sending > 32 bits */
 			bitlen -= 32;
 			dout += 4;
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 13/19] spi: mpc8xxx: Rename variable
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (11 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 12/19] spi: mpc8xxx: Make code more readable Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 14/19] spi: mpc8xxx: Document LEN setting better Jagan Teki
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

The variable "char_size" holds the number of bits to be transferred in
the current loop iteration. A better name would be "xfer_bitlen", which
we rename this variable to.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 83fd8b3cc1..63d956a295 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -114,10 +114,10 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 	while (num_blks--) {
 		int tm;
 		u32 tmpdout = 0;
-		uchar char_size = (bitlen >= 32 ? 32 : bitlen);
+		uchar xfer_bitlen = (bitlen >= 32 ? 32 : bitlen);
 
 		/* Shift data so it's msb-justified */
-		tmpdout = *(u32 *)dout >> (32 - char_size);
+		tmpdout = *(u32 *)dout >> (32 - xfer_bitlen);
 
 		/* The LEN field of the SPMODE register is set as follows:
 		 *
@@ -165,8 +165,8 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 			tmpdin = in_be32(&spi->rx);
 			setbits_be32(&spi->event, SPI_EV_NE);
 
-			*(u32 *)din = (tmpdin << (32 - char_size));
-			if (char_size == 32) {
+			*(u32 *)din = (tmpdin << (32 - xfer_bitlen));
+			if (xfer_bitlen == 32) {
 				/* Advance output buffer by 32 bits */
 				din += 4;
 			}
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 14/19] spi: mpc8xxx: Document LEN setting better
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (12 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 13/19] spi: mpc8xxx: Rename variable Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 15/19] spi: mpc8xxx: Re-order transfer setup Jagan Teki
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Instead of having a table right before the code implementing the length
setting for documentation, have inline comments for the if branches
actually implementing the length setting described table's entries
(which is readable thanks to the set_char_len function).

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 63d956a295..1dd5bd9799 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -119,21 +119,15 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		/* Shift data so it's msb-justified */
 		tmpdout = *(u32 *)dout >> (32 - xfer_bitlen);
 
-		/* The LEN field of the SPMODE register is set as follows:
-		 *
-		 * Bit length             setting
-		 * len <= 4               3
-		 * 4 < len <= 16          len - 1
-		 * len > 16               0
-		 */
-
 		clrbits_be32(&spi->mode, SPI_MODE_EN);
 
-		if (bitlen <= 4)
+		/* Set up length for this transfer */
+
+		if (bitlen <= 4) /* 4 bits or less */
 			set_char_len(spi, 3);
-		else if (bitlen <= 16)
+		else if (bitlen <= 16) /*@most 16 bits */
 			set_char_len(spi, bitlen - 1);
-		else
+		else /* more than 16 bits -> full 32 bit transfer */
 			set_char_len(spi, 0);
 
 		if (bitlen > 16) {
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 15/19] spi: mpc8xxx: Re-order transfer setup
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (13 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 14/19] spi: mpc8xxx: Document LEN setting better Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 16/19] spi: mpc8xxx: Fix if check Jagan Teki
                   ` (4 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Minize the time the adapter is disabled (via SPI_MODE_EN
clearing/setting) to just the character length setting, and only set up
the temporary data writing variable right before we need it, so there is
a more clear distinction between setting up the SPI adapter, and setting
up the data to be written.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 1dd5bd9799..1e7c0144c2 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -116,9 +116,6 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		u32 tmpdout = 0;
 		uchar xfer_bitlen = (bitlen >= 32 ? 32 : bitlen);
 
-		/* Shift data so it's msb-justified */
-		tmpdout = *(u32 *)dout >> (32 - xfer_bitlen);
-
 		clrbits_be32(&spi->mode, SPI_MODE_EN);
 
 		/* Set up length for this transfer */
@@ -130,14 +127,17 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		else /* more than 16 bits -> full 32 bit transfer */
 			set_char_len(spi, 0);
 
+		setbits_be32(&spi->mode, SPI_MODE_EN);
+
+		/* Shift data so it's msb-justified */
+		tmpdout = *(u32 *)dout >> (32 - xfer_bitlen);
+
 		if (bitlen > 16) {
 			/* Set up the next iteration if sending > 32 bits */
 			bitlen -= 32;
 			dout += 4;
 		}
 
-		setbits_be32(&spi->mode, SPI_MODE_EN);
-
 		/* Write the data out */
 		out_be32(&spi->tx, tmpdout);
 
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 16/19] spi: mpc8xxx: Fix if check
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (14 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 15/19] spi: mpc8xxx: Re-order transfer setup Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 17/19] spi: mpc8xxx: Use get_timer Jagan Teki
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

Decreasing the bit length and increasing the write data pointer should
be done when there are more than 32 bit of data, not 16 bit.

This did not produce incorrect behavior, because the only time where the
two checks produce different outcomes is the case of 16 < bitlen < 32,
and in this case the subsequent transmission is the last one regardless,
hence the additional bit length decrease and write data pointer increase
has no effect anyway.

Still, the correct check is the check for "bitlen > 32", so correct this
behavior.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 1e7c0144c2..e09e91c8e9 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -132,7 +132,7 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		/* Shift data so it's msb-justified */
 		tmpdout = *(u32 *)dout >> (32 - xfer_bitlen);
 
-		if (bitlen > 16) {
+		if (bitlen > 32) {
 			/* Set up the next iteration if sending > 32 bits */
 			bitlen -= 32;
 			dout += 4;
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 17/19] spi: mpc8xxx: Use get_timer
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (15 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 16/19] spi: mpc8xxx: Fix if check Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 18/19] spi: mpc8xxx: Convert to DM Jagan Teki
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

From: Mario Six <mario.six@gdsys.cc>

The comment before the transmission loop in conjunction with the
definition of SPI_TIMEOUT as 1000 implies that the loop is supposed to
have a timeout value of 1000 ms. But since there is no mdelay(1) or
similar in the loop body, the loop just runs 1000 times, without regard
for the time elapsed.

To correct this, use the standard get_timer functionality to properly
time out the loop after 1000 ms.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/spi/mpc8xxx_spi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index e09e91c8e9..63e1a150f8 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -112,9 +112,9 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 
 	/* Handle data in 32-bit chunks */
 	while (num_blks--) {
-		int tm;
 		u32 tmpdout = 0;
 		uchar xfer_bitlen = (bitlen >= 32 ? 32 : bitlen);
+		ulong start;
 
 		clrbits_be32(&spi->mode, SPI_MODE_EN);
 
@@ -148,7 +148,8 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 		 * or time out (1 second = 1000 ms)
 		 * The NE event must be read and cleared first
 		 */
-		for (tm = 0; tm < SPI_TIMEOUT; ++tm) {
+		start = get_timer(0);
+		do {
 			u32 event = in_be32(&spi->event);
 			bool have_ne = event & SPI_EV_NE;
 			bool have_nf = event & SPI_EV_NF;
@@ -173,9 +174,11 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 			 */
 			if (have_nf)
 				break;
-		}
 
-		if (tm >= SPI_TIMEOUT)
+			mdelay(1);
+		} while (get_timer(start) < SPI_TIMEOUT);
+
+		if (get_timer(start) >= SPI_TIMEOUT)
 			debug("*** %s: Time out during SPI transfer\n",
 			      __func__);
 
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 18/19] spi: mpc8xxx: Convert to DM
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (16 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 17/19] spi: mpc8xxx: Use get_timer Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 19/19] dm: MIGRATION: Update migration status for SPI Jagan Teki
  2019-05-20 16:52 ` [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

Support DM in the MPC8xxx SPI driver, and remove the legacy SPI
interface.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/spi/Kconfig       |  10 +--
 drivers/spi/mpc8xxx_spi.c | 144 ++++++++++++++++++++++++++++----------
 2 files changed, 112 insertions(+), 42 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index fb794adae7..e196f64e2f 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -131,6 +131,11 @@ config MPC8XX_SPI
 	help
 	  Enable support for SPI on MPC8XX
 
+config MPC8XXX_SPI
+	bool "MPC8XXX SPI Driver"
+	help
+	  Enable support for SPI on the MPC8XXX PowerPC SoCs.
+
 config MT7621_SPI
 	bool "MediaTek MT7621 SPI driver"
 	depends on ARCH_MT7620
@@ -364,11 +369,6 @@ config LPC32XX_SSP
 	help
 	  Enable support for SPI on LPC32xx
 
-config MPC8XXX_SPI
-	bool "MPC8XXX SPI Driver"
-	help
-	  Enable support for SPI on the MPC8XXX PowerPC SoCs.
-
 config MXC_SPI
 	bool "MXC SPI Driver"
 	help
diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 63e1a150f8..1c7bf10f91 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -5,10 +5,12 @@
  */
 
 #include <common.h>
-
+#include <dm.h>
+#include <errno.h>
 #include <malloc.h>
 #include <spi.h>
 #include <asm/mpc8xxx_spi.h>
+#include <asm-generic/gpio.h>
 
 enum {
 	SPI_EV_NE = BIT(31 - 22),	/* Receiver Not Empty */
@@ -30,6 +32,12 @@ enum {
 	SPI_COM_LST = BIT(31 - 9),
 };
 
+struct mpc8xxx_priv {
+	spi8xxx_t *spi;
+	struct gpio_desc gpios[16];
+	int max_cs;
+};
+
 static inline u32 to_prescale_mod(u32 val)
 {
 	return (min(val, (u32)15) << 16);
@@ -42,70 +50,90 @@ static void set_char_len(spi8xxx_t *spi, u32 val)
 
 #define SPI_TIMEOUT	1000
 
-struct spi_slave *spi_setup_slave(uint bus, uint cs, uint max_hz, uint mode)
+static int __spi_set_speed(spi8xxx_t *spi, uint speed)
 {
-	struct spi_slave *slave;
+	/* TODO(mario.six@gdsys.cc): This only ever sets one fixed speed */
 
-	if (!spi_cs_is_valid(bus, cs))
-		return NULL;
-
-	slave = spi_alloc_slave_base(bus, cs);
-	if (!slave)
-		return NULL;
-
-	/*
-	 * TODO: Some of the code in spi_init() should probably move
-	 * here, or into spi_claim_bus() below.
-	 */
+	/* Use SYSCLK / 8 (16.67MHz typ.) */
+	clrsetbits_be32(&spi->mode, SPI_MODE_PM_MASK, to_prescale_mod(1));
 
-	return slave;
+	return 0;
 }
 
-void spi_free_slave(struct spi_slave *slave)
+static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev)
 {
-	free(slave);
+	struct mpc8xxx_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	priv->spi = (spi8xxx_t *)dev_read_addr(dev);
+
+	/* TODO(mario.six at gdsys.cc): Read clock and save the value */
+
+	ret = gpio_request_list_by_name(dev, "gpios", priv->gpios,
+					ARRAY_SIZE(priv->gpios), GPIOD_IS_OUT | GPIOD_ACTIVE_LOW);
+	if (ret < 0)
+		return -EINVAL;
+
+	priv->max_cs = ret;
+
+	return 0;
 }
 
-void spi_init(void)
+static int mpc8xxx_spi_probe(struct udevice *dev)
 {
-	spi8xxx_t *spi = &((immap_t *)(CONFIG_SYS_IMMR))->spi;
+	struct mpc8xxx_priv *priv = dev_get_priv(dev);
 
 	/*
 	 * SPI pins on the MPC83xx are not muxed, so all we do is initialize
 	 * some registers
 	 */
-	out_be32(&spi->mode, SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN);
-	/* Use SYSCLK / 8 (16.67MHz typ.) */
-	clrsetbits_be32(&spi->mode, SPI_MODE_PM_MASK, to_prescale_mod(1));
+	out_be32(&priv->spi->mode, SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN);
+
+	__spi_set_speed(priv->spi, 16666667);
+
 	/* Clear all SPI events */
-	setbits_be32(&spi->event, 0xffffffff);
+	setbits_be32(&priv->spi->event, 0xffffffff);
 	/* Mask  all SPI interrupts */
-	clrbits_be32(&spi->mask, 0xffffffff);
+	clrbits_be32(&priv->spi->mask, 0xffffffff);
 	/* LST bit doesn't do anything, so disregard */
-	out_be32(&spi->com, 0);
+	out_be32(&priv->spi->com, 0);
+
+	return 0;
 }
 
-int spi_claim_bus(struct spi_slave *slave)
+static void mpc8xxx_spi_cs_activate(struct udevice *dev)
 {
-	return 0;
+	struct mpc8xxx_priv *priv = dev_get_priv(dev->parent);
+	struct dm_spi_slave_platdata *platdata = dev_get_parent_platdata(dev);
+
+	dm_gpio_set_dir_flags(&priv->gpios[platdata->cs], GPIOD_IS_OUT);
+	dm_gpio_set_value(&priv->gpios[platdata->cs], 0);
 }
 
-void spi_release_bus(struct spi_slave *slave)
+static void mpc8xxx_spi_cs_deactivate(struct udevice *dev)
 {
+	struct mpc8xxx_priv *priv = dev_get_priv(dev->parent);
+	struct dm_spi_slave_platdata *platdata = dev_get_parent_platdata(dev);
+
+	dm_gpio_set_dir_flags(&priv->gpios[platdata->cs], GPIOD_IS_OUT);
+	dm_gpio_set_value(&priv->gpios[platdata->cs], 1);
 }
 
-int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
-	     ulong flags)
+static int mpc8xxx_spi_xfer(struct udevice *dev, uint bitlen,
+			    const void *dout, void *din, ulong flags)
 {
-	spi8xxx_t *spi = &((immap_t *)(CONFIG_SYS_IMMR))->spi;
-	u32 tmpdin;
+	struct udevice *bus = dev->parent;
+	struct mpc8xxx_priv *priv = dev_get_priv(bus);
+	spi8xxx_t *spi = priv->spi;
+	struct dm_spi_slave_platdata *platdata = dev_get_parent_platdata(dev);
+	u32 tmpdin = 0;
 	int num_blks = DIV_ROUND_UP(bitlen, 32);
 
-	debug("%s: slave %u:%u dout %08X din %08X bitlen %u\n", __func__,
-	      slave->bus, slave->cs, *(uint *)dout, *(uint *)din, bitlen);
+	debug("%s: slave %s:%u dout %08X din %08X bitlen %u\n", __func__,
+	      bus->name, platdata->cs, *(uint *)dout, *(uint *)din, bitlen);
 
 	if (flags & SPI_XFER_BEGIN)
-		spi_cs_activate(slave);
+		mpc8xxx_spi_cs_activate(dev);
 
 	/* Clear all SPI events */
 	setbits_be32(&spi->event, 0xffffffff);
@@ -178,15 +206,57 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
 			mdelay(1);
 		} while (get_timer(start) < SPI_TIMEOUT);
 
-		if (get_timer(start) >= SPI_TIMEOUT)
+		if (get_timer(start) >= SPI_TIMEOUT) {
 			debug("*** %s: Time out during SPI transfer\n",
 			      __func__);
+			return -ETIMEDOUT;
+		}
 
 		debug("*** %s: transfer ended. Value=%08x\n", __func__, tmpdin);
 	}
 
 	if (flags & SPI_XFER_END)
-		spi_cs_deactivate(slave);
+		mpc8xxx_spi_cs_deactivate(dev);
+
+	return 0;
+}
+
+static int mpc8xxx_spi_set_speed(struct udevice *dev, uint speed)
+{
+	struct mpc8xxx_priv *priv = dev_get_priv(dev);
+
+	return __spi_set_speed(priv->spi, speed);
+}
 
+static int mpc8xxx_spi_set_mode(struct udevice *dev, uint mode)
+{
+	/* TODO(mario.six at gdsys.cc): Using SPI_CPHA (for clock phase) and
+	 * SPI_CPOL (for clock polarity) should work
+	 */
 	return 0;
 }
+
+static const struct dm_spi_ops mpc8xxx_spi_ops = {
+	.xfer		= mpc8xxx_spi_xfer,
+	.set_speed	= mpc8xxx_spi_set_speed,
+	.set_mode	= mpc8xxx_spi_set_mode,
+	/*
+	 * cs_info is not needed, since we require all chip selects to be
+	 * in the device tree explicitly
+	 */
+};
+
+static const struct udevice_id mpc8xxx_spi_ids[] = {
+	{ .compatible = "fsl,spi" },
+	{ }
+};
+
+U_BOOT_DRIVER(mpc8xxx_spi) = {
+	.name	= "mpc8xxx_spi",
+	.id	= UCLASS_SPI,
+	.of_match = mpc8xxx_spi_ids,
+	.ops	= &mpc8xxx_spi_ops,
+	.ofdata_to_platdata = mpc8xxx_spi_ofdata_to_platdata,
+	.probe	= mpc8xxx_spi_probe,
+	.priv_auto_alloc_size = sizeof(struct mpc8xxx_priv),
+};
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 19/19] dm: MIGRATION: Update migration status for SPI
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (17 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 18/19] spi: mpc8xxx: Convert to DM Jagan Teki
@ 2019-04-28 20:28 ` Jagan Teki
  2019-05-20 16:52 ` [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-04-28 20:28 UTC (permalink / raw)
  To: u-boot

Now, we have few driver are fully converted into dm and few
are partially converted.

So, update the migration status accordingly.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 doc/driver-model/MIGRATION.txt | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/doc/driver-model/MIGRATION.txt b/doc/driver-model/MIGRATION.txt
index df659f3dd9..d38be3538a 100644
--- a/doc/driver-model/MIGRATION.txt
+++ b/doc/driver-model/MIGRATION.txt
@@ -59,10 +59,7 @@ No dm conversion yet:
 	drivers/spi/cf_spi.c
 	drivers/spi/fsl_espi.c
 	drivers/spi/lpc32xx_ssp.c
-	drivers/spi/mpc8xx_spi.c
-	drivers/spi/mpc8xxx_spi.c
 	drivers/spi/mxs_spi.c
-	drivers/spi/sh_qspi.c
 	drivers/spi/sh_spi.c
 	drivers/spi/soft_spi_legacy.c
 
@@ -70,13 +67,12 @@ No dm conversion yet:
 	Deadline: 2019.04
 
 Partially converted:
-	drivers/spi/atcspi200_spi.c
 	drivers/spi/davinci_spi.c
 	drivers/spi/fsl_dspi.c
-	drivers/spi/fsl_qspi.c
 	drivers/spi/kirkwood_spi.c
 	drivers/spi/mxc_spi.c
 	drivers/spi/omap3_spi.c
+	drivers/spi/sh_qspi.c
 
 	Status: In progress
 	Deadline: 2019.07
-- 
2.18.0.321.gffc6fa0e3

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit Jagan Teki
@ 2019-04-29  9:17   ` Joakim Tjernlund
  2019-04-29 10:41     ` Jagan Teki
  0 siblings, 1 reply; 28+ messages in thread
From: Joakim Tjernlund @ 2019-04-29  9:17 UTC (permalink / raw)
  To: u-boot

On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> 
> From: Mario Six <mario.six@gdsys.cc>
> 
> We do nothing in the loop if the "not empty" event was not detected. To
> simplify the logic, check if this is the case, and skip the execution of
> the loop early to reduce the nesting level and flag checking.

Looked at the driver to refresh memory and noticed:
if (charSize == 32) {
	/* Advance output buffer by 32 bits */
	din += 4;
}
which suggests that only 32 bit char will increase the din ptr so does other bitlens
work for reading?

 Jocke

> 
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>  drivers/spi/mpc8xxx_spi.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
> index 962ef710f8..a2e698ea17 100644
> --- a/drivers/spi/mpc8xxx_spi.c
> +++ b/drivers/spi/mpc8xxx_spi.c
> @@ -149,25 +149,28 @@ int spi_xfer(struct spi_slave *slave, uint bitlen, const void *dout, void *din,
>                         bool have_ne = event & SPI_EV_NE;
>                         bool have_nf = event & SPI_EV_NF;
> 
> -                       if (have_ne) {
> -                               tmpdin = in_be32(&spi->rx);
> -                               setbits_be32(&spi->event, SPI_EV_NE);
> -
> -                               *(u32 *)din = (tmpdin << (32 - char_size));
> -                               if (char_size == 32) {
> -                                       /* Advance output buffer by 32 bits */
> -                                       din += 4;
> -                               }
> +                       if (!have_ne)
> +                               continue;
> +
> +                       tmpdin = in_be32(&spi->rx);
> +                       setbits_be32(&spi->event, SPI_EV_NE);
> +
> +                       *(u32 *)din = (tmpdin << (32 - char_size));
> +                       if (char_size == 32) {
> +                               /* Advance output buffer by 32 bits */
> +                               din += 4;
>                         }
> +
>                         /*
>                          * Only bail when we've had both NE and NF events.
>                          * This will cause timeouts on RO devices, so maybe
>                          * in the future put an arbitrary delay after writing
>                          * the device.  Arbitrary delays suck, though...
>                          */
> -                       if (have_ne && have_nf)
> +                       if (have_nf)
>                                 break;
>                 }
> +
>                 if (tm >= SPI_TIMEOUT)
>                         debug("*** %s: Time out during SPI transfer\n",
>                               __func__);
> --
> 2.18.0.321.gffc6fa0e3
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.denx.de%2Flistinfo%2Fu-boot&amp;data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7Cb423ca475f53471860b308d6cc195be8%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C636920806635383891&amp;sdata=PxiqErmkjcpBVL4yBUi2UYiJ5oqtBTI4fCnb4XBTpmE%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-04-29  9:17   ` Joakim Tjernlund
@ 2019-04-29 10:41     ` Jagan Teki
  2019-05-02  5:31       ` Mario Six
  0 siblings, 1 reply; 28+ messages in thread
From: Jagan Teki @ 2019-04-29 10:41 UTC (permalink / raw)
  To: u-boot

+ Mario

On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
<Joakim.Tjernlund@infinera.com> wrote:
>
> On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> >
> > From: Mario Six <mario.six@gdsys.cc>
> >
> > We do nothing in the loop if the "not empty" event was not detected. To
> > simplify the logic, check if this is the case, and skip the execution of
> > the loop early to reduce the nesting level and flag checking.
>
> Looked at the driver to refresh memory and noticed:
> if (charSize == 32) {
>         /* Advance output buffer by 32 bits */
>         din += 4;
> }
> which suggests that only 32 bit char will increase the din ptr so does other bitlens
> work for reading?

Mario, can you respond this?

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-04-29 10:41     ` Jagan Teki
@ 2019-05-02  5:31       ` Mario Six
  2019-05-02  9:07         ` Joakim Tjernlund
  0 siblings, 1 reply; 28+ messages in thread
From: Mario Six @ 2019-05-02  5:31 UTC (permalink / raw)
  To: u-boot

Hi Jagan and Jocke,

I'm back from vacation, so here's my answer:

On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> + Mario
>
> On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> <Joakim.Tjernlund@infinera.com> wrote:
> >
> > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > >
> > > From: Mario Six <mario.six@gdsys.cc>
> > >
> > > We do nothing in the loop if the "not empty" event was not detected. To
> > > simplify the logic, check if this is the case, and skip the execution of
> > > the loop early to reduce the nesting level and flag checking.
> >
> > Looked at the driver to refresh memory and noticed:
> > if (charSize == 32) {
> >         /* Advance output buffer by 32 bits */
> >         din += 4;
> > }
> > which suggests that only 32 bit char will increase the din ptr so does other bitlens
> > work for reading?
>
Yes, It will work. When charSize < 32 in a loop execution, we necessarily also
have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
(bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the data
to be sent/received and also the final loop execution, so we don't have to
advance the pointer anymore.

> Mario, can you respond this?

Best regards,
Mario

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-05-02  5:31       ` Mario Six
@ 2019-05-02  9:07         ` Joakim Tjernlund
  2019-05-14 13:53           ` Jagan Teki
  0 siblings, 1 reply; 28+ messages in thread
From: Joakim Tjernlund @ 2019-05-02  9:07 UTC (permalink / raw)
  To: u-boot

On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> Hi Jagan and Jocke,
> 
> I'm back from vacation, so here's my answer:
> 
> On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > + Mario
> > 
> > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > <Joakim.Tjernlund@infinera.com> wrote:
> > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > From: Mario Six <mario.six@gdsys.cc>
> > > > 
> > > > We do nothing in the loop if the "not empty" event was not detected. To
> > > > simplify the logic, check if this is the case, and skip the execution of
> > > > the loop early to reduce the nesting level and flag checking.
> > > 
> > > Looked at the driver to refresh memory and noticed:
> > > if (charSize == 32) {
> > >         /* Advance output buffer by 32 bits */
> > >         din += 4;
> > > }
> > > which suggests that only 32 bit char will increase the din ptr so does other bitlens
> > > work for reading?
> Yes, It will work. When charSize < 32 in a loop execution, we necessarily also
> have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
> (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the data
> to be sent/received and also the final loop execution, so we don't have to
> advance the pointer anymore.

Ahh, I see now.

But this over use of always 32 bits cause complexity/subtile bugs. The driver should use
the requested charsize(wordlen) throughout. As is now you cannot use the NF/NE flag as intended or
toggle LSB_FIRST
I think fsl_espi driver is worse though.

    Jocke

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-05-02  9:07         ` Joakim Tjernlund
@ 2019-05-14 13:53           ` Jagan Teki
  2019-05-15  5:02             ` Mario Six
  0 siblings, 1 reply; 28+ messages in thread
From: Jagan Teki @ 2019-05-14 13:53 UTC (permalink / raw)
  To: u-boot

On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
<Joakim.Tjernlund@infinera.com> wrote:
>
> On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> >
> >
> > Hi Jagan and Jocke,
> >
> > I'm back from vacation, so here's my answer:
> >
> > On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > + Mario
> > >
> > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > <Joakim.Tjernlund@infinera.com> wrote:
> > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > From: Mario Six <mario.six@gdsys.cc>
> > > > >
> > > > > We do nothing in the loop if the "not empty" event was not detected. To
> > > > > simplify the logic, check if this is the case, and skip the execution of
> > > > > the loop early to reduce the nesting level and flag checking.
> > > >
> > > > Looked at the driver to refresh memory and noticed:
> > > > if (charSize == 32) {
> > > >         /* Advance output buffer by 32 bits */
> > > >         din += 4;
> > > > }
> > > > which suggests that only 32 bit char will increase the din ptr so does other bitlens
> > > > work for reading?
> > Yes, It will work. When charSize < 32 in a loop execution, we necessarily also
> > have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
> > (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the data
> > to be sent/received and also the final loop execution, so we don't have to
> > advance the pointer anymore.
>
> Ahh, I see now.
>
> But this over use of always 32 bits cause complexity/subtile bugs. The driver should use
> the requested charsize(wordlen) throughout. As is now you cannot use the NF/NE flag as intended or
> toggle LSB_FIRST

Look like Joakim has a point here. better we can check the required
charsize instead of looking for magic 32 number. What do you say
Mario?

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-05-14 13:53           ` Jagan Teki
@ 2019-05-15  5:02             ` Mario Six
  2019-05-15  7:16               ` Joakim Tjernlund
  0 siblings, 1 reply; 28+ messages in thread
From: Mario Six @ 2019-05-15  5:02 UTC (permalink / raw)
  To: u-boot

On Tue, May 14, 2019 at 3:53 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
> <Joakim.Tjernlund@infinera.com> wrote:
> >
> > On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > >
> > >
> > > Hi Jagan and Jocke,
> > >
> > > I'm back from vacation, so here's my answer:
> > >
> > > On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > + Mario
> > > >
> > > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > > <Joakim.Tjernlund@infinera.com> wrote:
> > > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > > From: Mario Six <mario.six@gdsys.cc>
> > > > > >
> > > > > > We do nothing in the loop if the "not empty" event was not detected. To
> > > > > > simplify the logic, check if this is the case, and skip the execution of
> > > > > > the loop early to reduce the nesting level and flag checking.
> > > > >
> > > > > Looked at the driver to refresh memory and noticed:
> > > > > if (charSize == 32) {
> > > > >         /* Advance output buffer by 32 bits */
> > > > >         din += 4;
> > > > > }
> > > > > which suggests that only 32 bit char will increase the din ptr so does other bitlens
> > > > > work for reading?
> > > Yes, It will work. When charSize < 32 in a loop execution, we necessarily also
> > > have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
> > > (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the data
> > > to be sent/received and also the final loop execution, so we don't have to
> > > advance the pointer anymore.
> >
> > Ahh, I see now.
> >
> > But this over use of always 32 bits cause complexity/subtile bugs. The driver should use
> > the requested charsize(wordlen) throughout. As is now you cannot use the NF/NE flag as intended or
> > toggle LSB_FIRST
>
> Look like Joakim has a point here. better we can check the required
> charsize instead of looking for magic 32 number. What do you say
> Mario?

I agree, the driver code could be made more readable and cleaned up a bit, but
I don't know if it's worth postponing this series for a code readability issue
that's not a functional bug; I'd say that's an optimization that could be made
later on.

What do you guys think?

Best regards,
Mario

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit
  2019-05-15  5:02             ` Mario Six
@ 2019-05-15  7:16               ` Joakim Tjernlund
  0 siblings, 0 replies; 28+ messages in thread
From: Joakim Tjernlund @ 2019-05-15  7:16 UTC (permalink / raw)
  To: u-boot

On Wed, 2019-05-15 at 07:02 +0200, Mario Six wrote:
> On Tue, May 14, 2019 at 3:53 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
> > <Joakim.Tjernlund@infinera.com> wrote:
> > > On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > 
> > > > 
> > > > Hi Jagan and Jocke,
> > > > 
> > > > I'm back from vacation, so here's my answer:
> > > > 
> > > > On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > + Mario
> > > > > 
> > > > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > > > <Joakim.Tjernlund@infinera.com> wrote:
> > > > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > > > From: Mario Six <mario.six@gdsys.cc>
> > > > > > > 
> > > > > > > We do nothing in the loop if the "not empty" event was not detected. To
> > > > > > > simplify the logic, check if this is the case, and skip the execution of
> > > > > > > the loop early to reduce the nesting level and flag checking.
> > > > > > 
> > > > > > Looked at the driver to refresh memory and noticed:
> > > > > > if (charSize == 32) {
> > > > > >         /* Advance output buffer by 32 bits */
> > > > > >         din += 4;
> > > > > > }
> > > > > > which suggests that only 32 bit char will increase the din ptr so does other bitlens
> > > > > > work for reading?
> > > > Yes, It will work. When charSize < 32 in a loop execution, we necessarily also
> > > > have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and charSize =
> > > > (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of the data
> > > > to be sent/received and also the final loop execution, so we don't have to
> > > > advance the pointer anymore.
> > > 
> > > Ahh, I see now.
> > > 
> > > But this over use of always 32 bits cause complexity/subtile bugs. The driver should use
> > > the requested charsize(wordlen) throughout. As is now you cannot use the NF/NE flag as intended or
> > > toggle LSB_FIRST
> > 
> > Look like Joakim has a point here. better we can check the required
> > charsize instead of looking for magic 32 number. What do you say
> > Mario?
> 
> I agree, the driver code could be made more readable and cleaned up a bit, but
> I don't know if it's worth postponing this series for a code readability issue
> that's not a functional bug; I'd say that's an optimization that could be made
> later on.

I would not call it just an optimization but I am fine with applying this patch.

 Jocke

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM
  2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
                   ` (18 preceding siblings ...)
  2019-04-28 20:28 ` [U-Boot] [PATCH v2 19/19] dm: MIGRATION: Update migration status for SPI Jagan Teki
@ 2019-05-20 16:52 ` Jagan Teki
  19 siblings, 0 replies; 28+ messages in thread
From: Jagan Teki @ 2019-05-20 16:52 UTC (permalink / raw)
  To: u-boot

On Mon, Apr 29, 2019 at 1:59 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> This would be next version mpc8xxx dm conversion.
>
> No functional changes in v2 apart from rebase, and this would
> merge soon.
>
> Changes for v2:
> - rebase on master
> - patch for SPI MIGRATION status
>
> Any inputs?
> Jagan.
>
> Jagan Teki (2):
>   spi: mpc8xxx: Convert to DM
>   dm: MIGRATION: Update migration status for SPI
>
> Mario Six (17):
>   spi: mpc8xxx: Use short type names
>   spi: mpc8xxx: Fix comments
>   spi: mpc8xxx: Rename camel-case variables
>   spi: mpc8xxx: Fix space after cast
>   spi: mpc8xxx: Fix function names in strings
>   spi: mpc8xxx: Replace defines with enums
>   spi: mpc8xxx: Use IO accessors
>   spi: mpc8xxx: Simplify if
>   spi: mpc8xxx: Get rid of is_read
>   spi: mpc8xxx: Simplify logic a bit
>   spi: mpc8xxx: Reduce scope of loop variables
>   spi: mpc8xxx: Make code more readable
>   spi: mpc8xxx: Rename variable
>   spi: mpc8xxx: Document LEN setting better
>   spi: mpc8xxx: Re-order transfer setup
>   spi: mpc8xxx: Fix if check
>   spi: mpc8xxx: Use get_timer

Applied to u-boot-spi/master

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2019-05-20 16:52 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-28 20:28 [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 01/19] spi: mpc8xxx: Use short type names Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 02/19] spi: mpc8xxx: Fix comments Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 03/19] spi: mpc8xxx: Rename camel-case variables Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 04/19] spi: mpc8xxx: Fix space after cast Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 05/19] spi: mpc8xxx: Fix function names in strings Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 06/19] spi: mpc8xxx: Replace defines with enums Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 07/19] spi: mpc8xxx: Use IO accessors Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 08/19] spi: mpc8xxx: Simplify if Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 09/19] spi: mpc8xxx: Get rid of is_read Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 10/19] spi: mpc8xxx: Simplify logic a bit Jagan Teki
2019-04-29  9:17   ` Joakim Tjernlund
2019-04-29 10:41     ` Jagan Teki
2019-05-02  5:31       ` Mario Six
2019-05-02  9:07         ` Joakim Tjernlund
2019-05-14 13:53           ` Jagan Teki
2019-05-15  5:02             ` Mario Six
2019-05-15  7:16               ` Joakim Tjernlund
2019-04-28 20:28 ` [U-Boot] [PATCH v2 11/19] spi: mpc8xxx: Reduce scope of loop variables Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 12/19] spi: mpc8xxx: Make code more readable Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 13/19] spi: mpc8xxx: Rename variable Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 14/19] spi: mpc8xxx: Document LEN setting better Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 15/19] spi: mpc8xxx: Re-order transfer setup Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 16/19] spi: mpc8xxx: Fix if check Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 17/19] spi: mpc8xxx: Use get_timer Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 18/19] spi: mpc8xxx: Convert to DM Jagan Teki
2019-04-28 20:28 ` [U-Boot] [PATCH v2 19/19] dm: MIGRATION: Update migration status for SPI Jagan Teki
2019-05-20 16:52 ` [U-Boot] [PATCH v2 00/19] spi: Convert MPC8XXX to DM Jagan Teki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.