[PATCH v2 0/7] can: CAN network device driver interface and drivers

[PATCH v2 0/7] can: CAN network device driver interface and drivers

[Wolfgang Grandegger]

Hello,

here comes v2 of the patch series posted some time ago. See

  http://marc.info/?l=linux-netdev&m=123507025810612&w=4.

Changes since v1:

- The SysFS interface has been replaced by the Netlink interface as
  suggested by Patrick. For further information have a look to
  chapter 6.5 of "Documentation/networking/can.txt" and the header
  file "include/linux/can/netdev.h". The patch for the "ip" program of
  the iproute2 utility suite will be provided separately.

- The restart function is now properly protected by "dev->tx_lock"
  using the "netif_tx_[lock|unlock]" functions.

- For the  restart, the device driver must now handle CAN_MODE_STOP as
  well, apart from CAN_MODE_START to avoid race conditions.

- The "irq_lock" member of "struct can_priv" has been removed. The CAN
  controller driver may should provide its own synchronization if
  necessary.

- The restart function is now protected by "dev->tx_lock" using the
  "netif_tx_lock/unlock" functions.

- Cleanup timer usage for the restart function.

- The unused do_set/get_ctrlmode member of "struct can_priv" have been
  removed.

- "const" has been added to "struct net_device" for some functions not
  allowed to touch that structure.

- The library functions "can_set_bittiming" and "can_close_cleanup" have
  been renamed to the more general names "open_candev" and
  "close_candev", respectively.

- Use "del_timer_sync" instead of "del_timer" for the  re-start timer.

- The macro "ND2D()" has been replaced by "dev->dev.parent" as it does
  not make the code more readable.

- Fix improper BTR setting for triple-sampling for the SJA1000 as
  pointed out by Oliver.

- Dont use __u8 but __u32 as before for some members of "struct
  can_bittiming[_const]" to avoid alignment trouble.

- Definitions shared with user-space applications have been moved to
  "include/linux/can/netlink.h".

- Various other minor correction suggested on the netdev ML.

- The MSCAN driver for the MPC5200 has been removed. It needs to be
  presented on the Linuxppc-dev and Devicetree-discuss ML as well,
  which will be done by a sub-sequent patch.

- Add more source code documentation, especially for the structures
  and functions related to bit-timing.

The PF_CAN protocol family for the Controller Area Network is available
in the kernel since version 2.6.25 but drivers for real CAN devices are
still missing. This patch series adds a generic CAN network device
driver interface and, as a start, a few drivers. It is the result of the
on-going discussion and development of the Socket-CAN project hosted at
the BerliOS web-server (http://developer.berlios.de/projects/socketcan).
The patch series consists of the following patches:

 1/8) can: Documentation for the CAN device driver interface
 2/8) can: Update MAINTAINERS and CREDITS file
 3/8) can: CAN Network device driver and Netlink interface
 4/8) can: Driver for the SJA1000 CAN controller
 5/8) can: SJA1000 generic platform bus driver
 6/8) can: SJA1000 driver for EMS PCI cards
 7/8) can: SJA1000 driver for Kvaser PCI cards

Please consider these patches for inclusion.

Thanks,

Wolfgang.

[PATCH v2 1/7] [PATCH 1/8] can: Documentation for the CAN device driver interface

[Wolfgang Grandegger]

