[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@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(-)
  

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