Hello Jacob, just a few minor issues and a question about bus-off handling...
Am 03.04.2018 um 16: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.elshu...@theobroma-systems.com> > Signed-off-by: Jakob Unterwurzacher > <jakob.unterwurzac...@theobroma-systems.com> > Signed-off-by: Philipp Tomsich <philipp.toms...@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 | 1596 > ++++++++++++++++++++++++ > 5 files changed, 1923 insertions(+) > create mode 100644 Documentation/networking/can_ucan_protocol.rst > create mode 100644 drivers/net/can/usb/ucan.c > [..snip..] > 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..bb2d62dcbd7b > --- /dev/null > +++ b/drivers/net/can/usb/ucan.c > @@ -0,0 +1,1596 @@ > +// 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> [...snip...] > +static struct ucan_urb_context *ucan_alloc_context(struct ucan_priv *up) > +{ > + int i; > + unsigned long flags; > + struct ucan_urb_context *ret = NULL; > + > + if (WARN_ON_ONCE(!up->context_array)) > + return NULL; > + > + /* execute context operation atomically */ > + spin_lock_irqsave(&up->context_lock, flags); > + > + for (i = 0; i < up->device_info.tx_fifo; i++) { > + if (!up->context_array[i].allocated) { > + /* update context */ > + ret = &up->context_array[i]; > + up->context_array[i].allocated = true; > + > + /* stop queue if necessary */ > + up->available_tx_urbs--; > + if (!up->available_tx_urbs) > + netif_stop_queue(up->netdev); > + > + goto done_restore; break instead of goto!? > + } > + } > + > +done_restore: > + spin_unlock_irqrestore(&up->context_lock, flags); > + return ret; > +} > + > +static bool ucan_release_context(struct ucan_priv *up, > + struct ucan_urb_context *ctx) > +{ > + unsigned long flags; > + bool ret = false; > + > + if (WARN_ON_ONCE(!up->context_array)) > + return false; > + > + /* execute context operation atomically */ > + spin_lock_irqsave(&up->context_lock, flags); > + > + /* context was not allocated, maybe the device sent garbage */ > + if (!ctx->allocated) > + goto done_restore; if (ctx->allocated) { > + ctx->allocated = false; > + > + /* check if the queue needs to be woken */ > + if (!up->available_tx_urbs) > + netif_wake_queue(up->netdev); > + up->available_tx_urbs++; > + } > +done_restore: No need for a label. > + spin_unlock_irqrestore(&up->context_lock, flags); > + return ret; Hm, what is "ret" good for? The value is always "false". Maybe it's better to print an error message here and make this function void. > +} [...snip...] > +/* Handle a CAN error frame that we have received from the device. > + * Returns true if the can state has changed. > + */ > +static bool ucan_handle_error_frame(struct ucan_priv *up, > + struct ucan_message_in *m, > + canid_t canid) > +{ > + enum can_state new_state = up->can.state; > + 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_OVERFLOW) > + net_stats->rx_over_errors++; > + > + /* controller state bits: if multiple are set the worst wins */ > + if (d1 & CAN_ERR_CRTL_ACTIVE) > + new_state = CAN_STATE_ERROR_ACTIVE; > + > + if (d1 & (CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING)) > + new_state = CAN_STATE_ERROR_WARNING; > + > + if (d1 & (CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE)) > + new_state = CAN_STATE_ERROR_PASSIVE; > + } > + > + /* 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++; > + } > + > + /* no state change - we are done */ > + if (up->can.state == new_state) > + return false; > + > + /* we switched into a better state */ > + if (up->can.state > new_state) { > + up->can.state = new_state; > + return true; > + } > + > + /* 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); How does the CAN controller firmware handle bus-off? Does it recover automatically? "can_bus_off()" will restart the CAN controller after "restart-ms" or manually via netlink "restart" command by calling "ucan_set_mode(CAN_START_MODE)": # ip link set canX type can restart-ms 100 # ip link set canX type can restart See also: https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L597 > + netdev_info(up->netdev, "link has gone into BUS-OFF state\n"); "can_bus_off()" already prints a message. > + break; > + case CAN_STATE_ERROR_PASSIVE: > + can_stats->error_passive++; > + break; > + case CAN_STATE_ERROR_WARNING: > + can_stats->error_warning++; > + break; > + default: > + break; > + } > + return true; > +} > + > +/* 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; > + canid_t 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)) { > + netdev_warn(up->netdev, "invalid input message len: %d\n", len); > + return; > + } > + > + /* handle error frames */ > + canid = le32_to_cpu(m->msg.can_msg.id); > + if (canid & CAN_ERR_FLAG) { > + bool busstate_changed = ucan_handle_error_frame(up, m, canid); > + > + /* if berr-reporting is off only state changes get through */ > + if (!(up->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) && > + !busstate_changed) > + return; > + } else { > + canid_t canid_mask; > + /* compute the mask for canid */ > + canid_mask = CAN_RTR_FLAG; > + if (canid & CAN_EFF_FLAG) > + canid_mask |= CAN_EFF_MASK | CAN_EFF_FLAG; > + else > + canid_mask |= CAN_SFF_MASK; > + > + if (canid & ~canid_mask) > + netdev_warn(up->netdev, > + "masking unexpected set bits in canid > (canid %x, mask %x)", Line too long... there are a few more lines exceeding 80 chars. > + canid, canid_mask); > + > + canid &= canid_mask; > + } > + > + /* allocate skb */ > + skb = alloc_can_skb(up->netdev, &cf); > + if (!skb) > + return; > + > + /* fill the can frame */ > + cf->can_id = canid; > + > + /* compute DLC taking RTR_FLAG into account */ > + cf->can_dlc = ucan_get_can_dlc(&m->msg.can_msg, len); > + > + /* copy the payload of non RTR frames */ > + if (!(cf->can_id & CAN_RTR_FLAG) || (cf->can_id & CAN_ERR_FLAG)) > + memcpy(cf->data, m->msg.can_msg.data, cf->can_dlc); > + > + /* don't count error frames as real packets */ > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + > + /* pass it to Linux */ > + netif_rx(skb); > +} [...snip...] You may also want to replace it (ret) goto err; with return ret; in "ucan_open()" and "ucan_probe()". You can then add my: Acked-by: Wolfgang Grandegger <w...@grandegger.com> Thanks for your contribution! Wolfgang.