[-- Attachment #1: 01-driver-documentation.patch --]
[-- Type: text/plain, Size: 12679 bytes --]

This patch documents the CAN netowrk device drivers interface, removes
obsolete documentation and adds some useful links to CAN resources.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
---
 Documentation/networking/can.txt |  239 ++++++++++++++++++++++++++++++++-------
 1 file changed, 199 insertions(+), 40 deletions(-)

Index: net-next-2.6/Documentation/networking/can.txt
===================================================================
--- net-next-2.6.orig/Documentation/networking/can.txt	2009-05-12 10:47:10.546720893 +0200
+++ net-next-2.6/Documentation/networking/can.txt	2009-05-12 10:47:23.470719178 +0200
@@ -36,10 +36,15 @@
     6.2 local loopback of sent frames
     6.3 CAN controller hardware filters
     6.4 The virtual CAN driver (vcan)
-    6.5 currently supported CAN hardware
-    6.6 todo
+    6.5 The CAN network device driver interface
+      6.5.1 Netlink interface to set/get devices properties
+      6.5.2 Setting the CAN bit-timing
+      6.5.3 Starting and stopping the CAN network device
+    6.6 supported CAN hardware
 
-  7 Credits
+  7 Socket CAN resources
+
+  8 Credits
 
 ============================================================================
 
@@ -234,6 +239,8 @@
   the user application using the common CAN filter mechanisms. Inside
   this filter definition the (interested) type of errors may be
   selected. The reception of error frames is disabled by default.
+  The format of the CAN error frame is briefly decribed in the Linux
+  header file "include/linux/can/error.h".
 
 4. How to use Socket CAN
 ------------------------
@@ -605,61 +612,213 @@
   removal of vcan network devices can be managed with the ip(8) tool:
 
   - Create a virtual CAN network interface:
-       ip link add type vcan
+       $ ip link add type vcan
 
   - Create a virtual CAN network interface with a specific name 'vcan42':
-       ip link add dev vcan42 type vcan
+       $ ip link add dev vcan42 type vcan
 
   - Remove a (virtual CAN) network interface 'vcan42':
-       ip link del vcan42
-
-  The tool 'vcan' from the SocketCAN SVN repository on BerliOS is obsolete.
-
-  Virtual CAN network device creation in older Kernels:
-  In Linux Kernel versions < 2.6.24 the vcan driver creates 4 vcan
-  netdevices at module load time by default. This value can be changed
-  with the module parameter 'numdev'. E.g. 'modprobe vcan numdev=8'
-
-  6.5 currently supported CAN hardware
-
-  On the project website http://developer.berlios.de/projects/socketcan
-  there are different drivers available:
-
-    vcan:    Virtual CAN interface driver (if no real hardware is available)
-    sja1000: Philips SJA1000 CAN controller (recommended)
-    i82527:  Intel i82527 CAN controller
-    mscan:   Motorola/Freescale CAN controller (e.g. inside SOC MPC5200)
-    ccan:    CCAN controller core (e.g. inside SOC h7202)
-    slcan:   For a bunch of CAN adaptors that are attached via a
-             serial line ASCII protocol (for serial / USB adaptors)
+       $ ip link del vcan42
 
-  Additionally the different CAN adaptors (ISA/PCI/PCMCIA/USB/Parport)
-  from PEAK Systemtechnik support the CAN netdevice driver model
-  since Linux driver v6.0: http://www.peak-system.com/linux/index.htm
+  6.5 The CAN network device driver interface
 
-  Please check the Mailing Lists on the berlios OSS project website.
+  The CAN network device driver interface provides a generic interface
+  to setup, configure and monitor CAN network devices. The user can then
+  configure the CAN device, like setting the bit-timing parameters, via
+  the netlink interface using the program "ip" from the "IPROUTE2"
+  utility suite. The following chapter describes briefly how to use it.
+  Furthermore, the interface uses a common data structure and exports a
+  set of common functions, which all real CAN network device drivers
+  should use. Please have a look to the SJA1000 or MSCAN driver to
+  understand how to use them. The name of the module is can-dev.ko.
+
+  6.5.1 Netlink interface to set/get devices properties
+
+  The CAN device must be configured via netlink interface. The supported
+  netlink message types are defined and briefly described in
+  "include/linux/can/netlink.h". CAN link support for the program "ip"
+  of the IPROUTE2 utility suite is avaiable and it can be used as shown
+  below:
+
+  - Setting CAN device properties:
+
+    $ ip link set can0 type can help
+    Usage: ip link set DEVICE type can
+    	[ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
+    	[ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
+     	  phase-seg2 PHASE-SEG2 [ sjw SJW ] ]
+
+    	[ loopback { on | off } ]
+    	[ listen-only { on | off } ]
+    	[ triple-sampling { on | off } ]
+
+    	[ restart-ms TIME-MS ]
+    	[ restart ]
+
+    	Where: BITRATE       := { 1..1000000 }
+    	       SAMPLE-POINT  := { 0.000..0.999 }
+    	       TQ            := { NUMBER }
+    	       PROP-SEG      := { 1..8 }
+    	       PHASE-SEG1    := { 1..8 }
+    	       PHASE-SEG2    := { 1..8 }
+    	       SJW           := { 1..4 }
+    	       RESTART-MS    := { 0 | NUMBER }
+
+  - Display CAN device details and statistics:
+
+    $ ip -details -statistics link show can0
+    2: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UP qlen 10
+      link/can
+      can <TRIPLE-SAMPLING> state ERROR-ACTIVE restart-ms 100
+      bitrate 125000 sample_point 0.875
+      tq 125 prop-seg 6 phase-seg1 7 phase-seg2 2 sjw 1
+      sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
+      clock 8000000
+      re-started bus-errors arbit-lost error-warn error-pass bus-off
+      41         17457      0          41         42         41
+      RX: bytes  packets  errors  dropped overrun mcast
+      140859     17608    17457   0       0       0
+      TX: bytes  packets  errors  dropped carrier collsns
+      861        112      0       41      0       0
+
+  More info to the above output:
+
+    "<TRIPLE-SAMPLING>"
+	Shows the list of selected CAN controller modes: LOOPBACK,
+	LISTEN-ONLY, or TRIPLE-SAMPLING.
+
+    "state ERROR-ACTIVE"
+	The current state of the CAN controller: "ERROR-ACTIVE",
+	"ERROR-WARNING", "ERROR-PASSIVE", "BUS-OFF" or "STOPPED"
+
+    "restart-ms 100"
+	Automatic restart delay time. If set to a non-zero value, a
+	restart of the CAN controller will be triggered automatically
+	in case of a bus-off condition after the specified delay time
+	in milliseconds. By default it's off.
+
+    "bitrate 125000 sample_point 0.875"
+	Shows the real bit-rate in bits/sec and the sample-point in the
+	range 0.000..0.999. If the calculation of bit-timing parameters
+	is enabled in the kernel (CONFIG_CAN_CALC_BITTIMING=y), the
+	bit-timing can be defined by setting the "bitrate" argument.
+	Optionally the "sample-point" can be specified. By default it's
+	0.000 assuming CIA-recommended sample-points.
+
+    "tq 125 prop-seg 6 phase-seg1 7 phase-seg2 2 sjw 1"
+	Shows the time quanta in ns, propagation segment, phase buffer
+	segment 1 and 2 and the synchronisation jump width in units of
+	tq. They allow to define the CAN bit-timing in a hardware
+	independent format as proposed by the Bosch CAN 2.0 spec (see
+	chapter 8 of http://www.semiconductors.bosch.de/pdf/can2spec.pdf).
+
+    "sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
+     clock 8000000"
+	Shows the bit-timing constants of the CAN controller, here the
+	"sja1000". The minimum and maximum values of the time segment 1
+	and 2, the synchronisation jump width in units of tq, the
+	bitrate pre-scaler and the CAN system clock frequency in Hz.
+	These constants could be used for user-defined (non-standard)
+	bit-timing calculation algorithms in user-space.
+
+    "re-started bus-errors arbit-lost error-warn error-pass bus-off"
+	Shows the number of restarts, bus and arbitration lost errors,
+	and the state changes to the error-warning, error-passive and
+	bus-off state. RX overrun errors are listed in the "overrun"
+	field of the standard network statistics.
+
+  6.5.2 Setting the CAN bit-timing
+
+  The CAN bit-timing parameters can always be defined in a hardware
+  independent format as proposed in the Bosch CAN 2.0 specification
+  specifying the arguments "tq", "prop_seg", "phase_seg1", "phase_seg2"
+  and "sjw":
+
+    $ ip link set canX type can tq 125 prop-seg 6 \
+				phase-seg1 7 phase-seg2 2 sjw 1
+
+  If the kernel option CONFIG_CAN_CALC_BITTIMING is enabled, CIA
+  recommended CAN bit-timing parameters will be calculated if the bit-
+  rate is specified with the argument "bitrate":
+
+    $ ip link set canX type can bitrate 125000
+
+  Note that this works fine for the most common CAN controllers with
+  standard bit-rates but may *fail* for exotic bit-rates or CAN system
+  clock frequencies. Disabling CONFIG_CAN_CALC_BITTIMING saves some
+  space and allows user-space tools to solely determine and set the
+  bit-timing parameters. The CAN controller specific bit-timing
+  constants can be used for that purpose. They are listed by the
+  following command:
+
+    $ ip -details link show can0
+    ...
+      sja1000: clock 8000000 tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
+
+  6.5.3 Starting and stopping the CAN network device
+
+  A CAN network device is started or stopped as usual with the command
+  "ifconfig canX up/down" or "ip link set canX up/down". Be aware that
+  you *must* define proper bit-timing parameters for real CAN devices
+  before you can start it to avoid error-prone default settings:
+
+    $ ip link set canX up type can bitrate 125000
+
+  A device may enter the "bus-off" state if too much errors occurred on
+  the CAN bus. Then no more messages are received or sent. An automatic
+  bus-off recovery can be enabled by setting the "restart-ms" to a
+  non-zero value, e.g.:
+
+    $ ip link set canX type can restart-ms 100
+
+  Alternatively, the application may realize the "bus-off" condition
+  by monitoring CAN error frames and do a restart when appropriate with
+  the command:
+
+    $ ip link set canX type can restart
+
+  Note that a restart will also create a CAN error frame (see also
+  chapter 3.4).
+
+  6.6 Supported CAN hardware
+
+  Please check the "Kconfig" file in "drivers/net/can" to get an actual
+  list of the support CAN hardware. On the Socket CAN project website
+  (see chapter 7) there might be further drivers available, also for
+  older kernel versions.
+
+7. Socket CAN resources
+-----------------------
+
+  You can find further resources for Socket CAN like user space tools,
+  support for old kernel versions, more drivers, mailing lists, etc.
+  at the BerliOS OSS project website for Socket CAN:
 
-  6.6 todo
+    http://developer.berlios.de/projects/socketcan
 
-  The configuration interface for CAN network drivers is still an open
-  issue that has not been finalized in the socketcan project. Also the
-  idea of having a library module (candev.ko) that holds functions
-  that are needed by all CAN netdevices is not ready to ship.
-  Your contribution is welcome.
+  If you have questions, bug fixes, etc., don't hesitate to post them to
+  the Socketcan-Users mailing list. But please search the archives first.
 
-7. Credits
+8. Credits
 ----------
 
-  Oliver Hartkopp (PF_CAN core, filters, drivers, bcm)
+  Oliver Hartkopp (PF_CAN core, filters, drivers, bcm, SJA1000 driver)
   Urs Thuermann (PF_CAN core, kernel integration, socket interfaces, raw, vcan)
   Jan Kizka (RT-SocketCAN core, Socket-API reconciliation)
-  Wolfgang Grandegger (RT-SocketCAN core & drivers, Raw Socket-API reviews)
+  Wolfgang Grandegger (RT-SocketCAN core & drivers, Raw Socket-API reviews,
+                       CAN device driver interface, MSCAN driver)
   Robert Schwebel (design reviews, PTXdist integration)
   Marc Kleine-Budde (design reviews, Kernel 2.6 cleanups, drivers)
   Benedikt Spranger (reviews)
   Thomas Gleixner (LKML reviews, coding style, posting hints)
-  Andrey Volkov (kernel subtree structure, ioctls, mscan driver)
+  Andrey Volkov (kernel subtree structure, ioctls, MSCAN driver)
   Matthias Brukner (first SJA1000 CAN netdevice implementation Q2/2003)
   Klaus Hitschler (PEAK driver integration)
   Uwe Koppe (CAN netdevices with PF_PACKET approach)
   Michael Schulze (driver layer loopback requirement, RT CAN drivers review)
+  Pavel Pisa (Bit-timing calculation)
+  Sascha Hauer (SJA1000 platform driver)
+  Sebastian Haas (SJA1000 EMS PCI driver)
+  Markus Plessing (SJA1000 EMS PCI driver)
+  Per Dalen (SJA1000 Kvaser PCI driver)
+  Sam Ravnborg (reviews, coding style, kbuild help)

[PATCH v2 2/7] [PATCH 2/8] can: Update MAINTAINERS and CREDITS file

[Wolfgang Grandegger]

[-- Attachment #1: 02-driver-maintainers.patch --]
[-- Type: text/plain, Size: 1185 bytes --]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 CREDITS     |    4 ++++
 MAINTAINERS |    7 +++++++
 2 files changed, 11 insertions(+)

Index: net-next-2.6/CREDITS
===================================================================
--- net-next-2.6.orig/CREDITS	2009-05-12 10:47:10.501721461 +0200
+++ net-next-2.6/CREDITS	2009-05-12 10:47:24.371720546 +0200
@@ -1253,6 +1253,10 @@
 S: Sterling Heights, Michigan 48313
 S: USA
 
+N: Wolfgang Grandegger
+E: wg@grandegger.com
+D: Controller Area Network (device drivers)
+
 N: William Greathouse
 E: wgreathouse@smva.com
 E: wgreathouse@myfavoritei.com
Index: net-next-2.6/MAINTAINERS
===================================================================
--- net-next-2.6.orig/MAINTAINERS	2009-05-12 10:47:10.501721461 +0200
+++ net-next-2.6/MAINTAINERS	2009-05-12 10:47:24.374720415 +0200
@@ -1278,6 +1278,13 @@
 F:	include/linux/can/
 F:	include/linux/can.h
 
+CAN NETWORK DRIVERS
+P:	Wolfgang Grandegger
+M:	wg@grandegger.com
+L:	socketcan-core@lists.berlios.de (subscribers-only)
+W:	http://developer.berlios.de/projects/socketcan/
+S:	Maintained
+
 CELL BROADBAND ENGINE ARCHITECTURE
 P:	Arnd Bergmann
 M:	arnd@arndb.de

[PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Wolfgang Grandegger]

[-- Attachment #1: 03-driver-interface.patch --]
[-- Type: text/plain, Size: 26203 bytes --]

The CAN network device driver interface provides a generic interface to
setup, configure and monitor CAN network devices. It exports a set of
common data structures and functions, which all real CAN network device
drivers should use. Please have a look to the SJA1000 or MSCAN driver
to understand how to use them. The name of the module is can-dev.ko.

Furthermore, it adds a Netlink interface allowing to configure the CAN
device using the program "ip" from the iproute2 utility suite.

For further information please check "Documentation/networking/can.txt"

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
---
 drivers/net/can/Kconfig     |   23 +
 drivers/net/can/Makefile    |    5 
 drivers/net/can/dev.c       |  659 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/Kbuild    |    1 
 include/linux/can/dev.h     |   70 ++++
 include/linux/can/netlink.h |  113 +++++++
 6 files changed, 871 insertions(+)
 create mode 100644 drivers/net/can/dev.c
 create mode 100644 drivers/net/can/sysfs.c
 create mode 100644 drivers/net/can/sysfs.h
 create mode 100644 include/linux/can/dev.h

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================
--- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:10.449721030 +0200
+++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:25.061720603 +0200
@@ -12,6 +12,29 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called vcan.
 
+config CAN_DEV
+	tristate "Platform CAN drivers with Netlink support"
+	depends on CAN
+	default Y
+	---help---
+	  Enables the common framework for platform CAN drivers with Netlink
+	  support. This is the standard library for CAN drivers.
+	  If unsure, say Y.
+
+config CAN_CALC_BITTIMING
+	bool "CAN bit-timing calculation"
+	depends on CAN_DEV
+	default Y
+	---help---
+	  If enabled, CAN bit-timing parameters will be calculated for the
+	  bit-rate specified via Netlink argument "bitrate" when the device
+	  get started. This works fine for the most common CAN controllers
+	  with standard bit-rates but may fail for exotic bit-rates or CAN
+	  source clock frequencies. Disabling saves some space, but then the
+	  bit-timing parameters must be specified directly using the Netlink
+	  arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
+	  If unsure, say Y.
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN
Index: net-next-2.6/drivers/net/can/Makefile
===================================================================
--- net-next-2.6.orig/drivers/net/can/Makefile	2009-05-12 10:47:10.449721030 +0200
+++ net-next-2.6/drivers/net/can/Makefile	2009-05-12 10:47:25.061720603 +0200
@@ -3,3 +3,8 @@
 #
 
 obj-$(CONFIG_CAN_VCAN)		+= vcan.o
+
+obj-$(CONFIG_CAN_DEV)		+= can-dev.o
+can-dev-y			:= dev.o
+
+ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
Index: net-next-2.6/drivers/net/can/dev.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/drivers/net/can/dev.c	2009-05-12 10:47:25.073720638 +0200
@@ -0,0 +1,659 @@
+/*
+ * Copyright (C) 2005 Marc Kleine-Budde, Pengutronix
+ * Copyright (C) 2006 Andrey Volkov, Varma Electronics
+ * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/netdevice.h>
+#include <linux/if_arp.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/netlink.h>
+#include <net/rtnetlink.h>
+
+#define MOD_DESC "CAN device driver interface"
+
+MODULE_DESCRIPTION(MOD_DESC);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>");
+
+#ifdef CONFIG_CAN_CALC_BITTIMING
+#define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
+
+/*
+ * Bit-timing calculation derived from:
+ *
+ * Code based on LinCAN sources and H8S2638 project
+ * Copyright 2004-2006 Pavel Pisa - DCE FELK CVUT cz
+ * Copyright 2005      Stanislav Marek
+ * email: pisa@cmp.felk.cvut.cz
+ *
+ * Calculates proper bit-timing parameters for a specified bit-rate
+ * and sample-point, which can then be used to set the bit-timing
+ * registers of the CAN controller. You can find more information
+ * in the header file linux/can/netlink.h.
+ */
+static int can_update_spt(const struct can_bittiming_const *btc,
+			  int sampl_pt, int tseg, int *tseg1, int *tseg2)
+{
+	*tseg2 = tseg + 1 - (sampl_pt * (tseg + 1)) / 1000;
+	if (*tseg2 < btc->tseg2_min)
+		*tseg2 = btc->tseg2_min;
+	if (*tseg2 > btc->tseg2_max)
+		*tseg2 = btc->tseg2_max;
+	*tseg1 = tseg - *tseg2;
+	if (*tseg1 > btc->tseg1_max) {
+		*tseg1 = btc->tseg1_max;
+		*tseg2 = tseg - *tseg1;
+	}
+	return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
+}
+
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	const struct can_bittiming_const *btc = priv->bittiming_const;
+	long rate, best_rate = 0;
+	long best_error = 1000000000, error = 0;
+	int best_tseg = 0, best_brp = 0, brp = 0;
+	int tsegall, tseg = 0, tseg1 = 0, tseg2 = 0;
+	int spt_error = 1000, spt = 0, sampl_pt;
+	u64 v64;
+
+	if (!priv->bittiming_const)
+		return -ENOTSUPP;
+
+	/* Use CIA recommended sample points */
+	if (bt->sample_point) {
+		sampl_pt = bt->sample_point;
+	} else {
+		if (bt->bitrate > 800000)
+			sampl_pt = 750;
+		else if (bt->bitrate > 500000)
+			sampl_pt = 800;
+		else
+			sampl_pt = 875;
+	}
+
+	/* tseg even = round down, odd = round up */
+	for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;
+	     tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
+		tsegall = 1 + tseg / 2;
+		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
+		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
+		/* chose brp step which is possible in system */
+		brp = (brp / btc->brp_inc) * btc->brp_inc;
+		if ((brp < btc->brp_min) || (brp > btc->brp_max))
+			continue;
+		rate = priv->clock.freq / (brp * tsegall);
+		error = bt->bitrate - rate;
+		/* tseg brp biterror */
+		if (error < 0)
+			error = -error;
+		if (error > best_error)
+			continue;
+		best_error = error;
+		if (error == 0) {
+			spt = can_update_spt(btc, sampl_pt, tseg / 2,
+					     &tseg1, &tseg2);
+			error = sampl_pt - spt;
+			if (error < 0)
+				error = -error;
+			if (error > spt_error)
+				continue;
+			spt_error = error;
+		}
+		best_tseg = tseg / 2;
+		best_brp = brp;
+		best_rate = rate;
+		if (error == 0)
+			break;
+	}
+
+	if (best_error) {
+		/* Error in one-tenth of a percent */
+		error = (best_error * 1000) / bt->bitrate;
+		if (error > CAN_CALC_MAX_ERROR) {
+			dev_err(dev->dev.parent,
+				"bitrate error %ld.%ld%% too high\n",
+				error / 10, error % 10);
+			return -EDOM;
+		} else {
+			dev_warn(dev->dev.parent, "bitrate error %ld.%ld%%\n",
+				 error / 10, error % 10);
+		}
+	}
+
+	/* real sample point */
+	bt->sample_point = can_update_spt(btc, sampl_pt, best_tseg,
+					  &tseg1, &tseg2);
+
+	v64 = (u64)best_brp * 1000000000UL;
+	do_div(v64, priv->clock.freq);
+	bt->tq = (u32)v64;
+	bt->prop_seg = tseg1 / 2;
+	bt->phase_seg1 = tseg1 - bt->prop_seg;
+	bt->phase_seg2 = tseg2;
+	bt->sjw = 1;
+	bt->brp = best_brp;
+	/* real bit-rate */
+	bt->bitrate = priv->clock.freq / (bt->brp * (tseg1 + tseg2 + 1));
+
+	return 0;
+}
+#else /* !CONFIG_CAN_CALC_BITTIMING */
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+{
+	dev_err(dev->dev.parent, "bit-timing calculation not available\n");
+	return -EINVAL;
+}
+#endif /* CONFIG_CAN_CALC_BITTIMING */
+
+/*
+ * Checks the validity of the specified bit-timing parameters prop_seg,
+ * phase_seg1, phase_seg2 and sjw and tries to determine the bitrate
+ * prescaler value brp. You can find more information in the header
+ * file linux/can/netlink.h.
+ */
+static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	const struct can_bittiming_const *btc = priv->bittiming_const;
+	int tseg1, alltseg;
+	u64 brp64;
+
+	if (!priv->bittiming_const)
+		return -ENOTSUPP;
+
+	tseg1 = bt->prop_seg + bt->phase_seg1;
+	if (!bt->sjw)
+		bt->sjw = 1;
+	if (bt->sjw > btc->sjw_max ||
+	    tseg1 < btc->tseg1_min || tseg1 > btc->tseg1_max ||
+	    bt->phase_seg2 < btc->tseg2_min || bt->phase_seg2 > btc->tseg2_max)
+		return -ERANGE;
+
+	brp64 = (u64)priv->clock.freq * (u64)bt->tq;
+	if (btc->brp_inc > 1)
+		do_div(brp64, btc->brp_inc);
+	brp64 += 500000000UL - 1;
+	do_div(brp64, 1000000000UL); /* the practicable BRP */
+	if (btc->brp_inc > 1)
+		brp64 *= btc->brp_inc;
+	bt->brp = (u32)brp64;
+
+	if (bt->brp < btc->brp_min || bt->brp > btc->brp_max)
+		return -EINVAL;
+
+	alltseg = bt->prop_seg + bt->phase_seg1 + bt->phase_seg2 + 1;
+	bt->bitrate = priv->clock.freq / (bt->brp * alltseg);
+	bt->sample_point = ((tseg1 + 1) * 1000) / alltseg;
+
+	return 0;
+}
+
+int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	int err;
+
+	/* Check if the CAN device has bit-timing parameters */
+	if (priv->bittiming_const) {
+
+		/* Non-expert mode? Check if the bitrate has been pre-defined */
+		if (!bt->tq)
+			/* Determine bit-timing parameters */
+			err = can_calc_bittiming(dev, bt);
+		else
+			/* Check bit-timing params and calculate proper brp */
+			err = can_fixup_bittiming(dev, bt);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+/*
+ * Local echo of CAN messages
+ *
+ * CAN network devices *should* support a local echo functionality
+ * (see Documentation/networking/can.txt). To test the handling of CAN
+ * interfaces that do not support the local echo both driver types are
+ * implemented. In the case that the driver does not support the echo
+ * the IFF_ECHO remains clear in dev->flags. This causes the PF_CAN core
+ * to perform the echo as a fallback solution.
+ */
+static void can_flush_echo_skb(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	int i;
+
+	for (i = 0; i < CAN_ECHO_SKB_MAX; i++) {
+		if (priv->echo_skb[i]) {
+			kfree_skb(priv->echo_skb[i]);
+			priv->echo_skb[i] = NULL;
+			stats->tx_dropped++;
+			stats->tx_aborted_errors++;
+		}
+	}
+}
+
+/*
+ * Put the skb on the stack to be looped backed locally lateron
+ *
+ * The function is typically called in the start_xmit function
+ * of the device driver. The driver must protect access to
+ * priv->echo_skb, if necessary.
+ */
+void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, int idx)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	/* check flag whether this packet has to be looped back */
+	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
+		kfree_skb(skb);
+		return;
+	}
+
+	if (!priv->echo_skb[idx]) {
+		struct sock *srcsk = skb->sk;
+
+		if (atomic_read(&skb->users) != 1) {
+			struct sk_buff *old_skb = skb;
+
+			skb = skb_clone(old_skb, GFP_ATOMIC);
+			kfree_skb(old_skb);
+			if (!skb)
+				return;
+		} else
+			skb_orphan(skb);
+
+		skb->sk = srcsk;
+
+		/* make settings for echo to reduce code in irq context */
+		skb->protocol = htons(ETH_P_CAN);
+		skb->pkt_type = PACKET_BROADCAST;
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+		skb->dev = dev;
+
+		/* save this skb for tx interrupt echo handling */
+		priv->echo_skb[idx] = skb;
+	} else {
+		/* locking problem with netif_stop_queue() ?? */
+		dev_err(dev->dev.parent, "%s: BUG! echo_skb is occupied!\n",
+			__func__);
+		kfree_skb(skb);
+	}
+}
+EXPORT_SYMBOL_GPL(can_put_echo_skb);
+
+/*
+ * Get the skb from the stack and loop it back locally
+ *
+ * The function is typically called when the TX done interrupt
+ * is handled in the device driver. The driver must protect
+ * access to priv->echo_skb, if necessary.
+ */
+void can_get_echo_skb(struct net_device *dev, int idx)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	if ((dev->flags & IFF_ECHO) && priv->echo_skb[idx]) {
+		netif_rx(priv->echo_skb[idx]);
+		priv->echo_skb[idx] = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(can_get_echo_skb);
+
+/*
+ * CAN device restart for bus-off recovery
+ */
+int can_restart_now(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	int err;
+
+	/* Synchronize with dev->hard_start_xmit() */
+	netif_tx_lock(dev);
+
+	/* Ensure that no more messages can go out */
+	if (netif_carrier_ok(dev))
+		netif_carrier_off(dev);
+
+	/* Ensure that no more messages can come in */
+	err = priv->do_set_mode(dev, CAN_MODE_STOP);
+	if (err)
+		return err;
+
+	/*  Now it's save to clean up */
+	del_timer_sync(&priv->restart_timer);
+	can_flush_echo_skb(dev);
+
+	dev_dbg(dev->dev.parent, "restarted\n");
+	priv->can_stats.restarts++;
+
+	/* send restart message upstream */
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (skb == NULL)
+		return -ENOMEM;
+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_CAN);
+	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
+	memset(cf, 0, sizeof(struct can_frame));
+	cf->can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
+	cf->can_dlc = CAN_ERR_DLC;
+
+	netif_rx(skb);
+
+	dev->last_rx = jiffies;
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+
+	/* Now restart the device */
+	err = priv->do_set_mode(dev, CAN_MODE_START);
+	if (err)
+		return err;
+
+	netif_carrier_on(dev);
+
+	netif_tx_unlock(dev);
+
+	return 0;
+}
+
+static void can_restart_after(unsigned long data)
+{
+	struct net_device *dev = (struct net_device *)data;
+
+	can_restart_now(dev);
+}
+
+/*
+ * CAN bus-off
+ *
+ * This functions should be called when the device goes bus-off to
+ * tell the netif layer that no more packets can be sent or received.
+ * If enabled, a timer is started to trigger bus-off recovery.
+ */
+void can_bus_off(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	dev_dbg(dev->dev.parent, "bus-off\n");
+
+	netif_carrier_off(dev);
+	priv->can_stats.bus_off++;
+
+	if (priv->restart_ms) {
+		priv->restart_timer.function = can_restart_after;
+		priv->restart_timer.data = (unsigned long)dev;
+		priv->restart_timer.expires =
+			jiffies + (priv->restart_ms * HZ) / 1000;
+		add_timer(&priv->restart_timer);
+	}
+}
+EXPORT_SYMBOL_GPL(can_bus_off);
+
+static void can_setup(struct net_device *dev)
+{
+	dev->type = ARPHRD_CAN;
+	dev->mtu = sizeof(struct can_frame);
+	dev->hard_header_len = 0;
+	dev->addr_len = 0;
+	dev->tx_queue_len = 10;
+
+	/* New-style flags. */
+	dev->flags = IFF_NOARP;
+	dev->features = NETIF_F_NO_CSUM;
+}
+
+/*
+ * Allocate and setup space for the CAN network device
+ */
+struct net_device *alloc_candev(int sizeof_priv)
+{
+	struct net_device *dev;
+	struct can_priv *priv;
+
+	dev = alloc_netdev(sizeof_priv, "can%d", can_setup);
+	if (!dev)
+		return NULL;
+
+	priv = netdev_priv(dev);
+
+	priv->state = CAN_STATE_STOPPED;
+
+	init_timer(&priv->restart_timer);
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(alloc_candev);
+
+/*
+ * Free space of the CAN network device
+ */
+void free_candev(struct net_device *dev)
+{
+	free_netdev(dev);
+}
+EXPORT_SYMBOL_GPL(free_candev);
+
+/*
+ * Common open function when the device gets opened.
+ *
+ * This function should be called in the open function of the device
+ * driver.
+ */
+int open_candev(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	if (!priv->bittiming.tq && !priv->bittiming.bitrate) {
+		dev_err(dev->dev.parent, "bit-timing not yet defined\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(open_candev);
+
+/*
+ * Common close function for cleanup before the device gets closed.
+ *
+ * This function should be called in the close function of the device
+ * driver.
+ */
+void close_candev(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	del_timer_sync(&priv->restart_timer);
+	can_flush_echo_skb(dev);
+}
+EXPORT_SYMBOL_GPL(close_candev);
+
+/*
+ * CAN netlink interface
+ */
+static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
+	[IFLA_CAN_STATE]	= { .type = NLA_U32 },
+	[IFLA_CAN_CTRLMODE]	= { .len = sizeof(struct can_ctrlmode) },
+	[IFLA_CAN_RESTART_MS]	= { .type = NLA_U32 },
+	[IFLA_CAN_RESTART]	= { .type = NLA_U32 },
+	[IFLA_CAN_BITTIMING]	= { .len = sizeof(struct can_bittiming) },
+	[IFLA_CAN_BITTIMING_CONST]
+				= { .len = sizeof(struct can_bittiming_const) },
+	[IFLA_CAN_CLOCK]	= { .len = sizeof(struct can_clock) },
+};
+
+static int can_changelink(struct net_device *dev,
+			  struct nlattr *tb[], struct nlattr *data[])
+{
+	struct can_priv *priv = netdev_priv(dev);
+	int err;
+
+	/* We need synchronization with dev->stop() */
+	ASSERT_RTNL();
+
+	if (data[IFLA_CAN_CTRLMODE]) {
+		struct can_ctrlmode *cm;
+
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
+		priv->ctrlmode &= ~cm->mask;
+		priv->ctrlmode |= cm->flags;
+		printk("ctrlmode %#x mask %#x flags %#x\n", priv->ctrlmode,
+		       cm->mask, cm->flags);
+	}
+
+	if (data[IFLA_CAN_BITTIMING]) {
+		struct can_bittiming bt;
+
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
+		if ((!bt.bitrate && !bt.tq) || (bt.bitrate && bt.tq))
+			return -EINVAL;
+		err = can_get_bittiming(dev, &bt);
+		if (err)
+			return err;
+		memcpy(&priv->bittiming, &bt, sizeof(bt));
+
+		if (priv->do_set_bittiming) {
+			/* Finally, set the bit-timing registers */
+			err = priv->do_set_bittiming(dev);
+			if (err)
+				return err;
+		}
+	}
+
+	if (data[IFLA_CAN_RESTART_MS])
+		priv->restart_ms = nla_get_u32(data[IFLA_CAN_RESTART_MS]);
+
+	if (data[IFLA_CAN_RESTART]) {
+		if (!(dev->flags & IFF_UP))
+			return -EINVAL;
+		can_restart_now(dev);
+	}
+
+	return 0;
+}
+
+static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_ctrlmode cm = {.flags = priv->ctrlmode};
+	enum can_state state = priv->state;
+
+	if (priv->do_get_state)
+		priv->do_get_state(dev, &state);
+	NLA_PUT_U32(skb, IFLA_CAN_STATE, state);
+	NLA_PUT(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm);
+	NLA_PUT_U32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms);
+	NLA_PUT(skb, IFLA_CAN_BITTIMING,
+		sizeof(priv->bittiming), &priv->bittiming);
+	NLA_PUT(skb, IFLA_CAN_CLOCK, sizeof(cm), &priv->clock);
+	if (priv->bittiming_const)
+		NLA_PUT(skb, IFLA_CAN_BITTIMING_CONST,
+			sizeof(*priv->bittiming_const), priv->bittiming_const);
+
+	return 0;
+
+nla_put_failure:
+	return -EMSGSIZE;
+}
+
+static int can_fill_xstats(struct sk_buff *skb, const struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	NLA_PUT(skb, IFLA_INFO_XSTATS,
+		sizeof(priv->can_stats), &priv->can_stats);
+
+	return 0;
+
+nla_put_failure:
+	return -EMSGSIZE;
+}
+
+static struct rtnl_link_ops can_link_ops __read_mostly = {
+	.kind		= "can",
+	.maxtype	= IFLA_CAN_MAX,
+	.policy		= can_policy,
+	.setup		= can_setup,
+	.changelink	= can_changelink,
+	.fill_info	= can_fill_info,
+	.fill_xstats	= can_fill_xstats,
+};
+
+/*
+ * Register the CAN network device
+ */
+int register_candev(struct net_device *dev)
+{
+	int err;
+
+	rtnl_lock();
+
+	err = __rtnl_link_register(&can_link_ops);
+	if (err)
+		goto out;
+
+	err = dev_alloc_name(dev, dev->name);
+	if (err >= 0) {
+		dev->rtnl_link_ops = &can_link_ops;
+		err = register_netdevice(dev);
+	}
+
+	if (err < 0)
+		__rtnl_link_unregister(&can_link_ops);
+out:
+	rtnl_unlock();
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(register_candev);
+
+/*
+ * Unregister the CAN network device
+ */
+void unregister_candev(struct net_device *dev)
+{
+	rtnl_link_unregister(&can_link_ops);
+}
+EXPORT_SYMBOL_GPL(unregister_candev);
+
+static __init int can_dev_init(void)
+{
+	printk(KERN_INFO MOD_DESC "\n");
+
+	return 0;
+}
+module_init(can_dev_init);
+
+static __exit void can_dev_exit(void)
+{
+}
+module_exit(can_dev_exit);
+
+MODULE_ALIAS_RTNL_LINK("can");
Index: net-next-2.6/include/linux/can/dev.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/include/linux/can/dev.h	2009-05-12 10:47:25.077720929 +0200
@@ -0,0 +1,70 @@
+/*
+ * linux/can/dev.h
+ *
+ * Definitions for the CAN network device driver interface
+ *
+ * Copyright (C) 2006 Andrey Volkov <avolkov@varma-el.com>
+ *               Varma Electronics Oy
+ *
+ * Copyright (C) 2008 Wolfgang Grandegger <wg@grandegger.com>
+ *
+ * Send feedback to <socketcan-users@lists.berlios.de>
+ */
+
+#ifndef CAN_DEV_H
+#define CAN_DEV_H
+
+#include <linux/can/netlink.h>
+#include <linux/can/error.h>
+
+/*
+ * CAN mode
+ */
+enum can_mode {
+	CAN_MODE_STOP = 0,
+	CAN_MODE_START,
+	CAN_MODE_SLEEP
+};
+
+/*
+ * CAN common private data
+ */
+#define CAN_ECHO_SKB_MAX  4
+
+struct can_priv {
+	struct can_device_stats can_stats;
+
+	struct can_bittiming bittiming;
+	struct can_bittiming_const *bittiming_const;
+	struct can_clock clock;
+
+	enum can_state state;
+	u32 ctrlmode;
+
+	int restart_ms;
+	struct timer_list restart_timer;
+
+	struct sk_buff *echo_skb[CAN_ECHO_SKB_MAX];
+
+	int (*do_set_bittiming)(struct net_device *dev);
+	int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
+	int (*do_get_state)(const struct net_device *dev,
+			    enum can_state *state);
+};
+
+struct net_device *alloc_candev(int sizeof_priv);
+void free_candev(struct net_device *dev);
+
+int open_candev(struct net_device *dev);
+void close_candev(struct net_device *dev);
+
+int register_candev(struct net_device *dev);
+void unregister_candev(struct net_device *dev);
+
+int can_restart_now(struct net_device *dev);
+void can_bus_off(struct net_device *dev);
+
+void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, int idx);
+void can_get_echo_skb(struct net_device *dev, int idx);
+
+#endif /* CAN_DEV_H */
Index: net-next-2.6/include/linux/can/netlink.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/include/linux/can/netlink.h	2009-05-12 10:47:25.079720376 +0200
@@ -0,0 +1,113 @@
+/*
+ * linux/can/netlink.h
+ *
+ * Definitions for the CAN netlink interface
+ *
+ * Copyright (c) 2009 Wolfgang Grandegger <wg@grandegger.com>
+ *
+ * Send feedback to <socketcan-users@lists.berlios.de>
+ *
+ */
+
+#ifndef CAN_NETLINK_H
+#define CAN_NETLINK_H
+
+#include <linux/types.h>
+
+/*
+ * CAN bit-timing parameters
+ *
+ * For futher information, please read chapter "8 BIT TIMING
+ * REQUIREMENTS" of the "Bosch CAN Specification version 2.0"
+ * at http://www.semiconductors.bosch.de/pdf/can2spec.pdf.
+ */
+struct can_bittiming {
+	__u32 bitrate;		/* Bit-rate in bits/second */
+	__u32 sample_point;	/* Sample point in one-tenth of a percent */
+	__u32 tq;		/* Time quanta (TQ) in nanoseconds */
+	__u32 prop_seg;		/* Propagation segment in TQs */
+	__u32 phase_seg1;	/* Phase buffer segment 1 in TQs */
+	__u32 phase_seg2;	/* Phase buffer segment 2 in TQs */
+	__u32 sjw;		/* Synchronisation jump width in TQs */
+	__u32 brp;		/* Bit-rate prescaler */
+};
+
+/*
+ * CAN harware-dependent bit-timing constant
+ *
+ * Used for calculating and checking bit-timing parameters
+ */
+struct can_bittiming_const {
+	char name[16];		/* Name of the CAN controller hardware */
+	__u32 tseg1_min;	/* Time segement 1 = prop_seg + phase_seg1 */
+	__u32 tseg1_max;
+	__u32 tseg2_min;	/* Time segement 2 = phase_seg2 */
+	__u32 tseg2_max;
+	__u32 sjw_max;		/* Synchronisation jump width */
+	__u32 brp_min;		/* Bit-rate prescaler */
+	__u32 brp_max;
+	__u32 brp_inc;
+};
+
+/*
+ * CAN clock parameters
+ */
+struct can_clock {
+	__u32 freq;		/* CAN system clock frequency in Hz */
+};
+
+/*
+ * CAN operational and error states
+ */
+enum can_state {
+	CAN_STATE_ERROR_ACTIVE = 0,	/* RX/TX error count < 96 */
+	CAN_STATE_ERROR_WARNING,	/* RX/TX error count < 128 */
+	CAN_STATE_ERROR_PASSIVE,	/* RX/TX error count < 256 */
+	CAN_STATE_BUS_OFF,		/* RX/TX error count >= 256 */
+	CAN_STATE_STOPPED,		/* Device is stopped */
+	CAN_STATE_SLEEPING,		/* Device is sleeping */
+	CAN_STATE_MAX
+};
+
+/*
+ * CAN controller mode
+ */
+struct can_ctrlmode {
+	__u32 mask;
+	__u32 flags;
+};
+
+#define CAN_CTRLMODE_LOOPBACK	0x1	/* Loopback mode */
+#define CAN_CTRLMODE_LISTENONLY	0x2 	/* Listen-only mode */
+#define CAN_CTRLMODE_3_SAMPLES	0x4	/* Triple sampling mode */
+
+/*
+ * CAN device statistics
+ */
+struct can_device_stats {
+	__u32 bus_error;	/* Bus errors */
+	__u32 error_warning;	/* Changes to error warning state */
+	__u32 error_passive;	/* Changes to error passive state */
+	__u32 bus_off;		/* Changes to bus off state */
+	__u32 arbitration_lost; /* Arbitration lost errors */
+	__u32 restarts;		/* CAN controller re-starts */
+};
+
+/*
+ * CAN netlink interface
+ */
+enum {
+	IFLA_CAN_UNSPEC,
+	IFLA_CAN_BITTIMING,
+	IFLA_CAN_BITTIMING_CONST,
+	IFLA_CAN_CLOCK,
+	IFLA_CAN_STATE,
+	IFLA_CAN_CTRLMODE,
+	IFLA_CAN_RESTART_MS,
+	IFLA_CAN_RESTART,
+	__IFLA_CAN_MAX
+};
+
+#define IFLA_CAN_MAX	(__IFLA_CAN_MAX - 1)
+
+#endif /* CAN_NETLINK_H */
Index: net-next-2.6/include/linux/can/Kbuild
===================================================================
--- net-next-2.6.orig/include/linux/can/Kbuild	2009-05-12 10:47:10.450720893 +0200
+++ net-next-2.6/include/linux/can/Kbuild	2009-05-12 10:47:25.082720803 +0200
@@ -1,3 +1,4 @@
 header-y += raw.h
 header-y += bcm.h
 header-y += error.h
+header-y += netlink.h

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Andrew Morton]

On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <wg@grandegger.com> wrote:

> +int can_restart_now(struct net_device *dev)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	int err;
> +
> +	/* Synchronize with dev->hard_start_xmit() */
> +	netif_tx_lock(dev);
> +
> +	/* Ensure that no more messages can go out */
> +	if (netif_carrier_ok(dev))
> +		netif_carrier_off(dev);
> +
> +	/* Ensure that no more messages can come in */
> +	err = priv->do_set_mode(dev, CAN_MODE_STOP);
> +	if (err)
> +		return err;
> +
> +	/*  Now it's save to clean up */
> +	del_timer_sync(&priv->restart_timer);

This is deadlockable.

It calls del_timer_sync() while holding netif_tx_lock().  But the timer
handler (can_restart_now()) also takes netif_tx_lock().  So if the
timer handler is presently running, it's sitting there spinning in
netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
timer handler to complete.

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Andrew Morton]

