[PATCH] net: designware: Don't hang in reset with powered down phy

[PATCH] net: designware: Don't hang in reset with powered down phy

[Trent Piepho]

The dw MAC requires that all clock domains to be running for it to
finish a MAC reset.  This include the clock provided by the PHY.

If the PHY is powered down, bit BMCR_PDOWN set, then it won't be
generating a clock.  And so the MAC never comes out of reset.  On
shutdown, Linux will put the PHY in powerdown mode, so it can easily
be the case that the PHY is powered down on boot.

See Linux kernel commit 2d871aa07136fe6e576bde63072cf33e2c664e95.

Currently the MAC reset is done before the phy is probed.  We can't
power up the phy until it's probed, so the resets must be in the
opposite order.  The MAC reset is in device init but the PHY probe is
in device open.  Device init is done first, always, while open is done
later, and only if the device is used.

Rather than move the phy probe to init, this moves the MAC reset to
open.  It seems better to speed up boots that don't use ethernet by
skipping MAC reset than to slow them down by adding PHY probe.

Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
 drivers/net/designware.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Instead of phy call in the designware driver, it would be nicer to
use the phylib layer to abstract it.  Except that the barebox
phylib has no support for power management and doesn't need it.

So one either adds general PM support just to clear one bit in one
register, which seems like a bloated and overly complex solution.
Or comes up with a generalized method to clear one bit, which seems
pointless.

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 8006527..7109e97 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -218,6 +218,16 @@ static int dwc_ether_init(struct eth_device *dev)
 	struct dw_eth_dev *priv = dev->priv;
 	struct eth_mac_regs *mac_p = priv->mac_regs_p;
 	struct eth_dma_regs *dma_p = priv->dma_regs_p;
+	int bmcr;
+
+	/* Before we reset the mac, we must insure the PHY is not powered down
+	 * as the dw controller needs all clock domains to be running, including
+	 * the PHY clock, to come out of a mac reset.  */
+	bmcr = phy_read(dev->phydev, MII_BMCR);
+	if (bmcr & BMCR_PDOWN) {
+		bmcr &= ~BMCR_PDOWN;
+		phy_write(dev->phydev, MII_BMCR, bmcr);
+	}
 
 	if (mac_reset(dev) < 0)
 		return -1;
@@ -275,6 +285,8 @@ static int dwc_ether_open(struct eth_device *dev)
 	if (ret)
 		return ret;
 
+	dwc_ether_init(dev);
+
 	descs_init(dev);
 
 	/*
@@ -468,7 +480,6 @@ static int dwc_ether_probe(struct device_d *dev)
 	edev->priv = priv;
 
 	edev->parent = dev;
-	edev->init = dwc_ether_init;
 	edev->open = dwc_ether_open;
 	edev->send = dwc_ether_send;
 	edev->recv = dwc_ether_rx;
-- 
1.8.3.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: designware: Don't hang in reset with powered down phy

[Sascha Hauer]

On Tue, Nov 10, 2015 at 10:44:50PM +0000, Trent Piepho wrote:
> The dw MAC requires that all clock domains to be running for it to
> finish a MAC reset.  This include the clock provided by the PHY.
> 
> If the PHY is powered down, bit BMCR_PDOWN set, then it won't be
> generating a clock.  And so the MAC never comes out of reset.  On
> shutdown, Linux will put the PHY in powerdown mode, so it can easily
> be the case that the PHY is powered down on boot.
> 
> See Linux kernel commit 2d871aa07136fe6e576bde63072cf33e2c664e95.
> 
> Currently the MAC reset is done before the phy is probed.  We can't
> power up the phy until it's probed, so the resets must be in the
> opposite order.  The MAC reset is in device init but the PHY probe is
> in device open.  Device init is done first, always, while open is done
> later, and only if the device is used.
> 
> Rather than move the phy probe to init, this moves the MAC reset to
> open.  It seems better to speed up boots that don't use ethernet by
> skipping MAC reset than to slow them down by adding PHY probe.
> 
> Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
> ---
>  drivers/net/designware.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> Instead of phy call in the designware driver, it would be nicer to
> use the phylib layer to abstract it.  Except that the barebox
> phylib has no support for power management and doesn't need it.
> 
> So one either adds general PM support just to clear one bit in one
> register, which seems like a bloated and overly complex solution.
> Or comes up with a generalized method to clear one bit, which seems
> pointless.

I agree, but

> 
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index 8006527..7109e97 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -218,6 +218,16 @@ static int dwc_ether_init(struct eth_device *dev)
>  	struct dw_eth_dev *priv = dev->priv;
>  	struct eth_mac_regs *mac_p = priv->mac_regs_p;
>  	struct eth_dma_regs *dma_p = priv->dma_regs_p;
> +	int bmcr;
> +
> +	/* Before we reset the mac, we must insure the PHY is not powered down
> +	 * as the dw controller needs all clock domains to be running, including
> +	 * the PHY clock, to come out of a mac reset.  */
> +	bmcr = phy_read(dev->phydev, MII_BMCR);
> +	if (bmcr & BMCR_PDOWN) {
> +		bmcr &= ~BMCR_PDOWN;
> +		phy_write(dev->phydev, MII_BMCR, bmcr);
> +	}

