diff mbox series

[3/3] net: macb: add support for high speed interface

Message ID 1575890176-25630-1-git-send-email-mparab@cadence.com
State Not Applicable
Delegated to: David Miller
Headers show
Series net: macb: fix for fixed link, support for c45 mdio and 10G | expand

Commit Message

Milind Parab Dec. 9, 2019, 11:16 a.m. UTC 
This patch add support for high speed USXGMII PCS and 10G
speed in Cadence ethernet controller driver.

Signed-off-by: Milind Parab <mparab@cadence.com>
---
 drivers/net/ethernet/cadence/macb.h      |  50 ++++++++
 drivers/net/ethernet/cadence/macb_main.c | 142 ++++++++++++++++++++---
 2 files changed, 174 insertions(+), 18 deletions(-)

Comments

Russell King (Oracle) Dec. 9, 2019, 11:36 a.m. UTC | #1 
On Mon, Dec 09, 2019 at 11:16:16AM +0000, Milind Parab wrote:
> This patch add support for high speed USXGMII PCS and 10G
> speed in Cadence ethernet controller driver.

How has this been tested?

> Signed-off-by: Milind Parab <mparab@cadence.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      |  50 ++++++++
>  drivers/net/ethernet/cadence/macb_main.c | 142 ++++++++++++++++++++---
>  2 files changed, 174 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index dbf7070fcdba..b731807d1c49 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -76,10 +76,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 */
> @@ -164,6 +166,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 /* USXGMII control register */
> +#define GEM_USX_STATUS		0x0A88 /* USXGMII status register */
>  
>  #define GEM_TXBDCTRL	0x04cc /* TX Buffer Descriptor control register */
>  #define GEM_RXBDCTRL	0x04d0 /* RX Buffer Descriptor control register */
> @@ -270,11 +275,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 */
> @@ -455,11 +468,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
> @@ -494,6 +513,34 @@
>  #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_RX_SYNC_RESET_OFFSET		2
> +#define GEM_RX_SYNC_RESET_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_TX_FAULT_OFFSET			28
> +#define GEM_USX_TX_FAULT_SIZE			1
> +#define GEM_USX_RX_FAULT_OFFSET			27
> +#define GEM_USX_RX_FAULT_SIZE			1
> +#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
> @@ -656,6 +703,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
> @@ -724,6 +773,7 @@
>  	})
>  
>  #define MACB_READ_NSR(bp)	macb_readl(bp, NSR)
> +#define GEM_READ_USX_STATUS(bp)	gem_readl(bp, USX_STATUS)
>  
>  /* struct macb_dma_desc - Hardware DMA descriptor
>   * @addr: DMA address of data buffer
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 17297b01e85e..710ee18a0ef0 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -81,6 +81,18 @@ struct sifive_fu540_macb_mgmt {
>  #define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
>  #define MACB_WOL_ENABLED		(0x1 << 1)
>  
> +enum {
> +	HS_MAC_SPEED_100M,
> +	HS_MAC_SPEED_1000M,
> +	HS_MAC_SPEED_2500M,
> +	HS_MAC_SPEED_5000M,
> +	HS_MAC_SPEED_10000M,

Are these chip register definitions?  Shouldn't you be relying on fixed
values for these, rather than their position in an enumerated list?

> +};
> +
> +enum {
> +	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)
>   */
> @@ -90,6 +102,8 @@ struct sifive_fu540_macb_mgmt {
>  
>  #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
>  
> +#define MACB_USX_BLOCK_LOCK_TIMEOUT	1000000 /* in usecs */
> +
>  /* DMA buffer descriptor might be different size
>   * depends on hardware configuration:
>   *
> @@ -506,6 +520,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_USXGMII &&
>  	    !phy_interface_mode_is_rgmii(state->interface)) {
>  		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>  		return;
> @@ -518,6 +533,13 @@ static void macb_validate(struct phylink_config *config,
>  		return;
>  	}
>  
> +	if (state->interface == PHY_INTERFACE_MODE_USXGMII &&
> +	    !(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);
> @@ -527,6 +549,22 @@ static void macb_validate(struct phylink_config *config,
>  	phylink_set(mask, 100baseT_Half);
>  	phylink_set(mask, 100baseT_Full);
>  
> +	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
> +	    (state->interface == PHY_INTERFACE_MODE_NA ||
> +	     state->interface == PHY_INTERFACE_MODE_USXGMII)) {
> +		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);
> +		phylink_set(mask, 5000baseT_Full);
> +		phylink_set(mask, 2500baseX_Full);
> +		phylink_set(mask, 1000baseX_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +	}
> +
>  	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
>  	    (state->interface == PHY_INTERFACE_MODE_NA ||
>  	     state->interface == PHY_INTERFACE_MODE_GMII ||
> @@ -544,6 +582,60 @@ static void macb_validate(struct phylink_config *config,
>  		   __ETHTOOL_LINK_MODE_MASK_NBITS);
>  }
>  
> +static int gem_mac_usx_configure(struct macb *bp,
> +				 const struct phylink_link_state *state)
> +{
> +	u32 speed, config, val;
> +	int ret;
> +
> +	val = gem_readl(bp, NCFGR);
> +	val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
> +	if (state->pause & MLO_PAUSE_TX)
> +		val |= GEM_BIT(PAE);
> +	gem_writel(bp, NCFGR, val);
> +	gem_writel(bp, NCR, gem_readl(bp, NCR) | GEM_BIT(ENABLE_HS_MAC));
> +	gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) | GEM_BIT(FD));
> +	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);
> +	gem_writel(bp, USX_CONTROL, config | GEM_BIT(TX_EN));
> +	config = gem_readl(bp, USX_CONTROL);
> +	gem_writel(bp, USX_CONTROL, config | GEM_BIT(SIGNAL_OK));
> +	ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
> +				 val & GEM_BIT(USX_BLOCK_LOCK),
> +				 1, MACB_USX_BLOCK_LOCK_TIMEOUT);

What if there's no signal to lock on to?  That's treated as link down
and is not a failure.

> +	if (ret < 0) {
> +		netdev_warn(bp->dev, "USXGMII block lock failed");
> +		return -ETIMEDOUT;
> +	}
> +
> +	switch (state->speed) {
> +	case SPEED_10000:
> +		speed = HS_MAC_SPEED_10000M;
> +		break;
> +	case SPEED_5000:
> +		speed = HS_MAC_SPEED_5000M;
> +		break;
> +	case SPEED_2500:
> +		speed = HS_MAC_SPEED_2500M;
> +		break;
> +	case SPEED_1000:
> +		speed = HS_MAC_SPEED_1000M;
> +		break;
> +	default:
> +	case SPEED_100:
> +		speed = HS_MAC_SPEED_100M;
> +		break;
> +	}

So you only support fixed-mode (phy and fixed links) and not in-band
links here.

> +
> +	gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, speed,
> +						gem_readl(bp, HS_MAC_CONFIG)));
> +	gem_writel(bp, USX_CONTROL, GEM_BFINS(USX_CTRL_SPEED, speed,
> +					      gem_readl(bp, USX_CONTROL)));
> +	return 0;
> +}
> +
>  static void macb_mac_pcs_get_state(struct phylink_config *config,
>  				   struct phylink_link_state *state)
>  {
> @@ -565,30 +657,39 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
>  
>  	spin_lock_irqsave(&bp->lock, flags);
>  
> -	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
> +		if (gem_mac_usx_configure(bp, state) < 0) {
> +			spin_unlock_irqrestore(&bp->lock, flags);
> +			phylink_mac_change(bp->phylink, false);
> +			return;
> +		}
> +	} else {
> +		old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
>  
> -	/* Clear all the bits we might set later */
> -	ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) | MACB_BIT(FD) | MACB_BIT(PAE) |
> -		  GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
> +		/* Clear all the bits we might set later */
> +		ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) |
> +			  MACB_BIT(FD) | MACB_BIT(PAE) |
> +			  GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
>  
> -	if (state->speed == SPEED_1000)
> -		ctrl |= GEM_BIT(GBE);
> -	else if (state->speed == SPEED_100)
> -		ctrl |= MACB_BIT(SPD);
> +		if (state->speed == SPEED_1000)
> +			ctrl |= GEM_BIT(GBE);
> +		else if (state->speed == SPEED_100)
> +			ctrl |= MACB_BIT(SPD);
>  
> -	if (state->duplex)
> -		ctrl |= MACB_BIT(FD);
> +		if (state->duplex)
> +			ctrl |= MACB_BIT(FD);
>  
> -	/* We do not support MLO_PAUSE_RX yet */
> -	if (state->pause & MLO_PAUSE_TX)
> -		ctrl |= MACB_BIT(PAE);
> +		/* We do not support MLO_PAUSE_RX yet */
> +		if (state->pause & MLO_PAUSE_TX)
> +			ctrl |= MACB_BIT(PAE);
>  
> -	if (state->interface == PHY_INTERFACE_MODE_SGMII)
> -		ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
> +		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> +			ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>  
> -	/* Apply the new configuration, if any */
> -	if (old_ctrl ^ ctrl)
> -		macb_or_gem_writel(bp, NCFGR, ctrl);
> +		/* Apply the new configuration, if any */
> +		if (old_ctrl ^ ctrl)
> +			macb_or_gem_writel(bp, NCFGR, ctrl);
> +	}
>  
>  	bp->speed = state->speed;
>  
> @@ -3399,6 +3500,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;
> -- 
> 2.17.1
> 
>
Milind Parab Dec. 10, 2019, 11:20 a.m. UTC | #2 
>> This patch add support for high speed USXGMII PCS and 10G
>> speed in Cadence ethernet controller driver.
>
>How has this been tested?
>

