Need help with mdiobus_register and phy

I need help with a program I've discovered with my driver (ethernet/qualcomm/emac/) that occurs in the following process:

1. Load the driver
1a. Driver calls mdiobus_register (see emac_phy_config), which calls mdiobus_scan(), which calls get_phy_device()
1b. get_phy_device() performs MDIO bus operations, which means the bus and the external phy need to be active.
2. Bring up the interface
3. Bring down the interface. This calls phy_disconnect() which calls phy_detach() which calls phy_suspend(). This puts the external phy to sleep.
4. Unload the driver
5. Re-load the driver.
5a. Driver calls mdiobus_register again, but this time the call to get_phy_device() fails because the external phy is asleep and does not respond to MDIO transactions.

I don't know what my driver is doing wrong. I'm concerned that mdiobus_register() is causing MDIO transactions to occur before the rest of the EMAC has a chance to initialize. I've examined many other drivers, and I think I'm initializing things in the proper order.

The call to mdiobus_register() occurs before I know what phydev is. Since I need the phydev in order to talk to the phy driver, I don't see how I can "wake up" the external phy before calling mdiobus_register().

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Hi Timur

Normally, a sleeping PHY does respond to MDIO. Otherwise, how do you wake it?

So i assume this phy has some other means to wake it. What is this means?

Andrew

I'm guessing that someone has to call phy_resume() before/during the call to mdiobus_register, but I don't see how that's possible.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Please can you tell us what PHY which is, and how it is put to sleep and woken up.

If the PHY cannot be woken up using MDIO, then maybe you need to look at the mdio bus reset call?

Andrew

It's the at803x driver.

http://lxr.free-electrons.com/source/drivers/net/phy/at803x.c

It goes to sleep in its at803x_suspend() function, which is called by phy_suspend().

There is a corresponding at803x_resume(). The problem is that this is not called by mdiobus_register(). I'm guessing that mdiobus_register() assumes that the phy is awake.

It seems like a catch-22. mdiobus_register() assumes that the phy is awake, but you can't wake up the phy until after you call mdiobus_register().

I looked at that, but it won't work because there is no phydev when the reset function is called:

http://lxr.free-electrons.com/source/drivers/net/phy/mdio_bus.c#L328

It's the same catch-22.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

The at803x_resume() just does normal MDIO transactions. Which suggests the MDIO bus side of the device is still away. Or at least, the MII_BMCR register is.

So are you seeing that the reads to MII_PHYSID1 and MII_PHYSID2 return 0xffff, when called from get_phy_id()?

Andrew

Yes.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Have you tried using the ethernet-phy-id device tree property? It looks like that will allow you to skip get_phy_device and just create the phy device. You can then bring the phy out of sleep in the probe function?

Andrew

The problem I'm experiencing is with ACPI, so I can't use any of the fancy of_ apis like of_get_phy_id(). But I'll look into it.

Is it possible that at803x_suspend() is too aggressive? That's it's effectively disabling the phy? While the phy is suspended, should it still respond to MII_PHYSID1 and MII_PHYSID2 requests?

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

That is a basic assumption of the code. If you cannot read the IDs how are you supposed to know what device it is, and what quirks you need to work around its broken features...

Does the datasheet say anything about this?

I would say for this device, suspend() is too aggressive.

Andrew

I'll have to find the datasheet. Let me do some research and get back to you. Thanks for your help so far.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

This change in my driver makes the problem go away (I'm not sure if it's a "fix"):

@@ -992,7 +992,7 @@ int emac_mac_up(struct emac_adapter *adpt)

        emac_mac_rx_descs_refill(adpt, &adpt->rx_q);

        ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,

- PHY_INTERFACE_MODE_SGMII);
+ PHY_INTERFACE_MODE_NA);

With the interface not set as SGMII, the following code in at803x_suspend() is not executed:

/* also power-down SGMII interface */
ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG); phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL); phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN); phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);

I don't see any other driver issue BMCR_PDOWN in their functions. I added some printks for the PHYSID1 and PHYSID2 registers before and after BMCR_PDOWN:

