[dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation.
Ivan Boule
ivan.boule at 6wind.com
Thu Sep 26 09:27:33 CEST 2013
Hi Qinglai, Venky,
Thanks a lot for all these simplifications toward elegance. I really appreciate
Regards
Ivan
On 09/25/2013 06:59 PM, jigsaw wrote:
> 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 = 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
> <venky.venkatesan at intel.com> 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 will 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 into something like dev_rxtx_start() [ open to suggestions ].
>>
>> Regards,
>> -Venky
>>
>> -----Original Message-----
>> From: jigsaw [mailto:jigsaw at gmail.com]
>> Sent: Wednesday, September 25, 2013 7:39 AM
>> To: Venkatesan, Venky
>> Cc: Ivan Boule; dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation.
>>
>> 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 feasible.
>>
>> 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 <venky.venkatesan at intel.com> wrote:
>>> Qinglai/Ivan,
>>>
>>> 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).
>>>
>>> Regards,
>>> -Venky
>>>
>>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of jigsaw
>>> Sent: Wednesday, September 25, 2013 6:56 AM
>>> To: Ivan Boule
>>> Cc: dev at dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] Add support for 82599 Tx->Rx loopback operation.
>>>
>>> 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 <ivan.boule at 6wind.com> 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 <jigsaw at gmail.com>
>>>> ---
>>>> 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
More information about the dev
mailing list