Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Olivier Sobrie <hidden>
Date: 2012-08-08 13:30:05
On Wed, Aug 08, 2012 at 10:25:35AM +0200, Wolfgang Grandegger wrote:
Hi Oliver, On 08/08/2012 08:14 AM, Olivier Sobrie wrote:quoted
Hi Wolfgang, On Tue, Aug 07, 2012 at 08:26:38AM +0200, Wolfgang Grandegger wrote:quoted
On 08/06/2012 07:21 AM, Olivier Sobrie wrote:quoted
This driver provides support for several Kvaser CAN/USB devices. Such kind of devices supports up to three can network interfaces.s/can/CAN/quoted
It has been tested with a Kvaser USB Leaf Light (one network interface) connected to a pch_can interface. The firmware version of the Kvaser device was 2.5.205. List of Kvaser devices supported by the driver: - Kvaser Leaf prototype (P010v2 and v3) - Kvaser Leaf Light (P010v3) - Kvaser Leaf Professional HS - Kvaser Leaf SemiPro HS - Kvaser Leaf Professional LS - Kvaser Leaf Professional SWC - Kvaser Leaf Professional LIN - Kvaser Leaf SemiPro LS - Kvaser Leaf SemiPro SWC - Kvaser Memorator II, Prototype - Kvaser Memorator II HS/HS - Kvaser USBcan Professional HS/HS - Kvaser Leaf Light GI - Kvaser Leaf Professional HS (OBD-II connector) - Kvaser Memorator Professional HS/LS - Kvaser Leaf Light "China" - Kvaser BlackBird SemiPro - Kvaser OEM Mercury - Kvaser OEM Leaf - Kvaser USBcan RImpressive list! What CAN controllers are used inside the devices? SJA1000?I took this list from the Kvaser driver. However I only have a Kvaser Leaf Light device thus I'm not sure it will work with other ones. If you prefer I can only let Kvaser Leaf Light instead of the full list. In my device it looks to be a Renesas M16C controller.OK. Checking the manual, if available, could help to understand how the firmware handles bus errors and state changes.
Ok I'll try to find it.
quoted
quoted
quoted
Signed-off-by: Olivier Sobrie <redacted> --- Changes since v1: - added copyrights - kvaser_usb.h merged into kvaser.c - added kvaser_usb_get_endpoints to find eindpoints instead of hardcoding their address - some cleanup and comestic changes - fixed issues with errors handling - fixed restart-ms == 0 case - removed do_get_berr_counter method since the hardware doens't return good values for txerr and rxerr. If someone in the linux-usb mailing can review it, it would be nice. Concerning the errors, it behaves like that now: 1) Short-circuit CAN-H and CAN-L and restart-ms = 0 t0: short-circuit + 'cansend can1 123#112233' can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-warning,tx-error-warning} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-passive,tx-error-passive} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-off bus-error t1: remove short-circuit + 'ip link set can1 type can restart' can1 20000100 [8] 00 00 00 00 00 00 00 00 ERRORFRAME restarted-after-bus-off can1 20000004 [8] 00 0C 00 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-warning,tx-error-warning}Why do we get the last error message? Maybe the firmware does it that way (going down passive->warning->active).It goes in that order: warning -> passive -> bus off -> warning -> passive -> ...Just for curiosity? You don't see back to "error active"?
No but that's maybe because of my misunderstanding of the M16C_STATE_BUS_ERROR flag. What I see is: t1: M16C_STATE_BUS_ERROR t2: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_PASSIVE t3: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_OFF and then again t4: M16C_STATE_BUS_ERROR t2: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_PASSIVE t3: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_OFF Thus as you suggested below, the flag M16C_STATE_BUS_ERROR might not mean CAN_STATE_ERROR_WARNING...
quoted
quoted
quoted
2) Short-circuit CAN-H and CAN-L and restart-ms = 100 t0: short-circuit + cansend can1 123#112233 can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-warning,tx-error-warning} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-passive,tx-error-passive} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-off bus-error can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-warning,tx-error-warning} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error restarted-after-bus-off can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-warning,tx-error-warning} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-passive,tx-error-passive} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-off bus-error ... can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-warning,tx-error-warning} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error restarted-after-bus-off can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-warning,tx-error-warning} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-passive,tx-error-passive} protocol-violation{{tx-recessive-bit-error,error-on-tx}{}} bus-error t1: remove short-circuit can1 123 [3] 11 22 33 can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME protocol-violation{{back-to-error-active}{}}The order is still inverted but likely the firmware is doing it that way.Indeed the firmware does it that way: it sends the acknwledge of the frame beofre the state change. I can avoid that behavior by checking the state in the acknowledge frame and send the restart frame if the bus was off.Well, if the firmware does it wrong, I would not really care. Also, could you use timestamping to see if they come close together.
candump can1,0:0,#FFFFFFFF -td -e:
(001.369850) can1 123 [3] 11 22 33
(004.716034) can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME
protocol-violation{{back-to-error-active}{}}
Not so close... I'll add something in the tx acknowledge.
quoted
quoted
quoted
3) CAN-H and CAN-L disconnected t0: CAN-H and CAN-L disconnected + cansend can1 123#112233 can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-passive,tx-error-passive} can1 2000008C [8] 00 30 80 1B 00 00 00 00 ERRORFRAME controller-problem{rx-error-passive,tx-error-passive} protocol-violation{{error-on-tx}{acknowledge-delimiter}} bus-error t1: CAN-H and CAN-L reconnected can1 123 [3] 11 22 33 can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME controller-problem{rx-error-passive,tx-error-passive}Why do we get an error-passive message? Now I will have a closer look to the code...The firmware sends a CMD_CAN_ERROR_EVENT with the passive bit set...Maybe the order is again inverted. Do they come at the same time (visiable with candump -td).
(002.465349) can1 123 [3] 11 22 33
(004.562670) can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
controller-problem{rx-error-passive,tx-error-passive}
quoted
quoted
quoted
+static void kvaser_usb_rx_error(const struct kvaser_usb *dev, + const struct kvaser_msg *msg) +{ + struct can_frame *cf; + struct sk_buff *skb; + struct net_device_stats *stats; + struct kvaser_usb_net_priv *priv; + unsigned int new_state; + u8 channel, status; + + if (msg->id == CMD_CAN_ERROR_EVENT) { + channel = msg->u.error_event.channel; + status = msg->u.error_event.status; + } else { + channel = msg->u.chip_state_event.channel; + status = msg->u.chip_state_event.status; + } + + if (channel >= dev->nchannels) { + dev_err(dev->udev->dev.parent, + "Invalid channel number (%d)\n", channel); + return; + } + + priv = dev->nets[channel]; + stats = &priv->netdev->stats; + + if (status & M16C_STATE_BUS_RESET) { + kvaser_usb_unlink_tx_urbs(priv); + return; + } + skb = alloc_can_err_skb(priv->netdev, &cf); + if (!skb) { + stats->rx_dropped++; + return;Cleanup? kvaser_usb_unlink_tx_urbs()?If I get the M16C_STATE_BUS_RESET I'll not receive the ack frames anymore. I need to set the context->echo_index back to MAX_TX_URBS to not loose tx urbs. By the way I think a can_free_echo_skb() is missing...quoted
quoted
+ } + + if (status & M16C_STATE_BUS_OFF) { + cf->can_id |= CAN_ERR_BUSOFF; + + if (!priv->can.restart_ms) + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP); + + if (!priv->can.state != CAN_ERR_BUSOFF) { + priv->can.can_stats.bus_off++; + netif_carrier_off(priv->netdev); + } + + new_state = CAN_STATE_BUS_OFF; + } else if (status & M16C_STATE_BUS_PASSIVE) { + cf->can_id |= CAN_ERR_CRTL; + cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE | + CAN_ERR_CRTL_RX_PASSIVE;State changes should only be report when the state really changes. Therefore it should go under the if block below.Ok. Is it possible to get such sequence: can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME can1 20000088 [8] 00 10 90 00 00 00 00 00 ERRORFRAME can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME 2 questions: 1) If the bus is still in CAN_ERR_CRTL_RX_PASSIVE after the second frame, shouldn't we let the corresponding bit set, 0x10?No, see below.quoted
2) Can we send multiple times same frame wih same error bits set?Yes, because that's what the hardware reports.quoted
quoted
quoted
+ if (priv->can.state != CAN_STATE_ERROR_PASSIVE) + priv->can.can_stats.error_passive++; + + new_state = CAN_STATE_ERROR_PASSIVE; + } else if (status & M16C_STATE_BUS_ERROR) {Hm, strange, a bus error is not a state change. You use here if...else if... Isn't it possible that more than one bit is set.Indeed it is possible to have multiple bits set. e.g. M16C_STATE_BUS_PASSIVE + M16C_STATE_BUS_ERROR or M16C_STATE_BUS_OFF + M16C_STATE_BUS_ERROR.OK, that's the normal behaviour. Obviously they send bus errors together with the *actual* state. The hardware does usuallly report bus errors frequently while the error condition persists.quoted
What error should I report in case of M16C_STATE_BUS_ERROR?To make that clear, I have added an (old) output from the SJA1000, which is the defacto reference. Bus error reporting is enabled and no cable is connected. Watch the TX error count increasing and how the state changes:
With my hardware I do not get txerr and rxerr... I only get M16C_STATE_BUS_ERROR. Thus in fact I can only pass from ACTIVE to PASSIVE without reaching the WARNING state. People of Kvaser just told me txerr and rxerr was set with other hardwares than Kvaser Leaf Lighti... That's the one I've... I'm unlucky :-(. I'll adapt the code so that txerr and rxerr are reported for working hardwares..
$ ./candump -e 0xffff any
can0 20000088 [8] 00 00 80 19 00 08 00 00 ERRORFRAME
\ \ \-- ACK slot.
\ \-- error occured on transmission
\-- Bus-error | Protocol violations (data[2], data[3]).
can0 20000088 [8] 00 00 80 19 00 10 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 18 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 20 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 28 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 30 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 38 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 40 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 48 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 50 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 58 00 00 ERRORFRAME
can0 2000008C [8] 00 08 80 19 00 60 00 00 ERRORFRAME
\ \-- reached warning level for TX errors
\-- | Controller problems (see data[1]).
can0 20000088 [8] 00 00 80 19 00 68 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 70 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 78 00 00 ERRORFRAME
can0 2000008C [8] 00 20 80 19 00 80 00 00 ERRORFRAME
\ \-- reached passive level for TX errors
\-- | Controller problems (see data[1]).
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
\ \-- RXerror count
\-- TXerror count
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
...
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAMEOk. I'll only set CAN_ERR_PROT_LOC_ACK and not CAN_ERR_PROT_LOC_DEL.
quoted
quoted
quoted
+ cf->can_id |= CAN_ERR_CRTL; + cf->data[1] = CAN_ERR_CRTL_TX_WARNING | + CAN_ERR_CRTL_RX_WARNING;See above.quoted
+ if (priv->can.state != CAN_STATE_ERROR_WARNING) + priv->can.can_stats.error_warning++; + + new_state = CAN_STATE_ERROR_WARNING; + } else { + cf->can_id |= CAN_ERR_PROT; + cf->data[2] = CAN_ERR_PROT_ACTIVE; + + new_state = CAN_STATE_ERROR_ACTIVE; + } + + if (priv->can.restart_ms && + (priv->can.state >= CAN_STATE_BUS_OFF) && + (new_state < CAN_STATE_BUS_OFF)) { + cf->can_id |= CAN_ERR_RESTARTED; + priv->can.can_stats.restarts++; + netif_carrier_on(priv->netdev); + } + + if (msg->id == CMD_CAN_ERROR_EVENT) { + u8 error_factor = msg->u.error_event.error_factor; + + priv->can.can_stats.bus_error++; + stats->rx_errors++; + + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + + if (error_factor & M16C_EF_ACKE) + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK | + CAN_ERR_PROT_LOC_ACK_DEL); + if (error_factor & M16C_EF_CRCE) + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ | + CAN_ERR_PROT_LOC_CRC_DEL); + if (error_factor & M16C_EF_FORME) + cf->data[2] |= CAN_ERR_PROT_FORM; + if (error_factor & M16C_EF_STFE) + cf->data[2] |= CAN_ERR_PROT_STUFF; + if (error_factor & M16C_EF_BITE0) + cf->data[2] |= CAN_ERR_PROT_BIT0; + if (error_factor & M16C_EF_BITE1) + cf->data[2] |= CAN_ERR_PROT_BIT1; + if (error_factor & M16C_EF_TRE) + cf->data[2] |= CAN_ERR_PROT_TX; + } + + priv->can.state = new_state; + + if (!memcmp(cf, &priv->cf_err_old, sizeof(*cf))) { + kfree_skb(skb); + return; + }Hm, the firmware seems not to clear error conditions? Anyway, state change and error reporting is magic on many CAN controllers. Just the SJA1000 is doing it nicely.I added it to prevent sending two times the same error. It happens that the firmware sends multiple times the same error message.Can then be removed, I think, see above.
Ok.
quoted
quoted
quoted
+ netif_rx(skb); + + priv->cf_err_old = *cf; + + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; +} + +static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev, + const struct kvaser_msg *msg) +{ + struct kvaser_usb_net_priv *priv; + struct can_frame *cf; + struct sk_buff *skb; + struct net_device_stats *stats; + u8 channel = msg->u.rx_can.channel; + + if (channel >= dev->nchannels) { + dev_err(dev->udev->dev.parent, + "Invalid channel number (%d)\n", channel); + return; + } + + priv = dev->nets[channel]; + stats = &priv->netdev->stats; + + skb = alloc_can_skb(priv->netdev, &cf); + if (skb == NULL) {s/skb == NULL)/!skb/ ?quoted
+ stats->tx_dropped++; + return; + } + + cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) | + (msg->u.rx_can.msg[1] & 0x3f); + cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]); + + if (msg->id == CMD_RX_EXT_MESSAGE) { + cf->can_id <<= 18; + cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) | + ((msg->u.rx_can.msg[3] & 0xff) << 6) | + (msg->u.rx_can.msg[4] & 0x3f); + cf->can_id |= CAN_EFF_FLAG; + } + + if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) { + cf->can_id |= CAN_RTR_FLAG; + } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME | + MSG_FLAG_NERR)) { + cf->can_id |= CAN_ERR_FLAG; + cf->can_dlc = CAN_ERR_DLC; + cf->data[1] = CAN_ERR_CRTL_UNSPEC;What is the meaning of such errors? A comment, netdev_err() or netdev_dbg() would be nice.I never reached this error with the hardware I've... I don't know what's the meaning of this flag... I will add a trace.Then add a netdev_err().
Ok. Thank you, -- Olivier -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html