When ENET_OUT is being used as reference clock for an external PHY,
the clock must not be disabled while the PHY is active. Otherwise the
PHY may lose its internal state and require a reset to become
functional again.
A symptom for this bug is a network interface that constantly toggles
between UP and DOWN state:
fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 800f0000.ethernet eth0: Link is Down
fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 800f0000.ethernet eth0: Link is Down
[...]
Signed-off-by: Lothar Waßmann <[email protected]>
---
drivers/net/ethernet/freescale/fec_main.c | 34 +++++++++++++------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d2328fc..d9df4c5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1857,11 +1857,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
ret = clk_prepare_enable(fep->clk_ahb);
if (ret)
return ret;
- if (fep->clk_enet_out) {
- ret = clk_prepare_enable(fep->clk_enet_out);
- if (ret)
- goto failed_clk_enet_out;
- }
if (fep->clk_ptp) {
mutex_lock(&fep->ptp_clk_mutex);
ret = clk_prepare_enable(fep->clk_ptp);
@@ -1873,35 +1868,26 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
}
mutex_unlock(&fep->ptp_clk_mutex);
}
- if (fep->clk_ref) {
- ret = clk_prepare_enable(fep->clk_ref);
- if (ret)
- goto failed_clk_ref;
- }
+ ret = clk_prepare_enable(fep->clk_ref);
+ if (ret)
+ goto failed_clk_ref;
} else {
clk_disable_unprepare(fep->clk_ahb);
- if (fep->clk_enet_out)
- clk_disable_unprepare(fep->clk_enet_out);
if (fep->clk_ptp) {
mutex_lock(&fep->ptp_clk_mutex);
clk_disable_unprepare(fep->clk_ptp);
fep->ptp_clk_on = false;
mutex_unlock(&fep->ptp_clk_mutex);
}
- if (fep->clk_ref)
- clk_disable_unprepare(fep->clk_ref);
+ clk_disable_unprepare(fep->clk_ref);
}
return 0;
failed_clk_ref:
- if (fep->clk_ref)
- clk_disable_unprepare(fep->clk_ref);
+ clk_disable_unprepare(fep->clk_ref);
failed_clk_ptp:
- if (fep->clk_enet_out)
- clk_disable_unprepare(fep->clk_enet_out);
-failed_clk_enet_out:
- clk_disable_unprepare(fep->clk_ahb);
+ clk_disable_unprepare(fep->clk_ahb);
return ret;
}
@@ -3425,6 +3411,10 @@ fec_probe(struct platform_device *pdev)
if (ret)
goto failed_clk;
+ ret = clk_prepare_enable(fep->clk_enet_out);
+ if (ret)
+ goto failed_clk_enet_out;
+
ret = clk_prepare_enable(fep->clk_ipg);
if (ret)
goto failed_clk_ipg;
@@ -3509,6 +3499,8 @@ failed_init:
if (fep->reg_phy)
regulator_disable(fep->reg_phy);
failed_regulator:
+ clk_disable_unprepare(fep->clk_enet_out);
+failed_clk_enet_out:
clk_disable_unprepare(fep->clk_ipg);
failed_clk_ipg:
fec_enet_clk_enable(ndev, false);
@@ -3531,6 +3523,8 @@ fec_drv_remove(struct platform_device *pdev)
fec_ptp_stop(pdev);
unregister_netdev(ndev);
fec_enet_mii_remove(fep);
+ fec_enet_clk_enable(ndev, false);
+ clk_disable_unprepare(fep->clk_enet_out);
if (fep->reg_phy)
regulator_disable(fep->reg_phy);
of_node_put(fep->phy_node);
--
2.1.4
Hi,
Am 27.11.2015 um 14:39 schrieb Lothar Waßmann:
> When ENET_OUT is being used as reference clock for an external PHY,
> the clock must not be disabled while the PHY is active. Otherwise the
> PHY may lose its internal state and require a reset to become
> functional again.
>
> A symptom for this bug is a network interface that constantly toggles
> between UP and DOWN state:
> fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> fec 800f0000.ethernet eth0: Link is Down
> fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> fec 800f0000.ethernet eth0: Link is Down
> [...]
I would add a sentence about the solution, e.g. moving ENET_OUT handling to driver probe etc.
> Signed-off-by: Lothar Waßmann <[email protected]>
> ---
> drivers/net/ethernet/freescale/fec_main.c | 34 +++++++++++++------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d2328fc..d9df4c5 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1857,11 +1857,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
> ret = clk_prepare_enable(fep->clk_ahb);
> if (ret)
> return ret;
> - if (fep->clk_enet_out) {
> - ret = clk_prepare_enable(fep->clk_enet_out);
> - if (ret)
> - goto failed_clk_enet_out;
> - }
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> ret = clk_prepare_enable(fep->clk_ptp);
> @@ -1873,35 +1868,26 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
> }
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref) {
> - ret = clk_prepare_enable(fep->clk_ref);
> - if (ret)
> - goto failed_clk_ref;
> - }
> + ret = clk_prepare_enable(fep->clk_ref);
> + if (ret)
> + goto failed_clk_ref;
This change seems unrelated to the problem. At least, I can leave this part out
and the toggle still disappear after apply the remaining parts.
However, I've only my Duckbill (iMX28) around to test with.
> } else {
> clk_disable_unprepare(fep->clk_ahb);
> - if (fep->clk_enet_out)
> - clk_disable_unprepare(fep->clk_enet_out);
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> clk_disable_unprepare(fep->clk_ptp);
> fep->ptp_clk_on = false;
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref)
> - clk_disable_unprepare(fep->clk_ref);
> + clk_disable_unprepare(fep->clk_ref);
Same as above, might be unrelated.
> }
>
> return 0;
>
> failed_clk_ref:
> - if (fep->clk_ref)
> - clk_disable_unprepare(fep->clk_ref);
> + clk_disable_unprepare(fep->clk_ref);
dito
> failed_clk_ptp:
> - if (fep->clk_enet_out)
> - clk_disable_unprepare(fep->clk_enet_out);
> -failed_clk_enet_out:
> - clk_disable_unprepare(fep->clk_ahb);
> + clk_disable_unprepare(fep->clk_ahb);
>
> return ret;
> }
> @@ -3425,6 +3411,10 @@ fec_probe(struct platform_device *pdev)
> if (ret)
> goto failed_clk;
>
> + ret = clk_prepare_enable(fep->clk_enet_out);
> + if (ret)
> + goto failed_clk_enet_out;
> +
As enet_out is optional, shouldn't this block be guarded by
if (fep->clk_enet_out)... ?
> ret = clk_prepare_enable(fep->clk_ipg);
> if (ret)
> goto failed_clk_ipg;
> @@ -3509,6 +3499,8 @@ failed_init:
> if (fep->reg_phy)
> regulator_disable(fep->reg_phy);
> failed_regulator:
> + clk_disable_unprepare(fep->clk_enet_out);
here too?
> +failed_clk_enet_out:
> clk_disable_unprepare(fep->clk_ipg);
> failed_clk_ipg:
> fec_enet_clk_enable(ndev, false);
> @@ -3531,6 +3523,8 @@ fec_drv_remove(struct platform_device *pdev)
> fec_ptp_stop(pdev);
> unregister_netdev(ndev);
> fec_enet_mii_remove(fep);
> + fec_enet_clk_enable(ndev, false);
> + clk_disable_unprepare(fep->clk_enet_out);
and here too?
> if (fep->reg_phy)
> regulator_disable(fep->reg_phy);
> of_node_put(fep->phy_node);
Mit freundlichen Grüßen / Kind regards
Michael Heimpold
--
Software Engineer
I2SE GmbH Tel: +49 (0) 341 355667-00
Friedrich-Ebert-Str. 61 Fax: +49 (0) 341 355667-02
04109 Leipzig
Germany
Web: http://www.i2se.com/ Mail: [email protected]
VAT No.: DE 811528334
Amtsgericht Leipzig HRB 23784
Geschäftsführer/CEO: Carsten Ziermann
*** Diese E-Mail ist allein für den bezeichneten Adressaten bestimmt. Sie kann rechtlich vertrauliche Informationen enthalten. Wenn Sie diese E-Mail irrtümlich erhalten haben, informieren Sie bitte unverzüglich den Absender per E-Mail und löschen Sie diese E-Mail von Ihrem Computer, ohne Kopien anzufertigen.
Vielen Dank. ***
*** This email is for the exclusive use of the addressee. It may contain legally privileged information. If you have received this message in error, please notify the sender by email immediately and delete the message from your computer without making any copies.
Thank you. ***
From: Lothar Waßmann <[email protected]> Sent: Friday, November 27, 2015 9:39 PM
> To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Kevin Hao; Lothar
> Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe Reynes; Russell King;
> Uwe Kleine-König; [email protected]; [email protected];
> Stefan Agner
> Subject: [PATCH] net: fec: fix enet_out clock handling
>
> When ENET_OUT is being used as reference clock for an external PHY, the
> clock must not be disabled while the PHY is active. Otherwise the PHY may
> lose its internal state and require a reset to become functional again.
>
> A symptom for this bug is a network interface that constantly toggles
> between UP and DOWN state:
> fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control
> rx/tx fec 800f0000.ethernet eth0: Link is Down fec 800f0000.ethernet eth0:
> Link is Up - 100Mbps/Full - flow control rx/tx fec 800f0000.ethernet eth0:
> Link is Down [...]
>
> Signed-off-by: Lothar Waßmann <[email protected]>
> ---
> drivers/net/ethernet/freescale/fec_main.c | 34 +++++++++++++------------
> ------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
When MAC is not ready with clocks disabled, it is not necessary to supply clock for PHY. In fact, PHY also is not ready, why does it need clock ?
For your problem, you must add PHY reset in your dts file to resolve your problem.
I don't agree with this patch.
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index d2328fc..d9df4c5 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1857,11 +1857,6 @@ static int fec_enet_clk_enable(struct net_device
> *ndev, bool enable)
> ret = clk_prepare_enable(fep->clk_ahb);
> if (ret)
> return ret;
> - if (fep->clk_enet_out) {
> - ret = clk_prepare_enable(fep->clk_enet_out);
> - if (ret)
> - goto failed_clk_enet_out;
> - }
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> ret = clk_prepare_enable(fep->clk_ptp); @@ -1873,35
> +1868,26 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool
> enable)
> }
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref) {
> - ret = clk_prepare_enable(fep->clk_ref);
> - if (ret)
> - goto failed_clk_ref;
> - }
> + ret = clk_prepare_enable(fep->clk_ref);
> + if (ret)
> + goto failed_clk_ref;
> } else {
> clk_disable_unprepare(fep->clk_ahb);
> - if (fep->clk_enet_out)
> - clk_disable_unprepare(fep->clk_enet_out);
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> clk_disable_unprepare(fep->clk_ptp);
> fep->ptp_clk_on = false;
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref)
> - clk_disable_unprepare(fep->clk_ref);
> + clk_disable_unprepare(fep->clk_ref);
> }
>
> return 0;
>
> failed_clk_ref:
> - if (fep->clk_ref)
> - clk_disable_unprepare(fep->clk_ref);
> + clk_disable_unprepare(fep->clk_ref);
> failed_clk_ptp:
> - if (fep->clk_enet_out)
> - clk_disable_unprepare(fep->clk_enet_out);
> -failed_clk_enet_out:
> - clk_disable_unprepare(fep->clk_ahb);
> + clk_disable_unprepare(fep->clk_ahb);
>
> return ret;
> }
> @@ -3425,6 +3411,10 @@ fec_probe(struct platform_device *pdev)
> if (ret)
> goto failed_clk;
>
> + ret = clk_prepare_enable(fep->clk_enet_out);
> + if (ret)
> + goto failed_clk_enet_out;
> +
> ret = clk_prepare_enable(fep->clk_ipg);
> if (ret)
> goto failed_clk_ipg;
> @@ -3509,6 +3499,8 @@ failed_init:
> if (fep->reg_phy)
> regulator_disable(fep->reg_phy);
> failed_regulator:
> + clk_disable_unprepare(fep->clk_enet_out);
> +failed_clk_enet_out:
> clk_disable_unprepare(fep->clk_ipg);
> failed_clk_ipg:
> fec_enet_clk_enable(ndev, false);
> @@ -3531,6 +3523,8 @@ fec_drv_remove(struct platform_device *pdev)
> fec_ptp_stop(pdev);
> unregister_netdev(ndev);
> fec_enet_mii_remove(fep);
> + fec_enet_clk_enable(ndev, false);
> + clk_disable_unprepare(fep->clk_enet_out);
> if (fep->reg_phy)
> regulator_disable(fep->reg_phy);
> of_node_put(fep->phy_node);
> --
> 2.1.4
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> When MAC is not ready with clocks disabled, it is not necessary to
> supply clock for PHY. In fact, PHY also is not ready, why does it
> need clock ?
How about the case of the "PHY" is actually a switch? You can use the
MDIO bus separate from the MAC, you can configure the switch while the
MAC is down, etc. Packets can be flowing in and out of switch ports,
while the MAC is down. You only need the MAC up when the host wants to
send packets.
Andrew
From: Andrew Lunn <[email protected]> Sent: Sunday, November 29, 2015 12:44 AM
> To: Duan Fugang-B38611
> Cc: Lothar Wa?mann; David S. Miller; Estevam Fabio-R49496; Kevin Hao;
> Lucas Stach; Philippe Reynes; Russell King; Uwe Kleine-K?nig; linux-
> [email protected]; [email protected]; Stefan Agner
> Subject: Re: [PATCH] net: fec: fix enet_out clock handling
>
> > When MAC is not ready with clocks disabled, it is not necessary to
> > supply clock for PHY. In fact, PHY also is not ready, why does it need
> > clock ?
>
> How about the case of the "PHY" is actually a switch? You can use the
> MDIO bus separate from the MAC, you can configure the switch while the
> MAC is down, etc. Packets can be flowing in and out of switch ports,
> while the MAC is down. You only need the MAC up when the host wants to
> send packets.
>
> Andrew
Do you mean PHY switch also use the enet_out clock ?
- If not, this topic is not related to the patch.
- If so, we should add flag check, we can disable the enet_out clock when the phy is not switch or switch phy doesn't use the clock in real case, which can save power. And the patch title is not right, not "fix" since it is not a issue, just different usage for different cases.
Regards,
Andy
> Do you mean PHY switch also use the enet_out clock ?
Yes, the Marvell switches i've have connected to a Vybrid use enet_out
clock. There was a recent change to IMX pinctrl which broke the muxing
for ENET_OUT on Vydrid, which broke the probing of these switches. It
was no longer possible to mux the pin as ENET_OUT, and the switches
disappeared.
Andrew
On Fri, Nov 27, 2015 at 02:39:10PM +0100, Lothar Wa?mann wrote:
> When ENET_OUT is being used as reference clock for an external PHY,
> the clock must not be disabled while the PHY is active. Otherwise the
> PHY may lose its internal state and require a reset to become
> functional again.
>
> A symptom for this bug is a network interface that constantly toggles
> between UP and DOWN state:
> fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> fec 800f0000.ethernet eth0: Link is Down
> fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> fec 800f0000.ethernet eth0: Link is Down
Hi Lothar
When does this up/down happen? During normal operation when the link
is administrative up?
When did this start happening? Did it happen before
8fff755e9f8d net: fec: Ensure clocks are enabled while using mdio bus
Andrew
Hi,
> On Fri, Nov 27, 2015 at 02:39:10PM +0100, Lothar Waßmann wrote:
> > When ENET_OUT is being used as reference clock for an external PHY,
> > the clock must not be disabled while the PHY is active. Otherwise the
> > PHY may lose its internal state and require a reset to become
> > functional again.
> >
> > A symptom for this bug is a network interface that constantly toggles
> > between UP and DOWN state:
> > fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> > fec 800f0000.ethernet eth0: Link is Down
> > fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> > fec 800f0000.ethernet eth0: Link is Down
>
> Hi Lothar
>
> When does this up/down happen? During normal operation when the link
> is administrative up?
>
If booting with NFSROOT the rootfs cannot be mounted, because when the
interface is brought up the PHY starts toggling the link state.
> When did this start happening? Did it happen before
> 8fff755e9f8d net: fec: Ensure clocks are enabled while using mdio bus
>
No. The behaviour started exactly with this commit.
Lothar Waßmann
Hi,
> From: Lothar Waßmann <[email protected]> Sent: Friday, November 27, 2015 9:39 PM
> > To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Kevin Hao; Lothar
> > Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe Reynes; Russell King;
> > Uwe Kleine-König; [email protected]; [email protected];
> > Stefan Agner
> > Subject: [PATCH] net: fec: fix enet_out clock handling
> >
> > When ENET_OUT is being used as reference clock for an external PHY, the
> > clock must not be disabled while the PHY is active. Otherwise the PHY may
> > lose its internal state and require a reset to become functional again.
> >
> > A symptom for this bug is a network interface that constantly toggles
> > between UP and DOWN state:
> > fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control
> > rx/tx fec 800f0000.ethernet eth0: Link is Down fec 800f0000.ethernet eth0:
> > Link is Up - 100Mbps/Full - flow control rx/tx fec 800f0000.ethernet eth0:
> > Link is Down [...]
> >
> > Signed-off-by: Lothar Waßmann <[email protected]>
> > ---
> > drivers/net/ethernet/freescale/fec_main.c | 34 +++++++++++++------------
> > ------
> > 1 file changed, 14 insertions(+), 20 deletions(-)
> >
>
> When MAC is not ready with clocks disabled, it is not necessary to supply clock for PHY. In fact, PHY also is not ready, why does it need clock ?
> For your problem, you must add PHY reset in your dts file to resolve your problem.
>
The phy-reset-gpio property is set in the DTB. But fec_reset_phy()
which asserts the RESET is only called from within the probe() function.
It should probably be called from fec_restart() instead?
Lothar Waßmann
From: Lothar Waßmann <[email protected]> Sent: Monday, November 30, 2015 2:56 PM
> To: Duan Fugang-B38611
> Cc: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Kevin Hao; Lucas
> Stach; Philippe Reynes; Russell King; Uwe Kleine-K?nig; linux-
> [email protected]; [email protected]; Stefan Agner
> Subject: Re: [PATCH] net: fec: fix enet_out clock handling
>
> Hi,
>
> > From: Lothar Waßmann <[email protected]> Sent: Friday, November
> > 27, 2015 9:39 PM
> > > To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Kevin Hao;
> > > Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe Reynes;
> > > Russell King; Uwe Kleine-König; [email protected];
> > > [email protected]; Stefan Agner
> > > Subject: [PATCH] net: fec: fix enet_out clock handling
> > >
> > > When ENET_OUT is being used as reference clock for an external PHY,
> > > the clock must not be disabled while the PHY is active. Otherwise
> > > the PHY may lose its internal state and require a reset to become
> functional again.
> > >
> > > A symptom for this bug is a network interface that constantly
> > > toggles between UP and DOWN state:
> > > fec 800f0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control
> > > rx/tx fec 800f0000.ethernet eth0: Link is Down fec 800f0000.ethernet
> eth0:
> > > Link is Up - 100Mbps/Full - flow control rx/tx fec 800f0000.ethernet
> eth0:
> > > Link is Down [...]
> > >
> > > Signed-off-by: Lothar Waßmann <[email protected]>
> > > ---
> > > drivers/net/ethernet/freescale/fec_main.c | 34
> > > +++++++++++++------------
> > > ------
> > > 1 file changed, 14 insertions(+), 20 deletions(-)
> > >
> >
> > When MAC is not ready with clocks disabled, it is not necessary to
> supply clock for PHY. In fact, PHY also is not ready, why does it need
> clock ?
> > For your problem, you must add PHY reset in your dts file to resolve
> your problem.
> >
> The phy-reset-gpio property is set in the DTB. But fec_reset_phy() which
> asserts the RESET is only called from within the probe() function.
> It should probably be called from fec_restart() instead?
>
After enet_out clock enable, you can call fec_reset_phy() do phy reset. Don't put it in .fec_restart() function because
Cable hotplug test cause phy registers reset to HW default status.
Regards,
Andy
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?