On 04/10/2022 00:59, Doug Anderson wrote:
> Hi,
>
> On Mon, Oct 3, 2022 at 10:45 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>
>> On 03/10/2022 18:14, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Sep 30, 2022 at 1:06 PM 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.
>>>>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxx/
>>>
>>>
>>>> @@ -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
