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.y...@intel.com>
Signed-off-by: Mingjin Ye<mingjinx...@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
