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

Thank you very much to everyone who took the time to read and comment 
on the patch. 

Given that Marco doesn't seem to agree with the main idea of the patch, 
I don't think it is worth sending the corrected version so I will leave
it as is. 

My replies to Marco's comments are below.

Best regards,

Laurent

Hi Laurent,

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

Hi Marco, thank you for you time reviewing and sorry about the threading
of the emails, I will see to it next time.

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

quoted

Description:
External PHY reset from the FEC driver was introduced in commit [1]

to

quoted

mitigate an issue with iMX SoCs and LAN87xx PHYs. The issue occurs
because the FEC driver turns off the reference clock for power

saving

quoted

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.

Yes, you are right. LAN87xx don't like the REF_CLK being turned off while 
they are running, which is understandable, so this is why you should 
either avoid turning it off, or if you do, make sure you don't do it
while the PHY is running.

quoted

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,

quoted

namely to hold the PHY in reset during the time between the PHY

driver

quoted

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.

It's true that the PHY will be kept in reset until the interface is up, 
but then I don't really see the point of having the PHY running if the 
MAC is not listening anyway. When the interface is taken down, the PHY
layer asserts the reset, so this is basically the only place where 
the interface is down but the PHY is not held in reset, so in my view
it made sense to do this.
But fair enough, keeping the clock on does the job and that is what 
matters in the end. This is at the expense of power management though, 
for example the clock stays on during suspend-to-ram, though this
perhaps not the end of the world indeed.

quoted

To illustrate why this is sufficient, below is a representation of

the

quoted

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?

I gave it a try, but since FEC only uses this to reset the PHY at probe 
time, there is not much to expect. As can be expected, if we set 
phy-reset-gpios in the fec node, but not reset-gpios in the phy node, 
the PHY layer is unable to reset the PHY, so disabling the REF_CLK 
would mean trouble indeed. 
 

quoted

 the GPIOD_OUT_LOW flag, which sets the initial value to 0. The

reset

quoted

is  deasserted by phy_device_register() shortly after.  After that,
the PHY  runs without clock until the FEC is opened, which causes

the

quoted

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.

Yes right, I was assuming clocks is not set in reference to the original
solution in [1] but that may not have been obvious. 
 

quoted

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

quoted

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.

Fair enough, if you correctly configure your DT with the clocks property, 
the clock will stay on.  

Best regards,

Laurent


-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland 

-----------------------------