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

From: AngeloGioacchino Del Regno
Date: Mon Oct 14 2024 - 04:15:50 EST


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@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.


That's the way to go. Please drop all subnodes from this one except pcie-pins.

Cheers,
Angelo