| Message ID | 20200903114259.14013-2-dmurphy@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | DP83869 Feature additions | expand |
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 <dmurphy@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 > + ADVERTISED_Pause | ADVERTISED_Asym_Pause | \ > + ADVERTISED_100baseT_Full) You say 100Base-FX, yet you advertise 100Base-T? > /* This is the same bit mask as the BMCR so re-use the BMCR default */ > #define DP83869_FX_CTRL_DEFAULT MII_DP83869_BMCR_DEFAULT > > @@ -300,6 +305,7 @@ static int dp83869_configure_mode(struct phy_device *phydev, > { > int phy_ctrl_val; > int ret; > + int bmcr; Please keep reverse xmas tree ordering. > if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET || > dp83869->mode > DP83869_SGMII_COPPER_ETHERNET) > @@ -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? > break; > default: > return -EINVAL;
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 <dmurphy@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
Hello On 9/7/20 9:29 AM, Andrew Lunn wrote: > 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 <dmurphy@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. I will remove the TP and BNC. > >>> + 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. The note in the ethtool.h says /* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_* * macro for bits > 31. The only way to use indices > 31 is to * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API. */ Which was added by Heiner I guess I would prefer to add this in a separate patchset once I figure out how the ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API works > >>> @@ -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. No its a bit wonky in that respect. Dan
> The note in the ethtool.h says > > /* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit > * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_* > * macro for bits > 31. The only way to use indices > 31 is to > * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API. > */ > > Which was added by Heiner > > I guess I would prefer to add this in a separate patchset once I figure out > how the ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API works The phydev supported value is no longer a u32, it is now a bitmap. So you can do something like linkmode_set_bit(ETHTOOL_LINK_MODE_100BaseFX_Full_BIT, &supported); Andrew
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 | \ + ADVERTISED_Pause | ADVERTISED_Asym_Pause | \ + ADVERTISED_100baseT_Full) + /* This is the same bit mask as the BMCR so re-use the BMCR default */ #define DP83869_FX_CTRL_DEFAULT MII_DP83869_BMCR_DEFAULT @@ -300,6 +305,7 @@ static int dp83869_configure_mode(struct phy_device *phydev, { int phy_ctrl_val; int ret; + int bmcr; if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET || dp83869->mode > DP83869_SGMII_COPPER_ETHERNET) @@ -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); break; default: return -EINVAL;
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 <dmurphy@ti.com> --- drivers/net/phy/dp83869.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)