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