[PATCH net-next 0/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[PATCH net-next 0/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[Mohd Ayaan Anwar]

This series incorporates feedback from the RFC and adds the dt-bindings
and driver changes required to enable Gigabit Ethernet support on the
Qualcomm Shikra SoC. DTS changes will be sent out separately.

The series has grown from the RFC and can be broadly divided into three
areas: PHY power supply management, proper support for RGMII ID mode in
the Qualcomm ETHQOS driver, and Shikra specific clock dependencies
required for NOC access.

1. Shikra EVK boards use a GPIO-gated regulator for the DP83867 PHY
power supply. Patches 1 and 2 add supply management to the DP83867
driver so the PHY driver holds the regulator vote at probe. These
changes are largely inspired by similar work done for the QCA8081 PHY:
https://lore.kernel.org/netdev/20260605010022.968612-3-elder@riscstar.com/

2. The generic RGMII fixes from the RFC remain. As suggested, an
additional patch now emits a warning (patch 6) when the legacy "rgmii"
or "rgmii-txid" modes are detected. A new change (patch 7) now
initialises the RGMII link clock at SPEED_10 on probe instead of
SPEED_1000, dropping the unnecessary 250 MHz source before link up.

3. NOC clock voting (patch 8) now uses dev_pm_opp_set_rate() so the
required VDD_CX performance state can be propagated through the clock
controller. The Shikra binding (patch 3) is updated accordingly: an
if/else block constrains Shikra to exactly six clocks and requires
operating-points-v2.

Testing:
The following boards have been tested with this new (proper) handling
for RGMII ID:
  - Shikra CQ/IQ variants (with the TI DP83867 PHY)
  - QCS615 Ride (with the Micrel KSZ9031 PHY)
  - Talos EVK (again with the Micrel KSZ9031 PHY)
  - Talos Lyra EVK (TI DP83867 PHY, this board also has a GPIO-gated
    regulator for the PHY power supply)

Changes since RFC:
  - Two new patches add supply management for the DP83867, replacing
    the gpio-hog approach for PHY power -- Konrad, Andrew.
  - Use dev_pm_opp_set_rate() for the NOC AXI clock and require
    operating-points-v2 for Shikra -- Konrad.
  - Warn on legacy "rgmii"/"rgmii-txid" to encourage DTB migration --
    Andrew, Maxime.
  - Updated binding document for qcom,ethqos and snps,dwmac.
  - Drop the duplicate "axi" clock from Shikra's DT; use "axi-noc" and
    "pcie-tile-axi-noc" as the only additional clock-names.
  - Initialise RGMII link clock at SPEED_10 rather than SPEED_1000 on
    probe.
  - Link to RFC: https://lore.kernel.org/netdev/20260612-shikra_ethernet-v1-0-f0f4a1d19929@oss.qualcomm.com/

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
Mohd Ayaan Anwar (9):
      dt-bindings: net: ti,dp83867: add supply properties
      net: phy: dp83867: add regulator supply management
      dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible
      net: stmmac: qcom-ethqos: convert ethqos_rgmii_macro_init() to void
      net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass
      net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes
      net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed
      net: stmmac: qcom-ethqos: add per-platform NOC clock voting
      net: stmmac: qcom-ethqos: add Shikra EMAC support

 .../devicetree/bindings/net/qcom,ethqos.yaml       |  50 ++++-
 .../devicetree/bindings/net/snps,dwmac.yaml        |   2 +
 .../devicetree/bindings/net/ti,dp83867.yaml        |  14 ++
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 211 +++++++++++++++++++--
 drivers/net/phy/dp83867.c                          |  32 ++++
 5 files changed, 293 insertions(+), 16 deletions(-)
---
base-commit: 7042c8c193e5d634198b7c766bb3a01c8e3ee0e2
change-id: 20260903-shikra_ethernet-ae7bee5e804e

Best regards,
-- 
Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

[PATCH net-next 1/9] dt-bindings: net: ti,dp83867: add supply properties

[Mohd Ayaan Anwar]

The DP83867 PHY has four distinct power supplies:

  VDDA2P5 -- 2.5V analog supply
  VDD1P0  -- 1.0V digital supply
  VDDA1P8 -- 1.8V analog supply
  VDDIO   -- I/O supply, which can be 3.3V or 2.5V or 1.8V

The DP83867E/IS/CS datasheet (section 8.3) describes two power supply
configurations - "two-supply" and "three-supply".

  1. Two-Supply Configuration: VDDA2P5 and VDD1P0 connected, and the
     VDDA1P8 pins are left unconnected.
  2. Three-Supply Configuration: VDDA2P5, VDD1P0 and VDDA1P8 are all
     connected.

VDDIO is a separate fourth supply present in both configurations. The
datasheet specifies that it can either be 1.8V or 2.5V or 3.3V.

Some embedded board design expose a GPIO-controlled regulator to control
any one or more of these supplies. Therefore, document all four of the
available supply properties.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/net/ti,dp83867.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
index 4bc1f98fd9fe51260b767b0a1b8b7eaf974a9af9..e78f08ded3d0d449ab75bf33d814fd49587f8387 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
@@ -118,6 +118,20 @@ properties:
       Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h for applicable
       values.
 
+  vdda-2p5-supply:
+    description: 2.5V analog supply (connected to the VDDA2P5 pins).
+
+  vdd-1p0-supply:
+    description: 1.0V digital core supply (connected to the VDD1P0 pins).
+
+  vdda-1p8-supply:
+    description: 1.8V analog supply (connected to the VDDA1P8 pins).
+
+  vddio-supply:
+    description: |
+      I/O supply (connected to the VDDIO pins). The supply can be 1.8V,
+      2.5V or 3.3V.
+
 required:
   - reg
 

-- 
2.34.1

[PATCH net-next 2/9] net: phy: dp83867: add regulator supply management

[Mohd Ayaan Anwar]

Some embedded board designs use GPIO-controlled regulators for the
DP83867 power rails. Add dp83867_power_on() to enable all four supply
domains at probe time. Absent supplies are silently skipped, so boards
that do not describe them are unaffected.

When any supply is newly enabled the driver sleeps for 200 ms before
returning.  This satisfies the post power-up stabilisation requirement
mentioned in section 6.6 of the DP83867E/IS/CS datasheet.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 drivers/net/phy/dp83867.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 88255e92b4cdbd6da2e2c1d10f9c72348d28dbc3..e03b39fd080d12afe0d02a53ffef2a6adf73cd9f 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -15,6 +15,7 @@
 #include <linux/etherdevice.h>
 #include <linux/bitfield.h>
 #include <linux/nvmem-consumer.h>
+#include <linux/regulator/consumer.h>
 
 #include <dt-bindings/net/ti-dp83867.h>
 
@@ -719,9 +720,40 @@ static int dp83867_resume(struct phy_device *phydev)
 	return 0;
 }
 
+static int dp83867_power_on(struct phy_device *phydev)
+{
+	static const char * const supply_names[] = {
+		"vdda-2p5", "vdd-1p0", "vdda-1p8", "vddio",
+	};
+	struct device *dev = &phydev->mdio.dev;
+	u32 count = 0;
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(supply_names); i++) {
+		ret = devm_regulator_get_enable_optional(dev, supply_names[i]);
+		if (!ret)
+			count++;
+		else if (ret != -ENODEV)
+			return dev_err_probe(dev, ret,
+					     "failed to enable %s supply\n",
+					     supply_names[i]);
+	}
+
+	/* Datasheet section 6.6 suggests a 200ms post power-up stabilization */
+	if (count)
+		fsleep(200000);
+
+	return 0;
+}
+
 static int dp83867_probe(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867;
+	int ret;
+
+	ret = dp83867_power_on(phydev);
+	if (ret)
+		return ret;
 
 	dp83867 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83867),
 			       GFP_KERNEL);

-- 
2.34.1

[PATCH net-next 3/9] dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible

[Mohd Ayaan Anwar]

Shikra's EMAC requires two additional clocks for NOC interconnect
access (axi-noc, pcie-tile-axi-noc) beyond the standard four, and an
OPP table with required-opps to vote VDD_CX to SVS when the NOC clocks
are enabled.

Add qcom,shikra-ethqos to the compatible enum and use an if/else
block to constrain Shikra to exactly six clocks and require
operating-points-v2, while leaving existing compatibles unchanged.

Add the relevant compatible to the binding document for snps,dwmac as
well.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 .../devicetree/bindings/net/qcom,ethqos.yaml       | 50 ++++++++++++++++++++--
 .../devicetree/bindings/net/snps,dwmac.yaml        |  2 +
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
index 423959cb928d945aa3e758a3c803d12bd61ec42b..ad2cc706fa74041486cd90c3d0eb1dde0acaa27c 100644
--- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
@@ -14,9 +14,6 @@ description:
   dwmmac based Qualcomm ethernet devices which support Gigabit
   ethernet (version v2.3.0 and onwards).
 
-allOf:
-  - $ref: snps,dwmac.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -32,6 +29,7 @@ properties:
           - qcom,qcs404-ethqos
           - qcom,sa8775p-ethqos
           - qcom,sc8280xp-ethqos
+          - qcom,shikra-ethqos
           - qcom,sm8150-ethqos
 
   reg:
@@ -57,9 +55,11 @@ properties:
       - const: sfty
 
   clocks:
-    maxItems: 4
+    minItems: 4
+    maxItems: 6
 
   clock-names:
+    minItems: 4
     items:
       - const: stmmaceth
       - const: pclk
@@ -67,6 +67,8 @@ properties:
       - enum:
           - rgmii
           - phyaux
+      - const: axi-noc
+      - const: pcie-tile-axi-noc
 
   iommus:
     maxItems: 1
@@ -81,6 +83,11 @@ properties:
       - const: cpu-mac
       - const: mac-mem
 
+  operating-points-v2: true
+
+  opp-table:
+    type: object
+
   phys: true
 
   phy-names:
@@ -92,6 +99,41 @@ required:
   - clock-names
   - reg-names
 
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,shikra-ethqos
+    then:
+      properties:
+        clocks:
+          minItems: 6
+        clock-names:
+          items:
+            - const: stmmaceth
+            - const: pclk
+            - const: ptp_ref
+            - const: rgmii
+            - const: axi-noc
+            - const: pcie-tile-axi-noc
+      required:
+        - operating-points-v2
+    else:
+      properties:
+        clocks:
+          maxItems: 4
+        clock-names:
+          items:
+            - const: stmmaceth
+            - const: pclk
+            - const: ptp_ref
+            - enum:
+                - rgmii
+                - phyaux
+
 unevaluatedProperties: false
 
 examples:
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 2449311c6d28ed3fbf8c92526ce8b872900653f4..6a1aea371c0b42ef71f90d89276e15159c3b3277 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -74,6 +74,7 @@ properties:
         - qcom,qcs404-ethqos
         - qcom,sa8775p-ethqos
         - qcom,sc8280xp-ethqos
