Marc Kleine-Budde wrote:
> Hello,
>
> here "./strip-src -m -i -v 2.6.31" version of the at91 driver,
> featuring:
> - reworked RX-path
> - NAPI for RX and CAN frame errors
> TX and CAN state is still managed from irq handler
> - CAN chip is turned off after a bus off
> the NAPI is stopped via a work_queue, because
> NAPI cannot be disabled from IRQ context
> (where the bus off is detected).
Very nice. Do you really need to stop the NAPI in case of bus-off? You
stopped the device and no more message will come in anyhow.
> I have to figure out if disabling NAPI is possbile from NAPI conext, so
> CAN state transition might be moved to NAPI, too.
>
> Bus off and manual restart via the "ip" utility is working. However the
> last remaining bits of the auto recover handling must be removed,
> though.
Did you follow our recent discussion on how to handle auto recovery? The
preferred solution is to *stop* the device if priv->can.restart_ms ==
0. If it's > 0, the hardware should do the recovery and when the state
changes to error-active, an RESTARTED error message should be generated.
> The driver with NAPI receiving messages with a length of 1 on a 1 Mbit
> link without swapping the messages (tested with pengutronix cansequence).
Please use Vladislavs' canecho_gen/duts at 125kB/sec, which is more
sensitive. Don't forget to fix the unhanded errno ENOBUFS of the write
functions.
> However if I add some "ping -f" load the driver keeps working (tested
> with looking at the data bytes in the driver), but the userspace doesn't
> get enough cpu cycles so AFAICS the pacakges must be dropped in the
> network layer.
>
> Even on the 1 Mbit link the total number IRQs isn't reduced by NAPI
> (less than 1%). But with NAPI I can enable the Acknowledgement Error
> even on 1 Mbit, and the system doesn't lock up. It generated ~15K Int/s.
With NAPI, message processing is moved from the interrupt to the process
context, which efficiently avoids lockups. As longs as CPU resources are
available, no messages will be dropped.
> Adding again "ping -f" load, the CAN interrupts drop to ~11K Int/s,
> the Ethernet causes only ~7K Int/s.
>
> Maybe we can think of some really "slow" polling for such interrupts.
> IIRC the SJA1000 has the same problem.
I remember some discussion on this topic a long time ago and there it
was regarded important to get and inspect all bus errors. Throttling was
not accepted.
> Here's the driver, comments welcome. There are some functions, that
> might be moved out of the driver into can/dev.c (alloc_can*frame).
There are various general coding style issues:
In source files, functions should be separated with *one* blank line.
The way you break function declarations is sometimes very unusual, e.g.:
static void at91_irq_err_state
(struct net_device *dev, struct can_frame *cf, enum can_state new_state)
I think:
static void at91_irq_err_state(struct net_device *dev,
struct can_frame *cf,
enum can_state new_state)
or
static void at91_irq_err_state(struct net_device *dev,
struct can_frame *cf, enum can_state new_state)
is better readable. More comments inline.
> cheers, Marc
>
> --- /dev/null 2009-04-16 11:22:08.000000000 +0200
> +++ at91_can.c 2009-09-11 00:15:50.000000000 +0200
> @@ -0,0 +1,1235 @@
> +/*
> + * at91_can.c - CAN network driver for AT91 SoC CAN controller
> + *
> + * (C) 2007 by Hans J. Koch <[email protected]>
> + * (C) 2008, 2009 by Marc Kleine-Budde <[email protected]>
> + *
> + * This software may be distributed under the terms of the GNU General
> + * Public License ("GPL") version 2 as distributed in the 'COPYING'
> + * file from the main directory of the linux kernel source.
> + *
> + * Send feedback to <[email protected]>
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/netdevice.h>
> +#include <linux/spinlock.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/if_arp.h>
> +#include <linux/skbuff.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +
> +#include <socketcan/can.h>
> +#include <socketcan/can/error.h>
> +#include <socketcan/can/dev.h>
> +
> +#include <mach/board.h>
> +
> +#define DRV_NAME "at91_can"
> +#define AT91_NAPI_WEIGHT 12
> +
> +/*
> + * RX/TX Mailbox split
> + * don't dare to touch
> + */
> +#define AT91_MB_RX_NUM 12
> +#define AT91_MB_TX_SHIFT 2
> +
> +#define AT91_MB_RX_FIRST 0
> +#define AT91_MB_RX_LAST (AT91_MB_RX_FIRST + AT91_MB_RX_NUM - 1)
> +#define AT91_MB_RX_BANKS 2
> +#define AT91_MB_RX_BANK_WIDTH (AT91_MB_RX_NUM / AT91_MB_RX_BANKS)
> +#define AT91_MB_RX_BANK_LAST(i) (AT91_MB_RX_FIRST + \
> + AT91_MB_RX_BANK_WIDTH * ((i) + 1) - 1)
> +#define AT91_MB_RX_BANK_MASK(i) (((1 << AT91_MB_RX_BANK_WIDTH) - 1) << \
> + (AT91_MB_RX_BANK_WIDTH * (i)))
> +#define AT91_MB_RX_MASK(i) ((1 << (i)) - 1)
> +
> +#define AT91_MB_TX_NUM (1 << AT91_MB_TX_SHIFT)
> +#define AT91_MB_TX_FIRST (AT91_MB_RX_LAST + 1)
> +#define AT91_MB_TX_LAST (AT91_MB_TX_FIRST + AT91_MB_TX_NUM - 1)
> +
> +#define AT91_NEXT_PRIO_SHIFT (AT91_MB_TX_SHIFT)
> +#define AT91_NEXT_PRIO_MASK (0xf << AT91_MB_TX_SHIFT)
> +#define AT91_NEXT_MB_MASK (AT91_MB_TX_NUM - 1)
> +#define AT91_NEXT_MASK ((AT91_MB_TX_NUM - 1) |
> AT91_NEXT_PRIO_MASK)
> +
> +/* Common registers */
> +enum at91_reg {
> + AT91_MR = 0x000,
> + AT91_IER = 0x004,
> + AT91_IDR = 0x008,
> + AT91_IMR = 0x00C,
> + AT91_SR = 0x010,
> + AT91_BR = 0x014,
> + AT91_TIM = 0x018,
> + AT91_TIMESTP = 0x01C,
> + AT91_ECR = 0x020,
> + AT91_TCR = 0x024,
> + AT91_ACR = 0x028,
> +};
> +
> +/* Mailbox registers (0 <= i <= 15) */
> +#define AT91_MMR(i) (enum at91_reg)(0x200 + ((i) * 0x20))
> +#define AT91_MAM(i) (enum at91_reg)(0x204 + ((i) * 0x20))
> +#define AT91_MID(i) (enum at91_reg)(0x208 + ((i) * 0x20))
> +#define AT91_MFID(i) (enum at91_reg)(0x20C + ((i) * 0x20))
> +#define AT91_MSR(i) (enum at91_reg)(0x210 + ((i) * 0x20))
> +#define AT91_MDL(i) (enum at91_reg)(0x214 + ((i) * 0x20))
> +#define AT91_MDH(i) (enum at91_reg)(0x218 + ((i) * 0x20))
> +#define AT91_MCR(i) (enum at91_reg)(0x21C + ((i) * 0x20))
> +
> +/* Register bits */
> +#define AT91_MR_AT91EN BIT(0)
> +#define AT91_MR_LPM BIT(1)
> +#define AT91_MR_ABM BIT(2)
> +#define AT91_MR_OVL BIT(3)
> +#define AT91_MR_TEOF BIT(4)
> +#define AT91_MR_TTM BIT(5)
> +#define AT91_MR_TIMFRZ BIT(6)
> +#define AT91_MR_DRPT BIT(7)
> +
> +#define AT91_SR_RBSY BIT(29)
> +
> +#define AT91_MMR_PRIO_SHIFT (16)
> +
> +#define AT91_MID_MIDE BIT(29)
> +
> +#define AT91_MSR_MRTR BIT(20)
> +#define AT91_MSR_MABT BIT(22)
> +#define AT91_MSR_MRDY BIT(23)
> +#define AT91_MSR_MMI BIT(24)
> +
> +#define AT91_MCR_MRTR BIT(20)
> +#define AT91_MCR_MTCR BIT(23)
> +
> +/* Mailbox Modes */
> +enum at91_mb_mode {
> + AT91_MB_MODE_DISABLED = 0,
> + AT91_MB_MODE_RX = 1,
> + AT91_MB_MODE_RX_OVRWR = 2,
> + AT91_MB_MODE_TX = 3,
> + AT91_MB_MODE_CONSUMER = 4,
> + AT91_MB_MODE_PRODUCER = 5,
> +};
> +
> +/* Interrupt mask bits */
> +#define AT91_IRQ_MB_RX ((1 << (AT91_MB_RX_LAST + 1)) \
> + - (1 << AT91_MB_RX_FIRST))
> +#define AT91_IRQ_MB_TX ((1 << (AT91_MB_TX_LAST + 1)) \
> + - (1 << AT91_MB_TX_FIRST))
> +#define AT91_IRQ_MB_AL (AT91_IRQ_MB_RX | AT91_IRQ_MB_TX)
> +
> +#define AT91_IRQ_ERRA (1 << 16)
> +#define AT91_IRQ_WARN (1 << 17)
> +#define AT91_IRQ_ERRP (1 << 18)
> +#define AT91_IRQ_BOFF (1 << 19)
> +#define AT91_IRQ_SLEEP (1 << 20)
> +#define AT91_IRQ_WAKEUP (1 << 21)
> +#define AT91_IRQ_TOVF (1 << 22)
> +#define AT91_IRQ_TSTP (1 << 23)
> +#define AT91_IRQ_CERR (1 << 24)
> +#define AT91_IRQ_SERR (1 << 25)
> +#define AT91_IRQ_AERR (1 << 26)
> +#define AT91_IRQ_FERR (1 << 27)
> +#define AT91_IRQ_BERR (1 << 28)
> +
> +#define AT91_IRQ_ERR_ALL (0x1fff0000)
> +#define AT91_IRQ_ERR_FRAME (AT91_IRQ_CERR | AT91_IRQ_SERR | \
> + AT91_IRQ_AERR | AT91_IRQ_FERR | AT91_IRQ_BERR)
> +#define AT91_IRQ_ERR_LINE (AT91_IRQ_ERRA | AT91_IRQ_WARN | \
> + AT91_IRQ_ERRP | AT91_IRQ_BOFF)
> +
> +#define AT91_IRQ_ALL (0x1fffffff)
> +
> +struct at91_priv {
> + struct can_priv can; /* must be the first member! */
> + struct net_device *dev;
> + struct napi_struct napi;
> +
> + void __iomem *reg_base;
> +
> + u32 reg_sr;
> + unsigned int tx_next;
> + unsigned int tx_echo;
> + unsigned int rx_next;
> +
> + struct work_struct bus_off_task;
> +
> + struct clk *clk;
> + struct at91_can_data *pdata;
> +};
> +
> +
> +static struct can_bittiming_const at91_bittiming_const = {
> + .tseg1_min = 4,
> + .tseg1_max = 16,
> + .tseg2_min = 2,
> + .tseg2_max = 8,
> + .sjw_max = 4,
> + .brp_min = 2,
> + .brp_max = 128,
> + .brp_inc = 1,
> +};
Why do you don't use tab here fore aligment of the numbers?
> +
> +static inline int get_tx_next_mb(struct at91_priv *priv)
> +{
> + return (priv->tx_next & AT91_NEXT_MB_MASK) + AT91_MB_TX_FIRST;
> +}
> +
> +static inline int get_tx_next_prio(struct at91_priv *priv)
> +{
> + return (priv->tx_next >> AT91_NEXT_PRIO_SHIFT) & 0xf;
> +}
> +
> +static inline int get_tx_echo_mb(struct at91_priv *priv)
> +{
> + return (priv->tx_echo & AT91_NEXT_MB_MASK) + AT91_MB_TX_FIRST;
> +}
> +
> +
> +static inline u32 at91_read(struct net_device *dev, enum at91_reg reg)
> +{
> + struct at91_priv *priv = netdev_priv(dev);
> + return readl(priv->reg_base + reg);
> +}
> +
> +static inline void
> +at91_write(struct net_device *dev, enum at91_reg reg, u32 value)
> +{
> + struct at91_priv *priv = netdev_priv(dev);
> + writel(value, priv->reg_base + reg);
> +}
> +
> +
> +static inline void
> +set_mb_mode_prio(struct net_device *dev, unsigned int mb,
> + enum at91_mb_mode mode, int prio)
> +{
> + at91_write(dev, AT91_MMR(mb),
> + (mode << 24) | (prio << 16));
Fits on one line?
> +}
> +
> +static inline void
> +set_mb_mode(struct net_device *dev, unsigned int mb, enum at91_mb_mode mode)
> +{
> + set_mb_mode_prio(dev, mb, mode, 0);
> +}
> +
> +
> +static struct sk_buff *
> +alloc_can_frame(struct net_device *dev, struct can_frame **cf)
> +{
> + struct sk_buff *skb;
> +
> + skb = netdev_alloc_skb(dev, sizeof(struct can_frame));
> + if (unlikely(!skb))
> + return NULL;
> +
> + skb->protocol = htons(ETH_P_CAN);
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> +
> + return skb;
> +}
> +
> +
> +static struct sk_buff *
> +alloc_can_err_frame(struct net_device *dev, struct can_frame **cf)
> +{
> + struct sk_buff *skb;
> +
> + skb = alloc_can_frame(dev, cf);
> + if (unlikely(!skb))
> + return NULL;
> +
> + memset(*cf, 0, sizeof(struct can_frame));
> + (*cf)->can_id = CAN_ERR_FLAG;
> + (*cf)->can_dlc = CAN_ERR_DLC;
> +
> + return skb;
> +}
Yes, we should put similar functions into dev.[ch]. But I would use the
suffix "skb" instead of "frame" because the return value is of type
"static struct sk_buff *". Patches are welcome.
> +
> +/*
> + * Swtich transceiver on or off
> + */
> +static void at91_transceiver_switch(struct at91_priv *priv, int on)
> +{
> + if (priv->pdata && priv->pdata->transceiver_switch)
> + priv->pdata->transceiver_switch(on);
> +}
> +
> +
> +static void at91_setup_mailboxes(struct net_device *dev)
> +{
> + struct at91_priv *priv = netdev_priv(dev);
> + unsigned int i;
> +
> + /*
> + * The first 12 mailboxes are used as a reception FIFO. The
> + * last mailbox is configured with overwrite option. The
> + * overwrite flag indicates a FIFO overflow.
> + */
> + for (i = AT91_MB_RX_FIRST; i < AT91_MB_RX_LAST; i++)
> + set_mb_mode(dev, i, AT91_MB_MODE_RX);
> + set_mb_mode(dev, AT91_MB_RX_LAST, AT91_MB_MODE_RX_OVRWR);
> +
> + /* The last 4 mailboxes are used for transmitting. */
> + for (i = AT91_MB_TX_FIRST; i <= AT91_MB_TX_LAST; i++)
> + set_mb_mode_prio(dev, i, AT91_MB_MODE_TX, 0);
> +
> + /* Reset tx and rx helper pointers */
> + priv->tx_next = priv->tx_echo = priv->rx_next = 0;
> +}
> +
> +
> +static int at91_set_bittiming(struct net_device *dev)
> +{
> + struct at91_priv *priv = netdev_priv(dev);
> + struct can_bittiming *bt = &priv->can.bittiming;
> + u32 reg_br;
> +
> + reg_br = ((priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) << 24) |
> + ((bt->brp - 1) << 16) |
> + ((bt->sjw - 1) << 12) |
> + ((bt->prop_seg - 1) << 8) |
> + ((bt->phase_seg1 - 1) << 4) |
> + ((bt->phase_seg2 - 1) << 0);
No alignment please.
> +
> + dev_info(ND2D(dev), "writing AT91_BR: 0x%08x\n", reg_br);
> +
> + at91_write(dev, AT91_BR, reg_br);
> +
> + return 0;
> +}
> +
> +
> +static void at91_chip_start(struct net_device *dev)
> +{
> + struct at91_priv *priv = netdev_priv(dev);
> + u32 reg_mr, reg_ier;
> +
> + /* disable interrupts */
> + at91_write(dev, AT91_IDR, AT91_IRQ_ALL);
> +
> + /* disable chip */
> + reg_mr = at91_read(dev, AT91_MR);
> + at91_write(dev, AT91_MR, reg_mr & ~AT91_MR_AT91EN);
> +
> + at91_setup_mailboxes(dev);
> + at91_transceiver_switch(priv, 1);
> +
> + /* enable chip */
> + reg_mr = at91_read(dev, AT91_MR);
> + at91_write(dev, AT91_MR, reg_mr | AT91_MR_AT91EN);
> +
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + /* Enable interrupts */
> + reg_ier = AT91_IRQ_MB_RX | AT91_IRQ_ERRP | AT91_IRQ_ERR_FRAME;
> + at91_write(dev, AT91_IDR, AT91_IRQ_ALL);
> + at91_write(dev, AT91_IER, reg_ier);
> +
> + napi_enable(&priv->napi);
> +}
> +
> +
> +static void at91_chip_stop(struct net_device *dev, enum can_state state)
> +{
> + struct at91_priv *priv = netdev_priv(dev);
> + u32 reg_mr;
> +
> + napi_disable(&priv->napi);
> +
> + /* disable interrupts */
> + at91_write(dev, AT91_IDR, AT91_IRQ_ALL);
> +
> + reg_mr = at91_read(dev, AT91_MR);
> + at91_write(dev, AT91_MR, reg_mr & ~AT91_MR_AT91EN);
> +
> + at91_transceiver_switch(priv, 0);
> + priv->can.state = state;
> +}
> +
> +
> +/*
> + * theory of operation:
> + *
> + * Accoring to the datasheet priority 0 is the highest priority, 15 is
s/AAccoring/According/ ?
> + * the lowest. If two mailboxes have the same priority level the
> + * message of the mailbox with the lowest number is sent first.
> + *
> + * We use the first TX mailbox mailbox (AT91_MB_TX_FIRST) with prio 0,
s/mailbox mailbox/mailbox/ ?
> + * then the next mailbox with prio 0, and so on, until all mailboxes
> + * are used. Then we start from the beginning with mailbox
> + * AT91_MB_TX_FIRST, but with prio 1, mailbox AT91_MB_TX_FIRST + 1
> + * prio 1. When we reach the last mailbox with prio 15, we have to
> + * stop sending, waiting for all messages to be delivered, than start
s/than/then/
Thanks.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
