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

[PATCH net-next v3 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/20261015143849.483339-1-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 9) now uses dev_pm_opp_set_opp() 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 in v3:
 - Remove #ifdef CONFIG_OF from the dp83867_power_on() function. This is
   a result of the recent discussion regarding similar handling in the
   qca808x driver:
   https://lore.kernel.org/netdev/atCSWZQAyqvz62_K@aspen.lan/
 - Clean up the -ENODEV handling in dp83867_power_on() and use a boolean
   to identify if supplies are provided -- Lorenzo.
 - Patch 7's commit message now says set_clk_tx_rate instead of
   fix_mac_speed -- Sashiko.
 - Add a new patch to abstract away the common dwma4_addrs for different
   versions of ethqos rather than duplicating them -- Lorenzo.
 - Use OPP table to set both the required clock rates and power state,
   update ethqos bindings accordingly to specify how the clock rates are
   described in DTS -- Konrad, Lorenzo.
 - Link to v2: https://lore.kernel.org/r/20260908-shikra_ethernet-v2-0-bbe3389d0652@oss.qualcomm.com

Changes in v2:
  - Collected Maxime's Reviewed-by tags.
  - Updated Patch 9's commit message to explain the double enable vote
    on the "stmmaceth" clock. Open to discussion on how best to handle
    this.
  - Wrapped regulator vote for dp83867 inside an #ifdef CONFIG_OF check
    to avoid adding an unnecessary 200 ms delay on systems where
    devm_regulator_get_enable_optional() would return 0. The change now
    mirrors the QCA8081 patch -- Sashiko.
  - Fixed ethqos_set_clk_tx_rate() to double the rate for all PHY modes
    other than RGMII-ID -- Sashiko.
  - Moved warning about the use of non RGMII-ID mode to probe instead
    of fix_mac_speed to avoid spamming during each link up event --
    Sashiko.
  - Fixed error unwind drops OPP before stopping link_clk -- Sashiko.
  - Fixed OPP vote set in probe not cleaned up on error -- Sashiko.
  - Link to v1: https://lore.kernel.org/netdev/20260904-shikra_ethernet-v1-0-a50765996035@oss.qualcomm.com/#t

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):
      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: share common GMAC4 address table
      net: stmmac: qcom-ethqos: add per-platform NOC clock voting
      net: stmmac: qcom-ethqos: add Shikra EMAC support

 .../devicetree/bindings/net/qcom,ethqos.yaml       |  55 ++++-
 .../devicetree/bindings/net/snps,dwmac.yaml        |   2 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |   1 +
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 259 +++++++++++++++++----
 drivers/net/phy/dp83867.c                          |  32 +++
 5 files changed, 297 insertions(+), 52 deletions(-)
---
base-commit: 8b71f1f5a28347cefb1a4711c4eeb492d5c619fb
change-id: 20260903-shikra_ethernet-ae7bee5e804e

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

[PATCH net-next v3 1/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.

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Acked-by: Andrew Davis <afd@ti.com>
Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
I have kept the Acked-by's since the overall premise of the change
remains the same between v2 and v3, please let me know if you would like
me to remove them.
---
 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 a3cd5cb11ea0ac108219e05d990b378483b67d7a..092bc68bb9bc6561e2457f596206453dd84e56fc 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -16,6 +16,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>
 
@@ -730,11 +731,42 @@ 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;
+	bool supplies_enabled = false;
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(supply_names); i++) {
+		ret = devm_regulator_get_enable_optional(dev, supply_names[i]);
+		if (ret == -ENODEV)
+			continue;
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to enable %s supply\n",
+					     supply_names[i]);
+		supplies_enabled = true;
+	}
+
+	/* Datasheet section 6.6 suggests a 200ms post power-up stabilization */
+	if (supplies_enabled)
+		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);
 	if (!dp83867)

-- 
2.34.1

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

[Andrew Lunn]

On Sat, Oct 17, 2026 at 03:15:12PM +0530, Mohd Ayaan Anwar wrote:
> 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.
> 
> Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> Acked-by: Andrew Davis <afd@ti.com>
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

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

    Andrew

Re: [PATCH net-next v3 1/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: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With CONFIG_REGULATOR=n, dp83867_power_on()
  (drivers/net/phy/dp83867.c) sleeps 200 ms on every probe, even when…
- [Medium] The patch makes the driver read four new DT properties
  (vdda-2p5-supply, vdd-1p0-supply, vdda-1p8-supply, vddio-supply)…
- [Low] The commit message says the driver sleeps 200 ms "when any supply
  is newly enabled".

--

net: phy: dp83867: add regulator supply management

This commit adds dp83867_power_on(), which enables up to four optional
supplies (vdda-2p5, vdd-1p0, vdda-1p8, vddio) at the start of
dp83867_probe(). If any supply is present, the driver sleeps for 200 ms
to meet the datasheet's post power-up stabilization requirement.

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index a3cd5cb11ea0a..092bc68bb9bc6 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c

[ ... ]

> @@ -730,11 +731,42 @@ 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",
> +	};

[Severity: Medium]
These names become four new DT properties: vdda-2p5-supply,
vdd-1p0-supply, vdda-1p8-supply and vddio-supply. Should
Documentation/devicetree/bindings/net/ti,dp83867.yaml document them in
the same series?

Right now the binding lists no *-supply properties and ends with:

  unevaluatedProperties: false

The only schema it references, ethernet-controller.yaml, has no supply
properties either. A node that uses the new properties won't validate
against this schema.

ti,dp83867.yaml has no select, so dtbs_check may never apply it to board
DTs. Even so, the properties would still be undocumented ABI.

A related question: should the binding also say that an
ethernet-phy-idXXXX.XXXX compatible is required when these rails are
switched? fwnode_mdiobus_register_phy() only skips the MDIO ID read when
that compatible is present:

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_register_phy() {
    ...
	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);
    ...
}

Without that compatible, get_phy_device() reads the ID from a PHY that
is not powered yet. The phy_device is then never created, so
dp83867_probe() never runs to turn the rails on.

