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

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

-- 
2.17.1

This patch intends to add clocks management for stmmac driver:
1. Keep clocks disabled after probe stage.
2. Enable clocks when up the net device, and disable clocks when down
   the net device.
3. If the driver is built as module, it also keeps clocks disabled when
   the module is removed.

Signed-off-by: Joakim Zhang <redacted>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 60 ++++++++++++++++---
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 26b971cd4da5..35a79c00a477 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

@@ -113,6 +113,27 @@ static void stmmac_exit_fs(struct net_device *dev);
 
 #define STMMAC_COAL_TIMER(x) (ns_to_ktime((x) * NSEC_PER_USEC))
 
+static int stmmac_bus_clks_enable(struct stmmac_priv *priv, bool enabled)
+{
+	int ret = 0;
+
+	if (enabled) {
+		ret = clk_prepare_enable(priv->plat->stmmac_clk);
+		if (ret)
+			return ret;
+		ret = clk_prepare_enable(priv->plat->pclk);
+		if (ret) {
+			clk_disable_unprepare(priv->plat->stmmac_clk);
+			return ret;
+		}
+	} else {
+		clk_disable_unprepare(priv->plat->stmmac_clk);
+		clk_disable_unprepare(priv->plat->pclk);
+	}
+
+	return ret;
+}
+
 /**
  * stmmac_verify_args - verify the driver parameters.
  * Description: it checks the driver parameters and set a default in case of

@@ -2800,6 +2821,10 @@ static int stmmac_open(struct net_device *dev)
 	u32 chan;
 	int ret;
 
+	ret = stmmac_bus_clks_enable(priv, true);
+	if (ret)
+		return ret;
+
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI &&
 	    priv->hw->xpcs == NULL) {

@@ -2808,7 +2833,7 @@ static int stmmac_open(struct net_device *dev)
 			netdev_err(priv->dev,
 				   "%s: Cannot attach to PHY (error: %d)\n",
 				   __func__, ret);
-			return ret;
+			goto clk_enable_error;
 		}
 	}
 

@@ -2924,6 +2949,8 @@ static int stmmac_open(struct net_device *dev)
 	free_dma_desc_resources(priv);
 dma_desc_error:
 	phylink_disconnect_phy(priv->phylink);
+clk_enable_error:
+	stmmac_bus_clks_enable(priv, false);
 	return ret;
 }
 

@@ -2974,6 +3001,8 @@ static int stmmac_release(struct net_device *dev)
 
 	stmmac_release_ptp(priv);
 
+	stmmac_bus_clks_enable(priv, false);
+
 	return 0;
 }
 

@@ -4624,6 +4653,10 @@ static int stmmac_vlan_rx_kill_vid(struct net_device *ndev, __be16 proto, u16 vi
 	bool is_double = false;
 	int ret;
 
+	ret = stmmac_bus_clks_enable(priv, true);
+	if (ret)
+		return ret;
+
 	if (be16_to_cpu(proto) == ETH_P_8021AD)
 		is_double = true;
 

@@ -4632,10 +4665,15 @@ static int stmmac_vlan_rx_kill_vid(struct net_device *ndev, __be16 proto, u16 vi
 	if (priv->hw->num_vlan) {
 		ret = stmmac_del_hw_vlan_rx_fltr(priv, ndev, priv->hw, proto, vid);
 		if (ret)
-			return ret;
+			goto clk_enable_error;
 	}
 
-	return stmmac_vlan_update(priv, is_double);
+	ret = stmmac_vlan_update(priv, is_double);
+
+clk_enable_error:
+	stmmac_bus_clks_enable(priv, false);
+
+	return ret;
 }
 
 static const struct net_device_ops stmmac_netdev_ops = {

@@ -5111,6 +5149,8 @@ int stmmac_dvr_probe(struct device *device,
 	stmmac_init_fs(ndev);
 #endif
 
+	stmmac_bus_clks_enable(priv, false);
+
 	return ret;
 
 error_serdes_powerup:

@@ -5125,6 +5165,7 @@ int stmmac_dvr_probe(struct device *device,
 	stmmac_napi_del(ndev);
 error_hw_init:
 	destroy_workqueue(priv->wq);
+	stmmac_bus_clks_enable(priv, false);
 
 	return ret;
 }

@@ -5140,6 +5181,7 @@ int stmmac_dvr_remove(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct stmmac_priv *priv = netdev_priv(ndev);
+	bool netif_status = netif_running(ndev);
 
 	netdev_info(priv->dev, "%s: removing driver", __func__);
 

@@ -5157,8 +5199,8 @@ int stmmac_dvr_remove(struct device *dev)
 	phylink_destroy(priv->phylink);
 	if (priv->plat->stmmac_rst)
 		reset_control_assert(priv->plat->stmmac_rst);
-	clk_disable_unprepare(priv->plat->pclk);
-	clk_disable_unprepare(priv->plat->stmmac_clk);
+	if (netif_status)
+		stmmac_bus_clks_enable(priv, false);
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI)
 		stmmac_mdio_unregister(ndev);

@@ -5224,8 +5266,7 @@ int stmmac_suspend(struct device *dev)
 		pinctrl_pm_select_sleep_state(priv->device);
 		/* Disable clock in case of PWM is off */
 		clk_disable_unprepare(priv->plat->clk_ptp_ref);
