[PATCH v2 0/4] IPQ5018: Add and enable GEPHY RX and TX clocks

[PATCH v2 0/4] IPQ5018: Add and enable GEPHY RX and TX clocks

[George Moussalem via B4 Relay]

Greetings,

This patch series addresses a missing hardware description issue for the
Qualcomm IPQ5018 Internal Ethernet PHY, where the data paths fail to
function correctly unless their dedicated RX and TX clocks are
explicitly enabled.

Further testing revealed that leaving these clocks unmanaged by the
kernel, they were inadvertently left enabled by the bootloader / QSDK
platform, which masked the issue. Testing a fresh network configuration
path exposed that the data link fails to work without explicit software
gating.

To correctly introduce the required multi-clock properties, the IPQ5018
binding definition must first be split away from the shared
qca,ar803x.yaml schema. This isolation is required because ar803x
references the generic ethernet-phy.yaml, which enforces a strict
single-clock limit constraint. 

- Patch 1: Moves the clocks property and its restriction out of the
	   generic ethernet-phy.yaml schema to individual bindings files
	   that need it to allow for PHYs that require multiple clocks.
- Patch 2: Add clocks property to qca,ar803x.yaml for the IPQ5018 PHY.
- Patch 3: Appends the missing RX/TX clock definitions into the IPQ5018
           device tree before driver modification to avoid the driver
	   failing to probe.
- Patch 4: Updates the Qualcomm AT803x PHY driver framework to acquire,
	   enable, and gate these clocks upon link state changes for
	   runtime power optimization.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
Changes in v2:
- Added patch 1 to move the clocks property and its restriction out of
  the generic ethernet-phy.yaml schema to individual bindings files that
  need it to allow for PHYs that require multiple clocks.
- Reverted splitting out IPQ5018 from the shared qca,ar803x.yaml schema
  and simply added the clocks and clock-names properties to the
  definition of the IPQ5018 PHY.
- Corrected / updated commit title of patch 4 (qca,at803x -> at803x)
- Link to v1: https://lore.kernel.org/r/20260601-ipq5018-gephy-clocks-v1-0-2df8287712c3@outlook.com

---
George Moussalem (4):
      dt-bindings: net: ethernet-phy: move clocks property to invidivual PHY bindings
      dt-bindings: net: qca,ar803x: Add clocks property for IPQ5018 PHY
      arm64: qcom: ipq5018: Add GEPHY RX and TX clocks
      net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

 .../devicetree/bindings/net/ethernet-phy.yaml      |  6 ------
 .../devicetree/bindings/net/qca,ar803x.yaml        | 14 +++++++++++++
 .../devicetree/bindings/net/realtek,rtl82xx.yaml   |  6 ++++++
 arch/arm64/boot/dts/qcom/ipq5018.dtsi              |  3 +++
 drivers/net/phy/qcom/at803x.c                      | 23 ++++++++++++++++++++++
 5 files changed, 46 insertions(+), 6 deletions(-)
---
base-commit: 7da7f07112610a520567421dd2ffcb51beaefbcc
change-id: 20260601-ipq5018-gephy-clocks-e8a2440178a0

Best regards,
-- 
George Moussalem <george.moussalem@outlook.com>

[PATCH v2 1/4] dt-bindings: net: ethernet-phy: move clocks property to invidivual PHY bindings

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

Move the clock property and restriction from the ethernet-phy.yaml file
to the individual PHY binding files. This allows each PHY to manage its
own clock requirements.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
Commit 350b7a258f20 introduced the clocks property with a restriction to
maximum 1 to the main ethernet-phy.yaml binding for Realtek to add an
optional external clock source. This is restrictive to all PHY bindings,
as some PHYs may require more than 1 clock such as the IPQ5018 PHY which
requires 2 clocks (for RX and TX).

There are three other PHY drivers that require clock management:
- Micrel: requires 1 optional clock and the micrel.yaml file already
accomodates for the clock property.
- SMSC: requires an optional clock and the legacy bindings file
(smsc-lan87xx.txt) already accomodates for the clock property.
- BCM7xxx: requires an optional clock. I could not find a bindings file
for this PHY family.
---
 Documentation/devicetree/bindings/net/ethernet-phy.yaml    | 6 ------
 Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 21a1a63506f0..709ea976ef79 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -105,12 +105,6 @@ properties:
       1BR-10 names. The PHY must be configured to operate in BroadR-Reach mode
       by software.
 
