Skip to contentSkip to conversation

fec: Improve resources handling

PATCH In review Fabio Estevam 3 participants 9 messages Jul 21, 2013 – Jul 22, 2013

Subsystems (3)
Networking Network drivers Freescale i.MX / MXC FEC driver
lore
Share & tools
lore

Fabio Estevam

author
#0 lore raw

cover fec: Improve resources handling

This series improves clock handling in the driver by not enabling/disabling the optional ptp and enet_out clocks unconditionally, check for the return value of clk_prepare_enable and also handle clk_ptp in suspend/resume.

Remove an unneeded check in platform_get_resource() and also use devm_request_irq() that can help to simplify the code.

Fabio Estevam (6):
fec: Do not enable/disable optional clocks unconditionally
fec: Fix the order for enabling/disabling the clocks
fec: Enable/disable clk_ptp in suspend/resume
fec: Check the return value from clk_prepare_enable()
fec: Remove unneeded check in platform_get_resource()
fec: Use devm_request_irq()

 drivers/net/ethernet/freescale/fec_main.c | 110 ++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 37 deletions(-)
Signature · 1 line
1.8.1.2
author
#1 lore raw

PATCH 1/6 fec: Do not enable/disable optional clocks unconditionally

From: Fabio Estevam <[email protected]>

clk_enet_out and clk_ptp are optional clocks, so we should not enable/disable them unconditionally.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/net/ethernet/freescale/fec_main.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)
drivers/net/ethernet/freescale/fec_main.c +16 -8
index f0f0e96..de9bf7a 100644
@@ -2105,8 +2105,10 @@ fec_probe(struct platform_device *pdev)
 
 	clk_prepare_enable(fep->clk_ahb);
 	clk_prepare_enable(fep->clk_ipg);
-	clk_prepare_enable(fep->clk_enet_out);
-	clk_prepare_enable(fep->clk_ptp);
+	if (fep->clk_enet_out)
+		clk_prepare_enable(fep->clk_enet_out);
+	if (fep->clk_ptp)
+		clk_prepare_enable(fep->clk_ptp);
 
 	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
 	if (!IS_ERR(fep->reg_phy)) {

@@ -2179,8 +2181,10 @@ failed_init:
 failed_regulator:
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
-	clk_disable_unprepare(fep->clk_enet_out);
-	clk_disable_unprepare(fep->clk_ptp);
+	if (fep->clk_enet_out)
+		clk_disable_unprepare(fep->clk_enet_out);
+	if (fep->clk_ptp)
+		clk_disable_unprepare(fep->clk_ptp);
 failed_clk:
 failed_ioremap:
 	free_netdev(ndev);

@@ -2206,10 +2210,12 @@ fec_drv_remove(struct platform_device *pdev)
 	}
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
-	clk_disable_unprepare(fep->clk_ptp);
+	if (fep->clk_ptp)
+		clk_disable_unprepare(fep->clk_ptp);
 	if (fep->ptp_clock)
 		ptp_clock_unregister(fep->ptp_clock);
-	clk_disable_unprepare(fep->clk_enet_out);
+	if (fep->clk_enet_out)
+		clk_disable_unprepare(fep->clk_enet_out);
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
 	free_netdev(ndev);

@@ -2228,7 +2234,8 @@ fec_suspend(struct device *dev)
 		fec_stop(ndev);
 		netif_device_detach(ndev);
 	}
-	clk_disable_unprepare(fep->clk_enet_out);
+	if (fep->clk_enet_out)
+		clk_disable_unprepare(fep->clk_enet_out);
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
 

@@ -2251,7 +2258,8 @@ fec_resume(struct device *dev)
 			return ret;
 	}
 
-	clk_prepare_enable(fep->clk_enet_out);
+	if (fep->clk_enet_out)
+		clk_prepare_enable(fep->clk_enet_out);
 	clk_prepare_enable(fep->clk_ahb);
 	clk_prepare_enable(fep->clk_ipg);
 	if (netif_running(ndev)) {
Signature · 1 line
1.8.1.2
author
#2 lore raw

PATCH 2/6 fec: Fix the order for enabling/disabling the clocks

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

On fec_probe the clocks are enabled in the following order:

clk_ahb -> clk_ipg -> clk_enet_out -> clk_ptp

, so in the error and remove paths we should disabled them in the opposite order.

Also fix the order in the suspend/resume functions.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/net/ethernet/freescale/fec_main.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/net/ethernet/freescale/fec_main.c +8 -8
index de9bf7a..5e82200 100644
@@ -2179,12 +2179,12 @@ failed_init:
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
 failed_regulator:
-	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)
 		clk_disable_unprepare(fep->clk_ptp);
+	if (fep->clk_enet_out)
+		clk_disable_unprepare(fep->clk_enet_out);
+	clk_disable_unprepare(fep->clk_ipg);
+	clk_disable_unprepare(fep->clk_ahb);
 failed_clk:
 failed_ioremap:
 	free_netdev(ndev);

