Messages in this thread | | | From | Pierluigi Passaro <> | Subject | Re: [PATCH] net: mdio: force deassert MDIO reset signal | Date | Mon, 16 Jan 2023 09:51:20 +0000 |
| |
On Mon, Jan 16, 2023 at 3:41 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > 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? > Unfortunately the above doesn't change the condition: this problem is not C45 specific. The call fwnode_get_phy_id just parses the device tree and always passes. This is a sample device tree https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/arch/arm64/boot/dts/freescale/imx8qm-var-spear.dtsi#L168-L219
| |