+        - qcom,shikra-ethqos
         - qcom,sm8150-ethqos
         - renesas,r9a06g032-gmac
         - renesas,r9a08g046-gbeth
@@ -632,6 +633,7 @@ allOf:
                 - qcom,qcs404-ethqos
                 - qcom,sa8775p-ethqos
                 - qcom,sc8280xp-ethqos
+                - qcom,shikra-ethqos
                 - qcom,sm8150-ethqos
                 - snps,dwmac-4.00
                 - snps,dwmac-4.10a

-- 
2.34.1

[PATCH net-next 4/9] net: stmmac: qcom-ethqos: convert ethqos_rgmii_macro_init() to void

[Mohd Ayaan Anwar]

The return value is never checked by its sole caller and the speed
validation duplicates a check higher up the call stack.  Convert to
void and remove the dead code.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index ac7d6d3e205a1ab5b391def879d6f1033a0961b6..44270c25d874f72e7f971757fec659d36468c315 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -371,9 +371,8 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 	return 0;
 }
 
-static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
+static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 {
-	struct device *dev = &ethqos->pdev->dev;
 	unsigned int prg_rclk_dly, loopback;
 	unsigned int phase_shift;
 
@@ -384,11 +383,6 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 	/* Select RGMII, write 0 to interface select */
 	rgmii_clrmask(ethqos, RGMII_CONFIG_INTF_SEL, RGMII_IO_MACRO_CONFIG);
 
-	if (speed != SPEED_1000 && speed != SPEED_100 && speed != SPEED_10) {
-		dev_err(dev, "Invalid speed %d\n", speed);
-		return -EINVAL;
-	}
-
 	rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
 
 	if (speed == SPEED_1000) {
@@ -479,8 +473,6 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 
 	rgmii_updatel(ethqos, RGMII_CONFIG_LOOPBACK_EN, loopback,
 		      RGMII_IO_MACRO_CONFIG);
-
-	return 0;
 }
 
 static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,

-- 
2.34.1

[PATCH net-next 5/9] net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass

[Mohd Ayaan Anwar]

When "rgmii-id" is selected the PHY supplies both TX and RX delays, so
the MAC must not add its own. The driver currently falls through to the
generic DLL initialisation path which programs it to add a delay.

Power down the DLL and set DDR bypass mode for RGMII_ID, then program
the IO_MACRO via a new ethqos_rgmii_id_macro_init() helper. Also fix
ethqos_set_clk_tx_rate() to not double the clock rate in bypass mode at
100M/10M, and remove RGMII_ID from the phase-shift suppression in
ethqos_rgmii_macro_init() since RGMII_ID no longer reaches that path.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 67 +++++++++++++++++++++-
 1 file changed, 64 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 44270c25d874f72e7f971757fec659d36468c315..67738c1598c50ade22f2d548a6d66911c348af51 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -67,6 +67,9 @@
 /* SDC4_STATUS bits */
 #define SDC4_STATUS_DLL_LOCK			BIT(7)
 
+/* SDCC_USR_CTL bits */
+#define SDCC_USR_CTL_DDR_BYPASS			BIT(30)
+
 /* RGMII_IO_MACRO_CONFIG2 fields */
 #define RGMII_CONFIG2_RSVD_CONFIG15		GENMASK(31, 17)
 #define RGMII_CONFIG2_RGMII_CLK_SEL_CFG		BIT(16)
@@ -183,7 +186,15 @@ static int ethqos_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
 	if (rate < 0)
 		return rate;
 
-	return clk_set_rate(ethqos->link_clk, rate * 2);
+	/* Clock Rate Requirements:
+	 * MAC added delay: 250/50/5 Mhz for 1G/100M/10M
+	 * No MAC delay (DLL bypass): 250/25/2.5 Mhz for 1G/100M/10M
+	 */
+	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII ||
+	    speed == SPEED_1000)
+		rate *= 2;
+
+	return clk_set_rate(ethqos->link_clk, rate);
 }
 
 static void
@@ -405,8 +416,7 @@ static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 		      RGMII_IO_MACRO_CONFIG2);
 
 	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
-	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
-	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
+	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
 		phase_shift = 0;
 	else
 		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
@@ -475,6 +485,42 @@ static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 		      RGMII_IO_MACRO_CONFIG);
 }
 
+static void ethqos_rgmii_id_macro_init(struct qcom_ethqos *ethqos, int speed)
+{
+	rgmii_clrmask(ethqos, RGMII_CONFIG2_TX_TO_RX_LOOPBACK_EN,
+		      RGMII_IO_MACRO_CONFIG2);
+
+	if (speed == SPEED_1000)
+		rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
+	else
+		rgmii_clrmask(ethqos, RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
+	rgmii_setmask(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN, RGMII_IO_MACRO_CONFIG);
+	rgmii_clrmask(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL, RGMII_IO_MACRO_CONFIG);
+	rgmii_clrmask(ethqos, RGMII_CONFIG_PROG_SWAP, RGMII_IO_MACRO_CONFIG);
+
+	if (ethqos->has_emac_ge_3)
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
+			      RGMII_IO_MACRO_CONFIG2);
+	else
+		rgmii_setmask(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
+			      RGMII_IO_MACRO_CONFIG2);
+
+	rgmii_clrmask(ethqos, RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN,
+		      RGMII_IO_MACRO_CONFIG2);
+
+	if (speed == SPEED_1000)
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, RGMII_IO_MACRO_CONFIG2);
+	else
+		rgmii_setmask(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, RGMII_IO_MACRO_CONFIG2);
+
+	if (ethqos->rgmii_config_loopback_en)
+		rgmii_setmask(ethqos, RGMII_CONFIG_LOOPBACK_EN, RGMII_IO_MACRO_CONFIG);
+	else
+		rgmii_clrmask(ethqos, RGMII_CONFIG_LOOPBACK_EN, RGMII_IO_MACRO_CONFIG);
+
+	rgmii_setmask(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, RGMII_IO_MACRO_CONFIG2);
+}
+
 static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
 				       phy_interface_t interface, int speed,
 				       unsigned int mode)
@@ -493,6 +539,21 @@ static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
 
 	ethqos_set_func_clk_en(ethqos);
 
+	/* For rgmii-id mode, the PHY should add the required delays.
+	 * Therefore, power down the DLL and program it in bypass mode.
+	 * Program the IO_MACRO as per the settings recommended by the
+	 * programming guide for bypass mode. This will ensure that the
+	 * MAC core doesn't add any additional delays.
+	 */
+	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) {
+		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
+		rgmii_setmask(ethqos, SDCC_USR_CTL_DDR_BYPASS, SDCC_USR_CTL);
+
+		ethqos_rgmii_id_macro_init(ethqos, speed);
+
+		return;
+	}
+
 	/* Initialize the DLL first */
 
 	/* Set DLL_RST */

-- 
2.34.1

[PATCH net-next 6/9] net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes

[Mohd Ayaan Anwar]

The qcom-ethqos driver is moving towards using "rgmii-id" together
with PHY-provided delays. However, existing DTBs use "rgmii" and
"rgmii-txid" and must remain supported for backwards compatibility.

Warn when either of these legacy PHY modes is used to encourage users
to migrate to updated DTBs using "rgmii-id".

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 67738c1598c50ade22f2d548a6d66911c348af51..2ca2f03de30174c9771fdc517c0e21e3b392ad76 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -554,6 +554,8 @@ static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
 		return;
 	}
 
+	dev_warn(dev, "legacy RGMII phy-mode detected; consider upgrading to a newer DTB\n");
+
 	/* Initialize the DLL first */
 
 	/* Set DLL_RST */

-- 
2.34.1

[PATCH net-next 7/9] net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed

[Mohd Ayaan Anwar]

On probe the RGMII link clock is initialised at SPEED_1000, which
translates to a 250 MHz source clock even when no PHY link is present,
drawing unnecessary power.

Initialise at SPEED_10 instead; fix_mac_speed updates the rate once
a link is established.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 2ca2f03de30174c9771fdc517c0e21e3b392ad76..4f9bef662c5796ce3cca64b19a595bcdbebbf336 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -814,7 +814,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 				     "Failed to get serdes phy\n");
 
 	ethqos_set_clk_tx_rate(ethqos, NULL, plat_dat->phy_interface,
-			       SPEED_1000);
+			       SPEED_10);
 
 	qcom_ethqos_set_sgmii_loopback(ethqos, true);
 	ethqos_set_func_clk_en(ethqos);

-- 
2.34.1

[PATCH net-next 8/9] net: stmmac: qcom-ethqos: add per-platform NOC clock voting

[Mohd Ayaan Anwar]

Some SoCs gate the EMAC's path to the System NOC behind dedicated clocks
that must be enabled before the DMA can reach memory. Add
ethqos_noc_clk_cfg and the corresponding fields in the driver-data and
runtime structs so each compatible can declare its own set with per-clock
rates.  The clocks are acquired during probe and enabled/disabled
alongside the existing link clock in ethqos_clks_config().

No functional change for existing compatibles. This will help us when
we add support for Shikra.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 101 +++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 4f9bef662c5796ce3cca64b19a595bcdbebbf336..a31541506a4c930c76374d7d8b7eceb6f51f37a7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -7,6 +7,7 @@
 #include <linux/platform_device.h>
 #include <linux/phy.h>
 #include <linux/phy/phy.h>
+#include <linux/pm_opp.h>
 
 #include "stmmac.h"
 #include "stmmac_platform.h"
@@ -84,11 +85,18 @@
 
 #define SGMII_10M_RX_CLK_DVDR			0x31
 
+#define ETHQOS_MAX_NOC_CLKS			3
+
 struct ethqos_emac_por {
 	unsigned int offset;
 	unsigned int value;
 };
 
