| Message ID | 20240625-icc_bw_voting_from_ethqos-v2-3-eaa7cf9060f0@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add interconnect support for stmmac driver. | expand |
On Tue, Jun 25, 2024 at 04:49:30PM GMT, 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. > > Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > 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); I think you're doing this at the beginning and end of stmmac_mac_link_down(), is that intentional? I'm still curious if any of the netdev folks have any opinion on scaling things down like this on link down.
> I'm still curious if any of the netdev folks have any opinion on scaling > things down like this on link down. It does make sense, in that there are no frames to process, so the clock can be reduced. But i also think it is a bit of a workaround for poor hardware design. Often you can tell the MAC the link is down, and it can shut down a lot more, and even turn all the clocks off. I also wounder if there are going to be any side effects of this. Some Ethernet MACs export a clock to the PHY. Is that clock going to change? I don't think it will, because we are changing to a valid MAC speed, not 0. So the PHY has to work at this speed clock. But to make it easier to find issues like this, open() should probably set the clocks to a low speed until the link is up. That way, if there are going to be problems, the link should never come up, as opposed to the link never comes up after being lost the first time... Andrew
On 6/26/2024 7:58 AM, Andrew Halaney wrote: > On Tue, Jun 25, 2024 at 04:49:30PM GMT, 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. >> >> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> 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); > > > I think you're doing this at the beginning and end of > stmmac_mac_link_down(), is that intentional? > > I realised that bringing down the clock to 10Mbps should be the last operation of the link down process, the reason being if we bring down the clocks first it will deprive essential internal clocks to DMA/MTL modules which are required for Cleanup operations this might cause excessive delays in stopping DMA or flusing MTL queues. >
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.
On 6/28/2024 2:50 PM, Sagar Cheluvegowda wrote: > > > On 6/26/2024 7:58 AM, Andrew Halaney wrote: >> On Tue, Jun 25, 2024 at 04:49:30PM GMT, 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. >>> >>> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> >>> --- >>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> 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); >>> + The above fix_mac_speed needs to be removed, i lately realized this mistake. >>> 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); >> >> >> I think you're doing this at the beginning and end of >> stmmac_mac_link_down(), is that intentional? >> >> > > I realised that bringing down the clock to 10Mbps should be the last operation > of the link down process, the reason being if we bring down the clocks first it will > deprive essential internal clocks to DMA/MTL modules which are required for > Cleanup operations this might cause excessive delays in stopping DMA > or flusing MTL queues. > >>
On 6/28/2024 3:16 PM, Russell King (Oracle) wrote: > 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()? 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.
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); } static void stmmac_mac_link_up(struct phylink_config *config,
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. Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++ 1 file changed, 8 insertions(+)