| Message ID | 20180212124532.25776-3-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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
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)
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(-)