Could you factor out this code to a separate function which you name
phy_resume()? This way, should we ever need this functionality again we
hopefully stumble upon this and can revisit this.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2] net: designware: Don't hang in reset with powered down phy

[Trent Piepho]

The dw MAC requires that all clock domains to be running for it to
finish a MAC reset.  This include the clock provided by the PHY.

If the PHY is powered down, bit BMCR_PDOWN set, then it won't be
generating a clock.  And so the MAC never comes out of reset.  On
shutdown, Linux will put the PHY in powerdown mode, so it can easily
be the case that the PHY is powered down on boot.

See Linux kernel commit 2d871aa07136fe6e576bde63072cf33e2c664e95.

Currently the MAC reset is done before the phy is probed.  We can't
power up the phy until it's probed, so the resets must be in the
opposite order.  The MAC reset is in device init but the PHY probe is
in device open.  Device init is done first, always, while open is done
later, and only if the device is used.

Rather than move the phy probe to init, this moves the MAC reset to
open.  It seems better to speed up boots that doesn't use ethernet by
skipping MAC reset than to slow them down by adding PHY probe.

Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
 drivers/net/designware.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 8006527..04b6e7e 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -213,11 +213,34 @@ static void descs_init(struct eth_device *dev)
 	rx_descs_init(dev);
 }
 
