From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f49.google.com (mail-wg0-f49.google.com [74.125.82.49]) by dpdk.org (Postfix) with ESMTP id 481A358D7 for ; Wed, 25 Sep 2013 17:03:39 +0200 (CEST) Received: by mail-wg0-f49.google.com with SMTP id l18so6156969wgh.4 for ; Wed, 25 Sep 2013 08:04:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type; bh=3LYW1f/rvU6GkTqVCqdCRONNyziLzQDgn7FAuYvT09k=; b=F+9jln9rFozyBFXeViUDOkcs7yVNZNBygryMAX2ho9X9oSydFnnjYPO+dr3+7hFmTS k1sFGp+KvQHPPICgOXPPvwjjKBuXJKX6yTVi80myfQDBDgTAb7Hz2GKmpKd2VjfetWh1 K1FrKi4ZVlwizW+Ljxsmc6OvQ288LesrhcfgYaWHFLOimKMWw57YixkQR9ntlIluTaPV Q4XBvPndYEyV9dZ+tzQNJQK31Q09eQh5A0dvlCv8CJNXhmI8YMxBYfmYYQWb3eJwK7cy zQAPnbWF3lat+E8NpRHGKuKi0Vg6n7uIDBrArIyrLWhFSa4WIaxjB7zam528OELGuWo8 fOaA== X-Gm-Message-State: ALoCoQk4s+RNb1+ULvk72tFzgo8Z4p1uULR0PR89s8eRst7GDTd0Un0Gxdd68SWUYgx8I2PDAR8q X-Received: by 10.180.91.16 with SMTP id ca16mr22959508wib.57.1380121459701; Wed, 25 Sep 2013 08:04:19 -0700 (PDT) Received: from [10.16.0.189] (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id e1sm18745828wij.6.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 25 Sep 2013 08:04:19 -0700 (PDT) Message-ID: <5242FB6F.8040102@6wind.com> Date: Wed, 25 Sep 2013 17:04:15 +0200 From: Ivan Boule User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130827 Icedove/17.0.8 MIME-Version: 1.0 To: jigsaw References: <1379963780-5044-1-git-send-email-jigsaw@gmail.com> <5242E105.6030200@6wind.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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:03:39 -0000 Hi Qinglai, I understand your point. In fact, because loopback modes are only intended to be used in debug situations by developers that [are supposed to] know what they do, it is much simpler to use your approach. Please, can you add into the file librte_ether/rte_ethdev.h comments that are based on your arguments? I also agree with your rework proposal to meet Venky's demand to not change files in the ixgbe sub-directory. Best regards, Ivan On 09/25/2013 03:56 PM, jigsaw wrote: > 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 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 = 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 -- Ivan Boule 6WIND Development Engineer