Ira W. Snyder wrote: > On Tue, Feb 23, 2010 at 08:56:09PM +0100, Wolfgang Grandegger wrote: >> Ira W. Snyder wrote: >>> On Mon, Feb 22, 2010 at 08:42:56PM +0100, Wolfgang Grandegger wrote: >>>> Ira W. Snyder wrote: >>>>> This patch series adds support for the Janz CMOD-IO carrier board, as well >>>>> as the Janz VMOD-ICAN3 Intelligent CAN controller. The CMOD-IO carrier >>>>> board is a PCI to MODULbus bridge, into which plug MODULbus >>>>> daughterboards. >>>>> I only have access to two types of daughtercards, the VMOD-ICAN3 mentioned >>>>> above, and the VMOD-TTL GPIO controller. >>>>> >>>>> All of my boards have two VMOD-ICAN3 modules and one VMOD-TTL module. This >>>>> posting only contains drivers for the CMOD-IO carrier board and VMOD-ICAN3 >>>>> CAN interfaces. A driver for the VMOD-TTL GPIO module is under >>>>> development, >>>>> and will be posted shortly. This module is even worse to program nicely >>>>> than the ICAN3 module. >>>>> >>>>> Since the RFCv2 posting, the CAN driver has been much more thoroughly >>>>> tested. CAN bus-off works correctly, as does the generation of error >>>>> frames. The bus-off and error frame code has been adapted from the SJA1000 >>>>> driver, as the ICAN3 firmware reports most of the status registers used by >>>>> the SJA1000 code. >>>> Sounds good and from my point of view the driver is more or less ready >>>> for mainline inclusion. If that is your primary goal and you feel it is >>>> mature and stable enough, please send a proper patch series as described >>>> here: >>>> >>>> http://svn.berlios.de/svnroot/repos/socketcan/trunk/README.submitting-patches. >>>> >>>> As an alternative, I could apply it to the SVN trunk for the time being. >>>> There, the requirements for acceptance are not that high. >>>> >>>> I briefly browsed the patches. Here some quick comments: >>>> >>>> - I do still not find __devinit, __devexit, and friends in your drivers >>>> as described here: >>>> >>>> http://lxr.linux.no/#linux+v2.6.32/Documentation/PCI/pci.txt#L177 >>>> >>>> They are also missing in janz-ican3.c. >>>> >>>> - You may need to declare some structures "__attribute__((packed))", >>>> >>>> - Don't include sja1000/sja1000.h. It's only for drivers in sja1000. >>>> I know that some other drivers use SJA1000 definitions as well, but >>>> that requires a general solution. >>>> >>> Why not? I need some of the definitions for the SJA1000 error registers. >>> Is there any reason why it can't be include/linux/can/sja1000.h instead? >> Yes, it does also contain private declarations and definitions which >> should not be used outside sja1000. >> >>> It seems stupid to duplicate the register definitions in each new driver >>> that comes along. >> Yes. As I said above, this needs a general solution splitting sja1000.h >> in public register definitions and private stuff for the sja1000 drivers. >> >>>> - Some time ago we agreed to use "_" for the Socket-CAN file names: >>>> s/janz-ican3/janz_ican3/ >>>> >>>> - You still use many hard-code numbers in the code. Please define >>>> values for most of them to make the code more readable. >>>> >>> I missed a few of these in the version I sent. They'll be fixed for the >>> next version. >>> >>>> - There are still to much dev_dbg(). They should especially not be used >>>> in the xmit and recv path. >>>> >>>> - I see still a lot of duplicated code, especially for desc handling. >>>> Maybe some helper functions or combined i/o functions for send/recv >>>> could make the code more compact. >>>> >>>> - Checkpatch reports "lines too long". >>>> >>>> - s+<linux/janz.h>+<linux/mfd/janz.h>+ ? >>>> >>> Ok. >>> >>>> - Check MODULE_LICENSE(). It does not match with your copyright notes. >>>> >>> It will be changed to "GPL v2". I didn't know there was a difference >>> between "GPL" and "GPL v2" until I hunted down include/linux/module.h's >>> comments. I don't mind GPL v2 or later licensing, but I thought the >>> Linux kernel was GPL v2 only. I guess not. >> No, I think most code is under GPL v2 and *later*. If you have no >> particular reason, I would use that license (instead of the restricted >> v2). But that's your choice, of course. >> > > I have no problem with GPLv2+, in fact, I'd rather do that. I was under > the impression that the Linux kernel was all GPLv2-only. I'll just > change the comment, I guess.
Yes, to the usual GPL v2 and later bla bla. >>>> - About xmit flow control. What happens if you send messages quickly at >>>> 125 KB/s. You could use "cangen -g 0 can0" for that test. How many >>>> messages get dropped? >>>> >>> I let the cangen command run for a while: >>> $ ifconfig -a >>> can0 Link encap:UNSPEC HWaddr >>> 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00 >>> UP RUNNING NOARP MTU:16 Metric:1 >>> RX packets:473455 errors:0 dropped:0 overruns:0 frame:0 >>> TX packets:473455 errors:0 dropped:1831983 overruns:0 carrier:0 >> As I expected, most packets get dropped because of missing xmit flow >> control. >> > > Yep. I'm sure that reading the buffer status bits causes additional load > on the PCI bus as well, slowing things down. Unfortunately, I have no > idea how to get around this with this hardware, other than some crazy > scheme of reading the status bits from descriptors that we've sent in > the past. But I still have the problem of when to call > netif_wake_queue(). If there is no way to get a TX done notification, we have to live with it. Anyway, a real CAN application will usually not send messages like hell. > If you're curious, appendix c in the datasheet is where to look for the > description of how the firmware communicates with a driver. OK. I think you can enable bus-error interrupts. No way to enable the TX done interrupt? Where can I find the manual you mentioned above. >>> collisions:0 txqueuelen:10 >>> RX bytes:2719863 (2.5 MiB) TX bytes:2719863 (2.5 MiB) >>> Interrupt:22 >>> When running cangen, the TX/RX rate is about 32KB/sec (258 kbit/sec) at >>> roughly 5800 packets/sec. Seems pretty low for the CAN devices >>> configured like this: >>> >>> 5: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN >>> qlen 10 >>> link/can >>> can state ERROR-ACTIVE restart-ms 0 >>> bitrate 1000000 sample-point 0.750 >>> tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1 >>> janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1 >>> clock 8000000 >>> 6: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN >>> qlen 10 >>> link/can >>> can state ERROR-ACTIVE restart-ms 0 >>> bitrate 1000000 sample-point 0.750 >>> tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1 >>> janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1 >>> clock 8000000 >>> >>> Any ideas on how I can go faster? The kernel appears to be spending ~63% >>> of its CPU time running cangen, and ~37% in softirq context, running >>> events/0 (the workqueue thread). >> cangen retries immediately if the send() returns ENOBUFS resulting in a >> high CPU load. It would be better to sleep some time or use poll/select. >> The softirq load is due to the fact that you are dropping packets at >> high rate and it's even worse at lower bitrates, I guess. Does it get >> better if you return with NETDEV_TX_BUSY (and do not free the packet). >> > > Nope, there isn't any noticeable change. The cangen's CPU usage did go > down a little bit, but the transmit rate didn't improve, nor did the CPU > usage of the workqueue. The CPU usage by cangen isn't specific to your hardware, as I explained. Anyway, using NETDEV_TX_BUSY is the proper solution as it does not drop *good* packets. > I'm curious: can you get to 125KB/sec with some of the other PCI cards > based on SJA1000 + a PLX bridge chip? I have to try. Nevertheless, the sustained rate will be less than 1 MB/s due to re-arbitration on the bus. For me, this is not a software problem. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
