[PATCH 0/2] Upstream ONL patch for PHY BCM5461S

[PATCH 0/2] Upstream ONL patch for PHY BCM5461S

[Paul Menzel]

Dear Linux folks,


Looking a little bit at Open Network Linux, they carry some Linux
patches, but have not upstreamed them yet. This upstreams support for
the PHY BCM5461S. It’d be great, if you could help review it.


Kind regards,

Paul


Jeffrey Townsend (2):
  ethernet: igb: Support PHY BCM5461S
  ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

 drivers/net/ethernet/intel/igb/e1000_82575.c  | 23 ++++-
 .../net/ethernet/intel/igb/e1000_defines.h    |  1 +
 drivers/net/ethernet/intel/igb/e1000_hw.h     |  1 +
 drivers/net/ethernet/intel/igb/e1000_phy.c    | 89 +++++++++++++++++--
 drivers/net/ethernet/intel/igb/e1000_phy.h    |  2 +
 drivers/net/ethernet/intel/igb/igb_main.c     |  8 ++
 6 files changed, 118 insertions(+), 6 deletions(-)

-- 
2.29.1

[Intel-wired-lan] [PATCH 0/2] Upstream ONL patch for PHY BCM5461S

[Paul Menzel]

Dear Linux folks,


Looking a little bit at Open Network Linux, they carry some Linux
patches, but have not upstreamed them yet. This upstreams support for
the PHY BCM5461S. It?d be great, if you could help review it.


Kind regards,

Paul


Jeffrey Townsend (2):
  ethernet: igb: Support PHY BCM5461S
  ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

 drivers/net/ethernet/intel/igb/e1000_82575.c  | 23 ++++-
 .../net/ethernet/intel/igb/e1000_defines.h    |  1 +
 drivers/net/ethernet/intel/igb/e1000_hw.h     |  1 +
 drivers/net/ethernet/intel/igb/e1000_phy.c    | 89 +++++++++++++++++--
 drivers/net/ethernet/intel/igb/e1000_phy.h    |  2 +
 drivers/net/ethernet/intel/igb/igb_main.c     |  8 ++
 6 files changed, 118 insertions(+), 6 deletions(-)

-- 
2.29.1

[PATCH 1/2] ethernet: igb: Support PHY BCM5461S

[Paul Menzel]

From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

The BCM5461S PHY is used in switches.

The patch is taken from Open Network Linux, and it was added there as
patch

    packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch

in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
of this commit was already upstreamed in Linux commit eeb0149660 (igb:
support BCM54616 PHY) in 2017.

I applied the forward-ported

    packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch

added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].

[1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
[2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331

Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
Cc: John W Linville <linville@tuxdriver.com>
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/net/ethernet/intel/igb/e1000_82575.c  | 23 +++++-
 .../net/ethernet/intel/igb/e1000_defines.h    |  1 +
 drivers/net/ethernet/intel/igb/e1000_hw.h     |  1 +
 drivers/net/ethernet/intel/igb/e1000_phy.c    | 77 +++++++++++++++++++
 drivers/net/ethernet/intel/igb/e1000_phy.h    |  2 +
 drivers/net/ethernet/intel/igb/igb_main.c     |  8 ++
 6 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 50863fd87d53..83c14ae689b1 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -308,6 +308,12 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
 		phy->ops.set_d3_lplu_state = igb_set_d3_lplu_state_82580;
 		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_m88;
 		break;
+	case BCM5461S_PHY_ID:
+		phy->type		= e1000_phy_bcm5461s;
+		phy->ops.check_polarity	= NULL;
+		phy->ops.get_cable_length = NULL;
+		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
+		break;
 	case BCM54616_E_PHY_ID:
 		phy->type = e1000_phy_bcm54616;
 		break;
@@ -866,6 +872,16 @@ static s32 igb_get_phy_id_82575(struct e1000_hw *hw)
 			goto out;
 		}
 		ret_val = igb_get_phy_id(hw);
+		if (ret_val && hw->mac.type == e1000_i354) {
+			/* we do a special check for bcm5461s phy by setting
+			 * the phy->addr to 5 and doing the phy check again. This
+			 * call will succeed and retrieve a valid phy id if we have
+			 * the bcm5461s phy
+			 */
+			phy->addr = 5;
+			phy->type = e1000_phy_bcm5461s;
+			ret_val = igb_get_phy_id(hw);
+		}
 		goto out;
 	}
 
@@ -1253,6 +1269,9 @@ static s32 igb_get_cfg_done_82575(struct e1000_hw *hw)
 	    (hw->phy.type == e1000_phy_igp_3))
 		igb_phy_init_script_igp3(hw);
 
+	if (hw->phy.type == e1000_phy_bcm5461s)
+		igb_phy_init_script_5461s(hw);
+
 	return 0;
 }
 
@@ -1582,6 +1601,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
 	case e1000_i350:
 	case e1000_i210:
 	case e1000_i211:
+	case e1000_i354:
 		phpm_reg = rd32(E1000_82580_PHY_POWER_MGMT);
 		phpm_reg &= ~E1000_82580_PM_GO_LINKD;
 		wr32(E1000_82580_PHY_POWER_MGMT, phpm_reg);
@@ -1627,7 +1647,8 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
 		ret_val = igb_copper_link_setup_82580(hw);
 		break;
 	case e1000_phy_bcm54616:
-		ret_val = 0;
+		break;
+	case e1000_phy_bcm5461s:
 		break;
 	default:
 		ret_val = -E1000_ERR_PHY;
diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index d2e2c50ce257..0561ef6cb29c 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -886,6 +886,7 @@
 #define M88E1543_E_PHY_ID    0x01410EA0
 #define M88E1512_E_PHY_ID    0x01410DD0
 #define BCM54616_E_PHY_ID    0x03625D10
+#define BCM5461S_PHY_ID      0x002060C0
 
 /* M88E1000 Specific Registers */
 #define M88E1000_PHY_SPEC_CTRL     0x10  /* PHY Specific Control Register */
diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
index 5d87957b2627..a660675d6218 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -110,6 +110,7 @@ enum e1000_phy_type {
 	e1000_phy_82580,
 	e1000_phy_i210,
 	e1000_phy_bcm54616,
+	e1000_phy_bcm5461s,
 };
 
 enum e1000_bus_type {
diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 8c8eb82e6272..4e0b4ba09a00 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -126,6 +126,13 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
 	 * Control register.  The MAC will take care of interfacing with the
 	 * PHY to retrieve the desired data.
 	 */
+	if (phy->type == e1000_phy_bcm5461s) {
+		mdic = rd32(E1000_MDICNFG);
+		mdic &= ~E1000_MDICNFG_PHY_MASK;
+		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+		wr32(E1000_MDICNFG, mdic);
+	}
+
 	mdic = ((offset << E1000_MDIC_REG_SHIFT) |
 		(phy->addr << E1000_MDIC_PHY_SHIFT) |
 		(E1000_MDIC_OP_READ));
@@ -182,6 +189,13 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
 	 * Control register.  The MAC will take care of interfacing with the
 	 * PHY to retrieve the desired data.
 	 */
+	if (phy->type == e1000_phy_bcm5461s) {
+		mdic = rd32(E1000_MDICNFG);
+		mdic &= ~E1000_MDICNFG_PHY_MASK;
+		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+		wr32(E1000_MDICNFG, mdic);
+	}
+
 	mdic = (((u32)data) |
 		(offset << E1000_MDIC_REG_SHIFT) |
 		(phy->addr << E1000_MDIC_PHY_SHIFT) |
@@ -2628,3 +2642,66 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
 
 	return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
 }
+
+/**
+ *  igb_phy_init_script_5461s - Inits the BCM5461S PHY
+ *  @hw: pointer to the HW structure
+ *
+ *  Initializes a Broadcom Gigabit PHY.
+ **/
+s32 igb_phy_init_script_5461s(struct e1000_hw *hw)
+{
+	u16 mii_reg_led = 0;
+
+	/* 1. Speed LED (Set the Link LED mode), Shadow 00010, 0x1C.bit2=1 */
+	hw->phy.ops.write_reg(hw, 0x1C, 0x0800);
+	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
+	mii_reg_led |= 0x0004;
+	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
+
+	/* 2. Active LED (Set the Link LED mode), Shadow 01001, 0x1C.bit4=1, 0x10.bit5=0 */
+	hw->phy.ops.write_reg(hw, 0x1C, 0x2400);
+	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
+	mii_reg_led |= 0x0010;
+	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
+	hw->phy.ops.read_reg(hw, 0x10, &mii_reg_led);
+	mii_reg_led &= 0xffdf;
+	hw->phy.ops.write_reg(hw, 0x10, mii_reg_led);
+
+	return 0;
+}
+
+/**
+ *  igb_get_phy_info_5461s - Retrieve 5461s PHY information
+ *  @hw: pointer to the HW structure
+ *
+ *  Read PHY status to determine if link is up.  If link is up, then
+ *  set/determine 10base-T extended distance and polarity correction.  Read
+ *  PHY port status to determine MDI/MDIx and speed.  Based on the speed,
+ *  determine on the cable length, local and remote receiver.
+ **/
+s32 igb_get_phy_info_5461s(struct e1000_hw *hw)
+{
+	struct e1000_phy_info *phy = &hw->phy;
+	s32 ret_val;
+	bool link;
+
+	ret_val = igb_phy_has_link(hw, 1, 0, &link);
+	if (ret_val)
+		goto out;
+
+	if (!link) {
+		ret_val = -E1000_ERR_CONFIG;
+		goto out;
+	}
+
+	phy->polarity_correction = true;
+
+	phy->is_mdix = true;
+	phy->cable_length = E1000_CABLE_LENGTH_UNDEFINED;
+	phy->local_rx = e1000_1000t_rx_status_ok;
+	phy->remote_rx = e1000_1000t_rx_status_ok;
+
+out:
+	return ret_val;
+}
diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.h b/drivers/net/ethernet/intel/igb/e1000_phy.h
index 5894e4b1d0a8..aa888efc05f2 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.h
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
@@ -41,6 +41,8 @@ s32  igb_phy_has_link(struct e1000_hw *hw, u32 iterations,
 void igb_power_up_phy_copper(struct e1000_hw *hw);
 void igb_power_down_phy_copper(struct e1000_hw *hw);
 s32  igb_phy_init_script_igp3(struct e1000_hw *hw);
+s32  igb_phy_init_script_5461s(struct e1000_hw *hw);
+s32  igb_get_phy_info_5461s(struct e1000_hw *hw);
 s32  igb_initialize_M88E1512_phy(struct e1000_hw *hw);
 s32  igb_initialize_M88E1543_phy(struct e1000_hw *hw);
 s32  igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 5fc2c381da55..275fac4cbf63 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8923,11 +8923,19 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 		data->phy_id = adapter->hw.phy.addr;
 		break;
 	case SIOCGMIIREG:
+		adapter->hw.phy.addr = data->phy_id;
 		if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
 				     &data->val_out))
 			return -EIO;
 		break;
 	case SIOCSMIIREG:
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
+		adapter->hw.phy.addr = data->phy_id;
+		if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F,
+				      data->val_in))
+			return -EIO;
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.29.1

[Intel-wired-lan] [PATCH 1/2] ethernet: igb: Support PHY BCM5461S

[Paul Menzel]

From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

The BCM5461S PHY is used in switches.

The patch is taken from Open Network Linux, and it was added there as
patch

    packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch

in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
of this commit was already upstreamed in Linux commit eeb0149660 (igb:
support BCM54616 PHY) in 2017.

I applied the forward-ported

    packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch

added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].

[1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
[2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331

Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
Cc: John W Linville <linville@tuxdriver.com>
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/net/ethernet/intel/igb/e1000_82575.c  | 23 +++++-
 .../net/ethernet/intel/igb/e1000_defines.h    |  1 +
 drivers/net/ethernet/intel/igb/e1000_hw.h     |  1 +
 drivers/net/ethernet/intel/igb/e1000_phy.c    | 77 +++++++++++++++++++
 drivers/net/ethernet/intel/igb/e1000_phy.h    |  2 +
 drivers/net/ethernet/intel/igb/igb_main.c     |  8 ++
 6 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 50863fd87d53..83c14ae689b1 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -308,6 +308,12 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
 		phy->ops.set_d3_lplu_state = igb_set_d3_lplu_state_82580;
 		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_m88;
 		break;
+	case BCM5461S_PHY_ID:
+		phy->type		= e1000_phy_bcm5461s;
+		phy->ops.check_polarity	= NULL;
+		phy->ops.get_cable_length = NULL;
+		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
+		break;
 	case BCM54616_E_PHY_ID:
 		phy->type = e1000_phy_bcm54616;
 		break;
@@ -866,6 +872,16 @@ static s32 igb_get_phy_id_82575(struct e1000_hw *hw)
 			goto out;
 		}
 		ret_val = igb_get_phy_id(hw);
+		if (ret_val && hw->mac.type == e1000_i354) {
+			/* we do a special check for bcm5461s phy by setting
+			 * the phy->addr to 5 and doing the phy check again. This
+			 * call will succeed and retrieve a valid phy id if we have
+			 * the bcm5461s phy
+			 */
+			phy->addr = 5;
+			phy->type = e1000_phy_bcm5461s;
+			ret_val = igb_get_phy_id(hw);
+		}
 		goto out;
 	}
 
@@ -1253,6 +1269,9 @@ static s32 igb_get_cfg_done_82575(struct e1000_hw *hw)
 	    (hw->phy.type == e1000_phy_igp_3))
 		igb_phy_init_script_igp3(hw);
 
+	if (hw->phy.type == e1000_phy_bcm5461s)
+		igb_phy_init_script_5461s(hw);
+
 	return 0;
 }
 
@@ -1582,6 +1601,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
 	case e1000_i350:
 	case e1000_i210:
 	case e1000_i211:
+	case e1000_i354:
 		phpm_reg = rd32(E1000_82580_PHY_POWER_MGMT);
 		phpm_reg &= ~E1000_82580_PM_GO_LINKD;
 		wr32(E1000_82580_PHY_POWER_MGMT, phpm_reg);
@@ -1627,7 +1647,8 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
 		ret_val = igb_copper_link_setup_82580(hw);
 		break;
 	case e1000_phy_bcm54616:
-		ret_val = 0;
+		break;
+	case e1000_phy_bcm5461s:
 		break;
 	default:
 		ret_val = -E1000_ERR_PHY;
diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index d2e2c50ce257..0561ef6cb29c 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -886,6 +886,7 @@
 #define M88E1543_E_PHY_ID    0x01410EA0
 #define M88E1512_E_PHY_ID    0x01410DD0
 #define BCM54616_E_PHY_ID    0x03625D10
+#define BCM5461S_PHY_ID      0x002060C0
 
 /* M88E1000 Specific Registers */
 #define M88E1000_PHY_SPEC_CTRL     0x10  /* PHY Specific Control Register */
diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
index 5d87957b2627..a660675d6218 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -110,6 +110,7 @@ enum e1000_phy_type {
 	e1000_phy_82580,
 	e1000_phy_i210,
 	e1000_phy_bcm54616,
+	e1000_phy_bcm5461s,
 };
 
 enum e1000_bus_type {
diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 8c8eb82e6272..4e0b4ba09a00 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -126,6 +126,13 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
 	 * Control register.  The MAC will take care of interfacing with the
 	 * PHY to retrieve the desired data.
 	 */
+	if (phy->type == e1000_phy_bcm5461s) {
+		mdic = rd32(E1000_MDICNFG);
+		mdic &= ~E1000_MDICNFG_PHY_MASK;
+		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+		wr32(E1000_MDICNFG, mdic);
+	}
+
 	mdic = ((offset << E1000_MDIC_REG_SHIFT) |
 		(phy->addr << E1000_MDIC_PHY_SHIFT) |
 		(E1000_MDIC_OP_READ));
@@ -182,6 +189,13 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
 	 * Control register.  The MAC will take care of interfacing with the
 	 * PHY to retrieve the desired data.
 	 */
+	if (phy->type == e1000_phy_bcm5461s) {
+		mdic = rd32(E1000_MDICNFG);
+		mdic &= ~E1000_MDICNFG_PHY_MASK;
+		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+		wr32(E1000_MDICNFG, mdic);
+	}
+
 	mdic = (((u32)data) |
 		(offset << E1000_MDIC_REG_SHIFT) |
 		(phy->addr << E1000_MDIC_PHY_SHIFT) |
@@ -2628,3 +2642,66 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
 
 	return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
 }
+
+/**
+ *  igb_phy_init_script_5461s - Inits the BCM5461S PHY
+ *  @hw: pointer to the HW structure
+ *
+ *  Initializes a Broadcom Gigabit PHY.
+ **/
+s32 igb_phy_init_script_5461s(struct e1000_hw *hw)
+{
+	u16 mii_reg_led = 0;
+
+	/* 1. Speed LED (Set the Link LED mode), Shadow 00010, 0x1C.bit2=1 */
+	hw->phy.ops.write_reg(hw, 0x1C, 0x0800);
+	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
+	mii_reg_led |= 0x0004;
+	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
+
+	/* 2. Active LED (Set the Link LED mode), Shadow 01001, 0x1C.bit4=1, 0x10.bit5=0 */
+	hw->phy.ops.write_reg(hw, 0x1C, 0x2400);
+	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
+	mii_reg_led |= 0x0010;
+	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
+	hw->phy.ops.read_reg(hw, 0x10, &mii_reg_led);
+	mii_reg_led &= 0xffdf;
+	hw->phy.ops.write_reg(hw, 0x10, mii_reg_led);
+
+	return 0;
+}
+
+/**
+ *  igb_get_phy_info_5461s - Retrieve 5461s PHY information
+ *  @hw: pointer to the HW structure
+ *
+ *  Read PHY status to determine if link is up.  If link is up, then
+ *  set/determine 10base-T extended distance and polarity correction.  Read
+ *  PHY port status to determine MDI/MDIx and speed.  Based on the speed,
+ *  determine on the cable length, local and remote receiver.
+ **/
+s32 igb_get_phy_info_5461s(struct e1000_hw *hw)
+{
+	struct e1000_phy_info *phy = &hw->phy;
+	s32 ret_val;
+	bool link;
+
+	ret_val = igb_phy_has_link(hw, 1, 0, &link);
+	if (ret_val)
+		goto out;
+
+	if (!link) {
+		ret_val = -E1000_ERR_CONFIG;
+		goto out;
+	}
+
+	phy->polarity_correction = true;
+
+	phy->is_mdix = true;
+	phy->cable_length = E1000_CABLE_LENGTH_UNDEFINED;
+	phy->local_rx = e1000_1000t_rx_status_ok;
+	phy->remote_rx = e1000_1000t_rx_status_ok;
+
+out:
+	return ret_val;
+}
diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.h b/drivers/net/ethernet/intel/igb/e1000_phy.h
index 5894e4b1d0a8..aa888efc05f2 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.h
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
@@ -41,6 +41,8 @@ s32  igb_phy_has_link(struct e1000_hw *hw, u32 iterations,
 void igb_power_up_phy_copper(struct e1000_hw *hw);
 void igb_power_down_phy_copper(struct e1000_hw *hw);
 s32  igb_phy_init_script_igp3(struct e1000_hw *hw);
+s32  igb_phy_init_script_5461s(struct e1000_hw *hw);
+s32  igb_get_phy_info_5461s(struct e1000_hw *hw);
 s32  igb_initialize_M88E1512_phy(struct e1000_hw *hw);
 s32  igb_initialize_M88E1543_phy(struct e1000_hw *hw);
 s32  igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 5fc2c381da55..275fac4cbf63 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8923,11 +8923,19 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 		data->phy_id = adapter->hw.phy.addr;
 		break;
 	case SIOCGMIIREG:
+		adapter->hw.phy.addr = data->phy_id;
 		if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
 				     &data->val_out))
 			return -EIO;
 		break;
 	case SIOCSMIIREG:
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
+		adapter->hw.phy.addr = data->phy_id;
+		if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F,
+				      data->val_in))
+			return -EIO;
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.29.1

[PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

The ops field might no be defined, so add a check.

The patch is taken from Open Network Linux (ONL), and it was added there
as part of the patch

    packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch

in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
of this commit was already upstreamed in Linux commit eeb0149660 (igb:
support BCM54616 PHY) in 2017.

I applied the forward-ported

    packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch

added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].

[1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
[2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331

Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
Cc: John W Linville <linville@tuxdriver.com>
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/net/ethernet/intel/igb/e1000_phy.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 4e0b4ba09a00..4151e55a6d2a 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -1107,11 +1107,13 @@ s32 igb_setup_copper_link(struct e1000_hw *hw)
 		/* PHY will be set to 10H, 10F, 100H or 100F
 		 * depending on user settings.
 		 */
-		hw_dbg("Forcing Speed and Duplex\n");
-		ret_val = hw->phy.ops.force_speed_duplex(hw);
-		if (ret_val) {
-			hw_dbg("Error Forcing Speed and Duplex\n");
-			goto out;
+		if (hw->phy.ops.force_speed_duplex) {
+			hw_dbg("Forcing Speed and Duplex\n");
+			ret_val = hw->phy.ops.force_speed_duplex(hw);
+			if (ret_val) {
+				hw_dbg("Error Forcing Speed and Duplex\n");
+				goto out;
+			}
 		}
 	}
 
-- 
2.29.1

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

The ops field might no be defined, so add a check.

The patch is taken from Open Network Linux (ONL), and it was added there
as part of the patch

    packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch

in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
of this commit was already upstreamed in Linux commit eeb0149660 (igb:
support BCM54616 PHY) in 2017.

I applied the forward-ported

    packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch

added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].

[1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
[2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331

Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
Cc: John W Linville <linville@tuxdriver.com>
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/net/ethernet/intel/igb/e1000_phy.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 4e0b4ba09a00..4151e55a6d2a 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -1107,11 +1107,13 @@ s32 igb_setup_copper_link(struct e1000_hw *hw)
 		/* PHY will be set to 10H, 10F, 100H or 100F
 		 * depending on user settings.
 		 */
-		hw_dbg("Forcing Speed and Duplex\n");
-		ret_val = hw->phy.ops.force_speed_duplex(hw);
-		if (ret_val) {
-			hw_dbg("Error Forcing Speed and Duplex\n");
-			goto out;
+		if (hw->phy.ops.force_speed_duplex) {
+			hw_dbg("Forcing Speed and Duplex\n");
+			ret_val = hw->phy.ops.force_speed_duplex(hw);
+			if (ret_val) {
+				hw_dbg("Error Forcing Speed and Duplex\n");
+				goto out;
+			}
 		}
 	}
 
-- 
2.29.1

Re: [PATCH 1/2] ethernet: igb: Support PHY BCM5461S

[Paul Menzel]

Dear Linux folks,


Am 03.11.20 um 00:13 schrieb Paul Menzel:
> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> The BCM5461S PHY is used in switches.
> 
> The patch is taken from Open Network Linux, and it was added there as
> patch
> 
>      packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
> 
> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
> support BCM54616 PHY) in 2017.
> 
> I applied the forward-ported
> 
>      packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
> 
> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
> 
> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331
> 
> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> Cc: John W Linville <linville@tuxdriver.com>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>   drivers/net/ethernet/intel/igb/e1000_82575.c  | 23 +++++-
>   .../net/ethernet/intel/igb/e1000_defines.h    |  1 +
>   drivers/net/ethernet/intel/igb/e1000_hw.h     |  1 +
>   drivers/net/ethernet/intel/igb/e1000_phy.c    | 77 +++++++++++++++++++
>   drivers/net/ethernet/intel/igb/e1000_phy.h    |  2 +
>   drivers/net/ethernet/intel/igb/igb_main.c     |  8 ++
>   6 files changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index 50863fd87d53..83c14ae689b1 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -308,6 +308,12 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
>   		phy->ops.set_d3_lplu_state = igb_set_d3_lplu_state_82580;
>   		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_m88;
>   		break;
> +	case BCM5461S_PHY_ID:
> +		phy->type		= e1000_phy_bcm5461s;

Do not align the = with the one on the line below.

> +		phy->ops.check_polarity	= NULL;
> +		phy->ops.get_cable_length = NULL;
> +		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
> +		break;
>   	case BCM54616_E_PHY_ID:
>   		phy->type = e1000_phy_bcm54616;
>   		break;
> @@ -866,6 +872,16 @@ static s32 igb_get_phy_id_82575(struct e1000_hw *hw)
>   			goto out;
>   		}
>   		ret_val = igb_get_phy_id(hw);
> +		if (ret_val && hw->mac.type == e1000_i354) {
> +			/* we do a special check for bcm5461s phy by setting
> +			 * the phy->addr to 5 and doing the phy check again. This
> +			 * call will succeed and retrieve a valid phy id if we have
> +			 * the bcm5461s phy
> +			 */
> +			phy->addr = 5;
> +			phy->type = e1000_phy_bcm5461s;
> +			ret_val = igb_get_phy_id(hw);
> +		}
>   		goto out;
>   	}
>   
> @@ -1253,6 +1269,9 @@ static s32 igb_get_cfg_done_82575(struct e1000_hw *hw)
>   	    (hw->phy.type == e1000_phy_igp_3))
>   		igb_phy_init_script_igp3(hw);
>   
> +	if (hw->phy.type == e1000_phy_bcm5461s)
> +		igb_phy_init_script_5461s(hw);
> +
>   	return 0;
>   }
>   
> @@ -1582,6 +1601,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
>   	case e1000_i350:
>   	case e1000_i210:
>   	case e1000_i211:
> +	case e1000_i354:

