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

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

[Andrew Lunn]

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: tyler.baker@linaro.org
Cc: fabio.estevam@freescale.com
Cc: shawn.guo@linaro.org
---

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

[Fabio Estevam]

Hi Andrew,

On Sat, Jul 25, 2015 at 5:38 PM, Andrew Lunn <andrew@lunn.ch> 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: tyler.baker@linaro.org
> Cc: fabio.estevam@freescale.com
> Cc: shawn.guo@linaro.org
> ---
>
> 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 <fabio.estevam@freescale.com>

Thanks

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

[Tyler Baker]

Hi Andrew,

On 25 July 2015 at 13:38, Andrew Lunn <andrew@lunn.ch> 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: tyler.baker@linaro.org
> Cc: fabio.estevam@freescale.com
> Cc: shawn.guo@linaro.org
> ---
>
> 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 <tyler.baker@linaro.org>

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

[Andrew Lunn]

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

[David Miller]

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

[Uwe Kleine-König]

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
	7cc0: c7b4f944 c04b082c 00000000 c7b4f800 c70fa900 c7b4f800 c70fa900 c7292e88
	7ce0: c70ee200 c7b4f944 00000010 c04cfa34 00000000 0417a8c0 c07954b4 c7292e88
	7d00: c729f9e0 00000001 00000000 c7b4f800 c70fa900 c7296600 c07b66e0 c04d1d5c
	7d20: 00000000 c7ee5bf0 c7891388 00000020 c7292e88 c729f9e0 00000000 c729fbbc
	7d40: c7292e88 c729f9e0 00000000 c729fbbc 00a40000 c04cfddc 8d5d04da 00000000
	7d60: 8d4fdf3b c7292de0 8d5d04da c729f9e0 c7292e88 00000000 00000000 000005b4
	7d80: c729fd60 00000020 c07b66e0 c04e7e64 05b4000b 00000004 00000000 ffff8c9e
	7da0: 00000000 00000000 00000000 c729f9e0 c7292de0 c729fdb0 c729fd60 c7897de4
	7dc0: 00000000 c07707e0 c07b66e0 c04e9b10 00000001 c7897de4 00000000 c049d4c0
	7de0: 9c33a999 00000904 fe103136 c729f9e0 0417a8c0 00000002 c70fa900 c729fbbc
	7e00: c729fb78 c70fa900 00006f00 c04ec638 c7801280 c729100c 00000000 00000000
	7e20: 000000d0 00020000 00000000 00000000 00000000 6f00e494 f218a8c0 0417a8c0
	7e40: 00000000 00000000 00000000 00000000 00000000 00000000 c04e9200 c729f9e0
	7e60: 00000800 00000010 c729100c c74052a0 c74052a0 c07c0b7c c076d664 c0501888
	7e80: cccccccd 00000000 c729f9e0 c04dabf0 00000000 c0491a78 c07b66e0 00000002
	7ea0: c059a4d8 c74052a0 00000800 00000010 c729100c c74052a0 00000000 c07c0b7c
	7ec0: c076d664 c0501948 00000800 c729f9e0 c7291304 c05246d0 c7291000 c0526734
	7ee0: 00000004 00000001 0000003c 00000003 00000001 0002bf20 c7291304 c7804ca0
	7f00: 00000000 c7ee7c00 c076d664 c002e578 c076d664 c005c310 c077025c c7804ca0
	7f20: c076d664 c7804cb8 c7896000 c076d674 00000008 c07c0895 c076d664 c002e8f8
	7f40: c7896000 c076d7c4 c7804ca0 c781ddc0 00000000 c7804ca0 c002e8bc 00000000
	7f60: 00000000 00000000 00000000 c0033af0 0000ffff 00000000 0000ffff c7804ca0
	7f80: 00000000 c7897f84 c7897f84 00000000 c7897f90 c7897f90 c7897fac c781ddc0
	7fa0: c0033a30 00000000 00000000 c000a340 00000000 00000000 00000000 00000000
	7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
	7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 0000ffff 00aeffff
	[<c038ea88>] (fec_enet_start_xmit) from [<c04a59a0>] (dev_hard_start_xmit+0x244/0x490)
	[<c04a59a0>] (dev_hard_start_xmit) from [<c04c0004>] (sch_direct_xmit+0xe0/0x24c)
	[<c04c0004>] (sch_direct_xmit) from [<c04a5f60>] (__dev_queue_xmit+0x258/0x70c)
	[<c04a5f60>] (__dev_queue_xmit) from [<c04faee0>] (arp_xmit+0x7c/0x8c)
	[<c04faee0>] (arp_xmit) from [<c04fbc90>] (arp_solicit+0xc8/0x1a4)
	[<c04fbc90>] (arp_solicit) from [<c04ad914>] (neigh_probe+0x64/0xa0)
	[<c04ad914>] (neigh_probe) from [<c04b06ac>] (__neigh_event_send+0x278/0x2e4)
	[<c04b06ac>] (__neigh_event_send) from [<c04b082c>] (neigh_resolve_output+0x114/0x19c)
	[<c04b082c>] (neigh_resolve_output) from [<c04cfa34>] (ip_finish_output2+0x128/0x3a4)
	[<c04cfa34>] (ip_finish_output2) from [<c04d1d5c>] (ip_output+0xf0/0x108)
	[<c04d1d5c>] (ip_output) from [<c04cfddc>] (ip_queue_xmit+0x12c/0x370)
	[<c04cfddc>] (ip_queue_xmit) from [<c04e7e64>] (tcp_transmit_skb+0x470/0x9c0)
	[<c04e7e64>] (tcp_transmit_skb) from [<c04e9b10>] (tcp_connect+0x5c4/0x6f8)
	[<c04e9b10>] (tcp_connect) from [<c04ec638>] (tcp_v4_connect+0x27c/0x408)
	[<c04ec638>] (tcp_v4_connect) from [<c0501888>] (__inet_stream_connect+0x264/0x2f0)
	[<c0501888>] (__inet_stream_connect) from [<c0501948>] (inet_stream_connect+0x34/0x48)
	[<c0501948>] (inet_stream_connect) from [<c0526734>] (xs_tcp_setup_socket+0x60/0x444)
	[<c0526734>] (xs_tcp_setup_socket) from [<c002e578>] (process_one_work+0x144/0x488)
	[<c002e578>] (process_one_work) from [<c002e8f8>] (worker_thread+0x3c/0x520)
	[<c002e8f8>] (worker_thread) from [<c0033af0>] (kthread+0xc0/0xdc)
	[<c0033af0>] (kthread) from [<c000a340>] (ret_from_fork+0x14/0x34)
	Code: 03a02f7b 13a02014 e0833002 e3a02000 (e5832000) 
	---[ end trace 9bc5f5f5fa9af0cb ]---
	Kernel panic - not syncing: Fatal exception in interrupt

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

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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Andrew Lunn]

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:
> > 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
> 	7cc0: c7b4f944 c04b082c 00000000 c7b4f800 c70fa900 c7b4f800 c70fa900 c7292e88
> 	7ce0: c70ee200 c7b4f944 00000010 c04cfa34 00000000 0417a8c0 c07954b4 c7292e88
> 	7d00: c729f9e0 00000001 00000000 c7b4f800 c70fa900 c7296600 c07b66e0 c04d1d5c
> 	7d20: 00000000 c7ee5bf0 c7891388 00000020 c7292e88 c729f9e0 00000000 c729fbbc
> 	7d40: c7292e88 c729f9e0 00000000 c729fbbc 00a40000 c04cfddc 8d5d04da 00000000
> 	7d60: 8d4fdf3b c7292de0 8d5d04da c729f9e0 c7292e88 00000000 00000000 000005b4
> 	7d80: c729fd60 00000020 c07b66e0 c04e7e64 05b4000b 00000004 00000000 ffff8c9e
> 	7da0: 00000000 00000000 00000000 c729f9e0 c7292de0 c729fdb0 c729fd60 c7897de4
> 	7dc0: 00000000 c07707e0 c07b66e0 c04e9b10 00000001 c7897de4 00000000 c049d4c0
> 	7de0: 9c33a999 00000904 fe103136 c729f9e0 0417a8c0 00000002 c70fa900 c729fbbc
> 	7e00: c729fb78 c70fa900 00006f00 c04ec638 c7801280 c729100c 00000000 00000000
> 	7e20: 000000d0 00020000 00000000 00000000 00000000 6f00e494 f218a8c0 0417a8c0
> 	7e40: 00000000 00000000 00000000 00000000 00000000 00000000 c04e9200 c729f9e0
> 	7e60: 00000800 00000010 c729100c c74052a0 c74052a0 c07c0b7c c076d664 c0501888
> 	7e80: cccccccd 00000000 c729f9e0 c04dabf0 00000000 c0491a78 c07b66e0 00000002
> 	7ea0: c059a4d8 c74052a0 00000800 00000010 c729100c c74052a0 00000000 c07c0b7c
> 	7ec0: c076d664 c0501948 00000800 c729f9e0 c7291304 c05246d0 c7291000 c0526734
> 	7ee0: 00000004 00000001 0000003c 00000003 00000001 0002bf20 c7291304 c7804ca0
> 	7f00: 00000000 c7ee7c00 c076d664 c002e578 c076d664 c005c310 c077025c c7804ca0
> 	7f20: c076d664 c7804cb8 c7896000 c076d674 00000008 c07c0895 c076d664 c002e8f8
> 	7f40: c7896000 c076d7c4 c7804ca0 c781ddc0 00000000 c7804ca0 c002e8bc 00000000
> 	7f60: 00000000 00000000 00000000 c0033af0 0000ffff 00000000 0000ffff c7804ca0
> 	7f80: 00000000 c7897f84 c7897f84 00000000 c7897f90 c7897f90 c7897fac c781ddc0
> 	7fa0: c0033a30 00000000 00000000 c000a340 00000000 00000000 00000000 00000000
> 	7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 	7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 0000ffff 00aeffff
> 	[<c038ea88>] (fec_enet_start_xmit) from [<c04a59a0>] (dev_hard_start_xmit+0x244/0x490)
> 	[<c04a59a0>] (dev_hard_start_xmit) from [<c04c0004>] (sch_direct_xmit+0xe0/0x24c)
> 	[<c04c0004>] (sch_direct_xmit) from [<c04a5f60>] (__dev_queue_xmit+0x258/0x70c)
> 	[<c04a5f60>] (__dev_queue_xmit) from [<c04faee0>] (arp_xmit+0x7c/0x8c)
> 	[<c04faee0>] (arp_xmit) from [<c04fbc90>] (arp_solicit+0xc8/0x1a4)
> 	[<c04fbc90>] (arp_solicit) from [<c04ad914>] (neigh_probe+0x64/0xa0)
> 	[<c04ad914>] (neigh_probe) from [<c04b06ac>] (__neigh_event_send+0x278/0x2e4)
> 	[<c04b06ac>] (__neigh_event_send) from [<c04b082c>] (neigh_resolve_output+0x114/0x19c)
> 	[<c04b082c>] (neigh_resolve_output) from [<c04cfa34>] (ip_finish_output2+0x128/0x3a4)
> 	[<c04cfa34>] (ip_finish_output2) from [<c04d1d5c>] (ip_output+0xf0/0x108)
> 	[<c04d1d5c>] (ip_output) from [<c04cfddc>] (ip_queue_xmit+0x12c/0x370)
> 	[<c04cfddc>] (ip_queue_xmit) from [<c04e7e64>] (tcp_transmit_skb+0x470/0x9c0)
> 	[<c04e7e64>] (tcp_transmit_skb) from [<c04e9b10>] (tcp_connect+0x5c4/0x6f8)
> 	[<c04e9b10>] (tcp_connect) from [<c04ec638>] (tcp_v4_connect+0x27c/0x408)
> 	[<c04ec638>] (tcp_v4_connect) from [<c0501888>] (__inet_stream_connect+0x264/0x2f0)
> 	[<c0501888>] (__inet_stream_connect) from [<c0501948>] (inet_stream_connect+0x34/0x48)
> 	[<c0501948>] (inet_stream_connect) from [<c0526734>] (xs_tcp_setup_socket+0x60/0x444)
> 	[<c0526734>] (xs_tcp_setup_socket) from [<c002e578>] (process_one_work+0x144/0x488)
> 	[<c002e578>] (process_one_work) from [<c002e8f8>] (worker_thread+0x3c/0x520)
> 	[<c002e8f8>] (worker_thread) from [<c0033af0>] (kthread+0xc0/0xdc)
> 	[<c0033af0>] (kthread) from [<c000a340>] (ret_from_fork+0x14/0x34)
> 	Code: 03a02f7b 13a02014 e0833002 e3a02000 (e5832000) 
> 	---[ end trace 9bc5f5f5fa9af0cb ]---
> 	Kernel panic - not syncing: Fatal exception in interrupt
> 
> 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).

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

