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. > >> - 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 you're curious, appendix c in the datasheet is where to look for the description of how the firmware communicates with a driver. > > 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. I'm curious: can you get to 125KB/sec with some of the other PCI cards based on SJA1000 + a PLX bridge chip? Ira _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
