On Sat, Sep 05, 2020 at 11:17:55AM -0700, Jakub Kicinski wrote: > On Thu, 3 Sep 2020 06:42:57 -0500 Dan Murphy wrote: > > Add the ability to advertise the Fiber connection if the strap or the > > op-mode is configured for 100Base-FX. > > > > Auto negotiation is not supported on this PHY when in fiber mode. > > > > Signed-off-by: Dan Murphy <dmur...@ti.com> > > Some comments, I'm not very phy-knowledgeable so bear with me > (hopefully PHY maintainers can correct me, too). > > > diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c > > index 58103152c601..48a68474f89c 100644 > > --- a/drivers/net/phy/dp83869.c > > +++ b/drivers/net/phy/dp83869.c > > @@ -52,6 +52,11 @@ > > BMCR_FULLDPLX | \ > > BMCR_SPEED1000) > > > > +#define MII_DP83869_FIBER_ADVERTISE (ADVERTISED_TP | ADVERTISED_MII | \ > > + ADVERTISED_FIBRE | ADVERTISED_BNC | \ > > I'm not actually sure myself what the semantics of port type advertise > bits are, but if this is fiber why advertise TP and do you really have > BNC connectors? :S
Hi Jakub Normally, we start with a base of ETHTOOL_LINK_MODE_TP_BIT, ETHTOOL_LINK_MODE_MII_BIT and then use genphy_read_abilities() to read the standard registers in the PHY to determine what the PHY supports. The PHY driver has the ability of provide its own function to get the supported features, which is happening here. As far as i remember, there is no standard way to indicate a PHY is doing Fibre, not copper. I agree that TP and BMC make no sense here, since my understanding is that the device only supports Fibre when strapped for Fibre. It cannot swap to TP, and it has been at least 20 years since i last had a BNC cable in my hands. In this context, i've no idea what MII means. > > + ADVERTISED_Pause | > > ADVERTISED_Asym_Pause | \ > > + ADVERTISED_100baseT_Full) > > You say 100Base-FX, yet you advertise 100Base-T? 100Base-FX does not actually exist in ADVERTISED_X form. I guess this is historical. It was not widely supported, the broadcom PHYs appear to support it, but not much else. We were also running out of bits to represent these ADVERTISED_X values. Now that we have changed to linux bitmaps and have unlimited number of bits, it makes sense to add it. > > @@ -383,7 +389,37 @@ static int dp83869_configure_mode(struct phy_device > > *phydev, > > > > break; > > case DP83869_RGMII_1000_BASE: > > + break; > > case DP83869_RGMII_100_BASE: > > + /* Only allow advertising what this PHY supports */ > > + linkmode_and(phydev->advertising, phydev->advertising, > > + phydev->supported); > > + > > + linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, > > + phydev->supported); > > + linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, > > + phydev->advertising); > > + > > + /* Auto neg is not supported in fiber mode */ > > + bmcr = phy_read(phydev, MII_BMCR); > > + if (bmcr < 0) > > + return bmcr; > > + > > + phydev->autoneg = AUTONEG_DISABLE; > > + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, > > + phydev->supported); > > + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, > > + phydev->advertising); > > + > > + if (bmcr & BMCR_ANENABLE) { > > + ret = phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0); > > + if (ret < 0) > > + return ret; > > + } > > + > > + phy_modify_changed(phydev, MII_ADVERTISE, > > + MII_DP83869_FIBER_ADVERTISE, > > + MII_DP83869_FIBER_ADVERTISE); > > This only accesses standard registers, should it perhaps be a helper in > the kernel's phy code? I suspect the PHY is not following the standard when strapped to fibre. Andrew