* [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
@ 2015-04-17 20:23 ` Jonathan Toppins
0 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-04-17 20:23 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
From: Alan Liebthal <alanl@cumulusnetworks.com>
The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
the PHY layer. This adds support for this PHY to the Intel igb driver.
Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
drivers/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 | 79 ++++++++++++++++++++++++
drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
5 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 2dcc808..e222804 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -178,7 +178,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
ctrl_ext = rd32(E1000_CTRL_EXT);
- if (igb_sgmii_active_82575(hw)) {
+ if (igb_sgmii_active_82575(hw) && phy->type != e1000_phy_bcm5461s) {
phy->ops.reset = igb_phy_hw_reset_sgmii_82575;
ctrl_ext |= E1000_CTRL_I2C_ENA;
} else {
@@ -289,6 +289,11 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
case BCM54616_E_PHY_ID:
phy->type = e1000_phy_bcm54616;
break;
+ case BCM5461S_E_PHY_ID:
+ phy->type = e1000_phy_bcm5461s;
+ phy->ops.get_phy_info = igb_get_phy_info_5461s;
+ phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
+ break;
default:
ret_val = -E1000_ERR_PHY;
goto out;
@@ -841,6 +846,15 @@ 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;
+ ret_val = igb_get_phy_id(hw);
+ }
goto out;
}
@@ -1221,6 +1235,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;
}
@@ -1597,6 +1614,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
ret_val = igb_copper_link_setup_82580(hw);
break;
case e1000_phy_bcm54616:
+ 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 50d51e4..787224d 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -861,6 +861,7 @@
#define I210_I_PHY_ID 0x01410C00
#define M88E1543_E_PHY_ID 0x01410EA0
#define BCM54616_E_PHY_ID 0x03625D10
+#define BCM5461S_E_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 d82c96b..408a3e5 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -129,6 +129,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 c1bb64d..0f5a55b 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -148,6 +148,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));
@@ -204,6 +211,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) |
@@ -2509,3 +2523,68 @@ 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 7af4ffa..fcdd84c 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.h
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
@@ -61,6 +61,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_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data);
s32 igb_read_phy_reg_i2c(struct e1000_hw *hw, u32 offset, u16 *data);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
@ 2015-04-17 20:23 ` Jonathan Toppins
0 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-04-17 20:23 UTC (permalink / raw)
To: intel-wired-lan
From: Alan Liebthal <alanl@cumulusnetworks.com>
The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
the PHY layer. This adds support for this PHY to the Intel igb driver.
Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
drivers/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 | 79 ++++++++++++++++++++++++
drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
5 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 2dcc808..e222804 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -178,7 +178,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
ctrl_ext = rd32(E1000_CTRL_EXT);
- if (igb_sgmii_active_82575(hw)) {
+ if (igb_sgmii_active_82575(hw) && phy->type != e1000_phy_bcm5461s) {
phy->ops.reset = igb_phy_hw_reset_sgmii_82575;
ctrl_ext |= E1000_CTRL_I2C_ENA;
} else {
@@ -289,6 +289,11 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
case BCM54616_E_PHY_ID:
phy->type = e1000_phy_bcm54616;
break;
+ case BCM5461S_E_PHY_ID:
+ phy->type = e1000_phy_bcm5461s;
+ phy->ops.get_phy_info = igb_get_phy_info_5461s;
+ phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
+ break;
default:
ret_val = -E1000_ERR_PHY;
goto out;
@@ -841,6 +846,15 @@ 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;
+ ret_val = igb_get_phy_id(hw);
+ }
goto out;
}
@@ -1221,6 +1235,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;
}
@@ -1597,6 +1614,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
ret_val = igb_copper_link_setup_82580(hw);
break;
case e1000_phy_bcm54616:
+ 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 50d51e4..787224d 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -861,6 +861,7 @@
#define I210_I_PHY_ID 0x01410C00
#define M88E1543_E_PHY_ID 0x01410EA0
#define BCM54616_E_PHY_ID 0x03625D10
+#define BCM5461S_E_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 d82c96b..408a3e5 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -129,6 +129,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 c1bb64d..0f5a55b 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -148,6 +148,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));
@@ -204,6 +211,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) |
@@ -2509,3 +2523,68 @@ 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 7af4ffa..fcdd84c 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.h
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
@@ -61,6 +61,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_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data);
s32 igb_read_phy_reg_i2c(struct e1000_hw *hw, u32 offset, u16 *data);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
2015-04-17 20:23 ` [Intel-wired-lan] " Jonathan Toppins
@ 2015-04-17 20:24 ` Jonathan Toppins
-1 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-04-17 20:24 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
From: Alan Liebthal <alanl@cumulusnetworks.com>
Support setting the MII register via SIOCSMIIREG.
Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 720b785..1071a71 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
return -EIO;
break;
case SIOCSMIIREG:
+ 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;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
@ 2015-04-17 20:24 ` Jonathan Toppins
0 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-04-17 20:24 UTC (permalink / raw)
To: intel-wired-lan
From: Alan Liebthal <alanl@cumulusnetworks.com>
Support setting the MII register via SIOCSMIIREG.
Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 720b785..1071a71 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
return -EIO;
break;
case SIOCSMIIREG:
+ 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;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-04-17 20:23 ` [Intel-wired-lan] " Jonathan Toppins
@ 2015-04-27 15:44 ` Jonathan Toppins
-1 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-04-27 15:44 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
On 4/17/15 4:23 PM, Jonathan Toppins wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> the PHY layer. This adds support for this PHY to the Intel igb driver.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> drivers/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 | 79 ++++++++++++++++++++++++
> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> 5 files changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index 2dcc808..e222804 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -178,7 +178,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
>
> ctrl_ext = rd32(E1000_CTRL_EXT);
>
> - if (igb_sgmii_active_82575(hw)) {
> + if (igb_sgmii_active_82575(hw) && phy->type != e1000_phy_bcm5461s) {
> phy->ops.reset = igb_phy_hw_reset_sgmii_82575;
> ctrl_ext |= E1000_CTRL_I2C_ENA;
> } else {
> @@ -289,6 +289,11 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
> case BCM54616_E_PHY_ID:
> phy->type = e1000_phy_bcm54616;
> break;
> + case BCM5461S_E_PHY_ID:
> + phy->type = e1000_phy_bcm5461s;
> + phy->ops.get_phy_info = igb_get_phy_info_5461s;
> + phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
> + break;
> default:
> ret_val = -E1000_ERR_PHY;
> goto out;
> @@ -841,6 +846,15 @@ 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;
> + ret_val = igb_get_phy_id(hw);
> + }
> goto out;
> }
>
> @@ -1221,6 +1235,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;
> }
>
> @@ -1597,6 +1614,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
> ret_val = igb_copper_link_setup_82580(hw);
> break;
> case e1000_phy_bcm54616:
> + 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 50d51e4..787224d 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -861,6 +861,7 @@
> #define I210_I_PHY_ID 0x01410C00
> #define M88E1543_E_PHY_ID 0x01410EA0
> #define BCM54616_E_PHY_ID 0x03625D10
> +#define BCM5461S_E_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 d82c96b..408a3e5 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
> @@ -129,6 +129,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 c1bb64d..0f5a55b 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -148,6 +148,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));
> @@ -204,6 +211,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) |
> @@ -2509,3 +2523,68 @@ 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 7af4ffa..fcdd84c 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
> @@ -61,6 +61,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_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
> s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data);
> s32 igb_read_phy_reg_i2c(struct e1000_hw *hw, u32 offset, u16 *data);
>
Simple keepalive to make sure these have not been forgotten, the
patchworks entries are still in "new" state.
http://patchwork.ozlabs.org/patch/462222/
http://patchwork.ozlabs.org/patch/462223/
Regards,
-Jon
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
@ 2015-04-27 15:44 ` Jonathan Toppins
0 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-04-27 15:44 UTC (permalink / raw)
To: intel-wired-lan
On 4/17/15 4:23 PM, Jonathan Toppins wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> the PHY layer. This adds support for this PHY to the Intel igb driver.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> drivers/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 | 79 ++++++++++++++++++++++++
> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> 5 files changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index 2dcc808..e222804 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -178,7 +178,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
>
> ctrl_ext = rd32(E1000_CTRL_EXT);
>
> - if (igb_sgmii_active_82575(hw)) {
> + if (igb_sgmii_active_82575(hw) && phy->type != e1000_phy_bcm5461s) {
> phy->ops.reset = igb_phy_hw_reset_sgmii_82575;
> ctrl_ext |= E1000_CTRL_I2C_ENA;
> } else {
> @@ -289,6 +289,11 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
> case BCM54616_E_PHY_ID:
> phy->type = e1000_phy_bcm54616;
> break;
> + case BCM5461S_E_PHY_ID:
> + phy->type = e1000_phy_bcm5461s;
> + phy->ops.get_phy_info = igb_get_phy_info_5461s;
> + phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
> + break;
> default:
> ret_val = -E1000_ERR_PHY;
> goto out;
> @@ -841,6 +846,15 @@ 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;
> + ret_val = igb_get_phy_id(hw);
> + }
> goto out;
> }
>
> @@ -1221,6 +1235,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;
> }
>
> @@ -1597,6 +1614,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
> ret_val = igb_copper_link_setup_82580(hw);
> break;
> case e1000_phy_bcm54616:
> + 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 50d51e4..787224d 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -861,6 +861,7 @@
> #define I210_I_PHY_ID 0x01410C00
> #define M88E1543_E_PHY_ID 0x01410EA0
> #define BCM54616_E_PHY_ID 0x03625D10
> +#define BCM5461S_E_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 d82c96b..408a3e5 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
> @@ -129,6 +129,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 c1bb64d..0f5a55b 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -148,6 +148,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));
> @@ -204,6 +211,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) |
> @@ -2509,3 +2523,68 @@ 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 7af4ffa..fcdd84c 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
> @@ -61,6 +61,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_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
> s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data);
> s32 igb_read_phy_reg_i2c(struct e1000_hw *hw, u32 offset, u16 *data);
>
Simple keepalive to make sure these have not been forgotten, the
patchworks entries are still in "new" state.
http://patchwork.ozlabs.org/patch/462222/
http://patchwork.ozlabs.org/patch/462223/
Regards,
-Jon
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-04-27 15:44 ` [Intel-wired-lan] " Jonathan Toppins
@ 2015-05-02 1:45 ` Brown, Aaron F
-1 siblings, 0 replies; 36+ messages in thread
From: Brown, Aaron F @ 2015-05-02 1:45 UTC (permalink / raw)
To: Jonathan Toppins, Kirsher, Jeffrey T
Cc: Brandeburg, Jesse, Nelson, Shannon, Wyborny, Carolyn, Skidmore,
Donald C, Vick, Matthew, Ronciak, John, Williams, Mitch A,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Monday, April 27, 2015 8:45 AM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; gospo@cumulusnetworks.com;
> shm@cumulusnetworks.com; Alan Liebthal
> Subject: Re: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom
> 5461S
>
> On 4/17/15 4:23 PM, Jonathan Toppins wrote:
> > From: Alan Liebthal <alanl@cumulusnetworks.com>
> >
> > The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> > the PHY layer. This adds support for this PHY to the Intel igb driver.
> >
> > Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> > Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> > ---
> > drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> > drivers/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 | 79
> ++++++++++++++++++++++++
> > drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> > 5 files changed, 102 insertions(+), 1 deletion(-)
> >
I don't have access to the Broadcom 5461s, but will assume Jonathan / Alan checked this on it on their end. My set of regression systems did not have any problems with this, so...
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
...
...
> Simple keepalive to make sure these have not been forgotten, the
> patchworks entries are still in "new" state.
>
> http://patchwork.ozlabs.org/patch/462222/
> http://patchwork.ozlabs.org/patch/462223/
>
> Regards,
> -Jon
Nope, not forgotten, I've just been really busy with other things. I'll take a closer look at that other patch (2/2) next early next week.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
@ 2015-05-02 1:45 ` Brown, Aaron F
0 siblings, 0 replies; 36+ messages in thread
From: Brown, Aaron F @ 2015-05-02 1:45 UTC (permalink / raw)
To: intel-wired-lan
> From: netdev-owner at vger.kernel.org [mailto:netdev-owner at vger.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Monday, April 27, 2015 8:45 AM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
> lan at lists.osuosl.org; netdev at vger.kernel.org; gospo at cumulusnetworks.com;
> shm at cumulusnetworks.com; Alan Liebthal
> Subject: Re: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom
> 5461S
>
> On 4/17/15 4:23 PM, Jonathan Toppins wrote:
> > From: Alan Liebthal <alanl@cumulusnetworks.com>
> >
> > The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> > the PHY layer. This adds support for this PHY to the Intel igb driver.
> >
> > Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> > Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> > ---
> > drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> > drivers/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 | 79
> ++++++++++++++++++++++++
> > drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> > 5 files changed, 102 insertions(+), 1 deletion(-)
> >
I don't have access to the Broadcom 5461s, but will assume Jonathan / Alan checked this on it on their end. My set of regression systems did not have any problems with this, so...
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
...
...
> Simple keepalive to make sure these have not been forgotten, the
> patchworks entries are still in "new" state.
>
> http://patchwork.ozlabs.org/patch/462222/
> http://patchwork.ozlabs.org/patch/462223/
>
> Regards,
> -Jon
Nope, not forgotten, I've just been really busy with other things. I'll take a closer look at that other patch (2/2) next early next week.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
2015-04-17 20:24 ` [Intel-wired-lan] " Jonathan Toppins
@ 2015-05-05 17:25 ` Brown, Aaron F
-1 siblings, 0 replies; 36+ messages in thread
From: Brown, Aaron F @ 2015-05-05 17:25 UTC (permalink / raw)
To: Jonathan Toppins, Kirsher, Jeffrey T
Cc: Brandeburg, Jesse, Nelson, Shannon, Wyborny, Carolyn, Skidmore,
Donald C, Vick, Matthew, Ronciak, John, Williams, Mitch A,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Friday, April 17, 2015 1:24 PM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; gospo@cumulusnetworks.com;
> shm@cumulusnetworks.com; Alan Liebthal
> Subject: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
>
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> Support setting the MII register via SIOCSMIIREG.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
@ 2015-05-05 17:25 ` Brown, Aaron F
0 siblings, 0 replies; 36+ messages in thread
From: Brown, Aaron F @ 2015-05-05 17:25 UTC (permalink / raw)
To: intel-wired-lan
> From: netdev-owner at vger.kernel.org [mailto:netdev-owner at vger.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Friday, April 17, 2015 1:24 PM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
> lan at lists.osuosl.org; netdev at vger.kernel.org; gospo at cumulusnetworks.com;
> shm at cumulusnetworks.com; Alan Liebthal
> Subject: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
>
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> Support setting the MII register via SIOCSMIIREG.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
2015-05-05 17:25 ` [Intel-wired-lan] " Brown, Aaron F
@ 2015-05-05 19:31 ` Jonathan Toppins
-1 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-05 19:31 UTC (permalink / raw)
To: Brown, Aaron F, Kirsher, Jeffrey T
Cc: Brandeburg, Jesse, Nelson, Shannon, Wyborny, Carolyn, Skidmore,
Donald C, Vick, Matthew, Ronciak, John, Williams, Mitch A,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
On 5/5/15 1:25 PM, Brown, Aaron F wrote:
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
>> On Behalf Of Jonathan Toppins
>> Sent: Friday, April 17, 2015 1:24 PM
>> To: Kirsher, Jeffrey T
>> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
>> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
>> lan@lists.osuosl.org; netdev@vger.kernel.org; gospo@cumulusnetworks.com;
>> shm@cumulusnetworks.com; Alan Liebthal
>> Subject: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
>>
>> From: Alan Liebthal <alanl@cumulusnetworks.com>
>>
>> Support setting the MII register via SIOCSMIIREG.
>>
>> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>> ---
>> drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>
Thanks Aaron, appreciate the testing.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
@ 2015-05-05 19:31 ` Jonathan Toppins
0 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-05 19:31 UTC (permalink / raw)
To: intel-wired-lan
On 5/5/15 1:25 PM, Brown, Aaron F wrote:
>> From: netdev-owner at vger.kernel.org [mailto:netdev-owner at vger.kernel.org]
>> On Behalf Of Jonathan Toppins
>> Sent: Friday, April 17, 2015 1:24 PM
>> To: Kirsher, Jeffrey T
>> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
>> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
>> lan at lists.osuosl.org; netdev at vger.kernel.org; gospo at cumulusnetworks.com;
>> shm at cumulusnetworks.com; Alan Liebthal
>> Subject: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
>>
>> From: Alan Liebthal <alanl@cumulusnetworks.com>
>>
>> Support setting the MII register via SIOCSMIIREG.
>>
>> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>> ---
>> drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>
Thanks Aaron, appreciate the testing.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-04-17 20:23 ` [Intel-wired-lan] " Jonathan Toppins
` (2 preceding siblings ...)
(?)
@ 2015-05-07 11:49 ` Jeff Kirsher
2015-05-07 13:41 ` Andy Gospodarek
2015-05-07 16:32 ` Tim Harvey
-1 siblings, 2 replies; 36+ messages in thread
From: Jeff Kirsher @ 2015-05-07 11:49 UTC (permalink / raw)
To: intel-wired-lan
On Fri, 2015-04-17 at 13:23 -0700, Jonathan Toppins wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> the PHY layer. This adds support for this PHY to the Intel igb driver.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> drivers/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 | 79
> ++++++++++++++++++++++++
> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> 5 files changed, 102 insertions(+), 1 deletion(-)
After at lot of discussion on these changes, we would like to you to use
the PHYLib approach. I know that means adding PHYLib support to our
drivers, which we currently do not have.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150507/16fbf014/attachment.asc>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-07 11:49 ` Jeff Kirsher
@ 2015-05-07 13:41 ` Andy Gospodarek
2015-05-07 17:49 ` Alexander Duyck
2015-05-08 16:39 ` Ronciak, John
2015-05-07 16:32 ` Tim Harvey
1 sibling, 2 replies; 36+ messages in thread
From: Andy Gospodarek @ 2015-05-07 13:41 UTC (permalink / raw)
To: intel-wired-lan
On Thu, May 7, 2015 at 7:49 AM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Fri, 2015-04-17 at 13:23 -0700, Jonathan Toppins wrote:
>> From: Alan Liebthal <alanl@cumulusnetworks.com>
>>
>> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
>> the PHY layer. This adds support for this PHY to the Intel igb driver.
>>
>> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>> ---
>> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
>> drivers/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 | 79
>> ++++++++++++++++++++++++
>> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
>> 5 files changed, 102 insertions(+), 1 deletion(-)
>
> After at lot of discussion on these changes, we would like to you to use
> the PHYLib approach. I know that means adding PHYLib support to our
> drivers, which we currently do not have.
Jeff,
I think we would be willing to take on this task, but I would not like
that to be a gating factor for upstream acceptance of the
functionality added with this patch. I see that Aaron has commented
that no regressions were found with this patch, but based on current
status in patchwork, it looks like Dave is waiting for something a bit
more definitive before accepting it. Can you ACK it first so we have
support for this platform upstream and then we can go take a stab at
phylib support for igb?
Thanks,
-andy
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-04-17 20:23 ` [Intel-wired-lan] " Jonathan Toppins
@ 2015-05-07 16:18 ` Tim Harvey
-1 siblings, 0 replies; 36+ messages in thread
From: Tim Harvey @ 2015-05-07 16:18 UTC (permalink / raw)
To: Jonathan Toppins
Cc: jeffrey.t.kirsher, jesse.brandeburg, shannon.nelson,
carolyn.wyborny, donald.c.skidmore, matthew.vick, john.ronciak,
mitch.a.williams, intel-wired-lan, netdev, gospo, shm,
Alan Liebthal
On Fri, Apr 17, 2015 at 1:23 PM, Jonathan Toppins
<jtoppins@cumulusnetworks.com> wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> the PHY layer. This adds support for this PHY to the Intel igb driver.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
<snip>
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -148,6 +148,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));
> @@ -204,6 +211,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) |
> @@ -2509,3 +2523,68 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
>
> return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
> }
Jonathan,
Is this bcm5461s attached to an i210/i211? These changes look a lot
like some changes I'm trying to upstream to add support for i210/i211
which require the phy address in the MDICNFG register. If this is the
case, then I think the right approach is to check for hw->mac.type =
e1000_i210/e1000_i211 and I can submit my patch for review.
Regards,
Tim
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
@ 2015-05-07 16:18 ` Tim Harvey
0 siblings, 0 replies; 36+ messages in thread
From: Tim Harvey @ 2015-05-07 16:18 UTC (permalink / raw)
To: intel-wired-lan
On Fri, Apr 17, 2015 at 1:23 PM, Jonathan Toppins
<jtoppins@cumulusnetworks.com> wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> the PHY layer. This adds support for this PHY to the Intel igb driver.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
<snip>
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -148,6 +148,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));
> @@ -204,6 +211,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) |
> @@ -2509,3 +2523,68 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
>
> return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
> }
Jonathan,
Is this bcm5461s attached to an i210/i211? These changes look a lot
like some changes I'm trying to upstream to add support for i210/i211
which require the phy address in the MDICNFG register. If this is the
case, then I think the right approach is to check for hw->mac.type =
e1000_i210/e1000_i211 and I can submit my patch for review.
Regards,
Tim
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-07 11:49 ` Jeff Kirsher
2015-05-07 13:41 ` Andy Gospodarek
@ 2015-05-07 16:32 ` Tim Harvey
1 sibling, 0 replies; 36+ messages in thread
From: Tim Harvey @ 2015-05-07 16:32 UTC (permalink / raw)
To: intel-wired-lan
On Thu, May 7, 2015 at 4:49 AM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Fri, 2015-04-17 at 13:23 -0700, Jonathan Toppins wrote:
>> From: Alan Liebthal <alanl@cumulusnetworks.com>
>>
>> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
>> the PHY layer. This adds support for this PHY to the Intel igb driver.
>>
>> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>> ---
>> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
>> drivers/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 | 79
>> ++++++++++++++++++++++++
>> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
>> 5 files changed, 102 insertions(+), 1 deletion(-)
>
> After at lot of discussion on these changes, we would like to you to use
> the PHYLib approach. I know that means adding PHYLib support to our
> drivers, which we currently do not have.
>
Jeff,
Adding PHYLIB support to igb is what I was trying to do with my recent
series, by adding phy read/write functions which accept phy addr [1]
and by registering an mii_bus so that I could use a PHYLIB driver for
a switch we have connected to an i210 via MDIO [2].
I realize these didn't apply to your next-queue/dev-queue tree, but
could you give them a once-over and give me some direction on changes
I should make before I rebase them?
Regards,
Tim
[1]: http://patchwork.ozlabs.org/patch/466689/
[2]: http://patchwork.ozlabs.org/patch/466690/
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-07 16:18 ` [Intel-wired-lan] " Tim Harvey
@ 2015-05-07 16:57 ` Jonathan Toppins
-1 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-07 16:57 UTC (permalink / raw)
To: Tim Harvey
Cc: jeffrey.t.kirsher, jesse.brandeburg, shannon.nelson,
carolyn.wyborny, donald.c.skidmore, matthew.vick, john.ronciak,
mitch.a.williams, intel-wired-lan, netdev, gospo, shm,
Alan Liebthal
On 5/7/15 12:18 PM, Tim Harvey wrote:
> On Fri, Apr 17, 2015 at 1:23 PM, Jonathan Toppins
> <jtoppins@cumulusnetworks.com> wrote:
>> From: Alan Liebthal <alanl@cumulusnetworks.com>
>>
>> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
>> the PHY layer. This adds support for this PHY to the Intel igb driver.
>>
>> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>> ---
> <snip>
>> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
>> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
>> @@ -148,6 +148,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));
>> @@ -204,6 +211,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) |
>> @@ -2509,3 +2523,68 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
>>
>> return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
>> }
>
> Jonathan,
>
> Is this bcm5461s attached to an i210/i211? These changes look a lot
> like some changes I'm trying to upstream to add support for i210/i211
> which require the phy address in the MDICNFG register. If this is the
> case, then I think the right approach is to check for hw->mac.type =
> e1000_i210/e1000_i211 and I can submit my patch for review.
>
> Regards,
>
> Tim
>
Hi Tim,
The MAC in question are the ones integrated with Intel's Atom processor,
I don't recall the series off hand, output of lspci and /proc/cpuinfo
below. If you think your change may apply to this controller as well I
would be more than happy to apply your change and test.
Thanks,
-Jon
Supplementary Information.
Processor info (8 processors total):
root@qct-ly8-01:~# uname -a
Linux qct-ly8-01 3.2.60-1+deb7u1+cl2.5+1 #3.2.60-1+deb7u1+cl2.5+1 SMP
Mon Apr 13 23:18:31 PDT 2015 x86_64 GNU/Linux
root@qct-ly8-01:~# cat /proc/cpuinfo | grep "processor" | wc -l
8
root@qct-ly8-01:~# cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 77
model name : Intel(R) Atom(TM) CPU C2758 @ 2.40GHz
stepping : 8
microcode : 0x11d
cpu MHz : 2400.191
cache size : 1024 KB
physical id : 0
siblings : 8
core id : 0
cpu cores : 8
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 11
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx
rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology
nonstop_tsc aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx est tm2
ssse3 cx16 xtpr pdcm sse4_1 sse4_2 movbe popcnt tsc_deadline_timer aes
rdrand lahf_lm 3dnowprefetch arat epb dtherm tpr_shadow vnmi
flexpriority ept vpid smep erms
bogomips : 4800.38
clflush size : 64
cache_alignment : 64
address sizes : 36 bits physical, 48 bits virtual
power management:
...trimmed...
The PCI info for the controller:
root@qct-ly8-01:~# lspci -vvx -s 00:14.0
00:14.0 Ethernet controller: Intel Corporation Device 1f41 (rev 03)
Subsystem: Intel Corporation Device 1f41
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort-
<MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 20
Region 0: Memory at dff60000 (64-bit, non-prefetchable) [size=128K]
Region 2: I/O ports at 1000 [size=32]
Region 4: Memory at dff84000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
Address: 0000000000000000 Data: 0000
Masking: 00000000 Pending: 00000000
Capabilities: [70] MSI-X: Enable+ Count=10 Masked-
Vector table: BAR=4 offset=00000000
PBA: BAR=4 offset=00002000
Capabilities: [a0] Express (v2) Root Complex Integrated Endpoint, MSI 00
DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
ExtTag- RBE+ FLReset+
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
MaxPayload 128 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
LnkCap: Port #0, Speed unknown, Width x0, ASPM unknown, Latency L0
<64ns, L1 <1us
ClockPM- Surprise- LLActRep- BwNot-
LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed unknown, Width x0, TrErr- Train- SlotClk- DLActive-
BWMgmt- ABWMgmt-
DevCap2: Completion Timeout: Range ABCD, TimeoutDis+
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis-,
Selectable De-emphasis: -6dB
Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-,
EqualizationPhase1-
EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
Kernel driver in use: igb
00: 86 80 41 1f 07 04 10 00 03 00 00 02 10 00 00 00
10: 04 00 f6 df 00 00 00 00 01 10 00 00 00 00 00 00
20: 04 40 f8 df 00 00 00 00 00 00 00 00 86 80 41 1f
30: 00 00 00 00 40 00 00 00 00 00 00 00 0b 01 00 00
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
@ 2015-05-07 16:57 ` Jonathan Toppins
0 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-07 16:57 UTC (permalink / raw)
To: intel-wired-lan
On 5/7/15 12:18 PM, Tim Harvey wrote:
> On Fri, Apr 17, 2015 at 1:23 PM, Jonathan Toppins
> <jtoppins@cumulusnetworks.com> wrote:
>> From: Alan Liebthal <alanl@cumulusnetworks.com>
>>
>> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
>> the PHY layer. This adds support for this PHY to the Intel igb driver.
>>
>> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>> ---
> <snip>
>> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
>> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
>> @@ -148,6 +148,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));
>> @@ -204,6 +211,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) |
>> @@ -2509,3 +2523,68 @@ static s32 igb_set_master_slave_mode(struct e1000_hw *hw)
>>
>> return hw->phy.ops.write_reg(hw, PHY_1000T_CTRL, phy_data);
>> }
>
> Jonathan,
>
> Is this bcm5461s attached to an i210/i211? These changes look a lot
> like some changes I'm trying to upstream to add support for i210/i211
> which require the phy address in the MDICNFG register. If this is the
> case, then I think the right approach is to check for hw->mac.type =
> e1000_i210/e1000_i211 and I can submit my patch for review.
>
> Regards,
>
> Tim
>
Hi Tim,
The MAC in question are the ones integrated with Intel's Atom processor,
I don't recall the series off hand, output of lspci and /proc/cpuinfo
below. If you think your change may apply to this controller as well I
would be more than happy to apply your change and test.
Thanks,
-Jon
Supplementary Information.
Processor info (8 processors total):
root at qct-ly8-01:~# uname -a
Linux qct-ly8-01 3.2.60-1+deb7u1+cl2.5+1 #3.2.60-1+deb7u1+cl2.5+1 SMP
Mon Apr 13 23:18:31 PDT 2015 x86_64 GNU/Linux
root at qct-ly8-01:~# cat /proc/cpuinfo | grep "processor" | wc -l
8
root at qct-ly8-01:~# cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 77
model name : Intel(R) Atom(TM) CPU C2758 @ 2.40GHz
stepping : 8
microcode : 0x11d
cpu MHz : 2400.191
cache size : 1024 KB
physical id : 0
siblings : 8
core id : 0
cpu cores : 8
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 11
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx
rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology
nonstop_tsc aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx est tm2
ssse3 cx16 xtpr pdcm sse4_1 sse4_2 movbe popcnt tsc_deadline_timer aes
rdrand lahf_lm 3dnowprefetch arat epb dtherm tpr_shadow vnmi
flexpriority ept vpid smep erms
bogomips : 4800.38
clflush size : 64
cache_alignment : 64
address sizes : 36 bits physical, 48 bits virtual
power management:
...trimmed...
The PCI info for the controller:
root at qct-ly8-01:~# lspci -vvx -s 00:14.0
00:14.0 Ethernet controller: Intel Corporation Device 1f41 (rev 03)
Subsystem: Intel Corporation Device 1f41
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort-
<MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 20
Region 0: Memory at dff60000 (64-bit, non-prefetchable) [size=128K]
Region 2: I/O ports at 1000 [size=32]
Region 4: Memory@dff84000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
Address: 0000000000000000 Data: 0000
Masking: 00000000 Pending: 00000000
Capabilities: [70] MSI-X: Enable+ Count=10 Masked-
Vector table: BAR=4 offset=00000000
PBA: BAR=4 offset=00002000
Capabilities: [a0] Express (v2) Root Complex Integrated Endpoint, MSI 00
DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
ExtTag- RBE+ FLReset+
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
MaxPayload 128 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
LnkCap: Port #0, Speed unknown, Width x0, ASPM unknown, Latency L0
<64ns, L1 <1us
ClockPM- Surprise- LLActRep- BwNot-
LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed unknown, Width x0, TrErr- Train- SlotClk- DLActive-
BWMgmt- ABWMgmt-
DevCap2: Completion Timeout: Range ABCD, TimeoutDis+
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis-,
Selectable De-emphasis: -6dB
Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-,
EqualizationPhase1-
EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
Kernel driver in use: igb
00: 86 80 41 1f 07 04 10 00 03 00 00 02 10 00 00 00
10: 04 00 f6 df 00 00 00 00 01 10 00 00 00 00 00 00
20: 04 40 f8 df 00 00 00 00 00 00 00 00 86 80 41 1f
30: 00 00 00 00 40 00 00 00 00 00 00 00 0b 01 00 00
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-07 13:41 ` Andy Gospodarek
@ 2015-05-07 17:49 ` Alexander Duyck
2015-05-08 16:39 ` Ronciak, John
1 sibling, 0 replies; 36+ messages in thread
From: Alexander Duyck @ 2015-05-07 17:49 UTC (permalink / raw)
To: intel-wired-lan
On 05/07/2015 06:41 AM, Andy Gospodarek wrote:
> On Thu, May 7, 2015 at 7:49 AM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
>> On Fri, 2015-04-17 at 13:23 -0700, Jonathan Toppins wrote:
>>> From: Alan Liebthal <alanl@cumulusnetworks.com>
>>>
>>> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
>>> the PHY layer. This adds support for this PHY to the Intel igb driver.
>>>
>>> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
>>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>>> ---
>>> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
>>> drivers/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 | 79
>>> ++++++++++++++++++++++++
>>> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
>>> 5 files changed, 102 insertions(+), 1 deletion(-)
>> After at lot of discussion on these changes, we would like to you to use
>> the PHYLib approach. I know that means adding PHYLib support to our
>> drivers, which we currently do not have.
> Jeff,
>
> I think we would be willing to take on this task, but I would not like
> that to be a gating factor for upstream acceptance of the
> functionality added with this patch. I see that Aaron has commented
> that no regressions were found with this patch, but based on current
> status in patchwork, it looks like Dave is waiting for something a bit
> more definitive before accepting it. Can you ACK it first so we have
> support for this platform upstream and then we can go take a stab at
> phylib support for igb?
>
> Thanks,
>
> -andy
I agree with Jeff. We already have the code from Tim Harvey which is
adding PHYlib support for i210/i211, it shouldn't take much to extend
the same kind of SGMII support for i350/i354. Then with that you free
yourself up to add whatever PHY you need in the future without having to
get buy-off from Intel. As is this patch set looks fishy to me since
you are providing the
The fact is the merge window will still be open for a while since we are
only at RC2.
- Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
2015-04-17 20:24 ` [Intel-wired-lan] " Jonathan Toppins
@ 2015-05-07 17:52 ` Alexander Duyck
-1 siblings, 0 replies; 36+ messages in thread
From: Alexander Duyck @ 2015-05-07 17:52 UTC (permalink / raw)
To: Jonathan Toppins, jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
On 04/17/2015 01:24 PM, Jonathan Toppins wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> Support setting the MII register via SIOCSMIIREG.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 720b785..1071a71 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> return -EIO;
> break;
> case SIOCSMIIREG:
> + 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;
> }
How and why is this being used? From what I can tell it looks like it
is an easy way to break any of the existing interfaces if it is misused
since all you would need to do is specify a phy address that doesn't
match the existing PHY in the system and then you would likely lose
link, or possibly mess up the configuration on the system requiring.
I suspect this is a back door for some piece of user space code that is
being given far more permission than it should be.
- Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
@ 2015-05-07 17:52 ` Alexander Duyck
0 siblings, 0 replies; 36+ messages in thread
From: Alexander Duyck @ 2015-05-07 17:52 UTC (permalink / raw)
To: intel-wired-lan
On 04/17/2015 01:24 PM, Jonathan Toppins wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> Support setting the MII register via SIOCSMIIREG.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 720b785..1071a71 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> return -EIO;
> break;
> case SIOCSMIIREG:
> + 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;
> }
How and why is this being used? From what I can tell it looks like it
is an easy way to break any of the existing interfaces if it is misused
since all you would need to do is specify a phy address that doesn't
match the existing PHY in the system and then you would likely lose
link, or possibly mess up the configuration on the system requiring.
I suspect this is a back door for some piece of user space code that is
being given far more permission than it should be.
- Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-04-17 20:23 ` [Intel-wired-lan] " Jonathan Toppins
@ 2015-05-07 18:20 ` Alexander Duyck
-1 siblings, 0 replies; 36+ messages in thread
From: Alexander Duyck @ 2015-05-07 18:20 UTC (permalink / raw)
To: Jonathan Toppins, jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
On 04/17/2015 01:23 PM, Jonathan Toppins wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> the PHY layer. This adds support for this PHY to the Intel igb driver.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
Feedback below. I would say you need to talk to Quanta, and have them
talk to Intel about getting their EEPROM configuration fixed. It looks
like the part you have is lacking some EEPROM configuration since most
of this patch contains the typical stuff one would do to workaround such
an issue.
I'm assuming this isn't an SPF pluggable PHY since you are using MDIO
for the interface, so they really should be doing some of the
initialization in the EEPROM rather than pushing it off to the driver code.
> ---
> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> drivers/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 | 79 ++++++++++++++++++++++++
> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> 5 files changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index 2dcc808..e222804 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -178,7 +178,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
>
> ctrl_ext = rd32(E1000_CTRL_EXT);
>
> - if (igb_sgmii_active_82575(hw)) {
> + if (igb_sgmii_active_82575(hw) && phy->type != e1000_phy_bcm5461s) {
> phy->ops.reset = igb_phy_hw_reset_sgmii_82575;
> ctrl_ext |= E1000_CTRL_I2C_ENA;
> } else {
This doesn't look right to me. Why do you need a special exception for
this part? It seems like we should probably be using this patch
assuming you are using an external PHY as the I2C enable is what enables
external MII or I2C. Is it possible that the EEPROM is missing the bits
for setting up external MDIO correct? You might take a look at the code
related to igb_sgmii_uses_mdio_82575().
> @@ -289,6 +289,11 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
> case BCM54616_E_PHY_ID:
> phy->type = e1000_phy_bcm54616;
> break;
> + case BCM5461S_E_PHY_ID:
> + phy->type = e1000_phy_bcm5461s;
> + phy->ops.get_phy_info = igb_get_phy_info_5461s;
> + phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
> + break;
> default:
> ret_val = -E1000_ERR_PHY;
> goto out;
> @@ -841,6 +846,15 @@ 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;
> + ret_val = igb_get_phy_id(hw);
> + }
> goto out;
> }
>
This looks like a bad EEPROM to me. The phy ID should already be stored
in the EEPROM or be discoverable.
> @@ -1221,6 +1235,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;
> }
>
Yuck, okay so this platform definitely has an EEPROM issue. All of this
init stuff should really be in the EEPROM for the part. You should
probably have Quanta talk to Intel about getting the correct EEPROM
built for this thing.
> @@ -1597,6 +1614,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
> ret_val = igb_copper_link_setup_82580(hw);
> break;
> case e1000_phy_bcm54616:
> + 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 50d51e4..787224d 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -861,6 +861,7 @@
> #define I210_I_PHY_ID 0x01410C00
> #define M88E1543_E_PHY_ID 0x01410EA0
> #define BCM54616_E_PHY_ID 0x03625D10
> +#define BCM5461S_E_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 d82c96b..408a3e5 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
> @@ -129,6 +129,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 c1bb64d..0f5a55b 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -148,6 +148,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));
> @@ -204,6 +211,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) |
This bit is already done for you if you have a valid EEPROM.
> @@ -2509,3 +2523,68 @@ 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;
> +}
> +
This belongs in the device EEPROM, not in the driver. This is why Jeff
is suggesting PHYlib. At least if it is there then there only needs to
be one copy per device this is attached to while lacking an EEPROM
configuration.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
@ 2015-05-07 18:20 ` Alexander Duyck
0 siblings, 0 replies; 36+ messages in thread
From: Alexander Duyck @ 2015-05-07 18:20 UTC (permalink / raw)
To: intel-wired-lan
On 04/17/2015 01:23 PM, Jonathan Toppins wrote:
> From: Alan Liebthal <alanl@cumulusnetworks.com>
>
> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip for
> the PHY layer. This adds support for this PHY to the Intel igb driver.
>
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
Feedback below. I would say you need to talk to Quanta, and have them
talk to Intel about getting their EEPROM configuration fixed. It looks
like the part you have is lacking some EEPROM configuration since most
of this patch contains the typical stuff one would do to workaround such
an issue.
I'm assuming this isn't an SPF pluggable PHY since you are using MDIO
for the interface, so they really should be doing some of the
initialization in the EEPROM rather than pushing it off to the driver code.
> ---
> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> drivers/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 | 79 ++++++++++++++++++++++++
> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> 5 files changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index 2dcc808..e222804 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -178,7 +178,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
>
> ctrl_ext = rd32(E1000_CTRL_EXT);
>
> - if (igb_sgmii_active_82575(hw)) {
> + if (igb_sgmii_active_82575(hw) && phy->type != e1000_phy_bcm5461s) {
> phy->ops.reset = igb_phy_hw_reset_sgmii_82575;
> ctrl_ext |= E1000_CTRL_I2C_ENA;
> } else {
This doesn't look right to me. Why do you need a special exception for
this part? It seems like we should probably be using this patch
assuming you are using an external PHY as the I2C enable is what enables
external MII or I2C. Is it possible that the EEPROM is missing the bits
for setting up external MDIO correct? You might take a look at the code
related to igb_sgmii_uses_mdio_82575().
> @@ -289,6 +289,11 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
> case BCM54616_E_PHY_ID:
> phy->type = e1000_phy_bcm54616;
> break;
> + case BCM5461S_E_PHY_ID:
> + phy->type = e1000_phy_bcm5461s;
> + phy->ops.get_phy_info = igb_get_phy_info_5461s;
> + phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_82580;
> + break;
> default:
> ret_val = -E1000_ERR_PHY;
> goto out;
> @@ -841,6 +846,15 @@ 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;
> + ret_val = igb_get_phy_id(hw);
> + }
> goto out;
> }
>
This looks like a bad EEPROM to me. The phy ID should already be stored
in the EEPROM or be discoverable.
> @@ -1221,6 +1235,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;
> }
>
Yuck, okay so this platform definitely has an EEPROM issue. All of this
init stuff should really be in the EEPROM for the part. You should
probably have Quanta talk to Intel about getting the correct EEPROM
built for this thing.
> @@ -1597,6 +1614,7 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
> ret_val = igb_copper_link_setup_82580(hw);
> break;
> case e1000_phy_bcm54616:
> + 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 50d51e4..787224d 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -861,6 +861,7 @@
> #define I210_I_PHY_ID 0x01410C00
> #define M88E1543_E_PHY_ID 0x01410EA0
> #define BCM54616_E_PHY_ID 0x03625D10
> +#define BCM5461S_E_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 d82c96b..408a3e5 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
> @@ -129,6 +129,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 c1bb64d..0f5a55b 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -148,6 +148,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));
> @@ -204,6 +211,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) |
This bit is already done for you if you have a valid EEPROM.
> @@ -2509,3 +2523,68 @@ 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;
> +}
> +
This belongs in the device EEPROM, not in the driver. This is why Jeff
is suggesting PHYlib. At least if it is there then there only needs to
be one copy per device this is attached to while lacking an EEPROM
configuration.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
2015-05-07 17:52 ` [Intel-wired-lan] " Alexander Duyck
@ 2015-05-07 20:46 ` Rustad, Mark D
-1 siblings, 0 replies; 36+ messages in thread
From: Rustad, Mark D @ 2015-05-07 20:46 UTC (permalink / raw)
To: Alexander Duyck
Cc: Jonathan Toppins, Kirsher, Jeffrey T, Brandeburg, Jesse, Nelson,
Shannon, Wyborny, Carolyn, Skidmore, Donald C, Vick, Matthew,
Ronciak, John, Williams, Mitch A, intel-wired-lan, netdev, gospo,
shm, Alan Liebthal
[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]
> On May 7, 2015, at 10:52 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 720b785..1071a71 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>> return -EIO;
>> break;
>> case SIOCSMIIREG:
>> + 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;
>> }
>
> How and why is this being used? From what I can tell it looks like it is an easy way to break any of the existing interfaces if it is misused since all you would need to do is specify a phy address that doesn't match the existing PHY in the system and then you would likely lose link, or possibly mess up the configuration on the system requiring.
>
> I suspect this is a back door for some piece of user space code that is being given far more permission than it should be.
I don't know about a back door, but the real problem is that it has a
side-effect of changing the saved value of the phy addr. That is clearly
a problem and can't be allowed.
For the phylib stuff to really work as intended, the igb_write_phy_reg
really should take the phy address as a parameter instead of getting
the value from the structure itself. Or a new function should be
defined that has that interface.
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
@ 2015-05-07 20:46 ` Rustad, Mark D
0 siblings, 0 replies; 36+ messages in thread
From: Rustad, Mark D @ 2015-05-07 20:46 UTC (permalink / raw)
To: intel-wired-lan
> On May 7, 2015, at 10:52 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 720b785..1071a71 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>> return -EIO;
>> break;
>> case SIOCSMIIREG:
>> + 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;
>> }
>
> How and why is this being used? From what I can tell it looks like it is an easy way to break any of the existing interfaces if it is misused since all you would need to do is specify a phy address that doesn't match the existing PHY in the system and then you would likely lose link, or possibly mess up the configuration on the system requiring.
>
> I suspect this is a back door for some piece of user space code that is being given far more permission than it should be.
I don't know about a back door, but the real problem is that it has a
side-effect of changing the saved value of the phy addr. That is clearly
a problem and can't be allowed.
For the phylib stuff to really work as intended, the igb_write_phy_reg
really should take the phy address as a parameter instead of getting
the value from the structure itself. Or a new function should be
defined that has that interface.
--
Mark Rustad, Networking Division, Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150507/124da355/attachment.asc>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-07 13:41 ` Andy Gospodarek
2015-05-07 17:49 ` Alexander Duyck
@ 2015-05-08 16:39 ` Ronciak, John
2015-05-08 17:46 ` Andy Gospodarek
1 sibling, 1 reply; 36+ messages in thread
From: Ronciak, John @ 2015-05-08 16:39 UTC (permalink / raw)
To: intel-wired-lan
> I think we would be willing to take on this task, but I would not like that to be a
> gating factor for upstream acceptance of the functionality added with this
> patch. I see that Aaron has commented that no regressions were found with
> this patch, but based on current status in patchwork, it looks like Dave is
> waiting for something a bit more definitive before accepting it. Can you ACK it
> first so we have support for this platform upstream and then we can go take a
> stab at phylib support for igb?
So Andy, are you wanting us to accept the patch and that you will then redo things to move to PHYlib in the near future? What's the time line for that work? What happens if you guys don't get around to doing it? This can become very problematic for us as you can imagine. This also really isn't the upstream way of doing something like this. So I'm a bit hesitant to do it this way.
Can you explain your urgency?
Cheers,
John
> -----Original Message-----
> From: Andy Gospodarek [mailto:gospo at cumulusnetworks.com]
> Sent: Thursday, May 7, 2015 6:41 AM
> To: Kirsher, Jeffrey T
> Cc: Jonathan Toppins; Wyborny, Carolyn; Ronciak, John; intel-wired-
> lan at lists.osuosl.org; Shrijeet Mukherjee; Alan Liebthal; Fujinaka, Todd
> Subject: Re: [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
>
> On Thu, May 7, 2015 at 7:49 AM, Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> wrote:
> > On Fri, 2015-04-17 at 13:23 -0700, Jonathan Toppins wrote:
> >> From: Alan Liebthal <alanl@cumulusnetworks.com>
> >>
> >> The Quanta LY8 Ethernet management port uses a Broadcom 5461S chip
> >> for the PHY layer. This adds support for this PHY to the Intel igb driver.
> >>
> >> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> >> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> >> ---
> >> drivers/net/ethernet/intel/igb/e1000_82575.c | 20 +++++-
> >> drivers/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 | 79
> >> ++++++++++++++++++++++++
> >> drivers/net/ethernet/intel/igb/e1000_phy.h | 2 +
> >> 5 files changed, 102 insertions(+), 1 deletion(-)
> >
> > After at lot of discussion on these changes, we would like to you to
> > use the PHYLib approach. I know that means adding PHYLib support to
> > our drivers, which we currently do not have.
>
> Jeff,
>
> I think we would be willing to take on this task, but I would not like that to be a
> gating factor for upstream acceptance of the functionality added with this
> patch. I see that Aaron has commented that no regressions were found with
> this patch, but based on current status in patchwork, it looks like Dave is
> waiting for something a bit more definitive before accepting it. Can you ACK it
> first so we have support for this platform upstream and then we can go take a
> stab at phylib support for igb?
>
> Thanks,
>
> -andy
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
2015-05-07 20:46 ` [Intel-wired-lan] " Rustad, Mark D
@ 2015-05-08 16:57 ` Jonathan Toppins
-1 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-08 16:57 UTC (permalink / raw)
To: Rustad, Mark D, Alexander Duyck
Cc: Kirsher, Jeffrey T, Brandeburg, Jesse, Nelson, Shannon, Wyborny,
Carolyn, Skidmore, Donald C, Vick, Matthew, Ronciak, John,
Williams, Mitch A, intel-wired-lan, netdev, gospo, shm,
Alan Liebthal
On 5/7/15 4:46 PM, Rustad, Mark D wrote:
>> On May 7, 2015, at 10:52 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>> index 720b785..1071a71 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>> @@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>>> return -EIO;
>>> break;
>>> case SIOCSMIIREG:
>>> + 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;
>>> }
>>
>> How and why is this being used? From what I can tell it looks like it is an easy way to break any of the existing interfaces if it is misused since all you would need to do is specify a phy address that doesn't match the existing PHY in the system and then you would likely lose link, or possibly mess up the configuration on the system requiring.
>>
>> I suspect this is a back door for some piece of user space code that is being given far more permission than it should be.
>
> I don't know about a back door, but the real problem is that it has a
> side-effect of changing the saved value of the phy addr. That is clearly
> a problem and can't be allowed.
This patch was worse before... it had changes in SIOGMIIREG that had
similar side-effects, missed this one.
Checking with some of the platform people here this patch can be
dropped. We don't use it as a backdoor of any kind it was mainly used
for debugging/guess-what-the-phy-addr-was when we originally got the boards.
>
> For the phylib stuff to really work as intended, the igb_write_phy_reg
> really should take the phy address as a parameter instead of getting
> the value from the structure itself. Or a new function should be
> defined that has that interface.
>
> --
> Mark Rustad, Networking Division, Intel Corporation
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG
@ 2015-05-08 16:57 ` Jonathan Toppins
0 siblings, 0 replies; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-08 16:57 UTC (permalink / raw)
To: intel-wired-lan
On 5/7/15 4:46 PM, Rustad, Mark D wrote:
>> On May 7, 2015, at 10:52 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>> index 720b785..1071a71 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>> @@ -7141,6 +7141,11 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>>> return -EIO;
>>> break;
>>> case SIOCSMIIREG:
>>> + 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;
>>> }
>>
>> How and why is this being used? From what I can tell it looks like it is an easy way to break any of the existing interfaces if it is misused since all you would need to do is specify a phy address that doesn't match the existing PHY in the system and then you would likely lose link, or possibly mess up the configuration on the system requiring.
>>
>> I suspect this is a back door for some piece of user space code that is being given far more permission than it should be.
>
> I don't know about a back door, but the real problem is that it has a
> side-effect of changing the saved value of the phy addr. That is clearly
> a problem and can't be allowed.
This patch was worse before... it had changes in SIOGMIIREG that had
similar side-effects, missed this one.
Checking with some of the platform people here this patch can be
dropped. We don't use it as a backdoor of any kind it was mainly used
for debugging/guess-what-the-phy-addr-was when we originally got the boards.
>
> For the phylib stuff to really work as intended, the igb_write_phy_reg
> really should take the phy address as a parameter instead of getting
> the value from the structure itself. Or a new function should be
> defined that has that interface.
>
> --
> Mark Rustad, Networking Division, Intel Corporation
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-08 16:39 ` Ronciak, John
@ 2015-05-08 17:46 ` Andy Gospodarek
2015-05-08 21:32 ` Alexander Duyck
0 siblings, 1 reply; 36+ messages in thread
From: Andy Gospodarek @ 2015-05-08 17:46 UTC (permalink / raw)
To: intel-wired-lan
On Fri, May 8, 2015 at 12:39 PM, Ronciak, John <john.ronciak@intel.com> wrote:
>> I think we would be willing to take on this task, but I would not like that to be a
>> gating factor for upstream acceptance of the functionality added with this
>> patch. I see that Aaron has commented that no regressions were found with
>> this patch, but based on current status in patchwork, it looks like Dave is
>> waiting for something a bit more definitive before accepting it. Can you ACK it
>> first so we have support for this platform upstream and then we can go take a
>> stab at phylib support for igb?
> So Andy, are you wanting us to accept the patch and that you will then redo things to move to PHYlib in the near future? What's the time line for that work? What happens if you guys don't get around to doing it? This can become very problematic for us as you can imagine. This also really isn't the upstream way of doing something like this. So I'm a bit hesitant to do it this way.
>
> Can you explain your urgency?
>
> Cheers,
> John
>
John,
It is somewhat urgent as we would like to do some upstream kernel
development on these platforms. I would rather not have to coach
everyone/constantly rebase this patch for at least one more kernel
release and supply it to anyone (internal to Cumulus or outside) just
to run net-next on these platforms. Once this is added ONIE kernels
could also use a pure upstream kernel for booting and installing
various distros on it, so I see inclusion as a nice feature for the
community as well.
I was not aware of the patch from Tim Harvey that was adding phylib
support into igb when I sent the first email, so that may change the
scope of this effort a bit. Of course we would now be reliant on that
patch being included in Dave's tree before we can do our work and that
appears to have the status of 'changes requested' on the
intel-wired-lan list, so I see no guarantee that those will be added
by the time the merge window closes.
Thanks,
-andy
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-08 17:46 ` Andy Gospodarek
@ 2015-05-08 21:32 ` Alexander Duyck
2015-05-08 21:42 ` Jonathan Toppins
2015-05-11 14:46 ` Andy Gospodarek
0 siblings, 2 replies; 36+ messages in thread
From: Alexander Duyck @ 2015-05-08 21:32 UTC (permalink / raw)
To: intel-wired-lan
On 05/08/2015 10:46 AM, Andy Gospodarek wrote:
> On Fri, May 8, 2015 at 12:39 PM, Ronciak, John <john.ronciak@intel.com> wrote:
>>> I think we would be willing to take on this task, but I would not like that to be a
>>> gating factor for upstream acceptance of the functionality added with this
>>> patch. I see that Aaron has commented that no regressions were found with
>>> this patch, but based on current status in patchwork, it looks like Dave is
>>> waiting for something a bit more definitive before accepting it. Can you ACK it
>>> first so we have support for this platform upstream and then we can go take a
>>> stab at phylib support for igb?
>> So Andy, are you wanting us to accept the patch and that you will then redo things to move to PHYlib in the near future? What's the time line for that work? What happens if you guys don't get around to doing it? This can become very problematic for us as you can imagine. This also really isn't the upstream way of doing something like this. So I'm a bit hesitant to do it this way.
>>
>> Can you explain your urgency?
>>
>> Cheers,
>> John
>>
> John,
>
> It is somewhat urgent as we would like to do some upstream kernel
> development on these platforms. I would rather not have to coach
> everyone/constantly rebase this patch for at least one more kernel
> release and supply it to anyone (internal to Cumulus or outside) just
> to run net-next on these platforms. Once this is added ONIE kernels
> could also use a pure upstream kernel for booting and installing
> various distros on it, so I see inclusion as a nice feature for the
> community as well.
>
> I was not aware of the patch from Tim Harvey that was adding phylib
> support into igb when I sent the first email, so that may change the
> scope of this effort a bit. Of course we would now be reliant on that
> patch being included in Dave's tree before we can do our work and that
> appears to have the status of 'changes requested' on the
> intel-wired-lan list, so I see no guarantee that those will be added
> by the time the merge window closes.
>
> Thanks,
>
> -andy
Andy,
The patch as-is seems to have a number of issues since the interface you
are using has a misconfigured EEPROM. If you got that addressed you
could then probably cut down significantly on the code changes needed
since much of it seems to be just workarounds for stuff that should have
been taken care of in the EEPROM. For example, the PHY address and
external MDIO value are controlled via Initialization Control 3 & 4.
The configuration for the hardware should be there. The same goes for
the LED configuration. That is something that should start working at
power-on, not after the driver is loaded. I really think you should
work to get those resolved with Quanta then it would probably make your
driver work much easier.
Also it looks like the bcm5461 is already supported by PHYdev so there
shouldn't be much to do other than update igb to register a mii_bus, and
with any luck the PHYdev code already implemented would take care of the
rest (assuming the EEPROM is fixed).
- Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-08 21:32 ` Alexander Duyck
@ 2015-05-08 21:42 ` Jonathan Toppins
2015-05-08 22:05 ` Alexander Duyck
2015-05-11 14:46 ` Andy Gospodarek
1 sibling, 1 reply; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-08 21:42 UTC (permalink / raw)
To: intel-wired-lan
On 5/8/15 5:32 PM, Alexander Duyck wrote:
>
>
> On 05/08/2015 10:46 AM, Andy Gospodarek wrote:
>> On Fri, May 8, 2015 at 12:39 PM, Ronciak, John
>> <john.ronciak@intel.com> wrote:
>>>> I think we would be willing to take on this task, but I would not
>>>> like that to be a
>>>> gating factor for upstream acceptance of the functionality added
>>>> with this
>>>> patch. I see that Aaron has commented that no regressions were
>>>> found with
>>>> this patch, but based on current status in patchwork, it looks like
>>>> Dave is
>>>> waiting for something a bit more definitive before accepting it.
>>>> Can you ACK it
>>>> first so we have support for this platform upstream and then we can
>>>> go take a
>>>> stab at phylib support for igb?
>>> So Andy, are you wanting us to accept the patch and that you will
>>> then redo things to move to PHYlib in the near future? What's the
>>> time line for that work? What happens if you guys don't get around
>>> to doing it? This can become very problematic for us as you can
>>> imagine. This also really isn't the upstream way of doing something
>>> like this. So I'm a bit hesitant to do it this way.
>>>
>>> Can you explain your urgency?
>>>
>>> Cheers,
>>> John
>>>
>> John,
>>
>> It is somewhat urgent as we would like to do some upstream kernel
>> development on these platforms. I would rather not have to coach
>> everyone/constantly rebase this patch for at least one more kernel
>> release and supply it to anyone (internal to Cumulus or outside) just
>> to run net-next on these platforms. Once this is added ONIE kernels
>> could also use a pure upstream kernel for booting and installing
>> various distros on it, so I see inclusion as a nice feature for the
>> community as well.
>>
>> I was not aware of the patch from Tim Harvey that was adding phylib
>> support into igb when I sent the first email, so that may change the
>> scope of this effort a bit. Of course we would now be reliant on that
>> patch being included in Dave's tree before we can do our work and that
>> appears to have the status of 'changes requested' on the
>> intel-wired-lan list, so I see no guarantee that those will be added
>> by the time the merge window closes.
>>
>> Thanks,
>>
>> -andy
>
> Andy,
>
> The patch as-is seems to have a number of issues since the interface you
> are using has a misconfigured EEPROM. If you got that addressed you
> could then probably cut down significantly on the code changes needed
> since much of it seems to be just workarounds for stuff that should have
> been taken care of in the EEPROM. For example, the PHY address and
> external MDIO value are controlled via Initialization Control 3 & 4. The
> configuration for the hardware should be there. The same goes for the
> LED configuration. That is something that should start working at
> power-on, not after the driver is loaded. I really think you should
> work to get those resolved with Quanta then it would probably make your
> driver work much easier.
>
> Also it looks like the bcm5461 is already supported by PHYdev so there
> shouldn't be much to do other than update igb to register a mii_bus, and
> with any luck the PHYdev code already implemented would take care of the
> rest (assuming the EEPROM is fixed).
Alex, appreciate your comments on the EEPROM. I forwarded them to our
platform team to inquire on the Quanta side. The initial thinking seems
to be Quanta is not going to change a shipping product. Is the EEPROM
field upgradable from software or does it require a manufacturing line
change?
-Jon
>
> - Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-08 21:42 ` Jonathan Toppins
@ 2015-05-08 22:05 ` Alexander Duyck
2015-05-08 22:30 ` Jonathan Toppins
0 siblings, 1 reply; 36+ messages in thread
From: Alexander Duyck @ 2015-05-08 22:05 UTC (permalink / raw)
To: intel-wired-lan
On 05/08/2015 02:42 PM, Jonathan Toppins wrote:
> On 5/8/15 5:32 PM, Alexander Duyck wrote:
>>
>>
>> On 05/08/2015 10:46 AM, Andy Gospodarek wrote:
>>> On Fri, May 8, 2015 at 12:39 PM, Ronciak, John
>>> <john.ronciak@intel.com> wrote:
>>>>> I think we would be willing to take on this task, but I would not
>>>>> like that to be a
>>>>> gating factor for upstream acceptance of the functionality added
>>>>> with this
>>>>> patch. I see that Aaron has commented that no regressions were
>>>>> found with
>>>>> this patch, but based on current status in patchwork, it looks like
>>>>> Dave is
>>>>> waiting for something a bit more definitive before accepting it.
>>>>> Can you ACK it
>>>>> first so we have support for this platform upstream and then we can
>>>>> go take a
>>>>> stab at phylib support for igb?
>>>> So Andy, are you wanting us to accept the patch and that you will
>>>> then redo things to move to PHYlib in the near future? What's the
>>>> time line for that work? What happens if you guys don't get around
>>>> to doing it? This can become very problematic for us as you can
>>>> imagine. This also really isn't the upstream way of doing something
>>>> like this. So I'm a bit hesitant to do it this way.
>>>>
>>>> Can you explain your urgency?
>>>>
>>>> Cheers,
>>>> John
>>>>
>>> John,
>>>
>>> It is somewhat urgent as we would like to do some upstream kernel
>>> development on these platforms. I would rather not have to coach
>>> everyone/constantly rebase this patch for at least one more kernel
>>> release and supply it to anyone (internal to Cumulus or outside) just
>>> to run net-next on these platforms. Once this is added ONIE kernels
>>> could also use a pure upstream kernel for booting and installing
>>> various distros on it, so I see inclusion as a nice feature for the
>>> community as well.
>>>
>>> I was not aware of the patch from Tim Harvey that was adding phylib
>>> support into igb when I sent the first email, so that may change the
>>> scope of this effort a bit. Of course we would now be reliant on that
>>> patch being included in Dave's tree before we can do our work and that
>>> appears to have the status of 'changes requested' on the
>>> intel-wired-lan list, so I see no guarantee that those will be added
>>> by the time the merge window closes.
>>>
>>> Thanks,
>>>
>>> -andy
>>
>> Andy,
>>
>> The patch as-is seems to have a number of issues since the interface you
>> are using has a misconfigured EEPROM. If you got that addressed you
>> could then probably cut down significantly on the code changes needed
>> since much of it seems to be just workarounds for stuff that should have
>> been taken care of in the EEPROM. For example, the PHY address and
>> external MDIO value are controlled via Initialization Control 3 & 4. The
>> configuration for the hardware should be there. The same goes for the
>> LED configuration. That is something that should start working at
>> power-on, not after the driver is loaded. I really think you should
>> work to get those resolved with Quanta then it would probably make your
>> driver work much easier.
>>
>> Also it looks like the bcm5461 is already supported by PHYdev so there
>> shouldn't be much to do other than update igb to register a mii_bus, and
>> with any luck the PHYdev code already implemented would take care of the
>> rest (assuming the EEPROM is fixed).
>
> Alex, appreciate your comments on the EEPROM. I forwarded them to our
> platform team to inquire on the Quanta side. The initial thinking
> seems to be Quanta is not going to change a shipping product. Is the
> EEPROM field upgradable from software or does it require a
> manufacturing line change?
>
> -Jon
It should be field upgradable assuming they didn't do anything too goofy
with the design. If I recall correctly it can be modified via "ethtool
-E ". The i350 datasheet
(http://www.intel.com/content/www/us/en/ethernet-controllers/ethernet-controller-i350-datasheet.html)
should cover Init Ctrl 3 & 4 in section 6.2 of the data sheet, that
piece is pretty strait forward since those are just a few single byte
replacements. The tricky bit would be the PHY configuration Structure
(section 6.3.13) which you would probably need to take care of the LED
initialization at power-on. I recall having to deal with some of this
when the work was done to get the external Marvell PHY working in a
similar configuration. The fact is I am a bit rusty when it comes to
this stuff since I last worked on it several years ago so my information
may be out of date, and I am assuming the i354 EEPROM follows the same
layout as the i350 which may or may not be the case.
- Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-08 22:05 ` Alexander Duyck
@ 2015-05-08 22:30 ` Jonathan Toppins
2015-05-08 23:06 ` Alexander Duyck
0 siblings, 1 reply; 36+ messages in thread
From: Jonathan Toppins @ 2015-05-08 22:30 UTC (permalink / raw)
To: intel-wired-lan
On 5/8/15 6:05 PM, Alexander Duyck wrote:
>
>
> On 05/08/2015 02:42 PM, Jonathan Toppins wrote:
>> On 5/8/15 5:32 PM, Alexander Duyck wrote:
>>>
>>>
>>> On 05/08/2015 10:46 AM, Andy Gospodarek wrote:
>>>> On Fri, May 8, 2015 at 12:39 PM, Ronciak, John
>>>> <john.ronciak@intel.com> wrote:
>>>>>> I think we would be willing to take on this task, but I would not
>>>>>> like that to be a
>>>>>> gating factor for upstream acceptance of the functionality added
>>>>>> with this
>>>>>> patch. I see that Aaron has commented that no regressions were
>>>>>> found with
>>>>>> this patch, but based on current status in patchwork, it looks like
>>>>>> Dave is
>>>>>> waiting for something a bit more definitive before accepting it.
>>>>>> Can you ACK it
>>>>>> first so we have support for this platform upstream and then we can
>>>>>> go take a
>>>>>> stab at phylib support for igb?
>>>>> So Andy, are you wanting us to accept the patch and that you will
>>>>> then redo things to move to PHYlib in the near future? What's the
>>>>> time line for that work? What happens if you guys don't get around
>>>>> to doing it? This can become very problematic for us as you can
>>>>> imagine. This also really isn't the upstream way of doing something
>>>>> like this. So I'm a bit hesitant to do it this way.
>>>>>
>>>>> Can you explain your urgency?
>>>>>
>>>>> Cheers,
>>>>> John
>>>>>
>>>> John,
>>>>
>>>> It is somewhat urgent as we would like to do some upstream kernel
>>>> development on these platforms. I would rather not have to coach
>>>> everyone/constantly rebase this patch for at least one more kernel
>>>> release and supply it to anyone (internal to Cumulus or outside) just
>>>> to run net-next on these platforms. Once this is added ONIE kernels
>>>> could also use a pure upstream kernel for booting and installing
>>>> various distros on it, so I see inclusion as a nice feature for the
>>>> community as well.
>>>>
>>>> I was not aware of the patch from Tim Harvey that was adding phylib
>>>> support into igb when I sent the first email, so that may change the
>>>> scope of this effort a bit. Of course we would now be reliant on that
>>>> patch being included in Dave's tree before we can do our work and that
>>>> appears to have the status of 'changes requested' on the
>>>> intel-wired-lan list, so I see no guarantee that those will be added
>>>> by the time the merge window closes.
>>>>
>>>> Thanks,
>>>>
>>>> -andy
>>>
>>> Andy,
>>>
>>> The patch as-is seems to have a number of issues since the interface you
>>> are using has a misconfigured EEPROM. If you got that addressed you
>>> could then probably cut down significantly on the code changes needed
>>> since much of it seems to be just workarounds for stuff that should have
>>> been taken care of in the EEPROM. For example, the PHY address and
>>> external MDIO value are controlled via Initialization Control 3 & 4. The
>>> configuration for the hardware should be there. The same goes for the
>>> LED configuration. That is something that should start working at
>>> power-on, not after the driver is loaded. I really think you should
>>> work to get those resolved with Quanta then it would probably make your
>>> driver work much easier.
>>>
>>> Also it looks like the bcm5461 is already supported by PHYdev so there
>>> shouldn't be much to do other than update igb to register a mii_bus, and
>>> with any luck the PHYdev code already implemented would take care of the
>>> rest (assuming the EEPROM is fixed).
>>
>> Alex, appreciate your comments on the EEPROM. I forwarded them to our
>> platform team to inquire on the Quanta side. The initial thinking
>> seems to be Quanta is not going to change a shipping product. Is the
>> EEPROM field upgradable from software or does it require a
>> manufacturing line change?
>>
>> -Jon
>
> It should be field upgradable assuming they didn't do anything too goofy
> with the design. If I recall correctly it can be modified via "ethtool
> -E ". The i350 datasheet
> (http://www.intel.com/content/www/us/en/ethernet-controllers/ethernet-controller-i350-datasheet.html)
> should cover Init Ctrl 3 & 4 in section 6.2 of the data sheet, that
> piece is pretty strait forward since those are just a few single byte
> replacements. The tricky bit would be the PHY configuration Structure
> (section 6.3.13) which you would probably need to take care of the LED
> initialization at power-on. I recall having to deal with some of this
> when the work was done to get the external Marvell PHY working in a
> similar configuration. The fact is I am a bit rusty when it comes to
> this stuff since I last worked on it several years ago so my information
> may be out of date, and I am assuming the i354 EEPROM follows the same
> layout as the i350 which may or may not be the case.
>
> - Alex
>
Alex, appreciate the info, this seems like a possible alternative and
something we could handle all inside our distribution. If any of your
other Intel friends have more recent info would much appreciate it.
On the topic of phylib support, a point of clarification for me. Is the
thinking to ADD or USE phylib for igb? By this I mean:
ADD == igb keeps its current phy support but adds support for using
phys provided by phylib.
USE == igb uses phylib and gets rid of the phy support igb currently
has, this would obviously require enhancing phylib for any gaps
between igb phy support and phylib.
ADD seems like the middle ground but from a design perspective it seems
like USE would be the ultimate goal. Thoughts?
Thanks!
-Jon
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-08 22:30 ` Jonathan Toppins
@ 2015-05-08 23:06 ` Alexander Duyck
0 siblings, 0 replies; 36+ messages in thread
From: Alexander Duyck @ 2015-05-08 23:06 UTC (permalink / raw)
To: intel-wired-lan
On 05/08/2015 03:30 PM, Jonathan Toppins wrote:
> On 5/8/15 6:05 PM, Alexander Duyck wrote:
>>
>>
>> On 05/08/2015 02:42 PM, Jonathan Toppins wrote:
>>> On 5/8/15 5:32 PM, Alexander Duyck wrote:
>>>>
>>>>
>>>> On 05/08/2015 10:46 AM, Andy Gospodarek wrote:
>>>>> On Fri, May 8, 2015 at 12:39 PM, Ronciak, John
>>>>> <john.ronciak@intel.com> wrote:
>>>>>>> I think we would be willing to take on this task, but I would not
>>>>>>> like that to be a
>>>>>>> gating factor for upstream acceptance of the functionality added
>>>>>>> with this
>>>>>>> patch. I see that Aaron has commented that no regressions were
>>>>>>> found with
>>>>>>> this patch, but based on current status in patchwork, it looks like
>>>>>>> Dave is
>>>>>>> waiting for something a bit more definitive before accepting it.
>>>>>>> Can you ACK it
>>>>>>> first so we have support for this platform upstream and then we can
>>>>>>> go take a
>>>>>>> stab at phylib support for igb?
>>>>>> So Andy, are you wanting us to accept the patch and that you will
>>>>>> then redo things to move to PHYlib in the near future? What's the
>>>>>> time line for that work? What happens if you guys don't get around
>>>>>> to doing it? This can become very problematic for us as you can
>>>>>> imagine. This also really isn't the upstream way of doing something
>>>>>> like this. So I'm a bit hesitant to do it this way.
>>>>>>
>>>>>> Can you explain your urgency?
>>>>>>
>>>>>> Cheers,
>>>>>> John
>>>>>>
>>>>> John,
>>>>>
>>>>> It is somewhat urgent as we would like to do some upstream kernel
>>>>> development on these platforms. I would rather not have to coach
>>>>> everyone/constantly rebase this patch for at least one more kernel
>>>>> release and supply it to anyone (internal to Cumulus or outside) just
>>>>> to run net-next on these platforms. Once this is added ONIE kernels
>>>>> could also use a pure upstream kernel for booting and installing
>>>>> various distros on it, so I see inclusion as a nice feature for the
>>>>> community as well.
>>>>>
>>>>> I was not aware of the patch from Tim Harvey that was adding phylib
>>>>> support into igb when I sent the first email, so that may change the
>>>>> scope of this effort a bit. Of course we would now be reliant on
>>>>> that
>>>>> patch being included in Dave's tree before we can do our work and
>>>>> that
>>>>> appears to have the status of 'changes requested' on the
>>>>> intel-wired-lan list, so I see no guarantee that those will be added
>>>>> by the time the merge window closes.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -andy
>>>>
>>>> Andy,
>>>>
>>>> The patch as-is seems to have a number of issues since the
>>>> interface you
>>>> are using has a misconfigured EEPROM. If you got that addressed you
>>>> could then probably cut down significantly on the code changes needed
>>>> since much of it seems to be just workarounds for stuff that should
>>>> have
>>>> been taken care of in the EEPROM. For example, the PHY address and
>>>> external MDIO value are controlled via Initialization Control 3 &
>>>> 4. The
>>>> configuration for the hardware should be there. The same goes for the
>>>> LED configuration. That is something that should start working at
>>>> power-on, not after the driver is loaded. I really think you should
>>>> work to get those resolved with Quanta then it would probably make
>>>> your
>>>> driver work much easier.
>>>>
>>>> Also it looks like the bcm5461 is already supported by PHYdev so there
>>>> shouldn't be much to do other than update igb to register a
>>>> mii_bus, and
>>>> with any luck the PHYdev code already implemented would take care
>>>> of the
>>>> rest (assuming the EEPROM is fixed).
>>>
>>> Alex, appreciate your comments on the EEPROM. I forwarded them to our
>>> platform team to inquire on the Quanta side. The initial thinking
>>> seems to be Quanta is not going to change a shipping product. Is the
>>> EEPROM field upgradable from software or does it require a
>>> manufacturing line change?
>>>
>>> -Jon
>>
>> It should be field upgradable assuming they didn't do anything too goofy
>> with the design. If I recall correctly it can be modified via "ethtool
>> -E ". The i350 datasheet
>> (http://www.intel.com/content/www/us/en/ethernet-controllers/ethernet-controller-i350-datasheet.html)
>>
>> should cover Init Ctrl 3 & 4 in section 6.2 of the data sheet, that
>> piece is pretty strait forward since those are just a few single byte
>> replacements. The tricky bit would be the PHY configuration Structure
>> (section 6.3.13) which you would probably need to take care of the LED
>> initialization at power-on. I recall having to deal with some of this
>> when the work was done to get the external Marvell PHY working in a
>> similar configuration. The fact is I am a bit rusty when it comes to
>> this stuff since I last worked on it several years ago so my information
>> may be out of date, and I am assuming the i354 EEPROM follows the same
>> layout as the i350 which may or may not be the case.
>>
>> - Alex
>>
>
> Alex, appreciate the info, this seems like a possible alternative and
> something we could handle all inside our distribution. If any of your
> other Intel friends have more recent info would much appreciate it.
>
> On the topic of phylib support, a point of clarification for me. Is
> the thinking to ADD or USE phylib for igb? By this I mean:
> ADD == igb keeps its current phy support but adds support for using
> phys provided by phylib.
> USE == igb uses phylib and gets rid of the phy support igb currently
> has, this would obviously require enhancing phylib for any gaps
> between igb phy support and phylib.
>
> ADD seems like the middle ground but from a design perspective it
> seems like USE would be the ultimate goal. Thoughts?
>
> Thanks!
> -Jon
I agree with ADD, but it gets a bit messy. The problem is how to split
off the SGMII PHYs without impacting any other platforms.
My thought would be to just split off everything that uses SGMII and is
i354 and newer. Then that covers the cases both you and Tim Harvey
have, but I am not sure what PHYs are in use with those devices already
so we would need to make sure they have support in phylib.
- Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Intel-wired-lan] [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S
2015-05-08 21:32 ` Alexander Duyck
2015-05-08 21:42 ` Jonathan Toppins
@ 2015-05-11 14:46 ` Andy Gospodarek
1 sibling, 0 replies; 36+ messages in thread
From: Andy Gospodarek @ 2015-05-11 14:46 UTC (permalink / raw)
To: intel-wired-lan
On Fri, May 8, 2015 at 5:32 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
>
>
> On 05/08/2015 10:46 AM, Andy Gospodarek wrote:
>>
>> On Fri, May 8, 2015 at 12:39 PM, Ronciak, John <john.ronciak@intel.com>
>> wrote:
>>>>
>>>> I think we would be willing to take on this task, but I would not like
>>>> that to be a
>>>> gating factor for upstream acceptance of the functionality added with
>>>> this
>>>> patch. I see that Aaron has commented that no regressions were found
>>>> with
>>>> this patch, but based on current status in patchwork, it looks like Dave
>>>> is
>>>> waiting for something a bit more definitive before accepting it. Can
>>>> you ACK it
>>>> first so we have support for this platform upstream and then we can go
>>>> take a
>>>> stab at phylib support for igb?
>>>
>>> So Andy, are you wanting us to accept the patch and that you will then
>>> redo things to move to PHYlib in the near future? What's the time line for
>>> that work? What happens if you guys don't get around to doing it? This can
>>> become very problematic for us as you can imagine. This also really isn't
>>> the upstream way of doing something like this. So I'm a bit hesitant to do
>>> it this way.
>>>
>>> Can you explain your urgency?
>>>
>>> Cheers,
>>> John
>>>
>> John,
>>
>> It is somewhat urgent as we would like to do some upstream kernel
>> development on these platforms. I would rather not have to coach
>> everyone/constantly rebase this patch for at least one more kernel
>> release and supply it to anyone (internal to Cumulus or outside) just
>> to run net-next on these platforms. Once this is added ONIE kernels
>> could also use a pure upstream kernel for booting and installing
>> various distros on it, so I see inclusion as a nice feature for the
>> community as well.
>>
>> I was not aware of the patch from Tim Harvey that was adding phylib
>> support into igb when I sent the first email, so that may change the
>> scope of this effort a bit. Of course we would now be reliant on that
>> patch being included in Dave's tree before we can do our work and that
>> appears to have the status of 'changes requested' on the
>> intel-wired-lan list, so I see no guarantee that those will be added
>> by the time the merge window closes.
>>
>> Thanks,
>>
>> -andy
>
>
> Andy,
>
> The patch as-is seems to have a number of issues since the interface you are
> using has a misconfigured EEPROM. If you got that addressed you could then
> probably cut down significantly on the code changes needed since much of it
> seems to be just workarounds for stuff that should have been taken care of
> in the EEPROM. For example, the PHY address and external MDIO value are
> controlled via Initialization Control 3 & 4. The configuration for the
> hardware should be there. The same goes for the LED configuration. That is
> something that should start working at power-on, not after the driver is
> loaded. I really think you should work to get those resolved with Quanta
> then it would probably make your driver work much easier.
Alex,
I think we are talking past each other a bit, sorry I wasn't more
clear. My question was more about the direction to take (do we need
to push all of this to use phylib or can support for this phy be added
directly to igb). I certainly recognize that a v2 is needed either
way, but I was trying help help Jon flush out what direction to take.
Based on emails this weekend, it seems that the idea that adding
phylib support (rather than moving support for all phys currently
supported in igb to phylib) is the preferred direction as far as you
are concerned. That seems fair and as you stated not terribly
challenging (EEPROM issues aside).
Thanks,
-andy
>
> Also it looks like the bcm5461 is already supported by PHYdev so there
> shouldn't be much to do other than update igb to register a mii_bus, and
> with any luck the PHYdev code already implemented would take care of the
> rest (assuming the EEPROM is fixed).
>
> - Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2015-05-11 14:46 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-17 20:23 [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S Jonathan Toppins
2015-04-17 20:23 ` [Intel-wired-lan] " Jonathan Toppins
2015-04-17 20:24 ` [PATCH v1 net-next 2/2] igb: support SIOCSMIIREG Jonathan Toppins
2015-04-17 20:24 ` [Intel-wired-lan] " Jonathan Toppins
2015-05-05 17:25 ` Brown, Aaron F
2015-05-05 17:25 ` [Intel-wired-lan] " Brown, Aaron F
2015-05-05 19:31 ` Jonathan Toppins
2015-05-05 19:31 ` [Intel-wired-lan] " Jonathan Toppins
2015-05-07 17:52 ` Alexander Duyck
2015-05-07 17:52 ` [Intel-wired-lan] " Alexander Duyck
2015-05-07 20:46 ` Rustad, Mark D
2015-05-07 20:46 ` [Intel-wired-lan] " Rustad, Mark D
2015-05-08 16:57 ` Jonathan Toppins
2015-05-08 16:57 ` [Intel-wired-lan] " Jonathan Toppins
2015-04-27 15:44 ` [PATCH v1 net-next 1/2] igb: add PHY support for Broadcom 5461S Jonathan Toppins
2015-04-27 15:44 ` [Intel-wired-lan] " Jonathan Toppins
2015-05-02 1:45 ` Brown, Aaron F
2015-05-02 1:45 ` [Intel-wired-lan] " Brown, Aaron F
2015-05-07 11:49 ` Jeff Kirsher
2015-05-07 13:41 ` Andy Gospodarek
2015-05-07 17:49 ` Alexander Duyck
2015-05-08 16:39 ` Ronciak, John
2015-05-08 17:46 ` Andy Gospodarek
2015-05-08 21:32 ` Alexander Duyck
2015-05-08 21:42 ` Jonathan Toppins
2015-05-08 22:05 ` Alexander Duyck
2015-05-08 22:30 ` Jonathan Toppins
2015-05-08 23:06 ` Alexander Duyck
2015-05-11 14:46 ` Andy Gospodarek
2015-05-07 16:32 ` Tim Harvey
2015-05-07 16:18 ` Tim Harvey
2015-05-07 16:18 ` [Intel-wired-lan] " Tim Harvey
2015-05-07 16:57 ` Jonathan Toppins
2015-05-07 16:57 ` [Intel-wired-lan] " Jonathan Toppins
2015-05-07 18:20 ` Alexander Duyck
2015-05-07 18:20 ` [Intel-wired-lan] " Alexander Duyck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.