Any idea, why e1000_i350 is at the top?

>   		phpm_reg = rd32(E1000_82580_PHY_POWER_MGMT);
>   		phpm_reg &= ~E1000_82580_PM_GO_LINKD;
>   		wr32(E1000_82580_PHY_POWER_MGMT, phpm_reg);
> @@ -1627,7 +1647,8 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
>   		ret_val = igb_copper_link_setup_82580(hw);
>   		break;
>   	case e1000_phy_bcm54616:
> -		ret_val = 0;
> +		break;
> +	case e1000_phy_bcm5461s:
>   		break;

John, any idea, why you did not upstream the `ret_val = 0` line?

>   	default:
>   		ret_val = -E1000_ERR_PHY;
> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
> index d2e2c50ce257..0561ef6cb29c 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -886,6 +886,7 @@
>   #define M88E1543_E_PHY_ID    0x01410EA0
>   #define M88E1512_E_PHY_ID    0x01410DD0
>   #define BCM54616_E_PHY_ID    0x03625D10
> +#define BCM5461S_PHY_ID      0x002060C0

Should this be `BCM5461S_E_PHY_ID` for consistency? I have no idea, what 
`_E` means?

>   
>   /* M88E1000 Specific Registers */
>   #define M88E1000_PHY_SPEC_CTRL     0x10  /* PHY Specific Control Register */
> diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
> index 5d87957b2627..a660675d6218 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
> @@ -110,6 +110,7 @@ enum e1000_phy_type {
>   	e1000_phy_82580,
>   	e1000_phy_i210,
>   	e1000_phy_bcm54616,
> +	e1000_phy_bcm5461s,
>   };
>   
>   enum e1000_bus_type {
> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
> index 8c8eb82e6272..4e0b4ba09a00 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -126,6 +126,13 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
>   	 * Control register.  The MAC will take care of interfacing with the
>   	 * PHY to retrieve the desired data.
>   	 */
> +	if (phy->type == e1000_phy_bcm5461s) {
> +		mdic = rd32(E1000_MDICNFG);
> +		mdic &= ~E1000_MDICNFG_PHY_MASK;
> +		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
> +		wr32(E1000_MDICNFG, mdic);
> +	}
> +
>   	mdic = ((offset << E1000_MDIC_REG_SHIFT) |
>   		(phy->addr << E1000_MDIC_PHY_SHIFT) |
>   		(E1000_MDIC_OP_READ));
> @@ -182,6 +189,13 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
>   	 * Control register.  The MAC will take care of interfacing with the
>   	 * PHY to retrieve the desired data.
>   	 */
> +	if (phy->type == e1000_phy_bcm5461s) {
> +		mdic = rd32(E1000_MDICNFG);
> +		mdic &= ~E1000_MDICNFG_PHY_MASK;
> +		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
> +		wr32(E1000_MDICNFG, mdic);
> +	}
> +
>   	mdic = (((u32)data) |
>   		(offset << E1000_MDIC_REG_SHIFT) |
>   		(phy->addr << E1000_MDIC_PHY_SHIFT) |
> @@ -2628,3 +2642,66 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
>   
>   	return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
>   }
> +
> +/**
> + *  igb_phy_init_script_5461s - Inits the BCM5461S PHY
> + *  @hw: pointer to the HW structure
> + *
> + *  Initializes a Broadcom Gigabit PHY.
> + **/
> +s32 igb_phy_init_script_5461s(struct e1000_hw *hw)
> +{
> +	u16 mii_reg_led = 0;
> +
> +	/* 1. Speed LED (Set the Link LED mode), Shadow 00010, 0x1C.bit2=1 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x0800);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0004;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +
> +	/* 2. Active LED (Set the Link LED mode), Shadow 01001, 0x1C.bit4=1, 0x10.bit5=0 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x2400);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0010;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +	hw->phy.ops.read_reg(hw, 0x10, &mii_reg_led);
> +	mii_reg_led &= 0xffdf;
> +	hw->phy.ops.write_reg(hw, 0x10, mii_reg_led);
> +
> +	return 0;
> +}
> +
> +/**
> + *  igb_get_phy_info_5461s - Retrieve 5461s PHY information
> + *  @hw: pointer to the HW structure
> + *
> + *  Read PHY status to determine if link is up.  If link is up, then
> + *  set/determine 10base-T extended distance and polarity correction.  Read
> + *  PHY port status to determine MDI/MDIx and speed.  Based on the speed,
> + *  determine on the cable length, local and remote receiver.
> + **/
> +s32 igb_get_phy_info_5461s(struct e1000_hw *hw)
> +{
> +	struct e1000_phy_info *phy = &hw->phy;
> +	s32 ret_val;
> +	bool link;
> +
> +	ret_val = igb_phy_has_link(hw, 1, 0, &link);
> +	if (ret_val)
> +		goto out;
> +
> +	if (!link) {
> +		ret_val = -E1000_ERR_CONFIG;
> +		goto out;
> +	}
> +
> +	phy->polarity_correction = true;
> +
> +	phy->is_mdix = true;
> +	phy->cable_length = E1000_CABLE_LENGTH_UNDEFINED;
> +	phy->local_rx = e1000_1000t_rx_status_ok;
> +	phy->remote_rx = e1000_1000t_rx_status_ok;
> +
> +out:
> +	return ret_val;
> +}
> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.h b/drivers/net/ethernet/intel/igb/e1000_phy.h
> index 5894e4b1d0a8..aa888efc05f2 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
> @@ -41,6 +41,8 @@ s32  igb_phy_has_link(struct e1000_hw *hw, u32 iterations,
>   void igb_power_up_phy_copper(struct e1000_hw *hw);
>   void igb_power_down_phy_copper(struct e1000_hw *hw);
>   s32  igb_phy_init_script_igp3(struct e1000_hw *hw);
> +s32  igb_phy_init_script_5461s(struct e1000_hw *hw);
> +s32  igb_get_phy_info_5461s(struct e1000_hw *hw);
>   s32  igb_initialize_M88E1512_phy(struct e1000_hw *hw);
>   s32  igb_initialize_M88E1543_phy(struct e1000_hw *hw);
>   s32  igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 5fc2c381da55..275fac4cbf63 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -8923,11 +8923,19 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>   		data->phy_id = adapter->hw.phy.addr;
>   		break;
>   	case SIOCGMIIREG:
> +		adapter->hw.phy.addr = data->phy_id;

How is this related? No idea, why this is added. Jeffrey, do you remember?

>   		if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
>   				     &data->val_out))
>   			return -EIO;
>   		break;
>   	case SIOCSMIIREG:
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +		adapter->hw.phy.addr = data->phy_id;
> +		if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F,
> +				      data->val_in))
> +			return -EIO;
> +		break;

