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.

Reply via email to