diff mbox series

[v7,08/11] arm64: dts: mediatek: add ethernet support for mt8365-evk

Message ID 20230203-evk-board-support-v7-8-98cbdfac656e@baylibre.com
State New
Headers
Series Improve the MT8365 SoC and EVK board support |

Commit Message

Alexandre Mergnat May 11, 2023, 4:29 p.m. UTC 
  - Enable "vibr" and "vsim2" regulators to power the ethernet chip.

Tested-by: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 57 +++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

Comments

AngeloGioacchino Del Regno May 15, 2023, 11:47 a.m. UTC | #1 
Il 11/05/23 18:29, Alexandre Mergnat ha scritto:
> - Enable "vibr" and "vsim2" regulators to power the ethernet chip.
> 
> Tested-by: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 57 +++++++++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> index 3a472f620ac0..cf81dace466a 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> @@ -88,6 +88,28 @@ optee_reserved: optee@43200000 {
>   	};
>   };
>   
> +&ethernet {
> +	pinctrl-0 = <&ethernet_pins>;
> +	pinctrl-names = "default";
> +	phy-handle = <&eth_phy>;
> +	phy-mode = "rmii";
> +	/*
> +	 * Ethernet and HDMI (DSI0) are sharing pins.
> +	 * Only one can be enabled at a time and require the physical switch
> +	 * SW2101 to be set on LAN position
> +	 */
> +	status = "disabled";
> +
> +	mdio {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		eth_phy: ethernet-phy@0 {
> +			reg = <0>;
> +		};
> +	};
> +};
> +
>   &i2c0 {
>   	clock-frequency = <100000>;
>   	pinctrl-0 = <&i2c0_pins>;
> @@ -137,12 +159,47 @@ &mt6357_pmic {
>   	#interrupt-cells = <2>;
>   };
>   
> +/* Needed by analog switch (multiplexer), HDMI and ethernet */

What part of the ethernet HW needs this regulator?

> +&mt6357_vibr_reg {
> +	regulator-always-on;
> +};
> +
>   /* Needed by MSDC IP */
>   &mt6357_vmc_reg {
>   	regulator-always-on;
>   };
>   
> +/* Needed by ethernet */

Same question for this one. If a device needs us to turn on a regulator in
order for it to be powered (read: if the supply is not fixed-on), setting
that supply as always-on is not beneficial for anyone, as eventually in a
power-off sleep/idle/whatever-pm state, this device (whole chip or IP) *will*
leak some amount of power.

If hardware engineers decided to connect a device to a supply that *can be*
shut down entirely there must be a reason, right? :-)

Regards,
Angelo
Alexandre Mergnat May 22, 2023, 2:23 p.m. UTC | #2 
Hi Angelo,

Le lun. 15 mai 2023 à 13:47, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> a écrit :
>
> Il 11/05/23 18:29, Alexandre Mergnat ha scritto:
> > - Enable "vibr" and "vsim2" regulators to power the ethernet chip.
> >
> > Tested-by: Kevin Hilman <khilman@baylibre.com>
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> > ---
> >   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 57 +++++++++++++++++++++++++++++
> >   1 file changed, 57 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> > index 3a472f620ac0..cf81dace466a 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> > @@ -88,6 +88,28 @@ optee_reserved: optee@43200000 {
> >       };
> >   };
> >
> > +&ethernet {
> > +     pinctrl-0 = <&ethernet_pins>;
> > +     pinctrl-names = "default";
> > +     phy-handle = <&eth_phy>;
> > +     phy-mode = "rmii";
> > +     /*
> > +      * Ethernet and HDMI (DSI0) are sharing pins.
> > +      * Only one can be enabled at a time and require the physical switch
> > +      * SW2101 to be set on LAN position
> > +      */
> > +     status = "disabled";
> > +
> > +     mdio {
> > +             #address-cells = <1>;
> > +             #size-cells = <0>;
> > +
> > +             eth_phy: ethernet-phy@0 {
> > +                     reg = <0>;
> > +             };
> > +     };
> > +};
> > +
> >   &i2c0 {
> >       clock-frequency = <100000>;
> >       pinctrl-0 = <&i2c0_pins>;
> > @@ -137,12 +159,47 @@ &mt6357_pmic {
> >       #interrupt-cells = <2>;
> >   };
> >
> > +/* Needed by analog switch (multiplexer), HDMI and ethernet */
>
> What part of the ethernet HW needs this regulator?
>
> > +&mt6357_vibr_reg {
> > +     regulator-always-on;
> > +};
> > +
> >   /* Needed by MSDC IP */
> >   &mt6357_vmc_reg {
> >       regulator-always-on;
> >   };
> >
> > +/* Needed by ethernet */
>
> Same question for this one. If a device needs us to turn on a regulator in
> order for it to be powered (read: if the supply is not fixed-on), setting
> that supply as always-on is not beneficial for anyone, as eventually in a
> power-off sleep/idle/whatever-pm state, this device (whole chip or IP) *will*
> leak some amount of power.
>
> If hardware engineers decided to connect a device to a supply that *can be*
> shut down entirely there must be a reason, right? :-)

