[PATCH] arm64: dts: qcom: sc7180: Avoid glitching SPI CS at bootup on trogdor

[PATCH] arm64: dts: qcom: sc7180: Avoid glitching SPI CS at bootup on trogdor

[Douglas Anderson]

At boot time the following happens:
1. Device core gets ready to probe our SPI driver.
2. Device core applies SPI controller's "default" pinctrl.
3. Device core calls the SPI driver's probe() function which will
   eventually setup the chip select GPIO as "unasserted".

Thinking about the above, we can find:
a) For SPI devices that the BIOS inits (Cr50 and EC), the BIOS would
   have had them configured as "GENI" pins and not as "GPIO" pins.
b) It turns out that our BIOS also happens to init these pins as
   "output" (even though it doesn't need to since they're not muxed as
   GPIO) but leaves them at the default state of "low".
c) As soon as we apply the "default" chip select it'll switch the
   function to GPIO and stop driving the chip select high (which is
   how "GENI" was driving it) and start driving it low.
d) As of commit 9378f46040be ("UPSTREAM: spi: spi-geni-qcom: Use the
   new method of gpio CS control"), when the SPI core inits things it
   inits the GPIO to be "deasserted".  Prior to that commit the GPIO
   was left untouched until first use.
e) When the first transaction happens we'll assert the chip select and
   then deassert it after done.

So before the commit to change us to use gpio descriptors we used to
have a _really long_ assertion of chip select before our first
transaction (because it got pulled down and then the first "assert"
was a no-op).  That wasn't great but (apparently) didn't cause any
real harm.

After the commit to change us to use gpio descriptors we end up
glitching the chip select line during probe.  It would go low and then
high with no data transferred.  The other side ought to be robust
against this, but it certainly could cause some confusion.  It's known
to at least cause an error message on the EC console and it's believed
that, under certain timing conditions, it could be getting the EC into
a confused state causing the EC driver to fail to probe.

Let's fix things to avoid the glitch.  We'll add an extra pinctrl
entry that sets the value of the pin to output high (CS deasserted)
before doing anything else.  We'll do this in its own pinctrl node
that comes before the normal pinctrl entries to ensure that the order
is correct and that this gets applied before the mux change.

This change is in the trogdor board file rather than in the SoC dtsi
file because chip select polarity can be different depending on what's
hooked up and it doesn't feel worth it to spam the SoC dtsi file with
both options.  The board file would need to pick the right one anyway.

