Message-Id: <20230323102605.12.I6f03f86546e6ce9abb1d24fd9ece663c3a5b950c@changeid>
Date:   Thu, 23 Mar 2023 10:30:16 -0700
From:   Douglas Anderson <dianders@...omium.org>
To:     Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     Matthias Kaehlcke <mka@...omium.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-gpio@...r.kernel.org, Stephen Boyd <swboyd@...omium.org>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-spi@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>,
        Andy Gross <agross@...nel.org>, linux-kernel@...r.kernel.org
Subject: [PATCH 12/14] arm64: dts: qcom: sc7180: Fix trogdor qspi pin config

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@...omium.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(-)

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";
 			};
 
-- 
2.40.0.348.gf938b09366-goog

Powered by blists - more mailing lists