[PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Transforms the bcm-2835 native SPI-chip select to their gpio-cs equivalent.

This allows for some support of some optimizations that are not
possible due to HW-gliches on the CS line - especially filling
the FIFO before enabling SPI interrupts (by writing to CS register)
while the transfer is already in progress (See commit: e3a2be3030e2)

This patch also works arround some issues in bcm2835-pinctrl which does not
set the value when setting the GPIO as output - it just sets up output and
(typically) leaves the GPIO as low.  When a fix for this is merged then this
gpio_set_value can get removed from bcm2835_spi_setup.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |   49 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

Applies against: spi - for-next

There was a question if we could move the native-cs to GPIO in the driver
itself.

This patch implements this, but it is quite complex for the minimal gains.

Maybe the originally proposed simple warning recommending to use cs_gpio
in DT would be a sufficient alternative that introduces less code.

Tested with the following setup:
* native CS:
  * mcp2515
  * mmc_spi (patched to make it work on the RPI)
* gpio-CS:
  * mcp2515
  * enc28j60
* gpio-CS with "spi-cs-pol" set in DT:
  * fb_st7735r

Note: that there is a possible the risk that a transfer (for a different
device) is running at the time of this change - maybe some "extra" bus-locking
is needed during the change of the GPIO function to output.

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 88b808b..b2651fa 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -351,8 +351,15 @@ static void bcm2835_spi_set_cs(struct spi_device *spi, bool gpio_level)
 	bcm2835_wr(bs, BCM2835_SPI_CS, cs);
 }

+static int chip_match_name(struct gpio_chip *chip, void *data)
+{
+	return !strcmp(chip->label, data);
+}
+
 static int bcm2835_spi_setup(struct spi_device *spi)
 {
+	int err;
+	struct gpio_chip *chip;
 	/*
 	 * sanity checking the native-chipselects
 	 */
@@ -360,13 +367,45 @@ static int bcm2835_spi_setup(struct spi_device *spi)
 		return 0;
 	if (gpio_is_valid(spi->cs_gpio))
 		return 0;
-	if (spi->chip_select < 3)
+	if (spi->chip_select > 1) {
+		/* error in the case of native CS requested with CS > 1
+		 * officially there is a CS2, but it is not documented
+		 * which GPIO is connected with that...
+		 */
+		dev_err(&spi->dev,
+			"setup: only two native chip-selects are supported\n");
+		return -EINVAL;
+	}
+	/* now translate native cs to GPIO */
+
+	/* get the gpio chip for the base */
+	chip = gpiochip_find("pinctrl-bcm2835", chip_match_name);
+	if (!chip)
 		return 0;

-	/* error in the case of native CS requested with CS-id > 2 */
-	dev_err(&spi->dev,
-		"setup: only three native chip-selects are supported\n");
-	return -EINVAL;
+	/* and calculate the real CS */
+	spi->cs_gpio = chip->base + 8 - spi->chip_select;
+
+	/* 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;
+	}
+	/* the implementation of pinctrl-bcm2835 currently does not
+	 * set the GPIO value when using gpio_direction_output
+	 * so we are setting it here explicitly
+	 */
+	gpio_set_value(spi->cs_gpio, (spi->mode & SPI_CS_HIGH) ? 0 : 1);
+
+	return 0;
 }

 static int bcm2835_spi_probe(struct platform_device *pdev)
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

On Mon, Apr 06, 2015 at 05:16:31PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:

> There was a question if we could move the native-cs to GPIO in the driver
> itself.

> This patch implements this, but it is quite complex for the minimal gains.

> Maybe the originally proposed simple warning recommending to use cs_gpio
> in DT would be a sufficient alternative that introduces less code.

I'm not seeing any particular complexity here.  Can you be more explicit
please?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Martin Sperl]

> On 06.04.2015, at 19:29, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> I'm not seeing any particular complexity here.  Can you be more explicit
> please?

I just see the high-possibility of bitrot for something that is IMO better
moved to a modified device-tree config.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Mon, Apr 06, 2015 at 07:35:37PM +0200, Martin Sperl wrote:
> > On 06.04.2015, at 19:29, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > I'm not seeing any particular complexity here.  Can you be more explicit
> > please?

> I just see the high-possibility of bitrot for something that is IMO better
> moved to a modified device-tree config.

Requiring every system to update the DT to specify information that we
can already get easily (and using a less readable format in that update)
doesn't seem like a clear win for me...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Martin Sperl]

