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

[Florian Fainelli <florian.fainelli@broadcom.com>]

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