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

[Alexandre Mergnat]

Hi Angelo,

Le lun. 15 mai 2023 à 13:47, AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Signed-off-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
> > ---
> >   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