> On 06.04.2015, at 19:45, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> Requiring every system to update the DT to specify information that we
> can already get easily (and using a less readable format in that update)
> doesn't seem like a clear win for me...

If you prefer that, then apply it - i just wanted to offer both options!

One note though: as most systems will get "deploying" the rpi-foundation
pre-build kernels and their corresponding device tree it seems unlikely 
that a lot of alternate dts will get used...

So when they switch, then >99% of all users will be using the new code
plus the new device-tree that handles that.

That is why I see this as a good example of future bit-rot, where we
have to maintain 20+ lines versus about 5 lines with just a message...


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On Mon, Apr 06, 2015 at 07:58:15PM +0200, Martin Sperl wrote:

> One note though: as most systems will get "deploying" the rpi-foundation
> pre-build kernels and their corresponding device tree it seems unlikely 
> that a lot of alternate dts will get used...

> So when they switch, then >99% of all users will be using the new code
> plus the new device-tree that handles that.

People adding things to the standard kernel (presumably people push
their board support in there) would still have to work out which chip
select corresponds to which GPIO and write that mapping into their DT
with the corresponding loss of legibility there, and of course there's
the cost of modifying existing users.

It *would* be nice if the mapping to which pin has which function were
pushed into the pinctrl driver (and then possibly into the DT for it)
but generally this seems like something we should be doing more of not
less of.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Martin Sperl]

On 2015-04-06 20:10, Mark Brown wrote:
> People adding things to the standard kernel (presumably people push
> their board support in there) would still have to work out which
> chip select corresponds to which GPIO and write that mapping into
> their DT with the corresponding loss of legibility there, and of
> course there's the cost of modifying existing users.
>
> It *would* be nice if the mapping to which pin has which function
> were pushed into the pinctrl driver (and then possibly into the DT
> for it) but generally this seems like something we should be doing
> more of not less of.
>
The foundation gets to a point of using device-tree overlays to minimize
this kind of manual changes - the default still would include CS0=GPIO8
and CS1=GPIO7. If someone needs more CS, then the extra CS needs to get
configured in an overlay overriding those pins - so not a huge
difference

If we add "cs_gpios = <&gpio 8 0> <&gpio 7 0>" to the dtsi or assume it
is native, where this code essentially does what is needed to translate
it to the above.

Just tell me if we need spi_bus_lock on the "change" path to avoid
unpleasant behavior and I will create a patch incorporating that.
Spi_setup can sleep, so we can wait for the bus to become idle before
such a change...

Alternatively we could also add something like:
     int initial_chip_select_setup(int cs, int cspol);
as a method in spi_master that the framework would call prior to
registering all spi_devices with spi_device_register.

In the code for the spi-bcm2835 we can run the native-CS to GPIO
translation.

I could create a patch that handles this.

If you like it as is, merge it.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Stephen Warren]

On 04/06/2015 11:58 AM, Martin Sperl wrote:
> 
>> On 06.04.2015, at 19:45, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>
>> Requiring every system to update the DT to specify information that we
>> can already get easily (and using a less readable format in that update)
>> doesn't seem like a clear win for me...
> 
> If you prefer that, then apply it - i just wanted to offer both options!
> 
> One note though: as most systems will get "deploying" the rpi-foundation
> pre-build kernels and their corresponding device tree it seems unlikely 
> that a lot of alternate dts will get used...
> 
> So when they switch, then >99% of all users will be using the new code
> plus the new device-tree that handles that.

I wouldn't be at all surprised if many people maintained their own DT in
order to add in support for whatever extra peripherals they've connected
to the expansion header. The correct way to do this is of course to add
a patch on top of the kernel tree, and rebase, rebuild, and reinstall
the DTB whenever the kernel gets upgraded. However, the likelihood of
/everyone/ doing that seems low; I'd certainly expect to see some people
simply not upgrading their DTB.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Stephen Warren]

On 04/06/2015 11:16 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Transforms the bcm-2835 native SPI-chip select to their gpio-cs equivalent.
> 
> This allows for some support of some optimizations that are not
> possible due to HW-gliches on the CS line - especially filling
> the FIFO before enabling SPI interrupts (by writing to CS register)
> while the transfer is already in progress (See commit: e3a2be3030e2)
> 
> This patch also works arround some issues in bcm2835-pinctrl which does not
> set the value when setting the GPIO as output - it just sets up output and
> (typically) leaves the GPIO as low.  When a fix for this is merged then this
> gpio_set_value can get removed from bcm2835_spi_setup.

> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c