This looks also like an unrelated improvement. Maybe the igb folks could 
comment, if this is useful. Jeffrey, do you remember, what this is 
needed for?

>   	default:
>   		return -EOPNOTSUPP;
>   	}
> 


Kind regards,

Paul

[Intel-wired-lan] [PATCH 1/2] ethernet: igb: Support PHY BCM5461S

[Paul Menzel]

Dear Linux folks,


Am 03.11.20 um 00:13 schrieb Paul Menzel:
> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> The BCM5461S PHY is used in switches.
> 
> The patch is taken from Open Network Linux, and it was added there as
> patch
> 
>      packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
> 
> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
> support BCM54616 PHY) in 2017.
> 
> I applied the forward-ported
> 
>      packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
> 
> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
> 
> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331
> 
> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> Cc: John W Linville <linville@tuxdriver.com>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>   drivers/net/ethernet/intel/igb/e1000_82575.c  | 23 +++++-
>   .../net/ethernet/intel/igb/e1000_defines.h    |  1 +
>   drivers/net/ethernet/intel/igb/e1000_hw.h     |  1 +
>   drivers/net/ethernet/intel/igb/e1000_phy.c    | 77 +++++++++++++++++++
>   drivers/net/ethernet/intel/igb/e1000_phy.h    |  2 +
>   drivers/net/ethernet/intel/igb/igb_main.c     |  8 ++
>   6 files changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index 50863fd87d53..83c14ae689b1 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -308,6 +308,12 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
>   		phy->ops.set_d3_lplu_state = igb_set_d3_lplu_state_82580;
>   		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_m88;
>   		break;
> +	case BCM5461S_PHY_ID:
> +		phy->type		= e1000_phy_bcm5461s;

Do not align the = with the one on the line below.

> +		phy->ops.check_polarity	= NULL;
> +		phy->ops.get_cable_length = NULL;
> +		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
> +		break;
>   	case BCM54616_E_PHY_ID:
>   		phy->type = e1000_phy_bcm54616;
>   		break;
> @@ -866,6 +872,16 @@ static s32 igb_get_phy_id_82575(struct e1000_hw *hw)
>   			goto out;
>   		}
>   		ret_val = igb_get_phy_id(hw);
> +		if (ret_val && hw->mac.type == e1000_i354) {
> +			/* we do a special check for bcm5461s phy by setting
> +			 * the phy->addr to 5 and doing the phy check again. This
> +			 * call will succeed and retrieve a valid phy id if we have
> +			 * the bcm5461s phy
> +			 */
> +			phy->addr = 5;
> +			phy->type = e1000_phy_bcm5461s;
> +			ret_val = igb_get_phy_id(hw);
> +		}
>   		goto out;
>   	}
>   
> @@ -1253,6 +1269,9 @@ static s32 igb_get_cfg_done_82575(struct e1000_hw *hw)
>   	    (hw->phy.type == e1000_phy_igp_3))
>   		igb_phy_init_script_igp3(hw);
>   
> +	if (hw->phy.type == e1000_phy_bcm5461s)
> +		igb_phy_init_script_5461s(hw);
> +
>   	return 0;
>   }
>   
> @@ -1582,6 +1601,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
>   	case e1000_i350:
>   	case e1000_i210:
>   	case e1000_i211:
> +	case e1000_i354:

Any idea, why e1000_i350 is at the top?

>   		phpm_reg = rd32(E1000_82580_PHY_POWER_MGMT);
>   		phpm_reg &= ~E1000_82580_PM_GO_LINKD;
>   		wr32(E1000_82580_PHY_POWER_MGMT, phpm_reg);
> @@ -1627,7 +1647,8 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
>   		ret_val = igb_copper_link_setup_82580(hw);
>   		break;
>   	case e1000_phy_bcm54616:
> -		ret_val = 0;
> +		break;
> +	case e1000_phy_bcm5461s:
>   		break;

John, any idea, why you did not upstream the `ret_val = 0` line?

>   	default:
>   		ret_val = -E1000_ERR_PHY;
> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
> index d2e2c50ce257..0561ef6cb29c 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -886,6 +886,7 @@
>   #define M88E1543_E_PHY_ID    0x01410EA0
>   #define M88E1512_E_PHY_ID    0x01410DD0
>   #define BCM54616_E_PHY_ID    0x03625D10
> +#define BCM5461S_PHY_ID      0x002060C0

Should this be `BCM5461S_E_PHY_ID` for consistency? I have no idea, what 
`_E` means?

>   
>   /* M88E1000 Specific Registers */
>   #define M88E1000_PHY_SPEC_CTRL     0x10  /* PHY Specific Control Register */
> diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
> index 5d87957b2627..a660675d6218 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
> @@ -110,6 +110,7 @@ enum e1000_phy_type {
>   	e1000_phy_82580,
>   	e1000_phy_i210,
>   	e1000_phy_bcm54616,
> +	e1000_phy_bcm5461s,
>   };
>   
>   enum e1000_bus_type {
> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
> index 8c8eb82e6272..4e0b4ba09a00 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -126,6 +126,13 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
>   	 * Control register.  The MAC will take care of interfacing with the
>   	 * PHY to retrieve the desired data.
>   	 */
> +	if (phy->type == e1000_phy_bcm5461s) {
> +		mdic = rd32(E1000_MDICNFG);
> +		mdic &= ~E1000_MDICNFG_PHY_MASK;
> +		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
> +		wr32(E1000_MDICNFG, mdic);
> +	}
> +
>   	mdic = ((offset << E1000_MDIC_REG_SHIFT) |
>   		(phy->addr << E1000_MDIC_PHY_SHIFT) |
>   		(E1000_MDIC_OP_READ));
> @@ -182,6 +189,13 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
>   	 * Control register.  The MAC will take care of interfacing with the
>   	 * PHY to retrieve the desired data.
>   	 */
> +	if (phy->type == e1000_phy_bcm5461s) {
> +		mdic = rd32(E1000_MDICNFG);
> +		mdic &= ~E1000_MDICNFG_PHY_MASK;
> +		mdic |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
> +		wr32(E1000_MDICNFG, mdic);
> +	}
> +
>   	mdic = (((u32)data) |
>   		(offset << E1000_MDIC_REG_SHIFT) |
>   		(phy->addr << E1000_MDIC_PHY_SHIFT) |
> @@ -2628,3 +2642,66 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
>   
>   	return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
>   }
> +
> +/**
> + *  igb_phy_init_script_5461s - Inits the BCM5461S PHY
> + *  @hw: pointer to the HW structure
> + *
> + *  Initializes a Broadcom Gigabit PHY.
> + **/
> +s32 igb_phy_init_script_5461s(struct e1000_hw *hw)
> +{
> +	u16 mii_reg_led = 0;
> +
> +	/* 1. Speed LED (Set the Link LED mode), Shadow 00010, 0x1C.bit2=1 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x0800);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0004;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +
> +	/* 2. Active LED (Set the Link LED mode), Shadow 01001, 0x1C.bit4=1, 0x10.bit5=0 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x2400);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0010;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +	hw->phy.ops.read_reg(hw, 0x10, &mii_reg_led);
> +	mii_reg_led &= 0xffdf;
> +	hw->phy.ops.write_reg(hw, 0x10, mii_reg_led);
> +
> +	return 0;
> +}
> +
> +/**
> + *  igb_get_phy_info_5461s - Retrieve 5461s PHY information
> + *  @hw: pointer to the HW structure
> + *
> + *  Read PHY status to determine if link is up.  If link is up, then
> + *  set/determine 10base-T extended distance and polarity correction.  Read
> + *  PHY port status to determine MDI/MDIx and speed.  Based on the speed,
> + *  determine on the cable length, local and remote receiver.
> + **/
> +s32 igb_get_phy_info_5461s(struct e1000_hw *hw)
> +{
> +	struct e1000_phy_info *phy = &hw->phy;
> +	s32 ret_val;
> +	bool link;
> +
> +	ret_val = igb_phy_has_link(hw, 1, 0, &link);
> +	if (ret_val)
> +		goto out;
> +
> +	if (!link) {
> +		ret_val = -E1000_ERR_CONFIG;
> +		goto out;
> +	}
> +
> +	phy->polarity_correction = true;
> +
> +	phy->is_mdix = true;
> +	phy->cable_length = E1000_CABLE_LENGTH_UNDEFINED;
> +	phy->local_rx = e1000_1000t_rx_status_ok;
> +	phy->remote_rx = e1000_1000t_rx_status_ok;
> +
> +out:
> +	return ret_val;
> +}
> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.h b/drivers/net/ethernet/intel/igb/e1000_phy.h
> index 5894e4b1d0a8..aa888efc05f2 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
> @@ -41,6 +41,8 @@ s32  igb_phy_has_link(struct e1000_hw *hw, u32 iterations,
>   void igb_power_up_phy_copper(struct e1000_hw *hw);
>   void igb_power_down_phy_copper(struct e1000_hw *hw);
>   s32  igb_phy_init_script_igp3(struct e1000_hw *hw);
> +s32  igb_phy_init_script_5461s(struct e1000_hw *hw);
> +s32  igb_get_phy_info_5461s(struct e1000_hw *hw);
>   s32  igb_initialize_M88E1512_phy(struct e1000_hw *hw);
>   s32  igb_initialize_M88E1543_phy(struct e1000_hw *hw);
>   s32  igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 5fc2c381da55..275fac4cbf63 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -8923,11 +8923,19 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>   		data->phy_id = adapter->hw.phy.addr;
>   		break;
>   	case SIOCGMIIREG:
> +		adapter->hw.phy.addr = data->phy_id;

How is this related? No idea, why this is added. Jeffrey, do you remember?

>   		if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
>   				     &data->val_out))
>   			return -EIO;
>   		break;
>   	case SIOCSMIIREG:
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +		adapter->hw.phy.addr = data->phy_id;
> +		if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F,
> +				      data->val_in))
> +			return -EIO;
> +		break;