On Tue, 12 May 2009 23:30:52 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
> > +int can_restart_now(struct net_device *dev)
> > +{
> > +	struct can_priv *priv = netdev_priv(dev);
> > +	struct net_device_stats *stats = &dev->stats;
> > +	struct sk_buff *skb;
> > +	struct can_frame *cf;
> > +	int err;
> > +
> > +	/* Synchronize with dev->hard_start_xmit() */
> > +	netif_tx_lock(dev);
> > +
> > +	/* Ensure that no more messages can go out */
> > +	if (netif_carrier_ok(dev))
> > +		netif_carrier_off(dev);
> > +
> > +	/* Ensure that no more messages can come in */
> > +	err = priv->do_set_mode(dev, CAN_MODE_STOP);
> > +	if (err)
> > +		return err;
> > +
> > +	/*  Now it's save to clean up */
> > +	del_timer_sync(&priv->restart_timer);
> 
> This is deadlockable.
> 
> It calls del_timer_sync() while holding netif_tx_lock().  But the timer
> handler (can_restart_now()) also takes netif_tx_lock().  So if the
> timer handler is presently running, it's sitting there spinning in
> netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
> timer handler to complete.

Also, I wonder if it's safe to take netif_tx_lock() from a timer
handler when other parts of the code might be taking it from process
context (I didn't check).

lockdep should be able to detect this, and I trust this code has been
fully runtime tested with lockdep enabled?

<boggles at the size of the inlined netif_tx_lock()>

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Wolfgang Grandegger]

Andrew Morton wrote:
> On Tue, 12 May 2009 23:30:52 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <wg@grandegger.com> wrote:
>>
>>> +int can_restart_now(struct net_device *dev)
>>> +{
>>> +	struct can_priv *priv = netdev_priv(dev);
>>> +	struct net_device_stats *stats = &dev->stats;
>>> +	struct sk_buff *skb;
>>> +	struct can_frame *cf;
>>> +	int err;
>>> +
>>> +	/* Synchronize with dev->hard_start_xmit() */
>>> +	netif_tx_lock(dev);
>>> +
>>> +	/* Ensure that no more messages can go out */
>>> +	if (netif_carrier_ok(dev))
>>> +		netif_carrier_off(dev);
>>> +
>>> +	/* Ensure that no more messages can come in */
>>> +	err = priv->do_set_mode(dev, CAN_MODE_STOP);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	/*  Now it's save to clean up */
>>> +	del_timer_sync(&priv->restart_timer);
>> This is deadlockable.
>>
>> It calls del_timer_sync() while holding netif_tx_lock().  But the timer
>> handler (can_restart_now()) also takes netif_tx_lock().  So if the
>> timer handler is presently running, it's sitting there spinning in
>> netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
>> timer handler to complete.

Oops, I have obviously overlook that.

> Also, I wonder if it's safe to take netif_tx_lock() from a timer
> handler when other parts of the code might be taking it from process
> context (I didn't check).
> 
> lockdep should be able to detect this, and I trust this code has been
> fully runtime tested with lockdep enabled?

Well, CONFIG_PROVE_LOCKING would be cool, but I'm unable to enabled it
for my MPC5200 test system. Only 64bit PowerPC's seem to support
TRACE_IRQFLAGS_SUPPORT. I'm going to test the code on a PC as well.

Thanks,

Wolfgang.

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Andrew Morton]

On Wed, 13 May 2009 13:37:16 +0200 Wolfgang Grandegger <wg@grandegger.com> wrote:

> > Also, I wonder if it's safe to take netif_tx_lock() from a timer
> > handler when other parts of the code might be taking it from process
> > context (I didn't check).
> > 
> > lockdep should be able to detect this, and I trust this code has been
> > fully runtime tested with lockdep enabled?
> 
> Well, CONFIG_PROVE_LOCKING would be cool, but I'm unable to enabled it
> for my MPC5200 test system. Only 64bit PowerPC's seem to support
> TRACE_IRQFLAGS_SUPPORT. I'm going to test the code on a PC as well.

I discussed this off-list with Peter Zijlstra and Johannes Berg. 
Apparently lockdep _will_ detect this deadlockable situation - Johannes
recently added the capability because he had the same situation in
wireless code somewhere.

But of course it does require that the timer handler has executed at
least once.  Many handlers in the kernel never fire in normal operation.

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Wolfgang Grandegger]

Andrew Morton wrote:
> On Wed, 13 May 2009 13:37:16 +0200 Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
>>> Also, I wonder if it's safe to take netif_tx_lock() from a timer
>>> handler when other parts of the code might be taking it from process
>>> context (I didn't check).
>>>
>>> lockdep should be able to detect this, and I trust this code has been
>>> fully runtime tested with lockdep enabled?
>> Well, CONFIG_PROVE_LOCKING would be cool, but I'm unable to enabled it
>> for my MPC5200 test system. Only 64bit PowerPC's seem to support
>> TRACE_IRQFLAGS_SUPPORT. I'm going to test the code on a PC as well.
> 
> I discussed this off-list with Peter Zijlstra and Johannes Berg. 
> Apparently lockdep _will_ detect this deadlockable situation - Johannes
> recently added the capability because he had the same situation in
> wireless code somewhere.

Below is the kernel message I get with CONFIG_PROVE_LOCKING enabled when
I call can_restart_now() from the user context via netlink interface. I
have some difficulties interpreting the message, but it seems to confirm
your fears.

> But of course it does require that the timer handler has executed at
> least once.  Many handlers in the kernel never fire in normal operation.

I do not see problems if can_restart_now() is called via timer callback
(after replacing del_timer_sync with del_timer).

Wolfgang.



peak_pci 0000:01:08.0: setting BTR0=0x00 BTR1=0x14
can: controller area network core (rev 20090105 abi 8)
NET: Registered protocol family 29
can: request_module (can-proto-1) failed.
can: raw protocol (rev 20090105)
peak_pci 0000:01:08.0: error warning interrupt
peak_pci 0000:01:08.0: error passive interrupt
peak_pci 0000:01:08.0: error warning interrupt
peak_pci 0000:01:08.0: bus-off

=================================
[ INFO: inconsistent lock state ]
2.6.29.3 #1
---------------------------------
inconsistent {in-softirq-W} -> {softirq-on-W} usage.
ip/2847 [HC0[0]:SC0[0]:HE1:SE1] takes:
 (&dev->tx_global_lock){-+..}, at: [<f7e29806>] can_restart_now+0x26/0x1c1 [can_dev]
{in-softirq-W} state was registered at:
  [<c044957d>] __lock_acquire+0x244/0xb01
  [<c0449e95>] lock_acquire+0x5b/0x81
  [<c067c29b>] _spin_lock+0x1b/0x2a
  [<c06031fd>] netif_tx_lock+0x18/0x6a
  [<c06032a2>] dev_watchdog+0xf/0x10d
  [<c04331bc>] run_timer_softirq+0x13b/0x19b
  [<c043000e>] __do_softirq+0x98/0x136
  [<ffffffff>] 0xffffffff
irq event stamp: 1973
hardirqs last  enabled at (1973): [<c067b100>] __mutex_lock_common+0x2be/0x313
hardirqs last disabled at (1972): [<c067aeb4>] __mutex_lock_common+0x72/0x313
softirqs last  enabled at (1790): [<c05fe73b>] sk_filter+0x9a/0xa7
softirqs last disabled at (1788): [<c05fe6bf>] sk_filter+0x1e/0xa7

other info that might help us debug this:
1 lock held by ip/2847:
 #0:  (rtnl_mutex){--..}, at: [<c05fcef7>] rtnetlink_rcv+0x12/0x26

stack backtrace:
Pid: 2847, comm: ip Not tainted 2.6.29.3 #1
Call Trace:
 [<c0679d30>] ? printk+0xf/0x17
 [<c044860c>] valid_state+0x12a/0x13d
 [<c04489dc>] mark_lock+0x248/0x349
 [<c04495fe>] __lock_acquire+0x2c5/0xb01
 [<c04858e4>] ? handle_mm_fault+0x6a4/0x6b7
 [<c0449e95>] lock_acquire+0x5b/0x81
 [<f7e29806>] ? can_restart_now+0x26/0x1c1 [can_dev]
 [<c067c29b>] _spin_lock+0x1b/0x2a
 [<f7e29806>] ? can_restart_now+0x26/0x1c1 [can_dev]
 [<f7e29806>] can_restart_now+0x26/0x1c1 [can_dev]
 [<f7e29ab8>] can_changelink+0x117/0x12f [can_dev]
 [<c060a7aa>] ? nla_parse+0x57/0xb2
 [<f7e299a1>] ? can_changelink+0x0/0x12f [can_dev]
 [<c05fd306>] rtnl_newlink+0x249/0x3df
 [<c05fd1fe>] ? rtnl_newlink+0x141/0x3df
 [<c05fd0bd>] ? rtnl_newlink+0x0/0x3df
 [<c05fd0a3>] rtnetlink_rcv_msg+0x198/0x1b2
 [<c05fcf0b>] ? rtnetlink_rcv_msg+0x0/0x1b2
 [<c060a2a0>] netlink_rcv_skb+0x30/0x78
 [<c05fcf03>] rtnetlink_rcv+0x1e/0x26
 [<c0609e8a>] netlink_unicast+0xf6/0x156
 [<c060a130>] netlink_sendmsg+0x246/0x253
 [<c05e8b28>] __sock_sendmsg+0x45/0x4e
 [<c05e9303>] sock_sendmsg+0xb8/0xce
 [<c043c15f>] ? autoremove_wake_function+0x0/0x33
 [<c048348d>] ? might_fault+0x43/0x80
 [<c048348d>] ? might_fault+0x43/0x80
 [<c051aa39>] ? copy_from_user+0x2a/0x111
 [<c05eff69>] ? verify_iovec+0x40/0x6f
 [<c05e9458>] sys_sendmsg+0x13f/0x192
 [<c067e281>] ? do_page_fault+0x380/0x690
 [<c0447a3f>] ? register_lock_class+0x17/0x290
 [<c04487b2>] ? mark_lock+0x1e/0x349
 [<c04487b2>] ? mark_lock+0x1e/0x349
 [<c048348d>] ? might_fault+0x43/0x80
 [<c05ea3f4>] sys_socketcall+0x153/0x183
 [<c04038eb>] sysenter_do_call+0x12/0x3f

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Oliver Hartkopp]

Andrew Morton wrote:
> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger <wg@grandegger.com> wrote:
>
>   
>> +int can_restart_now(struct net_device *dev)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	struct sk_buff *skb;
>> +	struct can_frame *cf;
>> +	int err;
>> +
>> +	/* Synchronize with dev->hard_start_xmit() */
>> +	netif_tx_lock(dev);
>> +
>> +	/* Ensure that no more messages can go out */
>> +	if (netif_carrier_ok(dev))
>> +		netif_carrier_off(dev);
>> +
>> +	/* Ensure that no more messages can come in */
>> +	err = priv->do_set_mode(dev, CAN_MODE_STOP);
>> +	if (err)
>> +		return err;
>> +
>> +	/*  Now it's save to clean up */
>> +	del_timer_sync(&priv->restart_timer);
>>     
>
> This is deadlockable.
>
> It calls del_timer_sync() while holding netif_tx_lock().  But the timer
> handler (can_restart_now()) also takes netif_tx_lock().  So if the
> timer handler is presently running, it's sitting there spinning in
> netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
> timer handler to complete.
>
>
>   

Hi Wolfgang,

would it be an appropriate solution, just to invoke

netif_stop_queue() in can_bus_off()

and invoke

netif_wake_queue() in can_restart_now()

???

In a BUSOFF condition we're not able to send CAN frames anyway, so  we 
can disable the device queue and the we won't  need any netif_tx_lock() 
right?

AFAIK this was the original implementation before some of the latest 
improvement with the netif_carrier stuff.

What do you think?

Best regards,
Oliver

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Wolfgang Grandegger]

Oliver Hartkopp wrote:
> Andrew Morton wrote:
>> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger
>> <wg@grandegger.com> wrote:
>>
>>  
>>> +int can_restart_now(struct net_device *dev)
>>> +{
>>> +    struct can_priv *priv = netdev_priv(dev);
>>> +    struct net_device_stats *stats = &dev->stats;
>>> +    struct sk_buff *skb;
>>> +    struct can_frame *cf;
>>> +    int err;
>>> +
>>> +    /* Synchronize with dev->hard_start_xmit() */
>>> +    netif_tx_lock(dev);
>>> +
>>> +    /* Ensure that no more messages can go out */
>>> +    if (netif_carrier_ok(dev))
>>> +        netif_carrier_off(dev);
>>> +
>>> +    /* Ensure that no more messages can come in */
>>> +    err = priv->do_set_mode(dev, CAN_MODE_STOP);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    /*  Now it's save to clean up */
>>> +    del_timer_sync(&priv->restart_timer);
>>>     
>>
>> This is deadlockable.
>>
>> It calls del_timer_sync() while holding netif_tx_lock().  But the timer
>> handler (can_restart_now()) also takes netif_tx_lock().  So if the
>> timer handler is presently running, it's sitting there spinning in
>> netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
>> timer handler to complete.
>>
>>
>>   
> 
> Hi Wolfgang,
> 
> would it be an appropriate solution, just to invoke
> 
> netif_stop_queue() in can_bus_off()
> 
> and invoke
> 
> netif_wake_queue() in can_restart_now()
> 
> ???
> 
> In a BUSOFF condition we're not able to send CAN frames anyway, so  we
> can disable the device queue and the we won't  need any netif_tx_lock()
> right?
> 
> AFAIK this was the original implementation before some of the latest
> improvement with the netif_carrier stuff.
> 
> What do you think?

The problem is the "manual" restart triggered via the netlink interface,
which can occur in the middle of ndo_start_xmit().

Wolfgang.

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Oliver Hartkopp]

Wolfgang Grandegger wrote:
> Oliver Hartkopp wrote:
>   
>> Andrew Morton wrote:
>>     
>>> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger
>>> <wg@grandegger.com> wrote:
>>>
>>>  
>>>       
>>>> +int can_restart_now(struct net_device *dev)
>>>> +{
>>>> +    struct can_priv *priv = netdev_priv(dev);
>>>> +    struct net_device_stats *stats = &dev->stats;
>>>> +    struct sk_buff *skb;
>>>> +    struct can_frame *cf;
>>>> +    int err;
>>>> +
>>>> +    /* Synchronize with dev->hard_start_xmit() */
>>>> +    netif_tx_lock(dev);
>>>> +
>>>> +    /* Ensure that no more messages can go out */
>>>> +    if (netif_carrier_ok(dev))
>>>> +        netif_carrier_off(dev);
>>>> +
>>>> +    /* Ensure that no more messages can come in */
>>>> +    err = priv->do_set_mode(dev, CAN_MODE_STOP);
>>>> +    if (err)
>>>> +        return err;
>>>> +
>>>> +    /*  Now it's save to clean up */
>>>> +    del_timer_sync(&priv->restart_timer);
>>>>     
>>>>         
>>> This is deadlockable.
>>>
>>> It calls del_timer_sync() while holding netif_tx_lock().  But the timer
>>> handler (can_restart_now()) also takes netif_tx_lock().  So if the
>>> timer handler is presently running, it's sitting there spinning in
>>> netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
>>> timer handler to complete.
>>>
>>>
>>>   
>>>       
>> Hi Wolfgang,
>>
>> would it be an appropriate solution, just to invoke
>>
>> netif_stop_queue() in can_bus_off()
>>
>> and invoke
>>
>> netif_wake_queue() in can_restart_now()
>>
>> ???
>>
>> In a BUSOFF condition we're not able to send CAN frames anyway, so  we
>> can disable the device queue and the we won't  need any netif_tx_lock()
>> right?
>>
>> AFAIK this was the original implementation before some of the latest
>> improvement with the netif_carrier stuff.
>>
>> What do you think?
>>     
>
> The problem is the "manual" restart triggered via the netlink interface,
> which can occur in the middle of ndo_start_xmit().
>
>   

Ah, i see.

What if the manual restart via netlink would also stop the queue and 
start the timer?

Regards,
Oliver

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Wolfgang Grandegger]

Oliver Hartkopp wrote:
> Wolfgang Grandegger wrote:
>> Oliver Hartkopp wrote:
>>  
>>> Andrew Morton wrote:
>>>    
>>>> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger
>>>> <wg@grandegger.com> wrote:
>>>>
>>>>  
>>>>      
>>>>> +int can_restart_now(struct net_device *dev)
>>>>> +{
>>>>> +    struct can_priv *priv = netdev_priv(dev);
>>>>> +    struct net_device_stats *stats = &dev->stats;
>>>>> +    struct sk_buff *skb;
>>>>> +    struct can_frame *cf;
>>>>> +    int err;
>>>>> +
>>>>> +    /* Synchronize with dev->hard_start_xmit() */
>>>>> +    netif_tx_lock(dev);
>>>>> +
>>>>> +    /* Ensure that no more messages can go out */
>>>>> +    if (netif_carrier_ok(dev))
>>>>> +        netif_carrier_off(dev);
>>>>> +
>>>>> +    /* Ensure that no more messages can come in */
>>>>> +    err = priv->do_set_mode(dev, CAN_MODE_STOP);
>>>>> +    if (err)
>>>>> +        return err;
>>>>> +
>>>>> +    /*  Now it's save to clean up */
>>>>> +    del_timer_sync(&priv->restart_timer);
>>>>>             
>>>> This is deadlockable.
>>>>
>>>> It calls del_timer_sync() while holding netif_tx_lock().  But the timer
>>>> handler (can_restart_now()) also takes netif_tx_lock().  So if the
>>>> timer handler is presently running, it's sitting there spinning in
>>>> netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
>>>> timer handler to complete.
>>>>
>>>>
>>>>         
>>> Hi Wolfgang,
>>>
>>> would it be an appropriate solution, just to invoke
>>>
>>> netif_stop_queue() in can_bus_off()
>>>
>>> and invoke
>>>
>>> netif_wake_queue() in can_restart_now()
>>>
>>> ???
>>>
>>> In a BUSOFF condition we're not able to send CAN frames anyway, so  we
>>> can disable the device queue and the we won't  need any netif_tx_lock()
>>> right?
>>>
>>> AFAIK this was the original implementation before some of the latest
>>> improvement with the netif_carrier stuff.
>>>
>>> What do you think?
>>>     
>>
>> The problem is the "manual" restart triggered via the netlink interface,
>> which can occur in the middle of ndo_start_xmit().
>>
>>   
> 
> Ah, i see.
> 
> What if the manual restart via netlink would also stop the queue and
> start the timer?

It will not help if the restart is triggered in the middle of
ndo_start_xmit().

Wolfgang.

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Oliver Hartkopp]

Wolfgang Grandegger wrote:
> Oliver Hartkopp wrote:
>   
>> Wolfgang Grandegger wrote:
>>     
>>> Oliver Hartkopp wrote:
>>>  
>>>       
>>>> Andrew Morton wrote:
>>>>    
>>>>         
>>>>> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger
>>>>> <wg@grandegger.com> wrote:
>>>>>
>>>>>  
>>>>>      
>>>>>           
>>>>>> +int can_restart_now(struct net_device *dev)
>>>>>> +{
>>>>>> +    struct can_priv *priv = netdev_priv(dev);
>>>>>> +    struct net_device_stats *stats = &dev->stats;
>>>>>> +    struct sk_buff *skb;
>>>>>> +    struct can_frame *cf;
>>>>>> +    int err;
>>>>>> +
>>>>>> +    /* Synchronize with dev->hard_start_xmit() */
>>>>>> +    netif_tx_lock(dev);
>>>>>> +
>>>>>> +    /* Ensure that no more messages can go out */
>>>>>> +    if (netif_carrier_ok(dev))
>>>>>> +        netif_carrier_off(dev);
>>>>>> +
>>>>>> +    /* Ensure that no more messages can come in */
>>>>>> +    err = priv->do_set_mode(dev, CAN_MODE_STOP);
>>>>>> +    if (err)
>>>>>> +        return err;
>>>>>> +
>>>>>> +    /*  Now it's save to clean up */
>>>>>> +    del_timer_sync(&priv->restart_timer);
>>>>>>             
>>>>>>             
>>>>> This is deadlockable.
>>>>>
>>>>> It calls del_timer_sync() while holding netif_tx_lock().  But the timer
>>>>> handler (can_restart_now()) also takes netif_tx_lock().  So if the
>>>>> timer handler is presently running, it's sitting there spinning in
>>>>> netif_tx_lock().  And del_timer_sync() is sitting there waiting for the
>>>>> timer handler to complete.
>>>>>
>>>>>
>>>>>         
>>>>>           
>>>> Hi Wolfgang,
>>>>
>>>> would it be an appropriate solution, just to invoke
>>>>
>>>> netif_stop_queue() in can_bus_off()
>>>>
>>>> and invoke
>>>>
>>>> netif_wake_queue() in can_restart_now()
>>>>
>>>> ???
>>>>
>>>> In a BUSOFF condition we're not able to send CAN frames anyway, so  we
>>>> can disable the device queue and the we won't  need any netif_tx_lock()
>>>> right?
>>>>
>>>> AFAIK this was the original implementation before some of the latest
>>>> improvement with the netif_carrier stuff.
>>>>
>>>> What do you think?
>>>>     
>>>>         
>>> The problem is the "manual" restart triggered via the netlink interface,
>>> which can occur in the middle of ndo_start_xmit().
>>>
>>>   
>>>       
>> Ah, i see.
>>
>> What if the manual restart via netlink would also stop the queue and
>> start the timer?
>>     
>
> It will not help if the restart is triggered in the middle of
> ndo_start_xmit().
>
>   

Hi Wolfgang,

i think, i found a solution that removes the locking problem completely:

When a bus-off occurs in the controller, the communication on the CAN 
bus can be treated as unusable for this controller (let's say it is dead).
E.g. the SJA1000 set's its reset bit for that reason and waits to be 
initialized by the CPU again.

So IMO restarting the CAN controller while in operational state is not a 
valid use case.

When a bus-off (interrupt) occurs, we should

- invoke netif_carrier_off(dev)
- invoke netif_stop_queue(dev)
- set the state to CAN_STATE_BUS_OFF

and of course create the error message, clear the interrupts(!) and then 
leave the irq service function.

That's it.

When the automatic CAN controller restart is enabled: start the timer.

For the manual netlink function: Test for CAN_STATE_BUS_OFF (!) and 
invoke the current can_restart_now(dev) or start the timer with e.g. 1ms ...

This approach should make it and fulfills the bus-off intention of the 
CAN controllers ("disabled and wait for re-initialisation").

And there's no locking of the tx_queue needed anymore as the tx_queue is 
already stopped, when the restart is performed.

What do you think about this approach?

Regards,
Oliver

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Wolfgang Grandegger]

Oliver Hartkopp wrote:
> Wolfgang Grandegger wrote:
>> Oliver Hartkopp wrote:
>>  
>>> Wolfgang Grandegger wrote:
>>>    
>>>> Oliver Hartkopp wrote:
>>>>  
>>>>      
>>>>> Andrew Morton wrote:
>>>>>           
>>>>>> On Tue, 12 May 2009 11:28:00 +0200 Wolfgang Grandegger
>>>>>> <wg@grandegger.com> wrote:
>>>>>>
>>>>>>  
>>>>>>               
>>>>>>> +int can_restart_now(struct net_device *dev)
>>>>>>> +{
>>>>>>> +    struct can_priv *priv = netdev_priv(dev);
>>>>>>> +    struct net_device_stats *stats = &dev->stats;
>>>>>>> +    struct sk_buff *skb;
>>>>>>> +    struct can_frame *cf;
>>>>>>> +    int err;
>>>>>>> +
>>>>>>> +    /* Synchronize with dev->hard_start_xmit() */
>>>>>>> +    netif_tx_lock(dev);
>>>>>>> +
>>>>>>> +    /* Ensure that no more messages can go out */
>>>>>>> +    if (netif_carrier_ok(dev))
>>>>>>> +        netif_carrier_off(dev);
>>>>>>> +
>>>>>>> +    /* Ensure that no more messages can come in */
>>>>>>> +    err = priv->do_set_mode(dev, CAN_MODE_STOP);
>>>>>>> +    if (err)
>>>>>>> +        return err;
>>>>>>> +
>>>>>>> +    /*  Now it's save to clean up */
>>>>>>> +    del_timer_sync(&priv->restart_timer);
>>>>>>>                         
>>>>>> This is deadlockable.
>>>>>>
>>>>>> It calls del_timer_sync() while holding netif_tx_lock().  But the
>>>>>> timer
>>>>>> handler (can_restart_now()) also takes netif_tx_lock().  So if the
>>>>>> timer handler is presently running, it's sitting there spinning in
>>>>>> netif_tx_lock().  And del_timer_sync() is sitting there waiting
>>>>>> for the
>>>>>> timer handler to complete.
>>>>>>
>>>>>>
>>>>>>                   
>>>>> Hi Wolfgang,
>>>>>
>>>>> would it be an appropriate solution, just to invoke
>>>>>
>>>>> netif_stop_queue() in can_bus_off()
>>>>>
>>>>> and invoke
>>>>>
>>>>> netif_wake_queue() in can_restart_now()
>>>>>
>>>>> ???
>>>>>
>>>>> In a BUSOFF condition we're not able to send CAN frames anyway, so  we
>>>>> can disable the device queue and the we won't  need any
>>>>> netif_tx_lock()
>>>>> right?
>>>>>
>>>>> AFAIK this was the original implementation before some of the latest
>>>>> improvement with the netif_carrier stuff.
>>>>>
>>>>> What do you think?
>>>>>             
>>>> The problem is the "manual" restart triggered via the netlink
>>>> interface,
>>>> which can occur in the middle of ndo_start_xmit().
>>>>
>>>>         
>>> Ah, i see.
>>>
>>> What if the manual restart via netlink would also stop the queue and
>>> start the timer?
>>>     
>>
>> It will not help if the restart is triggered in the middle of
>> ndo_start_xmit().
>>
>>   
> 
> Hi Wolfgang,
> 
> i think, i found a solution that removes the locking problem completely:
> 
> When a bus-off occurs in the controller, the communication on the CAN
> bus can be treated as unusable for this controller (let's say it is dead).
> E.g. the SJA1000 set's its reset bit for that reason and waits to be
> initialized by the CPU again.
> 
> So IMO restarting the CAN controller while in operational state is not a
> valid use case.
>
> When a bus-off (interrupt) occurs, we should
> 
> - invoke netif_carrier_off(dev)
> - invoke netif_stop_queue(dev)
> - set the state to CAN_STATE_BUS_OFF
> 
> and of course create the error message, clear the interrupts(!) and then
> leave the irq service function.
> 
> That's it.

It's already like that.

> When the automatic CAN controller restart is enabled: start the timer.
> 
> For the manual netlink function: Test for CAN_STATE_BUS_OFF (!) and
> invoke the current can_restart_now(dev) or start the timer with e.g. 1ms
> ...
> 
> This approach should make it and fulfills the bus-off intention of the
> CAN controllers ("disabled and wait for re-initialisation").
> 
> And there's no locking of the tx_queue needed anymore as the tx_queue is
> already stopped, when the restart is performed.
> 
> What do you think about this approach?

That's a solution and would comply with the can spec, I believe, but it
should be discussed on the Socket-CAN mailing list. We should not change
policy just to avoid locking or simplify the code.

Wolfgang.

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Jonathan Corbet]

[Quick review ...]

> +/*
> + * CAN device restart for bus-off recovery
> + */
> +int can_restart_now(struct net_device *dev)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	int err;
> +
> +	/* Synchronize with dev->hard_start_xmit() */
> +	netif_tx_lock(dev);
> +
> +	/* Ensure that no more messages can go out */
> +	if (netif_carrier_ok(dev))
> +		netif_carrier_off(dev);
> +
> +	/* Ensure that no more messages can come in */
> +	err = priv->do_set_mode(dev, CAN_MODE_STOP);
> +	if (err)
> +		return err;

This leaves _xmit_lock held and carrier off.  Is that really what you want
to do?

> +
> +	/*  Now it's save to clean up */
> +	del_timer_sync(&priv->restart_timer);
> +	can_flush_echo_skb(dev);
> +
> +	dev_dbg(dev->dev.parent, "restarted\n");
> +	priv->can_stats.restarts++;
> +
> +	/* send restart message upstream */
> +	skb = dev_alloc_skb(sizeof(struct can_frame));
> +	if (skb == NULL)
> +		return -ENOMEM;

...here too...

> +	skb->dev = dev;
> +	skb->protocol = htons(ETH_P_CAN);
> +	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> +	memset(cf, 0, sizeof(struct can_frame));
> +	cf->can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
> +	cf->can_dlc = CAN_ERR_DLC;
> +
> +	netif_rx(skb);
> +
> +	dev->last_rx = jiffies;
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;
> +
> +	/* Now restart the device */
> +	err = priv->do_set_mode(dev, CAN_MODE_START);
> +	if (err)
> +		return err;

...and here too.  Do you maybe want to get rid of the middle-of-function
returns and switch to the "goto out" convention?

> +	netif_carrier_on(dev);
> +
> +	netif_tx_unlock(dev);
> +
> +	return 0;
> +}
> +
> +static void can_restart_after(unsigned long data)
> +{
> +	struct net_device *dev = (struct net_device *)data;
> +
> +	can_restart_now(dev);
> +}

can_restart_after what?  I find myself confused by this function and
wondering why it exists.

jon

Re: [PATCH v2 3/7] [PATCH 3/8] can: CAN Network device driver and Netlink interface

[Wolfgang Grandegger]

Jonathan Corbet wrote:
> [Quick review ...]
> 
>> +/*
>> + * CAN device restart for bus-off recovery
>> + */
>> +int can_restart_now(struct net_device *dev)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	struct sk_buff *skb;
>> +	struct can_frame *cf;
>> +	int err;
>> +
>> +	/* Synchronize with dev->hard_start_xmit() */
>> +	netif_tx_lock(dev);
>> +
>> +	/* Ensure that no more messages can go out */
>> +	if (netif_carrier_ok(dev))
>> +		netif_carrier_off(dev);
>> +
>> +	/* Ensure that no more messages can come in */
>> +	err = priv->do_set_mode(dev, CAN_MODE_STOP);
>> +	if (err)
>> +		return err;
> 
> This leaves _xmit_lock held and carrier off.  Is that really what you want
> to do?
> 
>> +
>> +	/*  Now it's save to clean up */
>> +	del_timer_sync(&priv->restart_timer);
>> +	can_flush_echo_skb(dev);
>> +
>> +	dev_dbg(dev->dev.parent, "restarted\n");
>> +	priv->can_stats.restarts++;
>> +
>> +	/* send restart message upstream */
>> +	skb = dev_alloc_skb(sizeof(struct can_frame));
>> +	if (skb == NULL)
>> +		return -ENOMEM;
> 
> ...here too...
> 
>> +	skb->dev = dev;
>> +	skb->protocol = htons(ETH_P_CAN);
>> +	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>> +	memset(cf, 0, sizeof(struct can_frame));
>> +	cf->can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
>> +	cf->can_dlc = CAN_ERR_DLC;
>> +
>> +	netif_rx(skb);
>> +
>> +	dev->last_rx = jiffies;
>> +	stats->rx_packets++;
>> +	stats->rx_bytes += cf->can_dlc;
>> +
>> +	/* Now restart the device */
>> +	err = priv->do_set_mode(dev, CAN_MODE_START);
>> +	if (err)
>> +		return err;
> 
> ...and here too.  Do you maybe want to get rid of the middle-of-function
> returns and switch to the "goto out" convention?

Right, needs to be fixed.

>> +	netif_carrier_on(dev);
>> +
>> +	netif_tx_unlock(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static void can_restart_after(unsigned long data)
>> +{
>> +	struct net_device *dev = (struct net_device *)data;
>> +
>> +	can_restart_now(dev);
>> +}
> 
> can_restart_after what?  I find myself confused by this function and
> wondering why it exists.

It's just a wrapper to make the compile happy. It's the timer callback
function used here:

          priv->restart_timer.function = can_restart_after;

in contrast to can_restart_now(), which can be called via netlink
interface as well. Would the name "can_restart_callback" be more
appropriate?

Wolfgang.

[PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Wolfgang Grandegger]

[-- Attachment #1: 04-sja1000-driver.patch --]
[-- Type: text/plain, Size: 24699 bytes --]

This patch adds the generic Socket-CAN driver for the Philips SJA1000
full CAN controller.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
---
 drivers/net/can/Kconfig           |    6 
 drivers/net/can/Makefile          |    2 
 drivers/net/can/sja1000/Makefile  |    7 
 drivers/net/can/sja1000/sja1000.c |  645 ++++++++++++++++++++++++++++++++++++++
 drivers/net/can/sja1000/sja1000.h |  177 ++++++++++
 5 files changed, 837 insertions(+)
 create mode 100644 drivers/net/can/sja1000/Makefile
 create mode 100644 drivers/net/can/sja1000/sja1000.c
 create mode 100644 drivers/net/can/sja1000/sja1000.h

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================
--- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:25.061720603 +0200
+++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:25.747720647 +0200
@@ -35,6 +35,12 @@
 	  arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
 	  If unsure, say Y.
 
+config CAN_SJA1000
+	depends on CAN_DEV
+	tristate "Philips SJA1000"
+	---help---
+	  Driver for the SJA1000 CAN controllers from Philips or NXP
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN
Index: net-next-2.6/drivers/net/can/Makefile
===================================================================
--- net-next-2.6.orig/drivers/net/can/Makefile	2009-05-12 10:47:25.061720603 +0200
+++ net-next-2.6/drivers/net/can/Makefile	2009-05-12 10:47:25.748720510 +0200
@@ -7,4 +7,6 @@
 obj-$(CONFIG_CAN_DEV)		+= can-dev.o
 can-dev-y			:= dev.o
 
+obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
+
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:25.753720385 +0200
@@ -0,0 +1,7 @@
+#
+#  Makefile for the SJA1000 CAN controller drivers.
+#
+
+obj-$(CONFIG_CAN_SJA1000) += sja1000.o
+
+ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
Index: net-next-2.6/drivers/net/can/sja1000/sja1000.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/drivers/net/can/sja1000/sja1000.c	2009-05-12 10:47:25.756720813 +0200
@@ -0,0 +1,645 @@
+/*
+ * sja1000.c -  Philips SJA1000 network device driver
+ *
+ * Copyright (c) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
+ * 38106 Braunschweig, GERMANY
+ *
+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of Volkswagen nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * Alternatively, provided that this notice is retained in full, this
+ * software may be distributed under the terms of the GNU General
+ * Public License ("GPL") version 2, in which case the provisions of the
+ * GPL apply INSTEAD OF those given above.
+ *
+ * The provided data structures and external interfaces from this code
+ * are not restricted to be used by modules with a GPL compatible license.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ *
+ * Send feedback to <socketcan-users@lists.berlios.de>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <linux/fcntl.h>
+#include <linux/interrupt.h>
+#include <linux/ptrace.h>
+#include <linux/string.h>
+#include <linux/errno.h>
+#include <linux/netdevice.h>
+#include <linux/if_arp.h>
+#include <linux/if_ether.h>
+#include <linux/skbuff.h>
+#include <linux/delay.h>
+
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+#include <linux/can/dev.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME "sja1000"
+
+MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION(DRV_NAME "CAN netdevice driver");
+
+static struct can_bittiming_const sja1000_bittiming_const = {
+	.name = DRV_NAME,
+	.tseg1_min = 1,
+	.tseg1_max = 16,
+	.tseg2_min = 1,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 64,
+	.brp_inc = 1,
+};
+
+static int sja1000_probe_chip(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	if (dev->base_addr && (priv->read_reg(dev, 0) == 0xFF)) {
+		printk(KERN_INFO "%s: probing @0x%lX failed\n",
+		       DRV_NAME, dev->base_addr);
+		return 0;
+	}
+	return 1;
+}
+
+static int set_reset_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	/* disable interrupts */
+	priv->write_reg(dev, REG_IER, IRQ_OFF);
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if (status & MOD_RM) {
+			priv->can.state = CAN_STATE_STOPPED;
+			return 0;
+		}
+
+		priv->write_reg(dev, REG_MOD, MOD_RM);	/* reset chip */
+		status = priv->read_reg(dev, REG_MOD);
+		udelay(10);
+	}
+
+	dev_err(dev->dev.parent, "setting SJA1000 into reset mode failed!\n");
+	return 1;
+
+}
+
+static int set_normal_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if ((status & MOD_RM) == 0) {
+			priv->can.state = CAN_STATE_ERROR_ACTIVE;
+			/* enable all interrupts */
+			priv->write_reg(dev, REG_IER, IRQ_ALL);
+
+			return 0;
+		}
+
+		/* set chip to normal mode */
+		priv->write_reg(dev, REG_MOD, 0x00);
+		status = priv->read_reg(dev, REG_MOD);
+		udelay(10);
+	}
+
+	dev_err(dev->dev.parent, "setting SJA1000 into normal mode failed!\n");
+	return 1;
+
+}
+
+static void sja1000_start(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	/* leave reset mode */
+	if (priv->can.state != CAN_STATE_STOPPED)
+		set_reset_mode(dev);
+
+	/* Clear error counters and error code capture */
+	priv->write_reg(dev, REG_TXERR, 0x0);
+	priv->write_reg(dev, REG_RXERR, 0x0);
+	priv->read_reg(dev, REG_ECC);
+
+	/* leave reset mode */
+	set_normal_mode(dev);
+}
+
+static int sja1000_set_mode(struct net_device *dev, enum can_mode mode)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	if (!priv->open_time)
+		return -EINVAL;
+
+	switch (mode) {
+	case CAN_MODE_START:
+		sja1000_start(dev);
+		if (netif_queue_stopped(dev))
+			netif_wake_queue(dev);
+		break;
+
+	case CAN_MODE_STOP:
+		if (priv->can.state != CAN_STATE_STOPPED)
+			set_reset_mode(dev);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int sja1000_set_bittiming(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	u8 btr0, btr1;
+
+	btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
+	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
+		(((bt->phase_seg2 - 1) & 0x7) << 4);
+	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		btr1 |= 0x80;
+
+	dev_info(dev->dev.parent,
+		 "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1);
+
+	priv->write_reg(dev, REG_BTR0, btr0);
+	priv->write_reg(dev, REG_BTR1, btr1);
+
+	return 0;
+}
+
+/*
+ * initialize SJA1000 chip:
+ *   - reset chip
+ *   - set output mode
+ *   - set baudrate
+ *   - enable interrupts
+ *   - start operating mode
+ */
+static void chipset_init(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	/* set clock divider and output control register */
+	priv->write_reg(dev, REG_CDR, priv->cdr | CDR_PELICAN);
+
+	/* set acceptance filter (accept all) */
+	priv->write_reg(dev, REG_ACCC0, 0x00);
+	priv->write_reg(dev, REG_ACCC1, 0x00);
+	priv->write_reg(dev, REG_ACCC2, 0x00);
+	priv->write_reg(dev, REG_ACCC3, 0x00);
+
+	priv->write_reg(dev, REG_ACCM0, 0xFF);
+	priv->write_reg(dev, REG_ACCM1, 0xFF);
+	priv->write_reg(dev, REG_ACCM2, 0xFF);
+	priv->write_reg(dev, REG_ACCM3, 0xFF);
+
+	priv->write_reg(dev, REG_OCR, priv->ocr | OCR_MODE_NORMAL);
+}
+
+/*
+ * transmit a CAN message
+ * message layout in the sk_buff should be like this:
+ * xx xx xx xx	 ff	 ll   00 11 22 33 44 55 66 77
+ * [  can-id ] [flags] [len] [can data (up to 8 bytes]
+ */
+static int sja1000_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	uint8_t fi;
+	uint8_t dlc;
+	canid_t id;
+	uint8_t dreg;
+	int i;
+
+	netif_stop_queue(dev);
+
+	fi = dlc = cf->can_dlc;
+	id = cf->can_id;
+
+	if (id & CAN_RTR_FLAG)
+		fi |= FI_RTR;
+
+	if (id & CAN_EFF_FLAG) {
+		fi |= FI_FF;
+		dreg = EFF_BUF;
+		priv->write_reg(dev, REG_FI, fi);
+		priv->write_reg(dev, REG_ID1, (id & 0x1fe00000) >> (5 + 16));
+		priv->write_reg(dev, REG_ID2, (id & 0x001fe000) >> (5 + 8));
+		priv->write_reg(dev, REG_ID3, (id & 0x00001fe0) >> 5);
+		priv->write_reg(dev, REG_ID4, (id & 0x0000001f) << 3);
+	} else {
+		dreg = SFF_BUF;
+		priv->write_reg(dev, REG_FI, fi);
+		priv->write_reg(dev, REG_ID1, (id & 0x000007f8) >> 3);
+		priv->write_reg(dev, REG_ID2, (id & 0x00000007) << 5);
+	}
+
+	for (i = 0; i < dlc; i++)
+		priv->write_reg(dev, dreg++, cf->data[i]);
+
+	stats->tx_bytes += dlc;
+	dev->trans_start = jiffies;
+
+	can_put_echo_skb(skb, dev, 0);
+
+	priv->write_reg(dev, REG_CMR, CMD_TR);
+
+	return 0;
+}
+
+static void sja1000_rx(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	uint8_t fi;
+	uint8_t dreg;
+	canid_t id;
+	uint8_t dlc;
+	int i;
+
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (skb == NULL)
+		return;
+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_CAN);
+
+	fi = priv->read_reg(dev, REG_FI);
+	dlc = fi & 0x0F;
+
+	if (fi & FI_FF) {
+		/* extended frame format (EFF) */
+		dreg = EFF_BUF;
+		id = (priv->read_reg(dev, REG_ID1) << (5 + 16))
+		    | (priv->read_reg(dev, REG_ID2) << (5 + 8))
+		    | (priv->read_reg(dev, REG_ID3) << 5)
+		    | (priv->read_reg(dev, REG_ID4) >> 3);
+		id |= CAN_EFF_FLAG;
+	} else {
+		/* standard frame format (SFF) */
+		dreg = SFF_BUF;
+		id = (priv->read_reg(dev, REG_ID1) << 3)
+		    | (priv->read_reg(dev, REG_ID2) >> 5);
+	}
+
+	if (fi & FI_RTR)
+		id |= CAN_RTR_FLAG;
+
+	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
+	memset(cf, 0, sizeof(struct can_frame));
+	cf->can_id = id;
+	cf->can_dlc = dlc;
+	for (i = 0; i < dlc; i++)
+		cf->data[i] = priv->read_reg(dev, dreg++);
+
+	while (i < 8)
+		cf->data[i++] = 0;
+
+	/* release receive buffer */
+	priv->write_reg(dev, REG_CMR, CMD_RRB);
+
+	netif_rx(skb);
+
+	dev->last_rx = jiffies;
+	stats->rx_packets++;
+	stats->rx_bytes += dlc;
+}
+
+static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	enum can_state state = priv->can.state;
+	uint8_t ecc, alc;
+
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (skb == NULL)
+		return -ENOMEM;
+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_CAN);
+	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
+	memset(cf, 0, sizeof(struct can_frame));
+	cf->can_id = CAN_ERR_FLAG;
+	cf->can_dlc = CAN_ERR_DLC;
+
+	if (isrc & IRQ_DOI) {
+		/* data overrun interrupt */
+		dev_dbg(dev->dev.parent, "data overrun interrupt\n");
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+		stats->rx_over_errors++;
+		stats->rx_errors++;
+		priv->write_reg(dev, REG_CMR, CMD_CDO);	/* clear bit */
+	}
+
+	if (isrc & IRQ_EI) {
+		/* error warning interrupt */
+		dev_dbg(dev->dev.parent, "error warning interrupt\n");
+
+		if (status & SR_BS) {
+			state = CAN_STATE_BUS_OFF;
+			cf->can_id |= CAN_ERR_BUSOFF;
+			can_bus_off(dev);
+		} else if (status & SR_ES) {
+			state = CAN_STATE_ERROR_WARNING;
+		} else
+			state = CAN_STATE_ERROR_ACTIVE;
+	}
+	if (isrc & IRQ_BEI) {
+		/* bus error interrupt */
+		priv->can.can_stats.bus_error++;
+		stats->rx_errors++;
+
+		ecc = priv->read_reg(dev, REG_ECC);
+
+		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+		switch (ecc & ECC_MASK) {
+		case ECC_BIT:
+			cf->data[2] |= CAN_ERR_PROT_BIT;
+			break;
+		case ECC_FORM:
+			cf->data[2] |= CAN_ERR_PROT_FORM;
+			break;
+		case ECC_STUFF:
+			cf->data[2] |= CAN_ERR_PROT_STUFF;
+			break;
+		default:
+			cf->data[2] |= CAN_ERR_PROT_UNSPEC;
+			cf->data[3] = ecc & ECC_SEG;
+			break;
+		}
+		/* Error occured during transmission? */
+		if ((ecc & ECC_DIR) == 0)
+			cf->data[2] |= CAN_ERR_PROT_TX;
+	}
+	if (isrc & IRQ_EPI) {
+		/* error passive interrupt */
+		dev_dbg(dev->dev.parent, "error passive interrupt\n");
+		if (status & SR_ES)
+			state = CAN_STATE_ERROR_PASSIVE;
+		else
+			state = CAN_STATE_ERROR_ACTIVE;
+	}
+	if (isrc & IRQ_ALI) {
+		/* arbitration lost interrupt */
+		dev_dbg(dev->dev.parent, "arbitration lost interrupt\n");
+		alc = priv->read_reg(dev, REG_ALC);
+		priv->can.can_stats.arbitration_lost++;
+		stats->rx_errors++;
+		cf->can_id |= CAN_ERR_LOSTARB;
+		cf->data[0] = alc & 0x1f;
+	}
+
+	if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
+					 state == CAN_STATE_ERROR_PASSIVE)) {
+		uint8_t rxerr = priv->read_reg(dev, REG_RXERR);
+		uint8_t txerr = priv->read_reg(dev, REG_TXERR);
+		cf->can_id |= CAN_ERR_CRTL;
+		if (state == CAN_STATE_ERROR_WARNING) {
+			priv->can.can_stats.error_warning++;
+			cf->data[1] = (txerr > rxerr) ?
+				CAN_ERR_CRTL_TX_WARNING :
+				CAN_ERR_CRTL_RX_WARNING;
+		} else {
+			priv->can.can_stats.error_passive++;
+			cf->data[1] = (txerr > rxerr) ?
+				CAN_ERR_CRTL_TX_PASSIVE :
+				CAN_ERR_CRTL_RX_PASSIVE;
+		}
+	}
+
+	priv->can.state = state;
+
+	netif_rx(skb);
+
+	dev->last_rx = jiffies;
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+
+	return 0;
+}
+
+irqreturn_t sja1000_interrupt(int irq, void *dev_id)
+{
+	struct net_device *dev = (struct net_device *)dev_id;
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	uint8_t isrc, status;
+	int n = 0;
+
+	/* Shared interrupts and IRQ off? */
+	if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
+		return IRQ_NONE;
+
+	if (priv->pre_irq)
+		priv->pre_irq(dev);
+
+	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
+		n++;
+		status = priv->read_reg(dev, REG_SR);
+
+		if (isrc & IRQ_WUI)
+			dev_warn(dev->dev.parent, "wakeup interrupt\n");
+
+		if (isrc & IRQ_TI) {
+			/* transmission complete interrupt */
+			stats->tx_packets++;
+			can_get_echo_skb(dev, 0);
+			netif_wake_queue(dev);
+		}
+		if (isrc & IRQ_RI) {
+			/* receive interrupt */
+			while (status & SR_RBS) {
+				sja1000_rx(dev);
+				status = priv->read_reg(dev, REG_SR);
+			}
+		}
+		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
+			/* error interrupt */
+			if (sja1000_err(dev, isrc, status))
+				break;
+		}
+	}
+
+	if (priv->post_irq)
+		priv->post_irq(dev);
+
+	if (n >= SJA1000_MAX_IRQ)
+		dev_dbg(dev->dev.parent, "%d messages handled in ISR", n);
+
+	return (n) ? IRQ_HANDLED : IRQ_NONE;
+}
+EXPORT_SYMBOL_GPL(sja1000_interrupt);
+
+static int sja1000_open(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	int err;
+
+	/* set chip into reset mode */
+	set_reset_mode(dev);
+
+	/* common open */
+	err = open_candev(dev);
+	if (err)
+		return err;
+
+	/* register interrupt handler, if not done by the device driver */
+	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER)) {
+		err = request_irq(dev->irq, &sja1000_interrupt, IRQF_SHARED,
+				  dev->name, (void *)dev);
+		if (err)
+			return -EAGAIN;
+	}
+
+	/* init and start chi */
+	sja1000_start(dev);
+	priv->open_time = jiffies;
+
+	netif_start_queue(dev);
+
+	return 0;
+}
+
+static int sja1000_close(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	netif_stop_queue(dev);
+	set_reset_mode(dev);
+
+	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER))
+		free_irq(dev->irq, (void *)dev);
+
+	close_candev(dev);
+
+	priv->open_time = 0;
+
+	return 0;
+}
+
+struct net_device *alloc_sja1000dev(int sizeof_priv)
+{
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+
+	dev = alloc_candev(sizeof(struct sja1000_priv) + sizeof_priv);
+	if (!dev)
+		return NULL;
+
+	priv = netdev_priv(dev);
+
+	priv->dev = dev;
+	priv->can.bittiming_const = &sja1000_bittiming_const;
+	priv->can.do_set_bittiming = sja1000_set_bittiming;
+	priv->can.do_set_mode = sja1000_set_mode;
+
+	if (sizeof_priv)
+		priv->priv = (void *)priv + sizeof(struct sja1000_priv);
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(alloc_sja1000dev);
+
+void free_sja1000dev(struct net_device *dev)
+{
+	free_candev(dev);
+}
+EXPORT_SYMBOL_GPL(free_sja1000dev);
+
+static const struct net_device_ops sja1000_netdev_ops = {
+       .ndo_open               = sja1000_open,
+       .ndo_stop               = sja1000_close,
+       .ndo_start_xmit         = sja1000_start_xmit,
+};
+
+int register_sja1000dev(struct net_device *dev)
+{
+	if (!sja1000_probe_chip(dev))
+		return -ENODEV;
+
+	dev->flags |= IFF_ECHO;	/* we support local echo */
+	dev->netdev_ops = &sja1000_netdev_ops;
+
+	set_reset_mode(dev);
+	chipset_init(dev);
+
+	return register_candev(dev);
+}
+EXPORT_SYMBOL_GPL(register_sja1000dev);
+
+void unregister_sja1000dev(struct net_device *dev)
+{
+	set_reset_mode(dev);
+	unregister_candev(dev);
+}
+EXPORT_SYMBOL_GPL(unregister_sja1000dev);
+
+static __init int sja1000_init(void)
+{
+	printk(KERN_INFO "%s CAN netdevice driver\n", DRV_NAME);
+
+	return 0;
+}
+
+module_init(sja1000_init);
+
+static __exit void sja1000_exit(void)
+{
+	printk(KERN_INFO "%s: driver removed\n", DRV_NAME);
+}
+
+module_exit(sja1000_exit);
Index: net-next-2.6/drivers/net/can/sja1000/sja1000.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/drivers/net/can/sja1000/sja1000.h	2009-05-12 10:47:25.759970718 +0200
@@ -0,0 +1,177 @@
+/*
+ * sja1000.h -  Philips SJA1000 network device driver
+ *
+ * Copyright (c) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
+ * 38106 Braunschweig, GERMANY
+ *
+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of Volkswagen nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * Alternatively, provided that this notice is retained in full, this
+ * software may be distributed under the terms of the GNU General
+ * Public License ("GPL") version 2, in which case the provisions of the
+ * GPL apply INSTEAD OF those given above.
+ *
+ * The provided data structures and external interfaces from this code
+ * are not restricted to be used by modules with a GPL compatible license.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ *
+ * Send feedback to <socketcan-users@lists.berlios.de>
+ *
+ */
+
+#ifndef SJA1000_DEV_H
+#define SJA1000_DEV_H
+
+#include <linux/can/dev.h>
+#include <linux/can/platform/sja1000.h>
+
+#define SJA1000_MAX_IRQ 20	/* max. number of interrupts handled in ISR */
+
+/* SJA1000 registers - manual section 6.4 (Pelican Mode) */
+#define REG_MOD		0x00
+#define REG_CMR		0x01
+#define REG_SR		0x02
+#define REG_IR		0x03
+#define REG_IER		0x04
+#define REG_ALC		0x0B
+#define REG_ECC		0x0C
+#define REG_EWL		0x0D
+#define REG_RXERR	0x0E
+#define REG_TXERR	0x0F
+#define REG_ACCC0	0x10
+#define REG_ACCC1	0x11
+#define REG_ACCC2	0x12
+#define REG_ACCC3	0x13
+#define REG_ACCM0	0x14
+#define REG_ACCM1	0x15
+#define REG_ACCM2	0x16
+#define REG_ACCM3	0x17
+#define REG_RMC		0x1D
+#define REG_RBSA	0x1E
+
+/* Common registers - manual section 6.5 */
+#define REG_BTR0	0x06
+#define REG_BTR1	0x07
+#define REG_OCR		0x08
+#define REG_CDR		0x1F
+
+#define REG_FI		0x10
+#define SFF_BUF		0x13
+#define EFF_BUF		0x15
+
+#define FI_FF		0x80
+#define FI_RTR		0x40
+
+#define REG_ID1		0x11
+#define REG_ID2		0x12
+#define REG_ID3		0x13
+#define REG_ID4		0x14
+
+#define CAN_RAM		0x20
+
+/* mode register */
+#define MOD_RM		0x01
+#define MOD_LOM		0x02
+#define MOD_STM		0x04
+#define MOD_AFM		0x08
+#define MOD_SM		0x10
+
+/* commands */
+#define CMD_SRR		0x10
+#define CMD_CDO		0x08
+#define CMD_RRB		0x04
+#define CMD_AT		0x02
+#define CMD_TR		0x01
+
+/* interrupt sources */
+#define IRQ_BEI		0x80
+#define IRQ_ALI		0x40
+#define IRQ_EPI		0x20
+#define IRQ_WUI		0x10
+#define IRQ_DOI		0x08
+#define IRQ_EI		0x04
+#define IRQ_TI		0x02
+#define IRQ_RI		0x01
+#define IRQ_ALL		0xFF
+#define IRQ_OFF		0x00
+
+/* status register content */
+#define SR_BS		0x80
+#define SR_ES		0x40
+#define SR_TS		0x20
+#define SR_RS		0x10
+#define SR_TCS		0x08
+#define SR_TBS		0x04
+#define SR_DOS		0x02
+#define SR_RBS		0x01
+
+#define SR_CRIT (SR_BS|SR_ES)
+
+/* ECC register */
+#define ECC_SEG		0x1F
+#define ECC_DIR		0x20
+#define ECC_ERR		6
+#define ECC_BIT		0x00
+#define ECC_FORM	0x40
+#define ECC_STUFF	0x80
+#define ECC_MASK	0xc0
+
+/*
+ * Flags for sja1000priv.flags
+ */
+#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
+
+/*
+ * SJA1000 private data structure
+ */
+struct sja1000_priv {
+	struct can_priv can;
+	long open_time;
+	struct sk_buff *echo_skb;
+
+	u8 (*read_reg) (const struct net_device *dev, int reg);
+	void (*write_reg) (const struct net_device *dev, int reg, u8 val);
+	void (*pre_irq) (const struct net_device *dev);
+	void (*post_irq) (const struct net_device *dev);
+
+	void *priv;		/* for board-specific data */
+	struct net_device *dev;
+
+	u8 ocr;
+	u8 cdr;
+	u32 flags;
+};
+
+struct net_device *alloc_sja1000dev(int sizeof_priv);
+void free_sja1000dev(struct net_device *dev);
+int register_sja1000dev(struct net_device *dev);
+void unregister_sja1000dev(struct net_device *dev);
+
+irqreturn_t sja1000_interrupt(int irq, void *dev_id);
+
+#endif /* SJA1000_DEV_H */

Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Jonathan Corbet]