[Uwe Kleine-König]

Hello,

On Mon, Aug 03, 2015 at 03:50:23PM +0200, Andrew Lunn wrote:
> > 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
    38	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    39	fec_runtime_resume: enable fep->clk_ipg
    40	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    41	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    42	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    43	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    44	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    45	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    46	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    47	fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
    48	fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
    49	fec 1002b000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
    50	Sending DHCP requests .
    51	fec_enet_start_xmit
    52	,
    53	fec_enet_start_xmit
    54	 OK
    55	IP-Config: Got DHCP answer from 192.168.23.4, my address is 192.168.24.64
    56	IP-Config: Complete:
    57	     device=eth0, hwaddr=4a:5c:f5:cb:22:15, ipaddr=192.168.24.64, mask=255.255.0.0, gw=192.168.23.254
    58	     host=192.168.24.64, domain=lab.pengutronix.de, nis-domain=(none)
    59	fec_runtime_suspend: disable fep->clk_ipg
    60	     bootserver=192.168.23.4, rootserver=192.168.23.4, rootpath=
    61	     nameserver0=192.168.23.254
    62	fec_enet_start_xmit

You see, fec_runtime_suspend is called even though _open was called. (And even
though get_sync was called once more than put_autosuspend).
Without the additional clk_prepare_enable in probe line 62 is where the
machine barfs.

I don't understand it starring at the code for a while. But the -1 in
line 2 looks fishy.

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