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

[Krzysztof Kozlowski]

On 12/10/2022 13:31, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 7, 2022 at 7:51 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>
>> DT schema expects TLMM pin configuration nodes to be named with
>> '-state' suffix and their optional children with '-pins' suffix.
>>
>> Merge subnodes named 'pinconf' and 'pinmux' into one entry, add function
>> where missing (required by bindings for GPIOs) and reorganize overriding
>> pins by boards.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>>
>> ---
>>
>> Not tested on hardware.
> 
> I applied these two patches to the top of mainline today and booted up
> a sc7180-trogdor-coachz. I didn't do any stress testing, but at least
> it boots up and basic smoke tests pass.
> 
>> Doug,
>>
>> I think this implements our conclusion from SDM845 patches (alignment of
>> pinctrl with DT schema).
> 
> Yeah, it looks really great! Hopefully others agree that this scheme
> looks great and it also validates nicely with your schema changes.
> Sorry for taking a few days to get back--your email coincided with a
> few vacation days for me.
> 
> I have a few nits and there's at least one problem (the glitching of
> the SPI chip select at boot).
> 
> 
>> Cc: Doug Anderson <dianders@xxxxxxxxxxxx>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7180-idp.dts       | 211 +++---
>>  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  |  36 +-
>>  .../dts/qcom/sc7180-trogdor-homestar.dtsi     |  41 +-
>>  .../dts/qcom/sc7180-trogdor-kingoftown-r0.dts |  16 +-
>>  .../dts/qcom/sc7180-trogdor-kingoftown.dtsi   |   8 +-
>>  .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi   |  16 +-
>>  .../dts/qcom/sc7180-trogdor-mrbland-rev0.dtsi |  25 +-
>>  .../boot/dts/qcom/sc7180-trogdor-mrbland.dtsi |  72 +-
>>  .../qcom/sc7180-trogdor-parade-ps8640.dtsi    |  32 +-
>>  .../boot/dts/qcom/sc7180-trogdor-pazquel.dtsi |   8 +-
>>  .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  |  14 +-
>>  .../qcom/sc7180-trogdor-quackingstick.dtsi    |  56 +-
>>  .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts |   8 +-
>>  .../dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi |  16 +-
>>  .../qcom/sc7180-trogdor-wormdingler-rev0.dtsi |  25 +-
>>  .../dts/qcom/sc7180-trogdor-wormdingler.dtsi  |  72 +-
>>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  | 655 +++++++-----------
>>  arch/arm64/boot/dts/qcom/sc7180.dtsi          | 410 +++++------
>>  18 files changed, 613 insertions(+), 1108 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> index 9dee131b1e24..3e93b13d275e 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> 
> [ ...cut... ]
> 
>> @@ -642,126 +596,131 @@ pinconf-rts {
>>                          * pulling RX low (by sending wakeup bytes).
>>                          */
>>                          pins = "gpio39";
>> +                       function = "gpio";
>>                          bias-pull-down;
> 
> optional nit: your new addition makes it obvious that the indentation of the
> surrounding lines is wrong. Maybe fix it as part of this patch?

Indeed, thanks, I'll fix it up.

> 
> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
>> index 1bd6c7dcd9e9..c66568a882b3 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
>> @@ -180,30 +180,19 @@ &wifi {
>>  /* PINCTRL - modifications to sc7180-trogdor.dtsi */
>>
>>  &en_pp3300_dx_edp {
>> -       pinmux {
>> -               pins = "gpio67";
>> -       };
>> -
>> -       pinconf {
>> -               pins = "gpio67";
>> -       };
>> +       pins = "gpio67";
>>  };
>>
>>  &sec_mi2s_active{
>> -       pinmux {
>> -               pins = "gpio49", "gpio50", "gpio51", "gpio52";
>> -               function = "mi2s_1";
>> -       };
>> +       pins = "gpio49", "gpio50", "gpio51", "gpio52";
> 
> Looks like the point of the homestar override is to add an extra pin
> (gpio52) but it forgot to update the list in the "pinconf" as well so
> gpio52 wasn't getting a drive strength and bias set. Your patch
> has the side effect of fixing this. That looks right to me (match
> GPIO51) given that the name of GPIO51 is AMP_DIN and GPIO52 AMP_DIN2.

I miss here something... There was no pinconf in
sc7180.dtsi/sc7180-trogdor-homestar.dtsi/homestar.dts

Where do you see the drive strength and bias set for the gpio49-51?

> 
> Assuming my analysis is correct, it's probably worth mentioning this
> behavior change in the commit message.
> 
> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index eae22e6e97c1..d923ddca8b8b 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -880,17 +880,17 @@ &sdhc_2 {
>>  };
>>
>>  &spi0 {
>> -       pinctrl-0 = <&qup_spi0_cs_gpio_init_high>, <&qup_spi0_cs_gpio>;
>> +       pinctrl-0 = <&qup_spi0_cs_gpio>;
> 
> I think this is going to cause a problem. This is pretty much a direct
> revert of commit e440e30e26dd ("arm64: dts: qcom: sc7180: Avoid glitching
> SPI CS at bootup on trogdor").
> 
> I was never crazy about the solution in the patch, but I really couldn't
> find another great way to solve it. I think the problem is fairly well
> described in that commit message, at least, and I'm certainly open to
> alternate solutions. Until then, I think this prevents landing your
> patch.
> 
> [ ... cut ... ]

Ugh, thanks for noticing this. My patch  is here incorrect also because
it is not functionally equivalent - I dropped entirely the output-high
from gpio37 (the CS).

I'll fix it.

> 
>> @@ -1467,197 +1315,174 @@ pinconf-rts {
>>                          * pulling RX low (by sending wakeup bytes).
>>                          */
>>                          pins = "gpio39";
>> +                       function = "gpio";
>>                          bias-pull-down;
> 
> optional nit: your new addition makes it obvious that the indentation of the
> surrounding lines is wrong. Maybe fix it as part of this patch?

Yep

> 
> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 58976a1ba06b..8f7845fa669c 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -1486,410 +1486,336 @@ tlmm: pinctrl@3500000 {
> 
> [ ... cut ... ]
> 
>> -                       qup_spi0_default: qup-spi0-default {
>> -                               pinmux {
>> -                                       pins = "gpio34", "gpio35",
>> -                                              "gpio36", "gpio37";
>> -                                       function = "qup00";
>> -                               };
>> +                       qup_spi0_default: qup-spi0-default-state {
>> +                               pins = "gpio34", "gpio35", "gpio36", "gpio37";
>> +                               function = "qup00";
>>                         };
>>
>> -                       qup_spi0_cs_gpio: qup-spi0-cs-gpio {
>> -                               pinmux {
>> +                       qup_spi0_cs_gpio: qup-spi0-cs-gpio-state {
>> +                               qup_spi0_spi: spi-pins {
>>                                         pins = "gpio34", "gpio35",
>>                                                "gpio36";
>>                                         function = "qup00";
>>                                 };
>>
>> -                               pinmux-cs {
>> +                               qup_spi0_cs: cs-pins {
>>                                         pins = "gpio37";
>>                                         function = "gpio";
>>                                 };
>>                         };
> 
> The way that the cs_gpio ended up after your patch series threw me for
> a loop. It's counterintutive that we have labels "qup_spi0_spi" and
> "qup_spi0_cs" and they're _not_ under "qup_spi0_default".
> 
> Here are two proposals and I'd be happy with either of them:
> 
> a) Get rid of the gpio nodes. Instead in the dtsi file do:
> 
>   qup_spi0_cs_gpio: qup-spi0-cs-gpio-state {
>     qup_spi0_spi: spi-pins {
>       pins = "gpio34", "gpio35", "gpio36";
>       function = "qup00";
>     };
> 
>     qup_spi0_cs: cs-pins {
>       pins = "gpio37";
>       function = "qup00";
>     };
>   };
> 
> In the board file just:
> 
>   &qup_spi0_cs {
>     function = "gpio";
>   };
> 
> b) Split the whole thing up. In the dtsi file pinctrl section:
> 
>   qup_spi0_spi: qup-spi0-spi-state {
>     pins = "gpio34", "gpio35", "gpio36";
>     function = "qup00";
>   };
> 
>   qup_spi0_cs: qup-spi0-cs-state {
>     pins = "gpio37";
>     function = "qup00";
>   };
> 
>   qup_spi0_cs_gpio: qup-spi0-cs-gpio-state {
>     pins = "gpio37";
>     function = "gpio";
>   };
> 
> ...in the dtsi file SPI section:
> 
>   pinctrl-0 = <&qup_spi0_spi> <&qup_spi0_cs>;
> 
> ...in the board SPI section:
> 
>   pinctrl-0 = <&qup_spi0_spi> <&qup_spi0_cs_gpio>;

OK, I'll go with the second.

> 
> [ ... cut ... ]
>> -                       qup_uart0_default: qup-uart0-default {
>> -                               pinmux {
>> -                                       pins = "gpio34", "gpio35",
>> -                                              "gpio36", "gpio37";
>> -                                       function = "qup00";
>> -                               };
>> +                       qup_uart0_default: qup-uart0-default-state {
>> +                               pins = "gpio34", "gpio35", "gpio36", "gpio37";
>> +                               function = "qup00";
>>                         };
> 
> It feels like all of the UARTs should be split up like uart3/uart8 are
> If any board actually uses these they will likely want different pulls
> and configs for the different pins.

OK

Best regards,
Krzysztof