| Message ID | a1884719d4381cc3254408229cf5d9f94d4f2995.1516744108.git.jan.kundrat@cesnet.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jan, On Tue, Jan 23, 2018 at 8:56 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote: > Commit b28b9149b37f added support for using an additional GPIO pin for > Chip Select. According to that commit and to my understanding of a > semi-public datahseet, it seems that the SPI hardware really insists on > driving *some* HW CS signal whenever a SPI transaction is in progress. > > The old code effectively forced the HW CS pin to be CS0. That means that > there could not be anything connected to the real CS0 signal when one > uses a GPIO CS. That assumption does not hold on e.g. Solidrun Clearfog > where some SOM models have a built-in SPI flash on SPI1, CS0. > > Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz> Thanks for your patch! > --- a/Documentation/devicetree/bindings/spi/spi-orion.txt > +++ b/Documentation/devicetree/bindings/spi/spi-orion.txt > @@ -28,6 +28,11 @@ Optional properties: > - clock-names : names of used clocks, mandatory if the second clock is > used, the name must be "core", and "axi" (the latter > is only for Armada 7K/8K). > +- orion-spi,cs-gpio-hw-cs : Because the SoC always wants to drive some HW Chip > + Select pin, it is necessary to specify which one > + should be used when Linux drives an additional > + GPIO as a software-controlled CS. Defaults to HW > + CS 0. Can't you just deduce an unused HW CS from the cs-gpios property? Renesas MSIOF also requires driving a native chip select, cfr. commit b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration"). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 23, 2018 at 9:56 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote: > > Commit b28b9149b37f added support for using an additional GPIO pin for > Chip Select. According to that commit and to my understanding of a > semi-public datahseet, it seems that the SPI hardware really insists on > driving *some* HW CS signal whenever a SPI transaction is in progress. > > The old code effectively forced the HW CS pin to be CS0. That means that > there could not be anything connected to the real CS0 signal when one > uses a GPIO CS. That assumption does not hold on e.g. Solidrun Clearfog > where some SOM models have a built-in SPI flash on SPI1, CS0. Like Geert said I also highly recommend to use existing properties. Better if you, in the meantime, can switch the driver to use GPIO descriptors, while core still use legacy numbers. It would make life easier in the future to switch SPI core to GPIO descriptors completely.
On středa 24. ledna 2018 14:41:27 CET, Geert Uytterhoeven wrote: > Can't you just deduce an unused HW CS from the cs-gpios property? Thanks for a review. Yes, I probably can. I just noticed that my local use of spi-orion's "direct mode" is probably the root cause of some weirdness that I'm seeing (such as the CS GPIO pin going up in the middle of a two-byte transaction, and other nasty issues that just go away once one adds a dev_info call...). > Renesas MSIOF also requires driving a native chip select, cfr. commit > b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration"). I'll take a look. It could be a candidate for some shared code, I suppose. Cheers, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 24, 2018 at 04:19:41PM +0200, Andy Shevchenko wrote: > It would make life easier in the future to switch SPI core to GPIO > descriptors completely. Linus has patches for that which I'm expecting very soon after the merge window.
On středa 24. ledna 2018 16:03:15 CET, Jan Kundrát wrote: >> Renesas MSIOF also requires driving a native chip select, cfr. commit >> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration"). > > I'll take a look. It could be a candidate for some shared code, I suppose. Hi Gert, if I take your code from that commit almost as-is, probing of my SPI controller fails with errno -16 (that's EBUSY, right?). The GPIO pin that I want to use is defined as a gpio-hog in the DT. Is my DT correct? Should I define the GPIO pin as an output hog? IS your code supposed to deal with this? With kind regards, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jan, On Wed, Jan 24, 2018 at 6:56 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote: > On středa 24. ledna 2018 16:03:15 CET, Jan Kundrát wrote: >>> Renesas MSIOF also requires driving a native chip select, cfr. commit >>> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration"). >> >> I'll take a look. It could be a candidate for some shared code, I suppose. > > Hi Gert, if I take your code from that commit almost as-is, probing of my > SPI controller fails with errno -16 (that's EBUSY, right?). The GPIO pin > that I want to use is defined as a gpio-hog in the DT. > > Is my DT correct? Should I define the GPIO pin as an output hog? IS your > code supposed to deal with this? Using a hog indeed marks it busy, as the hog is a user. Why do you use a hog in the first place? Shouldn't the SPI driver configure the GPIO, according to cs-gpios? Before the advent of DT, the platform code was supposed to take care of that, so this may be a too-literal conversion from platform code to DT. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jan, Geert, On 25/01/18 04:03, Jan Kundrát wrote: > On středa 24. ledna 2018 14:41:27 CET, Geert Uytterhoeven wrote: >> Can't you just deduce an unused HW CS from the cs-gpios property? > > Thanks for a review. Yes, I probably can. I just noticed that my local use > of spi-orion's "direct mode" is probably the root cause of some weirdness > that I'm seeing (such as the CS GPIO pin going up in the middle of a > two-byte transaction, and other nasty issues that just go away once one > adds a dev_info call...). > >> Renesas MSIOF also requires driving a native chip select, cfr. commit >> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration"). > > I'll take a look. It could be a candidate for some shared code, I suppose. How would this work? If I understand the sh-msiof driver it picks an unused native CS. In the case of b28b9149b37f the gpios supplement the native CS they do not replace it (the hardware I was using has a GPIO which controls a gate attached to CS0). My specific board isn't upstream but here's the snippet from its dts &spi0 { status = "okay"; cs-gpios = <0 &gpio0 17 GPIO_ACTIVE_LOW>; spi-flash@0 { /* This is on CS0 when GPIO 17 is high */ }; spi-sram@1 { /* This is on CS0 when GPIO 17 is low */ }; }; I'm not sure what else I could do. I can't claim the GPIO twice. If I could I could probably use spi-cs-high to handle the high/low toggle. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chris, On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote: > On 25/01/18 04:03, Jan Kundrát wrote: >> On středa 24. ledna 2018 14:41:27 CET, Geert Uytterhoeven wrote: >>> Can't you just deduce an unused HW CS from the cs-gpios property? >> >> Thanks for a review. Yes, I probably can. I just noticed that my local use >> of spi-orion's "direct mode" is probably the root cause of some weirdness >> that I'm seeing (such as the CS GPIO pin going up in the middle of a >> two-byte transaction, and other nasty issues that just go away once one >> adds a dev_info call...). >> >>> Renesas MSIOF also requires driving a native chip select, cfr. commit >>> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration"). >> >> I'll take a look. It could be a candidate for some shared code, I suppose. > > How would this work? If I understand the sh-msiof driver it picks an > unused native CS. Correct. The hardware always has to drive one of the 3 available native chip selects. > In the case of b28b9149b37f the gpios supplement the > native CS they do not replace it (the hardware I was using has a GPIO > which controls a gate attached to CS0). However, Jan wrote: > semi-public datahseet, it seems that the SPI hardware really insists on > driving *some* HW CS signal whenever a SPI transaction is in progress. If that is correct, it behaves like MSIOF, so you must leave one native chip select unused. If all native chip selects are in use, one of them must be replaced by a GPIO chip select (which could be the same physical pin, depending on pinmux capabilities). > My specific board isn't upstream but here's the snippet from its dts > > &spi0 { > status = "okay"; > cs-gpios = <0 > &gpio0 17 GPIO_ACTIVE_LOW>; > > spi-flash@0 { > /* This is on CS0 when GPIO 17 is high */ > }; > > spi-sram@1 { > /* This is on CS0 when GPIO 17 is low */ > }; > }; > > I'm not sure what else I could do. I can't claim the GPIO twice. If I > could I could probably use spi-cs-high to handle the high/low toggle. So this uses GPIO17 to drive a mux to connect CS0 to either the first or second device? But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you rely on CS0 still being driven when @1 is selected, right? That indeed won't work when an unused native chip select is driven when using cs-gpio. IMHO, this needs the mux to be described properly in DT. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
") Fcc: +sent-mail On Thu, Jan 25, 2018 at 09:51:06AM +0100, Geert Uytterhoeven wrote: > On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham > However, Jan wrote: > > semi-public datahseet, it seems that the SPI hardware really insists on > > driving *some* HW CS signal whenever a SPI transaction is in progress. > If that is correct, it behaves like MSIOF, so you must leave one native chip > select unused. If all native chip selects are in use, one of them must be > replaced by a GPIO chip select (which could be the same physical pin, > depending on pinmux capabilities). Using the same pin is the ideal thing obviously, or if not then something that we know isn't routed out of the SoC (which may or may not be possible with a given chip design). > > spi-flash@0 { > > /* This is on CS0 when GPIO 17 is high */ > > }; > > > > spi-sram@1 { > > /* This is on CS0 when GPIO 17 is low */ > > }; > > }; > > I'm not sure what else I could do. I can't claim the GPIO twice. If I > > could I could probably use spi-cs-high to handle the high/low toggle. > So this uses GPIO17 to drive a mux to connect CS0 to either the first or > second device? Interesting, coincidentally there was someone else sending a patch for muxing the other day which looked like having some support for chip select muxing would be the most sensible implementation. I've copied in Ben who had initially approached this with a mux for the full bus which I'm not so convincd about. > But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you > rely on CS0 still being driven when @1 is selected, right? > That indeed won't work when an unused native chip select is driven when > using cs-gpio. > IMHO, this needs the mux to be described properly in DT. Yes.
Hi Mark, Geert, On 26/01/18 00:57, Mark Brown wrote: > ") > Fcc: +sent-mail > > On Thu, Jan 25, 2018 at 09:51:06AM +0100, Geert Uytterhoeven wrote: >> On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham > >> However, Jan wrote: >>> semi-public datahseet, it seems that the SPI hardware really insists on >>> driving *some* HW CS signal whenever a SPI transaction is in progress. > >> If that is correct, it behaves like MSIOF, so you must leave one native chip >> select unused. If all native chip selects are in use, one of them must be >> replaced by a GPIO chip select (which could be the same physical pin, >> depending on pinmux capabilities). > > Using the same pin is the ideal thing obviously, or if not then > something that we know isn't routed out of the SoC (which may or may not > be possible with a given chip design). > >>> spi-flash@0 { >>> /* This is on CS0 when GPIO 17 is high */ >>> }; >>> >>> spi-sram@1 { >>> /* This is on CS0 when GPIO 17 is low */ >>> }; >>> }; > >>> I'm not sure what else I could do. I can't claim the GPIO twice. If I >>> could I could probably use spi-cs-high to handle the high/low toggle. > >> So this uses GPIO17 to drive a mux to connect CS0 to either the first or >> second device? Correct. GPIO17 directs CS0 with the addition of a logic gate. > Interesting, coincidentally there was someone else sending a patch for > muxing the other day which looked like having some support for chip > select muxing would be the most sensible implementation. I've copied in > Ben who had initially approached this with a mux for the full bus which > I'm not so convincd about. > >> But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you >> rely on CS0 still being driven when @1 is selected, right? >> That indeed won't work when an unused native chip select is driven when >> using cs-gpio. Yes that's my problem. > >> IMHO, this needs the mux to be described properly in DT. > > Yes. I'm more than happy to update the DT on this product. But I've no idea what that update would look like. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chris, On Thu, Jan 25, 2018 at 10:18 PM, Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote: > On 26/01/18 00:57, Mark Brown wrote: >> ") >> Fcc: +sent-mail >> >> On Thu, Jan 25, 2018 at 09:51:06AM +0100, Geert Uytterhoeven wrote: >>> On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham >> >>> However, Jan wrote: >>>> semi-public datahseet, it seems that the SPI hardware really insists on >>>> driving *some* HW CS signal whenever a SPI transaction is in progress. >> >>> If that is correct, it behaves like MSIOF, so you must leave one native chip >>> select unused. If all native chip selects are in use, one of them must be >>> replaced by a GPIO chip select (which could be the same physical pin, >>> depending on pinmux capabilities). >> >> Using the same pin is the ideal thing obviously, or if not then >> something that we know isn't routed out of the SoC (which may or may not >> be possible with a given chip design). >> >>>> spi-flash@0 { >>>> /* This is on CS0 when GPIO 17 is high */ >>>> }; >>>> >>>> spi-sram@1 { >>>> /* This is on CS0 when GPIO 17 is low */ >>>> }; >>>> }; >> >>>> I'm not sure what else I could do. I can't claim the GPIO twice. If I >>>> could I could probably use spi-cs-high to handle the high/low toggle. >> >>> So this uses GPIO17 to drive a mux to connect CS0 to either the first or >>> second device? > > Correct. GPIO17 directs CS0 with the addition of a logic gate. > >> Interesting, coincidentally there was someone else sending a patch for >> muxing the other day which looked like having some support for chip >> select muxing would be the most sensible implementation. I've copied in >> Ben who had initially approached this with a mux for the full bus which >> I'm not so convincd about. >> >>> But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you >>> rely on CS0 still being driven when @1 is selected, right? >>> That indeed won't work when an unused native chip select is driven when >>> using cs-gpio. > > Yes that's my problem. > >> >>> IMHO, this needs the mux to be described properly in DT. >> >> Yes. > > I'm more than happy to update the DT on this product. But I've no idea > what that update would look like. (Un)fortunately a mux driver is WIP, as Mark mentioned above. See https://lkml.org/lkml/2018/1/22/859 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 25, 2018 at 10:27:24PM +0100, Geert Uytterhoeven wrote: > On Thu, Jan 25, 2018 at 10:18 PM, Chris Packham > > I'm more than happy to update the DT on this product. But I've no idea > > what that update would look like. > (Un)fortunately a mux driver is WIP, as Mark mentioned above. > See https://lkml.org/lkml/2018/1/22/859 From the discussion there I think the case that Ben had was for a controller on the SPI bus rather than a mux on the CS so you're probably going to have to do this yourself. I've been thinking about what we might do in DT terms but I've not been getting too many ideas, we may need to talk it over with the DT people.
On středa 24. ledna 2018 19:17:35 CET, Geert Uytterhoeven wrote: > Using a hog indeed marks it busy, as the hog is a user. > > Why do you use a hog in the first place? Shouldn't the SPI driver configure > the GPIO, according to cs-gpios? That doesn't happen with spi-orion.c. If I remove my gpio-hog for the CS GPIO pin from the DT, then my device doesn't appear to be selected. Also, I can run gpio-hammer targetting my CS pin just fine -- and that seems dangerous. I don't see any "claim" of a GPIO neither in spi.c nor spi-orion.c. I did some very hacky grepping [1], and it seems that other drivers probably handle this on their own. The only other match is spi-lantiq-ssc.c which doesn't however touch the CS GPIO. Shouldn't this be really handled by the SPI core? (I don't think I'm volunteering with patches as I couldn't really test these.) Should I submit a patch which adapts spi-orion.c to essentially follow how e.g spi-imx.c allocates a GPIO pin for CS? Cheers, Jan [1] `git grep --files-with-match cs_gpio drivers/spi/*.c | xargs -n1 git grep -E --files-without-match 'gpio_direction_output|gpio_request' | cat` -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2018-01-23 at 20:56 +0100, Jan Kundrát wrote: > Commit b28b9149b37f added support for using an additional GPIO pin for > Chip Select. According to that commit and to my understanding of a > semi-public datahseet, it seems that the SPI hardware really insists on > driving *some* HW CS signal whenever a SPI transaction is in progress. It's pretty common for SPI hardware to have this limitation. Another way to fix it, besides default to 0 or to select an unused chipselect, is to use the cs number of the slave modulus the number of native chip selects. They are or were some other drivers that do this. The idea is that CS numbers don't need to be sequential and there isn't much of a cost of an unused chip select (one word in device tree property). So say you want two GPIO CS devices and they should make use of native CS 1, because that pin has no conflicts for this usage, and you also want a device on native chip select 0 that's not using a GPIO CS. The spi master has four native chip selects. Set up the DT like this: cs-gpios = <0>, <&gpio 42>, <0>, <0>, <0>, <&gpio 43>; flash@0 { reg = <0>; } /* on native CS 0 */ slave@1 { reg = <1>; } /* on GPIO 42, also native CS 1 */ slave@5 { reg = <5>; } /* on GPIO 43, also native CS 1 */ This allows assigning which native CS to use. It could pix muxing limitations don't allow just any CS to be chosen. If there's no reason to care, then selecting automatically is easier to use.
On úterý 30. ledna 2018 2:13:37 CET, Trent Piepho wrote: > Another way to fix it, besides default to 0 or to select an unused > chipselect, is to use the cs number of the slave modulus the number of > native chip selects. They are or were some other drivers that do this. > The idea is that CS numbers don't need to be sequential and there isn't > much of a cost of an unused chip select (one word in device tree > property). Hi Trent, are you talking about an integer modulo here? I don't think that modulo is a correct approach. If my HW has two CS and I use the following DT: cs-gpios = <0>, <&gpio0 1>, <&gpio0 2>, <0>; ...then the logical CS2 (at gpio0 pin 2) conflicts with the native CS0. Also, the spi-orion driver currently hard-codes that each and every model is supposed to have exactly eight HW CS pins. That's not true for my SoC (Marvell Armada 388, the Solidrun Clearfog Base board); the 88F6828 CPU only has four SPI CS signals. What my patch does is simply picking the *first* unused CS and going with that one because that looks like the easiest and also safest option. Cheers, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 30, 2018 at 09:03:10AM +0100, Jan Kundrát wrote: > On úterý 30. ledna 2018 2:13:37 CET, Trent Piepho wrote: > > Another way to fix it, besides default to 0 or to select an unused > > chipselect, is to use the cs number of the slave modulus the number of > > native chip selects. They are or were some other drivers that do this. > Hi Trent, are you talking about an integer modulo here? I don't think that > modulo is a correct approach. If my HW has two CS and I use the following > DT: > cs-gpios = <0>, <&gpio0 1>, <&gpio0 2>, <0>; > ...then the logical CS2 (at gpio0 pin 2) conflicts with the native CS0. You can't physically design working hardware for a system with this limitation that doesn't have a free chip select somehow - if the hardware requires that it controls an internal chip select line then using a GPIO chip select is going to require that there's some internal chip select line that's getting controlled so you need one that's not connected to an actual device that can be used for this purpose. > Also, the spi-orion driver currently hard-codes that each and every model is > supposed to have exactly eight HW CS pins. That's not true for my SoC > (Marvell Armada 388, the Solidrun Clearfog Base board); the 88F6828 CPU only > has four SPI CS signals. > What my patch does is simply picking the *first* unused CS and going with > that one because that looks like the easiest and also safest option. I would expect that you want to have this selected by the board designer (unless there's something convenient like a chip select present in the controller but not routed out of the IP you can guarantee will never be used). That will avoid issues if for example the DT is still not complete.
On úterý 30. ledna 2018 16:54:09 CET, Mark Brown wrote: > You can't physically design working hardware for a system with this > limitation that doesn't have a free chip select somehow - if the > hardware requires that it controls an internal chip select line then > using a GPIO chip select is going to require that there's some internal > chip select line that's getting controlled so you need one that's not > connected to an actual device that can be used for this purpose. Hi Mark, yes, I understand that. Sorry if I confused things by trying to explain why a modulo-algorithm won't work. I have an unused CS lane. It's just that the spi-orion driver currently hardcodes HW CS#0, and that one is not free on my board. The latest version of my patch [1] finds the first *GPIO* CS number. As you pointed out, this allocation means that a HW CS signal with the same number is definitely not connected to any other slave's CS. My patch therefore drives this "unused" HW CS whenever a GPIO CS is needed. This is similar to what e.g. spi-sh-msiof.c is doing now. Should I perhaps improve the patch description? I'm open to all comments here. I see that it might be a bit confusing that I also changed spi-orion.c so that it claims/requests/registers/allocates the GPIO for CS. Without that patch , spi-orion is different from other SPI masters; I have to manually create a gpio-hog for my CS GPIO. I'll be happy to rework my changes into two separate patches. >> Also, the spi-orion driver currently hard-codes that each and >> every model is >> supposed to have exactly eight HW CS pins. That's not true for my SoC >> (Marvell Armada 388, the Solidrun Clearfog Base board); the >> 88F6828 CPU only >> has four SPI CS signals. > >> What my patch does is simply picking the *first* unused CS and going with >> that one because that looks like the easiest and also safest option. > > I would expect that you want to have this selected by the board designer > (unless there's something convenient like a chip select present in the > controller but not routed out of the IP you can guarantee will never be > used). That will avoid issues if for example the DT is still not > complete. That's the approach that I used in my first version of this patch [2], but Geert suggested to follow some other SPI master's code and implement autodiscovery. I don't really care. Which approach is better from your point of view? I understand the preference to specify the "fake" HW CS manually. My board has a flash at CS#0, and it is marked as disabled in a default DT. But perhaps that's a special case. Your call, anyway. With kind regards, Jan [1] https://patchwork.kernel.org/patch/10187175/ [2] https://patchwork.kernel.org/patch/10182583/ -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2018-01-30 at 09:03 +0100, Jan Kundrát wrote: > On úterý 30. ledna 2018 2:13:37 CET, Trent Piepho wrote: > > Another way to fix it, besides default to 0 or to select an unused > > chipselect, is to use the cs number of the slave modulus the number of > > native chip selects. They are or were some other drivers that do this. > > The idea is that CS numbers don't need to be sequential and there isn't > > much of a cost of an unused chip select (one word in device tree > > property). > > Hi Trent, are you talking about an integer modulo here? I don't think that > modulo is a correct approach. If my HW has two CS and I use the following > DT: > > cs-gpios = <0>, <&gpio0 1>, <&gpio0 2>, <0>; > > ...then the logical CS2 (at gpio0 pin 2) conflicts with the native CS0. What you'd do is use this: cs-gpios = <0>, <&gpio0 1>, <0>, <&gpio0 2>; Put the slaves on cs 0, 1, and 3. The slaves on 1 and 3 use native CS1 with their gpios. > Also, the spi-orion driver currently hard-codes that each and every model > is supposed to have exactly eight HW CS pins. That's not true for my SoC > (Marvell Armada 388, the Solidrun Clearfog Base board); the 88F6828 CPU > only has four SPI CS signals. Yes, you need to know how many CS the driver will consider the device to have. That is an additional difficulty. > > What my patch does is simply picking the *first* unused CS and going with > that one because that looks like the easiest and also safest option. Yes, I agree the easiest to use. Don't need to know how many HW CS the driver will think there are. Don't need to manually place your gpios cs in the list to avoid native cs conflicts. But you do lose some flexibility. What if you don't want the first unused CS? Probably rather obscure to have that issue.
On Tue, 2018-01-30 at 17:09 +0100, Jan Kundrát wrote: > On úterý 30. ledna 2018 16:54:09 CET, Mark Brown wrote: > > > What my patch does is simply picking the *first* unused CS and going with > > > that one because that looks like the easiest and also safest option. > > > > I would expect that you want to have this selected by the board designer > > (unless there's something convenient like a chip select present in the > > controller but not routed out of the IP you can guarantee will never be > > used). That will avoid issues if for example the DT is still not > > complete. > > That's the approach that I used in my first version of this patch [2], but > Geert suggested to follow some other SPI master's code and implement > autodiscovery. I don't really care. Which approach is better from your > point of view? Auto selecting the first unused is certainly nice for the DT constructor if it works. Also seems less fragile to me. But what about possible future support for dynamic creation of spi devices? There's already a patch for that out there and there is also DT fragment loading that could be merged. There might not be a device on HW CS0 when the driver loads, but then one is added later, which will be too late since the HW CS was chosen at driver probe time. That issue can probably be fixed by choosing the HW CS at spi transfer setup time. Another problem is that I don't believe there is any way for the master to know about what SPI slaves exist. In Jan's patch, the assumption is that a non-gpio CS is used. But this is not strictly correct, as is does not distinguish between an unused CS and in-use HW CS. Both those cases are "not gpio". But perhaps this is ok. I think the instructions to the DT author should be: If you will have GPIO chip selects, you must choose one HW CS line to be consumed and used by all GPIO CS based slave(s). Choose by placing the first GPIO CS at the position of the HW CS that should be consumed by that GPIO CS and any remaining ones. E.g., if you first to have three GPIO CS, and want them to consume HW CS 1, the first GPIO CS *must* be placed at CS 1, the remaining two may be at any subsequent CS.
I just found out that this patch only takes effect *after* the SPI slaves
have been probed. That's because the HW CS signal selection takes place
after a call to spi_register_master/spi_register_controller which adds the
SPI clients on its own. I now also understand why MSIOF discovers the CS in
such a complicated manner. Sorry. I wrote this patch for spidev where it is
not a problem, so I haven't seen this before.
I see three options now:
1) Add an explicit DT property to let the user select an appropriate HW CS
signal. This has an advantage of being able to handle DT overlays in
future, as Trent pointed out.
2) Split the spi_register_controller into two functions and let
orion_spi_probe access the cs_gpios in between them. No other drivers need
to be modified, but perhaps the MSIOF might switch to using these two
halves afterwards.
3) Follow the code from MSIOF, consult the "cs" gpios property, manually
walk the CS GPIOs, and find out a free HW CS.
I've already sorted these in my order of preference :). Which one is
preferrable for you folks?
Looking further, it seems that the CS GPIO signals are first manipulated
when probing for the corresponding client device. Depending on a platform
and its choice of a default value for the corresponding GPIO pin, this
might mean that a GPIO CS only gets initialized to inactive ("high" in
default SPI settings) too late. Other devices might have been already
probed for, and the CS GPIO of the "next" device might have been held
active ("low") during that time. How should I fix that?
With kind regards,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2018-02-20 at 20:02 +0100, Jan Kundrát wrote: > I just found out that this patch only takes effect *after* the SPI slaves > have been probed. That's because the HW CS signal selection takes place > after a call to spi_register_master/spi_register_controller which adds the > SPI clients on its own. I now also understand why MSIOF discovers the CS in > such a complicated manner. Sorry. I wrote this patch for spidev where it is > not a problem, so I haven't seen this before. > > I see three options now: > > 1) Add an explicit DT property to let the user select an appropriate HW CS > signal. This has an advantage of being able to handle DT overlays in > future, as Trent pointed out. You could also use a modulo scheme to allow specifying the hw cs to use without adding a new property. > 2) Split the spi_register_controller into two functions and let > orion_spi_probe access the cs_gpios in between them. No other drivers need > to be modified, but perhaps the MSIOF might switch to using these two > halves afterwards. > > 3) Follow the code from MSIOF, consult the "cs" gpios property, manually > walk the CS GPIOs, and find out a free HW CS. 4) Try to determine the HW CS to use at spi_setup() time. This way the master should know about all _probed_ spi slaves already. It does not work if the kernel does not know about a device that is attached to a HW CS. > Looking further, it seems that the CS GPIO signals are first manipulated > when probing for the corresponding client device. Depending on a platform > and its choice of a default value for the corresponding GPIO pin, this > might mean that a GPIO CS only gets initialized to inactive ("high" in > default SPI settings) too late. Other devices might have been already > probed for, and the CS GPIO of the "next" device might have been held > active ("low") during that time. How should I fix that? Design the hardware so the GPIOs are high impedance out of reset and put a weak pullup on each chip select line. If you look at spi-imx or spi-dw-mmio, they request all GPIOs at the time the master is probed. They should probably also set them to de- asserted outputs at this time too. Other drivers, like spi-sirf, request the gpio in spi_setup(), which is IMHO flawed and they should not do that.
On Wed, Feb 21, 2018 at 7:58 PM, Trent Piepho <tpiepho@impinj.com> wrote: > On Tue, 2018-02-20 at 20:02 +0100, Jan Kundrát wrote: >> Looking further, it seems that the CS GPIO signals are first manipulated >> when probing for the corresponding client device. Depending on a platform >> and its choice of a default value for the corresponding GPIO pin, this >> might mean that a GPIO CS only gets initialized to inactive ("high" in >> default SPI settings) too late. Other devices might have been already >> probed for, and the CS GPIO of the "next" device might have been held >> active ("low") during that time. How should I fix that? > > Design the hardware so the GPIOs are high impedance out of reset and > put a weak pullup on each chip select line. Most (all?) GPIO controllers come up in input mode, to avoid line conflicts. So adding weak pull-up should be fine. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On středa 21. února 2018 19:58:25 CET, Trent Piepho wrote: > On Tue, 2018-02-20 at 20:02 +0100, Jan Kundrát wrote: >> I just found out that this patch only takes effect *after* the SPI slaves >> have been probed. That's because the HW CS signal selection takes place >> after a call to spi_register_master/spi_register_controller >> which adds the >> SPI clients on its own. I now also understand why MSIOF >> discovers the CS in >> such a complicated manner. Sorry. I wrote this patch for >> spidev where it is >> not a problem, so I haven't seen this before. >> >> I see three options now: >> >> 1) Add an explicit DT property to let the user select an >> appropriate HW CS >> signal. This has an advantage of being able to handle DT overlays in >> future, as Trent pointed out. > > You could also use a modulo scheme to allow specifying the hw cs to use > without adding a new property. Ah, right, I got your idea now. You're suggesting to simply use a logical_cs % number_of_hw_cs, and mandating the DT designer to come up with "proper" CS assignment. So if my device has four native CSes, one of which is unused and all others takes, and I need to add three more SPI slaves on a GPIO CS, I could do it like this: cs_gpios = <0>, <&gpio0 1>, <0>, <0>, <0>, <gpio0 2>, <0>, <0>, <0>, <gpio0 3>, <0>, <0>; To be honest, I find this rather inconvenient for a DT writer. This behavior would express a driver's implementation detail ("we're using modulos for finding a fake HW CS") into the DT. It would also need driver changes because not every driver supports this. For example, my silicon supports four native CSes, but the spi-orion driver doesn't know that and instead assumes a default of 8 CSes. > 4) Try to determine the HW CS to use at spi_setup() time. This way > the master should know about all _probed_ spi slaves already. It does > not work if the kernel does not know about a device that is attached to > a HW CS. I don't think that would work. The slaves are probed one-after-another, they can issue SPI transactions during the probe, and while probing GPIO CS#1, we have no idea whether HW CS#2 is used or not. A simple algorithm would therefore end up using a "free HW GPIO" without knowing that it's in fact used by a device which hasn't been probed yet. At best, we would get bus clashes, and at worst, a dead HW due to push-pull outputs working against each other. > Design the hardware so the GPIOs are high impedance out of reset and > put a weak pullup on each chip select line. "I cannot do that." My platform is very limited in terms of spare GPIOs, and the designers added a dual-use function to them. Some of the GPIOs are sampled at power-on-reset to configure the CPU's clock frequencies, interrupt vector instruction encoding, and boot device selection. I just *cannot* add pull-ups I would like to add. I could add physical inverters and use active-high CS pins, that's true. But that only works because we're still prototyping the expansion boards. It feels wrong to add extra silicon just because I cannot configure a generic OS to properly initialize GPIO CS pins. I would much prefer the Linux SPI drivers to initialize all CS pins prior to issuing SPI transfers to any device. With kind regards, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt index 8434a65fc12a..c7027af04fb2 100644 --- a/Documentation/devicetree/bindings/spi/spi-orion.txt +++ b/Documentation/devicetree/bindings/spi/spi-orion.txt @@ -28,6 +28,11 @@ Optional properties: - clock-names : names of used clocks, mandatory if the second clock is used, the name must be "core", and "axi" (the latter is only for Armada 7K/8K). +- orion-spi,cs-gpio-hw-cs : Because the SoC always wants to drive some HW Chip + Select pin, it is necessary to specify which one + should be used when Linux drives an additional + GPIO as a software-controlled CS. Defaults to HW + CS 0. Example: @@ -74,6 +79,23 @@ are used in the default indirect (PIO) mode): <MBUS_ID(0x01, 0x5e) 0 0 0xf1100000 0x10000>, /* SPI0-DEV1 */ <MBUS_ID(0x01, 0x9a) 0 0 0xf1110000 0x10000>; /* SPI1-DEV2 */ + +Example with a GPIO CS to be driven by software in addition to HW CS3. This +can be used when the board or the pinmux does not allow connections to HW CS +pins, for example: + + spi0: spi@10600 { + /* ... */ + cs-gpios = <0>, <&gpio0 22 GPIO_ACTIVE_HIGH>, <0>; + orion-spi,cs-gpio-hw-cs = <3>; + + something@1 { + reg = <1>; + /* this device should be now connected to a custom GPIO CS */ + }; + } + + For further information on the MBus bindings, please see the MBus DT documentation: Documentation/devicetree/bindings/bus/mvebu-mbus.txt diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c index 482a0cf3b7aa..b1b0537b4450 100644 --- a/drivers/spi/spi-orion.c +++ b/drivers/spi/spi-orion.c @@ -96,6 +96,7 @@ struct orion_spi { struct clk *clk; struct clk *axi_clk; const struct orion_spi_dev *devdata; + int cs_hw_gpio; struct orion_direct_acc direct_access[ORION_NUM_CHIPSELECTS]; }; @@ -324,13 +325,13 @@ static void orion_spi_set_cs(struct spi_device *spi, bool enable) struct orion_spi *orion_spi; int cs; + orion_spi = spi_master_get_devdata(spi->master); + if (gpio_is_valid(spi->cs_gpio)) - cs = 0; + cs = orion_spi->cs_hw_gpio; else cs = spi->chip_select; - orion_spi = spi_master_get_devdata(spi->master); - orion_spi_clrbits(orion_spi, ORION_SPI_IF_CTRL_REG, ORION_SPI_CS_MASK); orion_spi_setbits(orion_spi, ORION_SPI_IF_CTRL_REG, ORION_SPI_CS(cs)); @@ -645,6 +646,9 @@ static int orion_spi_probe(struct platform_device *pdev) tclk_hz = clk_get_rate(spi->clk); + if (!pdev->dev.of_node || of_property_read_u32(pdev->dev.of_node, "orion-spi,cs-gpio-hw-cs", &spi->cs_hw_gpio)) + spi->cs_hw_gpio = 0; + /* * With old device tree, armada-370-spi could be used with * Armada XP, however for this SoC the maximum frequency is
Commit b28b9149b37f added support for using an additional GPIO pin for Chip Select. According to that commit and to my understanding of a semi-public datahseet, it seems that the SPI hardware really insists on driving *some* HW CS signal whenever a SPI transaction is in progress. The old code effectively forced the HW CS pin to be CS0. That means that there could not be anything connected to the real CS0 signal when one uses a GPIO CS. That assumption does not hold on e.g. Solidrun Clearfog where some SOM models have a built-in SPI flash on SPI1, CS0. Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz> --- .../devicetree/bindings/spi/spi-orion.txt | 22 ++++++++++++++++++++++ drivers/spi/spi-orion.c | 10 +++++++--- 2 files changed, 29 insertions(+), 3 deletions(-)