Re: [PATCH v2 2/3] arm64: dts: qcom: sc7180: align TLMM pin configuration with DT schema

[Doug Anderson]

Hi,

On Thu, Oct 13, 2022 at 11:49 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index eae22e6e97c1..37abe131951c 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi

[ ... cut ... ]

>  &spi0 {
> -       pinctrl-0 = <&qup_spi0_cs_gpio_init_high>, <&qup_spi0_cs_gpio>;
> +       pinctrl-0 = <&qup_spi0_cs_gpio_init_high>, <&qup_spi0_spi>;
>         cs-gpios = <&tlmm 37 GPIO_ACTIVE_LOW>;
>  };

Something still looks wrong with the above. I would have expected:

  <&qup_spi0_cs_gpio_init_high>, <&qup_spi0_spi>, <&qup_spi0_cs_gpio>;

Specifically the old commit e440e30e26dd ("arm64: dts: qcom: sc7180:
Avoid glitching SPI CS at bootup on trogdor") only worked correctly
because "qup_spi0_cs_gpio_init_high" didn't specify a "function".
That meant it was guaranteed to _just_ set the GPIO output to be
high without changing the mux. Then later we'd change the mux and
the output would already be high and we'd have no glitch.

As I mentioned earlier, I didn't love that solution but I didn't
see a better way. Specifically, I don't think that the properties
within a device tree node are ordered. Thus with your new definition:

  qup_spi0_cs_gpio_init_high: qup-spi0-cs-gpio-init-high-state {
    pins = "gpio37";
    function = "gpio";
    output-high;
  };

Nothing tells the pinctrl subsystem whether it should apply the
'output-high' before the 'function = "gpio"' or vice versa. From
my previous investigation it seemed to set the function first
and then the output to be high. Maybe that's because I happened
to list the function first, but I wouldn't have thought it was
legal to rely on the ordering of properties.

On the other hand, values within a property _are_ ordered. That
means that when we specify:

 <&qup_spi0_cs_gpio_init_high>, <&qup_spi0_spi>, <&qup_spi0_cs_gpio>;

The pinctrl subsystem can see that we want "init_high" done first,
then the SPI pins setup, and then the GPIO setup.

I confirmed that with your patches applied that the EC was reporting
a glitch, though I haven't (yet) managed to reproduce the cros-ec
probe failure that we were seeing in the past.

Unfortunately, I then reverted your patches and the EC was _still_
glitching. :( It looks like things broke in commit b991f8c3622c ("pinctrl:
core: Handling pinmux and pinconf separately"). :( Sure enough,
reverting that patch fixes the glitching.

OK, several hours later and I've come up with a proposed solution [1].
Assuming that solution lands, then I think the answer is:

a) Totally get rid of the '_init_high' entries.
b) trogdor should just specify:
   <&qup_spi0_spi>, <&qup_spi0_cs_gpio>;

[ ... cut ... ]

> +&qup_spi0_spi {
> +       drive-strength = <2>;
> +       bias-disable;
>  };
>
>  &qup_spi0_cs_gpio {
> -       pinconf {
> -               pins = "gpio34", "gpio35", "gpio36", "gpio37";
> -               drive-strength = <2>;
> -               bias-disable;
> -       };
> +       drive-strength = <2>;
> +       bias-disable;
> +};
> +
> +&qup_spi6_spi {
> +       drive-strength = <2>;
> +       bias-disable;
>  };
>
>  &qup_spi6_cs_gpio {
> -       pinconf {
> -               pins = "gpio59", "gpio60", "gpio61", "gpio62";
> -               drive-strength = <2>;
> -               bias-disable;
> -       };
> +       drive-strength = <2>;
> +       bias-disable;
> +};
> +
> +&qup_spi10_spi {
> +       drive-strength = <2>;
> +       bias-disable;
>  };
>
>  &qup_spi10_cs_gpio {
> -       pinconf {
> -               pins = "gpio86", "gpio87", "gpio88", "gpio89";
> -               drive-strength = <2>;
> -               bias-disable;
> -       };
> +       drive-strength = <2>;
> +       bias-disable;
>  };

Mostly addressed by the above, but it should be noted that in your
patch you were specifying settings in the trogdor.dtsi file for
"qup_spi#_cs_gpio" but then never using it (it used the _init_high
versions).

[1] https://lore.kernel.org/r/20221014103217.1.I656bb2c976ed626e5d37294eb252c1cf3be769dc@changeid