+struct ethqos_noc_clk_cfg {
+	const char *id;
+	unsigned long rate;
+};
+
 struct ethqos_emac_driver_data {
 	const struct ethqos_emac_por *rgmii_por;
 	unsigned int num_rgmii_por;
@@ -98,6 +106,8 @@ struct ethqos_emac_driver_data {
 	const char *link_clk_name;
 	struct dwmac4_addrs dwmac4_addrs;
 	bool needs_sgmii_loopback;
+	const struct ethqos_noc_clk_cfg *noc_clk_cfg;
+	unsigned int num_noc_clks;
 };
 
 struct qcom_ethqos {
@@ -112,6 +122,10 @@ struct qcom_ethqos {
 	bool rgmii_config_loopback_en;
 	bool has_emac_ge_3;
 	bool needs_sgmii_loopback;
+
+	struct clk_bulk_data noc_clks[ETHQOS_MAX_NOC_CLKS];
+	unsigned long noc_clk_rates[ETHQOS_MAX_NOC_CLKS];
+	int num_noc_clks;
 };
 
 static u32 rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
@@ -691,15 +705,51 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
 static int ethqos_clks_config(void *priv, bool enabled)
 {
 	struct qcom_ethqos *ethqos = priv;
+	unsigned int i;
 	int ret = 0;
 
 	if (enabled) {
+		if (ethqos->num_noc_clks) {
+			ret = dev_pm_opp_set_rate(&ethqos->pdev->dev,
+						  ethqos->noc_clk_rates[0]);
+			if (ret) {
+				dev_err(&ethqos->pdev->dev,
+					"NOC OPP rate set failed: %d\n", ret);
+				return ret;
+			}
+
+			for (i = 1; i < ethqos->num_noc_clks; i++) {
+				ret = clk_set_rate(ethqos->noc_clks[i].clk,
+						   ethqos->noc_clk_rates[i]);
+				if (ret) {
+					dev_err(&ethqos->pdev->dev,
+						"NOC clock rate set failed: %d\n", ret);
+					dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
+					return ret;
+				}
+			}
+		}
+
 		ret = clk_prepare_enable(ethqos->link_clk);
 		if (ret) {
 			dev_err(&ethqos->pdev->dev, "link_clk enable failed\n");
+			if (ethqos->num_noc_clks)
+				dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
 			return ret;
 		}
 
+		if (ethqos->num_noc_clks) {
+			ret = clk_bulk_prepare_enable(ethqos->num_noc_clks,
+						      ethqos->noc_clks);
+			if (ret) {
+				dev_err(&ethqos->pdev->dev,
+					"NOC clocks enable failed: %d\n", ret);
+				dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
+				clk_disable_unprepare(ethqos->link_clk);
+				return ret;
+			}
+		}
+
 		/* Enable functional clock to prevent DMA reset to timeout due
 		 * to lacking PHY clock after the hardware block has been power
 		 * cycled. The actual configuration will be adjusted once
@@ -708,7 +758,12 @@ static int ethqos_clks_config(void *priv, bool enabled)
 		qcom_ethqos_set_sgmii_loopback(ethqos, true);
 		ethqos_set_func_clk_en(ethqos);
 	} else {
+		if (ethqos->num_noc_clks)
+			clk_bulk_disable_unprepare(ethqos->num_noc_clks,
+						   ethqos->noc_clks);
 		clk_disable_unprepare(ethqos->link_clk);
+		if (ethqos->num_noc_clks)
+			dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
 	}
 
 	return ret;
@@ -736,6 +791,46 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
 	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
 }
 
+/* Some SoCs gate NOC access behind dedicated clocks. Acquire them here
+ * so ethqos_clks_config() can enable/disable them at runtime. The OPP
+ * table is used to propagate the required VDD_CX performance state via
+ * dev_pm_opp_set_rate().
+ */
+static int qcom_ethqos_init_noc_clks(struct qcom_ethqos *ethqos,
+				     const struct ethqos_emac_driver_data *data)
+{
+	struct device *dev = &ethqos->pdev->dev;
+	unsigned int i;
+	int ret;
+
+	if (!data->num_noc_clks)
+		return 0;
+
+	for (i = 0; i < data->num_noc_clks; i++) {
+		ethqos->noc_clks[i].id = data->noc_clk_cfg[i].id;
+		ethqos->noc_clk_rates[i] = data->noc_clk_cfg[i].rate;
+	}
+	ethqos->num_noc_clks = data->num_noc_clks;
+
+	ret = devm_clk_bulk_get(dev, ethqos->num_noc_clks, ethqos->noc_clks);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get NOC clocks\n");
+
+	ret = devm_pm_opp_set_clkname(dev, data->noc_clk_cfg[0].id);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set OPP clock name\n");
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add OPP table\n");
+
+	ret = dev_pm_opp_set_rate(dev, data->noc_clk_cfg[0].rate);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set initial NOC OPP rate\n");
+
+	return 0;
+}
+
 static int qcom_ethqos_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -795,6 +890,12 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	ethqos->has_emac_ge_3 = data->has_emac_ge_3;
 	ethqos->needs_sgmii_loopback = data->needs_sgmii_loopback;
 
+	if (data->num_noc_clks) {
+		ret = qcom_ethqos_init_noc_clks(ethqos, data);
+		if (ret)
+			return ret;
+	}
+
 	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
 	if (IS_ERR(ethqos->link_clk))
 		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),

-- 
2.34.1

[PATCH net-next 9/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[Mohd Ayaan Anwar]

Shikra integrates two Qualcomm ETHQOS controllers based on the Synopsys
GMAC IP, similar to previous platforms.  Register qcom,shikra-ethqos
backed by a new shikra_data descriptor that enables the three NOC clocks
required for DMA memory access (axi-noc, pcie-tile-axi-noc, stmmaceth)
all at 120 MHz, and the 36-bit DMA address width.

Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index a31541506a4c930c76374d7d8b7eceb6f51f37a7..f96680456472d1fa7b72bd404c1ddc20e505f05c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -328,6 +328,36 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
 	},
 };
 
+static const struct ethqos_noc_clk_cfg shikra_noc_clks[] = {
+	{ "axi-noc", 120000000 },
+	{ "pcie-tile-axi-noc", 120000000 },
+	{ "stmmaceth", 120000000 },
+};
+
+static const struct ethqos_emac_driver_data shikra_data = {
+	.dma_addr_width = 36,
+	.has_emac_ge_3 = true,
+	.noc_clk_cfg = shikra_noc_clks,
+	.num_noc_clks = ARRAY_SIZE(shikra_noc_clks),
+	.rgmii_config_loopback_en = false,
+	.dwmac4_addrs = {
+		.dma_chan = 0x00008100,
+		.dma_chan_offset = 0x1000,
+		.mtl_chan = 0x00008000,
+		.mtl_chan_offset = 0x1000,
+		.mtl_ets_ctrl = 0x00008010,
+		.mtl_ets_ctrl_offset = 0x1000,
+		.mtl_txq_weight = 0x00008018,
+		.mtl_txq_weight_offset = 0x1000,
+		.mtl_send_slp_cred = 0x0000801c,
+		.mtl_send_slp_cred_offset = 0x1000,
+		.mtl_high_cred = 0x00008020,
+		.mtl_high_cred_offset = 0x1000,
+		.mtl_low_cred = 0x00008024,
+		.mtl_low_cred_offset = 0x1000,
+	},
+};
+
 static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 {
 	struct device *dev = &ethqos->pdev->dev;
@@ -957,6 +987,7 @@ static const struct of_device_id qcom_ethqos_match[] = {
 	{ .compatible = "qcom,qcs404-ethqos", .data = &emac_v2_3_0_data},
 	{ .compatible = "qcom,sa8775p-ethqos", .data = &emac_v4_0_0_data},
 	{ .compatible = "qcom,sc8280xp-ethqos", .data = &emac_v3_0_0_data},
+	{ .compatible = "qcom,shikra-ethqos", .data = &shikra_data},
 	{ .compatible = "qcom,sm8150-ethqos", .data = &emac_v2_1_0_data},
 	{ }
 };

-- 
2.34.1

Re: [PATCH net-next 0/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[Mohd Ayaan Anwar]

On Fri, Sep 04, 2026 at 12:13:42AM +0530, Mohd Ayaan Anwar wrote:
> This series incorporates feedback from the RFC and adds the dt-bindings
> and driver changes required to enable Gigabit Ethernet support on the
> Qualcomm Shikra SoC. DTS changes will be sent out separately.
> 
> The series has grown from the RFC and can be broadly divided into three
> areas: PHY power supply management, proper support for RGMII ID mode in
> the Qualcomm ETHQOS driver, and Shikra specific clock dependencies
> required for NOC access.
> 
> 1. Shikra EVK boards use a GPIO-gated regulator for the DP83867 PHY
> power supply. Patches 1 and 2 add supply management to the DP83867
> driver so the PHY driver holds the regulator vote at probe. These
> changes are largely inspired by similar work done for the QCA8081 PHY:
> https://lore.kernel.org/netdev/20260605010022.968612-3-elder@riscstar.com/
> 
> 2. The generic RGMII fixes from the RFC remain. As suggested, an
> additional patch now emits a warning (patch 6) when the legacy "rgmii"
> or "rgmii-txid" modes are detected. A new change (patch 7) now
> initialises the RGMII link clock at SPEED_10 on probe instead of
> SPEED_1000, dropping the unnecessary 250 MHz source before link up.
> 
> 3. NOC clock voting (patch 8) now uses dev_pm_opp_set_rate() so the
> required VDD_CX performance state can be propagated through the clock
> controller. The Shikra binding (patch 3) is updated accordingly: an
> if/else block constrains Shikra to exactly six clocks and requires
> operating-points-v2.
> 
> Testing:
> The following boards have been tested with this new (proper) handling
> for RGMII ID:
>   - Shikra CQ/IQ variants (with the TI DP83867 PHY)
>   - QCS615 Ride (with the Micrel KSZ9031 PHY)
>   - Talos EVK (again with the Micrel KSZ9031 PHY)
>   - Talos Lyra EVK (TI DP83867 PHY, this board also has a GPIO-gated
>     regulator for the PHY power supply)
> 
> Changes since RFC:
>   - Two new patches add supply management for the DP83867, replacing
>     the gpio-hog approach for PHY power -- Konrad, Andrew.
>   - Use dev_pm_opp_set_rate() for the NOC AXI clock and require
>     operating-points-v2 for Shikra -- Konrad.
>   - Warn on legacy "rgmii"/"rgmii-txid" to encourage DTB migration --
>     Andrew, Maxime.
>   - Updated binding document for qcom,ethqos and snps,dwmac.
>   - Drop the duplicate "axi" clock from Shikra's DT; use "axi-noc" and
>     "pcie-tile-axi-noc" as the only additional clock-names.
>   - Initialise RGMII link clock at SPEED_10 rather than SPEED_1000 on
>     probe.
>   - Link to RFC: https://lore.kernel.org/netdev/20260612-shikra_ethernet-v1-0-f0f4a1d19929@oss.qualcomm.com/
> 
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

Sashiko raised some valid concerns, I will update the series and send
out v2 shortly.

	Ayaan

Re: [PATCH net-next 4/9] net: stmmac: qcom-ethqos: convert ethqos_rgmii_macro_init() to void

[Maxime Chevallier]

Hi,

On 9/3/26 20:43, Mohd Ayaan Anwar wrote:
> The return value is never checked by its sole caller and the speed
> validation duplicates a check higher up the call stack.  Convert to
> void and remove the dead code.
> 
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net-next 7/9] net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed

[Maxime Chevallier]

Hi,

On 9/3/26 20:43, Mohd Ayaan Anwar wrote:
> On probe the RGMII link clock is initialised at SPEED_1000, which
> translates to a 250 MHz source clock even when no PHY link is present,
> drawing unnecessary power.
> 
> Initialise at SPEED_10 instead; fix_mac_speed updates the rate once
> a link is established.
> 
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net-next 1/9] dt-bindings: net: ti,dp83867: add supply properties

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Concern (invalidated): the four newly documented supply
  properties (vdda-2p5-supply, vdd-1p0-supply, vdda-1p8-supply, vddio-…

--

dt-bindings: net: ti,dp83867: add supply properties

This documents four power supply properties for the DP83867 PHY
(vdda-2p5-supply, vdd-1p0-supply, vdda-1p8-supply and vddio-supply),
matching the two-supply and three-supply configurations described in the
datasheet plus the separate VDDIO rail.

> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> index 4bc1f98fd9fe5..e78f08ded3d0d 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> @@ -118,6 +118,20 @@ properties:
>        Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h for applicable
>        values.
>  
> +  vdda-2p5-supply:
> +    description: 2.5V analog supply (connected to the VDDA2P5 pins).
> +
> +  vdd-1p0-supply:
> +    description: 1.0V digital core supply (connected to the VDD1P0 pins).
> +
> +  vdda-1p8-supply:
> +    description: 1.8V analog supply (connected to the VDDA1P8 pins).
> +
> +  vddio-supply:
> +    description: |
> +      I/O supply (connected to the VDDIO pins). The supply can be 1.8V,
> +      2.5V or 3.3V.
> +

[Severity: Low]
One question came up while reviewing this in isolation: do these four
property names have a consumer anywhere?

Looking at the rest of the series, they do. The following patch,
"net: phy: dp83867: add regulator supply management", adds
dp83867_power_on() in drivers/net/phy/dp83867.c, which walks exactly
these names:

  static const char * const supply_names[] = {
          "vdda-2p5", "vdd-1p0", "vdda-1p8", "vddio",
  };
  ...
          ret = devm_regulator_get_enable_optional(dev, supply_names[i]);

and dp83867_probe() calls it:

          ret = dp83867_power_on(phydev);
          if (ret)
                  return ret;

So the names match and devm_regulator_get_enable_optional() handles the
disable/put on teardown. No change requested here; noting it only so the
binding-only patch is not read as documenting unused properties.

One remaining observation, and it belongs to the driver patch rather than
this one: a rail such as VDDIO that gates the MDIO interface itself cannot
be brought up from a PHY probe callback, since probe only runs after the
PHY ID has already been read over MDIO. Is that ordering expected to be
handled by the MDIO bus or board regulator-always-on instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 2/9] net: phy: dp83867: add regulator supply management

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 0

- [High] The supplies are enabled from the PHY driver's ->probe callback,
  which is structurally too late for the use case the commit message…
- [Medium] dp83867_power_on() (drivers/net/phy/dp83867.c) treats a 0
  return from devm_regulator_get_enable_optional() as "this rail was…
- [Medium] dp83867_power_on() propagates every non--ENODEV error out of
  dp83867_probe(), including -EPROBE_DEFER, which…

--

net: phy: dp83867: add regulator supply management

This adds dp83867_power_on(), called from dp83867_probe(), which walks four
supply names, enables each one via devm_regulator_get_enable_optional(),
and sleeps 200 ms if any of them returned success.

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cdb..e03b39fd080d1 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c

[ ... ]

> @@ -719,9 +720,40 @@ static int dp83867_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +	static const char * const supply_names[] = {
> +		"vdda-2p5", "vdd-1p0", "vdda-1p8", "vddio",
> +	};
> +	struct device *dev = &phydev->mdio.dev;
> +	u32 count = 0;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(supply_names); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, supply_names[i]);
> +		if (!ret)
> +			count++;
> +		else if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable %s supply\n",
> +					     supply_names[i]);
> +	}

