Re: [PATCH net v2] net: phy: realtek: Reset after clock enable

[Russell King (Oracle)]

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@xxxxxxxxxxxxxxx
> Fixes: 7300c9b574cc ("net: phy: realtek: Add optional external PHY clock")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> ---
>  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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!