[BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[H. Nikolaus Schaller]

Hi Linus,
I am sorry to report that your patch

(1) c1c04cea13dc gpio: of: Fix logic inversion

together with the basic patch

(2) 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings

leads to a severe regression for our GTA04 board. The symptom was easy to see but really difficult
to understand: the LCD panel stopped working (did only show white pixels) after upgrading from
v5.0 to v5.1-rc1.

In a long bisect session, I was able to identify patch (1) as the first bad commit and started
to analyse what the code is supposed to do and then found the older patch (2).

I learned that it tries to handle a legacy "spi-cs-high" property of SPI slaves, but was stopped
from doing so by a bug (1). So only with both patches, the legacy handler becomes active which
explains why it was not noticed earlier.

Now, our GTA04 device tree from 2014 (v3.16-rc1) was already written without any legacy spi properties
in mind

(3) c2e138bc8ed80 ARM: dts: omap3-gta04: Add display support

So it has no spi-cs-high property and just defines constant 0 for the flags (GPIO_ACTIVE_HIGH after
some fix from 2015) assuming that this is sufficient. And it was until before v5.1-rc1.

This is now wrongly turned into an GPIO_ACTIVE_LOW by your legacy fix which makes the panel
stop feeling responsible to receive any initialization data over SPI.

Yes, there is a new message in the boot log for this case, but to be honest, nobody did see it
before I found the code that does print it. A non-working LCD panel was a too distracting indicator
and could have been everything (most likely drm or the panel driver or clock or regulator or
dma setup or whatever)...

So your assumption that this case is the exception as stated in commit message of (2) seems not
to be true. IMHO it should be standard (at least since 2015) that there is GPIO_ACTIVE_HIGH and
no spi-cs-high. So your patch (2) gives a legacy property a new priority over modern style.

BTW: the spi-cs-high is described as optional and indicative in the bindings document. But it
now appears to be required to get a cs-high.

By the way, the detection of spi-cs-high still seems to be wrong as of v5.1-rc1. Shouldn't it
look at the child node for "spi-cs-high" and not the controller node pointer? As far as I understand
the current code, all such legacy flags are therefore ignored and should break all boards that
use spi-cs-high and GPIO_ACTIVE_HIGH.

Of course, adding spi-cs-high; (at the controller node!) of our device tree can make it work,
but I do not like to be forced to introduce a legacy property to our DTS file, just because
gpio-lib tries to fix a legacy problem in the SPI subsystem (quite mind-twisting cross-subsystem
dependencies, aren't they?).

The real problem seems to be that the legacy spi-cs-high property itself still exists although
it should not be needed for ca. 5 years. From all this I conclude that nobody really needs this
legacy support or does test it. And if he/she does, they should better fix the cs-gpios entries
in the DTS and remove spi-cs-high completely.

Therefore I would suggest:
* revert both patches as soon as possible (v5.1-rc series) to remove the unexpected spi legacy
  code handler from the gpio subsystem.
* replace all uses of spi-cs-high by correct cs-gpios flags - unless they already are there.
  fgrep spi-cs-high arch/*/boot/dts/*.dts* shows only a handful of DTS candidates.
* fix spi-bus.txt documentation to describe this potential pitfall.

Then we can leave our GTA04 display and device tree untouched as from v3.16-rc1 up to v5.0.x.

BR and thanks,
Nikolaus

Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[Linus Walleij]

On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:

> (1) c1c04cea13dc gpio: of: Fix logic inversion
>
> together with the basic patch
>
> (2) 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings
>
> leads to a severe regression for our GTA04 board.

Sorry! :(

I found the same problem on my Nomadik board.

But I fixed it in that case by introducing a spi-cs-high into the DTS file:
https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2

> I learned that it tries to handle a legacy "spi-cs-high" property of SPI slaves, but was stopped
> from doing so by a bug (1). So only with both patches, the legacy handler becomes active which
> explains why it was not noticed earlier.
>
> Now, our GTA04 device tree from 2014 (v3.16-rc1) was already written without any legacy spi properties
> in mind

I'm sorry about that, however if you look at the DT binding document:
Documentation/devicetree/bindings/spi/spi-bus.txt

You will see that spi-cs-high is mandatory. So these DTS files are
incorrect.

> Therefore I would suggest:
> * revert both patches as soon as possible (v5.1-rc series) to remove the unexpected spi legacy
>   code handler from the gpio subsystem.
> * replace all uses of spi-cs-high by correct cs-gpios flags - unless they already are there.
>   fgrep spi-cs-high arch/*/boot/dts/*.dts* shows only a handful of DTS candidates.
> * fix spi-bus.txt documentation to describe this potential pitfall.

This does not work because there are devices that requires spi-cs-high to be
respected and the DTS second cell GPIO flag to be ignored.

Jan Kotas reported this problem.

They might have deployed DTB binaries that need to keep working, so we
cannot change it to ignore spi-cs-high like this. (I might give in if it can be
proven that all of them just recompile the DTS all the time and no
DTBs are in flash.)

I think in this case the oldest binding wins. The spi-cs-high was there before
we came up with the scheme to use the flags cell with GPIO phandles.

I think you simply have to patch these GTA04 DTS files to use
spi-cs-high.

But I'm open to other ideas, let's discuss this.

Yours,
Linus Walleij

Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[H. Nikolaus Schaller]

Hi Linus,

> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
> On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
>> (1) c1c04cea13dc gpio: of: Fix logic inversion
>> 
>> together with the basic patch
>> 
>> (2) 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings
>> 
>> leads to a severe regression for our GTA04 board.
> 
> Sorry! :(
> 
> I found the same problem on my Nomadik board.
> 
> But I fixed it in that case by introducing a spi-cs-high into the DTS file:
> https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2

Yes, that of course works and is our temporary solution.

And I see that you also have it on the controller node and not the slave node.

> 
>> I learned that it tries to handle a legacy "spi-cs-high" property of SPI slaves, but was stopped
>> from doing so by a bug (1). So only with both patches, the legacy handler becomes active which
>> explains why it was not noticed earlier.
>> 
>> Now, our GTA04 device tree from 2014 (v3.16-rc1) was already written without any legacy spi properties
>> in mind
> 
> I'm sorry about that, however if you look at the DT binding document:
> Documentation/devicetree/bindings/spi/spi-bus.txt

Shouldn't it be a property of the slave node and not the controller node?

> 
> You will see that spi-cs-high is mandatory. So these DTS files are
> incorrect.

How do you read that it is mandatory from

"All slave nodes can contain the following optional properties:
- spi-cs-high     - Empty property indicating device requires chip select
		    active high."

I read it as optional and indicative. Equal priority to cs-gpios.

> 
>> Therefore I would suggest:
>> * revert both patches as soon as possible (v5.1-rc series) to remove the unexpected spi legacy
>>  code handler from the gpio subsystem.
>> * replace all uses of spi-cs-high by correct cs-gpios flags - unless they already are there.
>>  fgrep spi-cs-high arch/*/boot/dts/*.dts* shows only a handful of DTS candidates.
>> * fix spi-bus.txt documentation to describe this potential pitfall.
> 
> This does not work because there are devices that requires spi-cs-high to be
> respected and the DTS second cell GPIO flag to be ignored.

Then, those should be fixed...

> 
> Jan Kotas reported this problem.

Thanks for adding him to the discussion.

> 
> They might have deployed DTB binaries that need to keep working,

Well, that is a weak argument. What if the GTA04 would have the DTB in FLASH
and would need it working (fortunately we always reflash kernel + dtbs)?

> so we
> cannot change it to ignore spi-cs-high like this. (I might give in if it can be
> proven that all of them just recompile the DTS all the time and no
> DTBs are in flash.)

BTW, on which node do these invariable DTBs have the spi-cs-high flag?
In the controller node (IMHO wrong) or the slave node (according to bindings doc)?

I have checked with randomly picked imx51-babbage.dts and it has it in the
slave node (pmic: mc13892@0). It also seems to be an example where different
CS lines want different settings.

If the all these DTBs have spi-cs-high in the slave node, none of them will
be fixed by the current code.

> 
> I think in this case the oldest binding wins.

Ok, it is the question when such deprecated things are really removed.
There is no clear answer...

> The spi-cs-high was there before
> we came up with the scheme to use the flags cell with GPIO phandles.

> 
> I think you simply have to patch these GTA04 DTS files to use
> spi-cs-high.

Ugly... Well, if DTS maintainers accept that?

> 
> But I'm open to other ideas, let's discuss this.

What about a CONFIG to explicitly enable/disable this legacy support?

IMHO it will need to be enabled for les than 1% of the kernel builds and
therefore also keeps the zImage smaller for all others. And avoids DTB
changes where the gpio flags are already correct.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[Andreas Kemnade]

On Sun, 24 Mar 2019 07:56:17 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> Hi Linus,
> 
> > Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@linaro.org>:
> > 
> > On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >   
> >> (1) c1c04cea13dc gpio: of: Fix logic inversion
> >> 
> >> together with the basic patch
> >> 
> >> (2) 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings
> >> 
> >> leads to a severe regression for our GTA04 board.  
> > 
> > Sorry! :(
> > 
> > I found the same problem on my Nomadik board.
> > 
> > But I fixed it in that case by introducing a spi-cs-high into the DTS file:
> > https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2  
> 
> Yes, that of course works and is our temporary solution.
> 
> And I see that you also have it on the controller node and not the slave node.
> 
So if I get it right is a check added for undocumented properties
(nothing about  spi-cs-high  in controller node in the bindings
documentation, only in the slave node) in the two patches mentioned.

And then you add that undocumented property to a dts file in your "fix".

That all sounds strange to me.

> >   
> >> I learned that it tries to handle a legacy "spi-cs-high" property of SPI slaves, but was stopped
> >> from doing so by a bug (1). So only with both patches, the legacy handler becomes active which
> >> explains why it was not noticed earlier.
> >> 
> >> Now, our GTA04 device tree from 2014 (v3.16-rc1) was already written without any legacy spi properties
> >> in mind  
> > 
> > I'm sorry about that, however if you look at the DT binding document:
> > Documentation/devicetree/bindings/spi/spi-bus.txt  
> 
> Shouldn't it be a property of the slave node and not the controller node?
> 
> > 
> > You will see that spi-cs-high is mandatory. So these DTS files are
> > incorrect.  
> 
> How do you read that it is mandatory from
> 
> "All slave nodes can contain the following optional properties:
> - spi-cs-high     - Empty property indicating device requires chip select
> 		    active high."
> 
> I read it as optional and indicative. Equal priority to cs-gpios.
> 
Well, it is in the list of optional properties. So the question is how
that "optional" is interpreted. Is it optional because you only use it
if cs is active high or is it optional because you can either indicate
that fact here or via gpio definition.

But again that flags makes no sense in the controller node.

Regards,
Andreas

Re: [Letux-kernel] [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[H. Nikolaus Schaller]

Hi Andreas,

> Am 30.03.2019 um 19:33 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> On Sun, 24 Mar 2019 07:56:17 +0100
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>> Hi Linus,
>> 
>>> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@linaro.org>:
>>> 
>>> On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> 
>>>> (1) c1c04cea13dc gpio: of: Fix logic inversion
>>>> 
>>>> together with the basic patch
>>>> 
>>>> (2) 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings
>>>> 
>>>> leads to a severe regression for our GTA04 board.  
>>> 
>>> Sorry! :(
>>> 
>>> I found the same problem on my Nomadik board.
>>> 
>>> But I fixed it in that case by introducing a spi-cs-high into the DTS file:
>>> https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2  
>> 
>> Yes, that of course works and is our temporary solution.
>> 
>> And I see that you also have it on the controller node and not the slave node.
>> 
> So if I get it right is a check added for undocumented properties
> (nothing about  spi-cs-high  in controller node in the bindings
> documentation, only in the slave node) in the two patches mentioned.
> 
> And then you add that undocumented property to a dts file in your "fix".
> 
> That all sounds strange to me.

> 
>>> 
>>>> I learned that it tries to handle a legacy "spi-cs-high" property of SPI slaves, but was stopped
>>>> from doing so by a bug (1). So only with both patches, the legacy handler becomes active which
>>>> explains why it was not noticed earlier.
>>>> 
>>>> Now, our GTA04 device tree from 2014 (v3.16-rc1) was already written without any legacy spi properties
>>>> in mind  
>>> 
>>> I'm sorry about that, however if you look at the DT binding document:
>>> Documentation/devicetree/bindings/spi/spi-bus.txt  
>> 
>> Shouldn't it be a property of the slave node and not the controller node?
>> 
>>> 
>>> You will see that spi-cs-high is mandatory. So these DTS files are
>>> incorrect.  
>> 
>> How do you read that it is mandatory from
>> 
>> "All slave nodes can contain the following optional properties:
>> - spi-cs-high     - Empty property indicating device requires chip select
>> 		    active high."
>> 
>> I read it as optional and indicative. Equal priority to cs-gpios.
>> 
> Well, it is in the list of optional properties. So the question is how
> that "optional" is interpreted. Is it optional because you only use it
> if cs is active high or is it optional because you can either indicate
> that fact here or via gpio definition.
> 
> But again that flags makes no sense in the controller node.

I have already prepared some fix for this controller/slave node issue and
a new CONFIG option to enable the legacy "spi-cs-high" only if needed.
Seems to be common practice that legacy API support (and legacy DTB
compatibility is also "API") is only enabled by CONFIGs on demand.

This can then replace our current GTA04 hack. Will post ASAP to LKML.

BR,
Nikolaus

Re: [Letux-kernel] [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[Linus Walleij]

On Sun, Mar 31, 2019 at 1:33 AM Andreas Kemnade <andreas@kemnade.info> wrote:

> > > But I fixed it in that case by introducing a spi-cs-high into the DTS file:
> > > https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2
> >
> > Yes, that of course works and is our temporary solution.
> >
> > And I see that you also have it on the controller node and not the slave node.
> >
> So if I get it right is a check added for undocumented properties
> (nothing about  spi-cs-high  in controller node in the bindings
> documentation, only in the slave node) in the two patches mentioned.
>
> And then you add that undocumented property to a dts file in your "fix".
>
> That all sounds strange to me.

Yeah it sounds strange because it is strange :)

I was simply confused and wrong. Sometimes we all do very uniformed
things, today it was me.

The flag should of course be on the slave node.

So now I have to fix my fix.

Yours,
Linus Walleij

Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[Linus Walleij]

(CC Kumar and Wolfgang who came up with spi-active-low, I think.)

On Sun, Mar 24, 2019 at 1:56 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> > Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@linaro.org>:

> > But I fixed it in that case by introducing a spi-cs-high into the DTS file:
> > https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2
>
> Yes, that of course works and is our temporary solution.
>
> And I see that you also have it on the controller node and not the slave node.

Yes I git it wrong, the slave should have it and another bug of mine
made it look at the controller not (some days I should not write
code in paths that do not get executed).

> > I'm sorry about that, however if you look at the DT binding document:
> > Documentation/devicetree/bindings/spi/spi-bus.txt
>
> Shouldn't it be a property of the slave node and not the controller node?

Indeed.

> > You will see that spi-cs-high is mandatory. So these DTS files are
> > incorrect.
>
> How do you read that it is mandatory from
>
> "All slave nodes can contain the following optional properties:
> - spi-cs-high     - Empty property indicating device requires chip select
>                     active high."
>
> I read it as optional and indicative. Equal priority to cs-gpios.

The basic problem is that spi-cs-high is defined negatively,
so by default a CS GPIO is active low. This clashes with the
default GPIO flag, as GPIO_ACTIVE_HIGH is zero, no flag,
and thus the default if nothing is specified, so if the GPIO flag is
zero it should be active high, but if "spi-active-high" is not on the
slave node it should be active low, so one of them have
to win, they can't both win.

I'd say that spi-cs-high existed before we standardized the GPIO
flags in the DT bindings. So people were relying on it for years before
we came up with the ACTIVE_HIGH/LOW flags.

commit f618ebfcbf9616a0fa9a78f5ecb69762f0fa3c59
Author: Wolfgang Ocker <weo@reccoware.de>
Date:   Wed Oct 15 15:00:47 2008 +0200

    of/spi: Support specifying chip select as active high via device tree

    The patch allows to specify that an SPI device needs an active high chip
    select.

    Signed-off-by: Wolfgang Ocker <weo@reccoware.de>
    Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

While the GPIO binding turns up 5 years later:

commit 71fab21fee07fd6d5f1a984db387cc5e4596f3fa
Author: Stephen Warren <swarren@nvidia.com>
Date:   Tue Feb 12 17:22:36 2013 -0700

    ARM: dt: add header to define GPIO flags

    Many GPIO device tree bindings use the same flags. Create a header to
    define those.

    Signed-off-by: Stephen Warren <swarren@nvidia.com>
    Acked-by: Rob Herring <rob.herring@calxeda.com>

And then this guy think it is a good idea to standardize this for
all GPIO phandles two years later:

commit 69d301fdd19635a39cb2b78e53fdd625b7a27924
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Thu Sep 24 15:05:45 2015 -0700

    gpio: add DT bindings for existing consumer flags

    It is customary for GPIO controllers to support open drain/collector
    and open source/emitter configurations. Add standard GPIO line flags
    to account for this and augment the documentation to say that these
    are the most generic bindings.

    Several people approached me to add new flags to the lines, and this
    makes sense, but let's first bind up the most common cases before we
    start to add exotic stuff.

    Thanks to H. Nikolaus Schaller for ideas on how to encode single-ended
    wiring such as open drain/source and open collector/emitter.

    Cc: Tony Lindgren <tony@atomide.com>
    Cc: Grygorii Strashko <grygorii.strashko@ti.com>
    Cc: H. Nikolaus Schaller <hns@goldelico.com>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> > This does not work because there are devices that requires spi-cs-high to be
> > respected and the DTS second cell GPIO flag to be ignored.
>
> Then, those should be fixed...

This can't be done because some old systems (mostly powerpc)
added between 2008-2013 do not know about GPIO flags and
have DTBs deployed in firmware that need to keep working.
They cannot be fixed.

> > They might have deployed DTB binaries that need to keep working,
>
> Well, that is a weak argument. What if the GTA04 would have the DTB in FLASH
> and would need it working (fortunately we always reflash kernel + dtbs)?

Usually the definition I use for "deployed DTB" is not
"deployed on my desk" but "deployed by a factory" i.e. someone
producing millions of TV sets or something. For example my monitor
is using a PowerPC CPU and has one of these DTBs in flash,
and expect it to keep working if I upgrade the kernel separately.

> BTW, on which node do these invariable DTBs have the spi-cs-high flag?
> In the controller node (IMHO wrong) or the slave node (according to bindings doc)?

Slave node I hope, the controller thing was just one of my stupid
mistakes.

> I have checked with randomly picked imx51-babbage.dts and it has it in the
> slave node (pmic: mc13892@0). It also seems to be an example where different
> CS lines want different settings.

I'm afraid the consumers of the old powerpc DTS files are not even
maintained inside the Linux kernel, as we sometimes assume. Those
were created and dispersed by vendors at a time when it was assumed
it was OK to maintain your DTS files somewhere out there on the
side (some people still hold this opinion, I don't).

> > I think in this case the oldest binding wins.
>
> Ok, it is the question when such deprecated things are really removed.
> There is no clear answer...

I think the DT bindings people would say never. They are deprecated
but they can never stop working. I guess in theory you could try to
figure out when the last piece of equipment using the old binding and
ever wanting a FW update stops working. That seems hard, but I don't
know much about these powerpc systems.

That is the reason why I try to contain these weirdness things inside
gpiolib-of.c rather than out amongst the different subsystems, so I can
have it under control.

> > I think you simply have to patch these GTA04 DTS files to use
> > spi-cs-high.
>
> Ugly... Well, if DTS maintainers accept that?

I suppose.

> What about a CONFIG to explicitly enable/disable this legacy support?
>
> IMHO it will need to be enabled for les than 1% of the kernel builds and
> therefore also keeps the zImage smaller for all others. And avoids DTB
> changes where the gpio flags are already correct.

Dunno about this, it looks fragile, I would prefer to keep all working.
But I will listen to reason.

I have thought about using of_machine_is_compatible("vendor,foo")
in gpiolib-of.h to whitelist some systems that will just use the
new way, or reversely blacklist some old systems that have to use the
old syntax. What do you think about this?

Yours,
Linus Walleij

Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[H. Nikolaus Schaller]

Hi Linus,

> Am 02.04.2019 um 06:02 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
> (CC Kumar and Wolfgang who came up with spi-active-low, I think.)
> 
> On Sun, Mar 24, 2019 at 1:56 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
>>> But I fixed it in that case by introducing a spi-cs-high into the DTS file:
>>> https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2
>> 
>> Yes, that of course works and is our temporary solution.
>> 
>> And I see that you also have it on the controller node and not the slave node.
> 
> Yes I git it wrong, the slave should have it and another bug of mine
> made it look at the controller not (some days I should not write
> code in paths that do not get executed).
> 
>>> I'm sorry about that, however if you look at the DT binding document:
>>> Documentation/devicetree/bindings/spi/spi-bus.txt
>> 
>> Shouldn't it be a property of the slave node and not the controller node?
> 
> Indeed.
> 
>>> You will see that spi-cs-high is mandatory. So these DTS files are
>>> incorrect.
>> 
>> How do you read that it is mandatory from
>> 
>> "All slave nodes can contain the following optional properties:
>> - spi-cs-high     - Empty property indicating device requires chip select
>>                    active high."
>> 
>> I read it as optional and indicative. Equal priority to cs-gpios.
> 
> The basic problem is that spi-cs-high is defined negatively,
> so by default a CS GPIO is active low. This clashes with the
> default GPIO flag, as GPIO_ACTIVE_HIGH is zero, no flag,
> and thus the default if nothing is specified, so if the GPIO flag is
> zero it should be active high, but if "spi-active-high" is not on the
> slave node it should be active low, so one of them have
> to win, they can't both win.
> 
> I'd say that spi-cs-high existed before we standardized the GPIO
> flags in the DT bindings. So people were relying on it for years before
> we came up with the ACTIVE_HIGH/LOW flags.
> 
> commit f618ebfcbf9616a0fa9a78f5ecb69762f0fa3c59
> Author: Wolfgang Ocker <weo@reccoware.de>
> Date:   Wed Oct 15 15:00:47 2008 +0200
> 
>    of/spi: Support specifying chip select as active high via device tree
> 
>    The patch allows to specify that an SPI device needs an active high chip
>    select.
> 
>    Signed-off-by: Wolfgang Ocker <weo@reccoware.de>
>    Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> 
> While the GPIO binding turns up 5 years later:
> 
> commit 71fab21fee07fd6d5f1a984db387cc5e4596f3fa
> Author: Stephen Warren <swarren@nvidia.com>
> Date:   Tue Feb 12 17:22:36 2013 -0700
> 
>    ARM: dt: add header to define GPIO flags
> 
>    Many GPIO device tree bindings use the same flags. Create a header to
>    define those.
> 
>    Signed-off-by: Stephen Warren <swarren@nvidia.com>
>    Acked-by: Rob Herring <rob.herring@calxeda.com>
> 
> And then this guy think it is a good idea to standardize this for
> all GPIO phandles two years later:
> 
> commit 69d301fdd19635a39cb2b78e53fdd625b7a27924
> Author: Linus Walleij <linus.walleij@linaro.org>
> Date:   Thu Sep 24 15:05:45 2015 -0700
> 
>    gpio: add DT bindings for existing consumer flags
> 
>    It is customary for GPIO controllers to support open drain/collector
>    and open source/emitter configurations. Add standard GPIO line flags
>    to account for this and augment the documentation to say that these
>    are the most generic bindings.
> 
>    Several people approached me to add new flags to the lines, and this
>    makes sense, but let's first bind up the most common cases before we
>    start to add exotic stuff.
> 
>    Thanks to H. Nikolaus Schaller for ideas on how to encode single-ended
>    wiring such as open drain/source and open collector/emitter.
> 
>    Cc: Tony Lindgren <tony@atomide.com>
>    Cc: Grygorii Strashko <grygorii.strashko@ti.com>
>    Cc: H. Nikolaus Schaller <hns@goldelico.com>

Yes, I remember to help with the open drain/collector definition :)

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

> 
>>> This does not work because there are devices that requires spi-cs-high to be
>>> respected and the DTS second cell GPIO flag to be ignored.
>> 
>> Then, those should be fixed...
> 
> This can't be done because some old systems (mostly powerpc)
> added between 2008-2013 do not know about GPIO flags and
> have DTBs deployed in firmware that need to keep working.
> They cannot be fixed.


The question is if it is even possible to deploy a new kernel
for such devices and if anyone wants to do...

This also gives another idea: make it depend on "powerpc".

> 
>>> They might have deployed DTB binaries that need to keep working,
>> 
>> Well, that is a weak argument. What if the GTA04 would have the DTB in FLASH
>> and would need it working (fortunately we always reflash kernel + dtbs)?
> 
> Usually the definition I use for "deployed DTB" is not
> "deployed on my desk" but "deployed by a factory" i.e. someone
> producing millions of TV sets or something. For example my monitor
> is using a PowerPC CPU and has one of these DTBs in flash,
> and expect it to keep working if I upgrade the kernel separately.

Ok, I see.

We basically have the same (devices deployed to unknown users), but
we can and do flash kernel + dtb in parallel so it is easier in our
case.

> 
>> BTW, on which node do these invariable DTBs have the spi-cs-high flag?
>> In the controller node (IMHO wrong) or the slave node (according to bindings doc)?
> 
> Slave node I hope, the controller thing was just one of my stupid
> mistakes.
> 
>> I have checked with randomly picked imx51-babbage.dts and it has it in the
>> slave node (pmic: mc13892@0). It also seems to be an example where different
>> CS lines want different settings.
> 
> I'm afraid the consumers of the old powerpc DTS files are not even
> maintained inside the Linux kernel, as we sometimes assume. Those
> were created and dispersed by vendors at a time when it was assumed
> it was OK to maintain your DTS files somewhere out there on the
> side (some people still hold this opinion, I don't).
> 
>>> I think in this case the oldest binding wins.
>> 
>> Ok, it is the question when such deprecated things are really removed.
>> There is no clear answer...
> 
> I think the DT bindings people would say never. They are deprecated
> but they can never stop working. I guess in theory you could try to
> figure out when the last piece of equipment using the old binding and
> ever wanting a FW update stops working. That seems hard, but I don't
> know much about these powerpc systems.

> 
> That is the reason why I try to contain these weirdness things inside
> gpiolib-of.c rather than out amongst the different subsystems, so I can
> have it under control.

> 
>>> I think you simply have to patch these GTA04 DTS files to use
>>> spi-cs-high.
>> 
>> Ugly... Well, if DTS maintainers accept that?
> 
> I suppose.
> 
>> What about a CONFIG to explicitly enable/disable this legacy support?
>> 
>> IMHO it will need to be enabled for les than 1% of the kernel builds and
>> therefore also keeps the zImage smaller for all others. And avoids DTB
>> changes where the gpio flags are already correct.

First of all, the new patch checking for presence of cs-gpios works for us!

So we are fine with upstream v5.1-rc3 on our devices and do not need to
"fix" the DTS.

> 
> Dunno about this, it looks fragile, I would prefer to keep all working.
> But I will listen to reason.

Reason why I propose a CONFIG option is:

if someone is able to compile and deploy a v5.1 kernel for some device which
has (old) and problematic DTB in ROM he/she must have access to the .config.
So it is easy to modify it to enable legacy handling of spi-cs-high. And keep
it disabled for all others.

That is the idea I have casted into my

	"[BUG/PATCH 3/4] gpiolib: make legacy handling for spi-cs-high optional"

sent separately. Maybe there is a better solution (e.g. replace SPI_MASTER
by SPI_LEGACY_MASTER or similar).

> 
> I have thought about using of_machine_is_compatible("vendor,foo")
> in gpiolib-of.h to whitelist some systems that will just use the
> new way, or reversely blacklist some old systems that have to use the
> old syntax. What do you think about this?

This would just automate modifying .config for a specific board. By adding
runtime code for everybody.

And it gives you the burden to collect all new "vendor,foo" strings every
now and then, instead of having "vendor" change some .config when building
a kernel image for "foo", especially for out-of-tree devices.

Since it requires help from vendors anyways (those would have to supply the
"vendor,foo" to you, you can maybe easier ask them to change their CONFIG.

For DTS of In-tree devices you could IMHO simply replace spi-cs-high by the
correct gpio flags so that there is no use of it in kernel.org any more
and nobody is tempted to use it as an example...

BR and thanks,
Nikolaus

Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[Mark Brown]

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

On Tue, Apr 02, 2019 at 07:05:35AM +0200, H. Nikolaus Schaller wrote:
> > Am 02.04.2019 um 06:02 schrieb Linus Walleij <linus.walleij@linaro.org>:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> >>> This does not work because there are devices that requires spi-cs-high to be
> >>> respected and the DTS second cell GPIO flag to be ignored.

> >> Then, those should be fixed...

> > This can't be done because some old systems (mostly powerpc)
> > added between 2008-2013 do not know about GPIO flags and
> > have DTBs deployed in firmware that need to keep working.
> > They cannot be fixed.

> The question is if it is even possible to deploy a new kernel
> for such devices and if anyone wants to do...

It's relatively common, especially with older devices, for people to be
perfectly happy to update the kernel and do so frequently but unwilling
to update the bootloader as the procedure for recovering a broken
bootloader is difficult or perhaps not even possible.

> This also gives another idea: make it depend on "powerpc".

That won't fly, the code has always been architecture neutral.

> > Dunno about this, it looks fragile, I would prefer to keep all working.
> > But I will listen to reason.

> Reason why I propose a CONFIG option is:

> if someone is able to compile and deploy a v5.1 kernel for some device which
> has (old) and problematic DTB in ROM he/she must have access to the .config.
> So it is easy to modify it to enable legacy handling of spi-cs-high. And keep
> it disabled for all others.

This assumes people aren't able to run a distro kernel...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[H. Nikolaus Schaller]

> Am 02.04.2019 um 07:31 schrieb Mark Brown <broonie@kernel.org>:
> 
> It's relatively common, especially with older devices, for people to be
> perfectly happy to update the kernel and do so frequently but unwilling
> to update the bootloader as the procedure for recovering a broken
> bootloader is difficult or perhaps not even possible.

Yes, that is the case Linus tries to solve, where it is not even possible
to modify the DTB. No idea how it could be solved if it would not be
possible to change our 6 years old DTB.

Fortunately we do not need to change it at all with the latest check for
cs-gpios.

> 
>> This also gives another idea: make it depend on "powerpc".
> 
> That won't fly, the code has always been architecture neutral.

Ok. Just an idea.

> 
>>> Dunno about this, it looks fragile, I would prefer to keep all working.
>>> But I will listen to reason.
> 
>> Reason why I propose a CONFIG option is:
> 
>> if someone is able to compile and deploy a v5.1 kernel for some device which
>> has (old) and problematic DTB in ROM he/she must have access to the .config.
>> So it is easy to modify it to enable legacy handling of spi-cs-high. And keep
>> it disabled for all others.
> 
> This assumes people aren't able to run a distro kernel...

Yes, indeed.

I have learned from Linus that the problem with legacy spi-cs-high are mostly embedded
powerpc systems deployed between 2008 and 2013 where the boot-loader can't be
changed. And where the dts is not maintained on kernel.org.

IMHO it is very unlikely that they are running distro kernels. Or is there an example
of such a system?

BR and thanks,
Nikolaus

Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[Mark Brown]

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

On Tue, Apr 02, 2019 at 08:37:23AM +0200, H. Nikolaus Schaller wrote:

> I have learned from Linus that the problem with legacy spi-cs-high are mostly embedded
> powerpc systems deployed between 2008 and 2013 where the boot-loader can't be
> changed. And where the dts is not maintained on kernel.org.

> IMHO it is very unlikely that they are running distro kernels. Or is there an example
> of such a system?

I used to work on embedded systems which were built by essentially
bolting expansion cards onto standard reference systems, for some of
them we shipped distro kernels with the extra hardware supported by
adding modules.  No idea if there's practical examples for specific
PowerPC systems though.

The burden when you're breaking things is more on showing that there
isn't a problem anyway...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]