Re: [PATCH] net: mdio: force deassert MDIO reset signal

From: Andrew Lunn <andrew@lunn.ch>
To: Lars-Peter Clausen <lars@metafoo.de>
Cc: Pierluigi Passaro <pierluigi.passaro@gmail.com>,
	hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	eran.m@variscite.com, nate.d@variscite.com,
	francesco.f@variscite.com, pierluigi.p@variscite.com
Subject: Re: [PATCH] net: mdio: force deassert MDIO reset signal
Date: Mon, 16 Jan 2023 00:55:27 +0100	[thread overview]
Message-ID: <Y8SSb+tJsfJ3/DvH@lunn.ch> (raw)
In-Reply-To: <f691f339-9e50-b968-01e1-1821a2f696e7@metafoo.de>

> Specifying the ID as part of the compatible string works for clause 22 PHYs,
> but for clause 45 PHYs it does not work. The code always wants to read the
> ID from the PHY itself. But I do not understand things well enough to tell
> whether that's a fundamental restriction of C45 or just our implementation
> and the implementation can be changed to fix it.
> 
> Do you have some thoughts on this?

Do you have more details about what goes wrong? Which PHY driver is
it? What compatibles do you put into DT for the PHY?

To some extent, the ID is only used to find the driver. A C45 device
has a lot of ID register, and all of them are used by phy_bus_match()
to see if a driver matches. So you need to be careful which ID you
pick, it needs to match the driver.

It is the driver which decides to use C22 or C45 to talk to the PHY.
However, we do have:

static int phy_probe(struct device *dev)
{
...
        else if (phydev->is_c45)
                err = genphy_c45_pma_read_abilities(phydev);
        else
                err = genphy_read_abilities(phydev);

so it could be a C45 PHY probed using an ID does not have
phydev->is_c45 set, and so it looks in the wrong place for the
capabilities. Make sure you also have the compatible
"ethernet-phy-ieee802.3-c45" which i think should cause is_c45 to be
set.

There is no fundamental restriction that i know of here, it probably
just needs somebody to debug it and find where it goes wrong.

Ah!

int fwnode_mdiobus_register_phy(struct mii_bus *bus,
                                struct fwnode_handle *child, u32 addr)
{
...
        rc = fwnode_property_match_string(child, "compatible",
                                          "ethernet-phy-ieee802.3-c45");
        if (rc >= 0)
                is_c45 = true;

        if (is_c45 || fwnode_get_phy_id(child, &phy_id))
                phy = get_phy_device(bus, addr, is_c45);
        else
                phy = phy_device_create(bus, addr, phy_id, 0, NULL);


So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
true. The if (is_c45 || is then true, so it does not need to call
fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
asks the PHY.

Try this, totally untested:

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index b782c35c4ac1..13be23f8ac97 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
        if (rc >= 0)
                is_c45 = true;
 
-       if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+       if (fwnode_get_phy_id (child, &phy_id))
                phy = get_phy_device(bus, addr, is_c45);
        else
-               phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+               phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
        if (IS_ERR(phy)) {
                rc = PTR_ERR(phy);
                goto clean_mii_ts;


     Andrew
