[PATCH] spi: bitbang: only toggle bitchanges

[PATCH] spi: bitbang: only toggle bitchanges

[Michael Grzeschik]

The current implementation of bitbang_txrx_be_cpha0 and
bitbang_txrx_be_cpha1 always call setmosi. That runs into several
unnecessary calls into the gpiolib when the level of the GPIO actually
has not to be changed.

This patch changes the routines to remember the last GPIO level
and only calls setmosi if an change has to be made. This
way it improves the transfer throughput.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/spi/spi-bitbang-txrx.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index c616e41..62bdf62 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -49,12 +49,16 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
 
+	bool oldbit = !(word & 1);
 	/* 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));
+			if ((word & (1 << 31)) != oldbit) {
+				setmosi(spi, word & (1 << 31));
+				oldbit = word & (1 << 31);
+			}
 		spidelay(nsecs);	/* T(setup) */
 
 		setsck(spi, !cpol);
@@ -76,13 +80,17 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
 
+	bool oldbit = !(word & (1 << 31));
 	/* clock starts@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));
+			if ((word & (1 << 31)) != oldbit) {
+				setmosi(spi, word & (1 << 31));
+				oldbit = word & (1 << 31);
+			}
 		spidelay(nsecs); /* T(setup) */
 
 		setsck(spi, cpol);
-- 
2.1.4

[PATCH] spi: bitbang: only toggle bitchanges

[Mark Brown]

On Tue, Mar 31, 2015 at 03:54:49PM +0200, Michael Grzeschik wrote:

>  		if ((flags & SPI_MASTER_NO_TX) == 0)
> -			setmosi(spi, word & (1 << 31));
> +			if ((word & (1 << 31)) != oldbit) {
> +				setmosi(spi, word & (1 << 31));
> +				oldbit = word & (1 << 31);
> +			}
>  		spidelay(nsecs);	/* T(setup) */

This change is fine but for legibility can you please add braces around
the outer if as well?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150331/d1710f9b/attachment.sig>

[PATCH v2] spi: bitbang: only toggle bitchanges

[Michael Grzeschik]

The current implementation of bitbang_txrx_be_cpha0 and
bitbang_txrx_be_cpha1 always call setmosi. That runs into several
unnecessary calls into the gpiolib when the level of the GPIO actually
has not to be changed.

This patch changes the routines to remember the last GPIO level
and only calls setmosi if an change has to be made. This
way it improves the transfer throughput.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v2: added missing braces

 drivers/spi/spi-bitbang-txrx.h | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index c616e41..06b34e5 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -49,12 +49,17 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
 
+	bool oldbit = !(word & 1);
 	/* 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));
+		if ((flags & SPI_MASTER_NO_TX) == 0) {
+			if ((word & (1 << 31)) != oldbit) {
+				setmosi(spi, word & (1 << 31));
+				oldbit = word & (1 << 31);
+			}
+		}
 		spidelay(nsecs);	/* T(setup) */
 
 		setsck(spi, !cpol);
@@ -76,13 +81,18 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
 
+	bool oldbit = !(word & (1 << 31));
 	/* clock starts@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));
+		if ((flags & SPI_MASTER_NO_TX) == 0) {
+			if ((word & (1 << 31)) != oldbit) {
+				setmosi(spi, word & (1 << 31));
+				oldbit = word & (1 << 31);
+			}
+		}
 		spidelay(nsecs); /* T(setup) */
 
 		setsck(spi, cpol);
-- 
2.1.4

[PATCH v2] spi: bitbang: only toggle bitchanges

[Mark Brown]

On Tue, Mar 31, 2015 at 04:35:01PM +0200, Michael Grzeschik wrote:
> The current implementation of bitbang_txrx_be_cpha0 and
> bitbang_txrx_be_cpha1 always call setmosi. That runs into several
> unnecessary calls into the gpiolib when the level of the GPIO actually
> has not to be changed.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150331/4c018f9a/attachment.sig>

[PATCH v2] spi: bitbang: only toggle bitchanges

[Jonas Gorski]

