diff mbox series

[v2,3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 842 this patch: 842
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 849 this patch: 849
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 849 this patch: 849
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 20 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 38 this patch: 38
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-26--12-00 (tests: 664)

Commit Message

Sagar Cheluvegowda June 25, 2024, 11:49 p.m. UTC
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(+)

Comments

Andrew Halaney June 26, 2024, 2:58 p.m. UTC | #1
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.
Andrew Lunn June 26, 2024, 3:10 p.m. UTC | #2
> 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
Sagar Cheluvegowda June 28, 2024, 9:50 p.m. UTC | #3
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.
 
>
Russell King (Oracle) June 28, 2024, 10:16 p.m. UTC | #4
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.
Sagar Cheluvegowda July 1, 2024, 7:18 p.m. UTC | #5
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.
>  
>>
Sagar Cheluvegowda July 2, 2024, 11:38 p.m. UTC | #6
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 mbox series

Patch

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,