[2/3] spi: spi-gpio: Delete references to non-GENERIC_BITBANG

Message ID 20180212124532.25776-3-linus.walleij@linaro.org
State New
Headers show
Series
  • Convert GPIO SPI to use descriptors only
Related show

Commit Message

Linus Walleij Feb. 12, 2018, 12:45 p.m.
The non-generic bitbang was a feature where a platform could optimize
SPI bit-banging by inlining the routines to hammer GPIO lines into
the GPIO bitbanging driver as direct register writes using a custom
set of GPIO library calls.

It does not work with multiplatform concepts, violates everything
about how GPIO is made generic and is just generally a bad idea,
even on legacy system. Also there is no single user in the entire
kernel (for good reasons).

Delete the remnants of this optimization.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/spi/spi-gpio.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

-- 
2.14.3

--
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

Comments

Linus Walleij Feb. 23, 2018, 10:46 a.m. | #1
On Wed, Feb 14, 2018 at 7:53 PM, Trent Piepho <tpiepho@impinj.com> wrote:
> On Wed, 2018-02-14 at 16:01 +0000, Mark Brown wrote:

>> On Mon, Feb 12, 2018 at 01:45:31PM +0100, Linus Walleij wrote:

>>

>> > The non-generic bitbang was a feature where a platform could optimize

>> > SPI bit-banging by inlining the routines to hammer GPIO lines into

>> > the GPIO bitbanging driver as direct register writes using a custom

>> > set of GPIO library calls.

>> > It does not work with multiplatform concepts, violates everything

>> > about how GPIO is made generic and is just generally a bad idea,

>> > even on legacy system. Also there is no single user in the entire

>> > kernel (for good reasons).

>>

>> It's not expected that users should go upstream and it doesn't seem

>> helpful to delete without replacement.  The original idea was to allow

>> things like setting multiple GPIOs at once if there were calls for that,

>> now that gpiolib has support for that we should at least convert to it

>> before removing the hook.

>

> This ability really does make a difference.  I wrote a bit-banged JTAG

> driver a powerpc embedded device.  Using the int based gpiolib, I could

> get about a 1 MHz clock.  By allowing one to define a wrapper that

> could take compiled in gpio settings, I could get that clock up to

> something like 20 MHz.  The device had to load an FPGA image via JTAG

> when it started, so this was something like 5 seconds of boot time that

> would become almost two minutes without the ability to do GPIOs really

> fast.  That's the difference between a product appearing to customers

> as quick compared to what they expect vs pathetic.

>

> Of course this never made it into the upstream kernel.  It only worked

> on that specific product.

>

> The biggest win was due to locking.  gpiolib needs each gpio set/get to

> be atomic vs other gpio consumers, so you end up with locks around each

> call.  This is huge overhead when a gpio only needs a single machine

> instruction.  I could tell the wrapper to lock the gpios at the start

> of a JTAG op and unlock after and reduce this overhead by orders of

> magnitude.


I'm dropping this patch for now, we could revisit it and look at the
new gpiolib API for driving multiple lines with single register writes.

I remember I actually pushed SPI as an example I wanted to see
converted when we implemented that API :/

Yours,
Linus Walleij
--
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 --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index b85a93cad44a..2e5f092813f1 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -53,34 +53,8 @@  struct spi_gpio {
 
 /*----------------------------------------------------------------------*/
 
-/*
- * Because the overhead of going through four GPIO procedure calls
- * per transferred bit can make performance a problem, this code
- * is set up so that you can use it in either of two ways:
- *
- *   - The slow generic way:  set up platform_data to hold the GPIO
- *     numbers used for MISO/MOSI/SCK, and issue procedure calls for
- *     each of them.  This driver can handle several such busses.
- *
- *   - The quicker inlined way:  only helps with platform GPIO code
- *     that inlines operations for constant GPIOs.  This can give
- *     you tight (fast!) inner loops, but each such bus needs a
- *     new driver.  You'll define a new C file, with Makefile and
- *     Kconfig support; the C code can be a total of six lines:
- *
- *		#define DRIVER_NAME	"myboard_spi2"
- *		#define	SPI_MISO_GPIO	119
- *		#define	SPI_MOSI_GPIO	120
- *		#define	SPI_SCK_GPIO	121
- *		#define	SPI_N_CHIPSEL	4
- *		#include "spi-gpio.c"
- */
-
 #ifndef DRIVER_NAME
 #define DRIVER_NAME	"spi_gpio"
-
-#define GENERIC_BITBANG	/* vs tight inlines */
-
 #endif
 
 /*----------------------------------------------------------------------*/
@@ -362,10 +336,8 @@  static int spi_gpio_probe(struct platform_device *pdev)
 		use_of = 1;
 
 	pdata = dev_get_platdata(&pdev->dev);
-#ifdef GENERIC_BITBANG
 	if (!pdata || (!use_of && !pdata->num_chipselect))
 		return -ENODEV;
-#endif
 
 	master = spi_alloc_master(&pdev->dev, sizeof(*spi_gpio));
 	if (!master)