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
