lkml.org 
[lkml]   [2023]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] net: mdio: force deassert MDIO reset signal
    Date
    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
    \
     
     \ /
      Last update: 2023-03-26 23:42    [W:3.486 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site