Date:   Fri, 26 Jun 2020 11:53:24 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Ioana Ciornei <ioana.ciornei@....com>
Subject: Re: [PATCH net-next 5/7] net: dsa: felix: delete .phylink_mac_an_restart
 code

Hi Russell,

On Thu, 25 Jun 2020 at 19:53, Russell King - ARM Linux admin
<linux@...linux.org.uk> wrote:
>
> On Thu, Jun 25, 2020 at 06:23:29PM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@....com>
> >
> > In hardware, the AN_RESTART field for these SerDes protocols (SGMII,
> > USXGMII) clears the resolved configuration from the PCS's
> > auto-negotiation state machine.
> >
> > But PHYLINK has a different interpretation of "AN restart". It assumes
> > that this Linux system is capable of re-triggering an auto-negotiation
> > sequence, something which is only possible with 1000Base-X and
> > 2500Base-X, where the auto-negotiation is symmetrical. In SGMII and
> > USXGMII, there's an AN master and an AN slave, and it isn't so much of
> > "auto-negotiation" as it is "PHY passing the resolved link state on to
> > the MAC".
>
> This is not "a different interpretation".
>
> The LX2160A documentation for this PHY says:
>
>   9             Restart Auto Negotiation
>  Restart_Auto_N Self-clearing Read / Write command bit, set to '1' to
>                 restart an auto negotiation sequence. Set to '0'
>                 (Reset value) in normal operation mode. Note: Controls
>                 the Clause 37 1000Base-X Auto-negotiation.
>
> It doesn't say anything about clearing anything for SGMII.
>
> Also, the Cisco SGMII specification does not indicate that it is
> possible to restart the "autonegotiation" - the PHY is the controlling
> end of the SGMII link.  There is no clause in the SGMII specification
> that indicates that changing the MAC's tx_config word to the PHY will
> have any effect on the PHY once the data path has been established.
>
> Finally, when a restart of negotiation is requested, and we have a PHY
> attached in SGMII mode, we will talk to that PHY to cause a restart of
> negotiation on the media side, which will implicitly cause the link
> to drop and re-establish, causing the SGMII side to indicate link down
> and subsequently re-establish according to the media side results.
>
> So, please, lay off your phylink bashing in your commit messages.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Sorry, I was in a bit of a hurry when writing this commit message, so it is a
bit imprecise as you point out. How about:

net: dsa: felix: delete .phylink_mac_an_restart code

The Cisco SGMII and USXGMII standards specify control information exchange to
be "achieved by using the Auto-Negotiation functionality defined in Clause 37
of the IEEE Specification 802.3z".

The differences to clause 37 auto-negotiation are specified by the respective
standards. In the case of SGMII, the differences are spelled out as being:

- A reduction of the link timer value, from 10 ms to 1.6 ms.
- A customization of the tx_config_reg[15:0], mostly to allow propagation of
  speed information.

A similar situation is going on for USXGMII as well: "USXGMII Auto-neg
mechanism is based on Clause 37 (Figure 37-6) plus additional management
control to select USXGMII mode".

The point is, both Cisco standards make explicit reference that they require an
auto-negotiation state machine implemented as per "Figure 37-6-Auto-Negotiation
state diagram" from IEEE 802.3. In the SGMII spec, it is very clearly pointed
out that both the MAC PCS (Figure 3 MAC Functional Block) and the PHY PCS
(Figure 2 PHY Functional Block) contain an auto-negotiation block defined by
"Auto-Negotiation Figure 37-6".

Since both ends of the SGMII/USXGMII link implement the same state machine
(just carry different tx_config_reg payloads, which they convey to their link
partner via /C/ ordered sets), naturally the ability to restart
auto-negotiation is symmetrical. The state machine in IEEE 802.3 Figure 37-6
specifies the signal that triggers an auto-negotiation restart as being
"mr_restart_an=TRUE".

Furthermore, clause "37.2.5.1.9 State diagram variable to management register
mapping", through its "Table 37-8-PCS state diagram variable to management
register mapping", requires a PCS compliant to clause 37 to expose the
mr_restart_an signal to management through MDIO register "0.9 Auto-Negotiation
restart", aka BMCR_ANRESTART in Linux terms.

The Felix PCS for SGMII and USXGMII is compliant to clause 37, so it exposes
BMCR_ANRESTART to the operating system. When this bit is asserted, the
following happens:

1. STATUS[Auto_Negotiation_Complete] goes from 1->0.
2. The PCS starts sending AN sequences instead of packets or IDLEs.
3. The PCS waits to receive AN sequences from PHY and matches them.
4. Once it has received  matching AN sequences and a PHY acknowledge,
   STATUS[Auto_Negotiation_Complete] goes from 0->1.
5. Normal packet transmission restarts.

Otherwise stated, the MAC PCS has the ability to re-trigger a switch of the
lane from data mode into configuration mode, then control information exchange
takes place, then the lane is switched back into data mode. These 5 steps are
collectively described as "restart AN state machine" by the PCS documentation.
This is all as per IEEE 802.3 Clause 37 AN state machine, which SGMII and
USXGMII do not touch at this fundamental level.

Now, it is true that the Cisco SGMII and USXGMII specs mention that the control
information exchange has a unidirectional meaning. That is, the PHY restarts
the clause 37 auto-negotiation upon any change in MDI auto-negotiation
parameters. PHYLINK takes this fact a bit further, and since the fact that the
MAC PCS conveys no new information to the PHY PCS (beyond acknowledging the
received config word), does not permit the MAC PCS to trigger a restart of the
clause 37 auto-negotiation for any other SERDES protocols than 1000Base-X and
2500Base-X. For those, the control information exchange _is_ bidirectional
(local PCS specifies its duplex and flow control abilities). For any other
SERDES protocols, the .phylink_mac_an_restart callback is dead code. This is
probably OK, I can't come up with a situation where it might be useful for the
MAC PCS to clear its cache of link state and ask for a new tx_config_reg. So
remove this code.

Thanks,
-Vladimir

Powered by blists - more mailing lists