diff mbox series

[resend,4/5] mpc8xxx_spi: always use 8-bit characters, don't read or write garbage

Message ID 20200211151947.26091-5-rasmus.villemoes@prevas.dk
State Accepted
Commit 391c40048b01dacd50243a594f74775a3ec60104
Delegated to: Tom Rini
Headers show
Series spi: mpc8xxx_spi: bug fixes, real ->set_speed and a pseudo-gpio driver | expand

Commit Message

Rasmus Villemoes Feb. 11, 2020, 3:20 p.m. UTC 
There are a few problems with the current driver.

First, it unconditionally reads from dout/writes to din whether or not
those pointers are NULL. So for example a simple "sf probe" ends up
writing four bytes at address 0:

=> md.l 0x0 8
00000000: 45454545 45454545 05050505 05050505    EEEEEEEE........
00000010: 00000000 00000000 07070707 07070707    ................
=> sf probe 0
mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB53618 din 00000000 bitlen 8
mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 0FB536B8 bitlen 48
SF: Detected s25sl032p with page size 256 Bytes, erase size 64 KiB, total 4 MiB
=> md.l 0x0 8
00000000: ff000000 45454545 05050505 05050505    ....EEEE........
00000010: 00000000 00000000 07070707 07070707    ................

(here I've change the first debug statement to a printf, and made it
print the din/dout pointers rather than the uints they point at).

Second, as we can also see above, it always writes a full 32 bits,
even if a smaller amount was requested. So for example

=> mw.l $loadaddr 0xaabbccdd 8
=> md.l $loadaddr 8
02000000: aabbccdd aabbccdd aabbccdd aabbccdd    ................
02000010: aabbccdd aabbccdd aabbccdd aabbccdd    ................
=> sf read $loadaddr 0x400 6
device 0 offset 0x400, size 0x6
mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din 00000000 bitlen 40
mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000000 bitlen 48
SF: 6 bytes @ 0x400 Read: OK
=> sf read 0x02000010 0x400 8
device 0 offset 0x400, size 0x8
mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB53848 din 00000000 bitlen 40
mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000010 bitlen 64
SF: 8 bytes @ 0x400 Read: OK
=> md.l $loadaddr 8
02000000: 45454545 45450000 aabbccdd aabbccdd    EEEEEE..........
02000010: 45454545 45454545 aabbccdd aabbccdd    EEEEEEEE........

Finally, when the bitlen is 24 mod 32 (e.g. requesting to read 3 or 7
bytes), the last three bytes and up being the wrong ones, since the
driver does a full 32 bit read and then shifts the wrong byte out:

=> mw.l $loadaddr 0xaabbccdd 4
=> md.l $loadaddr 4
02000000: aabbccdd aabbccdd aabbccdd aabbccdd    ................
=> sf read $loadaddr 0x444 10
device 0 offset 0x444, size 0x10
mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din 00000000 bitlen 40
mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000000 bitlen 128
SF: 16 bytes @ 0x444 Read: OK
=> md.l $loadaddr 4
02000000: 552d426f 6f742032 3031392e 30342d30    U-Boot 2019.04-0
=> mw.l $loadaddr 0xaabbccdd 4
=> sf read $loadaddr 0x444 0xb
device 0 offset 0x444, size 0xb
mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din 00000000 bitlen 40
mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000000 bitlen 88
SF: 11 bytes @ 0x444 Read: OK
=> md.l $loadaddr 4
02000000: 552d426f 6f742032 31392e00 aabbccdd    U-Boot 219......

Fix all of that by always using a character size of 8, and reject
transfers that are not a whole number of bytes. While it ends being
more work for the CPU, we're mostly bounded by the speed of the SPI
bus, and we avoid writing to the mode register in every loop.

Based on work by Klaus H. Sørensen.

Cc: Klaus H. Sorensen <khso@prevas.dk>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/spi/mpc8xxx_spi.c | 80 +++++++++++++--------------------------
 1 file changed, 27 insertions(+), 53 deletions(-)

Comments

Tom Rini March 31, 2020, 7:10 p.m. UTC | #1 
On Tue, Feb 11, 2020 at 03:20:25PM +0000, Rasmus Villemoes wrote:

> There are a few problems with the current driver.
> 
> First, it unconditionally reads from dout/writes to din whether or not
> those pointers are NULL. So for example a simple "sf probe" ends up
> writing four bytes at address 0:
> 
> => md.l 0x0 8
> 00000000: 45454545 45454545 05050505 05050505    EEEEEEEE........
> 00000010: 00000000 00000000 07070707 07070707    ................
> => sf probe 0
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB53618 din 00000000 bitlen 8
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 0FB536B8 bitlen 48
> SF: Detected s25sl032p with page size 256 Bytes, erase size 64 KiB, total 4 MiB
> => md.l 0x0 8
> 00000000: ff000000 45454545 05050505 05050505    ....EEEE........
> 00000010: 00000000 00000000 07070707 07070707    ................
> 
> (here I've change the first debug statement to a printf, and made it
> print the din/dout pointers rather than the uints they point at).
> 
> Second, as we can also see above, it always writes a full 32 bits,
> even if a smaller amount was requested. So for example
> 
> => mw.l $loadaddr 0xaabbccdd 8
> => md.l $loadaddr 8
> 02000000: aabbccdd aabbccdd aabbccdd aabbccdd    ................
> 02000010: aabbccdd aabbccdd aabbccdd aabbccdd    ................
> => sf read $loadaddr 0x400 6
> device 0 offset 0x400, size 0x6
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din 00000000 bitlen 40
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000000 bitlen 48
> SF: 6 bytes @ 0x400 Read: OK
> => sf read 0x02000010 0x400 8
> device 0 offset 0x400, size 0x8
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB53848 din 00000000 bitlen 40
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000010 bitlen 64
> SF: 8 bytes @ 0x400 Read: OK
> => md.l $loadaddr 8
> 02000000: 45454545 45450000 aabbccdd aabbccdd    EEEEEE..........
> 02000010: 45454545 45454545 aabbccdd aabbccdd    EEEEEEEE........
> 
> Finally, when the bitlen is 24 mod 32 (e.g. requesting to read 3 or 7
> bytes), the last three bytes and up being the wrong ones, since the
> driver does a full 32 bit read and then shifts the wrong byte out:
> 
> => mw.l $loadaddr 0xaabbccdd 4
> => md.l $loadaddr 4
> 02000000: aabbccdd aabbccdd aabbccdd aabbccdd    ................
> => sf read $loadaddr 0x444 10
> device 0 offset 0x444, size 0x10
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din 00000000 bitlen 40
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000000 bitlen 128
> SF: 16 bytes @ 0x444 Read: OK
> => md.l $loadaddr 4
> 02000000: 552d426f 6f742032 3031392e 30342d30    U-Boot 2019.04-0
> => mw.l $loadaddr 0xaabbccdd 4
> => sf read $loadaddr 0x444 0xb
> device 0 offset 0x444, size 0xb
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 0FB536E8 din 00000000 bitlen 40
> mpc8xxx_spi_xfer: slave spi@7000:0 dout 00000000 din 02000000 bitlen 88
> SF: 11 bytes @ 0x444 Read: OK
> => md.l $loadaddr 4
> 02000000: 552d426f 6f742032 31392e00 aabbccdd    U-Boot 219......
> 
> Fix all of that by always using a character size of 8, and reject
> transfers that are not a whole number of bytes. While it ends being
> more work for the CPU, we're mostly bounded by the speed of the SPI
> bus, and we avoid writing to the mode register in every loop.
> 
> Based on work by Klaus H. Sørensen.
> 
> Cc: Klaus H. Sorensen <khso@prevas.dk>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index ac4d0a9bae..8ef2451411 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -27,6 +27,7 @@  enum {
 	SPI_MODE_EN    = BIT(31 - 7),	/* Enable interface */
 
 	SPI_MODE_LEN_MASK = 0xf00000,
+	SPI_MODE_LEN_SHIFT = 20,
 	SPI_MODE_PM_MASK = 0xf0000,
 
 	SPI_COM_LST = BIT(31 - 9),
@@ -43,23 +44,8 @@  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
 
-static int __spi_set_speed(spi8xxx_t *spi, uint speed)
-{
-	/* TODO(mario.six@gdsys.cc): This only ever sets one fixed speed */
-
-	/* Use SYSCLK / 8 (16.67MHz typ.) */
-	clrsetbits_be32(&spi->mode, SPI_MODE_PM_MASK, to_prescale_mod(1));
-
-	return 0;
-}
-
 static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev)
 {
 	struct mpc8xxx_priv *priv = dev_get_priv(dev);
@@ -82,14 +68,22 @@  static int mpc8xxx_spi_ofdata_to_platdata(struct udevice *dev)
 static int mpc8xxx_spi_probe(struct udevice *dev)
 {
 	struct mpc8xxx_priv *priv = dev_get_priv(dev);
+	spi8xxx_t *spi = priv->spi;
 
 	/*
 	 * SPI pins on the MPC83xx are not muxed, so all we do is initialize
 	 * some registers
 	 */
-	out_be32(&priv->spi->mode, SPI_MODE_REV | SPI_MODE_MS | SPI_MODE_EN);
+	out_be32(&priv->spi->mode, SPI_MODE_REV | SPI_MODE_MS);
+
+	/* set len to 8 bits */
+	setbits_be32(&spi->mode, (8 - 1) << SPI_MODE_LEN_SHIFT);
 
-	__spi_set_speed(priv->spi, 16666667);
+	/* TODO(mario.six@gdsys.cc): This only ever sets one fixed speed */
+	/* Use SYSCLK / 8 (16.67MHz typ.) */
+	clrsetbits_be32(&spi->mode, SPI_MODE_PM_MASK, to_prescale_mod(1));
+
+	setbits_be32(&spi->mode, SPI_MODE_EN);
 
 	/* Clear all SPI events */
 	setbits_be32(&priv->spi->event, 0xffffffff);
@@ -126,50 +120,35 @@  static int mpc8xxx_spi_xfer(struct udevice *dev, uint bitlen,
 	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);
+	u32 tmpdin = 0, tmpdout = 0, n;
+	const u8 *cout = dout;
+	u8 *cin = din;
 
 	debug("%s: slave %s:%u dout %08X din %08X bitlen %u\n", __func__,
-	      bus->name, platdata->cs, *(uint *)dout, *(uint *)din, bitlen);
+	      bus->name, platdata->cs, (uint)dout, (uint)din, bitlen);
 	if (platdata->cs >= priv->cs_count) {
 		dev_err(dev, "chip select index %d too large (cs_count=%d)\n",
 			platdata->cs, priv->cs_count);
 		return -EINVAL;
 	}
+	if (bitlen % 8) {
+		printf("*** spi_xfer: bitlen must be multiple of 8\n");
+		return -ENOTSUPP;
+	}
 
 	if (flags & SPI_XFER_BEGIN)
 		mpc8xxx_spi_cs_activate(dev);
 
 	/* Clear all SPI events */
 	setbits_be32(&spi->event, 0xffffffff);
+	n = bitlen / 8;
 
-	/* Handle data in 32-bit chunks */
-	while (num_blks--) {
-		u32 tmpdout = 0;
-		uchar xfer_bitlen = (bitlen >= 32 ? 32 : bitlen);
+	/* Handle data in 8-bit chunks */
+	while (n--) {
 		ulong start;
 
-		clrbits_be32(&spi->mode, SPI_MODE_EN);
-
-		/* Set up length for this transfer */
-
-		if (bitlen <= 4) /* 4 bits or less */
-			set_char_len(spi, 3);
-		else if (bitlen <= 16) /* at most 16 bits */
-			set_char_len(spi, bitlen - 1);
-		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 > 32) {
-			/* Set up the next iteration if sending > 32 bits */
-			bitlen -= 32;
-			dout += 4;
-		}
+		if (cout)
+			tmpdout = *cout++;
 
 		/* Write the data out */
 		out_be32(&spi->tx, tmpdout);
@@ -193,11 +172,8 @@  static int mpc8xxx_spi_xfer(struct udevice *dev, uint bitlen,
 			tmpdin = in_be32(&spi->rx);
 			setbits_be32(&spi->event, SPI_EV_NE);
 
-			*(u32 *)din = (tmpdin << (32 - xfer_bitlen));
-			if (xfer_bitlen == 32) {
-				/* Advance output buffer by 32 bits */
-				din += 4;
-			}
+			if (cin)
+				*cin++ = tmpdin;
 
 			/*
 			 * Only bail when we've had both NE and NF events.
@@ -228,9 +204,7 @@  static int mpc8xxx_spi_xfer(struct udevice *dev, uint bitlen,
 
 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);
+	return 0;
 }
 
 static int mpc8xxx_spi_set_mode(struct udevice *dev, uint mode)