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

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

Commit Message

Linus Walleij April 20, 2019, 11:08 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>
---
ChangeLog RFT->v2:
- Rebased on v5.1-rc1

I would very much appreciate if someone took this for
a ride on top of linux-next (there are some fixes in
the -rcs you need) and see if all still works as expected.
---
 drivers/spi/spi-bcm2835.c | 42 ++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 16 deletions(-)

Comments

Martin Sperl April 20, 2019, 3:46 p.m. UTC | #1
> On 20.04.2019, at 13:08, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> 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.

It has been done to keep dt-backward compatibility.

When the driver was written dt had configured native-cs.

A native-cs implementation comes with lots of
limitations and some of which - especially dma mode
has some quirks with regards to native-cs (glitches,
deasserts after each finished dma transfer,...)

The only way to make this work properly was to use
Gpio.

So to properly support old device-trees with native-cs 
this gpio translation is needed, as we should not break
things for old dt...

Maybe this rational helps...

Martin
Stefan Wahren April 21, 2019, 12:27 p.m. UTC | #2
Am 20.04.19 um 17:46 schrieb Martin Sperl:
>
>> On 20.04.2019, at 13:08, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> 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.
> It has been done to keep dt-backward compatibility.
>
> When the driver was written dt had configured native-cs.

This is done in a very opaque way. The bcm2835-rpi.dtsi defines a pinmux
group alt0 which init SPI pins (including native CS). So we better use
spi0_gpio7 (including native CS) or even better create new groups
without native CS and use them.

Even worse the alt0 group is applied for all the RPi Zero, 1 and 2
boards, but not for the RPi 3 ones.

> A native-cs implementation comes with lots of
> limitations and some of which - especially dma mode
> has some quirks with regards to native-cs (glitches,
> deasserts after each finished dma transfer,...)
>
> The only way to make this work properly was to use
> Gpio.

So we better extend the current binding with cs-gpios as optional
property in order to point the user the right direction. And gave the
same warnings like in bcm2835-spi-aux.

Stefan

>
> So to properly support old device-trees with native-cs 
> this gpio translation is needed, as we should not break
> things for old dt...
>
> Maybe this rational helps...
>
> Martin
>
Martin Sperl April 21, 2019, 5:17 p.m. UTC | #3
> On 21.04.2019, at 14:27, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
>> Am 20.04.19 um 17:46 schrieb Martin Sperl:
>> 
>>> On 20.04.2019, at 13:08, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> 
>>> 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.
>> It has been done to keep dt-backward compatibility.
>> 
>> When the driver was written dt had configured native-cs.
> 
> This is done in a very opaque way. The bcm2835-rpi.dtsi defines a pinmux
> group alt0 which init SPI pins (including native CS). So we better use
> spi0_gpio7 (including native CS) or even better create new groups
> without native CS and use them.

I remember that there was a patch of mine to achieve this but it was
turned down because it would not describe the the hw and the altx 
Pin up still needed to get at least documented that way. And that
when enabling the spi Block the corresponding  gpio Block would 
need to get configured properly.

At that point I left the discussion as the effect of using gpio mode
was implemented by the driver. I can search for the patch
 from a few years ago...

> Even worse the alt0 group is applied for all the RPi Zero, 1 and 2
> boards, but not for the RPi 3 ones.
> 
>> A native-cs implementation comes with lots of
>> limitations and some of which - especially dma mode
>> has some quirks with regards to native-cs (glitches,
>> deasserts after each finished dma transfer,...)
>> 
>> The only way to make this work properly was to use
>> Gpio.
> 
> So we better extend the current binding with cs-gpios as optional
> property in order to point the user the right direction. And gave the
> same warnings like in bcm2835-spi-aux.

The difference is that aux has different issues and this driver
Is not working properly without gpio cs giving unexpected results
 and resulting in regressions.

 Keeping gpio cs being mandatory is the only way to avoid
regressions everything else is just relying on the legacy code
and thus avoiding the discussion.

