Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
On 16/11/2017 17:23, Andrew Lunn wrote: > Maybe take a look at your memory barriers. Most accesses using the > _relaxed() version, i.e, no barrier. And then there are specific > barriers when needed. One could be missing. > > As a quick test, drop the _relaxed. Force a barrier with each > access. If that works, it is a clear indication you have a barrier > problem. That was an interesting suggestion, thanks! Unfortunately, adding wmb() in dozens of strategic places doesn't prevent the issue where network connectivity is lost :-( Regards.
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
> I'm starting to think there is some kind of race condition between > SW and HW handling of descriptors. This might also explain the > out-of-order warnings. Hi Marc Maybe take a look at your memory barriers. Most accesses using the _relaxed() version, i.e, no barrier. And then there are specific barriers when needed. One could be missing. As a quick test, drop the _relaxed. Force a barrier with each access. If that works, it is a clear indication you have a barrier problem. Andrew
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
On 15/11/2017 17:15, Marc Gonzalez wrote: > Given the out-of-order datagrams, I'm wondering if it's possible > for the DMA engine to overwrite a not-yet-read descriptor? > > The EOC flag should stop the DMA engine though... > > Maybe some kind of race... > > I don't think I've been able to trigger the wedge when 256 descriptors > are used. I'm still taking stabs in the dark. Adding a 10 ms delay for every 1024 packets, and using 256 descriptors doesn't cause any hang, even after an hour. At 85 packets per ms, 10 ms is large enough to consume all available descriptors... Yet no issue. Only when I lower the number of available RX descriptors do I see the issue. And I don't need any delay... I'm starting to think there is some kind of race condition between SW and HW handling of descriptors. This might also explain the out-of-order warnings. Lowering the number of RX descriptors to 64, and adding no delay causes many OUT OF ORDER warnings, and the network locks up in under a minute: iperf3: OUT OF ORDER - incoming packet = 13031 and received packet = 0 AND SP = 13094 iperf3: OUT OF ORDER - incoming packet = 21441 and received packet = 0 AND SP = 21504 iperf3: OUT OF ORDER - incoming packet = 21442 and received packet = 0 AND SP = 21504 [ ID] Interval Transfer Bandwidth JitterLost/Total Datagrams [ 5] 0.00-1.00 sec 97.4 MBytes 817 Mbits/sec 0.016 ms 8793/81753 (11%) iperf3: OUT OF ORDER - incoming packet = 92503 and received packet = 0 AND SP = 92566 iperf3: OUT OF ORDER - incoming packet = 92504 and received packet = 0 AND SP = 92566 iperf3: OUT OF ORDER - incoming packet = 125503 and received packet = 0 AND SP = 125566 [ 5] 1.00-2.00 sec 101 MBytes 851 Mbits/sec 0.011 ms 8835/84797 (10%) iperf3: OUT OF ORDER - incoming packet = 198710 and received packet = 0 AND SP = 198773 iperf3: OUT OF ORDER - incoming packet = 243272 and received packet = 0 AND SP = 243335 iperf3: OUT OF ORDER - incoming packet = 243791 and received packet = 0 AND SP = 243854 [ 5] 2.00-3.00 sec 101 MBytes 851 Mbits/sec 0.016 ms 8771/84782 (10%) iperf3: OUT OF ORDER - incoming packet = 294545 and received packet = 0 AND SP = 294608 iperf3: OUT OF ORDER - incoming packet = 301719 and received packet = 0 AND SP = 301782 iperf3: OUT OF ORDER - incoming packet = 301720 and received packet = 0 AND SP = 301782 iperf3: OUT OF ORDER - incoming packet = 305218 and received packet = 0 AND SP = 305281 iperf3: OUT OF ORDER - incoming packet = 314655 and received packet = 0 AND SP = 314718 iperf3: OUT OF ORDER - incoming packet = 325473 and received packet = 0 AND SP = 325536 iperf3: OUT OF ORDER - incoming packet = 325474 and received packet = 0 AND SP = 325536 [ 5] 3.00-4.00 sec 101 MBytes 850 Mbits/sec 0.013 ms 8903/84798 (10%) iperf3: OUT OF ORDER - incoming packet = 340919 and received packet = 0 AND SP = 340982 iperf3: OUT OF ORDER - incoming packet = 340920 and received packet = 0 AND SP = 340982 iperf3: OUT OF ORDER - incoming packet = 346325 and received packet = 0 AND SP = 346388 iperf3: OUT OF ORDER - incoming packet = 346326 and received packet = 0 AND SP = 346388 [ 5] 4.00-5.00 sec 101 MBytes 851 Mbits/sec 0.012 ms 8868/84829 (10%) iperf3: OUT OF ORDER - incoming packet = 441873 and received packet = 0 AND SP = 441936 iperf3: OUT OF ORDER - incoming packet = 459258 and received packet = 0 AND SP = 459321 iperf3: OUT OF ORDER - incoming packet = 495604 and received packet = 0 AND SP = 495667 [ 5] 5.00-6.00 sec 101 MBytes 850 Mbits/sec 0.013 ms 8835/84760 (10%) iperf3: OUT OF ORDER - incoming packet = 558280 and received packet = 0 AND SP = 558343 iperf3: OUT OF ORDER - incoming packet = 558281 and received packet = 0 AND SP = 558343 iperf3: OUT OF ORDER - incoming packet = 587445 and received packet = 0 AND SP = 587508 iperf3: OUT OF ORDER - incoming packet = 587446 and received packet = 0 AND SP = 587508 [ 5] 6.00-7.00 sec 102 MBytes 854 Mbits/sec 0.016 ms 8580/84843 (10%) iperf3: OUT OF ORDER - incoming packet = 617324 and received packet = 0 AND SP = 617387 iperf3: OUT OF ORDER - incoming packet = 620412 and received packet = 0 AND SP = 620475 iperf3: OUT OF ORDER - incoming packet = 635845 and received packet = 0 AND SP = 635908 iperf3: OUT OF ORDER - incoming packet = 657102 and received packet = 0 AND SP = 657165 iperf3: OUT OF ORDER - incoming packet = 670672 and received packet = 0 AND SP = 670735 iperf3: OUT OF ORDER - incoming packet = 670673 and received packet = 0 AND SP = 670735 [ 5] 7.00-8.00 sec 102 MBytes 853 Mbits/sec 0.012 ms 8582/84757 (10%) iperf3: OUT OF ORDER - incoming packet = 676529 and received packet = 0 AND SP = 676592 iperf3: OUT OF ORDER - incoming packet = 682693 and received packet = 0 AND SP = 682756 iperf3: OUT OF ORDER - incoming packet = 682694 and received packet = 0 AND SP = 682756 iperf3: OUT OF ORDER - incoming packet =
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
On 15/11/2017 16:11, Måns Rullgård wrote: > Well, that's not good. I'll see if I can replicate it later this week. In my latest test, I started from v4.14 and only applied the following patch: diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c index e94159507847..5c109cc4bde8 100644 --- a/drivers/net/ethernet/aurora/nb8800.c +++ b/drivers/net/ethernet/aurora/nb8800.c @@ -317,6 +317,7 @@ static int nb8800_poll(struct napi_struct *napi, int budget) len = RX_BYTES_TRANSFERRED(rxd->report); + udelay(200); if (IS_RX_ERROR(rxd->report)) nb8800_rx_error(dev, rxd->report); else @@ -1246,10 +1247,12 @@ static int nb8800_hw_init(struct net_device *dev) nb8800_writeb(priv, NB8800_PQ1, val >> 8); nb8800_writeb(priv, NB8800_PQ2, val & 0xff); +#if 0 /* Auto-negotiate by default */ priv->pause_aneg = true; priv->pause_rx = true; priv->pause_tx = true; +#endif nb8800_mc_init(dev, 0); diff --git a/drivers/net/ethernet/aurora/nb8800.h b/drivers/net/ethernet/aurora/nb8800.h index aacc3cce2cc0..a2d4b841306a 100644 --- a/drivers/net/ethernet/aurora/nb8800.h +++ b/drivers/net/ethernet/aurora/nb8800.h @@ -8,7 +8,7 @@ #include #include -#define RX_DESC_COUNT 256 +#define RX_DESC_COUNT 16 #define TX_DESC_COUNT 256 #define NB8800_DESC_LOW4 Once I booted the SMP8759 board, I ran ip addr add 172.27.64.23/18 brd 172.27.127.255 dev eth0 ip link set eth0 up sleep 5 iperf3 -s And from the PC, I ran iperf3 -c 172.27.64.23 -u -b 0 -l 1400 -t 120 [ 5] local 172.27.64.23 port 5201 connected to 172.27.64.1 port 53771 [ ID] Interval Transfer Bandwidth JitterLost/Total Datagrams [ 5] 0.00-1.00 sec 5.89 MBytes 49.4 Mbits/sec 0.044 ms 77062/81472 (95%) [ 5] 1.00-2.00 sec 6.25 MBytes 52.5 Mbits/sec 0.052 ms 80112/84796 (94%) [ 5] 2.00-3.00 sec 6.26 MBytes 52.5 Mbits/sec 0.054 ms 80112/84797 (94%) [ 5] 3.00-4.00 sec 6.26 MBytes 52.5 Mbits/sec 0.053 ms 80128/84814 (94%) [ 5] 4.00-5.00 sec 6.26 MBytes 52.5 Mbits/sec 0.049 ms 80112/84798 (94%) [ 5] 5.00-6.00 sec 6.26 MBytes 52.5 Mbits/sec 0.052 ms 80096/84783 (94%) [ 5] 6.00-7.00 sec 6.26 MBytes 52.5 Mbits/sec 0.047 ms 80096/84784 (94%) [ 5] 7.00-8.00 sec 6.26 MBytes 52.5 Mbits/sec 0.060 ms 80128/84815 (94%) iperf3: OUT OF ORDER - incoming packet = 731264 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731265 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731266 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731267 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731268 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731269 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731270 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731271 and received packet = 0 AND SP = 731279 iperf3: OUT OF ORDER - incoming packet = 731272 and received packet = 0 AND SP = 731279 [ 5] 8.00-9.00 sec 4.17 MBytes 35.0 Mbits/sec 0.149 ms 53104/56220 (94%) [ 5] 9.00-10.00 sec 0.00 Bytes 0.00 bits/sec 0.149 ms 0/0 (0%) [ 5] 10.00-11.00 sec 0.00 Bytes 0.00 bits/sec 0.149 ms 0/0 (0%) [ 5] 11.00-12.00 sec 0.00 Bytes 0.00 bits/sec 0.149 ms 0/0 (0%) [ 5] 12.00-13.00 sec 0.00 Bytes 0.00 bits/sec 0.149 ms 0/0 (0%) [ 5] 13.00-14.00 sec 0.00 Bytes 0.00 bits/sec 0.149 ms 0/0 (0%) ^C - - - - - - - - - - - - - - - - - - - - - - - - - [ ID] Interval Transfer Bandwidth JitterLost/Total Datagrams [ 5] 0.00-17.04 sec 0.00 Bytes 0.00 bits/sec 0.149 ms 690950/731279 (94%) [SUM] 0.0-17.0 sec 9 datagrams received out-of-order iperf3: interrupt - the server has terminated # ping -c 10 172.27.64.1 PING 172.27.64.1 (172.27.64.1): 56 data bytes --- 172.27.64.1 ping statistics --- 10 packets transmitted, 0 packets received, 100% packet loss Given the out-of-order datagrams, I'm wondering if it's possible for the DMA engine to overwrite a not-yet-read descriptor? The EOC flag should stop the DMA engine though... Maybe some kind of race... I don't think I've been able to trigger the wedge when 256 descriptors are used.
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Marc Gonzalez writes: > On 14/11/2017 17:55, Måns Rullgård wrote: > >> Marc Gonzalez wrote: >> >>> I will run iperf3 tests with RX_DESC_COUNT lowered to 2. >>> Would that produce conclusive results? >>> Do you have other suggestions? >> >> Leave RX_DESC_COUNT alone but add a delay in the nb8800_poll() loop. >> That should ensure that queue is drained slowly enough for the buffers >> to run out. > > Using the following patch: > > diff --git a/drivers/net/ethernet/aurora/nb8800.c > b/drivers/net/ethernet/aurora/nb8800.c > index 31c3f0f10fbb..646300bb53b6 100644 > --- a/drivers/net/ethernet/aurora/nb8800.c > +++ b/drivers/net/ethernet/aurora/nb8800.c > @@ -317,6 +317,7 @@ static int nb8800_poll(struct napi_struct *napi, int > budget) > > len = RX_BYTES_TRANSFERRED(rxd->report); > > + udelay(200); > if (IS_RX_ERROR(rxd->report)) > nb8800_rx_error(dev, rxd->report); > else > @@ -1416,9 +1417,9 @@ static int nb8800_probe(struct platform_device *pdev) > netdev_info(dev, "MAC address %pM\n", dev->dev_addr); > > /* Auto-negotiate by default */ > - priv->pause_aneg = true; > - priv->pause_rx = true; > - priv->pause_tx = true; > + priv->pause_aneg = false; > + priv->pause_rx = false; > + priv->pause_tx = false; > > priv->ops = match->data; > priv->ops->power_down(dev); > diff --git a/drivers/net/ethernet/aurora/nb8800.h > b/drivers/net/ethernet/aurora/nb8800.h > index 23fefca54804..9b59ea776e4a 100644 > --- a/drivers/net/ethernet/aurora/nb8800.h > +++ b/drivers/net/ethernet/aurora/nb8800.h > @@ -7,7 +7,7 @@ > #include > #include > > -#define RX_DESC_COUNT 256 > +#define RX_DESC_COUNT 16 > #define TX_DESC_COUNT 256 > > #define NB8800_DESC_LOW4 > > I saw both tango4 and tango5 wedged... :-( Well, that's not good. I'll see if I can replicate it later this week. -- Måns Rullgård
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
On 14/11/2017 17:55, Måns Rullgård wrote: > Marc Gonzalez wrote: > >> I will run iperf3 tests with RX_DESC_COUNT lowered to 2. >> Would that produce conclusive results? >> Do you have other suggestions? > > Leave RX_DESC_COUNT alone but add a delay in the nb8800_poll() loop. > That should ensure that queue is drained slowly enough for the buffers > to run out. Using the following patch: diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c index 31c3f0f10fbb..646300bb53b6 100644 --- a/drivers/net/ethernet/aurora/nb8800.c +++ b/drivers/net/ethernet/aurora/nb8800.c @@ -317,6 +317,7 @@ static int nb8800_poll(struct napi_struct *napi, int budget) len = RX_BYTES_TRANSFERRED(rxd->report); + udelay(200); if (IS_RX_ERROR(rxd->report)) nb8800_rx_error(dev, rxd->report); else @@ -1416,9 +1417,9 @@ static int nb8800_probe(struct platform_device *pdev) netdev_info(dev, "MAC address %pM\n", dev->dev_addr); /* Auto-negotiate by default */ - priv->pause_aneg = true; - priv->pause_rx = true; - priv->pause_tx = true; + priv->pause_aneg = false; + priv->pause_rx = false; + priv->pause_tx = false; priv->ops = match->data; priv->ops->power_down(dev); diff --git a/drivers/net/ethernet/aurora/nb8800.h b/drivers/net/ethernet/aurora/nb8800.h index 23fefca54804..9b59ea776e4a 100644 --- a/drivers/net/ethernet/aurora/nb8800.h +++ b/drivers/net/ethernet/aurora/nb8800.h @@ -7,7 +7,7 @@ #include #include -#define RX_DESC_COUNT 256 +#define RX_DESC_COUNT 16 #define TX_DESC_COUNT 256 #define NB8800_DESC_LOW4 I saw both tango4 and tango5 wedged... :-( tango5: [ 5] local 172.27.64.23 port 5201 connected to 172.27.64.1 port 57415 [ ID] Interval Transfer Bandwidth JitterLost/Total Datagrams [ 5] 0.00-1.00 sec 6.33 MBytes 53.1 Mbits/sec 0.030 ms 73897/78442 (94%) [ 5] 1.00-2.00 sec 6.59 MBytes 55.3 Mbits/sec 0.040 ms 76944/81675 (94%) [ 5] 2.00-3.00 sec 6.59 MBytes 55.3 Mbits/sec 0.036 ms 76944/81675 (94%) [ 5] 3.00-4.00 sec 6.59 MBytes 55.3 Mbits/sec 0.038 ms 76976/81708 (94%) [ 5] 4.00-5.00 sec 6.59 MBytes 55.3 Mbits/sec 0.038 ms 76976/81708 (94%) [ 5] 5.00-6.00 sec 6.59 MBytes 55.3 Mbits/sec 0.039 ms 76944/81675 (94%) [ 5] 6.00-7.00 sec 6.59 MBytes 55.3 Mbits/sec 0.042 ms 76941/81674 (94%) [ 5] 7.00-8.00 sec 6.59 MBytes 55.3 Mbits/sec 0.048 ms 76976/81707 (94%) [ 5] 8.00-9.00 sec 6.59 MBytes 55.3 Mbits/sec 0.041 ms 76928/81660 (94%) [ 5] 9.00-10.00 sec 6.59 MBytes 55.3 Mbits/sec 0.039 ms 76960/81692 (94%) [ 5] 10.00-10.04 sec 279 KBytes 55.1 Mbits/sec 0.094 ms 3088/3284 (94%) - - - - - - - - - - - - - - - - - - - - - - - - - [ ID] Interval Transfer Bandwidth JitterLost/Total Datagrams [ 5] 0.00-10.04 sec 0.00 Bytes 0.00 bits/sec 0.094 ms 769574/816900 (94%) ^Ciperf3: interrupt - the server has terminated # ping -c 10 172.27.64.1 PING 172.27.64.1 (172.27.64.1): 56 data bytes --- 172.27.64.1 ping statistics --- 10 packets transmitted, 0 packets received, 100% packet loss tango4: [ 5] local 172.27.64.23 port 5201 connected to 172.27.64.1 port 52983 [ ID] Interval Transfer Bandwidth JitterLost/Total Datagrams [ 5] 0.00-1.00 sec 6.24 MBytes 52.3 Mbits/sec 0.035 ms 74019/78498 (94%) [ 5] 1.00-2.00 sec 6.51 MBytes 54.6 Mbits/sec 0.046 ms 77024/81702 (94%) [ 5] 2.00-3.00 sec 6.51 MBytes 54.7 Mbits/sec 0.050 ms 77024/81703 (94%) [ 5] 3.00-4.00 sec 6.51 MBytes 54.7 Mbits/sec 0.050 ms 77024/81703 (94%) [ 5] 4.00-5.00 sec 6.52 MBytes 54.7 Mbits/sec 0.052 ms 77024/81705 (94%) [ 5] 5.00-6.00 sec 6.52 MBytes 54.7 Mbits/sec 0.037 ms 76960/81640 (94%) [ 5] 6.00-7.00 sec 6.52 MBytes 54.7 Mbits/sec 0.043 ms 77008/81689 (94%) [ 5] 7.00-8.00 sec 6.52 MBytes 54.7 Mbits/sec 0.050 ms 77024/81704 (94%) [ 5] 8.00-9.00 sec 6.52 MBytes 54.7 Mbits/sec 0.045 ms 77024/81705 (94%) [ 5] 9.00-10.00 sec 6.52 MBytes 54.7 Mbits/sec 0.043 ms 77008/81688 (94%) [ 5] 10.00-10.04 sec 275 KBytes 54.5 Mbits/sec 0.103 ms 3040/3233 (94%) - - - - - - - - - - - - - - - - - - - - - - - - - [ ID] Interval Transfer Bandwidth JitterLost/Total Datagrams [ 5] 0.00-10.04 sec 0.00 Bytes 0.00 bits/sec 0.103 ms 770179/816970 (94%) ^Ciperf3: interrupt - the server has terminated # ping -c 10 172.27.64.1 PING 172.27.64.1 (172.27.64.1): 56 data bytes --- 172.27.64.1 ping statistics --- 10 packets transmitted, 0 packets received, 100% packet loss More tests needed...
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
On 14/11/2017 17:55, Måns Rullgård wrote: > Marc Gonzalez wrote: > >> On 14/11/2017 14:54, Måns Rullgård wrote: >> >>> The hack originated from your company. >> >> So why are you so insistent that we keep using it? > > Because it's the only way to support some chip variants. Ones you'd > apparently rather forget, but which nonetheless exist. Which chip variants do you have in mind? All tango3 board supports the reset method. BTW, could you test my patch series on your board? (I can't since the board is not supported upstream.) >>> Also, I have repeated asked you what happens if the tango5 runs out of >>> DMA buffers under normal operation. Does that also cause it to lock up? >>> If so, you have a much bigger problem on your hands. >> >> I will run iperf3 tests with RX_DESC_COUNT lowered to 2. >> Would that produce conclusive results? >> Do you have other suggestions? > > Leave RX_DESC_COUNT alone but add a delay in the nb8800_poll() loop. > That should ensure that queue is drained slowly enough for the buffers > to run out. OK. I will test this ASAP on tango4 and tango5. I'll try finding a tango3 board, to check the nb8800_mdio_cmd() quirk and the flow_control quirk. Again, the HW dev said they are not needed, so if he's wrong, his credibility his shot.
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Marc Gonzalez writes: > On 14/11/2017 14:54, Måns Rullgård wrote: > >> Marc Gonzalez writes: >> >>> On 14/11/2017 13:40, Måns Rullgård wrote: >>> Marc Gonzalez wrote: > Power entire ethernet block down in ndo_stop(). > Power it back up in ndo_open() and perform HW init. > Delete nb8800_dma_stop. Leave it alone, please. Not all chips might have a separate power domain for this. Also, it works just fine on the older chips. >>> >>> There is no need for separate power domains. The ethernet block is >>> clock-gated when it is held in reset. >> >> So you're not powering it down then. Please be accurate. > > Smirk. That looks like trolling. > >>> The reset register is implemented on all tango3, tango4, tango5 chips. >> >> It's still not a core feature. > > Correct. But it covers 100% of all chips using this driver. > There is no point in trying to implement support for chips that > have never existed, do not exist, and never will. You can't know that. >>> nb8800_dma_stop() is a hack. >> >> The hack originated from your company. > > So why are you so insistent that we keep using it? Because it's the only way to support some chip variants. Ones you'd apparently rather forget, but which nonetheless exist. >> Also, I have repeated asked you what happens if the tango5 runs out of >> DMA buffers under normal operation. Does that also cause it to lock up? >> If so, you have a much bigger problem on your hands. > > I will run iperf3 tests with RX_DESC_COUNT lowered to 2. > Would that produce conclusive results? > Do you have other suggestions? Leave RX_DESC_COUNT alone but add a delay in the nb8800_poll() loop. That should ensure that queue is drained slowly enough for the buffers to run out. -- Måns Rullgård
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
On 14/11/2017 14:54, Måns Rullgård wrote: > Marc Gonzalez writes: > >> On 14/11/2017 13:40, Måns Rullgård wrote: >> >>> Marc Gonzalez wrote: >>> Power entire ethernet block down in ndo_stop(). Power it back up in ndo_open() and perform HW init. Delete nb8800_dma_stop. >>> >>> Leave it alone, please. Not all chips might have a separate power >>> domain for this. Also, it works just fine on the older chips. >> >> There is no need for separate power domains. The ethernet block is >> clock-gated when it is held in reset. > > So you're not powering it down then. Please be accurate. Smirk. That looks like trolling. >> The reset register is implemented on all tango3, tango4, tango5 chips. > > It's still not a core feature. Correct. But it covers 100% of all chips using this driver. There is no point in trying to implement support for chips that have never existed, do not exist, and never will. >> nb8800_dma_stop() is a hack. > > The hack originated from your company. So why are you so insistent that we keep using it? >> The HW dev has stated that it is not supported. One cannot conclude >> that it "works fine" just because you've never triggered the error >> condition. (On tango5, the error condition triggers systematically.) > > That sounds like a problem for tango5. tango5 does have its share of issues. > Also, I have repeated asked you what happens if the tango5 runs out of > DMA buffers under normal operation. Does that also cause it to lock up? > If so, you have a much bigger problem on your hands. I will run iperf3 tests with RX_DESC_COUNT lowered to 2. Would that produce conclusive results? Do you have other suggestions? >> We have several customer bug reports on tango3 and tango4 chips complaining >> about "broken" ethernet after a link down / link up cycle. They are using a >> different driver, but it implements the same hack in enet_stop_rx(). >> There is a high probability that the DMA hack is responsible, and wedged the >> RX DMA state machine. > > But you have no idea what's really the problem? I have an idea that enet_stop_rx() wedged the RX DMA state machine.
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Marc Gonzalez writes: > On 14/11/2017 13:40, Måns Rullgård wrote: > >> Marc Gonzalez wrote: >> >>> Power entire ethernet block down in ndo_stop(). >>> Power it back up in ndo_open() and perform HW init. >>> Delete nb8800_dma_stop. >> >> Leave it alone, please. Not all chips might have a separate power >> domain for this. Also, it works just fine on the older chips. > > There is no need for separate power domains. The ethernet block is > clock-gated when it is held in reset. So you're not powering it down then. Please be accurate. > The reset register is implemented on all tango3, tango4, tango5 chips. It's still not a core feature. > nb8800_dma_stop() is a hack. The hack originated from your company. > The HW dev has stated that it is not supported. One cannot conclude > that it "works fine" just because you've never triggered the error > condition. (On tango5, the error condition triggers systematically.) That sounds like a problem for tango5. Also, I have repeated asked you what happens if the tango5 runs out of DMA buffers under normal operation. Does that also cause it to lock up? If so, you have a much bigger problem on your hands. > We have several customer bug reports on tango3 and tango4 chips complaining > about "broken" ethernet after a link down / link up cycle. They are using a > different driver, but it implements the same hack in enet_stop_rx(). > There is a high probability that the DMA hack is responsible, and wedged the > RX DMA state machine. But you have no idea what's really the problem? -- Måns Rullgård
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
On 14/11/2017 13:40, Måns Rullgård wrote: > Marc Gonzalez wrote: > >> Power entire ethernet block down in ndo_stop(). >> Power it back up in ndo_open() and perform HW init. >> Delete nb8800_dma_stop. > > Leave it alone, please. Not all chips might have a separate power > domain for this. Also, it works just fine on the older chips. There is no need for separate power domains. The ethernet block is clock-gated when it is held in reset. The reset register is implemented on all tango3, tango4, tango5 chips. nb8800_dma_stop() is a hack. The HW dev has stated that it is not supported. One cannot conclude that it "works fine" just because you've never triggered the error condition. (On tango5, the error condition triggers systematically.) We have several customer bug reports on tango3 and tango4 chips complaining about "broken" ethernet after a link down / link up cycle. They are using a different driver, but it implements the same hack in enet_stop_rx(). There is a high probability that the DMA hack is responsible, and wedged the RX DMA state machine. Since older chips do support the reset register, this patch implements the same method for all tango chips.
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Marc Gonzalez writes:
> Power entire ethernet block down in ndo_stop().
> Power it back up in ndo_open() and perform HW init.
> Delete nb8800_dma_stop.
Leave it alone, please. Not all chips might have a separate power
domain for this. Also, it works just fine on the older chips.
> Signed-off-by: Marc Gonzalez
> ---
> drivers/net/ethernet/aurora/nb8800.c | 146
> +--
> drivers/net/ethernet/aurora/nb8800.h | 4 +-
> 2 files changed, 40 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 09b8001e1ecc..b71d8fb80610 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -40,7 +40,7 @@
> #include "nb8800.h"
>
> static void nb8800_tx_done(struct net_device *dev);
> -static int nb8800_dma_stop(struct net_device *dev);
> +static void nb8800_hw_init(struct net_device *dev);
>
> static inline u8 nb8800_readb(struct nb8800_priv *priv, int reg)
> {
> @@ -862,61 +862,6 @@ static int nb8800_dma_init(struct net_device *dev)
> return -ENOMEM;
> }
>
> -static int nb8800_dma_stop(struct net_device *dev)
> -{
> - struct nb8800_priv *priv = netdev_priv(dev);
> - struct nb8800_tx_buf *txb = &priv->tx_bufs[0];
> - struct nb8800_tx_desc *txd = &priv->tx_descs[0];
> - int retry = 5;
> - u32 txcr;
> - u32 rxcr;
> - int err;
> - unsigned int i;
> -
> - /* wait for tx to finish */
> - err = readl_poll_timeout_atomic(priv->base + NB8800_TXC_CR, txcr,
> - !(txcr & TCR_EN) &&
> - priv->tx_done == priv->tx_next,
> - 1000, 100);
> - if (err)
> - return err;
> -
> - /* The rx DMA only stops if it reaches the end of chain.
> - * To make this happen, we set the EOC flag on all rx
> - * descriptors, put the device in loopback mode, and send
> - * a few dummy frames. The interrupt handler will ignore
> - * these since NAPI is disabled and no real frames are in
> - * the tx queue.
> - */
> -
> - for (i = 0; i < RX_DESC_COUNT; i++)
> - priv->rx_descs[i].desc.config |= DESC_EOC;
> -
> - txd->desc[0].s_addr =
> - txb->dma_desc + offsetof(struct nb8800_tx_desc, buf);
> - txd->desc[0].config = DESC_BTS(2) | DESC_DS | DESC_EOF | DESC_EOC | 8;
> - memset(txd->buf, 0, sizeof(txd->buf));
> -
> - nb8800_mac_af(dev, false);
> - nb8800_setb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
> -
> - do {
> - nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> - wmb();
> - nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
> -
> - err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
> - rxcr, !(rxcr & RCR_EN),
> - 1000, 10);
> - } while (err && --retry);
> -
> - nb8800_mac_af(dev, true);
> - nb8800_clearb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
> - nb8800_dma_reset(dev);
> -
> - return retry ? 0 : -ETIMEDOUT;
> -}
> -
> static void nb8800_pause_adv(struct net_device *dev)
> {
> struct nb8800_priv *priv = netdev_priv(dev);
> @@ -941,6 +886,11 @@ static int nb8800_open(struct net_device *dev)
> struct phy_device *phydev;
> int err;
>
> + priv->ops->power_up(dev);
> + nb8800_hw_init(dev);
> + priv->ops->init(dev);
> + nb8800_update_mac_addr(dev);
> +
> /* clear any pending interrupts */
> nb8800_writel(priv, NB8800_RXC_SR, 0xf);
> nb8800_writel(priv, NB8800_TXC_SR, 0xf);
> @@ -993,12 +943,11 @@ static int nb8800_stop(struct net_device *dev)
> netif_stop_queue(dev);
> napi_disable(&priv->napi);
>
> - nb8800_dma_stop(dev);
> - nb8800_mac_rx(dev, false);
> - nb8800_mac_tx(dev, false);
> -
> phy_disconnect(phydev);
>
> + priv->ops->power_down(dev);
> + priv->speed = 0;
> +
> free_irq(dev->irq, dev);
>
> nb8800_dma_free(dev);
> @@ -1150,7 +1099,7 @@ static const struct ethtool_ops nb8800_ethtool_ops = {
> .set_link_ksettings = phy_ethtool_set_link_ksettings,
> };
>
> -static int nb8800_hw_init(struct net_device *dev)
> +static void nb8800_hw_init(struct net_device *dev)
> {
> struct nb8800_priv *priv = netdev_priv(dev);
> u32 val;
> @@ -1230,20 +1179,14 @@ static int nb8800_hw_init(struct net_device *dev)
> nb8800_writeb(priv, NB8800_PQ1, val >> 8);
> nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
>
> - /* Auto-negotiate by default */
> - priv->pause_aneg = true;
> - priv->pause_rx = true;
> - priv->pause_tx = true;
> -
> nb8800_mc_init(dev, 0);
> -
> - return 0;
> }
>
> static int nb8800_tangox_init(struct net_device *dev)
> {
> struct nb8800_priv *priv = netdev_priv(dev);
> u32 pad_mode = PAD_MODE_MII;
[PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Power entire ethernet block down in ndo_stop().
Power it back up in ndo_open() and perform HW init.
Delete nb8800_dma_stop.
Signed-off-by: Marc Gonzalez
---
drivers/net/ethernet/aurora/nb8800.c | 146 +--
drivers/net/ethernet/aurora/nb8800.h | 4 +-
2 files changed, 40 insertions(+), 110 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c
b/drivers/net/ethernet/aurora/nb8800.c
index 09b8001e1ecc..b71d8fb80610 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -40,7 +40,7 @@
#include "nb8800.h"
static void nb8800_tx_done(struct net_device *dev);
-static int nb8800_dma_stop(struct net_device *dev);
+static void nb8800_hw_init(struct net_device *dev);
static inline u8 nb8800_readb(struct nb8800_priv *priv, int reg)
{
@@ -862,61 +862,6 @@ static int nb8800_dma_init(struct net_device *dev)
return -ENOMEM;
}
-static int nb8800_dma_stop(struct net_device *dev)
-{
- struct nb8800_priv *priv = netdev_priv(dev);
- struct nb8800_tx_buf *txb = &priv->tx_bufs[0];
- struct nb8800_tx_desc *txd = &priv->tx_descs[0];
- int retry = 5;
- u32 txcr;
- u32 rxcr;
- int err;
- unsigned int i;
-
- /* wait for tx to finish */
- err = readl_poll_timeout_atomic(priv->base + NB8800_TXC_CR, txcr,
- !(txcr & TCR_EN) &&
- priv->tx_done == priv->tx_next,
- 1000, 100);
- if (err)
- return err;
-
- /* The rx DMA only stops if it reaches the end of chain.
-* To make this happen, we set the EOC flag on all rx
-* descriptors, put the device in loopback mode, and send
-* a few dummy frames. The interrupt handler will ignore
-* these since NAPI is disabled and no real frames are in
-* the tx queue.
-*/
-
- for (i = 0; i < RX_DESC_COUNT; i++)
- priv->rx_descs[i].desc.config |= DESC_EOC;
-
- txd->desc[0].s_addr =
- txb->dma_desc + offsetof(struct nb8800_tx_desc, buf);
- txd->desc[0].config = DESC_BTS(2) | DESC_DS | DESC_EOF | DESC_EOC | 8;
- memset(txd->buf, 0, sizeof(txd->buf));
-
- nb8800_mac_af(dev, false);
- nb8800_setb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
-
- do {
- nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
- wmb();
- nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
-
- err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
- rxcr, !(rxcr & RCR_EN),
- 1000, 10);
- } while (err && --retry);
-
- nb8800_mac_af(dev, true);
- nb8800_clearb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
- nb8800_dma_reset(dev);
-
- return retry ? 0 : -ETIMEDOUT;
-}
-
static void nb8800_pause_adv(struct net_device *dev)
{
struct nb8800_priv *priv = netdev_priv(dev);
@@ -941,6 +886,11 @@ static int nb8800_open(struct net_device *dev)
struct phy_device *phydev;
int err;
+ priv->ops->power_up(dev);
+ nb8800_hw_init(dev);
+ priv->ops->init(dev);
+ nb8800_update_mac_addr(dev);
+
/* clear any pending interrupts */
nb8800_writel(priv, NB8800_RXC_SR, 0xf);
nb8800_writel(priv, NB8800_TXC_SR, 0xf);
@@ -993,12 +943,11 @@ static int nb8800_stop(struct net_device *dev)
netif_stop_queue(dev);
napi_disable(&priv->napi);
- nb8800_dma_stop(dev);
- nb8800_mac_rx(dev, false);
- nb8800_mac_tx(dev, false);
-
phy_disconnect(phydev);
+ priv->ops->power_down(dev);
+ priv->speed = 0;
+
free_irq(dev->irq, dev);
nb8800_dma_free(dev);
@@ -1150,7 +1099,7 @@ static const struct ethtool_ops nb8800_ethtool_ops = {
.set_link_ksettings = phy_ethtool_set_link_ksettings,
};
-static int nb8800_hw_init(struct net_device *dev)
+static void nb8800_hw_init(struct net_device *dev)
{
struct nb8800_priv *priv = netdev_priv(dev);
u32 val;
@@ -1230,20 +1179,14 @@ static int nb8800_hw_init(struct net_device *dev)
nb8800_writeb(priv, NB8800_PQ1, val >> 8);
nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
- /* Auto-negotiate by default */
- priv->pause_aneg = true;
- priv->pause_rx = true;
- priv->pause_tx = true;
-
nb8800_mc_init(dev, 0);
-
- return 0;
}
static int nb8800_tangox_init(struct net_device *dev)
{
struct nb8800_priv *priv = netdev_priv(dev);
u32 pad_mode = PAD_MODE_MII;
+ int clk_div;
switch (priv->phy_mode) {
case PHY_INTERFACE_MODE_MII:
@@ -1266,29 +1209,26 @@ static int nb8800_tangox_init(struct net_device *dev)
nb8800_writeb(priv, NB8800_TANGOX_PAD_MODE, pad_mode);
+ clk_div = DIV_ROUN
