[2/3] spi: bitbang: add lsb first support
diff mbox

Message ID 1394637636-29042-3-git-send-email-m.grzeschik@pengutronix.de
State New, archived
Headers show

Commit Message

Michael Grzeschik March 12, 2014, 3:20 p.m. UTC
The bitbang spi driver currently only supports the MSB mode. This patch
adds the possibility to clock the data in LSB mode.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/spi/spi-bitbang-txrx.h | 98 +++++++++++++++++++++++++++++-------------
 drivers/spi/spi-bitbang.c      |  3 +-
 2 files changed, 71 insertions(+), 30 deletions(-)

Comments

Gerhard Sittig March 12, 2014, 9:29 p.m. UTC | #1
On Wed, Mar 12, 2014 at 16:20 +0100, Michael Grzeschik wrote:
> 
> The bitbang spi driver currently only supports the MSB mode. This patch
> adds the possibility to clock the data in LSB mode.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/spi/spi-bitbang-txrx.h | 98 +++++++++++++++++++++++++++++-------------
>  drivers/spi/spi-bitbang.c      |  3 +-
>  2 files changed, 71 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
> index b6e348d..7f9c020 100644
> --- a/drivers/spi/spi-bitbang-txrx.h
> +++ b/drivers/spi/spi-bitbang-txrx.h
> @@ -49,22 +49,42 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
>  {
>  	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
>  
> -	/* clock starts at inactive polarity */
> -	for (word <<= (32 - bits); likely(bits); bits--) {
> -
> -		/* setup MSB (to slave) on trailing edge */
> -		if ((flags & SPI_MASTER_NO_TX) == 0)
> -			setmosi(spi, word & (1 << 31));
> -		spidelay(nsecs);	/* T(setup) */
> -
> -		setsck(spi, !cpol);
> -		spidelay(nsecs);
> -
> -		/* sample MSB (from slave) on leading edge */
> -		if ((flags & SPI_MASTER_NO_RX) == 0)
> -			word |= getmiso(spi);
> -		setsck(spi, cpol);
> -		word <<= 1;
> +	if (spi->mode & SPI_LSB_FIRST) {
> +		/* clock starts at inactive polarity */
> +		for (; likely(bits); bits--) {
> +
> +			/* setup MSB (to slave) on trailing edge */
> +			if ((flags & SPI_MASTER_NO_TX) == 0)
> +				setmosi(spi, word & 1);
> +			spidelay(nsecs);	/* T(setup) */
> +
> +			setsck(spi, !cpol);
> +			spidelay(nsecs);
> +
> +			/* sample LSB (from slave) on leading edge */
> +			if ((flags & SPI_MASTER_NO_RX) == 0)
> +				word |= getmiso(spi);
> +			setsck(spi, cpol);
> +			word >>= 1;
> +		}
> +	} else {
> +		/* clock starts at inactive polarity */
> +		for (word <<= (32 - bits); likely(bits); bits--) {
> +
> +			/* setup MSB (to slave) on trailing edge */
> +			if ((flags & SPI_MASTER_NO_TX) == 0)
> +				setmosi(spi, word & (1 << 31));
> +			spidelay(nsecs);        /* T(setup) */
> +
> +			setsck(spi, !cpol);
> +			spidelay(nsecs);
> +
> +			/* sample MSB (from slave) on leading edge */
> +			if ((flags & SPI_MASTER_NO_RX) == 0)
> +				word |= getmiso(spi);
> +			setsck(spi, cpol);
> +			word <<= 1;
> +		}
>  	}
>  	return word;
>  }

Would it be useful to not duplicate the transmission logic, but
instead to optionally "bit swap" the TX and RX data before and
after the common transmission logic?  Just a though whether this
might help maintenance.  There might even be existing and proven
code to bit swap integers?


virtually yours
Gerhard Sittig
Mark Brown March 12, 2014, 9:35 p.m. UTC | #2
On Wed, Mar 12, 2014 at 10:29:41PM +0100, Gerhard Sittig wrote:

> Would it be useful to not duplicate the transmission logic, but
> instead to optionally "bit swap" the TX and RX data before and
> after the common transmission logic?  Just a though whether this
> might help maintenance.  There might even be existing and proven
> code to bit swap integers?

I was wondering myself, but on the other hand that's an extra operation
and this is about as CPU intensive as one can get.
Gerhard Sittig March 13, 2014, 6:11 p.m. UTC | #3
On Wed, Mar 12, 2014 at 21:35 +0000, Mark Brown wrote:
> 
> On Wed, Mar 12, 2014 at 10:29:41PM +0100, Gerhard Sittig wrote:
> 
> > Would it be useful to not duplicate the transmission logic, but
> > instead to optionally "bit swap" the TX and RX data before and
> > after the common transmission logic?  Just a though whether this
> > might help maintenance.  There might even be existing and proven
> > code to bit swap integers?
> 
> I was wondering myself, but on the other hand that's an extra operation
> and this is about as CPU intensive as one can get.

Actually I thought that bitbanging pins including the timing
would be the most expensive part, and that swapping bits might
not be negligable but could be acceptable in comparison.  But
this was just a guess, no research was done.


http://graphics.stanford.edu/~seander/bithacks.html#BitReverseObvious
and the following sections suggest that code might get less
expensive when you accept to no longer see at first glance what's
happening.  Given a good identifier and an appropriate comment,
http://graphics.stanford.edu/~seander/bithacks.html#ReverseParallel
might be acceptable for mainline.

Naively reading the code, you would expect some 15 assembly
instructions and only two registers in use (for ARM).  A quick
experiment shows that the 32bit constants add some more work, yet
the complete swapping is done in 88 bytes of code (a sequence of
19 instructions and three constants).  Other architectures weight
in at some 120 bytes of code.

How do some 50 additional CPU instructions sound (calling the bit
swapper two times), in comparison to the whole SPI bitbanging
loop, for the optional case of LSB first transfers and 32bit
data?  In contrast to duplicating the core bitbanging routine's
body (two times for differing SPI modes).


BTW are these bitbang routines referenced from several sites, not
only spi-gpio.c.  They encode "be" in their name which might
become misleading then they operate in LSB mode as well.

And spidelay() appears to sometimes be a NOP -- could this be the
reason for unreliable data transmission and bit errors in certain
environments (optimized code, inlined GPIO access, fast CPU, slow
SPI slave), instead of the suspected receiver's bit shifting?


virtually yours
Gerhard Sittig
Mark Brown March 13, 2014, 6:41 p.m. UTC | #4
On Thu, Mar 13, 2014 at 07:11:36PM +0100, Gerhard Sittig wrote:
> On Wed, Mar 12, 2014 at 21:35 +0000, Mark Brown wrote:

> > I was wondering myself, but on the other hand that's an extra operation
> > and this is about as CPU intensive as one can get.

> Actually I thought that bitbanging pins including the timing
> would be the most expensive part, and that swapping bits might
> not be negligable but could be acceptable in comparison.  But
> this was just a guess, no research was done.

I've not done any research here either really.  My thinking here is
based on the fact that everything other than the delays is essentially
overhead on those delays that stretches the time taken to perform the
overall operation.

> BTW are these bitbang routines referenced from several sites, not
> only spi-gpio.c.  They encode "be" in their name which might
> become misleading then they operate in LSB mode as well.

Indeed.

> And spidelay() appears to sometimes be a NOP -- could this be the
> reason for unreliable data transmission and bit errors in certain
> environments (optimized code, inlined GPIO access, fast CPU, slow
> SPI slave), instead of the suspected receiver's bit shifting?

That's a very interesting observation - I hadn't noticed that this was
the case.  That code is pretty old, modern processors are a lot faster
and it does seem entirely possible that we're able to bash data out
quickly enough to cause problems for some systems, especially if the
timing is uneven which I'd not be surprised by.
Geert Uytterhoeven March 13, 2014, 7:22 p.m. UTC | #5
Hi Gerhard,

On Thu, Mar 13, 2014 at 7:11 PM, Gerhard Sittig <gsi@denx.de> wrote:
> http://graphics.stanford.edu/~seander/bithacks.html#BitReverseObvious
> and the following sections suggest that code might get less
> expensive when you accept to no longer see at first glance what's
> happening.  Given a good identifier and an appropriate comment,
> http://graphics.stanford.edu/~seander/bithacks.html#ReverseParallel
> might be acceptable for mainline.

I had faint memories of seeing this algorithm in the Linux kernel sources,
but I can't seem to find it anymore.

We do have a different implementation in lib/bitrev.c.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index b6e348d..7f9c020 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -49,22 +49,42 @@  bitbang_txrx_be_cpha0(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
 
-	/* clock starts at inactive polarity */
-	for (word <<= (32 - bits); likely(bits); bits--) {
-
-		/* setup MSB (to slave) on trailing edge */
-		if ((flags & SPI_MASTER_NO_TX) == 0)
-			setmosi(spi, word & (1 << 31));
-		spidelay(nsecs);	/* T(setup) */
-
-		setsck(spi, !cpol);
-		spidelay(nsecs);
-
-		/* sample MSB (from slave) on leading edge */
-		if ((flags & SPI_MASTER_NO_RX) == 0)
-			word |= getmiso(spi);
-		setsck(spi, cpol);
-		word <<= 1;
+	if (spi->mode & SPI_LSB_FIRST) {
+		/* clock starts at inactive polarity */
+		for (; likely(bits); bits--) {
+
+			/* setup MSB (to slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & 1);
+			spidelay(nsecs);	/* T(setup) */
+
+			setsck(spi, !cpol);
+			spidelay(nsecs);
+
+			/* sample LSB (from slave) on leading edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			setsck(spi, cpol);
+			word >>= 1;
+		}
+	} else {
+		/* clock starts at inactive polarity */
+		for (word <<= (32 - bits); likely(bits); bits--) {
+
+			/* setup MSB (to slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & (1 << 31));
+			spidelay(nsecs);        /* T(setup) */
+
+			setsck(spi, !cpol);
+			spidelay(nsecs);
+
+			/* sample MSB (from slave) on leading edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			setsck(spi, cpol);
+			word <<= 1;
+		}
 	}
 	return word;
 }
@@ -76,22 +96,42 @@  bitbang_txrx_be_cpha1(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
 
-	/* clock starts at inactive polarity */
-	for (word <<= (32 - bits); likely(bits); bits--) {
+	if (spi->mode & SPI_LSB_FIRST) {
+		/* clock starts at inactive polarity */
+		for (; likely(bits); bits--) {
+
+			/* setup MSB (to slave) on leading edge */
+			setsck(spi, !cpol);
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & 1);
+			spidelay(nsecs); /* T(setup) */
+
+			setsck(spi, cpol);
+			spidelay(nsecs);
+
+			/* sample MSB (from slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			word >>= 1;
+		}
+	} else {
+		/* clock starts at inactive polarity */
+		for (word <<= (32 - bits); likely(bits); bits--) {
 
-		/* setup MSB (to slave) on leading edge */
-		setsck(spi, !cpol);
-		if ((flags & SPI_MASTER_NO_TX) == 0)
-			setmosi(spi, word & (1 << 31));
-		spidelay(nsecs); /* T(setup) */
+			/* setup MSB (to slave) on leading edge */
+			setsck(spi, !cpol);
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & (1 << 31));
+			spidelay(nsecs); /* T(setup) */
 
-		setsck(spi, cpol);
-		spidelay(nsecs);
+			setsck(spi, cpol);
+			spidelay(nsecs);
 
-		/* sample MSB (from slave) on trailing edge */
-		if ((flags & SPI_MASTER_NO_RX) == 0)
-			word |= getmiso(spi);
-		word <<= 1;
+			/* sample MSB (from slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			word <<= 1;
+		}
 	}
 	return word;
 }
diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index bd222f6..3624f96 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -432,7 +432,8 @@  int spi_bitbang_start(struct spi_bitbang *bitbang)
 	spin_lock_init(&bitbang->lock);
 
 	if (!master->mode_bits)
-		master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags;
+		master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST
+			| bitbang->flags;
 
 	if (master->transfer || master->transfer_one_message)
 		return -EINVAL;