This looks also like an unrelated improvement. Maybe the igb folks could 
comment, if this is useful. Jeffrey, do you remember, what this is 
needed for?

>   	default:
>   		return -EOPNOTSUPP;
>   	}
> 


Kind regards,

Paul

Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Jakub Kicinski]

On Tue,  3 Nov 2020 00:13:07 +0100 Paul Menzel wrote:
> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> The ops field might no be defined, so add a check.

This change should be first, otherwise AFAIU if someone builds the
kernel in between the commits (e.g. for bisection) it will crash.

> The patch is taken from Open Network Linux (ONL), and it was added there
> as part of the patch
> 
>     packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
> 
> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
> support BCM54616 PHY) in 2017.
> 
> I applied the forward-ported
> 
>     packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
> 
> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
> 
> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331

No need to put this in every commit message.

We preserve the cover letter in tree as a merge commit message, so
explaining things once in the cover letter is sufficient.

> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

Jefferey will need to provide a sign-off as the author.

> Cc: John W Linville <linville@tuxdriver.com>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Jakub Kicinski]

On Tue,  3 Nov 2020 00:13:07 +0100 Paul Menzel wrote:
> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> The ops field might no be defined, so add a check.

This change should be first, otherwise AFAIU if someone builds the
kernel in between the commits (e.g. for bisection) it will crash.

> The patch is taken from Open Network Linux (ONL), and it was added there
> as part of the patch
> 
>     packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
> 
> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
> support BCM54616 PHY) in 2017.
> 
> I applied the forward-ported
> 
>     packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
> 
> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
> 
> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331

No need to put this in every commit message.

We preserve the cover letter in tree as a merge commit message, so
explaining things once in the cover letter is sufficient.

> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>

Jefferey will need to provide a sign-off as the author.

> Cc: John W Linville <linville@tuxdriver.com>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>

Re: [PATCH 1/2] ethernet: igb: Support PHY BCM5461S

[Florian Fainelli]

On 11/2/2020 3:13 PM, Paul Menzel wrote:
> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> The BCM5461S PHY is used in switches.
> 
> The patch is taken from Open Network Linux, and it was added there as
> patch
> 
>     packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
> 
> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
> support BCM54616 PHY) in 2017.
> 
> I applied the forward-ported
> 
>     packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
> 
> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
> 
> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331
> 
> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> Cc: John W Linville <linville@tuxdriver.com>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---

[snip]

