SPI: control CS via standard GPIO operations instead of SPI-HW

From: Martin Sperl <[email protected]>

Change the device tree to use cs-gpios for the spi bus master and standard gpio operation instead of relying on the HW with just 2 chip_selects using ALT0.

This reassigns the existing CS pins 7(=CS1) and 8(=CS0) as output instead of ALT0 (=SPI HW block controlled) and adds them in the list of cs-gpios for the spi-bus.

These pins aren't used by anything on the board, but are rather part of the expansion header. I wonder if we wouldn't be better off removing any configuration of the pins from the DT. After all, we can't guarantee how the user has connected them. The "default" usage, a/k/a the expansion header signal naming, isn't any guarantee.

Rather, the user should specify what they want to use the pin as; as a GPIO input, GPIO output, or an SPI chip-select.

While the existing DT already has this issue, note that this forces these pins to be driven as outputs. What if the user has hooked up an external device that drives these signals, and wants to use the pins as GPIO inputs?

This shouldn't be in the SoC .dtsi file. It's quite possible for someone to use other GPIOs as SPI CS. It's board or even use-case specific whether those are the correct values.

I would argue that we should not put any cs-gpios into any in-kernel DT file, since there's no on-board usage of SPI on the RPi boards.

For SPI to be useful, the user has to add a DT node to represent the SPI device itself anyway, so adding some properties to the controller to define which GPIOs to use for SPI CS can be done then too.

From what I have seen (and I am not an expert) is that with the

Quoted 10 lines · Stephen Warren

These pins aren't used by anything on the board, but are rather part of the expansion header. I wonder if we wouldn't be better off removing any configuration of the pins from the DT. After all, we can't guarantee how the user has connected them. The "default" usage, a/k/a the expansion header signal naming, isn't any guarantee.

Rather, the user should specify what they want to use the pin as; as a GPIO input, GPIO output, or an SPI chip-select.

...

Quoted 4 lines

While the existing DT already has this issue, note that this forces these pins to be driven as outputs. What if the user has hooked up an external device that drives these signals, and wants to use the pins as GPIO inputs?

Actually if you look at the Documentation on page 102 you will find that those pins are pulled high by default (if it is HW or firmware I am not sure) - so a hat designer needs to take this into consideration anyway. The only difference is that it is pulled high and not driven high/low.

Also with the "new" models there is the Firmware that will read those "hat-descriptors" from the eeprom and configure the GPIO "modes" and pullups based on this information.

But then it means in principle that this is a more general issue that just became apparent now.

Quoted 6 lines

This shouldn't be in the SoC .dtsi file. It's quite possible for someone to use other GPIOs as SPI CS. It's board or even use-case specific whether those are the correct values.

I would argue that we should not put any cs-gpios into any in-kernel DT file, since there's no on-board usage of SPI on the RPi boards.

but then: why not just make it optional and NOT configuring it at all and keep it "outside".

Quoted 3 lines

For SPI to be useful, the user has to add a DT node to represent the SPI device itself anyway, so adding some properties to the controller to define which GPIOs to use for SPI CS can be done then too.

From what I have seen (and I am not an expert) is that with the foundation device trees each "device" (spi/i2c/...) has a separate Gpio section, which gets referenced inside the spi/i2c/... block.

As far as I understood with this setup the GPIOs ALT only gets set up when the driver itself loads (probably while parsing the DT for the device)

So this is maybe the way forward for the whole default-dt?

For SPI it would look like this:

&gpio {
        spi0_pins: spi0_pins {
                brcm,pins = <7 8 9 10 11>;
                brcm,function = <4>; /* alt0 */
        };
	...
}

&spi0 {
	...
        pinctrl-0 = <&spi0_pins>;
	...
}

And if you keep spi0 disabled in the dtsi files then the ALT modes should not be set.

Obviously we could also split the gpio-block into "normal SPI" and "CS" pins, which would allow changing the "defaults" also in the dts that gets build.

So how should we proceed?

Quoted 2 lines