@@ -2216,8 +2216,8 @@ fec_drv_remove(struct platform_device *pdev)
 		ptp_clock_unregister(fep->ptp_clock);
 	if (fep->clk_enet_out)
 		clk_disable_unprepare(fep->clk_enet_out);
-	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
+	clk_disable_unprepare(fep->clk_ahb);
 	free_netdev(ndev);
 
 	return 0;

@@ -2236,8 +2236,8 @@ fec_suspend(struct device *dev)
 	}
 	if (fep->clk_enet_out)
 		clk_disable_unprepare(fep->clk_enet_out);
-	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
+	clk_disable_unprepare(fep->clk_ahb);
 
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);

@@ -2258,10 +2258,10 @@ fec_resume(struct device *dev)
 			return ret;
 	}
 
-	if (fep->clk_enet_out)
-		clk_prepare_enable(fep->clk_enet_out);
 	clk_prepare_enable(fep->clk_ahb);
 	clk_prepare_enable(fep->clk_ipg);
+	if (fep->clk_enet_out)
+		clk_prepare_enable(fep->clk_enet_out);
 	if (netif_running(ndev)) {
 		fec_restart(ndev, fep->full_duplex);
 		netif_device_attach(ndev);
Signature · 1 line
1.8.1.2
author
#3 lore raw

PATCH 3/6 fec: Enable/disable clk_ptp in suspend/resume

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

clk_ptp should also be enabled in fec_resume() and disabled in fec_suspend().

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/net/ethernet/freescale/fec_main.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/net/ethernet/freescale/fec_main.c +4 -0
index 5e82200..886a372 100644
@@ -2234,6 +2234,8 @@ fec_suspend(struct device *dev)
 		fec_stop(ndev);
 		netif_device_detach(ndev);
 	}
+	if (fep->clk_ptp)
+		clk_disable_unprepare(fep->clk_ptp);
 	if (fep->clk_enet_out)
 		clk_disable_unprepare(fep->clk_enet_out);
 	clk_disable_unprepare(fep->clk_ipg);

@@ -2262,6 +2264,8 @@ fec_resume(struct device *dev)
 	clk_prepare_enable(fep->clk_ipg);
 	if (fep->clk_enet_out)
 		clk_prepare_enable(fep->clk_enet_out);
+	if (fep->clk_ptp)
+		clk_prepare_enable(fep->clk_ptp);
 	if (netif_running(ndev)) {
 		fec_restart(ndev, fep->full_duplex);
 		netif_device_attach(ndev);
Signature · 1 line
1.8.1.2
author
#4 lore raw

PATCH 4/6 fec: Check the return value from clk_prepare_enable()

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

clk_prepare_enable() may fail, so let's check its return value and propagate it in the case of error.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/net/ethernet/freescale/fec_main.c | 66 +++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 12 deletions(-)
drivers/net/ethernet/freescale/fec_main.c +54 -12
index 886a372..acc1850 100644
@@ -2103,12 +2103,25 @@ fec_probe(struct platform_device *pdev)
 		fep->bufdesc_ex = 0;
 	}
 
-	clk_prepare_enable(fep->clk_ahb);
-	clk_prepare_enable(fep->clk_ipg);
-	if (fep->clk_enet_out)
-		clk_prepare_enable(fep->clk_enet_out);
-	if (fep->clk_ptp)
-		clk_prepare_enable(fep->clk_ptp);
+	ret = clk_prepare_enable(fep->clk_ahb);
+	if (ret)
+		goto failed_clk;
+
+	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)
+			goto failed_clk_enet_out;
+	}
+
+	if (fep->clk_ptp) {
+		ret = clk_prepare_enable(fep->clk_ptp);
+		if (ret)
+			goto failed_clk_ptp;
+	}
 
 	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
 	if (!IS_ERR(fep->reg_phy)) {

@@ -2181,9 +2194,12 @@ failed_init:
 failed_regulator:
 	if (fep->clk_ptp)
 		clk_disable_unprepare(fep->clk_ptp);
+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);
 failed_clk:
 failed_ioremap:

@@ -2260,18 +2276,44 @@ fec_resume(struct device *dev)
 			return ret;
 	}
 
-	clk_prepare_enable(fep->clk_ahb);
-	clk_prepare_enable(fep->clk_ipg);
-	if (fep->clk_enet_out)
-		clk_prepare_enable(fep->clk_enet_out);
-	if (fep->clk_ptp)
-		clk_prepare_enable(fep->clk_ptp);
+	ret = clk_prepare_enable(fep->clk_ahb);
+	if (ret)
+		goto failed_clk_ahb;
+
+	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)
+			goto failed_clk_enet_out;
+	}
+
+	if (fep->clk_ptp) {
+		ret = clk_prepare_enable(fep->clk_ptp);
+		if (ret)
+			goto failed_clk_ptp;
+	}
+
 	if (netif_running(ndev)) {
 		fec_restart(ndev, fep->full_duplex);
 		netif_device_attach(ndev);
 	}
 
 	return 0;
+
+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);
+failed_clk_ahb:
+	if (fep->reg_phy)
+		regulator_disable(fep->reg_phy);
+	return ret;
 }
 #endif /* CONFIG_PM_SLEEP */