This patch is tested in 10G fixed mode on SFP+ module. 

In our own lab, we have various hardware test platforms supporting SGMII (through a TI PHY and another build that connects to a Marvell 1G PHY), GMII (through a Marvell PHY), 10GBASE-R (direct connection to SFP+), USXGMII (currently we can emulate this using an SFP+ connection from/to our own hardware)

>> @@ -81,6 +81,18 @@ struct sifive_fu540_macb_mgmt {
>>  #define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
>>  #define MACB_WOL_ENABLED		(0x1 << 1)
>>
>> +enum {
>> +	HS_MAC_SPEED_100M,
>> +	HS_MAC_SPEED_1000M,
>> +	HS_MAC_SPEED_2500M,
>> +	HS_MAC_SPEED_5000M,
>> +	HS_MAC_SPEED_10000M,
>
>Are these chip register definitions?  Shouldn't you be relying on fixed
>values for these, rather than their position in an enumerated list?
>
Okay, yes it is safe to use fixed values instead of enum. I will change this.

>
>> +	gem_writel(bp, USX_CONTROL, config | GEM_BIT(TX_EN));
>> +	config = gem_readl(bp, USX_CONTROL);
>> +	gem_writel(bp, USX_CONTROL, config | GEM_BIT(SIGNAL_OK));
>> +	ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
>> +				 val & GEM_BIT(USX_BLOCK_LOCK),
>> +				 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
>
>What if there's no signal to lock on to?  That's treated as link down
>and is not a failure.
>
Yes, if there is no signal to lock on, that is treated as link down.
Is this okay or should there be some additional error handling needed?

>> +
>> +	switch (state->speed) {
>> +	case SPEED_10000:
>> +		speed = HS_MAC_SPEED_10000M;
>> +		break;
>> +	case SPEED_5000:
>> +		speed = HS_MAC_SPEED_5000M;
>> +		break;
>> +	case SPEED_2500:
>> +		speed = HS_MAC_SPEED_2500M;
>> +		break;
>> +	case SPEED_1000:
>> +		speed = HS_MAC_SPEED_1000M;
>> +		break;
>> +	default:
>> +	case SPEED_100:
>> +		speed = HS_MAC_SPEED_100M;
>> +		break;
>> +	}

>So you only support fixed-mode (phy and fixed links) and not in-band
>links here.
>
Yes, this is right.
Andrew Lunn Dec. 10, 2019, 1:33 p.m. UTC | #3 
> >How has this been tested?
> >
> 
> This patch is tested in 10G fixed mode on SFP+ module. 
> 
> In our own lab, we have various hardware test platforms supporting SGMII (through a TI PHY and another build that connects to a Marvell 1G PHY), GMII (through a Marvell PHY), 10GBASE-R (direct connection to SFP+), USXGMII (currently we can emulate this using an SFP+ connection from/to our own hardware)

Are any of these PHY using C45?

    Thanks
	Andrew
Milind Parab Dec. 11, 2019, 9:20 a.m. UTC | #4 
>> >How has this been tested?
>> >
>>
>> This patch is tested in 10G fixed mode on SFP+ module.
>>
>> In our own lab, we have various hardware test platforms supporting SGMII
>(through a TI PHY and another build that connects to a Marvell 1G PHY), GMII
>(through a Marvell PHY), 10GBASE-R (direct connection to SFP+), USXGMII
>(currently we can emulate this using an SFP+ connection from/to our own
>hardware)
>
>Are any of these PHY using C45?

No, none of these PHYs use C45. 
For C45 testing we had a simulated PHY. The simulated PHY implemented a Clause 45 slave interface.

>
>    Thanks
>	Andrew
Russell King (Oracle) Dec. 11, 2019, 9:37 a.m. UTC | #5 
[private email content deleted, added Cc list back since this is
important.]

I'm still not getting a good enough view of what you are doing and
how my understanding of your hardware fits with what you're doing
with the software.

My understanding is it's something like:

	----+
        SOC |             PCS
	MAC --(USXGMII)-- PHY ----- PHY or SFP
	    |
	----+

And you are just modelling the MAC part in phylink, where as phylink
has so far been used on systems which have this model - where phylink
knows about both the MAC and the PCS PHY:

	---------------+
	         PCS   |
	MAC ---- PHY ----- PHY or SFP
	     SOC       |
      	---------------+

This is why I recently renamed mac_link_state() to mac_pcs_get_state()
to make it clearer that it reads from the PCS not from the current
settings of the MAC.  So far, all such setups do not implement the PCS
PHY as an 802.3 register set; they implement it as part of the MAC
register set.

In the former case, if phylink is used to manage the connection between
the MAC and the PCS PHY, phylink has nothing to do with the SFP at all.

In the latter case, phylink is used to manage the connection between the
PCS PHY and external device, controlling the MAC as appropriate.

My problem is I believe your hardware is the former case, but you are
trying to implement the latter case by ignoring in-band mode.  As SFPs
rely on in-band mode, that isn't going to work.

The options for the former case are:

1) implement phylink covering both the MAC and the external PCS PHY
2) implement phylink just for the MAC to PCS PHY connection but not
   SFPs, and implement SFP support separately in the PCS PHY driver.

