Re: [PATCH] net: mdio: force deassert MDIO reset signal - Lars-Peter Clausen

From: Lars-Peter Clausen <lars@metafoo.de>
To: Andrew Lunn <andrew@lunn.ch>
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: Sun, 15 Jan 2023 18:21:40 -0800	[thread overview]
Message-ID: <cc338014-8a2b-87e9-7684-20b57aae4ac3@metafoo.de> (raw)
In-Reply-To: <Y8SSb+tJsfJ3/DvH@lunn.ch>

On 1/15/23 15:55, Andrew Lunn wrote:
>> 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;
>
I think part of the problem is that for C45 there are a few other fields 
that get populated by the ID detection, such as devices_in_package and 
mmds_present. Is this something we can do after running the PHY drivers 
probe function? Or is it too late at that point?