-  clocks:
-    maxItems: 1
-    description:
-      External clock connected to the PHY. If not specified it is assumed
-      that the PHY uses a fixed crystal or an internal oscillator.
-
   enet-phy-lane-swap:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
index 45033c31a2d5..8a26f6941dc4 100644
--- a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
+++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
@@ -38,6 +38,12 @@ properties:
       - ethernet-phy-id001c.cad0
       - ethernet-phy-id001c.cb00
 
+  clocks:
+    maxItems: 1
+    description:
+      External clock connected to the PHY. If not specified it is assumed
+      that the PHY uses a fixed crystal or an internal oscillator.
+
   leds: true
 
   realtek,aldps-enable:

-- 
2.53.0

[PATCH v2 2/4] dt-bindings: net: qca,ar803x: Add clocks property for IPQ5018 PHY

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

Further testing revealed that the RX and TX clocks of the IPQ5018 PHY
need to be explicitly enabled. As such, add the clocks property to the
schema.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 Documentation/devicetree/bindings/net/qca,ar803x.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
index 7ae5110e7aa2..07615640b0ed 100644
--- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
+++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
@@ -28,6 +28,16 @@ allOf:
         reg:
           const: 7  # This PHY is always at MDIO address 7 in the IPQ5018 SoC
 
+        clocks:
+          items:
+            - description: RX clock
+            - description: TX clock
+
+        clock-names:
+          items:
+            - const: rx
+            - const: tx
+
         resets:
           items:
             - description:
@@ -162,6 +172,7 @@ examples:
         };
     };
   - |
+    #include <dt-bindings/clock/qcom,gcc-ipq5018.h>
     #include <dt-bindings/reset/qcom,gcc-ipq5018.h>
 
     mdio {
@@ -172,6 +183,9 @@ examples:
             compatible = "ethernet-phy-id004d.d0c0";
             reg = <7>;
 
+            clocks = <&gcc GCC_GEPHY_RX_CLK>,
+                     <&gcc GCC_GEPHY_TX_CLK>;
+            clock-names = "rx", "tx";
             resets = <&gcc GCC_GEPHY_MISC_ARES>;
         };
     };

-- 
2.53.0

[PATCH v2 3/4] arm64: qcom: ipq5018: Add GEPHY RX and TX clocks

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

Add RX and TX clocks for the IPQ5018 GEPHY to enable the datapath.

Fixes: f5f2b835e316 ("arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus")
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq5018.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index 6f8004a22a1f..60c27a6f2b10 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -229,6 +229,9 @@ ge_phy: ethernet-phy@7 {
 				compatible = "ethernet-phy-id004d.d0c0";
 				reg = <7>;
 
+				clocks = <&gcc GCC_GEPHY_RX_CLK>,
+					 <&gcc GCC_GEPHY_TX_CLK>;
+				clock-names = "rx", "tx";
 				resets = <&gcc GCC_GEPHY_MISC_ARES>;
 			};
 		};

-- 
2.53.0

[PATCH v2 4/4] net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

[George Moussalem via B4 Relay]

From: George Moussalem <george.moussalem@outlook.com>

Acquire and enable the RX and TX clocks for the IPQ5018 PHY. These
clocks are required for the PHY's datapath to function correctly.
In addition, gate the clocks upon link state changes for improved power
management.

Fixes: d46502279a11 ("net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support")
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 drivers/net/phy/qcom/at803x.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index 63726cf98cd4..b7361a14220d 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -19,6 +19,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/of.h>
 #include <linux/phylink.h>
+#include <linux/clk.h>
 #include <linux/reset.h>
 #include <linux/phy_port.h>
 #include <dt-bindings/net/qca-ar803x.h>
@@ -176,6 +177,8 @@ struct at803x_context {
 };
 
 struct ipq5018_priv {
+	struct clk *rx_clk;
+	struct clk *tx_clk;
 	struct reset_control *rst;
 	bool set_short_cable_dac;
 };
