net: mdio: implement optional PHY reset before MDIO access

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.

drivers/net/phy/mdio_bus.c +3 -14

-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

Quoted 4 lines · 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.

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.

Quoted 5 lines

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.

Quoted 25 lines

drivers/net/phy/mdio_bus.c +9 -14

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

drivers/net/phy/mdio_device.c struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr) · line 116

Quoted 4 lines

drivers/net/phy/mdio_device.c +4 -0

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

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 <[email protected]>

drivers/net/mdio/fwnode_mdio.c +6 -0

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

Quoted 6 lines

drivers/net/mdio/fwnode_mdio.c +6 -0

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

drivers/net/mdio/fwnode_mdio.c int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, · line 128

Quoted 6 lines

drivers/net/mdio/fwnode_mdio.c +6 -0

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

Quoted 10 lines

drivers/net/mdio/fwnode_mdio.c +10 -0

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

drivers/net/mdio/fwnode_mdio.c int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, · line 144

Quoted 6 lines

drivers/net/mdio/fwnode_mdio.c +6 -0

+	mdio_device_unregister_reset(tmpdev);
+	mdio_device_free(tmpdev);
+
+	return 0;
+}
+

Andrew

Quoted 6 lines · Andrew Lunn

Quoted 6 lines

drivers/net/mdio/fwnode_mdio.c +6 -0

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

Quoted 8 lines

drivers/net/mdio/fwnode_mdio.c int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, · line 128

Quoted 6 lines

drivers/net/mdio/fwnode_mdio.c +6 -0

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

Quoted 4 lines

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.

Quoted 25 lines · Buday Csaba

Quoted 6 lines · Andrew Lunn

Quoted 6 lines

drivers/net/mdio/fwnode_mdio.c +6 -0

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

Quoted 8 lines

drivers/net/mdio/fwnode_mdio.c int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, · line 128

Quoted 6 lines

drivers/net/mdio/fwnode_mdio.c +6 -0

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

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 4 lines

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

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

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.

Quoted 3 lines · 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.

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.

Quoted 3 lines

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.

Quoted 7 lines

(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