| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAD=FV=UaSAvppTqqsZzNh7x_VZ5pVPROLP4AinK2NEWMUPnoQw@mail.gmail.com>
Date: Mon, 3 Oct 2022 09:14:12 -0700
From: Doug Anderson <dianders@...omium.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.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
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>;
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?
> /* 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
Powered by blists - more mailing lists