at803x_suspend:235 MII_PHYSID1=004d MII_PHYSID2=d074 at803x_suspend:242 MII_PHYSID1=ffff MII_PHYSID2=ffff

So after calling BMCR_PDOWN, the PHYSID1 and PHYSID2 registers are no longer readable. Is that expected?

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

You are making two changes here. Is it the SGMII power down which is causing the id registers to return 0xffff, or the BMCR_PDOWN.

The generic suspend code sets the PDOWN bit, so it is assuming the PHY will respond afterwards.

Andrew

Fair enough. I don't think it's specified anywhere what firmware is supposed to do.

What about specifying PHY_INTERFACE_MODE_NA on ACPI systems, but using the phy-mode property on device tree systems? That doesn't sound like a great idea.

Ok, it took me a while to figure this out. The driver does three writes:

phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL); phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN); phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);

The first clears the AT803X_BT_BX_REG_SEL bit. According to the datasheet, that changes the register set from copper to fiber mode. BMCR_PDOWN in fiber mode shuts off the SerDes bus. That's not true in copper mode.

Then after shutting down SerDes, it switches back to copper mode.

I also noticed the at803x_suspend already sends BMCR_PDOWN in copper mode earlier in the function.

So the question remains: should drivers shut down the SerDes bus when they suspend? In a sense, I'm wondering if we should revert

at803x: fix suspend/resume for SGMII link

However, the changelog for that patch makes it sound like it's a necessary fix.

So I'm torn. With the SerDes connection disabled, the driver no longer responds to ID register reads. That seems like something that would be broken on device tree as well, but I don't understand why no one noticed it before.

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

I would be curious to know about that as well.

After reading the spec again, it does not appear to me that a PHY with PDOWN set is guaranteed or even required to respond to other register reads such as MII_PHYID1/2, in which case we may have to implement a MDIO bus reset routine which clears PDOWN for all PHYs that we detect(ed), or as Andrew suggested, utilize the matching by compatible string with the PHY OUI in it.

Florian

The 8031 does respond normally when PDOWN is set. However, the ID registers are not available when the SerDes bus is also powered down. I'll call this PDOWN+. This is a special power-down sequence that the at803x driver does on suspend. See my other email for details.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

So we appear to have two ways to go:

1) Take the SerDes power down out of the suspend code for the at803x.

2) Assume MII_PHYID1/2 registers are not guaranteed to be available when the PHY is powered down. So get_phy_id should first read MII_BMCR. If it gets 0xffff, assume there is no PHY there. If the PDOWN bit is set, power up the PHY. Then reading the ID registers.

Andrew

Before we take approach #1, I'd like to hear from the developer of that patch, Zefir. According to him, that patch is necessary to fix a bug. I don't know if that bug exists only on his system, though.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Hi,

the problem we observed was that in SGMII mode after a suspend/resume cycle the PHY link states get out of sync (e.g. gianfar reports link up, at803x down) and power cycling the SerDes was a reliable way to get them re-synchronized again.

This obviously worked for some time after March 2016 and must have stopped only recently (maybe additional PHY suspends were added).

Anyway, since the SGMII reset is required, instead of reverting the patch in full I suggest to move the SGMII power down from at803x_suspend() and do a SerDes power cycle in at803x_resume(). Could you please test if the patch below fixes the problem?

Thanks,
Zefir
---

Subject: [PATCH] at803x: don't power down SerDes on suspend

Powering down SerDes renders registers inaccessible and with that re-initializing a suspended PHY fails.

Instead of powering down SerDes at suspend(), leave it as is and power-cycle it on resume().

---
 drivers/net/phy/at803x.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

index f279a89..282ffbb 100644
@@ -225,16 +225,6 @@ static int at803x_suspend(struct phy_device *phydev)
 
 	phy_write(phydev, MII_BMCR, value);
 
-	if (phydev->interface != PHY_INTERFACE_MODE_SGMII)
-		goto done;
-
-	/* also power-down SGMII interface */
-	ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
-	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
-	phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
-	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);
-
-done:
 	mutex_unlock(&phydev->lock);
 
 	return 0;

