| Message ID | 20180627000612.27263-7-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:07PM -0700, Paul Burton wrote: > When using a PHY connected via RGMII, as the pch_gbe driver presumes is > the case, the RX clock is provided by the PHY to the MAC. Various PHYs, > including both the AR8031 used by the Minnowboard & the RTL8211E used by > the MIPS Boston development board, will stop generating the RX clock > when the ethernet link is down (eg. the ethernet cable is unplugged). > > Various pieces of functionality in the EG20T MAC, ranging from basics > like completing a MAC reset to programming MAC addresses, rely upon the > RX clock being provided. When the clock is not provided these pieces of > functionality simply never complete, and the busy bits that indicate > they're in progress remain set indefinitely. > > The pch_gbe driver currently requires that the RX clock is always > provided, and attempts to enforce this by disabling the hibernation > feature of the AR8031 PHY to keep it generating the RX clock. This patch > moves us away from this model by only configuring the MAC when the PHY > indicates that the ethernet link is up. When the link is up we should be > able to safely expect that the RX clock is being provided, and therefore > safely reset & configure the MAC. Hi Paul I like the concept, but the implementation is not clear. Maybe it just needs more details in the commit message. What has the watchdog got to do with link up? And what happens on link down? Does the MAC need shutting down? I don't see such code here. Andrew
On 06/26/2018 05:06 PM, Paul Burton wrote: > When using a PHY connected via RGMII, as the pch_gbe driver presumes is > the case, the RX clock is provided by the PHY to the MAC. Various PHYs, > including both the AR8031 used by the Minnowboard & the RTL8211E used by > the MIPS Boston development board, will stop generating the RX clock > when the ethernet link is down (eg. the ethernet cable is unplugged). > > Various pieces of functionality in the EG20T MAC, ranging from basics > like completing a MAC reset to programming MAC addresses, rely upon the > RX clock being provided. When the clock is not provided these pieces of > functionality simply never complete, and the busy bits that indicate > they're in progress remain set indefinitely. > > The pch_gbe driver currently requires that the RX clock is always > provided, and attempts to enforce this by disabling the hibernation > feature of the AR8031 PHY to keep it generating the RX clock. This patch > moves us away from this model by only configuring the MAC when the PHY > indicates that the ethernet link is up. When the link is up we should be > able to safely expect that the RX clock is being provided, and therefore > safely reset & configure the MAC. What we ended up doing in the bcmgenet driver is loop back the RX and TX clocks such that we always have a clock that we can use to perform any MAC operation, including reset. Is this an option here? You might also want to split the allocation from the actual initialization if this is not done already. > > 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 | 44 +++++++++---------- > 1 file changed, 22 insertions(+), 22 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 eb290c1edce0..721ce29b6467 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 > @@ -1837,7 +1837,6 @@ static int pch_gbe_request_irq(struct pch_gbe_adapter *adapter) > int pch_gbe_up(struct pch_gbe_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > - struct pch_gbe_tx_ring *tx_ring = adapter->tx_ring; > struct pch_gbe_rx_ring *rx_ring = adapter->rx_ring; > int err = -EINVAL; > > @@ -1847,14 +1846,6 @@ int pch_gbe_up(struct pch_gbe_adapter *adapter) > goto out; > } > > - /* hardware has been reset, we need to reload some things */ > - pch_gbe_set_multi(netdev); > - > - pch_gbe_setup_tctl(adapter); > - pch_gbe_configure_tx(adapter); > - pch_gbe_setup_rctl(adapter); > - pch_gbe_configure_rx(adapter); > - > err = pch_gbe_request_irq(adapter); > if (err) { > netdev_err(netdev, > @@ -1867,18 +1858,9 @@ int pch_gbe_up(struct pch_gbe_adapter *adapter) > "Error: can't bring device up - alloc rx buffers pool failed\n"); > goto freeirq; > } > - pch_gbe_alloc_tx_buffers(adapter, tx_ring); > - pch_gbe_alloc_rx_buffers(adapter, rx_ring, rx_ring->count); > adapter->tx_queue_len = netdev->tx_queue_len; > - pch_gbe_enable_dma_rx(&adapter->hw); > - pch_gbe_enable_mac_rx(&adapter->hw); > > mod_timer(&adapter->watchdog_timer, jiffies); > - > - napi_enable(&adapter->napi); > - pch_gbe_irq_enable(adapter); > - netif_start_queue(adapter->netdev); > - > return 0; > > freeirq: > @@ -1930,6 +1912,8 @@ static void pch_gbe_watchdog(struct timer_list *t) > { > struct pch_gbe_adapter *adapter = from_timer(adapter, t, > watchdog_timer); > + struct pch_gbe_rx_ring *rx_ring = adapter->rx_ring; > + struct pch_gbe_tx_ring *tx_ring = adapter->tx_ring; > struct net_device *netdev = adapter->netdev; > struct pch_gbe_hw *hw = &adapter->hw; > > @@ -1950,12 +1934,32 @@ static void pch_gbe_watchdog(struct timer_list *t) > } > hw->mac.link_speed = ethtool_cmd_speed(&cmd); > hw->mac.link_duplex = cmd.duplex; > + > + pch_gbe_reset(adapter); > + > /* Set the RGMII control. */ > pch_gbe_set_rgmii_ctrl(adapter, hw->mac.link_speed, > hw->mac.link_duplex); > /* Set the communication mode */ > pch_gbe_set_mode(adapter, hw->mac.link_speed, > hw->mac.link_duplex); > + > + pch_gbe_set_multi(netdev); > + pch_gbe_setup_tctl(adapter); > + pch_gbe_configure_tx(adapter); > + pch_gbe_setup_rctl(adapter); > + pch_gbe_configure_rx(adapter); > + > + pch_gbe_alloc_tx_buffers(adapter, tx_ring); > + pch_gbe_alloc_rx_buffers(adapter, rx_ring, rx_ring->count); > + > + pch_gbe_enable_dma_rx(&adapter->hw); > + pch_gbe_enable_mac_rx(&adapter->hw); > + > + napi_enable(&adapter->napi); > + pch_gbe_irq_enable(adapter); > + netif_start_queue(adapter->netdev); > + > netdev_dbg(netdev, > "Link is Up %d Mbps %s-Duplex\n", > hw->mac.link_speed, > @@ -2568,7 +2572,6 @@ static int pch_gbe_probe(struct pci_dev *pdev, > (ETH_HLEN + ETH_FCS_LEN); > > pch_gbe_mac_load_mac_addr(&adapter->hw); > - pch_gbe_mac_reset_hw(&adapter->hw); > > /* setup the private structure */ > ret = pch_gbe_sw_init(adapter); > @@ -2610,9 +2613,6 @@ static int pch_gbe_probe(struct pci_dev *pdev, > adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING; > dev_info(&pdev->dev, "MAC address : %pM\n", netdev->dev_addr); > > - /* reset the hardware with the new settings */ > - pch_gbe_reset(adapter); > - > ret = register_netdev(netdev); > if (ret) > goto err_free_adapter; >
Hi Andrew, On Wed, Jun 27, 2018 at 07:30:14PM +0200, Andrew Lunn wrote: > On Tue, Jun 26, 2018 at 05:06:07PM -0700, Paul Burton wrote: > > When using a PHY connected via RGMII, as the pch_gbe driver presumes is > > the case, the RX clock is provided by the PHY to the MAC. Various PHYs, > > including both the AR8031 used by the Minnowboard & the RTL8211E used by > > the MIPS Boston development board, will stop generating the RX clock > > when the ethernet link is down (eg. the ethernet cable is unplugged). > > > > Various pieces of functionality in the EG20T MAC, ranging from basics > > like completing a MAC reset to programming MAC addresses, rely upon the > > RX clock being provided. When the clock is not provided these pieces of > > functionality simply never complete, and the busy bits that indicate > > they're in progress remain set indefinitely. > > > > The pch_gbe driver currently requires that the RX clock is always > > provided, and attempts to enforce this by disabling the hibernation > > feature of the AR8031 PHY to keep it generating the RX clock. This patch > > moves us away from this model by only configuring the MAC when the PHY > > indicates that the ethernet link is up. When the link is up we should be > > able to safely expect that the RX clock is being provided, and therefore > > safely reset & configure the MAC. > > Hi Paul > > I like the concept, but the implementation is not clear. Maybe it just > needs more details in the commit message. What has the watchdog got to > do with link up? pch_gbe_watchdog() polls for the link coming up or going down, so that's where we find out that the link is up. > And what happens on link down? Does the MAC need shutting down? I > don't see such code here. Well, depending upon the PHY the RX clock might stop which will prevent parts of the MAC from functioning properly. Exactly which parts I don't really know - the EG20T documentation is vague & unclear. I do know that: - We won't receive packets any more, of course. This should be fine without any extra handling because we just won't see any futher DMA complete interrupts (or the associated bit set when polling). - A MAC reset won't complete - ie. the pch_gbe_wait_clr_bit() in pch_gbe_reset()/pch_gbe_reset_hw() will time out. This I think should be OK because after this patch we won't generally reset the MAC when the link is down anyway, except perhaps the PCI error_state case in pch_gbe_down(). I'm not sure what the reset there is for... - Masking or unmasking MAC address registers won't complete - ie. the pch_gbe_wait_clr_bit() in pch_gbe_mac_mar_set() or pch_gbe_set_multi() will time out. This is again when the link is already known to be up, although there is a case in __pch_gbe_suspend() which is setting up WoL that I'm not so sure about... Thanks, Paul
Hi Florian, On Wed, Jun 27, 2018 at 10:54:24AM -0700, Florian Fainelli wrote: > On 06/26/2018 05:06 PM, Paul Burton wrote: > > When using a PHY connected via RGMII, as the pch_gbe driver presumes is > > the case, the RX clock is provided by the PHY to the MAC. Various PHYs, > > including both the AR8031 used by the Minnowboard & the RTL8211E used by > > the MIPS Boston development board, will stop generating the RX clock > > when the ethernet link is down (eg. the ethernet cable is unplugged). > > > > Various pieces of functionality in the EG20T MAC, ranging from basics > > like completing a MAC reset to programming MAC addresses, rely upon the > > RX clock being provided. When the clock is not provided these pieces of > > functionality simply never complete, and the busy bits that indicate > > they're in progress remain set indefinitely. > > > > The pch_gbe driver currently requires that the RX clock is always > > provided, and attempts to enforce this by disabling the hibernation > > feature of the AR8031 PHY to keep it generating the RX clock. This patch > > moves us away from this model by only configuring the MAC when the PHY > > indicates that the ethernet link is up. When the link is up we should be > > able to safely expect that the RX clock is being provided, and therefore > > safely reset & configure the MAC. > > What we ended up doing in the bcmgenet driver is loop back the RX and TX > clocks such that we always have a clock that we can use to perform any > MAC operation, including reset. > > Is this an option here? That sounds like a nice solution, but I don't see a way to do it in the EG20T datasheet[1]. > You might also want to split the allocation from the actual > initialization if this is not done already. Some of the buffer allocation is still happening in pch_gbe_up() rather than when the link actually comes up, but there is more that could probably be moved. Thanks, Paul [1] https://www.intel.com/content/www/us/en/intelligent-systems/queens-bay/platform-controller-hub-eg20t-datasheet.html
On Wed, Jun 27, 2018 at 10:54:28AM -0700, Paul Burton wrote: > Hi Andrew, > > On Wed, Jun 27, 2018 at 07:30:14PM +0200, Andrew Lunn wrote: > > On Tue, Jun 26, 2018 at 05:06:07PM -0700, Paul Burton wrote: > > > When using a PHY connected via RGMII, as the pch_gbe driver presumes is > > > the case, the RX clock is provided by the PHY to the MAC. Various PHYs, > > > including both the AR8031 used by the Minnowboard & the RTL8211E used by > > > the MIPS Boston development board, will stop generating the RX clock > > > when the ethernet link is down (eg. the ethernet cable is unplugged). > > > > > > Various pieces of functionality in the EG20T MAC, ranging from basics > > > like completing a MAC reset to programming MAC addresses, rely upon the > > > RX clock being provided. When the clock is not provided these pieces of > > > functionality simply never complete, and the busy bits that indicate > > > they're in progress remain set indefinitely. > > > > > > The pch_gbe driver currently requires that the RX clock is always > > > provided, and attempts to enforce this by disabling the hibernation > > > feature of the AR8031 PHY to keep it generating the RX clock. This patch > > > moves us away from this model by only configuring the MAC when the PHY > > > indicates that the ethernet link is up. When the link is up we should be > > > able to safely expect that the RX clock is being provided, and therefore > > > safely reset & configure the MAC. > > > > Hi Paul > > > > I like the concept, but the implementation is not clear. Maybe it just > > needs more details in the commit message. What has the watchdog got to > > do with link up? > > pch_gbe_watchdog() polls for the link coming up or going down, so that's > where we find out that the link is up. I was thinking it would be something like that. So could you please explain this in the commit message. Does the watchdog later become the adjust_link callback for phylib? Having a name based around adjust_link would make this clearer. That is the norm. But i understand this is a preparation step, so the rename might happen later? > > And what happens on link down? Does the MAC need shutting down? I > > don't see such code here. > > Well, depending upon the PHY the RX clock might stop which will prevent > parts of the MAC from functioning properly. The datasheet for the Atheros PHY suggests the clock will stop after a while. So again, commenting why you think nothing extra is needed would be good. Basically, there is a lot of non-obvious stuff going on here, and it helps both reviewer and future debugger to have a fuller explanation. Thanks 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 eb290c1edce0..721ce29b6467 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 @@ -1837,7 +1837,6 @@ static int pch_gbe_request_irq(struct pch_gbe_adapter *adapter) int pch_gbe_up(struct pch_gbe_adapter *adapter) { struct net_device *netdev = adapter->netdev; - struct pch_gbe_tx_ring *tx_ring = adapter->tx_ring; struct pch_gbe_rx_ring *rx_ring = adapter->rx_ring; int err = -EINVAL; @@ -1847,14 +1846,6 @@ int pch_gbe_up(struct pch_gbe_adapter *adapter) goto out; } - /* hardware has been reset, we need to reload some things */ - pch_gbe_set_multi(netdev); - - pch_gbe_setup_tctl(adapter); - pch_gbe_configure_tx(adapter); - pch_gbe_setup_rctl(adapter); - pch_gbe_configure_rx(adapter); - err = pch_gbe_request_irq(adapter); if (err) { netdev_err(netdev, @@ -1867,18 +1858,9 @@ int pch_gbe_up(struct pch_gbe_adapter *adapter) "Error: can't bring device up - alloc rx buffers pool failed\n"); goto freeirq; } - pch_gbe_alloc_tx_buffers(adapter, tx_ring); - pch_gbe_alloc_rx_buffers(adapter, rx_ring, rx_ring->count); adapter->tx_queue_len = netdev->tx_queue_len; - pch_gbe_enable_dma_rx(&adapter->hw); - pch_gbe_enable_mac_rx(&adapter->hw); mod_timer(&adapter->watchdog_timer, jiffies); - - napi_enable(&adapter->napi); - pch_gbe_irq_enable(adapter); - netif_start_queue(adapter->netdev); - return 0; freeirq: @@ -1930,6 +1912,8 @@ static void pch_gbe_watchdog(struct timer_list *t) { struct pch_gbe_adapter *adapter = from_timer(adapter, t, watchdog_timer); + struct pch_gbe_rx_ring *rx_ring = adapter->rx_ring; + struct pch_gbe_tx_ring *tx_ring = adapter->tx_ring; struct net_device *netdev = adapter->netdev; struct pch_gbe_hw *hw = &adapter->hw; @@ -1950,12 +1934,32 @@ static void pch_gbe_watchdog(struct timer_list *t) } hw->mac.link_speed = ethtool_cmd_speed(&cmd); hw->mac.link_duplex = cmd.duplex; + + pch_gbe_reset(adapter); + /* Set the RGMII control. */ pch_gbe_set_rgmii_ctrl(adapter, hw->mac.link_speed, hw->mac.link_duplex); /* Set the communication mode */ pch_gbe_set_mode(adapter, hw->mac.link_speed, hw->mac.link_duplex); + + pch_gbe_set_multi(netdev); + pch_gbe_setup_tctl(adapter); + pch_gbe_configure_tx(adapter); + pch_gbe_setup_rctl(adapter); + pch_gbe_configure_rx(adapter); + + pch_gbe_alloc_tx_buffers(adapter, tx_ring); + pch_gbe_alloc_rx_buffers(adapter, rx_ring, rx_ring->count); + + pch_gbe_enable_dma_rx(&adapter->hw); + pch_gbe_enable_mac_rx(&adapter->hw); + + napi_enable(&adapter->napi); + pch_gbe_irq_enable(adapter); + netif_start_queue(adapter->netdev); + netdev_dbg(netdev, "Link is Up %d Mbps %s-Duplex\n", hw->mac.link_speed, @@ -2568,7 +2572,6 @@ static int pch_gbe_probe(struct pci_dev *pdev, (ETH_HLEN + ETH_FCS_LEN); pch_gbe_mac_load_mac_addr(&adapter->hw); - pch_gbe_mac_reset_hw(&adapter->hw); /* setup the private structure */ ret = pch_gbe_sw_init(adapter); @@ -2610,9 +2613,6 @@ static int pch_gbe_probe(struct pci_dev *pdev, adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING; dev_info(&pdev->dev, "MAC address : %pM\n", netdev->dev_addr); - /* reset the hardware with the new settings */ - pch_gbe_reset(adapter); - ret = register_netdev(netdev); if (ret) goto err_free_adapter;
When using a PHY connected via RGMII, as the pch_gbe driver presumes is the case, the RX clock is provided by the PHY to the MAC. Various PHYs, including both the AR8031 used by the Minnowboard & the RTL8211E used by the MIPS Boston development board, will stop generating the RX clock when the ethernet link is down (eg. the ethernet cable is unplugged). Various pieces of functionality in the EG20T MAC, ranging from basics like completing a MAC reset to programming MAC addresses, rely upon the RX clock being provided. When the clock is not provided these pieces of functionality simply never complete, and the busy bits that indicate they're in progress remain set indefinitely. The pch_gbe driver currently requires that the RX clock is always provided, and attempts to enforce this by disabling the hibernation feature of the AR8031 PHY to keep it generating the RX clock. This patch moves us away from this model by only configuring the MAC when the PHY indicates that the ethernet link is up. When the link is up we should be able to safely expect that the RX clock is being provided, and therefore safely reset & configure the 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 | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-)