> Subject: Re: [PATCH] net: phylink: set the autoneg state in > phylink_phy_change > > On Thu, Jun 13, 2019 at 02:32:06PM +0000, Ioana Ciornei wrote: > > > > > Subject: Re: [PATCH] net: phylink: set the autoneg state in > > > phylink_phy_change > > > > > > On Thu, Jun 13, 2019 at 08:55:16AM +0000, Ioana Ciornei wrote: > > > > > Subject: Re: [PATCH] net: phylink: set the autoneg state in > > > > > phylink_phy_change > > > > > > > > > > On Thu, Jun 13, 2019 at 09:37:51AM +0300, Ioana Ciornei wrote: > > > > > > The phy_state field of phylink should carry only valid > > > > > > information especially when this can be passed to the .mac_config > callback. > > > > > > Update the an_enabled field with the autoneg state in the > > > > > > phylink_phy_change function. > > > > > > > > > > an_enabled is meaningless to mac_config for PHY mode. Why do > > > > > you think this is necessary? > > > > > > > > Well, it's not necessarily used in PHY mode but, from my opinion, > > > > it should > > > be set to the correct value nonetheless. > > > > > > > > Just to give you more context, I am working on adding phylink > > > > support on > > > NXP's DPAA2 platforms where any interaction between the PHY > > > management layer and the Ethernet devices is made through a firmware. > > > > When the .mac_config callback is invoked, the driver communicates > > > > the > > > new configuration to the firmware so that the corresponding > > > net_device can see the correct info. > > > > In this case, the an_enabled field is not used for other purpose > > > > than to > > > inform the net_device of the current configuration and nothing more. > > > > > > The fields that are applicable depend on the negotiation mode: > > > > > > - Non-inband (PHY or FIXED): set the speed, duplex and pause h/w > > > parameters as per the state's speed, duplex and pause settings. > > > Every other state setting should be ignored; they are not defined > > > for this mode of operation. > > > > > > - Inband SGMII: set for inband SGMII reporting of speed and duplex > > > h/w parameters. Set pause mode h/w parameters as per the state's > > > pause settings. Every other state setting should be ignored; they > > > are not defined for this mode of operation. > > > > > > - Inband 802.3z: set for 1G or 2.5G depending on the PHY interface mode. > > > If an_enabled is true, allow inband 802.3z to set the duplex h/w > > > parameter. If an_enabled and the MLO_PAUSE_AN bit of the pause > > > setting are true, allow 802.3z to set the pause h/w parameter. > > > Advertise capabilities depending on the 'advertising' setting. > > > > > > There's only one case where an_enabled is used, which is 802.3z > > > negotiation, because the MAC side is responsible for negotiating the > > > link mode. In all other cases, the MAC is not responsible for any > autonegotiation. > > > > It's clear for me that an_enabled is of use for the MAC only when clause 37 > auto-negotiation is used. > > > > However, the DPAA2 software architecture abstracts the MAC and the > network interface into 2 separate entities that are managed by two different > drivers. > > These drivers communicate only through a firmware. > > > > This means that any ethtool issued on a DPAA2 network interface will go > directly to the firmware for the latest link information. > > So you won't be calling phylink_ethtool_ksettings_get(), which means you > won't be returning correct information anyway. > > > When the MAC driver is not capable to inform the firmware of the proper > link configuration (eg whether the autoneg is on or not), the ethtool output > will not be the correct one. > > You don't get to know the list of supported link modes, so I don't see how > the ethtool information can be correct. > > I'd like to see the patches _before_ I consider accepting your proposed > phylink change.
Sure. I'll send an RFC as soon as possible. > > > > It is important to stick to the above, which will ensure correct > > > functioning of your driver - going off and doing your own thing > > > (such as reading from other > > > fields) is not guaranteed to give good results. > > > > You're right, but unfortunately I am not dealing with a straight-forward > architecture. > > At this point, I think you need to explain why you want to use phylink, as you > seem to be saying that your driver is unable to conform to what phylink > expects due to firmware. That's going to be in the cover letter. -- Ioana
