| Message ID | 20180904124950.17067-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: of: Handle SPI chipselect legacy bindings | expand |
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
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 :(
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
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.
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
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
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 --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,
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