On 10.03.2021 21:41, Florian Fainelli wrote:
> B50212E PHYs have been observed to get into an incorrect state with the
> visible effect of having both activity and link LEDs flashing
> alternatively instead of being turned off as intended when
> genphy_suspend() was issued. The BCM54810 is a similar design and
> equally suffers from that issue.
>
> The datasheet is not particularly clear whether a read/modify/write
> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
> utilized to enter the power down mode. When this was done the PHYs were
> always measured to have power levels that match the expectations and
> LEDs powered off.
>
> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
> ---
> drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index b8eb736fb456..b33ffd44f799 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
> return 0;
> }
>
> +static int bcm54xx_suspend(struct phy_device *phydev)
> +{
> + /* We cannot perform a read/modify/write like what genphy_suspend()
> + * does because depending on the time we can observe the PHY having
> + * both of its LEDs flashing indicating that it is in an incorrect
> + * state and not powered down as expected.
> + *
> + * There is not a clear indication in the datasheet whether a
> + * read/modify/write would be acceptable, but a blind write to the
> + * register has been proven to be functional unlike the
> + * Read/Modify/Write.
> + */
> + return phy_write(phydev, MII_BMCR, BMCR_PDOWN);
This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
forced mode. So you have to rely on somebody calling genphy_config_aneg()
to sync the register bits with the values cached in struct phy_device
on resume. Typically the phylib state machine takes care, but do we have
to consider use cases where this is not the case?
Heiner
> +}
> +
> static int bcm54xx_resume(struct phy_device *phydev)
> {
> int ret;
> @@ -778,7 +793,7 @@ static struct phy_driver broadcom_drivers[] = {
> .config_aneg = bcm5481_config_aneg,
> .config_intr = bcm_phy_config_intr,
> .handle_interrupt = bcm_phy_handle_interrupt,
> - .suspend = genphy_suspend,
> + .suspend = bcm54xx_suspend,
> .resume = bcm54xx_resume,
> }, {
> .phy_id = PHY_ID_BCM54811,
>
