From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ie0-x230.google.com (mail-ie0-x230.google.com [IPv6:2607:f8b0:4001:c03::230]) by dpdk.org (Postfix) with ESMTP id 8D42D1F3 for ; Wed, 25 Sep 2013 18:58:55 +0200 (CEST) Received: by mail-ie0-f176.google.com with SMTP id as1so10739778iec.7 for ; Wed, 25 Sep 2013 09:59:31 -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=TCH+JeEO0QRWltiJW/rN3NIaksWx5d/ThRD9Spkh4jw=; b=I+RViz4uQCX+5K/9EBIFpTokYokYggjdkxEaTKQ98Caq0IK8ypVeL77WtrP6xlPbqL o4REZyNhYB+gIVH7cQqzDGZLHfXFEmM8QF5y3bYZmgK3IJ3Klo8U18JGLS8hgqwhTw6N ZYwNLxFgQwguiWAMNrCKyTUhwCr16G/bySyDKLD/1POj3fBaxIH4/2j0YXqDc+3Yp1yy Xr2RJ54fpyJVl1BjAUEr8IBM0P3bMhWITW9MVbEdne9GjVQh5+39XHYEl+S8TR0oDFoZ a0fm5ls3D6QR0M6ERJOhWeN8jkrL2pwgiaF6hOvYGvwajj+eqP61FTJjcaR+FRb4vIbV NVDg== MIME-Version: 1.0 X-Received: by 10.43.82.69 with SMTP id ab5mr1956509icc.95.1380128370427; Wed, 25 Sep 2013 09:59:30 -0700 (PDT) Received: by 10.42.67.205 with HTTP; Wed, 25 Sep 2013 09:59:30 -0700 (PDT) In-Reply-To: <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> <1FD9B82B8BF2CF418D9A1000154491D973F50FCF@ORSMSX102.amr.corp.intel.com> Date: Wed, 25 Sep 2013 19:59:30 +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 16:58:56 -0000 Hi Venky, Good point. Thus we avoid not only replacing all callbacks, but also adding lpbk field in struct ixgbe_hw. All loopback-aware operations will be then restricted to function ixgbe_dev_start and ixgbe_dev_rxtx_start. The logic of ixgbe_dev_start will be sth. like: ... ... ixgbe_dev_tx_init(dev); ... ... if (loopback is configured) goto skip_setup_link; ... ... err =3D ixgbe_setup_link(hw, speed, negotiate, link_up); if (err) goto error; skip_setup_link: ... ... Meantime, as you suggested, the FLU bit will be set in ixgbe_dev_rxtx_start, if loopback is configured. I can send the patch later tomorrow if all agree with this proposal. thx & rgds, -qinglai On Wed, Sep 25, 2013 at 6:47 PM, Venkatesan, Venky wrote: > Thanks a bunch. It does work. > > One more suggestion to think about; Once you set the FLU (force link up)= bit (bit 0) in the AUTOC (0x42A0) register, the auto-negotiation state wil= l be set to AN_GOOD, the link-up indication should be set regardless. Would= you still need the callbacks? You could then merge the setup_link code int= o something like dev_rxtx_start() [ open to suggestions ]. > > Regards, > -Venky > > -----Original Message----- > From: jigsaw [mailto:jigsaw@gmail.com] > 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 ope= ration. > > 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 feasi= ble. > > The solution would be after calling ixgbe_init_shared_code in ixgbe_ethde= v.c, if the config is for loopback operation, we immediately replace the ca= llbacks 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 f= ile 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 t= he librte_pmd_ixgbe/ixgbe directory. Those files are derived directly from = the BSD driver baseline, and any changes will make future merges of newer c= ode 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@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 op= eration. >> >> Hi Ivan, >> >> Appreciations for your comments. >> I have one question regarding the new field, lpbk_mode, in struct rte_et= h_conf. >> >> I was thinking how to define the values for this field, then I had a dil= emma. >> >> 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 ha= ve 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 expose= d in a higher level API. >> >> However, if we don't expose these details here, like in the patch, the v= alue 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 loop= back 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 825= 99? 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 r= te_eth_conf, so that the solution is open for further changes. I should hav= e 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, u1= 6 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 =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_= 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_8= 2599; >>> - 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_capabi= lities_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_capabi= lities_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. Loo= pback is disabled. >>> */ >>> +#define IXGBE_LPBK_82599_TX_RX 0x1 /* Tx->Rx loopback op= eration is >>> enabled. */ >>> +#define IXGBE_LPBK_82599_RX_TX 0x2 /* Rx->Tx loopback op= eration 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