[Quick drive-by review continues...]

> +
> +static int sja1000_probe_chip(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);

Looking down toward the bottom, I see:

> +struct sja1000_priv {
> +	struct can_priv can;

So you're still using the "put the higher-level structure at the top so we
can treat it like either kind of pointer" trick.  I'd still recommend
against that.  Far better to do something like:

	struct can_priv *canpriv = netdev_priv(dev);
	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);

Of course, you can put that dance into a helper function.

> +	if (dev->base_addr && (priv->read_reg(dev, 0) == 0xFF)) {
> +		printk(KERN_INFO "%s: probing @0x%lX failed\n",
> +		       DRV_NAME, dev->base_addr);
> +		return 0;
> +	}
> +	return 1;
> +}

So zero is an error return?  That's contrary to usual practice.

> +static int set_reset_mode(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	unsigned char status = priv->read_reg(dev, REG_MOD);
> +	int i;
> +
> +	/* disable interrupts */
> +	priv->write_reg(dev, REG_IER, IRQ_OFF);
> +
> +	for (i = 0; i < 100; i++) {
> +		/* check reset bit */
> +		if (status & MOD_RM) {
> +			priv->can.state = CAN_STATE_STOPPED;
> +			return 0;
> +		}
> +
> +		priv->write_reg(dev, REG_MOD, MOD_RM);	/* reset chip */
> +		status = priv->read_reg(dev, REG_MOD);
> +		udelay(10);

Wouldn't you want to read the new state *after* the delay?

> +	}
> +
> +	dev_err(dev->dev.parent, "setting SJA1000 into reset mode failed!\n");
> +	return 1;
> +
> +}
> +
> +static int set_normal_mode(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	unsigned char status = priv->read_reg(dev, REG_MOD);
> +	int i;
> +
> +	for (i = 0; i < 100; i++) {
> +		/* check reset bit */
> +		if ((status & MOD_RM) == 0) {
> +			priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +			/* enable all interrupts */
> +			priv->write_reg(dev, REG_IER, IRQ_ALL);
> +
> +			return 0;
> +		}
> +
> +		/* set chip to normal mode */
> +		priv->write_reg(dev, REG_MOD, 0x00);
> +		status = priv->read_reg(dev, REG_MOD);
> +		udelay(10);

Here too?

> +	}
> +
> +	dev_err(dev->dev.parent, "setting SJA1000 into normal mode failed!\n");
> +	return 1;
> +
> +}
> +

