[V1,net-next,0/3] net: stmmac: implement clocks

Message

Joakim Zhang Feb. 23, 2021, 10:48 a.m. UTC

In stmmac driver, clocks are all enabled after device probed, this leads
to more power consumption. This patch set tries to implement clocks
management, and takes i.MX platform as a example.

Joakim Zhang (3):
  net: stmmac: add clocks management for gmac driver
  net: stmmac: add platform level clocks management
  net: stmmac: add platform level clocks management for i.MX

 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 60 +++++++++-------
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 70 ++++++++++++++++---
 include/linux/stmmac.h                        |  1 +
 3 files changed, 98 insertions(+), 33 deletions(-)

Comments

Jakub Kicinski Feb. 23, 2021, 4:45 p.m. UTC | #1

On Tue, 23 Feb 2021 18:48:15 +0800 Joakim Zhang wrote:
> In stmmac driver, clocks are all enabled after device probed, this leads
> to more power consumption. This patch set tries to implement clocks
> management, and takes i.MX platform as a example.

net-next is closed now and this is an optimization so please post as
RFC until net-next is open again (see the note at the end of the email).

I'm not an expert on this stuff, but is there a reason you're not
integrating this functionality with the power management subsystem?
I don't think it'd change the functionality, but it'd feel more
idiomatic to fit in the standard Linux framework.



# Form letter - net-next is closed

We have already sent the networking pull request for 5.12 and therefore
net-next is closed for new drivers, features, code refactoring and
optimizations. We are currently accepting bug fixes only.

Please repost when net-next reopens after 5.12-rc1 is cut.

Look out for the announcement on the mailing list or check:
http://vger.kernel.org/~davem/net-next.html

RFC patches sent for review only are obviously welcome at any time.

Joakim Zhang Feb. 24, 2021, 1:45 a.m. UTC | #2

> -----Original Message-----
> From: Jakub Kicinski <[e-mail]>
> Sent: 2021年2月24日 0:45
> To: Joakim Zhang <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> 
> On Tue, 23 Feb 2021 18:48:15 +0800 Joakim Zhang wrote:
> > In stmmac driver, clocks are all enabled after device probed, this
> > leads to more power consumption. This patch set tries to implement
> > clocks management, and takes i.MX platform as a example.

Hi Jakub,

Thanks for your kindly review!

> net-next is closed now and this is an optimization so please post as RFC until
> net-next is open again (see the note at the end of the email).

Ok, I will post as RFC during net-next on closed state.

> I'm not an expert on this stuff, but is there a reason you're not integrating this
> functionality with the power management subsystem?

Do you mean that implement runtime power management for driver? If yes, I think that is another feature, we can support later.

> I don't think it'd change the functionality, but it'd feel more idiomatic to fit in
> the standard Linux framework.

Yes, there is no functionality change, this patch set just adds clocks management.
In the driver now, we manage clocks at two point side:
1. enable clocks when probe driver, disable clocks when remove driver.
2. disable clocks when system suspend, enable clocks when system resume back.

This should not be enough, such as, even we close the NIC, the clocks still enabled. So this patch improve below:
Keep clocks disabled after driver probe, enable clocks when NIC up, and then disable clocks when NIC down.
The aim is to enable clocks when it needs, others keep clocks disabled.

Best Regards,
Joakim Zhang
> 
> 
> # Form letter - net-next is closed
> 
> We have already sent the networking pull request for 5.12 and therefore
> net-next is closed for new drivers, features, code refactoring and optimizations.
> We are currently accepting bug fixes only.
> 
> Please repost when net-next reopens after 5.12-rc1 is cut.
> 
> Look out for the announcement on the mailing list or check:
> https://eur01.safelinks.protection.outlook.com/?url=http:%2F%2Fvger.kernel.
> org%2F~davem%2Fnet-next.html&amp;data=04%7C01%7Cqiangqing.zhang%4
> 0nxp.com%7Ccfebfd0aac2b43ba9c9308d8d81a6194%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C0%7C637496955136816595%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C1000&amp;sdata=LLqX9utaTfnV5BV4JW6zoY76YzQiOe9Xlah58B
> 9jv1Y%3D&amp;reserved=0
> 
> RFC patches sent for review only are obviously welcome at any time.

Jakub Kicinski Feb. 24, 2021, 1:54 a.m. UTC | #3

