Message-ID: <a54ae202-fed9-1fff-5e71-b7a93f09c411@gmail.com>
Date:   Fri, 15 Jan 2021 18:36:45 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     "Badel, Laurent" <LaurentBadel@...on.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "m.felsch@...gutronix.de" <m.felsch@...gutronix.de>,
        "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: Subject: [PATCH v3 net-next 0/4] net:phy: Fix LAN87xx external
 reset

On 15.01.2021 18:19, 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. As a result, the PHYs 
> occasionnally (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.
> 
> 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 LAN8710/20):
> 
>  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
>  the GPIOD_OUT_LOW which sets the initial value to 0. The reset is
>  de-asserted by phy_device_register() shortly after.  After that, the PHY
>  runs without clock until the FEC is opened, which causes issues.
> 
>  2. At first opening of the FEC:
>  RST    111111111111111111111111111100000111111
>  CLK    000000000011111111111111111111111111111
>  After REF_CLK is enabled, phy_reset_after_clk_enable() causes a
>  short reset pulse. Reset is needed here because the PHY was running 
>  without clock before. 
>    
>  3. At closing of the FEC driver:
>  RST    111110000000000000000000000000000000000                 
>  CLK    111111111111000000000000000000000000000
>  FEC first disconnects the PHY, which asserts the reset, and then 
>  disables the clock.
>    
>  4. At subsequent openings of the FEC:
>  RST    000000000000000011111111111110000011111                  
>  CLK    000000000011111111111111111111111111111
>  FEC first enables the clock, then connects to the PHY which releases 
>  the reset. Here the second reset pulse (phy_reset_after_clk_enable()) 
>  is unnecessary, because REF_CLK is already running when the reset is 
>  first deasserted. 
>   
> This illustrates that the only place where the extra reset pulse is 
> actually needed, is at the first opening of the FEC driver, and the reason
> it is needed in the first place, is because the PHY has been running 
> without clock after it was probed. 
> 
> 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). Therefore, instead 
> of relying on extra calls to phy_reset_after_clk_enable(), the issue 
> addressed by commit [1] can be simply fixed by keeping the PHY in reset 
> when exiting from phy_probe(). In this way the PHY will always be in reset
> or power-down whenever the REF_CLK is turned off.
> 
> This should not cause issues, since as per the PAL documentation any 
> driver that has business with the PHY should at least call phy_attach(), 
> which will deassert the reset in due time. Therefore this fix probably 
> works equally well for any PHY, but out of caution the patch uses the 
> existing PHY_RST_AFTER_CLK_EN driver flag (which it renames), to implement
> the fix only for LAN8710/20/40 PHYs.
> 
> Previous versions:
> This is the 3rd version of the series;  below is a short description of
> the previous versions.
> 
> v1: 
> The solution in [1] has the unfortunate side-effect of breaking the PHY 
> interrupt system due to the hardware reset erasing the interrupt mask of
> the PHY. Patch series v1 suggested performing the extra reset before the 
> PHY is configured, by moving the call to phy_reset_after_clk_enable() from
> the FEC into phy_init_hw() instead. The patch was re-examinated after 
> finding an issue during resume from suspend, where the PHY also seemed to
> require a hardware reset to work properly. 
> Further investigation showed that this is in fact due to another
> peculiarity of the LAN87xx, which also erase their interrupt mask upon 
> software reset (which is done by phy_init_hw() on resuming from 
> suspend-to-ram), and is thus a separate issue that will be addressed in 
> a separate patch. 
> 
> v2:
> During this time the kernel had moved on and 2 new commits rendered the v1
> fix unnecessary: 
>   [3] allows the extra PHY reset to still be performed from the FEC, but 
>   before the interrupt mask is configured, thereby fixing the above 
>   interrupt mask issue.
>   [4] allows LAN87xx to take control of the REF_CLK directly, preventing
>   the FEC from disabling it and thus circumventing the entire REF_CLK 
>   issue.
> Patch v2 proposed to fix 2 potential issues with the solution from [4], 
> namely that (i) failing to set the PHY "clocks" DT property would silently 
> break the system (because FEC succeeds in disabling the REF_CLK, yet the 
> extra reset has been removed), and (ii) keeping the REF_CLK enabled
> defeated the power-saving purpose of commit [2].
> 
> The present patch fixes (i), and leaves it up to the user to use the 
> power-friendly clock management of [2] (leave the DT clocks property 
> unset), or keep the REF_CLK always enabled (set the clocks property). 
> It also simplifies the code by removing all calls to 
> phy_reset_after_clk_enable() and related code, and the function
> phy_reset_after_clk_enable() altogether.  
> 
> Tests: against net-next (5.11-rc3) with LAN8720 and LAN8742 and iMX283 
> SoC. Unfortunately unable to test LAN8740 which has a different form 
> factor.
> 
> References:
> [1] commit 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable()
>     support")
> [2] commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>     power")
> [3] commit 64a632da538a ("net: fec: Fix phy_device lookup for 
>     phy_reset_after_clk_enable()")
> [4] commit bedd8d78aba3 ("net: phy: smsc: LAN8710/20: add phy refclk in
>     support")
> 
> Laurent Badel (4):
>   Remove PHY reset in fec_main.c
>   Remove phy_reset_after_clk_enable()
>   Rename PHY_RST_AFTER_CLK_EN to PHY_RST_AFTER_PROBE
>   Add PHY reset after probe for PHYs with PHY_RST_AFTER_PROBE flag 
> 
>  drivers/net/ethernet/freescale/fec_main.c | 40 -----------------------
>  drivers/net/phy/phy_device.c              | 26 +--------------
>  drivers/net/phy/smsc.c                    |  4 +--
>  include/linux/phy.h                       |  3 +-
>  4 files changed, 4 insertions(+), 69 deletions(-)
> 

W/o knowing the exact issue in detail: For the sake of bisectability,
shouldn't patches 4 and a modified patch 3 be first? After patches 1 and 2
we may be in trouble, right?

And it seems you composed the mails manually:
- subject is prefixed with "Subject:"
- patch subjects in the cover letter don't match the ones of the
  respective patches

For the patch prefixes: Please add a space between net: and phy:

Powered by blists - more mailing lists