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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 03/10/2022 18:14, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 30, 2022 at 1:06 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> DT schema expects TLMM pin configuration nodes to be named with
>> '-state' suffix and their optional children with '-pins' suffix.
>>
>> The sdm854.dtsi file defined several pin configuration nodes which are
>> customized by the boards.  Therefore keep a additional "default-pins"
>> node inside so the boards can add more of configuration nodes.  Such
>> additional configuration nodes always need 'function' property now
>> (required by DT schema).
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi    | 344 +++++++-----------
>>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts    |  76 ++--
>>  .../arm64/boot/dts/qcom/sdm845-lg-common.dtsi |  60 ++-
>>  arch/arm64/boot/dts/qcom/sdm845-lg-judyln.dts |   2 +-
>>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts       |  60 ++-
>>  .../boot/dts/qcom/sdm845-oneplus-common.dtsi  |  88 ++---
>>  .../boot/dts/qcom/sdm845-shift-axolotl.dts    | 138 +++----
>>  .../dts/qcom/sdm845-sony-xperia-tama.dtsi     |   6 +-
>>  .../boot/dts/qcom/sdm845-xiaomi-beryllium.dts |  26 +-
>>  .../boot/dts/qcom/sdm845-xiaomi-polaris.dts   |  30 +-
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi          | 305 +++++++---------
>>  .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts |  33 +-
>>  .../boot/dts/qcom/sdm850-samsung-w737.dts     |  96 ++---
>>  13 files changed, 513 insertions(+), 751 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
>> index b5f11fbcc300..3403cdcdd49c 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
>> @@ -993,21 +993,21 @@ &wifi {
>>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
>>
>>  &qspi_cs0 {
>> -       pinconf {
>> +       default-pins {
>>                 pins = "gpio90";
>>                 bias-disable;
>>         };
>>  };
>>
>>  &qspi_clk {
>> -       pinconf {
>> +       default-pins {
>>                 pins = "gpio95";
>>                 bias-disable;
>>         };
>>  };
>>
>>  &qspi_data01 {
>> -       pinconf {
>> +       default-pins {
>>                 pins = "gpio91", "gpio92";
> 
> I haven't been fully involved in all the discussion here, but the
> above doesn't look like it matches the way that Bjorn wanted to go
> [1].  I would sorta expect it to look like this:
> 
>   /* QSPI always needs a clock and IO pins */
>   qspi_basic: {
>     qspi_clk: {
>       pins = "gpio95";
>       function = "qspi_clk";
>     };
>     qspi_data01: {
>       pins = "gpio95";
>       function = "qspi_clk";
>     };
>   }
> 
>   /* QSPI will need one or both chip selects */
>   qspi_cs0: qspi-cs0-state {
>     pins = "gpio90";
>     function = "qspi_cs";
>   };
> 
>   qspi_cs1: qspi-cs1-state {
>     pins = "gpio89";
>     function = "qspi_cs";
>   };
> 
>   /* If using all 4 data lines we need these */
>   qspi_data12: qspi-data12-state {
>     pins = "gpio93", "gpio94";
>     function = "qspi_data";
>   };
> 
> Basically grouping things together in a two-level node when it makes
> sense and then using 1-level nodes for "mixin" pins. Then you'd end up
> doing one of these things:
> 
> pinctrl-0 = <&qspi_basic &qspi_cs0>;
> pinctrl-0 = <&qspi_basic &qspi_cs1>;
> pinctrl-0 = <&qspi_basic &qspi_cs0 &qspi_data12>;


I don't get how my patch changes the existing approach? Such pattern was
already there.

Again - you end up or you ended up? We discuss here what this patch did.
So are you sure that this patch did something like that (and it wasn't
already there)?

> 
> Note that the extra tags of "qspi_clk" and "qspi_data01" are important
> since it lets the individual boards easily set pulls / drive strengths
> without needing to replicate the hierarchy of the SoC. So if a board
> wanted to set the pull of the cs0 line, just:
> 
> &qspi_cs0 {
>   bias-disable;
> };
> 
> [1] https://lore.kernel.org/lkml/CAD=FV=VUL4GmjaibAMhKNdpEso_Hg_R=XeMaqah1LSj_9-Ce4Q@mail.gmail.com/
> 
> 
>> @@ -1016,7 +1016,7 @@ pinconf {
>>  };
>>
>>  &qup_i2c3_default {
>> -       pinconf {
>> +       default-pins {
>>                 pins = "gpio41", "gpio42";
>>                 drive-strength = <2>;
> 
> I don't see any benefit to two-levels above. Why not just get rid of
> the "default-pins" and put the stuff directly under qup_i2c3_default?

For the same reason I told Konrad?

> 
> 
>>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
>>  &qup_spi2_default {
>> -       pinmux {
>> +       default-pins {
>>                 drive-strength = <16>;
>>         };
>>  };
>>
>>  &qup_uart3_default{
>> -       pinmux {
>> +       default-pins {
>>                 pins = "gpio41", "gpio42", "gpio43", "gpio44";
>>                 function = "qup3";
>>         };
>>  };
>>
>>  &qup_i2c10_default {
>> -       pinconf {
>> +       default-pins {
>>                 pins = "gpio55", "gpio56";
>>                 drive-strength = <2>;
>>                 bias-disable;
>> @@ -1144,37 +1144,37 @@ pinconf {
>>  };
>>
>>  &qup_uart6_default {
>> -       pinmux {
>> -               pins = "gpio45", "gpio46", "gpio47", "gpio48";
>> -               function = "qup6";
>> -       };
>> -
>> -       cts {
>> +       cts-pins {
>>                 pins = "gpio45";
>> +               function = "qup6";
>>                 bias-disable;
>>         };
>>
>> -       rts-tx {
>> +       rts-tx-pins {
>>                 pins = "gpio46", "gpio47";
>> +               function = "qup6";
>>                 drive-strength = <2>;
>>                 bias-disable;
>>         };
>>
>> -       rx {
>> +       rx-pins {
>>                 pins = "gpio48";
>> +               function = "qup6";
>>                 bias-pull-up;
>>         };
>>  };
> 
> I didn't check everything about this patch, but skimming through I
> believe that the uart6 handling here is wrong. You'll end up with:>
>   qup_uart6_default: qup-uart6-default-state {
>     default-pins {
>       pins = "gpio47", "gpio48";
>       function = "qup6";

This piece was removed.

>     };
> 
>     cts-pins {
>       pins = "gpio45";
>       function = "qup6";
>       bias-disable;
>     };
> 
>     rts-tx-pins {
>       pins = "gpio46", "gpio47";
>       function = "qup6";
>       drive-strength = <2>;
>       bias-disable;
>     };
> 
>     rx-pins {
>       pins = "gpio48";
>       function = "qup6";
>       bias-pull-up;
>     };
>   };
> 
> So pins 47 and 48 are each referenced in two nodes. That doesn't seem
> correct to me. IMO, better would have been:

Even though that particular piece was removed, so there is no double
reference, it would still be correct. Or rather - what is there
incorrect? Mentioning pin twice? This is ok, although not necessarily
the most readable.

> In Soc.dtsi:
> 
>   qup_uart6_txrx: qup-uart6-txrx-state {
>     qup_uart6_tx {
>       pins = "gpio47";
>       function = "qup6";
>     };
>     qup_uart6_rx {
>       pins = "gpio48";
>       function = "qup6";
>     };
>   };
>   qup_uart6_cts: qup-uart6-cts-state {
>     pins = "gpio45";
>     function = "qup6";
>   };
>   qup_uart6_rts: qup-uart6-rts-state {
>     pins = "gpio46";
>     function = "qup6";
>   };
> 
> In board.dts:
> 
>   &qup_uart6_cts {
>     bias-disable;
>   };
>   &qup_uart6_rts {
>     drive-strength = <2>;
>     bias-disable;
>   };
>   &qup_uart6_rx {
>     bias-pull-up;
>   };
>   &qup_uart6_tx {
>     drive-strength = <2>;
>     bias-disable;

It's not related to this patchset, but sounds good, please change the
DTS to match it. I can rebase my patch on top of it.

>   };
> 
> Also, as per latest agreement with Bjorn, we should be moving the
> default drive strength to the SoC.dtsi file, so going further:

How is it related to this patch? Sure, feel free to move drive strength
anywhere. We can discuss it. But it is not part of this patch.

> 
> In Soc.dtsi:
> 
>   qup_uart6_txrx: qup-uart6-txrx-state {
>     qup_uart6_tx {
>       pins = "gpio47";
>       function = "qup6";
>       drive-strength = <2>;
>     };
>     qup_uart6_rx {
>       pins = "gpio48";
>       function = "qup6";
>     };
>   };
>   qup_uart6_cts: qup-uart6-cts-state {
>     pins = "gpio45";
>     function = "qup6";
>   };
>   qup_uart6_rts: qup-uart6-rts-state {
>     pins = "gpio46";
>     function = "qup6";
>     drive-strength = <2>;

These are not part of DTSI. They exist in DTS, not in DTSI. You now
introduce a change entirely different than this patchset is doing. It
makes sense on its own, but it is not related to this patchset.

>   };
> 
> In board.dts:
> 
>   &qup_uart6_cts {
>     bias-disable;
>   };
>   &qup_uart6_rts {
>     bias-disable;
>   };
>   &qup_uart6_rx {
>     bias-pull-up;
>   };
>   &qup_uart6_tx {
>     bias-disable;
>   };
> 
> In the SoC.dtsi file we could default to just a tx/rx config:
> 
> pinctrl-0 = <&qup_uart6_txrx>;
> 
> ...if a board had the flow control lines hooked up, it could do:
> 
> pinctrl-0 = <&qup_uart6_txrx &qup_uart6_cts &qup_uart6_rts>;


Best regards,
Krzysztof