On Wed, 24 Feb 2021 01:45:40 +0000 Joakim Zhang wrote:
> > I'm not an expert on this stuff, but is there a reason you're not integrating this
> > functionality with the power management subsystem?  
> 
> Do you mean that implement runtime power management for driver? If
> yes, I think that is another feature, we can support later.

Runtime is a strong word, IIUC you can just implement the PM callbacks,
and always resume in .open and always suspend in .close. Pretty much
what you have already.

> > I don't think it'd change the functionality, but it'd feel more idiomatic to fit in
> > the standard Linux framework.  
> 
> Yes, there is no functionality change, this patch set just adds clocks management.
> In the driver now, we manage clocks at two point side:
> 1. enable clocks when probe driver, disable clocks when remove driver.
> 2. disable clocks when system suspend, enable clocks when system resume back.
> 
> This should not be enough, such as, even we close the NIC, the clocks still enabled. So this patch improve below:
> Keep clocks disabled after driver probe, enable clocks when NIC up, and then disable clocks when NIC down.
> The aim is to enable clocks when it needs, others keep clocks disabled.

Understood. Please double check ethtool callbacks work fine. People
often forget about those when disabling clocks in .close.

Joakim Zhang Feb. 24, 2021, 2:13 a.m. UTC | #4

> -----Original Message-----
> From: Jakub Kicinski <[e-mail]>
> Sent: 2021年2月24日 9:55
> To: Joakim Zhang <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> 
> On Wed, 24 Feb 2021 01:45:40 +0000 Joakim Zhang wrote:
> > > I'm not an expert on this stuff, but is there a reason you're not
> > > integrating this functionality with the power management subsystem?
> >
> > Do you mean that implement runtime power management for driver? If
> > yes, I think that is another feature, we can support later.
> 
> Runtime is a strong word, IIUC you can just implement the PM callbacks, and
> always resume in .open and always suspend in .close. Pretty much what you
> have already.
> 
> > > I don't think it'd change the functionality, but it'd feel more
> > > idiomatic to fit in the standard Linux framework.
> >
> > Yes, there is no functionality change, this patch set just adds clocks
> management.
> > In the driver now, we manage clocks at two point side:
> > 1. enable clocks when probe driver, disable clocks when remove driver.
> > 2. disable clocks when system suspend, enable clocks when system resume
> back.
> >
> > This should not be enough, such as, even we close the NIC, the clocks still
> enabled. So this patch improve below:
> > Keep clocks disabled after driver probe, enable clocks when NIC up, and then
> disable clocks when NIC down.
> > The aim is to enable clocks when it needs, others keep clocks disabled.
> 
> Understood. Please double check ethtool callbacks work fine. People often
> forget about those when disabling clocks in .close.

Hi Jakub,

If NIC is open then clocks are always enabled, so all ethtool callbacks should be okay.

Could you point me which ethtool callbacks could be invoked when NIC is closed? I'm not very familiar with ethtool use case. Thanks.

Best Regards,
Joakim Zhang

Jakub Kicinski Feb. 24, 2021, 2:34 a.m. UTC | #5

On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:
> > > The aim is to enable clocks when it needs, others keep clocks disabled.  
> > 
> > Understood. Please double check ethtool callbacks work fine. People often
> > forget about those when disabling clocks in .close.  
> 
> Hi Jakub,
> 
> If NIC is open then clocks are always enabled, so all ethtool
> callbacks should be okay.
> 
> Could you point me which ethtool callbacks could be invoked when NIC
> is closed? I'm not very familiar with ethtool use case. Thanks.

Well, all of them - ethtool does not check if the device is open.
User can access and configure the device when it's closed.
Often the callbacks access only driver data, but it's implementation
specific so you'll need to validate the callbacks stmmac implements.

Joakim Zhang Feb. 24, 2021, 2:47 a.m. UTC | #6

> -----Original Message-----
> From: Jakub Kicinski <[e-mail]>
> Sent: 2021年2月24日 10:35
> To: Joakim Zhang <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> 
> On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:
> > > > The aim is to enable clocks when it needs, others keep clocks disabled.
> > >
> > > Understood. Please double check ethtool callbacks work fine. People
> > > often forget about those when disabling clocks in .close.
> >
> > Hi Jakub,
> >
> > If NIC is open then clocks are always enabled, so all ethtool
> > callbacks should be okay.
> >
> > Could you point me which ethtool callbacks could be invoked when NIC
> > is closed? I'm not very familiar with ethtool use case. Thanks.
> 
> Well, all of them - ethtool does not check if the device is open.
> User can access and configure the device when it's closed.
> Often the callbacks access only driver data, but it's implementation specific so
> you'll need to validate the callbacks stmmac implements.

