diff mbox series

gpio: of: Handle SPI chipselect legacy bindings

Message ID 20180904124950.17067-1-linus.walleij@linaro.org
State Superseded
Headers show
Series gpio: of: Handle SPI chipselect legacy bindings | expand

Commit Message

Linus Walleij Sept. 4, 2018, 12:49 p.m. UTC
The SPI chipselect are assumed to be active low in the current
binding, so when we want to use GPIO descriptors and handle
the active low/high semantics in gpiolib, we need a special
parsing quirk to deal with this.

We check for the property "cs-active-high" and if that is
NOT present we assume the CS line is active low.

If the line is tagged as active low in the device tree and
has no "cs-active-high" property all is fine, the device
tree and the SPI bindings are in agreement.

If the line is tagged as active high in the device tree with
the second cell flag and has no "cs-active-high" property we
enforce active low semantics (as this is the exception we can
just tag on the flag).

If the line is tagged as active low with the second cell flag
AND tagged with "cs-active-high" the SPI active high property
takes precedence and we print a warning.

Cc: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
This will be merged as a precursor to a series switching
over to using GPIO descriptors for chip select handling
in the SPI subsystem. Currently no descriptors are used
for chip selects so the impact will be zero until we start
switching drivers over.
---
 drivers/gpio/gpiolib-of.c | 47 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Geert Uytterhoeven Sept. 5, 2018, 7:30 a.m. UTC | #1
Hi Linus,

On Tue, Sep 4, 2018 at 2:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> The SPI chipselect are assumed to be active low in the current

> binding, so when we want to use GPIO descriptors and handle

> the active low/high semantics in gpiolib, we need a special

> parsing quirk to deal with this.

>

> We check for the property "cs-active-high" and if that is


There are no users of this property => "spi-cs-high" (everywhere!)?

> NOT present we assume the CS line is active low.

>

> If the line is tagged as active low in the device tree and

> has no "cs-active-high" property all is fine, the device

> tree and the SPI bindings are in agreement.


OK.

> If the line is tagged as active high in the device tree with

> the second cell flag and has no "cs-active-high" property we

> enforce active low semantics (as this is the exception we can

> just tag on the flag).


My initial feeling said "spi-cs-high" is meant for native chipselects, not
for GPIO chipselects.  The latter already has a standard way to specify
polarity (the second cell).
So this has the potential of breaking things, and deserves a warning.

However, on second thought, the "cs-gpios" property is a property of the
SPI controller, so it can be assumed to follow the SPI standard (active
low) convention.
While "spi-cs-high" is a property of the SPI device on the SPI bus, and
thus inverts the standard behavior.
Hence what if both tagged active high with the second cell flag, and with
"cs-active-high"?
Cfr. arch/arm/boot/dts/imx27-phytec-phycore-som.dtsi
arch/arm/boot/dts/imx51-babbage.dts
arch/arm/boot/dts/imx51-digi-connectcore-som.dtsi
arch/arm/boot/dts/imx51-zii-scu2-mezz.dts
arch/arm/boot/dts/imx6q-evi.dts
arch/arm/boot/dts/imx51-zii-rdu1.dts
arch/arm/boot/dts/vf610-zii-dev-rev-b.dts

The last two are even more interesting, as they use spi-gpio, so there
are 3 flags
impacting chip select polarity (gpio-sck cell 2, cs-gpios cell 2,
cs-active-high).

Looks like the current expectation is that all two (or three) should agree?
That complicates DT overlays adding an SPI device to the bus, as
"cs-active-high" is a property of the device added, while modifying
"cs-gpios" means touching the controller's property (which in the case of
DT overlays used for connectors may or may not be present, as the connector
is the abstraction layer, and the underlying SPI controller may use native
or GPIO chip selects).

> If the line is tagged as active low with the second cell flag

> AND tagged with "cs-active-high" the SPI active high property

> takes precedence and we print a warning.


OK, makes sense.

>

> Cc: Mark Brown <broonie@kernel.org>

> Cc: linux-spi@vger.kernel.org

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> This will be merged as a precursor to a series switching

> over to using GPIO descriptors for chip select handling

> in the SPI subsystem. Currently no descriptors are used

> for chip selects so the impact will be zero until we start

> switching drivers over.


drivers/spi/spi-pxa2xx.c and drivers/spi/spi-sh-msiof.c do use descriptors
for chip selects.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Mark Brown Sept. 5, 2018, 11:44 a.m. UTC | #2
On Wed, Sep 05, 2018 at 09:30:10AM +0200, Geert Uytterhoeven wrote:

> The last two are even more interesting, as they use spi-gpio, so there

> are 3 flags

> impacting chip select polarity (gpio-sck cell 2, cs-gpios cell 2,

> cs-active-high).

> 

> Looks like the current expectation is that all two (or three) should agree?