+/* Get PHY out of power saving mode.  If this is needed elsewhere then
+ * consider making it part of phy-core and adding a resume method to
+ * the phy device ops.  */
+static int phy_resume(struct phy *phy)
+{
+	int bmcr;
+
+	bmcr = phy_read(phy, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
+	if (bmcr & BMCR_PDOWN) {
+		bmcr &= ~BMCR_PDOWN;
+		return phy_write(phy, MII_BMCR, bmcr);
+	}
+	return 0;
+}
+
 static int dwc_ether_init(struct eth_device *dev)
 {
 	struct dw_eth_dev *priv = dev->priv;
 	struct eth_mac_regs *mac_p = priv->mac_regs_p;
 	struct eth_dma_regs *dma_p = priv->dma_regs_p;
+	int bmcr;
+
+	/* Before we reset the mac, we must insure the PHY is not powered down
+	 * as the dw controller needs all clock domains to be running, including
+	 * the PHY clock, to come out of a mac reset.  */
+	phy_resume(dev->phydev);
 
 	if (mac_reset(dev) < 0)
 		return -1;
@@ -275,6 +298,8 @@ static int dwc_ether_open(struct eth_device *dev)
 	if (ret)
 		return ret;
 
+	dwc_ether_init(dev);
+
 	descs_init(dev);
 
 	/*
@@ -468,7 +493,6 @@ static int dwc_ether_probe(struct device_d *dev)
 	edev->priv = priv;
 
 	edev->parent = dev;
-	edev->init = dwc_ether_init;
 	edev->open = dwc_ether_open;
 	edev->send = dwc_ether_send;
 	edev->recv = dwc_ether_rx;
-- 
1.8.3.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v3] net: designware: Don't hang in reset with powered down phy

[Trent Piepho]

The dw MAC requires that all clock domains to be running for it to
finish a MAC reset.  This include the clock provided by the PHY.

If the PHY is powered down, bit BMCR_PDOWN set, then it won't be
generating a clock.  And so the MAC never comes out of reset.  On
shutdown, Linux will put the PHY in powerdown mode, so it can easily
be the case that the PHY is powered down on boot.

See Linux kernel commit 2d871aa07136fe6e576bde63072cf33e2c664e95.

Currently the MAC reset is done before the phy is probed.  We can't
power up the phy until it's probed, so the resets must be in the
opposite order.  The MAC reset is in device init but the PHY probe is
in device open.  Device init is done first, always, while open is done
later, and only if the device is used.

Rather than move the phy probe to init, this moves the MAC reset to
open.  It seems better to speed up boots that doesn't use ethernet by
skipping MAC reset than to slow them down by adding PHY probe.

Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---

Ignore v2, I missed a bunch of compiler warnings that scrolled off the
screen and barebox doesn't seem to be using -Werror.

 drivers/net/designware.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 8006527..966f64f 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -213,12 +213,34 @@ static void descs_init(struct eth_device *dev)
 	rx_descs_init(dev);
 }
 
+/* Get PHY out of power saving mode.  If this is needed elsewhere then
+ * consider making it part of phy-core and adding a resume method to
+ * the phy device ops.  */
+static int phy_resume(struct phy_device *phydev)
+{
+	int bmcr;
+
+	bmcr = phy_read(phydev, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
+	if (bmcr & BMCR_PDOWN) {
+		bmcr &= ~BMCR_PDOWN;
+		return phy_write(phydev, MII_BMCR, bmcr);
+	}
+	return 0;
+}
+
 static int dwc_ether_init(struct eth_device *dev)
 {
 	struct dw_eth_dev *priv = dev->priv;
 	struct eth_mac_regs *mac_p = priv->mac_regs_p;
 	struct eth_dma_regs *dma_p = priv->dma_regs_p;
 
+	/* Before we reset the mac, we must insure the PHY is not powered down
+	 * as the dw controller needs all clock domains to be running, including
+	 * the PHY clock, to come out of a mac reset.  */
+	phy_resume(dev->phydev);
+
 	if (mac_reset(dev) < 0)
 		return -1;
 
@@ -275,6 +297,8 @@ static int dwc_ether_open(struct eth_device *dev)
 	if (ret)
 		return ret;
 
+	dwc_ether_init(dev);
+
 	descs_init(dev);
 
 	/*
@@ -468,7 +492,6 @@ static int dwc_ether_probe(struct device_d *dev)
 	edev->priv = priv;
 
 	edev->parent = dev;
-	edev->init = dwc_ether_init;
 	edev->open = dwc_ether_open;
 	edev->send = dwc_ether_send;
 	edev->recv = dwc_ether_rx;
-- 
1.8.3.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v3] net: designware: Don't hang in reset with powered down phy

[Sascha Hauer]

On Wed, Nov 11, 2015 at 08:54:01PM +0000, Trent Piepho wrote:
> The dw MAC requires that all clock domains to be running for it to
> finish a MAC reset.  This include the clock provided by the PHY.
> 
> If the PHY is powered down, bit BMCR_PDOWN set, then it won't be
> generating a clock.  And so the MAC never comes out of reset.  On
> shutdown, Linux will put the PHY in powerdown mode, so it can easily
> be the case that the PHY is powered down on boot.
> 
> See Linux kernel commit 2d871aa07136fe6e576bde63072cf33e2c664e95.
> 
> Currently the MAC reset is done before the phy is probed.  We can't
> power up the phy until it's probed, so the resets must be in the
> opposite order.  The MAC reset is in device init but the PHY probe is
> in device open.  Device init is done first, always, while open is done
> later, and only if the device is used.
> 
> Rather than move the phy probe to init, this moves the MAC reset to
> open.  It seems better to speed up boots that doesn't use ethernet by
> skipping MAC reset than to slow them down by adding PHY probe.
> 
> Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
> ---
> 
> Ignore v2, I missed a bunch of compiler warnings that scrolled off the
> screen and barebox doesn't seem to be using -Werror.

Newer compilers tend to introduce more warnings, so using -Werror would
mean that an older barebox will likely not build with recent compiler
versions. I had this problem with other projects sometimes, so I don't
think that using -Werror is a good idea.

Anyway, applied, thanks

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox