From: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>
To: Mingjin Ye <mingjinx.ye@intel.com>, <dev@dpdk.org>
Cc: Qiming Yang <qiming.yang@intel.com>
Subject: Re: [PATCH v2] net/ice: support FEC feature
Date: Wed, 3 Jul 2024 19:31:17 +0100 [thread overview]
Message-ID: <6d9ad0c4-e5bf-47a0-b14d-512689262922@intel.com> (raw)
In-Reply-To: <20240702080244.1190884-1-mingjinx.ye@intel.com>
[-- Attachment #1: Type: text/plain, Size: 7130 bytes --]
Hi Minjin,
- please update release notes
- see comments inline
On 02/07/2024 09:02, Mingjin Ye wrote:
> This patch enable three Forward Error Correction(FEC) related ops
> in ice driver. As no speed information can get from HW, this patch
> only show FEC capability.
>
> Signed-off-by: Qiming Yang<qiming.yang@intel.com>
> Signed-off-by: Mingjin Ye<mingjinx.ye@intel.com>
> ---
> v2: fix some logic
> ---
> doc/guides/nics/features/ice.ini | 1 +
> doc/guides/nics/ice.rst | 5 +
> drivers/net/ice/ice_ethdev.c | 292 +++++++++++++++++++++++++++++++
> 3 files changed, 298 insertions(+)
>
<snip>
> +static int
> +ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa *speed_fec_capa,
> + unsigned int num)
> +{
> + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + struct ice_aqc_get_phy_caps_data pcaps = {0};
> + unsigned int capa_num;
> + int ret;
> +
> + ret = ice_aq_get_phy_caps(hw->port_info, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
> + &pcaps, NULL);
> + if (ret != ICE_SUCCESS)
> + goto done;
This breaks API. Please redefine with some supported error status
And please get rid of goto here, it is not necessary anymore
> +
> + /* first time to get capa_num */
> + capa_num = ice_fec_get_capa_num(&pcaps, NULL);
> + if (!speed_fec_capa || num < capa_num) {
> + ret = capa_num;
> + goto done;
> + }
> +
> + ret = ice_fec_get_capa_num(&pcaps, speed_fec_capa);
> +
> +done:
> + return ret;
> +}
> +
> +static int
> +ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
> +{
> +#define FEC_CAPA_NUM 10
I believe this macro is not needed
> + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> + bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
> + bool link_up;
> + u32 temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> + struct ice_link_status link_status = {0};
> + struct ice_aqc_get_phy_caps_data pcaps = {0};
> + struct ice_port_info *pi = hw->port_info;
> + u8 fec_config;
> + int ret;
> +
> + if (!pi)
> + return -ENOTSUP;
> +
> + ret = ice_get_link_info_safe(pf, enable_lse, &link_status);
> + if (ret != ICE_SUCCESS) {
> + PMD_DRV_LOG(ERR, "Failed to get link information: %d\n",
> + ret);
> + goto done;
Same problem here and below with API and returning error statuses. And I
think it is worth to get rid of goto here in this function as well.
> + }
> +
> + link_up = link_status.link_info & ICE_AQ_LINK_UP;
> +
> + ret = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
> + &pcaps, NULL);
> + if (ret != ICE_SUCCESS)
> + goto done;
> +
> + /* Get current FEC mode from port info */
> + if (link_up) {
> + switch (link_status.fec_info) {
> + case ICE_AQ_LINK_25G_KR_FEC_EN:
> + temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> + break;
> + case ICE_AQ_LINK_25G_RS_528_FEC_EN:
> + /* fall-through */
> + case ICE_AQ_LINK_25G_RS_544_FEC_EN:
> + temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> + break;
> + default:
> + temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> + break;
> + }
> + goto done;
> + }
> +
> + if (pcaps.caps & ICE_AQC_PHY_EN_AUTO_FEC) {
> + temp_fec_capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> + return 0;
> + }
> +
> + fec_config = pcaps.link_fec_options & ICE_AQC_PHY_FEC_MASK;
> +
> + if (fec_config & (ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> + ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> + ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> + ICE_AQC_PHY_FEC_25G_KR_REQ))
> + temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +
> + if (fec_config & (ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> + ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> + ICE_AQC_PHY_FEC_25G_RS_544_REQ))
> + temp_fec_capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +
> +done:
> + *fec_capa = temp_fec_capa;
> + return ret;
> +}
> +
> +static int
> +ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa)
> +{
> + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + struct ice_port_info *pi = hw->port_info;
> + struct ice_aqc_set_phy_cfg_data cfg = { 0 };
> + bool fec_auto = false, fec_kr = false, fec_rs = false;
> +
> + if (!pi)
> + return -ENOTSUP;
> +
> + if (fec_capa & ~(RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
> + RTE_ETH_FEC_MODE_CAPA_MASK(RS)))
please use additional tab to indent properly according to dpdk coding style
> + return -EINVAL;
> + /* Copy the current user PHY configuration. The current user PHY
> + * configuration is initialized during probe from PHY capabilities
> + * software mode, and updated on set PHY configuration.
> + */
> + memcpy(&cfg, &pi->phy.curr_user_phy_cfg, sizeof(cfg));
> +
> + if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
> + fec_auto = true;
> +
> + if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> + fec_kr = true;
> +
> + if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
> + fec_rs = true;
> +
> + if (fec_auto) {
> + if (fec_kr || fec_rs) {
> + if (fec_rs) {
> + cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> + ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> + ICE_AQC_PHY_FEC_25G_RS_544_REQ;
> + }
> + if (fec_kr) {
> + cfg.link_fec_opt |= ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> + ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> + ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> + ICE_AQC_PHY_FEC_25G_KR_REQ;
> + }
> + } else {
> + cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> + ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> + ICE_AQC_PHY_FEC_25G_RS_544_REQ |
> + ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> + ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> + ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> + ICE_AQC_PHY_FEC_25G_KR_REQ;
> + }
> + } else {
> + if (fec_kr ^ fec_rs) {
> + if (fec_rs) {
> + cfg.link_fec_opt = ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN |
> + ICE_AQC_PHY_FEC_25G_RS_528_REQ |
> + ICE_AQC_PHY_FEC_25G_RS_544_REQ;
> + } else {
> + cfg.link_fec_opt = ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN |
> + ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN |
> + ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ |
> + ICE_AQC_PHY_FEC_25G_KR_REQ;
> + }
> + } else {
> + return -EINVAL;
> + }
> + }
> +
> + /* Recovery of accidentally rewritten bit */
> + if (pi->phy.curr_user_phy_cfg.link_fec_opt &
> + ~ICE_AQC_PHY_FEC_MASK)
same indentation issue here, please add an extra tab
also it would be a bit more readable if you test link_fec_opt against
ICE_AQC_PHY_FEC_DIS instead of ~ICE_AQC_PHY_FEC_MASK. Additionally no
need to clean up this flag in cfg if it wasn't there in the first place.
> + cfg.link_fec_opt |= ICE_AQC_PHY_FEC_DIS;
> + else
> + cfg.link_fec_opt &= ICE_AQC_PHY_FEC_MASK;
> +
> + /* Proceed only if requesting different FEC mode */
> + if (pi->phy.curr_user_phy_cfg.link_fec_opt == cfg.link_fec_opt)
> + return 0;
> +
> + cfg.caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
> +
> + if (ice_aq_set_phy_cfg(pi->hw, pi, &cfg, NULL))
> + return -EAGAIN;
> +
> + return 0;
> +}
> +
> static int
> ice_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> struct rte_pci_device *pci_dev)
--
Regards,
Vladimir
[-- Attachment #2: Type: text/html, Size: 8517 bytes --]
next prev parent reply other threads:[~2024-07-03 18:31 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 9:45 [PATCH] " Mingjin Ye
2024-06-27 17:07 ` Medvedkin, Vladimir
2024-07-02 8:02 ` [PATCH v2] " Mingjin Ye
2024-07-03 18:31 ` Medvedkin, Vladimir [this message]
2024-07-04 6:50 ` [PATCH v3] " Mingjin Ye
2024-07-04 12:00 ` Medvedkin, Vladimir
2024-07-04 12:37 ` Bruce Richardson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6d9ad0c4-e5bf-47a0-b14d-512689262922@intel.com \
--to=vladimir.medvedkin@intel.com \
--cc=dev@dpdk.org \
--cc=mingjinx.ye@intel.com \
--cc=qiming.yang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).