All of lore.kernel.org
 help / color / mirror / Atom feed
* [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock
@ 2026-10-16 19:32 Marek Vasut
  2026-10-16 19:32 ` [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT Marek Vasut
  2026-10-16 19:59 ` [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock Frank Li
  0 siblings, 2 replies; 11+ messages in thread
From: Marek Vasut @ 2026-10-16 19:32 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Christophe Leroy (CS GROUP), David S. Miller,
	Andrew Lunn, Clark Wang, Claudiu Manoil, Conor Dooley,
	Eric Dumazet, Frank Li, Jakub Kicinski, Krzysztof Kozlowski,
	Paolo Abeni, Rob Herring, UNGLinuxDriver, Vladimir Oltean,
	Wei Fang, devicetree, imx, linux-arm-kernel, linux-kernel,
	linuxppc-dev

Document clock from which the MDIO MDC clock are derived from.
While those clock are always present in every SoC, they are not
described in legacy DTs, make them optional. The clock-frequency
DT property is not described here, it is already part of mdio.yaml .

Signed-off-by: Marek Vasut <marex@nabladev.com>
---
Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: Clark Wang <xiaoning.wang@nxp.com>
Cc: Claudiu Manoil <claudiu.manoil@nxp.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@kernel.org>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: UNGLinuxDriver@microchip.com
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Wei Fang <wei.fang@nxp.com>
Cc: devicetree@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: netdev@vger.kernel.org
---
 Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml b/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
index 71c43ece82954..708d410187baa 100644
--- a/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
@@ -31,6 +31,11 @@ properties:
   reg:
     maxItems: 1
 
+  clocks:
+    items:
+      - description: A reference to the input clock of the controller
+          from which the MDC frequency is derived.
+
 required:
   - compatible
   - reg
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 19:32 [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock Marek Vasut
@ 2026-10-16 19:32 ` Marek Vasut
  2026-10-16 20:04   ` Frank Li
                     ` (2 more replies)
  2026-10-16 19:59 ` [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock Frank Li
  1 sibling, 3 replies; 11+ messages in thread
From: Marek Vasut @ 2026-10-16 19:32 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Vladimir Oltean, Christophe Leroy (CS GROUP),
	David S. Miller, Andrew Lunn, Clark Wang, Claudiu Manoil,
	Conor Dooley, Eric Dumazet, Frank Li, Jakub Kicinski,
	Krzysztof Kozlowski, Paolo Abeni, Rob Herring, UNGLinuxDriver,
	Wei Fang, devicetree, imx, linux-arm-kernel, linux-kernel,
	linuxppc-dev

The current eMDIO MDC rate is hard-coded in the drivers which use the
eMDIO MDC, this is not flexible for various deployments of SoCs which
use the eMDIO interface, for example i.MX95 B0 MDC clock is currently
set to 644 kHz.

Introduce enetc_mdio_priv .mdc_div field which contains the divider
used for MDC clock and pass it around, including into functions which
configure the MDC clock. This way, drivers which use the eMDIO can set
the divider as needed. Introduce enetc_mdio_update_mdc_div() function
which calculates the divider from upstream clock and clock-frequency
DT property, both of which are optional to retain compatibility with
older DTs. This is modeled according to xgmac_mdio_set_mdc_freq().

Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Marek Vasut <marex@nabladev.com>
---
Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: Clark Wang <xiaoning.wang@nxp.com>
Cc: Claudiu Manoil <claudiu.manoil@nxp.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@kernel.org>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: UNGLinuxDriver@microchip.com
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Wei Fang <wei.fang@nxp.com>
Cc: devicetree@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: netdev@vger.kernel.org
---
NOTE: I could only test i.MX95 , I could not test LS or Felix
---
 drivers/net/dsa/ocelot/felix_vsc9959.c        |  5 +++
 .../net/ethernet/freescale/enetc/enetc_mdio.c | 39 ++++++++++++++++---
 .../ethernet/freescale/enetc/enetc_pci_mdio.c |  5 +++
 .../freescale/enetc/enetc_pf_common.c         | 10 +++++
 include/linux/fsl/enetc_mdio.h                |  6 +++
 5 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 8387dd208adb9..dbeec1b229325 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1008,8 +1008,13 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 	 * starting with the proper offset.
 	 */
 	mdio_priv->mdio_base = 0;
+	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
 
+	rc = enetc_mdio_update_mdc_div(bus);
+	if (rc)
+		return rc;
+
 	/* Needed in order to initialize the bus mutex lock */
 	rc = mdiobus_register(bus);
 	if (rc < 0) {
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
index 998aaa394e9c4..93d61c2f1464f 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /* Copyright 2019 NXP */
 
+#include <linux/clk.h>
 #include <linux/fsl/enetc_mdio.h>
 #include <linux/mdio.h>
 #include <linux/of_mdio.h>
@@ -22,9 +23,9 @@
  /* external MDIO only - driven on neg MDC edge */
 #define MDIO_CFG_NEG		BIT(23)
 
-#define ENETC_EMDIO_CFG \
+#define ENETC_EMDIO_CFG(mdc_div) \
 	(MDIO_CFG_HOLD(2) | \
-	 MDIO_CFG_CLKDIV(258) | \
+	 MDIO_CFG_CLKDIV(mdc_div) | \
 	 MDIO_CFG_NEG)
 
 #define MDIO_CTL_DEV_ADDR(x)	((x) & 0x1f)
@@ -63,7 +64,7 @@ int enetc_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum,
 	u16 dev_addr;
 	int ret;
 
-	mdio_cfg = ENETC_EMDIO_CFG;
+	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
 	dev_addr = regnum & 0x1f;
 	mdio_cfg &= ~MDIO_CFG_ENC45;
 
@@ -95,7 +96,7 @@ int enetc_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
 	u32 mdio_ctl, mdio_cfg;
 	int ret;
 
-	mdio_cfg = ENETC_EMDIO_CFG;
+	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
 	mdio_cfg |= MDIO_CFG_ENC45;
 
 	enetc_mdio_wr(mdio_priv, ENETC_MDIO_CFG, mdio_cfg);
@@ -133,7 +134,7 @@ int enetc_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
 	u16 dev_addr, value;
 	int ret;
 
-	mdio_cfg = ENETC_EMDIO_CFG;
+	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
 	dev_addr = regnum & 0x1f;
 	mdio_cfg &= ~MDIO_CFG_ENC45;
 
@@ -176,7 +177,7 @@ int enetc_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr,
 	u16 value;
 	int ret;
 
-	mdio_cfg = ENETC_EMDIO_CFG;
+	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
 	mdio_cfg |= MDIO_CFG_ENC45;
 
 	enetc_mdio_wr(mdio_priv, ENETC_MDIO_CFG, mdio_cfg);
@@ -231,6 +232,32 @@ struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs)
 }
 EXPORT_SYMBOL_GPL(enetc_hw_alloc);
 
+int enetc_mdio_update_mdc_div(struct mii_bus *bus)
+{
+	struct enetc_mdio_priv *mdio_priv = bus->priv;
+	struct device *dev = bus->parent;
+	struct clk *enet_clk;
+	u32 div, mdc_freq;
+
+	if (device_property_read_u32(dev, "clock-frequency", &mdc_freq))
+		return 0;
+
+	enet_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(enet_clk))
+		return dev_err_probe(dev, PTR_ERR(enet_clk),
+				     "Input clock unknown, not changing MDC frequency\n");
+
+	div = ((clk_get_rate(enet_clk) / mdc_freq) - 1) / 4;
+	if (div < 5 || div > 0x1ff)
+		return dev_err_probe(dev, -EINVAL,
+				     "Requested MDC frequency is out of range, ignoring\n");
+
+	mdio_priv->mdc_div = div;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(enetc_mdio_update_mdc_div);
+
 /* Lock for MDIO access errata on LS1028A */
 DEFINE_RWLOCK(enetc_mdio_lock);
 EXPORT_SYMBOL_GPL(enetc_mdio_lock);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c
index e108cac8288d3..4ff182d7d21ec 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c
@@ -71,8 +71,13 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev,
 	mdio_priv = bus->priv;
 	mdio_priv->hw = hw;
 	mdio_priv->mdio_base = ENETC_EMDIO_BASE;
+	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
 
+	err = enetc_mdio_update_mdc_div(bus);
+	if (err)
+		return err;
+
 	pcie_flr(pdev);
 	err = pci_enable_device_mem(pdev);
 	if (err) {
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
index 8206884294a47..adba3fdea32dc 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
@@ -300,6 +300,7 @@ static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
 	bus->parent = dev;
 	mdio_priv = bus->priv;
 	mdio_priv->hw = &pf->si->hw;
+	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
 
 	if (is_enetc_rev1(pf->si))
 		mdio_priv->mdio_base = ENETC_EMDIO_BASE;
@@ -308,6 +309,10 @@ static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
 
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
 
+	err = enetc_mdio_update_mdc_div(bus);
+	if (err)
+		return err;
+
 	err = of_mdiobus_register(bus, np);
 	if (err)
 		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
@@ -350,6 +355,7 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 	bus->phy_mask = ~0;
 	mdio_priv = bus->priv;
 	mdio_priv->hw = &pf->si->hw;
+	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
 
 	if (is_enetc_rev1(pf->si))
 		mdio_priv->mdio_base = ENETC_PM_IMDIO_BASE;
@@ -358,6 +364,10 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
 
+	err = enetc_mdio_update_mdc_div(bus);
+	if (err)
+		return err;
+
 	err = mdiobus_register(bus);
 	if (err) {
 		dev_err(dev, "cannot register internal MDIO bus (%d)\n", err);
diff --git a/include/linux/fsl/enetc_mdio.h b/include/linux/fsl/enetc_mdio.h
index 623ccfcbf39c3..13eb6939efe29 100644
--- a/include/linux/fsl/enetc_mdio.h
+++ b/include/linux/fsl/enetc_mdio.h
@@ -6,6 +6,8 @@
 
 #include <linux/phy.h>
 
+#define ENETC_MDIO_DEFAULT_DIVIDER		258
+
 /* PCS registers */
 #define ENETC_PCS_LINK_TIMER1			0x12
 #define ENETC_PCS_LINK_TIMER1_VAL		0x06a0
@@ -33,6 +35,7 @@ struct enetc_hw;
 struct enetc_mdio_priv {
 	struct enetc_hw *hw;
 	int mdio_base;
+	int mdc_div;
 };
 
 #if IS_REACHABLE(CONFIG_FSL_ENETC_MDIO)
@@ -44,6 +47,7 @@ int enetc_mdio_read_c45(struct mii_bus *bus, int phy_id, int devad, int regnum);
 int enetc_mdio_write_c45(struct mii_bus *bus, int phy_id, int devad, int regnum,
 			 u16 value);
 struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs);
+int enetc_mdio_update_mdc_div(struct mii_bus *bus);
 
 #else
 
@@ -62,6 +66,8 @@ static inline int enetc_mdio_write_c45(struct mii_bus *bus, int phy_id,
 static inline struct enetc_hw *enetc_hw_alloc(struct device *dev,
 					      void __iomem *port_regs)
 { return ERR_PTR(-EINVAL); }
+static inline int enetc_mdio_update_mdc_div(struct mii_bus *bus)
+{ return -EINVAL; }
 
 #endif
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock
  2026-10-16 19:32 [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock Marek Vasut
  2026-10-16 19:32 ` [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT Marek Vasut
@ 2026-10-16 19:59 ` Frank Li
  1 sibling, 0 replies; 11+ messages in thread
From: Frank Li @ 2026-10-16 19:59 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Christophe Leroy (CS GROUP), David S. Miller, Andrew Lunn,
	Clark Wang, Claudiu Manoil, Conor Dooley, Eric Dumazet, Frank Li,
	Jakub Kicinski, Krzysztof Kozlowski, Paolo Abeni, Rob Herring,
	UNGLinuxDriver, Vladimir Oltean, Wei Fang, devicetree, imx,
	linux-arm-kernel, linux-kernel, linuxppc-dev

On Fri, Oct 16, 2026 at 09:32:00PM +0200, Marek Vasut wrote:
> Document clock from which the MDIO MDC clock are derived from.
> While those clock are always present in every SoC, they are not
> described in legacy DTs, make them optional. The clock-frequency
> DT property is not described here, it is already part of mdio.yaml .
>
> Signed-off-by: Marek Vasut <marex@nabladev.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: Clark Wang <xiaoning.wang@nxp.com>
> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Eric Dumazet <edumazet@kernel.org>
> Cc: Frank Li <Frank.Li@nxp.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: UNGLinuxDriver@microchip.com
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> Cc: Wei Fang <wei.fang@nxp.com>
> Cc: devicetree@vger.kernel.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: netdev@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml b/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
> index 71c43ece82954..708d410187baa 100644
> --- a/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
> @@ -31,6 +31,11 @@ properties:
>    reg:
>      maxItems: 1
>
> +  clocks:
> +    items:
> +      - description: A reference to the input clock of the controller
> +          from which the MDC frequency is derived.
> +
>  required:
>    - compatible
>    - reg
> --
> 2.53.0
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 19:32 ` [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT Marek Vasut
@ 2026-10-16 20:04   ` Frank Li
  2026-10-16 22:04     ` Marek Vasut
  2026-10-17  2:24     ` Wei Fang
  2026-10-16 20:59   ` Andrew Lunn
  2026-10-17 19:34   ` sashiko-bot
  2 siblings, 2 replies; 11+ messages in thread
From: Frank Li @ 2026-10-16 20:04 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Vladimir Oltean, Christophe Leroy (CS GROUP),
	David S. Miller, Andrew Lunn, Clark Wang, Claudiu Manoil,
	Conor Dooley, Eric Dumazet, Frank Li, Jakub Kicinski,
	Krzysztof Kozlowski, Paolo Abeni, Rob Herring, UNGLinuxDriver,
	Wei Fang, devicetree, imx, linux-arm-kernel, linux-kernel,
	linuxppc-dev

On Fri, Oct 16, 2026 at 09:32:01PM +0200, Marek Vasut wrote:
> The current eMDIO MDC rate is hard-coded in the drivers which use the
> eMDIO MDC, this is not flexible for various deployments of SoCs which
> use the eMDIO interface, for example i.MX95 B0 MDC clock is currently
> set to 644 kHz.
>
> Introduce enetc_mdio_priv .mdc_div field which contains the divider
> used for MDC clock and pass it around, including into functions which
> configure the MDC clock. This way, drivers which use the eMDIO can set
> the divider as needed. Introduce enetc_mdio_update_mdc_div() function
> which calculates the divider from upstream clock and clock-frequency
> DT property, both of which are optional to retain compatibility with
> older DTs. This is modeled according to xgmac_mdio_set_mdc_freq().
>
> Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Marek Vasut <marex@nabladev.com>
> ---
> Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: Clark Wang <xiaoning.wang@nxp.com>
> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Eric Dumazet <edumazet@kernel.org>
> Cc: Frank Li <Frank.Li@nxp.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: UNGLinuxDriver@microchip.com
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> Cc: Wei Fang <wei.fang@nxp.com>
> Cc: devicetree@vger.kernel.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: netdev@vger.kernel.org
> ---
> NOTE: I could only test i.MX95 , I could not test LS or Felix
> ---
>  drivers/net/dsa/ocelot/felix_vsc9959.c        |  5 +++
>  .../net/ethernet/freescale/enetc/enetc_mdio.c | 39 ++++++++++++++++---
>  .../ethernet/freescale/enetc/enetc_pci_mdio.c |  5 +++
>  .../freescale/enetc/enetc_pf_common.c         | 10 +++++
>  include/linux/fsl/enetc_mdio.h                |  6 +++
>  5 files changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 8387dd208adb9..dbeec1b229325 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -1008,8 +1008,13 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
>  	 * starting with the proper offset.
>  	 */
>  	mdio_priv->mdio_base = 0;
> +	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
>  	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
>
> +	rc = enetc_mdio_update_mdc_div(bus);
> +	if (rc)
> +		return rc;
> +
>  	/* Needed in order to initialize the bus mutex lock */
>  	rc = mdiobus_register(bus);
>  	if (rc < 0) {
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> index 998aaa394e9c4..93d61c2f1464f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>  /* Copyright 2019 NXP */
>
> +#include <linux/clk.h>
>  #include <linux/fsl/enetc_mdio.h>
>  #include <linux/mdio.h>
>  #include <linux/of_mdio.h>
> @@ -22,9 +23,9 @@
>   /* external MDIO only - driven on neg MDC edge */
>  #define MDIO_CFG_NEG		BIT(23)
>
> -#define ENETC_EMDIO_CFG \
> +#define ENETC_EMDIO_CFG(mdc_div) \
>  	(MDIO_CFG_HOLD(2) | \
> -	 MDIO_CFG_CLKDIV(258) | \
> +	 MDIO_CFG_CLKDIV(mdc_div) | \
>  	 MDIO_CFG_NEG)
>
>  #define MDIO_CTL_DEV_ADDR(x)	((x) & 0x1f)
> @@ -63,7 +64,7 @@ int enetc_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum,
>  	u16 dev_addr;
>  	int ret;
>
> -	mdio_cfg = ENETC_EMDIO_CFG;
> +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
>  	dev_addr = regnum & 0x1f;
>  	mdio_cfg &= ~MDIO_CFG_ENC45;
>
> @@ -95,7 +96,7 @@ int enetc_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
>  	u32 mdio_ctl, mdio_cfg;
>  	int ret;
>
> -	mdio_cfg = ENETC_EMDIO_CFG;
> +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
>  	mdio_cfg |= MDIO_CFG_ENC45;
>
>  	enetc_mdio_wr(mdio_priv, ENETC_MDIO_CFG, mdio_cfg);
> @@ -133,7 +134,7 @@ int enetc_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
>  	u16 dev_addr, value;
>  	int ret;
>
> -	mdio_cfg = ENETC_EMDIO_CFG;
> +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
>  	dev_addr = regnum & 0x1f;
>  	mdio_cfg &= ~MDIO_CFG_ENC45;
>
> @@ -176,7 +177,7 @@ int enetc_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr,
>  	u16 value;
>  	int ret;
>
> -	mdio_cfg = ENETC_EMDIO_CFG;
> +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
>  	mdio_cfg |= MDIO_CFG_ENC45;
>
>  	enetc_mdio_wr(mdio_priv, ENETC_MDIO_CFG, mdio_cfg);
> @@ -231,6 +232,32 @@ struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs)
>  }
>  EXPORT_SYMBOL_GPL(enetc_hw_alloc);
>
> +int enetc_mdio_update_mdc_div(struct mii_bus *bus)
> +{
> +	struct enetc_mdio_priv *mdio_priv = bus->priv;
> +	struct device *dev = bus->parent;
> +	struct clk *enet_clk;
> +	u32 div, mdc_freq;
> +
> +	if (device_property_read_u32(dev, "clock-frequency", &mdc_freq))
> +		return 0;
> +
> +	enet_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(enet_clk))
> +		return dev_err_probe(dev, PTR_ERR(enet_clk),
> +				     "Input clock unknown, not changing MDC frequency\n");

binding said clock is optional. If return failure here, it will block
existing dts, which have not clock property.

should be devm_clk_get_optional()?

Frank

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 19:32 ` [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT Marek Vasut
  2026-10-16 20:04   ` Frank Li
@ 2026-10-16 20:59   ` Andrew Lunn
  2026-10-16 22:26     ` Marek Vasut
  2026-10-17 19:34   ` sashiko-bot
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2026-10-16 20:59 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Vladimir Oltean, Christophe Leroy (CS GROUP),
	David S. Miller, Andrew Lunn, Clark Wang, Claudiu Manoil,
	Conor Dooley, Eric Dumazet, Frank Li, Jakub Kicinski,
	Krzysztof Kozlowski, Paolo Abeni, Rob Herring, UNGLinuxDriver,
	Wei Fang, devicetree, imx, linux-arm-kernel, linux-kernel,
	linuxppc-dev

> +	div = ((clk_get_rate(enet_clk) / mdc_freq) - 1) / 4;
> +	if (div < 5 || div > 0x1ff)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Requested MDC frequency is out of range, ignoring\n");

Returning -EINVAL is not really 'ignoring' the invalid value. Since
these are optional properties which are present, if they are not valid
returning an error is good. So please fix the message.

Why is dev < 5 an issue?

	Andrew

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 20:04   ` Frank Li
@ 2026-10-16 22:04     ` Marek Vasut
  2026-10-17  2:24     ` Wei Fang
  1 sibling, 0 replies; 11+ messages in thread
From: Marek Vasut @ 2026-10-16 22:04 UTC (permalink / raw)
  To: Frank Li
  Cc: netdev, Vladimir Oltean, Christophe Leroy (CS GROUP),
	David S. Miller, Andrew Lunn, Clark Wang, Claudiu Manoil,
	Conor Dooley, Eric Dumazet, Frank Li, Jakub Kicinski,
	Krzysztof Kozlowski, Paolo Abeni, Rob Herring, UNGLinuxDriver,
	Wei Fang, devicetree, imx, linux-arm-kernel, linux-kernel,
	linuxppc-dev

On 10/16/26 10:04 PM, Frank Li wrote:

[...]

>> +int enetc_mdio_update_mdc_div(struct mii_bus *bus)
>> +{
>> +	struct enetc_mdio_priv *mdio_priv = bus->priv;
>> +	struct device *dev = bus->parent;
>> +	struct clk *enet_clk;
>> +	u32 div, mdc_freq;
>> +
>> +	if (device_property_read_u32(dev, "clock-frequency", &mdc_freq))
>> +		return 0;
>> +
>> +	enet_clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(enet_clk))
>> +		return dev_err_probe(dev, PTR_ERR(enet_clk),
>> +				     "Input clock unknown, not changing MDC frequency\n");
> 
> binding said clock is optional. If return failure here, it will block
> existing dts, which have not clock property.
> 
> should be devm_clk_get_optional()?
Existing DTs are fine, since they don't define clock-frequency property. 
And whoever wants to define clock-frequency property, they also must 
provide clock property.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 20:59   ` Andrew Lunn
@ 2026-10-16 22:26     ` Marek Vasut
  2026-10-17 17:39       ` Andrew Lunn
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2026-10-16 22:26 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Vladimir Oltean, Christophe Leroy (CS GROUP),
	David S. Miller, Andrew Lunn, Clark Wang, Claudiu Manoil,
	Conor Dooley, Eric Dumazet, Frank Li, Jakub Kicinski,
	Krzysztof Kozlowski, Paolo Abeni, Rob Herring, UNGLinuxDriver,
	Wei Fang, devicetree, imx, linux-arm-kernel, linux-kernel,
	linuxppc-dev

On 10/16/26 10:59 PM, Andrew Lunn wrote:
>> +	div = ((clk_get_rate(enet_clk) / mdc_freq) - 1) / 4;
>> +	if (div < 5 || div > 0x1ff)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Requested MDC frequency is out of range, ignoring\n");
> 
> Returning -EINVAL is not really 'ignoring' the invalid value. Since
> these are optional properties which are present, if they are not valid
> returning an error is good. So please fix the message.

All right, done in V2, thanks !

> Why is dev < 5 an issue?
The clock would be way too fast, on MX95 that would be over 33 MHz.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 20:04   ` Frank Li
  2026-10-16 22:04     ` Marek Vasut
@ 2026-10-17  2:24     ` Wei Fang
  2026-10-17 17:30       ` Marek Vasut
  1 sibling, 1 reply; 11+ messages in thread
From: Wei Fang @ 2026-10-17  2:24 UTC (permalink / raw)
  To: Frank Li (OSS), Marek Vasut
  Cc: netdev@vger.kernel.org, Vladimir Oltean,
	Christophe Leroy (CS GROUP), David S. Miller, Andrew Lunn,
	Clark Wang, Claudiu Manoil, Conor Dooley, Eric Dumazet, Frank Li,
	Jakub Kicinski, Krzysztof Kozlowski, Paolo Abeni, Rob Herring,
	UNGLinuxDriver@microchip.com, devicetree@vger.kernel.org,
	imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org

> On Fri, Oct 16, 2026 at 09:32:01PM +0200, Marek Vasut wrote:
> > The current eMDIO MDC rate is hard-coded in the drivers which use the
> > eMDIO MDC, this is not flexible for various deployments of SoCs which
> > use the eMDIO interface, for example i.MX95 B0 MDC clock is currently
> > set to 644 kHz.
> >
> > Introduce enetc_mdio_priv .mdc_div field which contains the divider
> > used for MDC clock and pass it around, including into functions which
> > configure the MDC clock. This way, drivers which use the eMDIO can set
> > the divider as needed. Introduce enetc_mdio_update_mdc_div() function
> > which calculates the divider from upstream clock and clock-frequency
> > DT property, both of which are optional to retain compatibility with
> > older DTs. This is modeled according to xgmac_mdio_set_mdc_freq().
> >
> > Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Signed-off-by: Marek Vasut <marex@nabladev.com>
> > ---
> > Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> > Cc: Clark Wang <xiaoning.wang@nxp.com>
> > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: Eric Dumazet <edumazet@kernel.org>
> > Cc: Frank Li <Frank.Li@nxp.com>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: UNGLinuxDriver@microchip.com
> > Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Cc: Wei Fang <wei.fang@nxp.com>
> > Cc: devicetree@vger.kernel.org
> > Cc: imx@lists.linux.dev
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: netdev@vger.kernel.org
> > ---
> > NOTE: I could only test i.MX95 , I could not test LS or Felix
> > ---
> >  drivers/net/dsa/ocelot/felix_vsc9959.c        |  5 +++
> >  .../net/ethernet/freescale/enetc/enetc_mdio.c | 39 ++++++++++++++++---
> >  .../ethernet/freescale/enetc/enetc_pci_mdio.c |  5 +++
> >  .../freescale/enetc/enetc_pf_common.c         | 10 +++++
> >  include/linux/fsl/enetc_mdio.h                |  6 +++
> >  5 files changed, 59 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c
> b/drivers/net/dsa/ocelot/felix_vsc9959.c
> > index 8387dd208adb9..dbeec1b229325 100644
> > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> > @@ -1008,8 +1008,13 @@ static int vsc9959_mdio_bus_alloc(struct ocelot
> *ocelot)
> >  	 * starting with the proper offset.
> >  	 */
> >  	mdio_priv->mdio_base = 0;
> > +	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
> >  	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
> >
> > +	rc = enetc_mdio_update_mdc_div(bus);
> > +	if (rc)
> > +		return rc;
> > +
> >  	/* Needed in order to initialize the bus mutex lock */
> >  	rc = mdiobus_register(bus);
> >  	if (rc < 0) {
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> > index 998aaa394e9c4..93d61c2f1464f 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> >  /* Copyright 2019 NXP */
> >
> > +#include <linux/clk.h>
> >  #include <linux/fsl/enetc_mdio.h>
> >  #include <linux/mdio.h>
> >  #include <linux/of_mdio.h>
> > @@ -22,9 +23,9 @@
> >   /* external MDIO only - driven on neg MDC edge */
> >  #define MDIO_CFG_NEG		BIT(23)
> >
> > -#define ENETC_EMDIO_CFG \
> > +#define ENETC_EMDIO_CFG(mdc_div) \
> >  	(MDIO_CFG_HOLD(2) | \
> > -	 MDIO_CFG_CLKDIV(258) | \
> > +	 MDIO_CFG_CLKDIV(mdc_div) | \
> >  	 MDIO_CFG_NEG)
> >
> >  #define MDIO_CTL_DEV_ADDR(x)	((x) & 0x1f)
> > @@ -63,7 +64,7 @@ int enetc_mdio_write_c22(struct mii_bus *bus, int
> phy_id, int regnum,
> >  	u16 dev_addr;
> >  	int ret;
> >
> > -	mdio_cfg = ENETC_EMDIO_CFG;
> > +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
> >  	dev_addr = regnum & 0x1f;
> >  	mdio_cfg &= ~MDIO_CFG_ENC45;
> >
> > @@ -95,7 +96,7 @@ int enetc_mdio_write_c45(struct mii_bus *bus, int
> phy_id, int dev_addr,
> >  	u32 mdio_ctl, mdio_cfg;
> >  	int ret;
> >
> > -	mdio_cfg = ENETC_EMDIO_CFG;
> > +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
> >  	mdio_cfg |= MDIO_CFG_ENC45;
> >
> >  	enetc_mdio_wr(mdio_priv, ENETC_MDIO_CFG, mdio_cfg);
> > @@ -133,7 +134,7 @@ int enetc_mdio_read_c22(struct mii_bus *bus, int
> phy_id, int regnum)
> >  	u16 dev_addr, value;
> >  	int ret;
> >
> > -	mdio_cfg = ENETC_EMDIO_CFG;
> > +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
> >  	dev_addr = regnum & 0x1f;
> >  	mdio_cfg &= ~MDIO_CFG_ENC45;
> >
> > @@ -176,7 +177,7 @@ int enetc_mdio_read_c45(struct mii_bus *bus, int
> phy_id, int dev_addr,
> >  	u16 value;
> >  	int ret;
> >
> > -	mdio_cfg = ENETC_EMDIO_CFG;
> > +	mdio_cfg = ENETC_EMDIO_CFG(mdio_priv->mdc_div);
> >  	mdio_cfg |= MDIO_CFG_ENC45;
> >
> >  	enetc_mdio_wr(mdio_priv, ENETC_MDIO_CFG, mdio_cfg);
> > @@ -231,6 +232,32 @@ struct enetc_hw *enetc_hw_alloc(struct device
> *dev, void __iomem *port_regs)
> >  }
> >  EXPORT_SYMBOL_GPL(enetc_hw_alloc);
> >
> > +int enetc_mdio_update_mdc_div(struct mii_bus *bus)
> > +{
> > +	struct enetc_mdio_priv *mdio_priv = bus->priv;
> > +	struct device *dev = bus->parent;
> > +	struct clk *enet_clk;
> > +	u32 div, mdc_freq;
> > +
> > +	if (device_property_read_u32(dev, "clock-frequency", &mdc_freq))
> > +		return 0;
> > +
> > +	enet_clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(enet_clk))
> > +		return dev_err_probe(dev, PTR_ERR(enet_clk),
> > +				     "Input clock unknown, not changing MDC
> frequency\n");
> 
> binding said clock is optional. If return failure here, it will block
> existing dts, which have not clock property.
> 

For EMDIO device, the clocks property is not added to
fsl,enetc-mdio.yaml, I suppose this property should be added to the
dt-binding doc if the clock is really needed here.

For ENETC device, we already have a 'ref' clock in the enetc node,
so devm_clk_get(dev, NULL) may get the 'ref' clock rather than the
system clock of NETC.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-17  2:24     ` Wei Fang
@ 2026-10-17 17:30       ` Marek Vasut
  0 siblings, 0 replies; 11+ messages in thread
From: Marek Vasut @ 2026-10-17 17:30 UTC (permalink / raw)
  To: Wei Fang, Frank Li (OSS)
  Cc: netdev@vger.kernel.org, Vladimir Oltean,
	Christophe Leroy (CS GROUP), David S. Miller, Andrew Lunn,
	Clark Wang, Claudiu Manoil, Conor Dooley, Eric Dumazet, Frank Li,
	Jakub Kicinski, Krzysztof Kozlowski, Paolo Abeni, Rob Herring,
	UNGLinuxDriver@microchip.com, devicetree@vger.kernel.org,
	imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org

On 10/17/26 4:24 AM, Wei Fang wrote:

[...]

>>> +int enetc_mdio_update_mdc_div(struct mii_bus *bus)
>>> +{
>>> +	struct enetc_mdio_priv *mdio_priv = bus->priv;
>>> +	struct device *dev = bus->parent;
>>> +	struct clk *enet_clk;
>>> +	u32 div, mdc_freq;
>>> +
>>> +	if (device_property_read_u32(dev, "clock-frequency", &mdc_freq))
>>> +		return 0;
>>> +
>>> +	enet_clk = devm_clk_get(dev, NULL);
>>> +	if (IS_ERR(enet_clk))
>>> +		return dev_err_probe(dev, PTR_ERR(enet_clk),
>>> +				     "Input clock unknown, not changing MDC
>> frequency\n");
>>
>> binding said clock is optional. If return failure here, it will block
>> existing dts, which have not clock property.
>>
> 
> For EMDIO device, the clocks property is not added to
> fsl,enetc-mdio.yaml, I suppose this property should be added to the
> dt-binding doc if the clock is really needed here.

Isn't this what 1/2 in this series does ? (maybe I misunderstood this 
paragraph)

> For ENETC device, we already have a 'ref' clock in the enetc node,
> so devm_clk_get(dev, NULL) may get the 'ref' clock rather than the
> system clock of NETC.
Shouldn't the clock be described in the MDIO DT node ?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 22:26     ` Marek Vasut
@ 2026-10-17 17:39       ` Andrew Lunn
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2026-10-17 17:39 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Vladimir Oltean, Christophe Leroy (CS GROUP),
	David S. Miller, Andrew Lunn, Clark Wang, Claudiu Manoil,
	Conor Dooley, Eric Dumazet, Frank Li, Jakub Kicinski,
	Krzysztof Kozlowski, Paolo Abeni, Rob Herring, UNGLinuxDriver,
	Wei Fang, devicetree, imx, linux-arm-kernel, linux-kernel,
	linuxppc-dev

> The clock would be way too fast, on MX95 that would be over 33 MHz.

O.K. I've run systems at ~ 12Mhz. The steps where quiet big, and it
was unreliable at the next possible speed. I guess you start needing
careful design, balanced lines, termination etc for really high speed.

	Andrew


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT
  2026-10-16 19:32 ` [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT Marek Vasut
  2026-10-16 20:04   ` Frank Li
  2026-10-16 20:59   ` Andrew Lunn
@ 2026-10-17 19:34   ` sashiko-bot
  2 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-17 19:34 UTC (permalink / raw)
  To: Marek Vasut; +Cc: devicetree, imx, robh, conor+dt, Frank.Li

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: dsa: netc: uninitialized mdc_div in netc_port_create_emdio_bus()
- [High] net: enetc: enetc_mdio_update_mdc_div() queries bus->parent instead of MDIO node
- [Medium] enetc: mii_bus and pci_iomap() leaks on enetc_mdio_update_mdc_div() error
- [Medium] net: enetc: MDIO_CFG_CLKDIV() drops LSB of 9-bit MDC divider
--

commit f237be9574cf74fc03c82f92befcff8d462ed078
Author: Marek Vasut <marex@nabladev.com>

enetc: Make eMDIO MDC rate configurable from DT

This commit adds an mdc_div field to struct enetc_mdio_priv and introduces
enetc_mdio_update_mdc_div() to calculate the MDC clock divider from an
optional upstream clock and clock-frequency DT property.

> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 8387dd208adb9..dbeec1b229325 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -1008,8 +1008,13 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
>  	 * starting with the proper offset.
>  	 */
>  	mdio_priv->mdio_base = 0;
> +	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
>  	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
>  
> +	rc = enetc_mdio_update_mdc_div(bus);
> +	if (rc)
> +		return rc;

[Severity: Medium]
Does this error path leak bus?

In vsc9959_mdio_bus_alloc(), bus is allocated earlier with non-devm
mdiobus_alloc_size():

	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
	if (!bus)
		return -ENOMEM;

If enetc_mdio_update_mdc_div(bus) fails (for example, when devm_clk_get()
defers or fails, or when the calculated divider is out of range), returning
rc directly skips calling mdiobus_free(bus), unlike the mdiobus_register()
error path right below.

> +
>  	/* Needed in order to initialize the bus mutex lock */
>  	rc = mdiobus_register(bus);
>  	if (rc < 0) {
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> index 998aaa394e9c4..93d61c2f1464f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c

[ ... ]

> @@ -22,9 +23,9 @@
>   /* external MDIO only - driven on neg MDC edge */
>  #define MDIO_CFG_NEG		BIT(23)
>  
> -#define ENETC_EMDIO_CFG \
> +#define ENETC_EMDIO_CFG(mdc_div) \
>  	(MDIO_CFG_HOLD(2) | \
> -	 MDIO_CFG_CLKDIV(258) | \
> +	 MDIO_CFG_CLKDIV(mdc_div) | \
>  	 MDIO_CFG_NEG)

[Severity: High]
What happens to netc_port_create_emdio_bus() in
drivers/net/dsa/netc/netc_main.c when ENETC_EMDIO_CFG uses
mdio_priv->mdc_div?

In drivers/net/dsa/netc/netc_main.c:netc_port_create_emdio_bus(), the
driver allocates struct enetc_mdio_priv via devm_mdiobus_alloc_size() and
wires up the shared enetc_mdio callbacks without initializing
mdio_priv->mdc_div:

	bus->name = "NXP NETC switch external MDIO Bus";
	bus->read = enetc_mdio_read_c22;
	bus->write = enetc_mdio_write_c22;
	bus->read_c45 = enetc_mdio_read_c45;
	bus->write_c45 = enetc_mdio_write_c45;
	bus->parent = dev;
	mdio_priv = bus->priv;
	mdio_priv->hw = hw;
	mdio_priv->mdio_base = NETC_EMDIO_BASE;

Because devm_mdiobus_alloc_size() zero-initializes bus->priv,
mdio_priv->mdc_div remains 0. Every subsequent C22 or C45 MDIO read or
write on a NETC switch port external MDIO bus will program
MDIO_CFG_CLKDIV(0) into ENETC_MDIO_CFG instead of
ENETC_MDIO_DEFAULT_DIVIDER (258), violating the hardware minimum divider
requirement of 5.

Should netc_port_create_emdio_bus() also initialize mdio_priv->mdc_div?

[ ... ]

> @@ -231,6 +232,32 @@ struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs)
>  }
>  EXPORT_SYMBOL_GPL(enetc_hw_alloc);
>  
> +int enetc_mdio_update_mdc_div(struct mii_bus *bus)
> +{
> +	struct enetc_mdio_priv *mdio_priv = bus->priv;
> +	struct device *dev = bus->parent;
> +	struct clk *enet_clk;
> +	u32 div, mdc_freq;
> +
> +	if (device_property_read_u32(dev, "clock-frequency", &mdc_freq))
> +		return 0;
> +
> +	enet_clk = devm_clk_get(dev, NULL);

[Severity: High]
Is bus->parent the right device to query for the clock-frequency property
and input clock across all callers of enetc_mdio_update_mdc_div()?

1. When called from enetc_mdiobus_create() -> enetc_mdio_probe() in
drivers/net/ethernet/freescale/enetc/enetc_pf_common.c, bus->parent is
&pf->si->pdev->dev (the parent ENETC PF PCI device), while the external
MDIO bus is represented by the child DT node np (the mdio subnode passed to
of_mdiobus_register(bus, np)). Any clock-frequency property placed in the
mdio child node is ignored because device_property_read_u32() inspects the
parent PF device instead.

2. Furthermore, on ENETC4 PF nodes (such as pci1131,e101 in
Documentation/devicetree/bindings/net/fsl,enetc.yaml and
arch/arm64/boot/dts/freescale/imx95.dtsi), the parent PF node's clocks
property is the MAC RGMII/RMII ref clock (IMX95_CLK_ENETREF), rather than
the NETC system clock that drives MDC.

3. When called from enetc_imdio_create() and vsc9959_mdio_bus_alloc(), the
bus is an internal on-chip PCS MDIO bus (imdio) with no DT node of its own,
so querying bus->parent inspects the parent PF or switch PCI device node.

4. Also, since enet_clk is only used here to read clk_get_rate(enet_clk),
does devm_clk_get() unnecessarily hold a managed clock reference on
bus->parent for the lifetime of the device?

> +	if (IS_ERR(enet_clk))
> +		return dev_err_probe(dev, PTR_ERR(enet_clk),
> +				     "Input clock unknown, not changing MDC frequency\n");
> +
> +	div = ((clk_get_rate(enet_clk) / mdc_freq) - 1) / 4;
> +	if (div < 5 || div > 0x1ff)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Requested MDC frequency is out of range, ignoring\n");
> +
> +	mdio_priv->mdc_div = div;

[Severity: Medium]
Does MDIO_CFG_CLKDIV() drop the least-significant bit of odd 9-bit dividers
calculated here?

In drivers/net/ethernet/freescale/enetc/enetc_mdio.c, MDIO_CFG_CLKDIV is
defined just above MDIO_CFG_ENC45:

	#define MDIO_CFG_CLKDIV(x)	((((x) >> 1) & 0xff) << 8)
	#define MDIO_CFG_HOLD(x)	(((x) << 2) & GENMASK(4, 2))
	#define MDIO_CFG_ENC45		BIT(6)

Shifting x right by 1 before shifting left by 8 is equivalent to
(((x) & 0x1fe) << 7), which clears bit 7 (the LSB of the 9-bit
MDIO_CLK_DIV[8:0] field at bits [15:7]) and turns any odd divider in
[5, 0x1ff] into div - 1. In particular, when div == 5, it passes the
div < 5 check above but programs 4 into the hardware register.

When commit dd8f467eda72 ("net/fsl: xgmac_mdio: Support setting the MDC
frequency") added xgmac_mdio_set_mdc_freq(), it updated the corresponding
macro from (((x>>1) & 0xff) << 8) to (((x) & 0x1ff) << 7). Should
MDIO_CFG_CLKDIV(x) be updated the same way here?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(enetc_mdio_update_mdc_div);
> +
>  /* Lock for MDIO access errata on LS1028A */
>  DEFINE_RWLOCK(enetc_mdio_lock);
>  EXPORT_SYMBOL_GPL(enetc_mdio_lock);
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c
> index e108cac8288d3..4ff182d7d21ec 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c
> @@ -71,8 +71,13 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev,
>  	mdio_priv = bus->priv;
>  	mdio_priv->hw = hw;
>  	mdio_priv->mdio_base = ENETC_EMDIO_BASE;
> +	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
>  	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
>  
> +	err = enetc_mdio_update_mdc_div(bus);
> +	if (err)
> +		return err;

[Severity: Medium]
Does returning err directly here leak the port_regs mapping?

In enetc_pci_mdio_probe(), port_regs is mapped with non-devm pci_iomap():

	port_regs = pci_iomap(pdev, 0, 0);

When enetc_mdio_update_mdc_div(bus) fails, returning directly bypasses the
err_pci_enable / err_mdiobus_alloc / err_hw_alloc cleanup path that calls
iounmap(port_regs).

> +
>  	pcie_flr(pdev);
>  	err = pci_enable_device_mem(pdev);
>  	if (err) {
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> index d32a195a04c97..6b519871856eb 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c

[ ... ]

> @@ -332,6 +337,7 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>  	bus->phy_mask = ~0;
>  	mdio_priv = bus->priv;
>  	mdio_priv->hw = &pf->si->hw;
> +	mdio_priv->mdc_div = ENETC_MDIO_DEFAULT_DIVIDER;
>  
>  	if (is_enetc_rev1(pf->si))
>  		mdio_priv->mdio_base = ENETC_PM_IMDIO_BASE;
> @@ -340,6 +346,10 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>  
>  	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-imdio", dev_name(dev));
>  
> +	err = enetc_mdio_update_mdc_div(bus);
> +	if (err)
> +		return err;

[Severity: Medium]
Does this error path also leak bus in enetc_imdio_create()?

Here bus is allocated with non-devm mdiobus_alloc_size():

	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
	if (!bus)
		return -ENOMEM;

Returning err directly when enetc_mdio_update_mdc_div(bus) fails skips the
free_mdio_bus label that calls mdiobus_free(bus).

> +
>  	err = mdiobus_register(bus);
>  	if (err) {
>  		dev_err(dev, "cannot register internal MDIO bus (%d)\n", err);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016193344.115518-1-marex@nabladev.com?part=2

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-10-17 19:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-16 19:32 [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock Marek Vasut
2026-10-16 19:32 ` [net-next,PATCH 2/2] enetc: Make eMDIO MDC rate configurable from DT Marek Vasut
2026-10-16 20:04   ` Frank Li
2026-10-16 22:04     ` Marek Vasut
2026-10-17  2:24     ` Wei Fang
2026-10-17 17:30       ` Marek Vasut
2026-10-16 20:59   ` Andrew Lunn
2026-10-16 22:26     ` Marek Vasut
2026-10-17 17:39       ` Andrew Lunn
2026-10-17 19:34   ` sashiko-bot
2026-10-16 19:59 ` [net-next,PATCH 1/2] dt-bindings: net: enetc-mdio: Document optional MDC clock Frank Li

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.