[07/14] net: axienet: Fix SGMII support

With SGMII, the MAC and the PHY can negotiate the link speed between
themselves, without the host needing to mediate between them.
Linux recognises this, and will call phylink's mac_config with the speed
member set to SPEED_UNKNOWN (-1).
Currently the axienet driver will bail out and complain about an
unsupported link speed.

Teach axienet's mac_config callback to leave the MAC's speed setting
alone if the requested speed is SPEED_UNKNOWN.

This fixes axienet operation when the hardware is using SGMII.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 21 ++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> With SGMII, the MAC and the PHY can negotiate the link speed between
> themselves, without the host needing to mediate between them.
> Linux recognises this, and will call phylink's mac_config with the speed
> member set to SPEED_UNKNOWN (-1).
> Currently the axienet driver will bail out and complain about an
> unsupported link speed.
> 
> Teach axienet's mac_config callback to leave the MAC's speed setting
> alone if the requested speed is SPEED_UNKNOWN.

Hi Andre

Is there an interrupt when SGMII signals a change in link state? If
so, you should call phylink_mac_change().

    Andrew

On Fri, 10 Jan 2020 15:04:15 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

Hi Andrew,

> On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > With SGMII, the MAC and the PHY can negotiate the link speed between
> > themselves, without the host needing to mediate between them.
> > Linux recognises this, and will call phylink's mac_config with the speed
> > member set to SPEED_UNKNOWN (-1).
> > Currently the axienet driver will bail out and complain about an
> > unsupported link speed.
> > 
> > Teach axienet's mac_config callback to leave the MAC's speed setting
> > alone if the requested speed is SPEED_UNKNOWN.  
> 
> Hi Andre
> 
> Is there an interrupt when SGMII signals a change in link state? If
> so, you should call phylink_mac_change().

Good point. The doc describes a "Auto-Negotiation Complete" interrupt status bit, which signal that " ... auto-negotiation of the SGMII or 1000BASE-X interface has completed."
But I have no clue whether that would trigger on a link status *change*. Is there a way to test this without pulling the cable? My board sits in a data centre, so is not easily accessible to me.

Cheers,
Andre.

> But I have no clue whether that would trigger on a link status
> *change*.

It should do, but without testing...

> Is there a way to test this without pulling the cable? My board sits
> in a data centre, so is not easily accessible to me.

Down and up the other end?

     Andrew

On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> With SGMII, the MAC and the PHY can negotiate the link speed between
> themselves, without the host needing to mediate between them.
> Linux recognises this, and will call phylink's mac_config with the speed
> member set to SPEED_UNKNOWN (-1).

I wonder whether you have read the documentation for the phylink
mac_config() method (if not, please read it, it contains some very
important information about what mac_config() should do.)  When
operating in SGMII in-band mode, state->speed and state->duplex are
not actually valid.

You'll probably want to submit a better patch after reading the
documentation.

Thanks.

On Fri, Jan 10, 2020 at 02:20:38PM +0000, Andre Przywara wrote:
> On Fri, 10 Jan 2020 15:04:15 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> Hi Andrew,
> 
> > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > themselves, without the host needing to mediate between them.
> > > Linux recognises this, and will call phylink's mac_config with the speed
> > > member set to SPEED_UNKNOWN (-1).
> > > Currently the axienet driver will bail out and complain about an
> > > unsupported link speed.
> > > 
> > > Teach axienet's mac_config callback to leave the MAC's speed setting
> > > alone if the requested speed is SPEED_UNKNOWN.  
> > 
> > Hi Andre
> > 
> > Is there an interrupt when SGMII signals a change in link state? If
> > so, you should call phylink_mac_change().
> 
> Good point. The doc describes a "Auto-Negotiation Complete" interrupt
> status bit, which signal that " ... auto-negotiation of the SGMII or
> 1000BASE-X interface has completed."

It depends what they mean by "Auto-negotiation complete" in SGMII.
SGMII can complete the handshake, yet the config_reg word indicate
link down.  If such an update causes an "Auto-negotiation complete"
interrupt, then that's sufficient.

However, looking at axienet_mac_pcs_get_state(), that is just reading
back what the MAC was set to in axienet_mac_config(), which is not
how this is supposed to work.  axienet_mac_pcs_get_state() is
supposed to get the results of the SGMII/1000BASE-X "negotiation".
That also needs to be fixed.

On Fri, Jan 10, 2020 at 03:04:09PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Jan 10, 2020 at 02:20:38PM +0000, Andre Przywara wrote:
> > On Fri, 10 Jan 2020 15:04:15 +0100
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > Hi Andrew,
> > 
> > > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > > themselves, without the host needing to mediate between them.
> > > > Linux recognises this, and will call phylink's mac_config with the speed
> > > > member set to SPEED_UNKNOWN (-1).
> > > > Currently the axienet driver will bail out and complain about an
> > > > unsupported link speed.
> > > > 
> > > > Teach axienet's mac_config callback to leave the MAC's speed setting
> > > > alone if the requested speed is SPEED_UNKNOWN.  
> > > 
> > > Hi Andre
> > > 
> > > Is there an interrupt when SGMII signals a change in link state? If
> > > so, you should call phylink_mac_change().
> > 
> > Good point. The doc describes a "Auto-Negotiation Complete" interrupt
> > status bit, which signal that " ... auto-negotiation of the SGMII or
> > 1000BASE-X interface has completed."
> 
> It depends what they mean by "Auto-negotiation complete" in SGMII.
> SGMII can complete the handshake, yet the config_reg word indicate
> link down.  If such an update causes an "Auto-negotiation complete"
> interrupt, then that's sufficient.
> 
> However, looking at axienet_mac_pcs_get_state(), that is just reading
> back what the MAC was set to in axienet_mac_config(), which is not
> how this is supposed to work.  axienet_mac_pcs_get_state() is
> supposed to get the results of the SGMII/1000BASE-X "negotiation".
> That also needs to be fixed.

I found "pg138-axi-ethernet.pdf" online, which I guess is this IP.
It says for SGMII:

The results of the SGMII auto-negotiation can be read from the SGMII
Management Auto-Negotiation Link Partner Ability Base register
(Table 2-54). The speed of the subsystem should then be set to match.

and similar for 1000BASE-X (referencing the same register.)

However, what they give in table 2-54 is the 1000BASE-X version of
the config_reg word, not the SGMII version (which is different.)

Hmm, I guess there's probably some scope for phylink to start
handling an IEEE 802.3 compliant PCS accessed over MDIO rather
than having each network driver implement this, but for now your
axienet_mac_pcs_get_state() implementation needs to be reading
from the register described in table 2-54 and interpreting the
results according to whether state->interface is 802.3z or not.

Also note, don't set state->interface in axienet_mac_pcs_get_state(),
you will be passed the currently selected interface that was last
configured via axienet_mac_config().

On Fri, Jan 10, 2020 at 03:22:15PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Jan 10, 2020 at 03:04:09PM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Jan 10, 2020 at 02:20:38PM +0000, Andre Przywara wrote:
> > > On Fri, 10 Jan 2020 15:04:15 +0100
> > > Andrew Lunn <andrew@lunn.ch> wrote:
> > > 
> > > Hi Andrew,
> > > 
> > > > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > > > themselves, without the host needing to mediate between them.
> > > > > Linux recognises this, and will call phylink's mac_config with the speed
> > > > > member set to SPEED_UNKNOWN (-1).
> > > > > Currently the axienet driver will bail out and complain about an
> > > > > unsupported link speed.
> > > > > 
> > > > > Teach axienet's mac_config callback to leave the MAC's speed setting
> > > > > alone if the requested speed is SPEED_UNKNOWN.  
> > > > 
> > > > Hi Andre
> > > > 
> > > > Is there an interrupt when SGMII signals a change in link state? If
> > > > so, you should call phylink_mac_change().
> > > 
> > > Good point. The doc describes a "Auto-Negotiation Complete" interrupt
> > > status bit, which signal that " ... auto-negotiation of the SGMII or
> > > 1000BASE-X interface has completed."
> > 
> > It depends what they mean by "Auto-negotiation complete" in SGMII.
> > SGMII can complete the handshake, yet the config_reg word indicate
> > link down.  If such an update causes an "Auto-negotiation complete"
> > interrupt, then that's sufficient.
> > 
> > However, looking at axienet_mac_pcs_get_state(), that is just reading
> > back what the MAC was set to in axienet_mac_config(), which is not
> > how this is supposed to work.  axienet_mac_pcs_get_state() is
> > supposed to get the results of the SGMII/1000BASE-X "negotiation".
> > That also needs to be fixed.
> 
> I found "pg138-axi-ethernet.pdf" online, which I guess is this IP.
> It says for SGMII:
> 
> The results of the SGMII auto-negotiation can be read from the SGMII
> Management Auto-Negotiation Link Partner Ability Base register
> (Table 2-54). The speed of the subsystem should then be set to match.
> 
> and similar for 1000BASE-X (referencing the same register.)
> 
> However, what they give in table 2-54 is the 1000BASE-X version of
> the config_reg word, not the SGMII version (which is different.)
> 
> Hmm, I guess there's probably some scope for phylink to start
> handling an IEEE 802.3 compliant PCS accessed over MDIO rather
> than having each network driver implement this, but for now your
> axienet_mac_pcs_get_state() implementation needs to be reading
> from the register described in table 2-54 and interpreting the
> results according to whether state->interface is 802.3z or not.
> 
> Also note, don't set state->interface in axienet_mac_pcs_get_state(),
> you will be passed the currently selected interface that was last
> configured via axienet_mac_config().

Maybe something like the below will help?

Basically, use phylink_mii_pcs_get_state() instead of
axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
access the internal PHY (as per C_PHYADDR parameter.)

You may have some fuzz (with gnu patch) while trying to apply this,
as you won't have the context for the first and last hunks in this
patch.

This will probably not be the final version of the patch anyway;
there's some possibility to pull some of the functionality out of
phylib into a more general library which would avoid some of the
functional duplication.

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 75a74a16dc3d..44198fdb3c01 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2073,4 +2073,105 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 }
 EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
 
