Re: [PATCH v4 net-next 0/5] net: phy: Fix SMSC LAN87xx external reset

Hi Laurent,

thanks for your patches :) Can you check your setup since we get 6
individual emails: 'git send-email --thread ...' ;)

On 21-01-18 16:57, Badel, Laurent wrote:

Description:
External PHY reset from the FEC driver was introduced in commit [1] to 
mitigate an issue with iMX SoCs and LAN87xx PHYs. The issue occurs 
because the FEC driver turns off the reference clock for power saving 
reasons [2], which doesn't work out well with LAN87xx PHYs which require 
a running REF_CLK during the power-up sequence.

Not only during the power-up sequence. The complete phy internal state
machine (the hardware state machine) gets confused if the clock is
turned off randomly.

As a result, the PHYs 
occasionally (and unpredictably) fail to establish a stable link and 
require a hardware reset to work reliably.

As previously noted [3], the solution in [1] integrates poorly with the
PHY abstraction layer, and it also performs many unnecessary resets. This
patch series suggests a simpler solution to this problem, namely to hold
the PHY in reset during the time between the PHY driver probe and the first
opening of the FEC driver.

Holding the Phy within reset during the FEC is in reset seems wrong to
me because: The clock can be supplied by an external crystal/oszi. This
would add unnecessary delays. Also this is again a FEC/SMSC combination
fix again. The phy has the same problem on other hosts if they are the
clock provider and toggling the ref-clk.

To illustrate why this is sufficient, below is a representation of the PHY
RST and REF_CLK status at relevant time points (note that RST signal is
active-low for LAN87xx):

 1. During system boot when the PHY is probed:
 RST    111111111111111111111000001111111111111
 CLK    000011111111111111111111111111111000000
 REF_CLK is enabled during fec_probe(), and there is a short reset pulse
 due to mdiobus_register_gpiod() which calls gpiod_get_optional() with

There is also a deprecated "phy-reset-gpios" did you test this as well?

 the GPIOD_OUT_LOW flag, which sets the initial value to 0. The reset is
 deasserted by phy_device_register() shortly after.  After that, the PHY
 runs without clock until the FEC is opened, which causes the unstable 
 link issue.

Nope that's not true, you can specify the clock within the device-tree
so the fec-ref-clk isn't disabled anymore.

 2. At first opening of the FEC:

...

Extensive testing with LAN8720 confirmed that the REF_CLK can be disabled
without problems as long as the PHY is either in reset or in power-down 
mode (which is relevant for suspend-to-ram as well).

You can't disable the clock. What you listing here means that the smsc
phy needs to be re-initialized after such an clock loss. If we can
disbale the clock randomly we wouldn't need to re-initialize the phy
again.

Regards,
  Marco