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

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

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