On 7/25/2025 7:21 PM, Sebastian Reichel wrote:
Hi,
On Thu, Jul 24, 2025 at 05:51:58PM +0100, Russell King (Oracle) wrote:
On Thu, Jul 24, 2025 at 04:39:42PM +0200, Sebastian Reichel wrote:
On Radxa ROCK 4D boards we are seeing some issues with PHY detection and stability (e.g. link loss or not capable of transceiving packages) after new board revisions switched from a dedicated crystal to providing the 25 MHz PHY input clock from the SoC instead.
This board is using a RTL8211F PHY, which is connected to an always-on regulator. Unfortunately the datasheet does not explicitly mention the power-up sequence regarding the clock, but it seems to assume that the clock is always-on (i.e. dedicated crystal).
By doing an explicit reset after enabling the clock, the issue on the boards could no longer be observed.
Note, that the RK3576 SoC used by the ROCK 4D board does not yet support system level PM, so the resume path has not been tested.
Cc: stable@vger.kernel.org Fixes: 7300c9b574cc ("net: phy: realtek: Add optional external PHY clock") Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.com
Changes in v2:
- Switch to PHY_RST_AFTER_CLK_EN + phy_reset_after_clk_enable(); the API is so far only used by the freescale/NXP MAC driver and does not seem to be written for usage from within the PHY driver, but I also don't see a good reason not to use it.
- Also reset after re-enabling the clock in rtl821x_resume()
- Link to v1: https://lore.kernel.org/r/20250704-phy-realtek-clock-fix-v1-1-63b33d204537@k...
drivers/net/phy/realtek/realtek_main.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c index c3dcb62574303374666b46a454cd4e10de455d24..cf128af0ec0c778262d70d6dc4524d06cbfccf1f 100644 --- a/drivers/net/phy/realtek/realtek_main.c +++ b/drivers/net/phy/realtek/realtek_main.c @@ -230,6 +230,7 @@ static int rtl821x_probe(struct phy_device *phydev) if (IS_ERR(priv->clk)) return dev_err_probe(dev, PTR_ERR(priv->clk), "failed to get phy clock\n");
- phy_reset_after_clk_enable(phydev);
Should this depend on priv->clk existing?
ret = phy_read_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR1); if (ret < 0) @@ -627,8 +628,10 @@ static int rtl821x_resume(struct phy_device *phydev) struct rtl821x_priv *priv = phydev->priv; int ret;
- if (!phydev->wol_enabled)
- if (!phydev->wol_enabled) { clk_prepare_enable(priv->clk);
phy_reset_after_clk_enable(phydev);Should this depend on priv->clk existing?
I'm thinking about platforms such as Jetson Xavier NX, which I believe uses a crystal, and doesn't appear to suffer from any problems.
Doing the extra reset should not hurt, but I can add it to speed up PHY initialization on systems with an always-on clock source. I will send a PATCHv3.
Assuming the PHY DT node is tagged with "always-on" then one might reasonably expect it to be used for Wake-on-LAN and that it should be able to register itself as the wake-up source. If we pulse the reset we might lose the details of the wake-up event within the PHY. So yes, being able to deal with that and doing the reset conditionally might be preferable.