On Wed, Feb 24, 2010 at 09:46:06AM +0100, Wolfgang Grandegger wrote: > 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. >
Yep, I have bus-error interrupts enabled, but I can't find a way to enable tx-done interrupts. Here is the product's webpage, followed by links to the hardware and firmware manuals. I've been referring to the software reference manual. http://www.janz.de/as/en/vmod-ican3.html http://www.janz.de/as/en/download-document/79-vmod-ican3-hardware-manual.html http://www.janz.de/as/en/download-document/12-ican-software-reference-manual.html You'll want to look through appendix c (interface implementation). This section describes how the communication interfaces with the firmware are laid out in memory, etc. You will also want to look through section 3 (services). This section describes the various messages I can send to the firmware to change its behavior. There *is* a way to transmit 11-bit ID CAN frames with a confirmation, but not 29-bit ID CAN frames, AFAICT. My reasoning for this comes from the description of the SetAFilMask message, which sets the filtering parameters (based on CAN ID) of the firmware. It states: You must not set mask value 1 or 3 for extended frame identifiers. The only other values (0 or 2) mean "reject" and "send to fast interface", respectively. This leads me to believe that I can only send/recv 29-bit CAN ID's through the fast interface. > >>> 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. > I was just curious if my implementation was "fast enough" or if I need to optimize it further. I'm worried that I'll start missing lots of RX messages on a heavily loaded CAN bus. Thanks, Ira _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
