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@xxxxxxxxxx>:
> 
> (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@xxxxxxxxxxxxx> wrote:
>>> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@xxxxxxxxxx>:
> 
>>> 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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
>    Signed-off-by: Kumar Gala <galak@xxxxxxxxxxxxxxxxxxx>
> 
> While the GPIO binding turns up 5 years later:
> 
> commit 71fab21fee07fd6d5f1a984db387cc5e4596f3fa
> Author: Stephen Warren <swarren@xxxxxxxxxx>
> 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@xxxxxxxxxx>
>    Acked-by: Rob Herring <rob.herring@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> 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@xxxxxxxxxxx>
>    Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
>    Cc: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>

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

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

> 
>>> 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