Re: [PATCH v5 1/2] ethdev: Add link_speed lanes support

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/24/2024 11:59 PM, Damodharam Ammepalli wrote:
> On Sun, Sep 22, 2024 at 10:02 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 9/4/2024 6:50 PM, Damodharam Ammepalli wrote:
>>> Update the eth_dev_ops structure with new function vectors
>>> to get, get capabilities and set ethernet link speed lanes.
>>> Update the testpmd to provide required config and information
>>> display infrastructure.
>>>
>>> The supporting ethernet controller driver will register callbacks
>>> to avail link speed lanes config and get services. This lanes
>>> configuration is applicable only when the nic is forced to fixed
>>> speeds. In Autonegiation mode, the hardware automatically
>>> negotiates the number of lanes.
>>>
>>> These are the new commands.
>>>
>>> testpmd> show port 0 speed_lanes capabilities
>>>
>>>  Supported speeds         Valid lanes
>>> -----------------------------------
>>>  10 Gbps                  1
>>>  25 Gbps                  1
>>>  40 Gbps                  4
>>>  50 Gbps                  1 2
>>>  100 Gbps                 1 2 4
>>>  200 Gbps                 2 4
>>>  400 Gbps                 4 8
>>> testpmd>
>>>
>>> testpmd>
>>> testpmd> port stop 0
>>> testpmd> port config 0 speed_lanes 4
>>> testpmd> port config 0 speed 200000 duplex full
>>> testpmd> port start 0
>>> testpmd>
>>> testpmd> show port info 0
>>>
>>> ********************* Infos for port 0  *********************
>>> MAC address: 14:23:F2:C3:BA:D2
>>> Device name: 0000:b1:00.0
>>> Driver name: net_bnxt
>>> Firmware-version: 228.9.115.0
>>> Connect to socket: 2
>>> memory allocation on the socket: 2
>>> Link status: up
>>> Link speed: 200 Gbps
>>> Active Lanes: 4
>>> Link duplex: full-duplex
>>> Autoneg status: Off
>>>
>>> Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
>>> ---
>>>  app/test-pmd/cmdline.c     | 248 ++++++++++++++++++++++++++++++++++++-
>>>  app/test-pmd/config.c      |   4 +
>>>  lib/ethdev/ethdev_driver.h |  91 ++++++++++++++
>>>  lib/ethdev/rte_ethdev.c    |  52 ++++++++
>>>  lib/ethdev/rte_ethdev.h    |  95 ++++++++++++++
>>>  lib/ethdev/version.map     |   5 +
>>>  6 files changed, 494 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index b7759e38a8..643102032e 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -284,6 +284,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>>>
>>>                       "dump_log_types\n"
>>>                       "    Dumps the log level for all the dpdk modules\n\n"
>>> +
>>> +                     "show port (port_id) speed_lanes capabilities"
>>> +                     "       Show speed lanes capabilities of a port.\n\n"
>>>               );
>>>       }
>>>
>>> @@ -823,6 +826,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>>>                       "port config (port_id) txq (queue_id) affinity (value)\n"
>>>                       "    Map a Tx queue with an aggregated port "
>>>                       "of the DPDK port\n\n"
>>> +
>>> +                     "port config (port_id|all) speed_lanes (0|1|4|8)\n"
>>>
>>
>> This help string, and the implementation, implies there has to be fixed
>> lane values, like 1, 4, 8. Why not leave this part to the capability
>> reporting, and not limit (and worry) those values in the command, so
>> from command's perspective it can be only <lane number>.
>>
> 
> ok, will update the help string to <lane number>
> 
>>> +                     "    Set number of lanes for all ports or port_id for a forced speed\n\n"
>>>               );
>>>       }
>>>
>>> @@ -1560,6 +1566,244 @@ static cmdline_parse_inst_t cmd_config_speed_specific = {
>>>       },
>>>  };
>>>
>>> +static int
>>> +parse_speed_lanes_cfg(portid_t pid, uint32_t lanes)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = rte_eth_speed_lanes_set(pid, lanes);
>>>
>>
>> As a sample application usage, I think it is better if it gets the
>> capability and verify that 'lanes' is withing the capability and later
>> set it, what do you think?
>>
>> <...>
> 
> Makes sense, will try out and get back to you soon.
> 
> 
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h> index
>> 548fada1c7..9444e0a836 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -357,6 +357,27 @@ struct rte_eth_link {
>>>  #define RTE_ETH_LINK_MAX_STR_LEN 40 /**< Max length of default link string. */
>>>  /**@}*/
>>>
>>> +/**
>>> + * Constants used to indicate the possible link speed lanes of an ethdev port.
>>> + */
>>> +#define RTE_ETH_SPEED_LANE_UNKNOWN  0  /**< speed lanes unsupported mode or default */
>>> +#define RTE_ETH_SPEED_LANE_1  1        /**< Link speed lane  1 */
>>> +#define RTE_ETH_SPEED_LANE_2  2        /**< Link speed lanes 2 */
>>> +#define RTE_ETH_SPEED_LANE_4  4        /**< Link speed lanes 4 */
>>> +#define RTE_ETH_SPEED_LANE_8  8        /**< Link speed lanes 8 */
>>> +
>>> +/* Translate from link speed lanes to speed lanes capa */
>>> +#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
>>> +
>>> +/* This macro indicates link speed lanes capa mask */
>>> +#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
>>> +
>>>
>>
>> I am not clear why we need these macros, why not use lane number as
>> unsigned integer, instead of macro (RTE_ETH_SPEED_LANE_2), it will be
>> more flexible.
>>
> 
> ok, I can replace the macros with unsigned integers
> 
>> Probably all we need is 'RTE_ETH_SPEED_LANES_TO_CAPA' one to use as
>> helper in drivers.
>> Again not sure about the ..CAPA_MASK one, does it actually produce a
>> mask value?
>>
>>
> once replacing the LANE_xx to unsigned integers, then I don't need x_CAPA_MASK.
> In the driver, I can call RTE_32(lane number) while populating the table.
> EG:
> 
>         { RTE_ETH_SPEED_NUM_100G, RTE_ETH_SPEED_LANES_CAPA_MASK(LANE_1) |
>                                 RTE_ETH_SPEED_LANES_CAPA_MASK(LANE_2) |
>                                 RTE_ETH_SPEED_LANES_CAPA_MASK(LANE_4) },
> will become
> 
>         { RTE_ETH_SPEED_NUM_100G, RTE_BIT32(1) |
>                                 RTE_BIT32(2) |
>                                 RTE_BIT32(4) },
> 

ack