-		clk_disable_unprepare(priv->plat->pclk);
-		clk_disable_unprepare(priv->plat->stmmac_clk);
+		stmmac_bus_clks_enable(priv, false);
 	}
 	mutex_unlock(&priv->lock);
 

@@ -5289,8 +5330,9 @@ int stmmac_resume(struct device *dev)
 	} else {
 		pinctrl_pm_select_default_state(priv->device);
 		/* enable the clk previously disabled */
-		clk_prepare_enable(priv->plat->stmmac_clk);
-		clk_prepare_enable(priv->plat->pclk);
+		ret = stmmac_bus_clks_enable(priv, true);
+		if (ret)
+			return ret;
 		if (priv->plat->clk_ptp_ref)
 			clk_prepare_enable(priv->plat->clk_ptp_ref);
 		/* reset the phy so that it's ready */

-- 
2.17.1

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:46
To: Joakim Zhang <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
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:

quoted

+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

Split clocks settings from init callback into clks_enable callback,
which could support platform level clocks management.

Signed-off-by: Joakim Zhang <redacted>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 60 +++++++++++--------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 223f69da7e95..9cd4dde716f0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c

@@ -90,6 +90,32 @@ imx8dxl_set_intf_mode(struct plat_stmmacenet_data *plat_dat)
 	return ret;
 }
 
+static int imx_dwmac_clks_enable(void *priv, bool enabled)
+{
+	struct imx_priv_data *dwmac = priv;
+	int ret = 0;
+
+	if (enabled) {
+		ret = clk_prepare_enable(dwmac->clk_mem);
+		if (ret) {
+			dev_err(dwmac->dev, "mem clock enable failed\n");
+			return ret;
+		}
+
+		ret = clk_prepare_enable(dwmac->clk_tx);
+		if (ret) {
+			dev_err(dwmac->dev, "tx clock enable failed\n");
+			clk_disable_unprepare(dwmac->clk_mem);
+			return ret;
+		}
+	} else {
+		clk_disable_unprepare(dwmac->clk_tx);
+		clk_disable_unprepare(dwmac->clk_mem);
+	}
+
+	return ret;
+}
+
 static int imx_dwmac_init(struct platform_device *pdev, void *priv)
 {
 	struct plat_stmmacenet_data *plat_dat;

@@ -98,39 +124,18 @@ static int imx_dwmac_init(struct platform_device *pdev, void *priv)
 
 	plat_dat = dwmac->plat_dat;
 
-	ret = clk_prepare_enable(dwmac->clk_mem);
-	if (ret) {
-		dev_err(&pdev->dev, "mem clock enable failed\n");
-		return ret;
-	}
-
-	ret = clk_prepare_enable(dwmac->clk_tx);
-	if (ret) {
-		dev_err(&pdev->dev, "tx clock enable failed\n");
-		goto clk_tx_en_failed;
-	}
-
 	if (dwmac->ops->set_intf_mode) {
 		ret = dwmac->ops->set_intf_mode(plat_dat);
 		if (ret)
-			goto intf_mode_failed;
+			return ret;
 	}
 
 	return 0;
-
-intf_mode_failed:
-	clk_disable_unprepare(dwmac->clk_tx);
-clk_tx_en_failed:
-	clk_disable_unprepare(dwmac->clk_mem);
-	return ret;
 }
 
 static void imx_dwmac_exit(struct platform_device *pdev, void *priv)
 {
-	struct imx_priv_data *dwmac = priv;
-
-	clk_disable_unprepare(dwmac->clk_tx);
-	clk_disable_unprepare(dwmac->clk_mem);
+	/* nothing to do now */
 }
 
 static void imx_dwmac_fix_speed(void *priv, unsigned int speed)

@@ -249,10 +254,15 @@ static int imx_dwmac_probe(struct platform_device *pdev)
 	plat_dat->addr64 = dwmac->ops->addr_width;
 	plat_dat->init = imx_dwmac_init;
 	plat_dat->exit = imx_dwmac_exit;
