On 21.2.2017 17:14, Marc Kleine-Budde wrote: > On 02/21/2017 03:57 PM, Anssi Hannula wrote: >> On 20.2.2017 12:27, Anssi Hannula wrote: >>> On 20.2.2017 11:42, Marc Kleine-Budde wrote: >>>> On 02/13/2017 04:39 PM, Anssi Hannula wrote: >>>>> The xilinx_can driver assumes that the TXOK interrupt only clears after >>>>> it has been acknowledged as many times as there have been successfully >>>>> sent frames. >>>>> >>>>> However, the documentation does not mention such behavior, instead >>>>> saying just that the interrupt is cleared when the clear bit is set. >>>>> >>>>> Similarly, testing seems to also suggest that it is immediately cleared >>>>> regardless of the amount of frames having been sent. Performing some >>>>> heavy TX load and then going back to idle has the tx_head drifting >>>>> further away from tx_tail over time, steadily reducing the amount of >>>>> frames the driver keeps in the TX FIFO (but not to zero, as the TXOK >>>>> interrupt always frees up space for 1 frame from the driver's >>>>> perspective, so frames continue to be sent) and delaying the local echo >>>>> frames. >>>>> >>>>> The TX FIFO tracking is also otherwise buggy as it does not account for >>>>> TX FIFO being cleared after software resets, causing >>>>> BUG!, TX FIFO full when queue awake! >>>>> messages to be output. >>>>> >>>>> Since there does not seem to be any way to accurately track the state of >>>>> the TX FIFO, drop the code trying to do that and perform >>>>> netif_stop_queue() based on whether the HW reports the FIFO being full. >>>>> Driver-side local echo support is also removed as it is not possible to >>>>> do accurately (relying instead on the CAN core fallback support). >>>>> >>>>> Tested with the integrated CAN on Zynq-7000 SoC. >>>>> >>>>> v2: Add FIFO space check before TX queue wake with locking to >>>>> synchronize with queue stop. This avoids waking the queue when xmit() >>>>> had just filled it. >>>> Removing the echo on TX-complete should be avoided if possible. Is there >>>> a way to get a indication that an individual package has been send? >>> Looking at the reference manual [1, chapter 18 and appendix B.5] I can't >>> see any. The TX FIFO interrupts and status registers available are TXOK >>> (1 or more messages sent), FIFO empty, FIFO full, FIFO under watermark >>> (watermark not programmable while on-bus). There is no FIFO free space >>> counter that I can see. >>> >>> One way would be to only put a single frame into the FIFO at a time, of >>> course, but at least to me that seems worse than losing echo-on-TX-complete. >> A correction to my last statement above, we could probably put up to 3 >> frames in TX FIFO at a time and still keep track of them using the TX >> FIFO watermark, allowing echo-on-TX-complete to still be supported, with >> something like this: >> if (tx fifo empty) >> all frames sent; >> else if (tx fifo under watermark 2) >> all except 1 frame sent; >> else if (txok) >> 1 frame sent; (all except 2) >> >> Though 3 is still much less than 64, the current TX FIFO max size. > Sounds good, can you work on a patch? Unfortunately I've just noticed that the soft-IP version of the controller (compatible-string "xlnx,axi-can-1.00.a" instead of "xlnx,zynq-can-1.0" found on Zynq SoCs) does not have the TX FIFO empty bit nor the watermark programming feature: https://www.xilinx.com/support/documentation/ip_documentation/can/v5_0/pg096-can.pdf So the above idea for tracking more than 1 frame in FIFO would not work on those. What do you think should be done here? >> I'd myself prefer keeping full FIFO over keeping echo-on-TX-complete, >> but not sure what is the general opinion. > Big FIFOs have the drawback of bigger latencies, as you cannot reorder > high priority packages inside the kernel. > > Marc > -- Anssi Hannula / Bitwise Oy +358503803997