| Message ID | 20180627000612.27263-4-paul.burton@mips.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: pch_gbe: Fixes, conversion to phylib, enable for MIPS | expand |
On Tue, Jun 26, 2018 at 05:06:04PM -0700, Paul Burton wrote: > The pch_gbe driver currently probes for the PHY ID & configures the PHY > every time the MAC is reset, even though we know that the PHY won't have > changed since the last MAC reset [1]. > > This patch moves the PHY probe to instead happen only once when the > driver is probed, saving time & moving us closer to the behavior we'll > have with phylib. > > [1] Please, someone patent PHY hotplugging & rigorously enforce said > patent such that nobody can do it. At least not with an EG20T MAC. Hi Paul It is already possible, and probably patented. SFP cages are usually used for fibre optical modules. But it is also possible to have copper modules, which contain a standard PHY. And SFP modules are hot-plugable... > > Signed-off-by: Paul Burton <paul.burton@mips.com> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: David S. Miller <davem@davemloft.net> > Cc: netdev@vger.kernel.org > --- > > Changes in v7: New patch > > .../ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 26 ++++++++++--------- > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > index 9651fa02d4bb..5157cea16773 100644 > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > @@ -617,8 +617,10 @@ static void pch_gbe_init_stats(struct pch_gbe_adapter *adapter) > static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > + struct pch_gbe_hw *hw = &adapter->hw; > u32 addr; > u16 bmcr, stat; > + s32 ret_val; > > /* Discover phy addr by searching addrs in order {1,0,2,..., 31} */ > for (addr = 0; addr < PCH_GBE_PHY_REGS_LEN; addr++) { > @@ -652,6 +654,16 @@ static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter) > adapter->mii.mdio_read = pch_gbe_mdio_read; > adapter->mii.mdio_write = pch_gbe_mdio_write; > adapter->mii.supports_gmii = mii_check_gmii_support(&adapter->mii); > + > + ret_val = pch_gbe_phy_get_id(hw); > + if (ret_val) { > + netdev_err(adapter->netdev, "pch_gbe_phy_get_id error\n"); > + return -EIO; > + } > + pch_gbe_phy_init_setting(hw); > + /* Setup Mac interface option RGMII */ > + pch_gbe_phy_set_rgmii(hw); > + > return 0; > } > > @@ -721,22 +733,12 @@ void pch_gbe_reset(struct pch_gbe_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > struct pch_gbe_hw *hw = &adapter->hw; > - s32 ret_val; > > pch_gbe_mac_reset_hw(hw); > /* reprogram multicast address register after reset */ > pch_gbe_set_multi(netdev); > /* Setup the receive address. */ > pch_gbe_mac_init_rx_addrs(hw, PCH_GBE_MAR_ENTRIES); > - > - ret_val = pch_gbe_phy_get_id(hw); > - if (ret_val) { > - netdev_err(adapter->netdev, "pch_gbe_phy_get_id error\n"); > - return; > - } > - pch_gbe_phy_init_setting(hw); > - /* Setup Mac interface option RGMII */ > - pch_gbe_phy_set_rgmii(hw); > } > > /** Up to here, everything looks O.K. > @@ -2577,6 +2579,8 @@ static int pch_gbe_probe(struct pci_dev *pdev, > if (ret) > goto err_free_netdev; > > + pch_gbe_check_options(adapter); > + > /* Initialize PHY */ > ret = pch_gbe_init_phy(adapter); > if (ret) { > @@ -2606,8 +2610,6 @@ static int pch_gbe_probe(struct pci_dev *pdev, > > INIT_WORK(&adapter->reset_task, pch_gbe_reset_task); > > - pch_gbe_check_options(adapter); > - > /* initialize the wol settings based on the eeprom settings */ > adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING; > dev_info(&pdev->dev, "MAC address : %pM\n", netdev->dev_addr); But these two changes seem unrelated. Should they be in a different patch? Andrew
Hi Andrew, On Wed, Jun 27, 2018 at 07:21:31PM +0200, Andrew Lunn wrote: > > [1] Please, someone patent PHY hotplugging & rigorously enforce said > > patent such that nobody can do it. At least not with an EG20T MAC. > > Hi Paul > > It is already possible, and probably patented. SFP cages are usually > used for fibre optical modules. But it is also possible to have copper > modules, which contain a standard PHY. And SFP modules are > hot-plugable... D'oh, but at least not relevant to the EG20T/pch_gbe :) > > @@ -2577,6 +2579,8 @@ static int pch_gbe_probe(struct pci_dev *pdev, > > if (ret) > > goto err_free_netdev; > > > > + pch_gbe_check_options(adapter); > > + > > /* Initialize PHY */ > > ret = pch_gbe_init_phy(adapter); > > if (ret) { > > @@ -2606,8 +2610,6 @@ static int pch_gbe_probe(struct pci_dev *pdev, > > > > INIT_WORK(&adapter->reset_task, pch_gbe_reset_task); > > > > - pch_gbe_check_options(adapter); > > - > > /* initialize the wol settings based on the eeprom settings */ > > adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING; > > dev_info(&pdev->dev, "MAC address : %pM\n", netdev->dev_addr); > > But these two changes seem unrelated. Should they be in a different > patch? This is actually needed because pch_gbe_check_options() sets up, amongst other things, the autoneg_advertised field in struct pch_gbe_phy_info and that needs to happen before pch_gbe_phy_init_setting() is called. Thanks, Paul
> This is actually needed because pch_gbe_check_options() sets up, amongst > other things, the autoneg_advertised field in struct pch_gbe_phy_info > and that needs to happen before pch_gbe_phy_init_setting() is called. Hi Paul Please add a comment to the commit message about this. Andrew
On Tue, Jun 26, 2018 at 05:06:04PM -0700, Paul Burton wrote: > The pch_gbe driver currently probes for the PHY ID & configures the PHY > every time the MAC is reset, even though we know that the PHY won't have > changed since the last MAC reset [1]. > > This patch moves the PHY probe to instead happen only once when the > driver is probed, saving time & moving us closer to the behavior we'll > have with phylib. > > [1] Please, someone patent PHY hotplugging & rigorously enforce said > patent such that nobody can do it. At least not with an EG20T MAC. I suppose one additional point here is that with the change to phylib, and the removal of the code to disable hibernation, the phyid becomes irrelevant to the MAC driver. So the important change here is moving phy initialization to the probe. Andrew
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c index 9651fa02d4bb..5157cea16773 100644 --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c @@ -617,8 +617,10 @@ static void pch_gbe_init_stats(struct pch_gbe_adapter *adapter) static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter) { struct net_device *netdev = adapter->netdev; + struct pch_gbe_hw *hw = &adapter->hw; u32 addr; u16 bmcr, stat; + s32 ret_val; /* Discover phy addr by searching addrs in order {1,0,2,..., 31} */ for (addr = 0; addr < PCH_GBE_PHY_REGS_LEN; addr++) { @@ -652,6 +654,16 @@ static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter) adapter->mii.mdio_read = pch_gbe_mdio_read; adapter->mii.mdio_write = pch_gbe_mdio_write; adapter->mii.supports_gmii = mii_check_gmii_support(&adapter->mii); + + ret_val = pch_gbe_phy_get_id(hw); + if (ret_val) { + netdev_err(adapter->netdev, "pch_gbe_phy_get_id error\n"); + return -EIO; + } + pch_gbe_phy_init_setting(hw); + /* Setup Mac interface option RGMII */ + pch_gbe_phy_set_rgmii(hw); + return 0; } @@ -721,22 +733,12 @@ void pch_gbe_reset(struct pch_gbe_adapter *adapter) { struct net_device *netdev = adapter->netdev; struct pch_gbe_hw *hw = &adapter->hw; - s32 ret_val; pch_gbe_mac_reset_hw(hw); /* reprogram multicast address register after reset */ pch_gbe_set_multi(netdev); /* Setup the receive address. */ pch_gbe_mac_init_rx_addrs(hw, PCH_GBE_MAR_ENTRIES); - - ret_val = pch_gbe_phy_get_id(hw); - if (ret_val) { - netdev_err(adapter->netdev, "pch_gbe_phy_get_id error\n"); - return; - } - pch_gbe_phy_init_setting(hw); - /* Setup Mac interface option RGMII */ - pch_gbe_phy_set_rgmii(hw); } /** @@ -2577,6 +2579,8 @@ static int pch_gbe_probe(struct pci_dev *pdev, if (ret) goto err_free_netdev; + pch_gbe_check_options(adapter); + /* Initialize PHY */ ret = pch_gbe_init_phy(adapter); if (ret) { @@ -2606,8 +2610,6 @@ static int pch_gbe_probe(struct pci_dev *pdev, INIT_WORK(&adapter->reset_task, pch_gbe_reset_task); - pch_gbe_check_options(adapter); - /* initialize the wol settings based on the eeprom settings */ adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING; dev_info(&pdev->dev, "MAC address : %pM\n", netdev->dev_addr);
The pch_gbe driver currently probes for the PHY ID & configures the PHY every time the MAC is reset, even though we know that the PHY won't have changed since the last MAC reset [1]. This patch moves the PHY probe to instead happen only once when the driver is probed, saving time & moving us closer to the behavior we'll have with phylib. [1] Please, someone patent PHY hotplugging & rigorously enforce said patent such that nobody can do it. At least not with an EG20T MAC. Signed-off-by: Paul Burton <paul.burton@mips.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: David S. Miller <davem@davemloft.net> Cc: netdev@vger.kernel.org --- Changes in v7: New patch .../ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-)