Re: [PATCH net 0/4] Restore and fix PHY reset for SMSC LAN8720

[Marco Felsch <m.felsch@pengutronix.de>]

Hi,

thanks for your patches :)

On 20-10-27 23:25, Badel, Laurent wrote:
> Subject: [PATCH net 0/4] Restore and fix PHY reset for SMSC LAN8720
> 
> Description:
> A recent patchset [1] added support in the SMSC PHY driver for managing
> the ref clock and therefore removed the PHY_RST_AFTER_CLK_EN flag for the
> LAN8720 chip. The ref clock is passed to the SMSC driver through a new
> property "clocks" in the device tree.
> 
> There appears to be two potential caveats:
> (i) Building kernel 5.9 without updating the DT with the "clocks"
> property for SMSC PHY, would break systems previously relying on the PHY
> reset workaround (SMSC driver cannot grab the ref clock, so it is still
> managed by FEC, but the PHY is not reset because PHY_RST_AFTER_CLK_EN is
> not set). This may lead to occasional loss of ethernet connectivity in
> these systems, that is difficult to debug.

IMHO reyling on PHY_RST_AFTER_CLK_EN was broken since the day of adding
this feature because:

1st) Each host driver needs to call the phy-reset logic. So this isn't a
     fix for all hosts using a LAN8720 phy.
2st) It interacts realy bad with the phy state machine. Only the state
     machine should be able to do this.

Why can't you add the clock?

> (ii) This defeats the purpose of a previous commit [2] that disabled the
> ref clock for power saving reasons. If a ref clock for the PHY is
> specified in DT, the SMSC driver will keep it always on (confirmed with 
> scope).

NACK, the clock provider can be any clock. This has nothing to do with
the FEC clocks. The FEC _can_ be used as clock provider.

> While this removes the need for additional PHY resets (only a 
> single reset is needed after power up), this prevents the FEC from saving
> power by disabling the refclk. Since there may be use cases where one is
> interested in saving power,

You can't just turn off the clock for the LAN8720 because of the phy
internal state machine. The state machine gets confused if the clock is
turned off/on randomly.

> keep this option available when no ref clock
> is specified for the PHY, by fixing issues with the PHY reset.

IMHO pulling the reset line everytime has a few disadvantages:
 - You need to ensure that the strapping pins are correct and
 - You need to ensure that the reset logic including the reset delays
   are keeped.

> Main changes proposed to address this:
> (a) Restore PHY_RST_AFTER_CLK_EN for LAN8720, but explicitly clear it if
> the SMSC driver succeeds in retrieving the ref clock.

IMHO NACK since this was the wrong approach.

> (b) Fix phy_reset_after_clk_enable() to work in interrupt mode, by
> re-configuring the PHY registers after reset.
> 
> Tests: against net tree 5.9, including allyes/no/modconfig. 10 pieces of
> an iMX28-EVK-based board were tested, 3 of which were found to exhibit
> issues when the "clocks" property was left unset. Issues were fixed by
> the present patchset.

All iMX machines are now DT-based why can't you just add the correct
clock provider?

Regards,
  Marco