diff mbox series

[net-next,09/12] igc: Remove _I_PHY_ID checking

Message ID 20210720232101.3087589-10-anthony.l.nguyen@intel.com
State New
Headers show
Series [net-next,01/12] e1000e: Add handshake with the CSME to support S0ix | expand

Commit Message

Tony Nguyen July 20, 2021, 11:20 p.m. UTC 
From: Sasha Neftin <sasha.neftin@intel.com>

i225 devices have only one PHY vendor. There is no point checking
_I_PHY_ID during the link establishment and auto-negotiation process.
This patch comes to clean up these pointless checkings.

Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_base.c | 10 +---------
 drivers/net/ethernet/intel/igc/igc_main.c |  3 +--
 drivers/net/ethernet/intel/igc/igc_phy.c  |  6 ++----
 3 files changed, 4 insertions(+), 15 deletions(-)

Comments

Andrew Lunn July 21, 2021, 2:50 p.m. UTC | #1 
On Tue, Jul 20, 2021 at 04:20:58PM -0700, Tony Nguyen wrote:
> From: Sasha Neftin <sasha.neftin@intel.com>

> 

> i225 devices have only one PHY vendor. There is no point checking

> _I_PHY_ID during the link establishment and auto-negotiation process.

> This patch comes to clean up these pointless checkings.


I don't know this hardware....

Is the PHY integrated into the MAC? Or is it external?

For the ixgbe, the InPhi CS4227 is now owned by Marvell, and it is
very difficult to get any information from them. At some point, it
would be nice to have a second source, maybe a Microchip PHY. The bits
of code you are removing make it easier to see where changes would be
needed to add support for a second PHY. Why would you want to limit it
to just one vendor?

       Andrew
Sasha Neftin July 21, 2021, 6:02 p.m. UTC | #2 
On 7/21/2021 17:50, Andrew Lunn wrote:
> On Tue, Jul 20, 2021 at 04:20:58PM -0700, Tony Nguyen wrote:

>> From: Sasha Neftin <sasha.neftin@intel.com>

>>

>> i225 devices have only one PHY vendor. There is no point checking

>> _I_PHY_ID during the link establishment and auto-negotiation process.

>> This patch comes to clean up these pointless checkings.

> 

> I don't know this hardware....

> 

> Is the PHY integrated into the MAC? Or is it external?

i225 controller offers a fully-integrated Media Access Control
(MAC) and Physical Layer (PHY) port.
Both components (MAC and PHY) supports 2.5G
> 

> For the ixgbe, the InPhi CS4227 is now owned by Marvell, and it is

> very difficult to get any information from them. At some point, it

> would be nice to have a second source, maybe a Microchip PHY. The bits

> of code you are removing make it easier to see where changes would be

> needed to add support for a second PHY. Why would you want to limit it

> to just one vendor?

Not like that i210 devices. There is only one PHY ID/type for this 
product. Only one port. No fiber, no SFP - only copper. No option for a 
second PHY.
> 

>         Andrew

> 

Sasha Neftin
Andrew Lunn July 21, 2021, 7:53 p.m. UTC | #3 
On Wed, Jul 21, 2021 at 09:02:13PM +0300, Sasha Neftin wrote:
> On 7/21/2021 17:50, Andrew Lunn wrote:

> > On Tue, Jul 20, 2021 at 04:20:58PM -0700, Tony Nguyen wrote:

> > > From: Sasha Neftin <sasha.neftin@intel.com>

> > > 

> > > i225 devices have only one PHY vendor. There is no point checking

> > > _I_PHY_ID during the link establishment and auto-negotiation process.

> > > This patch comes to clean up these pointless checkings.

> > 

> > I don't know this hardware....

> > 

> > Is the PHY integrated into the MAC? Or is it external?

> i225 controller offers a fully-integrated Media Access Control

> (MAC) and Physical Layer (PHY) port.

> Both components (MAC and PHY) supports 2.5G


Hi Sasha

Thanks for the info. Then this change make sense. But the commit
message could of been better. It is not really about there being one
PHY vendor, it is simply impossible for the PHY to be anything else,
so there is no need to check.

	Andrew
Sasha Neftin July 22, 2021, 5:22 a.m. UTC | #4 
On 7/21/2021 22:53, Andrew Lunn wrote:
> On Wed, Jul 21, 2021 at 09:02:13PM +0300, Sasha Neftin wrote:

>> On 7/21/2021 17:50, Andrew Lunn wrote:

>>> On Tue, Jul 20, 2021 at 04:20:58PM -0700, Tony Nguyen wrote:

>>>> From: Sasha Neftin <sasha.neftin@intel.com>

>>>>

>>>> i225 devices have only one PHY vendor. There is no point checking

>>>> _I_PHY_ID during the link establishment and auto-negotiation process.

>>>> This patch comes to clean up these pointless checkings.

>>>

>>> I don't know this hardware....

>>>

>>> Is the PHY integrated into the MAC? Or is it external?

>> i225 controller offers a fully-integrated Media Access Control

>> (MAC) and Physical Layer (PHY) port.

>> Both components (MAC and PHY) supports 2.5G

> 

> Hi Sasha

> 

> Thanks for the info. Then this change make sense. But the commit

> message could of been better. It is not really about there being one

> PHY vendor, it is simply impossible for the PHY to be anything else,

> so there is no need to check.

We will clarify it in the commit message.
> 

> 	Andrew

> 

Sasha Neftin
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
index d0700d48ecf9..84f142f5e472 100644
--- a/drivers/net/ethernet/intel/igc/igc_base.c
+++ b/drivers/net/ethernet/intel/igc/igc_base.c
@@ -187,15 +187,7 @@  static s32 igc_init_phy_params_base(struct igc_hw *hw)
 
 	igc_check_for_copper_link(hw);
 
-	/* Verify phy id and set remaining function pointers */
-	switch (phy->id) {
-	case I225_I_PHY_ID:
-		phy->type	= igc_phy_i225;
-		break;
-	default:
-		ret_val = -IGC_ERR_PHY;
-		goto out;
-	}
+	phy->type = igc_phy_i225;
 
 out:
 	return ret_val;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index f7cf97916390..a5278a8f491f 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5231,8 +5231,7 @@  bool igc_has_link(struct igc_adapter *adapter)
 		break;
 	}
 
-	if (hw->mac.type == igc_i225 &&
-	    hw->phy.id == I225_I_PHY_ID) {
+	if (hw->mac.type == igc_i225) {
 		if (!netif_carrier_ok(adapter->netdev)) {
 			adapter->flags &= ~IGC_FLAG_NEED_LINK_UPDATE;
 		} else if (!(adapter->flags & IGC_FLAG_NEED_LINK_UPDATE)) {
diff --git a/drivers/net/ethernet/intel/igc/igc_phy.c b/drivers/net/ethernet/intel/igc/igc_phy.c
index 83aeb5e7076f..5cad31c3c7b0 100644
--- a/drivers/net/ethernet/intel/igc/igc_phy.c
+++ b/drivers/net/ethernet/intel/igc/igc_phy.c
@@ -249,8 +249,7 @@  static 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) |
@@ -390,8 +389,7 @@  static 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) |