@@ -1062,6 +1065,16 @@ static int ipq5018_config_init(struct phy_device *phydev)
 
 static void ipq5018_link_change_notify(struct phy_device *phydev)
 {
+	struct ipq5018_priv *priv = phydev->priv;
+
+	if (phydev->link) {
+		clk_enable(priv->rx_clk);
+		clk_enable(priv->tx_clk);
+	} else {
+		clk_disable(priv->rx_clk);
+		clk_disable(priv->tx_clk);
+	}
+
 	/*
 	 * Reset the FIFO buffer upon link disconnects to clear any residual data
 	 * which may cause issues with the FIFO which it cannot recover from.
@@ -1084,6 +1097,16 @@ static int ipq5018_probe(struct phy_device *phydev)
 	priv->set_short_cable_dac = of_property_read_bool(dev->of_node,
 							  "qcom,dac-preset-short-cable");
 
+	priv->rx_clk = devm_clk_get_enabled(dev, "rx");
+	if (IS_ERR(priv->rx_clk))
+		return dev_err_probe(dev, PTR_ERR(priv->rx_clk),
+				     "failed to get and enable RX clock\n");
+
+	priv->tx_clk = devm_clk_get_enabled(dev, "tx");
+	if (IS_ERR(priv->tx_clk))
+		return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
+				     "failed to get and enable TX clock\n");
+
 	priv->rst = devm_reset_control_array_get_exclusive(dev);
 	if (IS_ERR(priv->rst))
 		return dev_err_probe(dev, PTR_ERR(priv->rst),

-- 
2.53.0

Re: [PATCH v2 2/4] dt-bindings: net: qca,ar803x: Add clocks property for IPQ5018 PHY

[Conor Dooley]

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

Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

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

Re: [PATCH v2 1/4] dt-bindings: net: ethernet-phy: move clocks property to invidivual PHY bindings

[Conor Dooley]

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

On Tue, Jun 02, 2026 at 10:50:37AM +0400, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> Move the clock property and restriction from the ethernet-phy.yaml file
> to the individual PHY binding files. This allows each PHY to manage its
> own clock requirements.
> 
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---
> Commit 350b7a258f20 introduced the clocks property with a restriction to
> maximum 1 to the main ethernet-phy.yaml binding for Realtek to add an
> optional external clock source. This is restrictive to all PHY bindings,
> as some PHYs may require more than 1 clock such as the IPQ5018 PHY which
> requires 2 clocks (for RX and TX).
> 
> There are three other PHY drivers that require clock management:
> - Micrel: requires 1 optional clock and the micrel.yaml file already
> accomodates for the clock property.
> - SMSC: requires an optional clock and the legacy bindings file
> (smsc-lan87xx.txt) already accomodates for the clock property.
> - BCM7xxx: requires an optional clock. I could not find a bindings file
> for this PHY family.

Have you done a large-scale dtbs_check run with this patch applied and
checked that it does not ban having a clock for phys with no dedicated
bindings?
I feel like weakening the limit of a single clock is probably more
accurate than outright banning clocks for ethernet phys without a
dedicated binding?

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml    | 6 ------
>  Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml | 6 ++++++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 21a1a63506f0..709ea976ef79 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -105,12 +105,6 @@ properties:
>        1BR-10 names. The PHY must be configured to operate in BroadR-Reach mode
>        by software.
>  
> -  clocks:
> -    maxItems: 1
> -    description:
> -      External clock connected to the PHY. If not specified it is assumed
> -      that the PHY uses a fixed crystal or an internal oscillator.
> -
>    enet-phy-lane-swap:
>      $ref: /schemas/types.yaml#/definitions/flag
>      description:
> diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
> index 45033c31a2d5..8a26f6941dc4 100644
> --- a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
> +++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
> @@ -38,6 +38,12 @@ properties:
>        - ethernet-phy-id001c.cad0
>        - ethernet-phy-id001c.cb00
>  
> +  clocks:
> +    maxItems: 1
> +    description:
> +      External clock connected to the PHY. If not specified it is assumed
> +      that the PHY uses a fixed crystal or an internal oscillator.
> +
>    leds: true
>  
>    realtek,aldps-enable:
> 
> -- 
> 2.53.0
> 
> 

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

Re: [PATCH v2 1/4] dt-bindings: net: ethernet-phy: move clocks property to invidivual PHY bindings

[George Moussalem]

On 6/2/26 20:34, Conor Dooley wrote:
> On Tue, Jun 02, 2026 at 10:50:37AM +0400, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <george.moussalem@outlook.com>
>>
>> Move the clock property and restriction from the ethernet-phy.yaml file
>> to the individual PHY binding files. This allows each PHY to manage its
>> own clock requirements.
>>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
>> ---
>> Commit 350b7a258f20 introduced the clocks property with a restriction to
>> maximum 1 to the main ethernet-phy.yaml binding for Realtek to add an
>> optional external clock source. This is restrictive to all PHY bindings,
>> as some PHYs may require more than 1 clock such as the IPQ5018 PHY which
>> requires 2 clocks (for RX and TX).
>>
>> There are three other PHY drivers that require clock management:
>> - Micrel: requires 1 optional clock and the micrel.yaml file already
>> accomodates for the clock property.
>> - SMSC: requires an optional clock and the legacy bindings file
>> (smsc-lan87xx.txt) already accomodates for the clock property.
>> - BCM7xxx: requires an optional clock. I could not find a bindings file
>> for this PHY family.
> 
> Have you done a large-scale dtbs_check run with this patch applied and
> checked that it does not ban having a clock for phys with no dedicated
> bindings?

I've ran: make V=s CHECK_DTBS=y DT_SCHEMA_FILES=/net/ ARCH=arm64

There were a whole lot of errors, mostly related to missing required
regulator/power-supply properties, but none related to clocks.

Also ran: make dt_binding_check DT_SCHEMA_FILES=/net/

This ran successfully apart from the one error I highlighted in my reply
to Rob on v1:

  DTC [C] Documentation/devicetree/bindings/net/renesas,ether.example.dtb
/home/george/src/linux-next/out/Documentation/devicetree/bindings/net/renesas,ether.example.dtb:
ethernet-phy@1 (ethernet-phy-id0022.1537): compatible:
['ethernet-phy-id0022.1537', 'ethernet-phy-ieee802.3-c22'] is too long
	from schema $id: http://devicetree.org/schemas/net/micrel.yaml

In addition, I ran `grep -r '#include <linux/clk.h>' drivers/net/phy` to
identify all phy drivers that manage clocks which I've reported above.
There's one more: xlnx_gmii2rgmii.c which acquires and enables 1 clock
which is also accounted for in its schema.

None of the phy-core files manage clocks.

> I feel like weakening the limit of a single clock is probably more
> accurate than outright banning clocks for ethernet phys without a
> dedicated binding?

If you think increasing the restriction is a better solution, I can
submit an updated version. Kindly let me know.

> 
> Cheers,
> Conor.
> 
>> ---
>>  Documentation/devicetree/bindings/net/ethernet-phy.yaml    | 6 ------
>>  Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml | 6 ++++++
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> index 21a1a63506f0..709ea976ef79 100644
>> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> @@ -105,12 +105,6 @@ properties:
>>        1BR-10 names. The PHY must be configured to operate in BroadR-Reach mode
>>        by software.
>>  
>> -  clocks:
>> -    maxItems: 1
>> -    description:
>> -      External clock connected to the PHY. If not specified it is assumed
>> -      that the PHY uses a fixed crystal or an internal oscillator.
>> -
>>    enet-phy-lane-swap:
>>      $ref: /schemas/types.yaml#/definitions/flag
>>      description:
>> diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
>> index 45033c31a2d5..8a26f6941dc4 100644
>> --- a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
>> +++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
>> @@ -38,6 +38,12 @@ properties:
>>        - ethernet-phy-id001c.cad0
>>        - ethernet-phy-id001c.cb00
>>  
>> +  clocks:
>> +    maxItems: 1
>> +    description:
>> +      External clock connected to the PHY. If not specified it is assumed
>> +      that the PHY uses a fixed crystal or an internal oscillator.
>> +
>>    leds: true
>>  
>>    realtek,aldps-enable:
>>
>> -- 
>> 2.53.0
>>
>>

Best regards,
George

Re: [PATCH v2 1/4] dt-bindings: net: ethernet-phy: move clocks property to invidivual PHY bindings

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing `clocks` from the common `ethernet-phy.yaml` without propagating it to dependent PHY schemas breaks schema validation.
--

commit 670ca8e864ec47d85a59d1ab5fa91fe5fe6d6c54
Author: George Moussalem <george.moussalem@outlook.com>

dt-bindings: net: ethernet-phy: move clocks property to invidivual PHY bindings

Move the clock property and restriction from the ethernet-phy.yaml file
to the individual PHY binding files. This allows each PHY to manage its
own clock requirements.

> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 21a1a63506f01..709ea976ef790 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -105,12 +105,6 @@ properties:
>        1BR-10 names. The PHY must be configured to operate in BroadR-Reach mode
>        by software.
>  
> -  clocks:
> -    maxItems: 1
> -    description:
> -      External clock connected to the PHY. If not specified it is assumed
> -      that the PHY uses a fixed crystal or an internal oscillator.
> -

[Severity: Medium]
Does removing the clocks property from the common ethernet-phy.yaml break
schema validation for other dependent PHY bindings?

Bindings such as Documentation/devicetree/bindings/net/micrel.yaml enforce
unevaluatedProperties: false and depend on this parent schema. Since they
do not define the clocks property themselves, will any valid device tree
configuring a hardware clock for these PHYs now fail dtbs_check with an
unevaluated property error?

For example, micrel.yaml allows clock-names, which logically requires a
clocks property to be present in the device tree, but without inheriting
it from ethernet-phy.yaml, it now lacks the schema allowance for it.

Could the clocks property be added to all dependent PHY bindings at the
same time it is removed from the common schema to prevent dtbs_check
breakages?

>    enet-phy-lane-swap:
>      $ref: /schemas/types.yaml#/definitions/flag
>      description:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-ipq5018-gephy-clocks-v2-0-65a1f1d881f3@outlook.com?part=1

Re: [PATCH v2 4/4] net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mixing devm_clk_get_enabled() with manual clk_enable()/clk_disable() causes enable count imbalances, devres teardown conflicts, and fails to gate clocks.
- [Medium] Missing error check for clk_enable() calls.
--

commit b2bbbd0f6e2e237442a168520c225e85c25f2d4a
Author: George Moussalem <george.moussalem@outlook.com>

net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

This commit adds RX and TX clock acquisition for the IPQ5018 PHY using
devm_clk_get_enabled(). It also attempts to manage power by manually toggling
these clocks in ipq5018_link_change_notify() based on the link state.

> diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
> index 63726cf98cd42..b7361a14220d8 100644
> --- a/drivers/net/phy/qcom/at803x.c
> +++ b/drivers/net/phy/qcom/at803x.c

[ ... ]

> @@ -1062,6 +1065,16 @@ static int ipq5018_config_init(struct phy_device *phydev)
>  
>  static void ipq5018_link_change_notify(struct phy_device *phydev)
>  {
> +	struct ipq5018_priv *priv = phydev->priv;
> +
> +	if (phydev->link) {
> +		clk_enable(priv->rx_clk);
> +		clk_enable(priv->tx_clk);

[Severity: Medium]
Should the return values of clk_enable() be checked here?

If clk_enable(priv->rx_clk) succeeds but clk_enable(priv->tx_clk) fails,
the hardware might be left in a partially enabled state without any logged
warning. Would it be better to explicitly rollback and disable the
previously enabled clocks if an error occurs to maintain a consistent state?

> +	} else {
> +		clk_disable(priv->rx_clk);
> +		clk_disable(priv->tx_clk);
> +	}
> +
>  	/*
>  	 * Reset the FIFO buffer upon link disconnects to clear any residual data
>  	 * which may cause issues with the FIFO which it cannot recover from.

[ ... ]

> @@ -1084,6 +1097,16 @@ static int ipq5018_probe(struct phy_device *phydev)
>  	priv->set_short_cable_dac = of_property_read_bool(dev->of_node,
>  							  "qcom,dac-preset-short-cable");
>  
> +	priv->rx_clk = devm_clk_get_enabled(dev, "rx");

[Severity: High]
Does mixing devm_clk_get_enabled() with manual clk_enable() and clk_disable()
cause enable count imbalances?

Since devm_clk_get_enabled() establishes a base enable count of 1 and
registers an unconditional devres teardown action to call
clk_disable_unprepare(), manually toggling the clocks in
ipq5018_link_change_notify() could lead to a few unintended behaviors:

1. The count initializes to 1. If the link comes up, the count increases
   to 2. When the link goes down, it drops to 1. The hardware clocks are
   never actually gated since the count never reaches 0.

2. If the driver is unbound while the link is down (after a manual
   clk_disable()), devres will call clk_disable() again, dropping the
   enable_count below zero and triggering a kernel WARN.

3. If phy_stop() halts the PHY, it invokes the notify callback before
   phydev->link is cleared. If the link was up, clk_enable() is called
   again, inflating the count to 3. Devres cleanup will then only drop
   the count by 1, permanently leaking the clock reference.

Could this be resolved by using devm_clk_get() instead, and handling the
prepare/enable states directly?

> +	if (IS_ERR(priv->rx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->rx_clk),
> +				     "failed to get and enable RX clock\n");
> +
> +	priv->tx_clk = devm_clk_get_enabled(dev, "tx");
> +	if (IS_ERR(priv->tx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
> +				     "failed to get and enable TX clock\n");
> +
>  	priv->rst = devm_reset_control_array_get_exclusive(dev);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-ipq5018-gephy-clocks-v2-0-65a1f1d881f3@outlook.com?part=4

Re: [PATCH v2 2/4] dt-bindings: net: qca,ar803x: Add clocks property for IPQ5018 PHY

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The PHY's link_change_notify callback naively toggles clocks based on link state without tracking its own enable state, leading to severe clock leaks and unbalanced disables.
--

commit b2bbbd0f6e2e237442a168520c225e85c25f2d4a
Author: George Moussalem <george.moussalem@outlook.com>

net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

Acquire and enable the RX and TX clocks for the IPQ5018 PHY. These clocks
are required for the PHY's datapath to function correctly. In addition,
gate the clocks upon link state changes for improved power management.

> diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
> index 63726cf98cd42..b7361a14220d8 100644
> --- a/drivers/net/phy/qcom/at803x.c
> +++ b/drivers/net/phy/qcom/at803x.c
[ ... ]
> @@ -1062,6 +1065,16 @@ static int ipq5018_config_init(struct phy_device *phydev)
>  
>  static void ipq5018_link_change_notify(struct phy_device *phydev)
>  {
> +	struct ipq5018_priv *priv = phydev->priv;
> +
> +	if (phydev->link) {
> +		clk_enable(priv->rx_clk);
> +		clk_enable(priv->tx_clk);
> +	} else {
> +		clk_disable(priv->rx_clk);
> +		clk_disable(priv->tx_clk);
> +	}
> +

[Severity: High]
Can this logic in ipq5018_link_change_notify() lead to unbalanced clock
enable and disable operations?

The callback is executed by phy_process_state_change() for every change in
phydev->state.

If an interface is brought down manually, phy_stop() transitions the state
to PHY_HALTED and calls phy_process_state_change(). At this exact moment,
phydev->link has not been updated yet and remains 1 if the interface was
running. Consequently, this callback executes a duplicate clk_enable() on an
already enabled clock, incrementing the enable count to 2 and causing a
permanent clock leak.

Conversely, if the interface is brought down when the link is already down
(meaning phydev->link == 0 and the clock was already disabled during the
PHY_UP to PHY_NOLINK transition), phy_process_state_change() triggers the
callback again. The callback reads phydev->link == 0 and executes a
duplicate clk_disable() on an already disabled clock.

This triggers an unbalanced clock disable kernel warning and underflows the
clock's enable count, which can corrupt the system's clock tree and block
power state transitions.

Could we track the clock's enable state within struct ipq5018_priv to prevent
these duplicate operations during state machine transitions?

>  	/*
>  	 * Reset the FIFO buffer upon link disconnects to clear any residual data
>  	 * which may cause issues with the FIFO which it cannot recover from.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-ipq5018-gephy-clocks-v2-0-65a1f1d881f3@outlook.com?part=2