On 9/24/2024 11:59 PM, Damodharam Ammepalli wrote:
> On Sun, Sep 22, 2024 at 10:02 AM Ferruh Yigit <ferruh.yi...@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.ammepa...@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
