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

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

[Sebastian Reichel]

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.

Cc: stable@vger.kernel.org
Fixes: 7300c9b574cc ("net: phy: realtek: Add optional external PHY clock")
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/net/phy/realtek/realtek_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index c3dcb62574303374666b46a454cd4e10de455d24..3a783f0c3b4f2a4f6aa63a16ad309e3471b0932a 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -231,6 +231,10 @@ static int rtl821x_probe(struct phy_device *phydev)
 		return dev_err_probe(dev, PTR_ERR(priv->clk),
 				     "failed to get phy clock\n");
 
+	/* enabling the clock might produce glitches, so hard-reset the PHY */
+	phy_device_reset(phydev, 1);
+	phy_device_reset(phydev, 0);
+
 	ret = phy_read_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR1);
 	if (ret < 0)
 		return ret;

---
base-commit: 4c06e63b92038fadb566b652ec3ec04e228931e8
change-id: 20250704-phy-realtek-clock-fix-6cd393e8cb2a

Best regards,
-- 
Sebastian Reichel <sre@kernel.org>

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

[Heiner Kallweit]

On 04.07.2025 19:48, 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.
> 
Is the SoC clock always on after boot? Or may it be disabled e.g.
during system suspend? Then you would have to do the PHY reset also
on resume from suspend.

> Cc: stable@vger.kernel.org
> Fixes: 7300c9b574cc ("net: phy: realtek: Add optional external PHY clock")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/net/phy/realtek/realtek_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index c3dcb62574303374666b46a454cd4e10de455d24..3a783f0c3b4f2a4f6aa63a16ad309e3471b0932a 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -231,6 +231,10 @@ static int rtl821x_probe(struct phy_device *phydev)
>  		return dev_err_probe(dev, PTR_ERR(priv->clk),
>  				     "failed to get phy clock\n");
>  
> +	/* enabling the clock might produce glitches, so hard-reset the PHY */
> +	phy_device_reset(phydev, 1);
> +	phy_device_reset(phydev, 0);
> +
>  	ret = phy_read_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR1);
>  	if (ret < 0)
>  		return ret;
> 
> ---
> base-commit: 4c06e63b92038fadb566b652ec3ec04e228931e8
> change-id: 20250704-phy-realtek-clock-fix-6cd393e8cb2a
> 
> Best regards,

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

Hi,

On Fri, Jul 04, 2025 at 10:18:29PM +0200, Heiner Kallweit wrote:
> On 04.07.2025 19:48, 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.
> > 
> Is the SoC clock always on after boot? Or may it be disabled e.g.
> during system suspend? Then you would have to do the PHY reset also
> on resume from suspend.

Upstream kernel does not yet support suspend/resume on Rockchip RK3576
(the SoC used by the ROCK 4D) and I missed, that the clock is disabled
in the PHY's suspend routine.

Detlev: You added the initial clock support to the driver. If I add
the reset in the resume routine, can you do regression testing on
the board you originally added the clock handling for?

Greetings,

-- Sebastian

> > Cc: stable@vger.kernel.org
> > Fixes: 7300c9b574cc ("net: phy: realtek: Add optional external PHY clock")
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  drivers/net/phy/realtek/realtek_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> > index c3dcb62574303374666b46a454cd4e10de455d24..3a783f0c3b4f2a4f6aa63a16ad309e3471b0932a 100644
> > --- a/drivers/net/phy/realtek/realtek_main.c
> > +++ b/drivers/net/phy/realtek/realtek_main.c
> > @@ -231,6 +231,10 @@ static int rtl821x_probe(struct phy_device *phydev)
> >  		return dev_err_probe(dev, PTR_ERR(priv->clk),
> >  				     "failed to get phy clock\n");
> >  
> > +	/* enabling the clock might produce glitches, so hard-reset the PHY */
> > +	phy_device_reset(phydev, 1);
> > +	phy_device_reset(phydev, 0);
> > +
> >  	ret = phy_read_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR1);
> >  	if (ret < 0)
> >  		return ret;
> > 
> > ---
> > base-commit: 4c06e63b92038fadb566b652ec3ec04e228931e8
> > change-id: 20250704-phy-realtek-clock-fix-6cd393e8cb2a
> > 
> > Best regards,
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Detlev Casanova]

Hi Sebastian,

On Friday, 4 July 2025 16:35:00 EDT Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Jul 04, 2025 at 10:18:29PM +0200, Heiner Kallweit wrote:
> > On 04.07.2025 19:48, 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.
> > 
> > Is the SoC clock always on after boot? Or may it be disabled e.g.
> > during system suspend? Then you would have to do the PHY reset also
> > on resume from suspend.
> 
> Upstream kernel does not yet support suspend/resume on Rockchip RK3576
> (the SoC used by the ROCK 4D) and I missed, that the clock is disabled
> in the PHY's suspend routine.
> 
> Detlev: You added the initial clock support to the driver. If I add
> the reset in the resume routine, can you do regression testing on
> the board you originally added the clock handling for?

No regression for me on the pre-release board. I can't really give you a 
Tested-by as this is a fix for a board I don't have.

Regards,
Detlev

> > > Cc: stable@vger.kernel.org
> > > Fixes: 7300c9b574cc ("net: phy: realtek: Add optional external PHY
> > > clock")
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > > 
> > >  drivers/net/phy/realtek/realtek_main.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/realtek/realtek_main.c
> > > b/drivers/net/phy/realtek/realtek_main.c index
> > > c3dcb62574303374666b46a454cd4e10de455d24..3a783f0c3b4f2a4f6aa63a16ad309
> > > e3471b0932a 100644 --- a/drivers/net/phy/realtek/realtek_main.c
> > > +++ b/drivers/net/phy/realtek/realtek_main.c
> > > @@ -231,6 +231,10 @@ static int rtl821x_probe(struct phy_device *phydev)
> > > 
> > >  		return dev_err_probe(dev, PTR_ERR(priv->clk),
> > >  		
> > >  				     "failed to get phy clock\n");
> > > 
> > > +	/* enabling the clock might produce glitches, so hard-reset the PHY 
*/
> > > +	phy_device_reset(phydev, 1);
> > > +	phy_device_reset(phydev, 0);
> > > +
> > > 
> > >  	ret = phy_read_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR1);
> > >  	if (ret < 0)
> > >  	
> > >  		return ret;
> > > 
> > > ---
> > > base-commit: 4c06e63b92038fadb566b652ec3ec04e228931e8
> > > change-id: 20250704-phy-realtek-clock-fix-6cd393e8cb2a
> > > 
> > > Best regards,

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

[Andrew Lunn]

On Fri, Jul 04, 2025 at 07:48:44PM +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.

I don't know if it helps in this situation, but look at
PHY_RST_AFTER_CLK_EN.

If it does not actually help, it might be worth mentioning it in the
commit message to stop reviewers saying you should see if it is
useful.

	Andrew