| Message ID | 20210223104818.1933-1-qiangqing.zhang@nxp.com |
|---|---|
| Headers | show |
| Series | net: stmmac: implement clocks | expand |
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.
On Tue, 23 Feb 2021 18:48:16 +0800 Joakim Zhang wrote:
> +static int stmmac_bus_clks_enable(struct stmmac_priv *priv, bool enabled)
nit: my personal preference is to not call functions .._enable() and
then make them have a parameter saying if it's enable or disable.
Call the function .._config() or .._set() or such.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月24日 0:45 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > 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&data=04%7C01%7Cqiangqing.zhang%4 > 0nxp.com%7Ccfebfd0aac2b43ba9c9308d8d81a6194%7C686ea1d3bc2b4c6fa92 > cd99c5c301635%7C0%7C0%7C637496955136816595%7CUnknown%7CTWFpb > GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6 > Mn0%3D%7C1000&sdata=LLqX9utaTfnV5BV4JW6zoY76YzQiOe9Xlah58B > 9jv1Y%3D&reserved=0 > > RFC patches sent for review only are obviously welcome at any time.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月24日 0:46 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V1 net-next 1/3] net: stmmac: add clocks management for > gmac driver > > On Tue, 23 Feb 2021 18:48:16 +0800 Joakim Zhang wrote: > > +static int stmmac_bus_clks_enable(struct stmmac_priv *priv, bool > > +enabled) > > nit: my personal preference is to not call functions .._enable() and then make > them have a parameter saying if it's enable or disable. > Call the function .._config() or .._set() or such. OK, thanks, will improve it. Best Regards, Joakim Zhang
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.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月24日 9:55 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > 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
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.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月24日 10:35 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > 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
On 2/23/2021 6:47 PM, Joakim Zhang wrote: > >> -----Original Message----- >> From: Jakub Kicinski <kuba@kernel.org> >> Sent: 2021年2月24日 10:35 >> To: Joakim Zhang <qiangqing.zhang@nxp.com> >> Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; >> joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; >> dl-linux-imx <linux-imx@nxp.com> >> 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. -- Florian
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月24日 10:35 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > 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
> 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
> -----Original Message----- > From: Florian Fainelli <f.fainelli@gmail.com> > Sent: 2021年2月24日 11:54 > To: Joakim Zhang <qiangqing.zhang@nxp.com>; Jakub Kicinski > <kuba@kernel.org> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > 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 <kuba@kernel.org> > >> Sent: 2021年2月24日 10:35 > >> To: Joakim Zhang <qiangqing.zhang@nxp.com> > >> Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > >> joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > >> dl-linux-imx <linux-imx@nxp.com> > >> 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
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2021年2月24日 21:02 > To: Jakub Kicinski <kuba@kernel.org> > Cc: Joakim Zhang <qiangqing.zhang@nxp.com>; peppe.cavallaro@st.com; > alexandre.torgue@st.com; joabreu@synopsys.com; davem@davemloft.net; > netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > 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
> 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 <Jose.Abreu@synopsys.com> 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
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2021年2月25日 10:34 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: netdev <netdev@vger.kernel.org> > 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 <Jose.Abreu@synopsys.com> > 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
> 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