I think the current implementation is "intended" to come out as a
logical or - if anything sets the line active high then it's set active
high.  I've not checked properly if that's actually the case in practice
though.  It's not the most beautiful or well thought out arrangement :(
Linus Walleij Sept. 5, 2018, 12:06 p.m. UTC | #3
On Wed, Sep 5, 2018 at 1:44 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Sep 05, 2018 at 09:30:10AM +0200, Geert Uytterhoeven wrote:

>

> > The last two are even more interesting, as they use spi-gpio, so there

> > are 3 flags

> > impacting chip select polarity (gpio-sck cell 2, cs-gpios cell 2,

> > cs-active-high).

> >

> > Looks like the current expectation is that all two (or three) should agree?

>

> I think the current implementation is "intended" to come out as a

> logical or - if anything sets the line active high then it's set active

> high.  I've not checked properly if that's actually the case in practice

> though.  It's not the most beautiful or well thought out arrangement :(


It's an artifact of a bit of confusion and not knowing where to go
in the early days of device tree introduction, not much to do about.
I guess we didn't even agree that GPIO phandles should have flags
at the time.

The idea with this patch (like with the regulator patches) is to
contain the confusion in gpiolib-of.c and try to make the subsystems
just use gpiolib, so the API to gpiolib becomes "narrow and deep"
as the saying goes, taking semantic clutter away from the
subsystem.

Yours,
Linus Walleij
Mark Brown Sept. 5, 2018, 1:36 p.m. UTC | #4
On Wed, Sep 05, 2018 at 02:06:47PM +0200, Linus Walleij wrote:

> The idea with this patch (like with the regulator patches) is to

> contain the confusion in gpiolib-of.c and try to make the subsystems

> just use gpiolib, so the API to gpiolib becomes "narrow and deep"

> as the saying goes, taking semantic clutter away from the

> subsystem.


Yeah, I think it's a sensible approach.
Linus Walleij Sept. 6, 2018, 2:18 p.m. UTC | #5
On Wed, Sep 5, 2018 at 9:30 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Sep 4, 2018 at 2:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:


> > We check for the property "cs-active-high" and if that is

>

> There are no users of this property => "spi-cs-high" (everywhere!)?


Sorry for not drinking coffee before patching :/

> > If the line is tagged as active high in the device tree with

> > the second cell flag and has no "cs-active-high" property we

> > enforce active low semantics (as this is the exception we can

> > just tag on the flag).

>

> My initial feeling said "spi-cs-high" is meant for native chipselects, not

> for GPIO chipselects.


I will try to not touch the native chipselects right now, I see that
as a SPI problem not a GPIO problem. Those indeed assume
active low as the default as well.

> The latter already has a standard way to specify

> polarity (the second cell).


Exactly and that is what I want to get to.

AFAICT what all drivers are doing
(including the two that actually use descriptors for cs) is get the
GPIO, assume it is set up as active high (i.e. no flag) then look at
spi->mode & SPI_CS_HIGH to determine if the line is instead
active high and in that case explicitly drive it high inside the SPI
master driver.

So I want to pull this into the gpiolib core and get rid of this
complexity from the SPI masters.

> So this has the potential of breaking things, and deserves a warning.


Hm I don't know about that, assuming it is active low and only
make it high on demand is what all drivers seem to be doing
right now, so there will be some warnings for things that was
business as usual before.

But indeed it is nicer if the flags agree. What about dev_info()?

> Hence what if both tagged active high with the second cell flag, and with

> "cs-active-high"?

> Cfr. arch/arm/boot/dts/imx27-phytec-phycore-som.dtsi

> arch/arm/boot/dts/imx51-babbage.dts

> arch/arm/boot/dts/imx51-digi-connectcore-som.dtsi

> arch/arm/boot/dts/imx51-zii-scu2-mezz.dts

> arch/arm/boot/dts/imx6q-evi.dts

> arch/arm/boot/dts/imx51-zii-rdu1.dts

> arch/arm/boot/dts/vf610-zii-dev-rev-b.dts


Active high is fine, because what all drivers do as I state
above is simply assume that no matter where the GPIO came
from, whether device tree or board file.

Then the SPI core code inverse the polarity internally.

So I want to move that semantic over to gpiolib.

> The last two are even more interesting, as they use spi-gpio, so there

> are 3 flags

> impacting chip select polarity (gpio-sck cell 2, cs-gpios cell 2,

> cs-active-high).


Currently I am dealing with the two latter, cs-gpios and
cs-active-high.

Maybe I should look into the clock as well. Probably possible
to deal with in separate patches.

> Looks like the current expectation is that all two (or three) should agree?


I think they all assume that the GPIO line is tagged as active
high and all polarity inversion lives inside the SPI core and
drivers.

> That complicates DT overlays adding an SPI device to the bus, as

> "cs-active-high" is a property of the device added, while modifying

