[PATCH net-next 0/2] macb SGMII fixed-link fixes

Some fixes to the macb driver for use in SGMII mode with a fixed-link (such as
for chip-to-chip connectivity).

Robert Hancock (2):
  net: macb: poll for fixed link state in SGMII mode
  net: macb: Disable PCS auto-negotiation for SGMII fixed-link mode

 drivers/net/ethernet/cadence/macb.h      | 14 +++++++++++
 drivers/net/ethernet/cadence/macb_main.c | 30 ++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

-- 
2.27.0

When using a fixed-link configuration in SGMII mode, it's not really
sensible to have auto-negotiation enabled since the link settings are
fixed by definition. In other configurations, such as an SGMII
connection to a PHY, it should generally be enabled.

Signed-off-by: Robert Hancock <redacted>
---
 drivers/net/ethernet/cadence/macb.h      | 14 ++++++++++++++
 drivers/net/ethernet/cadence/macb_main.c | 16 ++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index d8c68906525a..d8d87213697c 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h

@@ -159,6 +159,16 @@
 #define GEM_PEFTN		0x01f4 /* PTP Peer Event Frame Tx Ns */
 #define GEM_PEFRSL		0x01f8 /* PTP Peer Event Frame Rx Sec Low */
 #define GEM_PEFRN		0x01fc /* PTP Peer Event Frame Rx Ns */
+#define GEM_PCSCNTRL		0x0200 /* PCS Control */
+#define GEM_PCSSTS		0x0204 /* PCS Status */
+#define GEM_PCSPHYTOPID		0x0208 /* PCS PHY Top ID */
+#define GEM_PCSPHYBOTID		0x020c /* PCS PHY Bottom ID */
+#define GEM_PCSANADV		0x0210 /* PCS AN Advertisement */
+#define GEM_PCSANLPBASE		0x0214 /* PCS AN Link Partner Base */
+#define GEM_PCSANEXP		0x0218 /* PCS AN Expansion */
+#define GEM_PCSANNPTX		0x021c /* PCS AN Next Page TX */
+#define GEM_PCSANNPLP		0x0220 /* PCS AN Next Page LP */
+#define GEM_PCSANEXTSTS		0x023c /* PCS AN Extended Status */
 #define GEM_DCFG1		0x0280 /* Design Config 1 */
 #define GEM_DCFG2		0x0284 /* Design Config 2 */
 #define GEM_DCFG3		0x0288 /* Design Config 3 */

@@ -478,6 +488,10 @@
 #define GEM_HS_MAC_SPEED_OFFSET			0
 #define GEM_HS_MAC_SPEED_SIZE			3
 
+/* Bitfields in PCSCNTRL */
+#define GEM_PCSAUTONEG_OFFSET			12
+#define GEM_PCSAUTONEG_SIZE			1
+
 /* Bitfields in DCFG1. */
 #define GEM_IRQCOR_OFFSET			23
 #define GEM_IRQCOR_SIZE				1

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ca72a16c8da3..e7c123aadf56 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -694,6 +694,22 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 	if (old_ncr ^ ncr)
 		macb_or_gem_writel(bp, NCR, ncr);
 
+	/* Disable AN for SGMII fixed link configuration, enable otherwise.
+	 * Must be written after PCSSEL is set in NCFGR,
+	 * otherwise writes will not take effect.
+	 */
+	if (macb_is_gem(bp) && state->interface == PHY_INTERFACE_MODE_SGMII) {
+		u32 pcsctrl, old_pcsctrl;
+
+		old_pcsctrl = gem_readl(bp, PCSCNTRL);
+		if (mode == MLO_AN_FIXED)
+			pcsctrl = old_pcsctrl & ~GEM_BIT(PCSAUTONEG);
+		else
+			pcsctrl = old_pcsctrl | GEM_BIT(PCSAUTONEG);
+		if (old_pcsctrl != pcsctrl)
+			gem_writel(bp, PCSCNTRL, pcsctrl);
+	}
+
 	spin_unlock_irqrestore(&bp->lock, flags);
 }
 

-- 
2.27.0

When using a fixed-link configuration with GEM in SGMII mode, such as
for a chip-to-chip interconnect, the link state was always showing as
established regardless of the actual connectivity state. We can monitor
the pcs_link_state bit in the Network Status register to determine
whether the PCS link state is actually up.

