From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ie0-x232.google.com (mail-ie0-x232.google.com [IPv6:2607:f8b0:4001:c03::232]) by dpdk.org (Postfix) with ESMTP id E6E181F3 for ; Wed, 25 Sep 2013 16:37:51 +0200 (CEST) Received: by mail-ie0-f178.google.com with SMTP id to1so10783878ieb.37 for ; Wed, 25 Sep 2013 07:38:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=wCeYShLBiQnTrFE1YWiu8jtMcEBERmUWELDmf/T/teM=; b=ccy7T6JzCThYuy0GdlulT+ZybV/Ybh0mZZu7lqoZglikBuukxYF6tVU23avROVq5L9 dy+dth5DRwgTw7pYJN3fYiloy+hezg3Ogmmg3H2oJgmPfO3cGxCj6qppuAanlOsGtG9Y saFLbyRxYNJE/dltoBiYHeVHig/1Y9Ccot9RsDtpGnjqIRCc92BFxhGLae7CyV112Waz 521fg6oiLqU4OLkuv+bPbT+IoG3EW30FObBQD2jncKtvzZdv+1+03jU6+Jspk4aP+wYP BlgOtGh80AQAVfvssIVtlwz+BjLaC5as+G05zfmcRPGWQnmRobZEvQJVKAw5EMU6I4Q+ 78lg== MIME-Version: 1.0 X-Received: by 10.43.104.73 with SMTP id dl9mr16774252icc.39.1380119912011; Wed, 25 Sep 2013 07:38:32 -0700 (PDT) Received: by 10.42.67.205 with HTTP; Wed, 25 Sep 2013 07:38:31 -0700 (PDT) In-Reply-To: <1FD9B82B8BF2CF418D9A1000154491D973F50C7E@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> Date: Wed, 25 Sep 2013 17:38:31 +0300 Message-ID: From: jigsaw To: "Venkatesan, Venky" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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 14:37:52 -0000 Hi Venky, Thanks for you comments. >>I for one would prefer that the changes not really modify any files in th= e 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 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 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 >> 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 =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 >> *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_compl= ete); 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 =3D &hw->mac; @@ -68,7 +79,10 @@ void >> 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 &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 >> +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 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 >> +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 >> 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 >> + 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