[RFC 0/3] arm: mxs: sanitize enet_out clock handling

[RFC 0/3] arm: mxs: sanitize enet_out clock handling

[Wolfram Sang]

Handling enet_out on MX28 is cumbersome at the moment. Most boards need it
enabled and for that, they have to add code to mach-mxs.c (see sps1 as an
example). Since this is board specific, we better encode it in the devicetree,
that is the reason it was made for.

My proposal will overwrite the generic "clock" and "clock-names" properties
from imx28.dtsi in the board file. The original one has 2 entries, and boards
needing enet_out will overload it with three entries. The network driver will
enable the clock if it was specified. The old code enabling the clock will be
backward compatible but print a WARN if the legacy mode needs to be used.

This will make adding new board easier, since we don't have to touch mach-mxs.c
for this case. It scales much better.

Tested on a MX28EVK. Please review, comment, apply(?)

Thanks,

   Wolfram


Wolfram Sang (3):
  net: freescale: fec: add support for optional enet_out clk
  arm: mxs: add enet_out clock to devicetree
  arm: mach-mxs: make enabling enet_out a legacy function

 arch/arm/boot/dts/imx28-apf28.dts      |    3 +++
 arch/arm/boot/dts/imx28-apx4devkit.dts |    3 +++
 arch/arm/boot/dts/imx28-cfa10049.dts   |    3 +++
 arch/arm/boot/dts/imx28-evk.dts        |    3 +++
 arch/arm/boot/dts/imx28-sps1.dts       |    3 +++
 arch/arm/boot/dts/imx28-tx28.dts       |    3 +++
 arch/arm/mach-mxs/mach-mxs.c           |   24 ++++++++++++++++--------
 drivers/net/ethernet/freescale/fec.c   |   10 ++++++++++
 drivers/net/ethernet/freescale/fec.h   |    1 +
 9 files changed, 45 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[RFC 1/3] net: freescale: fec: add support for optional enet_out clk

[Wolfram Sang]

Some MX28 boards need the internal enet_out clock to be enabled. So, do
this in the driver iff the clock was referenced via devicetree.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---

davidm: Please don't apply before Shawn as the mxs-maintainer has given his ack
on the general procedure. Or maybe he can merge it via his tree?

 drivers/net/ethernet/freescale/fec.c |   10 ++++++++++
 drivers/net/ethernet/freescale/fec.h |    1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 0704bca..6573bd5 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1628,6 +1628,11 @@ fec_probe(struct platform_device *pdev)
 		goto failed_clk;
 	}
 
+	/* enet_out is optional, depends on board */
+	fep->clk_enet_out = devm_clk_get(&pdev->dev, "enet_out");
+	if (IS_ERR(fep->clk_enet_out))
+		fep->clk_enet_out = NULL;
+
 #ifdef CONFIG_FEC_PTP
 	fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
 	if (IS_ERR(fep->clk_ptp)) {
@@ -1638,6 +1643,7 @@ 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);
 #ifdef CONFIG_FEC_PTP
 	clk_prepare_enable(fep->clk_ptp);
 #endif
@@ -1679,6 +1685,7 @@ failed_register:
 failed_mii_init:
 failed_init:
 failed_regulator:
+	clk_disable_unprepare(fep->clk_enet_out);
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
 #ifdef CONFIG_FEC_PTP
@@ -1722,6 +1729,7 @@ fec_drv_remove(struct platform_device *pdev)
 	if (fep->ptp_clock)
 		ptp_clock_unregister(fep->ptp_clock);
 #endif
+	clk_disable_unprepare(fep->clk_enet_out);
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
 	iounmap(fep->hwp);
@@ -1747,6 +1755,7 @@ fec_suspend(struct device *dev)
 		fec_stop(ndev);
 		netif_device_detach(ndev);
 	}
+	clk_disable_unprepare(fep->clk_enet_out);
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
 