[Severity: Medium]
This propagates -EPROBE_DEFER out of dp83867_probe(), which
devm_regulator_get_enable_optional() returns when the phandle resolves but
the regulator provider has not registered yet.

Can that deferral end up with genphy bound instead?  In
phy_attach_direct(), if the MAC attaches the PHY while the dp83867 probe is
still deferred:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	if (!d->driver) {
		if (phydev->is_c45)
			d->driver = &genphy_c45_driver.mdiodrv.driver;
		else
			d->driver = &genphy_driver.mdiodrv.driver;

		phydev->is_genphy_driven = 1;
	}
	...
	if (phydev->is_genphy_driven) {
		err = d->driver->probe(d);
		if (err >= 0)
			err = device_bind_driver(d);
	...
}

Once genphy is bound, does the deferred retry still get a chance to bind
dp83867, or does __device_attach() return early because dev->driver is
already set?  If it is the latter, the port links with none of the
DP83867-specific setup (RGMII internal tx/rx delays, tx/rx FIFO depth, IO
impedance, clk-output-sel, rxctrl strap quirk) and nothing is logged.

Would it be worth mentioning the new probe-deferral point in the commit
message?

> +
> +	/* Datasheet section 6.6 suggests a 200ms post power-up stabilization */
> +	if (count)
> +		fsleep(200000);
> +
> +	return 0;
> +}

[Severity: Medium]
Does count actually track "newly enabled" supplies?  The commit message says

  "Absent supplies are silently skipped, so boards that do not describe
  them are unaffected."

and

  "When any supply is newly enabled the driver sleeps for 200 ms"

but with CONFIG_REGULATOR=n the stub never returns -ENODEV:

include/linux/regulator/consumer.h {
	static inline int devm_regulator_get_enable_optional(struct device *dev,
							     const char *id)
	{
		return 0;
	}
}

so count becomes 4 on every probe, including boards that describe no
supplies at all, and fsleep(200000) takes the msleep() branch:

include/linux/delay.h:fsleep() {
	...
	else
		msleep(DIV_ROUND_UP(usecs, USEC_PER_MSEC));
}

The driver has no depends on REGULATOR and no IS_ENABLED(CONFIG_REGULATOR)
guard, so isn't this a plain 200 ms stall per DP83867 probe for all existing
users?

The CONFIG_REGULATOR=y case looks similar for an already-on rail:

drivers/regulator/devres.c:_devm_regulator_get_enable() {
	r = _devm_regulator_get(dev, id, get_type);
	if (IS_ERR(r))
		return PTR_ERR(r);

	ret = regulator_enable(r);
	...
}

regulator_enable() is reference counted and returns 0 for a rail already
enabled by the bootloader, by regulator-always-on/boot-on, or by another
consumer, so no ramp happens yet the 200 ms "post power-up stabilization"
delay is still paid.  Should the delay instead be gated on something that
reflects an actual off to on transition?

>  static int dp83867_probe(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867;
> +	int ret;
> +
> +	ret = dp83867_power_on(phydev);
> +	if (ret)
> +		return ret;

[Severity: High]
Can the rails ever be enabled from here on the boards this targets?  The
commit message describes boards that "use GPIO-controlled regulators for the
DP83867 power rails", but drv->probe() only runs after the PHY has already
answered MDIO ID reads during bus enumeration:

of_mdiobus_register()
  __of_mdiobus_parse_phys()
    of_mdiobus_register_phy()
      fwnode_mdiobus_register_phy()
        get_phy_device()
          get_phy_c22_id()

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_register_phy() {
	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
		phy = get_phy_device(bus, addr, is_c45);
	else
		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
	...
}

Unless the DT child carries an explicit ethernet-phy-idAAAA.BBBB compatible,
an unpowered DP83867 does not answer and the address is simply skipped:

drivers/net/mdio/of_mdio.c:__of_mdiobus_parse_phys() {
	...
	if (rc == -ENODEV)
		dev_err(&mdio->dev,
			"MDIO device at address %d is missing.\n",
			addr);
	...
}

There is no deferred retry for that address, so dp83867_probe() and
dp83867_power_on() never run and the GPIO regulator is never enabled.

For the case where the PHY is found (rail already on at boot, or the ID
hard-coded in DT), reset release and register writes happen before the
supplies are touched:

drivers/net/phy/phy_device.c:phy_device_register() {
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);

	/* Run all of the fixups for this PHY */
	err = phy_scan_fixups(phydev);
	...
}

drivers/net/phy/phy_device.c:phy_probe() {
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);

	if (phydev->drv->probe) {
		err = phydev->drv->probe(phydev);
	...
}

Nothing in the new code re-asserts reset or re-applies configuration after
the rails stabilise, so does the part end up programmed while unpowered and
then left at POR/strap defaults?

Grepping for regulator under drivers/net/mdio/ and in
drivers/net/phy/mdio_bus.c turns up only the mdio-sun4i controller's own
supply, so should rail control for a device that must answer MDIO during
enumeration be handled before/at bus registration, or the rails described as
regulator-always-on/boot-on in DT, rather than in the PHY driver probe?

>  
>  	dp83867 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83867),
>  			       GFP_KERNEL);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 3/9] dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] The new `qcom,shikra-ethqos` conditional in
  Documentation/devicetree/bindings/net/qcom,ethqos.yaml enforces only…
