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

[PATCH net-next v2 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 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):
      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    | 218 +++++++++++++++++++--
 drivers/net/phy/dp83867.c                          |  33 ++++
 5 files changed, 300 insertions(+), 17 deletions(-)
---
base-commit: 7042c8c193e5d634198b7c766bb3a01c8e3ee0e2
change-id: 20260903-shikra_ethernet-ae7bee5e804e

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

[PATCH net-next v2 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

Re: [PATCH net-next v2 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] Reported concern (assessed INVALID): the binding patch adds four
  regulator-consumer properties (vdda-2p5-supply, vdd-1p0-supply,…

--

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

This documents four regulator consumer 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.  Documentation-only change, no
code is touched.

No Link: tags present in the commit header.

> 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 thing that came up while reading this in isolation: at this commit
drivers/net/phy/dp83867.c has no regulator API usage, so the four new
properties describe DT ABI with no in-tree consumer yet.  Is that
intentional for this patch of the series?

Checking the rest of the series answers it, so this looks fine as posted.
The following patch, "net: phy: dp83867: add regulator supply management",
adds dp83867_power_on() with:

  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 dp83867_power_on() before dp83867_of_init() and
before any register programming in dp83867_config_init(), with the
datasheet stabilization delay applied there.  The supply IDs match the
property names documented here exactly, and the enable reference taken by
devm_regulator_get_enable_optional() keeps the regulator core's late
cleanup from turning the rails back off.

So no change is being requested here; noting it only in case the
documentation is meant to land ahead of the driver change.

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

[PATCH net-next v2 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 | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 88255e92b4cdbd6da2e2c1d10f9c72348d28dbc3..dbeee7cad6f0cbfeb127bfd28f43ee2a16c78879 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,41 @@ static int dp83867_resume(struct phy_device *phydev)
 	return 0;
 }
 
+static int dp83867_power_on(struct phy_device *phydev)
+{
+#ifdef CONFIG_OF
+	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);
+#endif
+	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

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

[Andrew Davis]

On 9/7/26 3:23 PM, 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.
> 
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
> ---
>   drivers/net/phy/dp83867.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cdbd6da2e2c1d10f9c72348d28dbc3..dbeee7cad6f0cbfeb127bfd28f43ee2a16c78879 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,41 @@ static int dp83867_resume(struct phy_device *phydev)
>   	return 0;
>   }
>   
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +#ifdef CONFIG_OF
> +	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]);

Would be nice to have an API for getting all regulators without needing to
manually list all of their names, since we don't really care about them
individually. The of_regulator_bulk_get_all() is close but it would need
a "get_enable" variant, plus _optional, then made into a devm_ version.. :)

Well until such a thing comes along this looks fine,

Acked-by: Andrew Davis <afd@ti.com>

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

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

[Lorenzo Bianconi]

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

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

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  drivers/net/phy/dp83867.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cdbd6da2e2c1d10f9c72348d28dbc3..dbeee7cad6f0cbfeb127bfd28f43ee2a16c78879 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,41 @@ static int dp83867_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +#ifdef CONFIG_OF
> +	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]);

nit: I guess it easier to read if you do something like:

		if (ret != -ENODEV)
			return dev_err_probe();

		count += !ret;

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

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