> +
> +/**
> + *  igb_phy_init_script_5461s - Inits the BCM5461S PHY
> + *  @hw: pointer to the HW structure
> + *
> + *  Initializes a Broadcom Gigabit PHY.
> + **/
> +s32 igb_phy_init_script_5461s(struct e1000_hw *hw)
> +{
> +	u16 mii_reg_led = 0;
> +
> +	/* 1. Speed LED (Set the Link LED mode), Shadow 00010, 0x1C.bit2=1 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x0800);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0004;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +
> +	/* 2. Active LED (Set the Link LED mode), Shadow 01001, 0x1C.bit4=1, 0x10.bit5=0 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x2400);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0010;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +	hw->phy.ops.read_reg(hw, 0x10, &mii_reg_led);
> +	mii_reg_led &= 0xffdf;
> +	hw->phy.ops.write_reg(hw, 0x10, mii_reg_led);

Please try at least to re-use the definitions from
include/linux/brcmphy.h and add new ones where appropriate.

It is already painful enough to see that Intel does not use the PHY
library, there is no need to add insult to the injury by open coding all
of these register addresses and values.

Thanks
-- 
Florian

[Intel-wired-lan] [PATCH 1/2] ethernet: igb: Support PHY BCM5461S

[Florian Fainelli]

On 11/2/2020 3:13 PM, Paul Menzel wrote:
> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> The BCM5461S PHY is used in switches.
> 
> The patch is taken from Open Network Linux, and it was added there as
> patch
> 
>     packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
> 
> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
> support BCM54616 PHY) in 2017.
> 
> I applied the forward-ported
> 
>     packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
> 
> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
> 
> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331
> 
> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> Cc: John W Linville <linville@tuxdriver.com>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---

[snip]

> +
> +/**
> + *  igb_phy_init_script_5461s - Inits the BCM5461S PHY
> + *  @hw: pointer to the HW structure
> + *
> + *  Initializes a Broadcom Gigabit PHY.
> + **/
> +s32 igb_phy_init_script_5461s(struct e1000_hw *hw)
> +{
> +	u16 mii_reg_led = 0;
> +
> +	/* 1. Speed LED (Set the Link LED mode), Shadow 00010, 0x1C.bit2=1 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x0800);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0004;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +
> +	/* 2. Active LED (Set the Link LED mode), Shadow 01001, 0x1C.bit4=1, 0x10.bit5=0 */
> +	hw->phy.ops.write_reg(hw, 0x1C, 0x2400);
> +	hw->phy.ops.read_reg(hw, 0x1C, &mii_reg_led);
> +	mii_reg_led |= 0x0010;
> +	hw->phy.ops.write_reg(hw, 0x1C, mii_reg_led | 0x8000);
> +	hw->phy.ops.read_reg(hw, 0x10, &mii_reg_led);
> +	mii_reg_led &= 0xffdf;
> +	hw->phy.ops.write_reg(hw, 0x10, mii_reg_led);

Please try at least to re-use the definitions from
include/linux/brcmphy.h and add new ones where appropriate.

It is already painful enough to see that Intel does not use the PHY
library, there is no need to add insult to the injury by open coding all
of these register addresses and values.

Thanks
-- 
Florian

Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

Dear Jakub,


Am 03.11.20 um 01:19 schrieb Jakub Kicinski:
> On Tue,  3 Nov 2020 00:13:07 +0100 Paul Menzel wrote:
>> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
>>
>> The ops field might no be defined, so add a check.
> 
> This change should be first, otherwise AFAIU if someone builds the
> kernel in between the commits (e.g. for bisection) it will crash.

Patch `[PATCH 1/2] ethernet: igb: Support PHY BCM5461S` has

     phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;

so the ordering does not matter. I do not know, if Jeffrey can comment, 
but probably the check was just adding during development. Maybe an 
assert should be added instead?

>> The patch is taken from Open Network Linux (ONL), and it was added there
>> as part of the patch
>>
>>      packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
>>
>> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
>> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
>> support BCM54616 PHY) in 2017.
>>
>> I applied the forward-ported
>>
>>      packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
>>
>> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
>>
>> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
>> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331
> 
> No need to put this in every commit message.
> 
> We preserve the cover letter in tree as a merge commit message, so
> explaining things once in the cover letter is sufficient.

I remember, but still find it confusing. If I look at a commit with `git 
show …`, I normally do not think of also looking at a possible cover 
letter as not many subsystems/projects do it, and I assume a commit is 
self-contained.

Could you share your development process

>> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> Jefferey will need to provide a sign-off as the author.

According to *Developer's Certificate of Origin 1.1* [3], it’s my 
understanding, that it is *not* required. The items (a), (b), and (c) 
are connected by an *or*.

>         (b) The contribution is based upon previous work that, to the best
>             of my knowledge, is covered under an appropriate open source
>             license and I have the right under that license to submit that
>             work with modifications, whether created in whole or in part 
>             by me, under the same open source license (unless I am
>             permitted to submit under a different license), as indicated
>             in the file; or

>> Cc: John W Linville <linville@tuxdriver.com>
>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul


[3]: 
https://www.kernel.org/doc/html/v5.9/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

Dear Jakub,


Am 03.11.20 um 01:19 schrieb Jakub Kicinski:
> On Tue,  3 Nov 2020 00:13:07 +0100 Paul Menzel wrote:
>> From: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
>>
>> The ops field might no be defined, so add a check.
> 
> This change should be first, otherwise AFAIU if someone builds the
> kernel in between the commits (e.g. for bisection) it will crash.

Patch `[PATCH 1/2] ethernet: igb: Support PHY BCM5461S` has

     phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;

so the ordering does not matter. I do not know, if Jeffrey can comment, 
but probably the check was just adding during development. Maybe an 
assert should be added instead?

>> The patch is taken from Open Network Linux (ONL), and it was added there
>> as part of the patch
>>
>>      packages/base/any/kernels/3.16+deb8/patches/driver-support-intel-igb-bcm5461X-phy.patch
>>
>> in ONL commit f32316c63c (Support the BCM54616 and BCM5461S.) [1]. Part
>> of this commit was already upstreamed in Linux commit eeb0149660 (igb:
>> support BCM54616 PHY) in 2017.
>>
>> I applied the forward-ported
>>
>>      packages/base/any/kernels/5.4-lts/patches/0002-driver-support-intel-igb-bcm5461S-phy.patch
>>
>> added in ONL commit 5ace6bcdb3 (Add 5.4 LTS kernel build.) [2].
>>
>> [1]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/f32316c63ce3a64de125b7429115c6d45e942bd1
>> [2]: https://github.com/opencomputeproject/OpenNetworkLinux/commit/5ace6bcdb37cb8065dcd1d4404b3dcb6424f6331
> 
> No need to put this in every commit message.
> 
> We preserve the cover letter in tree as a merge commit message, so
> explaining things once in the cover letter is sufficient.

I remember, but still find it confusing. If I look at a commit with `git 
show ?`, I normally do not think of also looking at a possible cover 
letter as not many subsystems/projects do it, and I assume a commit is 
self-contained.

Could you share your development process

>> Cc: Jeffrey Townsend <jeffrey.townsend@bigswitch.com>
> 
> Jefferey will need to provide a sign-off as the author.

According to *Developer's Certificate of Origin 1.1* [3], it?s my 
understanding, that it is *not* required. The items (a), (b), and (c) 
are connected by an *or*.

>         (b) The contribution is based upon previous work that, to the best
>             of my knowledge, is covered under an appropriate open source
>             license and I have the right under that license to submit that
>             work with modifications, whether created in whole or in part 
>             by me, under the same open source license (unless I am
>             permitted to submit under a different license), as indicated
>             in the file; or

>> Cc: John W Linville <linville@tuxdriver.com>
>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul


[3]: 
https://www.kernel.org/doc/html/v5.9/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Jakub Kicinski]

On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
> According to *Developer's Certificate of Origin 1.1* [3], it’s my 
> understanding, that it is *not* required. The items (a), (b), and (c) 
> are connected by an *or*.
> 
> >         (b) The contribution is based upon previous work that, to the best
> >             of my knowledge, is covered under an appropriate open source
> >             license and I have the right under that license to submit that
> >             work with modifications, whether created in whole or in part 
> >             by me, under the same open source license (unless I am
> >             permitted to submit under a different license), as indicated
> >             in the file; or  

Ack, but then you need to put yourself as the author, because it's
you certifying that the code falls under (b).

At least that's my understanding.

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Jakub Kicinski]

On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
> According to *Developer's Certificate of Origin 1.1* [3], it?s my 
> understanding, that it is *not* required. The items (a), (b), and (c) 
> are connected by an *or*.
> 
> >         (b) The contribution is based upon previous work that, to the best
> >             of my knowledge, is covered under an appropriate open source
> >             license and I have the right under that license to submit that
> >             work with modifications, whether created in whole or in part 
> >             by me, under the same open source license (unless I am
> >             permitted to submit under a different license), as indicated
> >             in the file; or  

Ack, but then you need to put yourself as the author, because it's
you certifying that the code falls under (b).

At least that's my understanding.

Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

Dear Jakub, dear Greg,


Am 03.11.20 um 19:39 schrieb Jakub Kicinski:
> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
>> According to *Developer's Certificate of Origin 1.1* [3], it’s my
>> understanding, that it is *not* required. The items (a), (b), and (c)
>> are connected by an *or*.
>>
>>>          (b) The contribution is based upon previous work that, to the best
>>>              of my knowledge, is covered under an appropriate open source
>>>              license and I have the right under that license to submit that
>>>              work with modifications, whether created in whole or in part
>>>              by me, under the same open source license (unless I am
>>>              permitted to submit under a different license), as indicated
>>>              in the file; or
> 
> Ack, but then you need to put yourself as the author, because it's
> you certifying that the code falls under (b).
> 
> At least that's my understanding.

Greg, can you please clarify, if it’s fine, if I upstream a patch 
authored by somebody else and distributed under the GPLv2? I put them as 
the author and signed it off.

(In this case the change, adding an if condition, is also trivial.)


Kind regards,

Paul

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

Dear Jakub, dear Greg,


Am 03.11.20 um 19:39 schrieb Jakub Kicinski:
> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
>> According to *Developer's Certificate of Origin 1.1* [3], it?s my
>> understanding, that it is *not* required. The items (a), (b), and (c)
>> are connected by an *or*.
>>
>>>          (b) The contribution is based upon previous work that, to the best
>>>              of my knowledge, is covered under an appropriate open source
>>>              license and I have the right under that license to submit that
>>>              work with modifications, whether created in whole or in part
>>>              by me, under the same open source license (unless I am
>>>              permitted to submit under a different license), as indicated
>>>              in the file; or
> 
> Ack, but then you need to put yourself as the author, because it's
> you certifying that the code falls under (b).
> 
> At least that's my understanding.

Greg, can you please clarify, if it?s fine, if I upstream a patch 
authored by somebody else and distributed under the GPLv2? I put them as 
the author and signed it off.

(In this case the change, adding an if condition, is also trivial.)


Kind regards,

Paul

Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Greg KH]

On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:
> Dear Jakub, dear Greg,
> 
> 
> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:
> > On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
> > > According to *Developer's Certificate of Origin 1.1* [3], it’s my
> > > understanding, that it is *not* required. The items (a), (b), and (c)
> > > are connected by an *or*.
> > > 
> > > >          (b) The contribution is based upon previous work that, to the best
> > > >              of my knowledge, is covered under an appropriate open source
> > > >              license and I have the right under that license to submit that
> > > >              work with modifications, whether created in whole or in part
> > > >              by me, under the same open source license (unless I am
> > > >              permitted to submit under a different license), as indicated
> > > >              in the file; or
> > 
> > Ack, but then you need to put yourself as the author, because it's
> > you certifying that the code falls under (b).
> > 
> > At least that's my understanding.
> 
> Greg, can you please clarify, if it’s fine, if I upstream a patch authored
> by somebody else and distributed under the GPLv2? I put them as the author
> and signed it off.

