Andrew Lunn wrote:
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.
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.
>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.
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.
