[PATCH V2 0/2] net: phy: dp83822: Add optional external PHY clock

[PATCH V2 0/2] net: phy: dp83822: Add optional external PHY clock

[Stefan Wahren]

This small series implement support for external PHY clock for the
dp83822 driver.

Changes in V2:
- Make readability changes a separate patch as suggested by Andrew
- Make clk a local pointer as suggested by Andrew

Stefan Wahren (2):
  net: phy: dp83822: Improve readability in dp8382x_probe
  net: phy: dp83822: Add optional external PHY clock

 drivers/net/phy/dp83822.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH V2 1/2] net: phy: dp83822: Improve readability in dp8382x_probe

[Stefan Wahren]

Introduce a local pointer for device so devm_kzalloc() fit into
a single line. Also this makes following changes easier to read.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/phy/dp83822.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index c012dfab3171..d8c5b5cd1bc0 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -984,10 +984,10 @@ static int dp83822_attach_mdi_port(struct phy_device *phydev,
 
 static int dp8382x_probe(struct phy_device *phydev)
 {
+	struct device *dev = &phydev->mdio.dev;
 	struct dp83822_private *dp83822;
 
-	dp83822 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83822),
-			       GFP_KERNEL);
+	dp83822 = devm_kzalloc(dev, sizeof(*dp83822), GFP_KERNEL);
 	if (!dp83822)
 		return -ENOMEM;
 
-- 
2.43.0

[PATCH V2 2/2] net: phy: dp83822: Add optional external PHY clock

[Stefan Wahren]

In some cases, the PHY can use an external ref clock source instead of a
crystal.

Add an optional clock in the PHY node to make sure that the clock source
is enabled, if specified, before probing.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/phy/dp83822.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index d8c5b5cd1bc0..6fc86be9d593 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2017 Texas Instruments Inc.
  */
 
+#include <linux/clk.h>
 #include <linux/ethtool.h>
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
@@ -986,11 +987,18 @@ static int dp8382x_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct dp83822_private *dp83822;
+	struct clk *clk;
 
 	dp83822 = devm_kzalloc(dev, sizeof(*dp83822), GFP_KERNEL);
 	if (!dp83822)
 		return -ENOMEM;
 
+	clk = devm_clk_get_optional_enabled(dev, NULL);
+	if (IS_ERR(clk)) {
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Failed to request ref clock\n");
+	}
+
 	dp83822->tx_amplitude_100base_tx_index = -1;
 	dp83822->mac_termination_index = -1;
 	phydev->priv = dp83822;
-- 
2.43.0

Re: [PATCH V2 1/2] net: phy: dp83822: Improve readability in dp8382x_probe

[Andrew Lunn]

On Thu, May 28, 2026 at 08:46:41PM +0200, Stefan Wahren wrote:
> Introduce a local pointer for device so devm_kzalloc() fit into
> a single line. Also this makes following changes easier to read.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

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

    Andrew

Re: [PATCH V2 2/2] net: phy: dp83822: Add optional external PHY clock

[Andrew Lunn]

On Thu, May 28, 2026 at 08:46:42PM +0200, Stefan Wahren wrote:
> In some cases, the PHY can use an external ref clock source instead of a
> crystal.
> 
> Add an optional clock in the PHY node to make sure that the clock source
> is enabled, if specified, before probing.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

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

    Andrew

Re: [PATCH V2 2/2] net: phy: dp83822: Add optional external PHY clock

[Jakub Kicinski]

On Thu, 28 May 2026 20:46:42 +0200 Stefan Wahren wrote:
> In some cases, the PHY can use an external ref clock source instead of a
> crystal.
> 
> Add an optional clock in the PHY node to make sure that the clock source
> is enabled, if specified, before probing.


> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> index d8c5b5cd1bc0..6fc86be9d593 100644
> --- a/drivers/net/phy/dp83822.c
> +++ b/drivers/net/phy/dp83822.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2017 Texas Instruments Inc.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/ethtool.h>
>  #include <linux/etherdevice.h>
>  #include <linux/kernel.h>
> @@ -986,11 +987,18 @@ static int dp8382x_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
>  	struct dp83822_private *dp83822;
> +	struct clk *clk;
>  
>  	dp83822 = devm_kzalloc(dev, sizeof(*dp83822), GFP_KERNEL);
>  	if (!dp83822)
>  		return -ENOMEM;
>  
> +	clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(clk)) {
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "Failed to request ref clock\n");
> +	}