On Tue, Mar 31, 2015 at 4:35 PM, Michael Grzeschik
<m.grzeschik@pengutronix.de> wrote:
> The current implementation of bitbang_txrx_be_cpha0 and
> bitbang_txrx_be_cpha1 always call setmosi. That runs into several
> unnecessary calls into the gpiolib when the level of the GPIO actually
> has not to be changed.
>
> This patch changes the routines to remember the last GPIO level
> and only calls setmosi if an change has to be made. This
> way it improves the transfer throughput.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v2: added missing braces
>
>  drivers/spi/spi-bitbang-txrx.h | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
> index c616e41..06b34e5 100644
> --- a/drivers/spi/spi-bitbang-txrx.h
> +++ b/drivers/spi/spi-bitbang-txrx.h
> @@ -49,12 +49,17 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
>  {
>         /* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
>
> +       bool oldbit = !(word & 1);

Is it intentional you check the first bit (word & 1) here? Everywhere
else you use (word & 31).

>         /* 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));
> +               if ((flags & SPI_MASTER_NO_TX) == 0) {
> +                       if ((word & (1 << 31)) != oldbit) {

You are comparing a bool to an int, and ((word & (1 << 31)) will
always be != true (== 1). Your condition needs to be !!(word & (1 <<
31)) != oldbit .

> +                               setmosi(spi, word & (1 << 31));
> +                               oldbit = word & (1 << 31);
> +                       }
> +               }
>                 spidelay(nsecs);        /* T(setup) */
>
>                 setsck(spi, !cpol);
> @@ -76,13 +81,18 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
>  {
>         /* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
>
> +       bool oldbit = !(word & (1 << 31));
>         /* 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));
> +               if ((flags & SPI_MASTER_NO_TX) == 0) {
> +                       if ((word & (1 << 31)) != oldbit) {

Same issue here.

> +                               setmosi(spi, word & (1 << 31));
> +                               oldbit = word & (1 << 31);
> +                       }
> +               }
>                 spidelay(nsecs); /* T(setup) */
>
>                 setsck(spi, cpol);
> --

Regards
Jonas

[PATCH v2] spi: bitbang: only toggle bitchanges

[Michael Grzeschik]

On Tue, Mar 31, 2015 at 08:59:28PM +0200, Jonas Gorski wrote:
> On Tue, Mar 31, 2015 at 4:35 PM, Michael Grzeschik
> <m.grzeschik@pengutronix.de> wrote:
> > The current implementation of bitbang_txrx_be_cpha0 and
> > bitbang_txrx_be_cpha1 always call setmosi. That runs into several
> > unnecessary calls into the gpiolib when the level of the GPIO actually
> > has not to be changed.
> >
> > This patch changes the routines to remember the last GPIO level
> > and only calls setmosi if an change has to be made. This
> > way it improves the transfer throughput.
> >
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > v2: added missing braces
> >
> >  drivers/spi/spi-bitbang-txrx.h | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
> > index c616e41..06b34e5 100644
> > --- a/drivers/spi/spi-bitbang-txrx.h
> > +++ b/drivers/spi/spi-bitbang-txrx.h
> > @@ -49,12 +49,17 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
> >  {
> >         /* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
> >
> > +       bool oldbit = !(word & 1);
> 
> Is it intentional you check the first bit (word & 1) here? Everywhere
> else you use (word & 31).

Doh! I was reworking this from an _le_ variant of the same code.
This hunk unfortunately is an leftover... :/

Mark, should I send a corrected version or do you squash the 31 into the
applied patch?

Thanks for the hint,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2] spi: bitbang: only toggle bitchanges

[Mark Brown]

On Tue, Mar 31, 2015 at 10:13:00PM +0200, Michael Grzeschik wrote:

> Mark, should I send a corrected version or do you squash the 31 into the
> applied patch?

If you could send me a patch (either incremental or squashed is fine)
that'd be good - I'd prefer to have something from you as I'd need to
set up a test system for bitbang.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150331/49311aa4/attachment.sig>

[PATCH v3] spi: bitbang: only toggle bitchanges

[Michael Grzeschik]

The current implementation of bitbang_txrx_be_cpha0 and
bitbang_txrx_be_cpha1 always call setmosi. That runs into several
unnecessary calls into the gpiolib when the level of the GPIO actually
has not to be changed.

This patch changes the routines to remember the last GPIO level
and only calls setmosi if an change has to be made. This
way it improves the transfer throughput.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v2: - added missing braces
v3: - fixed initial value of oldbit
    - added !! to condition for bool comparison

 drivers/spi/spi-bitbang-txrx.h | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index c616e41..58606bf 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -49,12 +49,17 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
 
+	bool oldbit = !(word & (1 << 31));
 	/* 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));
+		if ((flags & SPI_MASTER_NO_TX) == 0) {
+			if (!!(word & (1 << 31)) != oldbit) {
+				setmosi(spi, word & (1 << 31));
+				oldbit = word & (1 << 31);
+			}
+		}
 		spidelay(nsecs);	/* T(setup) */
 
 		setsck(spi, !cpol);
@@ -76,13 +81,18 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
 
+	bool oldbit = !(word & (1 << 31));
 	/* clock starts@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));
+		if ((flags & SPI_MASTER_NO_TX) == 0) {
+			if (!!(word & (1 << 31)) != oldbit) {
+				setmosi(spi, word & (1 << 31));
+				oldbit = word & (1 << 31);
+			}
+		}
 		spidelay(nsecs); /* T(setup) */
 
 		setsck(spi, cpol);
-- 
2.1.4

[PATCH v3] spi: bitbang: only toggle bitchanges

[Mark Brown]

On Mon, Apr 13, 2015 at 12:59:30PM +0200, Michael Grzeschik wrote:
> The current implementation of bitbang_txrx_be_cpha0 and
> bitbang_txrx_be_cpha1 always call setmosi. That runs into several
> unnecessary calls into the gpiolib when the level of the GPIO actually
> has not to be changed.
> 
> This patch changes the routines to remember the last GPIO level
> and only calls setmosi if an change has to be made. This
> way it improves the transfer throughput.

Do you have any benchmarking or anything showing that this offers a
practical benefit?  Generally a bitbanging system would be using memory
mapped GPIOs for the bus so we're talking about replacing a memory
access with some shifts and logic operations here and while I can
believe that something that runs in cache is going to be an overall win
over going out to a device it's not quite so clear cut as it could be.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150418/1e85d2d5/attachment.sig>

[PATCH v3] spi: bitbang: only toggle bitchanges

[Marc Kleine-Budde]

On April 18, 2015 2:09:26 PM GMT+02:00, Mark Brown <broonie@kernel.org> wrote:
>On Mon, Apr 13, 2015 at 12:59:30PM +0200, Michael Grzeschik wrote:
>> The current implementation of bitbang_txrx_be_cpha0 and
>> bitbang_txrx_be_cpha1 always call setmosi. That runs into several
>> unnecessary calls into the gpiolib when the level of the GPIO
>actually
>> has not to be changed.
>> 
>> This patch changes the routines to remember the last GPIO level
>> and only calls setmosi if an change has to be made. This
>> way it improves the transfer throughput.
>
>Do you have any benchmarking or anything showing that this offers a
>practical benefit?  Generally a bitbanging system would be using memory
>mapped GPIOs for the bus so we're talking about replacing a memory
>access with some shifts and logic operations here and while I can
>believe that something that runs in cache is going to be an overall win
>over going out to a device it's not quite so clear cut as it could be.

IIRC we had a reduction of about 30% when programming an FPGA via SPI. But Michael can give you the exact numbers. 

Marc

[PATCH v3] spi: bitbang: only toggle bitchanges

[Mark Brown]

On Sat, Apr 18, 2015 at 03:18:49PM +0200, Marc Kleine-Budde wrote:

> IIRC we had a reduction of about 30% when programming an FPGA via SPI.
> But Michael can give you the exact numbers. 

OK, that's definitely worth writing home about - putting that in the
commit message would have helped a lot!  I've applied the patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150418/dc5213ca/attachment.sig>