On 6/28/2024 3:16 PM, Russell King (Oracle) wrote:

On Tue, Jun 25, 2024 at 04:49:30PM -0700, Sagar Cheluvegowda wrote:

quoted

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

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()?

This was a typo, i will remove this.

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.

I will drop this patch[3/3] from this series now and i will do some analysis
on platform level link up and link down functions and post the changes as a
new series altogether.