nit: unnecessary parenthesis

The AI says:

  Does this initialization sequence violate the DP83822 power-on
  requirements? The PHY framework deasserts the hardware reset line before
  invoking the probe callback. By enabling the external clock here, 
  the clock starts after the hardware reset is already deasserted.

  The datasheet requires the reset signal to be asserted for at least 
  50 ms after power and clocks are stable. Without performing a subsequent
  hardware reset here, could the PHY be left in an undefined state or
  lead to initialization failures?

Did it really read the datasheet or is this a hallucination?

FWIW it also says:

  Should this clock pointer be saved in the driver's private data
  structure (struct dp83822_private) instead of a local variable?

  Without storing it, the dp83822_suspend callback cannot disable the clock
  when Wake-on-LAN is disabled, which could prevent the clock provider from
  entering low-power states.

Re: [PATCH V2 2/2] net: phy: dp83822: Add optional external PHY clock

[Stefan Wahren]

Hi Jakub,

Am 02.06.26 um 05:01 schrieb Jakub Kicinski:
> On Thu, 28 May 2026 20:46:42 +0200 Stefan Wahren wrote:
>> In some cases, the PHY can use an external ref clock source instead of a
>> crystal.
>>
>> Add an optional clock in the PHY node to make sure that the clock source
>> is enabled, if specified, before probing.
>
>> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
>> index d8c5b5cd1bc0..6fc86be9d593 100644
>> --- a/drivers/net/phy/dp83822.c
>> +++ b/drivers/net/phy/dp83822.c
>> @@ -4,6 +4,7 @@
>>    * Copyright (C) 2017 Texas Instruments Inc.
>>    */
>>   
>> +#include <linux/clk.h>
>>   #include <linux/ethtool.h>
>>   #include <linux/etherdevice.h>
>>   #include <linux/kernel.h>
>> @@ -986,11 +987,18 @@ static int dp8382x_probe(struct phy_device *phydev)
>>   {
>>   	struct device *dev = &phydev->mdio.dev;
>>   	struct dp83822_private *dp83822;
>> +	struct clk *clk;
>>   
>>   	dp83822 = devm_kzalloc(dev, sizeof(*dp83822), GFP_KERNEL);
>>   	if (!dp83822)
>>   		return -ENOMEM;
>>   
>> +	clk = devm_clk_get_optional_enabled(dev, NULL);
>> +	if (IS_ERR(clk)) {
>> +		return dev_err_probe(dev, PTR_ERR(clk),
>> +				     "Failed to request ref clock\n");
>> +	}
> nit: unnecessary parenthesis
I didn't provide the whole story behind this patch. I have a custom 
i.MX93 board, which have 2 different Ethernet MAC (fec + stmmac). Each 
of the MACs are connected to one TI DP83825 PHY. Those PHYs share the 
same MDIO bus and the same input reference clock (RMII slave) from the 
i.MX93 processor. The MDIO is handled by the fec driver. Since the fec 
is only aware of a single reference clock (tied to the MAC), it's 
possible that during probe the reference clock is disabled because the 
refcount is zero. So the idea is to avoid glitches by adding another 
clock user (tied to the second PHY).

Here is the device tree extract:
&fec {
     pinctrl-names = "default";
     pinctrl-0 = <&pinctrl_fec>;
     phy-mode = "rmii";
     phy-handle = <&ethphy1>;
     assigned-clocks = <&clk IMX93_CLK_ENET_TIMER1>,
               <&clk IMX93_CLK_ENET_REF>,
               <&clk IMX93_CLK_ENET_REF_PHY>;
     assigned-clock-parents = <&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>,
                  <&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>,
                  <&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>;
     assigned-clock-rates = <100000000>, <50000000>, <50000000>;
     status = "okay";

     mdio: mdio {
         clock-frequency = <5000000>;
         #address-cells = <1>;
         #size-cells = <0>;

         ethphy1: ethernet-phy@1 {
             compatible = "ethernet-phy-ieee802.3-c22";
             reg = <1>;
             reset-gpios = <&gpio4 23 GPIO_ACTIVE_HIGH>;
             reset-assert-us = <30>;
         };

         ethphy2: ethernet-phy@2 {
             reg = <2>;
             compatible = "ethernet-phy-id2000.a140", 
"ethernet-phy-ieee802.3-c22";
             clocks = <&clk IMX93_CLK_ENET_REF_PHY>;
             reset-gpios = <&gpio4 13 GPIO_ACTIVE_HIGH>;
             reset-assert-us = <30>;
             reset-deassert-us = <50000>;
         };
     };
};

&eqos {
     phy-mode = "rmii";
     phy-handle = <&ethphy2>;
     assigned-clocks = <&clk IMX93_CLK_ENET_TIMER2>,
               <&clk IMX93_CLK_ENET>;
     assigned-clock-parents = <&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>,
                  <&clk IMX93_CLK_SYS_PLL_PFD1_DIV2>;
     assigned-clock-rates = <100000000>, <50000000>;

     status = "okay";
};
> The AI says:
>
>    Does this initialization sequence violate the DP83822 power-on
>    requirements? The PHY framework deasserts the hardware reset line before
>    invoking the probe callback. By enabling the external clock here,
>    the clock starts after the hardware reset is already deasserted.
The behavior description is correct, but i'm not sure this is really a 
violation. I assume that we should also declare PHY_RST_AFTER_CLK_EN in 
the flags in order to get the PHY reset after clock has been enabled right?
>    The datasheet requires the reset signal to be asserted for at least
>    50 ms after power and clocks are stable. Without performing a subsequent
>    hardware reset here, could the PHY be left in an undefined state or
>    lead to initialization failures?
>
> Did it really read the datasheet or is this a hallucination?
I don't know where these 50 ms comes from, because I found under "7.7 
Timing Requirements, Power-Up With Unstable XI Clock" a value of 10 us.
>
> FWIW it also says:
>
>    Should this clock pointer be saved in the driver's private data
>    structure (struct dp83822_private) instead of a local variable?
>
>    Without storing it, the dp83822_suspend callback cannot disable the clock
>    when Wake-on-LAN is disabled, which could prevent the clock provider from
>    entering low-power states.
I wasn't aware that additional power saving during runtime PM must be 
part of this change. So this is required?

Re: [PATCH V2 2/2] net: phy: dp83822: Add optional external PHY clock

[Andrew Lunn]

> > Did it really read the datasheet or is this a hallucination?
> I don't know where these 50 ms comes from, because I found under "7.7 Timing
> Requirements, Power-Up With Unstable XI Clock" a value of 10 us.

I guess it is an hallucination. There has been discussion about a
number of PHYs and how long a sleep is needed after releasing the
reset. It has maybe generalised that discussion and decided all PHYs
need 50ms?

> > FWIW it also says:
> > 
> >    Should this clock pointer be saved in the driver's private data
> >    structure (struct dp83822_private) instead of a local variable?
> > 
> >    Without storing it, the dp83822_suspend callback cannot disable the clock
> >    when Wake-on-LAN is disabled, which could prevent the clock provider from
> >    entering low-power states.
> I wasn't aware that additional power saving during runtime PM must be part
> of this change. So this is required?

The AI often points out issues outside the scope of a patchset. In
this case, i would say it is something which could be added later. The
power savings of a single clock is not that great, when just powering
down the PHY can save 1 watt.

     Andrew

Re: [PATCH V2 0/2] net: phy: dp83822: Add optional external PHY clock

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 28 May 2026 20:46:40 +0200 you wrote:
> This small series implement support for external PHY clock for the
> dp83822 driver.
> 
> Changes in V2:
> - Make readability changes a separate patch as suggested by Andrew
> - Make clk a local pointer as suggested by Andrew
> 
> [...]

Here is the summary with links:
  - [V2,1/2] net: phy: dp83822: Improve readability in dp8382x_probe
    https://git.kernel.org/netdev/net-next/c/d790b7064387
  - [V2,2/2] net: phy: dp83822: Add optional external PHY clock
    https://git.kernel.org/netdev/net-next/c/8a9f9bd2d070

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html