RFT: spi: bcm2835: Convert to use CS GPIO descriptors
diff mbox series

Message ID 20190130092551.14856-1-linus.walleij@linaro.org
State New, archived
Headers show
Series
  • RFT: spi: bcm2835: Convert to use CS GPIO descriptors
Related show

Commit Message

Linus Walleij Jan. 30, 2019, 9:25 a.m. UTC
This converts the BCM2835 SPI master driver to use GPIO
descriptors for chip select handling.

The BCM2835 driver was relying on the core to drive the
CS high/low so very small changes were needed for this
part. If it managed to request the CS from the device tree
node, all is pretty straight forward.

However for native GPIOs this driver has a quite unorthodox
loopback to request some GPIOs from the SoC GPIO chip by
looking it up from the device tree using gpiochip_find()
and then offseting hard into its numberspace. This has
been augmented a bit by using gpiochip_request_own_desc()
but this code really needs to be verified. If "native CS"
is actually an SoC GPIO, why is it even done this way?
Should this GPIO not just be defined in the device tree
like any other CS GPIO? I'm confused.

Cc: Lukas Wunner <lukas@wunner.de>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Chris Boot <bootc@bootc.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I would very much appreciate if someone took this for
a ride on top of the SPI tree (or linux-next) and see
if all still works as expected.
---
 drivers/spi/spi-bcm2835.c | 42 ++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 16 deletions(-)

Comments

Lukas Wunner Jan. 31, 2019, 6:17 a.m. UTC | #1
On Wed, Jan 30, 2019 at 10:25:51AM +0100, Linus Walleij wrote:
> However for native GPIOs this driver has a quite unorthodox
> loopback to request some GPIOs from the SoC GPIO chip by
> looking it up from the device tree using gpiochip_find()
> and then offseting hard into its numberspace. This has
> been augmented a bit by using gpiochip_request_own_desc()
> but this code really needs to be verified. If "native CS"
> is actually an SoC GPIO, why is it even done this way?
> Should this GPIO not just be defined in the device tree
> like any other CS GPIO? I'm confused.
[...]
> +
> +	/*
> +	 * Translate native CS to GPIO
> +	 *
> +	 * FIXME: poking around in the gpiolib internals like this is
> +	 * not very good practice. Find a way to locate the real problem
> +	 * and fix it. Why is the GPIO descriptor in spi->cs_gpiod
> +	 * sometimes not assigned correctly? Erroneous device trees?
> +	 */

The spi0 master on the Raspberry Pi has two bits in the CS register
(Control & Status register bits 0 and 1) to set Chip Select.

For this to work the SPI slaves' Chip Select pin needs to be connected
to GPIO 8 (Chip Select 0) / GPIO 7 (Chip Select 1) or alternatively
to GPIO 36 (Chip Select 0) / GPIO 35 (Chip Select 1).  And those GPIO pins
need to be set to function "alt0" in the pin controller.

