[PATCH v3] net: macb: add support for high speed interface

[PATCH v3] net: macb: add support for high speed interface

[Parshuram Thombare]

This patch adds support for 10GBASE-R interface to the linux driver for
Cadence's ethernet controller.
This controller has separate MAC's and PCS'es for low and high speed paths.
High speed PCS supports 100M, 1G, 2.5G, 5G and 10G through rate adaptation
implementation. However, since it doesn't support auto negotiation, linux
driver is modified to support 10GBASE-R insted of USXGMII. 

Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
Changes between v2 and v3:
1. Replace USXGMII interface by 10GBASE-R interface.
2. Adopted new phylink pcs_ops for high speed PCS.
3. Added pcs_get_state for high speed PCS.

---
 drivers/net/ethernet/cadence/macb.h      |   44 ++++++++++++
 drivers/net/ethernet/cadence/macb_main.c |  112 +++++++++++++++++++++++++++++-
 2 files changed, 155 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 5de47f6..1f5da4e 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -77,10 +77,12 @@
 #define MACB_RBQPH		0x04D4
 
 /* GEM register offsets. */
+#define GEM_NCR			0x0000 /* Network Control */
 #define GEM_NCFGR		0x0004 /* Network Config */
 #define GEM_USRIO		0x000c /* User IO */
 #define GEM_DMACFG		0x0010 /* DMA Configuration */
 #define GEM_JML			0x0048 /* Jumbo Max Length */
+#define GEM_HS_MAC_CONFIG	0x0050 /* GEM high speed config */
 #define GEM_HRB			0x0080 /* Hash Bottom */
 #define GEM_HRT			0x0084 /* Hash Top */
 #define GEM_SA1B		0x0088 /* Specific1 Bottom */
@@ -166,6 +168,9 @@
 #define GEM_DCFG7		0x0298 /* Design Config 7 */
 #define GEM_DCFG8		0x029C /* Design Config 8 */
 #define GEM_DCFG10		0x02A4 /* Design Config 10 */
+#define GEM_DCFG12		0x02AC /* Design Config 12 */
+#define GEM_USX_CONTROL		0x0A80 /* High speed PCS control register */
+#define GEM_USX_STATUS		0x0A88 /* High speed PCS status register */
 
 #define GEM_TXBDCTRL	0x04cc /* TX Buffer Descriptor control register */
 #define GEM_RXBDCTRL	0x04d0 /* RX Buffer Descriptor control register */
@@ -272,11 +277,19 @@
 #define MACB_IRXFCS_OFFSET	19
 #define MACB_IRXFCS_SIZE	1
 
+/* GEM specific NCR bitfields. */
+#define GEM_ENABLE_HS_MAC_OFFSET	31
+#define GEM_ENABLE_HS_MAC_SIZE		1
+
 /* GEM specific NCFGR bitfields. */
+#define GEM_FD_OFFSET		1 /* Full duplex */
+#define GEM_FD_SIZE		1
 #define GEM_GBE_OFFSET		10 /* Gigabit mode enable */
 #define GEM_GBE_SIZE		1
 #define GEM_PCSSEL_OFFSET	11
 #define GEM_PCSSEL_SIZE		1
+#define GEM_PAE_OFFSET		13 /* Pause enable */
+#define GEM_PAE_SIZE		1
 #define GEM_CLK_OFFSET		18 /* MDC clock division */
 #define GEM_CLK_SIZE		3
 #define GEM_DBW_OFFSET		21 /* Data bus width */
@@ -461,11 +474,17 @@
 #define MACB_REV_OFFSET				0
 #define MACB_REV_SIZE				16
 
+/* Bitfield in HS_MAC_CONFIG */
+#define GEM_HS_MAC_SPEED_OFFSET			0
+#define GEM_HS_MAC_SPEED_SIZE			3
+
 /* Bitfields in DCFG1. */
 #define GEM_IRQCOR_OFFSET			23
 #define GEM_IRQCOR_SIZE				1
 #define GEM_DBWDEF_OFFSET			25
 #define GEM_DBWDEF_SIZE				3
+#define GEM_NO_PCS_OFFSET			0
+#define GEM_NO_PCS_SIZE				1
 
 /* Bitfields in DCFG2. */
 #define GEM_RX_PKT_BUFF_OFFSET			20
