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

[Krzysztof Kozlowski]

On 14/10/2022 13:50, Doug Anderson wrote:
> 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>;

Yes, you're right.

> 
> 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.

Regardless of this issue, binding requiring a function does not allow to
keep the pin in previous state. Your glitch-workaround was actually an
use-case for such keep-old-function feature.

Yet, I am not sure if we should keep such ability. The firmware could
configure the pin to whatever. Firmware behavior could also change it
making the OS behavior non-predictable.

> 
> 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>;

Yes.

> 
> [ ... 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

Best regards,
Krzysztof