- [Low] Reported concern: the binding documents `qcom,shikra-ethqos` (a
  standalone compatible with no fallback), two new clocks (`axi-noc`,…

--

dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible

Adds the qcom,shikra-ethqos compatible to the ethqos and snps,dwmac
bindings, extends the clock list with axi-noc and pcie-tile-axi-noc, and
adds an if/else block constraining Shikra to six clocks while requiring
operating-points-v2.

> diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> index 423959cb928d9..ad2cc706fa740 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml

[ ... ]

> @@ -32,6 +29,7 @@ properties:
>            - qcom,qcs404-ethqos
>            - qcom,sa8775p-ethqos
>            - qcom,sc8280xp-ethqos
> +          - qcom,shikra-ethqos
>            - qcom,sm8150-ethqos

[Severity: Low]

This is a standalone compatible with no fallback, and at this point in the
series nothing consumes it, the two new NOC clocks, or the mandatory OPP
table.

Checking the end of the series, this is addressed by the later patches:
"net: stmmac: qcom-ethqos: add per-platform NOC clock voting" adds
qcom_ethqos_init_noc_clks() with devm_clk_bulk_get() and
devm_pm_opp_of_add_table(), and "net: stmmac: qcom-ethqos: add Shikra EMAC
support" adds shikra_noc_clks[] plus:

	{ .compatible = "qcom,shikra-ethqos", .data = &shikra_data},

to qcom_ethqos_match[], so the compatible does bind by the end of the
series. Noting it only for ordering visibility.

[ ... ]

> @@ -81,6 +83,11 @@ properties:
>        - const: cpu-mac
>        - const: mac-mem
>  
> +  operating-points-v2: true
> +
> +  opp-table:
> +    type: object
> +

[Severity: Medium]

The OPP properties are added here, but power-domains is never mentioned
anywhere in qcom,ethqos.yaml. It is only permitted through the inherited
$ref to snps,dwmac.yaml, which defines:

  power-domains:
    maxItems: 1

The commit message says the OPP table exists "to vote VDD_CX to SVS when
the NOC clocks are enabled" through required-opps. Can that vote take
effect without a power-domains phandle on the node?

A per-OPP required-opps entry reaches genpd only via:

drivers/base/power/common.c:dev_pm_domain_set_performance_state() {
	if (dev->pm_domain && dev->pm_domain->set_performance_state)
		return dev->pm_domain->set_performance_state(dev, state);

	return 0;
}

With no power-domains phandle, dev->pm_domain is NULL and this returns 0
without doing anything, so dev_pm_opp_set_rate() ->  _set_required_opps()
-> _set_opp_level() silently drops the CX corner while the two 120 MHz NOC
clocks run.

> @@ -92,6 +99,41 @@ required:
>    - clock-names
>    - reg-names
>  
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,shikra-ethqos
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 6
> +        clock-names:
> +          items:
> +            - const: stmmaceth
> +            - const: pclk
> +            - const: ptp_ref
> +            - const: rgmii
> +            - const: axi-noc
> +            - const: pcie-tile-axi-noc
> +      required:
> +        - operating-points-v2

[Severity: Medium]

Should power-domains also be listed in this required block, next to
operating-points-v2?

As written, a Shikra node carrying operating-points-v2 and an OPP table
full of required-opps but no power-domains = <&rpmhpd RPMHPD_CX> passes
dt_binding_check and dtbs_check cleanly, and the schema emits no
diagnostic for the missing phandle.

Grepping drivers/net/ethernet/stmicro/stmmac/ at the end of the series
returns no pm_domain references, so the device tree is the only place the
CX dependency can be expressed.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 4/9] net: stmmac: qcom-ethqos: convert ethqos_rgmii_macro_init() to void

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] net: stmmac: qcom-ethqos: removing the {SPEED_10, SPEED_100,
  SPEED_1000} guard from ethqos_rgmii_macro_init() is not the dead-code…

--

net: stmmac: qcom-ethqos: convert ethqos_rgmii_macro_init() to void

The patch changes ethqos_rgmii_macro_init() to return void because its
only caller ignores the return value, and drops the
{SPEED_10, SPEED_100, SPEED_1000} check together with the now unused
struct device pointer.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index ac7d6d3e205a1..44270c25d874f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -371,9 +371,8 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>  	return 0;
>  }
>  
> -static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
> +static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  {
> -	struct device *dev = &ethqos->pdev->dev;
>  	unsigned int prg_rclk_dly, loopback;
>  	unsigned int phase_shift;
>  
> @@ -384,11 +383,6 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  	/* Select RGMII, write 0 to interface select */
>  	rgmii_clrmask(ethqos, RGMII_CONFIG_INTF_SEL, RGMII_IO_MACRO_CONFIG);
>  
> -	if (speed != SPEED_1000 && speed != SPEED_100 && speed != SPEED_10) {
> -		dev_err(dev, "Invalid speed %d\n", speed);
> -		return -EINVAL;
> -	}
> -

[Severity: Medium]
Which check higher up the call stack does this duplicate?  The commit
message says "the speed validation duplicates a check higher up the call
stack", but the only speed check above ethqos_fix_mac_speed_rgmii() appears
to be the switch in stmmac_mac_link_up(), which accepts speeds well above
1000:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_link_up() {
	...
	case SPEED_2500:
		if (interface == PHY_INTERFACE_MODE_USXGMII)
			ctrl |= priv->hw->link.xgmii.speed2500;
		else
			ctrl |= priv->hw->link.speed2500;
		break;
	...
	default:
		netdev_err(priv->dev,
			   "unsupported speed %s on %s, leaving the MAC disabled\n",
			   phy_speed_to_str(speed), phy_modes(interface));
		return;
	}

	if (priv->plat->fix_mac_speed)
		priv->plat->fix_mac_speed(priv->plat->bsp_priv, interface,
					  speed, mode);
	...
}

So SPEED_2500 and above reach fix_mac_speed() unfiltered, and
ethqos_fix_mac_speed_rgmii() passes speed straight into
ethqos_rgmii_macro_init() without a check of its own.

The other speed validation in this driver, rgmii_clock() called from
ethqos_set_clk_tx_rate(), runs from priv->plat->set_clk_tx_rate() later in
stmmac_mac_link_up(), i.e. after fix_mac_speed(), so it also cannot keep an
out-of-range speed out of the macro programming.

Does the RGMII limit actually come from phylink's per-interface capability
filtering rather than from the call stack?  The fixed-link path looks like
it bypasses that filtering:

drivers/net/phy/phylink.c:phylink_parse_fixedlink() {
	...
	pl->link_config.speed = speed;
	...
	phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n", ...);
	...
}

The phylink_validate() result there is only used for the warning, and
phylink_get_fixed_state() then copies the stored config verbatim:

drivers/net/phy/phylink.c:phylink_get_fixed_state() {
	...
	*state = pl->link_config;
	...
}

which phylink_link_up() hands to mac_link_up() unchanged.

With phy-mode = "rgmii" (or rgmii-id/rxid/txid, all of which install
ethqos_fix_mac_speed_rgmii) and a fixed-link node specifying
speed = <2500>, ethqos_rgmii_macro_init() is now entered with speed ==
2500.  Before this patch it logged "Invalid speed 2500" and returned after
only the loopback-disable and INTF_SEL writes.  Now it runs to completion
and applies the full non-1000 configuration (DDR_MODE set, BYPASS_TX_ID_EN
set, PROG_SWAP and POS_NEG_DATA_SEL cleared, EXT_PRG_RCLK_DLY_CODE = 5,
RX_PROG_SWAP per has_emac_ge_3) while programming neither MAX_SPD_PRG_2 nor
MAX_SPD_PRG_9, so the speed divider keeps its power-on value and there is
no diagnostic left.

Would it make sense to either keep the check, or describe this behaviour
change and the loss of the diagnostic in the commit message?  Later commits
in this area do not reintroduce validation either: as of
70ec5f4a19736338086794f26222dcb9ec15b24c neither
ethqos_rgmii_macro_init() nor the newly added ethqos_rgmii_id_macro_init()
validates the speed.

>  	rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
>  
>  	if (speed == SPEED_1000) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 5/9] net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 1

- [High] ethqos_set_clk_tx_rate() applies the new DLL-bypass (no-MAC-
  delay) link clock rate to every RGMII variant except plain "rgmii",…
- [Low] The patch is titled "fix RGMII_ID mode to use DLL bypass" and the
  changelog states the driver "currently falls through to the generic…

Pre-existing issues:
- [Medium] For PHY_INTERFACE_MODE_RGMII_RXID the PHY supplies the RX
  delay, yet that mode still takes the legacy path in…

--

net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass

For rgmii-id the PHY supplies both delays, so this powers down the DLL,
sets DDR bypass and programs the IO_MACRO through a new
ethqos_rgmii_id_macro_init() helper.  It also changes the link clock rate
calculation in ethqos_set_clk_tx_rate() and drops RGMII_ID from the
phase-shift suppression in ethqos_rgmii_macro_init().

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 44270c25d874f..67738c1598c50 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> @@ -183,7 +186,15 @@ static int ethqos_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
>  	if (rate < 0)
>  		return rate;
>  
> -	return clk_set_rate(ethqos->link_clk, rate * 2);
> +	/* Clock Rate Requirements:
> +	 * MAC added delay: 250/50/5 Mhz for 1G/100M/10M
> +	 * No MAC delay (DLL bypass): 250/25/2.5 Mhz for 1G/100M/10M
> +	 */
> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII ||
> +	    speed == SPEED_1000)
> +		rate *= 2;
> +
> +	return clk_set_rate(ethqos->link_clk, rate);
>  }

[Severity: High]
Should this predicate match the one that actually selects the bypass
hardware configuration?

Here the un-doubled (bypass) rate is applied to every RGMII variant
except plain "rgmii", so rgmii-txid and rgmii-rxid also get the
un-doubled rate at 100M and 10M:

  ethqos_set_clk_tx_rate()
     if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII ||
         speed == SPEED_1000)
             rate *= 2;

But in ethqos_fix_mac_speed_rgmii() only PHY_INTERFACE_MODE_RGMII_ID
takes the new early return into the bypass configuration:

  if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) {
          rgmii_setmask(ethqos, SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
          rgmii_setmask(ethqos, SDCC_USR_CTL_DDR_BYPASS, SDCC_USR_CTL);
          ethqos_rgmii_id_macro_init(ethqos, speed);
          return;
  }

rgmii-txid and rgmii-rxid still run the full DLL init and
ethqos_rgmii_macro_init(), which sets RGMII_CONFIG_DDR_MODE and programs
PRG_RCLK_DLY, i.e. the "MAC added delay" configuration that the new
comment says needs 50 MHz at 100M and 5 MHz at 10M.  Those modes now get
25 MHz and 2.5 MHz instead, where before the patch the rate was always
doubled.

arch/arm64/boot/dts/qcom/sa8540p-ride.dts uses phy-mode = "rgmii-txid"
on both EMAC nodes, so is this a regression for that board whenever the
link comes up at 100 Mbps or 10 Mbps?

Would something like the following keep the two halves consistent?

  if (ethqos->phy_mode != PHY_INTERFACE_MODE_RGMII_ID ||
      speed == SPEED_1000)
          rate *= 2;

The condition is still unchanged at the tip of the series
(70ec5f4a19736338086794f26222dcb9ec15b24c); the later commit
"net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes" only adds a
dev_warn() on that path and does not restore the doubled rate.

>  
>  static void
> @@ -405,8 +416,7 @@ static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  		      RGMII_IO_MACRO_CONFIG2);
>  
>  	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>  		phase_shift = 0;
>  	else
>  		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;

[Severity: Medium]
This isn't a problem introduced by this patch, but was rgmii-rxid
considered while reworking this?

qcom_ethqos_probe() accepts it and routes it to the same handler:

  case PHY_INTERFACE_MODE_RGMII_RXID:
  case PHY_INTERFACE_MODE_RGMII_TXID:
          plat_dat->fix_mac_speed = ethqos_fix_mac_speed_rgmii;