You can't add someone else's signed-off-by, but you can add your own and
keep them as the author, has happened lots of time in the past.

Or, you can make the From: line be from you if the original author
doesn't want their name/email in the changelog, we've done that as well,
both are fine.

thanks,

greg k-h

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Greg KH]

On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:
> Dear Jakub, dear Greg,
> 
> 
> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:
> > On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
> > > According to *Developer's Certificate of Origin 1.1* [3], it?s my
> > > understanding, that it is *not* required. The items (a), (b), and (c)
> > > are connected by an *or*.
> > > 
> > > >          (b) The contribution is based upon previous work that, to the best
> > > >              of my knowledge, is covered under an appropriate open source
> > > >              license and I have the right under that license to submit that
> > > >              work with modifications, whether created in whole or in part
> > > >              by me, under the same open source license (unless I am
> > > >              permitted to submit under a different license), as indicated
> > > >              in the file; or
> > 
> > Ack, but then you need to put yourself as the author, because it's
> > you certifying that the code falls under (b).
> > 
> > At least that's my understanding.
> 
> Greg, can you please clarify, if it?s fine, if I upstream a patch authored
> by somebody else and distributed under the GPLv2? I put them as the author
> and signed it off.

You can't add someone else's signed-off-by, but you can add your own and
keep them as the author, has happened lots of time in the past.

Or, you can make the From: line be from you if the original author
doesn't want their name/email in the changelog, we've done that as well,
both are fine.

thanks,

greg k-h

Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

Dear Jakub, dear Greg,


Am 05.01.21 um 18:25 schrieb Greg KH:
> On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:

>> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:
>>> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
>>>> According to *Developer's Certificate of Origin 1.1* [3], it’s my
>>>> understanding, that it is *not* required. The items (a), (b), and (c)
>>>> are connected by an *or*.
>>>>
>>>>>           (b) The contribution is based upon previous work that, to the best
>>>>>               of my knowledge, is covered under an appropriate open source
>>>>>               license and I have the right under that license to submit that
>>>>>               work with modifications, whether created in whole or in part
>>>>>               by me, under the same open source license (unless I am
>>>>>               permitted to submit under a different license), as indicated
>>>>>               in the file; or
>>>
>>> Ack, but then you need to put yourself as the author, because it's
>>> you certifying that the code falls under (b).
>>>
>>> At least that's my understanding.
>>
>> Greg, can you please clarify, if it’s fine, if I upstream a patch authored
>> by somebody else and distributed under the GPLv2? I put them as the author
>> and signed it off.
> 
> You can't add someone else's signed-off-by, but you can add your own and
> keep them as the author, has happened lots of time in the past.
> 
> Or, you can make the From: line be from you if the original author
> doesn't want their name/email in the changelog, we've done that as well,
> both are fine.

Greg, thank you for the clarification.

Jakub, with that out of the way, can you please take patch 2/2?


Kind regards,

Paul

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Paul Menzel]

Dear Jakub, dear Greg,


Am 05.01.21 um 18:25 schrieb Greg KH:
> On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:

>> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:
>>> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:
>>>> According to *Developer's Certificate of Origin 1.1* [3], it?s my
>>>> understanding, that it is *not* required. The items (a), (b), and (c)
>>>> are connected by an *or*.
>>>>
>>>>>           (b) The contribution is based upon previous work that, to the best
>>>>>               of my knowledge, is covered under an appropriate open source
>>>>>               license and I have the right under that license to submit that
>>>>>               work with modifications, whether created in whole or in part
>>>>>               by me, under the same open source license (unless I am
>>>>>               permitted to submit under a different license), as indicated
>>>>>               in the file; or
>>>
>>> Ack, but then you need to put yourself as the author, because it's
>>> you certifying that the code falls under (b).
>>>
>>> At least that's my understanding.
>>
>> Greg, can you please clarify, if it?s fine, if I upstream a patch authored
>> by somebody else and distributed under the GPLv2? I put them as the author
>> and signed it off.
> 
> You can't add someone else's signed-off-by, but you can add your own and
> keep them as the author, has happened lots of time in the past.
> 
> Or, you can make the From: line be from you if the original author
> doesn't want their name/email in the changelog, we've done that as well,
> both are fine.

Greg, thank you for the clarification.

Jakub, with that out of the way, can you please take patch 2/2?


Kind regards,

Paul

Re: [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Jakub Kicinski]

On Tue, 19 Jan 2021 07:55:19 +0100 Paul Menzel wrote:
> Am 05.01.21 um 18:25 schrieb Greg KH:
> > On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:  
> >> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:  
> >>> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:  
> >>>> According to *Developer's Certificate of Origin 1.1* [3], it’s my
> >>>> understanding, that it is *not* required. The items (a), (b), and (c)
> >>>> are connected by an *or*.
> >>>>  
> >>>>>           (b) The contribution is based upon previous work that, to the best
> >>>>>               of my knowledge, is covered under an appropriate open source
> >>>>>               license and I have the right under that license to submit that
> >>>>>               work with modifications, whether created in whole or in part
> >>>>>               by me, under the same open source license (unless I am
> >>>>>               permitted to submit under a different license), as indicated
> >>>>>               in the file; or  
> >>>
> >>> Ack, but then you need to put yourself as the author, because it's
> >>> you certifying that the code falls under (b).
> >>>
> >>> At least that's my understanding.  
> >>
> >> Greg, can you please clarify, if it’s fine, if I upstream a patch authored
> >> by somebody else and distributed under the GPLv2? I put them as the author
> >> and signed it off.  
> > 
> > You can't add someone else's signed-off-by, but you can add your own and
> > keep them as the author, has happened lots of time in the past.
> > 
> > Or, you can make the From: line be from you if the original author
> > doesn't want their name/email in the changelog, we've done that as well,
> > both are fine.  
> 
> Greg, thank you for the clarification.
> 
> Jakub, with that out of the way, can you please take patch 2/2?

Please repost the patches, if you know how please add a lore link to
this posting, thanks!

[Intel-wired-lan] [PATCH 2/2] ethernet: igb: e1000_phy: Check for ops.force_speed_duplex existence

[Jakub Kicinski]

On Tue, 19 Jan 2021 07:55:19 +0100 Paul Menzel wrote:
> Am 05.01.21 um 18:25 schrieb Greg KH:
> > On Tue, Jan 05, 2021 at 06:16:59PM +0100, Paul Menzel wrote:  
> >> Am 03.11.20 um 19:39 schrieb Jakub Kicinski:  
> >>> On Tue, 3 Nov 2020 08:35:09 +0100 Paul Menzel wrote:  
> >>>> According to *Developer's Certificate of Origin 1.1* [3], it?s my
> >>>> understanding, that it is *not* required. The items (a), (b), and (c)
> >>>> are connected by an *or*.
> >>>>  
> >>>>>           (b) The contribution is based upon previous work that, to the best
> >>>>>               of my knowledge, is covered under an appropriate open source
> >>>>>               license and I have the right under that license to submit that
> >>>>>               work with modifications, whether created in whole or in part
> >>>>>               by me, under the same open source license (unless I am
> >>>>>               permitted to submit under a different license), as indicated
> >>>>>               in the file; or  
> >>>
> >>> Ack, but then you need to put yourself as the author, because it's
> >>> you certifying that the code falls under (b).
> >>>
> >>> At least that's my understanding.  
> >>
> >> Greg, can you please clarify, if it?s fine, if I upstream a patch authored
> >> by somebody else and distributed under the GPLv2? I put them as the author
> >> and signed it off.  
> > 
> > You can't add someone else's signed-off-by, but you can add your own and
> > keep them as the author, has happened lots of time in the past.
> > 
> > Or, you can make the From: line be from you if the original author
> > doesn't want their name/email in the changelog, we've done that as well,
> > both are fine.  
> 
> Greg, thank you for the clarification.
> 
> Jakub, with that out of the way, can you please take patch 2/2?

Please repost the patches, if you know how please add a lore link to
this posting, thanks!