2020-10-22 06:31:44

by Parshuram Raju Thombare

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

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 <[email protected]>
---
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


2020-10-22 06:35:52

by Russell King (Oracle)

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

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 <[email protected]>
> ---
> 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!

2020-10-22 16:22:12

by Parshuram Raju Thombare

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

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

2020-10-23 11:01:23

by Parshuram Raju Thombare

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

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

2020-10-23 11:30:29

by Russell King (Oracle)

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

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!

2020-10-23 18:16:19

by Parshuram Raju Thombare

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

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

2020-10-23 18:35:52

by Russell King (Oracle)

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

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!