Hi, On Mon, Oct 19, 2009 at 10:42 AM, Wolfgang Grandegger <[email protected]> wrote:
>> example I discovered by chance that when I disconnect the bus the >> mcp2510 goes in rx error passive and when I short-circuit it goes into >> tx error passive and then bus off). Any more testing is much > > That's the normal behavior, just the direction seems wrong. > let me add that I was cangen-ing in both directions to catch race conditions. Here comes the patch: ---8<----- This patch: - fixes a bug in interpreting RTR frames with no extended-id frames - fixes missing device name in bittiming struct and other minor buglets. - implements correct behaviour on bus-off (according to can_restart_ms setting). If can_restart_ms is zero the mcp251x is put on sleep until restarted. - fixes not handling CANINTF_MERRF irq - takes out warnings that give false positives on bus errors - fixes sending multiple errors frames on bus state transitions. Now only one frame is generated on each transition. Signed-off: Christian Pellegrin <[email protected]> -- Index: kernel/2.6/drivers/net/can/mcp251x.c =================================================================== --- kernel/2.6/drivers/net/can/mcp251x.c (revision 1070) +++ kernel/2.6/drivers/net/can/mcp251x.c (working copy) @@ -159,6 +159,7 @@ MODULE_PARM_DESC(mcp251x_enable_dma, "Enable SPI DMA. Default: 0 (Off)"); static struct can_bittiming_const mcp251x_bittiming_const = { + .name = DEVICE_NAME, .tseg1_min = 3, .tseg1_max = 16, .tseg2_min = 2, @@ -192,6 +193,7 @@ #define AFTER_SUSPEND_UP 1 #define AFTER_SUSPEND_DOWN 2 #define AFTER_SUSPEND_POWER 4 +#define AFTER_SUSPEND_RESTART 8 int restart_tx; }; @@ -381,16 +383,15 @@ frame->can_id |= ((rx_buf[2] & 3) << 16) | (rx_buf[3] << 8) | rx_buf[4] | (((rx_buf[1] << 3) | (rx_buf[2] >> 5)) << 18); + if ((rx_buf[5] >> 6) & 0x1) { + /* Remote transmission request */ + frame->can_id |= CAN_RTR_FLAG; + } } else { /* Standard ID format */ frame->can_id = (rx_buf[1] << 3) | (rx_buf[2] >> 5); } - if ((rx_buf[5] >> 6) & 0x1) { - /* Remote transmission request */ - frame->can_id |= CAN_RTR_FLAG; - } - /* Data length */ frame->can_dlc = rx_buf[5] & 0x0f; if (frame->can_dlc > 8) { @@ -427,16 +428,15 @@ frame->can_id |= ((rx_buf[2] & 3) << 16) | (rx_buf[3] << 8) | rx_buf[4] | (((rx_buf[1] << 3) | (rx_buf[2] >> 5)) << 18); + if ((rx_buf[5] >> 6) & 0x1) { + /* Remote transmission request */ + frame->can_id |= CAN_RTR_FLAG; + } } else { /* Standard ID format */ frame->can_id = (rx_buf[1] << 3) | (rx_buf[2] >> 5); } - if ((rx_buf[5] >> 6) & 0x1) { - /* Remote transmission request */ - frame->can_id |= CAN_RTR_FLAG; - } - /* Data length */ frame->can_dlc = rx_buf[5] & 0x0f; if (frame->can_dlc > 8) { @@ -516,7 +516,10 @@ switch (mode) { case CAN_MODE_START: /* we have to delay work since SPI I/O may sleep */ + priv->can.state = CAN_STATE_ERROR_ACTIVE; priv->restart_tx = 1; + if (priv->can.restart_ms == 0) + priv->after_suspend = AFTER_SUSPEND_RESTART; queue_work(priv->wq, &priv->irq_work); break; default: @@ -534,7 +537,8 @@ /* Enable interrupts */ mcp251x_write_reg(spi, CANINTE, CANINTE_ERRIE | CANINTE_TX2IE | CANINTE_TX1IE | - CANINTE_TX0IE | CANINTE_RX1IE | CANINTE_RX0IE); + CANINTE_TX0IE | CANINTE_RX1IE | CANINTE_RX0IE | + CANINTF_MERRF); if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) { /* Put device into loopback mode */ @@ -570,7 +574,6 @@ bt->sjw); /* Store original mode and set mode to config */ - state = mcp251x_read_reg(spi, CANCTRL); state = mcp251x_read_reg(spi, CANSTAT) & CANCTRL_REQOP_MASK; mcp251x_write_bits(spi, CANCTRL, CANCTRL_REQOP_MASK, CANCTRL_REQOP_CONF); @@ -712,10 +715,15 @@ struct net_device *net = priv->net; struct can_frame *frame; - WARN_ON(!priv->tx_skb); - WARN_ON(priv->tx_len); if (priv->tx_skb) { frame = (struct can_frame *)priv->tx_skb->data; + + if (priv->can.state == CAN_STATE_BUS_OFF) { + net->stats.tx_errors++; + mcp251x_clean(priv); + netif_wake_queue(net); + return; + } if (frame->can_dlc > CAN_FRAME_MAX_DATA_LEN) frame->can_dlc = CAN_FRAME_MAX_DATA_LEN; mcp251x_hw_tx(spi, frame, 0); @@ -736,11 +744,12 @@ enum can_state new_state; if (priv->after_suspend) { - /* Wait whilst the device wakes up WARN_ON */ mdelay(10); mcp251x_hw_reset(spi); mcp251x_setup(net, priv, spi); - if (priv->after_suspend & AFTER_SUSPEND_UP) { + if (priv->after_suspend & AFTER_SUSPEND_RESTART) { + mcp251x_set_normal_mode(spi); + } else if (priv->after_suspend & AFTER_SUSPEND_UP) { netif_device_attach(net); /* clear since we lost tx buffer */ if (priv->tx_skb || priv->tx_len) { @@ -755,17 +764,25 @@ return; } + if (priv->can.restart_ms == 0 && priv->can.state == CAN_STATE_BUS_OFF) { + while (!priv->force_quit && !freezing(current) && + (intf = mcp251x_read_reg(spi, CANINTF))) + mcp251x_write_bits(spi, CANINTF, intf, 0x00); + return; + } + while (!priv->force_quit && !freezing(current)) { + u8 eflag = mcp251x_read_reg(spi, EFLG); + + mcp251x_write_reg(spi, EFLG, 0x00); + if (priv->restart_tx) { priv->restart_tx = 0; - dev_dbg(&spi->dev, - "timeout in txing a packet, restarting\n"); mcp251x_write_reg(spi, TXBCTRL(0), 0); if (priv->tx_skb || priv->tx_len) { net->stats.tx_errors++; mcp251x_clean(priv); } - priv->can.can_stats.restarts++; netif_wake_queue(net); } @@ -787,8 +804,6 @@ /* if there are pending Tx buffers, restart queue */ txbnctrl = mcp251x_read_reg(spi, TXBCTRL(0)); if (!(txbnctrl & TXBCTRL_TXREQ)) { - WARN_ON(priv->tx_skb); - WARN_ON(!priv->tx_len); if (priv->tx_skb || priv->tx_len) { net->stats.tx_errors++; mcp251x_clean(priv); @@ -800,7 +815,6 @@ if (intf & CANINTF_ERRIF) { struct sk_buff *skb; struct can_frame *frame = NULL; - u8 eflag = mcp251x_read_reg(spi, EFLG); /* create error frame */ skb = alloc_can_err_skb(net, &frame); @@ -840,26 +854,22 @@ } } - /* update can state */ - if (eflag & EFLG_TXBO) { - new_state = CAN_STATE_BUS_OFF; - can_bus_off(net); - } else if (eflag & EFLG_TXEP) - new_state = CAN_STATE_ERROR_PASSIVE; - else if (eflag & EFLG_RXEP) - new_state = CAN_STATE_ERROR_PASSIVE; - else if (eflag & EFLG_TXWAR) - new_state = CAN_STATE_ERROR_WARNING; - else if (eflag & EFLG_RXWAR) - new_state = CAN_STATE_ERROR_WARNING; - else - new_state = CAN_STATE_ERROR_ACTIVE; - - mcp251x_write_reg(spi, EFLG, 0x00); - if (skb) netif_rx(skb); - } else + } + + /* update can state */ + if (eflag & EFLG_TXBO) + new_state = CAN_STATE_BUS_OFF; + else if (eflag & EFLG_TXEP) + new_state = CAN_STATE_ERROR_PASSIVE; + else if (eflag & EFLG_RXEP) + new_state = CAN_STATE_ERROR_PASSIVE; + else if (eflag & EFLG_TXWAR) + new_state = CAN_STATE_ERROR_WARNING; + else if (eflag & EFLG_RXWAR) + new_state = CAN_STATE_ERROR_WARNING; + else new_state = CAN_STATE_ERROR_ACTIVE; /* update can state statistics */ @@ -873,20 +883,22 @@ new_state <= CAN_STATE_BUS_OFF) priv->can.can_stats.error_passive++; break; - case CAN_STATE_BUS_OFF: - if (new_state <= CAN_STATE_ERROR_PASSIVE) - netif_carrier_on(net); - break; default: break; } priv->can.state = new_state; + if (priv->can.state == CAN_STATE_BUS_OFF) { + if (priv->can.restart_ms == 0) { + can_bus_off(net); + mcp251x_hw_sleep(spi); + return; + } + } + if (intf & (CANINTF_TX2IF | CANINTF_TX1IF | CANINTF_TX0IF)) { net->stats.tx_packets++; net->stats.tx_bytes += priv->tx_len - 1; - WARN_ON(priv->tx_skb); - WARN_ON(!priv->tx_len); if (priv->tx_len) { can_get_echo_skb(net, 0); priv->tx_len = 0; @@ -900,10 +912,7 @@ if (intf & CANINTF_RX1IF) mcp251x_hw_rx(spi, 1); - } - - mcp251x_read_reg(spi, CANSTAT); } static irqreturn_t mcp251x_can_isr(int irq, void *dev_id) -- Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room." _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
