| Message ID | 20250312203010.47429-4-gerhard@engleder-embedded.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support loopback mode speed selection | expand |
Hello Gerhard, On Wed, 12 Mar 2025 21:30:08 +0100 Gerhard Engleder <gerhard@engleder-embedded.com> wrote: > The KSZ9031 PHYs requires full duplex for loopback mode. Add PHY > specific set_loopback() to ensure this. > > Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> > --- > drivers/net/phy/micrel.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 289e1d56aa65..24882d30f685 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1032,6 +1032,29 @@ static int ksz9021_config_init(struct phy_device *phydev) > #define MII_KSZ9031RN_EDPD 0x23 > #define MII_KSZ9031RN_EDPD_ENABLE BIT(0) > > +static int ksz9031_set_loopback(struct phy_device *phydev, bool enable, > + int speed) > +{ > + u16 ctl = BMCR_LOOPBACK; > + int val; > + > + if (!enable) > + return genphy_loopback(phydev, enable, 0); > + > + if (speed == SPEED_10 || speed == SPEED_100 || speed == SPEED_1000) > + phydev->speed = speed; > + else if (speed) > + return -EINVAL; > + phydev->duplex = DUPLEX_FULL; > + > + ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); > + > + phy_write(phydev, MII_BMCR, ctl); > + > + return phy_read_poll_timeout(phydev, MII_BMSR, val, val & BMSR_LSTATUS, > + 5000, 500000, true); Maybe I don't fully get it, but it looks to me that you poll, waiting for the link to become up. As you are in local loopback mode, how does that work ? Do you need connection to an active LP for loopback to work, or does the BMSR_LSTATUS bit behave differently under local loopback ? Maxime
Am 13.03.2025 09:57, schrieb Maxime Chevallier: > Hello Gerhard, > > On Wed, 12 Mar 2025 21:30:08 +0100 > Gerhard Engleder <gerhard@engleder-embedded.com> wrote: > >> The KSZ9031 PHYs requires full duplex for loopback mode. Add PHY >> specific set_loopback() to ensure this. >> >> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> >> --- >> drivers/net/phy/micrel.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c >> index 289e1d56aa65..24882d30f685 100644 >> --- a/drivers/net/phy/micrel.c >> +++ b/drivers/net/phy/micrel.c >> @@ -1032,6 +1032,29 @@ static int ksz9021_config_init(struct >> phy_device *phydev) >> #define MII_KSZ9031RN_EDPD 0x23 >> #define MII_KSZ9031RN_EDPD_ENABLE BIT(0) >> >> +static int ksz9031_set_loopback(struct phy_device *phydev, bool >> enable, >> + int speed) >> +{ >> + u16 ctl = BMCR_LOOPBACK; >> + int val; >> + >> + if (!enable) >> + return genphy_loopback(phydev, enable, 0); >> + >> + if (speed == SPEED_10 || speed == SPEED_100 || speed == SPEED_1000) >> + phydev->speed = speed; >> + else if (speed) >> + return -EINVAL; >> + phydev->duplex = DUPLEX_FULL; >> + >> + ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); >> + >> + phy_write(phydev, MII_BMCR, ctl); >> + >> + return phy_read_poll_timeout(phydev, MII_BMSR, val, val & >> BMSR_LSTATUS, >> + 5000, 500000, true); > > Maybe I don't fully get it, but it looks to me that you poll, waiting > for the link to become up. As you are in local loopback mode, how does > that work ? Do you need connection to an active LP for loopback to > work, or does the BMSR_LSTATUS bit behave differently under local > loopback ? Yes I'm polling for link to come up. This is identical to genphy_loopback(). There is no need for a link partner to get a link up in loopback mode. BMSR_LSTATUS reflects the loopback as link in this case. This series allows to configure a loopback with defined speed without any link partner. Currently for PHY loopback a link partner is needed in some cases (starting with 1 Gbps loopback). Gerhard
Hi, On Thu, 13 Mar 2025 10:35:47 +0100 Gerhard Engleder <gerhard@engleder-embedded.com> wrote: > Am 13.03.2025 09:57, schrieb Maxime Chevallier: > > Hello Gerhard, > > > > On Wed, 12 Mar 2025 21:30:08 +0100 > > Gerhard Engleder <gerhard@engleder-embedded.com> wrote: > > > >> The KSZ9031 PHYs requires full duplex for loopback mode. Add PHY > >> specific set_loopback() to ensure this. > >> > >> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> > >> --- > >> drivers/net/phy/micrel.c | 24 ++++++++++++++++++++++++ > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > >> index 289e1d56aa65..24882d30f685 100644 > >> --- a/drivers/net/phy/micrel.c > >> +++ b/drivers/net/phy/micrel.c > >> @@ -1032,6 +1032,29 @@ static int ksz9021_config_init(struct > >> phy_device *phydev) > >> #define MII_KSZ9031RN_EDPD 0x23 > >> #define MII_KSZ9031RN_EDPD_ENABLE BIT(0) > >> > >> +static int ksz9031_set_loopback(struct phy_device *phydev, bool > >> enable, > >> + int speed) > >> +{ > >> + u16 ctl = BMCR_LOOPBACK; > >> + int val; > >> + > >> + if (!enable) > >> + return genphy_loopback(phydev, enable, 0); > >> + > >> + if (speed == SPEED_10 || speed == SPEED_100 || speed == SPEED_1000) > >> + phydev->speed = speed; > >> + else if (speed) > >> + return -EINVAL; > >> + phydev->duplex = DUPLEX_FULL; > >> + > >> + ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); > >> + > >> + phy_write(phydev, MII_BMCR, ctl); > >> + > >> + return phy_read_poll_timeout(phydev, MII_BMSR, val, val & > >> BMSR_LSTATUS, > >> + 5000, 500000, true); > > > > Maybe I don't fully get it, but it looks to me that you poll, waiting > > for the link to become up. As you are in local loopback mode, how does > > that work ? Do you need connection to an active LP for loopback to > > work, or does the BMSR_LSTATUS bit behave differently under local > > loopback ? > > Yes I'm polling for link to come up. This is identical to > genphy_loopback(). > There is no need for a link partner to get a link up in loopback mode. > BMSR_LSTATUS reflects the loopback as link in this case. > > This series allows to configure a loopback with defined speed without > any > link partner. Currently for PHY loopback a link partner is needed in > some > cases (starting with 1 Gbps loopback). Ok thanks a lot for the clarification ! It looks good to me in that case :) Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> Maxime
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 289e1d56aa65..24882d30f685 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1032,6 +1032,29 @@ static int ksz9021_config_init(struct phy_device *phydev) #define MII_KSZ9031RN_EDPD 0x23 #define MII_KSZ9031RN_EDPD_ENABLE BIT(0) +static int ksz9031_set_loopback(struct phy_device *phydev, bool enable, + int speed) +{ + u16 ctl = BMCR_LOOPBACK; + int val; + + if (!enable) + return genphy_loopback(phydev, enable, 0); + + if (speed == SPEED_10 || speed == SPEED_100 || speed == SPEED_1000) + phydev->speed = speed; + else if (speed) + return -EINVAL; + phydev->duplex = DUPLEX_FULL; + + ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); + + phy_write(phydev, MII_BMCR, ctl); + + return phy_read_poll_timeout(phydev, MII_BMSR, val, val & BMSR_LSTATUS, + 5000, 500000, true); +} + static int ksz9031_of_load_skew_values(struct phy_device *phydev, const struct device_node *of_node, u16 reg, size_t field_sz, @@ -5565,6 +5588,7 @@ static struct phy_driver ksphy_driver[] = { .resume = kszphy_resume, .cable_test_start = ksz9x31_cable_test_start, .cable_test_get_status = ksz9x31_cable_test_get_status, + .set_loopback = ksz9031_set_loopback, }, { .phy_id = PHY_ID_LAN8814, .phy_id_mask = MICREL_PHY_ID_MASK,
The KSZ9031 PHYs requires full duplex for loopback mode. Add PHY specific set_loopback() to ensure this. Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> --- drivers/net/phy/micrel.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)