and ethqos_rgmii_macro_init() programs the MAC RX delay regardless of
phy-mode:

  rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
                FIELD_PREP(SDCC_DDR_CONFIG_PRG_RCLK_DLY,
                           prg_rclk_dly), SDCC_HC_REG_DDR_CONFIG);

  rgmii_setmask(ethqos, SDCC_DDR_CONFIG_PRG_DLY_EN,
                SDCC_HC_REG_DDR_CONFIG);

(and SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE = 5 plus the EXT_PRG_RCLK_DLY
bits at 100M/10M).  For rgmii-rxid the PHY already supplies the RX delay,
which is the same class of issue the patch fixes on the rgmii-id side.
No in-tree qcom DTS uses "rgmii-rxid", so only out-of-tree boards would
see it.  Could the changelog say whether rgmii-rxid was evaluated and
left alone deliberately?

[ ... ]

> @@ -493,6 +539,21 @@ static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
>  
>  	ethqos_set_func_clk_en(ethqos);
>  
> +	/* For rgmii-id mode, the PHY should add the required delays.
> +	 * Therefore, power down the DLL and program it in bypass mode.
> +	 * Program the IO_MACRO as per the settings recommended by the
> +	 * programming guide for bypass mode. This will ensure that the
> +	 * MAC core doesn't add any additional delays.
> +	 */
> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) {
> +		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
> +		rgmii_setmask(ethqos, SDCC_USR_CTL_DDR_BYPASS, SDCC_USR_CTL);
> +
> +		ethqos_rgmii_id_macro_init(ethqos, speed);
> +
> +		return;
> +	}
> +

[Severity: Low]
The subject says "fix RGMII_ID mode to use DLL bypass" and the changelog
describes a user-visible timing misconfiguration, but the commit carries
only a Signed-off-by, with no Fixes: tag and no stable Cc.

Would this one be appropriate?

  Fixes: 164a9ebe9742 ("net: stmmac: dwmac-qcom-ethqos: Respect phy-mode and TX delay")

That commit introduced the per-phy-mode delay handling for rgmii-id and
rgmii-txid while leaving the DLL/RX delay path enabled for rgmii-id.

>  	/* Initialize the DLL first */
>  
>  	/* Set DLL_RST */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 6/9] net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] Unbounded log spam: the new dev_warn("legacy RGMII phy-mode
  detected; consider upgrading to a newer DTB") is placed in the body of…
- [Medium] The warning fires for a PHY mode the commit message never
  mentions.

--

net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes

This adds a dev_warn() in ethqos_fix_mac_speed_rgmii() for the RGMII
configuration paths that are not rgmii-id, to encourage migration to
DTBs using "rgmii-id" with PHY-provided delays.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 67738c1598c50..2ca2f03de3017 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -554,6 +554,8 @@ static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
>  		return;
>  	}
>  
> +	dev_warn(dev, "legacy RGMII phy-mode detected; consider upgrading to a newer DTB\n");

[Severity: Medium]
Can this print repeat without bound?  ethqos_fix_mac_speed_rgmii() is
installed as plat_dat->fix_mac_speed in qcom_ethqos_probe():

  ethqos->phy_mode = plat_dat->phy_interface;
  switch (ethqos->phy_mode) {
  case PHY_INTERFACE_MODE_RGMII:
  case PHY_INTERFACE_MODE_RGMII_ID:
  case PHY_INTERFACE_MODE_RGMII_RXID:
  case PHY_INTERFACE_MODE_RGMII_TXID:
          plat_dat->fix_mac_speed = ethqos_fix_mac_speed_rgmii;

and stmmac_mac_link_up() calls it on every resolved link:

  if (priv->plat->fix_mac_speed)
          priv->plat->fix_mac_speed(priv->plat->bsp_priv, interface,
                                    speed, mode);

The only early return before the new print is the
phy_mode == PHY_INTERFACE_MODE_RGMII_ID branch, so for the other RGMII
modes every link-up, cable re-plug, link flap, autoneg speed change,
ifdown/ifup and resume emits another identical warning.  The condition
being reported is the static DT phy-mode, which is already known at probe
time and does not change afterwards.

Would dev_warn_once(), or a single check in qcom_ethqos_probe() where the
phy_mode switch already exists, be preferable here?  dev_warn_ratelimited()
would at least bound the output.

[Severity: Medium]
The commit message says:

  "However, existing DTBs use "rgmii" and "rgmii-txid" and must remain
   supported for backwards compatibility."

  "Warn when either of these legacy PHY modes is used [...]"

but the code warns for anything reaching ethqos_fix_mac_speed_rgmii()
that is not PHY_INTERFACE_MODE_RGMII_ID, which also covers
PHY_INTERFACE_MODE_RGMII_RXID, dispatched to this callback by the switch
in qcom_ethqos_probe().

Documentation/devicetree/bindings/net/ethernet-controller.yaml lists
rgmii-rxid as the correct description when the TX delay is provided by the
PCB:

      # TX delay is provided by the PCB. See below
      - rgmii-rxid

So a board that legitimately describes itself as rgmii-rxid is now told to
move to "rgmii-id", which would misdescribe the hardware.

Should the condition be narrowed to the two modes named in the commit
message, or should the commit message explain why rgmii-rxid is deprecated
too?

>  	/* Initialize the DLL first */
>  
>  	/* Set DLL_RST */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 7/9] net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 1 · Low: 0

- [High] Lowering the probe-time link clock to the SPEED_10 rate makes
  the first link-up's RGMII IO-macro/DLL programming run with a…
- [High] The RGMII source-clock rate no longer matches the programmed IO-
  macro/DLL delay mode for phy-mode "rgmii-txid" and "rgmii-rxid" at…
- [Medium] The commit message's safety argument, "fix_mac_speed updates
  the rate once a link is established", describes a mechanism that no…

--

net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed

The probe-time initialisation of the RGMII link clock is changed from
SPEED_1000 to SPEED_10, so the source clock starts at the 10 Mbps rate
instead of 250 MHz while no link is present.

> net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed
>
> On probe the RGMII link clock is initialised at SPEED_1000, which
> translates to a 250 MHz source clock even when no PHY link is present,
> drawing unnecessary power.
>
> Initialise at SPEED_10 instead; fix_mac_speed updates the rate once
> a link is established.

[Severity: Medium]
Is the sentence "fix_mac_speed updates the rate once a link is established"
accurate for this driver?

Neither ethqos_fix_mac_speed_rgmii() nor ethqos_fix_mac_speed_sgmii()
contains a clk_set_rate() call.  The only place the link clock rate is
programmed is ethqos_set_clk_tx_rate():

	return clk_set_rate(ethqos->link_clk, rate);

which is installed as plat_dat->set_clk_tx_rate in qcom_ethqos_probe().

stmmac_mac_link_up() calls the two callbacks in this order:

	if (priv->plat->fix_mac_speed)
		priv->plat->fix_mac_speed(priv->plat->bsp_priv, interface,
					  speed, mode);
	...
	if (priv->plat->set_clk_tx_rate) {
		ret = priv->plat->set_clk_tx_rate(priv->plat->bsp_priv,
						priv->plat->clk_tx_i,
						interface, speed);

So the rate is updated by set_clk_tx_rate(), and it happens after
fix_mac_speed() rather than before it.  Could the changelog be reworded to
match the callback that actually applies the rate, and the ordering it runs
in?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 2ca2f03de3017..4f9bef662c579 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -814,7 +814,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  				     "Failed to get serdes phy\n");
>  
>  	ethqos_set_clk_tx_rate(ethqos, NULL, plat_dat->phy_interface,
> -			       SPEED_1000);
> +			       SPEED_10);

[Severity: High]
Does this leave the first link-up's DLL programming running at the 10 Mbps
reference rate on the legacy RGMII phy-modes?

Because stmmac_mac_link_up() runs fix_mac_speed() before set_clk_tx_rate(),
ethqos_fix_mac_speed_rgmii() always executes with the previously programmed
link_clk rate.  After this change that rate is, on the first link event,
whatever ethqos_set_clk_tx_rate() computed for SPEED_10: 5 MHz for
phy-mode "rgmii" and 2.5 MHz for "rgmii-txid"/"rgmii-rxid", instead of the
250 MHz it was before.

Only rgmii-id takes the bypass early return in
ethqos_fix_mac_speed_rgmii():

	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) {
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
		rgmii_setmask(ethqos, SDCC_USR_CTL_DDR_BYPASS, SDCC_USR_CTL);

		ethqos_rgmii_id_macro_init(ethqos, speed);

		return;
	}

rgmii, rgmii-txid and rgmii-rxid fall through to the DLL enable and lock
poll, and at 1G also to ethqos_dll_configure():

	/* wait for DLL LOCK */
	if (read_poll_timeout_atomic(rgmii_readl, val,
				     val & SDC4_STATUS_DLL_LOCK,
				     1000, 1000000, true,
				     ethqos, SDC4_STATUS))
		dev_err(dev, "Timeout while waiting for DLL lock\n");

	if (speed == SPEED_1000)
		ethqos_dll_configure(ethqos);

ethqos_dll_configure() programs a calibration constant that is explicitly a
function of the MCLK frequency:

	rgmii_updatel(ethqos, SDCC_DLL_CONFIG2_MCLK_FREQ_CALC,
		      FIELD_PREP(SDCC_DLL_CONFIG2_MCLK_FREQ_CALC, 26),
		      SDCC_HC_REG_DLL_CONFIG2);

Can the DLL lock and calibrate at 2.5/5 MHz?  A lock timeout is only logged
with dev_err() and the link is brought up anyway, and nothing re-runs the
DLL sequence after set_clk_tx_rate() raises the rate, so the calibration
performed at the stale rate is the one that stays in effect.  There is also
up to ~3 seconds of busy-wait from the three 1 s read_poll_timeout_atomic()
budgets in this path.

arch/arm64/boot/dts/qcom/sa8540p-ride.dts uses phy-mode = "rgmii-txid" on
both ethqos ports (ethernet1 with a fixed-link at 1000), so the very first
link-up on that board takes this path.  Would raising the rate before
calling fix_mac_speed(), or restricting this initialisation change to the
DLL-bypass mode, avoid that?

[Severity: High]
Separately, does the rate computed here match the delay mode that
ethqos_fix_mac_speed_rgmii() programs for rgmii-txid and rgmii-rxid?

ethqos_set_clk_tx_rate() doubles the rate only for plain RGMII or at 1G:

	/* Clock Rate Requirements:
	 * MAC added delay: 250/50/5 Mhz for 1G/100M/10M
	 * No MAC delay (DLL bypass): 250/25/2.5 Mhz for 1G/100M/10M
	 */
	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII ||
	    speed == SPEED_1000)
		rate *= 2;