> +	struct device *dev = &phydev->mdio.dev;
> +	bool supplies_enabled = false;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(supply_names); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, supply_names[i]);
> +		if (ret == -ENODEV)
> +			continue;

[Severity: Medium]
What happens here with CONFIG_REGULATOR=n? The stub returns 0, not
-ENODEV:

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

In that case the -ENODEV check never matches and supplies_enabled is set
on the first iteration. fsleep(200000) then runs on every probe, even
when the board describes no supplies.

DP83867_PHY in drivers/net/phy/Kconfig doesn't depend on REGULATOR, so
this config is valid. Each probe attempt, including -EPROBE_DEFER
retries, would add 200 ms per PHY.

This doesn't match the commit message:

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

Would an IS_ENABLED(CONFIG_REGULATOR) check work here? Another option is
devm_regulator_get_optional() followed by an explicit enable, so the
sleep only runs when a real regulator was found.

> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable %s supply\n",
> +					     supply_names[i]);
> +		supplies_enabled = true;

[Severity: Low]
The commit message says:

    When any supply is newly enabled the driver sleeps for 200 ms before
    returning.

Is "newly enabled" accurate? _devm_regulator_get_enable() returns the
result of regulator_enable():

drivers/regulator/devres.c:_devm_regulator_get_enable() {
    ...
	ret = regulator_enable(r);
	if (!ret)
		ret = devm_add_action_or_reset(dev, &regulator_action_disable, r);
    ...
}

That result is also 0 when the rail was already on. This covers
regulator-always-on, regulator-boot-on, a rail enabled by the
bootloader, or a rail shared with another consumer. So the 200 ms delay
runs whenever any supply is described, whatever its earlier state.

The rails are also enabled before dp83867_of_init(), which can fail with
-EPROBE_DEFER:

dp83867_probe()
  dp83867_of_init()
    dp83867_of_init_io_impedance()
      of_nvmem_cell_get() -> -EPROBE_DEFER

On that failure, devres runs regulator_action_disable for each enabled
supply and the PHY loses power.

Does each deferral then power-cycle the PHY and add another 200 ms on
the retry?

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com