Martin Sperl found out that controlling Chip Select in this way is
unreliable, cf. commit e3a2be3030e2 ("spi: bcm2835: fill FIFO before
enabling interrupts to reduce interrupts/message"):

   "To reduce the number of interrupts/message we fill the FIFO before
    enabling interrupts - for short messages this reduces the interrupt count
    from 2 to 1 interrupt.

    There have been rare cases where short (<200ns) chip-select switches with
    native CS have been observed during such operation, this is why this
    optimization is only enabled for GPIO-CS."

For this reason Martin amended the driver with commit a30a555d7435
("spi: bcm2835: transform native-cs to gpio-cs on first spi_setup")
such that Chip Select is never controlled via the two bits in the
CS register, but rather by having the SPI core drive the GPIO pins
explicitly.  I have sinced added further optimizations which likely
wouldn't work reliably with native CS.

The device tree in the downstream (Foundation) kernel tree has the
cs-gpios property, hence uses GPIO Chip Select by default.
The upstream (mainline) kernel does not have the cs-gpios property,
so I suppose on that one spi-bcm2835.c converts the native CS to a
GPIO CS on ->setup().  Older Foundation device trees may likewise
lack the cs-gpios property, I'm not sure.

The code in bcm2835_spi_setup() is broken in that it always assumes that
GPIO 8 / 7 is used.  It should instead check whether the function of
GPIO 8 / 7 or GPIO 36 / 35 is "alt0".  If neither of GPIO 8 / 36 (or
7 / 35 if "reg" is 1) is set to "alt0", the function should return a
negative error code.  I'm not sure what the expected behaviour is if
*both* GPIO 8 / 36 (or 7 / 35) are set to "alt0".  I suspect the correct
thing to do would be to drive both pins because we don't know which one
the slave is attached to.  Because the SPI core only supports a single
CS GPIO per slave, it would be necessary to request both pins in the
driver and add a ->set_cs() callback which sets either or both pins
depending on which one is set to "alt0".

So, it's complicated.

Personally I've never used the native CS to GPIO CS conversion mechanism
(because the Revolution Pi ships with a Foundation-based kernel and DT).

By the way, the spec reference for the above is page 155 (CS register of
the SPI master) and 102 (pin controller function table):
https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

Thanks,

Lukas
Linus Walleij Jan. 31, 2019, 8:54 a.m. UTC | #2
On Thu, Jan 31, 2019 at 7:17 AM Lukas Wunner <lukas@wunner.de> wrote:

> The device tree in the downstream (Foundation) kernel tree has the
> cs-gpios property, hence uses GPIO Chip Select by default.
> The upstream (mainline) kernel does not have the cs-gpios property,

That sounds like something that should be fixed, so also upstream
uses cs-gpios. I suppose we have to keep the hack around until
noone in the whole world use the old dts/dtb files. (Which means
I will maybe attempt delete it in some year or two.)

> The code in bcm2835_spi_setup() is broken in that it always assumes that
> GPIO 8 / 7 is used.  It should instead check whether the function of
> GPIO 8 / 7 or GPIO 36 / 35 is "alt0".  If neither of GPIO 8 / 36 (or
> 7 / 35 if "reg" is 1) is set to "alt0", the function should return a
> negative error code.  I'm not sure what the expected behaviour is if
> *both* GPIO 8 / 36 (or 7 / 35) are set to "alt0".  I suspect the correct
> thing to do would be to drive both pins because we don't know which one
> the slave is attached to.  Because the SPI core only supports a single
> CS GPIO per slave, it would be necessary to request both pins in the
> driver and add a ->set_cs() callback which sets either or both pins
> depending on which one is set to "alt0".

This looks like a nightmare. Better just use the GPIOs then.

> So, it's complicated.

Yeah I agree.

Do you think you can take this patch for a test drive on top of
the SPI tree or linux-next and see if it works as expected?

I can reword the comment if need be.

Yours,
Linus Walleij

Patch
diff mbox series

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 35aebdfd3b4e..464cbfe70778 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -33,7 +33,8 @@ 
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h> /* FIXME: using chip internals */
 #include <linux/of_irq.h>
 #include <linux/spi/spi.h>
 
@@ -881,12 +882,17 @@  static int bcm2835_spi_setup(struct spi_device *spi)
 {
 	int err;
 	struct gpio_chip *chip;
+	enum gpiod_flags flags;
 	/*
 	 * sanity checking the native-chipselects
 	 */
 	if (spi->mode & SPI_NO_CS)
 		return 0;
-	if (gpio_is_valid(spi->cs_gpio))
+	/*
+	 * The SPI core has successfully requested the CS GPIO line from the
+	 * device tree, so we are done.
+	 */
+	if (spi->cs_gpiod)
 		return 0;
 	if (spi->chip_select > 1) {
 		/* error in the case of native CS requested with CS > 1
@@ -897,7 +903,15 @@  static int bcm2835_spi_setup(struct spi_device *spi)
 			"setup: only two native chip-selects are supported\n");
 		return -EINVAL;
 	}
-	/* now translate native cs to GPIO */
+
+	/*
+	 * Translate native CS to GPIO
+	 *
+	 * FIXME: poking around in the gpiolib internals like this is
+	 * not very good practice. Find a way to locate the real problem
+	 * and fix it. Why is the GPIO descriptor in spi->cs_gpiod
+	 * sometimes not assigned correctly? Erroneous device trees?
+	 */
 
 	/* get the gpio chip for the base */
 	chip = gpiochip_find("pinctrl-bcm2835", chip_match_name);
@@ -905,21 +919,16 @@  static int bcm2835_spi_setup(struct spi_device *spi)
 		return 0;
 
 	/* and calculate the real CS */
-	spi->cs_gpio = chip->base + 8 - spi->chip_select;
+	flags = (spi->mode & SPI_CS_HIGH) ? GPIOD_OUT_LOW : GPIOD_OUT_HIGH;
+	spi->cs_gpiod = gpiochip_request_own_desc(chip, 8 + spi->chip_select,
+						  DRV_NAME,
+						  flags);
+	if (IS_ERR(spi->cs_gpiod))
+		return PTR_ERR(spi->cs_gpiod);
 
 	/* and set up the "mode" and level */
-	dev_info(&spi->dev, "setting up native-CS%i as GPIO %i\n",
-		 spi->chip_select, spi->cs_gpio);
-
-	/* set up GPIO as output and pull to the correct level */
-	err = gpio_direction_output(spi->cs_gpio,
-				    (spi->mode & SPI_CS_HIGH) ? 0 : 1);
-	if (err) {
-		dev_err(&spi->dev,
-			"could not set CS%i gpio %i as output: %i",
-			spi->chip_select, spi->cs_gpio, err);
-		return err;
-	}
+	dev_info(&spi->dev, "FIXME: setting up native-CS%i as GPIO\n",
+		 spi->chip_select);
 
 	return 0;
 }
@@ -939,6 +948,7 @@  static int bcm2835_spi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, master);
 
+	master->use_gpio_descriptors = true;
 	master->mode_bits = BCM2835_SPI_MODE_BITS;
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->num_chipselect = 3;