linux-kernel - Aw: Re: [PATCH v4 4/4] arm64: dts: mediatek: mt7988: add pinctrl support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <trinity-090e30b8-45a9-4a2b-98f1-e34904616b2d-1728651225070@3c-app-gmx-bs32>
Date: Fri, 11 Oct 2024 14:53:45 +0200
From: Frank Wunderlich <frank-w@...lic-files.de>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Frank Wunderlich <linux@...web.de>, Linus Walleij
 <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>, Sean Wang
 <sean.wang@...nel.org>, linux-gpio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 daniel@...rotopia.org, john@...ozen.org, ansuelsmth@...il.com,
 eladwf@...il.com
Subject: Aw: Re: [PATCH v4 4/4] arm64: dts: mediatek: mt7988: add pinctrl
 support

Hi

> Gesendet: Donnerstag, 10. Oktober 2024 um 14:36 Uhr
> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@...labora.com>
> Betreff: Re: [PATCH v4 4/4] arm64: dts: mediatek: mt7988: add pinctrl support
>
> Il 09/10/24 18:52, Frank Wunderlich ha scritto:
> > From: Frank Wunderlich <frank-w@...lic-files.de>
> >
> > Add mt7988a pinctrl node.
> >
> > Signed-off-by: Frank Wunderlich <frank-w@...lic-files.de>
> > ---
> > v2:
> > - fix wrong alignment of reg values
> > ---
> >   arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 241 ++++++++++++++++++++++
> >   1 file changed, 241 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> > index c9649b815276..7e15934efe0b 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> > @@ -3,6 +3,7 @@
> >   #include <dt-bindings/clock/mediatek,mt7988-clk.h>
> >   #include <dt-bindings/interrupt-controller/arm-gic.h>
> >   #include <dt-bindings/phy/phy.h>
> > +#include <dt-bindings/pinctrl/mt65xx.h>
> >
> >   / {
> >   	compatible = "mediatek,mt7988a";
> > @@ -105,6 +106,246 @@ clock-controller@...1e000 {
> >   			#clock-cells = <1>;
> >   		};
> >
> > +		pio: pinctrl@...1f000 {
> > +			compatible = "mediatek,mt7988-pinctrl";
> > +			reg = <0 0x1001f000 0 0x1000>,
> > +			      <0 0x11c10000 0 0x1000>,
> > +			      <0 0x11d00000 0 0x1000>,
> > +			      <0 0x11d20000 0 0x1000>,
> > +			      <0 0x11e00000 0 0x1000>,
> > +			      <0 0x11f00000 0 0x1000>,
> > +			      <0 0x1000b000 0 0x1000>;
> > +			reg-names = "gpio", "iocfg_tr",
> > +				    "iocfg_br", "iocfg_rb",
> > +				    "iocfg_lb", "iocfg_tl", "eint";
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			gpio-ranges = <&pio 0 0 84>;
> > +			interrupt-controller;
> > +			interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-parent = <&gic>;
> > +			#interrupt-cells = <2>;
> > +
> > +			mdio0_pins: mdio0-pins {
> > +				mux {
> > +					function = "eth";
> > +					groups = "mdc_mdio0";
> > +				};
> > +
> > +				conf {
> > +					pins = "SMI_0_MDC", "SMI_0_MDIO";
> > +					drive-strength = <MTK_DRIVE_8mA>;
>
> Please do *not* use the MTK_DRIVE_(x)mA definitions anymore.
>
> Here it is `drive-strength = <8>`.

OK

> > +				};
> > +			};
> > +
> > +			i2c0_pins: i2c0-g0-pins {
> > +				mux {
> > +					function = "i2c";
> > +					groups = "i2c0_1";
> > +				};
> > +			};
> > +
> > +			i2c1_pins: i2c1-g0-pins {
> > +				mux {
> > +					function = "i2c";
> > +					groups = "i2c1_0";
> > +				};
> > +			};
>
> Whatever pin can be configured with one or multiple groups that can be different
> must *not* be in the SoC dtsi, but rather in the *board* dts(i) file, as the wanted
> configuration of those pins is *not* soc-specific but board-specific.
>
>  From a fast look, I can see that at least the I2C pins can be assigned to different
> functions: for example, pins 15+16 can be either of i2c0_1, *or* u30_phy_i2c0, *or*
> u32_phy_i2c0, *or* xfi_phy0_i2c1 ... or others, even.
>
> Finally - I think that *most* of the muxing that you're declaring here must instead
> go to your board specific devicetree and not in mt7988a.dtsi.

As far as i see also mdio and uart0 sharing pins with other pin definitions.
It looks for me that nearly all (except pcie) needs to go in board(s) dts then...
imho this creates duplicates of same nodes, if 2 boards using the same pinconf.
But if it is the way to go, i drop all subnodes except the pcie-pins.

> Cheers,
> Angelo

regards Frank