On Tue, Jun 25, 2024 at 04:49:30PM -0700, Sagar Cheluvegowda wrote: > When mac link goes down we don't need to mainitain the clocks to operate > at higher frequencies, as an optimized solution to save power when > the link goes down we are trying to bring down the clocks to the > frequencies corresponding to the lowest speed possible. I thought I had already commented on a similar patch, but I can't find anything in my mailboxes to suggest I had. > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index ec7c61ee44d4..f0166f0bc25f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -996,6 +996,9 @@ static void stmmac_mac_link_down(struct phylink_config *config, > { > struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); > > + if (priv->plat->fix_mac_speed) > + priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode); > + > stmmac_mac_set(priv, priv->ioaddr, false); > priv->eee_active = false; > priv->tx_lpi_enabled = false; > @@ -1004,6 +1007,11 @@ static void stmmac_mac_link_down(struct phylink_config *config, > > if (priv->dma_cap.fpesel) > stmmac_fpe_link_state_handle(priv, false); > + > + stmmac_set_icc_bw(priv, SPEED_10); > + > + if (priv->plat->fix_mac_speed) > + priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode); Two things here: 1) Why do we need to call fix_mac_speed() at the start and end of this stmmac_mac_link_down()? 2) What if the MAC doesn't support 10M operation? For example, dwxgmac2 and dwxlgmac2 do not support anything below 1G. It feels that this is storing up a problem for the future, where a platform that uses e.g. xlgmac2 also implements fix_mac_speed() and then gets a surprise when it's called with SPEED_10. Personally, I don't like "fix_mac_speed", and I don't like it even more with this change. I would prefer to see link_up/link_down style operations so that platforms can do whatever they need to on those events, rather than being told what to do by a single call that may look identical irrespective of whether the link came up or went down. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!