But then it means in principle that this is a more general issue that just became apparent now.

Yes.

Quoted 11 lines

So this is maybe the way forward for the whole default-dt?

For SPI it would look like this:

&gpio {
       spi0_pins: spi0_pins {
               brcm,pins = <7 8 9 10 11>;
               brcm,function = <4>; /* alt0 */
       };
	...
}

Quoted 8 lines

&spi0 {
	...
       pinctrl-0 = <&spi0_pins>;
	...
}

And if you keep spi0 disabled in the dtsi files then the ALT modes should not be set.

Yes, so long as it's disabled by default that would be OK. However, I wonder why we don't just rely on the firmware to set up the pinmux, since as you mentioned it does it now?

Quoted 5 lines

Obviously we could also split the gpio-block into "normal SPI" and "CS" pins, which would allow changing the "defaults" also in the dts that gets build.

So how should we proceed?

If we do put any default CS GPIO setup in the kernel DT, we should indeed put it into a separate node (pinctrl state) so that the user can override it easily without any interactions with any other pins/...

From: Martin Sperl <[email protected]>

Change the device tree to use cs-gpios for the spi bus master and standard gpio operation instead of relying on the HW with just 2 chip_selects using ALT0.

This reassigns the existing CS pins 7(=CS1) and 8(=CS0) as output instead of ALT0 (=SPI HW block controlled) and adds them in the list of cs-gpios for the spi-bus.

Signed-off-by Martin Sperl <[email protected]>

---
 arch/arm/boot/dts/bcm2835-rpi.dtsi |    4 ++--
 arch/arm/boot/dts/bcm2835.dtsi     |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

arch/arm/boot/dts/bcm2835-rpi.dtsi +0 -0

index c706448..f0e36ab 100644
@@ -20,12 +20,12 @@
 	pinctrl-names = "default";
 
 	gpioout: gpioout {

From: Martin Sperl <[email protected]>

Allow the cs-gpios property in DT to be used instead of the fixed two chip-selects provided by the SPI-HW itself

Signed-off-by Martin Sperl <[email protected]>

---

There is the question if we still need to support the chip_selects provided by the hardware (plus the buggy CSPOL_HIGH support for those cases) or if we could just make the cs-gpios a required setting for this driver. Going with the GPIO only solution would clean up the code a bit.

 drivers/spi/spi-bcm2835.c |   53 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 7 deletions(-)

drivers/spi/spi-bcm2835.c +2 -0

index 419a782..128a152 100644
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2012 Chris Boot
  * Copyright (C) 2013 Stephen Warren
+ * Copyright (C) 2015 Martin Sperl
  *
  * This driver is inspired by:
  * spi-ath79.c, Copyright (C) 2009-2011 Gabor Juhos <[email protected]>

@@ -30,6 +31,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/spi/spi.h>
 
 /* SPI register offsets */

@@ -116,6 +118,18 @@ static inline void bcm2835_wr_fifo(struct bcm2835_spi *bs, int len)
 	}

You might want to use gpio_is_valid() instead of open coding it.

From: Martin Sperl <[email protected]>

Allow the cs-gpios property in DT to be used instead of the fixed two chip-selects provided by the SPI-HW itself

Signed-off-by Martin Sperl <[email protected]>

---

There is the question if we still need to support the chip_selects provided by the hardware (plus the buggy CSPOL_HIGH support for those cases) or if we could just make the cs-gpios a required setting for this driver. Going with the GPIO only solution would clean up the code a bit.

drivers/spi/spi-bcm2835.c +0 -0

@@ -205,12 +219,24 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
 		cs |= BCM2835_SPI_CS_CPHA;

drivers/spi/spi-bcm2835.c +6 -0

+			 * note that there is a bug in this when there are
+			 * multiple devices on the bus with at least one
+			 * having SPI_CS_HIGH set (the other CS_CSPOLX get
+			 * reset to 0 when any other device
+			 * starts a transfer
+			 */

BTW, you forgot to Cc the SPI maintainer, so he probably won't see your patch and apply it.