Re: [PATCH net-next v3 1/3] net: dp83869: Add ability to advertise Fiber connection

[Andrew Lunn <andrew@lunn.ch>]

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