[BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel
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
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
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
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
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
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
(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
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
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...
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
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...
-
Andreas Kemnade -
H. Nikolaus Schaller -
Linus Walleij -
Mark Brown