Re: [PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2024-06-28 22:17:17
Also in:
linux-arm-kernel, linux-arm-msm, linux-devicetree, lkml
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.
quoted hunk ↗ jump to hunk
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!