Messages in this thread | | | Date | Tue, 23 Aug 2022 11:37:12 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH v4.4] net: phy: Add driver for Motorcomm yt8521 gigabit ethernet phy |
| |
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?
| |