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

[Doug Anderson <dianders@xxxxxxxxxxxx>]

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

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?


>  /* 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";
    };

    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:

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;
  };

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

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>;
  };

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

-Doug