[PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

Subsystems: freescale imx / mxc fec driver, networking drivers, the rest

STALE4101d

8 messages, 5 authors, 2015-08-03 · open the first message on its own page

[PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: Andrew Lunn <andrew@lunn.ch>
Date: 2015-07-25 20:44:55

When a switch is attached to the mdio bus, the mdio bus can be used
while the interface is not open. If the IPG clock is not enabled, MDIO
reads/writes will simply time out.

Add support for runtime PM to control this clock. Enable/disable this
clock using runtime PM, with open()/close() and mdio read()/write()
function triggering runtime PM operations. Since PM is optional, the
IPG clock is enabled at probe and is no longer modified by
fec_enet_clk_enable(), thus if PM is not enabled in the kernel, it is
guaranteed the clock is running when MDIO operations are performed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Cc: <redacted>
Cc: <redacted>
Cc: <redacted>
---

v5 was accepted and then reverted, because it broke imx6.  Lucas Stach
debugged what was going wrong and provided a patch. Thanks Lucas. This
has been squashed into v6.

It would be good if this version had more testing on various platforms
before it is accepted. If you reported a problem last time, please do
test and send a Tested-by:

I'm out the office for a week starting tomorrow.

Thanks
	Andrew

v6: Move the runtime PM setup before the mdio probe.

    Also move autosuspend init calls before runtime_pm_enable() so
    that the RPM callbacks aren't invoked several times during the
    probe function.
---
 drivers/net/ethernet/freescale/fec_main.c | 89 ++++++++++++++++++++++++++-----
 1 file changed, 76 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1f89c59b4353..349365d85b92 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <linux/pm_runtime.h>
 #include <linux/ptrace.h>
 #include <linux/errno.h>
 #include <linux/ioport.h>
@@ -77,6 +78,7 @@ static void fec_enet_itr_coal_init(struct net_device *ndev);
 #define FEC_ENET_RAEM_V	0x8
 #define FEC_ENET_RAFL_V	0x8
 #define FEC_ENET_OPD_V	0xFFF0
+#define FEC_MDIO_PM_TIMEOUT  100 /* ms */
 
 static struct platform_device_id fec_devtype[] = {
 	{
@@ -1767,7 +1769,13 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct fec_enet_private *fep = bus->priv;
+	struct device *dev = &fep->pdev->dev;
 	unsigned long time_left;
+	int ret = 0;
+
+	ret = pm_runtime_get_sync(dev);
+	if (IS_ERR_VALUE(ret))
+		return ret;
 
 	fep->mii_timeout = 0;
 	init_completion(&fep->mdio_done);
@@ -1783,18 +1791,30 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (time_left == 0) {
 		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO read timeout\n");
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
+		goto out;
 	}
 
-	/* return value */
-	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
+	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
+
+out:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
 }
 
 static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			   u16 value)
 {
 	struct fec_enet_private *fep = bus->priv;
+	struct device *dev = &fep->pdev->dev;
 	unsigned long time_left;
+	int ret = 0;
+
+	ret = pm_runtime_get_sync(dev);
+	if (IS_ERR_VALUE(ret))
+		return ret;
 
 	fep->mii_timeout = 0;
 	init_completion(&fep->mdio_done);
@@ -1811,10 +1831,13 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	if (time_left == 0) {
 		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO write timeout\n");
-		return -ETIMEDOUT;
+		ret  = -ETIMEDOUT;
 	}
 
-	return 0;
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
 }
 
 static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
@@ -1826,9 +1849,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 		ret = clk_prepare_enable(fep->clk_ahb);
 		if (ret)
 			return ret;
-		ret = clk_prepare_enable(fep->clk_ipg);
-		if (ret)
-			goto failed_clk_ipg;
 		if (fep->clk_enet_out) {
 			ret = clk_prepare_enable(fep->clk_enet_out);
 			if (ret)
@@ -1852,7 +1872,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 		}
 	} else {
 		clk_disable_unprepare(fep->clk_ahb);
-		clk_disable_unprepare(fep->clk_ipg);
 		if (fep->clk_enet_out)
 			clk_disable_unprepare(fep->clk_enet_out);
 		if (fep->clk_ptp) {
@@ -1874,8 +1893,6 @@ failed_clk_ptp:
 	if (fep->clk_enet_out)
 		clk_disable_unprepare(fep->clk_enet_out);
 failed_clk_enet_out:
-		clk_disable_unprepare(fep->clk_ipg);
-failed_clk_ipg:
 		clk_disable_unprepare(fep->clk_ahb);
 
 	return ret;
@@ -2847,10 +2864,14 @@ fec_enet_open(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	int ret;
 
+	ret = pm_runtime_get_sync(&fep->pdev->dev);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
 	pinctrl_pm_select_default_state(&fep->pdev->dev);
 	ret = fec_enet_clk_enable(ndev, true);
 	if (ret)
-		return ret;
+		goto clk_enable;
 
 	/* I should reset the ring buffers here, but I don't yet know
 	 * a simple way to do that.
@@ -2881,6 +2902,9 @@ err_enet_mii_probe:
 	fec_enet_free_buffers(ndev);
 err_enet_alloc:
 	fec_enet_clk_enable(ndev, false);
+clk_enable:
+	pm_runtime_mark_last_busy(&fep->pdev->dev);
+	pm_runtime_put_autosuspend(&fep->pdev->dev);
 	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
 	return ret;
 }
@@ -2903,6 +2927,9 @@ fec_enet_close(struct net_device *ndev)
 
 	fec_enet_clk_enable(ndev, false);
 	pinctrl_pm_select_sleep_state(&fep->pdev->dev);
+	pm_runtime_mark_last_busy(&fep->pdev->dev);
+	pm_runtime_put_autosuspend(&fep->pdev->dev);
+
 	fec_enet_free_buffers(ndev);
 
 	return 0;
@@ -3388,6 +3415,10 @@ fec_probe(struct platform_device *pdev)
 	if (ret)
 		goto failed_clk;
 
+	ret = clk_prepare_enable(fep->clk_ipg);
+	if (ret)
+		goto failed_clk_ipg;
+
 	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
 	if (!IS_ERR(fep->reg_phy)) {
 		ret = regulator_enable(fep->reg_phy);
@@ -3400,6 +3431,11 @@ fec_probe(struct platform_device *pdev)
 		fep->reg_phy = NULL;
 	}
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, FEC_MDIO_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	fec_reset_phy(pdev);
 
 	if (fep->bufdesc_ex)
@@ -3447,6 +3483,10 @@ fec_probe(struct platform_device *pdev)
 
 	fep->rx_copybreak = COPYBREAK_DEFAULT;
 	INIT_WORK(&fep->tx_timeout_work, fec_enet_timeout_work);
+
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+
 	return 0;
 
 failed_register:
@@ -3457,6 +3497,8 @@ failed_init:
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
 failed_regulator:
+	clk_disable_unprepare(fep->clk_ipg);
+failed_clk_ipg:
 	fec_enet_clk_enable(ndev, false);
 failed_clk:
 failed_phy:
@@ -3568,7 +3610,28 @@ failed_clk:
 	return ret;
 }
 
-static SIMPLE_DEV_PM_OPS(fec_pm_ops, fec_suspend, fec_resume);
+static int __maybe_unused fec_runtime_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	clk_disable_unprepare(fep->clk_ipg);
+
+	return 0;
+}
+
+static int __maybe_unused fec_runtime_resume(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	return clk_prepare_enable(fep->clk_ipg);
+}
+
+static const struct dev_pm_ops fec_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(fec_suspend, fec_resume)
+	SET_RUNTIME_PM_OPS(fec_runtime_suspend, fec_runtime_resume, NULL)
+};
 
 static struct platform_driver fec_driver = {
 	.driver	= {
-- 
2.1.4

Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: Fabio Estevam <festevam@gmail.com>
Date: 2015-07-26 00:14:14

Hi Andrew,

On Sat, Jul 25, 2015 at 5:38 PM, Andrew Lunn [off-list ref] wrote:
When a switch is attached to the mdio bus, the mdio bus can be used
while the interface is not open. If the IPG clock is not enabled, MDIO
reads/writes will simply time out.

Add support for runtime PM to control this clock. Enable/disable this
clock using runtime PM, with open()/close() and mdio read()/write()
function triggering runtime PM operations. Since PM is optional, the
IPG clock is enabled at probe and is no longer modified by
fec_enet_clk_enable(), thus if PM is not enabled in the kernel, it is
guaranteed the clock is running when MDIO operations are performed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Cc: <redacted>
Cc: <redacted>
Cc: <redacted>
---

v5 was accepted and then reverted, because it broke imx6.  Lucas Stach
debugged what was going wrong and provided a patch. Thanks Lucas. This
has been squashed into v6.

It would be good if this version had more testing on various platforms
before it is accepted. If you reported a problem last time, please do
test and send a Tested-by:
With this version I can boot via NFS on a mx6 board:

Tested-by: Fabio Estevam <redacted>

Thanks

Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: Tyler Baker <hidden>
Date: 2015-07-26 01:26:37

Hi Andrew,

On 25 July 2015 at 13:38, Andrew Lunn [off-list ref] wrote:
When a switch is attached to the mdio bus, the mdio bus can be used
while the interface is not open. If the IPG clock is not enabled, MDIO
reads/writes will simply time out.

Add support for runtime PM to control this clock. Enable/disable this
clock using runtime PM, with open()/close() and mdio read()/write()
function triggering runtime PM operations. Since PM is optional, the
IPG clock is enabled at probe and is no longer modified by
fec_enet_clk_enable(), thus if PM is not enabled in the kernel, it is
guaranteed the clock is running when MDIO operations are performed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Cc: <redacted>
Cc: <redacted>
Cc: <redacted>
---

v5 was accepted and then reverted, because it broke imx6.  Lucas Stach
debugged what was going wrong and provided a patch. Thanks Lucas. This
has been squashed into v6.

It would be good if this version had more testing on various platforms
before it is accepted. If you reported a problem last time, please do
test and send a Tested-by:
I've tested this single patch (v6) on top of next-20150724 using the
kernelci.org bot to build and boot test[1][2] on a broad range of
platforms. With these results, I can confirm that the imx6 platforms
tested are booting fine, and that no new boot/build regressions have
been introduced when comparing these results to the next-20150724
baseline[3][4].

Tested-by: Tyler Baker <redacted>

Cheers,

Tyler

[1] http://kernelci.org/boot/all/job/tbaker/kernel/v4.2-rc3-4216-g8c8efbc4423a/
[2] http://kernelci.org/build/tbaker/kernel/v4.2-rc3-4216-g8c8efbc4423a/
[3] http://kernelci.org/boot/all/job/next/kernel/next-20150724/
[4] http://kernelci.org/build/next/kernel/next-20150724/

Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: Andrew Lunn <andrew@lunn.ch>
Date: 2015-07-26 16:43:15

On Sat, Jul 25, 2015 at 10:38:02PM +0200, Andrew Lunn wrote:
When a switch is attached to the mdio bus, the mdio bus can be used
while the interface is not open. If the IPG clock is not enabled, MDIO
reads/writes will simply time out.
Hi David

Now there are two tested-by's could you pick this patch up?

Or do you want a repost including the tested-by's?

Thanks
	Andrew

Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: David Miller <davem@davemloft.net>
Date: 2015-07-27 08:22:12

From: Andrew Lunn <andrew@lunn.ch>
Date: Sat, 25 Jul 2015 22:38:02 +0200
When a switch is attached to the mdio bus, the mdio bus can be used
while the interface is not open. If the IPG clock is not enabled, MDIO
reads/writes will simply time out.

Add support for runtime PM to control this clock. Enable/disable this
clock using runtime PM, with open()/close() and mdio read()/write()
function triggering runtime PM operations. Since PM is optional, the
IPG clock is enabled at probe and is no longer modified by
fec_enet_clk_enable(), thus if PM is not enabled in the kernel, it is
guaranteed the clock is running when MDIO operations are performed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Applied.

Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: Uwe Kleine-König <hidden>
Date: 2015-08-03 13:13:45

Hello,

On Mon, Aug 03, 2015 at 02:55:40PM +0200, Andrew Lunn wrote:
v5 was accepted and then reverted, because it broke imx6.  Lucas Stach
debugged what was going wrong and provided a patch. Thanks Lucas. This
has been squashed into v6.
This fixed-for-i.MX6 patch was included for 4.2-rc5 as commit
8fff755e9f8d0f70a595e79f248695ce6aef5cc3. Unfortunately it breaks
i.MX27 like so:

	Unhandled fault: external abort on non-linefetch (0x808) at 0xf442b014
	pgd = c0004000
	[f442b014] *pgd=10000452(bad)
	Internal error: : 808 [#1] PREEMPT ARM
	Modules linked in:
	CPU: 0 PID: 5 Comm: kworker/0:0H Not tainted 4.2.0-rc5 #19
	Hardware name: Freescale i.MX27 (Device Tree Support)
	Workqueue: rpciod xs_tcp_setup_socket
	task: c788eb80 ti: c7896000 task.ti: c7896000
	PC is at fec_enet_start_xmit+0x39c/0xe04
	LR is at fec_enet_start_xmit+0x2e8/0xe04
	pc : [<c038ea88>]    lr : [<c038e9d4>]    psr: 80000013
	sp : c7897ad8  ip : 00000018  fp : c71637a0
	r10: c7b4f800  r9 : c781f000  r8 : 00000000
	r7 : 00000001  r6 : c88e5018  r5 : c7b4f800  r4 : c88e5018
	r3 : f442b014  r2 : 00000000  r1 : 00000000  r0 : a7164800
	Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
	Control: 0005317f  Table: a0004000  DAC: 00000017
	Process kworker/0:0H (pid: 5, stack limit = 0xc7896190)
	Stack: (0xc7897ad8 to 0xc7898000)
	7ac0:                                                       00000001 00000000
	7ae0: c7b4fcc0 c7001fcf 0000cc00 c781e000 00000003 00001800 c7007b37 00000000
	7b00: c7a91880 008976fc 000001a0 c88e5010 00000000 0000002a a7164800 000072d8
	7b20: 00007210 00b43ef2 00000004 00007eac 00000002 00000200 00004000 c70eed00
	7b40: c71637a0 c71637a0 0000002a c0766a2c 00000000 c7b4f800 c0766a34 c04a59a0
	7b60: 00000000 00000001 00000000 00000001 00000000 00000000 c7a91880 c7897bbc
	7b80: c07c1374 00000000 c71637a0 00000000 00000000 c70eed00 c71637a0 c7a91880
	7ba0: c7b4f800 00000000 00000000 00000001 00000000 c04c0004 c71637a0 00000010
	7bc0: c71637a0 c70eed00 c7b4f800 00000000 c7a91880 c04a5f60 c70eed68 00000001
	7be0: c70ee322 00000000 00000004 fffffff4 c71637a0 c71637a0 c7b4f800 0417a8c0
	7c00: f218a8c0 00000003 c7994300 c7296600 c07b66e0 c04faee0 f218a8c0 00000003
	7c20: c7994300 c7296600 c07b66e0 c04fb534 f218a8c0 00000000 c7b4bc48 c70ee200
	7c40: c7b4f800 0417a8c0 f218a8c0 c04fbc90 f218a8c0 00000000 c7b4bc48 00000000
	7c60: c7b4f944 c7292e88 000000a4 c70ee218 c7b4f944 c71638c0 c7163860 c04931a0
	7c80: 00000000 c7163860 c70ee200 c7292e88 c70ee218 c7b4f944 00000010 c04ad914
	7ca0: 000002c0 c70ee200 00000001 c04b06ac c7292e88 c70ee200 00000000 c70ee200

72 more lines

Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: Andrew Lunn <andrew@lunn.ch>
Date: 2015-08-03 13:57:19

On Mon, Aug 03, 2015 at 03:13:40PM +0200, Uwe Kleine-König wrote:
Hello,

On Mon, Aug 03, 2015 at 02:55:40PM +0200, Andrew Lunn wrote:
quoted
v5 was accepted and then reverted, because it broke imx6.  Lucas Stach
debugged what was going wrong and provided a patch. Thanks Lucas. This
has been squashed into v6.
This fixed-for-i.MX6 patch was included for 4.2-rc5 as commit
8fff755e9f8d0f70a595e79f248695ce6aef5cc3. Unfortunately it breaks
i.MX27 like so:

	Unhandled fault: external abort on non-linefetch (0x808) at 0xf442b014
	pgd = c0004000
	[f442b014] *pgd=10000452(bad)
	Internal error: : 808 [#1] PREEMPT ARM
	Modules linked in:
	CPU: 0 PID: 5 Comm: kworker/0:0H Not tainted 4.2.0-rc5 #19
	Hardware name: Freescale i.MX27 (Device Tree Support)
	Workqueue: rpciod xs_tcp_setup_socket
	task: c788eb80 ti: c7896000 task.ti: c7896000
	PC is at fec_enet_start_xmit+0x39c/0xe04
	LR is at fec_enet_start_xmit+0x2e8/0xe04
	pc : [<c038ea88>]    lr : [<c038e9d4>]    psr: 80000013
	sp : c7897ad8  ip : 00000018  fp : c71637a0
	r10: c7b4f800  r9 : c781f000  r8 : 00000000
	r7 : 00000001  r6 : c88e5018  r5 : c7b4f800  r4 : c88e5018
	r3 : f442b014  r2 : 00000000  r1 : 00000000  r0 : a7164800
	Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
	Control: 0005317f  Table: a0004000  DAC: 00000017
	Process kworker/0:0H (pid: 5, stack limit = 0xc7896190)
	Stack: (0xc7897ad8 to 0xc7898000)
	7ac0:                                                       00000001 00000000
	7ae0: c7b4fcc0 c7001fcf 0000cc00 c781e000 00000003 00001800 c7007b37 00000000
	7b00: c7a91880 008976fc 000001a0 c88e5010 00000000 0000002a a7164800 000072d8
	7b20: 00007210 00b43ef2 00000004 00007eac 00000002 00000200 00004000 c70eed00
	7b40: c71637a0 c71637a0 0000002a c0766a2c 00000000 c7b4f800 c0766a34 c04a59a0
	7b60: 00000000 00000001 00000000 00000001 00000000 00000000 c7a91880 c7897bbc
	7b80: c07c1374 00000000 c71637a0 00000000 00000000 c70eed00 c71637a0 c7a91880
	7ba0: c7b4f800 00000000 00000000 00000001 00000000 c04c0004 c71637a0 00000010
	7bc0: c71637a0 c70eed00 c7b4f800 00000000 c7a91880 c04a5f60 c70eed68 00000001
	7be0: c70ee322 00000000 00000004 fffffff4 c71637a0 c71637a0 c7b4f800 0417a8c0

63 more quoted lines

Hi Uwe

I don't think it is as simple as this. If you are sending a packet,
fec_enet_open() must of been called. This does a pm_runtime_get_sync()
to ensure the ipg-clock is ticking, and fec_enet_clk_enable() to
enable all other clocks.

Can you debug this further to find out which clock is off, and where
it gets turned off?
As I think it would be bold to fix this commit once more for 4.2, I
suggest to revert 8fff755e9f8d0f70a595e79f248695ce6aef5cc3.
(To be honest I wonder why v5 of this commit was even taken for -rc3.)

The straight forward fix for the MDIO issue that was intended to be
addressed by 8fff755e9f8d is to add clk_prepare_enable and matching
_unprepare_disable calls to the mdio callbacks, right?
You mean v1 of the patch?

https://patchwork.ozlabs.org/patch/483668/

Or at least something similar. That idea got NACK because it is
thought to consume too much power.

	Andrew

Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using mdio bus

From: Uwe Kleine-König <hidden>
Date: 2015-08-03 15:03:03

Hello,

On Mon, Aug 03, 2015 at 03:50:23PM +0200, Andrew Lunn wrote:
quoted
The problem is that on i.MX27 there are two clocks involved that both
must be on to send a packet, while on i.MX6 it's only a single one
(abstracted by having ipg-clock = ahb-clock). With the suggested patch
only a single one is asserted to be on. This is enough for i.MX6 but
it's not for i.MX27 (and from looking at the device trees also i.MX25,
i.MX28, and i.MX35 are affected).
I don't think it is as simple as this. If you are sending a packet,
fec_enet_open() must of been called. This does a pm_runtime_get_sync()
to ensure the ipg-clock is ticking, and fec_enet_clk_enable() to
enable all other clocks.

Can you debug this further to find out which clock is off, and where
it gets turned off?
I added a call to pm_runtime_get_sync to fec_enet_start_xmit and put it
back at its end. This way I was able to boot with NFS-root which
resulted in an oops before. But this is wrong because if I set
FEC_MDIO_PM_TIMEOUT to 0 I get an oops anyhow.

I added the following patch:
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 32e3807c650e..508c4af4fde8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -64,6 +64,30 @@
 
 #include "fec.h"
 
+#define fec_pm_runtime_get_sync(_dev)	({				\
+	dev_info((_dev), "%s: get_sync (%d)", __func__, (_dev)->power.usage_count.counter); \
+	pm_runtime_get_sync((_dev));					\
+})
+#define pm_runtime_get_sync(_dev) fec_pm_runtime_get_sync(_dev)
+
+#define fec_pm_runtime_put_autosuspend(_dev)	({			\
+	dev_info((_dev), "%s: put_autosuspend (%d)", __func__, (_dev)->power.usage_count.counter); \
+	pm_runtime_put_autosuspend((_dev));				\
+})
+#define pm_runtime_put_autosuspend(_dev) fec_pm_runtime_put_autosuspend(_dev)
+
+#define fec_clk_prepare_enable(_clk)	({				\
+	pr_info("%s: enable " #_clk "\n", __func__);			\
+	clk_prepare_enable((_clk));					\
+})
+#define clk_prepare_enable(_clk) fec_clk_prepare_enable(_clk)
+
+#define fec_clk_disable_unprepare(_clk)	({				\
+	pr_info("%s: disable " #_clk "\n", __func__);			\
+	clk_disable_unprepare((_clk));					\
+})
+#define clk_disable_unprepare(_clk) fec_clk_disable_unprepare(_clk)
+
 static void set_multicast_list(struct net_device *ndev);
 static void fec_enet_itr_coal_init(struct net_device *ndev);
 
@@ -784,6 +808,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	struct netdev_queue *nq;
 	int ret;
 
+	pr_info("%s\n", __func__);
 	queue = skb_get_queue_mapping(skb);
 	txq = fep->tx_queue[queue];
 	nq = netdev_get_tx_queue(ndev, queue);
@@ -2864,6 +2889,7 @@ fec_enet_open(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	int ret;
 
+	pr_info("%s\n", __func__);
 	ret = pm_runtime_get_sync(&fep->pdev->dev);
 	if (IS_ERR_VALUE(ret))
 		return ret;
@@ -2932,6 +2958,8 @@ fec_enet_close(struct net_device *ndev)
 
 	fec_enet_free_buffers(ndev);
 
+	pr_info("%s\n", __func__);
+
 	return 0;
 }
 
@@ -3416,6 +3444,7 @@ fec_probe(struct platform_device *pdev)
 		goto failed_clk;
 
 	ret = clk_prepare_enable(fep->clk_ipg);
+	clk_prepare_enable(fep->clk_ipg);
 	if (ret)
 		goto failed_clk_ipg;
 
 
Which produced the following output (piped through nl for better
reference):
 
     1	fec_enet_open
     2	fec 1002b000.ethernet: fec_enet_open: get_sync (-1)
     3	fec_runtime_resume: enable fep->clk_ipg
     4	fec_enet_clk_enable: enable fep->clk_ahb
     5	fec 1002b000.ethernet: fec_enet_mdio_write: get_sync (0)
     6	fec 1002b000.ethernet: fec_enet_mdio_write: put_autosuspend (1)
     7	mmc0: new SD card at address 0007
     8	mmcblk0: mmc0:0007 SD01G 972 MiB (ro)
     9	 mmcblk0: p1
    10	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    11	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    12	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    13	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    14	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    15	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    16	fec 1002b000.ethernet: fec_enet_mdio_write: get_sync (0)
    17	fec 1002b000.ethernet: fec_enet_mdio_write: put_autosuspend (1)
    18	fec 1002b000.ethernet eth0: Freescale FEC PHY driver [Generic PHY] (mii_bus:phy_addr=1002b000.etherne:00, irq=-1)
    19	fec_runtime_suspend: disable fep->clk_ipg
    20	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    21	fec_runtime_resume: enable fep->clk_ipg
    22	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    23	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    24	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    25	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    26	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    27	fec_runtime_suspend: disable fep->clk_ipg
    28	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    29	fec_runtime_resume: enable fep->clk_ipg
    30	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    31	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    32	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    33	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    34	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    35	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    36	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    37	fec_runtime_suspend: disable fep->clk_ipg

34 more lines

quoted
As I think it would be bold to fix this commit once more for 4.2, I
suggest to revert 8fff755e9f8d0f70a595e79f248695ce6aef5cc3.
(To be honest I wonder why v5 of this commit was even taken for -rc3.)

The straight forward fix for the MDIO issue that was intended to be
addressed by 8fff755e9f8d is to add clk_prepare_enable and matching
_unprepare_disable calls to the mdio callbacks, right?
You mean v1 of the patch?

https://patchwork.ozlabs.org/patch/483668/

Or at least something similar. That idea got NACK because it is
thought to consume too much power.
I didn't follow the discussion. But yes, that's what I'd do. I'm with
Florian here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help