[PATCH net-next v5 0/4] net: mdio: implement optional PHY reset before MDIO access

[PATCH net-next v5 0/4] net: mdio: implement optional PHY reset before MDIO access

[Buday Csaba]

Some Ethernet PHY devices require a hard reset before any MDIO access can
be safely performed. This includes the auto-detection of the PHY ID, which
is necessary to bind the correct driver to the device.

The kernel currently does not provide a way to assert the reset before
reading the ID, making these devices usable only when the ID is hardcoded
in the Device Tree 'compatible' string.
(One notable exception is the FEC driver and its now deprecated
`phy-reset-gpios` property).

This patchset implements an optional reset before reading of the PHY ID
register, allowing such PHYs to be used with auto-detected ID. The reset
is only asserted when the current logic fails to detect the ID, ensuring
compatibility with existing systems.

There have been several earlier attempts to implement such functionality,
of which I have collected a few in the links section.

The links to the previous versions are also provided.
The most notable changes compared to v4 are:
 - -EPROBE_DEFER during the reset is propagated upward, while any other
   errors retain the original error code
 - The info level message for asserting the reset is split into a
   separate commit.

Link: https://lore.kernel.org/lkml/1499346330-12166-2-git-send-email-richard.leitner@skidata.com/
Link: https://lore.kernel.org/all/20230405-net-next-topic-net-phy-reset-v1-0-7e5329f08002@pengutronix.de/
Link: https://lore.kernel.org/netdev/20250709133222.48802-4-buday.csaba@prolan.hu/
Link: https://lore.kernel.org/all/20251013135557.62949-1-buday.csaba@prolan.hu/
Link: https://lore.kernel.org/all/20251015134503.107925-1-buday.csaba@prolan.hu/
Link: https://lore.kernel.org/netdev/cover.1760620093.git.buday.csaba@prolan.hu/
Link: https://lore.kernel.org/netdev/cover.1761124022.git.buday.csaba@prolan.hu/

Buday Csaba (4):
  net: mdio: common handling of phy reset properties
  net: mdio: change property read from fwnode_property_read_u32() to
    device_property_read_u32()
  net: mdio: reset PHY before attempting to access registers in
    fwnode_mdiobus_register_phy
  net: mdio: add message when resetting a PHY before registration

 drivers/net/mdio/fwnode_mdio.c | 48 +++++++++++++++++++++----
 drivers/net/phy/mdio_bus.c     | 39 ++------------------
 drivers/net/phy/mdio_device.c  | 66 ++++++++++++++++++++++++++++++++++
 include/linux/mdio.h           |  3 ++
 4 files changed, 113 insertions(+), 43 deletions(-)


base-commit: 00922eeaca3c5c2001781bcad40e0bd54d0fdbb6
-- 
2.39.5

[PATCH net-next v5 1/4] net: mdio: common handling of phy reset properties

[Buday Csaba]

Unify the handling of reset properties for an `mdio_device`.
Replace mdiobus_register_gpiod() and mdiobus_register_reset() with
mdio_device_register_reset() and mdio_device_unregister_reset(),
and move them from mdio_bus.c to mdio_device.c, where they belong.

The new functions handle both reset-controllers and reset-gpios,
and also read the corresponding firmware properties from the
device tree, which were previously handled in fwnode_mdio.c.
This makes tracking the reset properties easier.

The reset logic is unaltered, and should work as it did before.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
---
V4 -> V5:
 - fixed possible leak in mdio_device_register_reset() if both
   a reset-gpio and a reset-controller are present.
 - fixed whitespace
 - updated commit message
V3 -> V4: unmodified
V2 -> V3: fixed kernel-doc warnings
V1 -> V2: changed the return value of mdio_device_unregister_reset()
          to void
---
 drivers/net/mdio/fwnode_mdio.c |  5 ----
 drivers/net/phy/mdio_bus.c     | 39 ++-----------------------
 drivers/net/phy/mdio_device.c  | 53 ++++++++++++++++++++++++++++++++++
 include/linux/mdio.h           |  2 ++
 4 files changed, 57 insertions(+), 42 deletions(-)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 9b41d4697..ba7091518 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -92,11 +92,6 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 	if (fwnode_property_read_bool(child, "broken-turn-around"))
 		mdio->phy_ignore_ta_mask |= 1 << addr;
 
-	fwnode_property_read_u32(child, "reset-assert-us",
-				 &phy->mdio.reset_assert_delay);
-	fwnode_property_read_u32(child, "reset-deassert-us",
-				 &phy->mdio.reset_deassert_delay);
-
 	/* Associate the fwnode with the device structure so it
 	 * can be looked up later
 	 */
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index cad6ed3aa..cc3f9cfb1 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -33,33 +33,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/mdio.h>
 
