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

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:

quoted

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 <redacted>
Cc: Csókás Bence <redacted>

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