Martin
Linus Walleij April 21, 2019, 11:08 p.m. UTC | #4
On Sun, Apr 21, 2019 at 7:17 PM Martin Sperl <kernel@martin.sperl.org> wrote:
> > On 21.04.2019, at 14:27, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> >
> >> Am 20.04.19 um 17:46 schrieb Martin Sperl:
> >>
> >>> On 20.04.2019, at 13:08, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>
> >>> 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.
> >> It has been done to keep dt-backward compatibility.
> >>
> >> When the driver was written dt had configured native-cs.
> >
> > This is done in a very opaque way. The bcm2835-rpi.dtsi defines a pinmux
> > group alt0 which init SPI pins (including native CS). So we better use
> > spi0_gpio7 (including native CS) or even better create new groups
> > without native CS and use them.
>
> I remember that there was a patch of mine to achieve this but it was
> turned down because it would not describe the the hw and the altx
> Pin up still needed to get at least documented that way. And that
> when enabling the spi Block the corresponding  gpio Block would
> need to get configured properly.
>
> At that point I left the discussion as the effect of using gpio mode
> was implemented by the driver. I can search for the patch
>  from a few years ago...
>
> > Even worse the alt0 group is applied for all the RPi Zero, 1 and 2
> > boards, but not for the RPi 3 ones.
> >
> >> A native-cs implementation comes with lots of
> >> limitations and some of which - especially dma mode
> >> has some quirks with regards to native-cs (glitches,
> >> deasserts after each finished dma transfer,...)
> >>
> >> The only way to make this work properly was to use
> >> Gpio.
> >
> > So we better extend the current binding with cs-gpios as optional
> > property in order to point the user the right direction. And gave the
> > same warnings like in bcm2835-spi-aux.
>
> The difference is that aux has different issues and this driver
> Is not working properly without gpio cs giving unexpected results
>  and resulting in regressions.
>
>  Keeping gpio cs being mandatory is the only way to avoid
> regressions everything else is just relying on the legacy code
> and thus avoiding the discussion.

While all this is great and I am sorry if things are not what they
should ideally be, the patch as it stands actually doesn't change
the hack, combined with patch 1 it tries to preserve it and put
us in a better position for more cleanups later.

It would be great if someone could test patches 1+2 on
hardware and see if it works as intended!

Yours,
Linus Walleij
Martin Sperl April 22, 2019, 5:30 p.m. UTC | #5
Hi Linus!


> On 20.04.2019, at 13:08, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> 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>
> ---
> ChangeLog RFT->v2:
> - Rebased on v5.1-rc1
> 
> I would very much appreciate if someone took this for
> a ride on top of linux-next (there are some fixes in
> the -rcs you need) and see if all still works as expected.


Just applying this single patch gives me:
  CC [M]  drivers/spi/spi-bcm2835.o
drivers/spi/spi-bcm2835.c: In function ‘bcm2835_spi_setup’:
drivers/spi/spi-bcm2835.c:883:6: warning: unused variable ‘err’ [-Wunused-variable]
  int err;
      ^~~

Then loading the module:
[   30.403459] spi spi0.0: FIXME: setting up native-CS0 as GPIO

But when loading a spi-device driver it does not detect the device.

One note: the logic analyzer shows that the CS line is inverted
(high on SPI Clock running)

With the un-patched version I get the following in dmesg:
root@raspcm3:~# dmesg
[   27.987287] spi spi0.0: setting up native-CS0 as GPIO 2002

So you see that I am using the “original” DT that relies on the
translation to gpio by the driver.

Martin
Linus Walleij April 23, 2019, 8:43 a.m. UTC | #6
On Mon, Apr 22, 2019 at 7:30 PM <kernel@martin.sperl.org> wrote:

> Just applying this single patch gives me:
>   CC [M]  drivers/spi/spi-bcm2835.o
> drivers/spi/spi-bcm2835.c: In function ‘bcm2835_spi_setup’:
> drivers/spi/spi-bcm2835.c:883:6: warning: unused variable ‘err’ [-Wunused-variable]
>   int err;
>       ^~~

OK I'll fix this!

> Then loading the module:
> [   30.403459] spi spi0.0: FIXME: setting up native-CS0 as GPIO
>
> But when loading a spi-device driver it does not detect the device.
>
> One note: the logic analyzer shows that the CS line is inverted
> (high on SPI Clock running)

What did you apply the patch on?

Using v5.1-rc1 will not work, you need a later
RC because of other mistakes of mine :/
I would try just using mainline (Torvalds HEAD).

(I removed the GPIO line number print because we should
not rely on line numbers.)

> With the un-patched version I get the following in dmesg:
> root@raspcm3:~# dmesg
> [   27.987287] spi spi0.0: setting up native-CS0 as GPIO 2002
>
> So you see that I am using the “original” DT that relies on the
> translation to gpio by the driver.

Good this is what we need to test.

I suspect that it is the CS inversion that is the problem as
you point out.

Yours,
Linus Walleij
Martin Sperl April 23, 2019, 4:28 p.m. UTC | #7
> On 23.04.2019, at 10:43, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> 
>> Then loading the module:
>> [   30.403459] spi spi0.0: FIXME: setting up native-CS0 as GPIO
>> 
>> But when loading a spi-device driver it does not detect the device.
>> 
>> One note: the logic analyzer shows that the CS line is inverted
>> (high on SPI Clock running)
> 
> What did you apply the patch on?
> 
> Using v5.1-rc1 will not work, you need a later
> RC because of other mistakes of mine :/
> I would try just using mainline (Torvalds HEAD).
> 

I just tried 5.1.0-rc6.

But it still shows the same behavior with inverted CS.

After reboot before loading the driver GPIO is High and in mode Alt0.
After module load the GPIO is now output but switches level to LOW.

Martin

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;