Re: [PATCH v4.4] net: phy: Add driver for Motorcomm yt8521 gigabit ethernet phy

[Jakub Kicinski]

On Tue, 23 Aug 2022 16:14:16 +0200 Andrew Lunn wrote:
> On Mon, Aug 22, 2022 at 08:21:47PM -0700, Jakub Kicinski wrote:
> > On Wed, 17 Aug 2022 19:25:54 +0800 Frank wrote:  
> > > +static int yt8521_fiber_config_aneg(struct phy_device *phydev)
> > > +{
> > > +	int err, changed;
> > > +	u16 adv;
> > > +
> > > +	if (phydev->autoneg != AUTONEG_ENABLE)
> > > +		return yt8521_fiber_setup_forced(phydev);
> > > +
> > > +	err =  ytphy_modify_ext_with_lock(phydev, YTPHY_MISC_CONFIG_REG,
> > > +					  YTPHY_MCR_FIBER_SPEED_MASK,
> > > +					  YTPHY_MCR_FIBER_1000BX);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	/* enable Fiber auto sensing */
> > > +	err =  ytphy_modify_ext_with_lock(phydev, YT8521_LINK_TIMER_CFG2_REG,
> > > +					  0, YT8521_LTCR_EN_AUTOSEN);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	/* Setup fiber advertisement */
> > > +	adv = ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
> > > +	      ADVERTISE_1000XPSE_ASYM;  
> > 
> > Is it okay to ignore phydev->advertising and always set the same mask?  
> 
> The user could of changed the pause settings, which are going to be
> ignored here. Also, you should not assume the MAC can actually do
> asymmetric pause, not all can. phydev->advertising will be set to only
> include what the MAC can actually do.
> 
> The whole concept of having two line sides connected to one MAC and
> seeing which gets link first is unsupported in Linux. In theory, you
> want to be able to configure each line side differently. Maybe you
> want autoneg on copper, but fixed on fibre, asymmetric pause with
> fibre, but symmetric pause on copper, etc. Since there is only one
> instance of phydev here, you don't have anywhere to store two sets of
> configuration, nor any sort of kAPI to deal with two phydev structures
> etc. So the user experience is not so great.
> 
> With the Marvell Switches which also have this capability, i actually
> ignore it, use the phy-mode it decide which should be used, either
> copper or fibre, and leave the other powered off so it can never get
> link. There is at least one Marvell PHY which does however support
> first up wins, so this behaviour is not new. I just don't recommend
> it.
> 
> And it gets even more interesting when the SFP is actually copper. But
> since the integration with phylink is missing in this driver, that is
> not supported here.

Interesting. Just to confirm - regardless of the two-sided design..
-edness.. IIUC my question has merit and we need v5?