[PATCH 1/3] net: phy: smsc: add proper reset flags for LAN8710A

[PATCH 1/3] net: phy: smsc: add proper reset flags for LAN8710A

[Buday Csaba]

According to the LAN8710A datasheet (Rev. B, section 3.8.5.1), a hardware
reset is required after power-on, and the reference clock (REF_CLK) must be
established before asserting reset.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
Cc: Csókás Bence <csokas.bence@prolan.hu>
---
 drivers/net/phy/smsc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 150aea7c9c367..357e16aa4a736 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -737,6 +737,7 @@ static struct phy_driver smsc_phy_driver[] = {
 
 	/* PHY_BASIC_FEATURES */
 
+	.flags		= PHY_RST_AFTER_CLK_EN,
 	.probe		= smsc_phy_probe,
 
 	/* basic functions */
-- 
2.39.5

[PATCH 2/3] net: mdiobus: release reset_gpio in mdiobus_unregister_device()

[Buday Csaba]

reset_gpio is claimed in mdiobus_register_device(), but it is not
released in mdiobus_unregister_device().
When a device uses the reset_gpio property, it becomes impossible
to unregister it and register it again, because the GPIO remains
claimed.
This patch resolves that issue.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
Cc: Csókás Bence <csokas.bence@prolan.hu>
---
 drivers/net/phy/mdio_bus.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 591e8fd33d8ea..a508cd81cd4ed 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -97,6 +97,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);
 
 	mdiodev->bus->mdio_map[mdiodev->addr] = NULL;
@@ -814,9 +815,6 @@ void mdiobus_unregister(struct mii_bus *bus)
 		if (!mdiodev)
 			continue;
 
-		if (mdiodev->reset_gpio)
-			gpiod_put(mdiodev->reset_gpio);
-
 		mdiodev->device_remove(mdiodev);
 		mdiodev->device_free(mdiodev);
 	}
-- 
2.39.5

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

[Buday Csaba]

Some PHYs (e.g. LAN8710A) require a reset after power-on,even for
MDIO register access.
The current implementation of fwnode_mdiobus_register_phy() and
get_phy_device() attempt to read the id registers without ensuring
that the PHY had a reset before, which can fail on these devices.

This patch addresses that shortcoming, by always resetting the PHY
(when such property is given in the device tree). To keep the code
impact minimal, a change was also needed in phy_device_remove() to
prevent asserting the reset on device removal.

According to the documentation of phy_device_remove(), it should
reverse the effect of phy_device_register(). Since the reset GPIO
is in undefined state before that, it should be acceptable to leave
it unchanged during removal.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
Cc: Csókás Bence <csokas.bence@prolan.hu>
---
 drivers/net/mdio/fwnode_mdio.c | 20 ++++++++++++++++++--
 drivers/net/phy/phy_device.c   |  3 ---
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index aea0f03575689..36b60544327b6 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -139,8 +139,24 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	}
 
 	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
-	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
-		phy = get_phy_device(bus, addr, is_c45);
+	if (is_c45 || fwnode_get_phy_id(child, &phy_id)) {
+		/* get_phy_device is NOT SAFE HERE, since the PHY may need a HW RESET.
+		 * First create a dummy PHY device, reset the PHY, then call
+		 * get_phy_device.
+		 */
+		phy = phy_device_create(bus, addr, 0, 0, NULL);
+		if (!IS_ERR(phy)) {
+			if (is_of_node(child)) {
+				/* fwnode_mdiobus_phy_device_register performs the reset */
+				rc = fwnode_mdiobus_phy_device_register(bus, phy, child, addr);
+				if (!rc)
+					phy_device_remove(phy);
+				/* PHY has been reset at this point. */
+			}
+			phy_device_free(phy);
+			phy = get_phy_device(bus, addr, is_c45);
+		}
+	}
 	else
 		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
 	if (IS_ERR(phy)) {
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 13dea33d86ffa..da4ddce04e5fb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1102,9 +1102,6 @@ void phy_device_remove(struct phy_device *phydev)
 
 	device_del(&phydev->mdio.dev);
 
-	/* Assert the reset signal */
-	phy_device_reset(phydev, 1);
-
 	mdiobus_unregister_device(&phydev->mdio);
 }
 EXPORT_SYMBOL(phy_device_remove);
-- 
2.39.5

Re: [PATCH 2/3] net: mdiobus: release reset_gpio in mdiobus_unregister_device()

[Andrew Lunn]

On Wed, Jul 09, 2025 at 03:32:21PM +0200, Buday Csaba wrote:
> reset_gpio is claimed in mdiobus_register_device(), but it is not
> released in mdiobus_unregister_device().
> When a device uses the reset_gpio property, it becomes impossible
> to unregister it and register it again, because the GPIO remains
> claimed.
> This patch resolves that issue.
> 
> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
> Cc: Csókás Bence <csokas.bence@prolan.hu>

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

    Andrew

