From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 199E1C433F5 for ; Mon, 3 Oct 2022 17:46:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229849AbiJCRqB (ORCPT ); Mon, 3 Oct 2022 13:46:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38366 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229795AbiJCRpj (ORCPT ); Mon, 3 Oct 2022 13:45:39 -0400 Received: from mail-lf1-x130.google.com (mail-lf1-x130.google.com [IPv6:2a00:1450:4864:20::130]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6E21D22521 for ; Mon, 3 Oct 2022 10:45:34 -0700 (PDT) Received: by mail-lf1-x130.google.com with SMTP id d18so4608551lfb.0 for ; Mon, 03 Oct 2022 10:45:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date; bh=BJcZjBOQpJMQgVSTADZqsOnO/l6steMhAsQ4dlBAeSE=; b=DnkkOZKNHDxWdF31Zkdiup7xPttF3IyysDHVulbz1ZdalzxoEvQtwkfPusphseNfmP AjEvl6OkkRLyzu9eOr064ZburOZR1RsMO/JCSB+Stnx2LMNih34AQiqyGv13XgrSVwIh sNHkIQo35ZtwrWqpHGWaViRhAtA6zEejeYo8hszZ8r5q/qWLd8NqWkG18pzVrcDQXt1U xU5RqQ1TCpZd6KdeuwWLR9VFGIB8OiIOQuiPaBd3/+TrlgSwAw3tC7mmcc3PxP8wSP/H L8Aoy1HBq6H3my/nYvdsBfh52yN7IsKTQoh7cAKfmtSdNQCvWVz20a+Y4/IFO54pc/Ui mSFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=BJcZjBOQpJMQgVSTADZqsOnO/l6steMhAsQ4dlBAeSE=; b=flvIbybMnO0UyIREnYqrro7cJnuiFmnsxDf3slYVBpATLcHOVVNkxdDfSQs6Rwcpdd 5eNVJDX+xSI6aWrYld1LqzhNAvywJRZqDfYtL5ZGX6yERMjmHsKvgAFSMiohWCagIVBp O6QHmTpgU3OTHLraWgR6iqajrNpjangldp7mRc+hO1TB3Rqjjs9uP00Vfe7SxItLk3Nd ZDdmaL2KzJtmTwmlSs2l/K4p4c6lFgFvi2SboEsG5SZJjIUirRxCIQDaHM+iYVPbE18g pJPnGS+Lpw5c5aCt7tEpawlL1bFvJvX/XM59WjErHOuGshL01A5B/qTIwkNEW7ff1nwU AyMQ== X-Gm-Message-State: ACrzQf1NzusiB0Aqb+nBQrGsBi7YGZGejA4KarWiB9Cpz5aR2E+rWZNP YR5CUY9VZ1nbalFrbiDN7MVnqw== X-Google-Smtp-Source: AMsMyM5SjlEpVCfH7P8AmQWY91kFNxPcJPHWuCmTDmzTyAswKrJ0h/ExH2QCwadEkt8VOqJDdJLERQ== X-Received: by 2002:a05:6512:acb:b0:4a2:500f:af73 with SMTP id n11-20020a0565120acb00b004a2500faf73mr156929lfu.524.1664819132900; Mon, 03 Oct 2022 10:45:32 -0700 (PDT) Received: from [192.168.0.21] (78-11-189-27.static.ip.netia.com.pl. [78.11.189.27]) by smtp.gmail.com with ESMTPSA id h13-20020a056512220d00b0049aa7a56715sm1542592lfu.267.2022.10.03.10.45.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 03 Oct 2022 10:45:32 -0700 (PDT) Message-ID: <985e3982-e9c6-53d0-1aa8-7c8f7726926a@linaro.org> Date: Mon, 3 Oct 2022 19:45:31 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH 1/2] arm64: dts: qcom: sdm845: align TLMM pin configuration with DT schema Content-Language: en-US To: Doug Anderson Cc: Andy Gross , Bjorn Andersson , Konrad Dybcio , Linus Walleij , Rob Herring , Krzysztof Kozlowski , linux-arm-msm , "open list:GPIO SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , LKML References: <20220930200529.331223-1-krzysztof.kozlowski@linaro.org> <20220930200529.331223-2-krzysztof.kozlowski@linaro.org> From: Krzysztof Kozlowski In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 03/10/2022 18:14, Doug Anderson wrote: > Hi, > > On Fri, Sep 30, 2022 at 1:06 PM Krzysztof Kozlowski > 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 >> --- >> 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