Maybe phylink needs to split mac_pcs_get_state() so it can be supplied
by a separate driver, or by the MAC driver as appropriate - but that
brings with it other problems; phylink with a directly attached SFP
considers the state of the link between the PCS PHY and the external
device - not only speed but also interface mode for that part of the
link.  What you'd see in the mac_config() callback are interface modes
for that part of the link, not between the MAC and the PCS PHY.

To change that would require reworking almost every driver that has
already converted over to somehow remodel the built-in PCS and
COMPHY as a separate PCS PHY for phylink. I'm not entirely clear
whether that would work though.
Milind Parab Dec. 11, 2019, 11:55 a.m. UTC | #6 
>I'm still not getting a good enough view of what you are doing and
>how my understanding of your hardware fits with what you're doing
>with the software.
>
>My understanding is it's something like:
>
>	----+
>        SOC |             PCS
>	MAC --(USXGMII)-- PHY ----- PHY or SFP
>	    |
>	----+
>
>And you are just modelling the MAC part in phylink, where as phylink
>has so far been used on systems which have this model - where phylink
>knows about both the MAC and the PCS PHY:
>
>	---------------+
>	         PCS   |
>	MAC ---- PHY ----- PHY or SFP
>	     SOC       |
>      	---------------+
>

Here, I am describing the GEM component followed by the test setup. 
Please see, if the below explanation is depicting the correct picture