Re: [PATCH 1/3] net: phy: smsc: add proper reset flags for LAN8710A

[Andrew Lunn]

On Wed, Jul 09, 2025 at 03:32:20PM +0200, Buday Csaba wrote:
> According to the LAN8710A datasheet (Rev. B, section 3.8.5.1), a hardware
> reset is required after power-on, and the reference clock (REF_CLK) must be
> established before asserting reset.
> 
> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
> Cc: Csókás Bence <csokas.bence@prolan.hu>

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

    Andrew

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

[Andrew Lunn]

On Wed, Jul 09, 2025 at 03:32:22PM +0200, Buday Csaba wrote:
> Some PHYs (e.g. LAN8710A) require a reset after power-on,even for
> MDIO register access.
> The current implementation of fwnode_mdiobus_register_phy() and
> get_phy_device() attempt to read the id registers without ensuring
> that the PHY had a reset before, which can fail on these devices.
> 
> This patch addresses that shortcoming, by always resetting the PHY
> (when such property is given in the device tree). To keep the code
> impact minimal, a change was also needed in phy_device_remove() to
> prevent asserting the reset on device removal.
> 
> According to the documentation of phy_device_remove(), it should
> reverse the effect of phy_device_register(). Since the reset GPIO
> is in undefined state before that, it should be acceptable to leave
> it unchanged during removal.
> 
> Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
> Cc: Csókás Bence <csokas.bence@prolan.hu>

This appears to be a respost of the previous patch.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html says:

don’t repost your patches within one 24h period

    Andrew

---
pw-bot: cr

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

[Buday Csaba]

On Wed, Jul 09, 2025 at 03:41:45PM +0200, Andrew Lunn wrote:
> On Wed, Jul 09, 2025 at 03:32:22PM +0200, Buday Csaba wrote:
> > Some PHYs (e.g. LAN8710A) require a reset after power-on,even for
> > MDIO register access.
> > The current implementation of fwnode_mdiobus_register_phy() and
> > get_phy_device() attempt to read the id registers without ensuring
> > that the PHY had a reset before, which can fail on these devices.
> >
> > This patch addresses that shortcoming, by always resetting the PHY
> > (when such property is given in the device tree). To keep the code
> > impact minimal, a change was also needed in phy_device_remove() to
> > prevent asserting the reset on device removal.
> >
> > According to the documentation of phy_device_remove(), it should
> > reverse the effect of phy_device_register(). Since the reset GPIO
> > is in undefined state before that, it should be acceptable to leave
> > it unchanged during removal.
> >
> > Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
> > Cc: Csókás Bence <csokas.bence@prolan.hu>
>
> This appears to be a respost of the previous patch.
>
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html says:
>
> don’t repost your patches within one 24h period

Sorry for that, this is my first kernel patch, and git send-email tricked me.
I will paste your earlier reply below, and continue in this thread.

>
> This is specific to this device, so the driver for this device should
> take care of the reset.

I think it may affect every chip that has the `PHY_RST_AFTER_CLK_EN`
flag set. That is still not a terribly lot of devices, but it is more than
just this one. There are a lot of DT-s out there which use the (deprecated?)
`phy-reset-gpios` property of the fec driver. When that property is
present, that will reset the PHY chip before it gets to get_phy_device().
Even with this chip, only about 10% of them fail to report their ID without
reset (in our systems and startup configuration).

>
> To solve the chicken/egg, you need to put a compatible in the PHY node
> listing the ID of the PHY. That will cause the correct PHY driver to
> load, and probe. The probe can then reset it.

Yes, that approach does work — and I've tested it successfully with the
other two patches I sent. However, when a PHY ID is specified in the DT, it
is taken directly, not read from the hardware. This may lead to issues with
revision-specific logic, since the revision bits won’t necessarily match
the actual chip. For LAN8710A this isn’t currently a problem, but it may be
for other PHYs.

>
> We have to be careful about changing the reset behaviour, it is likely
> to break PHYs which currently work, but stop working when they get an
> unexpected reset.

I agree that changing the reset logic must be done with caution.

But these lines of code are actually the first, when the kernel tries to 
access the PHY. Is it not a good practice, to do that from a known,
reproducible state? That is what the reset is for. Without the reset, the
PHY may be in whatever state it was left previously. That could lead to 
hard to reproduce effects, like failing after restart, failing because
there was a change in the bootloader, etc.

A few lines later fwnode_mdiobus_phy_device_register() is called
anyway, that will also reset the PHY in the same conditions (DT based
system, and either `reset-gpios` or `reset-ctrl` is defined).

My intent was to ensure that if the system asks for a reset, it actually
happens before the PHY is accessed, rather than mid-way through
registration.

Changing phy_device_remove() may be more concerning. I can create a patch,
that leaves that intact, and only changes reset behaviour during reset, but
that will be a bit longer.

>
>     Andrew
>
> ---
> pw-bot: cr
>

Csaba