Re: [PATCH 1/2] arm64: dts: qcom: sc7180: align TLMM pin configuration with DT schema
From: Doug Anderson
Date: Wed Oct 12 2022 - 13:32:02 EST
Hi,
On Fri, Oct 7, 2022 at 7:51 AM 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.
>
> 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@xxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxxxx>
> ---
> 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?
> 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.
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 ... ]
> @@ -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?
> 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>;
[ ... 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.