[PATCH net-next v3 2/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       | 55 ++++++++++++++++++++--
 .../devicetree/bindings/net/snps,dwmac.yaml        |  2 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
index 423959cb928d945aa3e758a3c803d12bd61ec42b..1167f11adbf30c234186a415004a75a774aa4492 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,46 @@ 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
+        operating-points-v2:
+          description: |
+            The OPP table specifies the rates for the NOC access
+            clocks in "stmmaceth", "axi-noc", "pcie-tile-axi-noc"
+            order.
+      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 2218a492817fd3d864e0bc529b83fdcbb4ed023b..20b1da2f1096c94215902834a1ea014fb4ae44b2 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -75,6 +75,7 @@ properties:
         - qcom,qcs404-ethqos
         - qcom,sa8775p-ethqos
         - qcom,sc8280xp-ethqos
+        - qcom,shikra-ethqos
         - qcom,sm8150-ethqos
         - renesas,r9a06g032-gmac
         - renesas,r9a08g046-gbeth
@@ -585,6 +586,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

Re: [PATCH net-next v3 2/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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says Shikra's EMAC needs an OPP table whose
  required-opps vote VDD_CX to SVS while the NOC clocks are enabled.
- [Low] The commit message says the new if/else block constrains Shikra
  "while leaving existing compatibles unchanged".

--

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

This commit adds the qcom,shikra-ethqos compatible to the qcom,ethqos and
snps,dwmac bindings. It allows up to six clocks by adding axi-noc and
pcie-tile-axi-noc, and adds the operating-points-v2 and opp-table
properties. A new if/else block requires Shikra to have exactly six clocks
and an OPP table.

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

[ ... ]

> @@ -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,46 @@ 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
> +        operating-points-v2:
> +          description: |
> +            The OPP table specifies the rates for the NOC access
> +            clocks in "stmmaceth", "axi-noc", "pcie-tile-axi-noc"
> +            order.
> +      required:
> +        - operating-points-v2

[Severity: Medium]
Should power-domains also be required for Shikra here?

The commit message says the OPP table carries required-opps "to vote
VDD_CX to SVS when the NOC clocks are enabled". That vote only happens if
the device has a power domain attached. Here, power-domains is optional and
only comes in through the snps,dwmac.yaml reference (maxItems: 1).

If power-domains is missing, genpd never attaches the device, so
required_devs[0] stays NULL. _set_required_opps() then skips the entry and
does not return an error:

drivers/opp/core.c:_set_required_opps() {
    ...
		if (devs[index]) {
			required_opp = opp ? opp->required_opps[index] : NULL;

			ret = _set_opp_level(devs[index], required_opp);
    ...
}

As a result, a Shikra node with six clocks and an OPP table but no
power-domains passes dt_binding_check. The driver's ethqos_clks_config()
path through qcom_ethqos_set_opp() still succeeds, but CX is never raised
before the NOC clocks run at their OPP rates.

Other Shikra devices that vote CX through required-opps pair the OPP table
with the power domain. For example, sdhc_1 in
arch/arm64/boot/dts/qcom/shikra.dtsi has:

			power-domains = <&rpmpd RPMPD_VDDCX>;
			operating-points-v2 = <&sdhc1_opp_table>;

Could power-domains be added to the required list for Shikra? Could the
operating-points-v2 description also state that the OPP entries need
required-opps pointing at the CX domain?

> +    else:
> +      properties:
> +        clocks:
> +          maxItems: 4
> +        clock-names:
> +          items:
> +            - const: stmmaceth
> +            - const: pclk
> +            - const: ptp_ref
> +            - enum:
> +                - rgmii
> +                - phyaux
> +

[Severity: Low]
The commit message says the if/else block leaves "existing compatibles
unchanged". Is that accurate?

operating-points-v2 and opp-table are added to the top-level properties, so
they apply to every compatible. This else branch only re-limits clocks and
clock-names.

Before this patch, neither qcom,ethqos.yaml nor snps,dwmac.yaml defined
these properties, so unevaluatedProperties: false rejected them. Now nodes
for qcs404, qcs615, sa8775p, qcs8300, sc8280xp and sm8150 pass validation
with an OPP table, even though those platforms have no NOC clocks.

Should the else branch also include something like this?

        operating-points-v2: false
        opp-table: false

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com

[PATCH net-next v3 3/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.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
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 v3 4/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..53fb9de4d92c7f1b3c51c183064024f06150444d 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_ID ||
+	    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

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

[Andrew Lunn]

On Sat, Oct 17, 2026 at 03:15:15PM +0530, Mohd Ayaan Anwar wrote:
> 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.

How many different boards have you tested this on? How confident are
you there will not be regressions?

Since you are adding a new compatible "qcom,shikra-ethqos", you could
limit the change in behaviour to just this.

	Andrew

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

[Mohd Ayaan Anwar]

On Sat, Oct 17, 2026 at 04:00:27PM +0200, Andrew Lunn wrote:
> On Sat, Oct 17, 2026 at 03:15:15PM +0530, Mohd Ayaan Anwar wrote:
> > 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.
> 
> How many different boards have you tested this on? How confident are
> you there will not be regressions?
> 

Apart from the new compatible, I have tested this on three different
board designs of the QCS615 SoC. The EMAC on it is identical to the
"qcom,qcs404-ethqos" version. Two of the boards have the Microchip
KSZ9031 PHY while the third has the TI DP83867 PHY.

> Since you are adding a new compatible "qcom,shikra-ethqos", you could
> limit the change in behaviour to just this.
> 

That sounds like a better idea. I will restrict the new sequence to just
"qcom,shikra-ethqos" and "qcom,qcs404-ethqos" using something like
data->phy_managed_delays = true. I will change the warning patch to also
check for this boolean, so any users of "qcom,qcs404-ethqos" can be
notified.

The two other RGMII designs ("qcom,qcs404-ethqos" and
"qcom,sc8280xp-ethqos") can continue using the legacy flow.

	Ayaan

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: qcom-ethqos: rgmii_setmask() on multi-bit RGMII_CONFIG2_RSVD_CONFIG15
--

commit 405cfced2fa67ffbf18f898b4b34f3a6a9035507
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

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

This commit fixes PHY_INTERFACE_MODE_RGMII_ID handling in dwmac-qcom-ethqos by
powering down the DLL, enabling DDR bypass mode, adjusting the link clock rate
for 100M/10M speeds, and programming IO_MACRO via a new
ethqos_rgmii_id_macro_init() helper.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 44270c25d874f..53fb9de4d92c7 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)

[ ... ]

> @@ -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)
> +{

[ ... ]

> +	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);

[Severity: Medium]
Should this else branch be setting all 15 bits of RGMII_CONFIG2_RSVD_CONFIG15
in RGMII_IO_MACRO_CONFIG2 when configuring 100M or 10M speeds in
PHY_INTERFACE_MODE_RGMII_ID mode?

RGMII_CONFIG2_RSVD_CONFIG15 is defined as GENMASK(31, 17), where the 15 suffix
denotes the 15-bit width of the field (similar to RGMII_CONFIG_MAX_SPD_PRG_9
and RGMII_CONFIG_MAX_SPD_PRG_2).

Because rgmii_setmask() calls rgmii_updatel(ethqos, mask, mask, offset), it
writes 0x7fff into bits 31:17. Should this instead program a specific value into
that field using rgmii_updatel() and FIELD_PREP(RGMII_CONFIG2_RSVD_CONFIG15, ...)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f@oss.qualcomm.com?part=4

Re: [PATCH net-next v3 4/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 3 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In rgmii-id mode at 100M and 10M, ethqos_set_clk_tx_rate() now
  calls clk_set_rate(link_clk, 25000000) or clk_set_rate(link_clk,…
- [Medium] net: stmmac: qcom-ethqos: rgmii_setmask() on multi-bit
  RGMII_CONFIG2_RSVD_CONFIG15
- [Low] The commit is titled and described as a fix for a defect in the
  current tree, but it has no Fixes: tag.

--

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

In rgmii-id mode, this commit powers down the DLL, sets DDR bypass and
programs the IO_MACRO through a new ethqos_rgmii_id_macro_init() helper,
so the MAC adds no delay of its own. It also stops ethqos_set_clk_tx_rate()
from doubling the link clock at 100M/10M in rgmii-id mode.

> net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass
>
> 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.

[ ... ]

> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

[Severity: Low]
Should this have a Fixes: tag?

Before this patch, rgmii-id went through the generic DLL path in
ethqos_fix_mac_speed_rgmii() and then ethqos_rgmii_macro_init(). Only the
TX phase shift was suppressed there.

The MAC RX delay was still programmed:

  - at 1G: PRG_RCLK_DLY of 57 or 115, plus PRG_DLY_EN
  - at 10/100: EXT_PRG_RCLK_DLY_CODE of 5, plus EXT_PRG_RCLK_DLY and
    EXT_PRG_RCLK_DLY_EN

The PHY adds its own RX delay in this mode, so the RX delay was applied
twice.

A likely candidate is:

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

That commit made rgmii-id handling cover the TX delay only and left the
MAC RX delay in place.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 44270c25d874f..53fb9de4d92c7 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_ID ||
> +	    speed == SPEED_1000)
> +		rate *= 2;
> +
> +	return clk_set_rate(ethqos->link_clk, rate);

[Severity: Medium]
Can qcs404 and sc8280xp actually produce 25 MHz and 2.5 MHz on the link
clock?

On qcs404 the "rgmii" link clock is GCC_ETH_RGMII_CLK. Its parent's
frequency table only has these entries:

drivers/clk/qcom/gcc-qcs404.c:ftbl_emac_clk_src
    5 MHz, 50 MHz, 125 MHz, 250 MHz

On sc8280xp it is GCC_EMAC0_RGMII_CLK, and the lowest entry is 50 MHz:

drivers/clk/qcom/gcc-sc8280xp.c:ftbl_gcc_emac0_rgmii_clk_src
    50 MHz, 125 MHz, 250 MHz

Both branch clocks have CLK_SET_RATE_PARENT. The parent RCGs round up
rather than fail:

drivers/clk/qcom/clk-rcg2.c:clk_rcg2_determine_rate() {
	...
	return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, CEIL);
}

So in rgmii-id mode at 100M/10M, the request would silently become 50 MHz
or 5 MHz on qcs404, and 50 MHz on sc8280xp. clk_set_rate() would still
return 0.

Below 1G, ethqos_rgmii_id_macro_init() clears DDR_MODE and
TX_CLK_PHASE_SHIFT_EN and sets RSVD_CONFIG15, which assumes the 1x clock.
That gives a link clock at 2x the required rate at 100M on both SoCs and at
10M on qcs404. At 10M on sc8280xp it is 20x. No error is reported in any
of these cases.

A later patch in this series, "net: stmmac: qcom-ethqos: warn about legacy
RGMII PHY modes", tells users of these boards (qcs404-evb-4000 and
sa8540p-ride) to move to rgmii-id DTs, which puts them on this path.
Shikra, sm8150, sc8180x and qcs615 do have 25 MHz and 2.5 MHz entries.

Should the rate actually applied be checked here, for example with
clk_get_rate() after clk_set_rate()? Or does rgmii-id below 1G need
different handling on these SoCs?

[ ... ]

> @@ -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)