But DDR bypass is only programmed for PHY_INTERFACE_MODE_RGMII_ID, so
rgmii-txid and rgmii-rxid stay in MAC-added-delay mode and, per the comment
above, need 5 MHz at 10M and 50 MHz at 100M rather than 2.5/25 MHz.

Before this series the driver did clk_set_rate(link_clk, rate * 2)
unconditionally, which matched every mode reaching the legacy path; the
condition was narrowed by "net: stmmac: qcom-ethqos: fix RGMII_ID mode to
use DLL bypass" and is unchanged at the end of the series.  Can 10/100 Mbps
links pass traffic on sa8540p-ride (phy-mode "rgmii-txid") with a
half-frequency RGMII TXC?  This patch routes the probe-time initialisation
through the same calculation, so such boards are left at the bypass 10M
rate instead of the MAC-delay 10M rate.

>  
>  	qcom_ethqos_set_sgmii_loopback(ethqos, true);
>  	ethqos_set_func_clk_en(ethqos);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 8/9] net: stmmac: qcom-ethqos: add per-platform NOC clock voting

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 2 (1)

- [High] Incorrect power sequencing order in ethqos_clks_config() error
  path drops OPP performance state before disabling dependent clock.
- [Medium] Missing PM_OPP Kconfig dependency and undisclosed OPP/DT
  requirement: qcom_ethqos_init_noc_clks()…
- [Medium] Probe-time OPP/performance-state vote is never withdrawn on
  later probe failures.
- [Low] ETHQOS_MAX_NOC_CLKS is introduced but never enforced:
  qcom_ethqos_init_noc_clks() (dwmac-qcom-ethqos.c:809-815) copies…
