• Douglas Anderson's avatar
    arm64: dts: qcom: sc7180: Fix trogdor qspi pin config · ab752f03
    Douglas Anderson authored
    In commit 7ec3e673 ("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 d21f4b7f ("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: default avatarDouglas Anderson <dianders@chromium.org>
    Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
    Signed-off-by: default avatarBjorn Andersson <andersson@kernel.org>
    Link: https://lore.kernel.org/r/20230323102605.12.I6f03f86546e6ce9abb1d24fd9ece663c3a5b950c@changeid
    ab752f03
sc7180-idp.dts 14.6 KB