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

Ivan Boule ivan.boule at 6wind.com
Wed Sep 25 17:04:15 CEST 2013


    Hi Qinglai,

    I understand your point.
    In fact, because loopback modes are only intended to be used in
    debug situations by developers that [are supposed to] know what they
    do, it is much simpler to use your approach.
    Please, can you add into the file librte_ether/rte_ethdev.h comments
    that are based on your arguments?
    I also agree with your rework proposal to meet Venky's demand to not
    change files in the ixgbe sub-directory.
    Best regards,
    Ivan


On 09/25/2013 03:56 PM, jigsaw wrote:
> 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


-- 
Ivan Boule
6WIND Development Engineer



More information about the dev mailing list