IGC: Remove I225_I_PHY_ID checking
Checks
Commit Message
From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
i225 devices have only one PHY vendor. There is unnecessary to check _I_PHY_ID during the link establishment
and auto-negotiation process, the checking also caused devices like i225-IT failed. This patch is to remove the
mentioned unnecessary checking.
Cc: stable@dpdk.org
Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
---
drivers/net/igc/base/igc_api.c | 1 +
drivers/net/igc/base/igc_hw.h | 1 +
drivers/net/igc/base/igc_i225.c | 15 ++-------------
drivers/net/igc/base/igc_phy.c | 6 ++----
drivers/net/igc/igc_ethdev.c | 1 +
5 files changed, 7 insertions(+), 17 deletions(-)
Comments
> -----Original Message-----
> From: iotg.dpdk.ref.app@intel.com <iotg.dpdk.ref.app@intel.com>
> Sent: Monday, August 29, 2022 4:15 PM
> To: dev@dpdk.org
> Subject: [PATCH] IGC: Remove I225_I_PHY_ID checking
>
> From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
>
> i225 devices have only one PHY vendor. There is unnecessary to check
> _I_PHY_ID during the link establishment and auto-negotiation process, the
> checking also caused devices like i225-IT failed. This patch is to remove the
> mentioned unnecessary checking.
>
> Cc: stable@dpdk.org
> Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
Is this the expected author name?
> ---
> drivers/net/igc/base/igc_api.c | 1 +
> drivers/net/igc/base/igc_hw.h | 1 +
> drivers/net/igc/base/igc_i225.c | 15 ++-------------
> drivers/net/igc/base/igc_phy.c | 6 ++----
> drivers/net/igc/igc_ethdev.c | 1 +
> 5 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/igc/base/igc_api.c b/drivers/net/igc/base/igc_api.c
> index 9b791dc082..c9fc9ed4b0 100644
> --- a/drivers/net/igc/base/igc_api.c
> +++ b/drivers/net/igc/base/igc_api.c
> @@ -886,6 +886,7 @@ s32 igc_set_mac_type(struct igc_hw *hw)
> case IGC_DEV_ID_I225_V:
> case IGC_DEV_ID_I225_K:
> case IGC_DEV_ID_I225_I:
> + case IGC_DEV_ID_I225_IT:
> case IGC_DEV_ID_I220_V:
> case IGC_DEV_ID_I225_BLANK_NVM:
> case IGC_DEV_ID_I226_K:
> diff --git a/drivers/net/igc/base/igc_hw.h b/drivers/net/igc/base/igc_hw.h
> index 707a1883b4..e919a11c02 100644
> --- a/drivers/net/igc/base/igc_hw.h
> +++ b/drivers/net/igc/base/igc_hw.h
> @@ -164,6 +164,7 @@ struct igc_hw;
> #define IGC_DEV_ID_I225_V 0x15F3
> #define IGC_DEV_ID_I225_K 0x3100
> #define IGC_DEV_ID_I225_I 0x15F8
> +#define IGC_DEV_ID_I225_IT 0x0D9F
The patch's commit log claims to remove something,
but in code it also add some new device ID, could you clarify why we need this or it should be in a separate patch?
>-----Original Message-----
>From: Zhang, Qi Z <qi.z.zhang@intel.com>
>Sent: Tuesday, 30 August, 2022 7:17 PM
>To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; dev@dpdk.org
>Subject: RE: [PATCH] IGC: Remove I225_I_PHY_ID checking
> -----Original Message-----
> From: iotg.dpdk.ref.app@intel.com <iotg.dpdk.ref.app@intel.com>
> Sent: Monday, August 29, 2022 4:15 PM
> To: dev@dpdk.org
> Subject: [PATCH] IGC: Remove I225_I_PHY_ID checking
>
> From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
>
> i225 devices have only one PHY vendor. There is unnecessary to check
> _I_PHY_ID during the link establishment and auto-negotiation process,
> the checking also caused devices like i225-IT failed. This patch is to
> remove the mentioned unnecessary checking.
>
> Cc: stable@dpdk.org
> Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
>Is this the expected author name?
Yes, this is expected author name, the PDL email contain all developers working on the patch for better support and response.
> ---
> drivers/net/igc/base/igc_api.c | 1 +
> drivers/net/igc/base/igc_hw.h | 1 +
> drivers/net/igc/base/igc_i225.c | 15 ++-------------
> drivers/net/igc/base/igc_phy.c | 6 ++----
> drivers/net/igc/igc_ethdev.c | 1 +
> 5 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/igc/base/igc_api.c
> b/drivers/net/igc/base/igc_api.c index 9b791dc082..c9fc9ed4b0 100644
> --- a/drivers/net/igc/base/igc_api.c
> +++ b/drivers/net/igc/base/igc_api.c
> @@ -886,6 +886,7 @@ s32 igc_set_mac_type(struct igc_hw *hw)
> case IGC_DEV_ID_I225_V:
> case IGC_DEV_ID_I225_K:
> case IGC_DEV_ID_I225_I:
> + case IGC_DEV_ID_I225_IT:
> case IGC_DEV_ID_I220_V:
> case IGC_DEV_ID_I225_BLANK_NVM:
> case IGC_DEV_ID_I226_K:
> diff --git a/drivers/net/igc/base/igc_hw.h
> b/drivers/net/igc/base/igc_hw.h index 707a1883b4..e919a11c02 100644
> --- a/drivers/net/igc/base/igc_hw.h
> +++ b/drivers/net/igc/base/igc_hw.h
> @@ -164,6 +164,7 @@ struct igc_hw;
> #define IGC_DEV_ID_I225_V 0x15F3
> #define IGC_DEV_ID_I225_K 0x3100
> #define IGC_DEV_ID_I225_I 0x15F8
> +#define IGC_DEV_ID_I225_IT 0x0D9F
>The patch's commit log claims to remove something, but in code it also add some new device ID, could you clarify why we need this or it should be in a separate patch?
The checking issue was firstly discovered because of i225-IT not working on our lab and escalated to DPDK team for support, as informed by DPDK team after the patch sent, that the i225-IT device ID has been patched in DPDK next release, we'll send out v2 patch soon with only removing redundance checking without i225-IT device ID.
On Thu, Sep 1, 2022 at 10:08 AM Mah, Yock Gen <yock.gen.mah@intel.com> wrote:
>
> >-----Original Message-----
> >From: Zhang, Qi Z <qi.z.zhang@intel.com>
> >Sent: Tuesday, 30 August, 2022 7:17 PM
> >To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; dev@dpdk.org
> >Subject: RE: [PATCH] IGC: Remove I225_I_PHY_ID checking
>
> > -----Original Message-----
> > From: iotg.dpdk.ref.app@intel.com <iotg.dpdk.ref.app@intel.com>
> > Sent: Monday, August 29, 2022 4:15 PM
> > To: dev@dpdk.org
> > Subject: [PATCH] IGC: Remove I225_I_PHY_ID checking
> >
> > From: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
> >
> > i225 devices have only one PHY vendor. There is unnecessary to check
> > _I_PHY_ID during the link establishment and auto-negotiation process,
> > the checking also caused devices like i225-IT failed. This patch is to
> > remove the mentioned unnecessary checking.
> >
> > Cc: stable@dpdk.org
> > Signed-off-by: NSWE SWS DPDK Dev <iotg.dpdk.ref.app@intel.com>
>
> >Is this the expected author name?
> Yes, this is expected author name, the PDL email contain all developers working on the patch for better support and response.
This is not acceptable.
https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body
It is clearly stated that:
"""
The signoff must be a real name and not an alias or nickname. More
than one signoff is allowed.
"""
And all developers involved in the making of a patch must follow the
Developer’s Certificate of Origin.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1
In its current form, this patch cannot be merged in the DPDK project.
Besides, please register to the @dev mailing list.
On Thu, Sep 1, 2022 at 10:22 AM David Marchand
<david.marchand@redhat.com> wrote:
> Besides, please register to the @dev mailing list.
Bis.
Please register to the @dev mailing list.
@@ -886,6 +886,7 @@ s32 igc_set_mac_type(struct igc_hw *hw)
case IGC_DEV_ID_I225_V:
case IGC_DEV_ID_I225_K:
case IGC_DEV_ID_I225_I:
+ case IGC_DEV_ID_I225_IT:
case IGC_DEV_ID_I220_V:
case IGC_DEV_ID_I225_BLANK_NVM:
case IGC_DEV_ID_I226_K:
@@ -164,6 +164,7 @@ struct igc_hw;
#define IGC_DEV_ID_I225_V 0x15F3
#define IGC_DEV_ID_I225_K 0x3100
#define IGC_DEV_ID_I225_I 0x15F8
+#define IGC_DEV_ID_I225_IT 0x0D9F
#define IGC_DEV_ID_I220_V 0x15F7
#define IGC_DEV_ID_I225_BLANK_NVM 0x15FD
#define IGC_DEV_ID_I226_K 0x3102
@@ -173,19 +173,8 @@ static s32 igc_init_phy_params_i225(struct igc_hw *hw)
phy->ops.write_reg = igc_write_phy_reg_gpy;
ret_val = igc_get_phy_id(hw);
- /* Verify phy id and set remaining function pointers */
- switch (phy->id) {
- case I225_I_PHY_ID:
- case I226_LM_PHY_ID:
- phy->type = igc_phy_i225;
- phy->ops.set_d0_lplu_state = igc_set_d0_lplu_state_i225;
- phy->ops.set_d3_lplu_state = igc_set_d3_lplu_state_i225;
- /* TODO - complete with GPY PHY information */
- break;
- default:
- ret_val = -IGC_ERR_PHY;
- goto out;
- }
+
+ phy->type = igc_phy_i225;
out:
return ret_val;
@@ -1474,8 +1474,7 @@ s32 igc_phy_setup_autoneg(struct igc_hw *hw)
return ret_val;
}
- if ((phy->autoneg_mask & ADVERTISE_2500_FULL) &&
- hw->phy.id == I225_I_PHY_ID) {
+ if (phy->autoneg_mask & ADVERTISE_2500_FULL) {
/* Read the MULTI GBT AN Control Register - reg 7.32 */
ret_val = phy->ops.read_reg(hw, (STANDARD_AN_REG_MASK <<
MMD_DEVADDR_SHIFT) |
@@ -1615,8 +1614,7 @@ s32 igc_phy_setup_autoneg(struct igc_hw *hw)
ret_val = phy->ops.write_reg(hw, PHY_1000T_CTRL,
mii_1000t_ctrl_reg);
- if ((phy->autoneg_mask & ADVERTISE_2500_FULL) &&
- hw->phy.id == I225_I_PHY_ID)
+ if (phy->autoneg_mask & ADVERTISE_2500_FULL)
ret_val = phy->ops.write_reg(hw,
(STANDARD_AN_REG_MASK <<
MMD_DEVADDR_SHIFT) |
@@ -97,6 +97,7 @@ static const struct rte_pci_id pci_id_igc_map[] = {
{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_V) },
{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_I) },
{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_K) },
+ { RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I225_IT) },
{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I226_K) },
{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I226_LMVP) },
{ RTE_PCI_DEVICE(IGC_INTEL_VENDOR_ID, IGC_DEV_ID_I226_LM) },