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

From: Krzysztof Kozlowski
Date: Mon Oct 03 2022 - 13:46:05 EST

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.

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?

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