diff mbox series

[v3,06/13] arm64: dts: mediatek: mt7988: add basic ethernet-nodes

Message ID 20250608211452.72920-7-linux@fw-web.de
State New
Headers show
Series further mt7988 devicetree work | expand

Commit Message

Frank Wunderlich June 8, 2025, 9:14 p.m. UTC
From: Frank Wunderlich <frank-w@public-files.de>

Add basic ethernet related nodes.

Mac1+2 needs pcs (sgmii+usxgmii) to work correctly which will be linked
later when driver is merged.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 124 +++++++++++++++++++++-
 1 file changed, 121 insertions(+), 3 deletions(-)

Comments

Andrew Lunn June 8, 2025, 9:23 p.m. UTC | #1
> +			gmac0: mac@0 {
> +				compatible = "mediatek,eth-mac";
> +				reg = <0>;
> +				phy-mode = "internal";
> +
> +				fixed-link {
> +					speed = <10000>;
> +					full-duplex;
> +					pause;
> +				};

Maybe i've asked this before? What is on the other end of this link?
phy-mode internal and fixed link seems an odd combination. It might
just need some comments, if this is internally connected to a switch.

> +			mdio_bus: mdio-bus {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				/* internal 2.5G PHY */
> +				int_2p5g_phy: ethernet-phy@f {
> +					reg = <15>;

It is a bit odd mixing hex and decimal.

> +					compatible = "ethernet-phy-ieee802.3-c45";

I _think_ the coding standard say the compatible should be first.

> +					phy-mode = "internal";

A phy should not have a phy-mode.

	Andrew
Daniel Golle June 8, 2025, 9:24 p.m. UTC | #2
On Sun, Jun 08, 2025 at 11:14:39PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add basic ethernet related nodes.
> 
> Mac1+2 needs pcs (sgmii+usxgmii) to work correctly which will be linked
> later when driver is merged.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 124 +++++++++++++++++++++-
>  1 file changed, 121 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> index 560ec86dbec0..ee1e01d720fe 100644
> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> +
> +		eth: ethernet@15100000 {
> +			compatible = "mediatek,mt7988-eth";
> +			reg = <0 0x15100000 0 0x80000>,
> +			      <0 0x15400000 0 0x200000>;
> +			interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>;

It would be better to use MT7988 with RSS and add the additional
interrupts for doing so before introducing support for this SoC without
RSS. In this way we would avoid having to deal with keeping the DT
support compatible with the old (ie. 4 IRQs) way while also supporting the
RSS-way (with a total of 8 IRQs).
Alternatively, if we really want to support MT7988 with and without RSS we
should use 'interrupt-names' instead of unnamed interrupts imho.
Frank Wunderlich June 9, 2025, 10:28 a.m. UTC | #3
Hi Andrew

> Gesendet: Sonntag, 8. Juni 2025 um 23:23
> Von: "Andrew Lunn" <andrew@lunn.ch>
> An: "Frank Wunderlich" <linux@fw-web.de>
> Betreff: Re: [PATCH v3 06/13] arm64: dts: mediatek: mt7988: add basic ethernet-nodes
>
> > +			gmac0: mac@0 {
> > +				compatible = "mediatek,eth-mac";
> > +				reg = <0>;
> > +				phy-mode = "internal";
> > +
> > +				fixed-link {
> > +					speed = <10000>;
> > +					full-duplex;
> > +					pause;
> > +				};
> 
> Maybe i've asked this before? What is on the other end of this link?
> phy-mode internal and fixed link seems an odd combination. It might
> just need some comments, if this is internally connected to a switch.

yes you've asked in v1 and i responded :)

https://patchwork.kernel.org/project/linux-mediatek/patch/20250511141942.10284-9-linux@fw-web.de/

connected to internal (mt7530) switch. Which kind of comment do you want here? Only "connected to internal switch"
or some more details?

> > +			mdio_bus: mdio-bus {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				/* internal 2.5G PHY */
> > +				int_2p5g_phy: ethernet-phy@f {
> > +					reg = <15>;
> 
> It is a bit odd mixing hex and decimal.

do you prefer hex or decimal for both? for r3mini i used decimal for both, so i would change unit-address
to 15.

> > +					compatible = "ethernet-phy-ieee802.3-c45";
> 
> I _think_ the coding standard say the compatible should be first.

i can move this up of course

> > +					phy-mode = "internal";
> 
> A phy should not have a phy-mode.

not sure if this is needed for mt7988 internal 2.5g phy driver, but seems not when i look at the driver
(drivers/net/phy/mediatek/mtk-2p5ge.c). The switch phys also use this and also here i do not see any
access in the driver (drivers/net/dsa/mt7530-mmio.c + mt7530.c) on a quick look.
Afaik binding required the property and should be read by phylink (to be not unknown, but looks like
handled the same way).

Maybe daniel can describe a bit deeper.

> 	Andrew

regards Frank
Andrew Lunn June 9, 2025, 12:12 p.m. UTC | #4
> > > +			gmac0: mac@0 {
> > > +				compatible = "mediatek,eth-mac";
> > > +				reg = <0>;
> > > +				phy-mode = "internal";
> > > +
> > > +				fixed-link {
> > > +					speed = <10000>;
> > > +					full-duplex;
> > > +					pause;
> > > +				};
> > 
> > Maybe i've asked this before? What is on the other end of this link?
> > phy-mode internal and fixed link seems an odd combination. It might
> > just need some comments, if this is internally connected to a switch.
> 
> yes you've asked in v1 and i responded :)
> 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20250511141942.10284-9-linux@fw-web.de/
> 
> connected to internal (mt7530) switch. Which kind of comment do you want here? Only "connected to internal switch"
> or some more details?

"Connected to internal switch" will do. The word switch explains the
fixed-link, and internal the phy-mode.

It is not the case here, but i've seen DT misused like this because
the MAC is connected to a PHY and there is no PHY driver yet, so a
fixed link is used instead.

> > > +			mdio_bus: mdio-bus {
> > > +				#address-cells = <1>;
> > > +				#size-cells = <0>;
> > > +
> > > +				/* internal 2.5G PHY */
> > > +				int_2p5g_phy: ethernet-phy@f {
> > > +					reg = <15>;
> > 
> > It is a bit odd mixing hex and decimal.
> 
> do you prefer hex or decimal for both? for r3mini i used decimal for both, so i would change unit-address
> to 15.

I suspect decimal is more common, but i don't care.

> 
> > > +					compatible = "ethernet-phy-ieee802.3-c45";
> > 
> > I _think_ the coding standard say the compatible should be first.
> 
> i can move this up of course
> 
> > > +					phy-mode = "internal";
> > 
> > A phy should not have a phy-mode.
> 
> not sure if this is needed for mt7988 internal 2.5g phy driver, but seems not when i look at the driver
> (drivers/net/phy/mediatek/mtk-2p5ge.c). The switch phys also use this and also here i do not see any
> access in the driver (drivers/net/dsa/mt7530-mmio.c + mt7530.c) on a quick look.
> Afaik binding required the property and should be read by phylink (to be not unknown, but looks like
> handled the same way).

Which binding requires this? This is a PHY node, but i don't see
anything about it in ethernet-phy.yaml.

	Andrew
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
index 560ec86dbec0..ee1e01d720fe 100644
--- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
@@ -680,7 +680,28 @@  xphyu3port0: usb-phy@11e13000 {
 			};
 		};
 
-		clock-controller@11f40000 {
+		xfi_tphy0: phy@11f20000 {
+			compatible = "mediatek,mt7988-xfi-tphy";
+			reg = <0 0x11f20000 0 0x10000>;
+			resets = <&watchdog 14>;
+			clocks = <&xfi_pll CLK_XFIPLL_PLL_EN>,
+				 <&topckgen CLK_TOP_XFI_PHY_0_XTAL_SEL>;
+			clock-names = "xfipll", "topxtal";
+			mediatek,usxgmii-performance-errata;
+			#phy-cells = <0>;
+		};
+
+		xfi_tphy1: phy@11f30000 {
+			compatible = "mediatek,mt7988-xfi-tphy";
+			reg = <0 0x11f30000 0 0x10000>;
+			resets = <&watchdog 15>;
+			clocks = <&xfi_pll CLK_XFIPLL_PLL_EN>,
+				 <&topckgen CLK_TOP_XFI_PHY_1_XTAL_SEL>;
+			clock-names = "xfipll", "topxtal";
+			#phy-cells = <0>;
+		};
+
+		xfi_pll: clock-controller@11f40000 {
 			compatible = "mediatek,mt7988-xfi-pll";
 			reg = <0 0x11f40000 0 0x1000>;
 			resets = <&watchdog 16>;
@@ -714,19 +735,116 @@  phy_calibration_p3: calib@97c {
 			};
 		};
 
-		clock-controller@15000000 {
+		ethsys: clock-controller@15000000 {
 			compatible = "mediatek,mt7988-ethsys", "syscon";
 			reg = <0 0x15000000 0 0x1000>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 		};
 
-		clock-controller@15031000 {
+		ethwarp: clock-controller@15031000 {
 			compatible = "mediatek,mt7988-ethwarp";
 			reg = <0 0x15031000 0 0x1000>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 		};
+
+		eth: ethernet@15100000 {
+			compatible = "mediatek,mt7988-eth";
+			reg = <0 0x15100000 0 0x80000>,
+			      <0 0x15400000 0 0x200000>;
+			interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ethsys CLK_ETHDMA_CRYPT0_EN>,
+				 <&ethsys CLK_ETHDMA_FE_EN>,
+				 <&ethsys CLK_ETHDMA_GP2_EN>,
+				 <&ethsys CLK_ETHDMA_GP1_EN>,
+				 <&ethsys CLK_ETHDMA_GP3_EN>,
+				 <&ethwarp CLK_ETHWARP_WOCPU2_EN>,
+				 <&ethwarp CLK_ETHWARP_WOCPU1_EN>,
+				 <&ethwarp CLK_ETHWARP_WOCPU0_EN>,
+				 <&ethsys CLK_ETHDMA_ESW_EN>,
+				 <&topckgen CLK_TOP_ETH_GMII_SEL>,
+				 <&topckgen CLK_TOP_ETH_REFCK_50M_SEL>,
+				 <&topckgen CLK_TOP_ETH_SYS_200M_SEL>,
+				 <&topckgen CLK_TOP_ETH_SYS_SEL>,
+				 <&topckgen CLK_TOP_ETH_XGMII_SEL>,
+				 <&topckgen CLK_TOP_ETH_MII_SEL>,
+				 <&topckgen CLK_TOP_NETSYS_SEL>,
+				 <&topckgen CLK_TOP_NETSYS_500M_SEL>,
+				 <&topckgen CLK_TOP_NETSYS_PAO_2X_SEL>,
+				 <&topckgen CLK_TOP_NETSYS_SYNC_250M_SEL>,
+				 <&topckgen CLK_TOP_NETSYS_PPEFB_250M_SEL>,
+				 <&topckgen CLK_TOP_NETSYS_WARP_SEL>,
+				 <&ethsys CLK_ETHDMA_XGP1_EN>,
+				 <&ethsys CLK_ETHDMA_XGP2_EN>,
+				 <&ethsys CLK_ETHDMA_XGP3_EN>;
+			clock-names = "crypto", "fe", "gp2", "gp1",
+				      "gp3",
+				      "ethwarp_wocpu2", "ethwarp_wocpu1",
+				      "ethwarp_wocpu0", "esw", "top_eth_gmii_sel",
+				      "top_eth_refck_50m_sel", "top_eth_sys_200m_sel",
+				      "top_eth_sys_sel", "top_eth_xgmii_sel",
+				      "top_eth_mii_sel", "top_netsys_sel",
+				      "top_netsys_500m_sel", "top_netsys_pao_2x_sel",
+				      "top_netsys_sync_250m_sel",
+				      "top_netsys_ppefb_250m_sel",
+				      "top_netsys_warp_sel","xgp1", "xgp2", "xgp3";
+			assigned-clocks = <&topckgen CLK_TOP_NETSYS_2X_SEL>,
+					  <&topckgen CLK_TOP_NETSYS_GSW_SEL>,
+					  <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>,
+					  <&topckgen CLK_TOP_USXGMII_SBUS_1_SEL>,
+					  <&topckgen CLK_TOP_SGM_0_SEL>,
+					  <&topckgen CLK_TOP_SGM_1_SEL>;
+			assigned-clock-parents = <&apmixedsys CLK_APMIXED_NET2PLL>,
+						 <&topckgen CLK_TOP_NET1PLL_D4>,
+						 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+						 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+						 <&apmixedsys CLK_APMIXED_SGMPLL>,
+						 <&apmixedsys CLK_APMIXED_SGMPLL>;
+			mediatek,ethsys = <&ethsys>;
+			mediatek,infracfg = <&topmisc>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			gmac0: mac@0 {
+				compatible = "mediatek,eth-mac";
+				reg = <0>;
+				phy-mode = "internal";
+
+				fixed-link {
+					speed = <10000>;
+					full-duplex;
+					pause;
+				};
+			};
+
+			gmac1: mac@1 {
+				compatible = "mediatek,eth-mac";
+				reg = <1>;
+				status = "disabled";
+			};
+
+			gmac2: mac@2 {
+				compatible = "mediatek,eth-mac";
+				reg = <2>;
+				status = "disabled";
+			};
+
+			mdio_bus: mdio-bus {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				/* internal 2.5G PHY */
+				int_2p5g_phy: ethernet-phy@f {
+					reg = <15>;
+					compatible = "ethernet-phy-ieee802.3-c45";
+					phy-mode = "internal";
+				};
+			};
+		};
 	};
 
 	thermal-zones {