Re: [PATCH net-next] net: phy: perform a PHY reset on resume

Hello,

On Sat, Dec 11, 2021 at 02:15:54PM +0000, Russell King (Oracle) wrote:

I don't particularly like this - this impacts everyone who is using
phylib at this point, whereas no reset was happening if the reset was
already deasserted here.

I understand your concern, but I do not believe that this can create any
issue. The code should be able to handle the situation in which the PHY
is getting out of reset at that time.

In the opening remarks to this series, it is stated:

  If a hardware-design is able to control power to the Ethernet PHY and
  relying on software to do a reset, the PHY does no longer work after
  resuming from suspend, given the PHY does need a hardware-reset.

This requirement is conditional on the hardware design, it isn't a
universal requirement and won't apply everywhere. I think it needs to
be described in firmware that this action is required. That said...

Please check the datasheet on the PHY regarding application of power and
reset. You may find that the PHY datasheet requires the reset to be held
active from power up until the clock input is stable - this could mean
you need some other arrangement to assert the PHY reset signal after
re-applying power sooner than would happen by the proposed point in the
kernel.

I checked before sending this patch, the phy is a KSZ9131 [1] and
according to the power sequence timing  the reset should be toggled after
the power-up. No requirement on the clock or other signals.

The HW design is quite simple, we have a regulator controlling the PHY
power, and a GPIO controlling the reset. On suspend we remove the power
(FEC driver), on resume after enabling the power the PHY require a
reset, but nobody is doing it.

The issue here is that the phy regulator is handled by the FEC driver,
while the RESET_N GPIO can be controlled by both the FEC driver or the
phylib.
The initial proposal was to handle this into the FEC driver, but it was
not considered a good idea, therefore this new proposal.

One more comment about that, I do not believe that asserting the reset
in the suspend path is a good idea, in the general situation in which
the PHY is powered in suspend the power-consumption is likely to be
higher if the device is in reset compared to software power-down using
the BMCR register.

universal requirement and won't apply everywhere. I think it needs to
be described in firmware that this action is required. That said...

Are you thinking at a DTS property? Not sure to understand how do you
envision this. At the moment the regulator is not handled by the phylib,
and the property should be something like reset-on-resume, I guess ...

Francesco

[1] https://ww1.microchip.com/downloads/en/DeviceDoc/00002841C.pdf