Re: Need help with mdiobus_register and phy
From: Andrew Lunn <andrew@lunn.ch>
Date: 2016-10-14 17:25:16
On Fri, Oct 14, 2016 at 11:57:29AM -0500, Timur Tabi wrote:
quoted hunk ↗ jump to hunk
Andrew Lunn wrote:quoted
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.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);
It is normal to get the phy-mode from device tree. I've no idea what ACPI is supposed to do. Setting it to PHY_INTERFACE_MODE_NA means you assume the boot loader has correctly setup the hardware. You ACPI firmware might of done this, but there is no guarantee a device tree base bootloader has. So i would prefer not changing this.
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?
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