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

-----Original Message-----
From: Ismail, Mohammad Athari <redacted>
Sent: Tuesday, June 22, 2021 12:15 PM
To: Ismail, Mohammad Athari <redacted>
Subject:

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

Hi Andrew,

We also observe same issue on Marvell88E1510 PHY (C22 supported PHY) as well. It works with v5.12.11's genphy_loopback() but not net-next's.

Regards,
Athari

On Tue, Jun 22, 2021 at 04:21:47AM +0000, Ismail, Mohammad Athari wrote:

quoted

-----Original Message-----
From: Ismail, Mohammad Athari <redacted>
Sent: Tuesday, June 22, 2021 12:15 PM
To: Ismail, Mohammad Athari <redacted>
Subject:

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;
}

Hi Andrew,

We also observe same issue on Marvell88E1510 PHY (C22 supported PHY)
as well. It works with v5.12.11's genphy_loopback() but not
net-next's.

Ah, yes. The Marvell probably needs a software reset after the write
to the MII_BMSR register. But just setting the loopback bit also
probably needed a software reset as well, so i suspect it was broken
before this change.

Oleksij, rather that writing registers directly, we probably need to
use the phylib API calls to configure the PHY. That will handle
oddities like the Marvell needing a reset, or PHYs with other speeds
etc.

	Andrew