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