+	plat_dat->clks_enable = imx_dwmac_clks_enable;
 	plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
 	plat_dat->bsp_priv = dwmac;
 	dwmac->plat_dat = plat_dat;
 
+	ret = imx_dwmac_clks_enable(dwmac, true);
+	if (ret)
+		goto err_clks_enable;
+
 	ret = imx_dwmac_init(pdev, dwmac);
 	if (ret)
 		goto err_dwmac_init;

@@ -263,9 +273,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_dwmac_init:
 err_drv_probe:
 	imx_dwmac_exit(pdev, plat_dat->bsp_priv);
+err_dwmac_init:
+	imx_dwmac_clks_enable(dwmac, false);
+err_clks_enable:
 err_parse_dt:
 err_match_data:
 	stmmac_remove_config_dt(pdev, plat_dat);

-- 
2.17.1

This patch intends to add platform level clocks management. Some
platforms may have their own special clocks, they also need to be
managed dynamically. If you want to manage such clocks, please implement
clks_enable callback.

Signed-off-by: Joakim Zhang <redacted>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++++++
 include/linux/stmmac.h                            |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 35a79c00a477..3f12faa224aa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

@@ -126,9 +126,19 @@ static int stmmac_bus_clks_enable(struct stmmac_priv *priv, bool enabled)
 			clk_disable_unprepare(priv->plat->stmmac_clk);
 			return ret;
 		}
+		if (priv->plat->clks_enable) {
+			ret = priv->plat->clks_enable(priv->plat->bsp_priv, enabled);
+			if (ret) {
+				clk_disable_unprepare(priv->plat->stmmac_clk);
+				clk_disable_unprepare(priv->plat->pclk);
+				return ret;
+			}
+		}
 	} else {
 		clk_disable_unprepare(priv->plat->stmmac_clk);
 		clk_disable_unprepare(priv->plat->pclk);
+		if (priv->plat->clks_enable)
+			priv->plat->clks_enable(priv->plat->bsp_priv, enabled);
 	}
 
 	return ret;

diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index a302982de2d7..f2486a0ab5f1 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h

@@ -183,6 +183,7 @@ struct plat_stmmacenet_data {
 	int (*init)(struct platform_device *pdev, void *priv);
 	void (*exit)(struct platform_device *pdev, void *priv);
 	struct mac_device_info *(*setup)(void *priv);
+	int (*clks_enable)(void *priv, bool enabled);
 	void *bsp_priv;
 	struct clk *stmmac_clk;
 	struct clk *pclk;

-- 
2.17.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.

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: 2021年2月24日 0:45
To: Joakim Zhang <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks

On Tue, 23 Feb 2021 18:48:15 +0800 Joakim Zhang wrote:

quoted

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.

On Wed, 24 Feb 2021 01:45:40 +0000 Joakim Zhang wrote:

quoted

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.

quoted

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 <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks

On Wed, 24 Feb 2021 01:45:40 +0000 Joakim Zhang wrote:

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

This should not be enough, such as, even we close the NIC, the clocks still

enabled. So this patch improve below:

quoted

Keep clocks disabled after driver probe, enable clocks when NIC up, and then

disable clocks when NIC down.

quoted

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:

quoted

quoted

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 <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks

On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:

quoted

quoted

quoted

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:

quoted

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: 2021年2月24日 10:35
To: Joakim Zhang <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks

On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:

quoted

quoted

quoted

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: Florian Fainelli <f.fainelli@gmail.com>
Sent: 2021年2月24日 11:54
To: Joakim Zhang <redacted>; Jakub Kicinski
[off-list ref]
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks



On 2/23/2021 6:47 PM, Joakim Zhang wrote:

quoted

quoted

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: 2021年2月24日 10:35
To: Joakim Zhang <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks

On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:

quoted

quoted

quoted

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: Jakub Kicinski <kuba@kernel.org>
Sent: 2021年2月24日 10:35
To: Joakim Zhang <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org;
dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks

On Wed, 24 Feb 2021 02:13:05 +0000 Joakim Zhang wrote:

quoted

quoted

quoted

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: Andrew Lunn <andrew@lunn.ch>
Sent: 2021年2月24日 21:02
To: Jakub Kicinski <kuba@kernel.org>
Cc: Joakim Zhang <redacted>; peppe.cavallaro@st.com;
alexandre.torgue@st.com; joabreu@synopsys.com; davem@davemloft.net;
netdev@vger.kernel.org; dl-linux-imx [off-list ref]
Subject: Re: [PATCH V1 net-next 0/3] net: stmmac: implement clocks

quoted

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