- [Low] Concern that the added NOC-clock/OPP infrastructure (struct
  ethqos_noc_clk_cfg, the new driver-data and runtime fields,…

Pre-existing issues:
- [Low] Concern that
  Documentation/devicetree/bindings/net/qcom,ethqos.yaml documents…

--

net: stmmac: qcom-ethqos: add per-platform NOC clock voting

Adds struct ethqos_noc_clk_cfg plus driver-data and runtime fields so each
compatible can declare its own set of System NOC clocks with per-clock
rates.  The clocks are acquired in probe via qcom_ethqos_init_noc_clks()
and enabled/disabled next to the link clock in ethqos_clks_config(), with
the OPP framework used to carry the VDD_CX performance state.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 4f9bef662c579..a31541506a4c9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -7,6 +7,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy.h>
>  #include <linux/phy/phy.h>
> +#include <linux/pm_opp.h>
>  
>  #include "stmmac.h"
>  #include "stmmac_platform.h"
> @@ -84,11 +85,18 @@
>  
>  #define SGMII_10M_RX_CLK_DVDR			0x31
>  
> +#define ETHQOS_MAX_NOC_CLKS			3
> +
>  struct ethqos_emac_por {
>  	unsigned int offset;
>  	unsigned int value;
>  };
>  
> +struct ethqos_noc_clk_cfg {
> +	const char *id;
> +	unsigned long rate;
> +};
> +
>  struct ethqos_emac_driver_data {
>  	const struct ethqos_emac_por *rgmii_por;
>  	unsigned int num_rgmii_por;
> @@ -98,6 +106,8 @@ struct ethqos_emac_driver_data {
>  	const char *link_clk_name;
>  	struct dwmac4_addrs dwmac4_addrs;
>  	bool needs_sgmii_loopback;
> +	const struct ethqos_noc_clk_cfg *noc_clk_cfg;
> +	unsigned int num_noc_clks;
>  };
>  
>  struct qcom_ethqos {
> @@ -112,6 +122,10 @@ struct qcom_ethqos {
>  	bool rgmii_config_loopback_en;
>  	bool has_emac_ge_3;
>  	bool needs_sgmii_loopback;
> +
> +	struct clk_bulk_data noc_clks[ETHQOS_MAX_NOC_CLKS];
> +	unsigned long noc_clk_rates[ETHQOS_MAX_NOC_CLKS];
> +	int num_noc_clks;
>  };
>  
>  static u32 rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
> @@ -691,15 +705,51 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  static int ethqos_clks_config(void *priv, bool enabled)
>  {
>  	struct qcom_ethqos *ethqos = priv;
> +	unsigned int i;
>  	int ret = 0;
>  
>  	if (enabled) {
> +		if (ethqos->num_noc_clks) {

[Severity: Low]
This isn't a bug, but as submitted every new block here and in
qcom_ethqos_init_noc_clks() is gated on ethqos->num_noc_clks, and no
ethqos_emac_driver_data entry in this commit sets .noc_clk_cfg or
.num_noc_clks, so none of the new code runs yet.  The consumer arrives
later in the series with "net: stmmac: qcom-ethqos: add Shikra EMAC
support", which adds shikra_noc_clks[] and
{ .compatible = "qcom,shikra-ethqos", .data = &shikra_data }, so the
infrastructure does get exercised by the end of the series.  Was the split
intentional?

> +			ret = dev_pm_opp_set_rate(&ethqos->pdev->dev,
> +						  ethqos->noc_clk_rates[0]);
> +			if (ret) {
> +				dev_err(&ethqos->pdev->dev,
> +					"NOC OPP rate set failed: %d\n", ret);
> +				return ret;
> +			}
> +

[Severity: High]
The unwind order here looks inverted with respect to the disable
path added a few lines below.  When clk_bulk_prepare_enable() on
ethqos->noc_clks fails, this drops the performance state with
dev_pm_opp_set_rate(&ethqos->pdev->dev, 0) first and only then calls
clk_disable_unprepare(ethqos->link_clk), so between the two calls the
link clock is still running while the VDD_CX vote taken by the earlier
dev_pm_opp_set_rate(dev, ethqos->noc_clk_rates[0]) has already been
released.  The !enabled branch of the same function does it the other
way around - clk_bulk_disable_unprepare(), clk_disable_unprepare(link_clk),
then dev_pm_opp_set_rate(dev, 0) - which is the sequence I would expect
if the clocks require the vote to be held while they are ungated.

The same concern applies to the clk_prepare_enable(ethqos->link_clk)
failure just above, although there nothing else is enabled yet so the
ordering is moot.

Is the ordering in the error path deliberate, or should these two calls
be swapped so that every unwind mirrors the disable sequence?  If the
hardware genuinely does not care about the relative order of the vote
and the link clock, it would still be worth keeping the error path and
the disable path identical so the intended sequencing is obvious to the
next reader.

[ ... ]

> @@ -736,6 +791,46 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
>  	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
>  }
>  
> +/* Some SoCs gate NOC access behind dedicated clocks. Acquire them here
> + * so ethqos_clks_config() can enable/disable them at runtime. The OPP
> + * table is used to propagate the required VDD_CX performance state via
> + * dev_pm_opp_set_rate().
> + */
> +static int qcom_ethqos_init_noc_clks(struct qcom_ethqos *ethqos,
> +				     const struct ethqos_emac_driver_data *data)
> +{
> +	struct device *dev = &ethqos->pdev->dev;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!data->num_noc_clks)
> +		return 0;
> +
> +	for (i = 0; i < data->num_noc_clks; i++) {
> +		ethqos->noc_clks[i].id = data->noc_clk_cfg[i].id;
> +		ethqos->noc_clk_rates[i] = data->noc_clk_cfg[i].rate;
> +	}
> +	ethqos->num_noc_clks = data->num_noc_clks;

[Severity: Low]
Should this loop check data->num_noc_clks against ETHQOS_MAX_NOC_CLKS?
The macro is added by this patch but never enforced anywhere, while the
destinations are fixed-size members of the devm_kzalloc'd struct:

	struct clk_bulk_data noc_clks[ETHQOS_MAX_NOC_CLKS];
	unsigned long noc_clk_rates[ETHQOS_MAX_NOC_CLKS];

Today the only table added by the series (shikra_noc_clks[]) has exactly
three entries, so nothing overflows noc_clks[] or noc_clk_rates[] right
now.  A future driver-data entry with four clocks would write past both
arrays, and a table that sets .num_noc_clks without .noc_clk_cfg would
dereference NULL at data->noc_clk_cfg[i].  Would a static_assert() on each
table, or an explicit

	if (data->num_noc_clks > ETHQOS_MAX_NOC_CLKS)
		return -EINVAL;

be worth adding here?

> +
> +	ret = devm_clk_bulk_get(dev, ethqos->num_noc_clks, ethqos->noc_clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get NOC clocks\n");
> +
> +	ret = devm_pm_opp_set_clkname(dev, data->noc_clk_cfg[0].id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set OPP clock name\n");
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add OPP table\n");

[Severity: Medium]
Does this need a Kconfig dependency on PM_OPP?

With CONFIG_PM_OPP=n, include/linux/pm_opp.h provides stubs that return
-EOPNOTSUPP for devm_pm_opp_set_clkname() (via devm_pm_opp_set_config()),
devm_pm_opp_of_add_table() and dev_pm_opp_set_rate(), and each call here
is turned into a probe failure by dev_err_probe().

config DWMAC_QCOM_ETHQOS in drivers/net/ethernet/stmicro/stmmac/Kconfig
currently has only:

	depends on OF && (ARCH_QCOM || COMPILE_TEST)

and config PM_OPP in drivers/opp/Kconfig is a promptless "bool", i.e.
select-only, so PM_OPP=n is a legal configuration for this driver.  In
that build, any compatible declaring NOC clocks would fail to probe and
no netdev would be created.  Should DWMAC_QCOM_ETHQOS select PM_OPP, or
should these calls be made conditional?

Also, devm_pm_opp_of_add_table() makes an operating-points-v2 node a hard
requirement for such compatibles.  The commit message says:

  "The clocks are acquired during probe and enabled/disabled alongside
   the existing link clock in ethqos_clks_config()."

Could the new mandatory OPP table / device tree requirement be mentioned
there too?

> +
> +	ret = dev_pm_opp_set_rate(dev, data->noc_clk_cfg[0].rate);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set initial NOC OPP rate\n");
> +
> +	return 0;
> +}
> +
>  static int qcom_ethqos_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -795,6 +890,12 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos->has_emac_ge_3 = data->has_emac_ge_3;
>  	ethqos->needs_sgmii_loopback = data->needs_sgmii_loopback;
>  
> +	if (data->num_noc_clks) {
> +		ret = qcom_ethqos_init_noc_clks(ethqos, data);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
>  	if (IS_ERR(ethqos->link_clk))
>  		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),

[Severity: Medium]
Can the performance-state vote taken by the dev_pm_opp_set_rate() at the
end of qcom_ethqos_init_noc_clks() leak if probe fails right after?

The only code that drops it is ethqos_clks_config(priv, false) with its
dev_pm_opp_set_rate(dev, 0), and that path is installed only later in
probe:

qcom_ethqos_probe()
	qcom_ethqos_init_noc_clks()	/* dev_pm_opp_set_rate(dev, rate) */
	devm_clk_get(dev, "rgmii")	/* can return -EPROBE_DEFER */
	ethqos_clks_config(ethqos, true)
	devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos)

If devm_clk_get() defers or ethqos_clks_config(true) fails, probe returns
before ethqos_clks_disable is registered.  Devres teardown does not appear
to compensate: _opp_table_kref_release() and _opp_clear_config() in
drivers/opp/core.c do not call _set_opp_level(dev, NULL) or
_set_required_opps(..., NULL), and on the genpd side only the default
pstate is dropped:

drivers/pmdomain/core.c:genpd_dev_pm_detach() {
	...
	/* Drop the default performance state */
	if (dev_gpd_data(dev)->default_pstate) {
		dev_pm_genpd_set_performance_state(dev, 0);
		dev_gpd_data(dev)->default_pstate = 0;
	}
	...
}

so the VDD_CX vote requested through the OPP would stay in place for a
device that never bound, and genpd_remove_device() unlinks the device
without re-evaluating genpd->performance_state.

Is this initial dev_pm_opp_set_rate() needed at all?
ethqos_clks_config(ethqos, true) applies the same rate a few lines later
and _set_opp() early-returns when the OPP is unchanged, so dropping it (or
pairing it with its own devm action) would close the window.

[Severity: Low]
This isn't a bug introduced by this patch, but at this point in the series
Documentation/devicetree/bindings/net/qcom,ethqos.yaml already documents
qcom,shikra-ethqos with the axi-noc / pcie-tile-axi-noc clocks and a
required operating-points-v2 property, while qcom_ethqos_match[] has no
matching entry and no driver data setting .noc_clk_cfg / .num_noc_clks.
The pair becomes consistent with the later commit "net: stmmac:
qcom-ethqos: add Shikra EMAC support", which adds both the compatible and
shikra_data, so this is only a transient mismatch inside the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com

Re: [PATCH net-next 9/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] shikra_data (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-
  ethqos.c:337-359) is the only ETHQOS platform descriptor without…
- [Medium] Shikra is the first compatible with a nonzero num_noc_clks, so
  qcom_ethqos_probe() now calls qcom_ethqos_init_noc_clks(), which ends…
- [Medium] The PTP clock rate (`clk_ptp_rate`) is captured from the
  `stmmaceth` clock before its rate is updated to 120MHz, leading to…

--

net: stmmac: qcom-ethqos: add Shikra EMAC support

Adds a qcom,shikra-ethqos compatible backed by a new shikra_data
descriptor.  The descriptor enables three NOC clocks (axi-noc,
pcie-tile-axi-noc, stmmaceth) at 120 MHz and sets a 36-bit DMA address
width.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index a31541506a4c9..f96680456472d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -328,6 +328,36 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
>  	},
>  };
>  
> +static const struct ethqos_noc_clk_cfg shikra_noc_clks[] = {
> +	{ "axi-noc", 120000000 },
> +	{ "pcie-tile-axi-noc", 120000000 },
> +	{ "stmmaceth", 120000000 },
> +};
> +
> +static const struct ethqos_emac_driver_data shikra_data = {
> +	.dma_addr_width = 36,
> +	.has_emac_ge_3 = true,

[Severity: Medium]
Note that this list programs the main MAC clock ("stmmaceth") to
120 MHz, and shikra_data is the first descriptor to do so.  That
interacts badly with how the PTP reference rate is captured earlier in
probe.

qcom_ethqos_probe() calls devm_stmmac_probe_config_dt() before any of
the clock rates are touched, and stmmac_probe_config_dt() has the
fall-back path:

	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
	if (IS_ERR(plat->clk_ptp_ref)) {
		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
		plat->clk_ptp_ref = NULL;
		dev_info(&pdev->dev, "PTP uses main clock\n");

so plat_dat->clk_ptp_rate is a snapshot of whatever rate the bootloader
left on stmmaceth.  Only afterwards do qcom_ethqos_init_noc_clks() and
ethqos_clks_config() move that same clock to the 120 MHz requested
above.  And ethqos_ptp_clk_freq_config() cannot repair the value,
because it bails out first thing when the fall-back was taken:

	if (!plat_dat->clk_ptp_ref)
		return;

Does that leave stmmac using a stale clk_ptp_rate for the sub-second
increment and addend programming, i.e. PTP timestamps that advance at
the wrong rate?  If the boot rate happens to be 0, the check in
stmmac_init_tstamp_counter() rejects it outright ("Invalid PTP clock
rate") and hwtstamping fails.

Admittedly this needs a DT that omits ptp_ref, which the
qcom,shikra-ethqos binding does not allow, so the window is narrow.
Still, since the core explicitly supports the fall-back, it would be
good to know the intent here: is ptp_ref mandatory for Shikra by
construction, or should the stmmaceth rate be established before
devm_stmmac_probe_config_dt() runs (or clk_ptp_rate re-read after the
rate change) so the two cannot disagree?

[Severity: Medium]
Is the missing .rgmii_por / .num_rgmii_por pair intentional here?  This is
the only descriptor in the file without a POR table; emac_v2_1_0_data,
emac_v2_3_0_data, emac_v3_0_0_data and emac_v4_0_0_data all supply a
six-entry table.

Since .link_clk_name is also unset, qcom_ethqos_probe() asks for the clock
named "rgmii":

	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");

so this instance takes the ethqos_fix_mac_speed_rgmii() path, where the
POR restore loop runs zero times:

	/* Reset to POR values and enable clk */
	for (i = 0; i < ethqos->num_rgmii_por; i++)
		rgmii_writel(ethqos, ethqos->rgmii_por[i].value,
			     ethqos->rgmii_por[i].offset);

With num_rgmii_por == 0, RGMII_IO_MACRO_CONFIG, RGMII_IO_MACRO_CONFIG2,
SDCC_HC_REG_DLL_CONFIG(2), SDCC_HC_REG_DDR_CONFIG and SDCC_USR_CTL are
never returned to a known baseline before the speed-specific
read-modify-write sequence runs.

Can that leave a stale delay setting across a speed change?  In
ethqos_rgmii_macro_init(), the non-1000 Mbps branch does:

	rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
		      SDCC_HC_REG_DDR_CONFIG);

while the SPEED_1000 branch only programs PRG_RCLK_DLY and sets
PRG_DLY_EN.  Neither branch clears the other branch's enable bit; on the
existing ge_3 platforms the POR write (DDR_CONFIG = 0x80040800, with
EXT_PRG_RCLK_DLY_EN clear) is what clears it.  Does a 10/100 to 1000 Mbps
renegotiation on Shikra therefore end up with the external RX clock delay
path enabled together with the 1 G programmed delay?  The first link-up
would likewise be configured on top of whatever the bootloader left in the
IO macro and DLL.

Alternatively, if Shikra is not an RGMII part, then the "rgmii" link clock
name and the absent .needs_sgmii_loopback look inconsistent with
emac_v4_0_0_data.  The commit message only says "similar to previous
platforms", which does not say which of the two it is.

> +	.noc_clk_cfg = shikra_noc_clks,
> +	.num_noc_clks = ARRAY_SIZE(shikra_noc_clks),

[Severity: Medium]
This is the first descriptor with a nonzero num_noc_clks, so it is also the
first to activate the NOC/OPP voting path in qcom_ethqos_probe().  Can that
leave a performance state vote behind on a probe failure?

qcom_ethqos_init_noc_clks() ends with:

	ret = dev_pm_opp_set_rate(dev, data->noc_clk_cfg[0].rate);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to set initial NOC OPP rate\n");

which programs axi-noc and, through the required-opps, raises the device's
genpd performance state.  The paired teardown is only installed later:

	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
	if (IS_ERR(ethqos->link_clk))
		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
				     "Failed to get link_clk\n");

	ret = ethqos_clks_config(ethqos, true);
	if (ret)
		return ret;

	ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);

So if devm_clk_get() returns -EPROBE_DEFER (rgmii clock provider not yet
registered) or -ENOENT on an incomplete DT, probe returns before
ethqos_clks_disable is registered and dev_pm_opp_set_rate(dev, 0) is never
issued.

Does the devm cleanup undo the vote?  _opp_table_kref_release() in
drivers/opp/core.c drops current_opp, clears the OF table and puts the
clock, but performs no OPP transition:

	if (opp_table->current_opp)
		dev_pm_opp_put(opp_table->current_opp);
	_of_clear_opp_table(opp_table);
	/* Release automatically acquired single clk */
	if (!IS_ERR(opp_table->clk))
		clk_put(opp_table->clk);

And on the genpd side, genpd_dev_pm_detach() only drops the attach-time
default pstate:

	/* Drop the default performance state */
	if (dev_gpd_data(dev)->default_pstate) {
		dev_pm_genpd_set_performance_state(dev, 0);
		dev_gpd_data(dev)->default_pstate = 0;
	}

which is 0 here because the required-opps live on the OPP table entries,
and genpd_remove_device() does not re-evaluate genpd->performance_state.
Does that mean the elevated VDD_CX and 120 MHz axi-noc vote survives with
no driver bound?

A narrower variant is the dev_pm_opp_set_rate() failure inside
ethqos_clks_config(true), which returns the error without lowering the vote
taken in qcom_ethqos_init_noc_clks().

Would it work to register the cleanup action (or call
dev_pm_opp_set_rate(dev, 0)) immediately after the initial vote, or to move
the initial vote into the existing ethqos_clks_config() sequence where
failures already lower it?

> +	.rgmii_config_loopback_en = false,
> +	.dwmac4_addrs = {

[ ... ]

> +	},
> +};
> +

[ ... ]

> @@ -957,6 +987,7 @@ static const struct of_device_id qcom_ethqos_match[] = {
>  	{ .compatible = "qcom,qcs404-ethqos", .data = &emac_v2_3_0_data},
>  	{ .compatible = "qcom,sa8775p-ethqos", .data = &emac_v4_0_0_data},
>  	{ .compatible = "qcom,sc8280xp-ethqos", .data = &emac_v3_0_0_data},
> +	{ .compatible = "qcom,shikra-ethqos", .data = &shikra_data},
>  	{ .compatible = "qcom,sm8150-ethqos", .data = &emac_v2_1_0_data},
>  	{ }
>  };

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com