* [PATCH] IGC: Remove I225_I_PHY_ID checking
@ 2022-08-29 8:14 iotg.dpdk.ref.app
2022-08-30 11:17 ` Zhang, Qi Z
2022-08-31 22:51 ` [PATCH v2] " iotg.dpdk.ref.app
0 siblings, 2 replies; 16+ messages in thread
From: iotg.dpdk.ref.app @ 2022-08-29 8:14 UTC (permalink / raw)
To: dev
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(-)
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
#define IGC_DEV_ID_I220_V 0x15F7
#define IGC_DEV_ID_I225_BLANK_NVM 0x15FD
#define IGC_DEV_ID_I226_K 0x3102
diff --git a/drivers/net/igc/base/igc_i225.c b/drivers/net/igc/base/igc_i225.c
index 5f3d535490..bdc6f74976 100644
--- a/drivers/net/igc/base/igc_i225.c
+++ b/drivers/net/igc/base/igc_i225.c
@@ -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;
diff --git a/drivers/net/igc/base/igc_phy.c b/drivers/net/igc/base/igc_phy.c
index 43bbe69bca..2906bae21a 100644
--- a/drivers/net/igc/base/igc_phy.c
+++ b/drivers/net/igc/base/igc_phy.c
@@ -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) |
diff --git a/drivers/net/igc/igc_ethdev.c b/drivers/net/igc/igc_ethdev.c
index 7f221a5d34..2989b8d488 100644
--- a/drivers/net/igc/igc_ethdev.c
+++ b/drivers/net/igc/igc_ethdev.c
@@ -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) },
--
2.36.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] IGC: Remove I225_I_PHY_ID checking
2022-08-29 8:14 [PATCH] IGC: Remove I225_I_PHY_ID checking iotg.dpdk.ref.app
@ 2022-08-30 11:17 ` Zhang, Qi Z
2022-08-31 22:42 ` Mah, Yock Gen
2022-08-31 22:51 ` [PATCH v2] " iotg.dpdk.ref.app
1 sibling, 1 reply; 16+ messages in thread
From: Zhang, Qi Z @ 2022-08-30 11:17 UTC (permalink / raw)
To: IOTG DPDK Ref App, dev
> -----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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] IGC: Remove I225_I_PHY_ID checking
2022-08-30 11:17 ` Zhang, Qi Z
@ 2022-08-31 22:42 ` Mah, Yock Gen
2022-09-01 8:22 ` David Marchand
0 siblings, 1 reply; 16+ messages in thread
From: Mah, Yock Gen @ 2022-08-31 22:42 UTC (permalink / raw)
To: Zhang, Qi Z, IOTG DPDK Ref App, dev
>-----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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-08-29 8:14 [PATCH] IGC: Remove I225_I_PHY_ID checking iotg.dpdk.ref.app
2022-08-30 11:17 ` Zhang, Qi Z
@ 2022-08-31 22:51 ` iotg.dpdk.ref.app
2022-09-02 0:18 ` [PATCH v3] " yock.gen.mah
2022-10-11 11:04 ` [PATCH v2] " Kevin Traynor
1 sibling, 2 replies; 16+ messages in thread
From: iotg.dpdk.ref.app @ 2022-08-31 22:51 UTC (permalink / raw)
To: qi.z.zhang; +Cc: dev
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_i225.c | 15 ++-------------
drivers/net/igc/base/igc_phy.c | 6 ++----
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/drivers/net/igc/base/igc_i225.c b/drivers/net/igc/base/igc_i225.c
index 5f3d535490..af26602afb 100644
--- a/drivers/net/igc/base/igc_i225.c
+++ b/drivers/net/igc/base/igc_i225.c
@@ -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;
diff --git a/drivers/net/igc/base/igc_phy.c b/drivers/net/igc/base/igc_phy.c
index 43bbe69bca..2906bae21a 100644
--- a/drivers/net/igc/base/igc_phy.c
+++ b/drivers/net/igc/base/igc_phy.c
@@ -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) |
--
2.36.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] IGC: Remove I225_I_PHY_ID checking
2022-08-31 22:42 ` Mah, Yock Gen
@ 2022-09-01 8:22 ` David Marchand
2022-09-02 7:01 ` David Marchand
0 siblings, 1 reply; 16+ messages in thread
From: David Marchand @ 2022-09-01 8:22 UTC (permalink / raw)
To: Mah, Yock Gen, Zhang, Qi Z
Cc: IOTG DPDK Ref App, dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
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.
--
David Marchand
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] IGC: Remove I225_I_PHY_ID checking
2022-08-31 22:51 ` [PATCH v2] " iotg.dpdk.ref.app
@ 2022-09-02 0:18 ` yock.gen.mah
2022-09-04 1:55 ` Zhang, Qi Z
2022-10-11 11:04 ` [PATCH v2] " Kevin Traynor
1 sibling, 1 reply; 16+ messages in thread
From: yock.gen.mah @ 2022-09-02 0:18 UTC (permalink / raw)
To: qi.z.zhang; +Cc: dev
From: Mah Yock Gen <yock.gen.mah@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
<changelog>
Co-developed-by: Mah Yock Gen <yock.gen.mah@intel.com>
Signed-off-by: Mah Yock Gen <yock.gen.mah@intel.com>
Co-developed-by: Taripin Samuel <samuel.taripin@intel.com>
Signed-off-by: Taripin Samuel <samuel.taripin@intel.com>
Signed-off-by: Mah Yock Gen <yock.gen.mah@intel.com>
---
V3:
* Fixed coding style issue
V2:
* Removing i225-IT device id added in V1 patch, as it has been superseded
drivers/net/igc/base/igc_i225.c | 15 ++-------------
drivers/net/igc/base/igc_phy.c | 6 ++----
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/drivers/net/igc/base/igc_i225.c b/drivers/net/igc/base/igc_i225.c
index 5f3d535490..180d3cf687 100644
--- a/drivers/net/igc/base/igc_i225.c
+++ b/drivers/net/igc/base/igc_i225.c
@@ -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;
diff --git a/drivers/net/igc/base/igc_phy.c b/drivers/net/igc/base/igc_phy.c
index 43bbe69bca..2906bae21a 100644
--- a/drivers/net/igc/base/igc_phy.c
+++ b/drivers/net/igc/base/igc_phy.c
@@ -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) |
--
2.36.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] IGC: Remove I225_I_PHY_ID checking
2022-09-01 8:22 ` David Marchand
@ 2022-09-02 7:01 ` David Marchand
0 siblings, 0 replies; 16+ messages in thread
From: David Marchand @ 2022-09-02 7:01 UTC (permalink / raw)
To: Mah, Yock Gen, Zhang, Qi Z
Cc: IOTG DPDK Ref App, dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
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.
--
David Marchand
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v3] IGC: Remove I225_I_PHY_ID checking
2022-09-02 0:18 ` [PATCH v3] " yock.gen.mah
@ 2022-09-04 1:55 ` Zhang, Qi Z
0 siblings, 0 replies; 16+ messages in thread
From: Zhang, Qi Z @ 2022-09-04 1:55 UTC (permalink / raw)
To: Mah, Yock Gen; +Cc: dev
> -----Original Message-----
> From: Mah, Yock Gen <yock.gen.mah@intel.com>
> Sent: Friday, September 2, 2022 8:18 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: [PATCH v3] IGC: Remove I225_I_PHY_ID checking
>
> From: Mah Yock Gen <yock.gen.mah@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
>
> <changelog>
>
> Co-developed-by: Mah Yock Gen <yock.gen.mah@intel.com>
> Signed-off-by: Mah Yock Gen <yock.gen.mah@intel.com>
> Co-developed-by: Taripin Samuel <samuel.taripin@intel.com>
> Signed-off-by: Taripin Samuel <samuel.taripin@intel.com>
> Signed-off-by: Mah Yock Gen <yock.gen.mah@intel.com>
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
Applied to dpdk-next-net-intel with below minor changes.
1. rename title to "remove unnecessary PHY ID checking" to fix check-git-log warning
2. remove "<change log>" tag
3. remove duplicated "Signed-off-by"
Thanks
Qi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-08-31 22:51 ` [PATCH v2] " iotg.dpdk.ref.app
2022-09-02 0:18 ` [PATCH v3] " yock.gen.mah
@ 2022-10-11 11:04 ` Kevin Traynor
2022-10-12 7:45 ` Mah, Yock Gen
1 sibling, 1 reply; 16+ messages in thread
From: Kevin Traynor @ 2022-10-11 11:04 UTC (permalink / raw)
To: iotg.dpdk.ref.app, qi.z.zhang; +Cc: dev
On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
> 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_i225.c | 15 ++-------------
> drivers/net/igc/base/igc_phy.c | 6 ++----
> 2 files changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/igc/base/igc_i225.c b/drivers/net/igc/base/igc_i225.c
> index 5f3d535490..af26602afb 100644
> --- a/drivers/net/igc/base/igc_i225.c
> +++ b/drivers/net/igc/base/igc_i225.c
> @@ -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;
The commit log says it is removing a check on the ID, but it does not
say why these function pointers are being removed.
Why are they removed, were they not needed?
> - /* TODO - complete with GPY PHY information */
> - break;
> - default:
> - ret_val = -IGC_ERR_PHY;
> - goto out;
> - }
> + phy->type = igc_phy_i225;
> +
>
> out:
> return ret_val;
> diff --git a/drivers/net/igc/base/igc_phy.c b/drivers/net/igc/base/igc_phy.c
> index 43bbe69bca..2906bae21a 100644
> --- a/drivers/net/igc/base/igc_phy.c
> +++ b/drivers/net/igc/base/igc_phy.c
> @@ -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) |
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-10-11 11:04 ` [PATCH v2] " Kevin Traynor
@ 2022-10-12 7:45 ` Mah, Yock Gen
2022-10-12 8:39 ` Kevin Traynor
0 siblings, 1 reply; 16+ messages in thread
From: Mah, Yock Gen @ 2022-10-12 7:45 UTC (permalink / raw)
To: Kevin Traynor, IOTG DPDK Ref App, Zhang, Qi Z; +Cc: dev
-----Original Message-----
From: Kevin Traynor <ktraynor@redhat.com>
Sent: Tuesday, 11 October, 2022 7:05 PM
To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
> 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_i225.c | 15 ++-------------
> drivers/net/igc/base/igc_phy.c | 6 ++----
> 2 files changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/igc/base/igc_i225.c
> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 100644
> --- a/drivers/net/igc/base/igc_i225.c
> +++ b/drivers/net/igc/base/igc_i225.c
> @@ -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;
> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
> - Why are they removed, were they not needed?
i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
> - /* TODO - complete with GPY PHY information */
> - break;
> - default:
> - ret_val = -IGC_ERR_PHY;
> - goto out;
> - }
> + phy->type = igc_phy_i225;
> +
>
> out:
> return ret_val;
> diff --git a/drivers/net/igc/base/igc_phy.c
> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644
> --- a/drivers/net/igc/base/igc_phy.c
> +++ b/drivers/net/igc/base/igc_phy.c
> @@ -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) |
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-10-12 7:45 ` Mah, Yock Gen
@ 2022-10-12 8:39 ` Kevin Traynor
2022-10-18 12:54 ` Kevin Traynor
0 siblings, 1 reply; 16+ messages in thread
From: Kevin Traynor @ 2022-10-12 8:39 UTC (permalink / raw)
To: Mah, Yock Gen, IOTG DPDK Ref App, Zhang, Qi Z; +Cc: dev
On 12/10/2022 08:45, Mah, Yock Gen wrote:
>
>
> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday, 11 October, 2022 7:05 PM
> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>
> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>> 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_i225.c | 15 ++-------------
>> drivers/net/igc/base/igc_phy.c | 6 ++----
>> 2 files changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/igc/base/igc_i225.c
>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 100644
>> --- a/drivers/net/igc/base/igc_i225.c
>> +++ b/drivers/net/igc/base/igc_i225.c
>> @@ -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;
>
>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>
>> - Why are they removed, were they not needed?
>
>
> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>
Right, that's clear about the vendor ID check. But it's not clear to me
why the the resulting code like this:
phy->type = igc_phy_i225;
and not like this:
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;
So it is using dummy null functions instead:
https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
Do the device registers not need to be set anymore?
>> - /* TODO - complete with GPY PHY information */
>> - break;
>> - default:
>> - ret_val = -IGC_ERR_PHY;
>> - goto out;
>> - }
>> + phy->type = igc_phy_i225;
>> +
>>
>> out:
>> return ret_val;
>> diff --git a/drivers/net/igc/base/igc_phy.c
>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644
>> --- a/drivers/net/igc/base/igc_phy.c
>> +++ b/drivers/net/igc/base/igc_phy.c
>> @@ -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) |
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-10-12 8:39 ` Kevin Traynor
@ 2022-10-18 12:54 ` Kevin Traynor
2022-10-18 22:45 ` Mah, Yock Gen
0 siblings, 1 reply; 16+ messages in thread
From: Kevin Traynor @ 2022-10-18 12:54 UTC (permalink / raw)
To: Mah, Yock Gen, IOTG DPDK Ref App, Zhang, Qi Z; +Cc: dev
On 12/10/2022 09:39, Kevin Traynor wrote:
> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>
>>
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday, 11 October, 2022 7:05 PM
>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>
>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>> 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_i225.c | 15 ++-------------
>>> drivers/net/igc/base/igc_phy.c | 6 ++----
>>> 2 files changed, 4 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/igc/base/igc_i225.c
>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb 100644
>>> --- a/drivers/net/igc/base/igc_i225.c
>>> +++ b/drivers/net/igc/base/igc_i225.c
>>> @@ -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;
>>
>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>
>>> - Why are they removed, were they not needed?
>>
>>
>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>
>
> Right, that's clear about the vendor ID check. But it's not clear to me
> why the the resulting code like this:
>
> phy->type = igc_phy_i225;
>
> and not like this:
>
> 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;
>
> So it is using dummy null functions instead:
> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
>
> Do the device registers not need to be set anymore?
>
For main branch, it would be nice to have an answer to above.
It only adds a small readability benefit by removing some code branches,
but does change functionality which adds risk, so I don't think it's a
good candidate for stable branches.
>>> - /* TODO - complete with GPY PHY information */
>>> - break;
>>> - default:
>>> - ret_val = -IGC_ERR_PHY;
>>> - goto out;
>>> - }
>>> + phy->type = igc_phy_i225;
>>> +
>>>
>>> out:
>>> return ret_val;
>>> diff --git a/drivers/net/igc/base/igc_phy.c
>>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644
>>> --- a/drivers/net/igc/base/igc_phy.c
>>> +++ b/drivers/net/igc/base/igc_phy.c
>>> @@ -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) |
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-10-18 12:54 ` Kevin Traynor
@ 2022-10-18 22:45 ` Mah, Yock Gen
2022-10-19 8:34 ` Kevin Traynor
0 siblings, 1 reply; 16+ messages in thread
From: Mah, Yock Gen @ 2022-10-18 22:45 UTC (permalink / raw)
To: Kevin Traynor, IOTG DPDK Ref App, Zhang, Qi Z, Taripin, Samuel; +Cc: dev
-----Original Message-----
From: Kevin Traynor <ktraynor@redhat.com>
Sent: Tuesday, 18 October, 2022 8:54 PM
To: Mah, Yock Gen <yock.gen.mah@intel.com>; IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
On 12/10/2022 09:39, Kevin Traynor wrote:
> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>
>>
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday, 11 October, 2022 7:05 PM
>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>
>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>> 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_i225.c | 15 ++-------------
>>> drivers/net/igc/base/igc_phy.c | 6 ++----
>>> 2 files changed, 4 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/igc/base/igc_i225.c
>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb
>>> 100644
>>> --- a/drivers/net/igc/base/igc_i225.c
>>> +++ b/drivers/net/igc/base/igc_i225.c
>>> @@ -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;
>>
>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>
>>> - Why are they removed, were they not needed?
>>
>>
>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>
>
> Right, that's clear about the vendor ID check. But it's not clear to
> me why the the resulting code like this:
>
> phy->type = igc_phy_i225;
>
> and not like this:
>
> 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;
>
> So it is using dummy null functions instead:
> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
>
> Do the device registers not need to be set anymore?
>
> For main branch, it would be nice to have an answer to above.
> It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches.
This is not only added readability, but to fix real world issue, we were experiencing i225-IT not runnable issue without patching another case checking as below:
+++ b/drivers/net/igc/base/igc_phy.c
@@ -1881,6 +1881,7 @@ s32 igc_phy_force_speed_duplex_m88(struct igc_hw *hw)
case I210_I_PHY_ID:
/* fall-through */
case I225_I_PHY_ID:
+ case I225_IT_PHY_ID:
However, cleaner solution is to remove those unnecessary checking completely as it does in kernel also https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7c496de538eebd8212dc2a3c9a468386b264d0d4
>>> - /* TODO - complete with GPY PHY information */
>>> - break;
>>> - default:
>>> - ret_val = -IGC_ERR_PHY;
>>> - goto out;
>>> - }
>>> + phy->type = igc_phy_i225;
>>> +
>>>
>>> out:
>>> return ret_val;
>>> diff --git a/drivers/net/igc/base/igc_phy.c
>>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644
>>> --- a/drivers/net/igc/base/igc_phy.c
>>> +++ b/drivers/net/igc/base/igc_phy.c
>>> @@ -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) |
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-10-18 22:45 ` Mah, Yock Gen
@ 2022-10-19 8:34 ` Kevin Traynor
2022-12-20 15:47 ` Kevin Traynor
0 siblings, 1 reply; 16+ messages in thread
From: Kevin Traynor @ 2022-10-19 8:34 UTC (permalink / raw)
To: Mah, Yock Gen, IOTG DPDK Ref App, Zhang, Qi Z, Taripin, Samuel; +Cc: dev
On 18/10/2022 23:45, Mah, Yock Gen wrote:
>
>
> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday, 18 October, 2022 8:54 PM
> To: Mah, Yock Gen <yock.gen.mah@intel.com>; IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>
> On 12/10/2022 09:39, Kevin Traynor wrote:
>> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>>
>>>
>>> -----Original Message-----
>>> From: Kevin Traynor <ktraynor@redhat.com>
>>> Sent: Tuesday, 11 October, 2022 7:05 PM
>>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z
>>> <qi.z.zhang@intel.com>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>>
>>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>>> 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_i225.c | 15 ++-------------
>>>> drivers/net/igc/base/igc_phy.c | 6 ++----
>>>> 2 files changed, 4 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/net/igc/base/igc_i225.c
>>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb
>>>> 100644
>>>> --- a/drivers/net/igc/base/igc_i225.c
>>>> +++ b/drivers/net/igc/base/igc_i225.c
>>>> @@ -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;
>>>
>>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>>
>>>> - Why are they removed, were they not needed?
>>>
>>>
>>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>>
>>
>> Right, that's clear about the vendor ID check. But it's not clear to
>> me why the the resulting code like this:
>>
>> phy->type = igc_phy_i225;
>>
>> and not like this:
>>
>> 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;
>>
>> So it is using dummy null functions instead:
>> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
>>
>> Do the device registers not need to be set anymore?
>>
>
>> For main branch, it would be nice to have an answer to above.
>
>> It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches.
>
> This is not only added readability, but to fix real world issue, we were experiencing i225-IT not runnable issue without patching another case checking as below:
>
> +++ b/drivers/net/igc/base/igc_phy.c
> @@ -1881,6 +1881,7 @@ s32 igc_phy_force_speed_duplex_m88(struct igc_hw *hw)
> case I210_I_PHY_ID:
> /* fall-through */
> case I225_I_PHY_ID:
> + case I225_IT_PHY_ID:
>
> However, cleaner solution is to remove those unnecessary checking completely as it does in kernel also https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7c496de538eebd8212dc2a3c9a468386b264d0d4
>
ok, you are correct that it is more than readability. I can apply but
first it would be good if you can answer the original question and
explain why the functionality is changed for I225_I_PHY_ID by removing
the function pointers that set the registers.
>>>> - /* TODO - complete with GPY PHY information */
>>>> - break;
>>>> - default:
>>>> - ret_val = -IGC_ERR_PHY;
>>>> - goto out;
>>>> - }
>>>> + phy->type = igc_phy_i225;
>>>> +
>>>>
>>>> out:
>>>> return ret_val;
>>>> diff --git a/drivers/net/igc/base/igc_phy.c
>>>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644
>>>> --- a/drivers/net/igc/base/igc_phy.c
>>>> +++ b/drivers/net/igc/base/igc_phy.c
>>>> @@ -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) |
>>>
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-10-19 8:34 ` Kevin Traynor
@ 2022-12-20 15:47 ` Kevin Traynor
2022-12-21 3:01 ` Mah, Yock Gen
0 siblings, 1 reply; 16+ messages in thread
From: Kevin Traynor @ 2022-12-20 15:47 UTC (permalink / raw)
To: Mah, Yock Gen, IOTG DPDK Ref App, Zhang, Qi Z, Taripin, Samuel; +Cc: dev
On 19/10/2022 09:34, Kevin Traynor wrote:
> On 18/10/2022 23:45, Mah, Yock Gen wrote:
>>
>>
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday, 18 October, 2022 8:54 PM
>> To: Mah, Yock Gen <yock.gen.mah@intel.com>; IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>
>> On 12/10/2022 09:39, Kevin Traynor wrote:
>>> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>> Sent: Tuesday, 11 October, 2022 7:05 PM
>>>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>>>
>>>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>>>> 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_i225.c | 15 ++-------------
>>>>> drivers/net/igc/base/igc_phy.c | 6 ++----
>>>>> 2 files changed, 4 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/igc/base/igc_i225.c
>>>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb
>>>>> 100644
>>>>> --- a/drivers/net/igc/base/igc_i225.c
>>>>> +++ b/drivers/net/igc/base/igc_i225.c
>>>>> @@ -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;
>>>>
>>>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>>>
>>>>> - Why are they removed, were they not needed?
>>>>
>>>>
>>>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>>>
>>>
>>> Right, that's clear about the vendor ID check. But it's not clear to
>>> me why the the resulting code like this:
>>>
>>> phy->type = igc_phy_i225;
>>>
>>> and not like this:
>>>
>>> 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;
>>>
>>> So it is using dummy null functions instead:
>>> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
>>>
>>> Do the device registers not need to be set anymore?
>>>
>>
>>> For main branch, it would be nice to have an answer to above.
>>
>>> It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches.
>>
>> This is not only added readability, but to fix real world issue, we were experiencing i225-IT not runnable issue without patching another case checking as below:
>>
>> +++ b/drivers/net/igc/base/igc_phy.c
>> @@ -1881,6 +1881,7 @@ s32 igc_phy_force_speed_duplex_m88(struct igc_hw *hw)
>> case I210_I_PHY_ID:
>> /* fall-through */
>> case I225_I_PHY_ID:
>> + case I225_IT_PHY_ID:
>>
>> However, cleaner solution is to remove those unnecessary checking completely as it does in kernel also https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7c496de538eebd8212dc2a3c9a468386b264d0d4
>>
>
> ok, you are correct that it is more than readability. I can apply but
> first it would be good if you can answer the original question and
> explain why the functionality is changed for I225_I_PHY_ID by removing
> the function pointers that set the registers.
>
Hi, I didn't apply this to DPDK 21.11.3 because there was no explanation
of why the registers were changing after asking multiple times.
If it is required for a later 21.11 LTS release, please send a backport
to stable mailing list with an updated commit message explaining the
changes.
thanks,
Kevin.
>>>>> - /* TODO - complete with GPY PHY information */
>>>>> - break;
>>>>> - default:
>>>>> - ret_val = -IGC_ERR_PHY;
>>>>> - goto out;
>>>>> - }
>>>>> + phy->type = igc_phy_i225;
>>>>> +
>>>>>
>>>>> out:
>>>>> return ret_val;
>>>>> diff --git a/drivers/net/igc/base/igc_phy.c
>>>>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644
>>>>> --- a/drivers/net/igc/base/igc_phy.c
>>>>> +++ b/drivers/net/igc/base/igc_phy.c
>>>>> @@ -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) |
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
2022-12-20 15:47 ` Kevin Traynor
@ 2022-12-21 3:01 ` Mah, Yock Gen
0 siblings, 0 replies; 16+ messages in thread
From: Mah, Yock Gen @ 2022-12-21 3:01 UTC (permalink / raw)
To: Kevin Traynor, IOTG DPDK Ref App, Zhang, Qi Z, Taripin, Samuel; +Cc: dev
-----Original Message-----
From: Kevin Traynor <ktraynor@redhat.com>
Sent: Tuesday, 20 December, 2022 11:48 PM
To: Mah, Yock Gen <yock.gen.mah@intel.com>; IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Taripin, Samuel <samuel.taripin@intel.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
On 19/10/2022 09:34, Kevin Traynor wrote:
> On 18/10/2022 23:45, Mah, Yock Gen wrote:
>>
>>
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday, 18 October, 2022 8:54 PM
>> To: Mah, Yock Gen <yock.gen.mah@intel.com>; IOTG DPDK Ref App
>> <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>
>> On 12/10/2022 09:39, Kevin Traynor wrote:
>>> On 12/10/2022 08:45, Mah, Yock Gen wrote:
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>> Sent: Tuesday, 11 October, 2022 7:05 PM
>>>> To: IOTG DPDK Ref App <iotg.dpdk.ref.app@intel.com>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking
>>>>
>>>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote:
>>>>> 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_i225.c | 15 ++-------------
>>>>> drivers/net/igc/base/igc_phy.c | 6 ++----
>>>>> 2 files changed, 4 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/igc/base/igc_i225.c
>>>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb
>>>>> 100644
>>>>> --- a/drivers/net/igc/base/igc_i225.c
>>>>> +++ b/drivers/net/igc/base/igc_i225.c
>>>>> @@ -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;
>>>>
>>>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed.
>>>>
>>>>> - Why are they removed, were they not needed?
>>>>
>>>>
>>>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process.
>>>>
>>>
>>> Right, that's clear about the vendor ID check. But it's not clear to
>>> me why the the resulting code like this:
>>>
>>> phy->type = igc_phy_i225;
>>>
>>> and not like this:
>>>
>>> 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;
>>>
>>> So it is using dummy null functions instead:
>>> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61
>>>
>>> Do the device registers not need to be set anymore?
>>>
>>
>>> For main branch, it would be nice to have an answer to above.
>>
>>> It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches.
>>
>> This is not only added readability, but to fix real world issue, we were experiencing i225-IT not runnable issue without patching another case checking as below:
>>
>> +++ b/drivers/net/igc/base/igc_phy.c
>> @@ -1881,6 +1881,7 @@ s32 igc_phy_force_speed_duplex_m88(struct igc_hw *hw)
>> case I210_I_PHY_ID:
>> /* fall-through */
>> case I225_I_PHY_ID:
>> + case I225_IT_PHY_ID:
>>
>> However, cleaner solution is to remove those unnecessary checking
>> completely as it does in kernel also
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/co
>> mmit/?id=7c496de538eebd8212dc2a3c9a468386b264d0d4
>>
>
> ok, you are correct that it is more than readability. I can apply but
> first it would be good if you can answer the original question and
> explain why the functionality is changed for I225_I_PHY_ID by removing
> the function pointers that set the registers.
>
>Hi, I didn't apply this to DPDK 21.11.3 because there was no explanation of why the registers were changing after asking multiple times.
I>f it is required for a later 21.11 LTS release, please send a backport to stable mailing list with an updated commit message explaining the changes.
>thanks,
>Kevin.
Apology of the late reply, we were missed out your last message.
Yes, we will re-submit the patch for 21.11 LTS with clearer explanation.
Thanks,
Mah
>>>>> - /* TODO - complete with GPY PHY information */
>>>>> - break;
>>>>> - default:
>>>>> - ret_val = -IGC_ERR_PHY;
>>>>> - goto out;
>>>>> - }
>>>>> + phy->type = igc_phy_i225;
>>>>> +
>>>>>
>>>>> out:
>>>>> return ret_val;
>>>>> diff --git a/drivers/net/igc/base/igc_phy.c
>>>>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a
>>>>> 100644
>>>>> --- a/drivers/net/igc/base/igc_phy.c
>>>>> +++ b/drivers/net/igc/base/igc_phy.c
>>>>> @@ -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) |
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-12-21 10:19 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-29 8:14 [PATCH] IGC: Remove I225_I_PHY_ID checking iotg.dpdk.ref.app
2022-08-30 11:17 ` Zhang, Qi Z
2022-08-31 22:42 ` Mah, Yock Gen
2022-09-01 8:22 ` David Marchand
2022-09-02 7:01 ` David Marchand
2022-08-31 22:51 ` [PATCH v2] " iotg.dpdk.ref.app
2022-09-02 0:18 ` [PATCH v3] " yock.gen.mah
2022-09-04 1:55 ` Zhang, Qi Z
2022-10-11 11:04 ` [PATCH v2] " Kevin Traynor
2022-10-12 7:45 ` Mah, Yock Gen
2022-10-12 8:39 ` Kevin Traynor
2022-10-18 12:54 ` Kevin Traynor
2022-10-18 22:45 ` Mah, Yock Gen
2022-10-19 8:34 ` Kevin Traynor
2022-12-20 15:47 ` Kevin Traynor
2022-12-21 3:01 ` Mah, Yock Gen
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).