-static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
-{
-	/* Deassert the optional reset signal */
-	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
-						 "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(mdiodev->reset_gpio))
-		return PTR_ERR(mdiodev->reset_gpio);
-
-	if (mdiodev->reset_gpio)
-		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
-
-	return 0;
-}
-
-static int mdiobus_register_reset(struct mdio_device *mdiodev)
-{
-	struct reset_control *reset;
-
-	reset = reset_control_get_optional_exclusive(&mdiodev->dev, "phy");
-	if (IS_ERR(reset))
-		return PTR_ERR(reset);
-
-	mdiodev->reset_ctrl = reset;
-
-	return 0;
-}
-
 int mdiobus_register_device(struct mdio_device *mdiodev)
 {
 	int err;
@@ -68,16 +41,9 @@ int mdiobus_register_device(struct mdio_device *mdiodev)
 		return -EBUSY;
 
 	if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) {
-		err = mdiobus_register_gpiod(mdiodev);
+		err = mdio_device_register_reset(mdiodev);
 		if (err)
 			return err;
-
-		err = mdiobus_register_reset(mdiodev);
-		if (err)
-			return err;
-
-		/* Assert the reset signal */
-		mdio_device_reset(mdiodev, 1);
 	}
 
 	mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;
@@ -91,8 +57,7 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
 	if (mdiodev->bus->mdio_map[mdiodev->addr] != mdiodev)
 		return -EINVAL;
 
-	gpiod_put(mdiodev->reset_gpio);
-	reset_control_put(mdiodev->reset_ctrl);
+	mdio_device_unregister_reset(mdiodev);
 
 	mdiodev->bus->mdio_map[mdiodev->addr] = NULL;
 
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index f64176e0e..b56a75ee3 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -74,6 +74,59 @@ struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr)
 }
 EXPORT_SYMBOL(mdio_device_create);
 
+/**
+ * mdio_device_register_reset - Read and initialize the reset properties of
+ *				an mdio device
+ * @mdiodev: mdio_device structure
+ *
+ * Return: Zero if successful, negative error code on failure
+ */
+int mdio_device_register_reset(struct mdio_device *mdiodev)
+{
+	struct reset_control *reset;
+
+	/* Read optional firmware properties */
+	fwnode_property_read_u32(dev_fwnode(&mdiodev->dev), "reset-assert-us",
+				 &mdiodev->reset_assert_delay);
+	fwnode_property_read_u32(dev_fwnode(&mdiodev->dev), "reset-deassert-us",
+				 &mdiodev->reset_deassert_delay);
+
+	/* reset-gpio, bring up deasserted */
+	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev, "reset",
+						 GPIOD_OUT_LOW);
+	if (IS_ERR(mdiodev->reset_gpio))
+		return PTR_ERR(mdiodev->reset_gpio);
+
+	if (mdiodev->reset_gpio)
+		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
+
+	reset = reset_control_get_optional_exclusive(&mdiodev->dev, "phy");
+	if (IS_ERR(reset)) {
+		gpiod_put(mdiodev->reset_gpio);
+		return PTR_ERR(reset);
+	}
+
+	mdiodev->reset_ctrl = reset;
+
+	/* Assert the reset signal */
+	mdio_device_reset(mdiodev, 1);
+
+	return 0;
+}
+EXPORT_SYMBOL(mdio_device_register_reset);
+
+/**
+ * mdio_device_unregister_reset - uninitialize the reset properties of
+ *				  an mdio device
+ * @mdiodev: mdio_device structure
+ */
+void mdio_device_unregister_reset(struct mdio_device *mdiodev)
+{
+	gpiod_put(mdiodev->reset_gpio);
+	reset_control_put(mdiodev->reset_ctrl);
+}
+EXPORT_SYMBOL(mdio_device_unregister_reset);
+
 /**
  * mdio_device_register - Register the mdio device on the MDIO bus
  * @mdiodev: mdio_device structure to be added to the MDIO bus
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 42d6d47e4..d81b63fc7 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -90,6 +90,8 @@ static inline void *mdiodev_get_drvdata(struct mdio_device *mdio)
 
 void mdio_device_free(struct mdio_device *mdiodev);
 struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr);
+int mdio_device_register_reset(struct mdio_device *mdiodev);
+void mdio_device_unregister_reset(struct mdio_device *mdiodev);
 int mdio_device_register(struct mdio_device *mdiodev);
 void mdio_device_remove(struct mdio_device *mdiodev);
 void mdio_device_reset(struct mdio_device *mdiodev, int value);
-- 
2.39.5

Re: [PATCH net-next v5 1/4] net: mdio: common handling of phy reset properties

[Andrew Lunn]

On Wed, Oct 29, 2025 at 11:23:41AM +0100, Buday Csaba wrote:
> Unify the handling of reset properties for an `mdio_device`.
> Replace mdiobus_register_gpiod() and mdiobus_register_reset() with
> mdio_device_register_reset() and mdio_device_unregister_reset(),
> and move them from mdio_bus.c to mdio_device.c, where they belong.

You should probably mention here that there are two sets of reset
properties. One set applies to the bus as a whole, and the second is
per device on the bus. You would expect the whole bus properties to be
handled in mdio_bus.c, where as the per device properties might make
more sense in mdio_device.c. So you can comment you are only moving
the per device reset code.

> 
> The new functions handle both reset-controllers and reset-gpios,
> and also read the corresponding firmware properties from the
> device tree, which were previously handled in fwnode_mdio.c.
> This makes tracking the reset properties easier.

Please split this patch.

It is normal when moving a function to just move it, make no
additional changes. That makes the change smaller, easier to review,
since it is all about, does the new location make sense?

Once it is in the new location, you can then have patches which
refactor it.

> -static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
> -{
> -	/* Deassert the optional reset signal */
> -	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
> -						 "reset", GPIOD_OUT_LOW);
> -	if (IS_ERR(mdiodev->reset_gpio))
> -		return PTR_ERR(mdiodev->reset_gpio);
> -
> -	if (mdiodev->reset_gpio)
> -		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
> -
> -	return 0;
> -}
> -

