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: <985e3982-e9c6-53d0-1aa8-7c8f7726926a@linaro.org> Date: Mon, 3 Oct 2022 19:45:31 +0200 From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> To: Doug Anderson <dianders@...omium.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 On 03/10/2022 18:14, Doug Anderson wrote: > 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>; 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@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? 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
Powered by blists - more mailing lists