Re: [PATCH net-next v2 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: 1 · Low: 1

- [High] The rail enable sequence is placed in the PHY driver's probe
  (dp83867_probe() -> dp83867_power_on(),…
- [Medium] In dp83867_power_on() (drivers/net/phy/dp83867.c:723-748) the
  variable `count` is incremented for every return value of 0 from…
- [Low] dp83867_power_on() wraps its entire body in `#ifdef CONFIG_OF`
  (drivers/net/phy/dp83867.c:725-746) although nothing inside uses an OF…

--

net: phy: dp83867: add regulator supply management

This adds dp83867_power_on(), called from dp83867_probe(), which walks four
supply names with devm_regulator_get_enable_optional() and, if any of them
returned success, sleeps 200 ms for the datasheet post power-up
stabilisation.  A few questions about the new function below.

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

[ ... ]

> @@ -719,9 +720,41 @@ static int dp83867_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +#ifdef CONFIG_OF

[Severity: Low]
Is CONFIG_OF the right predicate here?  Nothing inside the guard uses an OF
API, only devm_regulator_get_enable_optional(), dev_err_probe() and
fsleep().

On CONFIG_OF=n the whole body disappears and dp83867_power_on() becomes
"return 0", so supplies that the regulator core would resolve through the
non-DT lookup path (regulator_lookup_by_name() in _regulator_get(), taken
when dev_of_node(dev) is NULL) are never enabled, and errors including
-EPROBE_DEFER are never propagated.

The rest of this file uses #if IS_ENABLED(CONFIG_OF_MDIO) for its genuinely
OF-dependent code (dp83867_of_init_io_impedance()).  Would keying this on
the regulator API instead, or dropping the guard entirely since the
!CONFIG_REGULATOR stub already makes the calls harmless, be more accurate?

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

[Severity: Medium]
Can count reliably mean "a rail was newly powered up" here?

With CONFIG_REGULATOR=n the API is a stub that can never return -ENODEV:

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

so all four lookups "succeed", count == 4, and every DP83867 probe pays
fsleep(200000), which for usecs >= USLEEP_RANGE_UPPER_BOUND lands in
msleep(200).  DP83867_PHY in drivers/net/phy/Kconfig has no
depends on REGULATOR, so this is a valid configuration.

With CONFIG_REGULATOR=y, _devm_regulator_get_enable() in
drivers/regulator/devres.c returns the result of regulator_enable():

	ret = regulator_enable(r);
	if (!ret)
		ret = devm_add_action_or_reset(dev, &regulator_action_disable, r);

and regulator_enable() also returns 0 for a rail that is already on
(always-on/boot-on, enabled by the bootloader, or shared with another
consumer), so the 200 ms wait is taken on every probe or re-bind merely
because the rails are described in DT, with no off->on transition.

Both cases seem to disagree with the commit message:

> 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 before
> returning.

Should the delay instead be gated on something that actually distinguishes
an off->on transition?

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

[Severity: High]
Can this call site ever power up a DP83867 whose rails are off at boot,
which is the board class the commit message describes?

phy_driver->probe() runs only after the PHY has been identified over MDIO.
For a DT node without an explicit "ethernet-phy-idXXXX.YYYY" compatible,
fwnode_mdiobus_register_phy() takes the scanning path:

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

and get_phy_device() -> get_phy_c22_id() gives up on an unpowered PHY:

drivers/net/phy/phy_device.c:get_phy_c22_id() {
	/* If the phy_id is mostly Fs, there is no device there */
	if ((*phy_id & 0x1fffffff) == 0x1fffffff)
		return -ENODEV;
}

With the rails off no phy_device with the TI ID is created, so this driver
never binds and dp83867_power_on() never runs.  In the case where the driver
does bind after a scan, the rails were already on, so the enable is a no-op
and the 200 ms is dead time on every bind.  Would this power-up need to
happen before MDIO identification (MDIO bus / reset-and-power stage) to have
the intended effect?  The only configuration where the new code can power a
dark PHY appears to be a DT node carrying an explicit phy-id compatible so
the ID read is skipped, and the binding example added earlier in the series
in Documentation/devicetree/bindings/net/ti,dp83867.yaml uses the scanned
form.

There is a second consequence of using the devm form here.
devm_regulator_get_enable_optional() installs regulator_action_disable() via
devres, so unbinding the PHY driver, or a later failure in dp83867_probe(),
disables these rails while the mdio device stays registered.  Does that
leave subsequent register access (generic PHY fallback, re-scan, ethtool)
talking to a powered-off device?

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

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

[PATCH net-next v2 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

Re: [PATCH net-next v2 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: 0 · Low: 2

- [Low] Documentation/devicetree/bindings/net/qcom,ethqos.yaml: the new
  `if: compatible contains qcom,shikra-ethqos` block makes `operating-…
- [Low] Concern (verified as resolved within this series): the binding
  adds `qcom,shikra-ethqos` with six mandatory clocks (including `axi-…

--

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

This adds the qcom,shikra-ethqos compatible to the qcom,ethqos and
snps,dwmac schemas, and introduces an if/else block that constrains Shikra
to six clocks (adding axi-noc and pcie-tile-axi-noc) while keeping the
existing compatibles at four. The Shikra branch also makes
operating-points-v2 mandatory.

A couple of questions below.

> 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]
At this commit there is no driver side for this compatible:
qcom_ethqos_match[] in drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
has no qcom,shikra-ethqos entry, and nothing acquires axi-noc or
pcie-tile-axi-noc or registers an OPP table.

Checking the rest of the series shows this is closed later by "net: stmmac:
qcom-ethqos: add per-platform NOC clock voting" and "net: stmmac:
qcom-ethqos: add Shikra EMAC support", which add:

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

and

  static const struct ethqos_noc_clk_cfg shikra_noc_clks[] = {
	{ "axi-noc", 120000000 },
	{ "pcie-tile-axi-noc", 120000000 },
	{ "stmmaceth", 120000000 },
  };

along with qcom_ethqos_init_noc_clks() doing devm_clk_bulk_get() plus
devm_pm_opp_of_add_table() and dev_pm_opp_set_rate(). The clock-names order
here (rgmii as the fourth name) also matches shikra_data, which does not
override link_clk_name.

So this looks consistent with the series as a whole; noting it only because
the binding lands ahead of its consumer and the intermediate bisect point
has no driver support.

[ ... ]

> @@ -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: Low]
The commit message says the OPP table is there so its required-opps can
"vote VDD_CX to SVS when the NOC clocks are enabled". Should power-domains
be in this required list as well?

The required-opps vote only reaches a genpd if the consumer device has one
attached. In drivers/opp/core.c, _set_required_opps() starts with:

	struct device **devs = opp_table->required_devs;
	...
	if (!devs)
		return 0;

and required_devs[] is only populated via _opp_set_required_dev(), reached
from genpd_set_required_opp_dev() in drivers/pmdomain/core.c during genpd
attach, i.e. only when the node has a power-domains phandle. Even past that
point, dev_pm_domain_set_performance_state() in drivers/base/power/common.c
does:

	if (dev->pm_domain && dev->pm_domain->set_performance_state)
		return dev->pm_domain->set_performance_state(dev, state);

	return 0;

power-domains is already permitted through the snps,dwmac.yaml $ref (it is
declared there as power-domains: maxItems: 1), so unevaluatedProperties:
false is not in the way; it is simply not required.

As written, a Shikra node with operating-points-v2 and no power-domains
passes dt_binding_check, and at runtime the CX performance-state request is
silently skipped while the NOC clocks still run at 120 MHz. No in-tree DTS
is affected today since there is no Shikra DTS yet.

[ ... ]

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

[PATCH net-next v2 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.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.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

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

[Lorenzo Bianconi]

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

> 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>
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@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
> 
> 

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

Re: [PATCH net-next v2 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] In `ethqos_rgmii_macro_init()`
  (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c) the patch…

--

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 it drops the speed validation from that
function on the grounds that the same check exists higher up the call stack.

> 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]
The commit message says "the speed validation duplicates a check higher up
the call stack".  Which check is that referring to?

The only path here is:

  stmmac_mac_link_up()
    -> priv->plat->fix_mac_speed()
      -> ethqos_fix_mac_speed_rgmii()
        -> ethqos_rgmii_macro_init()

stmmac_mac_link_up() only bails out in the default arm of its speed switch:

	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 everything above it are accepted and forwarded verbatim to
fix_mac_speed() regardless of the interface mode.

ethqos_fix_mac_speed_rgmii() does no RGMII-specific speed validation either;
it only distinguishes speed == SPEED_1000 from speed == SPEED_100/SPEED_10
while restoring POR values and bringing up the DLL, then passes speed
straight through.

So the removed test rejected speeds that are invalid for RGMII, while what
remains upstream only rejects speeds the core does not recognise at all.
Are these really the same check?

A speed above 1000 does appear reachable on an RGMII port through a fixed
link.  phylink_parse_fixedlink() keeps the requested speed even when it does
not match the validated supported set:

	if (c) {
		...
	} else {
		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
			     pl->link_config.speed);
	}

and phylink_get_fixed_state() then hands that speed to mac_link_up() via
*state = pl->link_config.  With phy-mode = "rgmii" plus
fixed-link { speed = <2500>; full-duplex; }, does 2500 now reach
ethqos_rgmii_macro_init() and silently take the non-1000 branches
(BYPASS_TX_ID_EN set, POS_NEG_DATA_SEL and PROG_SWAP cleared,
PRG_RCLK_DLY_CODE = 5, RX_PROG_SWAP cleared on pre-GE3 parts) while neither
MAX_SPD_PRG_2 nor MAX_SPD_PRG_9 is programmed, with the DLL already locked
by the caller?

The "Invalid speed %d" dev_err() was the only indication that the RGMII
configuration is unsupported.  Would it make sense to move the check into
ethqos_fix_mac_speed_rgmii(), before the POR restore and the DLL
reset/enable sequence, rather than dropping it?

For reference, no later patch in this series reinstates a speed check:
ethqos_fix_mac_speed_rgmii() and ethqos_rgmii_macro_init() still accept any
speed at the end of the series, and the newly added
ethqos_rgmii_id_macro_init() is equally unvalidated.

>  	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/20260908-shikra_ethernet-v2-0-bbe3389d0652%40oss.qualcomm.com

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

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

- [Medium] The patch is worded as a bug fix ("fix RGMII_ID mode to use
  DLL bypass", "Also fix ethqos_set_clk_tx_rate()") for two user-visible…

--

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

For phy-mode = "rgmii-id" the driver now powers down the DLL, sets DDR
bypass and programs the IO_MACRO through a new
ethqos_rgmii_id_macro_init() helper instead of running the generic DLL
init.  It also stops doubling the link clock rate at 100M/10M in that
mode, and drops RGMII_ID from the TX phase-shift suppression in
ethqos_rgmii_macro_init().

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

[ ... ]

> @@ -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: Medium]
The changelog describes this as two functional corrections, "fix RGMII_ID
mode to use DLL bypass" and "Also fix ethqos_set_clk_tx_rate() to not
double the clock rate in bypass mode at 100M/10M", but there is no Fixes:
tag.  Should one be added?

Both defects look reachable before this patch.  qcom_ethqos_probe() accepts
PHY_INTERFACE_MODE_RGMII_ID and installs the affected callback:

	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;

so an rgmii-id link previously ran the full DLL init and had the RX delay
programmed by ethqos_rgmii_macro_init() through
SDCC_DDR_CONFIG_PRG_RCLK_DLY / SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY, and also
got rate * 2 from ethqos_set_clk_tx_rate().

The partial mitigation for the TX side came in with 164a9ebe9742 ("net:
stmmac: dwmac-qcom-ethqos: Respect phy-mode and TX delay"), which only
suppressed RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN and left the RX delay and
the doubled rate in place.  Would that commit, or the original driver
commit, be the right Fixes: target here?

Related question: which platform reproduces the misprogramming, and on
which hardware was the new bypass register sequence in
ethqos_rgmii_id_macro_init() validated?  Grepping arch/arm64/boot/dts/qcom
turns up no ethqos node using phy-mode = "rgmii-id" (only sa8540p-ride.dts,
which uses "rgmii-txid"), so it is hard to tell from the changelog who is
affected and what the backport risk is.  Could that be stated in the commit
message?

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

[PATCH net-next v2 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 | 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 v2 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: 1 · Low: 1

- [Medium] Unactionable boot-time warning on every in-tree qcom ethqos
  platform: the new dev_warn() in qcom_ethqos_probe() (dwmac-qcom-…
- [Low] Commit message / code mismatch: the changelog of "net: stmmac:
  qcom-ethqos: warn about legacy RGMII PHY modes" states that "existing…

--

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

This adds a probe-time dev_warn() in qcom_ethqos_probe() when the ethernet
node uses one of the older RGMII phy-mode values, suggesting a move to
"rgmii-id" with PHY-provided delays.  PHY_INTERFACE_MODE_RGMII_ID is split
out of the shared case group so that it does not warn.

> 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: Low]
The changelog 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 warning in qcom_ethqos_probe() is attached to a group of three case
labels, so PHY_INTERFACE_MODE_RGMII_RXID also warns.

Is "rgmii-rxid" intended to be declared legacy as well?  The driver does
treat these modes differently, for example in ethqos_rgmii_macro_init():

	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
		phase_shift = 0;
	else
		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;

so which phy-modes are being deprecated seems worth stating exactly in the
commit message.

[Severity: Medium]
Is there an upstream DTB that this warning can point people at?  No in-tree
device tree for this driver uses "rgmii-id", and this series does not touch
arch/ at all, so every in-tree board hits the new warning on each probe:

  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";  (both MACs)

The binding example in Documentation/devicetree/bindings/net/qcom,ethqos.yaml
also still uses:

      phy-mode = "rgmii";

and the binding carries no phy-mode deprecation note, so the documented ABI
still recommends what the driver now calls legacy.

Converting those device trees is also the only way the advocated
PHY-provided-delay path gets exercised, since ethqos_set_clk_tx_rate() stops
doubling the link clock only for PHY_INTERFACE_MODE_RGMII_ID.  Would it make
sense to convert the in-tree DTs (and the binding example/text) in the same
series, or to hold off on the warning until a migration target exists?

> +		fallthrough;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
>  		plat_dat->fix_mac_speed = ethqos_fix_mac_speed_rgmii;
>  		break;
>  	case PHY_INTERFACE_MODE_2500BASEX:

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

[PATCH net-next v2 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.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
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 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 v2 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 2 potential
issue(s) to consider:

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

- [Medium] For legacy RGMII phy-modes (rgmii, rgmii-rxid, rgmii-txid) the
  RGMII IO-macro DLL is programmed and its lock polled inside…
- [Low] Commit message names the wrong callback: it states "fix_mac_speed
  updates the rate once a link is established", but neither…

--

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

The probe-time call to ethqos_set_clk_tx_rate() is changed from SPEED_1000
to SPEED_10 so the RGMII link clock is not left at 250 MHz while no link is
up.  Two questions below, one about the changelog wording and one about the
first link-up path.

> 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: Low]
Is "fix_mac_speed updates the rate once a link is established" the right
callback name here?  Neither ethqos_fix_mac_speed_rgmii() nor
ethqos_fix_mac_speed_sgmii() calls clk_set_rate() on ethqos->link_clk; the
only clk_set_rate(ethqos->link_clk, ...) in the file is in
ethqos_set_clk_tx_rate(), which probe installs as
plat_dat->set_clk_tx_rate.

stmmac_mac_link_up() invokes the two as separate callbacks, fix_mac_speed()
first and set_clk_tx_rate() later:

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

Could the changelog name set_clk_tx_rate instead, so the ordering is not
hidden?

> 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: Medium]
Does the first 1000 Mbps link-up now run the SDCC DLL bring-up with the
probe-time 10 Mbps clock rate?

For the legacy rgmii / rgmii-rxid / rgmii-txid modes,
ethqos_fix_mac_speed_rgmii() enables the DLL and polls for lock:

	if (speed != SPEED_100 && speed != SPEED_10) {
		/* Set DLL_EN */
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,
			      SDCC_HC_REG_DLL_CONFIG);
		...
		/* 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");
	}

and stmmac_mac_link_up() calls fix_mac_speed() before set_clk_tx_rate(), so
at that point ethqos->link_clk is still at the rate programmed at probe.
With SPEED_10 that is rgmii_clock(SPEED_10) = 2500000, doubled to 5 MHz for
the non-rgmii-id modes by ethqos_set_clk_tx_rate(), where it used to be
250 MHz:

	/* 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 the DLL cannot lock at that input rate, the poll busy-waits its full 1 s
budget in the link-up path and then continues with an unlocked DLL, since
fix_mac_speed() is not re-run after set_clk_tx_rate() raises the rate to
250 MHz.

Note the driver itself skips the DLL enable/lock for SPEED_10 and
SPEED_100, which suggests the block is expected to see the rate matching
the negotiated speed.  Is a minimum DLL input frequency involved here, and
if so should the probe-time rate stay high, or should the clock be raised
before fix_mac_speed() runs?

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

[PATCH net-next v2 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    | 106 +++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 2471871562ed9915398614d5c88d1c6673829db8..8871d285941162c72e2a8f79d2eef915d9283796 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)
@@ -689,15 +703,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);
+				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
@@ -706,7 +756,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;
@@ -734,6 +789,51 @@ 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
+ * 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 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;
@@ -795,6 +895,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

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

[Lorenzo Bianconi]

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

> 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    | 106 +++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 

[...]

>  static u32 rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
> @@ -689,15 +703,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]);

assuming the first clock is always an OPP device seems a bit fragile to me.
Can we find a way to enforce it? (e.g. have a dedicated clk_bulk_data struct
for it).

> +			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);
> +				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
> @@ -706,7 +756,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;
> @@ -734,6 +789,51 @@ 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
> + * 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 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;
> @@ -795,6 +895,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) {

I guess you can drop this check since it is already done in
qcom_ethqos_init_noc_clks(), right?

Regards,
Lorenzo

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

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

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

[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] New hard dependency on the PM_OPP subsystem is not expressed
  in Kconfig.

--

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

This adds an ethqos_noc_clk_cfg description 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
alongside the link clock in ethqos_clks_config(), with an OPP table used to
propagate the VDD_CX performance state.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 2471871562ed9..8871d28594116 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"

[ ... ]

> @@ -689,15 +703,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;
> +			}

[ ... ]

> @@ -734,6 +789,51 @@ 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
> + * 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)
> +{

[ ... ]

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

[Severity: Medium]
Should DWMAC_QCOM_ETHQOS gain a PM_OPP dependency now that these calls are
mandatory for compatibles that declare NOC clocks?

The Kconfig entry currently reads:

drivers/net/ethernet/stmicro/stmmac/Kconfig:
config DWMAC_QCOM_ETHQOS
	tristate "Qualcomm ETHQOS support"
	default ARCH_QCOM
	depends on OF && (ARCH_QCOM || COMPILE_TEST)

There is no depends on PM_OPP and no select PM_OPP, and PM_OPP itself is a
hidden symbol with no prompt:

drivers/opp/Kconfig:
config PM_OPP
	bool

so CONFIG_DWMAC_QCOM_ETHQOS=y with CONFIG_PM_OPP=n is a reachable
configuration. In that case the stubs are used:

include/linux/pm_opp.h:
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
	return -EOPNOTSUPP;
}
...
static inline int devm_pm_opp_of_add_table(struct device *dev)
{
	return -EOPNOTSUPP;
}

qcom_ethqos_probe() -> qcom_ethqos_init_noc_clks() ->
devm_pm_opp_set_clkname() would then return -EOPNOTSUPP, and the
dev_err_probe() above aborts probe with "Failed to set OPP clock name", so
no netdev is registered for any compatible that sets num_noc_clks
(qcom,shikra-ethqos, added later in this series).

The same applies on the runtime path: ethqos_clks_config(priv, true)
propagates the -EOPNOTSUPP from dev_pm_opp_set_rate() on every enable and
resume.

Would adding select PM_OPP (or depends on PM_OPP) to DWMAC_QCOM_ETHQOS, or
tolerating -EOPNOTSUPP on the OPP path, be preferable here?

> +
> +	return devm_add_action_or_reset(dev, qcom_ethqos_noc_opp_cleanup, dev);
> +}

[ ... ]

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

[PATCH net-next v2 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.

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. All three clocks in shikra_noc_clks[] must
run at 120 MHz for NOC access, and 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>
---
 .../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 8871d285941162c72e2a8f79d2eef915d9283796..c474f1e5a043777a52bcbff8a488640f6dbdca30 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;
@@ -962,6 +992,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 v2 9/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[Lorenzo Bianconi]

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

> 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.
> 
> 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. All three clocks in shikra_noc_clks[] must
> run at 120 MHz for NOC access, and 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>
> ---
>  .../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 8871d285941162c72e2a8f79d2eef915d9283796..c474f1e5a043777a52bcbff8a488640f6dbdca30 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 = {

nit: this part seems in common between emac_v3_0_0, emac_v4_0_0 and
shikraemac_v3_0_0, emac_v3_0_0 and shikra.
Can we add a common definition? E.g.

static const struct dwmac4_addrs qcom_emac_dwmac4_addrs {
	...
};

Regards,
Lorenzo

> +		.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;
> @@ -962,6 +992,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
> 
> 

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

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

- [High] shikra_data (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-
  ethqos.c:337) provides no .rgmii_por/.num_rgmii_por table, while the…
- [Medium] Adding the core-owned "stmmaceth" clock to shikra_noc_clks[]
  decouples that clock's programmed rate and enable state from the…

--

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

Adds a qcom,shikra-ethqos compatible with a new shikra_data descriptor that
declares three NOC clocks (axi-noc, pcie-tile-axi-noc, stmmaceth) at
120 MHz and a 36-bit DMA address width.  A couple of questions below about
the new descriptor.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 8871d28594116..c474f1e5a0437 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 },

[Severity: Medium]
Is it safe to put the core-owned "stmmaceth" clock into this table, given
that the 120 MHz rate and the OPP/VDD_CX vote then come from different
owners?

On the resume path the core enables the clock before this driver raises the
vote:

stmmac_platform.c:stmmac_bus_clks_config() {
	...
	ret = clk_prepare_enable(plat_dat->stmmac_clk);
	...
	if (plat_dat->clks_config) {
		ret = plat_dat->clks_config(plat_dat->bsp_priv, enabled);
	...
}

ethqos_clks_config(true) is the only place that calls
dev_pm_opp_set_rate(dev, ethqos->noc_clk_rates[0]), so for that window
stmmaceth is running at the previously programmed 120 MHz with the vote at
zero.  The same inversion happens at probe, where
devm_stmmac_probe_config_dt() enables stmmaceth before
qcom_ethqos_init_noc_clks() installs the OPP.

At detach the devm actions unwind in the opposite order to how they were
registered, so ethqos_clks_disable() runs before
devm_stmmac_remove_config_dt():

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

The bulk disable only drops the glue's extra reference, the core still holds
one, so does this withdraw the performance-state vote while stmmaceth is
still running at 120 MHz?

The mid-sequence failure paths in ethqos_clks_config() look similar: if
clk_set_rate() fails for a later index, or clk_bulk_prepare_enable() fails,
only dev_pm_opp_set_rate(dev, 0) is issued and the rates already raised are
not rolled back.  The original stmmaceth rate is never recorded or restored
either.

> +};
> +
> +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,

[Severity: High]
Was a .rgmii_por / .num_rgmii_por table meant to be part of this patch?

All the other descriptors (emac_v2_1_0_data, emac_v2_3_0_data,
emac_v3_0_0_data, emac_v4_0_0_data) supply one, and shikra_data leaves
.link_clk_name unset, so qcom_ethqos_probe() ends up doing
devm_clk_get(dev, "rgmii"):

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

With num_rgmii_por == 0 the reset loop at the top of
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);

so RGMII_IO_MACRO_CONFIG, SDCC_HC_REG_DLL_CONFIG, SDCC_HC_REG_DDR_CONFIG and
RGMII_IO_MACRO_CONFIG2 are never brought back to a baseline before the
speed-specific programming, which is read-modify-write.

Can that leave stale bits across a speed change?  For has_emac_ge_3 (true
here) at 10/100M, ethqos_rgmii_macro_init() arms the extended RX-clock delay
path:

		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
			      SDCC_HC_REG_DDR_CONFIG);

		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
			      SDCC_HC_REG_DDR_CONFIG);
	} else {

and the 1000M branch only programs the other path:

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

EXT_PRG_RCLK_DLY_EN is never cleared, so after 100M -> 1000M both delay
paths are enabled.  On the other platforms the POR write of 0x80040800 to
SDCC_HC_REG_DDR_CONFIG clears the extended path first.

Two related fields also look like they depend on the POR table:

- SDCC_DDR_CONFIG_TCXO_CYCLES_CNT is written nowhere else in the driver, yet
  the 1G delay values are derived from "TCXO period * TCXO_CYCLES_CNT /
  (2 * RX delay ns)", which assumes the POR value of 4.
- SDCC_HC_REG_DLL_CONFIG is never restored to its 0x2004642c baseline, and
  CDR_EN / CDR_EXT_EN set by ethqos_dll_configure() at 1G are not cleared
  when dropping to 10/100M, so residual values feed the DLL_RST/PDN toggle
  and the SDC4_STATUS_DLL_LOCK poll.
- RGMII_CONFIG_MAX_SPD_PRG_2 / MAX_SPD_PRG_9 are set for 100M/10M and not
  cleared for 1000M.

If Shikra is in fact not an RGMII part, then the descriptor and the binding
added in the same series look inconsistent the other way: the binding pins
the fourth clock to const "rgmii" and the descriptor sets the RGMII-only
.rgmii_config_loopback_en field.  Which of the two is intended?

> +	.dwmac4_addrs = {

[ ... ]

> +};
> +

[ ... ]

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

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

[Konrad Dybcio]

On 9/7/26 10:23 PM, Mohd Ayaan Anwar wrote:
> 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.
> 
> 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. All three clocks in shikra_noc_clks[] must
> run at 120 MHz for NOC access, and 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>
> ---
>  .../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 8871d285941162c72e2a8f79d2eef915d9283796..c474f1e5a043777a52bcbff8a488640f6dbdca30 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 },
> +};

Can we please put this into an OPP table, so that it also casts
the required power vote?

Konrad