[PATCH v1 1/2] net: emac: fix reset timeout with AR8035 phy

[PATCH v1 1/2] net: emac: fix reset timeout with AR8035 phy

[Christian Lamparter]

This patch fixes a problem where the AR8035 PHY can't be
detected on an Cisco Meraki MR24, if the ethernet cable is
not connected on boot.

Russell Senior provided steps to reproduce the issue:
|Disconnect ethernet cable, apply power, wait until device has booted,
|plug in ethernet, check for interfaces, no eth0 is listed.
|
|This appears to be a problem during probing of the AR8035 Phy chip.
|When ethernet has no link, the phy detection fails, and eth0 is not
|created. Plugging ethernet later has no effect, because there is no
|interface as far as the kernel is concerned. The relevant part of
|the boot log looks like this:
|this is the failing case:
|
|[    0.876611] /plb/opb/emac-rgmii@ef601500: input 0 in RGMII mode
|[    0.882532] /plb/opb/ethernet@ef600c00: reset timeout
|[    0.888546] /plb/opb/ethernet@ef600c00: can't find PHY!
|and the succeeding case:
|
|[    0.876672] /plb/opb/emac-rgmii@ef601500: input 0 in RGMII mode
|[    0.883952] eth0: EMAC-0 /plb/opb/ethernet@ef600c00, MAC 00:01:..
|[    0.890822] eth0: found Atheros 8035 Gigabit Ethernet PHY (0x01)

Based on the comment and the commit message of
commit 23fbb5a87c56 ("emac: Fix EMAC soft reset on 460EX/GT").
This is because the AR8035 PHY of the Meraki MR24 does not
provide the TX Clk, if the ethernet cable is not attached.
This causes the reset to timeout and the PHY detection code
in emac_init_phy() is unable to detect the AR8035 PHY.
As a result, the emac driver bails out early and the user
has no ethernet.

In order to stay compatible with existing configurations, the
driver will try the normal reset first and only falls back to
to the internal clock, after the first reset fails. If the
second reset fails as well, it will give up as before.

LEDE-Bug: #687 <https://bugs.lede-project.org/index.php?do=details&task_id=687>

Cc: Chris Blake <chrisrblake93@gmail.com>
Reported-by: Russell Senior <russell@personaltelco.net>
Fixes: 23fbb5a87c56e98 ("emac: Fix EMAC soft reset on 460EX/GT")
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 508923f39ccf..18af1116fa1d 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -343,6 +343,7 @@ static int emac_reset(struct emac_instance *dev)
 {
 	struct emac_regs __iomem *p = dev->emacp;
 	int n = 20;
+	bool try_internal_clock = false;
 
 	DBG(dev, "reset" NL);
 
@@ -355,6 +356,7 @@ static int emac_reset(struct emac_instance *dev)
 	}
 
 #ifdef CONFIG_PPC_DCR_NATIVE
