diff mbox series

[net-next,5/7] net: dsa: felix: delete .phylink_mac_an_restart code

Message ID 20200625152331.3784018-6-olteanv@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series PHYLINK integration improvements for Felix DSA driver | expand

Commit Message

Vladimir Oltean June 25, 2020, 3:23 p.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.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".

So, in PHYLINK's interpretation of "AN restart", it doesn't make sense
to do anything for SGMII and USXGMII. In fact, PHYLINK won't even call
us for any other SerDes protocol than 1000Base-X and 2500Base-X. But we
are not supporting those. So just remove this code.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c         | 10 -------
 drivers/net/dsa/ocelot/felix.h         |  1 -
 drivers/net/dsa/ocelot/felix_vsc9959.c | 37 --------------------------
 3 files changed, 48 deletions(-)

Comments

Russell King (Oracle) June 25, 2020, 4:53 p.m. UTC | #1 
On Thu, Jun 25, 2020 at 06:23:29PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.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.
Vladimir Oltean June 26, 2020, 8:53 a.m. UTC | #2 
Hi Russell,

On Thu, 25 Jun 2020 at 19:53, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jun 25, 2020 at 06:23:29PM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.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
Russell King (Oracle) June 26, 2020, 11:08 a.m. UTC | #3 
On Fri, Jun 26, 2020 at 11:53:24AM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Thu, 25 Jun 2020 at 19:53, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Jun 25, 2020 at 06:23:29PM +0300, Vladimir Oltean wrote:
> > > From: Vladimir Oltean <vladimir.oltean@nxp.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:

This is going over the top - most of this content should have been in a
discussion on the topic several months ago when this was first raised.
I accept that the SGMII link can be renegotiated from the MAC end, but
I still assert that it is pointless.

I've tried it here on the LX2160 with a Copper SFP plugged in that has a
Marvell 88E1111 PHY on-board, which I can monitor both the media and
SGMII side at the 88E1111 PHY.

Sure enough, if I set bit 9 in the LX2160 PCS, and monitor the "fiber"
page in the 88E1111, it reports that the link did drop.  However, that's
as far as the "renegotiation" goes - the link on the media side (as I've
said multiple times) does not renegotiate.

People expect the media side to renegotiate when they issue
"ethtool -r $IFACE" and this is what the phylink_mac_an_restart() method
is there for.  It caters for the case where is no other PHY as there is
with a copper link, the media terminating PHY is the MAC PCS PHY, as it
is for 1000BASE-X, and therefore the only place that media negotiation
can be restarted is the MAC PCS PHY.

Also as I've said multiple times, if you trigger a renegotiation at the
PHY end, then you end up with the media side renegotiating, and a fresh
exchange (in fact, two exchanges) of link information over the SGMII
link.  This is what the user expects.

If we don't have access to the PHY, then restarting the SGMII link
config exchange doesn't provide any benefit - the inaccessible PHY
_still_ doesn't renegotiate on the media side just because the SGMII
side wanted to re-exchange the config information.

Everything I've covered above with respect to the usefulness of
restarting the SGMII exchange are points that I've previously brought
up.

So, what practical use does triggering a fresh exchange of the SGMII
link configuration from the MAC side have?  The only use I can see is
if the SGMII MAC PCS is unreliable due to some hardware issue, doesn't
receive the link information correctly from the PHY, and needs manual
intervention from the MAC PCS side to "pursuade" it to get the correct
link information.  At that point we're into a severly unreliable
implementation that would likely be unsuitable for production systems.

I have no problem allowing other interface types to pass through _if_ it
can be shown that there is a proper practical use and benefit for doing
so, rather than just "the hardware lets us do this".

If we start allowing it for SGMII, we would be triggering a restart of
that configuration passing from both ends of the SGMII at a very similar
time - we would request the PHY to renegotiate, which triggers a fresh
exchange on the SGMII side when the media link fails, and we will also
be hitting the MAC PCS with its own "AN restart".  That doesn't sound
sane.  Depending on the timing, that could mean we end up with the MAC
PCS reporting _three_ different results: one from the local AN restart,
one from the link dropping, and one from the subsequent link
re-establishment.

So, I ask again, what practical use and benefit does restarting the
configuration exchange on a SGMII or USXGMII link have?  Give me a real
life use case where there's a problem with a link that this can solve.
Vladimir Oltean June 26, 2020, 11:19 a.m. UTC | #4 
On Fri, 26 Jun 2020 at 14:08, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>

[snip]

>
> So, I ask again, what practical use and benefit does restarting the
> configuration exchange on a SGMII or USXGMII link have?  Give me a real
> life use case where there's a problem with a link that this can solve.
>

You are pushing the discussion in an area that to me is pretty
insignificant, and where I did _not_ want to go. I said:

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

I was going to remove this code in the first place, it's just that you
didn't like the justification in the initial commit message. Fine. So
I asked you if this new commit message is OK. You said:

> This is going over the top