@@ -500,6 +519,28 @@
 #define GEM_RXBD_RDBUFF_OFFSET			8
 #define GEM_RXBD_RDBUFF_SIZE			4
 
+/* Bitfields in DCFG12. */
+#define GEM_HIGH_SPEED_OFFSET			26
+#define GEM_HIGH_SPEED_SIZE			1
+
+/* Bitfields in USX_CONTROL. */
+#define GEM_USX_CTRL_SPEED_OFFSET		14
+#define GEM_USX_CTRL_SPEED_SIZE			3
+#define GEM_SERDES_RATE_OFFSET			12
+#define GEM_SERDES_RATE_SIZE			2
+#define GEM_RX_SCR_BYPASS_OFFSET		9
+#define GEM_RX_SCR_BYPASS_SIZE			1
+#define GEM_TX_SCR_BYPASS_OFFSET		8
+#define GEM_TX_SCR_BYPASS_SIZE			1
+#define GEM_TX_EN_OFFSET			1
+#define GEM_TX_EN_SIZE				1
+#define GEM_SIGNAL_OK_OFFSET			0
+#define GEM_SIGNAL_OK_SIZE			1
+
+/* Bitfields in USX_STATUS. */
+#define GEM_USX_BLOCK_LOCK_OFFSET		0
+#define GEM_USX_BLOCK_LOCK_SIZE			1
+
 /* Bitfields in TISUBN */
 #define GEM_SUBNSINCR_OFFSET			0
 #define GEM_SUBNSINCRL_OFFSET			24
@@ -663,6 +704,8 @@
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
 #define MACB_CAPS_SG_DISABLED			0x40000000
 #define MACB_CAPS_MACB_IS_GEM			0x80000000
+#define MACB_CAPS_PCS				0x01000000
+#define MACB_CAPS_HIGH_SPEED			0x02000000
 
 /* LSO settings */
 #define MACB_LSO_UFO_ENABLE			0x01