> +/**
> + * mdio_device_register_reset - Read and initialize the reset properties of
> + *				an mdio device
> + * @mdiodev: mdio_device structure
> + *
> + * Return: Zero if successful, negative error code on failure
> + */
> +int mdio_device_register_reset(struct mdio_device *mdiodev)
> +{
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mdio_device_register_reset);

Before it did not require an EXPORT_SYMBOL, but now it does?  That
makes me wounder if it is in the correct place. This should be a core
function, why would something outside of the core need it?

Also, please use the _GPL variant.

	Andrew

Re: [PATCH net-next v5 1/4] net: mdio: common handling of phy reset properties

[Buday Csaba]

On Wed, Oct 29, 2025 at 02:03:30PM +0100, Andrew Lunn wrote:
> On Wed, Oct 29, 2025 at 11:23:41AM +0100, Buday Csaba wrote:
> > Unify the handling of reset properties for an `mdio_device`.
> > Replace mdiobus_register_gpiod() and mdiobus_register_reset() with
> > mdio_device_register_reset() and mdio_device_unregister_reset(),
> > and move them from mdio_bus.c to mdio_device.c, where they belong.
> 
> You should probably mention here that there are two sets of reset
> properties. One set applies to the bus as a whole, and the second is
> per device on the bus. You would expect the whole bus properties to be
> handled in mdio_bus.c, where as the per device properties might make
> more sense in mdio_device.c. So you can comment you are only moving
> the per device reset code.
> 

Okay, I will do that.

> > 
> > The new functions handle both reset-controllers and reset-gpios,
> > and also read the corresponding firmware properties from the
> > device tree, which were previously handled in fwnode_mdio.c.
> > This makes tracking the reset properties easier.
> 
> Please split this patch.
> 
> It is normal when moving a function to just move it, make no
> additional changes. That makes the change smaller, easier to review,
> since it is all about, does the new location make sense?
> 
> Once it is in the new location, you can then have patches which
> refactor it.

Understood, I will change it accordingly.

> 
> > -static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
> > -{
> > -	/* Deassert the optional reset signal */
> > -	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
> > -						 "reset", GPIOD_OUT_LOW);
> > -	if (IS_ERR(mdiodev->reset_gpio))
> > -		return PTR_ERR(mdiodev->reset_gpio);
> > -
> > -	if (mdiodev->reset_gpio)
> > -		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
> > -
> > -	return 0;
> > -}
> > -
> 
> > +/**
> > + * mdio_device_register_reset - Read and initialize the reset properties of
> > + *				an mdio device
> > + * @mdiodev: mdio_device structure
> > + *
> > + * Return: Zero if successful, negative error code on failure
> > + */
> > +int mdio_device_register_reset(struct mdio_device *mdiodev)
> > +{
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mdio_device_register_reset);
> 
> Before it did not require an EXPORT_SYMBOL, but now it does?  That
> makes me wounder if it is in the correct place. This should be a core
> function, why would something outside of the core need it?