The Cadence MAC, referred to as GEM is a hardware module which implements 10/100/1000Mbps Ethernet MAC 
with the following interface types: 
MII, RMII, GMII, RGMII which can operate in either half or full duplex mode; 
and also as a separate instance a full duplex 10G MAC with an XGMII interface.
GEM comprises the following constituent components:
- MAC controlling transmit, receive, address checking and loopback
- Configuration registers (REG_TOP) providing control and status registers, statistics registers and synchronization logic
- Two Physical Coding Sublayer (PCS) components; one comprising 8B/10B encode/decode, PCS transmit, PCS
receive, and PCS auto-negotiation and another implementing USXGMII functionality.

As our ethernet controller (GEM) have MAC as well as USXGMII PCS, we program both appropriately based on the values passed from Phylink. 
And for fixed-link,  Phylink correctly read out these values from device tree node and relay it to mac_config.
Also note that we are not setting "sfp" node in device tree.

We are modelling MAC + USXGMII PCS. 
The test configuration is as below

                                    +-------------------------+
                                    |              GEM        |
Host PC1   < ------------------->   | MAC ---- USXGMII PCS    |----- SerDes ------ SFP+ <------ Direct attach cable ------->  Chelsio 10G Card   <--->   Host PC2
                                    |        PCI based NIC    | 
                                    +-------------------------+