@@ -1759,6 +1768,7 @@ fec_resume(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	clk_prepare_enable(fep->clk_enet_out);
 	clk_prepare_enable(fep->clk_ahb);
 	clk_prepare_enable(fep->clk_ipg);
 	if (netif_running(ndev)) {
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index c5a3bc1..0648dbe 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -203,6 +203,7 @@ struct fec_enet_private {
 
 	struct clk *clk_ipg;
 	struct clk *clk_ahb;
+	struct clk *clk_enet_out;
 #ifdef CONFIG_FEC_PTP
 	struct clk *clk_ptp;
 #endif
-- 
1.7.10.4

[RFC 1/3] net: freescale: fec: add support for optional enet_out clk

[Shawn Guo]

Thanks for working on this, Wolfram.

On Tue, Jan 29, 2013 at 03:46:11PM +0100, Wolfram Sang wrote:
> Some MX28 boards need the internal enet_out clock to be enabled. So, do
> this in the driver iff the clock was referenced via devicetree.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> ---
> 
> davidm: Please don't apply before Shawn as the mxs-maintainer has given his ack
> on the general procedure. Or maybe he can merge it via his tree?
> 
I prefer to have the patch go via net tree for 3.9, and then we clean
up mxs platform code from 3.9-rc1.

Shawn

[RFC 1/3] net: freescale: fec: add support for optional enet_out clk

[Wolfram Sang]

On Wed, Jan 30, 2013 at 01:25:50PM +0800, Shawn Guo wrote:
> Thanks for working on this, Wolfram.
> 
> On Tue, Jan 29, 2013 at 03:46:11PM +0100, Wolfram Sang wrote:
> > Some MX28 boards need the internal enet_out clock to be enabled. So, do
> > this in the driver iff the clock was referenced via devicetree.
> > 
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> 
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> 
> > ---
> > 
> > davidm: Please don't apply before Shawn as the mxs-maintainer has given his ack
> > on the general procedure. Or maybe he can merge it via his tree?
> > 
> I prefer to have the patch go via net tree for 3.9, and then we clean
> up mxs platform code from 3.9-rc1.

Fine with me, too.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130131/57a58704/attachment.sig>

[RFC 2/3] arm: mxs: add enet_out clock to devicetree

[Wolfram Sang]

Put the clock to the devicetree, so the driver can take care of it
later. Then, we don't have to do the enabling as a workaround in board
init.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 arch/arm/boot/dts/imx28-apf28.dts      |    3 +++
 arch/arm/boot/dts/imx28-apx4devkit.dts |    3 +++
 arch/arm/boot/dts/imx28-cfa10049.dts   |    3 +++
 arch/arm/boot/dts/imx28-evk.dts        |    3 +++
 arch/arm/boot/dts/imx28-sps1.dts       |    3 +++
 arch/arm/boot/dts/imx28-tx28.dts       |    3 +++
 6 files changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/imx28-apf28.dts b/arch/arm/boot/dts/imx28-apf28.dts
index 7eb0758..3047c0c 100644
--- a/arch/arm/boot/dts/imx28-apf28.dts
+++ b/arch/arm/boot/dts/imx28-apf28.dts
@@ -75,6 +75,9 @@
 
 	ahb at 80080000 {
 		mac0: ethernet at 800f0000 {
+			/* add enet_out clock */
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>;
+			clock-names = "ipg", "ahb", "enet_out";
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
diff --git a/arch/arm/boot/dts/imx28-apx4devkit.dts b/arch/arm/boot/dts/imx28-apx4devkit.dts
index 5171667..b8c4b0c 100644
--- a/arch/arm/boot/dts/imx28-apx4devkit.dts
+++ b/arch/arm/boot/dts/imx28-apx4devkit.dts
@@ -159,6 +159,9 @@
 
 	ahb at 80080000 {
 		mac0: ethernet at 800f0000 {
+			/* add enet_out clock */
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>;
+			clock-names = "ipg", "ahb", "enet_out";
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
diff --git a/arch/arm/boot/dts/imx28-cfa10049.dts b/arch/arm/boot/dts/imx28-cfa10049.dts
index bdc80a4..7204468 100644
--- a/arch/arm/boot/dts/imx28-cfa10049.dts
+++ b/arch/arm/boot/dts/imx28-cfa10049.dts
@@ -145,6 +145,9 @@
 
 	ahb at 80080000 {
 		mac0: ethernet at 800f0000 {
+			/* add enet_out clock */
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>;
+			clock-names = "ipg", "ahb", "enet_out";
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
index 2da316e..6634d71 100644
--- a/arch/arm/boot/dts/imx28-evk.dts
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -224,6 +224,9 @@
 		};
 
 		mac0: ethernet at 800f0000 {
+			/* add enet_out clock */
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>;
+			clock-names = "ipg", "ahb", "enet_out";
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
diff --git a/arch/arm/boot/dts/imx28-sps1.dts b/arch/arm/boot/dts/imx28-sps1.dts
index e6cde8a..1df1859 100644
--- a/arch/arm/boot/dts/imx28-sps1.dts
+++ b/arch/arm/boot/dts/imx28-sps1.dts
@@ -112,6 +112,9 @@
 		};
 
 		mac0: ethernet at 800f0000 {
+			/* add enet_out clock */
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>;
+			clock-names = "ipg", "ahb", "enet_out";
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts
index 37be532..dcc90db 100644
--- a/arch/arm/boot/dts/imx28-tx28.dts
+++ b/arch/arm/boot/dts/imx28-tx28.dts
@@ -89,6 +89,9 @@
 
 	ahb at 80080000 {
 		mac0: ethernet at 800f0000 {
+			/* add enet_out clock */
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>;
+			clock-names = "ipg", "ahb", "enet_out";
 			phy-mode = "rmii";
 			pinctrl-names = "default", "gpio_mode";
 			pinctrl-0 = <&mac0_pins_a>;
-- 
1.7.10.4

[RFC 3/3] arm: mach-mxs: make enabling enet_out a legacy function

[Wolfram Sang]

enet_out is now handled via the driver, so handling it in board init
will print a warning.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 arch/arm/mach-mxs/mach-mxs.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index c66129b..1f2e3bf 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c
@@ -252,17 +252,25 @@ static void __init imx23_evk_init(void)
 	mxsfb_pdata.ld_intf_width = STMLCDIF_24BIT;
 }
 
-static inline void enable_clk_enet_out(void)
+static inline void legacy_enable_clk_enet_out(void)
 {
-	struct clk *clk = clk_get_sys("enet_out", NULL);
+	struct device_node *np;
+	struct clk *clk;
+
+	for_each_compatible_node(np, NULL, "fsl,imx28-fec")
+		if (of_property_match_string(np, "clock-names", "enet_out") >= 0)
+			return;
+
+	WARN(1, "Please specify enet_out-clk in devicetree.\n");
 
+	clk = clk_get_sys("enet_out", NULL);
 	if (!IS_ERR(clk))
 		clk_prepare_enable(clk);
 }
 
 static void __init imx28_evk_init(void)
 {
-	enable_clk_enet_out();
+	legacy_enable_clk_enet_out();
 	update_fec_mac_prop(OUI_FSL);
 
 	mxsfb_pdata.mode_list = mx28evk_video_modes;
@@ -292,7 +300,7 @@ static void __init m28evk_init(void)
 
 static void __init sc_sps1_init(void)
 {
-	enable_clk_enet_out();
+	legacy_enable_clk_enet_out();
 }
 
 static int apx4devkit_phy_fixup(struct phy_device *phy)
@@ -303,7 +311,7 @@ static int apx4devkit_phy_fixup(struct phy_device *phy)
 
 static void __init apx4devkit_init(void)
 {
-	enable_clk_enet_out();
+	legacy_enable_clk_enet_out();
 
 	if (IS_BUILTIN(CONFIG_PHYLIB))
 		phy_register_fixup_for_uid(PHY_ID_KSZ8051, MICREL_PHY_ID_MASK,
@@ -351,7 +359,7 @@ static void __init tx28_post_init(void)
 	struct pinctrl *pctl;
 	int ret;
 
-	enable_clk_enet_out();
+	legacy_enable_clk_enet_out();
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx28-fec");
 	pdev = of_find_device_by_node(np);
@@ -391,13 +399,13 @@ static void __init tx28_post_init(void)
 
 static void __init cfa10049_init(void)
 {
-	enable_clk_enet_out();
+	legacy_enable_clk_enet_out();
 	update_fec_mac_prop(OUI_CRYSTALFONTZ);
 }
 
 static void __init apf28_init(void)
 {
-	enable_clk_enet_out();
+	legacy_enable_clk_enet_out();
 
 	mxsfb_pdata.mode_list = apf28dev_video_modes;
 	mxsfb_pdata.mode_count = ARRAY_SIZE(apf28dev_video_modes);
-- 
1.7.10.4

[RFC 3/3] arm: mach-mxs: make enabling enet_out a legacy function

[Shawn Guo]

On Tue, Jan 29, 2013 at 03:46:13PM +0100, Wolfram Sang wrote:
> @@ -351,7 +359,7 @@ static void __init tx28_post_init(void)
>  	struct pinctrl *pctl;
>  	int ret;
>  
> -	enable_clk_enet_out();
> +	legacy_enable_clk_enet_out();

I think TX28 is the only case that really needs to turn on enet_out
clock at platform level, since it has some dirty work about PHY to do
here.  With with fec driver handling the clock, enable_clk_enet_out()
can just be removed for other boards.

Shawn

>  
>  	np = of_find_compatible_node(NULL, NULL, "fsl,imx28-fec");
>  	pdev = of_find_device_by_node(np);

[RFC 3/3] arm: mach-mxs: make enabling enet_out a legacy function

[Wolfram Sang]

On Wed, Jan 30, 2013 at 01:48:40PM +0800, Shawn Guo wrote:
> On Tue, Jan 29, 2013 at 03:46:13PM +0100, Wolfram Sang wrote:
> > @@ -351,7 +359,7 @@ static void __init tx28_post_init(void)
> >  	struct pinctrl *pctl;
> >  	int ret;
> >  
> > -	enable_clk_enet_out();
> > +	legacy_enable_clk_enet_out();
> 
> I think TX28 is the only case that really needs to turn on enet_out
> clock at platform level, since it has some dirty work about PHY to do
> here.  With with fec driver handling the clock, enable_clk_enet_out()
> can just be removed for other boards.

I would love to, but this will cause regression on boards which update
the kernel but not the devicetree (no third clock), or?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130130/f802a8ba/attachment.sig>

[RFC 3/3] arm: mach-mxs: make enabling enet_out a legacy function

[Shawn Guo]

On Wed, Jan 30, 2013 at 08:58:32AM +0100, Wolfram Sang wrote:
> On Wed, Jan 30, 2013 at 01:48:40PM +0800, Shawn Guo wrote:
> > On Tue, Jan 29, 2013 at 03:46:13PM +0100, Wolfram Sang wrote:
> > > @@ -351,7 +359,7 @@ static void __init tx28_post_init(void)
> > >  	struct pinctrl *pctl;
> > >  	int ret;
> > >  
> > > -	enable_clk_enet_out();
> > > +	legacy_enable_clk_enet_out();
> > 
> > I think TX28 is the only case that really needs to turn on enet_out
> > clock at platform level, since it has some dirty work about PHY to do
> > here.  With with fec driver handling the clock, enable_clk_enet_out()
> > can just be removed for other boards.
> 
> I would love to, but this will cause regression on boards which update
> the kernel but not the devicetree (no third clock), or?
> 
Ah, that's the consideration.  When someday arch/arm/boot/dts gets
maintained outside kernel tree, we may be required to keep devicetree
compatibility to different kernel versions.  But we haven't got there
yet.  Today people are supposed to use devicetree matching kernel.

Shawn

[RFC 0/3] arm: mxs: sanitize enet_out clock handling

[Trent Piepho]

On Tue, Jan 29, 2013 at 6:46 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Handling enet_out on MX28 is cumbersome at the moment. Most boards need it
> enabled and for that, they have to add code to mach-mxs.c (see sps1 as an
> example). Since this is board specific, we better encode it in the
> devicetree, that is the reason it was made for.
>
> My proposal will overwrite the generic "clock" and "clock-names" properties
> from imx28.dtsi in the board file. The original one has 2 entries, and boards
> needing enet_out will overload it with three entries. The network driver will
> enable the clock if it was specified. The old code enabling the clock will be
> backward compatible but print a WARN if the legacy mode needs to be used.

Trying this again with proper CCs.  gmane's reply interface don't
allow cross-posting or CCs, so I'm trying an alternate method where I
export the raw email from gmane, convert it to an mbox file, then
upload it via imap to gmail, so I can reply from there.  We'll see how
badly gmail mangles it.

Anyway, I found this same problem when adding support for a new imx28
board.  Ethernet stopped working as soon as I changed the board name
from imx28-evk since the imx28-evk board setup function stopped
running.  These patches seem like a good way to fix the problem
without require the same board specific code in the kernel to be
duplicated for each new board.

But as you say most boards need enet_out (AFAICT, all but the Denx
board).  So wouldn't it make more sense to add enet_out to the dtsi
file and then just override it in the one board that doesn't need it,
instead of overriding in every board except that one?

[RFC 0/3] arm: mxs: sanitize enet_out clock handling

[Shawn Guo]

On Fri, Mar 15, 2013 at 02:27:35PM -0700, Trent Piepho wrote:
> But as you say most boards need enet_out (AFAICT, all but the Denx
> board).  So wouldn't it make more sense to add enet_out to the dtsi
> file and then just override it in the one board that doesn't need it,
> instead of overriding in every board except that one?

Yes, sounds reasonable.  Since I'm not sure Wolfram is still interested
in the work, I will pick it up from here.

Shawn