[Severity: Medium]
Does the programming guide really expect all of bits 31:17 in
RGMII_IO_MACRO_CONFIG2 to be set at 100M and 10M?

RGMII_CONFIG2_RSVD_CONFIG15 is GENMASK(31, 17), which is a 15-bit
field. The suffix looks like the field width, as it is for
RGMII_CONFIG_MAX_SPD_PRG_9 (GENMASK(16, 8)) and
RGMII_CONFIG_MAX_SPD_PRG_2 (GENMASK(7, 6)).

rgmii_setmask() is just rgmii_updatel(ethqos, mask, mask, offset). So
below 1G, ethqos_rgmii_id_macro_init() writes 0x7fff into this field,
which is 0xfffe0000 in RGMII_IO_MACRO_CONFIG2.

The existing ethqos_rgmii_macro_init() path only ever clears this
field, so this is the first place a non-zero value is written to it.

If the hardware expects a specific value there, for example 1, then
10M/100M links in rgmii-id mode would run with every reserved bit in
that range set. The effect of that is unknown, and it could break
those link speeds.

Should this be rgmii_updatel() with
FIELD_PREP(RGMII_CONFIG2_RSVD_CONFIG15, val) for the value the guide
gives? If all ones really is what the guide asks for, could you add a
comment saying so? Otherwise it reads like a single-bit setmask applied
to a multi-bit field by mistake.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com

[PATCH net-next v3 5/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 | 4 +++-
 1 file changed, 3 insertions(+), 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 53fb9de4d92c7f1b3c51c183064024f06150444d..a92bf4a6a2dbd987893bd4a3890df770bb7a7e56 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -764,9 +764,11 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	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:
+		dev_warn(dev, "legacy RGMII phy-mode detected; consider upgrading to a newer DTB\n");
+		fallthrough;
+	case PHY_INTERFACE_MODE_RGMII_ID:
 		plat_dat->fix_mac_speed = ethqos_fix_mac_speed_rgmii;
 		break;
 	case PHY_INTERFACE_MODE_2500BASEX:

-- 
2.34.1

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new dev_warn() in qcom_ethqos_probe() calls "rgmii",
  "rgmii-rxid" and "rgmii-txid" legacy and tells the user to "consider…
- [Low] The commit message and the code disagree on which modes get the
  warning.
- [Low] In qcom_ethqos_probe() the legacy-mode dev_warn() runs early
  (line 769), before several steps that can return -EPROBE_DEFER: -…

--

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

This patch makes qcom_ethqos_probe() print a warning when the device tree
uses "rgmii", "rgmii-rxid" or "rgmii-txid". The aim is to move boards over
to "rgmii-id" with PHY-provided delays. The RGMII_ID case is moved below
the warning, so it still gets the same fix_mac_speed setup but does not
warn.

> 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".

[Severity: Low]
This isn't a bug, but the commit message names two legacy modes and the
code warns for three. In qcom_ethqos_probe(), PHY_INTERFACE_MODE_RGMII_RXID
is still in the same case group as RGMII and RGMII_TXID, so "rgmii-rxid"
prints the warning too.

No in-tree qcom DTS uses "rgmii-rxid", so the "existing DTBs" reason does
not cover it.

Could the commit message mention rgmii-rxid and say why it counts as
legacy?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 53fb9de4d92c7..a92bf4a6a2dbd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -764,9 +764,11 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	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:
> +		dev_warn(dev, "legacy RGMII phy-mode detected; consider upgrading to a newer DTB\n");

[Severity: Medium]
Is there a newer DTB for users to upgrade to? Every in-tree RGMII user of
this driver still uses one of the modes that now warn:

  arch/arm64/boot/dts/qcom/qcs404-evb-4000.dts:  phy-mode = "rgmii";
  arch/arm64/boot/dts/qcom/sa8155p-adp.dts:      phy-mode = "rgmii";
  arch/arm64/boot/dts/qcom/sa8540p-ride.dts:     phy-mode = "rgmii-txid";
                                                 (on both ports)

The example in Documentation/devicetree/bindings/net/qcom,ethqos.yaml
also uses phy-mode = "rgmii".

The series, up to and including "net: stmmac: qcom-ethqos: add Shikra
EMAC support", does not convert any of these files. The current mainline
DTBs would therefore hit this warning on every probe, and twice on
sa8540p-ride.

Should the DTS files and the binding example be converted first, or
should the warning wait until they are?

The text also does not name the recommended mode. Could the message
mention "rgmii-id"?

Moving to rgmii-id also takes more than a DT edit. In this driver,
RGMII_ID takes a different MAC path:

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;
	}
    ...
}

ethqos_set_clk_tx_rate() also uses a different link clock rate at
10/100M in this mode. No delay is added on the MAC side, so the PHY has to
add both the RX and TX delays.

If a board's PHY or PHY driver does not apply internal delays, could
following this warning break its RGMII link?

