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
