| Message ID | 20210204204904.294555-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS | expand |
On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote: > GENI SPI controller shows several issues if it manages the CS on its own > (see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to > use GPIO for CS")) for the details. Provide pinctrl entries for SPI > controllers using the same CS pin but in GPIO mode. > Doug, can you confirm that this is the final (or at least current) verdict? > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++ > 1 file changed, 380 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 3cea28058a91..03015174ec06 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -3046,6 +3046,25 @@ config { > }; > }; > > + qup_spi0_cs_gpio: qup-spi0-cs-gpio { There might be others who need the same states, but I would prefer if we move this to the device's dts. > + mux { Rather than splitting the properties in {mux, cs, config} I think it makes more sense to split them in {spi, cs} or something like that. Regards, Bjorn
Hi, On Thu, Feb 4, 2021 at 3:07 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote: > > > GENI SPI controller shows several issues if it manages the CS on its own > > (see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to > > use GPIO for CS")) for the details. Provide pinctrl entries for SPI > > controllers using the same CS pin but in GPIO mode. I'm curious: were you seeing real problems or are you just trying to optimize things more? The only known problem (other than performance) that I'm aware of is that if you get really high interrupt latency then setting the chip select can appear to "fail" (we timeout waiting for the interrupt saying that the chip select command was done). The SPI framework doesn't expect setting the chip select to be something that can fail so error handling isn't the most amazing in this case, though at least it shouldn't crash after the most recent fixes I sent to the SPI driver. > Doug, can you confirm that this is the final (or at least current) > verdict? As far as I know using a GPIO chip select is always superior on Qualcomm hardware unless you're forced into GPI/GSI mode. In GPI/GSI mode (I think) you lose full control of the chip select anyway. I wrote this long-winded explanation recently to explain it to someone else. Pasting here in case it's useful, though there's overlap w/ the commit message that Dmitry pointed at. --- In general the Linux kernel supports using any GPIO as a SPI chip select. However, it also supports the concept that a SPI controller may have its own way of controlling chip select. You can freely mix and match these approaches. For instance maybe you've got 4 devices on a single SPI "bus" (we never do this on Chrome OS designs but it is possible). The SPI controller itself might have the ability to control two chip selects. You could still make things work by hooking two of the peripherals up to GPIOs and two up to the SPI controller's native chip selects and it'd all be hunky dory. Or you could choose to not use any of the builtin chip selects and use all 4 on GPIOs. ...or 3 and 1. When a SPI controller has a builtin chip select, it can usually be configured in two ways: something more automatic where the controller asserts the chip select and deasserts it automatically around transfers or in a fully manual mode. In general Linux prefers the fully manual mode. The Linux API to SPI endpoint drivers is super flexible and allows them to assert/deassert chip select at will and it's hard to honor that flexibility when it's not manually controlled. If a SPI chip select is manually controlled it's not at all different from a GPIO configured in "output mode". Thus using a GPIO instead of a builtin chip select has no real downsides. On many SoCs, sc7180 included, pinmuxing allows you to reconfigure almost any pin as a GPIO. So it turns out that on sc7180 boards it's just a different way of looking at it to say whether we're hooked up to a GPIO or we're hooked up to the native chip select logic. Both are valid ways to look at it. On Qualcomm SoCs it's incredibly inefficient to control the native SPI chip select in "manual mode". It involves sending a packet (a command) to the SPI controller and waiting for it to respond that it set the chip select. However, it is plenty efficient to control GPIOs. Thus it is more efficient (with no real downsides) to envision that the chip select is hooked up to a GPIO instead of the native chip select. --- > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++ > > 1 file changed, 380 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > index 3cea28058a91..03015174ec06 100644 > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > @@ -3046,6 +3046,25 @@ config { > > }; > > }; > > > > + qup_spi0_cs_gpio: qup-spi0-cs-gpio { > > There might be others who need the same states, but I would prefer if we > move this to the device's dts. This is opposite to what Stephen requested, though it was in a review on our gerrit and not on lists [1]. :-P It definitely feels like a 6 of one half dozen of the other. Unless you're dead set on moving them to the board dts my bias would be towards keeping consistent with what was done on sc7180. If you really want this moved to the board file we should do it for sc7180 too. > > + mux { > > Rather than splitting the properties in {mux, cs, config} I think it > makes more sense to split them in {spi, cs} or something like that. In general pinconf doesn't belong in the SoC dts file. If there's no reason to change it seems like this should match what sc7180 did. [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2406557/1/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
On 05/02/2021 02:31, Doug Anderson wrote: > Hi, > > On Thu, Feb 4, 2021 at 3:07 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: >> >> On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote: >> >>> GENI SPI controller shows several issues if it manages the CS on its own >>> (see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to >>> use GPIO for CS")) for the details. Provide pinctrl entries for SPI >>> controllers using the same CS pin but in GPIO mode. > > I'm curious: were you seeing real problems or are you just trying to > optimize things more? The only known problem (other than performance) > that I'm aware of is that if you get really high interrupt latency > then setting the chip select can appear to "fail" (we timeout waiting > for the interrupt saying that the chip select command was done). The > SPI framework doesn't expect setting the chip select to be something > that can fail so error handling isn't the most amazing in this case, > though at least it shouldn't crash after the most recent fixes I sent > to the SPI driver. I have been observing strange behaviour from the SPI CAN interface. This change allowed me to narrow down the issue (with the GPI support for SPI) we had in our tree. [skipped the explanation. >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++ >>> 1 file changed, 380 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi >>> index 3cea28058a91..03015174ec06 100644 >>> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi >>> @@ -3046,6 +3046,25 @@ config { >>> }; >>> }; >>> >>> + qup_spi0_cs_gpio: qup-spi0-cs-gpio { >> >> There might be others who need the same states, but I would prefer if we >> move this to the device's dts. > > This is opposite to what Stephen requested, though it was in a review > on our gerrit and not on lists [1]. :-P > > It definitely feels like a 6 of one half dozen of the other. Unless > you're dead set on moving them to the board dts my bias would be > towards keeping consistent with what was done on sc7180. If you > really want this moved to the board file we should do it for sc7180 > too. > > >>> + mux { >> >> Rather than splitting the properties in {mux, cs, config} I think it >> makes more sense to split them in {spi, cs} or something like that. That was my original idea, with qup-spi-nocs config being in sm8250.dtsi and spi0_cs (defining only CS) belonging to the board dts. > > In general pinconf doesn't belong in the SoC dts file. If there's no > reason to change it seems like this should match what sc7180 did. > > > [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2406557/1/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi -- With best wishes Dmitry
On Thu 04 Feb 17:31 CST 2021, Doug Anderson wrote: > On Thu, Feb 4, 2021 at 3:07 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote: [..] > > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > > index 3cea28058a91..03015174ec06 100644 > > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > > @@ -3046,6 +3046,25 @@ config { > > > }; > > > }; > > > > > > + qup_spi0_cs_gpio: qup-spi0-cs-gpio { > > > > There might be others who need the same states, but I would prefer if we > > move this to the device's dts. > > This is opposite to what Stephen requested, though it was in a review > on our gerrit and not on lists [1]. :-P > > It definitely feels like a 6 of one half dozen of the other. Unless > you're dead set on moving them to the board dts my bias would be > towards keeping consistent with what was done on sc7180. If you > really want this moved to the board file we should do it for sc7180 > too. > What I dislike is the fact that we have a huge amount of these unused in the platform.dtsi, but let's align with what I agreed to on sc7180...Sorry for the short memory. > > > > + mux { > > > > Rather than splitting the properties in {mux, cs, config} I think it > > makes more sense to split them in {spi, cs} or something like that. > > In general pinconf doesn't belong in the SoC dts file. If there's no > reason to change it seems like this should match what sc7180 did. > Right, but I still would prefer the pinctrl state to be split by function/pins, rather than pinmux vs pinconf. That way it's natural to add pinconf properties to the various functional parts (i.e. bias or drive-strength for the spi pins vs cs). Do you have any concerns with this? Regards, Bjorn > > [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2406557/1/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
Hi, On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > > > > + mux { > > > > > > Rather than splitting the properties in {mux, cs, config} I think it > > > makes more sense to split them in {spi, cs} or something like that. > > > > In general pinconf doesn't belong in the SoC dts file. If there's no > > reason to change it seems like this should match what sc7180 did. > > > > Right, but I still would prefer the pinctrl state to be split by > function/pins, rather than pinmux vs pinconf. That way it's natural to > add pinconf properties to the various functional parts (i.e. bias or > drive-strength for the spi pins vs cs). > > Do you have any concerns with this? I read this a few times and I'm not exactly sure what you're proposing. Can you provide an example of what you want it to look like in this case? -Doug
On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote: > Hi, > > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > > > > + mux { > > > > > > > > Rather than splitting the properties in {mux, cs, config} I think it > > > > makes more sense to split them in {spi, cs} or something like that. > > > > > > In general pinconf doesn't belong in the SoC dts file. If there's no > > > reason to change it seems like this should match what sc7180 did. > > > > > > > Right, but I still would prefer the pinctrl state to be split by > > function/pins, rather than pinmux vs pinconf. That way it's natural to > > add pinconf properties to the various functional parts (i.e. bias or > > drive-strength for the spi pins vs cs). > > > > Do you have any concerns with this? > > I read this a few times and I'm not exactly sure what you're > proposing. Can you provide an example of what you want it to look > like in this case? > Today in most cases we group pinctrl properties by being a "conf" of a "mux" property, so we end up with: the_state: spi-state { all-the-mux-properties { pins = "gpio40", gpio41", "gpio42", "gpio43"; function = qup14"; }; repeat-pins-and-add-all-conf-properties { pins = "gpio40", gpio41", "gpio42", "gpio43"; drive-strength = <6>; bias-disable; }; }; This made sense to me after implementing the driver, there's muxing to be done and there's electrical configuration to configure. But what's actually trying to describe is a hardware state; i.e. that miso, mosi, clk and cs should be acting in a particular fashion. In particular this lends itself useful when the hardware state consists of different functions, a good example being the Bluetooth UART, or in the SPI-with-separate-GPIO: the_state: spi-state { miso-mosi-clk { pins = "gpio40", gpio41", "gpio42" function = qup14"; drive-strength = <6>; bias-disable; }; cs { pins = "gpio43"; function = "gpio"; drive-strength = <6>; bias-disable; }; }; For the case of uniform configuration across the state we've come to sprinkle a few different synonyms for "pinconf" and "pinmux" in the state nodes. But a few years ago someone updated the state parser to handle cases either directly in the state or in subnodes. So we can avoid these boilerplate nodes with a simple: the_state: spi-state { pins = "gpio40", gpio41", "gpio42", "gpio43"; function = qup14"; drive-strength = <6>; bias-disable; }; Regards, Bjorn
Hi, On Fri, Feb 5, 2021 at 8:48 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote: > > > Hi, > > > > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > + mux { > > > > > > > > > > Rather than splitting the properties in {mux, cs, config} I think it > > > > > makes more sense to split them in {spi, cs} or something like that. > > > > > > > > In general pinconf doesn't belong in the SoC dts file. If there's no > > > > reason to change it seems like this should match what sc7180 did. > > > > > > > > > > Right, but I still would prefer the pinctrl state to be split by > > > function/pins, rather than pinmux vs pinconf. That way it's natural to > > > add pinconf properties to the various functional parts (i.e. bias or > > > drive-strength for the spi pins vs cs). > > > > > > Do you have any concerns with this? > > > > I read this a few times and I'm not exactly sure what you're > > proposing. Can you provide an example of what you want it to look > > like in this case? > > > > Today in most cases we group pinctrl properties by being a "conf" of a > "mux" property, so we end up with: > > the_state: spi-state { > all-the-mux-properties { > pins = "gpio40", gpio41", "gpio42", "gpio43"; > function = qup14"; > }; > > repeat-pins-and-add-all-conf-properties { > pins = "gpio40", gpio41", "gpio42", "gpio43"; > drive-strength = <6>; > bias-disable; > }; > }; > > This made sense to me after implementing the driver, there's muxing to > be done and there's electrical configuration to configure. > > But what's actually trying to describe is a hardware state; i.e. that > miso, mosi, clk and cs should be acting in a particular fashion. > > In particular this lends itself useful when the hardware state consists > of different functions, a good example being the Bluetooth UART, or in > the SPI-with-separate-GPIO: > > the_state: spi-state { > miso-mosi-clk { > pins = "gpio40", gpio41", "gpio42" > function = qup14"; > drive-strength = <6>; > bias-disable; > }; > > cs { > pins = "gpio43"; > function = "gpio"; > drive-strength = <6>; > bias-disable; > }; > }; > > > For the case of uniform configuration across the state we've come to > sprinkle a few different synonyms for "pinconf" and "pinmux" in the > state nodes. But a few years ago someone updated the state parser to > handle cases either directly in the state or in subnodes. So we can > avoid these boilerplate nodes with a simple: > > the_state: spi-state { > pins = "gpio40", gpio41", "gpio42", "gpio43"; > function = qup14"; > drive-strength = <6>; > bias-disable; > }; OK, this makes sense to me. I always felt like the extra "pinconf" / "pinmux" made things awkward. I guess someone should try to convert some SoC dtsi fully over so we can see how it looks? In this case I think you'd have something like this, right? SoC dtsi: tlmm: pinctrl@... { qup_spi0_data_clk: qup-spi0-data-clk { pins = "gpio28", "gpio29", "gpio30"; function = "qup0"; }; qup_spi0_cs: qup-spi0-cs { pins = "gpio31"; function = "qup0"; }; qup_spi0_cs_gpio: qup-spi0-cs-gpio { pins = "gpio31"; function = "gpio"; }; }; Board dts: &spi0 { pinctrl-0 = <&qup_spi0_data_clk>, &<qup_spi0_cs_gpio>; }; &qup_spi0_data_clk { drive-strength = <6>; bias-disable; }; &qup_spi0_cs_gpio { drive-strength = <6>; bias-disable; };
On Mon 08 Feb 09:58 CST 2021, Doug Anderson wrote: > Hi, > > On Fri, Feb 5, 2021 at 8:48 AM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote: > > > > > Hi, > > > > > > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > + mux { > > > > > > > > > > > > Rather than splitting the properties in {mux, cs, config} I think it > > > > > > makes more sense to split them in {spi, cs} or something like that. > > > > > > > > > > In general pinconf doesn't belong in the SoC dts file. If there's no > > > > > reason to change it seems like this should match what sc7180 did. > > > > > > > > > > > > > Right, but I still would prefer the pinctrl state to be split by > > > > function/pins, rather than pinmux vs pinconf. That way it's natural to > > > > add pinconf properties to the various functional parts (i.e. bias or > > > > drive-strength for the spi pins vs cs). > > > > > > > > Do you have any concerns with this? > > > > > > I read this a few times and I'm not exactly sure what you're > > > proposing. Can you provide an example of what you want it to look > > > like in this case? > > > > > > > Today in most cases we group pinctrl properties by being a "conf" of a > > "mux" property, so we end up with: > > > > the_state: spi-state { > > all-the-mux-properties { > > pins = "gpio40", gpio41", "gpio42", "gpio43"; > > function = qup14"; > > }; > > > > repeat-pins-and-add-all-conf-properties { > > pins = "gpio40", gpio41", "gpio42", "gpio43"; > > drive-strength = <6>; > > bias-disable; > > }; > > }; > > > > This made sense to me after implementing the driver, there's muxing to > > be done and there's electrical configuration to configure. > > > > But what's actually trying to describe is a hardware state; i.e. that > > miso, mosi, clk and cs should be acting in a particular fashion. > > > > In particular this lends itself useful when the hardware state consists > > of different functions, a good example being the Bluetooth UART, or in > > the SPI-with-separate-GPIO: > > > > the_state: spi-state { > > miso-mosi-clk { > > pins = "gpio40", gpio41", "gpio42" > > function = qup14"; > > drive-strength = <6>; > > bias-disable; > > }; > > > > cs { > > pins = "gpio43"; > > function = "gpio"; > > drive-strength = <6>; > > bias-disable; > > }; > > }; > > > > > > For the case of uniform configuration across the state we've come to > > sprinkle a few different synonyms for "pinconf" and "pinmux" in the > > state nodes. But a few years ago someone updated the state parser to > > handle cases either directly in the state or in subnodes. So we can > > avoid these boilerplate nodes with a simple: > > > > the_state: spi-state { > > pins = "gpio40", gpio41", "gpio42", "gpio43"; > > function = qup14"; > > drive-strength = <6>; > > bias-disable; > > }; > > OK, this makes sense to me. I always felt like the extra "pinconf" / > "pinmux" made things awkward. I'm happy to hear that :) > I guess someone should try to convert some SoC dtsi fully over so we > can see how it looks? Sounds good. I feel fairly confident, so let's pick SM8250 and aim to land this patch in the "new" form. > In this case I think you'd have something like this, right? > > SoC dtsi: > > tlmm: pinctrl@... { > qup_spi0_data_clk: qup-spi0-data-clk { > pins = "gpio28", "gpio29", "gpio30"; > function = "qup0"; > }; > > qup_spi0_cs: qup-spi0-cs { > pins = "gpio31"; > function = "qup0"; > }; > > qup_spi0_cs_gpio: qup-spi0-cs-gpio { > pins = "gpio31"; > function = "gpio"; > }; > }; > > Board dts: > > &spi0 { > pinctrl-0 = <&qup_spi0_data_clk>, &<qup_spi0_cs_gpio>; > }; > > &qup_spi0_data_clk { > drive-strength = <6>; > bias-disable; > }; > > &qup_spi0_cs_gpio { > drive-strength = <6>; > bias-disable; > }; Correct. And providing a common state for the 3 non-cs pins and using the pinctrl-0 to select which kind of cs we want looks even better. Regards, Bjorn
On Mon, 8 Feb 2021 at 21:04, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Mon 08 Feb 09:58 CST 2021, Doug Anderson wrote: > > > Hi, > > > > On Fri, Feb 5, 2021 at 8:48 AM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote: > > > > > > > Hi, > > > > > > > > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > > > + mux { > > > > > > > > > > > > > > Rather than splitting the properties in {mux, cs, config} I think it > > > > > > > makes more sense to split them in {spi, cs} or something like that. > > > > > > > > > > > > In general pinconf doesn't belong in the SoC dts file. If there's no > > > > > > reason to change it seems like this should match what sc7180 did. > > > > > > > > > > > > > > > > Right, but I still would prefer the pinctrl state to be split by > > > > > function/pins, rather than pinmux vs pinconf. That way it's natural to > > > > > add pinconf properties to the various functional parts (i.e. bias or > > > > > drive-strength for the spi pins vs cs). > > > > > > > > > > Do you have any concerns with this? > > > > > > > > I read this a few times and I'm not exactly sure what you're > > > > proposing. Can you provide an example of what you want it to look > > > > like in this case? > > > > > > > > > > Today in most cases we group pinctrl properties by being a "conf" of a > > > "mux" property, so we end up with: > > > > > > the_state: spi-state { > > > all-the-mux-properties { > > > pins = "gpio40", gpio41", "gpio42", "gpio43"; > > > function = qup14"; > > > }; > > > > > > repeat-pins-and-add-all-conf-properties { > > > pins = "gpio40", gpio41", "gpio42", "gpio43"; > > > drive-strength = <6>; > > > bias-disable; > > > }; > > > }; > > > > > > This made sense to me after implementing the driver, there's muxing to > > > be done and there's electrical configuration to configure. > > > > > > But what's actually trying to describe is a hardware state; i.e. that > > > miso, mosi, clk and cs should be acting in a particular fashion. > > > > > > In particular this lends itself useful when the hardware state consists > > > of different functions, a good example being the Bluetooth UART, or in > > > the SPI-with-separate-GPIO: > > > > > > the_state: spi-state { > > > miso-mosi-clk { > > > pins = "gpio40", gpio41", "gpio42" > > > function = qup14"; > > > drive-strength = <6>; > > > bias-disable; > > > }; > > > > > > cs { > > > pins = "gpio43"; > > > function = "gpio"; > > > drive-strength = <6>; > > > bias-disable; > > > }; > > > }; > > > > > > > > > For the case of uniform configuration across the state we've come to > > > sprinkle a few different synonyms for "pinconf" and "pinmux" in the > > > state nodes. But a few years ago someone updated the state parser to > > > handle cases either directly in the state or in subnodes. So we can > > > avoid these boilerplate nodes with a simple: > > > > > > the_state: spi-state { > > > pins = "gpio40", gpio41", "gpio42", "gpio43"; > > > function = qup14"; > > > drive-strength = <6>; > > > bias-disable; > > > }; > > > > OK, this makes sense to me. I always felt like the extra "pinconf" / > > "pinmux" made things awkward. > > I'm happy to hear that :) > > > I guess someone should try to convert some SoC dtsi fully over so we > > can see how it looks? > > Sounds good. I feel fairly confident, so let's pick SM8250 and aim to > land this patch in the "new" form. OK. As a starting bit I will convert SPI pinctrl for now with the rest of pins to follow. > > > In this case I think you'd have something like this, right? > > > > SoC dtsi: > > > > tlmm: pinctrl@... { > > qup_spi0_data_clk: qup-spi0-data-clk { > > pins = "gpio28", "gpio29", "gpio30"; > > function = "qup0"; > > }; > > > > qup_spi0_cs: qup-spi0-cs { > > pins = "gpio31"; > > function = "qup0"; > > }; > > > > qup_spi0_cs_gpio: qup-spi0-cs-gpio { > > pins = "gpio31"; > > function = "gpio"; > > }; > > }; > > > > Board dts: > > > > &spi0 { > > pinctrl-0 = <&qup_spi0_data_clk>, &<qup_spi0_cs_gpio>; > > }; > > > > &qup_spi0_data_clk { > > drive-strength = <6>; > > bias-disable; > > }; > > > > &qup_spi0_cs_gpio { > > drive-strength = <6>; > > bias-disable; > > }; > > Correct. And providing a common state for the 3 non-cs pins and using > the pinctrl-0 to select which kind of cs we want looks even better. > > Regards, > Bjorn -- With best wishes Dmitry
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi index 3cea28058a91..03015174ec06 100644 --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi @@ -3046,6 +3046,25 @@ config { }; }; + qup_spi0_cs_gpio: qup-spi0-cs-gpio { + mux { + pins = "gpio28", "gpio29", + "gpio30"; + function = "qup0"; + }; + + cs { + pins = "gpio31"; + function = "gpio"; + }; + + config { + pins = "gpio28", "gpio29", "gpio30", "gpio31"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi0_default: qup-spi0-default { mux { pins = "gpio28", "gpio29", @@ -3061,6 +3080,25 @@ config { }; }; + qup_spi1_cs_gpio: qup-spi1-cs-gpio { + mux { + pins = "gpio4", "gpio5", + "gpio6"; + function = "qup1"; + }; + + cs { + pins = "gpio7"; + function = "gpio"; + }; + + config { + pins = "gpio4", "gpio5", "gpio6", "gpio7"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi1_default: qup-spi1-default { mux { pins = "gpio4", "gpio5", @@ -3076,6 +3114,25 @@ config { }; }; + qup_spi2_cs_gpio: qup-spi2-cs-gpio { + mux { + pins = "gpio115", "gpio116", + "gpio117"; + function = "qup2"; + }; + + cs { + pins = "gpio118"; + function = "gpio"; + }; + + config { + pins = "gpio115", "gpio116", "gpio117", "gpio118"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi2_default: qup-spi2-default { mux { pins = "gpio115", "gpio116", @@ -3091,6 +3148,25 @@ config { }; }; + qup_spi3_cs_gpio: qup-spi3-cs-gpio { + mux { + pins = "gpio119", "gpio120", + "gpio121"; + function = "qup3"; + }; + + cs { + pins = "gpio122"; + function = "gpio"; + }; + + config { + pins = "gpio119", "gpio120", "gpio121", "gpio122"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi3_default: qup-spi3-default { mux { pins = "gpio119", "gpio120", @@ -3106,6 +3182,25 @@ config { }; }; + qup_spi4_cs_gpio: qup-spi4-cs-gpio { + mux { + pins = "gpio8", "gpio9", + "gpio10"; + function = "qup4"; + }; + + cs { + pins = "gpio11"; + function = "gpio"; + }; + + config { + pins = "gpio8", "gpio9", "gpio10", "gpio11"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi4_default: qup-spi4-default { mux { pins = "gpio8", "gpio9", @@ -3121,6 +3216,25 @@ config { }; }; + qup_spi5_cs_gpio: qup-spi5-cs-gpio { + mux { + pins = "gpio12", "gpio13", + "gpio14"; + function = "qup5"; + }; + + cs { + pins = "gpio15"; + function = "gpio"; + }; + + config { + pins = "gpio12", "gpio13", "gpio14", "gpio15"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi5_default: qup-spi5-default { mux { pins = "gpio12", "gpio13", @@ -3136,6 +3250,25 @@ config { }; }; + qup_spi6_cs_gpio: qup-spi6-cs-gpio { + mux { + pins = "gpio16", "gpio17", + "gpio18"; + function = "qup6"; + }; + + cs { + pins = "gpio19"; + function = "gpio"; + }; + + config { + pins = "gpio16", "gpio17", "gpio18", "gpio19"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi6_default: qup-spi6-default { mux { pins = "gpio16", "gpio17", @@ -3151,6 +3284,25 @@ config { }; }; + qup_spi7_cs_gpio: qup-spi7-cs-gpio { + mux { + pins = "gpio20", "gpio21", + "gpio22"; + function = "qup7"; + }; + + cs { + pins = "gpio23"; + function = "gpio"; + }; + + config { + pins = "gpio20", "gpio21", "gpio22", "gpio23"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi7_default: qup-spi7-default { mux { pins = "gpio20", "gpio21", @@ -3166,6 +3318,25 @@ config { }; }; + qup_spi8_cs_gpio: qup-spi8-cs-gpio { + mux { + pins = "gpio24", "gpio25", + "gpio26"; + function = "qup8"; + }; + + cs { + pins = "gpio27"; + function = "gpio"; + }; + + config { + pins = "gpio24", "gpio25", "gpio26", "gpio27"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi8_default: qup-spi8-default { mux { pins = "gpio24", "gpio25", @@ -3181,6 +3352,25 @@ config { }; }; + qup_spi9_cs_gpio: qup-spi9-cs-gpio { + mux { + pins = "gpio125", "gpio126", + "gpio127"; + function = "qup9"; + }; + + cs { + pins = "gpio128"; + function = "gpio"; + }; + + config { + pins = "gpio125", "gpio126", "gpio127", "gpio128"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi9_default: qup-spi9-default { mux { pins = "gpio125", "gpio126", @@ -3196,6 +3386,25 @@ config { }; }; + qup_spi10_cs_gpio: qup-spi10-cs-gpio { + mux { + pins = "gpio129", "gpio130", + "gpio131"; + function = "qup10"; + }; + + cs { + pins = "gpio132"; + function = "gpio"; + }; + + config { + pins = "gpio129", "gpio130", "gpio131", "gpio132"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi10_default: qup-spi10-default { mux { pins = "gpio129", "gpio130", @@ -3211,6 +3420,25 @@ config { }; }; + qup_spi11_cs_gpio: qup-spi11-cs-gpio { + mux { + pins = "gpio60", "gpio61", + "gpio62"; + function = "qup11"; + }; + + cs { + pins = "gpio63"; + function = "gpio"; + }; + + config { + pins = "gpio60", "gpio61", "gpio62", "gpio63"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi11_default: qup-spi11-default { mux { pins = "gpio60", "gpio61", @@ -3226,6 +3454,25 @@ config { }; }; + qup_spi12_cs_gpio: qup-spi12-cs-gpio { + mux { + pins = "gpio32", "gpio33", + "gpio34"; + function = "qup12"; + }; + + cs { + pins = "gpio35"; + function = "gpio"; + }; + + config { + pins = "gpio32", "gpio33", "gpio34", "gpio35"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi12_default: qup-spi12-default { mux { pins = "gpio32", "gpio33", @@ -3241,6 +3488,25 @@ config { }; }; + qup_spi13_cs_gpio: qup-spi13-cs-gpio { + mux { + pins = "gpio36", "gpio37", + "gpio38"; + function = "qup13"; + }; + + cs { + pins = "gpio39"; + function = "gpio"; + }; + + config { + pins = "gpio36", "gpio37", "gpio38", "gpio39"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi13_default: qup-spi13-default { mux { pins = "gpio36", "gpio37", @@ -3256,6 +3522,25 @@ config { }; }; + qup_spi14_cs_gpio: qup-spi14-cs-gpio { + mux { + pins = "gpio40", "gpio41", + "gpio42"; + function = "qup14"; + }; + + cs { + pins = "gpio43"; + function = "gpio"; + }; + + config { + pins = "gpio40", "gpio41", "gpio42", "gpio43"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi14_default: qup-spi14-default { mux { pins = "gpio40", "gpio41", @@ -3271,6 +3556,25 @@ config { }; }; + qup_spi15_cs_gpio: qup-spi15-cs-gpio { + mux { + pins = "gpio44", "gpio45", + "gpio46"; + function = "qup15"; + }; + + cs { + pins = "gpio47"; + function = "gpio"; + }; + + config { + pins = "gpio44", "gpio45", "gpio46", "gpio47"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi15_default: qup-spi15-default { mux { pins = "gpio44", "gpio45", @@ -3286,6 +3590,25 @@ config { }; }; + qup_spi16_cs_gpio: qup-spi16-cs-gpio { + mux { + pins = "gpio48", "gpio49", + "gpio50"; + function = "qup16"; + }; + + cs { + pins = "gpio51"; + function = "gpio"; + }; + + config { + pins = "gpio48", "gpio49", "gpio50", "gpio51"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi16_default: qup-spi16-default { mux { pins = "gpio48", "gpio49", @@ -3301,6 +3624,25 @@ config { }; }; + qup_spi17_cs_gpio: qup-spi17-cs-gpio { + mux { + pins = "gpio52", "gpio53", + "gpio54"; + function = "qup17"; + }; + + cs { + pins = "gpio55"; + function = "gpio"; + }; + + config { + pins = "gpio52", "gpio53", "gpio54", "gpio55"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi17_default: qup-spi17-default { mux { pins = "gpio52", "gpio53", @@ -3316,6 +3658,25 @@ config { }; }; + qup_spi18_cs_gpio: qup-spi18-cs-gpio { + mux { + pins = "gpio56", "gpio57", + "gpio58"; + function = "qup18"; + }; + + cs { + pins = "gpio59"; + function = "gpio"; + }; + + config { + pins = "gpio56", "gpio57", "gpio58", "gpio59"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi18_default: qup-spi18-default { mux { pins = "gpio56", "gpio57", @@ -3331,6 +3692,25 @@ config { }; }; + qup_spi19_cs_gpio: qup-spi19-cs-gpio { + mux { + pins = "gpio0", "gpio1", + "gpio2"; + function = "qup19"; + }; + + cs { + pins = "gpio3"; + function = "gpio"; + }; + + config { + pins = "gpio0", "gpio1", "gpio2", "gpio3"; + drive-strength = <6>; + bias-disable; + }; + }; + qup_spi19_default: qup-spi19-default { mux { pins = "gpio0", "gpio1",
GENI SPI controller shows several issues if it manages the CS on its own (see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to use GPIO for CS")) for the details. Provide pinctrl entries for SPI controllers using the same CS pin but in GPIO mode. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++ 1 file changed, 380 insertions(+) -- 2.30.0