[Severity: Low]
Will this message repeat on deferred probe? The dev_warn() runs before
several steps in qcom_ethqos_probe() that can return -EPROBE_DEFER:

	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
	...
	ethqos->serdes_phy = devm_phy_optional_get(dev, "serdes");
	...
	return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);

Each deferral runs qcom_ethqos_probe() again and prints the same warning
again. Could the warning move later in probe, after the steps that can
defer?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com

[PATCH net-next v3 6/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; set_clk_tx_rate updates the rate once
a link is established.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
v3: changed fix_mac_speed to set_clk_tx_rate in the commit message since
that's the function actually changing the RGMII link clock rate.
---
 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 a92bf4a6a2dbd987893bd4a3890df770bb7a7e56..2471871562ed9915398614d5c88d1c6673829db8 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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] qcom-ethqos: 1G DLL configured at SPEED_10 in ethqos_fix_mac_speed_rgmii()
--

commit d682d4a3cd0ce180df92d414cfd6684dfbcfe09f
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

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

This commit changes the initial RGMII link clock rate in qcom_ethqos_probe()
from SPEED_1000 to SPEED_10 to reduce power consumption before a link is
established.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index a92bf4a6a2dbd..2471871562ed9 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 initializing ethqos->link_clk to SPEED_10 cause the SDCC DLL to be
configured and calibrated at 5 MHz instead of 250 MHz on the initial
1000 Mbps link-up in non-RGMII_ID modes (PHY_INTERFACE_MODE_RGMII,
PHY_INTERFACE_MODE_RGMII_RXID, and PHY_INTERFACE_MODE_RGMII_TXID)?

When a link comes up, stmmac_mac_link_up() in
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c calls fix_mac_speed()
before set_clk_tx_rate():

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_link_up() {
	...
	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);
	...
}