Fixes: cfbb97fde694 ("arm64: dts: qcom: Switch sc7180-trogdor to control SPI CS via GPIO")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 27 +++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 07c8b2c926c0..e6c58d12dacd 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -768,17 +768,17 @@ &sdhc_2 {
 };
 
 &spi0 {
-	pinctrl-0 = <&qup_spi0_cs_gpio>;
+	pinctrl-0 = <&qup_spi0_cs_gpio_init_high>, <&qup_spi0_cs_gpio>;
 	cs-gpios = <&tlmm 37 GPIO_ACTIVE_LOW>;
 };
 
 &spi6 {
-	pinctrl-0 = <&qup_spi6_cs_gpio>;
+	pinctrl-0 = <&qup_spi6_cs_gpio_init_high>, <&qup_spi6_cs_gpio>;
 	cs-gpios = <&tlmm 62 GPIO_ACTIVE_LOW>;
 };
 
 ap_spi_fp: &spi10 {
-	pinctrl-0 = <&qup_spi10_cs_gpio>;
+	pinctrl-0 = <&qup_spi10_cs_gpio_init_high>, <&qup_spi10_cs_gpio>;
 	cs-gpios = <&tlmm 89 GPIO_ACTIVE_LOW>;
 
 	cros_ec_fp: ec@0 {
@@ -1339,6 +1339,27 @@ pinconf {
 		};
 	};
 
+	qup_spi0_cs_gpio_init_high: qup-spi0-cs-gpio-init-high {
+		pinconf {
+			pins = "gpio37";
+			output-high;
+		};
+	};
+
+	qup_spi6_cs_gpio_init_high: qup-spi6-cs-gpio-init-high {
+		pinconf {
+			pins = "gpio62";
+			output-high;
+		};
+	};
+
+	qup_spi10_cs_gpio_init_high: qup-spi10-cs-gpio-init-high {
+		pinconf {
+			pins = "gpio89";
+			output-high;
+		};
+	};
+
 	qup_uart3_sleep: qup-uart3-sleep {
 		pinmux {
 			pins = "gpio38", "gpio39",
-- 
2.30.0.617.g56c4b15f3c-goog

Re: [PATCH] arm64: dts: qcom: sc7180: Avoid glitching SPI CS at bootup on trogdor

[Doug Anderson]

Hi,

On Thu, Feb 18, 2021 at 2:55 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> it's believed
> that, under certain timing conditions, it could be getting the EC into
> a confused state causing the EC driver to fail to probe.

Believed => confirmed

I _think_ <https://issuetracker.google.com/180655198> is public.  It
explains why this was causing the EC driver to fail to prove.  In
short: it turns out that when we glitched the EC it printed to its
console.  If the EC's uptime was long enough then it would spend
enough time printing the timestamp for this error message (a bunch of
64-bit divide by 10) that it wouldn't be ready for the message we sent
to it.  Doh!

-Doug

Re: [PATCH] arm64: dts: qcom: sc7180: Avoid glitching SPI CS at bootup on trogdor

[Stephen Boyd]

Quoting Douglas Anderson (2021-02-18 14:55:09)
> At boot time the following happens:
> 1. Device core gets ready to probe our SPI driver.
> 2. Device core applies SPI controller's "default" pinctrl.
> 3. Device core calls the SPI driver's probe() function which will
>    eventually setup the chip select GPIO as "unasserted".
> 
> Thinking about the above, we can find:
> a) For SPI devices that the BIOS inits (Cr50 and EC), the BIOS would
>    have had them configured as "GENI" pins and not as "GPIO" pins.
> b) It turns out that our BIOS also happens to init these pins as
>    "output" (even though it doesn't need to since they're not muxed as
>    GPIO) but leaves them at the default state of "low".
> c) As soon as we apply the "default" chip select it'll switch the
>    function to GPIO and stop driving the chip select high (which is
>    how "GENI" was driving it) and start driving it low.
> d) As of commit 9378f46040be ("UPSTREAM: spi: spi-geni-qcom: Use the
>    new method of gpio CS control"), when the SPI core inits things it
>    inits the GPIO to be "deasserted".  Prior to that commit the GPIO
>    was left untouched until first use.
> e) When the first transaction happens we'll assert the chip select and
>    then deassert it after done.
> 
> So before the commit to change us to use gpio descriptors we used to
> have a _really long_ assertion of chip select before our first
> transaction (because it got pulled down and then the first "assert"
> was a no-op).  That wasn't great but (apparently) didn't cause any
> real harm.
> 
> After the commit to change us to use gpio descriptors we end up
> glitching the chip select line during probe.  It would go low and then
> high with no data transferred.  The other side ought to be robust
> against this, but it certainly could cause some confusion.  It's known
> to at least cause an error message on the EC console and it's believed
> that, under certain timing conditions, it could be getting the EC into
> a confused state causing the EC driver to fail to probe.
> 
> Let's fix things to avoid the glitch.  We'll add an extra pinctrl
> entry that sets the value of the pin to output high (CS deasserted)
> before doing anything else.  We'll do this in its own pinctrl node
> that comes before the normal pinctrl entries to ensure that the order
> is correct and that this gets applied before the mux change.
> 
> This change is in the trogdor board file rather than in the SoC dtsi
> file because chip select polarity can be different depending on what's
> hooked up and it doesn't feel worth it to spam the SoC dtsi file with
> both options.  The board file would need to pick the right one anyway.
> 
> Fixes: cfbb97fde694 ("arm64: dts: qcom: Switch sc7180-trogdor to control SPI CS via GPIO")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH] arm64: dts: qcom: sc7180: Avoid glitching SPI CS at bootup on trogdor

[patchwork-bot+linux-arm-msm]

Hello:

This patch was applied to qcom/linux.git (refs/heads/for-next):

On Thu, 18 Feb 2021 14:55:09 -0800 you wrote:
> At boot time the following happens:
> 1. Device core gets ready to probe our SPI driver.
> 2. Device core applies SPI controller's "default" pinctrl.
> 3. Device core calls the SPI driver's probe() function which will
>    eventually setup the chip select GPIO as "unasserted".
> 
> Thinking about the above, we can find:
> a) For SPI devices that the BIOS inits (Cr50 and EC), the BIOS would
>    have had them configured as "GENI" pins and not as "GPIO" pins.
> b) It turns out that our BIOS also happens to init these pins as
>    "output" (even though it doesn't need to since they're not muxed as
>    GPIO) but leaves them at the default state of "low".
> c) As soon as we apply the "default" chip select it'll switch the
>    function to GPIO and stop driving the chip select high (which is
>    how "GENI" was driving it) and start driving it low.
> d) As of commit 9378f46040be ("UPSTREAM: spi: spi-geni-qcom: Use the
>    new method of gpio CS control"), when the SPI core inits things it
>    inits the GPIO to be "deasserted".  Prior to that commit the GPIO
>    was left untouched until first use.
> e) When the first transaction happens we'll assert the chip select and
>    then deassert it after done.
> 
> [...]

Here is the summary with links:
  - arm64: dts: qcom: sc7180: Avoid glitching SPI CS at bootup on trogdor
    https://git.kernel.org/qcom/c/deb625f19bc8

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html