The setup has a 10G fixed link between PCS and SFP+.
Our test setup is emulated on Xilinx Virtex VCU118 FPGA base board placed in a PCIe slot of a PC running Ubuntu. 
The FPGA base board contained an image implementing a NIC using Cadence PCIe and Ethernet IP cores. 
We had a separate PC also running Ubuntu containing  a Chelsio 10G NIC. 
We connected these with an SFP+ direct attach passive twinx-ax copper cable. Testing of the link was done with ping and iperf3
Also note that there is no PHY in the SFP+ cage. 
As I understand it everything regarding SFP+ in our setup is passive so there is nothing for PHYLINK to talk to. 
We do not have a module in the SFP+ cage, just a direct connection to a copper cable

>This is why I recently renamed mac_link_state() to mac_pcs_get_state()
>to make it clearer that it reads from the PCS not from the current
>settings of the MAC.  So far, all such setups do not implement the PCS
>PHY as an 802.3 register set; they implement it as part of the MAC
>register set.
>
>In the former case, if phylink is used to manage the connection between
>the MAC and the PCS PHY, phylink has nothing to do with the SFP at all.
>
>In the latter case, phylink is used to manage the connection between the
>PCS PHY and external device, controlling the MAC as appropriate.
>
>My problem is I believe your hardware is the former case, but you are
>trying to implement the latter case by ignoring in-band mode.  As SFPs
>rely on in-band mode, that isn't going to work.
>
>The options for the former case are:
>
>1) implement phylink covering both the MAC and the external PCS PHY
>2) implement phylink just for the MAC to PCS PHY connection but not
>   SFPs, and implement SFP support separately in the PCS PHY driver.
>
>Maybe phylink needs to split mac_pcs_get_state() so it can be supplied
>by a separate driver, or by the MAC driver as appropriate - but that
>brings with it other problems; phylink with a directly attached SFP
>considers the state of the link between the PCS PHY and the external
>device - not only speed but also interface mode for that part of the
>link.  What you'd see in the mac_config() callback are interface modes
>for that part of the link, not between the MAC and the PCS PHY.
>
>To change that would require reworking almost every driver that has
>already converted over to somehow remodel the built-in PCS and
>COMPHY as a separate PCS PHY for phylink. I'm not entirely clear
>whether that would work though.
>
>--
>RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https-
>3A__www.armlinux.org.uk_developer_patches_&d=DwIBAg&c=aUq983L2pue
>2FqKFoP6PGHMJQyoJ7kl3s3GZ-
>_haXqY&r=BDdk1JtITE_JJ0519WwqU7IKF80Cw1i55lZOGqv2su8&m=ei26OYsu0
>JYGaBZjJMg7WhXT8l_kdzu_QwlOu3RTUhY&s=YHxF2EUKwhleTZ-
>fO9lorELZWnn9kArzxliO1KM0uMc&e=
>
>FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps
>up
>
>According to speedtest.net: 11.9Mbps down 500kbps up
diff mbox series

