Re: [PATCH net v2 1/2] net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards

["SkyLake Huang (黃啟澤)" <SkyLake.Huang@mediatek.com>]

On Thu, 2024-03-21 at 19:29 +0300, Arınç ÜNAL via B4 Relay wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE
> features")
> brought EEE support but did not enable EEE on MT7531 switch MACs. EEE
> is
> enabled on MT7531 switch MACs either by pulling the LAN2LED0 pin low
> on the
> board (bootstrapping), or unsetting the EEE_DIS bit on the trap
> register.
> 
> There are existing boards that were not designed to pull the pin low.
> Therefore, unset the EEE_DIS bit on the trap register.
> 
> Unlike MT7530, the modifiable trap register won't be populated
> identical to
> the trap status register after reset. Therefore, read from the trap
> status
> register, modify the bits, then write to the modifiable trap
> register.
> 
> My testing on MT7531 shows a certain amount of traffic loss when EEE
> is
> enabled. That said, I haven't come across a board that enables EEE.
> So
> enable EEE on the switch MACs but disable EEE advertisement on the
> switch
> PHYs. This way, we don't change the behaviour of the majority of the
> boards
> that have this switch.
> 
> With this change, EEE can now be enabled using ethtool.
> 
> The disable EEE bit on the trap pertains to the LAN2LED0 pin which is
> usually used to control an LED. Once the bit is unset, the pin will
> be low.
> That will make the active low LED turn on.
> 
> The pin is controlled by the switch PHY. It seems that the PHY
> controls the
> pin in the way that it inverts the pin state. That means depending on
> the
> wiring of the LED connected to LAN2LED0 on the board, the LED may be
> on
> without an active link.
> 
> Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE
> features")
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 14 ++++++++++++++
>  drivers/net/dsa/mt7530.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 678b51f9cea6..6aa99b590329 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2458,6 +2458,20 @@ mt7531_setup(struct dsa_switch *ds)
>  /* Reset the switch through internal reset */
>  mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_SW_RST |
> SYS_CTRL_REG_RST);
>  
> +/* Allow modifying the trap and enable Energy-Efficient Ethernet
> (EEE).
> + */
> +val = mt7530_read(priv, MT7531_HWTRAP);
> +val |= CHG_STRAP;
> +val &= ~EEE_DIS;
> +mt7530_write(priv, MT7530_MHWTRAP, val);
You may try to set bit 6 of CL45 register dev=0x1f,
reg=0x403(PLL_GROUP_CTL_REG), which is an internal EEE switch called
RG_SYSPLL_DMY2.

Read it out first with "switch command", if you have it:
root@OpenWrt:/# switch phy cl45 r 0 0x1f 0x403
  Phy read dev_num=0x1f, reg=0x403, value=0x1091

And then set bit 6:
root@OpenWrt:/# $ switch phy cl45 w 0 0x1f 0x403 0x10d1
root@OpenWrt:/# ethtool --set-eee lan1 eee on tx-lpi on tx-timer 0x1e
advertise 0x28
root@OpenWrt:/# switch phy cl45 r 0 0x7 0x3c
 Phy read dev_num=0x7, reg=0x3c, value=0x6

Then you should be able to enable EEE via ethtool without clearing
EEE_DIS bit of MT7530_HWTRAP.
If above CL45 command is good for you, I think this can be moved to
mtk_gephy_config_init() @ mediatek-ge.c, which will lead to cleaner
implementation.
> +
> +/* Disable EEE advertisement on the switch PHYs. */
> +for (i = MT753X_CTRL_PHY_ADDR;
> +     i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) {
> +mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
> + 0);
> +}
Sorry, I still can't figure out why this is needed since we disable
MDIO_AN_EEE_ADV in mediatek-ge.c after reading previous threads. Would
you please provide something else (like dmesg log?) to show that
settings in mtk_gephy_config_init() may fail?
> +
>  if (!priv->p5_sgmii) {
>  mt7531_pll_setup(priv);
>  } else {
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index a71166e0a7fc..509ed5362236 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -457,6 +457,7 @@ enum mt7531_clk_skew {
>  #define  XTAL_FSEL_MBIT(7)
>  #define  PHY_ENBIT(6)
>  #define  CHG_STRAPBIT(8)
> +#define  EEE_DISBIT(4)
>  
>  /* Register for hw trap modification */
>  #define MT7530_MHWTRAP0x7804
> 
> -- 
> 2.40.1
> 
>