| Message ID | 1394637636-29042-3-git-send-email-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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
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;
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(-)