[dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation.

Venkatesan, Venky venky.venkatesan at intel.com
Wed Sep 25 16:12:33 CEST 2013


Qinglai/Ivan, 

I for one would prefer that the changes not really modify any files in the librte_pmd_ixgbe/ixgbe directory. Those files are derived directly from the BSD driver baseline, and any changes will make future merges of newer code more challenging. The changes should be limited to files in the librte_pmd_ixgbe directory (and ethdev). 

Regards,
-Venky


-----Original Message-----
From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of jigsaw
Sent: Wednesday, September 25, 2013 6:56 AM
To: Ivan Boule
Cc: dev at dpdk.org
Subject: Re: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation.

Hi Ivan,

Appreciations for your comments.
I have one question regarding the new field, lpbk_mode, in struct rte_eth_conf.

I was thinking how to define the values for this field, then I had a dilemma.

82599 has only two mode of loopback, either Tx->Rx or Rx->Tx.
But other controller may have different ideas. For instance, I350 can be set as either MAC, PHY or SGMII loopback mode.
So if we define loopback mode as a enum type in rte_ethdev.h, we then have to expose the driver level details.
That is, the enum rte_loopback_mode  will be sth. like:

enum rte_loopback_mode {
        RTE_LOOPBACK_NONE,
        RTE_LOOPBACK_82599_TX_RX,
        RTE_LOOPBACK_I350_MAC,
        RTE_LOOPBACK_I350_PHY,
        /* will be more if we add support for other controllers */ };

IMO it doesn't look so nice coz the hardware specific details are exposed in a higher level API.

However, if we don't expose these details here, like in the patch, the value is just a integer, user of the API may get confused, and he/she still has to be aware of what are possible values for his/her eth controller.

There may be more subtle problems. It's not clear to me whether the loopback mode of certain controller is mutually exclusive. For instance, is it possible that the Rx-Tx and Tx-Rx can be activated at the same time for 82599? If so then the lpbk_mode has to be defined as bitfield.

Having these questions in mind, I decided to expose just a uint32_t in rte_eth_conf, so that the solution is open for further changes. I should have stated my thoughts before sending the patch, and I hope it's not too late to open the discussion at this moment.

Looking forward to your advice.

thx &
rgds,
-qinglai


On Wed, Sep 25, 2013 at 4:11 PM, Ivan Boule <ivan.boule at 6wind.com> wrote:
> Hi Qinglai Xiao,
>
> See my remarks inline prefixed with IB> Best regards, Ivan
>
> On 09/23/2013 09:16 PM, Qinglai Xiao wrote:
>
> Signed-off-by: Qinglai Xiao <jigsaw at gmail.com>
> ---
>  lib/librte_ether/rte_ethdev.h            |    1 +
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c |  122
> +++++++++++++++++++++++++++++-
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h  |    7 ++
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c      |    8 ++
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c        |    6 ++
>  5 files changed, 141 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.h 
> b/lib/librte_ether/rte_ethdev.h index 2d7385f..f474e5b 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -670,6 +670,7 @@ struct rte_eth_conf {
>  	/**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */
>  	uint16_t link_duplex;
>  	/**< ETH_LINK_[HALF_DUPLEX|FULL_DUPLEX], or 0 for autonegotation */
> +	uint32_t lpbk; /**< Loopback operation. The value depends on 
> +ethernet
> controller. */
>  	struct rte_eth_rxmode rxmode; /**< Port RX configuration. */
>  	struct rte_eth_txmode txmode; /**< Port TX configuration. */
>  	union {
>
> IB> As RX->TX loopback mode is not supported, only introduce the
> configuration of the
>     TX->RX loopback mode in the DPDK API as follows:
>
> /**
>  * A set of flags to identify which loopback mode to use, if any.
>  */
> enum rte_loopback_mode {
>     RTE_LOOPBACK_NONE = 0, /**< No loopback */
>     RTE_LOOPBACK_TX_RX,    /**< TX->RX loopback mode */
> };
>
> struct rte_eth_conf {
>     uint16_t link_speed;
>
>     /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */
>     uint16_t link_duplex;
>     /**< ETH_LINK_[HALF_DUPLEX|FULL_DUPLEX], or 0 for autonegotation */
>     enum rte_loopback_mode lpbk_mode; /**< loopback mode. */
>     ...
>
> };
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c
> b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c
> index db07789..0416c01 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c
> @@ -48,6 +48,17 @@ STATIC s32 ixgbe_read_eeprom_82599(struct ixgbe_hw 
> *hw,  STATIC s32 ixgbe_read_eeprom_buffer_82599(struct ixgbe_hw *hw, u16 offset,
>  					  u16 words, u16 *data);
>
> +
> +STATIC s32 ixgbe_get_link_capabilities_82599_lpbk(struct ixgbe_hw *hw,
> +				      ixgbe_link_speed *speed,
> +				      bool *negotiation);
> +STATIC s32 ixgbe_check_mac_link_82599_lpbk(struct ixgbe_hw *hw,
> ixgbe_link_speed *speed,
> +				 bool *link_up, bool link_up_wait_to_complete); STATIC s32 
> +ixgbe_setup_mac_link_82599_lpbk(struct ixgbe_hw *hw,
> +			       ixgbe_link_speed speed, bool autoneg,
> +			       bool autoneg_wait_to_complete);
> +
> +
>  void ixgbe_init_mac_link_ops_82599(struct ixgbe_hw *hw)  {
>  	struct ixgbe_mac_info *mac = &hw->mac; @@ -68,7 +79,10 @@ void 
> ixgbe_init_mac_link_ops_82599(struct ixgbe_hw *hw)
>  		mac->ops.flap_tx_laser = NULL;
>  	}
>
> -	if (hw->phy.multispeed_fiber) {
> +	if (hw->lpbk == IXGBE_LPBK_82599_TX_RX) {
> +		/* Support for Tx->Rx loopback operation */
> +		mac->ops.setup_link = &ixgbe_setup_mac_link_82599_lpbk;
> +	} else if (hw->phy.multispeed_fiber) {
>  		/* Set up dual speed SFP+ support */
>  		mac->ops.setup_link = &ixgbe_setup_mac_link_multispeed_fiber;
>  	} else {
> @@ -266,8 +280,23 @@ s32 ixgbe_init_ops_82599(struct ixgbe_hw *hw)
>  	mac->ops.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing;
>
>  	/* Link */
> -	mac->ops.get_link_capabilities = &ixgbe_get_link_capabilities_82599;
> -	mac->ops.check_link = &ixgbe_check_mac_link_generic;
> +
> +	/* 82599 has two loopback operations: Tx->Rx and Rx->Tx
> +	 * Only Tx->Rx is supported for now.
> +	 */
> +	switch (hw->lpbk) {
> +	case IXGBE_LPBK_82599_TX_RX:
> +		mac->ops.get_link_capabilities = &ixgbe_get_link_capabilities_82599_lpbk;
> +		mac->ops.check_link = &ixgbe_check_mac_link_82599_lpbk;
> +		break;
> +
> +	case IXGBE_LPBK_82599_NONE: /* FALL THRU */
> +	default:
> +		mac->ops.get_link_capabilities = &ixgbe_get_link_capabilities_82599;
> +		mac->ops.check_link = &ixgbe_check_mac_link_generic;
> +		break;
> +	}
> +
>  	mac->ops.setup_rxpba = &ixgbe_set_rxpba_generic;
>  	ixgbe_init_mac_link_ops_82599(hw);
>
> @@ -2370,5 +2399,92 @@ reset_pipeline_out:
>  	return ret_val;
>  }
>
> +/**
> + *  ixgbe_get_link_capabilities_82599_lpbk - Determines link 
> +capabilities
> for Tx->Rx loopback setting
> + *  @hw: pointer to hardware structure
> + *  @speed: pointer to link speed
> + *  @negotiation: always false
> + *
> + *  For Tx->Rx loopback only. Rx->Tx loopback is not supported for now.
> + *
> + *  @speed is always set to IXGBE_LINK_SPEED_10GB_FULL,
> + *  @negotiation is always set to false.
> + **/
> +STATIC s32 ixgbe_get_link_capabilities_82599_lpbk(struct ixgbe_hw *hw,
> +				      ixgbe_link_speed *speed,
> +				      bool *negotiation)
> +{
> +	DEBUGFUNC("ixgbe_get_link_capabilities_82599_lpbk");
> +
> +	*speed = IXGBE_LINK_SPEED_10GB_FULL;
> +	*negotiation = false;
> +
> +	return IXGBE_SUCCESS;
> +}
>
> +/**
> + *  ixgbe_check_mac_link_82599_lpbk - Determine link and speed status 
> +for
> loopback Tx->Rx setting
> + *  @hw: pointer to hardware structure
> + *  @speed: pointer to link speed
> + *  @link_up: true when link is up
> + *  @link_up_wait_to_complete: bool used to wait for link up or not
> + *
> + *  For Tx->Rx loopback only. Rx->Tx loopback is not supported for now.
> + *
> + *  Regardless of link status (LINKS), always set @linkup to true,
> + *  and @speed to IXGBE_LINK_SPEED_10GB_FULL.
> + **/
> +STATIC s32 ixgbe_check_mac_link_82599_lpbk(struct ixgbe_hw *hw,
> ixgbe_link_speed *speed,
> +		bool *link_up, bool link_up_wait_to_complete) {
> +	DEBUGFUNC("ixgbe_check_mac_link_82599_lpbk");
> +
> +	*link_up = true;
> +	*speed = IXGBE_LINK_SPEED_10GB_FULL;
> +
> +	return 0;
> +}
> +
> +/**
> + *  ixgbe_setup_mac_link_82599_loopback - Set MAC link speed for 
> +Tx->Rx
> loopback operation.
> + *  @hw: pointer to hardware structure
> + *  @speed: new link speed
> + *  @autoneg: true if autonegotiation enabled
> + *  @autoneg_wait_to_complete: true when waiting for completion is 
> +needed
> + *
> + *  For Tx->Rx loopback only. Rx->Tx loopback is not supported for now.
> + *
> + *  @speed, @autoneg and @autoneg_wait_to_complete are ignored.
> + *  Just set AUTOC to IXGBE_AUTOC_LMS_10G_LINK_NO_AN | IXGBE_AUTOC_FLU.
> + **/
> +STATIC s32 ixgbe_setup_mac_link_82599_lpbk(struct ixgbe_hw *hw,
> +			       ixgbe_link_speed speed, bool autoneg,
> +			       bool autoneg_wait_to_complete) {
> +	u32 autoc;
> +	u32 status = IXGBE_SUCCESS;
> +
> +	DEBUGFUNC("ixgbe_setup_mac_link_82599_lpbk");
> +	autoc = IXGBE_AUTOC_LMS_10G_LINK_NO_AN | IXGBE_AUTOC_FLU;
> +
> +	if (ixgbe_verify_lesm_fw_enabled_82599(hw)) {
> +		status = hw->mac.ops.acquire_swfw_sync(hw,
> +				IXGBE_GSSR_MAC_CSR_SM);
> +		if (status != IXGBE_SUCCESS) {
> +			status = IXGBE_ERR_SWFW_SYNC;
> +			goto out;
> +		}
> +	}
> +
> +	/* Restart link */
> +	IXGBE_WRITE_REG(hw, IXGBE_AUTOC, autoc);
> +	ixgbe_reset_pipeline_82599(hw);
> +
> +	hw->mac.ops.release_swfw_sync(hw,
> +			IXGBE_GSSR_MAC_CSR_SM);
> +	msec_delay(50);
> +
> +out:
> +	return status;
> +}
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
> b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
> index 7fffd60..a31c9f7 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
> +++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
> @@ -2352,6 +2352,12 @@ enum ixgbe_fdir_pballoc_type {
>  #define FW_CEM_MAX_RETRIES		3
>  #define FW_CEM_RESP_STATUS_SUCCESS	0x1
>
> +/* Loopback operation types */
> +/* 82599 specific loopback operation types */
> +#define IXGBE_LPBK_82599_NONE		0x0 /* Default value. Loopback is disabled.
> */
> +#define IXGBE_LPBK_82599_TX_RX		0x1 /* Tx->Rx loopback operation is
> enabled. */
> +#define IXGBE_LPBK_82599_RX_TX		0x2 /* Rx->Tx loopback operation is
> enabled. */
> +
>  /* Host Interface Command Structures */
>
>  struct ixgbe_hic_hdr {
> @@ -3150,6 +3156,7 @@ struct ixgbe_hw {
>  	int api_version;
>  	bool force_full_reset;
>  	bool allow_unsupported_sfp;
> +	uint32_t lpbk;
>
> IB> A uint8_t is enough.
>
>  };
>
>
>  #define ixgbe_call_func(hw, func, params, error) \ diff --git 
> a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> index 9235f9d..09600bc 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> @@ -1137,6 +1137,14 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
>
>  	PMD_INIT_FUNC_TRACE();
>
> +	if (dev->data->dev_conf.lpbk) {
>
> IB> replace the line above by:
> +    if (dev->data->dev_conf.lpbk_mode != RTE_LOOPBACK_NONE) {
>
> +		struct ixgbe_hw *hw =
> +			IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +		hw->lpbk = dev->data->dev_conf.lpbk;
>
> IB> replace the line above by:
> +        /* Only supports TX->RX loopback mode for now. */
> +        hw->lpbk = IXGBE_LPBK_82599_TX_RX;
>
> +	}
> +
> +
>  	/* set flag to update link status after init */
>  	intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index 5fba01d..158da0e 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -3252,6 +3252,12 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>  	} else
>  		hlreg0 &= ~IXGBE_HLREG0_JUMBOEN;
>
> +	/*
> +	 * Both Tx->Rx and Rx->Tx requres IXGBE_HLREG0_LPBK to be set.
>
> IB> requres -> requires
>
> +	 */
> +	if (hw->lpbk)
> +		hlreg0 |= IXGBE_HLREG0_LPBK;
> +
>  	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, hlreg0);
>
>  	/* Setup RX queues */
>
>
>
> --
> Ivan Boule
> 6WIND Development Engineer


More information about the dev mailing list