| Message ID | 20230203-evk-board-support-v7-8-98cbdfac656e@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,01/11] arm64: defconfig: enable MT6357 regulator | expand |
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 { > }; > }; > > +ðernet { > + pinctrl-0 = <ðernet_pins>; > + pinctrl-names = "default"; > + phy-handle = <ð_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
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 { > > }; > > }; > > > > +ðernet { > > + pinctrl-0 = <ðernet_pins>; > > + pinctrl-names = "default"; > > + phy-handle = <ð_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 --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 { }; }; +ðernet { + pinctrl-0 = <ðernet_pins>; + pinctrl-names = "default"; + phy-handle = <ð_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>;