I am using these in the third patch in fwnode_mdio.c, so I assumed it was
necessary. But you are right, neither can be in a module. I will remove it.

> 
> Also, please use the _GPL variant.
> 
> 	Andrew
> 

Csaba

[PATCH net-next v5 2/4] net: mdio: change property read from fwnode_property_read_u32() to device_property_read_u32()

[Buday Csaba]

Change fwnode_property_read_u32() in mdio_device_register_reset()
to device_property_read_u32(), which is more appropriate here.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
---
V4 -> V5: tweaked commit message
V3 -> V4: unmodified
V2 -> V3: unmodified
V1 -> V2: added new patch based on maintainer request
---
 drivers/net/phy/mdio_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index b56a75ee3..e24bce474 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -86,9 +86,9 @@ int mdio_device_register_reset(struct mdio_device *mdiodev)
 	struct reset_control *reset;
 
 	/* Read optional firmware properties */
-	fwnode_property_read_u32(dev_fwnode(&mdiodev->dev), "reset-assert-us",
+	device_property_read_u32(&mdiodev->dev, "reset-assert-us",
 				 &mdiodev->reset_assert_delay);
-	fwnode_property_read_u32(dev_fwnode(&mdiodev->dev), "reset-deassert-us",
+	device_property_read_u32(&mdiodev->dev, "reset-deassert-us",
 				 &mdiodev->reset_deassert_delay);
 
 	/* reset-gpio, bring up deasserted */
-- 
2.39.5

Re: [PATCH net-next v5 2/4] net: mdio: change property read from fwnode_property_read_u32() to device_property_read_u32()

[Andrew Lunn]

On Wed, Oct 29, 2025 at 11:23:42AM +0100, Buday Csaba wrote:
> Change fwnode_property_read_u32() in mdio_device_register_reset()
> to device_property_read_u32(), which is more appropriate here.
> 
> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>

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

    Andrew

[PATCH net-next v5 3/4] net: mdio: reset PHY before attempting to access registers in fwnode_mdiobus_register_phy

[Buday Csaba]

When the ID of an Ethernet PHY is not provided by the 'compatible'
string in the device tree, its actual ID is read via the MDIO bus.
For some PHYs this could be unsafe, since a hard reset may be
necessary to safely access the MDIO registers.

Add a fallback mechanism for these devices: when reading the
ID fails, the reset will be asserted, and the ID read is retried.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
---
V4 -> V5:
 - when fwnode_reset_phy() returns with -EPROBE_DEFER, that error
   is now propagated upstream instead of being discarded.
 - info message removed (it will be a separate commit)
 - 'err' variable changed to 'rc' in fwnode_reset_phy()
 - removed last fwnode_handle_put() in fwnode_reset_phy()
   The cleanup is performed by mdio_device_free().
V3 -> V4: dropped DT property `phy-id-read-needs-reset` to control
	  the reset behaviour, asserting reset as a fallback
	  mechanism instead. Added info level message for resetting.
V2 -> V3: kernel-doc replaced with a comment (fixed warning)
V1 -> V2:
 - renamed fwnode_reset_phy_before_probe() to
   fwnode_reset_phy()
 - added kernel-doc for fwnode_reset_phy()
 - improved error handling in fwnode_reset_phy()
---
 drivers/net/mdio/fwnode_mdio.c | 38 +++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index ba7091518..9669da2b7 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -114,6 +114,34 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 }
 EXPORT_SYMBOL(fwnode_mdiobus_phy_device_register);
 
+/* Hard-reset a PHY before registration */
+static int fwnode_reset_phy(struct mii_bus *bus, u32 addr,
+			    struct fwnode_handle *phy_node)
+{
+	struct mdio_device *tmpdev;
+	int rc;
+
+	tmpdev = mdio_device_create(bus, addr);
+	if (IS_ERR(tmpdev))
+		return PTR_ERR(tmpdev);
+
+	fwnode_handle_get(phy_node);
+	device_set_node(&tmpdev->dev, phy_node);
+	rc = mdio_device_register_reset(tmpdev);
+	if (rc) {
+		mdio_device_free(tmpdev);
+		return rc;
+	}
+
+	mdio_device_reset(tmpdev, 1);
+	mdio_device_reset(tmpdev, 0);
+
+	mdio_device_unregister_reset(tmpdev);
+	mdio_device_free(tmpdev);
+
+	return 0;
+}
+
 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 				struct fwnode_handle *child, u32 addr)
 {
@@ -129,8 +157,16 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 		return PTR_ERR(mii_ts);
 
 	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
-	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+	if (is_c45 || fwnode_get_phy_id(child, &phy_id)) {
 		phy = get_phy_device(bus, addr, is_c45);
+		if (IS_ERR(phy)) {
+			rc = fwnode_reset_phy(bus, addr, child);
+			if (rc == -EPROBE_DEFER)
+				goto clean_mii_ts;
+			else if (!rc)
+				phy = get_phy_device(bus, addr, is_c45);
+		}
+	}
 	else
 		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
 	if (IS_ERR(phy)) {
-- 
2.39.5

Re: [PATCH net-next v5 3/4] net: mdio: reset PHY before attempting to access registers in fwnode_mdiobus_register_phy

[Andrew Lunn]

> +/* Hard-reset a PHY before registration */
> +static int fwnode_reset_phy(struct mii_bus *bus, u32 addr,
> +			    struct fwnode_handle *phy_node)
> +{
> +	struct mdio_device *tmpdev;
> +	int rc;
> +
> +	tmpdev = mdio_device_create(bus, addr);
> +	if (IS_ERR(tmpdev))
> +		return PTR_ERR(tmpdev);
> +
> +	fwnode_handle_get(phy_node);

You add a _get() here. Where is the corresponding _put()?

Also, fwnode_handle_get() returns a handle. Why do you throw it away?
What is the point of this get?

> +	device_set_node(&tmpdev->dev, phy_node);
> +	rc = mdio_device_register_reset(tmpdev);
> +	if (rc) {
> +		mdio_device_free(tmpdev);
> +		return rc;
> +	}
> +
> +	mdio_device_reset(tmpdev, 1);
> +	mdio_device_reset(tmpdev, 0);
> +
> +	mdio_device_unregister_reset(tmpdev);
> +	mdio_device_free(tmpdev);
> +
> +	return 0;
> +}
> +

	Andrew

Re: [PATCH net-next v5 3/4] net: mdio: reset PHY before attempting to access registers in fwnode_mdiobus_register_phy

[Buday Csaba]

On Wed, Oct 29, 2025 at 02:20:14PM +0100, Andrew Lunn wrote:
> > +/* Hard-reset a PHY before registration */
> > +static int fwnode_reset_phy(struct mii_bus *bus, u32 addr,
> > +			    struct fwnode_handle *phy_node)
> > +{
> > +	struct mdio_device *tmpdev;
> > +	int rc;
> > +
> > +	tmpdev = mdio_device_create(bus, addr);
> > +	if (IS_ERR(tmpdev))
> > +		return PTR_ERR(tmpdev);
> > +
> > +	fwnode_handle_get(phy_node);
> 
> You add a _get() here. Where is the corresponding _put()?

When mdio_device_free() is called, it eventually invokes
mdio_device_release(). There is the corresponding _put(), that will
release the reference. I also verified this with a stack trace.

> 
> Also, fwnode_handle_get() returns a handle. Why do you throw it away?
> What is the point of this get?
>

I copied this initialization stub from of_mdiobus_register_device()
in of_mdio.c. The same pattern is used there:

	fwnode_handle_get(fwnode);
	device_set_node(&mdiodev->dev, fwnode);

It is kind of awkward that we need to half-establish a device, just
to assert the reset, but I could not think of any better solution, that
does not lead to a large amount of code duplication.

> > +	device_set_node(&tmpdev->dev, phy_node);
> > +	rc = mdio_device_register_reset(tmpdev);
> > +	if (rc) {
> > +		mdio_device_free(tmpdev);
> > +		return rc;
> > +	}
> > +
> > +	mdio_device_reset(tmpdev, 1);
> > +	mdio_device_reset(tmpdev, 0);
> > +
> > +	mdio_device_unregister_reset(tmpdev);
> > +	mdio_device_free(tmpdev);
> > +
> > +	return 0;
> > +}
> > +
> 
> 	Andrew
> 

Csaba

Re: [PATCH net-next v5 3/4] net: mdio: reset PHY before attempting to access registers in fwnode_mdiobus_register_phy

[Andrew Lunn]

On Wed, Oct 29, 2025 at 03:15:27PM +0100, Buday Csaba wrote:
> On Wed, Oct 29, 2025 at 02:20:14PM +0100, Andrew Lunn wrote:
> > > +/* Hard-reset a PHY before registration */
> > > +static int fwnode_reset_phy(struct mii_bus *bus, u32 addr,
> > > +			    struct fwnode_handle *phy_node)
> > > +{
> > > +	struct mdio_device *tmpdev;
> > > +	int rc;
> > > +
> > > +	tmpdev = mdio_device_create(bus, addr);
> > > +	if (IS_ERR(tmpdev))
> > > +		return PTR_ERR(tmpdev);
> > > +
> > > +	fwnode_handle_get(phy_node);
> > 
> > You add a _get() here. Where is the corresponding _put()?
> 
> When mdio_device_free() is called, it eventually invokes
> mdio_device_release(). There is the corresponding _put(), that will
> release the reference. I also verified this with a stack trace.
> 
> > 
> > Also, fwnode_handle_get() returns a handle. Why do you throw it away?
> > What is the point of this get?
> >
> 
> I copied this initialization stub from of_mdiobus_register_device()
> in of_mdio.c. The same pattern is used there:
> 
> 	fwnode_handle_get(fwnode);
> 	device_set_node(&mdiodev->dev, fwnode);

This looks broken, but i'm not sure...

static int of_mdiobus_register_device(struct mii_bus *mdio,
				      struct device_node *child, u32 addr)
{
	struct fwnode_handle *fwnode = of_fwnode_handle(child);
	struct mdio_device *mdiodev;
	int rc;

	mdiodev = mdio_device_create(mdio, addr);
	if (IS_ERR(mdiodev))
		return PTR_ERR(mdiodev);

	/* Associate the OF node with the device structure so it
	 * can be looked up later.
	 */
	fwnode_handle_get(fwnode);
	device_set_node(&mdiodev->dev, fwnode);

	/* All data is now stored in the mdiodev struct; register it. */
	rc = mdio_device_register(mdiodev);
	if (rc) {
		device_set_node(&mdiodev->dev, NULL);
		fwnode_handle_put(fwnode);
		mdio_device_free(mdiodev);

In this error handling, it appears the fwnode is put() and then the
mdiodev freed. I assume that results in a call to
mdio_device_release() which does a second put() on fwnode.

That is why code like this should look symmetric. If the put() is in
free, the get() should be in the create.

> It is kind of awkward that we need to half-establish a device, just
> to assert the reset, but I could not think of any better solution, that
> does not lead to a large amount of code duplication.

And this is another argument against this approach. What does the
documentation say about what you can do with a half-established
device?

	Andrew

Re: [PATCH net-next v5 3/4] net: mdio: reset PHY before attempting to access registers in fwnode_mdiobus_register_phy

[Buday Csaba]

On Wed, Oct 29, 2025 at 04:15:27PM +0100, Andrew Lunn wrote:
> On Wed, Oct 29, 2025 at 03:15:27PM +0100, Buday Csaba wrote:
> > On Wed, Oct 29, 2025 at 02:20:14PM +0100, Andrew Lunn wrote:
> > > > +/* Hard-reset a PHY before registration */
> > > > +static int fwnode_reset_phy(struct mii_bus *bus, u32 addr,
> > > > +			    struct fwnode_handle *phy_node)
> > > > +{
> > > > +	struct mdio_device *tmpdev;
> > > > +	int rc;
> > > > +
> > > > +	tmpdev = mdio_device_create(bus, addr);
> > > > +	if (IS_ERR(tmpdev))
> > > > +		return PTR_ERR(tmpdev);
> > > > +
> > > > +	fwnode_handle_get(phy_node);
> > > 
> > > You add a _get() here. Where is the corresponding _put()?
> > 
> > When mdio_device_free() is called, it eventually invokes
> > mdio_device_release(). There is the corresponding _put(), that will
> > release the reference. I also verified this with a stack trace.
> > 
> > > 
> > > Also, fwnode_handle_get() returns a handle. Why do you throw it away?
> > > What is the point of this get?
> > >
> > 
> > I copied this initialization stub from of_mdiobus_register_device()
> > in of_mdio.c. The same pattern is used there:
> > 
> > 	fwnode_handle_get(fwnode);
> > 	device_set_node(&mdiodev->dev, fwnode);
> 
> This looks broken, but i'm not sure...
> 
> static int of_mdiobus_register_device(struct mii_bus *mdio,
> 				      struct device_node *child, u32 addr)
> {
> 	struct fwnode_handle *fwnode = of_fwnode_handle(child);
> 	struct mdio_device *mdiodev;
> 	int rc;
> 
> 	mdiodev = mdio_device_create(mdio, addr);
> 	if (IS_ERR(mdiodev))
> 		return PTR_ERR(mdiodev);
> 
> 	/* Associate the OF node with the device structure so it
> 	 * can be looked up later.
> 	 */
> 	fwnode_handle_get(fwnode);
> 	device_set_node(&mdiodev->dev, fwnode);
> 
> 	/* All data is now stored in the mdiodev struct; register it. */
> 	rc = mdio_device_register(mdiodev);
> 	if (rc) {
> 		device_set_node(&mdiodev->dev, NULL);
> 		fwnode_handle_put(fwnode);
> 		mdio_device_free(mdiodev);
> 
> In this error handling, it appears the fwnode is put() and then the
> mdiodev freed. I assume that results in a call to
> mdio_device_release() which does a second put() on fwnode.
> 
> That is why code like this should look symmetric. If the put() is in
> free, the get() should be in the create.

I totally agree with that, but I have nothing to do with that code.
It did also confuse me at first, that is why my earlier versions also had
a put(), just not in the error handling path.

> 
> > It is kind of awkward that we need to half-establish a device, just
> > to assert the reset, but I could not think of any better solution, that
> > does not lead to a large amount of code duplication.
> 
> And this is another argument against this approach. What does the
> documentation say about what you can do with a half-established
> device?
> 
> 	Andrew
> 

But that device never actually leaves fwnode_reset_phy(). It is contained.
That was the whole point of the first patch: to avoid code duplication
as much as possible.
In order to assert and deassert the reset, you have many things to set up:
read DT properties, claim the GPIO or the reset controller (which is only
possible for a device, is it not?), then perform the actual
assertion/deassertion.
These functions currently exist for an mdio device, why not use them?

After these patches fwnode_reset_phy() is reasonably structured, at least
I think so. The temporary device is created, reset properties initialized,
reset performed, then cleaned up on exit.

I understand your concerns about the functionality itself. Yes, it may be
better handled by the driver, and it is just to gain a little convenience.
But that is more of a philosophical argument. If I send a next version,
I can not address that. I can only address technical concerns. If your
opinion is, that this feature is not wanted, then there is no point in
sending a next version.
I personally think that autodetection works reasonably well for most of the
devices, so expanding this set a little bit is a nice thing.
But that decision is ultimately for you -maintainers- to make.

I also think that the documentation should reflect clearly when and why
specifying the PHY ID in the DT is necessary, and wheter it is preferred
or not. I would be happy to make such a patch, once the decision is made.

That would have have saved us a lot of development time, and I imagine
there are others in the same shoes.

Thank you for the thorough review!
Csaba

[PATCH net-next v5 4/4] net: mdio: add message when resetting a PHY before registration

[Buday Csaba]

Add an info level message when resetting a PHY before registration.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
---
V4 -> V5: moved the info message to a separate commit
---
 drivers/net/mdio/fwnode_mdio.c |  9 +++++++--
 drivers/net/phy/mdio_device.c  | 13 +++++++++++++
 include/linux/mdio.h           |  1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 9669da2b7..d753ebd37 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -133,8 +133,13 @@ static int fwnode_reset_phy(struct mii_bus *bus, u32 addr,
 		return rc;
 	}
 
-	mdio_device_reset(tmpdev, 1);
-	mdio_device_reset(tmpdev, 0);
+	if (mdio_device_has_reset(tmpdev)) {
+		dev_info(&bus->dev,
+			 "PHY device at address %d not detected, resetting PHY.\n",
+			 addr);
+		mdio_device_reset(tmpdev, 1);
+		mdio_device_reset(tmpdev, 0);
+	}
 
 	mdio_device_unregister_reset(tmpdev);
 	mdio_device_free(tmpdev);
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index e24bce474..f2cbcb6bb 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -171,6 +171,19 @@ void mdio_device_remove(struct mdio_device *mdiodev)
 }
 EXPORT_SYMBOL(mdio_device_remove);
 
+/**
+ * mdio_device_has_reset - Check if an MDIO device has reset properties defined
+ * @mdiodev: mdio_device structure
+ *
+ * Return: non-zero if the device has a reset GPIO or reset controller,
+ *         zero otherwise.
+ */
+int mdio_device_has_reset(struct mdio_device *mdiodev)
+{
+	return (mdiodev->reset_gpio || mdiodev->reset_ctrl);
+}
+EXPORT_SYMBOL(mdio_device_has_reset);
+
 void mdio_device_reset(struct mdio_device *mdiodev, int value)
 {
 	unsigned int d;
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index d81b63fc7..83cfc051e 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -92,6 +92,7 @@ void mdio_device_free(struct mdio_device *mdiodev);
 struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr);
 int mdio_device_register_reset(struct mdio_device *mdiodev);
 void mdio_device_unregister_reset(struct mdio_device *mdiodev);
+int mdio_device_has_reset(struct mdio_device *mdiodev);
 int mdio_device_register(struct mdio_device *mdiodev);
 void mdio_device_remove(struct mdio_device *mdiodev);
 void mdio_device_reset(struct mdio_device *mdiodev, int value);
-- 
2.39.5

Re: [PATCH net-next v5 4/4] net: mdio: add message when resetting a PHY before registration

[Andrew Lunn]

On Wed, Oct 29, 2025 at 11:23:44AM +0100, Buday Csaba wrote:
> Add an info level message when resetting a PHY before registration.

Please add to the commit message the answer to the question Why?

	Andrew

Re: [PATCH net-next v5 0/4] net: mdio: implement optional PHY reset before MDIO access

[Andrew Lunn]

On Wed, Oct 29, 2025 at 11:23:40AM +0100, Buday Csaba wrote:
> Some Ethernet PHY devices require a hard reset before any MDIO access can
> be safely performed. This includes the auto-detection of the PHY ID, which
> is necessary to bind the correct driver to the device.

nitpicking a bit, but this last part is not strictly correct. You can
also bind the correct driver to the PHY using a compatible. So it is
not 'necessary'. It is maybe the preferred way to do it, although the
DT Maintainers my disagree and say compatible is the preferred way.

> The kernel currently does not provide a way to assert the reset before
> reading the ID, making these devices usable only when the ID is hardcoded
> in the Device Tree 'compatible' string.

Which is what you say here.

> (One notable exception is the FEC driver and its now deprecated
> `phy-reset-gpios` property).

> This patchset implements an optional reset before reading of the PHY ID
> register, allowing such PHYs to be used with auto-detected ID. The reset
> is only asserted when the current logic fails to detect the ID, ensuring
> compatibility with existing systems.

O.K, that is new.

One of the arguments raised against making this more complex is that
next somebody will want to add clock support. And should that be
enabled before or after the reset? And then regulators, and what order
should that be done in? The core cannot answer these questions, only
the driver can. The compatible should be used to get the driver loaded
and then it can enable these resources in the correct order.

I will look at the patches anyway.

  Andrew

Re: [PATCH net-next v5 0/4] net: mdio: implement optional PHY reset before MDIO access

[Buday Csaba]

On Wed, Oct 29, 2025 at 01:43:32PM +0100, Andrew Lunn wrote:
> On Wed, Oct 29, 2025 at 11:23:40AM +0100, Buday Csaba wrote:
> > Some Ethernet PHY devices require a hard reset before any MDIO access can
> > be safely performed. This includes the auto-detection of the PHY ID, which
> > is necessary to bind the correct driver to the device.
> 
> nitpicking a bit, but this last part is not strictly correct. You can
> also bind the correct driver to the PHY using a compatible. So it is
> not 'necessary'. It is maybe the preferred way to do it, although the
> DT Maintainers my disagree and say compatible is the preferred way.
> 

I have also gotten the impression, that DT people generally prefer
hardcoding the ID. I can not argue with that. But that should be clearly
reflected in the documentation. Now the description in ethernet-phy.yaml
suggests that a correct ID only is a workaround for misbehaving PHYs:

"If the PHY reports an incorrect ID (or none at all) then the
compatible list may contain an entry with the correct PHY ID
in the above form."

> > The kernel currently does not provide a way to assert the reset before
> > reading the ID, making these devices usable only when the ID is hardcoded
> > in the Device Tree 'compatible' string.
> 
> Which is what you say here.
> 
> > (One notable exception is the FEC driver and its now deprecated
> > `phy-reset-gpios` property).
> 
> > This patchset implements an optional reset before reading of the PHY ID
> > register, allowing such PHYs to be used with auto-detected ID. The reset
> > is only asserted when the current logic fails to detect the ID, ensuring
> > compatibility with existing systems.
> 
> O.K, that is new.
> 
> One of the arguments raised against making this more complex is that
> next somebody will want to add clock support. And should that be
> enabled before or after the reset? And then regulators, and what order
> should that be done in? The core cannot answer these questions, only
> the driver can. The compatible should be used to get the driver loaded
> and then it can enable these resources in the correct order.
> 

Again, I can not argue with that. I can only tell that these patch fixed
our problem - I hope that others may also benefit from it.

> I will look at the patches anyway.
> 
>   Andrew
> 

Really appreciate it, thanks!

Csaba