@@ -1201,6 +1244,7 @@ struct macb {
 	struct mii_bus		*mii_bus;
 	struct phylink		*phylink;
 	struct phylink_config	phylink_config;
+	struct phylink_pcs	phylink_pcs;
 
 	u32			caps;
 	unsigned int		dma_burst_length;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 883e47c..8b44876 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -84,6 +84,9 @@ struct sifive_fu540_macb_mgmt {
 #define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
 #define MACB_WOL_ENABLED		(0x1 << 1)
 
+#define HS_SPEED_10000M			4
+#define MACB_SERDES_RATE_10G		1
+
 /* Graceful stop timeouts in us. We should allow up to
  * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
  */
@@ -513,6 +516,7 @@ static void macb_validate(struct phylink_config *config,
 	    state->interface != PHY_INTERFACE_MODE_RMII &&
 	    state->interface != PHY_INTERFACE_MODE_GMII &&
 	    state->interface != PHY_INTERFACE_MODE_SGMII &&
+	    state->interface != PHY_INTERFACE_MODE_10GBASER &&
 	    !phy_interface_mode_is_rgmii(state->interface)) {
 		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 		return;
@@ -525,10 +529,31 @@ static void macb_validate(struct phylink_config *config,
 		return;
 	}
 
+	if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
+	    !(bp->caps & MACB_CAPS_HIGH_SPEED &&
+	      bp->caps & MACB_CAPS_PCS)) {
+		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+		return;
+	}
+
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
 	phylink_set(mask, Asym_Pause);
 
+	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
+	    (state->interface == PHY_INTERFACE_MODE_NA ||
+	     state->interface == PHY_INTERFACE_MODE_10GBASER)) {
+		phylink_set(mask, 10000baseCR_Full);
+		phylink_set(mask, 10000baseER_Full);
+		phylink_set(mask, 10000baseKR_Full);
+		phylink_set(mask, 10000baseLR_Full);
+		phylink_set(mask, 10000baseLRM_Full);
+		phylink_set(mask, 10000baseSR_Full);
+		phylink_set(mask, 10000baseT_Full);
+		if (state->interface != PHY_INTERFACE_MODE_NA)
+			goto out;
+	}
+
 	phylink_set(mask, 10baseT_Half);
 	phylink_set(mask, 10baseT_Full);
 	phylink_set(mask, 100baseT_Half);
@@ -545,12 +570,70 @@ static void macb_validate(struct phylink_config *config,
 		if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
 			phylink_set(mask, 1000baseT_Half);
 	}
-
+out:
 	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
 	bitmap_and(state->advertising, state->advertising, mask,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
+static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+				 phy_interface_t interface, int speed,
+				 int duplex)
+{
+	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+	u32 config;
+
+	config = gem_readl(bp, USX_CONTROL);
+	config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config);
+	config &= ~GEM_BIT(TX_SCR_BYPASS);
+	config &= ~GEM_BIT(RX_SCR_BYPASS);
+	config |= GEM_BIT(TX_EN);
+	gem_writel(bp, USX_CONTROL, GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M,
+					      config));
+}
+
+static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
+				   struct phylink_link_state *state)
+{
+	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+	u32 val;
+
+	state->speed = SPEED_10000;
+	state->duplex = 1;
+	state->an_complete = 1;
+
+	val = gem_readl(bp, USX_STATUS);
+	state->link = !!(val & GEM_BIT(USX_BLOCK_LOCK));
+	val = gem_readl(bp, NCFGR);
+	if (val & GEM_BIT(PAE))
+		state->pause = MLO_PAUSE_RX;
+}
+
+static int macb_usx_pcs_config(struct phylink_pcs *pcs,
+			       unsigned int mode,
+			       phy_interface_t interface,
+			       const unsigned long *advertising,
+			       bool permit_pause_to_mac)
+{
+	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
+	u32 val;
+
+	val = gem_readl(bp, NCFGR);
+	val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
+	gem_writel(bp, NCFGR, val);
+
+	val = gem_readl(bp, USX_CONTROL);
+	gem_writel(bp, USX_CONTROL, val | GEM_BIT(SIGNAL_OK));
+
+	return 0;
+}
+
+static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
+	.pcs_get_state = macb_usx_pcs_get_state,
+	.pcs_config = macb_usx_pcs_config,
+	.pcs_link_up = macb_usx_pcs_link_up,
+};
+
 static void macb_mac_pcs_get_state(struct phylink_config *config,
 				   struct phylink_link_state *state)
 {
@@ -588,6 +671,9 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 	if (old_ctrl ^ ctrl)
 		macb_or_gem_writel(bp, NCFGR, ctrl);
 
+	if (bp->phy_interface == PHY_INTERFACE_MODE_10GBASER)
+		gem_writel(bp, NCR, gem_readl(bp, NCR) | GEM_BIT(ENABLE_HS_MAC));
+
 	spin_unlock_irqrestore(&bp->lock, flags);
 }
 
@@ -664,6 +750,10 @@ static void macb_mac_link_up(struct phylink_config *config,
 
 	macb_or_gem_writel(bp, NCFGR, ctrl);
 
+	if (bp->phy_interface == PHY_INTERFACE_MODE_10GBASER)
+		gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, HS_SPEED_10000M,
+							gem_readl(bp, HS_MAC_CONFIG)));
+
 	spin_unlock_irqrestore(&bp->lock, flags);
 
 	/* Enable Rx and Tx */
@@ -672,9 +762,24 @@ static void macb_mac_link_up(struct phylink_config *config,
 	netif_tx_wake_all_queues(ndev);
 }
 
