Date:   Thu, 13 Jun 2019 14:32:06 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
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


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

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

--
Ioana

> >
> > --
> > Ioana
> >
> >
> > >
> > > >
> > > > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 5d0af041b8f9..dd1feb7b5472 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -688,6 +688,7 @@ static void phylink_phy_change(struct
> > > > phy_device
> > > *phydev, bool up,
> > > >  		pl->phy_state.pause |= MLO_PAUSE_ASYM;
> > > >  	pl->phy_state.interface = phydev->interface;
> > > >  	pl->phy_state.link = up;
> > > > +	pl->phy_state.an_enabled = phydev->autoneg;
> > > >  	mutex_unlock(&pl->state_mutex);
> > > >
> > > >  	phylink_run_resolve(pl);
> > > > --
> > > > 1.9.1
> > > >
> > > >
> >
> >
> 

Powered by blists - more mailing lists