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

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

Posted by Sebastian Reichel 7 months, 1 week ago

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

Posted by Andrew Lunn 7 months, 1 week ago

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

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

Posted by Heiner Kallweit 7 months, 1 week ago

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

Posted by Sebastian Reichel 7 months, 1 week ago

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,
>

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

Posted by Detlev Casanova 7 months ago

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,