Patch

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index dbf7070fcdba..b731807d1c49 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -76,10 +76,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 */
@@ -164,6 +166,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 /* USXGMII control register */
+#define GEM_USX_STATUS		0x0A88 /* USXGMII status register */
 
 #define GEM_TXBDCTRL	0x04cc /* TX Buffer Descriptor control register */
 #define GEM_RXBDCTRL	0x04d0 /* RX Buffer Descriptor control register */
@@ -270,11 +275,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 */
@@ -455,11 +468,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
@@ -494,6 +513,34 @@ 
 #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_RX_SYNC_RESET_OFFSET		2
+#define GEM_RX_SYNC_RESET_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_TX_FAULT_OFFSET			28
+#define GEM_USX_TX_FAULT_SIZE			1
+#define GEM_USX_RX_FAULT_OFFSET			27
+#define GEM_USX_RX_FAULT_SIZE			1
+#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
@@ -656,6 +703,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
@@ -724,6 +773,7 @@ 
 	})
 
 #define MACB_READ_NSR(bp)	macb_readl(bp, NSR)
+#define GEM_READ_USX_STATUS(bp)	gem_readl(bp, USX_STATUS)
 
 /* struct macb_dma_desc - Hardware DMA descriptor
  * @addr: DMA address of data buffer
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 17297b01e85e..710ee18a0ef0 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -81,6 +81,18 @@  struct sifive_fu540_macb_mgmt {
 #define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
 #define MACB_WOL_ENABLED		(0x1 << 1)
 
+enum {
+	HS_MAC_SPEED_100M,
+	HS_MAC_SPEED_1000M,
+	HS_MAC_SPEED_2500M,
+	HS_MAC_SPEED_5000M,
+	HS_MAC_SPEED_10000M,
+};
+
+enum {
+	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)
  */
