| Message ID | 20241009165222.5670-5-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add pinctrl support for mt7988 | expand |
Il 09/10/24 18:52, Frank Wunderlich ha scritto: > From: Frank Wunderlich <frank-w@public-files.de> > > Add mt7988a pinctrl node. > > Signed-off-by: Frank Wunderlich <frank-w@public-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@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>`. > + }; > + }; > + > + 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. Cheers, Angelo
Hi > Gesendet: Donnerstag, 10. Oktober 2024 um 14:36 Uhr > Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.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@public-files.de> > > > > Add mt7988a pinctrl node. > > > > Signed-off-by: Frank Wunderlich <frank-w@public-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@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
Il 11/10/24 14:53, Frank Wunderlich ha scritto: > Hi > >> Gesendet: Donnerstag, 10. Oktober 2024 um 14:36 Uhr >> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.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@public-files.de> >>> >>> Add mt7988a pinctrl node. >>> >>> Signed-off-by: Frank Wunderlich <frank-w@public-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@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. > That's the way to go. Please drop all subnodes from this one except pcie-pins. Cheers, Angelo
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>; + }; + }; + + i2c0_pins: i2c0-g0-pins { + mux { + function = "i2c"; + groups = "i2c0_1"; + }; + }; + + i2c1_pins: i2c1-g0-pins { + mux { + function = "i2c"; + groups = "i2c1_0"; + }; + }; + + i2c1_sfp_pins: i2c1-sfp-g0-pins { + mux { + function = "i2c"; + groups = "i2c1_sfp"; + }; + }; + + i2c2_0_pins: i2c2-g0-pins { + mux { + function = "i2c"; + groups = "i2c2_0"; + }; + }; + + i2c2_1_pins: i2c2-g1-pins { + mux { + function = "i2c"; + groups = "i2c2_1"; + }; + }; + + gbe0_led0_pins: gbe0-led0-pins { + mux { + function = "led"; + groups = "gbe0_led0"; + }; + }; + + gbe1_led0_pins: gbe1-led0-pins { + mux { + function = "led"; + groups = "gbe1_led0"; + }; + }; + + gbe2_led0_pins: gbe2-led0-pins { + mux { + function = "led"; + groups = "gbe2_led0"; + }; + }; + + gbe3_led0_pins: gbe3-led0-pins { + mux { + function = "led"; + groups = "gbe3_led0"; + }; + }; + + gbe0_led1_pins: gbe0-led1-pins { + mux { + function = "led"; + groups = "gbe0_led1"; + }; + }; + + gbe1_led1_pins: gbe1-led1-pins { + mux { + function = "led"; + groups = "gbe1_led1"; + }; + }; + + gbe2_led1_pins: gbe2-led1-pins { + mux { + function = "led"; + groups = "gbe2_led1"; + }; + }; + + gbe3_led1_pins: gbe3-led1-pins { + mux { + function = "led"; + groups = "gbe3_led1"; + }; + }; + + i2p5gbe_led0_pins: 2p5gbe-led0-pins { + mux { + function = "led"; + groups = "2p5gbe_led0"; + }; + }; + + i2p5gbe_led1_pins: 2p5gbe-led1-pins { + mux { + function = "led"; + groups = "2p5gbe_led1"; + }; + }; + + mmc0_pins_emmc_45: mmc0-emmc-45-pins { + mux { + function = "flash"; + groups = "emmc_45"; + }; + }; + + mmc0_pins_emmc_51: mmc0-emmc-51-pins { + mux { + function = "flash"; + groups = "emmc_51"; + }; + }; + + mmc0_pins_sdcard: mmc0-sdcard-pins { + mux { + function = "flash"; + groups = "sdcard"; + }; + }; + + uart0_pins: uart0-pins { + mux { + function = "uart"; + groups = "uart0"; + }; + }; + + snfi_pins: snfi-pins { + mux { + function = "flash"; + groups = "snfi"; + }; + }; + + spi0_pins: spi0-pins { + mux { + function = "spi"; + groups = "spi0"; + }; + }; + + spi0_flash_pins: spi0-flash-pins { + mux { + function = "spi"; + groups = "spi0", "spi0_wp_hold"; + }; + }; + + spi1_pins: spi1-pins { + mux { + function = "spi"; + groups = "spi1"; + }; + }; + + spi2_pins: spi2-pins { + mux { + function = "spi"; + groups = "spi2"; + }; + }; + + spi2_flash_pins: spi2-flash-pins { + mux { + function = "spi"; + groups = "spi2", "spi2_wp_hold"; + }; + }; + + pcie0_pins: pcie0-pins { + mux { + function = "pcie"; + groups = "pcie_2l_0_pereset", "pcie_clk_req_n0_0", + "pcie_wake_n0_0"; + }; + }; + + pcie1_pins: pcie1-pins { + mux { + function = "pcie"; + groups = "pcie_2l_1_pereset", "pcie_clk_req_n1", + "pcie_wake_n1_0"; + }; + }; + + pcie2_pins: pcie2-pins { + mux { + function = "pcie"; + groups = "pcie_1l_0_pereset", "pcie_clk_req_n2_0", + "pcie_wake_n2_0"; + }; + }; + + pcie3_pins: pcie3-pins { + mux { + function = "pcie"; + groups = "pcie_1l_1_pereset", "pcie_clk_req_n3", + "pcie_wake_n3_0"; + }; + }; + }; + pwm@10048000 { compatible = "mediatek,mt7988-pwm"; reg = <0 0x10048000 0 0x1000>;