Thanks Jakub, I will check these callbacks.

Best Regards,
Joakim Zhang

Florian Fainelli Feb. 24, 2021, 3:54 a.m. UTC | #7

On 2/23/2021 6:47 PM, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Jakub Kicinski <[e-mail]>
>> Sent: 2021年2月24日 10:35
>> To: Joakim Zhang <[e-mail]>
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> dl-linux-imx <[e-mail]>
>> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
>>
>> On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:
>>>>> The aim is to enable clocks when it needs, others keep clocks disabled.
>>>>
>>>> Understood. Please double check ethtool callbacks work fine. People
>>>> often forget about those when disabling clocks in .close.
>>>
>>> Hi Jakub,
>>>
>>> If NIC is open then clocks are always enabled, so all ethtool
>>> callbacks should be okay.
>>>
>>> Could you point me which ethtool callbacks could be invoked when NIC
>>> is closed? I'm not very familiar with ethtool use case. Thanks.
>>
>> Well, all of them - ethtool does not check if the device is open.
>> User can access and configure the device when it's closed.
>> Often the callbacks access only driver data, but it's implementation specific so
>> you'll need to validate the callbacks stmmac implements.
> 
> Thanks Jakub, I will check these callbacks.

You can implement ethtool_ops::begin and ethtool_ops::complete where you
would enable the clock, and respectively disable it just for the time of
the operation. The ethtool framework guarantees that begin is called at
the beginning and complete at the end. You can also make sure that if
the interface is disabled you only return a cached copy of the
settings/MIB counters (they are not updating since the HW is disabled)
and conversely only store parameters in a cached structure and apply
those when the network device gets opened again. Either way would work.

Joakim Zhang Feb. 24, 2021, 9:03 a.m. UTC | #8

> -----Original Message-----
> From: Jakub Kicinski <[e-mail]>
> Sent: 2021年2月24日 10:35
> To: Joakim Zhang <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> 
> On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:
> > > > The aim is to enable clocks when it needs, others keep clocks disabled.
> > >
> > > Understood. Please double check ethtool callbacks work fine. People
> > > often forget about those when disabling clocks in .close.
> >
> > Hi Jakub,
> >
> > If NIC is open then clocks are always enabled, so all ethtool
> > callbacks should be okay.
> >
> > Could you point me which ethtool callbacks could be invoked when NIC
> > is closed? I'm not very familiar with ethtool use case. Thanks.
> 
> Well, all of them - ethtool does not check if the device is open.
> User can access and configure the device when it's closed.
> Often the callbacks access only driver data, but it's implementation specific so
> you'll need to validate the callbacks stmmac implements.

Hi Jakub,

I check the code, ethtool from stmmac driver only can be used when net is running now, so the clocks are enabled.
net/ethtool/ioctl.c -> dev_ethtool()
	[...]
	if (dev->ethtool_ops->begin) {
		rc = dev->ethtool_ops->begin(dev);
		if (rc < 0)
			return rc;
	}
	[...]

Stmmac driver implement begin callback like below:
	static int stmmac_check_if_running(struct net_device *dev)
	{
		if (!netif_running(dev))
			return -EBUSY;
		return 0;
	}

Best Regards,
Joakim Zhang

Andrew Lunn Feb. 24, 2021, 1:01 p.m. UTC | #9

> Understood. Please double check ethtool callbacks work fine. People
> often forget about those when disabling clocks in .close.

The MDIO bus can also be used at any time, not just when the interface
is open. For example the MAC could be connected to an Ethernet switch,
which is managed by the MDIO bus. Or some PHYs have a temperature
sensor which is registered with HWMON when the PHY is probed.

You said you copied the FEC driver. Take a look at that, it was
initially broken in this way, and i needed to extend it when i got a
board with an Ethernet switch attached to the FEC.

      Andrew

Joakim Zhang Feb. 25, 2021, 1:42 a.m. UTC | #10

