Re: [PATCH v3] net: phy: add Maxlinear GPY115/21x/24x driver

On 18/6/2021 10:01 pm, Andrew Lunn wrote:

This email was sent from outside of MaxLinear.

quoted

Net-next:

int genphy_loopback(struct phy_device *phydev, bool enable)
{
      if (enable) {
          u16 val, ctl = BMCR_LOOPBACK;
          int ret;

          if (phydev->speed == SPEED_1000)
              ctl |= BMCR_SPEED1000;
          else if (phydev->speed == SPEED_100)
              ctl |= BMCR_SPEED100;

          if (phydev->duplex == DUPLEX_FULL)
              ctl |= BMCR_FULLDPLX;

          phy_modify(phydev, MII_BMCR, ~0, ctl);

          ret = phy_read_poll_timeout(phydev, MII_BMSR, val,
                          val & BMSR_LSTATUS,
                      5000, 500000, true);
          if (ret)
              return ret;
      } else {
          phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);

          phy_config_aneg(phydev);
      }

      return 0;
}

v5.12.11:

int genphy_loopback(struct phy_device *phydev, bool enable)
{
      return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
                enable ? BMCR_LOOPBACK : 0);
}


Not sure whether anyone else reported similar issue.

The commit message says:

     net: phy: genphy_loopback: add link speed configuration

     In case of loopback, in most cases we need to disable autoneg support
     and force some speed configuration. Otherwise, depending on currently
     active auto negotiated link speed, the loopback may or may not work.

quoted

Should I use phy_modify to set the LOOPBACK bit only in my driver
implementation as force speed with loopback enable does not work in our
device?

So you appear to have the exact opposite problem, you need to use
auto-neg, with yourself, in order to have link. So there are two
solutions:

1) As you say, implement it in your driver

2) Add a second generic implementation, which enables autoneg, if it
is not enabled, sets the loopback bit, and waits for the link to come
up.

Does your PHY driver error out when asked to do a forced mode? It
probably should, if your silicon does not support that part of C22.

          Andrew

Thank you for prompt reply.

The forced mode is successful because we support forced mode in C22.

The problem happens in the speed change no matter it's forced or 
auto-neg in our device.

If I keep current status and just set the loopback bit, it works.

This may be specific to our device, if nobody else report issue in the 
genphy_loopback implementation in net-next.

Then I will implement it in my driver.


Thanks & Regards,

Xu Liang