+static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
+			    phy_interface_t interface)
+{
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct macb *bp = netdev_priv(ndev);
+
+	if (interface == PHY_INTERFACE_MODE_10GBASER) {
+		bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
+		phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
+	}
+
+	return 0;
+}
+
 static const struct phylink_mac_ops macb_phylink_ops = {
 	.validate = macb_validate,
 	.mac_pcs_get_state = macb_mac_pcs_get_state,
+	.mac_prepare = macb_mac_prepare,
 	.mac_an_restart = macb_mac_an_restart,
 	.mac_config = macb_mac_config,
 	.mac_link_down = macb_mac_link_down,
@@ -3523,6 +3628,11 @@ static void macb_configure_caps(struct macb *bp,
 		dcfg = gem_readl(bp, DCFG1);
 		if (GEM_BFEXT(IRQCOR, dcfg) == 0)
 			bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
+		if (GEM_BFEXT(NO_PCS, dcfg) == 0)
+			bp->caps |= MACB_CAPS_PCS;
+		dcfg = gem_readl(bp, DCFG12);
+		if (GEM_BFEXT(HIGH_SPEED, dcfg) == 1)
+			bp->caps |= MACB_CAPS_HIGH_SPEED;
 		dcfg = gem_readl(bp, DCFG2);
 		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
 			bp->caps |= MACB_CAPS_FIFO_MODE;
-- 
1.7.1

Re: [PATCH v3] net: macb: add support for high speed interface

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 07:44:05PM +0200, Parshuram Thombare wrote:
> This patch adds support for 10GBASE-R interface to the linux driver for
> Cadence's ethernet controller.
> This controller has separate MAC's and PCS'es for low and high speed paths.
> High speed PCS supports 100M, 1G, 2.5G, 5G and 10G through rate adaptation
> implementation. However, since it doesn't support auto negotiation, linux
> driver is modified to support 10GBASE-R insted of USXGMII. 
> 
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
> Changes between v2 and v3:
> 1. Replace USXGMII interface by 10GBASE-R interface.
> 2. Adopted new phylink pcs_ops for high speed PCS.
> 3. Added pcs_get_state for high speed PCS.

Hi,

If you're going to support pcs_ops for the 10GBASE-R mode, can you
also convert macb to use pcs_ops for the lower speed modes as well?

The reason is that when you have pcs_ops in place, there are slight
behaviour differences in the way phylink calls the MAC functions,
and in the long run I would like to eventually retire the old ways
(so we don't have to keep compatible with old in-kernel APIs alive
for ever.)

I think all that needs to happen is a pcs_ops for the non-10GBASE-R
mode which moves macb_mac_pcs_get_state() and macb_mac_an_restart()
to it, and implements a stub pcs_config(). So it should be simple
to do.

Further comments below.

> +static int macb_usx_pcs_config(struct phylink_pcs *pcs,
> +			       unsigned int mode,
> +			       phy_interface_t interface,
> +			       const unsigned long *advertising,
> +			       bool permit_pause_to_mac)
> +{
> +	struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
> +	u32 val;
> +
> +	val = gem_readl(bp, NCFGR);
> +	val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
> +	gem_writel(bp, NCFGR, val);

This looks like it's configuring the MAC rather than the PCS - it
should probably be in mac_prepare() or mac_config().

Note that the order of calls when changing the interface mode will
be:

- mac_prepare()
- mac_config()
- pcs_config()
- pcs_an_restart() optionally
- mac_finish()

> +static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
> +			    phy_interface_t interface)
> +{
> +	struct net_device *ndev = to_net_dev(config->dev);
> +	struct macb *bp = netdev_priv(ndev);
> +
> +	if (interface == PHY_INTERFACE_MODE_10GBASER) {
> +		bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
> +		phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
> +	}

What happens if phylink requests 10GBASE-R and subsequently selects
a different interface mode? You end up with the PCS still registered
and phylink will use it even when not in 10GBASE-R mode - so its
functions will also be called.

Note also that if a PCS is registered, phylink will omit to call
mac_config() unless the interface mode is being changed. If no PCS
is registered, it will call mac_config() in the old way (which
includes link up events.)

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH v3] net: macb: add support for high speed interface

[Parshuram Raju Thombare]

>If you're going to support pcs_ops for the 10GBASE-R mode, can you
>also convert macb to use pcs_ops for the lower speed modes as well?

Ok, I will add pcs_ops for low speed as well. In fact, having single
pcs_ops and using check for interface type within each method of
pcs_ops to decide appropriate action will also solve the below mentioned
issue when phylink requests 10GBASE-R and subsequently selects a
different interface mode.

Since macb_mac_an_restart  is empty, macb_mac_pcs_get_state only
sets "state->link = 0" and there is nothing to be done in pcs_config, there will
be no changes in pcs_ops methods pcs_get_state, pcs_config and pcs_link_up,
except guarding current code in these methods by
interface == PHY_INTERFACE_MODE_10GBASER check.

pcs_config and pcs_link_up passes "interface" as an argument, and in
pcs_get_state call "state->interface" appeared to be populated just before
calling it and hence should be valid.

 528         state->interface = pl->link_config.interface;
 ...
 535
 536         if (pl->pcs_ops)
 537                 pl->pcs_ops->pcs_get_state(pl->pcs, state);
 538         else if (pl->mac_ops->mac_pcs_get_state)
 539                 pl->mac_ops->mac_pcs_get_state(pl->config, state);
 540         else
 541                 state->link = 0;
 
>> +	val = gem_readl(bp, NCFGR);
>> +	val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
>> +	gem_writel(bp, NCFGR, val);
>
>This looks like it's configuring the MAC rather than the PCS - it
>should probably be in mac_prepare() or mac_config().

Ok

>What happens if phylink requests 10GBASE-R and subsequently selects
>a different interface mode? You end up with the PCS still registered
>and phylink will use it even when not in 10GBASE-R mode - so its
>functions will also be called.

RE: [PATCH v3] net: macb: add support for high speed interface

[Parshuram Raju Thombare]

Hi,

I was trying to find out any ethernet driver where this issue of selecting appropriate
pcs_ops due to phylink changing interface mode dynamically is handled. 
But, apparently, so far only mvpp2 has adapted pcs_ops. And even in mvpp2, it is
not obvious how this case is handled. 

Also, apart from interface mode changed due to SFPs with different types of PHY
being used, it is not clear when phylink selects interface mode different than it
initially requested to the ethernet driver.

>pcs_config and pcs_link_up passes "interface" as an argument, and in
>pcs_get_state call "state->interface" appeared to be populated just before
>calling it and hence should be valid.

It seems state->interface in pcs_get_state is not always valid when SFPs with
different types of PHY are used.  
There is a chance of SFP with different type of PHY is inserted, eventually invoking
phylink_resolve for interface mode different than phylink initially requested,
and causing major reconfiguration.

However, pcs_get_state is called before major reconfiguration, where selecting
which pcs_ops and PCS to be used is difficult without correct interface mode.  

As struct phylink and hence phy_state is private to phylink layer, IMO this need to be
handled at phylink level by passing appropriate interface mode to all necessary
methods registered by drivers.

Something like

523 static void phylink_mac_pcs_get_state(struct phylink *pl,
 524                                       struct phylink_link_state *state)
 525 {
 526         linkmode_copy(state->advertising, pl->link_config.advertising);
 527         linkmode_zero(state->lp_advertising);
 528         if (pl->phydev)
 529                 state->interface = pl->phy_state.interface;
 530         else
 531                 state->interface = pl->link_config.interface;
 532         state->an_enabled = pl->link_config.an_enabled;
 533         state->speed = SPEED_UNKNOWN;
 534         state->duplex = DUPLEX_UNKNOWN;
 535         state->pause = MLO_PAUSE_NONE;
 536         state->an_complete = 0;
 537         state->link = 1;
 538
 539         if (pl->pcs_ops)
 540                 pl->pcs_ops->pcs_get_state(pl->pcs, state);
 541         else
 542                 pl->mac_ops->mac_pcs_get_state(pl->config, state);


Regards,
Parshuram Thombare

Re: [PATCH v3] net: macb: add support for high speed interface

[Russell King - ARM Linux admin]

On Fri, Oct 23, 2020 at 10:59:42AM +0000, Parshuram Raju Thombare wrote:
> Hi,
> 
> I was trying to find out any ethernet driver where this issue of selecting appropriate
> pcs_ops due to phylink changing interface mode dynamically is handled. 
> But, apparently, so far only mvpp2 has adapted pcs_ops. And even in mvpp2, it is
> not obvious how this case is handled. 

Yes, mvpp2 is the only driver that has been converted.  I'm not sure
why you say that it's not obvious how it's handled.

Whenever the interface changes, we go through the full reconfiguration
procedure that I've already outlined. This involves calling the
mac_prepare() method which calls into mvpp2_mac_prepare() and its
child mvpp2__mac_prepare().

In mvpp2__mac_prepare(), we switch the "ops" for port->phylink_pcs,
and then back in mvpp2_mac_prepare(), we ensure that the PCS instance
is set to the port->phylink_pcs (which internally updates the
pl->pcs_ops pointer in phylink.)

That results in phylink switching between the XLG and GMAC PCS
operations depending on the interface in use.

> Also, apart from interface mode changed due to SFPs with different types of PHY
> being used, it is not clear when phylink selects interface mode different than it
> initially requested to the ethernet driver.

The ethernet driver's initial interface mode has no real bearing when a
SFP is inserted. A SFP or SFP+ module can require one of several
different interface modes, and generally can not be programmed to
operate in any other mode.

For example, a 1G fiber SFP can _only_ operate in 1000BASE-X. SGMII and
10G modes are not permissible. Trying to use RGMII (for example) is a
nonsense because there aren't enough pins on the module to connect
RGMII, and the module would not know what to do with it.

If the module is a 10M/100M/1G copper SFP, then things get more fun -
and what can be done depends whether the SFP has an accessible PHY we
can drive. The PHY may support 1000BASE-X and/or SGMII, and there is
no real way to determine which the SFP supports. The EEPROM does _not_
help with this. We work around that at present by always using SGMII
(there are copper modules that the PHY is inaccessible but is in SGMII
mode - Mikrotik S-RJ01 for example). For others where the PHY is
accessible, it is generally an 88E1111, which may power up in either
SGMII or 1000BASE-X mode. We always select SGMII for these, and the
88E1111 driver knows how to reprogram the PHY to operate in this mode.

If it's a SFP+ module, then similar games occur. If it's a fiber module,
then 10GBASE-R needs to be selected, since that's the protocol that is
defined to be used over 10G fiber connections. Otherwise, again, it's
up to the PHY - and the PHY can be one that switches between 10GBASE-R,
2500BASE-X, and SGMII depending on the speed that was negotiated on its
copper side.

There is nothing simple here - but as far as the MAC driver is
concerned, phylink will ask the MAC driver to reconfigure itself for
the interface mode as appropriate.

> >pcs_config and pcs_link_up passes "interface" as an argument, and in
> >pcs_get_state call "state->interface" appeared to be populated just before
> >calling it and hence should be valid.
> 
> It seems state->interface in pcs_get_state is not always valid when SFPs with
> different types of PHY are used.
> 
> There is a chance of SFP with different type of PHY is inserted,
> eventually invoking phylink_resolve for interface mode different than
> phylink initially requested, and causing major reconfiguration.
> 
> However, pcs_get_state is called before major reconfiguration, where selecting
> which pcs_ops and PCS to be used is difficult without correct interface mode.  

Correct - state->interface will always be the currently configured
interface mode, because that's the one that the MAC hardware is
configured for. Other PCS interfaces may be powered down and/or
disconnected from the serdes lane.

However, the interface will not change at the point you are referring
to when in in-band mode (there is no support for dynamic changes of
interface in that circumstance.) The change of interface happens when
a SFP module is being brought up either via the phylink_sfp_config*()
functions, or if there is a PHY, via the phylib driver propagating its
operating interface mode back through phylink (but in this case, we
will not be in in-band mode, and pcs_get_state() will not be called.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH v3] net: macb: add support for high speed interface

[Parshuram Raju Thombare]

>Whenever the interface changes, we go through the full reconfiguration
>procedure that I've already outlined. This involves calling the
>mac_prepare() method which calls into mvpp2_mac_prepare() and its
>child mvpp2__mac_prepare().

Ok, I misunderstood it as interface mode change between successive mac_prepare().
If major reconfiguration is certain to happen after every interface mode change,
I will make another small modification in mac_prepare method to set appropriate
pcs_ops for selected interface mode. 
pcs_ops for low speed, however, will just be existing non 10GBASE-R functions renamed.
This will allow us to get rid of old API's for non 10GBASE-R PCS. I hope you are ok with
these changes done in the same patch.

Re: [PATCH v3] net: macb: add support for high speed interface

[Russell King - ARM Linux admin]

On Fri, Oct 23, 2020 at 01:34:09PM +0000, Parshuram Raju Thombare wrote:
> >Whenever the interface changes, we go through the full reconfiguration
> >procedure that I've already outlined. This involves calling the
> >mac_prepare() method which calls into mvpp2_mac_prepare() and its
> >child mvpp2__mac_prepare().
> 
> Ok, I misunderstood it as interface mode change between successive mac_prepare().
> If major reconfiguration is certain to happen after every interface mode change,
> I will make another small modification in mac_prepare method to set appropriate
> pcs_ops for selected interface mode. 
> pcs_ops for low speed, however, will just be existing non 10GBASE-R functions renamed.
> This will allow us to get rid of old API's for non 10GBASE-R PCS. I hope you are ok with
> these changes done in the same patch.

Yes, that sounds good.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!