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
From: Jeffrey Townsend <[email protected]>
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 <[email protected]>
Cc: John W Linville <[email protected]>
Signed-off-by: Paul Menzel <[email protected]>
---
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
From: Jeffrey Townsend <[email protected]>
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 <[email protected]>
Cc: John W Linville <[email protected]>
Signed-off-by: Paul Menzel <[email protected]>
---
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
Dear Linux folks,
Am 03.11.20 um 00:13 schrieb Paul Menzel:
> From: Jeffrey Townsend <[email protected]>
>
> 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 <[email protected]>
> Cc: John W Linville <[email protected]>
> Signed-off-by: Paul Menzel <[email protected]>
> ---
> 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
On Tue, 3 Nov 2020 00:13:07 +0100 Paul Menzel wrote:
> From: Jeffrey Townsend <[email protected]>
>
> 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 <[email protected]>
Jefferey will need to provide a sign-off as the author.
> Cc: John W Linville <[email protected]>
> Signed-off-by: Paul Menzel <[email protected]>
On 11/2/2020 3:13 PM, Paul Menzel wrote:
> From: Jeffrey Townsend <[email protected]>
>
> 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 <[email protected]>
> Cc: John W Linville <[email protected]>
> Signed-off-by: Paul Menzel <[email protected]>
> ---
[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
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 <[email protected]>
>>
>> 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 <[email protected]>
>
> 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 <[email protected]>
>> Signed-off-by: Paul Menzel <[email protected]>
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
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.
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
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
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
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!