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

From: Doug Anderson
Date: Mon Oct 03 2022 - 12:14:37 EST

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