So let's cut this short: we agree about everything now, hardware
behavior and software behavior. Could you edit my commit message in a
way that you agree with, and paste it here so that I could include it
in v2?

Thanks,
-Vladimir
Russell King (Oracle) June 26, 2020, 11:32 a.m. UTC | #5 
On Fri, Jun 26, 2020 at 02:19:42PM +0300, Vladimir Oltean wrote:
> On Fri, 26 Jun 2020 at 14:08, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> 
> [snip]
> 
> >
> > So, I ask again, what practical use and benefit does restarting the
> > configuration exchange on a SGMII or USXGMII link have?  Give me a real
> > life use case where there's a problem with a link that this can solve.
> >
> 
> You are pushing the discussion in an area that to me is pretty
> insignificant, and where I did _not_ want to go. I said:
> 
> > 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.
> 
> I was going to remove this code in the first place, it's just that you
> didn't like the justification in the initial commit message. Fine. So
> I asked you if this new commit message is OK. You said:
> 
> > This is going over the top
> 
> So let's cut this short: we agree about everything now, hardware
> behavior and software behavior. Could you edit my commit message in a
> way that you agree with, and paste it here so that I could include it
> in v2?

No.  Too busy.
diff mbox series

Patch

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index da337c63e7ca..4ec05090121c 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -296,15 +296,6 @@  static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
 						  state->speed);
 }
 
-static void felix_phylink_mac_an_restart(struct dsa_switch *ds, int port)
-{
-	struct ocelot *ocelot = ds->priv;
-	struct felix *felix = ocelot_to_felix(ocelot);
-
-	if (felix->info->pcs_an_restart)
-		felix->info->pcs_an_restart(ocelot, port);
-}
-
 static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
 					unsigned int link_an_mode,
 					phy_interface_t interface)
@@ -812,7 +803,6 @@  static const struct dsa_switch_ops felix_switch_ops = {
 	.phylink_validate	= felix_phylink_validate,
 	.phylink_mac_link_state	= felix_phylink_mac_pcs_get_state,
 	.phylink_mac_config	= felix_phylink_mac_config,
-	.phylink_mac_an_restart	= felix_phylink_mac_an_restart,
 	.phylink_mac_link_down	= felix_phylink_mac_link_down,
 	.phylink_mac_link_up	= felix_phylink_mac_link_up,
 	.port_enable		= felix_port_enable,
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index a891736ca006..4a4cebcf04a7 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -31,7 +31,6 @@  struct felix_info {
 	void	(*pcs_init)(struct ocelot *ocelot, int port,
 			    unsigned int link_an_mode,
 			    const struct phylink_link_state *state);
-	void	(*pcs_an_restart)(struct ocelot *ocelot, int port);
 	void	(*pcs_link_state)(struct ocelot *ocelot, int port,
 				  struct phylink_link_state *state);
 	int	(*prevalidate_phy_mode)(struct ocelot *ocelot, int port,
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index c1220b488f9c..7d2673dab7d3 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -728,42 +728,6 @@  static int vsc9959_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-static void vsc9959_pcs_an_restart_sgmii(struct phy_device *pcs)
-{
-	phy_set_bits(pcs, MII_BMCR, BMCR_ANRESTART);
-}
-
-static void vsc9959_pcs_an_restart_usxgmii(struct phy_device *pcs)
-{
-	phy_write_mmd(pcs, MDIO_MMD_VEND2, MII_BMCR,
-		      USXGMII_BMCR_RESET |
-		      USXGMII_BMCR_AN_EN |
-		      USXGMII_BMCR_RST_AN);
-}
-
-static void vsc9959_pcs_an_restart(struct ocelot *ocelot, int port)
-{
-	struct felix *felix = ocelot_to_felix(ocelot);
-	struct phy_device *pcs = felix->pcs[port];
-
-	if (!pcs)
-		return;
-
-	switch (pcs->interface) {
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-		vsc9959_pcs_an_restart_sgmii(pcs);
-		break;
-	case PHY_INTERFACE_MODE_USXGMII:
-		vsc9959_pcs_an_restart_usxgmii(pcs);
-		break;
-	default:
-		dev_err(ocelot->dev, "Invalid PCS interface type %s\n",
-			phy_modes(pcs->interface));
-		break;
-	}
-}
-
 /* We enable SGMII AN only when the PHY has managed = "in-band-status" in the
  * device tree. If we are in MLO_AN_PHY mode, we program directly state->speed
  * into the PCS, which is retrieved out-of-band over MDIO. This also has the
@@ -1412,7 +1376,6 @@  struct felix_info felix_info_vsc9959 = {
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9959_mdio_bus_free,
 	.pcs_init		= vsc9959_pcs_init,
-	.pcs_an_restart		= vsc9959_pcs_an_restart,
 	.pcs_link_state		= vsc9959_pcs_link_state,
 	.prevalidate_phy_mode	= vsc9959_prevalidate_phy_mode,
 	.port_setup_tc          = vsc9959_port_setup_tc,