Signed-off-by: Robert Hancock <redacted>
---
 drivers/net/ethernet/cadence/macb_main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 15362d016a87..ca72a16c8da3 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -847,6 +847,15 @@ static int macb_phylink_connect(struct macb *bp)
 	return 0;
 }
 
+static void macb_get_pcs_fixed_state(struct phylink_config *config,
+				     struct phylink_link_state *state)
+{
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct macb *bp = netdev_priv(ndev);
+
+	state->link = (macb_readl(bp, NSR) & MACB_BIT(NSR_LINK)) != 0;
+}
+
 /* based on au1000_eth. c*/
 static int macb_mii_probe(struct net_device *dev)
 {

@@ -855,6 +864,11 @@ static int macb_mii_probe(struct net_device *dev)
 	bp->phylink_config.dev = &dev->dev;
 	bp->phylink_config.type = PHYLINK_NETDEV;
 
+	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
+		bp->phylink_config.poll_fixed_state = true;
+		bp->phylink_config.get_fixed_state = macb_get_pcs_fixed_state;
+	}
+
 	bp->phylink = phylink_create(&bp->phylink_config, bp->pdev->dev.fwnode,
 				     bp->phy_interface, &macb_phylink_ops);
 	if (IS_ERR(bp->phylink)) {

-- 
2.27.0

On Thu, Mar 11, 2021 at 02:18:13PM -0600, Robert Hancock wrote:

When using a fixed-link configuration in SGMII mode, it's not really
sensible to have auto-negotiation enabled since the link settings are
fixed by definition. In other configurations, such as an SGMII
connection to a PHY, it should generally be enabled.

So how do you tell the PCS it should be doing 10Mbps over the SGMII
link? I'm assuming it is the PCS which does the bit replication, not
the MAC?

I'm surprised you are even using SGMII with a fixed link. 1000BaseX is
the norm, and then you don't need to worry about the speed.

    Andrew

On Sat, 2021-03-13 at 02:45 +0100, Andrew Lunn wrote:

On Thu, Mar 11, 2021 at 02:18:13PM -0600, Robert Hancock wrote:

quoted

When using a fixed-link configuration in SGMII mode, it's not really
sensible to have auto-negotiation enabled since the link settings are
fixed by definition. In other configurations, such as an SGMII
connection to a PHY, it should generally be enabled.

So how do you tell the PCS it should be doing 10Mbps over the SGMII
link? I'm assuming it is the PCS which does the bit replication, not
the MAC?

I'm not sure if this is the same for all devices using this Cadence IP, but the
register documentation I have for the Xilinx UltraScale+ MPSoC we are using
indicates this PCS is only capable of 1000 Mbps speeds:

https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_control.html

So it doesn't actually seem applicable in this case.

I'm surprised you are even using SGMII with a fixed link. 1000BaseX is
the norm, and then you don't need to worry about the speed.

That would be a bit simpler, yes - but it seems like this hardware is set up
more for SGMII mode - it's not entirely clear to me that 1000BaseX is supported
in the hardware, and it's not currently supported in the driver that I can see.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

On Sun, Mar 14, 2021 at 11:22:03PM +0000, Robert Hancock wrote:

On Sat, 2021-03-13 at 02:45 +0100, Andrew Lunn wrote:

quoted

On Thu, Mar 11, 2021 at 02:18:13PM -0600, Robert Hancock wrote:

quoted

When using a fixed-link configuration in SGMII mode, it's not really
sensible to have auto-negotiation enabled since the link settings are
fixed by definition. In other configurations, such as an SGMII
connection to a PHY, it should generally be enabled.

So how do you tell the PCS it should be doing 10Mbps over the SGMII
link? I'm assuming it is the PCS which does the bit replication, not
the MAC?

I'm not sure if this is the same for all devices using this Cadence IP, but the
register documentation I have for the Xilinx UltraScale+ MPSoC we are using
indicates this PCS is only capable of 1000 Mbps speeds:

https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_control.html

So it doesn't actually seem applicable in this case.

quoted

I'm surprised you are even using SGMII with a fixed link. 1000BaseX is
the norm, and then you don't need to worry about the speed.

That would be a bit simpler, yes - but it seems like this hardware is set up
more for SGMII mode - it's not entirely clear to me that 1000BaseX is supported
in the hardware, and it's not currently supported in the driver that I can see.

This hardware just seems odd. If it was not for the fact the
documentation say SGMII all over the place, i would be temped to say
it is actually doing 1000BaseX.

Assuming the documentation is not totally wrong, your code seems
sensible for the hardware.

	Andrew