+do_retry:
 	/*
 	 * PPC460EX/GT Embedded Processor Advanced User's Manual
 	 * section 28.10.1 Mode Register 0 (EMACx_MR0) states:
@@ -362,10 +364,19 @@ static int emac_reset(struct emac_instance *dev)
 	 * of the EMAC. If none is present, select the internal clock
 	 * (SDR0_ETH_CFG[EMACx_PHY_CLK] = 1).
 	 * After a soft reset, select the external clock.
+	 *
+	 * The AR8035-A PHY Meraki MR24 does not provide a TX Clk if the
+	 * ethernet cable is not attached. This causes the reset to timeout
+	 * and the PHY detection code in emac_init_phy() is unable to
+	 * communicate and detect the AR8035-A PHY. As a result, the emac
+	 * driver bails out early and the user has no ethernet.
+	 * In order to stay compatible with existing configurations, the
+	 * driver will fall back to and switch to the internal clock, after
+	 * the first reset fails.
 	 */
 	if (emac_has_feature(dev, EMAC_FTR_460EX_PHY_CLK_FIX)) {
-		if (dev->phy_address == 0xffffffff &&
-		    dev->phy_map == 0xffffffff) {
+		if (try_internal_clock || (dev->phy_address == 0xffffffff &&
+					   dev->phy_map == 0xffffffff)) {
 			/* No PHY: select internal loop clock before reset */
 			dcri_clrset(SDR0, SDR0_ETH_CFG,
 				    0, SDR0_ETH_CFG_ECS << dev->cell_index);
@@ -383,8 +394,8 @@ static int emac_reset(struct emac_instance *dev)
 
 #ifdef CONFIG_PPC_DCR_NATIVE
 	if (emac_has_feature(dev, EMAC_FTR_460EX_PHY_CLK_FIX)) {
-		if (dev->phy_address == 0xffffffff &&
-		    dev->phy_map == 0xffffffff) {
+		if (try_internal_clock || (dev->phy_address == 0xffffffff &&
+					   dev->phy_map == 0xffffffff)) {
 			/* No PHY: restore external clock source after reset */
 			dcri_clrset(SDR0, SDR0_ETH_CFG,
 				    SDR0_ETH_CFG_ECS << dev->cell_index, 0);
@@ -396,9 +407,16 @@ static int emac_reset(struct emac_instance *dev)
 		dev->reset_failed = 0;
 		return 0;
 	} else {
-		emac_report_timeout_error(dev, "reset timeout");
-		dev->reset_failed = 1;
-		return -ETIMEDOUT;
+		if (emac_has_feature(dev, EMAC_FTR_460EX_PHY_CLK_FIX) &&
+		    !try_internal_clock) {
+			/* do a retry with the internal clock */
+			try_internal_clock = true;
+			goto do_retry;
+		} else {
+			emac_report_timeout_error(dev, "reset timeout");
+			dev->reset_failed = 1;
+			return -ETIMEDOUT;
+		}
 	}
 }
 
-- 
2.11.0

[PATCH v1 2/2] net: emac: fix and unify emac_mdio functions

[Christian Lamparter]

emac_mdio_read_link() was not copying the requested phy settings
back into the emac driver's own phy api. This has caused a link
speed mismatch issue for the AR8035 as the emac driver kept
trying to connect with 10/100MBps on a 1GBit/s link.

This patch also unifies shared code between emac_setup_aneg()
and emac_mdio_setup_forced(). And furthermore it removes
a chunk of emac_mdio_init_phy(), that was copying the same
data into itself.

Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 41 ++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 18af1116fa1d..8cfb148cfdb0 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -2478,20 +2478,24 @@ static int emac_mii_bus_reset(struct mii_bus *bus)
 	return emac_reset(dev);
 }
 
+static int emac_mdio_phy_start_aneg(struct mii_phy *phy,
+				    struct phy_device *phy_dev)
+{
+	phy_dev->autoneg = phy->autoneg;
+	phy_dev->speed = phy->speed;
+	phy_dev->duplex = phy->duplex;
+	phy_dev->advertising = phy->advertising;
+	return phy_start_aneg(phy_dev);
+}
+
 static int emac_mdio_setup_aneg(struct mii_phy *phy, u32 advertise)
 {
 	struct net_device *ndev = phy->dev;
 	struct emac_instance *dev = netdev_priv(ndev);
 
-	dev->phy.autoneg = AUTONEG_ENABLE;
-	dev->phy.speed = SPEED_1000;
-	dev->phy.duplex = DUPLEX_FULL;
-	dev->phy.advertising = advertise;
 	phy->autoneg = AUTONEG_ENABLE;
-	phy->speed = dev->phy.speed;
-	phy->duplex = dev->phy.duplex;
 	phy->advertising = advertise;
-	return phy_start_aneg(dev->phy_dev);
+	return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
 }
 
 static int emac_mdio_setup_forced(struct mii_phy *phy, int speed, int fd)
@@ -2499,13 +2503,10 @@ static int emac_mdio_setup_forced(struct mii_phy *phy, int speed, int fd)
 	struct net_device *ndev = phy->dev;
 	struct emac_instance *dev = netdev_priv(ndev);
 
-	dev->phy.autoneg =  AUTONEG_DISABLE;
-	dev->phy.speed = speed;
-	dev->phy.duplex = fd;
 	phy->autoneg = AUTONEG_DISABLE;
 	phy->speed = speed;
 	phy->duplex = fd;
-	return phy_start_aneg(dev->phy_dev);
+	return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
 }
 
 static int emac_mdio_poll_link(struct mii_phy *phy)
@@ -2527,16 +2528,17 @@ static int emac_mdio_read_link(struct mii_phy *phy)
 {
 	struct net_device *ndev = phy->dev;
 	struct emac_instance *dev = netdev_priv(ndev);
+	struct phy_device *phy_dev = dev->phy_dev;
 	int res;
 
-	res = phy_read_status(dev->phy_dev);
+	res = phy_read_status(phy_dev);
 	if (res)
 		return res;
 
-	dev->phy.speed = phy->speed;
-	dev->phy.duplex = phy->duplex;
-	dev->phy.pause = phy->pause;
-	dev->phy.asym_pause = phy->asym_pause;
+	phy->speed = phy_dev->speed;
+	phy->duplex = phy_dev->duplex;
+	phy->pause = phy_dev->pause;
+	phy->asym_pause = phy_dev->asym_pause;
 	return 0;
 }
 
@@ -2546,13 +2548,6 @@ static int emac_mdio_init_phy(struct mii_phy *phy)
 	struct emac_instance *dev = netdev_priv(ndev);
 
 	phy_start(dev->phy_dev);
-	dev->phy.autoneg = phy->autoneg;
-	dev->phy.speed = phy->speed;
-	dev->phy.duplex = phy->duplex;
-	dev->phy.advertising = phy->advertising;
-	dev->phy.pause = phy->pause;
-	dev->phy.asym_pause = phy->asym_pause;
-
 	return phy_init_hw(dev->phy_dev);
 }
 
-- 
2.11.0

Re: [PATCH v1 1/2] net: emac: fix reset timeout with AR8035 phy

[Andrew Lunn]

> In order to stay compatible with existing configurations, the
> driver will try the normal reset first and only falls back to
> to the internal clock, after the first reset fails. If the
> second reset fails as well, it will give up as before.

Hi Christian

This gets things probed correctly. But should you swap back to the PHY
clock when the PHY declares the link up? Is there code already to do
this?

	Andrew

Re: [PATCH v1 2/2] net: emac: fix and unify emac_mdio functions

[Andrew Lunn]

On Mon, Jun 05, 2017 at 10:49:40PM +0200, Christian Lamparter wrote:
> emac_mdio_read_link() was not copying the requested phy settings
> back into the emac driver's own phy api. This has caused a link
> speed mismatch issue for the AR8035 as the emac driver kept
> trying to connect with 10/100MBps on a 1GBit/s link.

Hi Christian

In the long run, you might want to remove the emac phy drivers. Linux
has PHYLIB drivers for all but the bcm5248.

       Andrew

Re: [PATCH v1 1/2] net: emac: fix reset timeout with AR8035 phy

[Christian Lamparter]

On Monday, June 5, 2017 11:26:17 PM CEST Andrew Lunn wrote:
> > In order to stay compatible with existing configurations, the
> > driver will try the normal reset first and only falls back to
> > to the internal clock, after the first reset fails. If the
> > second reset fails as well, it will give up as before.
> 
> Hi Christian
> 
> This gets things probed correctly. But should you swap back to the PHY
> clock when the PHY declares the link up? Is there code already to do
> this?
> 
> 	Andrew
> 
Oh, sorry. I omitted this from the commit message. But the proposed 
emac_reset() code  switched to the internal clock only after the first attempt
has failed AND only for the duration of the reset.

If the reset succeeds or the reset times out, the clock is always switched 
back to the external clock.

Thanks,
Christian

Re: [PATCH v1 1/2] net: emac: fix reset timeout with AR8035 phy

[Andrew Lunn]

On Mon, Jun 05, 2017 at 11:44:46PM +0200, Christian Lamparter wrote:
> On Monday, June 5, 2017 11:26:17 PM CEST Andrew Lunn wrote:
> > > In order to stay compatible with existing configurations, the
> > > driver will try the normal reset first and only falls back to
> > > to the internal clock, after the first reset fails. If the
> > > second reset fails as well, it will give up as before.
> > 
> > Hi Christian
> > 
> > This gets things probed correctly. But should you swap back to the PHY
> > clock when the PHY declares the link up? Is there code already to do
> > this?
> > 
> > 	Andrew
> > 
> Oh, sorry. I omitted this from the commit message. But the proposed 
> emac_reset() code  switched to the internal clock only after the first attempt
> has failed AND only for the duration of the reset.
> 
> If the reset succeeds or the reset times out, the clock is always switched 
> back to the external clock.

Thanks for the clarification. Maybe add it to the commit message.

       Andrew

Re: [PATCH v1 1/2] net: emac: fix reset timeout with AR8035 phy

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 5 Jun 2017 23:48:57 +0200

> On Mon, Jun 05, 2017 at 11:44:46PM +0200, Christian Lamparter wrote:
>> On Monday, June 5, 2017 11:26:17 PM CEST Andrew Lunn wrote:
>> > > In order to stay compatible with existing configurations, the
>> > > driver will try the normal reset first and only falls back to
>> > > to the internal clock, after the first reset fails. If the
>> > > second reset fails as well, it will give up as before.
>> > 
>> > Hi Christian
>> > 
>> > This gets things probed correctly. But should you swap back to the PHY
>> > clock when the PHY declares the link up? Is there code already to do
>> > this?
>> > 
>> > 	Andrew
>> > 
>> Oh, sorry. I omitted this from the commit message. But the proposed 
>> emac_reset() code  switched to the internal clock only after the first attempt
>> has failed AND only for the duration of the reset.
>> 
>> If the reset succeeds or the reset times out, the clock is always switched 
>> back to the external clock.
> 
> Thanks for the clarification. Maybe add it to the commit message.

Indeed, please do.

 

Re: [PATCH v1 2/2] net: emac: fix and unify emac_mdio functions

[Christian Lamparter]

On Monday, June 5, 2017 11:43:33 PM CEST Andrew Lunn wrote:
> On Mon, Jun 05, 2017 at 10:49:40PM +0200, Christian Lamparter wrote:
> > emac_mdio_read_link() was not copying the requested phy settings
> > back into the emac driver's own phy api. This has caused a link
> > speed mismatch issue for the AR8035 as the emac driver kept
> > trying to connect with 10/100MBps on a 1GBit/s link.

> In the long run, you might want to remove the emac phy drivers. 
> Linux has PHYLIB drivers for all but the bcm5248.

Hello Andrew

Back in February I added PHYLIB support to emac.
<https://www.spinics.net/lists/netdev/msg421734.html> ;)

I could add a separate patch that adds a oneliner message like:
    pr_info("EMAC supports PHYLIB. Please convert your device to it.\n");
at the right place (emac_mii_phy_probe) to let people know about it.

Is that Ok? If not, please tell me what the appropriate deprecation 
notice should look like.

About the MR24:
I do have a patchset to convert the MR24 to use PHYLIB's at803x as well.
<https://github.com/chunkeey/apm82181-lede/commit/82c50b7f4fca68ce905d4a7a3559700635bf227f>
But because of AT8035 "special tx/rx delay" requirements, this
will need a patched at803x.c driver as well.

Regards,
Christian

Re: [PATCH v1 1/2] net: emac: fix reset timeout with AR8035 phy

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

Hi Christian,

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.12-rc4 next-20170605]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christian-Lamparter/net-emac-fix-reset-timeout-with-AR8035-phy/20170606-113953
config: powerpc-allmodconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   drivers/net//ethernet/ibm/emac/core.c: In function 'emac_reset':
>> drivers/net//ethernet/ibm/emac/core.c:414:4: error: label 'do_retry' used but not defined
       goto do_retry;
       ^~~~

vim +/do_retry +414 drivers/net//ethernet/ibm/emac/core.c

   408			return 0;
   409		} else {
   410			if (emac_has_feature(dev, EMAC_FTR_460EX_PHY_CLK_FIX) &&
   411			    !try_internal_clock) {
   412				/* do a retry with the internal clock */
   413				try_internal_clock = true;
 > 414				goto do_retry;
   415			} else {
   416				emac_report_timeout_error(dev, "reset timeout");
   417				dev->reset_failed = 1;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53803 bytes --]