Message-ID: <20190613144143.lx54zqh5qg47cead@shell.armlinux.org.uk>
Date:   Thu, 13 Jun 2019 15:41:43 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
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.

> > 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Powered by blists - more mailing lists