+static void phylink_decode_advertisement(struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
+
+	linkmode_and(u, state->lp_advertising, state->advertising);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
+		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->lp_advertising))
+			state->pause |= MLO_PAUSE_TX;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->advertising))
+			state->pause |= MLO_PAUSE_RX;
+	}
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
+		state->speed = SPEED_2500;
+		state->duplex = DUPLEX_FULL;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
+		state->pause = SPEED_1000;
+		state->duplex = DUPLEX_FULL;
+	} else {
+		state->link = false;
+	}
+}
+
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int addr = config->pcs_mii_addr;
+	int bmsr, lpa;
+
+	bmsr = mdiobus_read(bus, addr, MII_BMSR);
+	lpa = mdiobus_read(bus, addr, MII_LPA);
+	if (bmsr < 0 || lpa < 0) {
+		state->link = false;
+		return;
+	}
+
+	state->link = !!(bmsr & BMSR_LSTATUS);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+					 state->lp_advertising);
+		goto lpa_8023z;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
+					 state->lp_advertising);
+	lpa_8023z:
+		if (lpa & LPA_1000XPAUSE)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_1000XPAUSE_ASYM)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_LPACK)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					 state->lp_advertising);
+		phylink_decode_advertisement(state);
+		break;
+
+	case PHY_INTERFACE_MODE_SGMII:
+		switch (lpa & 0x8c00) {
+		case 0x8000:
+			state->speed = SPEED_10;
+			break;
+		case 0x8400:
+			state->speed = SPEED_100;
+			break;
+		case 0x8800:
+			state->speed = SPEED_1000;
+			break;
+		default:
+			state->link = false;
+			break;
+		}
+		switch (lpa & 0x9000) {
+		case 0x9000:
+			state->duplex = DUPLEX_FULL;
+			break;
+		case 0x8000:
+			state->duplex = DUPLEX_HALF;
+			break;
+		}
+		break;
+
+	default:
+		state->link = false;
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 4ea76e083847..cf0fa39b4b21 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -65,6 +65,9 @@ enum phylink_op_type {
 struct phylink_config {
 	struct device *dev;
 	enum phylink_op_type type;
+
+	struct mii_bus *pcs_mii;
+	int pcs_mii_addr;
 };
 
 /**
@@ -292,4 +295,7 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 						const phy_interface_t *pref,
 						size_t nprefs);
 
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state);
+
 #endif

On Fri, 10 Jan 2020 14:58:49 +0000
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > With SGMII, the MAC and the PHY can negotiate the link speed between
> > themselves, without the host needing to mediate between them.
> > Linux recognises this, and will call phylink's mac_config with the speed
> > member set to SPEED_UNKNOWN (-1).  
> 
> I wonder whether you have read the documentation for the phylink
> mac_config() method (if not, please read it, it contains some very
> important information about what mac_config() should do.)  When
> operating in SGMII in-band mode, state->speed and state->duplex are
> not actually valid.
> 
> You'll probably want to submit a better patch after reading the
> documentation.

Sure, I am admittedly quite clueless about phylink in particular, and found the available information quite daunting.
So I tried my best in looking at what other drivers do. From what I got there is that you speed=-1 should be ignored, but the other fields still handled.
Also I was somewhat puzzled, as I was expecting "mode" being MLO_AN_INBAND. But in fact it's called twice with MLO_AN_PHY, and mac_pcs_get_state() never gets called:

[  166.516583] xilinx_axienet 7fe00000.ethernet eth0: PHY [axienet-7fe00000:01] driver [Generic PHY]
[  166.547309] xilinx_axienet 7fe00000.ethernet eth0: configuring for phy/sgmii link mode
[  166.572343] axienet_mac_config(mode=0, speed=-1, duplex=255, pause=16, link=0, an_en=1)
udhcpc: sending discover
[  168.652152] axienet_mac_config(mode=0, speed=-1, duplex=255, pause=0, link=1, an_en=0)
[  168.683538] xilinx_axienet 7fe00000.ethernet eth0: Link is Up - Unknown/Unknown - flow control off
[  168.712560] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
udhcpc: sending discover
udhcpc: sending select for 10.1.x.y
udhcpc: lease of 10.1.x.y obtained, lease time 691200

I was just wondering if the DT description is giving Linux a wrong impression, but I have phy-mode set to sgmii, also just tried phy-connection-type on top of that. The DT snippet is the same as the example in patch 14. The PHY is a Marvell 88E1111, connected via SGMII.
 
I would be grateful for any advice!

Cheers,
Andre.

On Fri, Jan 10, 2020 at 05:32:49PM +0000, Andre Przywara wrote:
> On Fri, 10 Jan 2020 14:58:49 +0000
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Fri, Jan 10, 2020 at 11:54:08AM +0000, Andre Przywara wrote:
> > > With SGMII, the MAC and the PHY can negotiate the link speed between
> > > themselves, without the host needing to mediate between them.
> > > Linux recognises this, and will call phylink's mac_config with the speed
> > > member set to SPEED_UNKNOWN (-1).  
> > 
> > I wonder whether you have read the documentation for the phylink
> > mac_config() method (if not, please read it, it contains some very
> > important information about what mac_config() should do.)  When
> > operating in SGMII in-band mode, state->speed and state->duplex are
> > not actually valid.
> > 
> > You'll probably want to submit a better patch after reading the
> > documentation.
> 
> Sure, I am admittedly quite clueless about phylink in particular, and found the available information quite daunting.
> So I tried my best in looking at what other drivers do. From what I got there is that you speed=-1 should be ignored, but the other fields still handled.
> Also I was somewhat puzzled, as I was expecting "mode" being MLO_AN_INBAND. But in fact it's called twice with MLO_AN_PHY, and mac_pcs_get_state() never gets called:

Okay.  When phylink is in PHY mode, it operates just the same as the
more conventional phylib setup: phylib reports the negotiation results
to the network driver which sets the MAC up appropriately.

The only difference between the phylib way of doing things and phylink
is that phylink is in the path, so mac_config() gets called to setup
the MAC with the results of the PHY negotiation.  This will be the case
irrespective of which PHY interface mode is being used.

So, in PHY mode, we don't care whether there is in-band signalling or
not - and the reason that's vague is because it _is_ already vague
with existing phylib setups using SGMII.

So, basically, the MLO_AN_PHY mode is the complete equivalent of
phylib without phylink.


MLO_AN_FIXED is just like MLO_AN_PHY, except phylink is operating in
fixed-link mode - similar to the old fixed-link emulated PHY setup that
phylib offered, but without needing a MII bus and squeezing the
information through phylib's interfaces.  From the point of view of a
MAC driver, however, it's just the same as MLO_AN_PHY.


If you configure phylink for inband mode by placing

	managed = "in-band-status";

in DT, then phylink will operate in MLO_AN_INBAND mode.  It will also
operate in that mode if the MAC is connected directly to a SFP cage
and a SFP is inserted that requires inband mode.

Exactly how inband mode operates depends in the nature of the inband
signalling.  There's two different schemes:

SGMII: the PHY communicates the speed and duplex settings to the MAC
PCS through the in-band control word.  Pause mode is not available
via the in-band control word.  SGMII can operate at 10M, 100M or 1G,
half or full duplex.  The PHY may or may not be accessible.

Here, phylink will read the speed and duplex from the MAC PCS rather
than the PHY, and if the PHY is accessible, phylink will merge the
negotiated pause mode information and pass this over to the MAC.

(Note: there are some vendor extensions to pass pause mode through
SGMII as well, but I haven't seen a MAC that supports them yet.)

1000BASE-X (aka 802.3z): the link partner advertises its capabilities
via the in-band control word, which are:
	- full duplex
	- half duplex
	- pause
	- asym pause

and each end of the link has to resolve the capabilities to agree the
operating mode of the link.  As only a single speed is supported in
this mode, there is no need to advertise any speed capabilities (if
the link operates at dis-similar speeds - for example, 2500BASE-X at
one end and 1000BASE-X on the other, there's no way to get the control
word through.)

Here, phylink will only read from the MAC PCS to discover the results
of the negotiation; there will be no call to mac_config().


Phylink currently expects the result of the in-band negotiation at
the MAC PCS to be propagated to the MAC by hardware (as this is what
happens with mvneta and mvpp2, the first two MACs that phylink
supports.)  If there is hardware that requires something else, then
that will need to be revisited, and will result in not only code but
also documentation updates as well.

I hope this helps you to understand phylink.

> Phylink currently expects the result of the in-band negotiation at
> the MAC PCS to be propagated to the MAC by hardware (as this is what
> happens with mvneta and mvpp2, the first two MACs that phylink
> supports.)  If there is hardware that requires something else, then
> that will need to be revisited, and will result in not only code but
> also documentation updates as well.

Hi Russell

This is an issue i'm having at the moment with Marvell switches. They
do not propagate the results to the MAC. So when i get an interrupt
from the SERDES that the link is up, i'm programming the MAC as
needed.

	Andrew

On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:
> Maybe something like the below will help?
> 
> Basically, use phylink_mii_pcs_get_state() instead of
> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
> access the internal PHY (as per C_PHYADDR parameter.)
> 
> You may have some fuzz (with gnu patch) while trying to apply this,
> as you won't have the context for the first and last hunks in this
> patch.
> 
> This will probably not be the final version of the patch anyway;
> there's some possibility to pull some of the functionality out of
> phylib into a more general library which would avoid some of the
> functional duplication.

Hi Andre,

Did you have a chance to see whether this helps?

Russell.

> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 75a74a16dc3d..44198fdb3c01 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2073,4 +2073,105 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>  }
>  EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
>  
> +static void phylink_decode_advertisement(struct phylink_link_state *state)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
> +
> +	linkmode_and(u, state->lp_advertising, state->advertising);
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
> +		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				      state->lp_advertising))
> +			state->pause |= MLO_PAUSE_TX;
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				      state->advertising))
> +			state->pause |= MLO_PAUSE_RX;
> +	}
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
> +		state->speed = SPEED_2500;
> +		state->duplex = DUPLEX_FULL;
> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
> +		state->pause = SPEED_1000;
> +		state->duplex = DUPLEX_FULL;
> +	} else {
> +		state->link = false;
> +	}
> +}
> +
> +void phylink_mii_pcs_get_state(struct phylink_config *config,
> +			       struct phylink_link_state *state)
> +{
> +	struct mii_bus *bus = config->pcs_mii;
> +	int addr = config->pcs_mii_addr;
> +	int bmsr, lpa;
> +
> +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
> +	lpa = mdiobus_read(bus, addr, MII_LPA);
> +	if (bmsr < 0 || lpa < 0) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	state->link = !!(bmsr & BMSR_LSTATUS);
> +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		if (lpa & LPA_1000XFULL)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +					 state->lp_advertising);
> +		goto lpa_8023z;
> +
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		if (lpa & LPA_1000XFULL)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
> +					 state->lp_advertising);
> +	lpa_8023z:
> +		if (lpa & LPA_1000XPAUSE)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +					 state->lp_advertising);
> +		if (lpa & LPA_1000XPAUSE_ASYM)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +					 state->lp_advertising);
> +		if (lpa & LPA_LPACK)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +					 state->lp_advertising);
> +		phylink_decode_advertisement(state);
> +		break;
> +
> +	case PHY_INTERFACE_MODE_SGMII:
> +		switch (lpa & 0x8c00) {
> +		case 0x8000:
> +			state->speed = SPEED_10;
> +			break;
> +		case 0x8400:
> +			state->speed = SPEED_100;
> +			break;
> +		case 0x8800:
> +			state->speed = SPEED_1000;
> +			break;
> +		default:
> +			state->link = false;
> +			break;
> +		}
> +		switch (lpa & 0x9000) {
> +		case 0x9000:
> +			state->duplex = DUPLEX_FULL;
> +			break;
> +		case 0x8000:
> +			state->duplex = DUPLEX_HALF;
> +			break;
> +		}
> +		break;
> +
> +	default:
> +		state->link = false;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
> +
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 4ea76e083847..cf0fa39b4b21 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -65,6 +65,9 @@ enum phylink_op_type {
>  struct phylink_config {
>  	struct device *dev;
>  	enum phylink_op_type type;
> +
> +	struct mii_bus *pcs_mii;
> +	int pcs_mii_addr;
>  };
>  
>  /**
> @@ -292,4 +295,7 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>  						const phy_interface_t *pref,
>  						size_t nprefs);
>  
> +void phylink_mii_pcs_get_state(struct phylink_config *config,
> +			       struct phylink_link_state *state);
> +
>  #endif
> 
> -- 
> 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
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:
> On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:
>> Maybe something like the below will help?
>>
>> Basically, use phylink_mii_pcs_get_state() instead of
>> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
>> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
>> access the internal PHY (as per C_PHYADDR parameter.)
>>
>> You may have some fuzz (with gnu patch) while trying to apply this,
>> as you won't have the context for the first and last hunks in this
>> patch.
>>
>> This will probably not be the final version of the patch anyway;
>> there's some possibility to pull some of the functionality out of
>> phylib into a more general library which would avoid some of the
>> functional duplication.
> 
> Hi Andre,
> 
> Did you have a chance to see whether this helps?

Sorry, I needed some time to wrap my head around your reply first. Am I am still not fully finished with this process ;-)
Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it seems to work, because it actually calls axienet_mac_pcs_get_state() to learn the actual negotiated parameters. Then in turn it calls mac_config with the proper speed instead of -1:
[  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for inband/sgmii link mode
[  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255, pause=16)
...
[  153.818568] axienet_mac_pcs_get_state(config): speed=1000, interface=4, pause=0
[  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1, pause=0)

Without that DT property it never called mac_pcs_get_state(), so never learnt about the actual settings.
But the actual MAC setting was already right (1 GBps, FD). Whether this was by chance (reset value?) or because this was set by the PHY via SGMII, I don't know.
So in my case I think I *need* to have the managed = ... property in my DT.

But I was wondering if we need this patch anyway, regardless of the proper way to check for the connection setting in this case. Because at the moment calling mac_config with speed=-1 will *delete* the current MAC speed setting and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of the well-known values. I am not sure that is desired behaviour, or speed=-1 just means: don't touch the speed setting. After all we call mac_config with speed=-1 first, even when later fixing this up (see above).

Thanks,
Andre.

>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 75a74a16dc3d..44198fdb3c01 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -2073,4 +2073,105 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>>  }
>>  EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
>>  
>> +static void phylink_decode_advertisement(struct phylink_link_state *state)
>> +{
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
>> +
>> +	linkmode_and(u, state->lp_advertising, state->advertising);
>> +
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
>> +		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
>> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
>> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +				      state->lp_advertising))
>> +			state->pause |= MLO_PAUSE_TX;
>> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +				      state->advertising))
>> +			state->pause |= MLO_PAUSE_RX;
>> +	}
>> +
>> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
>> +		state->speed = SPEED_2500;
>> +		state->duplex = DUPLEX_FULL;
>> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
>> +		state->pause = SPEED_1000;
>> +		state->duplex = DUPLEX_FULL;
>> +	} else {
>> +		state->link = false;
>> +	}
>> +}
>> +
>> +void phylink_mii_pcs_get_state(struct phylink_config *config,
>> +			       struct phylink_link_state *state)
>> +{
>> +	struct mii_bus *bus = config->pcs_mii;
>> +	int addr = config->pcs_mii_addr;
>> +	int bmsr, lpa;
>> +
>> +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
>> +	lpa = mdiobus_read(bus, addr, MII_LPA);
>> +	if (bmsr < 0 || lpa < 0) {
>> +		state->link = false;
>> +		return;
>> +	}
>> +
>> +	state->link = !!(bmsr & BMSR_LSTATUS);
>> +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
>> +
>> +	switch (state->interface) {
>> +	case PHY_INTERFACE_MODE_1000BASEX:
>> +		if (lpa & LPA_1000XFULL)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
>> +					 state->lp_advertising);
>> +		goto lpa_8023z;
>> +
>> +	case PHY_INTERFACE_MODE_2500BASEX:
>> +		if (lpa & LPA_1000XFULL)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
>> +					 state->lp_advertising);
>> +	lpa_8023z:
>> +		if (lpa & LPA_1000XPAUSE)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +					 state->lp_advertising);
>> +		if (lpa & LPA_1000XPAUSE_ASYM)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>> +					 state->lp_advertising);
>> +		if (lpa & LPA_LPACK)
>> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>> +					 state->lp_advertising);
>> +		phylink_decode_advertisement(state);
>> +		break;
>> +
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +		switch (lpa & 0x8c00) {
>> +		case 0x8000:
>> +			state->speed = SPEED_10;
>> +			break;
>> +		case 0x8400:
>> +			state->speed = SPEED_100;
>> +			break;
>> +		case 0x8800:
>> +			state->speed = SPEED_1000;
>> +			break;
>> +		default:
>> +			state->link = false;
>> +			break;
>> +		}
>> +		switch (lpa & 0x9000) {
>> +		case 0x9000:
>> +			state->duplex = DUPLEX_FULL;
>> +			break;
>> +		case 0x8000:
>> +			state->duplex = DUPLEX_HALF;
>> +			break;
>> +		}
>> +		break;
>> +
>> +	default:
>> +		state->link = false;
>> +		break;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
>> +
>>  MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
>> index 4ea76e083847..cf0fa39b4b21 100644
>> --- a/include/linux/phylink.h
>> +++ b/include/linux/phylink.h
>> @@ -65,6 +65,9 @@ enum phylink_op_type {
>>  struct phylink_config {
>>  	struct device *dev;
>>  	enum phylink_op_type type;
>> +
>> +	struct mii_bus *pcs_mii;
>> +	int pcs_mii_addr;
>>  };
>>  
>>  /**
>> @@ -292,4 +295,7 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>>  						const phy_interface_t *pref,
>>  						size_t nprefs);
>>  
>> +void phylink_mii_pcs_get_state(struct phylink_config *config,
>> +			       struct phylink_link_state *state);
>> +
>>  #endif
>>
>> -- 
>> 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
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>

On Mon, Jan 20, 2020 at 02:50:28PM +0000, Andre Przywara wrote:
> On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:
> > On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:
> >> Maybe something like the below will help?
> >>
> >> Basically, use phylink_mii_pcs_get_state() instead of
> >> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
> >> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
> >> access the internal PHY (as per C_PHYADDR parameter.)
> >>
> >> You may have some fuzz (with gnu patch) while trying to apply this,
> >> as you won't have the context for the first and last hunks in this
> >> patch.
> >>
> >> This will probably not be the final version of the patch anyway;
> >> there's some possibility to pull some of the functionality out of
> >> phylib into a more general library which would avoid some of the
> >> functional duplication.
> > 
> > Hi Andre,
> > 
> > Did you have a chance to see whether this helps?
> 
> Sorry, I needed some time to wrap my head around your reply first. Am I am still not fully finished with this process ;-)
> Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it seems to work, because it actually calls axienet_mac_pcs_get_state() to learn the actual negotiated parameters. Then in turn it calls mac_config with the proper speed instead of -1:
> [  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for inband/sgmii link mode
> [  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255, pause=16)
> ...
> [  153.818568] axienet_mac_pcs_get_state(config): speed=1000, interface=4, pause=0
> [  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1, pause=0)
> 
> Without that DT property it never called mac_pcs_get_state(), so never learnt about the actual settings.
> But the actual MAC setting was already right (1 GBps, FD). Whether this was by chance (reset value?) or because this was set by the PHY via SGMII, I don't know.
> So in my case I think I *need* to have the managed = ... property in my DT.

I really don't like this guess-work.  The specifications are freely
available out there, so there's really no need for this.

pg051-tri-mode-eth-mac.pdf describes the ethernet controller, and
Table 2-32 therein describes the EMMC register.

Bits 31 and 30 comprise a two-bit field which indicates the speed that
has been configured.  When the Xilinx IP has been configured for a
fixed speed, it adopts a hard-coded value (in other words, it is read-
only).  When it is read-writable, it defaults to "10" - 1G speed.

So, I think this just works by coincidence, not by proper design,
and therefore your patch in this sub-thread is incorrect since it's
masking the problem.

> But I was wondering if we need this patch anyway, regardless of the proper way to check for the connection setting in this case. Because at the moment calling mac_config with speed=-1 will *delete* the current MAC speed setting and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of the well-known values. I am not sure that is desired behaviour, or speed=-1 just means: don't touch the speed setting. After all we call mac_config with speed=-1 first, even when later fixing this up (see above).

Have you tested 100M and 10M speeds as well - I suspect you'll find
that, as you're relying on the IP default EMMC register setting, it
just won't work with your patches as they stand, because there is
nothing to read the in-band result.  I also don't see anything in
either pg051-tri-mode-eth-mac.pdf or pg047-gig-eth-pcs-pma.pdf which
indicates that the PCS negotiation results are passed automatically
between either IP blocks.

Therefore, I think you _will_ need something like the patch I've
proposed to make this Xilinx IP work properly.

I've augmented the patch with further 1000BASE-X support, including
adding support for configuring the advertisement in the PG047 PCS
registers.  To allow this IP to support 1000BASE-X, from what I
read in these documents, that will also be necessary.

8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phylink: helpers for 802.3 clause 37/SGMII register sets

Implement helpers for PCS accessed via the MII bus using register
sets conforming to 802.3 clause 37. Advertisements for clause 37
and Cisco SGMII are supported by these helpers.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 186 ++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |   9 ++
 2 files changed, 195 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e260098d3719..ed82407240b8 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2081,4 +2081,190 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 }
 EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
 
+static void phylink_decode_advertisement(struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
+
+	linkmode_and(u, state->lp_advertising, state->advertising);
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
+		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->lp_advertising))
+			state->pause |= MLO_PAUSE_TX;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->advertising))
+			state->pause |= MLO_PAUSE_RX;
+	}
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
+		state->speed = SPEED_2500;
+		state->duplex = DUPLEX_FULL;
+	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
+		state->pause = SPEED_1000;
+		state->duplex = DUPLEX_FULL;
+	} else {
+		state->link = false;
+	}
+}
+
+static void phylink_decode_sgmii_word(struct phylink_link_state *state,
+				      uint16_t config_reg)
+{
+	if (!(lpa & BIT(15))) {
+		state->link = false;
+		return;
+	}
+
+	switch (lpa & 0x0c00) {
+	case 0x0000:
+		state->speed = SPEED_10;
+		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
+		break;
+	case 0x0400:
+		state->speed = SPEED_100;
+		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
+		break;
+	case 0x0800:
+		state->speed = SPEED_1000;
+		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
+		break;
+	default:
+		state->link = false;
+		break;
+	}
+}
+
+/**
+ * phylink_mii_pcs_get_state - read the MAC PCS state
+ * @config: a pointer to a &struct phylink_config.
+ * @state: a pointer to a &struct phylink_link_state.
+ *
+ * Helper for MAC PCS supporting the 802.3 register set for clause 37
+ * negotiation and/or SGMII control.
+ *
+ * Read the MAC PCS state from the MII device configured in @config and
+ * parse the Clause 37 or Cisco SGMII link partner negotiation word into
+ * the phylink @state structure. This is suitable to be directly plugged
+ * into the mac_pcs_get_state() member of the struct phylink_mac_ops
+ * structure.
+ */
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int addr = config->pcs_mii_addr;
+	int bmsr, lpa;
+
+	bmsr = mdiobus_read(bus, addr, MII_BMSR);
+	lpa = mdiobus_read(bus, addr, MII_LPA);
+	if (bmsr < 0 || lpa < 0) {
+		state->link = false;
+		return;
+	}
+
+	state->link = !!(bmsr & BMSR_LSTATUS);
+	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
+	if (!state->link)
+		return;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+					 state->lp_advertising);
+		goto lpa_8023z;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		if (lpa & LPA_1000XFULL)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
+					 state->lp_advertising);
+	lpa_8023z:
+		if (lpa & LPA_1000XPAUSE)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_1000XPAUSE_ASYM)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+					 state->lp_advertising);
+		if (lpa & LPA_LPACK)
+			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					 state->lp_advertising);
+		phylink_decode_advertisement(state);
+		break;
+
+	case PHY_INTERFACE_MODE_SGMII:
+		phylink_decode_sgmii_word(state, lpa);
+		break;
+
+	default:
+		state->link = false;
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
+
+/**
+ * phylink_mii_pcs_set_advertisement - configure the clause 37 PCS advertisement
+ * @config: a pointer to a &struct phylink_config.
+ * @state: a pointer to the state being configured.
+ *
+ * Helper for MAC PCS supporting the 802.3 register set for clause 37
+ * negotiation and/or SGMII control.
+ *
+ * Configure the clause 37 PCS advertisement as specified by @state. This
+ * does not trigger a renegotiation; phylink will do that via the
+ * mac_an_restart() method of the struct phylink_mac_ops structure.
+ */
+int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
+				      const struct phylink_link_state *state)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int addr = config->pcs_mii_addr;
+	u16 adv;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		adv = ADVERTISE_1000XFULL;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				      state->advertising))
+			adv |= ADVERTISE_1000XPAUSE;
+		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				      state->advertising))
+			adv |= ADVERTISE_1000XPSE_ASYM;
+		return mdiobus_write(bus, addr, MII_ADVERTISE, adv);
+
+	default:
+		/* Nothing to do for other modes */
+		return 0;
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_set_advertisement);
+
+/**
+ * phylink_mii_pcs_an_restart - restart 802.3z autonegotiation
+ * @config: a pointer to a &struct phylink_config.
+ *
+ * Helper for MAC PCS supporting the 802.3 register set for clause 37
+ * negotiation.
+ *
+ * Restart the clause 37 negotiation with the link partner. This is
+ * suitable to be directly plugged into the mac_pcs_get_state() member
+ * of the struct phylink_mac_ops structure.
+ */
+void phylink_mii_pcs_an_restart(struct phylink_config *config)
+{
+	struct mii_bus *bus = config->pcs_mii;
+	int val, addr = config->pcs_mii_addr;
+
+	val = mdiobus_read(bus, addr, MII_BMCR);
+	if (val >= 0) {
+		val |= BMCR_ANRESTART;
+
+		mdiobus_write(bus, addr, MII_BMCR, val);
+	}
+}
+EXPORT_SYMBOL_GPL(phylink_mii_pcs_an_restart);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 4ea76e083847..d51f45fc5f9a 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -65,6 +65,9 @@ enum phylink_op_type {
 struct phylink_config {
 	struct device *dev;
 	enum phylink_op_type type;
+
+	struct mii_bus *pcs_mii;
+	int pcs_mii_addr;
 };
 
 /**
@@ -292,4 +295,10 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
 						const phy_interface_t *pref,
 						size_t nprefs);
 
+void phylink_mii_pcs_get_state(struct phylink_config *config,
+			       struct phylink_link_state *state);
+int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
+				      const struct phylink_link_state *state);
+void phylink_mii_pcs_an_restart(struct phylink_config *config);
+
 #endif

On Mon, 20 Jan 2020 15:45:54 +0000
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

Hi Russell,

sorry for the delay, some other stuff bubbling up, then I couldn't access the board ...

> On Mon, Jan 20, 2020 at 02:50:28PM +0000, Andre Przywara wrote:
> > On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:  
> > > On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:  
> > >> Maybe something like the below will help?
> > >>
> > >> Basically, use phylink_mii_pcs_get_state() instead of
> > >> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
> > >> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
> > >> access the internal PHY (as per C_PHYADDR parameter.)
> > >>
> > >> You may have some fuzz (with gnu patch) while trying to apply this,
> > >> as you won't have the context for the first and last hunks in this
> > >> patch.
> > >>
> > >> This will probably not be the final version of the patch anyway;
> > >> there's some possibility to pull some of the functionality out of
> > >> phylib into a more general library which would avoid some of the
> > >> functional duplication.  
> > > 
> > > Hi Andre,
> > > 
> > > Did you have a chance to see whether this helps?  
> > 
> > Sorry, I needed some time to wrap my head around your reply first. Am I am still not fully finished with this process ;-)
> > Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it seems to work, because it actually calls axienet_mac_pcs_get_state() to learn the actual negotiated parameters. Then in turn it calls mac_config with the proper speed instead of -1:
> > [  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for inband/sgmii link mode
> > [  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255, pause=16)
> > ...
> > [  153.818568] axienet_mac_pcs_get_state(config): speed=1000, interface=4, pause=0
> > [  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1, pause=0)
> > 
> > Without that DT property it never called mac_pcs_get_state(), so never learnt about the actual settings.
> > But the actual MAC setting was already right (1 GBps, FD). Whether this was by chance (reset value?) or because this was set by the PHY via SGMII, I don't know.
> > So in my case I think I *need* to have the managed = ... property in my DT.  
> 
> I really don't like this guess-work.  The specifications are freely
> available out there, so there's really no need for this.
> 
> pg051-tri-mode-eth-mac.pdf describes the ethernet controller, and
> Table 2-32 therein describes the EMMC register.
> 
> Bits 31 and 30 comprise a two-bit field which indicates the speed that
> has been configured.  When the Xilinx IP has been configured for a
> fixed speed, it adopts a hard-coded value (in other words, it is read-
> only).  When it is read-writable, it defaults to "10" - 1G speed.
> 
> So, I think this just works by coincidence, not by proper design,
> and therefore your patch in this sub-thread is incorrect since it's
> masking the problem.
> 
> > But I was wondering if we need this patch anyway, regardless of the proper way to check for the connection setting in this case. Because at the moment calling mac_config with speed=-1 will *delete* the current MAC speed setting and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of the well-known values. I am not sure that is desired behaviour, or speed=-1 just means: don't touch the speed setting. After all we call mac_config with speed=-1 first, even when later fixing this up (see above).  
> 
> Have you tested 100M and 10M speeds as well - I suspect you'll find
> that, as you're relying on the IP default EMMC register setting, it
> just won't work with your patches as they stand, because there is
> nothing to read the in-band result.  I also don't see anything in
> either pg051-tri-mode-eth-mac.pdf or pg047-gig-eth-pcs-pma.pdf which
> indicates that the PCS negotiation results are passed automatically
> between either IP blocks.
> 
> Therefore, I think you _will_ need something like the patch I've
> proposed to make this Xilinx IP work properly.

OK, I think I begin to understand where you are coming from: Despite using SGMII there is *no* automatic in-band passing of the PHY link status to the MAC (I was working on that assumption and was treating the default 1Gbps as a result of that auto-negotiation).
And since the registers that the manual mentions are actually PHY registers, we need to use MDIO to access them.
And just when I was wondering how I should do this I realised that this is exactly what your patch does ...

So I filled the gaps in there, and that indeed seems to improve now.
Some questions:
- I still always see mac_config() being called with speed=-1 first. With the current mac_config implementation this screws up the MAC setup, but is later corrected (see below). But I would still get that "Speed other than 10, 100 or 1Gbps is not supported" message. So if this speed=-1 some special case that needs extra handling? Where does it actually come from?
- Checking the phylink doc for mac_config() I understand that when using MLO_AN_INBAND, I should "place the link into inband negotiation mode". Does that mean that it should call phylink_mii_pcs_an_restart()? Or is this the responsibility of phylink?
- When using managed = "in-band-status", I see a second call to mac_config() having the right parameters (1Gbps, FD) now, as read by phylink_mii_pcs_get_state(). So this gets eventually set up correctly now, thanks to your patch.
- I initialise "lp->phylink_config.pcs_mii = lp->mii_bus;" in axienet_probe(), just before calling phylink_create(). Where would be the best place to set the PHY address (phylink_config.pcs_mii_addr)? That is not known yet at this point, I guess? (I hacked it to 1 just to test your code).
- When *not* using managed = "in-band-status", I see mac_config still being called with MLO_AN_PHY and speed=-1. Is that expected? Is there something else missing, possibly in the DT? Shouldn't phylink ask the PHY via MDIO about the status first, then come back with the results as parameters to mac_config()? The phylink mac_config() doc just says that we should configure the MAC according to speed, duplex and pause passed in.

Regarding 10/100 Mbps: I can't test any other speeds, because this is on an FPGA in some data centre, and I can't control the other side. I am already happy that I have *some* Ethernet cable connected to it ;-)

Cheers,
Andre.
 
> I've augmented the patch with further 1000BASE-X support, including
> adding support for configuring the advertisement in the PG047 PCS
> registers.  To allow this IP to support 1000BASE-X, from what I
> read in these documents, that will also be necessary.
> 
> 8<===
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] net: phylink: helpers for 802.3 clause 37/SGMII register sets
> 
> Implement helpers for PCS accessed via the MII bus using register
> sets conforming to 802.3 clause 37. Advertisements for clause 37
> and Cisco SGMII are supported by these helpers.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phylink.c | 186 ++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |   9 ++
>  2 files changed, 195 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e260098d3719..ed82407240b8 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2081,4 +2081,190 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>  }
>  EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
>  
> +static void phylink_decode_advertisement(struct phylink_link_state *state)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
> +
> +	linkmode_and(u, state->lp_advertising, state->advertising);
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
> +		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, u)) {
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				      state->lp_advertising))
> +			state->pause |= MLO_PAUSE_TX;
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				      state->advertising))
> +			state->pause |= MLO_PAUSE_RX;
> +	}
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
> +		state->speed = SPEED_2500;
> +		state->duplex = DUPLEX_FULL;
> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, u)) {
> +		state->pause = SPEED_1000;
> +		state->duplex = DUPLEX_FULL;
> +	} else {
> +		state->link = false;
> +	}
> +}
> +
> +static void phylink_decode_sgmii_word(struct phylink_link_state *state,
> +				      uint16_t config_reg)
> +{
> +	if (!(lpa & BIT(15))) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	switch (lpa & 0x0c00) {
> +	case 0x0000:
> +		state->speed = SPEED_10;
> +		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
> +		break;
> +	case 0x0400:
> +		state->speed = SPEED_100;
> +		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
> +		break;
> +	case 0x0800:
> +		state->speed = SPEED_1000;
> +		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
> +		break;
> +	default:
> +		state->link = false;
> +		break;
> +	}
> +}
> +
> +/**
> + * phylink_mii_pcs_get_state - read the MAC PCS state
> + * @config: a pointer to a &struct phylink_config.
> + * @state: a pointer to a &struct phylink_link_state.
> + *
> + * Helper for MAC PCS supporting the 802.3 register set for clause 37
> + * negotiation and/or SGMII control.
> + *
> + * Read the MAC PCS state from the MII device configured in @config and
> + * parse the Clause 37 or Cisco SGMII link partner negotiation word into
> + * the phylink @state structure. This is suitable to be directly plugged
> + * into the mac_pcs_get_state() member of the struct phylink_mac_ops
> + * structure.
> + */
> +void phylink_mii_pcs_get_state(struct phylink_config *config,
> +			       struct phylink_link_state *state)
> +{
> +	struct mii_bus *bus = config->pcs_mii;
> +	int addr = config->pcs_mii_addr;
> +	int bmsr, lpa;
> +
> +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
> +	lpa = mdiobus_read(bus, addr, MII_LPA);
> +	if (bmsr < 0 || lpa < 0) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	state->link = !!(bmsr & BMSR_LSTATUS);
> +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> +	if (!state->link)
> +		return;
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		if (lpa & LPA_1000XFULL)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +					 state->lp_advertising);
> +		goto lpa_8023z;
> +
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		if (lpa & LPA_1000XFULL)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
> +					 state->lp_advertising);
> +	lpa_8023z:
> +		if (lpa & LPA_1000XPAUSE)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +					 state->lp_advertising);
> +		if (lpa & LPA_1000XPAUSE_ASYM)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +					 state->lp_advertising);
> +		if (lpa & LPA_LPACK)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +					 state->lp_advertising);
> +		phylink_decode_advertisement(state);
> +		break;
> +
> +	case PHY_INTERFACE_MODE_SGMII:
> +		phylink_decode_sgmii_word(state, lpa);
> +		break;
> +
> +	default:
> +		state->link = false;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
> +
> +/**
> + * phylink_mii_pcs_set_advertisement - configure the clause 37 PCS advertisement
> + * @config: a pointer to a &struct phylink_config.
> + * @state: a pointer to the state being configured.
> + *
> + * Helper for MAC PCS supporting the 802.3 register set for clause 37
> + * negotiation and/or SGMII control.
> + *
> + * Configure the clause 37 PCS advertisement as specified by @state. This
> + * does not trigger a renegotiation; phylink will do that via the
> + * mac_an_restart() method of the struct phylink_mac_ops structure.
> + */
> +int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
> +				      const struct phylink_link_state *state)
> +{
> +	struct mii_bus *bus = config->pcs_mii;
> +	int addr = config->pcs_mii_addr;
> +	u16 adv;
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		adv = ADVERTISE_1000XFULL;
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				      state->advertising))
> +			adv |= ADVERTISE_1000XPAUSE;
> +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +				      state->advertising))
> +			adv |= ADVERTISE_1000XPSE_ASYM;
> +		return mdiobus_write(bus, addr, MII_ADVERTISE, adv);
> +
> +	default:
> +		/* Nothing to do for other modes */
> +		return 0;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_pcs_set_advertisement);
> +
> +/**
> + * phylink_mii_pcs_an_restart - restart 802.3z autonegotiation
> + * @config: a pointer to a &struct phylink_config.
> + *
> + * Helper for MAC PCS supporting the 802.3 register set for clause 37
> + * negotiation.
> + *
> + * Restart the clause 37 negotiation with the link partner. This is
> + * suitable to be directly plugged into the mac_pcs_get_state() member
> + * of the struct phylink_mac_ops structure.
> + */
> +void phylink_mii_pcs_an_restart(struct phylink_config *config)
> +{
> +	struct mii_bus *bus = config->pcs_mii;
> +	int val, addr = config->pcs_mii_addr;
> +
> +	val = mdiobus_read(bus, addr, MII_BMCR);
> +	if (val >= 0) {
> +		val |= BMCR_ANRESTART;
> +
> +		mdiobus_write(bus, addr, MII_BMCR, val);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phylink_mii_pcs_an_restart);
> +
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 4ea76e083847..d51f45fc5f9a 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -65,6 +65,9 @@ enum phylink_op_type {
>  struct phylink_config {
>  	struct device *dev;
>  	enum phylink_op_type type;
> +
> +	struct mii_bus *pcs_mii;
> +	int pcs_mii_addr;
>  };
>  
>  /**
> @@ -292,4 +295,10 @@ phy_interface_t phylink_select_serdes_interface(unsigned long *interfaces,
>  						const phy_interface_t *pref,
>  						size_t nprefs);
>  
> +void phylink_mii_pcs_get_state(struct phylink_config *config,
> +			       struct phylink_link_state *state);
> +int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
> +				      const struct phylink_link_state *state);
> +void phylink_mii_pcs_an_restart(struct phylink_config *config);
> +
>  #endif

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Monday, January 27, 2020 10:35 PM
> To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Radhey Shyam
> Pandey <radheys@xilinx.com>; Michal Simek <michals@xilinx.com>; linux-
> kernel@vger.kernel.org; Robert Hancock <hancock@sedsystems.ca>; David S .
> Miller <davem@davemloft.net>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 07/14] net: axienet: Fix SGMII support
> 
> On Mon, 20 Jan 2020 15:45:54 +0000
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> Hi Russell,
> 
> sorry for the delay, some other stuff bubbling up, then I couldn't access the
> board ...
> 
> > On Mon, Jan 20, 2020 at 02:50:28PM +0000, Andre Przywara wrote:
> > > On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:
> > > > On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin
> wrote:
> > > >> Maybe something like the below will help?
> > > >>
> > > >> Basically, use phylink_mii_pcs_get_state() instead of
> > > >> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
> > > >> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
> > > >> access the internal PHY (as per C_PHYADDR parameter.)
> > > >>
> > > >> You may have some fuzz (with gnu patch) while trying to apply this,
> > > >> as you won't have the context for the first and last hunks in this
> > > >> patch.
> > > >>
> > > >> This will probably not be the final version of the patch anyway;
> > > >> there's some possibility to pull some of the functionality out of
> > > >> phylib into a more general library which would avoid some of the
> > > >> functional duplication.
> > > >
> > > > Hi Andre,
> > > >
> > > > Did you have a chance to see whether this helps?
> > >
> > > Sorry, I needed some time to wrap my head around your reply first. Am I am
> still not fully finished with this process ;-)
> > > Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it
> seems to work, because it actually calls axienet_mac_pcs_get_state() to learn
> the actual negotiated parameters. Then in turn it calls mac_config with the
> proper speed instead of -1:
> > > [  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for
> inband/sgmii link mode
> > > [  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255,
> pause=16)
> > > ...
> > > [  153.818568] axienet_mac_pcs_get_state(config): speed=1000,
> interface=4, pause=0
> > > [  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1,
> pause=0)
> > >
> > > Without that DT property it never called mac_pcs_get_state(), so never
> learnt about the actual settings.
> > > But the actual MAC setting was already right (1 GBps, FD). Whether this was
> by chance (reset value?) or because this was set by the PHY via SGMII, I don't
> know.
> > > So in my case I think I *need* to have the managed = ... property in my DT.
> >
> > I really don't like this guess-work.  The specifications are freely
> > available out there, so there's really no need for this.
> >
> > pg051-tri-mode-eth-mac.pdf describes the ethernet controller, and
> > Table 2-32 therein describes the EMMC register.
> >
> > Bits 31 and 30 comprise a two-bit field which indicates the speed that
> > has been configured.  When the Xilinx IP has been configured for a
> > fixed speed, it adopts a hard-coded value (in other words, it is read-
> > only).  When it is read-writable, it defaults to "10" - 1G speed.
> >
> > So, I think this just works by coincidence, not by proper design,
> > and therefore your patch in this sub-thread is incorrect since it's
> > masking the problem.
> >
> > > But I was wondering if we need this patch anyway, regardless of the proper
> way to check for the connection setting in this case. Because at the moment
> calling mac_config with speed=-1 will *delete* the current MAC speed setting
> and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of
> the well-known values. I am not sure that is desired behaviour, or speed=-1 just
> means: don't touch the speed setting. After all we call mac_config with speed=-
> 1 first, even when later fixing this up (see above).
> >
> > Have you tested 100M and 10M speeds as well - I suspect you'll find
> > that, as you're relying on the IP default EMMC register setting, it
> > just won't work with your patches as they stand, because there is
> > nothing to read the in-band result.  I also don't see anything in
> > either pg051-tri-mode-eth-mac.pdf or pg047-gig-eth-pcs-pma.pdf which
> > indicates that the PCS negotiation results are passed automatically
> > between either IP blocks.
> >
> > Therefore, I think you _will_ need something like the patch I've
> > proposed to make this Xilinx IP work properly.
> 
> OK, I think I begin to understand where you are coming from: Despite using
> SGMII there is *no* automatic in-band passing of the PHY link status to the MAC
> (I was working on that assumption and was treating the default 1Gbps as a result
> of that auto-negotiation).
> And since the registers that the manual mentions are actually PHY registers, we
> need to use MDIO to access them.
> And just when I was wondering how I should do this I realised that this is exactly
> what your patch does ...
> 
> So I filled the gaps in there, and that indeed seems to improve now.
> Some questions:
> - I still always see mac_config() being called with speed=-1 first. With the current
> mac_config implementation this screws up the MAC setup, but is later corrected
> (see below). But I would still get that "Speed other than 10, 100 or 1Gbps is not
> supported" message. So if this speed=-1 some special case that needs extra
> handling? Where does it actually come from?
> - Checking the phylink doc for mac_config() I understand that when using
> MLO_AN_INBAND, I should "place the link into inband negotiation mode". Does
> that mean that it should call phylink_mii_pcs_an_restart()? Or is this the
> responsibility of phylink?
> - When using managed = "in-band-status", I see a second call to mac_config()
> having the right parameters (1Gbps, FD) now, as read by
> phylink_mii_pcs_get_state(). So this gets eventually set up correctly now, thanks
> to your patch.
> - I initialise "lp->phylink_config.pcs_mii = lp->mii_bus;" in axienet_probe(), just
> before calling phylink_create(). Where would be the best place to set the PHY
> address (phylink_config.pcs_mii_addr)? That is not known yet at this point, I
> guess? (I hacked it to 1 just to test your code).
> - When *not* using managed = "in-band-status", I see mac_config still being
> called with MLO_AN_PHY and speed=-1. Is that expected? Is there something
> else missing, possibly in the DT? Shouldn't phylink ask the PHY via MDIO about
> the status first, then come back with the results as parameters to mac_config()?
> The phylink mac_config() doc just says that we should configure the MAC
> according to speed, duplex and pause passed in.
> 
> Regarding 10/100 Mbps: I can't test any other speeds, because this is on an
> FPGA in some data centre, and I can't control the other side. I am already happy
> that I have *some* Ethernet cable connected to it ;-)

I can help with validating  10/100 Mbps. Related to calling phylink advertisements 
functions-  are we invoking phylink_mii_pcs_set_advertisement from validate
and then in mac_link_state() method call phylink_mii_pcs_get_state?

> 
> Cheers,
> Andre.
> 
> > I've augmented the patch with further 1000BASE-X support, including
> > adding support for configuring the advertisement in the PG047 PCS
> > registers.  To allow this IP to support 1000BASE-X, from what I
> > read in these documents, that will also be necessary.
> >
> > 8<===
> > From: Russell King <rmk+kernel@armlinux.org.uk>
> > Subject: [PATCH] net: phylink: helpers for 802.3 clause 37/SGMII register sets
> >
> > Implement helpers for PCS accessed via the MII bus using register
> > sets conforming to 802.3 clause 37. Advertisements for clause 37
> > and Cisco SGMII are supported by these helpers.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phylink.c | 186
> ++++++++++++++++++++++++++++++++++++++
> >  include/linux/phylink.h   |   9 ++
> >  2 files changed, 195 insertions(+)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index e260098d3719..ed82407240b8 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -2081,4 +2081,190 @@ phy_interface_t
> phylink_select_serdes_interface(unsigned long *interfaces,
> >  }
> >  EXPORT_SYMBOL_GPL(phylink_select_serdes_interface);
> >
> > +static void phylink_decode_advertisement(struct phylink_link_state *state)
> > +{
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(u);
> > +
> > +	linkmode_and(u, state->lp_advertising, state->advertising);
> > +
> > +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, u)) {
> > +		state->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
> > +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> u)) {
> > +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > +				      state->lp_advertising))
> > +			state->pause |= MLO_PAUSE_TX;
> > +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > +				      state->advertising))
> > +			state->pause |= MLO_PAUSE_RX;
> > +	}
> > +
> > +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, u)) {
> > +		state->speed = SPEED_2500;
> > +		state->duplex = DUPLEX_FULL;
> > +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> u)) {
> > +		state->pause = SPEED_1000;
> > +		state->duplex = DUPLEX_FULL;
> > +	} else {
> > +		state->link = false;
> > +	}
> > +}
> > +
> > +static void phylink_decode_sgmii_word(struct phylink_link_state *state,
> > +				      uint16_t config_reg)
> > +{
> > +	if (!(lpa & BIT(15))) {
> > +		state->link = false;
> > +		return;
> > +	}
> > +
> > +	switch (lpa & 0x0c00) {
> > +	case 0x0000:
> > +		state->speed = SPEED_10;
> > +		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
> > +		break;
> > +	case 0x0400:
> > +		state->speed = SPEED_100;
> > +		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
> > +		break;
> > +	case 0x0800:
> > +		state->speed = SPEED_1000;
> > +		state->duplex = lpa & 0x1000 ? DUPLEX_FULL : DUPLEX_HALF;
> > +		break;
> > +	default:
> > +		state->link = false;
> > +		break;
> > +	}
> > +}
> > +
> > +/**
> > + * phylink_mii_pcs_get_state - read the MAC PCS state
> > + * @config: a pointer to a &struct phylink_config.
> > + * @state: a pointer to a &struct phylink_link_state.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 register set for clause 37
> > + * negotiation and/or SGMII control.
> > + *
> > + * Read the MAC PCS state from the MII device configured in @config and
> > + * parse the Clause 37 or Cisco SGMII link partner negotiation word into
> > + * the phylink @state structure. This is suitable to be directly plugged
> > + * into the mac_pcs_get_state() member of the struct phylink_mac_ops
> > + * structure.
> > + */
> > +void phylink_mii_pcs_get_state(struct phylink_config *config,
> > +			       struct phylink_link_state *state)
> > +{
> > +	struct mii_bus *bus = config->pcs_mii;
> > +	int addr = config->pcs_mii_addr;
> > +	int bmsr, lpa;
> > +
> > +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > +	lpa = mdiobus_read(bus, addr, MII_LPA);
> > +	if (bmsr < 0 || lpa < 0) {
> > +		state->link = false;
> > +		return;
> > +	}
> > +
> > +	state->link = !!(bmsr & BMSR_LSTATUS);
> > +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > +	if (!state->link)
> > +		return;
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +		if (lpa & LPA_1000XFULL)
> > +
> 	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> > +					 state->lp_advertising);
> > +		goto lpa_8023z;
> > +
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		if (lpa & LPA_1000XFULL)
> > +
> 	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
> > +					 state->lp_advertising);
> > +	lpa_8023z:
> > +		if (lpa & LPA_1000XPAUSE)
> > +			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > +					 state->lp_advertising);
> > +		if (lpa & LPA_1000XPAUSE_ASYM)
> > +
> 	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > +					 state->lp_advertising);
> > +		if (lpa & LPA_LPACK)
> > +
> 	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > +					 state->lp_advertising);
> > +		phylink_decode_advertisement(state);
> > +		break;
> > +
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +		phylink_decode_sgmii_word(state, lpa);
> > +		break;
> > +
> > +	default:
> > +		state->link = false;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_pcs_get_state);
> > +
> > +/**
> > + * phylink_mii_pcs_set_advertisement - configure the clause 37 PCS
> advertisement
> > + * @config: a pointer to a &struct phylink_config.
> > + * @state: a pointer to the state being configured.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 register set for clause 37
> > + * negotiation and/or SGMII control.
> > + *
> > + * Configure the clause 37 PCS advertisement as specified by @state. This
> > + * does not trigger a renegotiation; phylink will do that via the
> > + * mac_an_restart() method of the struct phylink_mac_ops structure.
> > + */
> > +int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
> > +				      const struct phylink_link_state *state)
> > +{
> > +	struct mii_bus *bus = config->pcs_mii;
> > +	int addr = config->pcs_mii_addr;
> > +	u16 adv;
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		adv = ADVERTISE_1000XFULL;
> > +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > +				      state->advertising))
> > +			adv |= ADVERTISE_1000XPAUSE;
> > +		if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > +				      state->advertising))
> > +			adv |= ADVERTISE_1000XPSE_ASYM;
> > +		return mdiobus_write(bus, addr, MII_ADVERTISE, adv);
> > +
> > +	default:
> > +		/* Nothing to do for other modes */
> > +		return 0;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_pcs_set_advertisement);
> > +
> > +/**
> > + * phylink_mii_pcs_an_restart - restart 802.3z autonegotiation
> > + * @config: a pointer to a &struct phylink_config.
> > + *
> > + * Helper for MAC PCS supporting the 802.3 register set for clause 37
> > + * negotiation.
> > + *
> > + * Restart the clause 37 negotiation with the link partner. This is
> > + * suitable to be directly plugged into the mac_pcs_get_state() member
> > + * of the struct phylink_mac_ops structure.
> > + */
> > +void phylink_mii_pcs_an_restart(struct phylink_config *config)
> > +{
> > +	struct mii_bus *bus = config->pcs_mii;
> > +	int val, addr = config->pcs_mii_addr;
> > +
> > +	val = mdiobus_read(bus, addr, MII_BMCR);
> > +	if (val >= 0) {
> > +		val |= BMCR_ANRESTART;
> > +
> > +		mdiobus_write(bus, addr, MII_BMCR, val);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_mii_pcs_an_restart);
> > +
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > index 4ea76e083847..d51f45fc5f9a 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -65,6 +65,9 @@ enum phylink_op_type {
> >  struct phylink_config {
> >  	struct device *dev;
> >  	enum phylink_op_type type;
> > +
> > +	struct mii_bus *pcs_mii;
> > +	int pcs_mii_addr;
> >  };
> >
> >  /**
> > @@ -292,4 +295,10 @@ phy_interface_t
> phylink_select_serdes_interface(unsigned long *interfaces,
> >  						const phy_interface_t *pref,
> >  						size_t nprefs);
> >
> > +void phylink_mii_pcs_get_state(struct phylink_config *config,
> > +			       struct phylink_link_state *state);
> > +int phylink_mii_pcs_set_advertisement(struct phylink_config *config,
> > +				      const struct phylink_link_state *state);
> > +void phylink_mii_pcs_an_restart(struct phylink_config *config);
> > +
> >  #endif

On Mon, Jan 27, 2020 at 05:04:36PM +0000, Andre Przywara wrote:
> On Mon, 20 Jan 2020 15:45:54 +0000
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> Hi Russell,
> 
> sorry for the delay, some other stuff bubbling up, then I couldn't access the board ...
> 
> > On Mon, Jan 20, 2020 at 02:50:28PM +0000, Andre Przywara wrote:
> > > On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:  
> > > > On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:  
> > > >> Maybe something like the below will help?
> > > >>
> > > >> Basically, use phylink_mii_pcs_get_state() instead of
> > > >> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
> > > >> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
> > > >> access the internal PHY (as per C_PHYADDR parameter.)
> > > >>
> > > >> You may have some fuzz (with gnu patch) while trying to apply this,
> > > >> as you won't have the context for the first and last hunks in this
> > > >> patch.
> > > >>
> > > >> This will probably not be the final version of the patch anyway;
> > > >> there's some possibility to pull some of the functionality out of
> > > >> phylib into a more general library which would avoid some of the
> > > >> functional duplication.  
> > > > 
> > > > Hi Andre,
> > > > 
> > > > Did you have a chance to see whether this helps?  
> > > 
> > > Sorry, I needed some time to wrap my head around your reply first. Am I am still not fully finished with this process ;-)
> > > Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it seems to work, because it actually calls axienet_mac_pcs_get_state() to learn the actual negotiated parameters. Then in turn it calls mac_config with the proper speed instead of -1:
> > > [  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for inband/sgmii link mode
> > > [  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255, pause=16)
> > > ...
> > > [  153.818568] axienet_mac_pcs_get_state(config): speed=1000, interface=4, pause=0
> > > [  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1, pause=0)
> > > 
> > > Without that DT property it never called mac_pcs_get_state(), so never learnt about the actual settings.
> > > But the actual MAC setting was already right (1 GBps, FD). Whether this was by chance (reset value?) or because this was set by the PHY via SGMII, I don't know.
> > > So in my case I think I *need* to have the managed = ... property in my DT.  
> > 
> > I really don't like this guess-work.  The specifications are freely
> > available out there, so there's really no need for this.
> > 
> > pg051-tri-mode-eth-mac.pdf describes the ethernet controller, and
> > Table 2-32 therein describes the EMMC register.
> > 
> > Bits 31 and 30 comprise a two-bit field which indicates the speed that
> > has been configured.  When the Xilinx IP has been configured for a
> > fixed speed, it adopts a hard-coded value (in other words, it is read-
> > only).  When it is read-writable, it defaults to "10" - 1G speed.
> > 
> > So, I think this just works by coincidence, not by proper design,
> > and therefore your patch in this sub-thread is incorrect since it's
> > masking the problem.
> > 
> > > But I was wondering if we need this patch anyway, regardless of the proper way to check for the connection setting in this case. Because at the moment calling mac_config with speed=-1 will *delete* the current MAC speed setting and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of the well-known values. I am not sure that is desired behaviour, or speed=-1 just means: don't touch the speed setting. After all we call mac_config with speed=-1 first, even when later fixing this up (see above).  
> > 
> > Have you tested 100M and 10M speeds as well - I suspect you'll find
> > that, as you're relying on the IP default EMMC register setting, it
> > just won't work with your patches as they stand, because there is
> > nothing to read the in-band result.  I also don't see anything in
> > either pg051-tri-mode-eth-mac.pdf or pg047-gig-eth-pcs-pma.pdf which
> > indicates that the PCS negotiation results are passed automatically
> > between either IP blocks.
> > 
> > Therefore, I think you _will_ need something like the patch I've
> > proposed to make this Xilinx IP work properly.
> 
> OK, I think I begin to understand where you are coming from: Despite using SGMII there is *no* automatic in-band passing of the PHY link status to the MAC (I was working on that assumption and was treating the default 1Gbps as a result of that auto-negotiation).
> And since the registers that the manual mentions are actually PHY registers, we need to use MDIO to access them.
> And just when I was wondering how I should do this I realised that this is exactly what your patch does ...

Yep!  I'm running out of time this evening, but I'll try to get through
as many of your questions as possible before I have to head off.

> So I filled the gaps in there, and that indeed seems to improve now.
> Some questions:
> - I still always see mac_config() being called with speed=-1 first. With the current mac_config implementation this screws up the MAC setup, but is later corrected (see below). But I would still get that "Speed other than 10, 100 or 1Gbps is not supported" message. So if this speed=-1 some special case that needs extra handling? Where does it actually come from?

Yes - that's because we need to do an initial configuration when the
interface is brought up.  Consider the case where we're using
1000base-X:

MAC1 <--> MAC-PCS1 <--> SFP1 <--fiber--> SFP1 <--> MAC-PCS2 <--> MAC2

First, the negotiation is handled purely by the two MAC-PCS blocks,
so these need to be initially configured according to the modes we
wish to advertise.

Second, the MAC and MAC-PCS blocks need to be configured for 1000base-X
mode rather than SGMII, RGMII or whatever else.

Third, depending on the SFP actually plugged in, we may need to
configure 1000base-X, or we may need to configure SGMII.  In more
extreme examples, this inflates to 2500base-X and even 10GBASE-R
modes at the MAC-PCS/serdes.

At the moment, with phylink's current assumption that the MAC PCS
and MAC are tightly integrated, we get away with setting an incomplete
initial configuration, but solving this is rather difficult.  We would
need to read the MAC PCS state and pass the full state that back to
the MAC.

However, one of the guarantees right now is that mac_pcs_get_state()
will be called with state->interface reflecting the previously
configured interface mode in the preceding mac_config() call, which
means mac_pcs_get_state() can interpret the hardware state according to
how it should be configured, so calling mac_pcs_get_state() prior to
the first mac_config() call to get a complete state breaks this
assumption.

What's needed is to split mac_config() into a PCS configuration call
that can be made, then call mac_pcs_get_state(), and pass the resulting
full state to mac_config() - which is great in theory, but needs the
mashed up situation with mvneta/mvpp2 sorted.

You're not the only one with this issue, and when I've raised it
previously (such as earlier today in response to a patch being posted)
their immediate reaction is to go into discussion mode about finding a
different workaround for it - which has the effect that I'm busy
reading their emails and writing responses rather than working towards
a solution to the problem!

> - Checking the phylink doc for mac_config() I understand that when using MLO_AN_INBAND, I should "place the link into inband negotiation mode". Does that mean that it should call phylink_mii_pcs_an_restart()? Or is this the responsibility of phylink?

phylink_mii_pcs_an_restart() is an implementation for the
mac_an_restart() operation in struct phylink_mac_ops.

And... to try and cover two emails in one response (there's another
reply in this thread from someone else, sorry I can't check your
name right now) - phylink_mii_pcs_set_advertisement() is a helper for
use in mac_config().  To deal further with that reply, the validate()
callback must not change any hardware state, and therefore must not
call phylink_mii_pcs_set_advertisement().

> - When using managed = "in-band-status", I see a second call to mac_config() having the right parameters (1Gbps, FD) now, as read by phylink_mii_pcs_get_state(). So this gets eventually set up correctly now, thanks to your patch.

Yes, that will happen on the first link resolution, which will occur
shortly after the call to phylink_start().

> - I initialise "lp->phylink_config.pcs_mii = lp->mii_bus;" in axienet_probe(), just before calling phylink_create(). Where would be the best place to set the PHY address (phylink_config.pcs_mii_addr)? That is not known yet at this point, I guess? (I hacked it to 1 just to test your code).

It only needs to be set before any of the phylink_mii_pcs_*() functions
are called - which basically means at the latest before the first call
to phylink_start().

> - When *not* using managed = "in-band-status", I see mac_config still being called with MLO_AN_PHY and speed=-1. Is that expected? Is there something else missing, possibly in the DT? Shouldn't phylink ask the PHY via MDIO about the status first, then come back with the results as parameters to mac_config()? The phylink mac_config() doc just says that we should configure the MAC according to speed, duplex and pause passed in.

That's probably the same as your earlier point - the initial
configuration being set, rather than the resolve.  With PHY mode,
mac_config() won't be called for a link resolution unless the link
is up.

> Regarding 10/100 Mbps: I can't test any other speeds, because this is on an FPGA in some data centre, and I can't control the other side. I am already happy that I have *some* Ethernet cable connected to it ;-)

I was going to ask you whether the hardware was available, assuming it
was on your desk, but if it's in a data centre somewhere, it suggests
it isn't that widely available.

However, I've been thinking about Andrew Lunn's issues with the serdes
block on Marvell DSA bridges, and that's basically implementing exactly
the same as your PCS PHY - and I have those boards.  So, that gives me
a way to test this code.  If I get a chance tomorrow, I'll try to make
some progress there.

I also suspect that with the LX2160A hardware on the other end of one
of the fiber links from the ZII board, I may be able to trick the ZII
board into thinking there's a SGMII PHY on the other end too... I know
how to set the 16-bit control word used for the inband configuration
to anything I desire!

Hi Andre/Russell,

Just wondering where things got to with the changes for SGMII on Xilinx 
axienet that you were discussing (below)? I am looking into our Xilinx 
setup using 1000BaseX SFP and trying to get it working "properly" with 
newer kernels. My understanding is that the requirements for 1000BaseX 
and SGMII are somewhat similar. I gathered that SGMII was working 
somewhat already, but that not all link modes had been tested. However, 
it appears 1000BaseX is not yet working in the stock kernel.

The way I had this working before with a 4.19-based kernel was basically 
a hack to phylink to allow the Xilinx PCS/PMA PHY to be configured 
sufficiently as a PHY for it to work, and mostly ignored the link status 
of the SFP PHY itself, even though we were using in-band signalling mode 
with an SFP module. That was using this patch:

https://patchwork.ozlabs.org/project/netdev/patch/1559330285-30246-5-git-send-email-hancock@sedsystems.ca/

Of course, that's basically just a hack which I suspect mostly worked by 
luck. I see that there are some helpers that were added to phylink to 
allow setting PHY advertisements and reading PHY status from clause 22 
PHY devices, so I'm guessing that is the way to go in this case? 
Something like:

axienet_mac_config: if using in-band mode, use 
phylink_mii_c22_pcs_set_advertisement to configure the Xilinx PHY.

axienet_mac_pcs_get_state: use phylink_mii_c22_pcs_get_state to get the 
MAC PCS state from the Xilinx PHY

axienet_mac_an_restart: if using in-band mode, use 
phylink_mii_c22_pcs_an_restart to restart autonegotiation on Xilinx PHY

To use those c22 functions, we need to find the mdio_device that's 
referenced by the phy-handle in the device tree - I guess we can just 
use some of the guts of of_phy_find_device to do that?

One concern I have is that there may be things that the PHY subsystem 
would configure on the device that may need to be replicated in order to 
get it to actually work - things like setting auto-negotiate 
enable/disable, the BMCR_ISOLATE bit, etc - is that something that 
belongs in our mac_config or in the phylink core in 
phylink_mii_c22_pcs_set_advertisement etc?

On 2020-01-27 12:53 p.m., Russell King - ARM Linux admin wrote:
> On Mon, Jan 27, 2020 at 05:04:36PM +0000, Andre Przywara wrote:
>> On Mon, 20 Jan 2020 15:45:54 +0000
>> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
>>
>> Hi Russell,
>>
>> sorry for the delay, some other stuff bubbling up, then I couldn't access the board ...
>>
>>> On Mon, Jan 20, 2020 at 02:50:28PM +0000, Andre Przywara wrote:
>>>> On 18/01/2020 11:22, Russell King - ARM Linux admin wrote:
>>>>> On Fri, Jan 10, 2020 at 05:04:57PM +0000, Russell King - ARM Linux admin wrote:
>>>>>> Maybe something like the below will help?
>>>>>>
>>>>>> Basically, use phylink_mii_pcs_get_state() instead of
>>>>>> axienet_mac_pcs_get_state(), and setup lp->phylink_config.pcs_mii
>>>>>> to point at the MII bus, and lp->phylink_config.pcs_mii_addr to
>>>>>> access the internal PHY (as per C_PHYADDR parameter.)
>>>>>>
>>>>>> You may have some fuzz (with gnu patch) while trying to apply this,
>>>>>> as you won't have the context for the first and last hunks in this
>>>>>> patch.
>>>>>>
>>>>>> This will probably not be the final version of the patch anyway;
>>>>>> there's some possibility to pull some of the functionality out of
>>>>>> phylib into a more general library which would avoid some of the
>>>>>> functional duplication.
>>>>>
>>>>> Hi Andre,
>>>>>
>>>>> Did you have a chance to see whether this helps?
>>>>
>>>> Sorry, I needed some time to wrap my head around your reply first. Am I am still not fully finished with this process ;-)
>>>> Anyway I observed that when I add 'managed = "in-band-status"' to the DT, it seems to work, because it actually calls axienet_mac_pcs_get_state() to learn the actual negotiated parameters. Then in turn it calls mac_config with the proper speed instead of -1:
>>>> [  151.682532] xilinx_axienet 7fe00000.ethernet eth0: configuring for inband/sgmii link mode
>>>> [  151.710743] axienet_mac_config(config, mode=2, speed=-1, duplex=255, pause=16)
>>>> ...
>>>> [  153.818568] axienet_mac_pcs_get_state(config): speed=1000, interface=4, pause=0
>>>> [  153.842244] axienet_mac_config(config, mode=2, speed=1000, duplex=1, pause=0)
>>>>
>>>> Without that DT property it never called mac_pcs_get_state(), so never learnt about the actual settings.
>>>> But the actual MAC setting was already right (1 GBps, FD). Whether this was by chance (reset value?) or because this was set by the PHY via SGMII, I don't know.
>>>> So in my case I think I *need* to have the managed = ... property in my DT.
>>>
>>> I really don't like this guess-work.  The specifications are freely
>>> available out there, so there's really no need for this.
>>>
>>> pg051-tri-mode-eth-mac.pdf describes the ethernet controller, and
>>> Table 2-32 therein describes the EMMC register.
>>>
>>> Bits 31 and 30 comprise a two-bit field which indicates the speed that
>>> has been configured.  When the Xilinx IP has been configured for a
>>> fixed speed, it adopts a hard-coded value (in other words, it is read-
>>> only).  When it is read-writable, it defaults to "10" - 1G speed.
>>>
>>> So, I think this just works by coincidence, not by proper design,
>>> and therefore your patch in this sub-thread is incorrect since it's
>>> masking the problem.
>>>
>>>> But I was wondering if we need this patch anyway, regardless of the proper way to check for the connection setting in this case. Because at the moment calling mac_config with speed=-1 will *delete* the current MAC speed setting and leave it as 10 Mbps (because this is encoded as 0), when speed is not one of the well-known values. I am not sure that is desired behaviour, or speed=-1 just means: don't touch the speed setting. After all we call mac_config with speed=-1 first, even when later fixing this up (see above).
>>>
>>> Have you tested 100M and 10M speeds as well - I suspect you'll find
>>> that, as you're relying on the IP default EMMC register setting, it
>>> just won't work with your patches as they stand, because there is
>>> nothing to read the in-band result.  I also don't see anything in
>>> either pg051-tri-mode-eth-mac.pdf or pg047-gig-eth-pcs-pma.pdf which
>>> indicates that the PCS negotiation results are passed automatically
>>> between either IP blocks.
>>>
>>> Therefore, I think you _will_ need something like the patch I've
>>> proposed to make this Xilinx IP work properly.
>>
>> OK, I think I begin to understand where you are coming from: Despite using SGMII there is *no* automatic in-band passing of the PHY link status to the MAC (I was working on that assumption and was treating the default 1Gbps as a result of that auto-negotiation).
>> And since the registers that the manual mentions are actually PHY registers, we need to use MDIO to access them.
>> And just when I was wondering how I should do this I realised that this is exactly what your patch does ...
> 
> Yep!  I'm running out of time this evening, but I'll try to get through
> as many of your questions as possible before I have to head off.
> 
>> So I filled the gaps in there, and that indeed seems to improve now.
>> Some questions:
>> - I still always see mac_config() being called with speed=-1 first. With the current mac_config implementation this screws up the MAC setup, but is later corrected (see below). But I would still get that "Speed other than 10, 100 or 1Gbps is not supported" message. So if this speed=-1 some special case that needs extra handling? Where does it actually come from?
> 
> Yes - that's because we need to do an initial configuration when the
> interface is brought up.  Consider the case where we're using
> 1000base-X:
> 
> MAC1 <--> MAC-PCS1 <--> SFP1 <--fiber--> SFP1 <--> MAC-PCS2 <--> MAC2
> 
> First, the negotiation is handled purely by the two MAC-PCS blocks,
> so these need to be initially configured according to the modes we
> wish to advertise.
> 
> Second, the MAC and MAC-PCS blocks need to be configured for 1000base-X
> mode rather than SGMII, RGMII or whatever else.
> 
> Third, depending on the SFP actually plugged in, we may need to
> configure 1000base-X, or we may need to configure SGMII.  In more
> extreme examples, this inflates to 2500base-X and even 10GBASE-R
> modes at the MAC-PCS/serdes.
> 
> At the moment, with phylink's current assumption that the MAC PCS
> and MAC are tightly integrated, we get away with setting an incomplete
> initial configuration, but solving this is rather difficult.  We would
> need to read the MAC PCS state and pass the full state that back to
> the MAC.
> 
> However, one of the guarantees right now is that mac_pcs_get_state()
> will be called with state->interface reflecting the previously
> configured interface mode in the preceding mac_config() call, which
> means mac_pcs_get_state() can interpret the hardware state according to
> how it should be configured, so calling mac_pcs_get_state() prior to
> the first mac_config() call to get a complete state breaks this
> assumption.
> 
> What's needed is to split mac_config() into a PCS configuration call
> that can be made, then call mac_pcs_get_state(), and pass the resulting
> full state to mac_config() - which is great in theory, but needs the
> mashed up situation with mvneta/mvpp2 sorted.
> 
> You're not the only one with this issue, and when I've raised it
> previously (such as earlier today in response to a patch being posted)
> their immediate reaction is to go into discussion mode about finding a
> different workaround for it - which has the effect that I'm busy
> reading their emails and writing responses rather than working towards
> a solution to the problem!
> 
>> - Checking the phylink doc for mac_config() I understand that when using MLO_AN_INBAND, I should "place the link into inband negotiation mode". Does that mean that it should call phylink_mii_pcs_an_restart()? Or is this the responsibility of phylink?
> 
> phylink_mii_pcs_an_restart() is an implementation for the
> mac_an_restart() operation in struct phylink_mac_ops.
> 
> And... to try and cover two emails in one response (there's another
> reply in this thread from someone else, sorry I can't check your
> name right now) - phylink_mii_pcs_set_advertisement() is a helper for
> use in mac_config().  To deal further with that reply, the validate()
> callback must not change any hardware state, and therefore must not
> call phylink_mii_pcs_set_advertisement().
> 
>> - When using managed = "in-band-status", I see a second call to mac_config() having the right parameters (1Gbps, FD) now, as read by phylink_mii_pcs_get_state(). So this gets eventually set up correctly now, thanks to your patch.
> 
> Yes, that will happen on the first link resolution, which will occur
> shortly after the call to phylink_start().
> 
>> - I initialise "lp->phylink_config.pcs_mii = lp->mii_bus;" in axienet_probe(), just before calling phylink_create(). Where would be the best place to set the PHY address (phylink_config.pcs_mii_addr)? That is not known yet at this point, I guess? (I hacked it to 1 just to test your code).
> 
> It only needs to be set before any of the phylink_mii_pcs_*() functions
> are called - which basically means at the latest before the first call
> to phylink_start().
> 
>> - When *not* using managed = "in-band-status", I see mac_config still being called with MLO_AN_PHY and speed=-1. Is that expected? Is there something else missing, possibly in the DT? Shouldn't phylink ask the PHY via MDIO about the status first, then come back with the results as parameters to mac_config()? The phylink mac_config() doc just says that we should configure the MAC according to speed, duplex and pause passed in.
> 
> That's probably the same as your earlier point - the initial
> configuration being set, rather than the resolve.  With PHY mode,
> mac_config() won't be called for a link resolution unless the link
> is up.
> 
>> Regarding 10/100 Mbps: I can't test any other speeds, because this is on an FPGA in some data centre, and I can't control the other side. I am already happy that I have *some* Ethernet cable connected to it ;-)
> 
> I was going to ask you whether the hardware was available, assuming it
> was on your desk, but if it's in a data centre somewhere, it suggests
> it isn't that widely available.
> 
> However, I've been thinking about Andrew Lunn's issues with the serdes
> block on Marvell DSA bridges, and that's basically implementing exactly
> the same as your PCS PHY - and I have those boards.  So, that gives me
> a way to test this code.  If I get a chance tomorrow, I'll try to make
> some progress there.
> 
> I also suspect that with the LX2160A hardware on the other end of one
> of the fiber links from the ZII board, I may be able to trick the ZII
> board into thinking there's a SGMII PHY on the other end too... I know
> how to set the 16-bit control word used for the inband configuration
> to anything I desire!
>

On Tue, Apr 21, 2020 at 07:45:47PM -0600, Robert Hancock wrote:
> Hi Andre/Russell,
> 
> Just wondering where things got to with the changes for SGMII on Xilinx
> axienet that you were discussing (below)? I am looking into our Xilinx setup
> using 1000BaseX SFP and trying to get it working "properly" with newer
> kernels. My understanding is that the requirements for 1000BaseX and SGMII
> are somewhat similar. I gathered that SGMII was working somewhat already,
> but that not all link modes had been tested. However, it appears 1000BaseX
> is not yet working in the stock kernel.
> 
> The way I had this working before with a 4.19-based kernel was basically a
> hack to phylink to allow the Xilinx PCS/PMA PHY to be configured
> sufficiently as a PHY for it to work, and mostly ignored the link status of
> the SFP PHY itself, even though we were using in-band signalling mode with
> an SFP module. That was using this patch:
> 
> https://patchwork.ozlabs.org/project/netdev/patch/1559330285-30246-5-git-send-email-hancock@sedsystems.ca/
> 
> Of course, that's basically just a hack which I suspect mostly worked by
> luck. I see that there are some helpers that were added to phylink to allow
> setting PHY advertisements and reading PHY status from clause 22 PHY
> devices, so I'm guessing that is the way to go in this case? Something like:
> 
> axienet_mac_config: if using in-band mode, use
> phylink_mii_c22_pcs_set_advertisement to configure the Xilinx PHY.
> 
> axienet_mac_pcs_get_state: use phylink_mii_c22_pcs_get_state to get the MAC
> PCS state from the Xilinx PHY
> 
> axienet_mac_an_restart: if using in-band mode, use
> phylink_mii_c22_pcs_an_restart to restart autonegotiation on Xilinx PHY
> 
> To use those c22 functions, we need to find the mdio_device that's
> referenced by the phy-handle in the device tree - I guess we can just use
> some of the guts of of_phy_find_device to do that?

Please see the code for DPAA2 - it's changed slightly since I sent a
copy to the netdev mailing list, and it still isn't clear whether this
is the final approach (DPAA2 has some fun stuff such as several
different PHYs at address 0.) NXP basically didn't like the approach
I had in the patches I sent to netdev, we had a call, they presented
an alternative appraoch, I implemented it, then they decided my
original approach was the better solution for their situation.

See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=cex7

specifically the patches from:

  "dpaa2-mac: add 1000BASE-X/SGMII PCS support"

through to:

  "net: phylink: add interface to configure clause 22 PCS PHY"

You may also need some of the patches further down in the net-queue
branch:

  "net: phylink: avoid mac_config calls"

through to:

  "net: phylink: rejig link state tracking"

> One concern I have is that there may be things that the PHY subsystem would
> configure on the device that may need to be replicated in order to get it to
> actually work - things like setting auto-negotiate enable/disable, the
> BMCR_ISOLATE bit, etc - is that something that belongs in our mac_config or
> in the phylink core in phylink_mii_c22_pcs_set_advertisement etc?

I think some of that is addressed in the above patches, except for
the isolate bit - do your PHYs come up with the isolate bit set?
Under what circumstances would you need to set it?

Let me know how you get on.

Thanks.

On 2020-04-22 1:51 a.m., Russell King - ARM Linux admin wrote:
> On Tue, Apr 21, 2020 at 07:45:47PM -0600, Robert Hancock wrote:
>> Hi Andre/Russell,
>>
>> Just wondering where things got to with the changes for SGMII on Xilinx
>> axienet that you were discussing (below)? I am looking into our Xilinx setup
>> using 1000BaseX SFP and trying to get it working "properly" with newer
>> kernels. My understanding is that the requirements for 1000BaseX and SGMII
>> are somewhat similar. I gathered that SGMII was working somewhat already,
>> but that not all link modes had been tested. However, it appears 1000BaseX
>> is not yet working in the stock kernel.
>>
>> The way I had this working before with a 4.19-based kernel was basically a
>> hack to phylink to allow the Xilinx PCS/PMA PHY to be configured
>> sufficiently as a PHY for it to work, and mostly ignored the link status of
>> the SFP PHY itself, even though we were using in-band signalling mode with
>> an SFP module. That was using this patch:
>>
>> https://patchwork.ozlabs.org/project/netdev/patch/1559330285-30246-5-git-send-email-hancock@sedsystems.ca/
>>
>> Of course, that's basically just a hack which I suspect mostly worked by
>> luck. I see that there are some helpers that were added to phylink to allow
>> setting PHY advertisements and reading PHY status from clause 22 PHY
>> devices, so I'm guessing that is the way to go in this case? Something like:
>>
>> axienet_mac_config: if using in-band mode, use
>> phylink_mii_c22_pcs_set_advertisement to configure the Xilinx PHY.
>>
>> axienet_mac_pcs_get_state: use phylink_mii_c22_pcs_get_state to get the MAC
>> PCS state from the Xilinx PHY
>>
>> axienet_mac_an_restart: if using in-band mode, use
>> phylink_mii_c22_pcs_an_restart to restart autonegotiation on Xilinx PHY
>>
>> To use those c22 functions, we need to find the mdio_device that's
>> referenced by the phy-handle in the device tree - I guess we can just use
>> some of the guts of of_phy_find_device to do that?
> 
> Please see the code for DPAA2 - it's changed slightly since I sent a
> copy to the netdev mailing list, and it still isn't clear whether this
> is the final approach (DPAA2 has some fun stuff such as several
> different PHYs at address 0.) NXP basically didn't like the approach
> I had in the patches I sent to netdev, we had a call, they presented
> an alternative appraoch, I implemented it, then they decided my
> original approach was the better solution for their situation.
> 
> See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=cex7
> 
> specifically the patches from:
> 
>    "dpaa2-mac: add 1000BASE-X/SGMII PCS support"
> 
> through to:
> 
>    "net: phylink: add interface to configure clause 22 PCS PHY"
> 
> You may also need some of the patches further down in the net-queue
> branch:
> 
>    "net: phylink: avoid mac_config calls"
> 
> through to:
> 
>    "net: phylink: rejig link state tracking"

Thanks for the info. I've yet to decide whether or how I'm going to 
attempt this task at the moment - it seems like there have been a lot of 
changes to phylink and other related code lately and it appears to be a 
bit of a task to backport them all into a released kernel version, more 
so back to 5.4 which is the latest LTS kernel which we would ideally 
want to use. So I might end up trying to open-code this more inside the 
driver and eventually switching it to use the shared code when it's 
merged, or maybe just deferring the proper fix for this until the 
infrastructure is more in place in mainline.

> 
>> One concern I have is that there may be things that the PHY subsystem would
>> configure on the device that may need to be replicated in order to get it to
>> actually work - things like setting auto-negotiate enable/disable, the
>> BMCR_ISOLATE bit, etc - is that something that belongs in our mac_config or
>> in the phylink core in phylink_mii_c22_pcs_set_advertisement etc?
> 
> I think some of that is addressed in the above patches, except for
> the isolate bit - do your PHYs come up with the isolate bit set?
> Under what circumstances would you need to set it?

I believe it does come up isolated - from the Xilinx PG047 document for 
the PCS/PMA core:

"Start-up Sequencing
IEEE 802.3-2008 clause 22.2.4.1.6 states that by default, a PHY should 
power up in an isolate state (electrically isolated from the GMII).
If you are using the core with the optional management interface, it is 
necessary to write to the PCS Configuration Register 0 to take the core 
out of the isolate state."

Also, in PG138 for the AXI Ethernet core, under Gigabit Ethernet PCS/PMA 
Management Registers, it has this note:

"When using the 1000BASE-X TEMAC core (C_TYPE = 1 and C_PHY_TYPE = 5), 
set the isolate bit to zero (Control register 0 bit 10). The subsystem 
is not operational until this is completed."

> 
> Let me know how you get on.
> 
> Thanks.
>

On 2020-04-22 1:51 a.m., Russell King - ARM Linux admin wrote:
> On Tue, Apr 21, 2020 at 07:45:47PM -0600, Robert Hancock wrote:
>> Hi Andre/Russell,
>>
>> Just wondering where things got to with the changes for SGMII on Xilinx
>> axienet that you were discussing (below)? I am looking into our Xilinx setup
>> using 1000BaseX SFP and trying to get it working "properly" with newer
>> kernels. My understanding is that the requirements for 1000BaseX and SGMII
>> are somewhat similar. I gathered that SGMII was working somewhat already,
>> but that not all link modes had been tested. However, it appears 1000BaseX
>> is not yet working in the stock kernel.
>>
>> The way I had this working before with a 4.19-based kernel was basically a
>> hack to phylink to allow the Xilinx PCS/PMA PHY to be configured
>> sufficiently as a PHY for it to work, and mostly ignored the link status of
>> the SFP PHY itself, even though we were using in-band signalling mode with
>> an SFP module. That was using this patch:
>>
>> https://patchwork.ozlabs.org/project/netdev/patch/1559330285-30246-5-git-send-email-hancock@sedsystems.ca/
>>
>> Of course, that's basically just a hack which I suspect mostly worked by
>> luck. I see that there are some helpers that were added to phylink to allow
>> setting PHY advertisements and reading PHY status from clause 22 PHY
>> devices, so I'm guessing that is the way to go in this case? Something like:
>>
>> axienet_mac_config: if using in-band mode, use
>> phylink_mii_c22_pcs_set_advertisement to configure the Xilinx PHY.
>>
>> axienet_mac_pcs_get_state: use phylink_mii_c22_pcs_get_state to get the MAC
>> PCS state from the Xilinx PHY
>>
>> axienet_mac_an_restart: if using in-band mode, use
>> phylink_mii_c22_pcs_an_restart to restart autonegotiation on Xilinx PHY
>>
>> To use those c22 functions, we need to find the mdio_device that's
>> referenced by the phy-handle in the device tree - I guess we can just use
>> some of the guts of of_phy_find_device to do that?
> 
> Please see the code for DPAA2 - it's changed slightly since I sent a
> copy to the netdev mailing list, and it still isn't clear whether this
> is the final approach (DPAA2 has some fun stuff such as several
> different PHYs at address 0.) NXP basically didn't like the approach
> I had in the patches I sent to netdev, we had a call, they presented
> an alternative appraoch, I implemented it, then they decided my
> original approach was the better solution for their situation.
> 
> See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=cex7
> 
> specifically the patches from:
> 
>    "dpaa2-mac: add 1000BASE-X/SGMII PCS support"
> 
> through to:
> 
>    "net: phylink: add interface to configure clause 22 PCS PHY"
> 
> You may also need some of the patches further down in the net-queue
> branch:
> 
>    "net: phylink: avoid mac_config calls"
> 
> through to:
> 
>    "net: phylink: rejig link state tracking"

I've been playing with this a bit on a 5.4 kernel with some of these 
patches backported. However, I'm running into something that my previous 
hacks for this basically dealt with as a side effect: when phylink_start 
is called, sfp_upstream_start gets called, an SFP module is detected, 
phylink_connect_phy gets called, but then it hits this condition and 
bails out, because we are using INBAND mode with 1000BaseX:

	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
		     phy_interface_mode_is_8023z(interface))))
		return -EINVAL;

That same code is still in the latest version in the arm-linux cex7 
branch, except now in phylink_attach_phy, and from what I can see would 
behave similarly.

I guess I'm not sure how this is supposed to work when the PHY on the 
SFP module gets detected, i.e. if there's supposed to be another code 
path that this is supposed to go down, or this is something that just 
hasn't been fully implemented yet?

On Tue, Apr 28, 2020 at 03:59:45PM -0600, Robert Hancock wrote:
> On 2020-04-22 1:51 a.m., Russell King - ARM Linux admin wrote:
> > On Tue, Apr 21, 2020 at 07:45:47PM -0600, Robert Hancock wrote:
> > > Hi Andre/Russell,
> > > 
> > > Just wondering where things got to with the changes for SGMII on Xilinx
> > > axienet that you were discussing (below)? I am looking into our Xilinx setup
> > > using 1000BaseX SFP and trying to get it working "properly" with newer
> > > kernels. My understanding is that the requirements for 1000BaseX and SGMII
> > > are somewhat similar. I gathered that SGMII was working somewhat already,
> > > but that not all link modes had been tested. However, it appears 1000BaseX
> > > is not yet working in the stock kernel.
> > > 
> > > The way I had this working before with a 4.19-based kernel was basically a
> > > hack to phylink to allow the Xilinx PCS/PMA PHY to be configured
> > > sufficiently as a PHY for it to work, and mostly ignored the link status of
> > > the SFP PHY itself, even though we were using in-band signalling mode with
> > > an SFP module. That was using this patch:
> > > 
> > > https://patchwork.ozlabs.org/project/netdev/patch/1559330285-30246-5-git-send-email-hancock@sedsystems.ca/
> > > 
> > > Of course, that's basically just a hack which I suspect mostly worked by
> > > luck. I see that there are some helpers that were added to phylink to allow
> > > setting PHY advertisements and reading PHY status from clause 22 PHY
> > > devices, so I'm guessing that is the way to go in this case? Something like:
> > > 
> > > axienet_mac_config: if using in-band mode, use
> > > phylink_mii_c22_pcs_set_advertisement to configure the Xilinx PHY.
> > > 
> > > axienet_mac_pcs_get_state: use phylink_mii_c22_pcs_get_state to get the MAC
> > > PCS state from the Xilinx PHY
> > > 
> > > axienet_mac_an_restart: if using in-band mode, use
> > > phylink_mii_c22_pcs_an_restart to restart autonegotiation on Xilinx PHY
> > > 
> > > To use those c22 functions, we need to find the mdio_device that's
> > > referenced by the phy-handle in the device tree - I guess we can just use
> > > some of the guts of of_phy_find_device to do that?
> > 
> > Please see the code for DPAA2 - it's changed slightly since I sent a
> > copy to the netdev mailing list, and it still isn't clear whether this
> > is the final approach (DPAA2 has some fun stuff such as several
> > different PHYs at address 0.) NXP basically didn't like the approach
> > I had in the patches I sent to netdev, we had a call, they presented
> > an alternative appraoch, I implemented it, then they decided my
> > original approach was the better solution for their situation.
> > 
> > See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=cex7
> > 
> > specifically the patches from:
> > 
> >    "dpaa2-mac: add 1000BASE-X/SGMII PCS support"
> > 
> > through to:
> > 
> >    "net: phylink: add interface to configure clause 22 PCS PHY"
> > 
> > You may also need some of the patches further down in the net-queue
> > branch:
> > 
> >    "net: phylink: avoid mac_config calls"
> > 
> > through to:
> > 
> >    "net: phylink: rejig link state tracking"
> 
> I've been playing with this a bit on a 5.4 kernel with some of these patches
> backported. However, I'm running into something that my previous hacks for
> this basically dealt with as a side effect: when phylink_start is called,
> sfp_upstream_start gets called, an SFP module is detected,
> phylink_connect_phy gets called, but then it hits this condition and bails
> out, because we are using INBAND mode with 1000BaseX:
> 
> 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
> 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
> 		     phy_interface_mode_is_8023z(interface))))
> 		return -EINVAL;

I'm expecting SGMII mode to be used when there's an external PHY as
that gives greatest flexibility (as it allows 10 and 100Mbps speeds
as well.)  From what I remember, these blocks support SGMII, so it
should just be a matter of adding that.

> I guess I'm not sure how this is supposed to work when the PHY on the SFP
> module gets detected, i.e. if there's supposed to be another code path that
> this is supposed to go down, or this is something that just hasn't been
> fully implemented yet?

Copper PHYs work fine - using SGMII mode everywhere so far.

The problem is, if you want to use them as 1000BASE-X, you generally
have to ensure that the PHY is appropriately programmed for 1000BASE-X
negotiation, and the copper side advertisement only indicates 1G
support. Not all copper PHYs have the PHY accessible for such
programming, and in that case it becomes an exercise of "read the
SFP documentation before buying"!

The other complication is... there's nothing in the module EEPROM
that really says whether they are 1000BASE-X or SGMII.

What saves us thus far is that most copper SFPs use the Marvell
88E1111 chip, which is I2C accessible, and we drive that using
phylib - and the phylib Marvell driver knows how to ensure that
the PHY is configured for SGMII mode.  I'm not sure the same is
true with 1000BASE-X mode.

On 2020-04-28 5:01 p.m., Russell King - ARM Linux admin wrote:
> On Tue, Apr 28, 2020 at 03:59:45PM -0600, Robert Hancock wrote:
>> On 2020-04-22 1:51 a.m., Russell King - ARM Linux admin wrote:
>>> On Tue, Apr 21, 2020 at 07:45:47PM -0600, Robert Hancock wrote:
>>>> Hi Andre/Russell,
>>>>
>>>> Just wondering where things got to with the changes for SGMII on Xilinx
>>>> axienet that you were discussing (below)? I am looking into our Xilinx setup
>>>> using 1000BaseX SFP and trying to get it working "properly" with newer
>>>> kernels. My understanding is that the requirements for 1000BaseX and SGMII
>>>> are somewhat similar. I gathered that SGMII was working somewhat already,
>>>> but that not all link modes had been tested. However, it appears 1000BaseX
>>>> is not yet working in the stock kernel.
>>>>
>>>> The way I had this working before with a 4.19-based kernel was basically a
>>>> hack to phylink to allow the Xilinx PCS/PMA PHY to be configured
>>>> sufficiently as a PHY for it to work, and mostly ignored the link status of
>>>> the SFP PHY itself, even though we were using in-band signalling mode with
>>>> an SFP module. That was using this patch:
>>>>
>>>> https://patchwork.ozlabs.org/project/netdev/patch/1559330285-30246-5-git-send-email-hancock@sedsystems.ca/
>>>>
>>>> Of course, that's basically just a hack which I suspect mostly worked by
>>>> luck. I see that there are some helpers that were added to phylink to allow
>>>> setting PHY advertisements and reading PHY status from clause 22 PHY
>>>> devices, so I'm guessing that is the way to go in this case? Something like:
>>>>
>>>> axienet_mac_config: if using in-band mode, use
>>>> phylink_mii_c22_pcs_set_advertisement to configure the Xilinx PHY.
>>>>
>>>> axienet_mac_pcs_get_state: use phylink_mii_c22_pcs_get_state to get the MAC
>>>> PCS state from the Xilinx PHY
>>>>
>>>> axienet_mac_an_restart: if using in-band mode, use
>>>> phylink_mii_c22_pcs_an_restart to restart autonegotiation on Xilinx PHY
>>>>
>>>> To use those c22 functions, we need to find the mdio_device that's
>>>> referenced by the phy-handle in the device tree - I guess we can just use
>>>> some of the guts of of_phy_find_device to do that?
>>>
>>> Please see the code for DPAA2 - it's changed slightly since I sent a
>>> copy to the netdev mailing list, and it still isn't clear whether this
>>> is the final approach (DPAA2 has some fun stuff such as several
>>> different PHYs at address 0.) NXP basically didn't like the approach
>>> I had in the patches I sent to netdev, we had a call, they presented
>>> an alternative appraoch, I implemented it, then they decided my
>>> original approach was the better solution for their situation.
>>>
>>> See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=cex7
>>>
>>> specifically the patches from:
>>>
>>>     "dpaa2-mac: add 1000BASE-X/SGMII PCS support"
>>>
>>> through to:
>>>
>>>     "net: phylink: add interface to configure clause 22 PCS PHY"
>>>
>>> You may also need some of the patches further down in the net-queue
>>> branch:
>>>
>>>     "net: phylink: avoid mac_config calls"
>>>
>>> through to:
>>>
>>>     "net: phylink: rejig link state tracking"
>>
>> I've been playing with this a bit on a 5.4 kernel with some of these patches
>> backported. However, I'm running into something that my previous hacks for
>> this basically dealt with as a side effect: when phylink_start is called,
>> sfp_upstream_start gets called, an SFP module is detected,
>> phylink_connect_phy gets called, but then it hits this condition and bails
>> out, because we are using INBAND mode with 1000BaseX:
>>
>> 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
>> 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
>> 		     phy_interface_mode_is_8023z(interface))))
>> 		return -EINVAL;
> 
> I'm expecting SGMII mode to be used when there's an external PHY as
> that gives greatest flexibility (as it allows 10 and 100Mbps speeds
> as well.)  From what I remember, these blocks support SGMII, so it
> should just be a matter of adding that.

They do support SGMII, but unfortunately it's not a runtime configurable 
parameter, it's a synthesis-level parameter on the FPGA IP core so you 
have to pick one or the other for any given build. We want to be able to 
support various fiber module types as well, and my understanding is that 
at least some of those only do 1000BaseX, so that ends up being the 
standard in common that we are using.

> 
>> I guess I'm not sure how this is supposed to work when the PHY on the SFP
>> module gets detected, i.e. if there's supposed to be another code path that
>> this is supposed to go down, or this is something that just hasn't been
>> fully implemented yet?
> 
> Copper PHYs work fine - using SGMII mode everywhere so far.
> 
> The problem is, if you want to use them as 1000BASE-X, you generally
> have to ensure that the PHY is appropriately programmed for 1000BASE-X
> negotiation, and the copper side advertisement only indicates 1G
> support. Not all copper PHYs have the PHY accessible for such
> programming, and in that case it becomes an exercise of "read the
> SFP documentation before buying"!
> 
> The other complication is... there's nothing in the module EEPROM
> that really says whether they are 1000BASE-X or SGMII.
> 
> What saves us thus far is that most copper SFPs use the Marvell
> 88E1111 chip, which is I2C accessible, and we drive that using
> phylib - and the phylib Marvell driver knows how to ensure that
> the PHY is configured for SGMII mode.  I'm not sure the same is
> true with 1000BASE-X mode.
>

On Tue, Apr 28, 2020 at 05:51:58PM -0600, Robert Hancock wrote:
> On 2020-04-28 5:01 p.m., Russell King - ARM Linux admin wrote:
> > On Tue, Apr 28, 2020 at 03:59:45PM -0600, Robert Hancock wrote:
> > > On 2020-04-22 1:51 a.m., Russell King - ARM Linux admin wrote:
> > > > On Tue, Apr 21, 2020 at 07:45:47PM -0600, Robert Hancock wrote:
> > > > > Hi Andre/Russell,
> > > > > 
> > > > > Just wondering where things got to with the changes for SGMII on Xilinx
> > > > > axienet that you were discussing (below)? I am looking into our Xilinx setup
> > > > > using 1000BaseX SFP and trying to get it working "properly" with newer
> > > > > kernels. My understanding is that the requirements for 1000BaseX and SGMII
> > > > > are somewhat similar. I gathered that SGMII was working somewhat already,
> > > > > but that not all link modes had been tested. However, it appears 1000BaseX
> > > > > is not yet working in the stock kernel.
> > > > > 
> > > > > The way I had this working before with a 4.19-based kernel was basically a
> > > > > hack to phylink to allow the Xilinx PCS/PMA PHY to be configured
> > > > > sufficiently as a PHY for it to work, and mostly ignored the link status of
> > > > > the SFP PHY itself, even though we were using in-band signalling mode with
> > > > > an SFP module. That was using this patch:
> > > > > 
> > > > > https://patchwork.ozlabs.org/project/netdev/patch/1559330285-30246-5-git-send-email-hancock@sedsystems.ca/
> > > > > 
> > > > > Of course, that's basically just a hack which I suspect mostly worked by
> > > > > luck. I see that there are some helpers that were added to phylink to allow
> > > > > setting PHY advertisements and reading PHY status from clause 22 PHY
> > > > > devices, so I'm guessing that is the way to go in this case? Something like:
> > > > > 
> > > > > axienet_mac_config: if using in-band mode, use
> > > > > phylink_mii_c22_pcs_set_advertisement to configure the Xilinx PHY.
> > > > > 
> > > > > axienet_mac_pcs_get_state: use phylink_mii_c22_pcs_get_state to get the MAC
> > > > > PCS state from the Xilinx PHY
> > > > > 
> > > > > axienet_mac_an_restart: if using in-band mode, use
> > > > > phylink_mii_c22_pcs_an_restart to restart autonegotiation on Xilinx PHY
> > > > > 
> > > > > To use those c22 functions, we need to find the mdio_device that's
> > > > > referenced by the phy-handle in the device tree - I guess we can just use
> > > > > some of the guts of of_phy_find_device to do that?
> > > > 
> > > > Please see the code for DPAA2 - it's changed slightly since I sent a
> > > > copy to the netdev mailing list, and it still isn't clear whether this
> > > > is the final approach (DPAA2 has some fun stuff such as several
> > > > different PHYs at address 0.) NXP basically didn't like the approach
> > > > I had in the patches I sent to netdev, we had a call, they presented
> > > > an alternative appraoch, I implemented it, then they decided my
> > > > original approach was the better solution for their situation.
> > > > 
> > > > See http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=cex7
> > > > 
> > > > specifically the patches from:
> > > > 
> > > >     "dpaa2-mac: add 1000BASE-X/SGMII PCS support"
> > > > 
> > > > through to:
> > > > 
> > > >     "net: phylink: add interface to configure clause 22 PCS PHY"
> > > > 
> > > > You may also need some of the patches further down in the net-queue
> > > > branch:
> > > > 
> > > >     "net: phylink: avoid mac_config calls"
> > > > 
> > > > through to:
> > > > 
> > > >     "net: phylink: rejig link state tracking"
> > > 
> > > I've been playing with this a bit on a 5.4 kernel with some of these patches
> > > backported. However, I'm running into something that my previous hacks for
> > > this basically dealt with as a side effect: when phylink_start is called,
> > > sfp_upstream_start gets called, an SFP module is detected,
> > > phylink_connect_phy gets called, but then it hits this condition and bails
> > > out, because we are using INBAND mode with 1000BaseX:
> > > 
> > > 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
> > > 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
> > > 		     phy_interface_mode_is_8023z(interface))))
> > > 		return -EINVAL;
> > 
> > I'm expecting SGMII mode to be used when there's an external PHY as
> > that gives greatest flexibility (as it allows 10 and 100Mbps speeds
> > as well.)  From what I remember, these blocks support SGMII, so it
> > should just be a matter of adding that.
> 
> They do support SGMII, but unfortunately it's not a runtime configurable
> parameter, it's a synthesis-level parameter on the FPGA IP core so you have
> to pick one or the other for any given build. We want to be able to support
> various fiber module types as well, and my understanding is that at least
> some of those only do 1000BaseX, so that ends up being the standard in
> common that we are using.

1G Fibre modules are all 1000BaseX only.