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

[Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>]

Hi

> Gesendet: Donnerstag, 10. Oktober 2024 um 14:36 Uhr
> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> >
> > Add mt7988a pinctrl node.
> >
> > Signed-off-by: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> > ---
> > 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@1001e000 {
> >   			#clock-cells = <1>;
> >   		};
> >
> > +		pio: pinctrl@1001f000 {
> > +			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