> @@ -360,13 +367,45 @@ static int bcm2835_spi_setup(struct spi_device *spi)
>  		return 0;
>  	if (gpio_is_valid(spi->cs_gpio))
>  		return 0;
> -	if (spi->chip_select < 3)
> +	if (spi->chip_select > 1) {
> +		/* error in the case of native CS requested with CS > 1
> +		 * officially there is a CS2, but it is not documented
> +		 * which GPIO is connected with that...
> +		 */
> +		dev_err(&spi->dev,
> +			"setup: only two native chip-selects are supported\n");

I believe the bcm283x have 2 types of SPI controller. There is 1
instance of the HW that this driver controls (SPI0), and that has CS0,
CS1. There are two instances of a different SPI HW block (SPI1, SPI2),
and those each have CS0, CS1, CS2. At least, that's my interpretation of
the table that shows the pinctrl module's per-pin alternate function values.

> +		return -EINVAL;
> +	}
> +	/* now translate native cs to GPIO */
> +
> +	/* get the gpio chip for the base */
> +	chip = gpiochip_find("pinctrl-bcm2835", chip_match_name);
> +	if (!chip)
>  		return 0;

Should that be an error? Not being able to find the gpiochip implies the
code can't manipulate the CS lines at all, I think?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Martin Sperl]

> On 07.04.2015, at 05:01, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> I believe the bcm283x have 2 types of SPI controller. There is 1
> instance of the HW that this driver controls (SPI0), and that has CS0,
> CS1. There are two instances of a different SPI HW block (SPI1, SPI2),
> and those each have CS0, CS1, CS2. At least, that's my interpretation of
> the table that shows the pinctrl module's per-pin alternate function values.

Yes and no - SPI1 and SPI2 are a totally different beasts as these are 
"auxiliar devices" that have 2x2 word fifos, no DMA and minimal interrupt
support and use a distinct register layout compared to SPI0. 
See BCM2835 Arm Peripherials page 20-27 for details.

Essentially these are intended to get used for low speed devices or devices
that run short transfers (<4-8 bytes)

So these devices would need a totally separate SPI driver, which this driver
does not and can not handle...

> Should that be an error? Not being able to find the gpiochip implies the
> code can't manipulate the CS lines at all, I think?
Will add an dev_warn_once and contine without optimizations - the code 
still does check for the cs_gpio, so we would run without it? 
It could just be a rename of the pinctrl driver that would trigger this.

You want it as an error?

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Stephen Warren]

On 04/06/2015 11:45 PM, Martin Sperl wrote:
>
>> On 07.04.2015, at 05:01, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> I believe the bcm283x have 2 types of SPI controller. There is 1
>> instance of the HW that this driver controls (SPI0), and that has CS0,
>> CS1. There are two instances of a different SPI HW block (SPI1, SPI2),
>> and those each have CS0, CS1, CS2. At least, that's my interpretation of
>> the table that shows the pinctrl module's per-pin alternate function values.
>
> Yes and no - SPI1 and SPI2 are a totally different beasts as these are
> "auxiliar devices" that have 2x2 word fifos, no DMA and minimal interrupt
> support and use a distinct register layout compared to SPI0.
> See BCM2835 Arm Peripherials page 20-27 for details.
>
> Essentially these are intended to get used for low speed devices or devices
> that run short transfers (<4-8 bytes)
>
> So these devices would need a totally separate SPI driver, which this driver
> does not and can not handle...

That's exactly what I said.

>> Should that be an error? Not being able to find the gpiochip implies the
>> code can't manipulate the CS lines at all, I think?
 >
> Will add an dev_warn_once and contine without optimizations - the code
> still does check for the cs_gpio, so we would run without it?
> It could just be a rename of the pinctrl driver that would trigger this.
>
> You want it as an error?

If the rest of the driver is expected to always check whether 
spi->cs_gpio is valid or not, and support the gpio-is-not-valid case, I 
guess it's OK for that case *not* to error out.

I think we should at least warn to indicate that something unexpected 
happened.

Shouldn't spi->cs_gpio be set to e.g. -1 if the pinctrl device can't be 
found, so that the rest of the driver does know that the GPIO is invalid?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

On Mon, Apr 06, 2015 at 05:16:31PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Transforms the bcm-2835 native SPI-chip select to their gpio-cs equivalent.

This doesn't apply against current code, please check and resend.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: bcm2835: transform native-cs to gpio-cs on first spi_setup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]

On Mon, Apr 06, 2015 at 05:16:31PM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Transforms the bcm-2835 native SPI-chip select to their gpio-cs equivalent.

Sorry, acutally it does - applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]