Signature · 1 line
1.8.1.2
author
#5 lore raw

PATCH 5/6 fec: Remove unneeded check in platform_get_resource()

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

As devm_ioremap_resource() is used, there is no need to explicitely check the return value from platform_get_resource(), as this is something that devm_ioremap_resource() takes care by itself.

Also, place platform_get_resource() prior to devm_ioremap_resource() for better code readability.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/net/ethernet/freescale/fec_main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/net/ethernet/freescale/fec_main.c +1 -4
index acc1850..03028c4 100644
@@ -2033,10 +2033,6 @@ fec_probe(struct platform_device *pdev)
 	if (of_id)
 		pdev->id_entry = of_id->data;
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!r)
-		return -ENXIO;
-
 	/* Init network device */
 	ndev = alloc_etherdev(sizeof(struct fec_enet_private));
 	if (!ndev)

@@ -2054,6 +2050,7 @@ fec_probe(struct platform_device *pdev)
 		fep->pause_flag |= FEC_PAUSE_FLAG_AUTONEG;
 #endif
 
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	fep->hwp = devm_ioremap_resource(&pdev->dev, r);
 	if (IS_ERR(fep->hwp)) {
 		ret = PTR_ERR(fep->hwp);
Signature · 1 line
1.8.1.2
author
#6 lore raw

PATCH 6/6 fec: Use devm_request_irq()

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

Using devm_request_irq() can make the code smaller and cleaner.

Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/net/ethernet/freescale/fec_main.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)
drivers/net/ethernet/freescale/fec_main.c +3 -18
index 03028c4..0642006 100644
@@ -2149,14 +2149,10 @@ fec_probe(struct platform_device *pdev)
 			ret = irq;
 			goto failed_irq;
 		}
-		ret = request_irq(irq, fec_enet_interrupt, IRQF_DISABLED, pdev->name, ndev);
-		if (ret) {
-			while (--i >= 0) {
-				irq = platform_get_irq(pdev, i);
-				free_irq(irq, ndev);
-			}
+		ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
+				       IRQF_DISABLED, pdev->name, ndev);
+		if (ret)
 			goto failed_irq;
-		}
 	}
 
 	ret = fec_enet_mii_init(pdev);

@@ -2180,11 +2176,6 @@ failed_register:
 	fec_enet_mii_remove(fep);
 failed_mii_init:
 failed_irq:
-	for (i = 0; i < FEC_IRQ_NUM; i++) {
-		irq = platform_get_irq(pdev, i);
-		if (irq > 0)
-			free_irq(irq, ndev);
-	}
 failed_init:
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);

@@ -2210,17 +2201,11 @@ fec_drv_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
-	int i;
 
 	cancel_delayed_work_sync(&(fep->delay_work.delay_work));
 	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
 	del_timer_sync(&fep->time_keep);
-	for (i = 0; i < FEC_IRQ_NUM; i++) {
-		int irq = platform_get_irq(pdev, i);
-		if (irq > 0)
-			free_irq(irq, ndev);
-	}
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
 	if (fep->clk_ptp)
Signature · 1 line
1.8.1.2
#7 lore raw
In reply to Fabio Estevam

2013/7/22 Fabio Estevam <[email protected]>:

Quoted 15 lines

This series improves clock handling in the driver by not enabling/disabling the optional ptp and enet_out clocks unconditionally, check for the return value of clk_prepare_enable and also handle clk_ptp in suspend/resume.

Remove an unneeded check in platform_get_resource() and also use devm_request_irq() that can help to simplify the code.

Fabio Estevam (6):
fec: Do not enable/disable optional clocks unconditionally
fec: Fix the order for enabling/disabling the clocks
fec: Enable/disable clk_ptp in suspend/resume
fec: Check the return value from clk_prepare_enable()
fec: Remove unneeded check in platform_get_resource()
fec: Use devm_request_irq()

Quoted 2 lines
 drivers/net/ethernet/freescale/fec_main.c | 110 ++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 37 deletions(-)

Acked

Quoted 8 lines

--
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#8 lore raw

From: Frank Li <[email protected]>
Date: Mon, 22 Jul 2013 12:21:58 +0800

Quoted 16 lines

2013/7/22 Fabio Estevam <[email protected]>:

Quoted 15 lines

This series improves clock handling in the driver by not enabling/disabling the optional ptp and enet_out clocks unconditionally, check for the return value of clk_prepare_enable and also handle clk_ptp in suspend/resume.

Remove an unneeded check in platform_get_resource() and also use devm_request_irq() that can help to simplify the code.

Fabio Estevam (6):
fec: Do not enable/disable optional clocks unconditionally
fec: Fix the order for enabling/disabling the clocks
fec: Enable/disable clk_ptp in suspend/resume
fec: Check the return value from clk_prepare_enable()
fec: Remove unneeded check in platform_get_resource()
fec: Use devm_request_irq()

Quoted 4 lines
Quoted 2 lines
 drivers/net/ethernet/freescale/fec_main.c | 110 ++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 37 deletions(-)

Acked

Series applied to net-next, thanks.