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

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

On 04/10/2022 16:55, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 4, 2022 at 1:40 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 04/10/2022 00:59, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Oct 3, 2022 at 10:45 AM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> 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.
>>>
>>> Before your patch things were split in two nodes, the muxing and the
>>> configuration. AKA when you combined the soc.dtsi and the board.dts
>>> you'd have:
>>>
>>> qspi_cs0: qspi-cs0-state {
>>>   pinmux {
>>>     pins = "...";
>>>     ... muxing properties ...
>>>   };
>>>   pinconf {
>>>     pins = "...";
>>>     ... config properties ...
>>>   };
>>> };
>>
>> Which was also not good pattern. Muxing and configuring is basically the
>> same function of pin controller. Splitting them makes no sense at all.
> 
> Agreed, it was not a good pattern.
> 
> 
>>> Your patch combines the "pinmux" and "pinconf" nodes into one. So when
>>> you combine the soc.dtsi and the board.dts after your patch you now
>>> have:
>>>
>>> qspi_cs0: qspi-cs0-state {
>>>   default-pins {
>>>     pins = "...";
>>>     ... muxing properties ...
>>>     ... config properties ...
>>>   };
>>> };
>>>
>>>
>>> That's fine and is functionally correct. ...but IMO it sets a bad
>>> example for people to follow (though, of course, it's really up to
>>> Bjorn). The "default-pins" subnode serves no purpose. If you're
>>> touching all this stuff anyway you might as well not end up with
>>> something that's a bad example for people to follow.
>>
>> I understand, you're right. I wanted to keep minimal amount of changes
>> to the DTS layout but since I am touching almost everything, I can
>> rework it.
>>
>>
>>>> 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?
>>>
>>> OK. I looked at what you end up with for "qup_uart9" after your
>>> patches and it's definitely not my favorite place to end up at. If
>>> nothing else you are double-specifying "function" in both
>>> "default-pins" and "tx-pins"/"rx-pins". If those disagree then what
>>> happens?
>>
>> The same happens and happened before. My patch does nothing related to
>> allowing or disallowing wrong muxing/config nodes. In the past you could
>> have conflicting setups. Now it's exactly the same.
> 
> Right that you could have had conflicting setups before, too. However,
> in the past we always avoided specifying the function for a given pin
> in more than one node.
> 
> The old way was super verbose and had a lot of repetition for sure,
> but it sorta made sense if you thought of it in words. AKA the old way
> said this after combining the soc.dtsi and the board.dts
> 
> * For pins 45, 46, 47, 48: set the function to "qup6"
> * For pin 45: set a pulldown
> * For pins 46 and 47: set the drive strength to 2 and disable pulls
> * For pin 48: set a pullup
> 
> So it repeated the "for pins" part and that was most definitely non
> ideal. However, with your new setup you are sometimes doing:
> 
> * For pins 47 and 48: set the function to "qup6"
> * For pin 45: set the function to "qup6" and set a pulldown
> * For pins 46 and 47: set the function to "qup6" and the drive
> strength to 2 and disable pulls
> * For pin 48: set the function to "qup6" and set a pullup
> 
> That repeats the 'set the function to "qup6"' more than I'd like.

Indeed, it got more duplicated/error-prone.

> 
> 
>> The double-specifying of "function" is not a result of '-state'/'-pins'
>> reorganization but aligning with common TLMM schema *which requires
>> function* for every node.
> 
> I guess now that we've talked through it, I'd say that if we have to
> specify a function for every node then we should strive for each pin
> to be in exactly one node in any given active configuration. That
> makes the schema happy and also avoids double-specifying the function.
> 
> I think in all of the examples I've given that this is true.

OK, we can rework entire DTS to follow this approach.

> 
> 
>>> In general also we end up specifying that extra level of
>>> "default-pins" in many cases for no purpose. We also end up
>>> replicating hierarchy in the board dts files (the dts files are
>>> replicating the "default-pins" nodes from the parent).
>>
>> OK, let's fix this. That will need either:
>> 1. /delete-node with copying of most of properties from default-pins
>> 2. or move the CTS/TX/RX config pins to the DTSI.
> 
> I think in my proposal the CTS/TX/RX stuff moved to the dtsi which solved it.

OK

> 
> 
>>>>>>  /* 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.
>>>
>>> It was? How/where? I tried applying your patch and I still see "qup6"
>>> under the default-pins node in sdm845.dtsi.
>>>
>>
>> Ah, you're right. The default-pins are coming from DTSI.
>>
>> So delete-node?
> 
> I would prefer the solution I proposed. Re-pasting here:
> 
> 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;
>   };
> 

OK

Best regards,
Krzysztof