In ethqos_fix_mac_speed_rgmii(), for non-RGMII_ID modes at SPEED_1000, the
driver enables the SDCC DLL, polls SDC4_STATUS for SDC4_STATUS_DLL_LOCK, and
calls ethqos_dll_configure() (which programs MCLK_FREQ_CALC for 250 MHz and
waits for DDR calibration):

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:ethqos_fix_mac_speed_rgmii() {
	...
	if (speed != SPEED_100 && speed != SPEED_10) {
		/* Set DLL_EN */
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,
			      SDCC_HC_REG_DLL_CONFIG);

		/* Set CK_OUT_EN */
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
			      SDCC_HC_REG_DLL_CONFIG);

		/* Set USR_CTL bit 26 with mask of 3 bits */
		if (!ethqos->has_emac_ge_3)
			rgmii_updatel(ethqos, GENMASK(26, 24), BIT(26),
				      SDCC_USR_CTL);

		/* 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);
	...
}

Prior to commit 9192320a65b7 ("net: stmmac: qcom-ethqos: convert to
set_clk_tx_rate() method"), ethqos_fix_mac_speed() updated ethqos->link_clk
before configuring the DLL and RGMII macro. After that commit moved the clock
rate update to set_clk_tx_rate(), the initial 1000 Mbps link-up after probe
only had a 250 MHz clock during ethqos_fix_mac_speed_rgmii() because
qcom_ethqos_probe() initialized ethqos->link_clk to SPEED_1000.

With qcom_ethqos_probe() now setting ethqos->link_clk to SPEED_10, won't the
first 1000 Mbps link-up run ethqos_fix_mac_speed_rgmii() and
ethqos_dll_configure() while ethqos->link_clk is still at 5 MHz, and only
switch ethqos->link_clk to 250 MHz afterwards in ethqos_set_clk_tx_rate()
without re-locking or recalibrating the DLL?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f@oss.qualcomm.com?part=6

Re: [PATCH net-next v3 6/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 1 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Probe now sets the RGMII link clock to the 10M rate.

--

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

At probe, this patch sets the RGMII link clock using SPEED_10 instead of
SPEED_1000, so the 250 MHz source clock is no longer running while there
is no link. The commit message says set_clk_tx_rate will update the rate
once a link comes up.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index a92bf4a6a2dbd..2471871562ed9 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]
After this change, is the 1G DLL calibrated against the wrong link_clk
rate on the first gigabit link-up after boot?

In stmmac_mac_link_up(), fix_mac_speed() runs before set_clk_tx_rate():

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_link_up() {
    ...
	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);
    ...
}

In the non-ID RGMII modes (rgmii, rgmii-rxid, rgmii-txid), the first
SPEED_1000 link-up then runs the whole 1G DLL bring-up in
ethqos_fix_mac_speed_rgmii() while link_clk is still at 5 MHz:

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:ethqos_fix_mac_speed_rgmii() {
    ...
		/* 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_set_clk_tx_rate() raises link_clk to 250 MHz only after this. As
far as I can tell, nothing recalibrates the DLL at the new rate until the
next link-up.

Before this patch, probe left link_clk at 250 MHz, so the first 1G
calibration saw the correct rate. That masked an ordering change from
commit 9192320a65b7 ("net: stmmac: qcom-ethqos: convert to
set_clk_tx_rate() method"). Before that commit, ethqos_fix_mac_speed()
called ethqos_update_link_clk() before ethqos_configure().

Could this produce "Timeout while waiting for DLL lock" or CK_OUT_EN
timeout errors? It could also mean up to about 3 seconds of busy-waiting
in read_poll_timeout_atomic() on the link-up path, or a DLL tuned to the
wrong period until the link flaps.

In-tree boards that use these modes include qcs404-evb-4000.dts (rgmii),
sa8155p-adp.dts (rgmii) and sa8540p-ride.dts (rgmii-txid, two ports).
rgmii-id (DLL bypassed) and 10/100 links are not affected.

Later patches in the series do not appear to change this ordering or the
probe rate.

Would it make sense to call set_clk_tx_rate() before fix_mac_speed() in
stmmac_mac_link_up()? The other option is to update the link clock inside
the qcom fix_mac_speed path before the DLL is configured.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com

[PATCH net-next v3 7/9] net: stmmac: qcom-ethqos: share common GMAC4 address table

[Mohd Ayaan Anwar]

Several ETHQOS versions use identical GMAC4 DMA and MTL address offsets.
Store the descriptor once and let per-compatible data reference it.

Use its presence, rather than has_emac_ge_3, when passing it to stmmac
core. This keeps address-table ownership independent of the feature
flag.

No functional change intended.

Suggested-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
---
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 57 +++++++++-------------
 1 file changed, 22 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 2471871562ed9915398614d5c88d1c6673829db8..370eb05caabc00a20d887ac88c1ffa4473bc4021 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -96,7 +96,7 @@ struct ethqos_emac_driver_data {
 	bool has_emac_ge_3;
 	u8 dma_addr_width;
 	const char *link_clk_name;
-	struct dwmac4_addrs dwmac4_addrs;
+	const struct dwmac4_addrs *dwmac4_addrs;
 	bool needs_sgmii_loopback;
 };
 
@@ -247,6 +247,23 @@ static const struct ethqos_emac_driver_data emac_v2_1_0_data = {
 	.has_emac_ge_3 = false,
 };
 
+static const struct dwmac4_addrs ethqos_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 const struct ethqos_emac_por emac_v3_0_0_por[] = {
 	{ .offset = RGMII_IO_MACRO_CONFIG,	.value = 0x40c01343 },
 	{ .offset = SDCC_HC_REG_DLL_CONFIG,	.value = 0x2004642c },
@@ -261,22 +278,7 @@ static const struct ethqos_emac_driver_data emac_v3_0_0_data = {
 	.num_rgmii_por = ARRAY_SIZE(emac_v3_0_0_por),
 	.rgmii_config_loopback_en = false,
 	.has_emac_ge_3 = true,
-	.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,
-	},
+	.dwmac4_addrs = &ethqos_dwmac4_addrs,
 };
 
 static const struct ethqos_emac_por emac_v4_0_0_por[] = {
@@ -296,22 +298,7 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
 	.link_clk_name = "phyaux",
 	.needs_sgmii_loopback = true,
 	.dma_addr_width = 36,
-	.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,
-	},
+	.dwmac4_addrs = &ethqos_dwmac4_addrs,
 };
 
 static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
@@ -830,8 +817,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	plat_dat->dump_debug_regs = rgmii_dump;
 	plat_dat->ptp_clk_freq_config = ethqos_ptp_clk_freq_config;
 	plat_dat->core_type = DWMAC_CORE_GMAC4;
-	if (ethqos->has_emac_ge_3)
-		plat_dat->dwmac4_addrs = &data->dwmac4_addrs;
+	if (data->dwmac4_addrs)
+		plat_dat->dwmac4_addrs = data->dwmac4_addrs;
 	plat_dat->pmt = true;
 	if (of_property_read_bool(np, "snps,tso"))
 		plat_dat->flags |= STMMAC_FLAG_TSO_EN;

-- 
2.34.1

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

[Mohd Ayaan Anwar]

Some SoCs gate the EMAC DMA path behind dedicated AXI clocks. Add
per-platform clock-name lists and configure them as a single
multi-clock OPP. The OPP core programs every clock rate and propagates
the required power state.

Select PM_OPP because DWMAC_QCOM_ETHQOS calls PM OPP APIs for
compatibles that describe AXI OPP clocks.

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>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |   1 +
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 104 +++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index ab3c4cf964239d724945651f7f7b5448118ad5f3..c86fbe2680ad46b6d444c1469b90872dff9b26b4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -146,6 +146,7 @@ config DWMAC_QCOM_ETHQOS
 	tristate "Qualcomm ETHQOS support"
 	default ARCH_QCOM
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	select PM_OPP
 	help
 	  Support for the Qualcomm ETHQOS core.
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 370eb05caabc00a20d887ac88c1ffa4473bc4021..9c602755532e141f4b822721686b1481b29b7d4c 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"
@@ -98,6 +99,8 @@ struct ethqos_emac_driver_data {
 	const char *link_clk_name;
 	const struct dwmac4_addrs *dwmac4_addrs;
 	bool needs_sgmii_loopback;
+	const char * const *noc_clk_names;
+	unsigned int num_noc_clks;
 };
 
 struct qcom_ethqos {
@@ -112,6 +115,9 @@ struct qcom_ethqos {
 	bool rgmii_config_loopback_en;
 	bool has_emac_ge_3;
 	bool needs_sgmii_loopback;
+
+	struct clk_bulk_data *noc_clks;
+	unsigned int num_noc_clks;
 };
 
 static u32 rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
@@ -673,18 +679,61 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
 	return ret;
 }
 
+static int qcom_ethqos_set_opp(struct qcom_ethqos *ethqos)
+{
+	struct device *dev = &ethqos->pdev->dev;
+	unsigned long freq = ULONG_MAX;
+	struct dev_pm_opp *opp;
+	int ret;
+
+	opp = dev_pm_opp_find_freq_floor_indexed(dev, &freq, 0);
+	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
+		dev_err(dev, "Failed to find AXI NOC OPP: %d\n", ret);
+		return ret;
+	}
+
+	ret = dev_pm_opp_set_opp(dev, opp);
+	dev_pm_opp_put(opp);
+
+	return ret;
+}
+
 static int ethqos_clks_config(void *priv, bool enabled)
 {
 	struct qcom_ethqos *ethqos = priv;
 	int ret = 0;
 
 	if (enabled) {
+		if (ethqos->num_noc_clks) {
+			ret = qcom_ethqos_set_opp(ethqos);
+			if (ret) {
+				dev_err(&ethqos->pdev->dev,
+					"NOC OPP set failed: %d\n", ret);
+				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);
+				clk_disable_unprepare(ethqos->link_clk);
+				dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
+				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
@@ -693,7 +742,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;
@@ -721,6 +775,52 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
 	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
 }
 
+static void qcom_ethqos_noc_opp_cleanup(void *dev)
+{
+	dev_pm_opp_set_rate(dev, 0);
+}
+
+/* Some SoCs gate NOC access behind dedicated clocks. Acquire them here
+ * and set up an OPP table to vote for the required rate and power.
+ */
+static int qcom_ethqos_init_noc_clks(struct qcom_ethqos *ethqos,
+				     const struct ethqos_emac_driver_data *data)
+{
+	struct dev_pm_opp_config config = {
+		.clk_names = data->noc_clk_names,
+		.config_clks = dev_pm_opp_config_clks_simple,
+	};
+	struct device *dev = &ethqos->pdev->dev;
+	unsigned int i;
+	int ret;
+
+	if (!data->num_noc_clks)
+		return 0;
+
+	ethqos->noc_clks = devm_kcalloc(dev, data->num_noc_clks,
+					sizeof(*ethqos->noc_clks), GFP_KERNEL);
+	if (!ethqos->noc_clks)
+		return -ENOMEM;
+
+	for (i = 0; i < data->num_noc_clks; i++)
+		ethqos->noc_clks[i].id = data->noc_clk_names[i];
+	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_config(dev, &config);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to configure OPP clocks\n");
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add OPP table\n");
+
+	return devm_add_action_or_reset(dev, qcom_ethqos_noc_opp_cleanup, dev);
+}
+
 static int qcom_ethqos_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -782,6 +882,10 @@ 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;
 
