[PATCH 1/2] arm64: dts: qcom: sc7180: Fix trogdor qspi pull direction

[PATCH 1/2] arm64: dts: qcom: sc7180: Fix trogdor qspi pull direction

[Douglas Anderson]

Though it shouldn't matter very much, we've decided that it's slightly
better to park the qspi lines for trogdor with an internal pulldown
instead of an internal pullup. There was a footnote that Cr50 (which
connects to these lines too) may have pulldowns configured on one of
the data lines and we don't want to have fighting pulls. This also
means that if the pulls somehow get left powered in S3 (which I'm
uncertain about) that they won't be pulling up lines on an unpowered
SPI part.

Originally the pullup was picked because SPI transfers are active low
and thus the high state is somewhat more "idle", but that really isn't
that important because the chip select won't be asserted when the bus
is idle. The chip select has a nice external pullup on it that's
powered by the same power rail as the SPI flash.

This shouldn't have any functionality impact w/ reading/writing the
SPI since the lines are always push-pull when SPI transfers are
actually taking place.

Fixes: 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor and lazor dt")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 423630c4d02c..de40abcd18db 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -1054,7 +1054,7 @@ &qspi_clk {
 
 &qspi_data01 {
 	/* High-Z when no transfers; nice to park the lines */
-	bias-pull-up;
+	bias-pull-down;
 };
 
 &qup_i2c2_default {
-- 
2.39.1.581.gbfd45094c4-goog

[PATCH 2/2] arm64: dts: qcom: sc7280: Fix herobrine qspi pull direction

[Douglas Anderson]

Though it shouldn't matter very much, we've decided that it's slightly
better to park the qspi lines for herobrine with an internal pulldown
instead of an internal pullup. There is an external pulldown on one of
the data lines on the board and we don't want to have fighting pulls.
This also means that if the pulls somehow get left powered in S3
(which I'm uncertain about) that they won't be pulling up lines on an
unpowered SPI part.

Originally the pullup was picked because SPI transfers are active low
and thus the high state is somewhat more "idle", but that really isn't
that important because the chip select won't be asserted when the bus
is idle. The chip select has a nice external pullup on it that's
powered by the same power rail as the SPI flash.

This shouldn't have any functionality impact w/ reading/writing the
SPI since the lines are always push-pull when SPI transfers are
actually taking place.

Fixes: 116f7cc43d28 ("arm64: dts: qcom: sc7280: Add herobrine-r1")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index b6137816f2f3..7d787b12c10f 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -703,7 +703,7 @@ &qspi_clk {
 
 &qspi_data01 {
 	/* High-Z when no transfers; nice to park the lines */
-	bias-pull-up;
+	bias-pull-down;
 	drive-strength = <8>;
 };
 
-- 
2.39.1.581.gbfd45094c4-goog

Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Fix trogdor qspi pull direction

[Stephen Boyd]

Quoting Douglas Anderson (2023-02-13 16:57:51)
> Though it shouldn't matter very much, we've decided that it's slightly
> better to park the qspi lines for trogdor with an internal pulldown
> instead of an internal pullup. There was a footnote that Cr50 (which
> connects to these lines too) may have pulldowns configured on one of
> the data lines and we don't want to have fighting pulls.

Ok.

> This also
> means that if the pulls somehow get left powered in S3 (which I'm
> uncertain about) that they won't be pulling up lines on an unpowered
> SPI part.

As far as I know, the pulls are maintained in S3. There's verbage about
"keeper" on the pins.

The SPI part is powered in S3 though. I believe it only loses power in
S5. Can you reword this statement?

The fighting pulls should be resolved though. Or maybe it is better to
simply not put any pull on the line? Presumably the pull is there to
avoid seeing 0->1 transitions on the data lines when inactive, but I'm
not really convinced that is going to happen because the SPI chip itself
would have to be doing that driving, and the chip select isn't changing.

>
> Originally the pullup was picked because SPI transfers are active low
> and thus the high state is somewhat more "idle", but that really isn't
> that important because the chip select won't be asserted when the bus
> is idle. The chip select has a nice external pullup on it that's
> powered by the same power rail as the SPI flash.
>
> This shouldn't have any functionality impact w/ reading/writing the
> SPI since the lines are always push-pull when SPI transfers are
> actually taking place.
>

Right.

Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Fix trogdor qspi pull direction

[Stephen Boyd]

Quoting Stephen Boyd (2023-02-15 21:46:52)
> Quoting Douglas Anderson (2023-02-13 16:57:51)
> > Though it shouldn't matter very much, we've decided that it's slightly
> > better to park the qspi lines for trogdor with an internal pulldown
> > instead of an internal pullup. There was a footnote that Cr50 (which
> > connects to these lines too) may have pulldowns configured on one of
> > the data lines and we don't want to have fighting pulls.
>
> Ok.
>
> > This also
> > means that if the pulls somehow get left powered in S3 (which I'm
> > uncertain about) that they won't be pulling up lines on an unpowered
> > SPI part.
>
> As far as I know, the pulls are maintained in S3. There's verbage about
> "keeper" on the pins.
>
> The SPI part is powered in S3 though. I believe it only loses power in
> S5. Can you reword this statement?

I see that we list pp1800_l13a in sc7180-trogdor.dtsi but don't mark it
always on. I suspect it is turned off at late init, but then wifi turns
it on itself because it is the IO voltage for the wcn chip. We're at the
mercy of the wifi firmware here? Shouldn't we just mark it always on and
boot on? I wonder how this is working.