In theory yes, but mistakes happen. For MT8195, ethernet is supplied by VSYS.
Curiously, all other SoC except one haven't the regulator drived by
the mdio driver.
Why is it powered by a regulator which can be turned off here, whereas
the ethernet
chip is able to Wake-on-Lan ? Maybe the vibrator regulator has been chosen
to supply enough current for all analog switches (to share pins between ethernet
and HDMI), I don't know.

Should I create a new mdio binding/driver/node related to the ethernet chip to
enable/disable power ? Currently I found only one who does that: "mdio-sun4i",
so I'm not really confident.
OR
Should I introduce the regulator management directly into mtk_star_emac
binding/drive, even if it's more related to mdio ?
OR
Should I put a comment in the DTS which warns that vibr & vmc must be on
to have ethernet working ?

Do you have a better suggestion?

Finally, we are speaking about power optimization where the feature isn't
already supported for this SoC. Can we do this step by step ? Because setting
the regulators always-on doesn't look bad when ethernet is enabled (for WoL).

Do you have a better suggestion?

Regards,
Alex
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
index 3a472f620ac0..cf81dace466a 100644
--- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
@@ -88,6 +88,28 @@  optee_reserved: optee@43200000 {
 	};
 };
 
+&ethernet {
+	pinctrl-0 = <&ethernet_pins>;
+	pinctrl-names = "default";
+	phy-handle = <&eth_phy>;
+	phy-mode = "rmii";
+	/*
+	 * Ethernet and HDMI (DSI0) are sharing pins.
+	 * Only one can be enabled at a time and require the physical switch
+	 * SW2101 to be set on LAN position
+	 */
+	status = "disabled";
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		eth_phy: ethernet-phy@0 {
+			reg = <0>;
+		};
+	};
+};
+
 &i2c0 {
 	clock-frequency = <100000>;
 	pinctrl-0 = <&i2c0_pins>;
@@ -137,12 +159,47 @@  &mt6357_pmic {
 	#interrupt-cells = <2>;
 };
 
+/* Needed by analog switch (multiplexer), HDMI and ethernet */
+&mt6357_vibr_reg {
+	regulator-always-on;
+};
+
 /* Needed by MSDC IP */
 &mt6357_vmc_reg {
 	regulator-always-on;
 };
 
+/* Needed by ethernet */
+&mt6357_vsim2_reg {
+	regulator-always-on;
+};
+
 &pio {
+	ethernet_pins: ethernet-pins {
+		phy_reset_pins {
+			pinmux = <MT8365_PIN_133_TDM_TX_DATA1__FUNC_GPIO133>;
+		};
+
+		rmii_pins {
+			pinmux = <MT8365_PIN_0_GPIO0__FUNC_EXT_TXD0>,
+				 <MT8365_PIN_1_GPIO1__FUNC_EXT_TXD1>,
+				 <MT8365_PIN_2_GPIO2__FUNC_EXT_TXD2>,
+				 <MT8365_PIN_3_GPIO3__FUNC_EXT_TXD3>,
+				 <MT8365_PIN_4_GPIO4__FUNC_EXT_TXC>,
+				 <MT8365_PIN_5_GPIO5__FUNC_EXT_RXER>,
+				 <MT8365_PIN_6_GPIO6__FUNC_EXT_RXC>,
+				 <MT8365_PIN_7_GPIO7__FUNC_EXT_RXDV>,
+				 <MT8365_PIN_8_GPIO8__FUNC_EXT_RXD0>,
+				 <MT8365_PIN_9_GPIO9__FUNC_EXT_RXD1>,
+				 <MT8365_PIN_10_GPIO10__FUNC_EXT_RXD2>,
+				 <MT8365_PIN_11_GPIO11__FUNC_EXT_RXD3>,
+				 <MT8365_PIN_12_GPIO12__FUNC_EXT_TXEN>,
+				 <MT8365_PIN_13_GPIO13__FUNC_EXT_COL>,
+				 <MT8365_PIN_14_GPIO14__FUNC_EXT_MDIO>,
+				 <MT8365_PIN_15_GPIO15__FUNC_EXT_MDC>;
+		};
+	};
+
 	gpio_keys: gpio-keys-pins {
 		pins {
 			pinmux = <MT8365_PIN_24_KPCOL0__FUNC_KPCOL0>;