+	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 v3 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),
and the 36-bit DMA address width.

The NOC clock rates are set using an OPP table. As part of the NOC
clock voting logic, the qcom-ethqos glue driver takes a second enable
reference on the "stmmaceth" clock, which is already enabled by the
stmmac core. Since, all three clocks must run at 120 MHzs, managing
"stmmaceth" through the same clk_bulk path keeps the rate-setting and
enable/disable together.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 9c602755532e141f4b822721686b1481b29b7d4c..a4e083a8517d095e5571b1340f9dd4b5e0c6e81f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -307,6 +307,22 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
 	.dwmac4_addrs = &ethqos_dwmac4_addrs,
 };
 
+static const char * const shikra_noc_clk_names[] = {
+	"stmmaceth",
+	"axi-noc",
+	"pcie-tile-axi-noc",
+	NULL,
+};
+
+static const struct ethqos_emac_driver_data shikra_data = {
+	.dma_addr_width = 36,
+	.has_emac_ge_3 = true,
+	.noc_clk_names = shikra_noc_clk_names,
+	.num_noc_clks = ARRAY_SIZE(shikra_noc_clk_names) - 1,
+	.rgmii_config_loopback_en = false,
+	.dwmac4_addrs = &ethqos_dwmac4_addrs,
+};
+
 static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 {
 	struct device *dev = &ethqos->pdev->dev;
@@ -947,6 +963,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 v3 9/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: qcom-ethqos: missing rgmii_por in shikra_data
--

commit 8e4d72d76d93b838333086304a2ba657b2b5e8e1
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

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

This patch adds support for the Qualcomm Shikra ETHQOS controller by
registering the qcom,shikra-ethqos compatible string and defining the
shikra_data driver data descriptor with its three NOC clocks and 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 9c602755532e1..a4e083a8517d0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -307,6 +307,22 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
>  	.dwmac4_addrs = &ethqos_dwmac4_addrs,
>  };
>  
> +static const char * const shikra_noc_clk_names[] = {
> +	"stmmaceth",
> +	"axi-noc",
> +	"pcie-tile-axi-noc",
> +	NULL,
> +};
> +
> +static const struct ethqos_emac_driver_data shikra_data = {
> +	.dma_addr_width = 36,
> +	.has_emac_ge_3 = true,
> +	.noc_clk_names = shikra_noc_clk_names,
> +	.num_noc_clks = ARRAY_SIZE(shikra_noc_clk_names) - 1,
> +	.rgmii_config_loopback_en = false,
> +	.dwmac4_addrs = &ethqos_dwmac4_addrs,
> +};

[Severity: Medium]
Should shikra_data also populate .rgmii_por and .num_rgmii_por like all the
other struct ethqos_emac_driver_data descriptors (emac_v2_3_0_data,
emac_v2_1_0_data, emac_v3_0_0_data, and emac_v4_0_0_data) in
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c?

