| Message ID | b5d586566a1b523b9be65567b52898e33058a1cf.1500660210.git. |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf > Of John W. Linville > Sent: Friday, July 21, 2017 11:12 AM > To: netdev@vger.kernel.org > Cc: intel-wired-lan@lists.osuosl.org; John W. Linville > <linville@tuxdriver.com> > Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY > > The management port on an Edgecore AS7712-32 switch uses an igb MAC, > but > it uses a BCM54616 PHY. Without a patch like this, loading the igb > module produces dmesg output like this: > > [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation. > [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2 > > Signed-off-by: John W. Linville <linville@tuxdriver.com> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++ > drivers/net/ethernet/intel/igb/e1000_defines.h | 1 + > drivers/net/ethernet/intel/igb/e1000_hw.h | 1 + > 3 files changed, 8 insertions(+) I do not have the specific hardware (Edgecore switch) but as far as regression tests go this works fine. Tested-by: Aaron Brown <aaron.f.brown@intel.com>
On Thu, Jul 27, 2017 at 12:40:01AM +0000, Brown, Aaron F wrote: > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf > > Of John W. Linville > > Sent: Friday, July 21, 2017 11:12 AM > > To: netdev@vger.kernel.org > > Cc: intel-wired-lan@lists.osuosl.org; John W. Linville > > <linville@tuxdriver.com> > > Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY > > > > The management port on an Edgecore AS7712-32 switch uses an igb MAC, > > but > > it uses a BCM54616 PHY. Without a patch like this, loading the igb > > module produces dmesg output like this: > > > > [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation. > > [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2 > > > > Signed-off-by: John W. Linville <linville@tuxdriver.com> > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > --- > > drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++ > > drivers/net/ethernet/intel/igb/e1000_defines.h | 1 + > > drivers/net/ethernet/intel/igb/e1000_hw.h | 1 + > > 3 files changed, 8 insertions(+) > > I do not have the specific hardware (Edgecore switch) but as far as regression tests go this works fine. > Tested-by: Aaron Brown <aaron.f.brown@intel.com> Sorry, missed the initial post, so replying to a reply. Linux has supported the BCM54616 PHY since April 2015. If the Intel drivers used the Linux PHY drivers, you would not of had this problem. It would be good if somebody spent the time to migrate these MAC drivers to use the common Linux PHY infrastructure. Andrew
On 07/27/2017 11:37 AM, Andrew Lunn wrote: > On Thu, Jul 27, 2017 at 12:40:01AM +0000, Brown, Aaron F wrote: >>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf >>> Of John W. Linville >>> Sent: Friday, July 21, 2017 11:12 AM >>> To: netdev@vger.kernel.org >>> Cc: intel-wired-lan@lists.osuosl.org; John W. Linville >>> <linville@tuxdriver.com> >>> Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY >>> >>> The management port on an Edgecore AS7712-32 switch uses an igb MAC, >>> but >>> it uses a BCM54616 PHY. Without a patch like this, loading the igb >>> module produces dmesg output like this: >>> >>> [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation. >>> [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2 >>> >>> Signed-off-by: John W. Linville <linville@tuxdriver.com> >>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >>> --- >>> drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++ >>> drivers/net/ethernet/intel/igb/e1000_defines.h | 1 + >>> drivers/net/ethernet/intel/igb/e1000_hw.h | 1 + >>> 3 files changed, 8 insertions(+) >> >> I do not have the specific hardware (Edgecore switch) but as far as regression tests go this works fine. >> Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > Sorry, missed the initial post, so replying to a reply. > > Linux has supported the BCM54616 PHY since April 2015. If the Intel > drivers used the Linux PHY drivers, you would not of had this problem. > > It would be good if somebody spent the time to migrate these MAC > drivers to use the common Linux PHY infrastructure. > > Andrew > Thank you. This was a point I made in 2015 when I posted the original patch for igb to support this phy. http://marc.info/?l=linux-netdev&m=142870703206646&w=2 http://marc.info/?l=linux-netdev&m=142870703806647&w=2 http://marc.info/?l=linux-netdev&m=142870703806648&w=2 http://marc.info/?l=linux-netdev&m=142930225730399&w=2 http://marc.info/?l=linux-netdev&m=142930226230400&w=2 It would be good to accept this patch and then later work to port older drivers to use phylib. It might be worthwhile to start beating up on new driver submissions that don't use phylib. I recall someone mentioning that there were plans to have phylib support [Q]SFP[+] modules as well. I am very interested in that work, if someone has patches/plans I would like to take a look. -Jon
On 07/27/2017 08:37 AM, Andrew Lunn wrote: > On Thu, Jul 27, 2017 at 12:40:01AM +0000, Brown, Aaron F wrote: >>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf >>> Of John W. Linville >>> Sent: Friday, July 21, 2017 11:12 AM >>> To: netdev@vger.kernel.org >>> Cc: intel-wired-lan@lists.osuosl.org; John W. Linville >>> <linville@tuxdriver.com> >>> Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY >>> >>> The management port on an Edgecore AS7712-32 switch uses an igb MAC, >>> but >>> it uses a BCM54616 PHY. Without a patch like this, loading the igb >>> module produces dmesg output like this: >>> >>> [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation. >>> [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2 >>> >>> Signed-off-by: John W. Linville <linville@tuxdriver.com> >>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >>> --- >>> drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++ >>> drivers/net/ethernet/intel/igb/e1000_defines.h | 1 + >>> drivers/net/ethernet/intel/igb/e1000_hw.h | 1 + >>> 3 files changed, 8 insertions(+) >> >> I do not have the specific hardware (Edgecore switch) but as far as regression tests go this works fine. >> Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > Sorry, missed the initial post, so replying to a reply. > > Linux has supported the BCM54616 PHY since April 2015. If the Intel > drivers used the Linux PHY drivers, you would not of had this problem. > > It would be good if somebody spent the time to migrate these MAC > drivers to use the common Linux PHY infrastructure. I suspect there is a design pattern within the Intel drivers to share as much low-level code as possible between OSes and only have some Linux-ism where necessary (e.g: net_device, ethtool etc.). PHY code is a pain in general, especially if you are serious about testing interoperability (which is where you can spend tons of $$$ with little reward but just say: yes it works), so it may make sense to share it across different OSes. I too, wish there was more sharing, but considering that this works for the Intel driver, there is little incentive in doing this I suppose...
> I recall someone mentioning that there were plans to have phylib support > [Q]SFP[+] modules as well. I am very interested in that work, if someone > has patches/plans I would like to take a look. Hi Jon The first part was posted yesterday: https://www.spinics.net/lists/netdev/msg445767.html Andrew
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev- > owner@vger.kernel.org] On Behalf Of Florian Fainelli > Sent: Thursday, July 27, 2017 10:58 AM > To: Andrew Lunn <andrew@lunn.ch>; Brown, Aaron F > <aaron.f.brown@intel.com> > Cc: John W. Linville <linville@tuxdriver.com>; netdev@vger.kernel.org; intel- > wired-lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY > > On 07/27/2017 08:37 AM, Andrew Lunn wrote: > > On Thu, Jul 27, 2017 at 12:40:01AM +0000, Brown, Aaron F wrote: > >>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On > Behalf > >>> Of John W. Linville > >>> Sent: Friday, July 21, 2017 11:12 AM > >>> To: netdev@vger.kernel.org > >>> Cc: intel-wired-lan@lists.osuosl.org; John W. Linville > >>> <linville@tuxdriver.com> > >>> Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY > >>> > >>> The management port on an Edgecore AS7712-32 switch uses an igb MAC, > >>> but > >>> it uses a BCM54616 PHY. Without a patch like this, loading the igb > >>> module produces dmesg output like this: > >>> > >>> [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation. > >>> [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2 > >>> > >>> Signed-off-by: John W. Linville <linville@tuxdriver.com> > >>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > >>> --- > >>> drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++ > >>> drivers/net/ethernet/intel/igb/e1000_defines.h | 1 + > >>> drivers/net/ethernet/intel/igb/e1000_hw.h | 1 + > >>> 3 files changed, 8 insertions(+) > >> > >> I do not have the specific hardware (Edgecore switch) but as far as > regression tests go this works fine. > >> Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > > > Sorry, missed the initial post, so replying to a reply. > > > > Linux has supported the BCM54616 PHY since April 2015. If the Intel > > drivers used the Linux PHY drivers, you would not of had this problem. > > > > It would be good if somebody spent the time to migrate these MAC > > drivers to use the common Linux PHY infrastructure. > > I suspect there is a design pattern within the Intel drivers to share as > much low-level code as possible between OSes and only have some > Linux-ism where necessary (e.g: net_device, ethtool etc.). > > PHY code is a pain in general, especially if you are serious about > testing interoperability (which is where you can spend tons of $$$ with > little reward but just say: yes it works), so it may make sense to share > it across different OSes. > > I too, wish there was more sharing, but considering that this works for > the Intel driver, there is little incentive in doing this I suppose... > -- > Florian Yes Florian you are correct generally, but future implementations have not been ruled out. With this driver, we had our custom phy init code tested and released for several products before phylib existed. I began research on a phylib implementation for it at one point, but internal product decisions ended up lowering the priority of that work. Thanks, Carolyn Carolyn Wyborny Linux Development Networking Division Intel Corporation
diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c index ee443985581f..a87bc7dcea80 100644 --- a/drivers/net/ethernet/intel/igb/e1000_82575.c +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c @@ -339,6 +339,9 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw) phy->ops.set_d3_lplu_state = igb_set_d3_lplu_state_82580; phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_m88; break; + case BCM54616_E_PHY_ID: + phy->type = e1000_phy_bcm54616; + break; default: ret_val = -E1000_ERR_PHY; goto out; @@ -1658,6 +1661,9 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw) case e1000_phy_82580: ret_val = igb_copper_link_setup_82580(hw); break; + case e1000_phy_bcm54616: + ret_val = 0; + break; default: ret_val = -E1000_ERR_PHY; break; diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h index 2688180a7acd..ce95b7ec1bc7 100644 --- a/drivers/net/ethernet/intel/igb/e1000_defines.h +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h @@ -867,6 +867,7 @@ #define I210_I_PHY_ID 0x01410C00 #define M88E1543_E_PHY_ID 0x01410EA0 #define M88E1512_E_PHY_ID 0x01410DD0 +#define BCM54616_E_PHY_ID 0x03625D10 /* 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 2fb2213cd562..644d8f59debe 100644 --- a/drivers/net/ethernet/intel/igb/e1000_hw.h +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h @@ -128,6 +128,7 @@ enum e1000_phy_type { e1000_phy_ife, e1000_phy_82580, e1000_phy_i210, + e1000_phy_bcm54616, }; enum e1000_bus_type {
The management port on an Edgecore AS7712-32 switch uses an igb MAC, but it uses a BCM54616 PHY. Without a patch like this, loading the igb module produces dmesg output like this: [ 3.439125] igb: Copyright (c) 2007-2014 Intel Corporation. [ 3.439866] igb: probe of 0000:00:14.0 failed with error -2 Signed-off-by: John W. Linville <linville@tuxdriver.com> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> --- drivers/net/ethernet/intel/igb/e1000_82575.c | 6 ++++++ drivers/net/ethernet/intel/igb/e1000_defines.h | 1 + drivers/net/ethernet/intel/igb/e1000_hw.h | 1 + 3 files changed, 8 insertions(+)