| Message ID | 20230323102605.12.I6f03f86546e6ce9abb1d24fd9ece663c3a5b950c@changeid |
|---|---|
| State | Accepted |
| Commit | ab752f03e2feb3323dfd9c1ce161ac759ce09634 |
| Headers | show |
| Series | Control Quad SPI pinctrl better on Qualcomm Chromebooks | expand |
On 23/03/2023 18:30, Douglas Anderson wrote: > In commit 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial > trogdor and lazor dt") we specified the pull settings on the boot SPI > (the qspi) data lines as pullups to "park" the lines. This seemed like > the right thing to do, but I never really probed the lines to confirm. > > &qup_i2c2_default { > @@ -1336,6 +1340,22 @@ p_sensor_int_l: p-sensor-int-l-state { > bias-disable; > }; > > + qspi_sleep: qspi-sleep-state { > + pins = "gpio63", "gpio64", "gpio65", "gpio68"; > + > + /* > + * When we're not actively transferring we want pins as GPIOs > + * with output disabled so that the quad SPI IP block stops > + * driving them. We rely on the normal pulls configured in > + * the active state and don't redefine them here. Also note > + * that we don't need the reverse (output-enable) in the > + * normal mode since the "output-enable" only matters for > + * GPIO function. > + */ > + function = "gpio"; > + output-disable; Doug, I acked some of your patches, but I assumed you tested all this. It turns out you never run dtbs_check on the patches you sent. Best regards, Krzysztof
Hi, On Fri, Apr 7, 2023 at 11:11 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 23/03/2023 18:30, Douglas Anderson wrote: > > In commit 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial > > trogdor and lazor dt") we specified the pull settings on the boot SPI > > (the qspi) data lines as pullups to "park" the lines. This seemed like > > the right thing to do, but I never really probed the lines to confirm. > > > > > > &qup_i2c2_default { > > @@ -1336,6 +1340,22 @@ p_sensor_int_l: p-sensor-int-l-state { > > bias-disable; > > }; > > > > + qspi_sleep: qspi-sleep-state { > > + pins = "gpio63", "gpio64", "gpio65", "gpio68"; > > + > > + /* > > + * When we're not actively transferring we want pins as GPIOs > > + * with output disabled so that the quad SPI IP block stops > > + * driving them. We rely on the normal pulls configured in > > + * the active state and don't redefine them here. Also note > > + * that we don't need the reverse (output-enable) in the > > + * normal mode since the "output-enable" only matters for > > + * GPIO function. > > + */ > > + function = "gpio"; > > + output-disable; > > Doug, > > I acked some of your patches, but I assumed you tested all this. It > turns out you never run dtbs_check on the patches you sent. I'm fairly certain that I ran dtbs_check and confirmed that no new errors were introduced on the device tree files that this patch series cleaned up. Did I miss one? I did not try to go through and fix all examples of people using "input-enable" across all Qualcomm device trees, though. Those old device trees still work even if they're using the now-deprecated bindings. When deprecating something my understanding is that it's not required to go back and immediately transition all old device tree files. If having the "input-enable: false" in the bindings is causing huge problems we could do a blank search-and-replace to rename it to "output-disable", at least for places under "tlmm". Even if there are cases where it's superfluous it would at least make the bindings validate. -Doug
On 07/04/2023 21:53, Doug Anderson wrote: > Hi, > > On Fri, Apr 7, 2023 at 11:11 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 23/03/2023 18:30, Douglas Anderson wrote: >>> In commit 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial >>> trogdor and lazor dt") we specified the pull settings on the boot SPI >>> (the qspi) data lines as pullups to "park" the lines. This seemed like >>> the right thing to do, but I never really probed the lines to confirm. >>> >> >> >>> &qup_i2c2_default { >>> @@ -1336,6 +1340,22 @@ p_sensor_int_l: p-sensor-int-l-state { >>> bias-disable; >>> }; >>> >>> + qspi_sleep: qspi-sleep-state { >>> + pins = "gpio63", "gpio64", "gpio65", "gpio68"; >>> + >>> + /* >>> + * When we're not actively transferring we want pins as GPIOs >>> + * with output disabled so that the quad SPI IP block stops >>> + * driving them. We rely on the normal pulls configured in >>> + * the active state and don't redefine them here. Also note >>> + * that we don't need the reverse (output-enable) in the >>> + * normal mode since the "output-enable" only matters for >>> + * GPIO function. >>> + */ >>> + function = "gpio"; >>> + output-disable; >> >> Doug, >> >> I acked some of your patches, but I assumed you tested all this. It >> turns out you never run dtbs_check on the patches you sent. > > I'm fairly certain that I ran dtbs_check and confirmed that no new > errors were introduced on the device tree files that this patch series > cleaned up. Did I miss one? You missed everything. Before the patchset almost all pinctrl bindings were passing on arm64 DTS. Just one or two things to fix. After the patchset: many new warnings. > I did not try to go through and fix all > examples of people using "input-enable" across all Qualcomm device > trees, though. You introduced new warnings, so it is expected to do. > Those old device trees still work even if they're using > the now-deprecated bindings. When deprecating something my > understanding is that it's not required to go back and immediately > transition all old device tree files. You did not deprecate anything. You disallowed property causing many new warnings to pop up. > > If having the "input-enable: false" in the bindings is causing huge > problems we could do a blank search-and-replace to rename it to > "output-disable", at least for places under "tlmm". Even if there are > cases where it's superfluous it would at least make the bindings > validate. There are different ways to fix it, the point is that none of the ways were used. I fixed it up: https://lore.kernel.org/linux-arm-msm/574d3aa5-21f4-014a-8cc7-7549df59ff3c@linaro.org/ https://lore.kernel.org/linux-arm-msm/20230407180655.128771-1-krzysztof.kozlowski@linaro.org/ https://lore.kernel.org/linux-arm-msm/20230407180045.126952-1-krzysztof.kozlowski@linaro.org/ https://lore.kernel.org/linux-arm-msm/20230407175807.124394-1-krzysztof.kozlowski@linaro.org/ Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts index c3bdd3295c02..44c27b4eac45 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts @@ -354,7 +354,7 @@ &qfprom { &qspi { status = "okay"; pinctrl-names = "default"; - pinctrl-0 = <&qspi_clk &qspi_cs0 &qspi_data01>; + pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data0>, <&qspi_data1>; flash@0 { compatible = "jedec,spi-nor"; @@ -512,8 +512,11 @@ &qspi_cs0 { bias-disable; }; -&qspi_data01 { - /* High-Z when no transfers; nice to park the lines */ +&qspi_data0 { + bias-pull-up; +}; + +&qspi_data1 { bias-pull-up; }; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi index 39100b0c1140..ca6920de7ea8 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi @@ -424,8 +424,9 @@ &qfprom { &qspi { status = "okay"; - pinctrl-names = "default"; - pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data0>, <&qspi_data1>; + pinctrl-1 = <&qspi_sleep>; flash@0 { compatible = "jedec,spi-nor"; @@ -1046,17 +1047,20 @@ &pri_mi2s_mclk_active { }; &qspi_cs0 { - bias-disable; + bias-disable; /* External pullup */ }; &qspi_clk { drive-strength = <8>; - bias-disable; + bias-disable; /* Rely on Cr50 internal pulldown */ }; -&qspi_data01 { - /* High-Z when no transfers; nice to park the lines */ - bias-pull-up; +&qspi_data0 { + bias-disable; /* Rely on Cr50 internal pulldown */ +}; + +&qspi_data1 { + bias-pull-down; }; &qup_i2c2_default { @@ -1336,6 +1340,22 @@ p_sensor_int_l: p-sensor-int-l-state { bias-disable; }; + qspi_sleep: qspi-sleep-state { + pins = "gpio63", "gpio64", "gpio65", "gpio68"; + + /* + * When we're not actively transferring we want pins as GPIOs + * with output disabled so that the quad SPI IP block stops + * driving them. We rely on the normal pulls configured in + * the active state and don't redefine them here. Also note + * that we don't need the reverse (output-enable) in the + * normal mode since the "output-enable" only matters for + * GPIO function. + */ + function = "gpio"; + output-disable; + }; + qup_uart3_sleep: qup-uart3-sleep-state { cts-pins { /* diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index fe62ce516c4e..b2fcf0b58722 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -1535,8 +1535,13 @@ qspi_cs1: qspi-cs1-state { function = "qspi_cs"; }; - qspi_data01: qspi-data01-state { - pins = "gpio64", "gpio65"; + qspi_data0: qspi-data0-state { + pins = "gpio64"; + function = "qspi_data"; + }; + + qspi_data1: qspi-data1-state { + pins = "gpio65"; function = "qspi_data"; };
In commit 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor and lazor dt") we specified the pull settings on the boot SPI (the qspi) data lines as pullups to "park" the lines. This seemed like the right thing to do, but I never really probed the lines to confirm. Since that time, I've done A LOT of research, experiements and poking of the lines with a voltmeter. A first batch of discoveries: - There is an external pullup on CS (clearly shown on schematics) - There are weak external pulldowns on CLK/MOSI (believed to be Cr50's internal pulldowns) - There is no pull on MISO. - When qspi isn't actively transferring it still drives CS, CLK, and MOSI. CS and MOSI are driven high and CLK is driven low. It does not drive MISO and (if no internal pulls are enabled) the line floats. The above means that it's good to have some sort of pull on MISO, at the very least. The pullup that we had before was actually fine (and my voltmeter confirms that it actually affected the state of the pin) but a pulldown would work equally well (and would match MOSI and CLK better). The above also means that we could save a tiny bit of power (not measurable by my setup) by setting up a sleep state for these pins. If nothing else this prevents us from driving high against Cr50's internal pulldown on MOSI. However, Qualcomm has also asserted in the past that it burns a little extra power to drive a pin, especially since these are configured with a slightly higher drive strength Let's fix all this. Since the external pulls are different for the two data lines, we'll split them into separate configs. Then we'll change the MISO pin to a pulldown and add a sleep state. On a slightly tangental (but not totally unrelated note), I also discovered some interesting things with these pins in suspend. First, I found that if we don't switch the pins to GPIO that the qspi peripheral continues to drive them in suspend. That'll be solved by what we're already doing above. Second, I found that something in the system suspend path (after Linux stops running) reconfigures these pins so that they don't have their normal pulls enabled but instead change to "keepers" (bias-bus-hold in DT speak). If a pin was floating before we entered suspend then it would stop floating. I found that I could manually pull a pin to a different level and then probe it and it would stay there. This is exactly keeper behavior. With the solution we have the switch to "keeper" doesn't matter too much but it's good to document. While talking about "keepers", it can also be noted that I found that the "keepers" on these pins were at least enough to win a fight against Cr50's internal pulls. That means it's best to make sure that the state of the pins are already correct before the mysterious transition to a keeper. Otherwise we'll burn (a small amount of) power in S3 via this fight. Luckily with the current solution we don't hit this case. NOTE: I've left "sc7180-idp" behavior totally alone in this patch. I didn't add a sleep state and I didn't change any pulls--I just adapted it to the fact that the data lines have separate configs. Qualcomm doesn't provide me with schematics for IDP and thus I don't actually know how the pulls are configured. Since this is just a development platform and worked well enough, it seems safer to leave it alone. Dependencies: - This patch has a hard dependency on ("pinctrl: qcom: Support OUTPUT_ENABLE; deprecate INPUT_ENABLE"). Something in the boot code seemed to have been confused and thought it needed to set the "OUTPUT ENABLE" bit for these pins even though it was using them as SPI. Thus if we don't honor the "output-disable" property we could end up driving the SPI pins while in sleep mode. - In general, it's probably best not to backport this to a kernel that doesn't have commit d21f4b7ffc22 ("pinctrl: qcom: Avoid glitching lines when we first mux to output"). That landed a while ago, but it's still good to be explicit in case someone was backporting. If we don't have that then there might be a glitch when we first switch over to GPIO before we disable the output. - This patch _doesn't_ really have any dependency on the qspi driver patch that supports setting the pinctrl sleep state--they can go in either order. If we define the sleep state and the driver never selects it that's fine. If the driver tries to select a sleep state that we don't define that's fine. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- v1 of this patch was ("arm64: dts: qcom: sc7180: Fix trogdor qspi pull direction") [1]. Since then, I've spent time running experiments where I tried lots of different combinations and then probed the GPIOs with a multimeter to figure out what's happening. As a result, it's now at the end of a somewhat larger series. I should note that I've removed the "Fixes" tag of this patch. While it still technically does "fix" the old behavior, the old behavior really wasn't terrible (a miniscule amount of extra power draw). It's probably not worth the risk that adding "Fixes" will cause it to get backported without the pinctrl support (see "Dependencies" in the patch description). [1] https://lore.kernel.org/r/20230213165743.1.I6f03f86546e6ce9abb1d24fd9ece663c3a5b950c@changeid arch/arm64/boot/dts/qcom/sc7180-idp.dts | 9 ++++-- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 34 ++++++++++++++++---- arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++++-- 3 files changed, 40 insertions(+), 12 deletions(-)