-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Wolfgang Grandegger wrote: >> 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.
hmmm...I'll try >> I have to figure out if disabling NAPI is possbile from NAPI conext, so >> CAN state transition might be moved to NAPI, too. for the record: no, we would get a deadlock then..(waiting in NAPI for NAPI to end) >> 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. okay, will change. >> 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. using select/poll would be better than calling sched_yield >> 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. As we see with the above numbers, we lose about 4000 errors per second if ethernet traffic hits the box. I was just thinking about polling this particular error if it hits the chip. >> 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. okay > 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) I took this one > 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? As Wolfram pointed out: > There are a few of those. Such kind of indentation is ususally > rejected upstream, too. If one of the member names ever changes, "=" > will get misaligned again (or you'd need to alter lines just for > beautification which would make git blame less useful). So, one space > after the names should do. >> + >> +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? it does, fixed >> +} >> + >> +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. fixed. patches against which tree? >> + >> +/* >> + * 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. okay, fixed >> + >> + 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/ ? ^^ :), but I got it >> + * 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/ ? fixed >> + * 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/ fixed, tnx cheers, Marc - -- Pengutronix e.K. | Marc Kleine-Budde | Linux Solutions for Science and Industry | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkqquJwACgkQjTAFq1RaXHOf/QCeK2CYAwo/lNa1kwizyM/KwWqp FHAAoJDGvo9Mhco8Ag79gb1i+zKzQbNb =h13P -----END PGP SIGNATURE----- _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
