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@kernel.org
---
 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);
 
 	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);
+	}
 
 	ret = genphy_resume(phydev);
 	if (ret < 0)
@@ -1617,7 +1620,7 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= rtl821x_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
-		.flags		= PHY_ALWAYS_CALL_SUSPEND,
+		.flags		= PHY_ALWAYS_CALL_SUSPEND | PHY_RST_AFTER_CLK_EN,
 		.led_hw_is_supported = rtl8211x_led_hw_is_supported,
 		.led_hw_control_get = rtl8211f_led_hw_control_get,
 		.led_hw_control_set = rtl8211f_led_hw_control_set,

---
base-commit: 89be9a83ccf1f88522317ce02f854f30d6115c41
change-id: 20250704-phy-realtek-clock-fix-6cd393e8cb2a

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

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@kernel.org
> ---
>  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!

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@kernel.org
> > ---
> >  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.

Greetings,

-- Sebastian

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@kernel.org
>>> ---
>>>   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.
-- 
Florian

On 7/24/25 07:39, 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

Hoping this does not regress other Ethernet controllers and boards in 
the process...
-- 
Florian

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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew