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

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

[Joakim Zhang]

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

[PATCH V1 net-next 1/3] net: stmmac: add clocks management for gmac driver

[Joakim Zhang]

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 <qiangqing.zhang@nxp.com>
---
 .../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

Re: [PATCH V1 net-next 1/3] net: stmmac: add clocks management for gmac driver

[Jakub Kicinski]

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.

RE: [PATCH V1 net-next 1/3] net: stmmac: add clocks management for gmac driver

[Joakim Zhang]

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

[PATCH V1 net-next 2/3] net: stmmac: add platform level clocks management

[Joakim Zhang]

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 <qiangqing.zhang@nxp.com>
---
 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

[PATCH V1 net-next 3/3] net: stmmac: add platform level clocks management for i.MX

[Joakim Zhang]

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

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 .../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

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

[Jakub Kicinski]

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.

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

[Joakim Zhang]

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

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

[Jakub Kicinski]

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.

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

[Joakim Zhang]

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

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

[Jakub Kicinski]

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.

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

[Joakim Zhang]

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

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

[Florian Fainelli]

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

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

[Joakim Zhang]

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

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

[Joakim Zhang]

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

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

[Andrew Lunn]

> 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

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

[Joakim Zhang]

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

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

[Andrew Lunn]

> 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

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

[Joakim Zhang]

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

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

[Andrew Lunn]

> 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