From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by dpdk.org (Postfix) with ESMTP id 18F496848 for ; Wed, 25 Sep 2013 16:11:55 +0200 (CEST) Received: from azsmga002.ch.intel.com ([10.2.17.35]) by azsmga101.ch.intel.com with ESMTP; 25 Sep 2013 07:12:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.90,978,1371106800"; d="scan'208";a="299200735" Received: from orsmsx104.amr.corp.intel.com ([10.22.225.131]) by AZSMGA002.ch.intel.com with ESMTP; 25 Sep 2013 07:12:35 -0700 Received: from orsmsx154.amr.corp.intel.com (10.22.226.12) by ORSMSX104.amr.corp.intel.com (10.22.225.131) with Microsoft SMTP Server (TLS) id 14.3.123.3; Wed, 25 Sep 2013 07:12:34 -0700 Received: from orsmsx102.amr.corp.intel.com ([169.254.1.143]) by ORSMSX154.amr.corp.intel.com ([169.254.11.17]) with mapi id 14.03.0123.003; Wed, 25 Sep 2013 07:12:34 -0700 From: "Venkatesan, Venky" To: jigsaw , Ivan Boule Thread-Topic: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation. Thread-Index: AQHOufclvnQNbSoto0uzoheY00MtAJnWfSuQ Date: Wed, 25 Sep 2013 14:12:33 +0000 Message-ID: <1FD9B82B8BF2CF418D9A1000154491D973F50C7E@ORSMSX102.amr.corp.intel.com> References: <1379963780-5044-1-git-send-email-jigsaw@gmail.com> <5242E105.6030200@6wind.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 14:11:58 -0000 Qinglai/Ivan,=20 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).=20 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 opera= tion. Hi Ivan, Appreciations for your comments. I have one question regarding the new field, lpbk_mode, in struct rte_eth_c= onf. I was thinking how to define the values for this field, then I had a dilemm= a. 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 se= t 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 i= n a higher level API. However, if we don't expose these details here, like in the patch, the valu= e 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 loopbac= k mode of certain controller is mutually exclusive. For instance, is it pos= sible 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 s= tated 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 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 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=20 > +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=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_fiber; > } 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_82599; > - 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_capabilities_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_capabilities_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 status=20 > +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=20 > +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. Loopback is disable= d. > */ > +#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=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 > + 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