> -----Original Message-----
> From: Florian Fainelli <[e-mail]>
> Sent: 2021年2月24日 11:54
> To: Joakim Zhang <[e-mail]>; Jakub Kicinski
> <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> 
> 
> 
> On 2/23/2021 6:47 PM, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Jakub Kicinski <[e-mail]>
> >> Sent: 2021年2月24日 10:35
> >> To: Joakim Zhang <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> dl-linux-imx <[e-mail]>
> >> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> >>
> >> On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:
> >>>>> The aim is to enable clocks when it needs, others keep clocks disabled.
> >>>>
> >>>> Understood. Please double check ethtool callbacks work fine. People
> >>>> often forget about those when disabling clocks in .close.
> >>>
> >>> Hi Jakub,
> >>>
> >>> If NIC is open then clocks are always enabled, so all ethtool
> >>> callbacks should be okay.
> >>>
> >>> Could you point me which ethtool callbacks could be invoked when NIC
> >>> is closed? I'm not very familiar with ethtool use case. Thanks.
> >>
> >> Well, all of them - ethtool does not check if the device is open.
> >> User can access and configure the device when it's closed.
> >> Often the callbacks access only driver data, but it's implementation
> >> specific so you'll need to validate the callbacks stmmac implements.
> >
> > Thanks Jakub, I will check these callbacks.
> 
> You can implement ethtool_ops::begin and ethtool_ops::complete where you
> would enable the clock, and respectively disable it just for the time of the
> operation. The ethtool framework guarantees that begin is called at the
> beginning and complete at the end. You can also make sure that if the interface
> is disabled you only return a cached copy of the settings/MIB counters (they
> are not updating since the HW is disabled) and conversely only store
> parameters in a cached structure and apply those when the network device
> gets opened again. Either way would work.

Hi Florian,

Thanks for you hint. Yes, I noticed stmmac driver has implemented ethtool_ops::begin, which let ethtool only can be used when interface is enabled. Thanks a lot.

Best Regards,
Joakim Zhang
> --
> Florian

Joakim Zhang Feb. 25, 2021, 2:15 a.m. UTC | #11

> -----Original Message-----
> From: Andrew Lunn <[e-mail]>
> Sent: 2021年2月24日 21:02
> To: Jakub Kicinski <[e-mail]>
> Cc: Joakim Zhang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> 
> > Understood. Please double check ethtool callbacks work fine. People
> > often forget about those when disabling clocks in .close.
> 
> The MDIO bus can also be used at any time, not just when the interface is open.
> For example the MAC could be connected to an Ethernet switch, which is
> managed by the MDIO bus. Or some PHYs have a temperature sensor which is
> registered with HWMON when the PHY is probed.

Hi Andrew,

I don't have experience with Ethernet switch, according to your points, you mean we can connect STMMAC to an Ethernet switch, and then Ethernet switch managed STMMAC by the MDIO bus but without checking whether STMMAC interface is opened or not, so STMMAC needs clocks for MDIO even interface is closed, right?

> You said you copied the FEC driver. Take a look at that, it was initially broken in
> this way, and i needed to extend it when i got a board with an Ethernet switch
> attached to the FEC.

Could you point me how to implement clocks management to cover above Ethernet switch case? Or can we upstream this first and then fix it later for such case?

Best Regards,
Joakim Zhang
> 
>       Andrew

Andrew Lunn Feb. 25, 2021, 2:33 a.m. UTC | #12

> Hi Andrew,
> 

> I don't have experience with Ethernet switch, according to your
> points, you mean we can connect STMMAC to an Ethernet switch, and
> then Ethernet switch managed STMMAC by the MDIO bus but without
> checking whether STMMAC interface is opened or not, so STMMAC needs
> clocks for MDIO even interface is closed, right?

Correct. The MDIO bus has a different life cycle to the MAC. If any of
stmmac_xgmac2_mdio_read(), stmmac_xgmac2_mdio_write(),
stmmac_mdio_read(), and stmmac_mdio_write() need clocks ticking, you
need to ensure the clock is ticking, because these functions can be
called while the interface is not opened.

> > You said you copied the FEC driver. Take a look at that, it was initially broken in
> > this way, and i needed to extend it when i got a board with an Ethernet switch
> > attached to the FEC.
> 

> Could you point me how to implement clocks management to cover above
> Ethernet switch case? Or can we upstream this first and then fix it
> later for such case?

I actually got is wrong on the first attempt. So you need to look at:

42ea4457ae net: fec: normalize return value of pm_runtime_get_sync() in MDIO write
14d2b7c1a9 net: fec: fix initial runtime PM refcount
8fff755e9f net: fec: Ensure clocks are enabled while using mdio bus

And no, you cannot fix it later, because your patches potentially
break existing systems using an Ethernet switch. See:

ommit da29f2d84bd10234df570b7f07cbd0166e738230
Author: Jose Abreu <[e-mail]>
Date:   Tue Jan 7 13:35:42 2020 +0100

    net: stmmac: Fixed link does not need MDIO Bus
    
    When using fixed link we don't need the MDIO bus support.

