The main goal of this series is to do a better job of controling the
pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This
is essentially 'v2' of my previous attempt in the patch ("arm64: dts:
qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's
spiraled out a bit and there are no patches that are exactly the same
I've reset to v1.
The early patches in this series are just no-op cleanup patches that
can be applied. They're not terribly critical but since they are
"Fixes" I've listed them first.
The next patch in the series is a very simple and (hopefully)
non-controversial SPI patch. It can be applied independently if
anything else.
Next, we have a bunch of pinctrl patches (including the device tree
bindings related to them). I dunno what folks are going to think about
these. If everyone hates them, we can drop them and just change the
later patches in the series to use "input-enable" instead of
"output-disable". It feels ugly to me, but it maybe less upheval.
Next I removed the now-deprecated "input-enable" property from all
Chromebooks. None of them were necessary.
Finally, I did what I really wanted to do in the first place: attempt
to cleanup the pinctrl states of the Quad SPI. These patches have a
hard requirement on the pinctrl change.
[1] https://lore.kernel.org/r/20230213165743.1.I6f03f86546e6ce9abb1d24fd9ece663c3a5b950c@changeid
Douglas Anderson (14):
arm64: dts: sc7180: Rename qspi data12 as data23
arm64: dts: sc7280: Rename qspi data12 as data23
arm64: dts: sdm845: Rename qspi data12 as data23
arm64: dts: qcom: sc7180: Annotate l13a on trogdor to always-on
spi: spi-qcom-qspi: Support pinctrl sleep states
dt-bindings: pinctrl: qcom: tlmm should use output-disable, not
input-enable
dt-bindings: pinctrl: qcom: Add output-enable
pinctrl: qcom: Support OUTPUT_ENABLE; deprecate INPUT_ENABLE
arm64: dts: qcom: sc7180: Remove superfluous "input-enable"s from
trogdor
arm64: dts: qcom: sc7280: Remove superfluous "input-enable"s from
idp-ec-h1
arm64: dts: qcom: sdm845: Remove superfluous "input-enable"s from
cheza
arm64: dts: qcom: sc7180: Fix trogdor qspi pin config
arm64: dts: qcom: sc7280: Fix qspi pin config
arm64: dts: qcom: sdm845: Fix cheza qspi pin config
.../bindings/pinctrl/qcom,tlmm-common.yaml | 4 +-
arch/arm64/boot/dts/qcom/sc7180-idp.dts | 9 ++--
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 41 +++++++++++++------
arch/arm64/boot/dts/qcom/sc7180.dtsi | 11 +++--
.../boot/dts/qcom/sc7280-chrome-common.dtsi | 25 ++++++++++-
.../arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 14 ++++---
.../arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 2 -
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 13 +++---
arch/arm64/boot/dts/qcom/sc7280.dtsi | 11 +++--
arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 37 ++++++++++++-----
arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +++--
drivers/pinctrl/qcom/pinctrl-msm.c | 36 +++++++++++++---
drivers/spi/spi-qcom-qspi.c | 5 +++
13 files changed, 165 insertions(+), 54 deletions(-)
--
2.40.0.348.gf938b09366-goog
There are 4 qspi data pins: data0, data1, data2, and data3. Currently
we have a shared pin state for data0 and data1 (2 lane config) and a
pin state for data2 and data3 (you'd enable both this and the 2 lane
state for 4 lanes). The second state is obviously misnamed. Fix it.
Fixes: 7720ea001b52 ("arm64: dts: qcom: sc7280: Add QSPI node")
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index bdcb74925313..71e2e51c7c7f 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -4342,7 +4342,7 @@ qspi_data01: qspi-data01-state {
function = "qspi_data";
};
- qspi_data12: qspi-data12-state {
+ qspi_data23: qspi-data23-state {
pins = "gpio16", "gpio17";
function = "qspi_data";
};
--
2.40.0.348.gf938b09366-goog
It's fairly common practice for drivers to switch to a "sleep" pinctrl
state at the end of its runtime_suspend function and then back to
"default" at the beginning of runtime_resume. Let's do that for
spi-qcom-qspi.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/spi/spi-qcom-qspi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index c334dfec4117..7851cf1986cc 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -8,6 +8,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/pm_runtime.h>
#include <linux/pm_opp.h>
#include <linux/spi/spi.h>
@@ -581,6 +582,8 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev)
return ret;
}
+ pinctrl_pm_select_sleep_state(dev);
+
return 0;
}
@@ -590,6 +593,8 @@ static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev)
struct qcom_qspi *ctrl = spi_master_get_devdata(master);
int ret;
+ pinctrl_pm_select_default_state(dev);
+
ret = icc_enable(ctrl->icc_path_cpu_to_qspi);
if (ret) {
dev_err_ratelimited(ctrl->dev, "%s: ICC enable failed for cpu: %d\n",
--
2.40.0.348.gf938b09366-goog
The l13a rail on trogdor devices has always been intended to be
always-on on both S0 and S3. Different trogdor variants use l13a in
slightly different ways, but the overall theme is that it's a 1.8V
rail that the board uses for things that it wants powered in on S0 and
S3. On many boards this includes the boot SPI (AKA qspi).
For all intents and purposes this patch is actually a no-op since
something else in the system seems to already be keeping the rail on
all the time (confirmed via multimeter). That "something else" was
postulated to be the modem but the rail is on / stays on even without
the modem/wifi coming up so it's likely the boot config. In any case,
making the fact that this is always-on explicit seems like a good
idea.
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 423630c4d02c..1f2e1f701761 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -512,6 +512,8 @@ pp1800_l13a: ldo13 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+ regulator-always-on;
+ regulator-boot-on;
};
pp1800_prox:
--
2.40.0.348.gf938b09366-goog
As evidenced by the Qualcomm TLMM Linux driver, the TLMM IP block in
Qualcomm SoCs has a bit to enable/disable the output for a pin that's
configured as a GPIO but _not_ a bit to enable/disable an input
buffer. Current device trees that are specifying "input-enable" for
pins managed by TLMM are either doing so needlessly or are using it to
mean "output-disable".
Presumably the current convention of using "input-enable" to mean
"output-disable" stems from the fact that "output-disable" is a "new"
property from 2017. It was introduced in commit 425562429d4f
("pinctrl: generic: Add output-enable property"). The "input-enable"
handling in Qualcomm drivers is from 2015 introduced in commit
407f5e392f9c ("pinctrl: qcom: handle input-enable pinconf property").
Given that there's no other use for "input-enable" for TLMM, we can
still handle old device trees in code, but let's encourage people to
move to the proper / documented property by updating the bindings.
Signed-off-by: Douglas Anderson <[email protected]>
---
.../devicetree/bindings/pinctrl/qcom,tlmm-common.yaml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
index cb5ba1bd6f8d..5a815c199642 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
@@ -75,7 +75,8 @@ $defs:
bias-pull-down: true
bias-pull-up: true
bias-disable: true
- input-enable: true
+ input-enable: false
+ output-disable: true
output-high: true
output-low: true
--
2.40.0.348.gf938b09366-goog
The Qualcomm pinctrl driver has been violating the documented meaning
of PIN_CONFIG_INPUT_ENABLE. That documentation says:
Note that this does not affect the pin's ability to drive output.
...yet the Qualcomm driver's sole action when asked to "enable input"
on a pin is to disable its output.
The Qualcomm driver's implementation stems from the fact that
"output-disable" is a "new" property from 2017. It was introduced in
commit 425562429d4f ("pinctrl: generic: Add output-enable
property"). The "input-enable" handling in Qualcomm drivers is from
2015 introduced in commit 407f5e392f9c ("pinctrl: qcom: handle
input-enable pinconf property").
Let's change the Qualcomm driver to move us in the right direction. As
part of this:
1. We'll now support PIN_CONFIG_OUTPUT_ENABLE
2. We'll still support using PIN_CONFIG_INPUT_ENABLE to disable a
pin's output (in violation of the docs) with a big comment in the
code. This is needed because old device trees have "input-enable"
in them and, in some cases, people might need the old
behavior. While we could programmatically change all old device
trees, it doesn't really hurt to keep supporting the old behavior
and we're _supposed_ to try to be compatible with old device trees
anyway.
It can also be noted that the PIN_CONFIG_INPUT_ENABLE handling code
seems to have purposefully ignored its argument. That means that old
boards that had _either_ "input-disable" or "input-enable" in them
would have had the effect of disabling a pin's output. While we could
change this behavior, since we're only leaving the
PIN_CONFIG_INPUT_ENABLE there for backward compatibility we might as
well be fully backward compatible.
NOTE: despite the fact that we'll still support
PIN_CONFIG_INPUT_ENABLE for _setting_ config, we take it away from
msm_config_group_get(). This appears to be only used for populating
debugfs and fixing debugfs to "output enabled" where relevant instead
of "input enabled" makes more sense and has more truthiness.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 36 +++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index daeb79a9a602..4515f375c5e8 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -323,6 +323,7 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
break;
case PIN_CONFIG_OUTPUT:
case PIN_CONFIG_INPUT_ENABLE:
+ case PIN_CONFIG_OUTPUT_ENABLE:
*bit = g->oe_bit;
*mask = 1;
break;
@@ -414,11 +415,9 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
val = msm_readl_io(pctrl, g);
arg = !!(val & BIT(g->in_bit));
break;
- case PIN_CONFIG_INPUT_ENABLE:
- /* Pin is output */
- if (arg)
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ if (!arg)
return -EINVAL;
- arg = 1;
break;
default:
return -ENOTSUPP;
@@ -502,9 +501,36 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
arg = 1;
break;
case PIN_CONFIG_INPUT_ENABLE:
- /* disable output */
+ /*
+ * According to pinctrl documentation this should
+ * actually be a no-op.
+ *
+ * The docs are explicit that "this does not affect
+ * the pin's ability to drive output" but what we do
+ * here is to modify the output enable bit. Thus, to
+ * follow the docs we should remove that.
+ *
+ * The docs say that we should enable any relevant
+ * input buffer, but TLMM there is no input buffer that
+ * can be enabled/disabled. It's always on.
+ *
+ * The points above, explain why this _should_ be a
+ * no-op. However, for historical reasons and to
+ * support old device trees, we'll violate the docs
+ * still affect the output.
+ *
+ * It should further be noted that this old historical
+ * behavior actually overrides arg to 0. That means
+ * that "input-enable" and "input-disable" in a device
+ * tree would _both_ disable the output. We'll
+ * continue to preserve this behavior as well since
+ * we have no other use for this attribute.
+ */
arg = 0;
break;
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ arg = !!arg;
+ break;
default:
dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
param);
--
2.40.0.348.gf938b09366-goog
As talked about in the patch ("dt-bindings: pinctrl: qcom: tlmm should
use output-disable, not input-enable"), using "input-enable" in
pinctrl states for Qualcomm TLMM pinctrl devices was either
superfluous or there to disable a pin's output.
Looking at the sc7280-idp-ec-h1.dtsi file:
* ap_ec_int_l, h1_ap_int_odl: Superfluous. The pins will be configured
as inputs automatically by the Linux GPIO subsystem (presumably the
reference for other OSes using these device trees).
That means that in none of the cases for sc7280-idp-ec-h1.dtsi did we
need to change "input-enable" to "output-disable" and we can just
remove these superfluous properties.
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
index 3cfeb118d379..ebae545c587c 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
@@ -82,14 +82,12 @@ &tlmm {
ap_ec_int_l: ap-ec-int-l-state {
pins = "gpio18";
function = "gpio";
- input-enable;
bias-pull-up;
};
h1_ap_int_odl: h1-ap-int-odl-state {
pins = "gpio104";
function = "gpio";
- input-enable;
bias-pull-up;
};
--
2.40.0.348.gf938b09366-goog
As talked about in the patch ("dt-bindings: pinctrl: qcom: tlmm should
use output-disable, not input-enable"), using "input-enable" in
pinctrl states for Qualcomm TLMM pinctrl devices was either
superfluous or there to disable a pin's output.
Looking at cheza
* ec_ap_int_l, h1_ap_int_odl: Superfluous. The pins will be configured
as inputs automatically by the Linux GPIO subsystem (presumably the
reference for other OSes using these device trees).
* bios_flash_wp_l: Superfluous. This pin is exposed to userspace
through the kernel's GPIO API and will be configured automatically.
That means that in none of the cases for cheza did we need to change
"input-enable" to "output-disable" and we can just remove these
superfluous properties.
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
index f2b48241d15c..588165ee74b3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
@@ -1155,14 +1155,12 @@ ap_edp_bklten: ap-edp-bklten-state {
bios_flash_wp_r_l: bios-flash-wp-r-l-state {
pins = "gpio128";
function = "gpio";
- input-enable;
bias-disable;
};
ec_ap_int_l: ec-ap-int-l-state {
pins = "gpio122";
function = "gpio";
- input-enable;
bias-pull-up;
};
@@ -1190,7 +1188,6 @@ en_pp3300_dx_edp: en-pp3300-dx-edp-state {
h1_ap_int_odl: h1-ap-int-odl-state {
pins = "gpio129";
function = "gpio";
- input-enable;
bias-pull-up;
};
--
2.40.0.348.gf938b09366-goog
In the patch ("dt-bindings: pinctrl: qcom: tlmm should use
output-disable, not input-enable") we allowed setting "output-disable"
for TLMM pinctrl states. Let's also add "output-enable".
At first blush this seems a needless thing to do. Specifically:
- In Linux (and presumably any other OSes using the same device trees)
the GPIO/pinctrl driver knows to automatically enable the output
when a GPIO is changed to an output. Thus in most cases specifying
"output-enable" is superfluous and should be avoided.
- If we need to set a pin's default state we already have
"output-high" and "output-low" and these properties already imply
"output-enabled" (at least on the Linux Qualcomm TLMM driver).
However, there is one instance where "output-enable" seems like it
could be useful: sleep states. It's not uncommon to want to configure
pins as inputs (with appropriate pulls) when the driver controlling
them is in a low power state. Then we want the pins back to outputs
when the driver wants things running normally. To accomplish this we'd
want to be able to use "output-enable". Then the "default" state could
have "output-enable" and the "sleep" state could have
"output-disable".
NOTE: in all instances I'm aware of, we'd only want to use
"output-enable" on pins that are configured as "gpio". The Qualcomm
documentation that I have access to says that "output-enable" only
does something useful when in GPIO mode.
Signed-off-by: Douglas Anderson <[email protected]>
---
Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
index 5a815c199642..90b7d75840c1 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
@@ -77,6 +77,7 @@ $defs:
bias-disable: true
input-enable: false
output-disable: true
+ output-enable: true
output-high: true
output-low: true
--
2.40.0.348.gf938b09366-goog
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 <[email protected]>
---
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
As talked about in the patch ("dt-bindings: pinctrl: qcom: tlmm should
use output-disable, not input-enable"), using "input-enable" in
pinctrl states for Qualcomm TLMM pinctrl devices was either
superfluous or there to disable a pin's output.
Looking at trogdor:
* ap_ec_int_l, fp_to_ap_irq_l, h1_ap_int_odl, p_sensor_int_l:
Superfluous. The pins will be configured as inputs automatically by
the Linux GPIO subsystem (presumably the reference for other OSes
using these device trees).
* bios_flash_wp_l: Superfluous. This pin is exposed to userspace
through the kernel's GPIO API and will be configured automatically.
That means that in none of the cases for trogdor did we need to change
"input-enable" to "output-disable" and we can just remove these
superfluous properties.
Signed-off-by: Douglas Anderson <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 1f2e1f701761..39100b0c1140 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -1206,7 +1206,6 @@ amp_en: amp-en-state {
ap_ec_int_l: ap-ec-int-l-state {
pins = "gpio94";
function = "gpio";
- input-enable;
bias-pull-up;
};
@@ -1229,7 +1228,6 @@ ap_suspend_l_neuter: ap-suspend-l-neuter-state {
bios_flash_wp_l: bios-flash-wp-l-state {
pins = "gpio66";
function = "gpio";
- input-enable;
bias-disable;
};
@@ -1271,7 +1269,6 @@ fp_rst_l: fp-rst-l-state {
fp_to_ap_irq_l: fp-to-ap-irq-l-state {
pins = "gpio4";
function = "gpio";
- input-enable;
/* Has external pullup */
bias-disable;
@@ -1286,7 +1283,6 @@ fpmcu_boot0: fpmcu-boot0-state {
h1_ap_int_odl: h1-ap-int-odl-state {
pins = "gpio42";
function = "gpio";
- input-enable;
bias-pull-up;
};
@@ -1335,7 +1331,6 @@ pen_rst_odl: pen-rst-odl-state {
p_sensor_int_l: p-sensor-int-l-state {
pins = "gpio24";
function = "gpio";
- input-enable;
/* Has external pullup */
bias-disable;
--
2.40.0.348.gf938b09366-goog
Similar to sc7180 (see the patch ("arm64: dts: qcom: sc7180: Fix
trogdor qspi pin config")), we should adjust the qspi pin config for
sc7280.
I won't re-describe all the research/arguments in the sc7180 patch
here, but there are a few differences for sc7280 worth noting:
1. On herobrine the SPI flash (qspi) is wired up differently on the
board. Rather than Cr50 and the AP being wired directly together,
there's actually a mux that will _either_ connect the AP to the
flash or Cr50 to the flash. This means that the internal pulls on
Cr50 don't affect us and we should enable our own pulldowns.
2. On herobrine, EEs added an external pulldown on the MISO line. The
argument in the schematic said that we added it (but not one on
MOSI and CLK) because Cr50 already enabled pulldowns on MOSI and
CLK. ...though, as per #1, those Cr50 pulldowns would only affect
the line when the mux was swung to Cr50.
The ironic result of #1 and #2 is that the external pulldowns on
CLK/MISO/MOSI on herobrine are _exactly opposite_ of the ones on
trogdor.
3. While I still don't have the actual exact schematics for all
variants of IDP/CRD that were produced, I have some reference
schematics that give me a belief of how the qspi is hooked up
there. From this, I'm fairly certain that all of the older variants
of IDP/CRD either have a pulldown on the CLK/MOSI/MISO lines (maybe
through a direct connect to Cr50) or have no pull (in other words,
they don't have a pullup). I'll go ahead and enable internal
pulldowns on all the lines since that won't hurt to double-pull if
there's an external pulldown and it's nice to have a pulldown if
there's nothing external. Note that this only affects _older_
CRDs. Newer revs are considered "herobrine" (see the hoglin/zoglin
device trees).
4. I didn't find the same strange "auto-switch-to-keeper" at suspend
when probing on sc7280. Whatever pulls (or lack thereof) I left at
suspend time seemed to persist into suspend.
Signed-off-by: Douglas Anderson <[email protected]>
---
.../boot/dts/qcom/sc7280-chrome-common.dtsi | 25 +++++++++++++++++--
.../arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 14 +++++++----
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 13 ++++++----
arch/arm64/boot/dts/qcom/sc7280.dtsi | 9 +++++--
4 files changed, 47 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
index 16fb20369c01..f562e4d2b655 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
@@ -60,8 +60,9 @@ &pmk8350_pon {
*/
&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>;
spi_flash: flash@0 {
compatible = "jedec,spi-nor";
@@ -85,3 +86,23 @@ wifi-firmware {
iommus = <&apps_smmu 0x1c02 0x1>;
};
};
+
+/* PINCTRL - chrome-common pinctrl */
+
+&tlmm {
+ qspi_sleep: qspi-sleep-state {
+ pins = "gpio12", "gpio13", "gpio14", "gpio15";
+
+ /*
+ * 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;
+ };
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index b6137816f2f3..e651f633341f 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -692,18 +692,22 @@ &pcie1_clkreq_n {
};
&qspi_cs0 {
- bias-disable;
+ bias-disable; /* External pullup */
drive-strength = <8>;
};
&qspi_clk {
- bias-disable;
+ bias-pull-down; /* No external pulls */
drive-strength = <8>;
};
-&qspi_data01 {
- /* High-Z when no transfers; nice to park the lines */
- bias-pull-up;
+&qspi_data0 {
+ bias-pull-down; /* No external pulls */
+ drive-strength = <8>;
+};
+
+&qspi_data1 {
+ bias-disable; /* External pulldown */
drive-strength = <8>;
};
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 8b5293e7fd2a..6aaa77abc00b 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -636,16 +636,19 @@ &pcie1_clkreq_n {
};
&qspi_cs0 {
- bias-disable;
+ bias-disable; /* External pullup */
};
&qspi_clk {
- bias-disable;
+ bias-pull-down; /* No external pulls or external pulldown */
};
-&qspi_data01 {
- /* High-Z when no transfers; nice to park the lines */
- bias-pull-up;
+&qspi_data0 {
+ bias-pull-down; /* No external pulls or external pulldown */
+};
+
+&qspi_data1 {
+ bias-pull-down; /* No external pulls or external pulldown */
};
&qup_uart5_tx {
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 71e2e51c7c7f..b98994cc8616 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -4337,8 +4337,13 @@ qspi_cs1: qspi-cs1-state {
function = "qspi_cs";
};
- qspi_data01: qspi-data01-state {
- pins = "gpio12", "gpio13";
+ qspi_data0: qspi-data0-state {
+ pins = "gpio12";
+ function = "qspi_data";
+ };
+
+ qspi_data1: qspi-data1-state {
+ pins = "gpio13";
function = "qspi_data";
};
--
2.40.0.348.gf938b09366-goog
Cheza's SPI flash hookups (qspi) are exactly the same as trogdor's.
Apply the same solution that's described in the patch ("arm64: dts:
qcom: sc7180: Fix trogdor qspi pin config")
Signed-off-by: Douglas Anderson <[email protected]>
---
I think cheza is only very lightly used today (it was never sold, but
there are various people still using the dev boards) and I'm not
personally setup to test this. It's fairly straightforward but has
only been compile-tested.
arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 34 +++++++++++++++++-----
arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 ++++--
2 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
index 588165ee74b3..64ad8d1ed433 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
@@ -319,8 +319,9 @@ venus_mem: memory@96000000 {
&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";
@@ -995,16 +996,19 @@ &wifi {
/* PINCTRL - additions to nodes defined in sdm845.dtsi */
&qspi_cs0 {
- bias-disable;
+ bias-disable; /* External pullup */
};
&qspi_clk {
- 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_i2c3_default {
@@ -1233,6 +1237,22 @@ pen_rst_l: pen-rst-l-state {
output-high;
};
+ qspi_sleep: qspi-sleep-state {
+ pins = "gpio90", "gpio91", "gpio92", "gpio95";
+
+ /*
+ * 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;
+ };
+
sdc2_clk: sdc2-clk-state {
pins = "sdc2_clk";
bias-disable;
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index aafc7cc7edd8..dce2cb29347b 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2758,8 +2758,13 @@ qspi_cs1: qspi-cs1-state {
function = "qspi_cs";
};
- qspi_data01: qspi-data01-state {
- pins = "gpio91", "gpio92";
+ qspi_data0: qspi-data0-state {
+ pins = "gpio91";
+ function = "qspi_data";
+ };
+
+ qspi_data1: qspi-data1-state {
+ pins = "gpio92";
function = "qspi_data";
};
--
2.40.0.348.gf938b09366-goog
On Thu, Mar 23, 2023 at 10:30:12AM -0700, Douglas Anderson wrote:
> The Qualcomm pinctrl driver has been violating the documented meaning
> of PIN_CONFIG_INPUT_ENABLE. That documentation says:
>
> Note that this does not affect the pin's ability to drive output.
>
> ...yet the Qualcomm driver's sole action when asked to "enable input"
> on a pin is to disable its output.
>
Seemed like a good idea at the time...
> The Qualcomm driver's implementation stems from the fact that
> "output-disable" is a "new" property from 2017. It was introduced in
> commit 425562429d4f ("pinctrl: generic: Add output-enable
> property"). The "input-enable" handling in Qualcomm drivers is from
> 2015 introduced in commit 407f5e392f9c ("pinctrl: qcom: handle
> input-enable pinconf property").
>
> Let's change the Qualcomm driver to move us in the right direction. As
> part of this:
> 1. We'll now support PIN_CONFIG_OUTPUT_ENABLE
> 2. We'll still support using PIN_CONFIG_INPUT_ENABLE to disable a
> pin's output (in violation of the docs) with a big comment in the
> code. This is needed because old device trees have "input-enable"
> in them and, in some cases, people might need the old
> behavior. While we could programmatically change all old device
> trees, it doesn't really hurt to keep supporting the old behavior
> and we're _supposed_ to try to be compatible with old device trees
> anyway.
>
> It can also be noted that the PIN_CONFIG_INPUT_ENABLE handling code
> seems to have purposefully ignored its argument. That means that old
> boards that had _either_ "input-disable" or "input-enable" in them
> would have had the effect of disabling a pin's output. While we could
> change this behavior, since we're only leaving the
> PIN_CONFIG_INPUT_ENABLE there for backward compatibility we might as
> well be fully backward compatible.
>
It made total sense to to spell input-disable as output-{high,low} back
then, but we're wiser now. Thanks for fixing it.
> NOTE: despite the fact that we'll still support
> PIN_CONFIG_INPUT_ENABLE for _setting_ config, we take it away from
> msm_config_group_get(). This appears to be only used for populating
> debugfs and fixing debugfs to "output enabled" where relevant instead
> of "input enabled" makes more sense and has more truthiness.
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
>
> drivers/pinctrl/qcom/pinctrl-msm.c | 36 +++++++++++++++++++++++++-----
> 1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index daeb79a9a602..4515f375c5e8 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -323,6 +323,7 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
> break;
> case PIN_CONFIG_OUTPUT:
> case PIN_CONFIG_INPUT_ENABLE:
> + case PIN_CONFIG_OUTPUT_ENABLE:
> *bit = g->oe_bit;
> *mask = 1;
> break;
> @@ -414,11 +415,9 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
> val = msm_readl_io(pctrl, g);
> arg = !!(val & BIT(g->in_bit));
> break;
> - case PIN_CONFIG_INPUT_ENABLE:
> - /* Pin is output */
> - if (arg)
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + if (!arg)
> return -EINVAL;
> - arg = 1;
> break;
> default:
> return -ENOTSUPP;
> @@ -502,9 +501,36 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
> arg = 1;
> break;
> case PIN_CONFIG_INPUT_ENABLE:
> - /* disable output */
> + /*
> + * According to pinctrl documentation this should
> + * actually be a no-op.
> + *
> + * The docs are explicit that "this does not affect
> + * the pin's ability to drive output" but what we do
> + * here is to modify the output enable bit. Thus, to
> + * follow the docs we should remove that.
> + *
> + * The docs say that we should enable any relevant
> + * input buffer, but TLMM there is no input buffer that
> + * can be enabled/disabled. It's always on.
> + *
> + * The points above, explain why this _should_ be a
> + * no-op. However, for historical reasons and to
> + * support old device trees, we'll violate the docs
> + * still affect the output.
> + *
> + * It should further be noted that this old historical
> + * behavior actually overrides arg to 0. That means
> + * that "input-enable" and "input-disable" in a device
> + * tree would _both_ disable the output. We'll
> + * continue to preserve this behavior as well since
> + * we have no other use for this attribute.
> + */
> arg = 0;
> break;
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + arg = !!arg;
> + break;
> default:
> dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> param);
> --
> 2.40.0.348.gf938b09366-goog
>
On 23/03/2023 18:30, Douglas Anderson wrote:
> As evidenced by the Qualcomm TLMM Linux driver, the TLMM IP block in
> Qualcomm SoCs has a bit to enable/disable the output for a pin that's
> configured as a GPIO but _not_ a bit to enable/disable an input
> buffer. Current device trees that are specifying "input-enable" for
> pins managed by TLMM are either doing so needlessly or are using it to
> mean "output-disable".
>
> Presumably the current convention of using "input-enable" to mean
> "output-disable" stems from the fact that "output-disable" is a "new"
> property from 2017. It was introduced in commit 425562429d4f
> ("pinctrl: generic: Add output-enable property"). The "input-enable"
> handling in Qualcomm drivers is from 2015 introduced in commit
> 407f5e392f9c ("pinctrl: qcom: handle input-enable pinconf property").
>
> Given that there's no other use for "input-enable" for TLMM, we can
> still handle old device trees in code, but let's encourage people to
> move to the proper / documented property by updating the bindings.
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 23/03/2023 18:30, Douglas Anderson wrote:
> In the patch ("dt-bindings: pinctrl: qcom: tlmm should use
> output-disable, not input-enable") we allowed setting "output-disable"
> for TLMM pinctrl states. Let's also add "output-enable".
>
> At first blush this seems a needless thing to do. Specifically:
> - In Linux (and presumably any other OSes using the same device trees)
> the GPIO/pinctrl driver knows to automatically enable the output
> when a GPIO is changed to an output. Thus in most cases specifying
> "output-enable" is superfluous and should be avoided.
> - If we need to set a pin's default state we already have
> "output-high" and "output-low" and these properties already imply
> "output-enabled" (at least on the Linux Qualcomm TLMM driver).
>
> However, there is one instance where "output-enable" seems like it
> could be useful: sleep states. It's not uncommon to want to configure
> pins as inputs (with appropriate pulls) when the driver controlling
> them is in a low power state. Then we want the pins back to outputs
> when the driver wants things running normally. To accomplish this we'd
> want to be able to use "output-enable". Then the "default" state could
> have "output-enable" and the "sleep" state could have
> "output-disable".
>
> NOTE: in all instances I'm aware of, we'd only want to use
> "output-enable" on pins that are configured as "gpio". The Qualcomm
> documentation that I have access to says that "output-enable" only
> does something useful when in GPIO mode.
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Thu, Mar 23, 2023 at 6:31 PM Douglas Anderson <[email protected]> wrote:
> The main goal of this series is to do a better job of cI can apply ontroling the
> pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This
> is essentially 'v2' of my previous attempt in the patch ("arm64: dts:
> qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's
> spiraled out a bit and there are no patches that are exactly the same
> I've reset to v1.
>
> The early patches in this series are just no-op cleanup patches that
> can be applied. They're not terribly critical but since they are
> "Fixes" I've listed them first.
>
> The next patch in the series is a very simple and (hopefully)
> non-controversial SPI patch. It can be applied independently if
> anything else.
>
> Next, we have a bunch of pinctrl patches (including the device tree
> bindings related to them). I dunno what folks are going to think about
> these. If everyone hates them, we can drop them and just change the
> later patches in the series to use "input-enable" instead of
> "output-disable". It feels ugly to me, but it maybe less upheval.
>
> Next I removed the now-deprecated "input-enable" property from all
> Chromebooks. None of them were necessary.
>
> Finally, I did what I really wanted to do in the first place: attempt
> to cleanup the pinctrl states of the Quad SPI. These patches have a
> hard requirement on the pinctrl change.
This looks good to me (TM)
Do you have a merge plan?
I can queue the pinctrl patch into the pinctrl tree, and
the pinctrl binding patches.
Will you take the rest to the SPI and SoC tree?
Acked-by: Linus Walleij <[email protected]>
FWIW
Yours,
Linus Walleij
Hi,
On Mon, Mar 27, 2023 at 2:45 PM Linus Walleij <[email protected]> wrote:
>
> On Thu, Mar 23, 2023 at 6:31 PM Douglas Anderson <[email protected]> wrote:
>
> > The main goal of this series is to do a better job of cI can apply ontroling the
> > pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This
> > is essentially 'v2' of my previous attempt in the patch ("arm64: dts:
> > qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's
> > spiraled out a bit and there are no patches that are exactly the same
> > I've reset to v1.
> >
> > The early patches in this series are just no-op cleanup patches that
> > can be applied. They're not terribly critical but since they are
> > "Fixes" I've listed them first.
> >
> > The next patch in the series is a very simple and (hopefully)
> > non-controversial SPI patch. It can be applied independently if
> > anything else.
> >
> > Next, we have a bunch of pinctrl patches (including the device tree
> > bindings related to them). I dunno what folks are going to think about
> > these. If everyone hates them, we can drop them and just change the
> > later patches in the series to use "input-enable" instead of
> > "output-disable". It feels ugly to me, but it maybe less upheval.
> >
> > Next I removed the now-deprecated "input-enable" property from all
> > Chromebooks. None of them were necessary.
> >
> > Finally, I did what I really wanted to do in the first place: attempt
> > to cleanup the pinctrl states of the Quad SPI. These patches have a
> > hard requirement on the pinctrl change.
>
> This looks good to me (TM)
>
> Do you have a merge plan?
> I can queue the pinctrl patch into the pinctrl tree, and
> the pinctrl binding patches.
>
> Will you take the rest to the SPI and SoC tree?
>
> Acked-by: Linus Walleij <[email protected]>
My thoughts:
1. Mark could land the SPI patch at any time, assuming he's OK with
it. It can land totally independently.
2. First 7 dts patches could land in the Qualcomm tree. There are no
dependencies on these ones other than the commit message of some of
the later dts patches talking about the pinctrl patches.
...then...
Option A:
3. You land the pinctrl and binding patches in an immutable branch and
merge into pinctrl.
4. Bjorn merges the immutable branch into the Qulacomm tree and places
the last 3 dts patches atop.
Option B:
3. You Ack the pinctrl patches and Bjorn lands them all, plus the last
3 dts patches.
Option C:
3. You land the pinctrl patches, then we want a few months and land
the last 3 dts patches.
On Thu, 23 Mar 2023 10:30:04 -0700, Douglas Anderson wrote:
> The main goal of this series is to do a better job of controling the
> pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This
> is essentially 'v2' of my previous attempt in the patch ("arm64: dts:
> qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's
> spiraled out a bit and there are no patches that are exactly the same
> I've reset to v1.
>
> [...]
Applied to
broonie/spi.git for-next
Thanks!
[05/14] spi: spi-qcom-qspi: Support pinctrl sleep states
commit: 0098c52745112c4387942a37559ababeaf072f0c
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
On Mon, Mar 27, 2023 at 11:51 PM Doug Anderson <[email protected]> wrote:
> 1. Mark could land the SPI patch at any time, assuming he's OK with
> it. It can land totally independently.
OK this happened.
> Option A:
>
> 3. You land the pinctrl and binding patches in an immutable branch and
> merge into pinctrl.
>
> 4. Bjorn merges the immutable branch into the Qulacomm tree and places
> the last 3 dts patches atop.
Looks most appetizing.
I have applied patches 6,7,8 to this immutable branch:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-qcom-quad-spi
and I merged that into my "devel" branch for v6.4.
Bjorn can grab the branch if he wants it.
Yours,
Linus Walleij
On Wed, Mar 29, 2023 at 10:50:34AM +0200, Linus Walleij wrote:
> On Mon, Mar 27, 2023 at 11:51 PM Doug Anderson <[email protected]> wrote:
>
> > 1. Mark could land the SPI patch at any time, assuming he's OK with
> > it. It can land totally independently.
>
> OK this happened.
>
> > Option A:
> >
> > 3. You land the pinctrl and binding patches in an immutable branch and
> > merge into pinctrl.
> >
> > 4. Bjorn merges the immutable branch into the Qulacomm tree and places
> > the last 3 dts patches atop.
>
> Looks most appetizing.
>
> I have applied patches 6,7,8 to this immutable branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-qcom-quad-spi
>
> and I merged that into my "devel" branch for v6.4.
>
> Bjorn can grab the branch if he wants it.
>
Thank you,
Bjorn
> Yours,
> Linus Walleij
On Thu, 23 Mar 2023 10:30:04 -0700, Douglas Anderson wrote:
> The main goal of this series is to do a better job of controling the
> pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This
> is essentially 'v2' of my previous attempt in the patch ("arm64: dts:
> qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's
> spiraled out a bit and there are no patches that are exactly the same
> I've reset to v1.
>
> [...]
Applied, thanks!
[01/14] arm64: dts: sc7180: Rename qspi data12 as data23
commit: d84f8f2687bdc67f20262e822b206419bcfd0038
[02/14] arm64: dts: sc7280: Rename qspi data12 as data23
commit: 14acf21c0d3f7b7298ffcd2e5b5db4a476ec6202
[03/14] arm64: dts: sdm845: Rename qspi data12 as data23
commit: 37f7349b56decc91c66f8039712e63740b1f25f9
[04/14] arm64: dts: qcom: sc7180: Annotate l13a on trogdor to always-on
commit: ced32c299e5d6c447ad0b80d7a16b44e0e72e8e0
[09/14] arm64: dts: qcom: sc7180: Remove superfluous "input-enable"s from trogdor
commit: e8df226339fa032c49f8db4281903930d018a22c
[10/14] arm64: dts: qcom: sc7280: Remove superfluous "input-enable"s from idp-ec-h1
commit: 6d4794d658a0967a7f257f16d6a7a48afb8c8e05
[11/14] arm64: dts: qcom: sdm845: Remove superfluous "input-enable"s from cheza
commit: 406fed87083578d07c7cea9483b85b51469594e0
[12/14] arm64: dts: qcom: sc7180: Fix trogdor qspi pin config
commit: ab752f03e2feb3323dfd9c1ce161ac759ce09634
[13/14] arm64: dts: qcom: sc7280: Fix qspi pin config
commit: 5f89df31096d67c244d8f36502f651ce701ddcde
[14/14] arm64: dts: qcom: sdm845: Fix cheza qspi pin config
commit: 9f5cdeb7031062a36e135ebb88bd99c03f32e5ee
Best regards,
--
Bjorn Andersson <[email protected]>
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
<[email protected]> 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
> <[email protected]> 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/[email protected]/
https://lore.kernel.org/linux-arm-msm/[email protected]/
https://lore.kernel.org/linux-arm-msm/[email protected]/
https://lore.kernel.org/linux-arm-msm/[email protected]/
Best regards,
Krzysztof