Hi Linus, > Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@xxxxxxxxxx>: > > On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> 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
