From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id A68BF1F3 for ; Wed, 25 Sep 2013 17:47:15 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 25 Sep 2013 08:47:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.90,978,1371106800"; d="scan'208";a="383410070" Received: from orsmsx101.amr.corp.intel.com ([10.22.225.128]) by orsmga001.jf.intel.com with ESMTP; 25 Sep 2013 08:47:54 -0700 Received: from orsmsx155.amr.corp.intel.com (10.22.240.21) by ORSMSX101.amr.corp.intel.com (10.22.225.128) with Microsoft SMTP Server (TLS) id 14.3.123.3; Wed, 25 Sep 2013 08:47:53 -0700 Received: from orsmsx102.amr.corp.intel.com ([169.254.1.143]) by ORSMSX155.amr.corp.intel.com ([169.254.7.94]) with mapi id 14.03.0123.003; Wed, 25 Sep 2013 08:47:52 -0700 From: "Venkatesan, Venky" To: jigsaw Thread-Topic: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation. Thread-Index: AQHOufz7jLVYWAj7pESq0qaJ/i24hZnWliww Date: Wed, 25 Sep 2013 15:47:52 +0000 Message-ID: <1FD9B82B8BF2CF418D9A1000154491D973F50FCF@ORSMSX102.amr.corp.intel.com> References: <1379963780-5044-1-git-send-email-jigsaw@gmail.com> <5242E105.6030200@6wind.com> <1FD9B82B8BF2CF418D9A1000154491D973F50C7E@ORSMSX102.amr.corp.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.22.254.139] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Sep 2013 15:47:16 -0000 Thanks a bunch. It does work.=20 One more suggestion to think about; Once you set the FLU (force link up) b= it (bit 0) in the AUTOC (0x42A0) register, the auto-negotiation state will = be set to AN_GOOD, the link-up indication should be set regardless. Would y= ou still need the callbacks? You could then merge the setup_link code into = something like dev_rxtx_start() [ open to suggestions ].=20 Regards,=20 -Venky -----Original Message----- From: jigsaw [mailto:jigsaw@gmail.com]=20 Sent: Wednesday, September 25, 2013 7:39 AM To: Venkatesan, Venky Cc: Ivan Boule; dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback opera= tion. Hi Venky, Thanks for you comments. >>I for one would prefer that the changes not really modify any files in=20 >>the librte_pmd_ixgbe/ixgbe directory With this restriction it is gonna be little bit difficult but still feasibl= e. 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 call= backs 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 fil= e 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 wrote: > Qinglai/Ivan, > > I for one would prefer that the changes not really modify any files in th= e librte_pmd_ixgbe/ixgbe directory. Those files are derived directly from t= he BSD driver baseline, and any changes will make future merges of newer co= de more challenging. The changes should be limited to files in the librte_p= md_ixgbe directory (and ethdev). > > Regards, > -Venky > > > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of jigsaw > Sent: Wednesday, September 25, 2013 6:56 AM > To: Ivan Boule > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback ope= ration. > > 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 dile= mma. > > 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 hav= e 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 va= lue is just a integer, user of the API may get confused, and he/she still h= as 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 loopb= ack mode of certain controller is mutually exclusive. For instance, is it p= ossible that the Rx-Tx and Tx-Rx can be activated at the same time for 8259= 9? 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 rt= e_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 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 >> --- >> 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=20 >> 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=20 >> autonegotation */ >> + uint32_t lpbk; /**< Loopback operation. The value depends on=20 >> +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 =3D 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=20 >> *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=20 >> +ixgbe_check_mac_link_82599_lpbk(struct ixgbe_hw *hw, >> ixgbe_link_speed *speed, >> + bool *link_up, bool=20 >> +link_up_wait_to_complete); STATIC s32 ixgbe_setup_mac_link_82599_lpbk(s= truct 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 =3D &hw->mac; @@ -68,7 +79,10 @@ void=20 >> ixgbe_init_mac_link_ops_82599(struct ixgbe_hw *hw) >> mac->ops.flap_tx_laser =3D NULL; >> } >> >> - if (hw->phy.multispeed_fiber) { >> + if (hw->lpbk =3D=3D IXGBE_LPBK_82599_TX_RX) { >> + /* Support for Tx->Rx loopback operation */ >> + mac->ops.setup_link =3D &ixgbe_setup_mac_link_82599_lpbk; >> + } else if (hw->phy.multispeed_fiber) { >> /* Set up dual speed SFP+ support */ >> mac->ops.setup_link =3D &ixgbe_setup_mac_link_multispeed_f= iber; >> } else { >> @@ -266,8 +280,23 @@ s32 ixgbe_init_ops_82599(struct ixgbe_hw *hw) >> mac->ops.set_vlan_anti_spoofing =3D=20 >> &ixgbe_set_vlan_anti_spoofing; >> >> /* Link */ >> - mac->ops.get_link_capabilities =3D &ixgbe_get_link_capabilities_82= 599; >> - mac->ops.check_link =3D &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 =3D &ixgbe_get_link_capabil= ities_82599_lpbk; >> + mac->ops.check_link =3D &ixgbe_check_mac_link_82599_lpbk; >> + break; >> + >> + case IXGBE_LPBK_82599_NONE: /* FALL THRU */ >> + default: >> + mac->ops.get_link_capabilities =3D &ixgbe_get_link_capabil= ities_82599; >> + mac->ops.check_link =3D &ixgbe_check_mac_link_generic; >> + break; >> + } >> + >> mac->ops.setup_rxpba =3D &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=20 >> +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 =3D IXGBE_LINK_SPEED_10GB_FULL; >> + *negotiation =3D false; >> + >> + return IXGBE_SUCCESS; >> +} >> >> +/** >> + * ixgbe_check_mac_link_82599_lpbk - Determine link and speed=20 >> +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 =3D true; >> + *speed =3D 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=20 >> +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 =3D IXGBE_SUCCESS; >> + >> + DEBUGFUNC("ixgbe_setup_mac_link_82599_lpbk"); >> + autoc =3D IXGBE_AUTOC_LMS_10G_LINK_NO_AN | IXGBE_AUTOC_FLU; >> + >> + if (ixgbe_verify_lesm_fw_enabled_82599(hw)) { >> + status =3D hw->mac.ops.acquire_swfw_sync(hw, >> + IXGBE_GSSR_MAC_CSR_SM); >> + if (status !=3D IXGBE_SUCCESS) { >> + status =3D 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. Loop= back is disabled. >> */ >> +#define IXGBE_LPBK_82599_TX_RX 0x1 /* Tx->Rx loopback ope= ration is >> enabled. */ >> +#define IXGBE_LPBK_82599_RX_TX 0x2 /* Rx->Tx loopback ope= ration 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=20 >> 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 !=3D RTE_LOOPBACK_NONE) { >> >> + struct ixgbe_hw *hw =3D >> + =20 >> + IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >> + >> + hw->lpbk =3D dev->data->dev_conf.lpbk; >> >> IB> replace the line above by: >> + /* Only supports TX->RX loopback mode for now. */ >> + hw->lpbk =3D IXGBE_LPBK_82599_TX_RX; >> >> + } >> + >> + >> /* set flag to update link status after init */ >> intr->flags |=3D 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 &=3D ~IXGBE_HLREG0_JUMBOEN; >> >> + /* >> + * Both Tx->Rx and Rx->Tx requres IXGBE_HLREG0_LPBK to be set. >> >> IB> requres -> requires >> >> + */ >> + if (hw->lpbk) >> + hlreg0 |=3D IXGBE_HLREG0_LPBK; >> + >> IXGBE_WRITE_REG(hw, IXGBE_HLREG0, hlreg0); >> >> /* Setup RX queues */ >> >> >> >> -- >> Ivan Boule >> 6WIND Development Engineer