Message-ID: <6c20f710-e02e-1702-3985-4e995f8a7e7a@linaro.org>
Date:   Tue, 4 Oct 2022 10:40:15 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] arm64: dts: qcom: sdm845: align TLMM pin
 configuration with DT schema

On 04/10/2022 00:59, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 3, 2022 at 10:45 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@...aro.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@...aro.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@...aro.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.

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

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.

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

>>>>  /* 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?

> 
>>>     };
>>>
>>>     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.
> 
> I guess this gets into the corners of pinctrl that I haven't poked at
> lots. I guess it should be OK unless the SoC.dtsi and the board.dts
> disagree about the "function". In such a case I guess it would be a
> problem. So I guess what you end up will be OK but I don't like that
> "function" is specified for the same pin in two different sub-nodes.

OK.

> 
> 
>>> 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.
> 
> I guess it's related in that the patchset is touching everything and
> one would assume that something touched so recently would represent
> the current best practices. Maybe that's a weak argument, but if I saw
> a patch that was about trying to clean up all the pinctrl across all
> the older SoCs that I would assume that the pinctrl would be clean
> after that patch and would be a good example to follow as best
> practice. Thus it's relevant to talk about whether this patch is
> ending us up at best practice or not.
> 
> 
>>>   };
>>>
>>> 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.
> 
> Moving the drive strength can certainly be discussed / done in a later 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.
> 
> It is relevant to discuss because it would be the correct way to solve
> the same issue with "uart9" that you used to justify why you needed an
> extra "uart9-default" subnode.

The goal of this patchset is to solve dtbs_check. It's goal is not
"generic guidance how Qualcomm pin configuration nodes should be
written", because whatever you and I agree, it does not much matter. The
maintainers decision matter.

If there is a consensus/preferred way to organize it, sure, document it
somewhere, store it in email/lore in concise way, and I can fix up the
patch to match it. It is just not my goal now, so I am not pushing
towards any such direction.

Best regards,
Krzysztof

Powered by blists - more mailing lists