From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Bjorn Andersson <andersson@kernel.org>,
Andy Gross <agross@kernel.org>,
Konrad Dybcio <konrad.dybcio@somainline.org>,
Linus Walleij <linus.walleij@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
linux-arm-msm@vger.kernel.org, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] arm64: dts: qcom: sc7180: align TLMM pin configuration with DT schema
Date: Thu, 13 Oct 2022 10:59:33 -0400 [thread overview]
Message-ID: <4aa8450f-4504-c65e-56f1-0625584fb8cd@linaro.org> (raw)
In-Reply-To: <CAD=FV=UAcn=yeCZ_jum9kGgqsdKsPpya-FPumYUWO=iyp-kKYw@mail.gmail.com>
On 12/10/2022 13:31, Doug Anderson wrote:
> Hi,
>
> On Fri, Oct 7, 2022 at 7:51 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> DT schema expects TLMM pin configuration nodes to be named with
>> '-state' suffix and their optional children with '-pins' suffix.
>>
>> Merge subnodes named 'pinconf' and 'pinmux' into one entry, add function
>> where missing (required by bindings for GPIOs) and reorganize overriding
>> pins by boards.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Not tested on hardware.
>
> I applied these two patches to the top of mainline today and booted up
> a sc7180-trogdor-coachz. I didn't do any stress testing, but at least
> it boots up and basic smoke tests pass.
>
>> Doug,
>>
>> I think this implements our conclusion from SDM845 patches (alignment of
>> pinctrl with DT schema).
>
> Yeah, it looks really great! Hopefully others agree that this scheme
> looks great and it also validates nicely with your schema changes.
> Sorry for taking a few days to get back--your email coincided with a
> few vacation days for me.
>
> I have a few nits and there's at least one problem (the glitching of
> the SPI chip select at boot).
>
>
>> Cc: Doug Anderson <dianders@chromium.org>
>> ---
>> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 211 +++---
>> .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 36 +-
>> .../dts/qcom/sc7180-trogdor-homestar.dtsi | 41 +-
>> .../dts/qcom/sc7180-trogdor-kingoftown-r0.dts | 16 +-
>> .../dts/qcom/sc7180-trogdor-kingoftown.dtsi | 8 +-
>> .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 16 +-
>> .../dts/qcom/sc7180-trogdor-mrbland-rev0.dtsi | 25 +-
>> .../boot/dts/qcom/sc7180-trogdor-mrbland.dtsi | 72 +-
>> .../qcom/sc7180-trogdor-parade-ps8640.dtsi | 32 +-
>> .../boot/dts/qcom/sc7180-trogdor-pazquel.dtsi | 8 +-
>> .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 14 +-
>> .../qcom/sc7180-trogdor-quackingstick.dtsi | 56 +-
>> .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 8 +-
>> .../dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi | 16 +-
>> .../qcom/sc7180-trogdor-wormdingler-rev0.dtsi | 25 +-
>> .../dts/qcom/sc7180-trogdor-wormdingler.dtsi | 72 +-
>> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 655 +++++++-----------
>> arch/arm64/boot/dts/qcom/sc7180.dtsi | 410 +++++------
>> 18 files changed, 613 insertions(+), 1108 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> index 9dee131b1e24..3e93b13d275e 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>
> [ ...cut... ]
>
>> @@ -642,126 +596,131 @@ pinconf-rts {
>> * pulling RX low (by sending wakeup bytes).
>> */
>> pins = "gpio39";
>> + function = "gpio";
>> bias-pull-down;
>
> optional nit: your new addition makes it obvious that the indentation of the
> surrounding lines is wrong. Maybe fix it as part of this patch?
Indeed, thanks, I'll fix it up.
>
>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
>> index 1bd6c7dcd9e9..c66568a882b3 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
>> @@ -180,30 +180,19 @@ &wifi {
>> /* PINCTRL - modifications to sc7180-trogdor.dtsi */
>>
>> &en_pp3300_dx_edp {
>> - pinmux {
>> - pins = "gpio67";
>> - };
>> -
>> - pinconf {
>> - pins = "gpio67";
>> - };
>> + pins = "gpio67";
>> };
>>
>> &sec_mi2s_active{
>> - pinmux {
>> - pins = "gpio49", "gpio50", "gpio51", "gpio52";
>> - function = "mi2s_1";
>> - };
>> + pins = "gpio49", "gpio50", "gpio51", "gpio52";
>
> Looks like the point of the homestar override is to add an extra pin
> (gpio52) but it forgot to update the list in the "pinconf" as well so
> gpio52 wasn't getting a drive strength and bias set. Your patch
> has the side effect of fixing this. That looks right to me (match
> GPIO51) given that the name of GPIO51 is AMP_DIN and GPIO52 AMP_DIN2.
I miss here something... There was no pinconf in
sc7180.dtsi/sc7180-trogdor-homestar.dtsi/homestar.dts
Where do you see the drive strength and bias set for the gpio49-51?
>
> Assuming my analysis is correct, it's probably worth mentioning this
> behavior change in the commit message.
>
>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index eae22e6e97c1..d923ddca8b8b 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -880,17 +880,17 @@ &sdhc_2 {
>> };
>>
>> &spi0 {
>> - pinctrl-0 = <&qup_spi0_cs_gpio_init_high>, <&qup_spi0_cs_gpio>;
>> + pinctrl-0 = <&qup_spi0_cs_gpio>;
>
> I think this is going to cause a problem. This is pretty much a direct
> revert of commit e440e30e26dd ("arm64: dts: qcom: sc7180: Avoid glitching
> SPI CS at bootup on trogdor").
>
> I was never crazy about the solution in the patch, but I really couldn't
> find another great way to solve it. I think the problem is fairly well
> described in that commit message, at least, and I'm certainly open to
> alternate solutions. Until then, I think this prevents landing your
> patch.
>
> [ ... cut ... ]
Ugh, thanks for noticing this. My patch is here incorrect also because
it is not functionally equivalent - I dropped entirely the output-high
from gpio37 (the CS).
I'll fix it.
>
>> @@ -1467,197 +1315,174 @@ pinconf-rts {
>> * pulling RX low (by sending wakeup bytes).
>> */
>> pins = "gpio39";
>> + function = "gpio";
>> bias-pull-down;
>
> optional nit: your new addition makes it obvious that the indentation of the
> surrounding lines is wrong. Maybe fix it as part of this patch?
Yep
>
>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 58976a1ba06b..8f7845fa669c 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -1486,410 +1486,336 @@ tlmm: pinctrl@3500000 {
>
> [ ... cut ... ]
>
>> - qup_spi0_default: qup-spi0-default {
>> - pinmux {
>> - pins = "gpio34", "gpio35",
>> - "gpio36", "gpio37";
>> - function = "qup00";
>> - };
>> + qup_spi0_default: qup-spi0-default-state {
>> + pins = "gpio34", "gpio35", "gpio36", "gpio37";
>> + function = "qup00";
>> };
>>
>> - qup_spi0_cs_gpio: qup-spi0-cs-gpio {
>> - pinmux {
>> + qup_spi0_cs_gpio: qup-spi0-cs-gpio-state {
>> + qup_spi0_spi: spi-pins {
>> pins = "gpio34", "gpio35",
>> "gpio36";
>> function = "qup00";
>> };
>>
>> - pinmux-cs {
>> + qup_spi0_cs: cs-pins {
>> pins = "gpio37";
>> function = "gpio";
>> };
>> };
>
> The way that the cs_gpio ended up after your patch series threw me for
> a loop. It's counterintutive that we have labels "qup_spi0_spi" and
> "qup_spi0_cs" and they're _not_ under "qup_spi0_default".
>
> Here are two proposals and I'd be happy with either of them:
>
> a) Get rid of the gpio nodes. Instead in the dtsi file do:
>
> qup_spi0_cs_gpio: qup-spi0-cs-gpio-state {
> qup_spi0_spi: spi-pins {
> pins = "gpio34", "gpio35", "gpio36";
> function = "qup00";
> };
>
> qup_spi0_cs: cs-pins {
> pins = "gpio37";
> function = "qup00";
> };
> };
>
> In the board file just:
>
> &qup_spi0_cs {
> function = "gpio";
> };
>
> b) Split the whole thing up. In the dtsi file pinctrl section:
>
> qup_spi0_spi: qup-spi0-spi-state {
> pins = "gpio34", "gpio35", "gpio36";
> function = "qup00";
> };
>
> qup_spi0_cs: qup-spi0-cs-state {
> pins = "gpio37";
> function = "qup00";
> };
>
> qup_spi0_cs_gpio: qup-spi0-cs-gpio-state {
> pins = "gpio37";
> function = "gpio";
> };
>
> ...in the dtsi file SPI section:
>
> pinctrl-0 = <&qup_spi0_spi> <&qup_spi0_cs>;
>
> ...in the board SPI section:
>
> pinctrl-0 = <&qup_spi0_spi> <&qup_spi0_cs_gpio>;
OK, I'll go with the second.
>
> [ ... cut ... ]
>> - qup_uart0_default: qup-uart0-default {
>> - pinmux {
>> - pins = "gpio34", "gpio35",
>> - "gpio36", "gpio37";
>> - function = "qup00";
>> - };
>> + qup_uart0_default: qup-uart0-default-state {
>> + pins = "gpio34", "gpio35", "gpio36", "gpio37";
>> + function = "qup00";
>> };
>
> It feels like all of the UARTs should be split up like uart3/uart8 are
> If any board actually uses these they will likely want different pulls
> and configs for the different pins.
OK
Best regards,
Krzysztof
next prev parent reply other threads:[~2022-10-13 14:59 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-07 14:51 [PATCH 1/2] arm64: dts: qcom: sc7180: align TLMM pin configuration with DT schema Krzysztof Kozlowski
2022-10-07 14:51 ` [PATCH 2/2] dt-bindings: pinctrl: qcom,sc7180: convert to dtschema Krzysztof Kozlowski
2022-10-10 13:00 ` Rob Herring
2022-10-12 17:42 ` Doug Anderson
2022-10-13 12:36 ` Krzysztof Kozlowski
2022-10-12 17:31 ` [PATCH 1/2] arm64: dts: qcom: sc7180: align TLMM pin configuration with DT schema Doug Anderson
2022-10-13 14:59 ` Krzysztof Kozlowski [this message]
2022-10-13 15:11 ` Doug Anderson
2022-10-13 15:28 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4aa8450f-4504-c65e-56f1-0625584fb8cd@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=agross@kernel.org \
--cc=andersson@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=konrad.dybcio@somainline.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).