With .rgmii_por and .num_rgmii_por omitted, qcom_ethqos_probe() leaves
ethqos->rgmii_por as NULL and ethqos->num_rgmii_por as 0 while registering
ethqos_fix_mac_speed_rgmii() as plat_dat->fix_mac_speed. When phylink brings
up or changes the RGMII link speed, the POR reset loop in
ethqos_fix_mac_speed_rgmii() does nothing:

	/* 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);

Subsequent configuration in ethqos_fix_mac_speed_rgmii() and
ethqos_rgmii_id_macro_init() relies on read-modify-write updates assuming
RGMII_IO_MACRO_CONFIG, SDCC_HC_REG_DLL_CONFIG, SDCC_HC_REG_DDR_CONFIG,
SDCC_HC_REG_DLL_CONFIG2, SDCC_USR_CTL, and RGMII_IO_MACRO_CONFIG2 were first
reset to their baseline POR defaults.

Without this reset, non-POR state left by a bootloader or kexec is never
cleared in any RGMII mode (including PHY_INTERFACE_MODE_RGMII_ID, where
ethqos_rgmii_id_macro_init() relies on POR defaults for fields such as
RGMII_CONFIG_INTF_SEL, RGMII_CONFIG_MAX_SPD_PRG_2, and
RGMII_CONFIG_MAX_SPD_PRG_9). Additionally, in PHY_INTERFACE_MODE_RGMII,
PHY_INTERFACE_MODE_RGMII_RXID, and PHY_INTERFACE_MODE_RGMII_TXID modes, speed
transitions between 1000M and 100M/10M will leave stale bits set from the
previous speed (such as SDCC_DLL_CONFIG_DLL_EN, SDCC_DLL_CONFIG_CDR_EN,
SDCC_DLL_CONFIG_CDR_EXT_EN, and SDCC_DDR_CONFIG_PRG_RCLK_DLY when switching
from 1000M to 100M/10M, or SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY, and SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE
when switching from 100M/10M to 1000M).

[ ... ]

> @@ -947,6 +963,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://sashiko.dev/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f@oss.qualcomm.com?part=9

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

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new shikra_data descriptor
  (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:317-324) has…
- [Medium] The commit message justifies taking a second enable reference
  on 'stmmaceth' (by putting it in shikra_noc_clk_names, so it goes…

--

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

This patch adds Shikra EMAC support. It registers the qcom,shikra-ethqos
compatible with a new shikra_data descriptor. The descriptor enables three
NOC clocks (stmmaceth, axi-noc, pcie-tile-axi-noc), whose rates are set
through an OPP table, 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 9c602755532e1..a4e083a8517d0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -307,6 +307,22 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
>  	.dwmac4_addrs = &ethqos_dwmac4_addrs,
>  };
>  
> +static const char * const shikra_noc_clk_names[] = {
> +	"stmmaceth",

[Severity: Medium]
The commit message says that managing "stmmaceth" through the same
clk_bulk path:

  keeps the rate-setting and enable/disable together.

Is that accurate? The stmmac core still enables stmmac_clk by itself,
before the OPP vote is applied.

At probe, stmmac_probe_config_dt() calls
clk_prepare_enable(plat->stmmac_clk). That happens well before
qcom_ethqos_init_noc_clks() and ethqos_clks_config(ethqos, true) set up
the OPP.

On runtime resume and noirq system resume, stmmac_bus_clks_config()
enables stmmac_clk first, and only then calls the glue clks_config():

drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:stmmac_bus_clks_config() {
    ...
	if (enabled) {
		ret = clk_prepare_enable(plat_dat->stmmac_clk);
		...
		ret = clk_prepare_enable(plat_dat->pclk);
		...
		if (plat_dat->clks_config) {
			ret = plat_dat->clks_config(plat_dat->bsp_priv, enabled);
    ...
}

On suspend, ethqos_clks_config(false) calls dev_pm_opp_set_rate(dev, 0).
That reaches _disable_opp_table(), which drops the bandwidth, level and
required-opps votes but does not change clock rates. So stmmaceth stays at
120 MHz.

On the next resume, is stmmaceth then enabled at 120 MHz before
qcom_ethqos_set_opp() restores the VDD_CX required-opps vote? The binding
commit says that vote is needed when the NOC clocks are enabled.

If so, the second enable reference doesn't keep rate-setting and
enable/disable together. Should the commit message rationale be updated?

> +	"axi-noc",
> +	"pcie-tile-axi-noc",
> +	NULL,
> +};
> +
> +static const struct ethqos_emac_driver_data shikra_data = {
> +	.dma_addr_width = 36,
> +	.has_emac_ge_3 = true,
> +	.noc_clk_names = shikra_noc_clk_names,
> +	.num_noc_clks = ARRAY_SIZE(shikra_noc_clk_names) - 1,
> +	.rgmii_config_loopback_en = false,
> +	.dwmac4_addrs = &ethqos_dwmac4_addrs,
> +};

[Severity: Medium]
Is it intentional that shikra_data has no .rgmii_por and .num_rgmii_por?

Every other descriptor has a POR table. That includes the other
has_emac_ge_3 descriptors, emac_v3_0_0_data and emac_v4_0_0_data.

Shikra still uses the RGMII path. qcom_ethqos_probe() installs
ethqos_fix_mac_speed_rgmii() for every RGMII phy-mode, and the binding
requires the "rgmii" link clock for Shikra.

With num_rgmii_por == 0, the reset loop in ethqos_fix_mac_speed_rgmii()
does nothing:

	/* 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);

Most of the programming after this loop is read-modify-write. It seems to
rely on the POR rewrite to clear bits left over from an earlier speed.

For example, in ethqos_rgmii_macro_init(), the 10/100 branch sets
SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN. The 1G branch only sets
PRG_RCLK_DLY and PRG_DLY_EN:

	if (speed != SPEED_1000) {
		...
		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
			      SDCC_HC_REG_DDR_CONFIG);
	} else {
		...
		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_PRG_DLY_EN,
			      SDCC_HC_REG_DDR_CONFIG);
	}

On v3/v4, the POR value 0x80040800 for SDCC_HC_REG_DDR_CONFIG clears this
bit first. After a 10/100 to 1G renegotiation on Shikra, would the external
RX clock delay stay enabled along with the 1G programmed delay?

The reverse case has the same problem. The 1G path and
ethqos_dll_configure() set DLL_EN, CK_OUT_EN, CDR_EN and CDR_EXT_EN in
SDCC_HC_REG_DLL_CONFIG. After a 1G to 10/100 change, nothing clears them,
even though the 10/100 path deliberately skips enabling the DLL:

	if (speed != SPEED_100 && speed != SPEED_10) {
		/* Set DLL_EN */
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,
			      SDCC_HC_REG_DLL_CONFIG);

On v3/v4, the POR value 0x2004642c clears these bits.

The driver also never programs some fields itself.
SDCC_DDR_CONFIG_TCXO_CYCLES_CNT is one: the v3/v4 POR sets it to 4, which
matches the "52 * 4" comment in the PRG_RCLK_DLY calculation.

In rgmii-id mode, SDCC_HC_REG_DDR_CONFIG and SDCC_HC_REG_DLL_CONFIG2 are not
touched at all. On Shikra they keep whatever the hardware reset or the
bootloader left there.

Could Shikra get its own POR table? If skipping it is safe, could the
commit message explain why?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com

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

[Mohd Ayaan Anwar]

On Sat, Oct 17, 2026 at 03:15:11PM +0530, Mohd Ayaan Anwar wrote:
> 
>       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: share common GMAC4 address table
>       net: stmmac: qcom-ethqos: add per-platform NOC clock voting
>       net: stmmac: qcom-ethqos: add Shikra EMAC support

There should be 10 patches, not 9.
Looks like I messed up some b4 setting and it dropped the bindings
change for dp83867. It is the same as v2:
https://lore.kernel.org/all/20260908-shikra_ethernet-v2-1-bbe3389d0652@oss.qualcomm.com/

I hope I can resend the series after 24 hours.

	Ayaan