@@ -254,9 +244,11 @@ static int at803x_resume(struct phy_device *phydev)
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII)
 		goto done;
 
-	/* also power-up SGMII interface */
+	/* reset SGMII interface for re-synchronization */
 	ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
 	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
+	phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
+
 	value = phy_read(phydev, MII_BMCR) & ~(BMCR_PDOWN | BMCR_ISOLATE);
 	phy_write(phydev, MII_BMCR, value);
 	phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);

2.7.4

I have never seen the original problem that you noticed. When I use the generic phy driver instead of the at803x driver, everything works great for me. Perhaps the problem that you noticed only occurs with the Gianfar NIC?

Anyway, I tested the change you suggested, and it fixes the problem for me. I moved the power-down code to before the power-up code. But like I said, since I never experienced the original problem, I don't know if that works for you.

I suggest you make the changes in the driver yourself and test it, and then I will test whether that patch also works for me.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

You mean it works for you in SGMII mode without glitches? Then it might in fact be an unfortunate rare or unique combination that we chose.

I'd need some time to get that tested, and since our device might be the only one requiring the fix, I am fine with keeping the patch private. Feel free to revert it, if required, I'll provide a revised one.

Thanks
Zefir

That is correct.

That is what I expect.

I'm in no hurry to get this fixed. Do you need a couple weeks to run some tests? It would be good to know for sure that your fix is needed only on your platform.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Ok then, if you can wait some days, I'll prepare and provide you a more detailed failure report to allow you testing if the issue happens with other NICs.

That sounds great.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

I am now able again to reproduce the issue reliably, let's see if it is helpful enough to check if this a problem unique to our setup.

As described earlier, the HW is as follows:
(copper)--[at8031]--(SGMII)--[eTSEC/mpc85xx]

We are using OpenWRT/LEDE as SW platform, therefore I can't do the tests on the latest kernel but provide a reference to inspect the problem. The kernel used is 4.4.21 with the relevant drivers (ath803x and gianfar_driver) being either clean or patched with OWRT/LEDE modifications (issue happens in either case).

The process for detecting the failure is rather trivial: continuously put the Ethernet interface down and up and try to get some data through until it fails. The script I used to run on the device (over serial) is:
#----------------------------------------------------------
HOST=192.168.1.100
TIMEOUT=20

while true; do
ifconfig eth0 down
ifconfig eth0 up

ping $HOST -c 1 -w $TIMEOUT || {
echo "Ping failed after $TIMEOUT seconds"
exit 1
}
done
#----------------------------------------------------------
In my setup, it takes less than 5 minutes for the script to exit. Once running into this state, the relevant registers read from at803x are as follows:
* copper status (0x01): 0x796d
* fiber status (0x01): 0x6149
* copper phy specific status (0x11): 0xbc10
* fiber phy specific status (0x11): 0x8100
* copper/fiber status (0x1b): 0x020e

In essence
* the copper link is up with autonegotiation completed (switch/peer link indication is also up), but
* the SGMII link is down
(bits LINK and MR_AN_COMPLETE in PHY specific status fiber cleared)
* although SGMII is synchronized (SYNC_STATUS bit in same register set)

The SGMII link does not recover from this state thereafter, the only reliable way to get it up again I found is to do a power down/up cycle at the fiber/SGMII side, which is what the patch I contributed does.

Now the interesting (and new for me) part is: if I remove the at803x driver from the system and use the generic phy instead, the problem does not happen (or at least not while running for one full day). I don't see any significant differences between what at803x does above genphy (gpio based HW reset is not run for 8031 in at803x_link_change_notify()). Using the genphy is not an option for us, since we have the PHY also attached to fiber and need dedicated handling.

Hope this helps for the beginning, let me know if you need more information.

Cheers,
Zefir

Update: the failure also happened with genphy after running for another day.

I haven't had a chance to try to reproduce this failure, but to me this sounds like you have a problem with gianfar or your board, and not the at803x driver.

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.