From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4647C45460; Fri, 14 Jun 2024 20:27:25 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CBB8740B9A; Fri, 14 Jun 2024 20:27:24 +0200 (CEST) Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com [209.85.208.176]) by mails.dpdk.org (Postfix) with ESMTP id ABABA4060B for ; Fri, 14 Jun 2024 20:27:22 +0200 (CEST) Received: by mail-lj1-f176.google.com with SMTP id 38308e7fff4ca-2ec1620a956so13377641fa.1 for ; Fri, 14 Jun 2024 11:27:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; t=1718389642; x=1718994442; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=K7TTy9quFftwzTf6frCPSc0R/XF1Zo/XkUtrUEi1/Fw=; b=BCgZtirEzhPXofZpuPyXxOCGlLpHnZppCa3hKAQDAUro5yBYOLOKxGgLQr8sHOscg0 o/G79E19jYy0M4JH0HYSJ+v83EamyOtFpGkwhjI7FeRMlmVRf207n8ywvMZ7h8KyodkH +8FikZft9Pjg4LKHgrTRLIEbOj2Rd88PiDMHM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718389642; x=1718994442; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=K7TTy9quFftwzTf6frCPSc0R/XF1Zo/XkUtrUEi1/Fw=; b=jwXnawqRkdjvg/hnd4Rlr/9+PzKOjvX3qKgcTgqRLhr5R/Urr4smL1zDUXqY5K1+d2 CQ2D+Lxv4Na0Zxo3CMi2isQhBFpZyayNtfSnZKL7GJBpqbOPJqOIWfsWgM11UJNb47vX /xrIguqv7t+pYQ+NTHsSDRvpsp/IBHRAl18Z3oF60rQw/uF0SJShy47xvPWjqnN1WEMB TsA7dvR3vcXoGXzir4RS13K1vSRcce+ZkkEssiRFoS6fAft3OQBoT8d8Y82xuLTM1Ej5 VKs0t2QH/CYvMSnsPL0b3XKkg5MGBIZJpysfL2XWGnhr8NAi71cOM3vtq5t8wlSotSjS Odmw== X-Gm-Message-State: AOJu0Yyj6b0CAvisRHttdtVEPvOGaz1ssfQ2s6NjEI3IK5RmfvP7ex3e X+IerC3VPDx5iVIdwskHgJexVkg7GYg0aq37jLjf6Hv4eZwGLYO7aTOymECR0VJBx8v1kFEFAyU 1coqq1EcFicIi9k32Uz0jHzUuZ5EKrp/BKbQw0p4ie/TnzGsePFFvSy2jTxn0kcQQ0XN6M03k/5 cUw8Hl X-Google-Smtp-Source: AGHT+IF/oibm2drKzqfBuDcLSpsyhwWzcWX3LgBeYvLZpIpcDMJWtD+F+Kn9Rzb+lBuZ56urc9+lsSsiDKjoMqQdfRQ= X-Received: by 2002:a2e:9593:0:b0:2eb:eb8b:738 with SMTP id 38308e7fff4ca-2ec0e5d1345mr22137201fa.29.1718389641979; Fri, 14 Jun 2024 11:27:21 -0700 (PDT) MIME-Version: 1.0 References: <20240602024504.179506-1-damodharam.ammepalli@broadcom.com> <20240602024504.179506-2-damodharam.ammepalli@broadcom.com> <72d5782d-d4ca-4e1d-b4f6-2aca979c3506@amd.com> In-Reply-To: <72d5782d-d4ca-4e1d-b4f6-2aca979c3506@amd.com> From: Damodharam Ammepalli Date: Fri, 14 Jun 2024 11:27:09 -0700 Message-ID: Subject: Re: [PATCH v2 1/4] lib/ethdev: Add link_speed lanes support into rte lib To: Ferruh Yigit Cc: dev@dpdk.org, Kalesh AP , Ajit Khaparde , Dengdui Huang Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Jun 11, 2024 at 4:39=E2=80=AFPM Ferruh Yigit = wrote: > > On 6/2/2024 3:45 AM, Damodharam Ammepalli wrote: > > Update the eth_dev_ops structure with new function vectors > > to get and set number of speed lanes. This will help user to > > configure the same fixed speed with different number of lanes > > based on the physical carrier type. > > > > Signed-off-by: Damodharam Ammepalli > > Reviewed-by: Kalesh AP > > Reviewed-by: Ajit Khaparde > > --- > > lib/ethdev/ethdev_driver.h | 49 +++++++++++++++++++++++++++++++++++ > > lib/ethdev/rte_ethdev.c | 26 +++++++++++++++++++ > > lib/ethdev/rte_ethdev.h | 52 ++++++++++++++++++++++++++++++++++++++ > > lib/ethdev/version.map | 2 ++ > > 4 files changed, 129 insertions(+) > > > > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h > > index 0dbf2dd6a2..b1f473e4de 100644 > > --- a/lib/ethdev/ethdev_driver.h > > +++ b/lib/ethdev/ethdev_driver.h > > @@ -1179,6 +1179,51 @@ typedef int (*eth_rx_descriptor_dump_t)(const st= ruct rte_eth_dev *dev, > > uint16_t queue_id, uint16_t offse= t, > > uint16_t num, FILE *file); > > > > +/** > > + * @internal > > + * Get number of current active lanes and max supported lanes > > > > There is already a 'eth_speed_lanes_get_capa' dev_ops, why max supported > lanes returned in this call, instead of capa? > > > + * > > + * @param dev > > + * ethdev handle of port. > > + * @param speed_lanes_capa > > + * Number of active lanes that the link is trained up. > > + * Max number of lanes supported by HW > > + * @return > > + * Negative errno value on error, 0 on success. > > + * > > + * @retval 0 > > + * Success, get speed_lanes data success. > > > > Success, speed_lanes_capa filled. > > > + * @retval -ENOTSUP > > + * Operation is not supported. > > + * @retval -EIO > > + * Device is removed. > > + */ > > +typedef int (*eth_speed_lanes_get_t)(struct rte_eth_dev *dev, > > + struct rte_eth_speed_lanes_capa *spe= ed_lanes_capa); > > + > > +/** > > + * @internal > > + * Set speed lanes > > > > As we are on the subject, we all understand what "speed lane" is in this > context, but I am not sure if the naming is descriptive enough, how this > is referenced in datasheet? > In the data sheet there is no special description used. Ethtool says "link modes" Eg: caa4bbe ethtool: Add support for 200Gbps (50Gbps per lane) link mode The cmdline is ethtool -s < int> speed 200000 lanes < int> Taking our BRCM data sheet as reference. The user can configure 100Gb in three ways, and the hardware trains up the link with these two user fed configs (speed + lane + drivers default phy auto params) with the mapped signalling mode given below. Eg: with this user config, speed 100 lanes 2 , the PHY will train up fixed speed with PAM4-56 signalling (if the cable and HW supports) 100Gb (NRZ: 25G per lane, 4 lanes) link speed 100Gb (PAM4-56: 50G per lane, 2 lanes) link speed 100Gb (PAM4-112: 100G per lane, 1 lane) link speed Let me know if you have a proposal to update the function name or/and function comments? > > + * > > + * @param dev > > + * ethdev handle of port. > > + * @param speed_lanes_capa > > + * Non-negative number of lanes > > + * > > + * @return > > + * Negative errno value on error, 0 on success. > > + * > > + * @retval 0 > > + * Success, set lanes success. > > + * @retval -ENOTSUP > > + * Operation is not supported. > > + * @retval -EINVAL > > + * Unsupported mode requested. > > + * @retval -EIO > > + * Device is removed. > > + */ > > +typedef int (*eth_speed_lanes_set_t)(struct rte_eth_dev *dev, uint32_t= speed_lanes_capa); > > + > > These new dev_ops seems inserted in between 'rx_descriptor_dump' & > 'tx_descriptor_dump' dev_ops. > Can you please move them just below 'eth_link_update_t'? > Is this change only into the struct eth_dev_ops or applies to the function declarations also? > > > > /** > > * @internal > > * Dump Tx descriptor info to a file. > > @@ -1474,6 +1519,10 @@ struct eth_dev_ops { > > eth_count_aggr_ports_t count_aggr_ports; > > /** Map a Tx queue with an aggregated port of the DPDK port */ > > eth_map_aggr_tx_affinity_t map_aggr_tx_affinity; > > + /** Get number of speed lanes supported and active lanes */ > > + eth_speed_lanes_get_t speed_lanes_get; > > + /** Set number of speed lanes */ > > + eth_speed_lanes_set_t speed_lanes_set; > > }; > > > > /** > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > > index f1c658f49e..45e2f7645b 100644 > > --- a/lib/ethdev/rte_ethdev.c > > +++ b/lib/ethdev/rte_ethdev.c > > @@ -7008,4 +7008,30 @@ int rte_eth_dev_map_aggr_tx_affinity(uint16_t po= rt_id, uint16_t tx_queue_id, > > return ret; > > } > > > > +int > > +rte_eth_speed_lanes_get(uint16_t port_id, struct rte_eth_speed_lanes_c= apa *capa) > > +{ > > + struct rte_eth_dev *dev; > > + > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > + dev =3D &rte_eth_devices[port_id]; > > + > > + if (*dev->dev_ops->speed_lanes_get =3D=3D NULL) > > + return -ENOTSUP; > > + return eth_err(port_id, (*dev->dev_ops->speed_lanes_get)(dev, cap= a)); > > > > Shouldn't we verify if 'capa' is not NULL in API? > > > +} > > + > > +int > > +rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa) > > +{ > > + struct rte_eth_dev *dev; > > + > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > + dev =3D &rte_eth_devices[port_id]; > > + > > + if (*dev->dev_ops->speed_lanes_set =3D=3D NULL) > > + return -ENOTSUP; > > + return eth_err(port_id, (*dev->dev_ops->speed_lanes_set)(dev, spe= ed_lanes_capa)); > > +} > > + > > RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO); > > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > > index 147257d6a2..caae1f27c6 100644 > > --- a/lib/ethdev/rte_ethdev.h > > +++ b/lib/ethdev/rte_ethdev.h > > @@ -1997,6 +1997,12 @@ struct rte_eth_fec_capa { > > uint32_t capa; /**< FEC capabilities bitmask */ > > }; > > > > +/* A structure used to get and set lanes capabilities per link speed *= / > > +struct rte_eth_speed_lanes_capa { > > + uint32_t active_lanes; > > + uint32_t max_lanes_cap; > > +}; > > + > > #define RTE_ETH_ALL RTE_MAX_ETHPORTS > > > > /* Macros to check for valid port */ > > @@ -6917,6 +6923,52 @@ rte_eth_tx_queue_count(uint16_t port_id, uint16_= t queue_id) > > return rc; > > } > > > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior = notice > > + * > > + * Get maximum speed lanes supported by the NIC. > > > > Isn't this API to get the current lane number? > > > + * > > + * @param port_id > > + * The port identifier of the Ethernet device. > > + * @param speed_lanes_capa > > + * speed_lanes_capa is out only with max speed lanes capabilities. > > + * If set to NULL, then its assumed zero or not supported. > > > > Why NULL 'capa' is supported? > > > + * > > + * @return > > + * - A non-negative value of active lanes that currently link is up = with. > > + * - A non-negative value that this HW scales up to for all speeds. > > > > Isn't the return value only for status, error or success, and data > stored in 'capa' pointer? > > > > + * - (-ENOTSUP) if underlying hardware OR driver doesn't support. > > + * that operation. > > + * - (-EIO) if device is removed. > > + * - (-ENODEV) if *port_id* invalid. > > + * - (-EINVAL) if *speed_lanes_capa* invalid > > > > This is 'get' function, how 'speed_lanes_capa' can be invalid? > > > + */ > > +__rte_experimental > > +int rte_eth_speed_lanes_get(uint16_t port_id, struct rte_eth_speed_lan= es_capa *capa); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior = notice > > + * > > + * Set speed lanes supported by the NIC. > > > > Should we document somewhere that this is only for the case Auto > Negotiation (AN) is disabled, otherwise AN will figure out the lanes. > yes. may be in the command help, we can describe this. > > + * > > + * @param port_id > > + * The port identifier of the Ethernet device. > > + * @param speed_lanes > > + * speed_lanes a non-zero value of number lanes for this speeds. > > > > Please reword 'this speeds' > > > + * > > + * @return > > + * - (>=3D0) valid input and supported by driver or hardware. > > > > Lanes set successfully? > > > > + * - (-ENOTSUP) if underlying hardware OR driver doesn't support. > > + * that operation. > > + * - (-EIO) if device is removed. > > + * - (-ENODEV) if *port_id* invalid. > > + * - (-EINVAL) if *speed_lanes* invalid > > + */ > > +__rte_experimental > > +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_cap= a); > > + > > #ifdef __cplusplus > > } > > #endif > > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map > > index 79f6f5293b..9c27980f3a 100644 > > --- a/lib/ethdev/version.map > > +++ b/lib/ethdev/version.map > > @@ -325,6 +325,8 @@ EXPERIMENTAL { > > rte_flow_template_table_resizable; > > rte_flow_template_table_resize; > > rte_flow_template_table_resize_complete; > > + rte_eth_speed_lanes_get; > > + rte_eth_speed_lanes_set; > > > > Please follow the syntax in the file, add "# added in 24.07" comment and > add new APIs under it alphabetically sorted way. > As you suggested, I have consolidated all the 4 patches (testpmd + lib) into one single patch and addressed the comments in my local patch. If you can comment on the function name changes, will update and push a new revision soon. Thanks Damo --=20 This electronic communication and the information and any files transmitted= =20 with it, or attached to it, are confidential and are intended solely for=20 the use of the individual or entity to whom it is addressed and may contain= =20 information that is confidential, legally privileged, protected by privacy= =20 laws, or otherwise restricted from disclosure to anyone else. If you are=20 not the intended recipient or the person responsible for delivering the=20 e-mail to the intended recipient, you are hereby notified that any use,=20 copying, distributing, dissemination, forwarding, printing, or copying of= =20 this e-mail is strictly prohibited. If you received this e-mail in error,= =20 please return the e-mail to the sender, delete it from your computer, and= =20 destroy any printed copy of it.