Message-ID: <MW4PR17MB4243C724D3F7EB57EF9D964FDFA30@MW4PR17MB4243.namprd17.prod.outlook.com>
Date:   Tue, 19 Jan 2021 13:25:01 +0000
From:   "Badel, Laurent" <LaurentBadel@...on.com>
To:     Marco Felsch <m.felsch@...gutronix.de>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "fugang.duan@....com" <fugang.duan@....com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "richard.leitner@...data.com" <richard.leitner@...data.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "marex@...x.de" <marex@...x.de>
Subject: 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:
> > 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.

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.

> 
> > 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.

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.

> > 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?

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. 
 
> >  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.

Yes right, I was assuming clocks is not set in reference to the original
solution in [1] but that may not have been obvious. 
 
> > 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.

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 

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

Powered by blists - more mailing lists