Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

[Heiner Kallweit <hkallweit1@gmail.com>]

On 10.03.2021 22:15, Florian Fainelli wrote:
> On 3/10/21 1:07 PM, Heiner Kallweit wrote:
>> 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.fainelli@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?
> 
> Good point, how about if we had forced the link before suspending, does
> PHYLIB take care of re-applying the same parameters? It arguably should
> do that in all cases given that power to the PHY can be cut depending on
> the suspend mode.
> 

When entering power-down mode the link is lost and we go to HALTED state.
On resume, phy_start() sets UP state and state machine calls
phy_start_aneg(), which takes care of syncing the BMCR forced mode bits.
A potential issue arises if we have a driver that doesn't use the
phylib state machine and prefers to do it on its own.
IIRC I once stumbled across this when I also relied on the phylib state
machine running in a change.
I'm not sure whether we can run into a problem, but it's worth spending
a thought before somebody complains after applying the change.