* [PATCH v2 0/1] can: ucan: add driver for Theobroma Systems UCAN devices @ 2018-03-13 17:35 Jakob Unterwurzacher 2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher 2018-03-13 17:40 ` [PATCH v2 0/1] Open questions Jakob Unterwurzacher 0 siblings, 2 replies; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-13 17:35 UTC (permalink / raw) To: jakob.unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-kernel This is the second iteration of the Theobroma Systems CAN/USB adapter driver. Thanks to feedback from the list and additonal internal stress-testing, the driver, the wire protocol and the device firmware have been improved. Among other changes, error states are now handled, and an explicit tx completion message has been added. A few questions and review comments are still open. I will post an email gathering all answers in a reply to this cover letter. Jakob Unterwurzacher (1): can: ucan: add driver for Theobroma Systems UCAN devices Documentation/networking/can_ucan_protocol.rst | 315 +++++ Documentation/networking/index.rst | 1 + drivers/net/can/usb/Kconfig | 10 + drivers/net/can/usb/Makefile | 1 + drivers/net/can/usb/ucan.c | 1587 ++++++++++++++++++++++++ 5 files changed, 1914 insertions(+) create mode 100644 Documentation/networking/can_ucan_protocol.rst create mode 100644 drivers/net/can/usb/ucan.c -- 2.11.0 Cc: Martin Elshuber <martin.elshuber@theobroma-systems.com> Cc: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> Cc: Wolfgang Grandegger <wg@grandegger.com> Cc: Marc Kleine-Budde <mkl@pengutronix.de> Cc: linux-can@vger.kernel.org Cc: linux-kernel@vger.kernel.org ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:35 [PATCH v2 0/1] can: ucan: add driver for Theobroma Systems UCAN devices Jakob Unterwurzacher @ 2018-03-13 17:35 ` Jakob Unterwurzacher 2018-03-13 17:44 ` Marc Kleine-Budde ` (4 more replies) 2018-03-13 17:40 ` [PATCH v2 0/1] Open questions Jakob Unterwurzacher 1 sibling, 5 replies; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-13 17:35 UTC (permalink / raw) To: jakob.unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-kernel The UCAN driver supports the microcontroller-based USB/CAN adapters from Theobroma Systems. There are two form-factors that run essentially the same firmware: * Seal: standalone USB stick ( https://www.theobroma-systems.com/seal ) * Mule: integrated on the PCB of various System-on-Modules from Theobroma Systems like the A31-µQ7 and the RK3399-Q7 ( https://www.theobroma-systems.com/rk3399-q7 ) The USB wire protocol has been designed to be as generic and hardware-indendent as possible in the hope of being useful for implementation on other microcontrollers. Signed-off-by: Martin Elshuber <martin.elshuber@theobroma-systems.com> Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> --- Documentation/networking/can_ucan_protocol.rst | 315 +++++ Documentation/networking/index.rst | 1 + drivers/net/can/usb/Kconfig | 10 + drivers/net/can/usb/Makefile | 1 + drivers/net/can/usb/ucan.c | 1587 ++++++++++++++++++++++++ 5 files changed, 1914 insertions(+) create mode 100644 Documentation/networking/can_ucan_protocol.rst create mode 100644 drivers/net/can/usb/ucan.c diff --git a/Documentation/networking/can_ucan_protocol.rst b/Documentation/networking/can_ucan_protocol.rst new file mode 100644 index 000000000000..d859b36200b4 --- /dev/null +++ b/Documentation/networking/can_ucan_protocol.rst @@ -0,0 +1,315 @@ +================= +The UCAN Protocol +================= + +UCAN is the protocol used by the microcontroller-based USB-CAN +adapter that is integrated on System-on-Modules from Theobroma Systems +and that is also available as a standalone USB stick. + +The UCAN protocol has been designed to be hardware-independent. +It is modeled closely after how Linux represents CAN devices +internally. All multi-byte integers are encoded as Little Endian. + +All structures mentioned in this document are defined in +``drivers/net/can/usb/ucan.c``. + +USB Endpoints +============= + +UCAN devices use three USB endpoints: + +CONTROL endpoint + The driver sends device management commands on this endpoint + +IN endpoint + The device sends CAN data frames and CAN error frames + +OUT endpoint + The driver sends CAN data frames on the out endpoint + + +CONTROL Messages +================ + +UCAN devices are configured using vendor requests on the control pipe. + +To support multiple CAN interfaces in a single USB device all +configuration commands target the corresponding interface in the USB +descriptor. + +The driver uses ``ucan_ctrl_command_in/out`` and +``ucan_device_request_in`` to deliver commands to the device. + +Setup Packet +------------ + +================= ===================================================== +``bmRequestType`` Direction | Vendor | (Interface or Device) +``bRequest`` Command Number +``wValue`` Subcommand Number (16 Bit) or 0 if not used +``wIndex`` USB Interface Index (0 for device commands) +``wLength`` * Host to Device - Number of bytes to transmit + * Device to Host - Maximum Number of bytes to + receive. If the device send less. Commom ZLP + semantics are used. +================= ===================================================== + +Error Handling +-------------- + +The device indicates failed control commands by stalling the +pipe. + +Device Commands +--------------- + +UCAN_DEVICE_GET_FW_STRING +~~~~~~~~~~~~~~~~~~~~~~~~~ + +*Dev2Host; optional* + +Request the device firmware string. + + +Interface Commands +------------------ + +UCAN_COMMAND_START +~~~~~~~~~~~~~~~~~~ + +*Host2Dev; mandatory* + +Bring the CAN interface up. + +Payload Format + ``ucan_ctl_payload_t.cmd_start`` + +==== ============================ +mode or mask of ``UCAN_MODE_*`` +==== ============================ + +UCAN_COMMAND_STOP +~~~~~~~~~~~~~~~~~~ + +*Host2Dev; mandatory* + +Stop the CAN interface + +Payload Format + *empty* + +UCAN_COMMAND_RESET +~~~~~~~~~~~~~~~~~~ + +*Host2Dev; mandatory* + +Reset the CAN controller (including error counters) + +Payload Format + *empty* + +UCAN_COMMAND_GET +~~~~~~~~~~~~~~~~ + +*Host2Dev; mandatory* + +Get Information from the Device + +Subcommands +^^^^^^^^^^^ + +UCAN_COMMAND_GET_INFO + Request the device information structure ``ucan_ctl_payload_t.device_info``. + + See the ``device_info`` field for details, and + ``uapi/linux/can/netlink.h`` for an explanation of the + ``can_bittiming fields``. + + Payload Format + ``ucan_ctl_payload_t.device_info`` + +UCAN_COMMAND_GET_PROTOCOL_VERSION + + Request the device protocol version + ``ucan_ctl_payload_t.protocol_version``. The current protocol version is 3. + + Payload Format + ``ucan_ctl_payload_t.protocol_version`` + +.. note:: Devices that do not implement this command use the old + protocol version 1 + +UCAN_COMMAND_SET_BITTIMING +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +*Host2Dev; mandatory* + +Setup bittiming by sending the the structure +``ucan_ctl_payload_t.cmd_set_bittiming`` (see ``struct bittiming`` for +details) + +Payload Format + ``ucan_ctl_payload_t.cmd_set_bittiming``. + +UCAN_SLEEP/WAKE +~~~~~~~~~~~~~~~ + +*Host2Dev; optional* + +Configure sleep and wake modes. Not yet supported by the driver. + +UCAN_FILTER +~~~~~~~~~~~ + +*Host2Dev; optional* + +Setup hardware CAN filters. Not yet supported by the driver. + +Allowed interface commands +-------------------------- + +================== =================== ================== +Legal Device State Command New Device State +================== =================== ================== +stopped SET_BITTIMING stopped +stopped START started +started STOP or RESET stopped +stopped STOP or RESET stopped +started RESTART started +any GET *no change* +================== =================== ================== + +IN Message Format +================= + +A data packet on the USB IN endpoint contains one or more +``ucan_message_in`` values. If multiple messages are batched in a USB +data packet, the ``len`` field can be used to jump to the next +``ucan_message_in`` value (take care to sanity-check the ``len`` value +against the actual data size). + +.. _can_ucan_in_message_len: + +``len`` field +------------- + +Each ``ucan_message_in`` must be aligned to a 4-byte boundary (relative +to the start of the start of the data buffer). That means that there +may be padding bytes between multiple ``ucan_message_in`` values: + +.. code:: + + +----------------------------+ < 0 + | | + | struct ucan_message_in | + | | + +----------------------------+ < len + [padding] + +----------------------------+ < round_up(len, 4) + | | + | struct ucan_message_in | + | | + +----------------------------+ + [...] + +``type`` field +-------------- + +The ``type`` field specifies the type of the message. + +UCAN_IN_RX +~~~~~~~~~~ + +``subtype`` + zero + +Data received from the CAN bus (ID + payload). + +UCAN_IN_TX_COMPLETE +~~~~~~~~~~~~~~~~~~~ + +``subtype`` + zero + +The CAN device has sent a message to the CAN bus. It answers with a +set of echo-ids from previous UCAN_OUT_TX messages + +Flow Control +------------ + +When receiving CAN messages there is no flow control on the USB +buffer. The driver has to handle inbound message quickly enough to +avoid drops. I case the device buffer overflow the condition is +reported by sending corresponding error frames (see +:ref:`can_ucan_error_handling`) + + +OUT Message Format +================== + +A data packet on the USB OUT endpoint contains one or more ``struct +ucan_message_out`` values. If multiple messages are batched into one +data packet, the device uses the ``len`` field to jump to the next +ucan_message_out value. Each ucan_message_out must be aligned to 4 +bytes (relative to the start of the data buffer). The mechanism is +same as described in :ref:`can_ucan_in_message_len`. + +.. code:: + + +----------------------------+ < 0 + | | + | struct ucan_message_out | + | | + +----------------------------+ < len + [padding] + +----------------------------+ < round_up(len, 4) + | | + | struct ucan_message_out | + | | + +----------------------------+ + [...] + +``type`` field +-------------- + +In protocol version 3 only ``UCAN_OUT_TX`` is defined, others are used +only by legacy devices (protocol version 1). + +UCAN_OUT_TX +~~~~~~~~~~~ +``subtype`` + echo id to be replied within a CAN_IN_TX_COMPLETE message + +Transmit a CAN frame. (parameters: ``id``, ``data``) + +Flow Control +------------ + +When the device outbound buffers are full it starts sending *NAKs* on +the *OUT* pipe until more buffers are available. The driver stops the +queue when a certain threshold of out packets are incomplete. + +.. _can_ucan_error_handling: + +CAN Error Handling +================== + +If error reporting is turned on the device encodes errors into CAN +error frames (see ``uapi/linux/can/error.h``) and sends it using the +IN endpoint. The driver updates its error statistics and forwards +it. + +Although UCAN devices can suppress error frames completely, in Linux +the driver is always interested. Hence, the device is always started with +the ``UCAN_MODE_BERR_REPORT`` set. Filtering those messages for the +user space is done by the driver. + +Example Conversation +==================== + +#) Device is connected to USB +#) Host sends command ``UCAN_COMMAND_RESET``, subcmd 0 +#) Host sends command ``UCAN_COMMAND_GET``, subcmd ``UCAN_COMMAND_GET_INFO`` +#) Device sends ``UCAN_IN_DEVICE_INFO`` +#) Host sends command ``UCAN_OUT_SET_BITTIMING`` +#) Host sends command ``UCAN_COMMAND_START``, subcmd 0, mode ``UCAN_MODE_BERR_REPORT`` diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst index 90966c2692d8..18903968cebf 100644 --- a/Documentation/networking/index.rst +++ b/Documentation/networking/index.rst @@ -8,6 +8,7 @@ Contents: batman-adv can + can_ucan_protocol kapi z8530book msg_zerocopy diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig index c36f4bdcbf4f..490cdce1f1da 100644 --- a/drivers/net/can/usb/Kconfig +++ b/drivers/net/can/usb/Kconfig @@ -89,4 +89,14 @@ config CAN_MCBA_USB This driver supports the CAN BUS Analyzer interface from Microchip (http://www.microchip.com/development-tools/). +config CAN_UCAN + tristate "Theobroma Systems UCAN interface" + ---help--- + This driver supports the Theobroma Systems + UCAN USB-CAN interface. + + UCAN is an microcontroller-based USB-CAN interface that + is integrated on System-on-Modules made by Theobroma Systems + (https://www.theobroma-systems.com/som-products). + endmenu diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile index 49ac7b99ba32..4176e8358232 100644 --- a/drivers/net/can/usb/Makefile +++ b/drivers/net/can/usb/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/ obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o +obj-$(CONFIG_CAN_UCAN) += ucan.o diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c new file mode 100644 index 000000000000..61348e8c4747 --- /dev/null +++ b/drivers/net/can/usb/ucan.c @@ -0,0 +1,1587 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* Driver for Theobroma Systems UCAN devices Protocol Version 3 + * + * Copyright (C) 2018 Theobroma Systems Design und Consulting GmbH + * + * + * General Description: + * + * The USB Device uses three Endpoints: + * + * CONTROL Endpoint: Is used the setup the device (start, stop, + * info, configure). + * + * IN Endpoint: The device sends CAN Frame Messages and Device + * Information using the IN endpoint. + * + * OUT Endpoint: The driver sends configuration requests, and CAN + * Frames on the out endpoint. + * + * Error Handling: + * + * If error reporting is turned on the device encodes error into CAN + * error frames (see uapi/linux/can/error.h) and sends it using the + * IN Endpoint. The driver updates statistics and forward it. + */ + +#include <linux/can.h> +#include <linux/can/dev.h> +#include <linux/can/error.h> +#include <linux/module.h> +#include <linux/netdevice.h> +#include <linux/signal.h> +#include <linux/skbuff.h> +#include <linux/slab.h> +#include <linux/usb.h> + +#include <linux/can.h> +#include <linux/can/dev.h> +#include <linux/can/error.h> + +#define UCAN_MAX_RX_URBS 8 +/* the CAN controller needs a while to enable/disable the bus */ +#define UCAN_USB_CTL_PIPE_TIMEOUT 1000 +/* this driver currently supports protocol version 3 only */ +#define UCAN_PROTOCOL_VERSION_MIN 3 +#define UCAN_PROTOCOL_VERSION_MAX 3 + +/* UCAN Message Definitions -------------------------------------------- + * + * ucan_message_out_t and ucan_message_in_t define the messages + * transmitted on the OUT and IN endpoint. + * + * Multibyte fields are transmitted with little endianness + * + * INTR Endpoint: a single uint32_t storing the current space in the fifo + * + * OUT Endpoint: single message of type ucan_message_out_t is + * transmitted on the out endpoint + * + * IN Endpoint: multiple messages ucan_message_in_t concateted in + * the following way: + * + * m[n].len <=> the length if message n(including the header in bytes) + * m[n] is is aligned to a 4 byte boundary, hence + * offset(m[0]) := 0; + * offset(m[n+1]) := offset(m[n]) + (m[n].len + 3) & 3 + * + * this implies that + * offset(m[n]) % 4 <=> 0 + */ + +/* Device Global Commands */ +enum { + UCAN_DEVICE_GET_FW_STRING = 0, +}; + +/* UCAN Commands */ +enum { + /* start the can transceiver - val defines the operation mode */ + UCAN_COMMAND_START = 0, + /* cancel pending transmissions and stop the can transceiver */ + UCAN_COMMAND_STOP = 1, + /* send can transceiver into low-power sleep mode */ + UCAN_COMMAND_SLEEP = 2, + /* wake up can transceiver from low-power sleep mode */ + UCAN_COMMAND_WAKEUP = 3, + /* reset the can transceiver */ + UCAN_COMMAND_RESET = 4, + /* get piece of info from the can transceiver - subcmd defines what + * piece + */ + UCAN_COMMAND_GET = 5, + /* clear or disable hardware filter - subcmd defines which of the two */ + UCAN_COMMAND_FILTER = 6, + /* Setup bittiming */ + UCAN_COMMAND_SET_BITTIMING = 7, + /* recover from bus-off state */ + UCAN_COMMAND_RESTART = 8, +}; + +/* UCAN_COMMAND_START and UCAN_COMMAND_GET_INFO operation modes (bitmap). + * Undefined bits must be set to 0. + */ +enum { + UCAN_MODE_LOOPBACK = (1 << 0), + UCAN_MODE_SILENT = (1 << 1), + UCAN_MODE_3_SAMPLES = (1 << 2), + UCAN_MODE_ONE_SHOT = (1 << 3), + UCAN_MODE_BERR_REPORT = (1 << 4), +}; + +/* UCAN_COMMAND_GET subcommands */ +enum { + UCAN_COMMAND_GET_INFO = 0, + UCAN_COMMAND_GET_PROTOCOL_VERSION = 1, +}; + +/* UCAN_COMMAND_FILTER subcommands */ +enum { + UCAN_FILTER_CLEAR = 0, + UCAN_FILTER_DISABLE = 1, + UCAN_FILTER_ENABLE = 2, +}; + +/* OUT endpoint message types */ +enum { + UCAN_OUT_TX = 2, /* transmit a CAN frame */ +}; + +/* IN endpoint message types */ +enum { + UCAN_IN_TX_COMPLETE = 1, /* CAN frame transmission completed */ + UCAN_IN_RX = 2, /* CAN frame received */ +}; + +struct ucan_ctl_cmd_start { + u16 mode; /* oring any of UCAN_MODE_* */ +} __packed; + +struct ucan_ctl_cmd_set_bittiming { + u32 tq; /* Time quanta (TQ) in nanoseconds */ + u16 brp; /* TQ Prescaler */ + u16 sample_point; /* Samplepoint on tenth percent */ + u8 prop_seg; /* Propagation segment in TQs */ + u8 phase_seg1; /* Phase buffer segment 1 in TQs */ + u8 phase_seg2; /* Phase buffer segment 2 in TQs */ + u8 sjw; /* Synchronisation jump width in TQs */ +} __packed; + +struct ucan_ctl_cmd_device_info { + u32 freq; /* Clock Frequency for tq generation */ + u8 tx_fifo; /* Size of the transmission fifo */ + u8 sjw_max; /* can_bittiming fields... */ + u8 tseg1_min; + u8 tseg1_max; + u8 tseg2_min; + u8 tseg2_max; + u16 brp_inc; + u32 brp_min; + u32 brp_max; /* ...can_bittiming fields */ + u16 ctrlmodes; /* supported control modes */ + u16 hwfilter; /* Number of HW filter banks */ + u16 rxmboxes; /* Number of receive Mailboxes */ +} __packed; + +struct ucan_ctl_cmd_get_protocol_version { + u32 version; +} __packed; + +union ucan_ctl_payload { + /*************************************************** + * Setup Bittiming + * bmRequest == UCAN_COMMAND_START + ***************************************************/ + struct ucan_ctl_cmd_start cmd_start; + /*************************************************** + * Setup Bittiming + * bmRequest == UCAN_COMMAND_SET_BITTIMING + ***************************************************/ + struct ucan_ctl_cmd_set_bittiming cmd_set_bittiming; + /*************************************************** + * Get Device Information + * bmRequest == UCAN_COMMAND_GET; wValue = UCAN_COMMAND_GET_INFO + ***************************************************/ + struct ucan_ctl_cmd_device_info cmd_get_device_info; + /*************************************************** + * Get Protocol Version + * bmRequest == UCAN_COMMAND_GET; + * wValue = UCAN_COMMAND_GET_PROTOCOL_VERSION + ***************************************************/ + struct ucan_ctl_cmd_get_protocol_version cmd_get_protocol_version; + + u8 raw[128]; +} __packed; + +enum { + UCAN_TX_COMPLETE_SUCCESS = (1 << 0), +}; + +/* Transmission Complete within ucan_message_in */ +struct ucan_tx_complete_entry_t { + u8 echo_id; + u8 flags; +} __packed __aligned(0x2); + +/* CAN Data message format within ucan_message_in/out */ +struct ucan_can_msg { + /* note DLC is computed by + * msg.len - sizeof (msg.len) + * - sizeof (msg.type) + * - sizeof (msg.can_msg.id) + */ + u32 id; + + union { + u8 data[CAN_MAX_DLEN]; /* Data of CAN frames */ + u8 dlc; /* RTR dlc */ + }; +} __packed; + +/* OUT Endpoint, outbound messages */ +struct ucan_message_out { + u16 len; /* Length of the content include header */ + u8 type; /* UCAN_OUT_TX and friends */ + u8 subtype; /* command sub type */ + union { + /*************************************************** + * Transmit CAN frame + * (type == UCAN_TX) && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) + * subtype stores the echo id + ***************************************************/ + struct ucan_can_msg can_msg; + } msg; +} __packed __aligned(0x4); + +/* IN Endpoint, inbound messages */ +struct ucan_message_in { + u16 len; /* Length of the content include header */ + u8 type; /* UCAN_IN_RX and friends */ + u8 subtype; /* command sub type */ + + union { + /*************************************************** + * CAN Frame received + * (type == UCAN_IN_RX) + * && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) + ***************************************************/ + struct ucan_can_msg can_msg; + + /*************************************************** + * CAN transmission complete + * (type == UCAN_IN_TX_COMPLETE) + ***************************************************/ + struct ucan_tx_complete_entry_t can_tx_complete_msg[0]; + + } __aligned(0x4) msg; +} __packed; + +/* Macros to calculate message lengths */ +#define UCAN_OUT_HDR_SIZE offsetof(struct ucan_message_out, msg) + +#define UCAN_IN_HDR_SIZE offsetof(struct ucan_message_in, msg) +#define UCAN_IN_LEN(member) (UCAN_OUT_HDR_SIZE + sizeof(member)) + +struct ucan_priv; + +/* Context Information for transmission URBs */ +struct ucan_urb_context { + struct ucan_priv *up; + u32 echo_index; + u8 dlc; + atomic_t allocated; +}; + +/* Information reported by the USB device */ +struct ucan_device_info { + struct can_bittiming_const bittiming_const; + u8 tx_fifo; +}; + +/* Driver private data */ +struct ucan_priv { + struct can_priv can; /* must be the first member */ + + u8 intf_index; + struct usb_device *udev; + struct usb_interface *intf; + struct net_device *netdev; + + struct usb_endpoint_descriptor *out_ep; + struct usb_endpoint_descriptor *in_ep; + + struct usb_anchor rx_urbs; + struct usb_anchor tx_urbs; + + union ucan_ctl_payload *ctl_msg_buffer; + struct ucan_device_info device_info; + + atomic_t available_tx_urbs; + struct ucan_urb_context *tx_contexts; +}; + +static u8 ucan_compute_dlc(u16 len, struct ucan_can_msg *msg) +{ + u16 res = 0; + + if (msg->id & CAN_RTR_FLAG) + res = msg->dlc; + else + res = len - (UCAN_IN_HDR_SIZE + sizeof(msg->id)); + + if (res > CAN_MAX_DLEN) + return -1; + + return res; +} + +static void ucan_release_contexts(struct ucan_priv *up) +{ + if (!up->tx_contexts) + return; + + atomic_set(&up->available_tx_urbs, 0); + + kfree(up->tx_contexts); + up->tx_contexts = NULL; +} + +static int ucan_allocate_contexts(struct ucan_priv *up) +{ + int i; + + /* release contexts if any */ + ucan_release_contexts(up); + + up->tx_contexts = kmalloc_array(up->device_info.tx_fifo, + sizeof(*up->tx_contexts), + GFP_KERNEL); + if (!up->tx_contexts) { + dev_err(&up->udev->dev, "Not enough memory to allocate tx contexts\n"); + return -ENOMEM; + } + + memset(up->tx_contexts, 0, + sizeof(*up->tx_contexts) * up->device_info.tx_fifo); + for (i = 0; i < up->device_info.tx_fifo; i++) { + atomic_set(&up->tx_contexts[i].allocated, 0); + up->tx_contexts[i].up = up; + up->tx_contexts[i].echo_index = i; + } + + atomic_set(&up->available_tx_urbs, up->device_info.tx_fifo); + + return 0; +} + +static struct ucan_urb_context *ucan_allocate_context(struct ucan_priv *up) +{ + int i, allocated, avail; + + if (!up->tx_contexts) + return NULL; + + for (i = 0; i < up->device_info.tx_fifo; i++) { + allocated = atomic_cmpxchg(&up->tx_contexts[i].allocated, 0, 1); + if (allocated == 0) { + avail = atomic_sub_return(1, &up->available_tx_urbs); + if (avail == 0) + netif_stop_queue(up->netdev); + return &up->tx_contexts[i]; + } + } + return NULL; +} + +static void ucan_release_context(struct ucan_priv *up, + struct ucan_urb_context *ctx) +{ + WARN_ON_ONCE(!up->tx_contexts); + if (!up->tx_contexts) + return; + + if (atomic_cmpxchg(&ctx->allocated, 1, 0) == 0) { + dev_warn(&up->udev->dev, + "context %p (#%ld) was not allocated\n", + ctx, ctx - up->tx_contexts); + } else { + atomic_inc(&up->available_tx_urbs); + netif_wake_queue(up->netdev); + } +} + +static int ucan_ctrl_command_out(struct ucan_priv *up, + u8 cmd, + u16 subcmd, + size_t datalen) +{ + if (datalen > sizeof(union ucan_ctl_payload)) + return -ENOMEM; + + return usb_control_msg(up->udev, + usb_sndctrlpipe(up->udev, 0), + cmd, + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE, + cpu_to_le16(subcmd), + up->intf_index, + up->ctl_msg_buffer, + datalen, + UCAN_USB_CTL_PIPE_TIMEOUT); +} + +static int ucan_device_request_in(struct ucan_priv *up, + u8 cmd, + u16 subcmd, + size_t datalen) +{ + if (datalen > sizeof(union ucan_ctl_payload)) + return -ENOMEM; + + return usb_control_msg(up->udev, + usb_rcvctrlpipe(up->udev, 0), + cmd, + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + cpu_to_le16(subcmd), + 0, + up->ctl_msg_buffer, + datalen, + UCAN_USB_CTL_PIPE_TIMEOUT); +} + +/* Parse the device information structure reported by the device and + * setup private variables accordingly + */ +static void ucan_parse_device_info(struct ucan_priv *up, + struct ucan_ctl_cmd_device_info + *ctl_cmd_device_info) +{ + struct can_bittiming_const *bittiming = + &up->device_info.bittiming_const; + u16 ctrlmodes; + + /* store the data */ + up->can.clock.freq = le32_to_cpu(ctl_cmd_device_info->freq); + up->device_info.tx_fifo = ctl_cmd_device_info->tx_fifo; + strcpy(bittiming->name, "ucan"); + bittiming->tseg1_min = ctl_cmd_device_info->tseg1_min; + bittiming->tseg1_max = ctl_cmd_device_info->tseg1_max; + bittiming->tseg2_min = ctl_cmd_device_info->tseg2_min; + bittiming->tseg2_max = ctl_cmd_device_info->tseg2_max; + bittiming->sjw_max = ctl_cmd_device_info->sjw_max; + bittiming->brp_min = le32_to_cpu(ctl_cmd_device_info->brp_min); + bittiming->brp_max = le32_to_cpu(ctl_cmd_device_info->brp_max); + bittiming->brp_inc = le16_to_cpu(ctl_cmd_device_info->brp_inc); + + ctrlmodes = le16_to_cpu(ctl_cmd_device_info->ctrlmodes); + + up->can.ctrlmode_supported = 0; + + if (ctrlmodes & UCAN_MODE_LOOPBACK) + up->can.ctrlmode_supported |= CAN_CTRLMODE_LOOPBACK; + if (ctrlmodes & UCAN_MODE_SILENT) + up->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY; + if (ctrlmodes & UCAN_MODE_3_SAMPLES) + up->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES; + if (ctrlmodes & UCAN_MODE_ONE_SHOT) + up->can.ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT; + if (ctrlmodes & UCAN_MODE_BERR_REPORT) + up->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING; +} + +/* Handle a CAN error frame that we have received from the device */ +static void ucan_handle_error_frame(struct ucan_priv *up, + struct ucan_message_in *m, + u32 canid) +{ + enum can_state new_state = CAN_STATE_ERROR_ACTIVE; + struct net_device_stats *net_stats = &up->netdev->stats; + struct can_device_stats *can_stats = &up->can.can_stats; + + if (canid & CAN_ERR_LOSTARB) + can_stats->arbitration_lost++; + + if (canid & CAN_ERR_BUSERROR) + can_stats->bus_error++; + + if (canid & CAN_ERR_ACK) + net_stats->tx_errors++; + + if (canid & CAN_ERR_BUSOFF) + new_state = CAN_STATE_BUS_OFF; + + /* controller problems, details in data[1] */ + if (canid & CAN_ERR_CRTL) { + u8 d1 = m->msg.can_msg.data[1]; + + if (d1 & (CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE)) + new_state = max(new_state, (enum can_state) + CAN_STATE_ERROR_PASSIVE); + + if (d1 & (CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING)) + new_state = max(new_state, (enum can_state) + CAN_STATE_ERROR_WARNING); + + if (d1 & CAN_ERR_CRTL_RX_OVERFLOW) + net_stats->rx_over_errors++; + } + + /* protocol error, details in data[2] */ + if (canid & CAN_ERR_PROT) { + u8 d2 = m->msg.can_msg.data[2]; + + if (d2 & CAN_ERR_PROT_TX) + net_stats->tx_errors++; + else + net_stats->rx_errors++; + } + + /* we switched into a better state */ + if (up->can.state >= new_state) { + up->can.state = new_state; + return; + } + + /* we switched into a worse state */ + up->can.state = new_state; + switch (new_state) { + case CAN_STATE_BUS_OFF: + can_stats->bus_off++; + can_bus_off(up->netdev); + netdev_info(up->netdev, + "link has gone into BUS-OFF state\n"); + break; + case CAN_STATE_ERROR_PASSIVE: + can_stats->error_passive++; + break; + case CAN_STATE_ERROR_WARNING: + can_stats->error_warning++; + break; + default: + break; + } +} + +/* Callback on reception of a can frame via the IN endpoint + * + * This function allocates an skb and transferres it to the Linux + * network stack + */ +static void ucan_rx_can_msg(struct ucan_priv *up, struct ucan_message_in *m) +{ + int len; + u32 canid; + struct can_frame *cf; + struct sk_buff *skb; + struct net_device_stats *stats = &up->netdev->stats; + + /* get the contents of the length field */ + len = le16_to_cpu(m->len); + + /* check sanity */ + if (len < UCAN_IN_HDR_SIZE + sizeof(m->msg.can_msg.id)) { + dev_warn(&up->udev->dev, "invalid input message len\n"); + return; + } + + /* handle error frames */ + canid = le32_to_cpu(m->msg.can_msg.id); + if (canid & CAN_ERR_FLAG) { + ucan_handle_error_frame(up, m, canid); + /* drop frame if berr-reporting is off */ + if (!(up->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) + return; + } + + /* allocate skb */ + skb = alloc_can_skb(up->netdev, &cf); + if (!skb) + return; + + /* fill the can frame */ + cf->can_id = le32_to_cpu(m->msg.can_msg.id); + + /* compute DLC taking RTR_FLAG into account */ + cf->can_dlc = ucan_compute_dlc(len, &m->msg.can_msg); + + if (cf->can_dlc > CAN_MAX_DLEN) { + dev_warn(&up->udev->dev, + "dropping CAN frame due to DLC field\n"); + goto err_freeskb; + } + + if (cf->can_id & CAN_EFF_FLAG) + cf->can_id &= + (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG | CAN_ERR_FLAG); + else + cf->can_id &= (CAN_SFF_MASK | CAN_RTR_FLAG | CAN_ERR_FLAG); + + if ((cf->can_id & CAN_RTR_FLAG) != CAN_RTR_FLAG) + memcpy(cf->data, m->msg.can_msg.data, cf->can_dlc); + + /* don't count error frames as real packets */ + if (!(canid & CAN_ERR_FLAG)) { + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; + } + + /* pass it to Linux */ + netif_rx(skb); + + return; +err_freeskb: + kfree_skb(skb); +} + +/* callback indicating completed transmission */ +static void ucan_tx_complete_msg(struct ucan_priv *up, + struct ucan_message_in *m) +{ + u16 count, i; + u8 echo_id; + u16 len = le16_to_cpu(m->len); + + struct ucan_urb_context *context; + + if (len < UCAN_IN_HDR_SIZE || (len % 2 != 0)) { + dev_dbg(&up->udev->dev, "%s Invalid tx complete length\n", + __func__); + } + + count = (len - UCAN_IN_HDR_SIZE) / 2; + for (i = 0; i < count; i++) { + /* we did not submit such echo ids */ + echo_id = m->msg.can_tx_complete_msg[i].echo_id; + if (echo_id >= up->device_info.tx_fifo) { + up->netdev->stats.tx_errors++; + dev_err(&up->udev->dev, + "device answered with invalid echo_id\n"); + continue; + } + + context = &up->tx_contexts[echo_id]; + if (atomic_read(&context->allocated) == 0) { + dev_err(&up->udev->dev, + "device answered with unallocated echo id %d\n", + echo_id); + continue; + } + + if (m->msg.can_tx_complete_msg[i].flags & + UCAN_TX_COMPLETE_SUCCESS) { + /* update statistics */ + up->netdev->stats.tx_packets++; + up->netdev->stats.tx_bytes += context->dlc; + can_get_echo_skb(up->netdev, context->echo_index); + } else { + up->netdev->stats.tx_dropped++; + can_free_echo_skb(up->netdev, context->echo_index); + } + + /* Release context and restart queue if necessary */ + ucan_release_context(up, context); + } +} + +/* callback on reception of a USB message */ +static void ucan_read_bulk_callback(struct urb *urb) +{ + int ret; + int pos; + struct ucan_priv *up = urb->context; + struct net_device *netdev = up->netdev; + struct ucan_message_in *m; + + /* the device is not up and the driver should not receive any + * data on the bulk in pipe + */ + WARN_ON(!up->tx_contexts); + if (!up->tx_contexts) { + usb_free_coherent(up->udev, + up->in_ep->wMaxPacketSize, + urb->transfer_buffer, + urb->transfer_dma); + return; + } + + /* check URB status */ + switch (urb->status) { + case 0: + break; + case -ENOENT: + case -EPIPE: + case -EPROTO: + case -ESHUTDOWN: + case -ETIME: + /* urb is not resubmitted -> free dma data */ + usb_free_coherent(up->udev, + up->in_ep->wMaxPacketSize, + urb->transfer_buffer, + urb->transfer_dma); + dev_dbg(&up->udev->dev, "%s ENOENT|ESHUTDOWN|ETIME\n", + __func__); + return; + default: + goto resubmit; + } + + /* sanity check */ + if (!netif_device_present(netdev)) + return; + + /* iterate over input */ + pos = 0; + while (pos < urb->actual_length) { + int len; + + /* check sanity (length of header) */ + if ((urb->actual_length - pos) < UCAN_IN_HDR_SIZE) { + dev_warn(&up->udev->dev, + "invalid input message %d; too short (no header)\n", + urb->actual_length); + goto resubmit; + } + + /* setup the message address */ + m = (struct ucan_message_in *) + ((u8 *)urb->transfer_buffer + pos); + len = le16_to_cpu(m->len); + + /* check sanity (length of content) */ + if (urb->actual_length - pos < len) { + dev_warn(&up->udev->dev, "invalid input message al:%d pos:%d len:%d; too short (no data)\n", + urb->actual_length, pos, len); + print_hex_dump(KERN_WARNING, + "raw data: ", + DUMP_PREFIX_ADDRESS, + 16, + 1, + urb->transfer_buffer, + urb->actual_length, + true); + + goto resubmit; + } + + switch (le16_to_cpu(m->type)) { + case UCAN_IN_RX: + ucan_rx_can_msg(up, m); + break; + case UCAN_IN_TX_COMPLETE: + ucan_tx_complete_msg(up, m); + break; + default: + dev_warn(&up->udev->dev, + "invalid input message type\n"); + break; + } + + /* proceed to next message */ + pos += len; + /* align to 4 byte boundary */ + pos = round_up(pos, 4); + } + +resubmit: + /* resubmit urb when done */ + usb_fill_bulk_urb(urb, up->udev, + usb_rcvbulkpipe(up->udev, + up->in_ep->bEndpointAddress & + USB_ENDPOINT_NUMBER_MASK), + urb->transfer_buffer, + up->in_ep->wMaxPacketSize, + ucan_read_bulk_callback, + up); + + usb_anchor_urb(urb, &up->rx_urbs); + ret = usb_submit_urb(urb, GFP_KERNEL); + + if (ret < 0) { + dev_err(&up->udev->dev, + "failed resubmitting read bulk urb: %d\n", ret); + + usb_unanchor_urb(urb); + usb_free_coherent(up->udev, up->in_ep->wMaxPacketSize, + urb->transfer_buffer, + urb->transfer_dma); + + if (ret == -ENODEV) + netif_device_detach(netdev); + } +} + +/* callback after transmission of a USB message */ +static void ucan_write_bulk_callback(struct urb *urb) +{ + struct ucan_urb_context *context = urb->context; + struct ucan_priv *up; + + /* get the urb context */ + WARN_ON_ONCE(!context); + if (!context) + return; + + /* free up our allocated buffer */ + usb_free_coherent(urb->dev, + sizeof(struct ucan_message_out), + urb->transfer_buffer, + urb->transfer_dma); + + up = context->up; + WARN_ON_ONCE(!up); + if (!up) + return; + + /* sanity check */ + if (!netif_device_present(up->netdev)) + return; + + /* transmission failed (USB - the device will not send a TX complete) */ + if (urb->status) { + dev_warn_once(&up->udev->dev, + "failed to transmit USB message to device: %d\n", + urb->status); + + /* update counters an cleanup */ + can_free_echo_skb(up->netdev, context->echo_index); + + up->netdev->stats.tx_dropped++; + + /* release context and restart the queue if necessary */ + ucan_release_context(up, context); + } +} + +static void ucan_cleanup_rx_urbs(struct ucan_priv *up, struct urb **urbs) +{ + int i; + + for (i = 0; i < UCAN_MAX_RX_URBS; i++) { + if (urbs[i]) { + usb_unanchor_urb(urbs[i]); + usb_free_coherent(up->udev, up->in_ep->wMaxPacketSize, + urbs[i]->transfer_buffer, + urbs[i]->transfer_dma); + usb_free_urb(urbs[i]); + } + } + + memset(urbs, 0, sizeof(*urbs) * UCAN_MAX_RX_URBS); +} + +static int ucan_prepare_and_anchor_rx_urbs(struct ucan_priv *up, + struct urb **urbs) +{ + int i; + + memset(urbs, 0, sizeof(*urbs) * UCAN_MAX_RX_URBS); + + for (i = 0; i < UCAN_MAX_RX_URBS; i++) { + void *buf; + + urbs[i] = usb_alloc_urb(0, GFP_KERNEL); + if (!urbs[i]) + goto err; + + buf = usb_alloc_coherent(up->udev, up->in_ep->wMaxPacketSize, + GFP_KERNEL, &urbs[i]->transfer_dma); + if (!buf) { + /* cleanup this urb */ + usb_free_urb(urbs[i]); + urbs[i] = NULL; + goto err; + } + + usb_fill_bulk_urb(urbs[i], up->udev, + usb_rcvbulkpipe(up->udev, + up->in_ep->bEndpointAddress & + USB_ENDPOINT_NUMBER_MASK), + buf, + up->in_ep->wMaxPacketSize, + ucan_read_bulk_callback, + up); + + urbs[i]->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; + + usb_anchor_urb(urbs[i], &up->rx_urbs); + } + return 0; + +err: + /* cleanup other unsubmitted urbs */ + ucan_cleanup_rx_urbs(up, urbs); + return -ENOMEM; +} + +/* Submits rx urbs with the semantic: Either submit all, or cleanup + * everything. I case of errors submitted urbs are killed and all urbs in + * the array are freed. I case of no errors every entry in the urb + * array is set to NULL. + */ +static int ucan_submit_rx_urbs(struct ucan_priv *up, struct urb **urbs) +{ + int i, ret; + + /* Iterate over all urbs to submit. On success remove the urb + * from the list. + */ + for (i = 0; i < UCAN_MAX_RX_URBS; i++) { + ret = usb_submit_urb(urbs[i], GFP_KERNEL); + if (ret) { + dev_err(&up->udev->dev, + "Could not submit urb; code: %d\n", ret); + goto err; + } + + /* Anchor URB and drop reference, USB core will take + * care of freeing it + */ + usb_free_urb(urbs[i]); + urbs[i] = NULL; + } + return 0; + +err: + /* Cleanup unsubmitted urbs */ + ucan_cleanup_rx_urbs(up, urbs); + + /* Kill urbs that are already submitted */ + usb_kill_anchored_urbs(&up->rx_urbs); + + return ret; +} + +/* Open the network device */ +static int ucan_open(struct net_device *netdev) +{ + int ret, ret_cleanup; + u16 ctrlmode; + struct urb *urbs[UCAN_MAX_RX_URBS]; + struct ucan_priv *up = netdev_priv(netdev); + + ret = ucan_allocate_contexts(up); + if (ret) + goto err; + + /* Allocate and prepare IN URBS - allocated and anchored + * urbs are stored in urbs[] for clean + */ + ret = ucan_prepare_and_anchor_rx_urbs(up, urbs); + if (ret) + goto err_contexts; + + /* Check the control mode */ + ctrlmode = 0; + if (up->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) + ctrlmode |= UCAN_MODE_LOOPBACK; + if (up->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) + ctrlmode |= UCAN_MODE_SILENT; + if (up->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) + ctrlmode |= UCAN_MODE_3_SAMPLES; + if (up->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) + ctrlmode |= UCAN_MODE_ONE_SHOT; + + /* Enable this in any case - filtering is down within the + * receive path + */ + ctrlmode |= UCAN_MODE_BERR_REPORT; + up->ctl_msg_buffer->cmd_start.mode = cpu_to_le16(ctrlmode); + + /* Driver is ready to receive data - start the USB device */ + ret = ucan_ctrl_command_out(up, UCAN_COMMAND_START, 0, 2); + if (ret < 0) { + dev_err(&up->udev->dev, + "Could not start UCAN device, code: %d\n", + ret); + goto err_reset; + } + + /* Call CAN layer open */ + ret = open_candev(netdev); + if (ret) + goto err_stop; + + /* Driver is ready to receive data. Submit RX URBS */ + ret = ucan_submit_rx_urbs(up, urbs); + if (ret) + goto err_stop; + + up->can.state = CAN_STATE_ERROR_ACTIVE; + + /* Start the network queue */ + netif_start_queue(netdev); + + return 0; + +err_stop: + /* The device have started already stop it */ + ret_cleanup = ucan_ctrl_command_out(up, UCAN_COMMAND_STOP, 0, 0); + if (ret_cleanup < 0) + dev_err(&up->udev->dev, + "Could not stop UCAN device, code: %d\n", + ret_cleanup); + +err_reset: + /* The device might have received data, reset it for + * consistent state + */ + ret_cleanup = ucan_ctrl_command_out(up, UCAN_COMMAND_RESET, 0, 0); + if (ret_cleanup < 0) + dev_err(&up->udev->dev, + "Could not reset UCAN device, code: %d\n", + ret_cleanup); + + /* clean up unsubmitted urbs */ + ucan_cleanup_rx_urbs(up, urbs); + +err_contexts: + ucan_release_contexts(up); + +err: + return ret; +} + +static struct urb *ucan_prepare_tx_urb(struct ucan_priv *up, + struct ucan_urb_context *context, + struct can_frame *cf) +{ + int mlen; + struct urb *urb; + struct ucan_message_out *m; + + /* create a URB, and a buffer for it, and copy the data to the URB */ + urb = usb_alloc_urb(0, GFP_ATOMIC); + if (!urb) { + netdev_err(up->netdev, "No memory left for URBs\n"); + return NULL; + } + + m = usb_alloc_coherent(up->udev, + sizeof(struct ucan_message_out), + GFP_ATOMIC, + &urb->transfer_dma); + if (!m) { + netdev_err(up->netdev, "No memory left for USB buffer\n"); + usb_free_urb(urb); + return NULL; + } + + /* build the USB message */ + m->type = UCAN_OUT_TX; + m->msg.can_msg.id = cpu_to_le32(cf->can_id); + + if (cf->can_id & CAN_RTR_FLAG) { + mlen = UCAN_OUT_HDR_SIZE + + offsetof(struct ucan_can_msg, dlc) + + sizeof(m->msg.can_msg.dlc); + m->msg.can_msg.dlc = cf->can_dlc; + } else { + mlen = UCAN_OUT_HDR_SIZE + + sizeof(m->msg.can_msg.id) + cf->can_dlc; + memcpy(m->msg.can_msg.data, cf->data, cf->can_dlc); + } + m->len = cpu_to_le16(mlen); + + context->dlc = cf->can_dlc; + + m->subtype = context->echo_index; + + /* build the urb */ + usb_fill_bulk_urb(urb, up->udev, + usb_sndbulkpipe(up->udev, + up->out_ep->bEndpointAddress & + USB_ENDPOINT_NUMBER_MASK), + m, mlen, ucan_write_bulk_callback, context); + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; + + return urb; +} + +static void ucan_clean_up_tx_urb(struct ucan_priv *up, struct urb *urb) +{ + usb_free_coherent(up->udev, sizeof(struct ucan_message_out), + urb->transfer_buffer, urb->transfer_dma); + usb_free_urb(urb); +} + +/* callback when Linux needs to send a can frame */ +static netdev_tx_t ucan_start_xmit(struct sk_buff *skb, + struct net_device *netdev) +{ + int ret; + struct urb *urb; + struct ucan_urb_context *context; + struct ucan_priv *up = netdev_priv(netdev); + struct can_frame *cf = (struct can_frame *)skb->data; + + /* check skb */ + if (can_dropped_invalid_skb(netdev, skb)) + return NETDEV_TX_OK; + + /* allocate a context and slow down tx path, if fifo state is low */ + context = ucan_allocate_context(up); + + WARN_ON_ONCE(!context); + if (!context) + return NETDEV_TX_BUSY; + + /* prepare urb for transmission */ + urb = ucan_prepare_tx_urb(up, context, cf); + if (!urb) + goto drop; + + /* put the skb on can loopback stack */ + can_put_echo_skb(skb, up->netdev, context->echo_index); + + /* transmit it */ + usb_anchor_urb(urb, &up->tx_urbs); + ret = usb_submit_urb(urb, GFP_ATOMIC); + + /* cleanup urb */ + if (ret) { + /* on error, clean up */ + usb_unanchor_urb(urb); + ucan_clean_up_tx_urb(up, urb); + ucan_release_context(up, context); + + /* remove the skb from the echo stack - this also + * frees the skb + */ + can_free_echo_skb(up->netdev, context->echo_index); + + if (ret == -ENODEV) { + netif_device_detach(up->netdev); + } else { + netdev_warn(up->netdev, "failed tx_urb %d\n", ret); + up->netdev->stats.tx_dropped++; + } + return NETDEV_TX_OK; + } + + netif_trans_update(netdev); + + /* release ref, as we do not need the urb anymore */ + usb_free_urb(urb); + + return NETDEV_TX_OK; + +drop: + ucan_release_context(up, context); + dev_kfree_skb(skb); + up->netdev->stats.tx_dropped++; + + return NETDEV_TX_OK; +} + +/* Device goes down + * + * Clean up used resources + */ +static int ucan_close(struct net_device *netdev) +{ + int ret; + struct ucan_priv *up = netdev_priv(netdev); + + up->can.state = CAN_STATE_STOPPED; + + /* stop sending data */ + usb_kill_anchored_urbs(&up->tx_urbs); + + /* stop receiving data */ + usb_kill_anchored_urbs(&up->rx_urbs); + + /* stop and reset can device */ + ret = ucan_ctrl_command_out(up, UCAN_COMMAND_STOP, 0, 0); + if (ret < 0) + dev_err(&up->udev->dev, + "Could not stop UCAN device, code: %d\n", ret); + + ret = ucan_ctrl_command_out(up, UCAN_COMMAND_RESET, 0, 0); + if (ret < 0) + dev_err(&up->udev->dev, + "Could not reset UCAN device, code: %d\n", ret); + + netif_stop_queue(netdev); + + ucan_release_contexts(up); + + close_candev(up->netdev); + return 0; +} + +/* CAN driver callbacks */ +static const struct net_device_ops ucan_netdev_ops = { + .ndo_open = ucan_open, + .ndo_stop = ucan_close, + .ndo_start_xmit = ucan_start_xmit, + .ndo_change_mtu = can_change_mtu, +}; + +/* Request to set bittiming + * + * This function generates an USB set bittiming message and transmits + * it to the device + */ +static int ucan_set_bittiming(struct net_device *netdev) +{ + int ret; + struct ucan_priv *up = netdev_priv(netdev); + struct ucan_ctl_cmd_set_bittiming *cmd_set_bittiming; + + cmd_set_bittiming = &up->ctl_msg_buffer->cmd_set_bittiming; + cmd_set_bittiming->tq = cpu_to_le32(up->can.bittiming.tq); + cmd_set_bittiming->brp = cpu_to_le16(up->can.bittiming.brp); + cmd_set_bittiming->sample_point = + cpu_to_le32(up->can.bittiming.sample_point); + cmd_set_bittiming->prop_seg = up->can.bittiming.prop_seg; + cmd_set_bittiming->phase_seg1 = up->can.bittiming.phase_seg1; + cmd_set_bittiming->phase_seg2 = up->can.bittiming.phase_seg2; + cmd_set_bittiming->sjw = up->can.bittiming.sjw; + + dev_dbg(&up->udev->dev, + "Setup bittiming\n" + " bitrate: %d\n" + " sample-point: %d\n" + " tq: %d\n" + " prop_seg: %d\n" + " phase_seg1 %d\n" + " phase_seg2 %d\n" + " sjw %d\n" + " brp %d\n", + up->can.bittiming.bitrate, up->can.bittiming.sample_point, + up->can.bittiming.tq, up->can.bittiming.prop_seg, + up->can.bittiming.phase_seg1, up->can.bittiming.phase_seg2, + up->can.bittiming.sjw, up->can.bittiming.brp); + + ret = ucan_ctrl_command_out(up, UCAN_COMMAND_SET_BITTIMING, + 0, + sizeof(*cmd_set_bittiming)); + return (ret < 0) ? ret : 0; +} + +/* Restart the device to get it out of BUS-OFF state. + * Called when the user runs "ip link set can1 type can restart". + */ +static int ucan_set_mode(struct net_device *netdev, enum can_mode mode) +{ + int ret; + struct ucan_priv *up = netdev_priv(netdev); + + switch (mode) { + case CAN_MODE_START: + dev_dbg(&up->udev->dev, "Restarting device"); + ret = ucan_ctrl_command_out(up, UCAN_COMMAND_RESTART, 0, 0); + + /* check if queue can be restarted */ + if (atomic_read(&up->available_tx_urbs) > 0) + netif_wake_queue(up->netdev); + + return ret; + default: + return -EOPNOTSUPP; + } +} + +/* Probe the device, reset it and gather general device information */ +static int ucan_probe(struct usb_interface *intf, + const struct usb_device_id *id) +{ + int ret; + int i; + u32 protocol_version; + struct usb_device *udev; + struct net_device *netdev; + struct usb_host_interface *iface_desc; + struct ucan_priv *up; + struct usb_endpoint_descriptor *ep; + struct usb_endpoint_descriptor *out_ep; + struct usb_endpoint_descriptor *in_ep; + union ucan_ctl_payload *ctl_msg_buffer; + char firmware_str[sizeof(union ucan_ctl_payload) + 1]; + + udev = interface_to_usbdev(intf); + + /************************************** + * Stage 1 - Interface Parsing + ************************************** + * + * Identifie the device USB interface descriptor and its + * endpoints. Probing is aborted on errors. + */ + + /* check if the interface is sane */ + ret = -ENODEV; + iface_desc = intf->cur_altsetting; + if (!iface_desc) + goto err; + + /* interface sanity check */ + if (iface_desc->desc.bNumEndpoints != 2) { + dev_err(&udev->dev, + "Incompatible (possibly old) interface. It must have 2 endpoints but has %d\n", + iface_desc->desc.bNumEndpoints); + goto err; + } + + dev_info(&udev->dev, "Found UCAN device on interface #%d\n", + iface_desc->desc.iInterface); + + /* check interface endpoints */ + in_ep = NULL; + out_ep = NULL; + for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) { + ep = &iface_desc->endpoint[i].desc; + + if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != 0) && + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == + USB_ENDPOINT_XFER_BULK)) { + /* In Endpoint */ + in_ep = ep; + } else if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == + 0) && + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) == + USB_ENDPOINT_XFER_BULK)) { + /* Out Endpoint */ + out_ep = ep; + } + } + + /* check if interface is sane */ + if (!in_ep || !out_ep) { + dev_err(&udev->dev, "invalid endpoint configuration\n"); + goto err; + } + if (in_ep->wMaxPacketSize < sizeof(struct ucan_message_in)) { + dev_err(&udev->dev, "invalid in_ep MaxPacketSize\n"); + goto err; + } + if (out_ep->wMaxPacketSize < sizeof(struct ucan_message_out)) { + dev_err(&udev->dev, "invalid out_ep MaxPacketSize\n"); + goto err; + } + + /************************************** + * Stage 2 - Device Identification + ************************************** + * + * The device interface seems to be a ucan device. Do further + * compatibility checks. On error probing is aborted, on + * success this stage leaves the ctl_msg_buffer with the + * reported contents of a GET_INFO command (supported + * bittimings, tx_fifo depth). This information is used in + * Stage 3 for the final driver initialisation. + */ + + /* Prepare Memory for control transferes */ + ctl_msg_buffer = + devm_kzalloc(&udev->dev, sizeof(union ucan_ctl_payload), + GFP_KERNEL); + if (!ctl_msg_buffer) + goto err; + + /* get protocol version + * + * note: ucan_ctrl_command_* wrappers connot be used yet + * because `up` is initialised in Stage 3 + */ + ret = usb_control_msg(udev, + usb_rcvctrlpipe(udev, 0), + UCAN_COMMAND_GET, + USB_DIR_IN | USB_TYPE_VENDOR | + USB_RECIP_INTERFACE, + cpu_to_le16(UCAN_COMMAND_GET_PROTOCOL_VERSION), + iface_desc->desc.bInterfaceNumber, + ctl_msg_buffer, + sizeof(union ucan_ctl_payload), + UCAN_USB_CTL_PIPE_TIMEOUT); + + /* older firmware version do not support this command - those + * are not supported by this drive + */ + if (ret != 4) { + dev_err(&udev->dev, + "Could not read protocol version, ret=%d. The firmware on this device is too old, please update!\n", + ret); + if (ret >= 0) + ret = -EINVAL; + goto err; + } + + /* this driver currently supports protocol version 3 only */ + protocol_version = + le32_to_cpu(ctl_msg_buffer->cmd_get_protocol_version.version); + if (protocol_version < UCAN_PROTOCOL_VERSION_MIN || + protocol_version > UCAN_PROTOCOL_VERSION_MAX) { + dev_err(&udev->dev, "Device protocol version %d is not supported", + protocol_version); + ret = -EINVAL; + goto err; + } + dev_info(&udev->dev, "Device protocol version %d ok", protocol_version); + + /* request the device information and store it in ctl_msg_buffer + * + * note: ucan_ctrl_command_* wrappers connot be used yet + * because `up` is initialised in Stage 3 + */ + ret = usb_control_msg(udev, + usb_rcvctrlpipe(udev, 0), + UCAN_COMMAND_GET, + USB_DIR_IN | USB_TYPE_VENDOR | + USB_RECIP_INTERFACE, + cpu_to_le16(UCAN_COMMAND_GET_INFO), + iface_desc->desc.bInterfaceNumber, + ctl_msg_buffer, + sizeof(ctl_msg_buffer->cmd_get_device_info), + UCAN_USB_CTL_PIPE_TIMEOUT); + + if (ret < 0) { + dev_err(&udev->dev, "Failed to retrieve device info\n"); + goto err; + } + if (ret < sizeof(ctl_msg_buffer->cmd_get_device_info)) { + dev_err(&udev->dev, "Device reported invalid device info\n"); + ret = -EINVAL; + goto err; + } + if (ctl_msg_buffer->cmd_get_device_info.tx_fifo == 0) { + dev_err(&udev->dev, "Device reported invalid tx-fifo size\n"); + ret = -EINVAL; + goto err; + } + + /************************************** + * Stage 3 - Driver Initialisation + ************************************** + * + * Register device to Linux, prepare private structures and + * reset the device. + */ + + /* allocate driver resources */ + netdev = alloc_candev(sizeof(struct ucan_priv), + ctl_msg_buffer->cmd_get_device_info.tx_fifo); + if (!netdev) { + dev_err(&udev->dev, "Cannot allocate candev\n"); + ret = -ENOMEM; + goto err; + } + + up = netdev_priv(netdev); + + /* initialze data */ + up->udev = udev; + up->intf = intf; + up->netdev = netdev; + up->intf_index = iface_desc->desc.bInterfaceNumber; + up->in_ep = in_ep; + up->out_ep = out_ep; + up->ctl_msg_buffer = ctl_msg_buffer; + up->tx_contexts = NULL; + atomic_set(&up->available_tx_urbs, 0); + + up->can.state = CAN_STATE_STOPPED; + up->can.bittiming_const = &up->device_info.bittiming_const; + up->can.do_set_bittiming = ucan_set_bittiming; + up->can.do_set_mode = &ucan_set_mode; + netdev->netdev_ops = &ucan_netdev_ops; + + usb_set_intfdata(intf, up); + SET_NETDEV_DEV(netdev, &intf->dev); + + /* parse device information + * the data retrieved in Stage 2 is still available in + * up->ctl_msg_buffer + */ + ucan_parse_device_info(up, &ctl_msg_buffer->cmd_get_device_info); + + /* just print some device information - if available */ + ret = ucan_device_request_in(up, UCAN_DEVICE_GET_FW_STRING, 0, + sizeof(union ucan_ctl_payload)); + if (ret > 0) { + /* copy string while ensuring zero terminiation */ + strncpy(firmware_str, up->ctl_msg_buffer->raw, + sizeof(union ucan_ctl_payload)); + firmware_str[sizeof(union ucan_ctl_payload)] = '\0'; + } else { + strcpy(firmware_str, "unknown firmware"); + } + + /* device is compatible, reset it */ + ret = ucan_ctrl_command_out(up, UCAN_COMMAND_RESET, 0, 0); + if (ret < 0) + goto err_free_candev; + + init_usb_anchor(&up->rx_urbs); + init_usb_anchor(&up->tx_urbs); + + up->can.state = CAN_STATE_STOPPED; + + /* register the device */ + ret = register_candev(netdev); + if (ret) + goto err_free_candev; + + /* initialisation complete, log device info */ + dev_info(&up->udev->dev, + "Device reports:\n" + " Clock frequency [Hz] : %d\n" + " TX FIFO length : %d\n" + " Time segment 1 [min-max] : %d-%d\n" + " Time segment 2 [min-max] : %d-%d\n" + " SWJ [max] : %d\n" + " Prescaler [min-max,step] : %d-%d,%d\n" + " Supported modes : %s%s%s%s%s = 0x%x\n" + " Firmware : %s", + up->can.clock.freq, up->device_info.tx_fifo, + up->can.bittiming_const->tseg1_min, + up->can.bittiming_const->tseg1_max, + up->can.bittiming_const->tseg2_min, + up->can.bittiming_const->tseg2_max, + up->can.bittiming_const->sjw_max, + up->can.bittiming_const->brp_min, + up->can.bittiming_const->brp_max, + up->can.bittiming_const->brp_inc, + (up->can.ctrlmode_supported & CAN_CTRLMODE_LOOPBACK) ? + "LOOPBACK" : "", + (up->can.ctrlmode_supported & CAN_CTRLMODE_LISTENONLY) ? + " | LISTENONLY" : "", + (up->can.ctrlmode_supported & CAN_CTRLMODE_3_SAMPLES) ? + " | 3_SAMPLES" : "", + (up->can.ctrlmode_supported & CAN_CTRLMODE_ONE_SHOT) ? + " | ONE_SHOT" : "", + (up->can.ctrlmode_supported & CAN_CTRLMODE_BERR_REPORTING) ? + " | BERR_REPORTING" : "", + up->can.ctrlmode_supported, + firmware_str); + + dev_info(&udev->dev, "Registered UCAN device at %s\n", + netdev->name); + + /* success */ + return 0; + +err_free_candev: + free_candev(netdev); +err: + return ret; +} + +/* disconnect the device */ +static void ucan_disconnect(struct usb_interface *intf) +{ + struct usb_device *udev; + struct ucan_priv *up = usb_get_intfdata(intf); + + udev = interface_to_usbdev(intf); + + usb_set_intfdata(intf, NULL); + + if (up) { + unregister_netdev(up->netdev); + free_candev(up->netdev); + } +} + +static struct usb_device_id ucan_table[] = { + /* Mule (soldered onto compute modules) */ + {USB_DEVICE_INTERFACE_NUMBER(0x2294, 0x425a, 0)}, + /* Seal (standalone USB stick) */ + {USB_DEVICE_INTERFACE_NUMBER(0x2294, 0x425b, 0)}, + {} /* Terminating entry */ +}; + +MODULE_DEVICE_TABLE(usb, ucan_table); +/* driver callbacks */ +static struct usb_driver ucan_driver = { + .name = "ucan", + .probe = ucan_probe, + .disconnect = ucan_disconnect, + .id_table = ucan_table, +}; + +module_usb_driver(ucan_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Martin Elshuber, Theobroma Systems Design und Consulting GmbH <martin.elshuber@theobroma-systems.com>"); +MODULE_DESCRIPTION("Driver for Theobroma Systems UCAN devices"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher @ 2018-03-13 17:44 ` Marc Kleine-Budde 2018-03-13 17:53 ` Jakob Unterwurzacher 2018-03-14 7:51 ` Marc Kleine-Budde ` (3 subsequent siblings) 4 siblings, 1 reply; 25+ messages in thread From: Marc Kleine-Budde @ 2018-03-13 17:44 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, linux-can, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2819 bytes --] On 03/13/2018 06:35 PM, Jakob Unterwurzacher wrote: [...] Please mark all multibyte values going over the USB as either le or be. > +struct ucan_ctl_cmd_start { > + u16 mode; /* oring any of UCAN_MODE_* */ > +} __packed; > + > +struct ucan_ctl_cmd_set_bittiming { > + u32 tq; /* Time quanta (TQ) in nanoseconds */ > + u16 brp; /* TQ Prescaler */ > + u16 sample_point; /* Samplepoint on tenth percent */ > + u8 prop_seg; /* Propagation segment in TQs */ > + u8 phase_seg1; /* Phase buffer segment 1 in TQs */ > + u8 phase_seg2; /* Phase buffer segment 2 in TQs */ > + u8 sjw; /* Synchronisation jump width in TQs */ > +} __packed; > + > +struct ucan_ctl_cmd_device_info { > + u32 freq; /* Clock Frequency for tq generation */ > + u8 tx_fifo; /* Size of the transmission fifo */ > + u8 sjw_max; /* can_bittiming fields... */ > + u8 tseg1_min; > + u8 tseg1_max; > + u8 tseg2_min; > + u8 tseg2_max; > + u16 brp_inc; > + u32 brp_min; > + u32 brp_max; /* ...can_bittiming fields */ > + u16 ctrlmodes; /* supported control modes */ > + u16 hwfilter; /* Number of HW filter banks */ > + u16 rxmboxes; /* Number of receive Mailboxes */ > +} __packed; > + > +struct ucan_ctl_cmd_get_protocol_version { > + u32 version; > +} __packed; > + > +union ucan_ctl_payload { > + /*************************************************** > + * Setup Bittiming > + * bmRequest == UCAN_COMMAND_START > + ***************************************************/ Please use standard 'net' comment style: /* this is a multiline * comment */ > + struct ucan_ctl_cmd_start cmd_start; > + /*************************************************** > + * Setup Bittiming > + * bmRequest == UCAN_COMMAND_SET_BITTIMING > + ***************************************************/ > + struct ucan_ctl_cmd_set_bittiming cmd_set_bittiming; > + /*************************************************** > + * Get Device Information > + * bmRequest == UCAN_COMMAND_GET; wValue = UCAN_COMMAND_GET_INFO > + ***************************************************/ > + struct ucan_ctl_cmd_device_info cmd_get_device_info; > + /*************************************************** > + * Get Protocol Version > + * bmRequest == UCAN_COMMAND_GET; > + * wValue = UCAN_COMMAND_GET_PROTOCOL_VERSION > + ***************************************************/ > + struct ucan_ctl_cmd_get_protocol_version cmd_get_protocol_version; > + > + u8 raw[128]; > +} __packed; Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:44 ` Marc Kleine-Budde @ 2018-03-13 17:53 ` Jakob Unterwurzacher 2018-03-13 17:56 ` Marc Kleine-Budde 0 siblings, 1 reply; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-13 17:53 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, linux-can, linux-kernel On 13.03.18 18:44, Marc Kleine-Budde wrote: > On 03/13/2018 06:35 PM, Jakob Unterwurzacher wrote: > > [...] > > Please mark all multibyte values going over the USB as either le or be. This is documented in can_ucan_protocol.rst, All multi-byte integers are encoded as Little Endian. but it's probably worth mentioning here as well. > > Please use standard 'net' comment style: > > /* this is a multiline > * comment > */ > >> + struct ucan_ctl_cmd_start cmd_start; >> + /*************************************************** >> + * Setup Bittiming >> + * bmRequest == UCAN_COMMAND_SET_BITTIMING >> + ***************************************************/ Understood. Thanks, Jakob ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:53 ` Jakob Unterwurzacher @ 2018-03-13 17:56 ` Marc Kleine-Budde 0 siblings, 0 replies; 25+ messages in thread From: Marc Kleine-Budde @ 2018-03-13 17:56 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, linux-can, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 787 bytes --] On 03/13/2018 06:53 PM, Jakob Unterwurzacher wrote: > On 13.03.18 18:44, Marc Kleine-Budde wrote: >> On 03/13/2018 06:35 PM, Jakob Unterwurzacher wrote: >> >> [...] >> >> Please mark all multibyte values going over the USB as either le or be. > > This is documented in can_ucan_protocol.rst, > > All multi-byte integers are encoded as Little Endian. > > but it's probably worth mentioning here as well. Yes, but in the code, use "__le16" and "__le32" instead of "u16" and "u32" Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher 2018-03-13 17:44 ` Marc Kleine-Budde @ 2018-03-14 7:51 ` Marc Kleine-Budde 2018-03-14 9:09 ` Jakob Unterwurzacher 2018-03-14 9:11 ` [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices Wolfgang Grandegger ` (2 subsequent siblings) 4 siblings, 1 reply; 25+ messages in thread From: Marc Kleine-Budde @ 2018-03-14 7:51 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, linux-can, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 35283 bytes --] On 03/13/2018 06:35 PM, Jakob Unterwurzacher wrote: > The UCAN driver supports the microcontroller-based USB/CAN > adapters from Theobroma Systems. There are two form-factors > that run essentially the same firmware: > > * Seal: standalone USB stick ( https://www.theobroma-systems.com/seal ) > > * Mule: integrated on the PCB of various System-on-Modules from > Theobroma Systems like the A31-µQ7 and the RK3399-Q7 > ( https://www.theobroma-systems.com/rk3399-q7 ) > > The USB wire protocol has been designed to be as generic and > hardware-indendent as possible in the hope of being useful for > implementation on other microcontrollers. > > Signed-off-by: Martin Elshuber <martin.elshuber@theobroma-systems.com> > Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > Documentation/networking/can_ucan_protocol.rst | 315 +++++ > Documentation/networking/index.rst | 1 + > drivers/net/can/usb/Kconfig | 10 + > drivers/net/can/usb/Makefile | 1 + > drivers/net/can/usb/ucan.c | 1587 ++++++++++++++++++++++++ > 5 files changed, 1914 insertions(+) > create mode 100644 Documentation/networking/can_ucan_protocol.rst > create mode 100644 drivers/net/can/usb/ucan.c > > diff --git a/Documentation/networking/can_ucan_protocol.rst b/Documentation/networking/can_ucan_protocol.rst > new file mode 100644 > index 000000000000..d859b36200b4 > --- /dev/null > +++ b/Documentation/networking/can_ucan_protocol.rst > @@ -0,0 +1,315 @@ > +================= > +The UCAN Protocol > +================= > + > +UCAN is the protocol used by the microcontroller-based USB-CAN > +adapter that is integrated on System-on-Modules from Theobroma Systems > +and that is also available as a standalone USB stick. > + > +The UCAN protocol has been designed to be hardware-independent. > +It is modeled closely after how Linux represents CAN devices > +internally. All multi-byte integers are encoded as Little Endian. > + > +All structures mentioned in this document are defined in > +``drivers/net/can/usb/ucan.c``. > + > +USB Endpoints > +============= > + > +UCAN devices use three USB endpoints: > + > +CONTROL endpoint > + The driver sends device management commands on this endpoint > + > +IN endpoint > + The device sends CAN data frames and CAN error frames > + > +OUT endpoint > + The driver sends CAN data frames on the out endpoint > + > + > +CONTROL Messages > +================ > + > +UCAN devices are configured using vendor requests on the control pipe. > + > +To support multiple CAN interfaces in a single USB device all > +configuration commands target the corresponding interface in the USB > +descriptor. > + > +The driver uses ``ucan_ctrl_command_in/out`` and > +``ucan_device_request_in`` to deliver commands to the device. > + > +Setup Packet > +------------ > + > +================= ===================================================== > +``bmRequestType`` Direction | Vendor | (Interface or Device) > +``bRequest`` Command Number > +``wValue`` Subcommand Number (16 Bit) or 0 if not used > +``wIndex`` USB Interface Index (0 for device commands) > +``wLength`` * Host to Device - Number of bytes to transmit > + * Device to Host - Maximum Number of bytes to > + receive. If the device send less. Commom ZLP > + semantics are used. > +================= ===================================================== > + > +Error Handling > +-------------- > + > +The device indicates failed control commands by stalling the > +pipe. > + > +Device Commands > +--------------- > + > +UCAN_DEVICE_GET_FW_STRING > +~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +*Dev2Host; optional* > + > +Request the device firmware string. > + > + > +Interface Commands > +------------------ > + > +UCAN_COMMAND_START > +~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Bring the CAN interface up. > + > +Payload Format > + ``ucan_ctl_payload_t.cmd_start`` > + > +==== ============================ > +mode or mask of ``UCAN_MODE_*`` > +==== ============================ > + > +UCAN_COMMAND_STOP > +~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Stop the CAN interface > + > +Payload Format > + *empty* > + > +UCAN_COMMAND_RESET > +~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Reset the CAN controller (including error counters) > + > +Payload Format > + *empty* > + > +UCAN_COMMAND_GET > +~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Get Information from the Device > + > +Subcommands > +^^^^^^^^^^^ > + > +UCAN_COMMAND_GET_INFO > + Request the device information structure ``ucan_ctl_payload_t.device_info``. > + > + See the ``device_info`` field for details, and > + ``uapi/linux/can/netlink.h`` for an explanation of the > + ``can_bittiming fields``. > + > + Payload Format > + ``ucan_ctl_payload_t.device_info`` > + > +UCAN_COMMAND_GET_PROTOCOL_VERSION > + > + Request the device protocol version > + ``ucan_ctl_payload_t.protocol_version``. The current protocol version is 3. > + > + Payload Format > + ``ucan_ctl_payload_t.protocol_version`` > + > +.. note:: Devices that do not implement this command use the old > + protocol version 1 > + > +UCAN_COMMAND_SET_BITTIMING > +~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Setup bittiming by sending the the structure > +``ucan_ctl_payload_t.cmd_set_bittiming`` (see ``struct bittiming`` for > +details) > + > +Payload Format > + ``ucan_ctl_payload_t.cmd_set_bittiming``. > + > +UCAN_SLEEP/WAKE > +~~~~~~~~~~~~~~~ > + > +*Host2Dev; optional* > + > +Configure sleep and wake modes. Not yet supported by the driver. > + > +UCAN_FILTER > +~~~~~~~~~~~ > + > +*Host2Dev; optional* > + > +Setup hardware CAN filters. Not yet supported by the driver. > + > +Allowed interface commands > +-------------------------- > + > +================== =================== ================== > +Legal Device State Command New Device State > +================== =================== ================== > +stopped SET_BITTIMING stopped > +stopped START started > +started STOP or RESET stopped > +stopped STOP or RESET stopped > +started RESTART started > +any GET *no change* > +================== =================== ================== > + > +IN Message Format > +================= > + > +A data packet on the USB IN endpoint contains one or more > +``ucan_message_in`` values. If multiple messages are batched in a USB > +data packet, the ``len`` field can be used to jump to the next > +``ucan_message_in`` value (take care to sanity-check the ``len`` value > +against the actual data size). > + > +.. _can_ucan_in_message_len: > + > +``len`` field > +------------- > + > +Each ``ucan_message_in`` must be aligned to a 4-byte boundary (relative > +to the start of the start of the data buffer). That means that there > +may be padding bytes between multiple ``ucan_message_in`` values: > + > +.. code:: > + > + +----------------------------+ < 0 > + | | > + | struct ucan_message_in | > + | | > + +----------------------------+ < len > + [padding] > + +----------------------------+ < round_up(len, 4) > + | | > + | struct ucan_message_in | > + | | > + +----------------------------+ > + [...] > + > +``type`` field > +-------------- > + > +The ``type`` field specifies the type of the message. > + > +UCAN_IN_RX > +~~~~~~~~~~ > + > +``subtype`` > + zero > + > +Data received from the CAN bus (ID + payload). > + > +UCAN_IN_TX_COMPLETE > +~~~~~~~~~~~~~~~~~~~ > + > +``subtype`` > + zero > + > +The CAN device has sent a message to the CAN bus. It answers with a > +set of echo-ids from previous UCAN_OUT_TX messages > + > +Flow Control > +------------ > + > +When receiving CAN messages there is no flow control on the USB > +buffer. The driver has to handle inbound message quickly enough to > +avoid drops. I case the device buffer overflow the condition is > +reported by sending corresponding error frames (see > +:ref:`can_ucan_error_handling`) > + > + > +OUT Message Format > +================== > + > +A data packet on the USB OUT endpoint contains one or more ``struct > +ucan_message_out`` values. If multiple messages are batched into one > +data packet, the device uses the ``len`` field to jump to the next > +ucan_message_out value. Each ucan_message_out must be aligned to 4 > +bytes (relative to the start of the data buffer). The mechanism is > +same as described in :ref:`can_ucan_in_message_len`. > + > +.. code:: > + > + +----------------------------+ < 0 > + | | > + | struct ucan_message_out | > + | | > + +----------------------------+ < len > + [padding] > + +----------------------------+ < round_up(len, 4) > + | | > + | struct ucan_message_out | > + | | > + +----------------------------+ > + [...] > + > +``type`` field > +-------------- > + > +In protocol version 3 only ``UCAN_OUT_TX`` is defined, others are used > +only by legacy devices (protocol version 1). > + > +UCAN_OUT_TX > +~~~~~~~~~~~ > +``subtype`` > + echo id to be replied within a CAN_IN_TX_COMPLETE message > + > +Transmit a CAN frame. (parameters: ``id``, ``data``) > + > +Flow Control > +------------ > + > +When the device outbound buffers are full it starts sending *NAKs* on > +the *OUT* pipe until more buffers are available. The driver stops the > +queue when a certain threshold of out packets are incomplete. > + > +.. _can_ucan_error_handling: > + > +CAN Error Handling > +================== > + > +If error reporting is turned on the device encodes errors into CAN > +error frames (see ``uapi/linux/can/error.h``) and sends it using the > +IN endpoint. The driver updates its error statistics and forwards > +it. > + > +Although UCAN devices can suppress error frames completely, in Linux > +the driver is always interested. Hence, the device is always started with > +the ``UCAN_MODE_BERR_REPORT`` set. Filtering those messages for the > +user space is done by the driver. > + > +Example Conversation > +==================== > + > +#) Device is connected to USB > +#) Host sends command ``UCAN_COMMAND_RESET``, subcmd 0 > +#) Host sends command ``UCAN_COMMAND_GET``, subcmd ``UCAN_COMMAND_GET_INFO`` > +#) Device sends ``UCAN_IN_DEVICE_INFO`` > +#) Host sends command ``UCAN_OUT_SET_BITTIMING`` > +#) Host sends command ``UCAN_COMMAND_START``, subcmd 0, mode ``UCAN_MODE_BERR_REPORT`` > diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst > index 90966c2692d8..18903968cebf 100644 > --- a/Documentation/networking/index.rst > +++ b/Documentation/networking/index.rst > @@ -8,6 +8,7 @@ Contents: > > batman-adv > can > + can_ucan_protocol > kapi > z8530book > msg_zerocopy > diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig > index c36f4bdcbf4f..490cdce1f1da 100644 > --- a/drivers/net/can/usb/Kconfig > +++ b/drivers/net/can/usb/Kconfig > @@ -89,4 +89,14 @@ config CAN_MCBA_USB > This driver supports the CAN BUS Analyzer interface > from Microchip (http://www.microchip.com/development-tools/). > > +config CAN_UCAN > + tristate "Theobroma Systems UCAN interface" > + ---help--- > + This driver supports the Theobroma Systems > + UCAN USB-CAN interface. > + > + UCAN is an microcontroller-based USB-CAN interface that > + is integrated on System-on-Modules made by Theobroma Systems > + (https://www.theobroma-systems.com/som-products). > + > endmenu > diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile > index 49ac7b99ba32..4176e8358232 100644 > --- a/drivers/net/can/usb/Makefile > +++ b/drivers/net/can/usb/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o > obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/ > obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o > obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o > +obj-$(CONFIG_CAN_UCAN) += ucan.o > diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c > new file mode 100644 > index 000000000000..61348e8c4747 > --- /dev/null > +++ b/drivers/net/can/usb/ucan.c > @@ -0,0 +1,1587 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* Driver for Theobroma Systems UCAN devices Protocol Version 3 > + * > + * Copyright (C) 2018 Theobroma Systems Design und Consulting GmbH > + * > + * > + * General Description: > + * > + * The USB Device uses three Endpoints: > + * > + * CONTROL Endpoint: Is used the setup the device (start, stop, > + * info, configure). > + * > + * IN Endpoint: The device sends CAN Frame Messages and Device > + * Information using the IN endpoint. > + * > + * OUT Endpoint: The driver sends configuration requests, and CAN > + * Frames on the out endpoint. > + * > + * Error Handling: > + * > + * If error reporting is turned on the device encodes error into CAN > + * error frames (see uapi/linux/can/error.h) and sends it using the > + * IN Endpoint. The driver updates statistics and forward it. > + */ > + > +#include <linux/can.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > +#include <linux/module.h> > +#include <linux/netdevice.h> > +#include <linux/signal.h> > +#include <linux/skbuff.h> > +#include <linux/slab.h> > +#include <linux/usb.h> > + > +#include <linux/can.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > + > +#define UCAN_MAX_RX_URBS 8 > +/* the CAN controller needs a while to enable/disable the bus */ > +#define UCAN_USB_CTL_PIPE_TIMEOUT 1000 > +/* this driver currently supports protocol version 3 only */ > +#define UCAN_PROTOCOL_VERSION_MIN 3 > +#define UCAN_PROTOCOL_VERSION_MAX 3 > + > +/* UCAN Message Definitions -------------------------------------------- > + * > + * ucan_message_out_t and ucan_message_in_t define the messages > + * transmitted on the OUT and IN endpoint. > + * > + * Multibyte fields are transmitted with little endianness > + * > + * INTR Endpoint: a single uint32_t storing the current space in the fifo > + * > + * OUT Endpoint: single message of type ucan_message_out_t is > + * transmitted on the out endpoint > + * > + * IN Endpoint: multiple messages ucan_message_in_t concateted in > + * the following way: > + * > + * m[n].len <=> the length if message n(including the header in bytes) > + * m[n] is is aligned to a 4 byte boundary, hence > + * offset(m[0]) := 0; > + * offset(m[n+1]) := offset(m[n]) + (m[n].len + 3) & 3 > + * > + * this implies that > + * offset(m[n]) % 4 <=> 0 > + */ > + > +/* Device Global Commands */ > +enum { > + UCAN_DEVICE_GET_FW_STRING = 0, > +}; > + > +/* UCAN Commands */ > +enum { > + /* start the can transceiver - val defines the operation mode */ > + UCAN_COMMAND_START = 0, Just one spaces in front of the '='. > + /* cancel pending transmissions and stop the can transceiver */ > + UCAN_COMMAND_STOP = 1, > + /* send can transceiver into low-power sleep mode */ > + UCAN_COMMAND_SLEEP = 2, > + /* wake up can transceiver from low-power sleep mode */ > + UCAN_COMMAND_WAKEUP = 3, > + /* reset the can transceiver */ > + UCAN_COMMAND_RESET = 4, > + /* get piece of info from the can transceiver - subcmd defines what > + * piece > + */ > + UCAN_COMMAND_GET = 5, > + /* clear or disable hardware filter - subcmd defines which of the two */ > + UCAN_COMMAND_FILTER = 6, > + /* Setup bittiming */ > + UCAN_COMMAND_SET_BITTIMING = 7, > + /* recover from bus-off state */ > + UCAN_COMMAND_RESTART = 8, > +}; > + > +/* UCAN_COMMAND_START and UCAN_COMMAND_GET_INFO operation modes (bitmap). > + * Undefined bits must be set to 0. > + */ > +enum { > + UCAN_MODE_LOOPBACK = (1 << 0), Use BIT() > + UCAN_MODE_SILENT = (1 << 1), > + UCAN_MODE_3_SAMPLES = (1 << 2), > + UCAN_MODE_ONE_SHOT = (1 << 3), > + UCAN_MODE_BERR_REPORT = (1 << 4), > +}; > + > +/* UCAN_COMMAND_GET subcommands */ > +enum { > + UCAN_COMMAND_GET_INFO = 0, > + UCAN_COMMAND_GET_PROTOCOL_VERSION = 1, > +}; > + > +/* UCAN_COMMAND_FILTER subcommands */ > +enum { > + UCAN_FILTER_CLEAR = 0, > + UCAN_FILTER_DISABLE = 1, > + UCAN_FILTER_ENABLE = 2, > +}; > + > +/* OUT endpoint message types */ > +enum { > + UCAN_OUT_TX = 2, /* transmit a CAN frame */ > +}; > + > +/* IN endpoint message types */ > +enum { > + UCAN_IN_TX_COMPLETE = 1, /* CAN frame transmission completed */ > + UCAN_IN_RX = 2, /* CAN frame received */ > +}; > + > +struct ucan_ctl_cmd_start { > + u16 mode; /* oring any of UCAN_MODE_* */ __le16 as discussed in the previous mail. > +} __packed; > + > +struct ucan_ctl_cmd_set_bittiming { > + u32 tq; /* Time quanta (TQ) in nanoseconds */ __le32 > + u16 brp; /* TQ Prescaler */ > + u16 sample_point; /* Samplepoint on tenth percent */ > + u8 prop_seg; /* Propagation segment in TQs */ > + u8 phase_seg1; /* Phase buffer segment 1 in TQs */ > + u8 phase_seg2; /* Phase buffer segment 2 in TQs */ > + u8 sjw; /* Synchronisation jump width in TQs */ > +} __packed; > + > +struct ucan_ctl_cmd_device_info { > + u32 freq; /* Clock Frequency for tq generation */ > + u8 tx_fifo; /* Size of the transmission fifo */ > + u8 sjw_max; /* can_bittiming fields... */ > + u8 tseg1_min; > + u8 tseg1_max; > + u8 tseg2_min; > + u8 tseg2_max; > + u16 brp_inc; > + u32 brp_min; > + u32 brp_max; /* ...can_bittiming fields */ > + u16 ctrlmodes; /* supported control modes */ > + u16 hwfilter; /* Number of HW filter banks */ > + u16 rxmboxes; /* Number of receive Mailboxes */ > +} __packed; > + > +struct ucan_ctl_cmd_get_protocol_version { > + u32 version; > +} __packed; > + > +union ucan_ctl_payload { > + /*************************************************** > + * Setup Bittiming > + * bmRequest == UCAN_COMMAND_START > + ***************************************************/ > + struct ucan_ctl_cmd_start cmd_start; > + /*************************************************** > + * Setup Bittiming > + * bmRequest == UCAN_COMMAND_SET_BITTIMING > + ***************************************************/ > + struct ucan_ctl_cmd_set_bittiming cmd_set_bittiming; > + /*************************************************** > + * Get Device Information > + * bmRequest == UCAN_COMMAND_GET; wValue = UCAN_COMMAND_GET_INFO > + ***************************************************/ > + struct ucan_ctl_cmd_device_info cmd_get_device_info; > + /*************************************************** > + * Get Protocol Version > + * bmRequest == UCAN_COMMAND_GET; > + * wValue = UCAN_COMMAND_GET_PROTOCOL_VERSION > + ***************************************************/ > + struct ucan_ctl_cmd_get_protocol_version cmd_get_protocol_version; > + > + u8 raw[128]; > +} __packed; > + > +enum { > + UCAN_TX_COMPLETE_SUCCESS = (1 << 0), > +}; > + > +/* Transmission Complete within ucan_message_in */ > +struct ucan_tx_complete_entry_t { > + u8 echo_id; > + u8 flags; > +} __packed __aligned(0x2); > + > +/* CAN Data message format within ucan_message_in/out */ > +struct ucan_can_msg { > + /* note DLC is computed by > + * msg.len - sizeof (msg.len) > + * - sizeof (msg.type) > + * - sizeof (msg.can_msg.id) > + */ > + u32 id; > + > + union { > + u8 data[CAN_MAX_DLEN]; /* Data of CAN frames */ > + u8 dlc; /* RTR dlc */ > + }; > +} __packed; > + > +/* OUT Endpoint, outbound messages */ > +struct ucan_message_out { > + u16 len; /* Length of the content include header */ > + u8 type; /* UCAN_OUT_TX and friends */ one space after 'u8' > + u8 subtype; /* command sub type */ > + union { > + /*************************************************** > + * Transmit CAN frame > + * (type == UCAN_TX) && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) > + * subtype stores the echo id > + ***************************************************/ > + struct ucan_can_msg can_msg; > + } msg; > +} __packed __aligned(0x4); > + > +/* IN Endpoint, inbound messages */ > +struct ucan_message_in { > + u16 len; /* Length of the content include header */ > + u8 type; /* UCAN_IN_RX and friends */ > + u8 subtype; /* command sub type */ > + > + union { > + /*************************************************** > + * CAN Frame received > + * (type == UCAN_IN_RX) > + * && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) > + ***************************************************/ > + struct ucan_can_msg can_msg; > + > + /*************************************************** > + * CAN transmission complete > + * (type == UCAN_IN_TX_COMPLETE) > + ***************************************************/ > + struct ucan_tx_complete_entry_t can_tx_complete_msg[0]; > + > + } __aligned(0x4) msg; > +} __packed; > + > +/* Macros to calculate message lengths */ > +#define UCAN_OUT_HDR_SIZE offsetof(struct ucan_message_out, msg) > + > +#define UCAN_IN_HDR_SIZE offsetof(struct ucan_message_in, msg) > +#define UCAN_IN_LEN(member) (UCAN_OUT_HDR_SIZE + sizeof(member)) > + > +struct ucan_priv; > + > +/* Context Information for transmission URBs */ > +struct ucan_urb_context { > + struct ucan_priv *up; > + u32 echo_index; > + u8 dlc; > + atomic_t allocated; > +}; > + > +/* Information reported by the USB device */ > +struct ucan_device_info { > + struct can_bittiming_const bittiming_const; > + u8 tx_fifo; > +}; > + > +/* Driver private data */ > +struct ucan_priv { > + struct can_priv can; /* must be the first member */ > + > + u8 intf_index; > + struct usb_device *udev; > + struct usb_interface *intf; > + struct net_device *netdev; > + > + struct usb_endpoint_descriptor *out_ep; > + struct usb_endpoint_descriptor *in_ep; > + > + struct usb_anchor rx_urbs; > + struct usb_anchor tx_urbs; > + > + union ucan_ctl_payload *ctl_msg_buffer; > + struct ucan_device_info device_info; > + > + atomic_t available_tx_urbs; > + struct ucan_urb_context *tx_contexts; > +}; > + > +static u8 ucan_compute_dlc(u16 len, struct ucan_can_msg *msg) We usually put pointers we're working on as first parameters. > +{ > + u16 res = 0; Why is the function a u8 while res a u16? > + > + if (msg->id & CAN_RTR_FLAG) > + res = msg->dlc; > + else > + res = len - (UCAN_IN_HDR_SIZE + sizeof(msg->id)); > + > + if (res > CAN_MAX_DLEN) > + return -1; > + > + return res; > +} > + > +static void ucan_release_contexts(struct ucan_priv *up) > +{ > + if (!up->tx_contexts) > + return; > + > + atomic_set(&up->available_tx_urbs, 0); > + > + kfree(up->tx_contexts); > + up->tx_contexts = NULL; > +} > + > +static int ucan_allocate_contexts(struct ucan_priv *up) > +{ > + int i; > + > + /* release contexts if any */ > + ucan_release_contexts(up); > + > + up->tx_contexts = kmalloc_array(up->device_info.tx_fifo, > + sizeof(*up->tx_contexts), > + GFP_KERNEL); > + if (!up->tx_contexts) { > + dev_err(&up->udev->dev, "Not enough memory to allocate tx contexts\n"); As fas as I know, the kernel will print an error message if kmalloc fails. > + return -ENOMEM; > + } > + > + memset(up->tx_contexts, 0, > + sizeof(*up->tx_contexts) * up->device_info.tx_fifo); use kcalloc(), then you don't need the memset() > + for (i = 0; i < up->device_info.tx_fifo; i++) { > + atomic_set(&up->tx_contexts[i].allocated, 0); > + up->tx_contexts[i].up = up; > + up->tx_contexts[i].echo_index = i; > + } > + > + atomic_set(&up->available_tx_urbs, up->device_info.tx_fifo); > + > + return 0; > +} > + > +static struct ucan_urb_context *ucan_allocate_context(struct ucan_priv *up) > +{ > + int i, allocated, avail; > + > + if (!up->tx_contexts) > + return NULL; > + > + for (i = 0; i < up->device_info.tx_fifo; i++) { > + allocated = atomic_cmpxchg(&up->tx_contexts[i].allocated, 0, 1); > + if (allocated == 0) { if (!allocated) > + avail = atomic_sub_return(1, &up->available_tx_urbs); > + if (avail == 0) if (!avail) > + netif_stop_queue(up->netdev); > + return &up->tx_contexts[i]; > + } > + } > + return NULL; > +} > + > +static void ucan_release_context(struct ucan_priv *up, > + struct ucan_urb_context *ctx) > +{ > + WARN_ON_ONCE(!up->tx_contexts); > + if (!up->tx_contexts) if ((WARN_ON_ONCE(!up->tx_contexts)) > + return; > + > + if (atomic_cmpxchg(&ctx->allocated, 1, 0) == 0) { > + dev_warn(&up->udev->dev, > + "context %p (#%ld) was not allocated\n", > + ctx, ctx - up->tx_contexts); This should also not happen, right? If so, make it WARN_ON_ONCE() > + } else { > + atomic_inc(&up->available_tx_urbs); > + netif_wake_queue(up->netdev); > + } > +} > + > +static int ucan_ctrl_command_out(struct ucan_priv *up, > + u8 cmd, > + u16 subcmd, > + size_t datalen) Why is datalen a size_t? In usb_control_msg() it's a u16. > +{ > + if (datalen > sizeof(union ucan_ctl_payload)) > + return -ENOMEM; This should or even cannot happen. You call this function directly. > + > + return usb_control_msg(up->udev, > + usb_sndctrlpipe(up->udev, 0), > + cmd, > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE, > + cpu_to_le16(subcmd), > + up->intf_index, > + up->ctl_msg_buffer, > + datalen, > + UCAN_USB_CTL_PIPE_TIMEOUT); > +} > + > +static int ucan_device_request_in(struct ucan_priv *up, > + u8 cmd, > + u16 subcmd, > + size_t datalen) > +{ > + if (datalen > sizeof(union ucan_ctl_payload)) > + return -ENOMEM; > + > + return usb_control_msg(up->udev, > + usb_rcvctrlpipe(up->udev, 0), > + cmd, > + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > + cpu_to_le16(subcmd), > + 0, > + up->ctl_msg_buffer, > + datalen, > + UCAN_USB_CTL_PIPE_TIMEOUT); > +} > + > +/* Parse the device information structure reported by the device and > + * setup private variables accordingly > + */ > +static void ucan_parse_device_info(struct ucan_priv *up, > + struct ucan_ctl_cmd_device_info > + *ctl_cmd_device_info) > +{ > + struct can_bittiming_const *bittiming = > + &up->device_info.bittiming_const; > + u16 ctrlmodes; > + > + /* store the data */ > + up->can.clock.freq = le32_to_cpu(ctl_cmd_device_info->freq); > + up->device_info.tx_fifo = ctl_cmd_device_info->tx_fifo; > + strcpy(bittiming->name, "ucan"); > + bittiming->tseg1_min = ctl_cmd_device_info->tseg1_min; > + bittiming->tseg1_max = ctl_cmd_device_info->tseg1_max; > + bittiming->tseg2_min = ctl_cmd_device_info->tseg2_min; > + bittiming->tseg2_max = ctl_cmd_device_info->tseg2_max; > + bittiming->sjw_max = ctl_cmd_device_info->sjw_max; > + bittiming->brp_min = le32_to_cpu(ctl_cmd_device_info->brp_min); > + bittiming->brp_max = le32_to_cpu(ctl_cmd_device_info->brp_max); > + bittiming->brp_inc = le16_to_cpu(ctl_cmd_device_info->brp_inc); > + > + ctrlmodes = le16_to_cpu(ctl_cmd_device_info->ctrlmodes); > + > + up->can.ctrlmode_supported = 0; > + > + if (ctrlmodes & UCAN_MODE_LOOPBACK) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_LOOPBACK; > + if (ctrlmodes & UCAN_MODE_SILENT) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY; > + if (ctrlmodes & UCAN_MODE_3_SAMPLES) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES; > + if (ctrlmodes & UCAN_MODE_ONE_SHOT) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT; > + if (ctrlmodes & UCAN_MODE_BERR_REPORT) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING; > +} > + > +/* Handle a CAN error frame that we have received from the device */ > +static void ucan_handle_error_frame(struct ucan_priv *up, > + struct ucan_message_in *m, > + u32 canid) canid_t? > +{ > + enum can_state new_state = CAN_STATE_ERROR_ACTIVE; > + struct net_device_stats *net_stats = &up->netdev->stats; > + struct can_device_stats *can_stats = &up->can.can_stats; > + > + if (canid & CAN_ERR_LOSTARB) > + can_stats->arbitration_lost++; > + > + if (canid & CAN_ERR_BUSERROR) > + can_stats->bus_error++; > + > + if (canid & CAN_ERR_ACK) > + net_stats->tx_errors++; > + > + if (canid & CAN_ERR_BUSOFF) > + new_state = CAN_STATE_BUS_OFF; > + > + /* controller problems, details in data[1] */ > + if (canid & CAN_ERR_CRTL) { > + u8 d1 = m->msg.can_msg.data[1]; > + > + if (d1 & (CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE)) > + new_state = max(new_state, (enum can_state) > + CAN_STATE_ERROR_PASSIVE); > + > + if (d1 & (CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING)) > + new_state = max(new_state, (enum can_state) > + CAN_STATE_ERROR_WARNING); > + > + if (d1 & CAN_ERR_CRTL_RX_OVERFLOW) > + net_stats->rx_over_errors++; > + } > + > + /* protocol error, details in data[2] */ > + if (canid & CAN_ERR_PROT) { > + u8 d2 = m->msg.can_msg.data[2]; > + > + if (d2 & CAN_ERR_PROT_TX) > + net_stats->tx_errors++; > + else > + net_stats->rx_errors++; > + } > + > + /* we switched into a better state */ > + if (up->can.state >= new_state) { > + up->can.state = new_state; > + return; > + } > + > + /* we switched into a worse state */ > + up->can.state = new_state; > + switch (new_state) { > + case CAN_STATE_BUS_OFF: > + can_stats->bus_off++; > + can_bus_off(up->netdev); > + netdev_info(up->netdev, > + "link has gone into BUS-OFF state\n"); > + break; > + case CAN_STATE_ERROR_PASSIVE: > + can_stats->error_passive++; > + break; > + case CAN_STATE_ERROR_WARNING: > + can_stats->error_warning++; > + break; > + default: > + break; > + } > +} > + > +/* Callback on reception of a can frame via the IN endpoint > + * > + * This function allocates an skb and transferres it to the Linux > + * network stack > + */ > +static void ucan_rx_can_msg(struct ucan_priv *up, struct ucan_message_in *m) > +{ > + int len; > + u32 canid; canid_t > + struct can_frame *cf; > + struct sk_buff *skb; > + struct net_device_stats *stats = &up->netdev->stats; > + > + /* get the contents of the length field */ > + len = le16_to_cpu(m->len); > + > + /* check sanity */ > + if (len < UCAN_IN_HDR_SIZE + sizeof(m->msg.can_msg.id)) { > + dev_warn(&up->udev->dev, "invalid input message len\n"); > + return; > + } > + > + /* handle error frames */ > + canid = le32_to_cpu(m->msg.can_msg.id); > + if (canid & CAN_ERR_FLAG) { > + ucan_handle_error_frame(up, m, canid); > + /* drop frame if berr-reporting is off */ > + if (!(up->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) > + return; > + } > + > + /* allocate skb */ > + skb = alloc_can_skb(up->netdev, &cf); > + if (!skb) > + return; > + > + /* fill the can frame */ > + cf->can_id = le32_to_cpu(m->msg.can_msg.id); = canid; > + > + /* compute DLC taking RTR_FLAG into account */ > + cf->can_dlc = ucan_compute_dlc(len, &m->msg.can_msg); > + > + if (cf->can_dlc > CAN_MAX_DLEN) { Just get_can_dlc(); > + dev_warn(&up->udev->dev, > + "dropping CAN frame due to DLC field\n"); > + goto err_freeskb; > + } > + > + if (cf->can_id & CAN_EFF_FLAG) > + cf->can_id &= > + (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG | CAN_ERR_FLAG); > + else > + cf->can_id &= (CAN_SFF_MASK | CAN_RTR_FLAG | CAN_ERR_FLAG); What about moving the common flags in front of the if()? > + > + if ((cf->can_id & CAN_RTR_FLAG) != CAN_RTR_FLAG) if (cf->can_id & CAN_RTR_FLAG) should be enough > + memcpy(cf->data, m->msg.can_msg.data, cf->can_dlc); > + > + /* don't count error frames as real packets */ > + if (!(canid & CAN_ERR_FLAG)) { > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + } Please count them, too. > + > + /* pass it to Linux */ > + netif_rx(skb); > + > + return; > +err_freeskb: > + kfree_skb(skb); > +} > + > +/* callback indicating completed transmission */ > +static void ucan_tx_complete_msg(struct ucan_priv *up, > + struct ucan_message_in *m) > +{ > + u16 count, i; > + u8 echo_id; > + u16 len = le16_to_cpu(m->len); > + > + struct ucan_urb_context *context; > + > + if (len < UCAN_IN_HDR_SIZE || (len % 2 != 0)) { > + dev_dbg(&up->udev->dev, "%s Invalid tx complete length\n", > + __func__); Can you handle packages with wrong length? > + } > + > + count = (len - UCAN_IN_HDR_SIZE) / 2; > + for (i = 0; i < count; i++) { > + /* we did not submit such echo ids */ > + echo_id = m->msg.can_tx_complete_msg[i].echo_id; > + if (echo_id >= up->device_info.tx_fifo) { > + up->netdev->stats.tx_errors++; > + dev_err(&up->udev->dev, > + "device answered with invalid echo_id\n"); > + continue; > + } > + > + context = &up->tx_contexts[echo_id]; > + if (atomic_read(&context->allocated) == 0) { > + dev_err(&up->udev->dev, > + "device answered with unallocated echo id %d\n", > + echo_id); > + continue; > + } > + > + if (m->msg.can_tx_complete_msg[i].flags & > + UCAN_TX_COMPLETE_SUCCESS) { > + /* update statistics */ > + up->netdev->stats.tx_packets++; > + up->netdev->stats.tx_bytes += context->dlc; > + can_get_echo_skb(up->netdev, context->echo_index); > + } else { > + up->netdev->stats.tx_dropped++; > + can_free_echo_skb(up->netdev, context->echo_index); > + } > + > + /* Release context and restart queue if necessary */ > + ucan_release_context(up, context); > + } > +} > + > +/* callback on reception of a USB message */ > +static void ucan_read_bulk_callback(struct urb *urb) > +{ > + int ret; > + int pos; > + struct ucan_priv *up = urb->context; > + struct net_device *netdev = up->netdev; > + struct ucan_message_in *m; > + > + /* the device is not up and the driver should not receive any > + * data on the bulk in pipe > + */ > + WARN_ON(!up->tx_contexts); > + if (!up->tx_contexts) { if (WARN_ON()) ... I'll review the rest later. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 7:51 ` Marc Kleine-Budde @ 2018-03-14 9:09 ` Jakob Unterwurzacher 2018-03-14 9:36 ` rx_packets/bytes stats for error frames (was: Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices) Marc Kleine-Budde 0 siblings, 1 reply; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-14 9:09 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, linux-can, linux-kernel On 14.03.18 08:51, Marc Kleine-Budde wrote: >> + memcpy(cf->data, m->msg.can_msg.data, cf->can_dlc); >> + >> + /* don't count error frames as real packets */ >> + if (!(canid & CAN_ERR_FLAG)) { >> + stats->rx_packets++; >> + stats->rx_bytes += cf->can_dlc; >> + } > Please count them, too. We do count them, as errors! This is what happens when you transmit a single CAN frame with nothing connected: "TX errors" shoots up but "RX packets" stays zero. > root@rk3399-q7:~# ip -details -statistics link show can0 [...] > RX: bytes packets errors dropped overrun mcast > 0 0 0 0 0 0 > TX: bytes packets errors dropped carrier collsns > 0 0 20425 0 0 0 ^ permalink raw reply [flat|nested] 25+ messages in thread
* rx_packets/bytes stats for error frames (was: Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices) 2018-03-14 9:09 ` Jakob Unterwurzacher @ 2018-03-14 9:36 ` Marc Kleine-Budde 2018-03-14 9:46 ` rx_packets/bytes stats for error frames Wolfgang Grandegger 0 siblings, 1 reply; 25+ messages in thread From: Marc Kleine-Budde @ 2018-03-14 9:36 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, linux-can, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1079 bytes --] On 03/14/2018 10:09 AM, Jakob Unterwurzacher wrote: > On 14.03.18 08:51, Marc Kleine-Budde wrote: >>> + memcpy(cf->data, m->msg.can_msg.data, cf->can_dlc); >>> + >>> + /* don't count error frames as real packets */ >>> + if (!(canid & CAN_ERR_FLAG)) { >>> + stats->rx_packets++; >>> + stats->rx_bytes += cf->can_dlc; >>> + } >> Please count them, too. > > We do count them, as errors! > > This is what happens when you transmit a single CAN frame with nothing > connected: "TX errors" shoots up but "RX packets" stays zero. This is handled not consistent in the existing CAN drivers. In flexcan all and c_can (all but rx overflow) are counted as rx_packets and rx_bytes. (I haven't looked at the other drivers.) I tend to count the error frames as ordinary frames. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: rx_packets/bytes stats for error frames 2018-03-14 9:36 ` rx_packets/bytes stats for error frames (was: Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices) Marc Kleine-Budde @ 2018-03-14 9:46 ` Wolfgang Grandegger 2018-03-14 9:57 ` Jakob Unterwurzacher 0 siblings, 1 reply; 25+ messages in thread From: Wolfgang Grandegger @ 2018-03-14 9:46 UTC (permalink / raw) To: Marc Kleine-Budde, Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, linux-can, linux-kernel Am 14.03.2018 um 10:36 schrieb Marc Kleine-Budde: > On 03/14/2018 10:09 AM, Jakob Unterwurzacher wrote: >> On 14.03.18 08:51, Marc Kleine-Budde wrote: >>>> + memcpy(cf->data, m->msg.can_msg.data, cf->can_dlc); >>>> + >>>> + /* don't count error frames as real packets */ >>>> + if (!(canid & CAN_ERR_FLAG)) { >>>> + stats->rx_packets++; >>>> + stats->rx_bytes += cf->can_dlc; >>>> + } >>> Please count them, too. >> >> We do count them, as errors! >> >> This is what happens when you transmit a single CAN frame with nothing >> connected: "TX errors" shoots up but "RX packets" stays zero. > > This is handled not consistent in the existing CAN drivers. In flexcan > all and c_can (all but rx overflow) are counted as rx_packets and > rx_bytes. (I haven't looked at the other drivers.) > > I tend to count the error frames as ordinary frames. +1, I think we should count the packets and bytes delivered to the network layer. Wolfgang. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: rx_packets/bytes stats for error frames 2018-03-14 9:46 ` rx_packets/bytes stats for error frames Wolfgang Grandegger @ 2018-03-14 9:57 ` Jakob Unterwurzacher 0 siblings, 0 replies; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-14 9:57 UTC (permalink / raw) To: Wolfgang Grandegger, Marc Kleine-Budde Cc: Martin Elshuber, Philipp Tomsich, linux-can, linux-kernel On 14.03.18 10:46, Wolfgang Grandegger wrote: >>> We do count them, as errors! >>> >>> This is what happens when you transmit a single CAN frame with nothing >>> connected: "TX errors" shoots up but "RX packets" stays zero. >> >> This is handled not consistent in the existing CAN drivers. In flexcan >> all and c_can (all but rx overflow) are counted as rx_packets and >> rx_bytes. (I haven't looked at the other drivers.) >> >> I tend to count the error frames as ordinary frames. > > +1, I think we should count the packets and bytes delivered to the > network layer. Documentation/ABI/testing/sysfs-class-net-statistics says: > What: /sys/class/<iface>/statistics/rx_packets > Date: April 2005 > KernelVersion: 2.6.12 > Contact: netdev@vger.kernel.org > Description: > Indicates the total number of good packets received by this > network device. I considered error frames not "good packets", but I can change that for v3, no problem. Best regards, Jakob ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher 2018-03-13 17:44 ` Marc Kleine-Budde 2018-03-14 7:51 ` Marc Kleine-Budde @ 2018-03-14 9:11 ` Wolfgang Grandegger 2018-03-14 9:14 ` Jakob Unterwurzacher 2018-03-16 7:01 ` kbuild test robot 2018-03-16 12:14 ` kbuild test robot 4 siblings, 1 reply; 25+ messages in thread From: Wolfgang Grandegger @ 2018-03-14 9:11 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel Hello, Am 13.03.2018 um 18:35 schrieb Jakob Unterwurzacher: > The UCAN driver supports the microcontroller-based USB/CAN > adapters from Theobroma Systems. There are two form-factors > that run essentially the same firmware: > > * Seal: standalone USB stick ( https://www.theobroma-systems.com/seal ) > > * Mule: integrated on the PCB of various System-on-Modules from > Theobroma Systems like the A31-µQ7 and the RK3399-Q7 > ( https://www.theobroma-systems.com/rk3399-q7 ) > > The USB wire protocol has been designed to be as generic and > hardware-indendent as possible in the hope of being useful for > implementation on other microcontrollers. > > Signed-off-by: Martin Elshuber <martin.elshuber@theobroma-systems.com> > Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > Documentation/networking/can_ucan_protocol.rst | 315 +++++ > Documentation/networking/index.rst | 1 + > drivers/net/can/usb/Kconfig | 10 + > drivers/net/can/usb/Makefile | 1 + > drivers/net/can/usb/ucan.c | 1587 ++++++++++++++++++++++++ > 5 files changed, 1914 insertions(+) > create mode 100644 Documentation/networking/can_ucan_protocol.rst > create mode 100644 drivers/net/can/usb/ucan.c > > diff --git a/Documentation/networking/can_ucan_protocol.rst b/Documentation/networking/can_ucan_protocol.rst > new file mode 100644 > index 000000000000..d859b36200b4 > --- /dev/null > +++ b/Documentation/networking/can_ucan_protocol.rst > @@ -0,0 +1,315 @@ > +================= > +The UCAN Protocol > +================= > + > +UCAN is the protocol used by the microcontroller-based USB-CAN > +adapter that is integrated on System-on-Modules from Theobroma Systems > +and that is also available as a standalone USB stick. > + > +The UCAN protocol has been designed to be hardware-independent. > +It is modeled closely after how Linux represents CAN devices > +internally. All multi-byte integers are encoded as Little Endian. > + > +All structures mentioned in this document are defined in > +``drivers/net/can/usb/ucan.c``. > + > +USB Endpoints > +============= > + > +UCAN devices use three USB endpoints: > + > +CONTROL endpoint > + The driver sends device management commands on this endpoint > + > +IN endpoint > + The device sends CAN data frames and CAN error frames > + > +OUT endpoint > + The driver sends CAN data frames on the out endpoint > + > + > +CONTROL Messages > +================ > + > +UCAN devices are configured using vendor requests on the control pipe. > + > +To support multiple CAN interfaces in a single USB device all > +configuration commands target the corresponding interface in the USB > +descriptor. > + > +The driver uses ``ucan_ctrl_command_in/out`` and > +``ucan_device_request_in`` to deliver commands to the device. > + > +Setup Packet > +------------ > + > +================= ===================================================== > +``bmRequestType`` Direction | Vendor | (Interface or Device) > +``bRequest`` Command Number > +``wValue`` Subcommand Number (16 Bit) or 0 if not used > +``wIndex`` USB Interface Index (0 for device commands) > +``wLength`` * Host to Device - Number of bytes to transmit > + * Device to Host - Maximum Number of bytes to > + receive. If the device send less. Commom ZLP > + semantics are used. > +================= ===================================================== > + > +Error Handling > +-------------- > + > +The device indicates failed control commands by stalling the > +pipe. > + > +Device Commands > +--------------- > + > +UCAN_DEVICE_GET_FW_STRING > +~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +*Dev2Host; optional* > + > +Request the device firmware string. > + > + > +Interface Commands > +------------------ > + > +UCAN_COMMAND_START > +~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Bring the CAN interface up. > + > +Payload Format > + ``ucan_ctl_payload_t.cmd_start`` > + > +==== ============================ > +mode or mask of ``UCAN_MODE_*`` > +==== ============================ > + > +UCAN_COMMAND_STOP > +~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Stop the CAN interface > + > +Payload Format > + *empty* > + > +UCAN_COMMAND_RESET > +~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Reset the CAN controller (including error counters) > + > +Payload Format > + *empty* > + > +UCAN_COMMAND_GET > +~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Get Information from the Device > + > +Subcommands > +^^^^^^^^^^^ > + > +UCAN_COMMAND_GET_INFO > + Request the device information structure ``ucan_ctl_payload_t.device_info``. > + > + See the ``device_info`` field for details, and > + ``uapi/linux/can/netlink.h`` for an explanation of the > + ``can_bittiming fields``. > + > + Payload Format > + ``ucan_ctl_payload_t.device_info`` > + > +UCAN_COMMAND_GET_PROTOCOL_VERSION > + > + Request the device protocol version > + ``ucan_ctl_payload_t.protocol_version``. The current protocol version is 3. > + > + Payload Format > + ``ucan_ctl_payload_t.protocol_version`` > + > +.. note:: Devices that do not implement this command use the old > + protocol version 1 > + > +UCAN_COMMAND_SET_BITTIMING > +~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +*Host2Dev; mandatory* > + > +Setup bittiming by sending the the structure > +``ucan_ctl_payload_t.cmd_set_bittiming`` (see ``struct bittiming`` for > +details) > + > +Payload Format > + ``ucan_ctl_payload_t.cmd_set_bittiming``. > + > +UCAN_SLEEP/WAKE > +~~~~~~~~~~~~~~~ > + > +*Host2Dev; optional* > + > +Configure sleep and wake modes. Not yet supported by the driver. > + > +UCAN_FILTER > +~~~~~~~~~~~ > + > +*Host2Dev; optional* > + > +Setup hardware CAN filters. Not yet supported by the driver. > + > +Allowed interface commands > +-------------------------- > + > +================== =================== ================== > +Legal Device State Command New Device State > +================== =================== ================== > +stopped SET_BITTIMING stopped > +stopped START started > +started STOP or RESET stopped > +stopped STOP or RESET stopped > +started RESTART started > +any GET *no change* > +================== =================== ================== > + > +IN Message Format > +================= > + > +A data packet on the USB IN endpoint contains one or more > +``ucan_message_in`` values. If multiple messages are batched in a USB > +data packet, the ``len`` field can be used to jump to the next > +``ucan_message_in`` value (take care to sanity-check the ``len`` value > +against the actual data size). > + > +.. _can_ucan_in_message_len: > + > +``len`` field > +------------- > + > +Each ``ucan_message_in`` must be aligned to a 4-byte boundary (relative > +to the start of the start of the data buffer). That means that there > +may be padding bytes between multiple ``ucan_message_in`` values: > + > +.. code:: > + > + +----------------------------+ < 0 > + | | > + | struct ucan_message_in | > + | | > + +----------------------------+ < len > + [padding] > + +----------------------------+ < round_up(len, 4) > + | | > + | struct ucan_message_in | > + | | > + +----------------------------+ > + [...] > + > +``type`` field > +-------------- > + > +The ``type`` field specifies the type of the message. > + > +UCAN_IN_RX > +~~~~~~~~~~ > + > +``subtype`` > + zero > + > +Data received from the CAN bus (ID + payload). > + > +UCAN_IN_TX_COMPLETE > +~~~~~~~~~~~~~~~~~~~ > + > +``subtype`` > + zero > + > +The CAN device has sent a message to the CAN bus. It answers with a > +set of echo-ids from previous UCAN_OUT_TX messages > + > +Flow Control > +------------ > + > +When receiving CAN messages there is no flow control on the USB > +buffer. The driver has to handle inbound message quickly enough to > +avoid drops. I case the device buffer overflow the condition is > +reported by sending corresponding error frames (see > +:ref:`can_ucan_error_handling`) > + > + > +OUT Message Format > +================== > + > +A data packet on the USB OUT endpoint contains one or more ``struct > +ucan_message_out`` values. If multiple messages are batched into one > +data packet, the device uses the ``len`` field to jump to the next > +ucan_message_out value. Each ucan_message_out must be aligned to 4 > +bytes (relative to the start of the data buffer). The mechanism is > +same as described in :ref:`can_ucan_in_message_len`. > + > +.. code:: > + > + +----------------------------+ < 0 > + | | > + | struct ucan_message_out | > + | | > + +----------------------------+ < len > + [padding] > + +----------------------------+ < round_up(len, 4) > + | | > + | struct ucan_message_out | > + | | > + +----------------------------+ > + [...] > + > +``type`` field > +-------------- > + > +In protocol version 3 only ``UCAN_OUT_TX`` is defined, others are used > +only by legacy devices (protocol version 1). > + > +UCAN_OUT_TX > +~~~~~~~~~~~ > +``subtype`` > + echo id to be replied within a CAN_IN_TX_COMPLETE message > + > +Transmit a CAN frame. (parameters: ``id``, ``data``) > + > +Flow Control > +------------ > + > +When the device outbound buffers are full it starts sending *NAKs* on > +the *OUT* pipe until more buffers are available. The driver stops the > +queue when a certain threshold of out packets are incomplete. > + > +.. _can_ucan_error_handling: > + > +CAN Error Handling > +================== > + > +If error reporting is turned on the device encodes errors into CAN > +error frames (see ``uapi/linux/can/error.h``) and sends it using the > +IN endpoint. The driver updates its error statistics and forwards > +it. > + > +Although UCAN devices can suppress error frames completely, in Linux > +the driver is always interested. Hence, the device is always started with > +the ``UCAN_MODE_BERR_REPORT`` set. Filtering those messages for the > +user space is done by the driver. > + > +Example Conversation > +==================== > + > +#) Device is connected to USB > +#) Host sends command ``UCAN_COMMAND_RESET``, subcmd 0 > +#) Host sends command ``UCAN_COMMAND_GET``, subcmd ``UCAN_COMMAND_GET_INFO`` > +#) Device sends ``UCAN_IN_DEVICE_INFO`` > +#) Host sends command ``UCAN_OUT_SET_BITTIMING`` > +#) Host sends command ``UCAN_COMMAND_START``, subcmd 0, mode ``UCAN_MODE_BERR_REPORT`` > diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst > index 90966c2692d8..18903968cebf 100644 > --- a/Documentation/networking/index.rst > +++ b/Documentation/networking/index.rst > @@ -8,6 +8,7 @@ Contents: > > batman-adv > can > + can_ucan_protocol > kapi > z8530book > msg_zerocopy > diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig > index c36f4bdcbf4f..490cdce1f1da 100644 > --- a/drivers/net/can/usb/Kconfig > +++ b/drivers/net/can/usb/Kconfig > @@ -89,4 +89,14 @@ config CAN_MCBA_USB > This driver supports the CAN BUS Analyzer interface > from Microchip (http://www.microchip.com/development-tools/). > > +config CAN_UCAN > + tristate "Theobroma Systems UCAN interface" > + ---help--- > + This driver supports the Theobroma Systems > + UCAN USB-CAN interface. > + > + UCAN is an microcontroller-based USB-CAN interface that > + is integrated on System-on-Modules made by Theobroma Systems > + (https://www.theobroma-systems.com/som-products). > + > endmenu > diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile > index 49ac7b99ba32..4176e8358232 100644 > --- a/drivers/net/can/usb/Makefile > +++ b/drivers/net/can/usb/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o > obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/ > obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o > obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o > +obj-$(CONFIG_CAN_UCAN) += ucan.o > diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c > new file mode 100644 > index 000000000000..61348e8c4747 > --- /dev/null > +++ b/drivers/net/can/usb/ucan.c > @@ -0,0 +1,1587 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* Driver for Theobroma Systems UCAN devices Protocol Version 3 > + * > + * Copyright (C) 2018 Theobroma Systems Design und Consulting GmbH > + * > + * > + * General Description: > + * > + * The USB Device uses three Endpoints: > + * > + * CONTROL Endpoint: Is used the setup the device (start, stop, > + * info, configure). > + * > + * IN Endpoint: The device sends CAN Frame Messages and Device > + * Information using the IN endpoint. > + * > + * OUT Endpoint: The driver sends configuration requests, and CAN > + * Frames on the out endpoint. > + * > + * Error Handling: > + * > + * If error reporting is turned on the device encodes error into CAN > + * error frames (see uapi/linux/can/error.h) and sends it using the > + * IN Endpoint. The driver updates statistics and forward it. > + */ > + > +#include <linux/can.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > +#include <linux/module.h> > +#include <linux/netdevice.h> > +#include <linux/signal.h> > +#include <linux/skbuff.h> > +#include <linux/slab.h> > +#include <linux/usb.h> > + > +#include <linux/can.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > + > +#define UCAN_MAX_RX_URBS 8 > +/* the CAN controller needs a while to enable/disable the bus */ > +#define UCAN_USB_CTL_PIPE_TIMEOUT 1000 > +/* this driver currently supports protocol version 3 only */ > +#define UCAN_PROTOCOL_VERSION_MIN 3 > +#define UCAN_PROTOCOL_VERSION_MAX 3 > + > +/* UCAN Message Definitions -------------------------------------------- > + * > + * ucan_message_out_t and ucan_message_in_t define the messages > + * transmitted on the OUT and IN endpoint. > + * > + * Multibyte fields are transmitted with little endianness > + * > + * INTR Endpoint: a single uint32_t storing the current space in the fifo > + * > + * OUT Endpoint: single message of type ucan_message_out_t is > + * transmitted on the out endpoint > + * > + * IN Endpoint: multiple messages ucan_message_in_t concateted in > + * the following way: > + * > + * m[n].len <=> the length if message n(including the header in bytes) > + * m[n] is is aligned to a 4 byte boundary, hence > + * offset(m[0]) := 0; > + * offset(m[n+1]) := offset(m[n]) + (m[n].len + 3) & 3 > + * > + * this implies that > + * offset(m[n]) % 4 <=> 0 > + */ > + > +/* Device Global Commands */ > +enum { > + UCAN_DEVICE_GET_FW_STRING = 0, > +}; > + > +/* UCAN Commands */ > +enum { > + /* start the can transceiver - val defines the operation mode */ > + UCAN_COMMAND_START = 0, > + /* cancel pending transmissions and stop the can transceiver */ > + UCAN_COMMAND_STOP = 1, > + /* send can transceiver into low-power sleep mode */ > + UCAN_COMMAND_SLEEP = 2, > + /* wake up can transceiver from low-power sleep mode */ > + UCAN_COMMAND_WAKEUP = 3, > + /* reset the can transceiver */ > + UCAN_COMMAND_RESET = 4, > + /* get piece of info from the can transceiver - subcmd defines what > + * piece > + */ > + UCAN_COMMAND_GET = 5, > + /* clear or disable hardware filter - subcmd defines which of the two */ > + UCAN_COMMAND_FILTER = 6, > + /* Setup bittiming */ > + UCAN_COMMAND_SET_BITTIMING = 7, > + /* recover from bus-off state */ > + UCAN_COMMAND_RESTART = 8, > +}; > + > +/* UCAN_COMMAND_START and UCAN_COMMAND_GET_INFO operation modes (bitmap). > + * Undefined bits must be set to 0. > + */ > +enum { > + UCAN_MODE_LOOPBACK = (1 << 0), > + UCAN_MODE_SILENT = (1 << 1), > + UCAN_MODE_3_SAMPLES = (1 << 2), > + UCAN_MODE_ONE_SHOT = (1 << 3), > + UCAN_MODE_BERR_REPORT = (1 << 4), > +}; > + > +/* UCAN_COMMAND_GET subcommands */ > +enum { > + UCAN_COMMAND_GET_INFO = 0, > + UCAN_COMMAND_GET_PROTOCOL_VERSION = 1, > +}; > + > +/* UCAN_COMMAND_FILTER subcommands */ > +enum { > + UCAN_FILTER_CLEAR = 0, > + UCAN_FILTER_DISABLE = 1, > + UCAN_FILTER_ENABLE = 2, > +}; > + > +/* OUT endpoint message types */ > +enum { > + UCAN_OUT_TX = 2, /* transmit a CAN frame */ > +}; > + > +/* IN endpoint message types */ > +enum { > + UCAN_IN_TX_COMPLETE = 1, /* CAN frame transmission completed */ > + UCAN_IN_RX = 2, /* CAN frame received */ > +}; > + > +struct ucan_ctl_cmd_start { > + u16 mode; /* oring any of UCAN_MODE_* */ > +} __packed; > + > +struct ucan_ctl_cmd_set_bittiming { > + u32 tq; /* Time quanta (TQ) in nanoseconds */ > + u16 brp; /* TQ Prescaler */ > + u16 sample_point; /* Samplepoint on tenth percent */ > + u8 prop_seg; /* Propagation segment in TQs */ > + u8 phase_seg1; /* Phase buffer segment 1 in TQs */ > + u8 phase_seg2; /* Phase buffer segment 2 in TQs */ > + u8 sjw; /* Synchronisation jump width in TQs */ > +} __packed; > + > +struct ucan_ctl_cmd_device_info { > + u32 freq; /* Clock Frequency for tq generation */ > + u8 tx_fifo; /* Size of the transmission fifo */ > + u8 sjw_max; /* can_bittiming fields... */ > + u8 tseg1_min; > + u8 tseg1_max; > + u8 tseg2_min; > + u8 tseg2_max; > + u16 brp_inc; > + u32 brp_min; > + u32 brp_max; /* ...can_bittiming fields */ > + u16 ctrlmodes; /* supported control modes */ > + u16 hwfilter; /* Number of HW filter banks */ > + u16 rxmboxes; /* Number of receive Mailboxes */ > +} __packed; > + > +struct ucan_ctl_cmd_get_protocol_version { > + u32 version; > +} __packed; > + > +union ucan_ctl_payload { > + /*************************************************** > + * Setup Bittiming > + * bmRequest == UCAN_COMMAND_START > + ***************************************************/ > + struct ucan_ctl_cmd_start cmd_start; > + /*************************************************** > + * Setup Bittiming > + * bmRequest == UCAN_COMMAND_SET_BITTIMING > + ***************************************************/ > + struct ucan_ctl_cmd_set_bittiming cmd_set_bittiming; > + /*************************************************** > + * Get Device Information > + * bmRequest == UCAN_COMMAND_GET; wValue = UCAN_COMMAND_GET_INFO > + ***************************************************/ > + struct ucan_ctl_cmd_device_info cmd_get_device_info; > + /*************************************************** > + * Get Protocol Version > + * bmRequest == UCAN_COMMAND_GET; > + * wValue = UCAN_COMMAND_GET_PROTOCOL_VERSION > + ***************************************************/ > + struct ucan_ctl_cmd_get_protocol_version cmd_get_protocol_version; > + > + u8 raw[128]; > +} __packed; > + > +enum { > + UCAN_TX_COMPLETE_SUCCESS = (1 << 0), > +}; > + > +/* Transmission Complete within ucan_message_in */ > +struct ucan_tx_complete_entry_t { > + u8 echo_id; > + u8 flags; > +} __packed __aligned(0x2); > + > +/* CAN Data message format within ucan_message_in/out */ > +struct ucan_can_msg { > + /* note DLC is computed by > + * msg.len - sizeof (msg.len) > + * - sizeof (msg.type) > + * - sizeof (msg.can_msg.id) > + */ > + u32 id; > + > + union { > + u8 data[CAN_MAX_DLEN]; /* Data of CAN frames */ > + u8 dlc; /* RTR dlc */ > + }; > +} __packed; > + > +/* OUT Endpoint, outbound messages */ > +struct ucan_message_out { > + u16 len; /* Length of the content include header */ > + u8 type; /* UCAN_OUT_TX and friends */ > + u8 subtype; /* command sub type */ > + union { > + /*************************************************** > + * Transmit CAN frame > + * (type == UCAN_TX) && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) > + * subtype stores the echo id > + ***************************************************/ > + struct ucan_can_msg can_msg; > + } msg; > +} __packed __aligned(0x4); > + > +/* IN Endpoint, inbound messages */ > +struct ucan_message_in { > + u16 len; /* Length of the content include header */ > + u8 type; /* UCAN_IN_RX and friends */ > + u8 subtype; /* command sub type */ > + > + union { > + /*************************************************** > + * CAN Frame received > + * (type == UCAN_IN_RX) > + * && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) > + ***************************************************/ > + struct ucan_can_msg can_msg; > + > + /*************************************************** > + * CAN transmission complete > + * (type == UCAN_IN_TX_COMPLETE) > + ***************************************************/ > + struct ucan_tx_complete_entry_t can_tx_complete_msg[0]; > + > + } __aligned(0x4) msg; > +} __packed; > + > +/* Macros to calculate message lengths */ > +#define UCAN_OUT_HDR_SIZE offsetof(struct ucan_message_out, msg) > + > +#define UCAN_IN_HDR_SIZE offsetof(struct ucan_message_in, msg) > +#define UCAN_IN_LEN(member) (UCAN_OUT_HDR_SIZE + sizeof(member)) > + > +struct ucan_priv; > + > +/* Context Information for transmission URBs */ > +struct ucan_urb_context { > + struct ucan_priv *up; > + u32 echo_index; > + u8 dlc; > + atomic_t allocated; > +}; > + > +/* Information reported by the USB device */ > +struct ucan_device_info { > + struct can_bittiming_const bittiming_const; > + u8 tx_fifo; > +}; > + > +/* Driver private data */ > +struct ucan_priv { > + struct can_priv can; /* must be the first member */ > + > + u8 intf_index; > + struct usb_device *udev; > + struct usb_interface *intf; > + struct net_device *netdev; > + > + struct usb_endpoint_descriptor *out_ep; > + struct usb_endpoint_descriptor *in_ep; > + > + struct usb_anchor rx_urbs; > + struct usb_anchor tx_urbs; > + > + union ucan_ctl_payload *ctl_msg_buffer; > + struct ucan_device_info device_info; > + > + atomic_t available_tx_urbs; > + struct ucan_urb_context *tx_contexts; > +}; > + > +static u8 ucan_compute_dlc(u16 len, struct ucan_can_msg *msg) > +{ > + u16 res = 0; > + > + if (msg->id & CAN_RTR_FLAG) > + res = msg->dlc; > + else > + res = len - (UCAN_IN_HDR_SIZE + sizeof(msg->id)); > + > + if (res > CAN_MAX_DLEN) > + return -1; > + > + return res; > +} > + > +static void ucan_release_contexts(struct ucan_priv *up) > +{ > + if (!up->tx_contexts) > + return; > + > + atomic_set(&up->available_tx_urbs, 0); > + > + kfree(up->tx_contexts); > + up->tx_contexts = NULL; > +} > + > +static int ucan_allocate_contexts(struct ucan_priv *up) > +{ > + int i; > + > + /* release contexts if any */ > + ucan_release_contexts(up); > + > + up->tx_contexts = kmalloc_array(up->device_info.tx_fifo, > + sizeof(*up->tx_contexts), > + GFP_KERNEL); > + if (!up->tx_contexts) { > + dev_err(&up->udev->dev, "Not enough memory to allocate tx contexts\n"); > + return -ENOMEM; > + } > + > + memset(up->tx_contexts, 0, > + sizeof(*up->tx_contexts) * up->device_info.tx_fifo); > + for (i = 0; i < up->device_info.tx_fifo; i++) { > + atomic_set(&up->tx_contexts[i].allocated, 0); > + up->tx_contexts[i].up = up; > + up->tx_contexts[i].echo_index = i; > + } > + > + atomic_set(&up->available_tx_urbs, up->device_info.tx_fifo); > + > + return 0; > +} > + > +static struct ucan_urb_context *ucan_allocate_context(struct ucan_priv *up) > +{ > + int i, allocated, avail; > + > + if (!up->tx_contexts) > + return NULL; > + > + for (i = 0; i < up->device_info.tx_fifo; i++) { > + allocated = atomic_cmpxchg(&up->tx_contexts[i].allocated, 0, 1); > + if (allocated == 0) { > + avail = atomic_sub_return(1, &up->available_tx_urbs); > + if (avail == 0) > + netif_stop_queue(up->netdev); > + return &up->tx_contexts[i]; > + } > + } > + return NULL; > +} > + > +static void ucan_release_context(struct ucan_priv *up, > + struct ucan_urb_context *ctx) > +{ > + WARN_ON_ONCE(!up->tx_contexts); > + if (!up->tx_contexts) > + return; > + > + if (atomic_cmpxchg(&ctx->allocated, 1, 0) == 0) { > + dev_warn(&up->udev->dev, > + "context %p (#%ld) was not allocated\n", > + ctx, ctx - up->tx_contexts); > + } else { > + atomic_inc(&up->available_tx_urbs); > + netif_wake_queue(up->netdev); > + } > +} > + > +static int ucan_ctrl_command_out(struct ucan_priv *up, > + u8 cmd, > + u16 subcmd, > + size_t datalen) > +{ > + if (datalen > sizeof(union ucan_ctl_payload)) > + return -ENOMEM; > + > + return usb_control_msg(up->udev, > + usb_sndctrlpipe(up->udev, 0), > + cmd, > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE, > + cpu_to_le16(subcmd), > + up->intf_index, > + up->ctl_msg_buffer, > + datalen, > + UCAN_USB_CTL_PIPE_TIMEOUT); > +} > + > +static int ucan_device_request_in(struct ucan_priv *up, > + u8 cmd, > + u16 subcmd, > + size_t datalen) > +{ > + if (datalen > sizeof(union ucan_ctl_payload)) > + return -ENOMEM; > + > + return usb_control_msg(up->udev, > + usb_rcvctrlpipe(up->udev, 0), > + cmd, > + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > + cpu_to_le16(subcmd), > + 0, > + up->ctl_msg_buffer, > + datalen, > + UCAN_USB_CTL_PIPE_TIMEOUT); > +} > + > +/* Parse the device information structure reported by the device and > + * setup private variables accordingly > + */ > +static void ucan_parse_device_info(struct ucan_priv *up, > + struct ucan_ctl_cmd_device_info > + *ctl_cmd_device_info) > +{ > + struct can_bittiming_const *bittiming = > + &up->device_info.bittiming_const; > + u16 ctrlmodes; > + > + /* store the data */ > + up->can.clock.freq = le32_to_cpu(ctl_cmd_device_info->freq); > + up->device_info.tx_fifo = ctl_cmd_device_info->tx_fifo; > + strcpy(bittiming->name, "ucan"); > + bittiming->tseg1_min = ctl_cmd_device_info->tseg1_min; > + bittiming->tseg1_max = ctl_cmd_device_info->tseg1_max; > + bittiming->tseg2_min = ctl_cmd_device_info->tseg2_min; > + bittiming->tseg2_max = ctl_cmd_device_info->tseg2_max; > + bittiming->sjw_max = ctl_cmd_device_info->sjw_max; > + bittiming->brp_min = le32_to_cpu(ctl_cmd_device_info->brp_min); > + bittiming->brp_max = le32_to_cpu(ctl_cmd_device_info->brp_max); > + bittiming->brp_inc = le16_to_cpu(ctl_cmd_device_info->brp_inc); > + > + ctrlmodes = le16_to_cpu(ctl_cmd_device_info->ctrlmodes); > + > + up->can.ctrlmode_supported = 0; > + > + if (ctrlmodes & UCAN_MODE_LOOPBACK) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_LOOPBACK; > + if (ctrlmodes & UCAN_MODE_SILENT) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY; > + if (ctrlmodes & UCAN_MODE_3_SAMPLES) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES; > + if (ctrlmodes & UCAN_MODE_ONE_SHOT) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT; > + if (ctrlmodes & UCAN_MODE_BERR_REPORT) > + up->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING; > +} > + > +/* Handle a CAN error frame that we have received from the device */ > +static void ucan_handle_error_frame(struct ucan_priv *up, > + struct ucan_message_in *m, > + u32 canid) > +{ > + enum can_state new_state = CAN_STATE_ERROR_ACTIVE; > + struct net_device_stats *net_stats = &up->netdev->stats; > + struct can_device_stats *can_stats = &up->can.can_stats; > + > + if (canid & CAN_ERR_LOSTARB) > + can_stats->arbitration_lost++; > + > + if (canid & CAN_ERR_BUSERROR) > + can_stats->bus_error++; > + > + if (canid & CAN_ERR_ACK) > + net_stats->tx_errors++; > + > + if (canid & CAN_ERR_BUSOFF) > + new_state = CAN_STATE_BUS_OFF; > + > + /* controller problems, details in data[1] */ > + if (canid & CAN_ERR_CRTL) { > + u8 d1 = m->msg.can_msg.data[1]; > + > + if (d1 & (CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE)) > + new_state = max(new_state, (enum can_state) > + CAN_STATE_ERROR_PASSIVE); > + > + if (d1 & (CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING)) > + new_state = max(new_state, (enum can_state) > + CAN_STATE_ERROR_WARNING); > + > + if (d1 & CAN_ERR_CRTL_RX_OVERFLOW) > + net_stats->rx_over_errors++; > + } > + > + /* protocol error, details in data[2] */ > + if (canid & CAN_ERR_PROT) { > + u8 d2 = m->msg.can_msg.data[2]; > + > + if (d2 & CAN_ERR_PROT_TX) > + net_stats->tx_errors++; > + else > + net_stats->rx_errors++; > + } > + > + /* we switched into a better state */ > + if (up->can.state >= new_state) { > + up->can.state = new_state; > + return; > + } > + > + /* we switched into a worse state */ > + up->can.state = new_state; > + switch (new_state) { > + case CAN_STATE_BUS_OFF: > + can_stats->bus_off++; > + can_bus_off(up->netdev); > + netdev_info(up->netdev, > + "link has gone into BUS-OFF state\n"); > + break; > + case CAN_STATE_ERROR_PASSIVE: > + can_stats->error_passive++; > + break; > + case CAN_STATE_ERROR_WARNING: > + can_stats->error_warning++; > + break; > + default: > + break; > + } > +} > + > +/* Callback on reception of a can frame via the IN endpoint > + * > + * This function allocates an skb and transferres it to the Linux > + * network stack > + */ > +static void ucan_rx_can_msg(struct ucan_priv *up, struct ucan_message_in *m) > +{ > + int len; > + u32 canid; > + struct can_frame *cf; > + struct sk_buff *skb; > + struct net_device_stats *stats = &up->netdev->stats; > + > + /* get the contents of the length field */ > + len = le16_to_cpu(m->len); > + > + /* check sanity */ > + if (len < UCAN_IN_HDR_SIZE + sizeof(m->msg.can_msg.id)) { > + dev_warn(&up->udev->dev, "invalid input message len\n"); > + return; > + } > + > + /* handle error frames */ > + canid = le32_to_cpu(m->msg.can_msg.id); > + if (canid & CAN_ERR_FLAG) { > + ucan_handle_error_frame(up, m, canid); > + /* drop frame if berr-reporting is off */ > + if (!(up->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) > + return; > + } You still do not generate error messages for state changes, IIUC. Wolfgang. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 9:11 ` [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices Wolfgang Grandegger @ 2018-03-14 9:14 ` Jakob Unterwurzacher 2018-03-14 9:17 ` Wolfgang Grandegger 0 siblings, 1 reply; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-14 9:14 UTC (permalink / raw) To: Wolfgang Grandegger Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel On 14.03.18 10:11, Wolfgang Grandegger wrote: >> + /* handle error frames */ >> + canid = le32_to_cpu(m->msg.can_msg.id); >> + if (canid & CAN_ERR_FLAG) { >> + ucan_handle_error_frame(up, m, canid); >> + /* drop frame if berr-reporting is off */ >> + if (!(up->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) >> + return; >> + } > > You still do not generate error messages for state changes, IIUC. The hardware generates error frames on state changes - is that what you mean? In our testing, the state shows up (and updates) correctly in ip -details -statistics link show can0 Best regards, Jakob ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 9:14 ` Jakob Unterwurzacher @ 2018-03-14 9:17 ` Wolfgang Grandegger 2018-03-14 9:21 ` Jakob Unterwurzacher 2018-03-14 9:25 ` Wolfgang Grandegger 0 siblings, 2 replies; 25+ messages in thread From: Wolfgang Grandegger @ 2018-03-14 9:17 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel Am 14.03.2018 um 10:14 schrieb Jakob Unterwurzacher: > On 14.03.18 10:11, Wolfgang Grandegger wrote: >>> + /* handle error frames */ >>> + canid = le32_to_cpu(m->msg.can_msg.id); >>> + if (canid & CAN_ERR_FLAG) { >>> + ucan_handle_error_frame(up, m, canid); >>> + /* drop frame if berr-reporting is off */ >>> + if (!(up->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) >>> + return; >>> + } >> >> You still do not generate error messages for state changes, IIUC. > > The hardware generates error frames on state changes - is that what you > mean? > > In our testing, the state shows up (and updates) correctly in > > ip -details -statistics link show can0 Counting the state changes is one thing but you should also generate error messages for them. Wolfgang. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 9:17 ` Wolfgang Grandegger @ 2018-03-14 9:21 ` Jakob Unterwurzacher 2018-03-14 9:25 ` Wolfgang Grandegger 1 sibling, 0 replies; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-14 9:21 UTC (permalink / raw) To: Wolfgang Grandegger Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel On 14.03.18 10:17, Wolfgang Grandegger wrote: > Counting the state changes is one thing but you should also generate > error messages for them. We do for BUS-OFF already (see below), but not for other states - will do in v3. > + /* we switched into a worse state */ > + up->can.state = new_state; > + switch (new_state) { > + case CAN_STATE_BUS_OFF: > + can_stats->bus_off++; > + can_bus_off(up->netdev); > + netdev_info(up->netdev, > + "link has gone into BUS-OFF state\n"); > + break; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 9:17 ` Wolfgang Grandegger 2018-03-14 9:21 ` Jakob Unterwurzacher @ 2018-03-14 9:25 ` Wolfgang Grandegger 2018-03-14 9:48 ` Jakob Unterwurzacher 1 sibling, 1 reply; 25+ messages in thread From: Wolfgang Grandegger @ 2018-03-14 9:25 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel Am 14.03.2018 um 10:17 schrieb Wolfgang Grandegger: > > > Am 14.03.2018 um 10:14 schrieb Jakob Unterwurzacher: >> On 14.03.18 10:11, Wolfgang Grandegger wrote: >>>> + /* handle error frames */ >>>> + canid = le32_to_cpu(m->msg.can_msg.id); >>>> + if (canid & CAN_ERR_FLAG) { >>>> + ucan_handle_error_frame(up, m, canid); >>>> + /* drop frame if berr-reporting is off */ >>>> + if (!(up->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) >>>> + return; >>>> + } >>> >>> You still do not generate error messages for state changes, IIUC. >> >> The hardware generates error frames on state changes - is that what >> you mean? >> >> In our testing, the state shows up (and updates) correctly in >> >> ip -details -statistics link show can0 > > Counting the state changes is one thing but you should also generate > error messages for them. The usual test here is: $ candump -td -e any,0:0,#FFFFFFFF should report proper state changes, if you send messages with 1. no cable connected 2. CAN high and low short-circuited Also downwards if the hardware error is gone and you continue to send messages. Wolfgang. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 9:25 ` Wolfgang Grandegger @ 2018-03-14 9:48 ` Jakob Unterwurzacher 2018-03-14 10:04 ` Wolfgang Grandegger 0 siblings, 1 reply; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-14 9:48 UTC (permalink / raw) To: Wolfgang Grandegger Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel On 14.03.18 10:25, Wolfgang Grandegger wrote: >> Counting the state changes is one thing but you should also generate >> error messages for them. > > The usual test here is: > > $ candump -td -e any,0:0,#FFFFFFFF > > should report proper state changes, if you send messages with > > 1. no cable connected > 2. CAN high and low short-circuited > > Also downwards if the hardware error is gone and you continue to send > messages. Yes, we do, the hardware does it. Testcases: (1) no cable connected > root@rk3399-q7:~# candump -td -e any,0:0,#FFFFFFFF | head -n 1000 > (000.000000) can0 178 [0] > (000.000410) can0 20000030 [8] 00 00 00 00 00 00 08 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{8}{0}} > (000.000445) can0 20000030 [8] 00 00 00 00 00 00 10 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{16}{0}} > (000.000425) can0 20000030 [8] 00 00 00 00 00 00 18 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{24}{0}} > (000.000451) can0 20000030 [8] 00 00 00 00 00 00 20 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{32}{0}} > (000.000429) can0 20000030 [8] 00 00 00 00 00 00 28 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{40}{0}} > (000.000448) can0 20000030 [8] 00 00 00 00 00 00 30 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{48}{0}} > (000.000433) can0 20000030 [8] 00 00 00 00 00 00 38 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{56}{0}} > (000.000437) can0 20000030 [8] 00 00 00 00 00 00 40 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{64}{0}} > (000.000443) can0 20000030 [8] 00 00 00 00 00 00 48 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{72}{0}} > (000.000437) can0 20000030 [8] 00 00 00 00 00 00 50 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{80}{0}} > (000.000498) can0 20000030 [8] 00 00 00 00 00 00 58 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{88}{0}} > (000.000394) can0 20000034 [8] 00 0C 00 00 00 00 60 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{96}{0}} > (000.000433) can0 20000034 [8] 00 0C 00 00 00 00 68 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{104}{0}} > (000.000437) can0 20000034 [8] 00 0C 00 00 00 00 70 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{112}{0}} > (000.000443) can0 20000034 [8] 00 0C 00 00 00 00 78 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{120}{0}} > (000.000444) can0 20000034 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{128}{0}} > (000.000495) can0 20000024 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > no-acknowledgement-on-tx > error-counter-tx-rx{{128}{0}} Repeats ad infinitum... (1b) cable gets connected: > (000.000883) can0 20000034 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{128}{0}} > (000.000996) can0 20000004 [8] 00 0C 00 00 00 00 7F 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning} > error-counter-tx-rx{{127}{0}} (2) short circuit > root@rk3399-q7:~# candump -td -e any,0:0,#FFFFFFFF | head -n 1000 > (000.000000) can0 6AB [7] 33 39 62 29 00 F5 31 > (000.000201) can0 20000018 [8] 00 00 08 00 00 00 18 00 ERRORFRAME > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{24}{0}} > (000.000141) can0 2000000C [8] 00 3C 08 00 00 00 88 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > error-counter-tx-rx{{136}{0}} > (000.000202) can0 2000001C [8] 00 3C 08 00 00 00 90 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{144}{0}} > (000.000174) can0 2000001C [8] 00 3C 08 00 00 00 98 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{152}{0}} > (000.000229) can0 2000001C [8] 00 3C 08 00 00 00 A0 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{160}{0}} > (000.000191) can0 2000001C [8] 00 3C 08 00 00 00 A8 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{168}{0}} > (000.000227) can0 2000001C [8] 00 3C 08 00 00 00 B0 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{176}{0}} > (000.000204) can0 2000001C [8] 00 3C 08 00 00 00 B8 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{184}{0}} > (000.000200) can0 2000001C [8] 00 3C 08 00 00 00 C0 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{192}{0}} > (000.000216) can0 2000001C [8] 00 3C 08 00 00 00 C8 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{200}{0}} > (000.000214) can0 2000001C [8] 00 3C 08 00 00 00 D0 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{208}{0}} > (000.000188) can0 2000001C [8] 00 3C 08 00 00 00 D8 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{216}{0}} > (000.000206) can0 2000001C [8] 00 3C 08 00 00 00 E0 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{224}{0}} > (000.000202) can0 2000001C [8] 00 3C 08 00 00 00 E8 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{232}{0}} > (000.000218) can0 2000001C [8] 00 3C 08 00 00 00 F0 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{240}{0}} > (000.000203) can0 2000001C [8] 00 3C 08 00 00 00 F8 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > transceiver-status > error-counter-tx-rx{{248}{0}} > (000.007153) can0 2000004C [8] 00 3C 08 00 00 00 F8 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > protocol-violation{{tx-dominant-bit-error}{}} > bus-off > error-counter-tx-rx{{248}{0}} ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 9:48 ` Jakob Unterwurzacher @ 2018-03-14 10:04 ` Wolfgang Grandegger 2018-03-14 10:19 ` Jakob Unterwurzacher 2018-03-14 19:07 ` Jakob Unterwurzacher 0 siblings, 2 replies; 25+ messages in thread From: Wolfgang Grandegger @ 2018-03-14 10:04 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel Hello Jakob, Am 14.03.2018 um 10:48 schrieb Jakob Unterwurzacher: > On 14.03.18 10:25, Wolfgang Grandegger wrote: >>> Counting the state changes is one thing but you should also generate >>> error messages for them. >> >> The usual test here is: >> >> $ candump -td -e any,0:0,#FFFFFFFF >> >> should report proper state changes, if you send messages with >> >> 1. no cable connected >> 2. CAN high and low short-circuited >> >> Also downwards if the hardware error is gone and you continue to send >> messages. > Yes, we do, the hardware does it. Testcases: > > (1) no cable connected > >> root@rk3399-q7:~# candump -td -e any,0:0,#FFFFFFFF | head -n 1000 >> (000.000000) can0 178 [0] >> (000.000410) can0 20000030 [8] 00 00 00 00 00 00 08 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{8}{0}} >> (000.000445) can0 20000030 [8] 00 00 00 00 00 00 10 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{16}{0}} >> (000.000425) can0 20000030 [8] 00 00 00 00 00 00 18 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{24}{0}} >> (000.000451) can0 20000030 [8] 00 00 00 00 00 00 20 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{32}{0}} >> (000.000429) can0 20000030 [8] 00 00 00 00 00 00 28 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{40}{0}} >> (000.000448) can0 20000030 [8] 00 00 00 00 00 00 30 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{48}{0}} >> (000.000433) can0 20000030 [8] 00 00 00 00 00 00 38 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{56}{0}} >> (000.000437) can0 20000030 [8] 00 00 00 00 00 00 40 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{64}{0}} >> (000.000443) can0 20000030 [8] 00 00 00 00 00 00 48 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{72}{0}} >> (000.000437) can0 20000030 [8] 00 00 00 00 00 00 50 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{80}{0}} >> (000.000498) can0 20000030 [8] 00 00 00 00 00 00 58 00 ERRORFRAME >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{88}{0}} >> (000.000394) can0 20000034 [8] 00 0C 00 00 00 00 60 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning} >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{96}{0}} >> (000.000433) can0 20000034 [8] 00 0C 00 00 00 00 68 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning} >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{104}{0}} >> (000.000437) can0 20000034 [8] 00 0C 00 00 00 00 70 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning} >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{112}{0}} >> (000.000443) can0 20000034 [8] 00 0C 00 00 00 00 78 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning} >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{120}{0}} >> (000.000444) can0 20000034 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} Just, controller-problem{rx-error-passive,tx-error-passive} >> >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{128}{0}} >> (000.000495) can0 20000024 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} >> >> no-acknowledgement-on-tx >> error-counter-tx-rx{{128}{0}} > > Repeats ad infinitum... Good! And without "berr-reporting" just the state changes should show up. > > > (1b) cable gets connected: > >> (000.000883) can0 20000034 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} >> >> transceiver-status >> no-acknowledgement-on-tx >> error-counter-tx-rx{{128}{0}} >> (000.000996) can0 20000004 [8] 00 0C 00 00 00 00 7F 00 ERRORFRAME >> controller-problem{rx-error-warning,tx-error-warning} >> error-counter-tx-rx{{127}{0}} Back to error active is missing!? Have a look to: https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L364 Wolfgang. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 10:04 ` Wolfgang Grandegger @ 2018-03-14 10:19 ` Jakob Unterwurzacher 2018-03-14 19:07 ` Jakob Unterwurzacher 1 sibling, 0 replies; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-14 10:19 UTC (permalink / raw) To: Wolfgang Grandegger Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel On 14.03.18 11:04, Wolfgang Grandegger wrote: >>> controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > > Just, > > controller-problem{rx-error-passive,tx-error-passive} Ok. >> (1b) cable gets connected: >> >>> (000.000883) can0 20000034 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME >>> controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} >>> >>> transceiver-status >>> no-acknowledgement-on-tx >>> error-counter-tx-rx{{128}{0}} >>> (000.000996) can0 20000004 [8] 00 0C 00 00 00 00 7F 00 ERRORFRAME >>> controller-problem{rx-error-warning,tx-error-warning} >>> error-counter-tx-rx{{127}{0}} > > Back to error active is missing!? Have a look to: > > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L364 Should be sent out once the error counter drops further, will check. Thanks, Jakob ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 10:04 ` Wolfgang Grandegger 2018-03-14 10:19 ` Jakob Unterwurzacher @ 2018-03-14 19:07 ` Jakob Unterwurzacher 2018-03-15 6:58 ` Wolfgang Grandegger 1 sibling, 1 reply; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-14 19:07 UTC (permalink / raw) To: Wolfgang Grandegger Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel On 14.03.18 11:04, Wolfgang Grandegger wrote: >>> (000.000443) can0 20000034 [8] 00 0C 00 00 00 00 78 00 ERRORFRAME >>> controller-problem{rx-error-warning,tx-error-warning} >>> transceiver-status >>> no-acknowledgement-on-tx >>> error-counter-tx-rx{{120}{0}} >>> (000.000444) can0 20000034 [8] 00 3C 00 00 00 00 80 00 ERRORFRAME >>> controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} > > Just, > > controller-problem{rx-error-passive,tx-error-passive} >[...] > > Back to error active is missing!? That was indeed missing. We have fixed the missing back-to-error-active in our firmware. Also, we no longer send the controller status in every error frame, but only on state changes (see below) which seems to be how other drivers are handling things. Thanks, Jakob *** test output *** Disconnect cable, send one frame > root@rk3399-q7:~# candump -td -e any,0:0,#FFFFFFFF | head -n 100 > (000.000000) can0 6E7 [2] 7A F9 > (000.000558) can0 20000030 [8] 00 00 00 00 00 00 08 00 ERRORFRAME > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{8}{0}} [...] > (000.000567) can0 20000034 [8] 00 0C 00 00 00 00 60 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{96}{0}} [...] > (000.000527) can0 20000034 [8] 00 30 00 00 00 00 80 00 ERRORFRAME > controller-problem{rx-error-passive,tx-error-passive} > transceiver-status > no-acknowledgement-on-tx > error-counter-tx-rx{{128}{0}} [...] Reconnect cable > (000.000687) can1 6E7 [2] 7A F9 > (000.000015) can0 20000004 [8] 00 0C 00 00 00 00 7F 00 ERRORFRAME > controller-problem{rx-error-warning,tx-error-warning} > error-counter-tx-rx{{127}{0}} Send more frames > (046.485245) can0 61B [2] E2 D8 > (000.000621) can1 61B [2] E2 D8 [...] > (000.199224) can0 3E6 [0] > (000.000477) can1 3E6 [0] > (000.000044) can0 20000004 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME > controller-problem{back-to-error-active} > error-counter-tx-rx{{95}{0}} ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-14 19:07 ` Jakob Unterwurzacher @ 2018-03-15 6:58 ` Wolfgang Grandegger 0 siblings, 0 replies; 25+ messages in thread From: Wolfgang Grandegger @ 2018-03-15 6:58 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: Martin Elshuber, Philipp Tomsich, Marc Kleine-Budde, linux-can, linux-kernel Am 14.03.2018 um 20:07 schrieb Jakob Unterwurzacher: > On 14.03.18 11:04, Wolfgang Grandegger wrote: >>>> (000.000443) can0 20000034 [8] 00 0C 00 00 00 00 78 00 >>>> ERRORFRAME >>>> controller-problem{rx-error-warning,tx-error-warning} >>>> transceiver-status >>>> no-acknowledgement-on-tx >>>> error-counter-tx-rx{{120}{0}} >>>> (000.000444) can0 20000034 [8] 00 3C 00 00 00 00 80 00 >>>> ERRORFRAME >>>> >>>> controller-problem{rx-error-warning,tx-error-warning,rx-error-passive,tx-error-passive} >> >> >> Just, >> >> controller-problem{rx-error-passive,tx-error-passive} [...] >> >> Back to error active is missing!? > > That was indeed missing. We have fixed the missing back-to-error-active > in our firmware. > > Also, we no longer send the controller status in every error frame, but > only on state changes (see below) which seems to be how other drivers > are handling things. Good, it would also be useful, if berr-reporting could be enabled by the firmware. This avoids a lot of traffic and load, especially if no cable is connected. Wolfgang. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher ` (2 preceding siblings ...) 2018-03-14 9:11 ` [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices Wolfgang Grandegger @ 2018-03-16 7:01 ` kbuild test robot 2018-03-16 12:14 ` kbuild test robot 4 siblings, 0 replies; 25+ messages in thread From: kbuild test robot @ 2018-03-16 7:01 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: kbuild-all, jakob.unterwurzacher, Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1746 bytes --] Hi Jakob, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.16-rc5] [cannot apply to next-20180315] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jakob-Unterwurzacher/can-ucan-add-driver-for-Theobroma-Systems-UCAN-devices/20180316-111528 config: i386-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): drivers/net/can/usb/ucan.c: In function 'ucan_release_context': >> drivers/net/can/usb/ucan.c:386:21: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'int' [-Wformat=] "context %p (#%ld) was not allocated\n", ~~^ %d ctx, ctx - up->tx_contexts); ~~~~~~~~~~~~~~~~~~~~~ vim +386 drivers/net/can/usb/ucan.c 376 377 static void ucan_release_context(struct ucan_priv *up, 378 struct ucan_urb_context *ctx) 379 { 380 WARN_ON_ONCE(!up->tx_contexts); 381 if (!up->tx_contexts) 382 return; 383 384 if (atomic_cmpxchg(&ctx->allocated, 1, 0) == 0) { 385 dev_warn(&up->udev->dev, > 386 "context %p (#%ld) was not allocated\n", 387 ctx, ctx - up->tx_contexts); 388 } else { 389 atomic_inc(&up->available_tx_urbs); 390 netif_wake_queue(up->netdev); 391 } 392 } 393 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 63071 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices 2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher ` (3 preceding siblings ...) 2018-03-16 7:01 ` kbuild test robot @ 2018-03-16 12:14 ` kbuild test robot 4 siblings, 0 replies; 25+ messages in thread From: kbuild test robot @ 2018-03-16 12:14 UTC (permalink / raw) To: Jakob Unterwurzacher Cc: kbuild-all, jakob.unterwurzacher, Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-kernel Hi Jakob, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.16-rc5] [cannot apply to next-20180316] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jakob-Unterwurzacher/can-ucan-add-driver-for-Theobroma-Systems-UCAN-devices/20180316-111528 reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ sparse warnings: (new ones prefixed by >>) >> drivers/net/can/usb/ucan.c:406:25: sparse: incorrect type in argument 5 (different base types) @@ expected unsigned short [unsigned] [usertype] value @@ got short [unsigned] [usertype] value @@ drivers/net/can/usb/ucan.c:406:25: expected unsigned short [unsigned] [usertype] value drivers/net/can/usb/ucan.c:406:25: got restricted __le16 [usertype] <noident> drivers/net/can/usb/ucan.c:425:25: sparse: incorrect type in argument 5 (different base types) @@ expected unsigned short [unsigned] [usertype] value @@ got short [unsigned] [usertype] value @@ drivers/net/can/usb/ucan.c:425:25: expected unsigned short [unsigned] [usertype] value drivers/net/can/usb/ucan.c:425:25: got restricted __le16 [usertype] <noident> >> drivers/net/can/usb/ucan.c:444:30: sparse: cast to restricted __le32 drivers/net/can/usb/ucan.c:452:30: sparse: cast to restricted __le32 drivers/net/can/usb/ucan.c:453:30: sparse: cast to restricted __le32 >> drivers/net/can/usb/ucan.c:454:30: sparse: cast to restricted __le16 drivers/net/can/usb/ucan.c:456:21: sparse: cast to restricted __le16 drivers/net/can/usb/ucan.c:559:15: sparse: cast to restricted __le16 drivers/net/can/usb/ucan.c:568:17: sparse: cast to restricted __le32 drivers/net/can/usb/ucan.c:582:22: sparse: cast to restricted __le32 drivers/net/can/usb/ucan.c:622:19: sparse: cast to restricted __le16 >> drivers/net/can/usb/ucan.c:681:44: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned long [unsigned] [usertype] size @@ got d long [unsigned] [usertype] size @@ drivers/net/can/usb/ucan.c:681:44: expected unsigned long [unsigned] [usertype] size drivers/net/can/usb/ucan.c:681:44: got restricted __le16 [usertype] wMaxPacketSize drivers/net/can/usb/ucan.c:698:44: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned long [unsigned] [usertype] size @@ got d long [unsigned] [usertype] size @@ drivers/net/can/usb/ucan.c:698:44: expected unsigned long [unsigned] [usertype] size drivers/net/can/usb/ucan.c:698:44: got restricted __le16 [usertype] wMaxPacketSize drivers/net/can/usb/ucan.c:728:23: sparse: cast to restricted __le16 drivers/net/can/usb/ucan.c:746:25: sparse: cast to restricted __le16 >> drivers/net/can/usb/ucan.c:772:36: sparse: incorrect type in argument 5 (different base types) @@ expected int [signed] buffer_length @@ got restricted __le1int [signed] buffer_length @@ drivers/net/can/usb/ucan.c:772:36: expected int [signed] buffer_length drivers/net/can/usb/ucan.c:772:36: got restricted __le16 [usertype] wMaxPacketSize drivers/net/can/usb/ucan.c:784:54: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned long [unsigned] [usertype] size @@ got d long [unsigned] [usertype] size @@ drivers/net/can/usb/ucan.c:784:54: expected unsigned long [unsigned] [usertype] size drivers/net/can/usb/ucan.c:784:54: got restricted __le16 [usertype] wMaxPacketSize drivers/net/can/usb/ucan.c:842:62: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned long [unsigned] [usertype] size @@ got d long [unsigned] [usertype] size @@ drivers/net/can/usb/ucan.c:842:62: expected unsigned long [unsigned] [usertype] size drivers/net/can/usb/ucan.c:842:62: got restricted __le16 [usertype] wMaxPacketSize drivers/net/can/usb/ucan.c:866:61: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned long [unsigned] [usertype] size @@ got d long [unsigned] [usertype] size @@ drivers/net/can/usb/ucan.c:866:61: expected unsigned long [unsigned] [usertype] size drivers/net/can/usb/ucan.c:866:61: got restricted __le16 [usertype] wMaxPacketSize drivers/net/can/usb/ucan.c:880:44: sparse: incorrect type in argument 5 (different base types) @@ expected int [signed] buffer_length @@ got restricted __le1int [signed] buffer_length @@ drivers/net/can/usb/ucan.c:880:44: expected int [signed] buffer_length drivers/net/can/usb/ucan.c:880:44: got restricted __le16 [usertype] wMaxPacketSize >> drivers/net/can/usb/ucan.c:968:44: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [unsigned] [usertype] mode @@ got short [unsigned] [usertype] mode @@ drivers/net/can/usb/ucan.c:968:44: expected unsigned short [unsigned] [usertype] mode drivers/net/can/usb/ucan.c:968:44: got restricted __le16 [usertype] <noident> >> drivers/net/can/usb/ucan.c:1051:27: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [unsigned] [usertype] id @@ got ed int [unsigned] [usertype] id @@ drivers/net/can/usb/ucan.c:1051:27: expected unsigned int [unsigned] [usertype] id drivers/net/can/usb/ucan.c:1051:27: got restricted __le32 [usertype] <noident> >> drivers/net/can/usb/ucan.c:1063:16: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [unsigned] [usertype] len @@ got short [unsigned] [usertype] len @@ drivers/net/can/usb/ucan.c:1063:16: expected unsigned short [unsigned] [usertype] len drivers/net/can/usb/ucan.c:1063:16: got restricted __le16 [usertype] <noident> >> drivers/net/can/usb/ucan.c:1212:31: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [unsigned] [usertype] tq @@ got ed int [unsigned] [usertype] tq @@ drivers/net/can/usb/ucan.c:1212:31: expected unsigned int [unsigned] [usertype] tq drivers/net/can/usb/ucan.c:1212:31: got restricted __le32 [usertype] <noident> >> drivers/net/can/usb/ucan.c:1213:32: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [unsigned] [usertype] brp @@ got short [unsigned] [usertype] brp @@ drivers/net/can/usb/ucan.c:1213:32: expected unsigned short [unsigned] [usertype] brp drivers/net/can/usb/ucan.c:1213:32: got restricted __le16 [usertype] <noident> >> drivers/net/can/usb/ucan.c:1214:41: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [unsigned] [usertype] sample_point @@ got short [unsigned] [usertype] sample_point @@ drivers/net/can/usb/ucan.c:1214:41: expected unsigned short [unsigned] [usertype] sample_point drivers/net/can/usb/ucan.c:1214:41: got restricted __le32 [usertype] <noident> >> drivers/net/can/usb/ucan.c:1334:18: sparse: restricted __le16 degrades to integer drivers/net/can/usb/ucan.c:1338:19: sparse: restricted __le16 degrades to integer drivers/net/can/usb/ucan.c:1372:31: sparse: incorrect type in argument 5 (different base types) @@ expected unsigned short [unsigned] [usertype] value @@ got short [unsigned] [usertype] value @@ drivers/net/can/usb/ucan.c:1372:31: expected unsigned short [unsigned] [usertype] value drivers/net/can/usb/ucan.c:1372:31: got restricted __le16 [usertype] <noident> drivers/net/can/usb/ucan.c:1392:17: sparse: cast to restricted __le32 drivers/net/can/usb/ucan.c:1412:31: sparse: incorrect type in argument 5 (different base types) @@ expected unsigned short [unsigned] [usertype] value @@ got short [unsigned] [usertype] value @@ drivers/net/can/usb/ucan.c:1412:31: expected unsigned short [unsigned] [usertype] value drivers/net/can/usb/ucan.c:1412:31: got restricted __le16 [usertype] <noident> vim +406 drivers/net/can/usb/ucan.c 393 394 static int ucan_ctrl_command_out(struct ucan_priv *up, 395 u8 cmd, 396 u16 subcmd, 397 size_t datalen) 398 { 399 if (datalen > sizeof(union ucan_ctl_payload)) 400 return -ENOMEM; 401 402 return usb_control_msg(up->udev, 403 usb_sndctrlpipe(up->udev, 0), 404 cmd, 405 USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE, > 406 cpu_to_le16(subcmd), 407 up->intf_index, 408 up->ctl_msg_buffer, 409 datalen, 410 UCAN_USB_CTL_PIPE_TIMEOUT); 411 } 412 413 static int ucan_device_request_in(struct ucan_priv *up, 414 u8 cmd, 415 u16 subcmd, 416 size_t datalen) 417 { 418 if (datalen > sizeof(union ucan_ctl_payload)) 419 return -ENOMEM; 420 421 return usb_control_msg(up->udev, 422 usb_rcvctrlpipe(up->udev, 0), 423 cmd, 424 USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, 425 cpu_to_le16(subcmd), 426 0, 427 up->ctl_msg_buffer, 428 datalen, 429 UCAN_USB_CTL_PIPE_TIMEOUT); 430 } 431 432 /* Parse the device information structure reported by the device and 433 * setup private variables accordingly 434 */ 435 static void ucan_parse_device_info(struct ucan_priv *up, 436 struct ucan_ctl_cmd_device_info 437 *ctl_cmd_device_info) 438 { 439 struct can_bittiming_const *bittiming = 440 &up->device_info.bittiming_const; 441 u16 ctrlmodes; 442 443 /* store the data */ > 444 up->can.clock.freq = le32_to_cpu(ctl_cmd_device_info->freq); 445 up->device_info.tx_fifo = ctl_cmd_device_info->tx_fifo; 446 strcpy(bittiming->name, "ucan"); 447 bittiming->tseg1_min = ctl_cmd_device_info->tseg1_min; 448 bittiming->tseg1_max = ctl_cmd_device_info->tseg1_max; 449 bittiming->tseg2_min = ctl_cmd_device_info->tseg2_min; 450 bittiming->tseg2_max = ctl_cmd_device_info->tseg2_max; 451 bittiming->sjw_max = ctl_cmd_device_info->sjw_max; 452 bittiming->brp_min = le32_to_cpu(ctl_cmd_device_info->brp_min); 453 bittiming->brp_max = le32_to_cpu(ctl_cmd_device_info->brp_max); > 454 bittiming->brp_inc = le16_to_cpu(ctl_cmd_device_info->brp_inc); 455 456 ctrlmodes = le16_to_cpu(ctl_cmd_device_info->ctrlmodes); 457 458 up->can.ctrlmode_supported = 0; 459 460 if (ctrlmodes & UCAN_MODE_LOOPBACK) 461 up->can.ctrlmode_supported |= CAN_CTRLMODE_LOOPBACK; 462 if (ctrlmodes & UCAN_MODE_SILENT) 463 up->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY; 464 if (ctrlmodes & UCAN_MODE_3_SAMPLES) 465 up->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES; 466 if (ctrlmodes & UCAN_MODE_ONE_SHOT) 467 up->can.ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT; 468 if (ctrlmodes & UCAN_MODE_BERR_REPORT) 469 up->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING; 470 } 471 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 0/1] Open questions 2018-03-13 17:35 [PATCH v2 0/1] can: ucan: add driver for Theobroma Systems UCAN devices Jakob Unterwurzacher 2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher @ 2018-03-13 17:40 ` Jakob Unterwurzacher [not found] ` <06378497-1ACE-4333-810F-4E3E4706CCD5@theobroma-systems.com> 1 sibling, 1 reply; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-13 17:40 UTC (permalink / raw) Cc: Martin Elshuber, Philipp Tomsich, Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-kernel This email addresses open questions from the v1 review cycle. Three emails are answered, you can skip to sections by searching for the header text: * On 07.02.18 08:17, Wolfgang Grandegger wrote * On 09.02.18 11:40, Marc Kleine-Budde wrote * On 06.02.18 12:58, Andri Yngvason wrote Best regards, Jakob On 07.02.18 08:17, Wolfgang Grandegger wrote: > - please remove dev_dbg's just useful for driver development, > especially in the TX and RX path. Done > - I do not see CAN error states being handled. It's > mandatory! Done > - The TX done is handled when the transmission callback is done. > This has some unnice side effects. A real TX done message would > be much better. Done > Is "enpoint" a typo? Here an in a few other places! Done >> +/* Callback when the device sends the IRQ sate * > > Typo! Done; Interrupt Endpoint is gone in favor of TX_COMPLETE message (Used for Flow Control). >> + if (cf->can_dlc > sizeof(m->msg.can_msg.data)) >> + goto err_freeskb; > > Is'nt this worth an error message? > >> + >> + if (cf->can_dlc < 0) >> + goto err_freeskb; > > Ditto. sizeof: Added error message can_dlc < 0: if statement is unneccessary (can_dlc is u8, and comparision always evaluates fasle); statment deleted >> + if (cf->can_id & CAN_RTR_FLAG) { >> + cf->can_id |= CAN_RTR_FLAG; > > This flags is already set. Fixed >> + cf->can_id |= CAN_ERR_FLAG; > > Ditto. Fixed; Error Frame handling revised >> + ret = usb_submit_urb(up->irq_urb, GFP_KERNEL); >> + if (ret) { >> + kfree(up->irq_data); >> + usb_free_urb(up->irq_urb); > > Please use labels to cleanup in reverse order. > >> + goto err; >> + } Done; Interrupt Endpoint is gone in favor of TX_COMPLETE message (Used for Flow Control). >> + urb = usb_alloc_urb(0, GFP_KERNEL); >> + if (!urb) >> + goto err; > > What about: > > kfree(up->irq_data); > usb_free_urb(up->irq_urb); Revised open code; Interrupt Endpoint is gone ... you know already about this. >> + /* Infvalid interface Settings */ > > Typo. Fixed; probe code revised >> + if (up->in_ep->wMaxPacketSize < sizeof(struct ucan_message_in)) >> + goto err_free_candev; >> + if (up->out_ep->wMaxPacketSize < sizeof(struct ucan_message_out)) >> + goto err_free_candev; >> + if (up->irq_ep->wMaxPacketSize < sizeof(u32)) >> + goto err_free_candev; > > Could be or'ed together. I believe separating them support readability On 09.02.18 11:40, Marc Kleine-Budde wrote: > Does the controller have a serial number and expose it via the USB? Does > it show up as ID_SERIAL_SHORT by "udevadm info --path=...."? Yes! # udevadm info /sys/class/net/can0 | grep ID_SERIAL_SHORT E: ID_SERIAL_SHORT=004000405750511920353631 >> +++ b/Documentation/networking/can_ucan_protocol.txt > > Can you convert the Textfilen into .rst > (http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html)? Done; Updated file reflecting protocol changes >> +The UCAN protocol has been designed to be hardware-independent. >> +It is modeled closely after how Linux represents CAN devices > > modelled We believe it is modeled in en_US (modelled is used in en_GB) >> +All structures mentioned in this document are defined in >> +drivers/net/can/usb/ucan.h . > > Please fold the .h into the .c file. As no one besides the driver uses them. Done >> +IN enpoint: The device sends CAN frames and device > > endpoint Done >> +the device uses the "len" field to jump to the next >> +ucan_message_out value. Each ucan_message_out must be aliged > > aligned Done >> +* UCAN_OUT_COMMAND: Send a simple command to the device (parameters: cmd, >> + subcmd, val). See the comments in drivers/net/can/usb/ucan.h for details. > > merge .h -> .c Done >> + UCAN is an microcontroller-based USB-CAN interface that >> + is integrated on System-on-Modules made by Theobroma Systems >> + (https://www.theobroma-systems.com/som-products). > > Why not link to the controller's website directly? Did not exist at the time, now it does and we are linking to https://www.theobroma-systems.com/seal . >> + * Copyright (C) 2015 Theobroma Systems Design und Consulting GmbH >> + * >> + * This driver is inspired by the 4.0.0 version of drivers/net/can/usb/ems_usb.c > > Do you need to add the ems_usb.c's copyright here? We used the ems driver as starting point for the v1 driver that we posted. However, we are now at protocol version 3 and not much is left of the ems driver, we think we can drop this statement. >> +#include "ucan.h" >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_AUTHOR("Martin Elshuber, Theobroma Systems Design und Consulting GmbH <martin.elshuber@theobroma-systems.com>"); >> +MODULE_DESCRIPTION("Driver for Theobroma Systems UCAN devices"); > > I think we usually have these at the end of the driver. Done, moved to the end >> +#define MAX_TX_URBS 8 >> +#define MAX_RX_URBS 8 >> +#define TX_QUEUE_STOP_THRESHOLD 1 > > Please add a common prefix to these defines, e.g. UCAN_ Done >> +static struct usb_device_id ucan_table[] = { >> + {USB_DEVICE(0x2294, 0x425a)}, >> + {} /* Terminating entry */ >> +}; >> + >> +MODULE_DEVICE_TABLE(usb, ucan_table); > > Usually we have this directly in front of the function that uses it. Done >> +/* Driver private data */ >> +struct ucan { > > We usually name this struct ucan_priv, and the variable using it just > "priv". We renamed the struct to ucan_priv but left "up" (user pointer) as it is used everywhere. >> + u8 *tx_msg_buffer; >> + u8 *rx_msg_buffer; > > If you use void *, you save a lot of casts. Variables removed. Code was revised. >> + m->type = __cpu_to_le16(UCAN_OUT_COMMAND); >> + m->len = __cpu_to_le16(UCAN_OUT_LEN(m->msg.command)); > > Why do you use __cpu_to_* instead of cpu_to_*? Fixed, converted to cpu_to_*. >> + m->msg.command.cmd = cmd; >> + m->msg.command.subcmd = subcmd; >> + m->msg.command.val = __cpu_to_le16(value); >> + >> + return usb_bulk_msg(up->udev, >> + usb_sndbulkpipe(up->udev, up->out_ep->bEndpointAddress & >> + USB_ENDPOINT_NUMBER_MASK), >> + m, __le16_to_cpu(m->len), &len, 1000); > > Why do you assign &len? It's not used. You can pass in a NULL pointer, > if you are not interested in the result. > > What I don't like is the back and forth conversion of m->len. Better use > len. > > len = UCAN_OUT_LEN(m->msg.command); > m->len = cpu_to_le16(len); > > and use "len" in the usb_bulk_msg() call. Protocol version 3 does not use usb_bulk_msg anymore, control requests are moved to the control pipe. >> +/* Request the device Information */ >> +static int ucan_get_device_info(struct ucan *up) >> +{ >> + int ret; >> + int len; >> + struct can_bittiming_const *bittiming = >> + &up->device_info.bittiming_const; >> + struct ucan_message_in *m = (struct ucan_message_in *)up->rx_msg_buffer; > > Please move these two in front of the "ret" and "len". Done >> + /* retrieve the information */ >> + ret = usb_bulk_msg(up->udev, >> + usb_rcvbulkpipe(up->udev, up->in_ep->bEndpointAddress & >> + USB_ENDPOINT_NUMBER_MASK), >> + m, up->in_ep->wMaxPacketSize, &len, 1000); >> + if (ret) >> + return ret; >> + > > Please check first if "len" is the message length you think you have > received, before looking at the message's data. > >> + /* check sanity */ >> + if (m->type != __cpu_to_le16(UCAN_IN_DEVICE_INFO)) >> + return -EINVAL; >> + >> + if (__le16_to_cpu(m->len) != UCAN_IN_LEN(m->msg.device_info)) >> + return -EINVAL; Code revised. The check is now at the end of Stage 2 in ucan_probe. >> + bittiming->tseg2_max = m->msg.device_info.tseg2_max; >> + bittiming->sjw_max = m->msg.device_info.sjw_max; > Please use just one space before the "=". Done >> + if (__le16_to_cpu(m->msg.device_info.ctrlmodes) & UCAN_MODE_LOOPBACK) >> + up->can.ctrlmode_supported |= CAN_CTRLMODE_LOOPBACK; > > You might want to convert the ctrlmodes to cpu just once and assign it > to a locale variable. Done >> +/* Callback when the device sends the IRQ sate * > > Please remove the stray "*" at the end of this line. Removed >> + switch (urb->status) { >> + case 0: >> + WRITE_ONCE(up->free_slots, __le32_to_cpu(*up->irq_data)); > > Can you please create and use a struct for the irq_data, too. irq_data does not exist anymore >> + /* fill the can frame */ >> + cf->can_id = __le32_to_cpu(m->msg.can_msg.id); >> + cf->can_dlc = len - (UCAN_IN_HDR_SIZE + sizeof(m->msg.can_msg.id)); > > please add get_can_dlc(....) > > Please move the special RTR dlc handling here. > >> + >> + if (cf->can_dlc > sizeof(m->msg.can_msg.data)) >> + goto err_freeskb; > > no need to check, if you use get_can_dlc(). Done >> + if (cf->can_dlc < 0) >> + goto err_freeskb; > > This is always true, as dlc is an unsigned value. Removed >> + /* pass it to Linux */ >> + netif_receive_skb(skb); > > netif_rx(skb); > >> + >> + stats->rx_packets++; >> + stats->rx_bytes += cf->can_dlc; > > skb and thus cf are not valid anymore after netif_rx(); Fixed >> +/* callback on reception of a USB message */ >> +static void ucan_read_bulk_callback(struct urb *urb) >> +{ >> + int ret; >> + int len; >> + int pos; > = 0; I think it is better to leave them uninitialized to give the compiler a chance to detect possible read of garbage. pos is initialised just before the loop to show the code reader the initial value where it is necessary. len was moved into the loop scope. >> + /* copy the message header */ >> + memcpy(&m, urb->transfer_buffer, UCAN_IN_HDR_SIZE); > > Uh, that's only a half filled struct for now :) Yes the body was copied later. But it has been changed to cast instead of copy. >> + // copy remainder of packet > > no C++ comments Fixed (searched for all "//") >> + switch (__le16_to_cpu(m.type)) { >> + case UCAN_IN_RX: >> + ucan_rx_can_msg(up, &m); >> + break; >> + default: >> + dev_warn(&up->udev->dev, >> + "invalid input message type\n"); >> + break; >> + } > > The type check can be done before copying the data, right? Copy was removed (see 2 comments above) >> + /* get the urb context */ >> + if (WARN_ON(!context)) >> + return; > > Can this happen? Not unless there is a bug in the code. But we want to get a message before dereferencing a pointer. >> + /* urb state check */ >> + if (urb->status) >> + netdev_info(up->netdev, "Tx URB aborted (%d)\n", urb->status); >> + >> + netif_trans_update(up->netdev); > > not needed Removed, only kept in start_xmit >> + /* restart the queue if necessary */ >> + if (netif_queue_stopped(up->netdev)) >> + netif_wake_queue(up->netdev); > > Just call netif_wake_queue() directly. Fixed in two places (note; this code was revised) > As Wolfgang pointed out, please cleanup your error handling. I think we fixed the issues >> + m = usb_alloc_coherent(up->udev, size, GFP_ATOMIC, &urb->transfer_dma); >> + if (!m) { >> + netdev_err(netdev, "No memory left for USB buffer\n"); >> + usb_free_urb(urb); >> + goto drop; >> + } > > I think you can remove the netdev_err() as the framework will generate > error error messages in case of OOM. Unless there is a buffer left in a pool Linux calls dma_alloc_attrs which hands it over to the driver to allocate a buffer. If the driver does not print a message I do not see any message about that >> + /* build the USB message */ >> + if (cf->can_dlc > sizeof(m->msg.can_msg.data)) >> + cf->can_dlc = sizeof(m->msg.can_msg.data); > > Frames with illegal dlc have aready been dropped in > can_dropped_invalid_skb(). Done >> + WARN_ON_ONCE(!context); >> + if (!context) { >> + usb_free_coherent(up->udev, size, m, urb->transfer_dma); >> + usb_free_urb(urb); >> + goto drop; > > you should return NETDEV_TX_BUSY in case of no context. Done. Is it correct to not free the SKB in this case? I am not sure about this. >> + /* Slow down tx path, if fifo state is low */ >> + if ((atomic_read(&up->active_tx_urbs) >= MAX_TX_URBS) || >> + (READ_ONCE(up->free_slots) < TX_QUEUE_STOP_THRESHOLD)) { > > What's the difference between free_slots and tx_urbs? Why double account? Code was revised. There is only one variable "atomic_t avail". On open it is initialized to the tx_queue size of the device. Once it decreases to zero the queue is stopped. This variable it tightly coupled to the number of available contexts and is maintained within the respective functions On 06.02.18 12:58, Andri Yngvason wrote: >> +An UCAN device uses three USB endpoints: > Even tough "U" is a vowel, in this case, most english speaking people will read > this as "Yoocan", which makes it awkward to say "An" in front of it. I recommend > that you change the "An" to an "A". Fixed. > Nitpick: Is it not evident by the function's name what it does? >> +/* Request the device Information */ >> +static int ucan_get_device_info(struct ucan *up) >> +{ Yes, maybe, but I'd rather still have a function comment > Missing one "t" in the word "state": >> +/* Callback when the device sends the IRQ sate * Was removed (IRQ endpoint is gone) >> + if (cf->data[1] & > I think you mean to say CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE: >> + (CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING)) { >> + up->can.can_stats.error_passive++; >> + } > > Question: Does UCAN send CAN_ERR_CRTL_ACTIVE when the controller has recovered > back to error active? > > It seems that up->can.state is not set anywhere. The state should be set so that > it can be reported via netlink. up->can.state is now updated. Upon restart the driver sets the bus-state to ERROR_PASSIVE. Upon CAN events (transmission or reception) the device sends an error frame upon bus state change (see updated documenation). In case of BUS-OFF recover this indicates either ERROR_ACTIVE or BUS_OFF. > Nitpick: This function has a lot of code segments with comments that explain > what they do. It might be a good idea to turn those segments into their own > functions. This applies to other functions aswell. Anyhow, this is one of those > things that some people would call "a matter of taste", so don't sweat it. >> +/* callback when Linux needs to send a can frame */ >> +static netdev_tx_t ucan_start_xmit(struct sk_buff *skb, >> + struct net_device *netdev) >> +{ Code structure has been improved a little > What is the purpose of having a separate message struct for RTR? Does it not > just complicate things? Sure, you can shave off 8 bytes, but what's the point? >> + /*************************************************** >> + * Transmit CAN frame >> + * (type == UCAN_TX) && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) >> + ***************************************************/ We removed it > Same comment as above for the RTR: >> + /*************************************************** >> + * CAN Frame received >> + * (type == UCAN_IN_RX) >> + * && ((msg.can_msg.id & CAN_RTR_FLAG) == 0) >> + ***************************************************/ We removed it ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <06378497-1ACE-4333-810F-4E3E4706CCD5@theobroma-systems.com>]
* Re: [PATCH v2 0/1] Open questions [not found] ` <06378497-1ACE-4333-810F-4E3E4706CCD5@theobroma-systems.com> @ 2018-03-13 17:48 ` Jakob Unterwurzacher 2018-03-13 17:48 ` Marc Kleine-Budde 1 sibling, 0 replies; 25+ messages in thread From: Jakob Unterwurzacher @ 2018-03-13 17:48 UTC (permalink / raw) To: Dr. Philipp Tomsich Cc: Martin Elshuber, Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-kernel On 13.03.18 18:42, Dr. Philipp Tomsich wrote: > >> On 13 Mar 2018, at 18:40, Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> wrote: >> >>>> + /* get the urb context */ >>>> + if (WARN_ON(!context)) >>>> + return; >>> >>> Can this happen? >> >> Not unless there is a bug in the code. But we want to get a message >> before dereferencing a pointer. > > Why not use BUG_ON(!context)? I believe BUG_ON kills the kernel - the intention was to get loud message, but we'd rather not crash the user's machine. Best regards, Jakob ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 0/1] Open questions [not found] ` <06378497-1ACE-4333-810F-4E3E4706CCD5@theobroma-systems.com> 2018-03-13 17:48 ` Jakob Unterwurzacher @ 2018-03-13 17:48 ` Marc Kleine-Budde 1 sibling, 0 replies; 25+ messages in thread From: Marc Kleine-Budde @ 2018-03-13 17:48 UTC (permalink / raw) To: Dr. Philipp Tomsich, Jakob Unterwurzacher Cc: Martin Elshuber, Wolfgang Grandegger, linux-can, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 835 bytes --] On 03/13/2018 06:42 PM, Dr. Philipp Tomsich wrote: > >> On 13 Mar 2018, at 18:40, Jakob Unterwurzacher >> <jakob.unterwurzacher@theobroma-systems.com >> <mailto:jakob.unterwurzacher@theobroma-systems.com>> wrote: >> >> >> + /* get the urb context */ >> >> + if (WARN_ON(!context)) >> >> + return; >> > >> > Can this happen? >> >> Not unless there is a bug in the code. But we want to get a message >> before dereferencing a pointer. > > Why not use BUG_ON(!context)? As this will halt the whole system. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-03-16 12:15 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 17:35 [PATCH v2 0/1] can: ucan: add driver for Theobroma Systems UCAN devices Jakob Unterwurzacher
2018-03-13 17:35 ` [PATCH v2 1/1] " Jakob Unterwurzacher
2018-03-13 17:44 ` Marc Kleine-Budde
2018-03-13 17:53 ` Jakob Unterwurzacher
2018-03-13 17:56 ` Marc Kleine-Budde
2018-03-14 7:51 ` Marc Kleine-Budde
2018-03-14 9:09 ` Jakob Unterwurzacher
2018-03-14 9:36 ` rx_packets/bytes stats for error frames (was: Re: [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices) Marc Kleine-Budde
2018-03-14 9:46 ` rx_packets/bytes stats for error frames Wolfgang Grandegger
2018-03-14 9:57 ` Jakob Unterwurzacher
2018-03-14 9:11 ` [PATCH v2 1/1] can: ucan: add driver for Theobroma Systems UCAN devices Wolfgang Grandegger
2018-03-14 9:14 ` Jakob Unterwurzacher
2018-03-14 9:17 ` Wolfgang Grandegger
2018-03-14 9:21 ` Jakob Unterwurzacher
2018-03-14 9:25 ` Wolfgang Grandegger
2018-03-14 9:48 ` Jakob Unterwurzacher
2018-03-14 10:04 ` Wolfgang Grandegger
2018-03-14 10:19 ` Jakob Unterwurzacher
2018-03-14 19:07 ` Jakob Unterwurzacher
2018-03-15 6:58 ` Wolfgang Grandegger
2018-03-16 7:01 ` kbuild test robot
2018-03-16 12:14 ` kbuild test robot
2018-03-13 17:40 ` [PATCH v2 0/1] Open questions Jakob Unterwurzacher
[not found] ` <06378497-1ACE-4333-810F-4E3E4706CCD5@theobroma-systems.com>
2018-03-13 17:48 ` Jakob Unterwurzacher
2018-03-13 17:48 ` Marc Kleine-Budde
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).