@@ -90,6 +102,8 @@  struct sifive_fu540_macb_mgmt {
 
 #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
 
+#define MACB_USX_BLOCK_LOCK_TIMEOUT	1000000 /* in usecs */
+
 /* DMA buffer descriptor might be different size
  * depends on hardware configuration:
  *
@@ -506,6 +520,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_USXGMII &&
 	    !phy_interface_mode_is_rgmii(state->interface)) {
 		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 		return;
@@ -518,6 +533,13 @@  static void macb_validate(struct phylink_config *config,
 		return;
 	}
 
+	if (state->interface == PHY_INTERFACE_MODE_USXGMII &&
+	    !(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);
@@ -527,6 +549,22 @@  static void macb_validate(struct phylink_config *config,
 	phylink_set(mask, 100baseT_Half);
 	phylink_set(mask, 100baseT_Full);
 
+	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
+	    (state->interface == PHY_INTERFACE_MODE_NA ||
+	     state->interface == PHY_INTERFACE_MODE_USXGMII)) {
+		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);
+		phylink_set(mask, 5000baseT_Full);
+		phylink_set(mask, 2500baseX_Full);
+		phylink_set(mask, 1000baseX_Full);
+		phylink_set(mask, 1000baseT_Full);
+	}
+
 	if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
 	    (state->interface == PHY_INTERFACE_MODE_NA ||
 	     state->interface == PHY_INTERFACE_MODE_GMII ||
@@ -544,6 +582,60 @@  static void macb_validate(struct phylink_config *config,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
+static int gem_mac_usx_configure(struct macb *bp,
+				 const struct phylink_link_state *state)
+{
+	u32 speed, config, val;
+	int ret;
+
+	val = gem_readl(bp, NCFGR);
+	val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val);
+	if (state->pause & MLO_PAUSE_TX)
+		val |= GEM_BIT(PAE);
+	gem_writel(bp, NCFGR, val);
+	gem_writel(bp, NCR, gem_readl(bp, NCR) | GEM_BIT(ENABLE_HS_MAC));
+	gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) | GEM_BIT(FD));
+	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);
+	gem_writel(bp, USX_CONTROL, config | GEM_BIT(TX_EN));
+	config = gem_readl(bp, USX_CONTROL);
+	gem_writel(bp, USX_CONTROL, config | GEM_BIT(SIGNAL_OK));
+	ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
+				 val & GEM_BIT(USX_BLOCK_LOCK),
+				 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
+	if (ret < 0) {
+		netdev_warn(bp->dev, "USXGMII block lock failed");
+		return -ETIMEDOUT;
+	}
+
+	switch (state->speed) {
+	case SPEED_10000:
+		speed = HS_MAC_SPEED_10000M;
+		break;
+	case SPEED_5000:
+		speed = HS_MAC_SPEED_5000M;
+		break;
+	case SPEED_2500:
+		speed = HS_MAC_SPEED_2500M;
+		break;
+	case SPEED_1000:
+		speed = HS_MAC_SPEED_1000M;
+		break;
+	default:
+	case SPEED_100:
+		speed = HS_MAC_SPEED_100M;
+		break;
+	}
+
+	gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, speed,
+						gem_readl(bp, HS_MAC_CONFIG)));
+	gem_writel(bp, USX_CONTROL, GEM_BFINS(USX_CTRL_SPEED, speed,
+					      gem_readl(bp, USX_CONTROL)));
+	return 0;
+}
+
 static void macb_mac_pcs_get_state(struct phylink_config *config,
 				   struct phylink_link_state *state)
 {
@@ -565,30 +657,39 @@  static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 
 	spin_lock_irqsave(&bp->lock, flags);
 
-	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
+	if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) {
+		if (gem_mac_usx_configure(bp, state) < 0) {
+			spin_unlock_irqrestore(&bp->lock, flags);
+			phylink_mac_change(bp->phylink, false);
+			return;
+		}
+	} else {
+		old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
 
-	/* Clear all the bits we might set later */
-	ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) | MACB_BIT(FD) | MACB_BIT(PAE) |
-		  GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
+		/* Clear all the bits we might set later */
+		ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) |
+			  MACB_BIT(FD) | MACB_BIT(PAE) |
+			  GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
 
-	if (state->speed == SPEED_1000)
-		ctrl |= GEM_BIT(GBE);
-	else if (state->speed == SPEED_100)
-		ctrl |= MACB_BIT(SPD);
+		if (state->speed == SPEED_1000)
+			ctrl |= GEM_BIT(GBE);
+		else if (state->speed == SPEED_100)
+			ctrl |= MACB_BIT(SPD);
 
-	if (state->duplex)
-		ctrl |= MACB_BIT(FD);
+		if (state->duplex)
+			ctrl |= MACB_BIT(FD);
 
-	/* We do not support MLO_PAUSE_RX yet */
-	if (state->pause & MLO_PAUSE_TX)
-		ctrl |= MACB_BIT(PAE);
+		/* We do not support MLO_PAUSE_RX yet */
+		if (state->pause & MLO_PAUSE_TX)
+			ctrl |= MACB_BIT(PAE);
 
-	if (state->interface == PHY_INTERFACE_MODE_SGMII)
-		ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
+		if (state->interface == PHY_INTERFACE_MODE_SGMII)
+			ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
 
-	/* Apply the new configuration, if any */
-	if (old_ctrl ^ ctrl)
-		macb_or_gem_writel(bp, NCFGR, ctrl);
+		/* Apply the new configuration, if any */
+		if (old_ctrl ^ ctrl)
+			macb_or_gem_writel(bp, NCFGR, ctrl);
+	}
 
 	bp->speed = state->speed;
 
@@ -3399,6 +3500,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;