...
    Tested-by: Florian Fainelli <f.fainelli@gmail> # Lamobo R1 (fixed-link + MDIO sub node for roboswitch).

So there are boards which make use of a switch and MDIO. Florian might
however be able to run tests for you, if you ask him.

   Andrew

Joakim Zhang Feb. 25, 2021, 11:48 a.m. UTC | #13

> -----Original Message-----
> From: Andrew Lunn <[e-mail]>
> Sent: 2021年2月25日 10:34
> To: Joakim Zhang <[e-mail]>
> Cc: netdev <[e-mail]>
> Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks
> 
> > Hi Andrew,
> >
> 
> > I don't have experience with Ethernet switch, according to your
> > points, you mean we can connect STMMAC to an Ethernet switch, and then
> > Ethernet switch managed STMMAC by the MDIO bus but without checking
> > whether STMMAC interface is opened or not, so STMMAC needs clocks for
> > MDIO even interface is closed, right?
> 
> Correct. The MDIO bus has a different life cycle to the MAC. If any of
> stmmac_xgmac2_mdio_read(), stmmac_xgmac2_mdio_write(),
> stmmac_mdio_read(), and stmmac_mdio_write() need clocks ticking, you need
> to ensure the clock is ticking, because these functions can be called while the
> interface is not opened.

Hi Andrew,

Thanks for you explanation, I still don't quite understand what the use case it is, could you give me more details, thanks a lot!
AFAIK now, there are two connections methods, we can abstract the layer:
	MAC <-> MAC, there is no PHY attached. It seems to know as Fixed link, right?
	MAC+PHY <-> PHY+MAC
From your expression, you should use an external Ethernet switch, if yes, why Ethernet switch needs to use MDIO bus to access another MAC's(STMMAC) PHY?

 
> > > You said you copied the FEC driver. Take a look at that, it was
> > > initially broken in this way, and i needed to extend it when i got a
> > > board with an Ethernet switch attached to the FEC.
> >
> 
> > Could you point me how to implement clocks management to cover above
> > Ethernet switch case? Or can we upstream this first and then fix it
> > later for such case?
> 
> I actually got is wrong on the first attempt. So you need to look at:
> 
> 42ea4457ae net: fec: normalize return value of pm_runtime_get_sync() in
> MDIO write
> 14d2b7c1a9 net: fec: fix initial runtime PM refcount 8fff755e9f net: fec: Ensure
> clocks are enabled while using mdio bus
> 
> And no, you cannot fix it later, because your patches potentially break existing
> systems using an Ethernet switch. See:
> 
> ommit da29f2d84bd10234df570b7f07cbd0166e738230
> Author: Jose Abreu <[e-mail]>
> Date:   Tue Jan 7 13:35:42 2020 +0100
> 
>     net: stmmac: Fixed link does not need MDIO Bus
> 
>     When using fixed link we don't need the MDIO bus support.
> 
> ...
>     Tested-by: Florian Fainelli <f.fainelli@gmail> # Lamobo R1 (fixed-link +
> MDIO sub node for roboswitch).
> 
> So there are boards which make use of a switch and MDIO. Florian might
> however be able to run tests for you, if you ask him.

Hi Florian,

I am curious about " fixed-link + MDIO sub node for roboswitch ", how does this implement?

Florian, Andrew, I will send a V2, it could still has defects. Welcome to review the patch, I don't expect it to break existing systems. Thanks.

Best Regards,
Joakim Zhang
> 
>    Andrew

Andrew Lunn Feb. 25, 2021, 1:14 p.m. UTC | #14

> Hi Andrew,
> 
> Thanks for you explanation, I still don't quite understand what the use case it is, could you give me more details, thanks a lot!
> AFAIK now, there are two connections methods, we can abstract the layer:
> 	MAC <-> MAC, there is no PHY attached. It seems to know as Fixed link, right?

Yes, this is the most common way of connecting a switch.

> 	MAC+PHY <-> PHY+MAC

Switches can be connected like this, but not often. Why pay for two
PHYs which you do not need?

> From your expression, you should use an external Ethernet switch, if
> yes, why Ethernet switch needs to use MDIO bus to access another
> MAC's(STMMAC) PHY?

The switch is on the same board as the MAC. Take a look at:
https://netdevconf.info/2.1/papers/distributed-switch-architecture.pdf

It explains the DSA architecture.

   Andrew