[...]

> +irqreturn_t sja1000_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *dev = (struct net_device *)dev_id;
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	uint8_t isrc, status;
> +	int n = 0;
> +
> +	/* Shared interrupts and IRQ off? */
> +	if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
> +		return IRQ_NONE;
> +
> +	if (priv->pre_irq)
> +		priv->pre_irq(dev);
> +
> +	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
> +		n++;
> +		status = priv->read_reg(dev, REG_SR);
> +
> +		if (isrc & IRQ_WUI)
> +			dev_warn(dev->dev.parent, "wakeup interrupt\n");

How many of these might you get?  Should this be rate limited?

> +		if (isrc & IRQ_TI) {
> +			/* transmission complete interrupt */
> +			stats->tx_packets++;
> +			can_get_echo_skb(dev, 0);
> +			netif_wake_queue(dev);
> +		}
> +		if (isrc & IRQ_RI) {
> +			/* receive interrupt */
> +			while (status & SR_RBS) {
> +				sja1000_rx(dev);
> +				status = priv->read_reg(dev, REG_SR);
> +			}
> +		}
> +		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
> +			/* error interrupt */
> +			if (sja1000_err(dev, isrc, status))
> +				break;
> +		}
> +	}
> +
> +	if (priv->post_irq)
> +		priv->post_irq(dev);
> +
> +	if (n >= SJA1000_MAX_IRQ)
> +		dev_dbg(dev->dev.parent, "%d messages handled in ISR", n);
> +
> +	return (n) ? IRQ_HANDLED : IRQ_NONE;
> +}
> +EXPORT_SYMBOL_GPL(sja1000_interrupt);
> +
> +static int sja1000_open(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	int err;
> +
> +	/* set chip into reset mode */
> +	set_reset_mode(dev);
> +
> +	/* common open */
> +	err = open_candev(dev);
> +	if (err)
> +		return err;
> +
> +	/* register interrupt handler, if not done by the device driver */
> +	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER)) {
> +		err = request_irq(dev->irq, &sja1000_interrupt, IRQF_SHARED,
> +				  dev->name, (void *)dev);
> +		if (err)
> +			return -EAGAIN;

If you return here you fail, but you've not undone open_candev().  Looking
there, it seems no harm will be done - until somebody changes open_candev()
someday. 

> +	}
> +
> +	/* init and start chi */
> +	sja1000_start(dev);
> +	priv->open_time = jiffies;
> +
> +	netif_start_queue(dev);
> +
> +	return 0;
> +}
> +

[...]

> +/*
> + * SJA1000 private data structure
> + */
> +struct sja1000_priv {
> +	struct can_priv can;
> +	long open_time;
> +	struct sk_buff *echo_skb;
> +
> +	u8 (*read_reg) (const struct net_device *dev, int reg);
> +	void (*write_reg) (const struct net_device *dev, int reg, u8 val);
> +	void (*pre_irq) (const struct net_device *dev);
> +	void (*post_irq) (const struct net_device *dev);

What are the locking rules for functions like ->read_reg() now?  Entirely
up to the lower level?  Would be good to document that near the structure
definition. 

> +
> +	void *priv;		/* for board-specific data */
> +	struct net_device *dev;
> +
> +	u8 ocr;
> +	u8 cdr;
> +	u32 flags;

The meaning of these fields is not exactly clear.

> +};

jon

Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Wolfgang Grandegger]

Jonathan Corbet wrote:
> [Quick drive-by review continues...]
> 
>> +
>> +static int sja1000_probe_chip(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
> 
> Looking down toward the bottom, I see:
> 
>> +struct sja1000_priv {
>> +	struct can_priv can;
> 
> So you're still using the "put the higher-level structure at the top so we
> can treat it like either kind of pointer" trick.  I'd still recommend
> against that.  Far better to do something like:
> 
> 	struct can_priv *canpriv = netdev_priv(dev);
> 	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);
> 
> Of course, you can put that dance into a helper function.

There is no way to initialize the value returned by netdev_priv() as it
does not point to a member of struct net_device. I already commented here:

  http://marc.info/?l=linux-netdev&m=124120212106891&w=2

Have I missed something?

>> +	if (dev->base_addr && (priv->read_reg(dev, 0) == 0xFF)) {
>> +		printk(KERN_INFO "%s: probing @0x%lX failed\n",
>> +		       DRV_NAME, dev->base_addr);
>> +		return 0;
>> +	}
>> +	return 1;
>> +}
> 
> So zero is an error return?  That's contrary to usual practice.

OK, will fix.

>> +static int set_reset_mode(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	unsigned char status = priv->read_reg(dev, REG_MOD);
>> +	int i;
>> +
>> +	/* disable interrupts */
>> +	priv->write_reg(dev, REG_IER, IRQ_OFF);
>> +
>> +	for (i = 0; i < 100; i++) {
>> +		/* check reset bit */
>> +		if (status & MOD_RM) {
>> +			priv->can.state = CAN_STATE_STOPPED;
>> +			return 0;
>> +		}
>> +
>> +		priv->write_reg(dev, REG_MOD, MOD_RM);	/* reset chip */
>> +		status = priv->read_reg(dev, REG_MOD);
>> +		udelay(10);
> 
> Wouldn't you want to read the new state *after* the delay?

Yes, that makes more sense.

>> +	}
>> +
>> +	dev_err(dev->dev.parent, "setting SJA1000 into reset mode failed!\n");
>> +	return 1;

Will fix this return value as well.

>> +
>> +}
>> +
>> +static int set_normal_mode(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	unsigned char status = priv->read_reg(dev, REG_MOD);
>> +	int i;
>> +
>> +	for (i = 0; i < 100; i++) {
>> +		/* check reset bit */
>> +		if ((status & MOD_RM) == 0) {
>> +			priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +			/* enable all interrupts */
>> +			priv->write_reg(dev, REG_IER, IRQ_ALL);
>> +
>> +			return 0;
>> +		}
>> +
>> +		/* set chip to normal mode */
>> +		priv->write_reg(dev, REG_MOD, 0x00);
>> +		status = priv->read_reg(dev, REG_MOD);
>> +		udelay(10);
> 
> Here too?
> 
>> +	}
>> +
>> +	dev_err(dev->dev.parent, "setting SJA1000 into normal mode failed!\n");
>> +	return 1;
>> +
>> +}
>> +
> 
> [...]
> 
>> +irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>> +{
>> +	struct net_device *dev = (struct net_device *)dev_id;
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	uint8_t isrc, status;
>> +	int n = 0;
>> +
>> +	/* Shared interrupts and IRQ off? */
>> +	if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
>> +		return IRQ_NONE;
>> +
>> +	if (priv->pre_irq)
>> +		priv->pre_irq(dev);
>> +
>> +	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
>> +		n++;
>> +		status = priv->read_reg(dev, REG_SR);
>> +
>> +		if (isrc & IRQ_WUI)
>> +			dev_warn(dev->dev.parent, "wakeup interrupt\n");
> 
> How many of these might you get?  Should this be rate limited?

None, because the driver does not (yet) support the sleep mode. For that
reason it's a warning.

>> +		if (isrc & IRQ_TI) {
>> +			/* transmission complete interrupt */
>> +			stats->tx_packets++;
>> +			can_get_echo_skb(dev, 0);
>> +			netif_wake_queue(dev);
>> +		}
>> +		if (isrc & IRQ_RI) {
>> +			/* receive interrupt */
>> +			while (status & SR_RBS) {
>> +				sja1000_rx(dev);
>> +				status = priv->read_reg(dev, REG_SR);
>> +			}
>> +		}
>> +		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
>> +			/* error interrupt */
>> +			if (sja1000_err(dev, isrc, status))
>> +				break;
>> +		}
>> +	}
>> +
>> +	if (priv->post_irq)
>> +		priv->post_irq(dev);
>> +
>> +	if (n >= SJA1000_MAX_IRQ)
>> +		dev_dbg(dev->dev.parent, "%d messages handled in ISR", n);
>> +
>> +	return (n) ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +EXPORT_SYMBOL_GPL(sja1000_interrupt);
>> +
>> +static int sja1000_open(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	int err;
>> +
>> +	/* set chip into reset mode */
>> +	set_reset_mode(dev);
>> +
>> +	/* common open */
>> +	err = open_candev(dev);
>> +	if (err)
>> +		return err;
>> +
>> +	/* register interrupt handler, if not done by the device driver */
>> +	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER)) {
>> +		err = request_irq(dev->irq, &sja1000_interrupt, IRQF_SHARED,
>> +				  dev->name, (void *)dev);
>> +		if (err)
>> +			return -EAGAIN;
> 
> If you return here you fail, but you've not undone open_candev().  Looking
> there, it seems no harm will be done - until somebody changes open_candev()
> someday. 

Right, the missing close_candev() does currently not harm but that might
change in the future. Will change.

>> +	}
>> +
>> +	/* init and start chi */
>> +	sja1000_start(dev);
>> +	priv->open_time = jiffies;
>> +
>> +	netif_start_queue(dev);
>> +
>> +	return 0;
>> +}
>> +
> 
> [...]
> 
>> +/*
>> + * SJA1000 private data structure
>> + */
>> +struct sja1000_priv {
>> +	struct can_priv can;
>> +	long open_time;
>> +	struct sk_buff *echo_skb;
>> +
>> +	u8 (*read_reg) (const struct net_device *dev, int reg);
>> +	void (*write_reg) (const struct net_device *dev, int reg, u8 val);
>> +	void (*pre_irq) (const struct net_device *dev);
>> +	void (*post_irq) (const struct net_device *dev);
> 
> What are the locking rules for functions like ->read_reg() now?  Entirely
> up to the lower level?  Would be good to document that near the structure
> definition. 

Yes, it's up to the lower level.

>> +
>> +	void *priv;		/* for board-specific data */
>> +	struct net_device *dev;
>> +
>> +	u8 ocr;
>> +	u8 cdr;
>> +	u32 flags;
> 
> The meaning of these fields is not exactly clear.

I will add a description.

Wolfgang.

Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Jonathan Corbet]

On Thu, 14 May 2009 11:03:53 +0200
Wolfgang Grandegger <wg@grandegger.com> wrote:

> > So you're still using the "put the higher-level structure at the top so we
> > can treat it like either kind of pointer" trick.  I'd still recommend
> > against that.  Far better to do something like:
> > 
> > 	struct can_priv *canpriv = netdev_priv(dev);
> > 	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);
> > 
> > Of course, you can put that dance into a helper function.  
> 
> There is no way to initialize the value returned by netdev_priv() as it
> does not point to a member of struct net_device. I already commented here:
> 
>   http://marc.info/?l=linux-netdev&m=124120212106891&w=2
> 
> Have I missed something?

I'm confused.  It points to the struct can_priv that you registered at
the beginning.  Since that structure is contained within struct
sja1000_priv, you can use container_of(), as described above, to get
it.

I would probably just write something like:

static inline struct sja1000_priv *to_sja1000_priv(struct net_device *dev)
{
	return container_of(netdev_priv(dev), struct sja1000_priv, can);
}

So have *I* missed something?

jon

Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Wolfgang Grandegger]

Jonathan Corbet wrote:
> On Thu, 14 May 2009 11:03:53 +0200
> Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
>>> So you're still using the "put the higher-level structure at the top so we
>>> can treat it like either kind of pointer" trick.  I'd still recommend
>>> against that.  Far better to do something like:
>>>
>>> 	struct can_priv *canpriv = netdev_priv(dev);
>>> 	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);
>>>
>>> Of course, you can put that dance into a helper function.  
>> There is no way to initialize the value returned by netdev_priv() as it
>> does not point to a member of struct net_device. I already commented here:
>>
>>   http://marc.info/?l=linux-netdev&m=124120212106891&w=2
>>
>> Have I missed something?
> 
> I'm confused.  It points to the struct can_priv that you registered at
> the beginning.  Since that structure is contained within struct
> sja1000_priv, you can use container_of(), as described above, to get
> it.
> 
> I would probably just write something like:
> 
> static inline struct sja1000_priv *to_sja1000_priv(struct net_device *dev)
> {
> 	return container_of(netdev_priv(dev), struct sja1000_priv, can);
> }
> 
> So have *I* missed something?

Furthermore, the higher layer needs to known the location of the member
"struct sja1000_priv can", e.g. by defining:

  dev->priv = &dev_specific_priv->can;

But "struct net_device" does not have a "priv" member. netdev_priv(dev)
always points to the beginning of the private data area. See:

   http://lxr.linux.no/linux+v2.6.29/include/linux/netdevice.h#L953

Wolfgang.

Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Wolfgang Grandegger]

Wolfgang Grandegger wrote:
> Jonathan Corbet wrote:
>> On Thu, 14 May 2009 11:03:53 +0200
>> Wolfgang Grandegger <wg@grandegger.com> wrote:
>>
>>>> So you're still using the "put the higher-level structure at the top so we
>>>> can treat it like either kind of pointer" trick.  I'd still recommend
>>>> against that.  Far better to do something like:
>>>>
>>>> 	struct can_priv *canpriv = netdev_priv(dev);
>>>> 	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);
>>>>
>>>> Of course, you can put that dance into a helper function.  
>>> There is no way to initialize the value returned by netdev_priv() as it
>>> does not point to a member of struct net_device. I already commented here:
>>>
>>>   http://marc.info/?l=linux-netdev&m=124120212106891&w=2
>>>
>>> Have I missed something?
>> I'm confused.  It points to the struct can_priv that you registered at
>> the beginning.  Since that structure is contained within struct
>> sja1000_priv, you can use container_of(), as described above, to get
>> it.
>>
>> I would probably just write something like:
>>
>> static inline struct sja1000_priv *to_sja1000_priv(struct net_device *dev)
>> {
>> 	return container_of(netdev_priv(dev), struct sja1000_priv, can);
>> }
>>
>> So have *I* missed something?
> 
> Furthermore, the higher layer needs to known the location of the member
> "struct sja1000_priv can", e.g. by defining:
> 
>   dev->priv = &dev_specific_priv->can;
> 
> But "struct net_device" does not have a "priv" member. netdev_priv(dev)
> always points to the beginning of the private data area. See:
> 
>    http://lxr.linux.no/linux+v2.6.29/include/linux/netdevice.h#L953

I could use container_of() as you suggested, of course, if the "struct
can_priv" remains the first member of  "struct sja1000_priv". Would that
already be an improvement?

Wolfgang.

Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Jonathan Corbet]

On Sat, 16 May 2009 08:57:03 +0200
Wolfgang Grandegger <wg@grandegger.com> wrote:

> > But "struct net_device" does not have a "priv" member. netdev_priv(dev)
> > always points to the beginning of the private data area. See:
> > 
> >    http://lxr.linux.no/linux+v2.6.29/include/linux/netdevice.h#L953  
> 
> I could use container_of() as you suggested, of course, if the "struct
> can_priv" remains the first member of  "struct sja1000_priv". Would that
> already be an improvement?

Ah, OK, I'm a little slow, sorry.  I do get what the problem is here.

Using container_of() in that situation doesn't help a whole lot.  The
point is to get away from this assumption that struct can_priv is the
first element of the structure.