> "cs-gpios" means touching the controller's property (which in the case of

> DT overlays used for connectors may or may not be present, as the connector

> is the abstraction layer, and the underlying SPI controller may use native

> or GPIO chip selects).


The more I look at device tree overlays the less I believe in
it. But I'm a dissident. I don't want to distance myself from
the people driving the effort so I try to be diplomatic and see
if I can get it working anyways. The concept seems simple,
the devil is in the details.

I guess I work with the systems we have and try to make
reasonable API work so that these things can get a reasonable
encapsulation.

Yours,
Linus Walleij
Linus Walleij Sept. 6, 2018, 2:23 p.m. UTC | #6
On Thu, Sep 6, 2018 at 4:18 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 5, 2018 at 9:30 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Tue, Sep 4, 2018 at 2:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:


> > The last two are even more interesting, as they use spi-gpio, so there

> > are 3 flags

> > impacting chip select polarity (gpio-sck cell 2, cs-gpios cell 2,

> > cs-active-high).

>

> Currently I am dealing with the two latter, cs-gpios and

> cs-active-high.

>

> Maybe I should look into the clock as well. Probably possible

> to deal with in separate patches.


Oh, haha I already fixed gpio-sck, see:
commit c85823390215e52d68d3826df92a447ed31e5c80
"gpio: of: Support SPI nonstandard GPIO properties"

commit 9b00bc7b901ff672a9252002d3810fdf9489bc64
"spi: spi-gpio: Rewrite to use GPIO descriptors"

Yours,
Linus Walleij
Geert Uytterhoeven Sept. 6, 2018, 3:03 p.m. UTC | #7
Hi Linus,

On Thu, Sep 6, 2018 at 4:18 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 5, 2018 at 9:30 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Tue, Sep 4, 2018 at 2:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:


> > The last two are even more interesting, as they use spi-gpio, so there

> > are 3 flags

> > impacting chip select polarity (gpio-sck cell 2, cs-gpios cell 2,

> > cs-active-high).

>

> Currently I am dealing with the two latter, cs-gpios and

> cs-active-high.


This time it's me who lacked coffee ;-)

Scrap that, of course gpio-sck is not related to chip select polarity,
so there are only 2, not 3, things to agree.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index a4f1157d6aa0..cec8351909ce 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -57,7 +57,8 @@  static struct gpio_desc *of_xlate_and_get_gpiod_flags(struct gpio_chip *chip,
 }
 
 static void of_gpio_flags_quirks(struct device_node *np,
-				 enum of_gpio_flags *flags)
+				 enum of_gpio_flags *flags,
+				 int index)
 {
 	/*
 	 * Some GPIO fixed regulator quirks.
@@ -91,6 +92,48 @@  static void of_gpio_flags_quirks(struct device_node *np,
 		pr_info("%s uses legacy open drain flag - update the DTS if you can\n",
 			of_node_full_name(np));
 	}
+
+	/*
+	 * Legacy handling of SPI active high chip select. If we have a
+	 * property named "cs-gpios" we need to inspect the child node
+	 * to determine if the flags should have inverted semantics.
+	 */
+	if (IS_ENABLED(CONFIG_SPI_MASTER) &&
+	    of_property_read_bool(np, "cs-gpios")) {
+		struct device_node *child;
+		u32 cs;
+		int ret;
+
+		for_each_child_of_node(np, child) {
+			ret = of_property_read_u32(child, "reg", &cs);
+			if (!ret)
+				continue;
+			if (cs == index) {
+				/*
+				 * SPI children have active low chip selects
+				 * by default. This can be specified negatively
+				 * by just omitting "spi-cs-high" in the
+				 * device node, or actively by tagging on
+				 * GPIO_ACTIVE_LOW as flag in the device
+				 * tree. If the line is simultaneously
+				 * tagged as active low in the device tree
+				 * and has the "spi-cs-high" set, we get a
+				 * conflict and the "spi-cs-high" flag will
+				 * take precedence.
+				 */
+				if (of_property_read_bool(np, "spi-cs-high")) {
+					if (*flags & OF_GPIO_ACTIVE_LOW) {
+						pr_warn("%s GPIO handle specifies active low - ignored\n",
+							of_node_full_name(np));
+						*flags &= ~OF_GPIO_ACTIVE_LOW;
+					}
+				} else {
+					*flags |= OF_GPIO_ACTIVE_LOW;
+				}
+				break;
+			}
+		}
+	}
 }
 
 /**
@@ -131,7 +174,7 @@  struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		goto out;
 
 	if (flags)
-		of_gpio_flags_quirks(np, flags);
+		of_gpio_flags_quirks(np, flags, index);
 
 	pr_debug("%s: parsed '%s' property of node '%pOF[%d]' - status (%d)\n",
 		 __func__, propname, np, index,