Messages in this thread | | | Date | Tue, 23 Aug 2022 16:14:16 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH v4.4] net: phy: Add driver for Motorcomm yt8521 gigabit ethernet phy |
| |
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.
Andrew
| |