Perhaps what you have is as good as it gets for now, but please add a
comment to the structure noting that struct can_priv has to come first
and why.

Thanks,

jon

[PATCH v2 5/7] [PATCH 5/8] can: SJA1000 generic platform bus driver

[Wolfgang Grandegger]

[-- Attachment #1: 05-sja1000-platform-driver.patch --]
[-- Type: text/plain, Size: 7904 bytes --]

This driver adds support for the SJA1000 chips connected to the
"platform bus", which can be found on various embedded systems.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
---
 drivers/net/can/Kconfig                    |   10 +
 drivers/net/can/sja1000/Makefile           |    1 
 drivers/net/can/sja1000/sja1000_platform.c |  169 +++++++++++++++++++++++++++++
 include/linux/can/platform/sja1000.h       |   32 +++++
 4 files changed, 212 insertions(+)
 create mode 100644 drivers/net/can/sja1000/sja1000_platform.c
 create mode 100644 include/linux/can/platform/sja1000.h

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================
--- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:25.747720647 +0200
+++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:26.411720627 +0200
@@ -41,6 +41,16 @@
 	---help---
 	  Driver for the SJA1000 CAN controllers from Philips or NXP
 
+config CAN_SJA1000_PLATFORM
+	depends on CAN_SJA1000
+	tristate "Generic Platform Bus based SJA1000 driver"
+	---help---
+	  This driver adds support for the SJA1000 chips connected to
+	  the "platform bus" (Linux abstraction for directly to the
+	  processor attached devices).  Which can be found on various
+	  boards from Phytec (http://www.phytec.de) like the PCM027,
+	  PCM038.
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN
Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================
--- net-next-2.6.orig/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:25.753720385 +0200
+++ net-next-2.6/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:26.412720490 +0200
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
+obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c	2009-05-12 10:47:26.416720502 +0200
@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) 2005 Sascha Hauer, Pengutronix
+ * Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/platform/sja1000.h>
+#include <linux/io.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME "sja1000_platform"
+
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
+MODULE_LICENSE("GPL v2");
+
+static u8 sp_read_reg(const struct net_device *dev, int reg)
+{
+	return ioread8((void __iomem *)(dev->base_addr + reg));
+}
+
+static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
+{
+	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
+}
+
+static int sp_probe(struct platform_device *pdev)
+{
+	int err, irq;
+	void __iomem *addr;
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+	struct resource *res_mem, *res_irq;
+	struct sja1000_platform_data *pdata;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data provided!\n");
+		err = -ENODEV;
+		goto exit;
+	}
+
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res_mem || !res_irq) {
+		err = -ENODEV;
+		goto exit;
+	}
+
+	if (!request_mem_region(res_mem->start,
+				res_mem->end - res_mem->start + 1,
+				DRV_NAME)) {
+		err = -EBUSY;
+		goto exit;
+	}
+
+	addr = ioremap_nocache(res_mem->start,
+			       res_mem->end - res_mem->start + 1);
+	if (!addr) {
+		err = -ENOMEM;
+		goto exit_release;
+	}
+
+	irq = res_irq->start;
+	if (res_irq->flags & IRQF_TRIGGER_MASK)
+		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);
+
+	dev = alloc_sja1000dev(0);
+	if (!dev) {
+		err = -ENOMEM;
+		goto exit_iounmap;
+	}
+	priv = netdev_priv(dev);
+
+	priv->read_reg = sp_read_reg;
+	priv->write_reg = sp_write_reg;
+	priv->can.clock.freq = pdata->clock;
+	priv->ocr = pdata->ocr;
+	priv->cdr = pdata->cdr;
+
+	dev->irq = irq;
+	dev->base_addr = (unsigned long)addr;
+
+	dev_set_drvdata(&pdev->dev, dev);
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	err = register_sja1000dev(dev);
+	if (err) {
+		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
+			DRV_NAME, err);
+		goto exit_free;
+	}
+
+	dev_info(&pdev->dev, "%s device registered (base_addr=%#lx, irq=%d)\n",
+		 DRV_NAME, dev->base_addr, dev->irq);
+	return 0;
+
+ exit_free:
+	free_sja1000dev(dev);
+ exit_iounmap:
+	iounmap(addr);
+ exit_release:
+	release_mem_region(res_mem->start, res_mem->end - res_mem->start + 1);
+ exit:
+	return err;
+}
+
+static int sp_remove(struct platform_device *pdev)
+{
+	struct net_device *dev = dev_get_drvdata(&pdev->dev);
+	struct resource *res;
+
+	unregister_sja1000dev(dev);
+	dev_set_drvdata(&pdev->dev, NULL);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, res->end - res->start + 1);
+
+	if (dev->base_addr)
+		iounmap((void __iomem *)dev->base_addr);
+
+	free_sja1000dev(dev);
+
+	return 0;
+}
+
+static struct platform_driver sp_driver = {
+	.probe = sp_probe,
+	.remove = sp_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init sp_init(void)
+{
+	return platform_driver_register(&sp_driver);
+}
+
+static void __exit sp_exit(void)
+{
+	platform_driver_unregister(&sp_driver);
+}
+
+module_init(sp_init);
+module_exit(sp_exit);
Index: net-next-2.6/include/linux/can/platform/sja1000.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/include/linux/can/platform/sja1000.h	2009-05-12 10:47:26.419720650 +0200
@@ -0,0 +1,32 @@
+#ifndef _CAN_PLATFORM_SJA1000_H_
+#define _CAN_PLATFORM_SJA1000_H_
+
+/* clock divider register */
+#define CDR_CLKOUT_MASK 0x07
+#define CDR_CLK_OFF	0x08 /* Clock off (CLKOUT pin) */
+#define CDR_RXINPEN	0x20 /* TX1 output is RX irq output */
+#define CDR_CBP		0x40 /* CAN input comparator bypass */
+#define CDR_PELICAN	0x80 /* PeliCAN mode */
+
+/* output control register */
+#define OCR_MODE_BIPHASE  0x00
+#define OCR_MODE_TEST     0x01
+#define OCR_MODE_NORMAL   0x02
+#define OCR_MODE_CLOCK    0x03
+#define OCR_TX0_INVERT    0x04
+#define OCR_TX0_PULLDOWN  0x08
+#define OCR_TX0_PULLUP    0x10
+#define OCR_TX0_PUSHPULL  0x18
+#define OCR_TX1_INVERT    0x20
+#define OCR_TX1_PULLDOWN  0x40
+#define OCR_TX1_PULLUP    0x80
+#define OCR_TX1_PUSHPULL  0xc0
+
+struct sja1000_platform_data {
+	u32 clock;	/* CAN bus oscillator frequency in Hz */
+
+	u8 ocr;		/* output control register */
+	u8 cdr;		/* clock divider register */
+};
+
+#endif	/* !_CAN_PLATFORM_SJA1000_H_ */

Re: [PATCH v2 5/7] [PATCH 5/8] can: SJA1000 generic platform bus driver

[Jonathan Corbet]

On Tue, 12 May 2009 11:28:02 +0200
Wolfgang Grandegger <wg@grandegger.com> wrote:

> This driver adds support for the SJA1000 chips connected to the
> "platform bus", which can be found on various embedded systems.

[...]

> +
> +static u8 sp_read_reg(const struct net_device *dev, int reg)
> +{
> +	return ioread8((void __iomem *)(dev->base_addr + reg));
> +}
> +
> +static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
> +{
> +	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
> +}

So there's no locking around accesses to the hardware at all.  How do you
protect against concurrent access?

[...]

> +static int sp_remove(struct platform_device *pdev)
> +{
> +	struct net_device *dev = dev_get_drvdata(&pdev->dev);
> +	struct resource *res;
> +
> +	unregister_sja1000dev(dev);
> +	dev_set_drvdata(&pdev->dev, NULL);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(res->start, res->end - res->start + 1);
> +
> +	if (dev->base_addr)
> +		iounmap((void __iomem *)dev->base_addr);

Seems like you should unmap it before releasing it back to the kernel.
Nobody else is ever going to jump in and try to map it, but still...

> +	free_sja1000dev(dev);
> +
> +	return 0;
> +}

jon

Re: [PATCH v2 5/7] [PATCH 5/8] can: SJA1000 generic platform bus driver

[Wolfgang Grandegger]

Jonathan Corbet wrote:
> On Tue, 12 May 2009 11:28:02 +0200
> Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
>> This driver adds support for the SJA1000 chips connected to the
>> "platform bus", which can be found on various embedded systems.
> 
> [...]
> 
>> +
>> +static u8 sp_read_reg(const struct net_device *dev, int reg)
>> +{
>> +	return ioread8((void __iomem *)(dev->base_addr + reg));
>> +}
>> +
>> +static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
>> +{
>> +	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
>> +}
> 
> So there's no locking around accesses to the hardware at all.  How do you
> protect against concurrent access?

There is no concurrent access to the same register and PCI register
accesses do not need to be serialized.

> [...]
> 
>> +static int sp_remove(struct platform_device *pdev)
>> +{
>> +	struct net_device *dev = dev_get_drvdata(&pdev->dev);
>> +	struct resource *res;
>> +
>> +	unregister_sja1000dev(dev);
>> +	dev_set_drvdata(&pdev->dev, NULL);
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	release_mem_region(res->start, res->end - res->start + 1);
>> +
>> +	if (dev->base_addr)
>> +		iounmap((void __iomem *)dev->base_addr);
> 
> Seems like you should unmap it before releasing it back to the kernel.
> Nobody else is ever going to jump in and try to map it, but still...

Will fix.

Wolfgang.

Re: [PATCH v2 5/7] [PATCH 5/8] can: SJA1000 generic platform bus driver

[Sascha Hauer]

Hi Wolfgang,

some comments inline.

Sascha

On Tue, May 12, 2009 at 11:28:02AM +0200, Wolfgang Grandegger wrote:
> This driver adds support for the SJA1000 chips connected to the
> "platform bus", which can be found on various embedded systems.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
> ---
>  drivers/net/can/Kconfig                    |   10 +
>  drivers/net/can/sja1000/Makefile           |    1 
>  drivers/net/can/sja1000/sja1000_platform.c |  169 +++++++++++++++++++++++++++++
>  include/linux/can/platform/sja1000.h       |   32 +++++
>  4 files changed, 212 insertions(+)
>  create mode 100644 drivers/net/can/sja1000/sja1000_platform.c
>  create mode 100644 include/linux/can/platform/sja1000.h
> 
> Index: net-next-2.6/drivers/net/can/Kconfig
> ===================================================================
> --- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:25.747720647 +0200
> +++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:26.411720627 +0200
> @@ -41,6 +41,16 @@
>  	---help---
>  	  Driver for the SJA1000 CAN controllers from Philips or NXP
>  
> +config CAN_SJA1000_PLATFORM
> +	depends on CAN_SJA1000
> +	tristate "Generic Platform Bus based SJA1000 driver"
> +	---help---
> +	  This driver adds support for the SJA1000 chips connected to
> +	  the "platform bus" (Linux abstraction for directly to the
> +	  processor attached devices).  Which can be found on various
> +	  boards from Phytec (http://www.phytec.de) like the PCM027,
> +	  PCM038.
> +
>  config CAN_DEBUG_DEVICES
>  	bool "CAN devices debugging messages"
>  	depends on CAN
> Index: net-next-2.6/drivers/net/can/sja1000/Makefile
> ===================================================================
> --- net-next-2.6.orig/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:25.753720385 +0200
> +++ net-next-2.6/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:26.412720490 +0200
> @@ -3,5 +3,6 @@
>  #
>  
>  obj-$(CONFIG_CAN_SJA1000) += sja1000.o
> +obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c	2009-05-12 10:47:26.416720502 +0200
> @@ -0,0 +1,169 @@
> +/*
> + * Copyright (C) 2005 Sascha Hauer, Pengutronix
> + * Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the version 2 of the GNU General Public License
> + * as published by the Free Software Foundation
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/netdevice.h>
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/irq.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/platform/sja1000.h>
> +#include <linux/io.h>
> +
> +#include "sja1000.h"
> +
> +#define DRV_NAME "sja1000_platform"
> +
> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
> +MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
> +MODULE_LICENSE("GPL v2");
> +
> +static u8 sp_read_reg(const struct net_device *dev, int reg)
> +{
> +	return ioread8((void __iomem *)(dev->base_addr + reg));
> +}
> +
> +static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
> +{
> +	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
> +}
> +
> +static int sp_probe(struct platform_device *pdev)
> +{
> +	int err, irq;
> +	void __iomem *addr;
> +	struct net_device *dev;
> +	struct sja1000_priv *priv;
> +	struct resource *res_mem, *res_irq;
> +	struct sja1000_platform_data *pdata;
> +
> +	pdata = pdev->dev.platform_data;
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "No platform data provided!\n");
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +
> +	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!res_mem || !res_irq) {
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +
> +	if (!request_mem_region(res_mem->start,
> +				res_mem->end - res_mem->start + 1,
> +				DRV_NAME)) {

resource_size(res_mem) please, also for the other occurrences in this
file

> +		err = -EBUSY;
> +		goto exit;
> +	}
> +
> +	addr = ioremap_nocache(res_mem->start,
> +			       res_mem->end - res_mem->start + 1);
> +	if (!addr) {
> +		err = -ENOMEM;
> +		goto exit_release;
> +	}
> +
> +	irq = res_irq->start;
> +	if (res_irq->flags & IRQF_TRIGGER_MASK)
> +		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);

You shouldn't use set_irq_type on not yet requested irqs but instead
pass the flags to the real driver and pass them in request_irq.

> +
> +	dev = alloc_sja1000dev(0);
> +	if (!dev) {
> +		err = -ENOMEM;
> +		goto exit_iounmap;
> +	}
> +	priv = netdev_priv(dev);
> +
> +	priv->read_reg = sp_read_reg;
> +	priv->write_reg = sp_write_reg;
> +	priv->can.clock.freq = pdata->clock;
> +	priv->ocr = pdata->ocr;
> +	priv->cdr = pdata->cdr;
> +
> +	dev->irq = irq;
> +	dev->base_addr = (unsigned long)addr;
> +
> +	dev_set_drvdata(&pdev->dev, dev);
> +	SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +	err = register_sja1000dev(dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> +			DRV_NAME, err);
> +		goto exit_free;
> +	}
> +
> +	dev_info(&pdev->dev, "%s device registered (base_addr=%#lx, irq=%d)\n",
> +		 DRV_NAME, dev->base_addr, dev->irq);

dev_info will already print the driver name twice, printing DRV_NAME
again seems unnecessary.

> +	return 0;
> +
> + exit_free:
> +	free_sja1000dev(dev);
> + exit_iounmap:
> +	iounmap(addr);
> + exit_release:
> +	release_mem_region(res_mem->start, res_mem->end - res_mem->start + 1);
> + exit:
> +	return err;
> +}
> +
> +static int sp_remove(struct platform_device *pdev)
> +{
> +	struct net_device *dev = dev_get_drvdata(&pdev->dev);
> +	struct resource *res;
> +
> +	unregister_sja1000dev(dev);
> +	dev_set_drvdata(&pdev->dev, NULL);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(res->start, res->end - res->start + 1);
> +
> +	if (dev->base_addr)
> +		iounmap((void __iomem *)dev->base_addr);
> +
> +	free_sja1000dev(dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver sp_driver = {
> +	.probe = sp_probe,
> +	.remove = sp_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init sp_init(void)
> +{
> +	return platform_driver_register(&sp_driver);
> +}
> +
> +static void __exit sp_exit(void)
> +{
> +	platform_driver_unregister(&sp_driver);
> +}
> +
> +module_init(sp_init);
> +module_exit(sp_exit);
> Index: net-next-2.6/include/linux/can/platform/sja1000.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ net-next-2.6/include/linux/can/platform/sja1000.h	2009-05-12 10:47:26.419720650 +0200
> @@ -0,0 +1,32 @@
> +#ifndef _CAN_PLATFORM_SJA1000_H_
> +#define _CAN_PLATFORM_SJA1000_H_
> +
> +/* clock divider register */
> +#define CDR_CLKOUT_MASK 0x07
> +#define CDR_CLK_OFF	0x08 /* Clock off (CLKOUT pin) */
> +#define CDR_RXINPEN	0x20 /* TX1 output is RX irq output */
> +#define CDR_CBP		0x40 /* CAN input comparator bypass */
> +#define CDR_PELICAN	0x80 /* PeliCAN mode */
> +
> +/* output control register */
> +#define OCR_MODE_BIPHASE  0x00
> +#define OCR_MODE_TEST     0x01
> +#define OCR_MODE_NORMAL   0x02
> +#define OCR_MODE_CLOCK    0x03
> +#define OCR_TX0_INVERT    0x04
> +#define OCR_TX0_PULLDOWN  0x08
> +#define OCR_TX0_PULLUP    0x10
> +#define OCR_TX0_PUSHPULL  0x18
> +#define OCR_TX1_INVERT    0x20
> +#define OCR_TX1_PULLDOWN  0x40
> +#define OCR_TX1_PULLUP    0x80
> +#define OCR_TX1_PUSHPULL  0xc0
> +
> +struct sja1000_platform_data {
> +	u32 clock;	/* CAN bus oscillator frequency in Hz */
> +
> +	u8 ocr;		/* output control register */
> +	u8 cdr;		/* clock divider register */
> +};
> +
> +#endif	/* !_CAN_PLATFORM_SJA1000_H_ */
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 5/7] [PATCH 5/8] can: SJA1000 generic platform bus driver

[Wolfgang Grandegger]

Hi Sascha,

Sascha Hauer wrote:
> Hi Wolfgang,
> 
> some comments inline.
> 
> Sascha
> 
> On Tue, May 12, 2009 at 11:28:02AM +0200, Wolfgang Grandegger wrote:
>> This driver adds support for the SJA1000 chips connected to the
>> "platform bus", which can be found on various embedded systems.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
>> ---
>>  drivers/net/can/Kconfig                    |   10 +
>>  drivers/net/can/sja1000/Makefile           |    1 
>>  drivers/net/can/sja1000/sja1000_platform.c |  169 +++++++++++++++++++++++++++++
>>  include/linux/can/platform/sja1000.h       |   32 +++++
>>  4 files changed, 212 insertions(+)
>>  create mode 100644 drivers/net/can/sja1000/sja1000_platform.c
>>  create mode 100644 include/linux/can/platform/sja1000.h
>>
>> Index: net-next-2.6/drivers/net/can/Kconfig
>> ===================================================================
>> --- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:25.747720647 +0200
>> +++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:26.411720627 +0200
>> @@ -41,6 +41,16 @@
>>  	---help---
>>  	  Driver for the SJA1000 CAN controllers from Philips or NXP
>>  
>> +config CAN_SJA1000_PLATFORM
>> +	depends on CAN_SJA1000
>> +	tristate "Generic Platform Bus based SJA1000 driver"
>> +	---help---
>> +	  This driver adds support for the SJA1000 chips connected to
>> +	  the "platform bus" (Linux abstraction for directly to the
>> +	  processor attached devices).  Which can be found on various
>> +	  boards from Phytec (http://www.phytec.de) like the PCM027,
>> +	  PCM038.
>> +
>>  config CAN_DEBUG_DEVICES
>>  	bool "CAN devices debugging messages"
>>  	depends on CAN
>> Index: net-next-2.6/drivers/net/can/sja1000/Makefile
>> ===================================================================
>> --- net-next-2.6.orig/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:25.753720385 +0200
>> +++ net-next-2.6/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:26.412720490 +0200
>> @@ -3,5 +3,6 @@
>>  #
>>  
>>  obj-$(CONFIG_CAN_SJA1000) += sja1000.o
>> +obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
>>  
>>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>> Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
>> ===================================================================
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c	2009-05-12 10:47:26.416720502 +0200
>> @@ -0,0 +1,169 @@
>> +/*
>> + * Copyright (C) 2005 Sascha Hauer, Pengutronix
>> + * Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the version 2 of the GNU General Public License
>> + * as published by the Free Software Foundation
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/delay.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/irq.h>
>> +#include <linux/can.h>
>> +#include <linux/can/dev.h>
>> +#include <linux/can/platform/sja1000.h>
>> +#include <linux/io.h>
>> +
>> +#include "sja1000.h"
>> +
>> +#define DRV_NAME "sja1000_platform"
>> +
>> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
>> +MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
>> +MODULE_LICENSE("GPL v2");
>> +
>> +static u8 sp_read_reg(const struct net_device *dev, int reg)
>> +{
>> +	return ioread8((void __iomem *)(dev->base_addr + reg));
>> +}
>> +
>> +static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
>> +{
>> +	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
>> +}
>> +
>> +static int sp_probe(struct platform_device *pdev)
>> +{
>> +	int err, irq;
>> +	void __iomem *addr;
>> +	struct net_device *dev;
>> +	struct sja1000_priv *priv;
>> +	struct resource *res_mem, *res_irq;
>> +	struct sja1000_platform_data *pdata;
>> +
>> +	pdata = pdev->dev.platform_data;
>> +	if (!pdata) {
>> +		dev_err(&pdev->dev, "No platform data provided!\n");
>> +		err = -ENODEV;
>> +		goto exit;
>> +	}
>> +
>> +	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +	if (!res_mem || !res_irq) {
>> +		err = -ENODEV;
>> +		goto exit;
>> +	}
>> +
>> +	if (!request_mem_region(res_mem->start,
>> +				res_mem->end - res_mem->start + 1,
>> +				DRV_NAME)) {
> 
> resource_size(res_mem) please, also for the other occurrences in this
> file

OK.

>> +		err = -EBUSY;
>> +		goto exit;
>> +	}
>> +
>> +	addr = ioremap_nocache(res_mem->start,
>> +			       res_mem->end - res_mem->start + 1);
>> +	if (!addr) {
>> +		err = -ENOMEM;
>> +		goto exit_release;
>> +	}
>> +
>> +	irq = res_irq->start;
>> +	if (res_irq->flags & IRQF_TRIGGER_MASK)
>> +		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);
> 
> You shouldn't use set_irq_type on not yet requested irqs but instead
> pass the flags to the real driver and pass them in request_irq.

Why? I would require an extra member in the struct sja1000_priv.

Wolfgang.

Re: [PATCH v2 5/7] [PATCH 5/8] can: SJA1000 generic platform bus driver

[Sascha Hauer]

On Fri, May 15, 2009 at 11:35:06AM +0200, Wolfgang Grandegger wrote:
> Hi Sascha,
> 
> Sascha Hauer wrote:
> > Hi Wolfgang,
> > 
> > some comments inline.
> > 
> > Sascha
> > 
> > On Tue, May 12, 2009 at 11:28:02AM +0200, Wolfgang Grandegger wrote:
> >> This driver adds support for the SJA1000 chips connected to the
> >> "platform bus", which can be found on various embedded systems.
> >>
> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >> Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
> >> ---
> >>  drivers/net/can/Kconfig                    |   10 +
> >>  drivers/net/can/sja1000/Makefile           |    1 
> >>  drivers/net/can/sja1000/sja1000_platform.c |  169 +++++++++++++++++++++++++++++
> >>  include/linux/can/platform/sja1000.h       |   32 +++++
> >>  4 files changed, 212 insertions(+)
> >>  create mode 100644 drivers/net/can/sja1000/sja1000_platform.c
> >>  create mode 100644 include/linux/can/platform/sja1000.h
> >>
> >> Index: net-next-2.6/drivers/net/can/Kconfig
> >> ===================================================================
> >> --- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:25.747720647 +0200
> >> +++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:26.411720627 +0200
> >> @@ -41,6 +41,16 @@
> >>  	---help---
> >>  	  Driver for the SJA1000 CAN controllers from Philips or NXP
> >>  
> >> +config CAN_SJA1000_PLATFORM
> >> +	depends on CAN_SJA1000
> >> +	tristate "Generic Platform Bus based SJA1000 driver"
> >> +	---help---
> >> +	  This driver adds support for the SJA1000 chips connected to
> >> +	  the "platform bus" (Linux abstraction for directly to the
> >> +	  processor attached devices).  Which can be found on various
> >> +	  boards from Phytec (http://www.phytec.de) like the PCM027,
> >> +	  PCM038.
> >> +
> >>  config CAN_DEBUG_DEVICES
> >>  	bool "CAN devices debugging messages"
> >>  	depends on CAN
> >> Index: net-next-2.6/drivers/net/can/sja1000/Makefile
> >> ===================================================================
> >> --- net-next-2.6.orig/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:25.753720385 +0200
> >> +++ net-next-2.6/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:26.412720490 +0200
> >> @@ -3,5 +3,6 @@
> >>  #
> >>  
> >>  obj-$(CONFIG_CAN_SJA1000) += sja1000.o
> >> +obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
> >>  
> >>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> >> Index: net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c
> >> ===================================================================
> >> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> >> +++ net-next-2.6/drivers/net/can/sja1000/sja1000_platform.c	2009-05-12 10:47:26.416720502 +0200
> >> @@ -0,0 +1,169 @@
> >> +/*
> >> + * Copyright (C) 2005 Sascha Hauer, Pengutronix
> >> + * Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the version 2 of the GNU General Public License
> >> + * as published by the Free Software Foundation
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, write to the Free Software
> >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/netdevice.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/irq.h>
> >> +#include <linux/can.h>
> >> +#include <linux/can/dev.h>
> >> +#include <linux/can/platform/sja1000.h>
> >> +#include <linux/io.h>
> >> +
> >> +#include "sja1000.h"
> >> +
> >> +#define DRV_NAME "sja1000_platform"
> >> +
> >> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
> >> +MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
> >> +MODULE_LICENSE("GPL v2");
> >> +
> >> +static u8 sp_read_reg(const struct net_device *dev, int reg)
> >> +{
> >> +	return ioread8((void __iomem *)(dev->base_addr + reg));
> >> +}
> >> +
> >> +static void sp_write_reg(const struct net_device *dev, int reg, u8 val)
> >> +{
> >> +	iowrite8(val, (void __iomem *)(dev->base_addr + reg));
> >> +}
> >> +
> >> +static int sp_probe(struct platform_device *pdev)
> >> +{
> >> +	int err, irq;
> >> +	void __iomem *addr;
> >> +	struct net_device *dev;
> >> +	struct sja1000_priv *priv;
> >> +	struct resource *res_mem, *res_irq;
> >> +	struct sja1000_platform_data *pdata;
> >> +
> >> +	pdata = pdev->dev.platform_data;
> >> +	if (!pdata) {
> >> +		dev_err(&pdev->dev, "No platform data provided!\n");
> >> +		err = -ENODEV;
> >> +		goto exit;
> >> +	}
> >> +
> >> +	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> +	if (!res_mem || !res_irq) {
> >> +		err = -ENODEV;
> >> +		goto exit;
> >> +	}
> >> +
> >> +	if (!request_mem_region(res_mem->start,
> >> +				res_mem->end - res_mem->start + 1,
> >> +				DRV_NAME)) {
> > 
> > resource_size(res_mem) please, also for the other occurrences in this
> > file
> 
> OK.
> 
> >> +		err = -EBUSY;
> >> +		goto exit;
> >> +	}
> >> +
> >> +	addr = ioremap_nocache(res_mem->start,
> >> +			       res_mem->end - res_mem->start + 1);
> >> +	if (!addr) {
> >> +		err = -ENOMEM;
> >> +		goto exit_release;
> >> +	}
> >> +
> >> +	irq = res_irq->start;
> >> +	if (res_irq->flags & IRQF_TRIGGER_MASK)
> >> +		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);
> > 
> > You shouldn't use set_irq_type on not yet requested irqs but instead
> > pass the flags to the real driver and pass them in request_irq.
> 
> Why? I would require an extra member in the struct sja1000_priv.

You change a resource you do not own. What if request_irq returns with
-EBUSY? You already screwed up any other potential user.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 5/7] [PATCH 5/8] can: SJA1000 generic platform bus driver

[Wolfgang Grandegger]

Sascha Hauer wrote:
> On Fri, May 15, 2009 at 11:35:06AM +0200, Wolfgang Grandegger wrote:
>> Hi Sascha,
>>
>> Sascha Hauer wrote:
>>> Hi Wolfgang,
[...}
>>>> +	irq = res_irq->start;
>>>> +	if (res_irq->flags & IRQF_TRIGGER_MASK)
>>>> +		set_irq_type(irq, res_irq->flags & IRQF_TRIGGER_MASK);
>>> You shouldn't use set_irq_type on not yet requested irqs but instead
>>> pass the flags to the real driver and pass them in request_irq.
>> Why? I would require an extra member in the struct sja1000_priv.
> 
> You change a resource you do not own. What if request_irq returns with
> -EBUSY? You already screwed up any other potential user.

OK, I added irq_flags to struct sja1000_priv, which  will then be used
by the request_irq(). The driver must to set it appropriately, including
IRQF_SHARED. This also fixes the problem, that drivers used IRQF_SHARED
without reason.

Thanks,

Wolfgang.

[PATCH v2 6/7] [PATCH 6/8] can: SJA1000 driver for EMS PCI cards

[Wolfgang Grandegger]

[-- Attachment #1: 06-sja1000-ems-pci-driver.patch --]
[-- Type: text/plain, Size: 10889 bytes --]

The patch adds support for the one or two channel CPC-PCI and CPC-PCIe
cards from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de).

Signed-off-by: Sebastian Haas <haas@ems-wuensche.com>
Signed-off-by: Markus Plessing <plessing@ems-wuensche.com>
Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/Kconfig           |    7 
 drivers/net/can/sja1000/Makefile  |    1 
 drivers/net/can/sja1000/ems_pci.c |  328 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/net/can/sja1000/ems_pci.c

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================
--- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:26.411720627 +0200
+++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:27.116970063 +0200
@@ -51,6 +51,13 @@
 	  boards from Phytec (http://www.phytec.de) like the PCM027,
 	  PCM038.
 
+config CAN_EMS_PCI
+	tristate "EMS CPC-PCI and CPC-PCIe Card"
+	depends on PCI && CAN_SJA1000
+	---help---
+	  This driver is for the one or two channel CPC-PCI and CPC-PCIe
+	  cards from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de).
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN
Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================
--- net-next-2.6.orig/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:26.412720490 +0200
+++ net-next-2.6/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:27.116970063 +0200
@@ -4,5 +4,6 @@
 
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
 obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
+obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
Index: net-next-2.6/drivers/net/can/sja1000/ems_pci.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/drivers/net/can/sja1000/ems_pci.c	2009-05-12 10:47:27.120968957 +0200
@@ -0,0 +1,328 @@
+/*
+ * Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
+ * Copyright (C) 2008 Markus Plessing <plessing@ems-wuensche.com>
+ * Copyright (C) 2008 Sebastian Haas <haas@ems-wuensche.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/io.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME  "ems_pci"
+
+MODULE_AUTHOR("Sebastian Haas <haas@ems-wuenche.com>");
+MODULE_DESCRIPTION("Socket-CAN driver for EMS CPC-PCI/PCIe CAN cards");
+MODULE_SUPPORTED_DEVICE("EMS CPC-PCI/PCIe CAN card");
+MODULE_LICENSE("GPL v2");
+
+#define EMS_PCI_MAX_CHAN 2
+
+struct ems_pci_card {
+	int channels;
+
+	struct pci_dev *pci_dev;
+	struct net_device *net_dev[EMS_PCI_MAX_CHAN];
+
+	void __iomem *conf_addr;
+	void __iomem *base_addr;
+};
+
+#define EMS_PCI_CAN_CLOCK (16000000 / 2)
+
+/*
+ * Register definitions and descriptions are from LinCAN 0.3.3.
+ *
+ * PSB4610 PITA-2 bridge control registers
+ */
+#define PITA2_ICR           0x00	/* Interrupt Control Register */
+#define PITA2_ICR_INT0      0x00000002	/* [RC] INT0 Active/Clear */
+#define PITA2_ICR_INT0_EN   0x00020000	/* [RW] Enable INT0 */
+
+#define PITA2_MISC          0x1c	/* Miscellaneous Register */
+#define PITA2_MISC_CONFIG   0x04000000	/* Multiplexed parallel interface */
+
+/*
+ * The board configuration is probably following:
+ * RX1 is connected to ground.
+ * TX1 is not connected.
+ * CLKO is not connected.
+ * Setting the OCR register to 0xDA is a good idea.
+ * This means  normal output mode , push-pull and the correct polarity.
+ */
+#define EMS_PCI_OCR         (OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL)
+
+/*
+ * In the CDR register, you should set CBP to 1.
+ * You will probably also want to set the clock divider value to 7
+ * (meaning direct oscillator output) because the second SJA1000 chip
+ * is driven by the first one CLKOUT output.
+ */
+#define EMS_PCI_CDR             (CDR_CBP | CDR_CLKOUT_MASK)
+#define EMS_PCI_MEM_SIZE        4096  /* Size of the remapped io-memory */
+#define EMS_PCI_CAN_BASE_OFFSET 0x400 /* offset where the controllers starts */
+#define EMS_PCI_CAN_CTRL_SIZE   0x200 /* memory size for each controller */
+
+#define EMS_PCI_PORT_BYTES  0x4     /* Each register occupies 4 bytes */
+
+#define EMS_PCI_VENDOR_ID   0x110a  /* PCI device and vendor ID */
+#define EMS_PCI_DEVICE_ID   0x2104
+
+static struct pci_device_id ems_pci_tbl[] = {
+	{EMS_PCI_VENDOR_ID, EMS_PCI_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
+	{0,}
+};
+MODULE_DEVICE_TABLE(pci, ems_pci_tbl);
+
+/*
+ * Helper to read internal registers from card logic (not CAN)
+ */
+static u8 ems_pci_readb(struct ems_pci_card *card, unsigned int port)
+{
+	return readb((void __iomem *)card->base_addr
+			+ (port * EMS_PCI_PORT_BYTES));
+}
+
+static u8 ems_pci_read_reg(const struct net_device *dev, int port)
+{
+	return readb((void __iomem *)dev->base_addr
+			+ (port * EMS_PCI_PORT_BYTES));
+}
+
+static void ems_pci_write_reg(const struct net_device *dev, int port, u8 val)
+{
+	writeb(val, (void __iomem *)dev->base_addr
+		+ (port * EMS_PCI_PORT_BYTES));
+}
+
+static void ems_pci_post_irq(const struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct ems_pci_card *card = (struct ems_pci_card *)priv->priv;
+
+	/* reset int flag of pita */
+	writel(PITA2_ICR_INT0_EN | PITA2_ICR_INT0, card->conf_addr
+		+ PITA2_ICR);
+}
+
+/*
+ * Check if a CAN controller is present at the specified location
+ * by trying to set 'em into the PeliCAN mode
+ */
+static inline int ems_pci_check_chan(struct net_device *dev)
+{
+	unsigned char res;
+
+	/* Make sure SJA1000 is in reset mode */
+	ems_pci_write_reg(dev, REG_MOD, 1);
+
+	ems_pci_write_reg(dev, REG_CDR, CDR_PELICAN);
+
+	/* read reset-values */
+	res = ems_pci_read_reg(dev, REG_CDR);
+
+	if (res == CDR_PELICAN)
+		return 1;
+
+	return 0;
+}
+
+static void ems_pci_del_card(struct pci_dev *pdev)
+{
+	struct ems_pci_card *card = pci_get_drvdata(pdev);
+	struct net_device *dev;
+	int i = 0;
+
+	for (i = 0; i < card->channels; i++) {
+		dev = card->net_dev[i];
+
+		if (!dev)
+			continue;
+
+		dev_info(&pdev->dev, "Removing %s.\n", dev->name);
+		unregister_sja1000dev(dev);
+		free_sja1000dev(dev);
+	}
+
+	if (card->base_addr != NULL)
+		pci_iounmap(card->pci_dev, card->base_addr);
+
+	if (card->conf_addr != NULL)
+		pci_iounmap(card->pci_dev, card->conf_addr);
+
+	kfree(card);
+
+	pci_disable_device(pdev);
+	pci_set_drvdata(pdev, NULL);
+}
+
+static void ems_pci_card_reset(struct ems_pci_card *card)
+{
+	/* Request board reset */
+	writeb(0, card->base_addr);
+}
+
+/*
+ * Probe PCI device for EMS CAN signature and register each available
+ * CAN channel to SJA1000 Socket-CAN subsystem.
+ */
+static int __devinit ems_pci_add_card(struct pci_dev *pdev,
+					const struct pci_device_id *ent)
+{
+	struct sja1000_priv *priv;
+	struct net_device *dev;
+	struct ems_pci_card *card;
+	int err, i;
+
+	/* Enabling PCI device */
+	if (pci_enable_device(pdev) < 0) {
+		dev_err(&pdev->dev, "Enabling PCI device failed\n");
+		return -ENODEV;
+	}
+
+	/* Allocating card structures to hold addresses, ... */
+	card = kzalloc(sizeof(struct ems_pci_card), GFP_KERNEL);
+	if (card == NULL) {
+		dev_err(&pdev->dev, "Unable to allocate memory\n");
+		pci_disable_device(pdev);
+		return -ENOMEM;
+	}
+
+	pci_set_drvdata(pdev, card);
+
+	card->pci_dev = pdev;
+
+	card->channels = 0;
+
+	/* Remap PITA configuration space, and controller memory area */
+	card->conf_addr = pci_iomap(pdev, 0, EMS_PCI_MEM_SIZE);
+	if (card->conf_addr == NULL) {
+		err = -ENOMEM;
+
+		goto failure_cleanup;
+	}
+
+	card->base_addr = pci_iomap(pdev, 1, EMS_PCI_MEM_SIZE);
+	if (card->base_addr == NULL) {
+		err = -ENOMEM;
+
+		goto failure_cleanup;
+	}
+
+	/* Configure PITA-2 parallel interface (enable MUX) */
+	writel(PITA2_MISC_CONFIG, card->conf_addr + PITA2_MISC);
+
+	/* Check for unique EMS CAN signature */
+	if (ems_pci_readb(card, 0) != 0x55 ||
+	    ems_pci_readb(card, 1) != 0xAA ||
+	    ems_pci_readb(card, 2) != 0x01 ||
+	    ems_pci_readb(card, 3) != 0xCB ||
+	    ems_pci_readb(card, 4) != 0x11) {
+		dev_err(&pdev->dev, "Not EMS Dr. Thomas Wuensche interface\n");
+
+		err = -ENODEV;
+		goto failure_cleanup;
+	}
+
+	ems_pci_card_reset(card);
+
+	/* Detect available channels */
+	for (i = 0; i < EMS_PCI_MAX_CHAN; i++) {
+		dev = alloc_sja1000dev(0);
+		if (dev == NULL) {
+			err = -ENOMEM;
+			goto failure_cleanup;
+		}
+
+		card->net_dev[i] = dev;
+		priv = netdev_priv(dev);
+		priv->priv = card;
+
+		dev->irq = pdev->irq;
+		dev->base_addr = (unsigned long)(card->base_addr
+						+ EMS_PCI_CAN_BASE_OFFSET
+						+ (i * EMS_PCI_CAN_CTRL_SIZE));
+
+		/* Check if channel is present */
+		if (ems_pci_check_chan(dev)) {
+			priv->read_reg  = ems_pci_read_reg;
+			priv->write_reg = ems_pci_write_reg;
+			priv->post_irq  = ems_pci_post_irq;
+			priv->can.clock.freq = EMS_PCI_CAN_CLOCK;
+			priv->ocr = EMS_PCI_OCR;
+			priv->cdr = EMS_PCI_CDR;
+
+			SET_NETDEV_DEV(dev, &pdev->dev);
+
+			/* Enable interrupts from card */
+			writel(PITA2_ICR_INT0_EN, card->conf_addr + PITA2_ICR);
+
+			/* Register SJA1000 device */
+			err = register_sja1000dev(dev);
+			if (err) {
+				dev_err(&pdev->dev, "Registering device failed "
+							"(err=%d)\n", err);
+				free_sja1000dev(dev);
+				goto failure_cleanup;
+			}
+
+			card->channels++;
+
+			dev_info(&pdev->dev, "Channel #%d at %#lX, irq %d\n",
+						i + 1, dev->base_addr,
+						dev->irq);
+		} else {
+			free_sja1000dev(dev);
+		}
+	}
+
+	return 0;
+
+failure_cleanup:
+	dev_err(&pdev->dev, "Error: %d. Cleaning Up.\n", err);
+
+	ems_pci_del_card(pdev);
+
+	return err;
+}
+
+static struct pci_driver ems_pci_driver = {
+	.name = DRV_NAME,
+	.id_table = ems_pci_tbl,
+	.probe = ems_pci_add_card,
+	.remove = ems_pci_del_card,
+};
+
+static int __init ems_pci_init(void)
+{
+	return pci_register_driver(&ems_pci_driver);
+}
+
+static void __exit ems_pci_exit(void)
+{
+	pci_unregister_driver(&ems_pci_driver);
+}
+
+module_init(ems_pci_init);
+module_exit(ems_pci_exit);
+

[PATCH v2 7/7] [PATCH 7/8] can: SJA1000 driver for Kvaser PCI cards

[Wolfgang Grandegger]

[-- Attachment #1: 07-sja1000-kvaser-pci-driver.patch --]
[-- Type: text/plain, Size: 13056 bytes --]

The patch adds support for the PCI cards: PCIcan and PCIcanx
(1, 2 or 4 channel) from Kvaser (http://www.kvaser.com).

Signed-off-by: Per Dalen <per.dalen@cnw.se>
Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/Kconfig              |    7 
 drivers/net/can/sja1000/Makefile     |    1 
 drivers/net/can/sja1000/kvaser_pci.c |  415 +++++++++++++++++++++++++++++++++++
 3 files changed, 423 insertions(+)
 create mode 100644 drivers/net/can/sja1000/kvaser_pci.c

Index: net-next-2.6/drivers/net/can/Kconfig
===================================================================
--- net-next-2.6.orig/drivers/net/can/Kconfig	2009-05-12 10:47:27.116970063 +0200
+++ net-next-2.6/drivers/net/can/Kconfig	2009-05-12 10:47:27.844971068 +0200
@@ -58,6 +58,13 @@
 	  This driver is for the one or two channel CPC-PCI and CPC-PCIe
 	  cards from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de).
 
+config CAN_KVASER_PCI
+	tristate "Kvaser PCIcanx and Kvaser PCIcan PCI Cards"
+	depends on PCI && CAN_SJA1000
+	---help---
+	  This driver is for the the PCIcanx and PCIcan cards (1, 2 or
+	  4 channel) from Kvaser (http://www.kvaser.com).
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN
Index: net-next-2.6/drivers/net/can/sja1000/Makefile
===================================================================
--- net-next-2.6.orig/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:27.116970063 +0200
+++ net-next-2.6/drivers/net/can/sja1000/Makefile	2009-05-12 10:47:27.845970931 +0200
@@ -5,5 +5,6 @@
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
 obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
 obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o
+obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
Index: net-next-2.6/drivers/net/can/sja1000/kvaser_pci.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-next-2.6/drivers/net/can/sja1000/kvaser_pci.c	2009-05-12 10:47:27.849970943 +0200
@@ -0,0 +1,415 @@
+/*
+ * Copyright (C) 2008 Per Dalen <per.dalen@cnw.se>
+ *
+ * Parts of this software are based on (derived) the following:
+ *
+ * - Kvaser linux driver, version 4.72 BETA
+ *   Copyright (C) 2002-2007 KVASER AB
+ *
+ * - Lincan driver, version 0.3.3, OCERA project
+ *   Copyright (C) 2004 Pavel Pisa
+ *   Copyright (C) 2001 Arnaud Westenberg
+ *
+ * - Socketcan SJA1000 drivers
+ *   Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
+ *   Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ *   Copyright (c) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
+ *   38106 Braunschweig, GERMANY
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/netdevice.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/io.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME  "kvaser_pci"
+
+MODULE_AUTHOR("Per Dalen <per.dalen@cnw.se>");
+MODULE_DESCRIPTION("Socket-CAN driver for KVASER PCAN PCI cards");
+MODULE_SUPPORTED_DEVICE("KVASER PCAN PCI CAN card");
+MODULE_LICENSE("GPL v2");
+
+#define MAX_NO_OF_CHANNELS        4 /* max no of channels on
+				       a single card */
+
+struct kvaser_pci {
+	int channel;
+	struct pci_dev *pci_dev;
+	struct net_device *slave_dev[MAX_NO_OF_CHANNELS-1];
+	void __iomem *conf_addr;
+	void __iomem *res_addr;
+	int no_channels;
+	u8 xilinx_ver;
+};
+
+#define KVASER_PCI_CAN_CLOCK      (16000000 / 2)
+
+/*
+ * The board configuration is probably following:
+ * RX1 is connected to ground.
+ * TX1 is not connected.
+ * CLKO is not connected.
+ * Setting the OCR register to 0xDA is a good idea.
+ * This means  normal output mode , push-pull and the correct polarity.
+ */
+#define KVASER_PCI_OCR            (OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL)
+
+/*
+ * In the CDR register, you should set CBP to 1.
+ * You will probably also want to set the clock divider value to 0
+ * (meaning divide-by-2), the Pelican bit, and the clock-off bit
+ * (you will have no need for CLKOUT anyway).
+ */
+#define KVASER_PCI_CDR            (CDR_CBP | CDR_CLKOUT_MASK)
+
+/*
+ * These register values are valid for revision 14 of the Xilinx logic.
+ */
+#define XILINX_VERINT             7   /* Lower nibble simulate interrupts,
+					 high nibble version number. */
+
+#define XILINX_PRESUMED_VERSION   14
+
+/*
+ * Important S5920 registers
+ */
+#define S5920_INTCSR              0x38
+#define S5920_PTCR                0x60
+#define INTCSR_ADDON_INTENABLE_M  0x2000
+
+
+#define KVASER_PCI_PORT_BYTES     0x20
+
+#define PCI_CONFIG_PORT_SIZE      0x80      /* size of the config io-memory */
+#define PCI_PORT_SIZE             0x80      /* size of a channel io-memory */
+#define PCI_PORT_XILINX_SIZE      0x08      /* size of a xilinx io-memory */
+
+#define KVASER_PCI_VENDOR_ID1     0x10e8    /* the PCI device and vendor IDs */
+#define KVASER_PCI_DEVICE_ID1     0x8406
+
+#define KVASER_PCI_VENDOR_ID2     0x1a07    /* the PCI device and vendor IDs */
+#define KVASER_PCI_DEVICE_ID2     0x0008
+
+static struct pci_device_id kvaser_pci_tbl[] = {
+	{KVASER_PCI_VENDOR_ID1, KVASER_PCI_DEVICE_ID1, PCI_ANY_ID, PCI_ANY_ID,},
+	{KVASER_PCI_VENDOR_ID2, KVASER_PCI_DEVICE_ID2, PCI_ANY_ID, PCI_ANY_ID,},
+	{ 0,}
+};
+
+MODULE_DEVICE_TABLE(pci, kvaser_pci_tbl);
+
+static u8 kvaser_pci_read_reg(const struct net_device *dev, int port)
+{
+	return ioread8((void __iomem *)(dev->base_addr + port));
+}
+
+static void kvaser_pci_write_reg(const struct net_device *dev, int port, u8 val)
+{
+	iowrite8(val, (void __iomem *)(dev->base_addr + port));
+}
+
+static void kvaser_pci_disable_irq(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct kvaser_pci *board = priv->priv;
+	u32 tmp;
+
+	/* Disable interrupts from card */
+	tmp = ioread32(board->conf_addr + S5920_INTCSR);
+	tmp &= ~INTCSR_ADDON_INTENABLE_M;
+	iowrite32(tmp, board->conf_addr + S5920_INTCSR);
+}
+
+static void kvaser_pci_enable_irq(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct kvaser_pci *board = priv->priv;
+	u32 tmp;
+
+	/* Enable interrupts from card */
+	tmp = ioread32(board->conf_addr + S5920_INTCSR);
+	tmp |= INTCSR_ADDON_INTENABLE_M;
+	iowrite32(tmp, board->conf_addr + S5920_INTCSR);
+}
+
+static int number_of_sja1000_chip(void __iomem *base_addr)
+{
+	u8 status;
+	int i;
+
+	for (i = 0; i < MAX_NO_OF_CHANNELS; i++) {
+		/* reset chip */
+		iowrite8(MOD_RM, base_addr +
+			 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);
+		status = ioread8(base_addr +
+				 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);
+		udelay(10);
+		/* check reset bit */
+		if (!(status & MOD_RM))
+			break;
+	}
+
+	return i;
+}
+
+static void kvaser_pci_del_chan(struct net_device *dev)
+{
+	struct sja1000_priv *priv;
+	struct kvaser_pci *board;
+	int i;
+
+	if (!dev)
+		return;
+	priv = netdev_priv(dev);
+	if (!priv)
+		return;
+	board = priv->priv;
+	if (!board)
+		return;
+
+	dev_info(&board->pci_dev->dev, "Removing device %s\n",
+		 dev->name);
+
+	for (i = 0; i < board->no_channels - 1; i++) {
+		if (board->slave_dev[i]) {
+			dev_info(&board->pci_dev->dev, "Removing device %s\n",
+				 board->slave_dev[i]->name);
+			unregister_sja1000dev(board->slave_dev[i]);
+			free_sja1000dev(board->slave_dev[i]);
+		}
+	}
+	unregister_sja1000dev(dev);
+
+	/* Disable PCI interrupts */
+	kvaser_pci_disable_irq(dev);
+
+	pci_iounmap(board->pci_dev, (void __iomem *)dev->base_addr);
+	pci_iounmap(board->pci_dev, board->conf_addr);
+	pci_iounmap(board->pci_dev, board->res_addr);
+
+	free_sja1000dev(dev);
+}
+
+static int kvaser_pci_add_chan(struct pci_dev *pdev, int channel,
+			       struct net_device **master_dev,
+			       void __iomem *conf_addr,
+			       void __iomem *res_addr,
+			       unsigned long base_addr)
+{
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+	struct kvaser_pci *board;
+	int err, init_step;
+
+	dev = alloc_sja1000dev(sizeof(struct kvaser_pci));
+	if (dev == NULL)
+		return -ENOMEM;
+
+	priv = netdev_priv(dev);
+	board = priv->priv;
+
+	board->pci_dev = pdev;
+	board->channel = channel;
+
+	/*S5920*/
+	board->conf_addr = conf_addr;
+
+	/*XILINX board wide address*/
+	board->res_addr = res_addr;
+
+	if (channel == 0) {
+		board->xilinx_ver =
+			ioread8(board->res_addr + XILINX_VERINT) >> 4;
+		init_step = 2;
+
+		/* Assert PTADR# - we're in passive mode so the other bits are
+		   not important */
+		iowrite32(0x80808080UL, board->conf_addr + S5920_PTCR);
+
+		/* Disable interrupts from card */
+		kvaser_pci_disable_irq(dev);
+		/* Enable interrupts from card */
+		kvaser_pci_enable_irq(dev);
+	} else {
+		struct sja1000_priv *master_priv = netdev_priv(*master_dev);
+		struct kvaser_pci *master_board = master_priv->priv;
+		master_board->slave_dev[channel - 1] = dev;
+		master_board->no_channels = channel + 1;
+		board->xilinx_ver = master_board->xilinx_ver;
+	}
+
+	dev->base_addr = base_addr + channel * KVASER_PCI_PORT_BYTES;
+
+	priv->read_reg = kvaser_pci_read_reg;
+	priv->write_reg = kvaser_pci_write_reg;
+
+	priv->can.clock.freq = KVASER_PCI_CAN_CLOCK;
+
+	priv->ocr = KVASER_PCI_OCR;
+	priv->cdr = KVASER_PCI_CDR;
+
+	/* Register and setup interrupt handling */
+	dev->irq = pdev->irq;
+	init_step = 4;
+
+	dev_info(&pdev->dev, "base_addr=%#lx conf_addr=%p irq=%d\n",
+		 dev->base_addr, board->conf_addr, dev->irq);
+
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	/* Register SJA1000 device */
+	err = register_sja1000dev(dev);
+	if (err) {
+		dev_err(&pdev->dev, "Registering device failed (err=%d)\n",
+			err);
+		goto failure;
+	}
+
+	if (channel == 0)
+		*master_dev = dev;
+
+	return 0;
+
+failure:
+	kvaser_pci_del_chan(dev);
+	return err;
+}
+
+static int __devinit kvaser_pci_init_one(struct pci_dev *pdev,
+					 const struct pci_device_id *ent)
+{
+	int err;
+	struct net_device *master_dev = NULL;
+	struct sja1000_priv *priv;
+	struct kvaser_pci *board;
+	int no_channels;
+	void __iomem *base_addr = NULL;
+	void __iomem *conf_addr = NULL;
+	void __iomem *res_addr = NULL;
+	int i;
+
+	dev_info(&pdev->dev, "initializing device %04x:%04x\n",
+		 pdev->vendor, pdev->device);
+
+	err = pci_enable_device(pdev);
+	if (err)
+		goto failure;
+
+	err = pci_request_regions(pdev, DRV_NAME);
+	if (err)
+		goto failure_release_pci;
+
+	/*S5920*/
+	conf_addr = pci_iomap(pdev, 0, PCI_CONFIG_PORT_SIZE);
+	if (conf_addr == NULL) {
+		err = -ENODEV;
+		goto failure_iounmap;
+	}
+
+	/*XILINX board wide address*/
+	res_addr = pci_iomap(pdev, 2, PCI_PORT_XILINX_SIZE);
+	if (res_addr == NULL) {
+		err = -ENOMEM;
+		goto failure_iounmap;
+	}
+
+	base_addr = pci_iomap(pdev, 1, PCI_PORT_SIZE);
+	if (base_addr == NULL) {
+		err = -ENOMEM;
+		goto failure_iounmap;
+	}
+
+	no_channels = number_of_sja1000_chip(base_addr);
+	if (no_channels == 0) {
+		err = -ENOMEM;
+		goto failure_iounmap;
+	}
+
+	for (i = 0; i < no_channels; i++) {
+		err = kvaser_pci_add_chan(pdev, i, &master_dev,
+					  conf_addr, res_addr,
+					  (unsigned long)base_addr);
+		if (err)
+			goto failure_cleanup;
+	}
+
+	priv = netdev_priv(master_dev);
+	board = priv->priv;
+
+	dev_info(&pdev->dev, "xilinx version=%d number of channels=%d\n",
+		 board->xilinx_ver, board->no_channels);
+
+	pci_set_drvdata(pdev, master_dev);
+	return 0;
+
+failure_cleanup:
+	kvaser_pci_del_chan(master_dev);
+
+failure_iounmap:
+	if (conf_addr == NULL)
+		pci_iounmap(pdev, conf_addr);
+	if (res_addr == NULL)
+		pci_iounmap(pdev, res_addr);
+	if (base_addr == NULL)
+		pci_iounmap(pdev, base_addr);
+
+	pci_release_regions(pdev);
+
+failure_release_pci:
+	pci_disable_device(pdev);
+
+failure:
+	return err;
+
+}
+
+static void __devexit kvaser_pci_remove_one(struct pci_dev *pdev)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+
+	kvaser_pci_del_chan(dev);
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+	pci_set_drvdata(pdev, NULL);
+}
+
+static struct pci_driver kvaser_pci_driver = {
+	.name = DRV_NAME,
+	.id_table = kvaser_pci_tbl,
+	.probe = kvaser_pci_init_one,
+	.remove = __devexit_p(kvaser_pci_remove_one),
+};
+
+static int __init kvaser_pci_init(void)
+{
+	return pci_register_driver(&kvaser_pci_driver);
+}
+
+static void __exit kvaser_pci_exit(void)
+{
+	pci_unregister_driver(&kvaser_pci_driver);
+}
+
+module_init(kvaser_pci_init);
+module_exit(kvaser_pci_exit);

Re: [PATCH v2 7/7] [PATCH 7/8] can: SJA1000 driver for Kvaser PCI cards

[Jonathan Corbet]

On Tue, 12 May 2009 11:28:04 +0200
Wolfgang Grandegger <wg@grandegger.com> wrote:

> The patch adds support for the PCI cards: PCIcan and PCIcanx
> (1, 2 or 4 channel) from Kvaser (http://www.kvaser.com).

> +static void kvaser_pci_disable_irq(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	struct kvaser_pci *board = priv->priv;
> +	u32 tmp;

I've seen certain reviewers getting grumpy about variables called "tmp"
recently.  

> +	/* Disable interrupts from card */
> +	tmp = ioread32(board->conf_addr + S5920_INTCSR);
> +	tmp &= ~INTCSR_ADDON_INTENABLE_M;
> +	iowrite32(tmp, board->conf_addr + S5920_INTCSR);
> +}

[...]

> +static int number_of_sja1000_chip(void __iomem *base_addr)
> +{
> +	u8 status;
> +	int i;
> +
> +	for (i = 0; i < MAX_NO_OF_CHANNELS; i++) {
> +		/* reset chip */
> +		iowrite8(MOD_RM, base_addr +
> +			 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);
> +		status = ioread8(base_addr +
> +				 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);
> +		udelay(10);
> +		/* check reset bit */
> +		if (!(status & MOD_RM))
> +			break;

Why would you delay before checking status?  It ain't gonna change.

> +	}
> +
> +	return i;
> +}
> +
> +static void kvaser_pci_del_chan(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv;
> +	struct kvaser_pci *board;
> +	int i;
> +
> +	if (!dev)
> +		return;
> +	priv = netdev_priv(dev);
> +	if (!priv)
> +		return;

Can this happen?

> +	board = priv->priv;
> +	if (!board)
> +		return;
> +
> +	dev_info(&board->pci_dev->dev, "Removing device %s\n",
> +		 dev->name);
> +
> +	for (i = 0; i < board->no_channels - 1; i++) {
> +		if (board->slave_dev[i]) {
> +			dev_info(&board->pci_dev->dev, "Removing device %s\n",
> +				 board->slave_dev[i]->name);
> +			unregister_sja1000dev(board->slave_dev[i]);
> +			free_sja1000dev(board->slave_dev[i]);
> +		}
> +	}
> +	unregister_sja1000dev(dev);
> +
> +	/* Disable PCI interrupts */
> +	kvaser_pci_disable_irq(dev);

It seems to me like you might want to disable interrupts *before* tearing
down all that structure?  What happens if it interrupts after the sja1000
layer has forgotten about it?

> +	pci_iounmap(board->pci_dev, (void __iomem *)dev->base_addr);
> +	pci_iounmap(board->pci_dev, board->conf_addr);
> +	pci_iounmap(board->pci_dev, board->res_addr);
> +
> +	free_sja1000dev(dev);
> +}
> +
> +static int kvaser_pci_add_chan(struct pci_dev *pdev, int channel,
> +			       struct net_device **master_dev,
> +			       void __iomem *conf_addr,
> +			       void __iomem *res_addr,
> +			       unsigned long base_addr)
> +{
> +	struct net_device *dev;
> +	struct sja1000_priv *priv;
> +	struct kvaser_pci *board;
> +	int err, init_step;
> +
> +	dev = alloc_sja1000dev(sizeof(struct kvaser_pci));
> +	if (dev == NULL)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(dev);

Here you don't have the !priv check.

> +	board = priv->priv;
> +
> +	board->pci_dev = pdev;
> +	board->channel = channel;
> +
> +	/*S5920*/
> +	board->conf_addr = conf_addr;
> +
> +	/*XILINX board wide address*/
> +	board->res_addr = res_addr;
> +
> +	if (channel == 0) {
> +		board->xilinx_ver =
> +			ioread8(board->res_addr + XILINX_VERINT) >> 4;
> +		init_step = 2;
> +
> +		/* Assert PTADR# - we're in passive mode so the other bits are
> +		   not important */
> +		iowrite32(0x80808080UL, board->conf_addr + S5920_PTCR);
> +
> +		/* Disable interrupts from card */
> +		kvaser_pci_disable_irq(dev);
> +		/* Enable interrupts from card */
> +		kvaser_pci_enable_irq(dev);

I'm sure there's a perfectly good reason for the disable/enable dance.
Presumably the hardware needs it or it misbehaves?  Worth commenting.

> +	} else {
> +		struct sja1000_priv *master_priv = netdev_priv(*master_dev);
> +		struct kvaser_pci *master_board = master_priv->priv;
> +		master_board->slave_dev[channel - 1] = dev;
> +		master_board->no_channels = channel + 1;
> +		board->xilinx_ver = master_board->xilinx_ver;
> +	}
> +
> +	dev->base_addr = base_addr + channel * KVASER_PCI_PORT_BYTES;
> +
> +	priv->read_reg = kvaser_pci_read_reg;
> +	priv->write_reg = kvaser_pci_write_reg;
> +
> +	priv->can.clock.freq = KVASER_PCI_CAN_CLOCK;
> +
> +	priv->ocr = KVASER_PCI_OCR;
> +	priv->cdr = KVASER_PCI_CDR;
> +
> +	/* Register and setup interrupt handling */
> +	dev->irq = pdev->irq;
> +	init_step = 4;
> +
> +	dev_info(&pdev->dev, "base_addr=%#lx conf_addr=%p irq=%d\n",
> +		 dev->base_addr, board->conf_addr, dev->irq);
> +
> +	SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +	/* Register SJA1000 device */
> +	err = register_sja1000dev(dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Registering device failed (err=%d)\n",
> +			err);
> +		goto failure;
> +	}
> +
> +	if (channel == 0)
> +		*master_dev = dev;
> +
> +	return 0;
> +
> +failure:
> +	kvaser_pci_del_chan(dev);
> +	return err;
> +}
> +
> +static int __devinit kvaser_pci_init_one(struct pci_dev *pdev,
> +					 const struct pci_device_id *ent)
> +{
> +	int err;
> +	struct net_device *master_dev = NULL;
> +	struct sja1000_priv *priv;
> +	struct kvaser_pci *board;
> +	int no_channels;
> +	void __iomem *base_addr = NULL;
> +	void __iomem *conf_addr = NULL;
> +	void __iomem *res_addr = NULL;
> +	int i;
> +
> +	dev_info(&pdev->dev, "initializing device %04x:%04x\n",
> +		 pdev->vendor, pdev->device);
> +
> +	err = pci_enable_device(pdev);
> +	if (err)
> +		goto failure;
> +
> +	err = pci_request_regions(pdev, DRV_NAME);
> +	if (err)
> +		goto failure_release_pci;
> +
> +	/*S5920*/
> +	conf_addr = pci_iomap(pdev, 0, PCI_CONFIG_PORT_SIZE);
> +	if (conf_addr == NULL) {
> +		err = -ENODEV;
> +		goto failure_iounmap;

Why go there?  You know there's nothing to unmap.

> +	}
> +
> +	/*XILINX board wide address*/
> +	res_addr = pci_iomap(pdev, 2, PCI_PORT_XILINX_SIZE);
> +	if (res_addr == NULL) {
> +		err = -ENOMEM;
> +		goto failure_iounmap;
> +	}
> +
> +	base_addr = pci_iomap(pdev, 1, PCI_PORT_SIZE);
> +	if (base_addr == NULL) {
> +		err = -ENOMEM;
> +		goto failure_iounmap;
> +	}
> +
> +	no_channels = number_of_sja1000_chip(base_addr);
> +	if (no_channels == 0) {
> +		err = -ENOMEM;
> +		goto failure_iounmap;
> +	}
> +
> +	for (i = 0; i < no_channels; i++) {
> +		err = kvaser_pci_add_chan(pdev, i, &master_dev,
> +					  conf_addr, res_addr,
> +					  (unsigned long)base_addr);
> +		if (err)
> +			goto failure_cleanup;
> +	}
> +
> +	priv = netdev_priv(master_dev);
> +	board = priv->priv;
> +
> +	dev_info(&pdev->dev, "xilinx version=%d number of channels=%d\n",
> +		 board->xilinx_ver, board->no_channels);
> +
> +	pci_set_drvdata(pdev, master_dev);
> +	return 0;
> +
> +failure_cleanup:
> +	kvaser_pci_del_chan(master_dev);
> +
> +failure_iounmap:
> +	if (conf_addr == NULL)
> +		pci_iounmap(pdev, conf_addr);
> +	if (res_addr == NULL)
> +		pci_iounmap(pdev, res_addr);
> +	if (base_addr == NULL)
> +		pci_iounmap(pdev, base_addr);
> +
> +	pci_release_regions(pdev);
> +
> +failure_release_pci:
> +	pci_disable_device(pdev);
> +
> +failure:
> +	return err;
> +
> +}

jon

Re: [PATCH v2 7/7] [PATCH 7/8] can: SJA1000 driver for Kvaser PCI cards

[Wolfgang Grandegger]

Jonathan Corbet wrote:
> On Tue, 12 May 2009 11:28:04 +0200
> Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
>> The patch adds support for the PCI cards: PCIcan and PCIcanx
>> (1, 2 or 4 channel) from Kvaser (http://www.kvaser.com).
> 
>> +static void kvaser_pci_disable_irq(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	struct kvaser_pci *board = priv->priv;
>> +	u32 tmp;
> 
> I've seen certain reviewers getting grumpy about variables called "tmp"
> recently.  

No problem. I'm going to change the name to something more reasonable.

>> +	/* Disable interrupts from card */
>> +	tmp = ioread32(board->conf_addr + S5920_INTCSR);
>> +	tmp &= ~INTCSR_ADDON_INTENABLE_M;
>> +	iowrite32(tmp, board->conf_addr + S5920_INTCSR);
>> +}
> 
> [...]
> 
>> +static int number_of_sja1000_chip(void __iomem *base_addr)
>> +{
>> +	u8 status;
>> +	int i;
>> +
>> +	for (i = 0; i < MAX_NO_OF_CHANNELS; i++) {
>> +		/* reset chip */
>> +		iowrite8(MOD_RM, base_addr +
>> +			 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);
>> +		status = ioread8(base_addr +
>> +				 (i * KVASER_PCI_PORT_BYTES) + REG_MOD);
>> +		udelay(10);
>> +		/* check reset bit */
>> +		if (!(status & MOD_RM))
>> +			break;
> 
> Why would you delay before checking status?  It ain't gonna change.

Right.

>> +	}
>> +
>> +	return i;
>> +}
>> +
>> +static void kvaser_pci_del_chan(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv;
>> +	struct kvaser_pci *board;
>> +	int i;
>> +
>> +	if (!dev)
>> +		return;
>> +	priv = netdev_priv(dev);
>> +	if (!priv)
>> +		return;
> 
> Can this happen?

No, it's a bug if it happens.

>> +	board = priv->priv;
>> +	if (!board)
>> +		return;
>> +
>> +	dev_info(&board->pci_dev->dev, "Removing device %s\n",
>> +		 dev->name);
>> +
>> +	for (i = 0; i < board->no_channels - 1; i++) {
>> +		if (board->slave_dev[i]) {
>> +			dev_info(&board->pci_dev->dev, "Removing device %s\n",
>> +				 board->slave_dev[i]->name);
>> +			unregister_sja1000dev(board->slave_dev[i]);
>> +			free_sja1000dev(board->slave_dev[i]);
>> +		}
>> +	}
>> +	unregister_sja1000dev(dev);
>> +
>> +	/* Disable PCI interrupts */
>> +	kvaser_pci_disable_irq(dev);
> 
> It seems to me like you might want to disable interrupts *before* tearing
> down all that structure?  What happens if it interrupts after the sja1000
> layer has forgotten about it?

Right.

>> +	pci_iounmap(board->pci_dev, (void __iomem *)dev->base_addr);
>> +	pci_iounmap(board->pci_dev, board->conf_addr);
>> +	pci_iounmap(board->pci_dev, board->res_addr);
>> +
>> +	free_sja1000dev(dev);
>> +}
>> +
>> +static int kvaser_pci_add_chan(struct pci_dev *pdev, int channel,
>> +			       struct net_device **master_dev,
>> +			       void __iomem *conf_addr,
>> +			       void __iomem *res_addr,
>> +			       unsigned long base_addr)
>> +{
>> +	struct net_device *dev;
>> +	struct sja1000_priv *priv;
>> +	struct kvaser_pci *board;
>> +	int err, init_step;
>> +
>> +	dev = alloc_sja1000dev(sizeof(struct kvaser_pci));
>> +	if (dev == NULL)
>> +		return -ENOMEM;
>> +
>> +	priv = netdev_priv(dev);
> 
> Here you don't have the !priv check.
> 
>> +	board = priv->priv;
>> +
>> +	board->pci_dev = pdev;
>> +	board->channel = channel;
>> +
>> +	/*S5920*/
>> +	board->conf_addr = conf_addr;
>> +
>> +	/*XILINX board wide address*/
>> +	board->res_addr = res_addr;
>> +
>> +	if (channel == 0) {
>> +		board->xilinx_ver =
>> +			ioread8(board->res_addr + XILINX_VERINT) >> 4;
>> +		init_step = 2;
>> +
>> +		/* Assert PTADR# - we're in passive mode so the other bits are
>> +		   not important */
>> +		iowrite32(0x80808080UL, board->conf_addr + S5920_PTCR);
>> +
>> +		/* Disable interrupts from card */
>> +		kvaser_pci_disable_irq(dev);
>> +		/* Enable interrupts from card */
>> +		kvaser_pci_enable_irq(dev);
> 
> I'm sure there's a perfectly good reason for the disable/enable dance.
> Presumably the hardware needs it or it misbehaves?  Worth commenting.

Hm, I think the hardware does not need it. I will check with original other.

>> +	} else {
>> +		struct sja1000_priv *master_priv = netdev_priv(*master_dev);
>> +		struct kvaser_pci *master_board = master_priv->priv;
>> +		master_board->slave_dev[channel - 1] = dev;
>> +		master_board->no_channels = channel + 1;
>> +		board->xilinx_ver = master_board->xilinx_ver;
>> +	}
>> +
>> +	dev->base_addr = base_addr + channel * KVASER_PCI_PORT_BYTES;
>> +
>> +	priv->read_reg = kvaser_pci_read_reg;
>> +	priv->write_reg = kvaser_pci_write_reg;
>> +
>> +	priv->can.clock.freq = KVASER_PCI_CAN_CLOCK;
>> +
>> +	priv->ocr = KVASER_PCI_OCR;
>> +	priv->cdr = KVASER_PCI_CDR;
>> +
>> +	/* Register and setup interrupt handling */
>> +	dev->irq = pdev->irq;
>> +	init_step = 4;
>> +
>> +	dev_info(&pdev->dev, "base_addr=%#lx conf_addr=%p irq=%d\n",
>> +		 dev->base_addr, board->conf_addr, dev->irq);
>> +
>> +	SET_NETDEV_DEV(dev, &pdev->dev);
>> +
>> +	/* Register SJA1000 device */
>> +	err = register_sja1000dev(dev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "Registering device failed (err=%d)\n",
>> +			err);
>> +		goto failure;
>> +	}
>> +
>> +	if (channel == 0)
>> +		*master_dev = dev;
>> +
>> +	return 0;
>> +
>> +failure:
>> +	kvaser_pci_del_chan(dev);
>> +	return err;
>> +}
>> +
>> +static int __devinit kvaser_pci_init_one(struct pci_dev *pdev,
>> +					 const struct pci_device_id *ent)
>> +{
>> +	int err;
>> +	struct net_device *master_dev = NULL;
>> +	struct sja1000_priv *priv;
>> +	struct kvaser_pci *board;
>> +	int no_channels;
>> +	void __iomem *base_addr = NULL;
>> +	void __iomem *conf_addr = NULL;
>> +	void __iomem *res_addr = NULL;
>> +	int i;
>> +
>> +	dev_info(&pdev->dev, "initializing device %04x:%04x\n",
>> +		 pdev->vendor, pdev->device);
>> +
>> +	err = pci_enable_device(pdev);
>> +	if (err)
>> +		goto failure;
>> +
>> +	err = pci_request_regions(pdev, DRV_NAME);
>> +	if (err)
>> +		goto failure_release_pci;
>> +
>> +	/*S5920*/
>> +	conf_addr = pci_iomap(pdev, 0, PCI_CONFIG_PORT_SIZE);
>> +	if (conf_addr == NULL) {
>> +		err = -ENODEV;
>> +		goto failure_iounmap;
> 
> Why go there?  You know there's nothing to unmap.

OK.

I will resend a revised patch a.s.a.p.

Thanks,

Wolfgang.