Hi Venky, Thanks for you comments.
>>I for one would prefer that the changes not really modify any files in the >>librte_pmd_ixgbe/ixgbe directory With this restriction it is gonna be little bit difficult but still feasible. The solution would be after calling ixgbe_init_shared_code in ixgbe_ethdev.c, if the config is for loopback operation, we immediately replace the callbacks of: mac->ops.get_link_capabilities mac->ops.check_link mac->ops.setup_link And of course the implementation of these callbacks can be in a new src file under librte_pmd_ixgbe, thus keep the baseline untouched. Sounds like a plan? thx & rgds, -qinglai On Wed, Sep 25, 2013 at 5:12 PM, Venkatesan, Venky <venky.venkatesan at intel.com> wrote: > 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
