| Message ID | 1260430072-21106-1-git-send-email-21cnbao@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Barry Song wrote: > Signed-off-by: Barry Song <21cnbao@gmail.com> > Signed-off-by: H.J. Oertel <oe@port.de> > --- > -v3: use structures to describe the layout of registers Wow, that was quick, thanks for your effort and patience. Please use checkpath.pl to detect the obvious coding style problems, especially the "WARNING: line over 80 characters". I also think the declaration of reg should have the __iomem as well: struct bfin_can_regs __iomem *reg = priv->membase; > drivers/net/can/Kconfig | 9 + > drivers/net/can/Makefile | 1 + > drivers/net/can/bfin_can.c | 836 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 846 insertions(+), 0 deletions(-) > create mode 100644 drivers/net/can/bfin_can.c > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index bb803fa..8c485aa 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -54,6 +54,15 @@ config CAN_MCP251X > ---help--- > Driver for the Microchip MCP251x SPI CAN controllers. > > +config CAN_BFIN > + depends on CAN_DEV && (BF534 || BF536 || BF537 || BF538 || BF539 || BF54x) > + tristate "Analog Devices Blackfin on-chip CAN" > + ---help--- > + Driver for the Analog Devices Blackfin on-chip CAN controllers > + > + To compile this driver as a module, choose M here: the > + module will be called bfin_can. > + > source "drivers/net/can/mscan/Kconfig" > > source "drivers/net/can/sja1000/Kconfig" > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > index 56899fe..7a702f2 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -14,5 +14,6 @@ obj-$(CONFIG_CAN_MSCAN) += mscan/ > obj-$(CONFIG_CAN_AT91) += at91_can.o > obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o > obj-$(CONFIG_CAN_MCP251X) += mcp251x.o > +obj-$(CONFIG_CAN_BFIN) += bfin_can.o > > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c > new file mode 100644 > index 0000000..b94169d > --- /dev/null > +++ b/drivers/net/can/bfin_can.c > @@ -0,0 +1,836 @@ > +/* > + * Blackfin On-Chip CAN Driver > + * > + * Copyright 2004-2009 Analog Devices Inc. Consider adding your copyright here, with a name and address. > + * > + * Enter bugs at http://blackfin.uclinux.org/ > + * > + * Licensed under the GPL-2 or later. Please add the usual "GPL-2 or later" bla-bla here. > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/types.h> I think you don't need "types.h" as the code no longer uses "uint*_t". > +#include <linux/bitops.h> > +#include <linux/interrupt.h> > +#include <linux/errno.h> > +#include <linux/netdevice.h> > +#include <linux/skbuff.h> > +#include <linux/platform_device.h> > + > +#include <linux/can.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > + > +#include <asm/portmux.h> > + > +#define DRV_NAME "bfin_can" > +#define BFIN_CAN_TIMEOUT 100 > + > +/* > + * transmit and receive channels > + */ > +#define TRANSMIT_CHL 24 > +#define RECEIVE_STD_CHL 0 > +#define RECEIVE_EXT_CHL 4 > +#define RECEIVE_RTR_CHL 8 > +#define RECEIVE_EXT_RTR_CHL 12 > +#define MAX_CHL_NUMBER 32 The alignment of the numbers is inconsistant. > +/* > + * bfin can registers layout > + */ > +struct bfin_can_mask_regs { > + u16 aml; > + u16 dummy1; > + u16 amh; > + u16 dummy2; > +}; > + > +struct bfin_can_channel_regs { > + u16 data[8]; > + u16 dlc; > + u16 dummy1; > + u16 tsv; > + u16 dummy2; > + u16 id0; > + u16 dummy3; > + u16 id1; > + u16 dummy4; > +}; > + > +struct bfin_can_regs { > + /* > + * global control and status registers > + */ > + u16 mc1; /* offset 0 */ > + u16 dummy1; > + u16 md1; /* offset 4 */ > + u16 rsv1[13]; > + u16 mbtif1; /* offset 0x20 */ > + u16 dummy2; > + u16 mbrif1; /* offset 0x24 */ > + u16 dummy3; > + u16 mbim1; /* offset 0x28 */ > + u16 rsv2[11]; Ditto. > + u16 mc2; /* offset 0x40 */ > + u16 dummy4; > + u16 md2; /* offset 0x44 */ > + u16 dummy5; > + u16 trs2; /* offset 0x48 */ > + u16 rsv3[11]; > + u16 mbtif2; /* offset 0x60 */ > + u16 dummy6; > + u16 mbrif2; /* offset 0x64 */ > + u16 dummy7; > + u16 mbim2; /* offset 0x68 */ > + u16 rsv4[11]; > + u16 clk; /* offset 0x80 */ > + u16 dummy8; > + u16 timing; /* offset 0x84 */ > + u16 rsv5[3]; > + u16 status; /* offset 0x8c */ > + u16 dummy9; > + u16 cec; /* offset 0x90 */ > + u16 dummy10; > + u16 gis; /* offset 0x94 */ > + u16 dummy11; > + u16 gim; /* offset 0x98 */ > + u16 rsv6[3]; > + u16 ctrl; /* offset 0xa0 */ > + u16 dummy12; > + u16 intr; /* offset 0xa4 */ > + u16 rsv7[7]; > + u16 esr; /* offset 0xb4 */ > + u16 rsv8[37]; > + > + /* > + * channel(mailbox) mask and message registers > + */ > + struct bfin_can_mask_regs msk[MAX_CHL_NUMBER]; /* offset 0x100 */ Don't align "msk", please. > + struct bfin_can_channel_regs chl[MAX_CHL_NUMBER]; /* offset 0x200 */ > +}; > + > +/* > + * bfin can private data > + */ > +struct bfin_can_priv { > + struct can_priv can; /* must be the first member */ > + struct net_device *dev; > + void __iomem *membase; > + int rx_irq; > + int tx_irq; > + int err_irq; > + unsigned short *pin_list; > +}; > + > +/* > + * bfin can timing parameters > + */ > +static struct can_bittiming_const bfin_can_bittiming_const = { > + .name = DRV_NAME, > + .tseg1_min = 1, > + .tseg1_max = 16, > + .tseg2_min = 1, > + .tseg2_max = 8, > + .sjw_max = 4, > + /* Although the BRP field can be set to any value, it is recommended Should be: /* * Although the ... > + * that the value be greater than or equal to 4, as restrictions > + * apply to the bit timing configuration when BRP is less than 4. > + */ > + .brp_min = 4, > + .brp_max = 1024, > + .brp_inc = 1, > +}; Well, I'm still not a friend of the following inline functions, especially the *one-liners* which are called just *once*. With the usage of structs they seem even more useless. > +/* > + * inline functions to read/write ID, data length and payload of CAN frame > + */ > +static inline void bfin_can_write_oid(struct bfin_can_priv *priv, int channel, canid_t id) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + writew((id << 2) + AME, ®->chl[channel].id1); > +} > + > +static inline void bfin_can_write_oid_rtr(struct bfin_can_priv *priv, int channel, canid_t id) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + writew((id << 2) + AME + RTR, ®->chl[channel].id1); > +} > + > +static inline canid_t bfin_can_read_oid(struct bfin_can_priv *priv, int channel) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + return (readw(®->chl[channel].id1) & 0x1ffc) >> 2; > +} > + > +static inline void bfin_can_write_xoid(struct bfin_can_priv *priv, int channel, canid_t id) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + writew(id, ®->chl[channel].id0); > + writew(((id & 0x1FFF0000) >> 16) + IDE + AME, ®->chl[channel].id1); > +} > + > +static inline void bfin_can_write_xoid_rtr(struct bfin_can_priv *priv, int channel, canid_t id) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + writew(id, ®->chl[channel].id0); > + writew(((id & 0x1FFF0000) >> 16) + IDE + AME + RTR, ®->chl[channel].id1); > +} > + > +static inline canid_t bfin_can_read_xoid(struct bfin_can_priv *priv, int channel) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + return ((readw(®->chl[channel].id1) & 0x1FFF) << 16) + readw(®->chl[channel].id0); > +} > + > +static inline void bfin_can_write_dlc(struct bfin_can_priv *priv, int channel, u8 dlc) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + writew(dlc, ®->chl[channel].dlc); > +} Look, "writew(dlc, ®->chl[channel].dlc)" makes already pretty clear what the call does. > +static inline u8 bfin_can_read_dlc(struct bfin_can_priv *priv, int channel) > +{ > + struct bfin_can_regs *reg = priv->membase; > + > + return readw(®->chl[channel].dlc); > +} Ditto. > +static inline void bfin_can_write_data(struct bfin_can_priv *priv, int channel, u8 *data, u8 dlc) > +{ > + struct bfin_can_regs *reg = priv->membase; > + int i; > + u16 val; > + > + for (i = 0; i < 8; i += 2) { > + val = ((7 - i) < dlc ? (data[7 - i]) : 0) + > + ((6 - i) < dlc ? (data[6 - i] << 8) : 0); > + writew(val, ®->chl[channel].data[i]); > + } > +} > + > +static inline void bfin_can_read_data(struct bfin_can_priv *priv, int channel, u8 *data, u8 dlc) > +{ > + struct bfin_can_regs *reg = priv->membase; > + int i; > + u16 val; > + > + for (i = 0; i < 8; i += 2) { > + val = readw(®->chl[channel].data[i]); > + data[7 - i] = (7 - i) < dlc ? val : 0; > + data[6 - i] = (6 - i) < dlc ? (val >> 8) : 0; > + } > +} > + > +static int bfin_can_set_bittiming(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + struct can_bittiming *bt = &priv->can.bittiming; > + u16 clk, timing; > + > + clk = bt->brp - 1; > + timing = ((bt->sjw - 1) << 8) | (bt->prop_seg + bt->phase_seg1 - 1) | > + ((bt->phase_seg2 - 1) << 4); > + > + /* > + * If the SAM bit is set, the input signal is oversampled three times at the SCLK rate. > + */ > + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > + timing |= SAM; > + > + writew(clk, ®->clk); > + writew(timing, ®->timing); > + > + dev_info(dev->dev.parent, "setting CLOCK=0x%04x TIMING=0x%04x\n", clk, timing); > + return 0; > +} > + > +static void bfin_can_set_reset_mode(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + int timeout = BFIN_CAN_TIMEOUT; > + int i; > + > + /* disable interrupts */ > + writew(0, ®->mbim1); > + writew(0, ®->mbim2); > + writew(0, ®->gim); > + > + /* reset can and enter configuration mode */ > + writew(SRS | CCR, ®->ctrl); > + SSYNC(); > + writew(CCR, ®->ctrl); > + SSYNC(); > + while (!(readw(®->ctrl) & CCA)) { > + udelay(10); > + if (--timeout == 0) { > + dev_err(dev->dev.parent, "fail to enter configuration mode\n"); > + BUG(); > + } > + } Isn't using BUG() to harsh here? Using and handling a proper return code might already be sufficient. > + > + /* > + * All mailbox configurations are marked as inactive > + * by writing to CAN Mailbox Configuration Registers 1 and 2 > + * For all bits: 0 - Mailbox disabled, 1 - Mailbox enabled > + */ > + writew(0, ®->mc1); > + writew(0, ®->mc2); > + > + /* Set Mailbox Direction */ > + writew(0xFFFF, ®->md1); /* mailbox 1-16 are RX */ > + writew(0, ®->md2); /* mailbox 17-32 are TX */ > + > + /* RECEIVE_STD_CHL */ > + for (i = 0; i < 2; i++) { > + writew(0, ®->chl[RECEIVE_STD_CHL + i].id0); > + writew(AME, ®->chl[RECEIVE_STD_CHL + i].id1); > + writew(0, ®->chl[RECEIVE_STD_CHL + i].dlc); > + writew(0x1FFF, ®->msk[RECEIVE_STD_CHL + i].amh); > + writew(0xFFFF, ®->msk[RECEIVE_STD_CHL + i].aml); > + } > + > + /* RECEIVE_EXT_CHL */ > + for (i = 0; i < 2; i++) { > + writew(0, ®->chl[RECEIVE_EXT_CHL + i].id0); > + writew(AME | IDE, ®->chl[RECEIVE_EXT_CHL + i].id1); > + writew(0, ®->chl[RECEIVE_EXT_CHL + i].dlc); > + writew(0x1FFF, ®->msk[RECEIVE_EXT_CHL + i].amh); > + writew(0xFFFF, ®->msk[RECEIVE_EXT_CHL + i].aml); > + } > + > + writew(BIT(TRANSMIT_CHL - 16), ®->mc2); > + writew(BIT(RECEIVE_STD_CHL) + BIT(RECEIVE_EXT_CHL), ®->mc1); > + SSYNC(); > + > + priv->can.state = CAN_STATE_STOPPED; > +} > + > +static void bfin_can_set_normal_mode(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + int timeout = BFIN_CAN_TIMEOUT; > + > + /* > + * leave configuration mode > + */ > + writew(readw(®->ctrl) & ~CCR, ®->ctrl); > + > + while (readw(®->status) & CCA) { > + udelay(10); > + if (--timeout == 0) { > + dev_err(dev->dev.parent, "fail to leave configuration mode\n"); > + BUG(); > + } > + } > + > + /* > + * clear _All_ tx and rx interrupts > + */ > + writew(0xFFFF, ®->mbtif1); > + writew(0xFFFF, ®->mbtif2); > + writew(0xFFFF, ®->mbrif1); > + writew(0xFFFF, ®->mbrif2); > + > + /* > + * clear global interrupt status register > + */ > + writew(0x7FF, ®->gis); /* overwrites with '1' */ > + > + /* > + * Initialize Interrupts > + * - set bits in the mailbox interrupt mask register > + * - global interrupt mask > + */ > + writew(BIT(RECEIVE_STD_CHL) + BIT(RECEIVE_EXT_CHL), ®->mbim1); > + writew(BIT(TRANSMIT_CHL - 16), ®->mbim2); > + > + writew(EPIM | BOIM | RMLIM, ®->gim); > + SSYNC(); > +} > + > +static void bfin_can_start(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + > + /* leave reset mode */ > + if (priv->can.state != CAN_STATE_STOPPED) > + bfin_can_set_reset_mode(dev); > + > + /* leave reset mode */ > + bfin_can_set_normal_mode(dev); > +} > + > +static int bfin_can_set_mode(struct net_device *dev, enum can_mode mode) > +{ > + switch (mode) { > + case CAN_MODE_START: > + bfin_can_start(dev); > + if (netif_queue_stopped(dev)) > + netif_wake_queue(dev); > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int bfin_can_start_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + struct can_frame *cf = (struct can_frame *)skb->data; > + u8 dlc; > + canid_t id; > + > + netif_stop_queue(dev); > + > + dlc = cf->can_dlc; > + id = cf->can_id; > + > + /* fill id and data length code */ > + if (id & CAN_EFF_FLAG) { > + if (id & CAN_RTR_FLAG) > + bfin_can_write_xoid_rtr(priv, TRANSMIT_CHL, id); > + else > + bfin_can_write_xoid(priv, TRANSMIT_CHL, id); > + } else { > + if (id & CAN_RTR_FLAG) > + bfin_can_write_oid_rtr(priv, TRANSMIT_CHL, id); > + else > + bfin_can_write_oid(priv, TRANSMIT_CHL, id); > + } > + > + bfin_can_write_data(priv, TRANSMIT_CHL, cf->data, dlc); > + > + bfin_can_write_dlc(priv, TRANSMIT_CHL, dlc); > + > + dev->trans_start = jiffies; > + > + can_put_echo_skb(skb, dev, 0); > + > + /* set transmit request */ > + writew(BIT(TRANSMIT_CHL - 16), ®->trs2); > + return 0; > +} > + > +static void bfin_can_rx(struct net_device *dev, u16 isrc) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct net_device_stats *stats = &dev->stats; > + struct bfin_can_regs *reg = priv->membase; > + struct can_frame *cf; > + struct sk_buff *skb; > + canid_t id; > + u8 dlc; > + int obj; > + > + skb = alloc_can_skb(dev, &cf); > + if (skb == NULL) > + return; > + > + /* get id and data length code */ > + if (isrc & BIT(RECEIVE_EXT_CHL)) { > + /* extended frame format (EFF) */ > + id = bfin_can_read_xoid(priv, RECEIVE_EXT_CHL); > + id |= CAN_EFF_FLAG; > + obj = RECEIVE_EXT_CHL; > + } else { > + /* standard frame format (SFF) */ > + id = bfin_can_read_oid(priv, RECEIVE_STD_CHL); > + obj = RECEIVE_STD_CHL; > + } > + if (readw(®->chl[obj].id1) & RTR) > + id |= CAN_RTR_FLAG; > + dlc = bfin_can_read_dlc(priv, obj); > + > + cf->can_id = id; > + cf->can_dlc = dlc; > + > + bfin_can_read_data(priv, obj, cf->data, dlc); > + > + netif_rx(skb); > + > + stats->rx_packets++; > + stats->rx_bytes += dlc; > +} > + > +static int bfin_can_err(struct net_device *dev, u16 isrc, u16 status) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + struct net_device_stats *stats = &dev->stats; > + struct can_frame *cf; > + struct sk_buff *skb; > + enum can_state state = priv->can.state; > + > + skb = alloc_can_err_skb(dev, &cf); > + if (skb == NULL) > + return -ENOMEM; > + > + if (isrc & RMLIS) { > + /* data overrun interrupt */ > + dev_dbg(dev->dev.parent, "data overrun interrupt\n"); > + cf->can_id |= CAN_ERR_CRTL; > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > + stats->rx_over_errors++; > + stats->rx_errors++; > + } > + > + if (isrc & BOIS) { > + dev_dbg(dev->dev.parent, "bus-off mode interrupt\n"); > + Empty line? > + state = CAN_STATE_BUS_OFF; > + cf->can_id |= CAN_ERR_BUSOFF; > + can_bus_off(dev); > + } > + > + if (isrc & EPIS) { > + /* error passive interrupt */ > + dev_dbg(dev->dev.parent, "error passive interrupt\n"); > + state = CAN_STATE_ERROR_PASSIVE; > + } > + > + if ((isrc & EWTIS) || (isrc & EWRIS)) { > + dev_dbg(dev->dev.parent, "Error Warning Transmit/Receive Interrupt\n"); > + state = CAN_STATE_ERROR_WARNING; > + } > + > + if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING || > + state == CAN_STATE_ERROR_PASSIVE)) { > + u16 cec = readw(®->cec); > + u8 rxerr = cec; > + u8 txerr = cec >> 8; Add an empty line here, please. > + cf->can_id |= CAN_ERR_CRTL; > + if (state == CAN_STATE_ERROR_WARNING) { > + priv->can.can_stats.error_warning++; > + cf->data[1] = (txerr > rxerr) ? > + CAN_ERR_CRTL_TX_WARNING : > + CAN_ERR_CRTL_RX_WARNING; > + } else { > + priv->can.can_stats.error_passive++; > + cf->data[1] = (txerr > rxerr) ? > + CAN_ERR_CRTL_TX_PASSIVE : > + CAN_ERR_CRTL_RX_PASSIVE; > + } > + } > + > + if (status) { > + priv->can.can_stats.bus_error++; > + > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > + > + if (status & BEF) > + cf->data[2] |= CAN_ERR_PROT_BIT; > + else if (status & FER) > + cf->data[2] |= CAN_ERR_PROT_FORM; > + else if (status & SER) > + cf->data[2] |= CAN_ERR_PROT_STUFF; > + else > + cf->data[2] |= CAN_ERR_PROT_UNSPEC; > + } Add {} here as well. > + priv->can.state = state; > + > + netif_rx(skb); > + > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + > + return 0; > +} > + > +irqreturn_t bfin_can_interrupt(int irq, void *dev_id) > +{ > + struct net_device *dev = dev_id; > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + struct net_device_stats *stats = &dev->stats; > + u16 status, isrc; > + > + if ((irq == priv->tx_irq) && readw(®->mbtif2)) { > + /* transmission complete interrupt */ > + writew(0xFFFF, ®->mbtif2); > + stats->tx_packets++; > + stats->tx_bytes += bfin_can_read_dlc(priv, TRANSMIT_CHL); > + can_get_echo_skb(dev, 0); > + netif_wake_queue(dev); > + } else if ((irq == priv->rx_irq) && readw(®->mbrif1)) { > + /* receive interrupt */ > + isrc = readw(®->mbrif1); > + writew(0xFFFF, ®->mbrif1); > + bfin_can_rx(dev, isrc); > + } else if ((irq == priv->err_irq) && readw(®->gis)) { > + /* error interrupt */ > + isrc = readw(®->gis); > + status = readw(®->esr); > + writew(0x7FF, ®->gis); > + bfin_can_err(dev, isrc, status); > + } else > + return IRQ_NONE; Use {} here as well. > + > + return IRQ_HANDLED; > +} > + > +static int bfin_can_open(struct net_device *dev) > +{ > + int err; > + > + /* set chip into reset mode */ > + bfin_can_set_reset_mode(dev); > + > + /* common open */ > + err = open_candev(dev); > + if (err) > + return err; > + > + /* init and start chi */ > + bfin_can_start(dev); > + > + netif_start_queue(dev); > + > + return 0; > +} > + > +static int bfin_can_close(struct net_device *dev) > +{ > + netif_stop_queue(dev); > + bfin_can_set_reset_mode(dev); > + > + close_candev(dev); > + > + return 0; > +} > + > +struct net_device *alloc_bfin_candev(void) > +{ > + struct net_device *dev; > + struct bfin_can_priv *priv; > + > + dev = alloc_candev(sizeof(*priv)); > + if (!dev) > + return NULL; > + > + priv = netdev_priv(dev); > + > + priv->dev = dev; > + priv->can.bittiming_const = &bfin_can_bittiming_const; > + priv->can.do_set_bittiming = bfin_can_set_bittiming; > + priv->can.do_set_mode = bfin_can_set_mode; > + > + return dev; > +} > + > +static const struct net_device_ops bfin_can_netdev_ops = { > + .ndo_open = bfin_can_open, > + .ndo_stop = bfin_can_close, > + .ndo_start_xmit = bfin_can_start_xmit, > +}; > + > +static int __devinit bfin_can_probe(struct platform_device *pdev) > +{ > + int err; > + struct net_device *dev; > + struct bfin_can_priv *priv; > + struct resource *res_mem, *rx_irq, *tx_irq, *err_irq; > + unsigned short *pdata; > + > + pdata = pdev->dev.platform_data; > + if (!pdata) { > + dev_err(&pdev->dev, "No platform data provided!\n"); > + err = -EINVAL; > + goto exit; > + } > + > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + rx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + tx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 1); > + err_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 2); > + if (!res_mem || !rx_irq || !tx_irq || !err_irq) { > + err = -EINVAL; > + goto exit; > + } > + > + if (!request_mem_region(res_mem->start, resource_size(res_mem), > + dev_name(&pdev->dev))) { > + err = -EBUSY; > + goto exit; > + } > + > + /* request peripheral pins */ > + err = peripheral_request_list(pdata, dev_name(&pdev->dev)); > + if (err) > + goto exit_mem_release; > + > + dev = alloc_bfin_candev(); > + if (!dev) { > + err = -ENOMEM; > + goto exit_peri_pin_free; > + } > + > + /* register interrupt handler */ > + err = request_irq(rx_irq->start, &bfin_can_interrupt, 0, > + "bfin-can-rx", (void *)dev); > + if (err) > + goto exit_candev_free; > + err = request_irq(tx_irq->start, &bfin_can_interrupt, 0, > + "bfin-can-tx", (void *)dev); > + if (err) > + goto exit_rx_irq_free; > + err = request_irq(err_irq->start, &bfin_can_interrupt, 0, > + "bfin-can-err", (void *)dev); > + if (err) > + goto exit_tx_irq_free; If possible, please request/free IRQs in the open and stop functions. > + priv = netdev_priv(dev); > + priv->membase = (void __iomem *)res_mem->start; > + priv->rx_irq = rx_irq->start; > + priv->tx_irq = tx_irq->start; > + priv->err_irq = err_irq->start; > + priv->pin_list = pdata; > + priv->can.clock.freq = get_sclk(); > + > + dev_set_drvdata(&pdev->dev, dev); > + SET_NETDEV_DEV(dev, &pdev->dev); > + > + dev->flags |= IFF_ECHO; /* we support local echo */ > + dev->netdev_ops = &bfin_can_netdev_ops; > + > + bfin_can_set_reset_mode(dev); > + > + err = register_candev(dev); > + if (err) { > + dev_err(&pdev->dev, "registering failed (err=%d)\n", err); > + goto exit_err_irq_free; > + } > + > + dev_info(&pdev->dev, "%s device registered (reg_base=%p, rx_irq=%d, tx_irq=%d, err_irq=%d, sclk=%d)\n", > + DRV_NAME, (void *)priv->membase, priv->rx_irq, priv->tx_irq, priv->err_irq, > + priv->can.clock.freq); > + return 0; > + > +exit_err_irq_free: > + free_irq(err_irq->start, dev); > +exit_tx_irq_free: > + free_irq(tx_irq->start, dev); > +exit_rx_irq_free: > + free_irq(rx_irq->start, dev); > +exit_candev_free: > + free_candev(dev); > +exit_peri_pin_free: > + peripheral_free_list(pdata); > +exit_mem_release: > + release_mem_region(res_mem->start, resource_size(res_mem)); > +exit: > + return err; > +} > + > +static int __devexit bfin_can_remove(struct platform_device *pdev) > +{ > + struct net_device *dev = dev_get_drvdata(&pdev->dev); > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct resource *res; > + > + bfin_can_set_reset_mode(dev); > + > + unregister_candev(dev); > + > + dev_set_drvdata(&pdev->dev, NULL); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + release_mem_region(res->start, resource_size(res)); > + > + free_irq(priv->rx_irq, dev); > + free_irq(priv->tx_irq, dev); > + free_irq(priv->err_irq, dev); > + peripheral_free_list(priv->pin_list); > + > + free_candev(dev); > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int bfin_can_suspend(struct platform_device *pdev, pm_message_t mesg) > +{ > + struct net_device *dev = dev_get_drvdata(&pdev->dev); > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + int timeout = BFIN_CAN_TIMEOUT; > + > + if (netif_running(dev)) { > + /* enter sleep mode */ > + writew(readw(®->ctrl) | SMR, ®->ctrl); > + SSYNC(); > + while (!(readw(®->intr) & SMACK)) { > + udelay(10); > + if (--timeout == 0) { > + dev_err(dev->dev.parent, "fail to enter sleep mode\n"); > + BUG(); > + } > + } It's already the third time that this timeout check is used. A common function would make sense. > + } > + > + return 0; > +} > + > +static int bfin_can_resume(struct platform_device *pdev) > +{ > + struct net_device *dev = dev_get_drvdata(&pdev->dev); > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct bfin_can_regs *reg = priv->membase; > + > + if (netif_running(dev)) { > + /* leave sleep mode */ > + writew(0, ®->intr); > + SSYNC(); > + } > + > + return 0; > +} > +#else > +#define bfin_can_suspend NULL > +#define bfin_can_resume NULL > +#endif /* CONFIG_PM */ > + > +static struct platform_driver bfin_can_driver = { > + .probe = bfin_can_probe, > + .remove = __devexit_p(bfin_can_remove), > + .suspend = bfin_can_suspend, > + .resume = bfin_can_resume, > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, > + }, > +}; > + > +static int __init bfin_can_init(void) > +{ > + return platform_driver_register(&bfin_can_driver); > +} > + Remove empty line above, please. > +module_init(bfin_can_init); > + > +static void __exit bfin_can_exit(void) > +{ > + platform_driver_unregister(&bfin_can_driver); > +} > + Ditto. > +module_exit(bfin_can_exit); > + > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>"); > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Blackfin on-chip CAN netdevice driver"); Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: > Barry Song wrote: >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/types.h> > > I think you don't need "types.h" as the code no longer uses "uint*_t". linux/types.h declares all types, like u* which this driver still uses > Well, I'm still not a friend of the following inline functions, > especially the *one-liners* which are called just *once*. With the usage > of structs they seem even more useless. seems like it would make more sense to not even use the read/write functions either. just declare the regs as volatile and assign/read the struct directly. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 6:04 PM, Mike Frysinger <vapier.adi@gmail.com> wrote: > On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >> Barry Song wrote: >>> +#include <linux/module.h> >>> +#include <linux/init.h> >>> +#include <linux/kernel.h> >>> +#include <linux/types.h> >> >> I think you don't need "types.h" as the code no longer uses "uint*_t". > > linux/types.h declares all types, like u* which this driver still uses > >> Well, I'm still not a friend of the following inline functions, >> especially the *one-liners* which are called just *once*. With the usage >> of structs they seem even more useless. > > seems like it would make more sense to not even use the read/write > functions either. just declare the regs as volatile and assign/read > the struct directly. volatile will cause check patch warning in fact. > -mike > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 05:05, Barry Song wrote: > On Thu, Dec 10, 2009 at 6:04 PM, Mike Frysinger wrote: >> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>> Barry Song wrote: >>>> +#include <linux/module.h> >>>> +#include <linux/init.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/types.h> >>> >>> I think you don't need "types.h" as the code no longer uses "uint*_t". >> >> linux/types.h declares all types, like u* which this driver still uses >> >>> Well, I'm still not a friend of the following inline functions, >>> especially the *one-liners* which are called just *once*. With the usage >>> of structs they seem even more useless. >> >> seems like it would make more sense to not even use the read/write >> functions either. just declare the regs as volatile and assign/read >> the struct directly. > > volatile will cause check patch warning in fact. checkpatch output is not something that needs to be 100% clean. you review the output and determine whether it makes sense to fix or ignore. in this case, you ignore. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 5:11 PM, Wolfgang Grandegger <wg@grandegger.com> wrote: > Barry Song wrote: >> Signed-off-by: Barry Song <21cnbao@gmail.com> >> Signed-off-by: H.J. Oertel <oe@port.de> >> --- >> -v3: use structures to describe the layout of registers > > Wow, that was quick, thanks for your effort and patience. > > Please use checkpath.pl to detect the obvious coding style problems, > especially the "WARNING: line over 80 characters". > > I also think the declaration of reg should have the __iomem as well: > > struct bfin_can_regs __iomem *reg = priv->membase; > >> drivers/net/can/Kconfig | 9 + >> drivers/net/can/Makefile | 1 + >> drivers/net/can/bfin_can.c | 836 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 846 insertions(+), 0 deletions(-) >> create mode 100644 drivers/net/can/bfin_can.c >> >> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig >> index bb803fa..8c485aa 100644 >> --- a/drivers/net/can/Kconfig >> +++ b/drivers/net/can/Kconfig >> @@ -54,6 +54,15 @@ config CAN_MCP251X >> ---help--- >> Driver for the Microchip MCP251x SPI CAN controllers. >> >> +config CAN_BFIN >> + depends on CAN_DEV && (BF534 || BF536 || BF537 || BF538 || BF539 || BF54x) >> + tristate "Analog Devices Blackfin on-chip CAN" >> + ---help--- >> + Driver for the Analog Devices Blackfin on-chip CAN controllers >> + >> + To compile this driver as a module, choose M here: the >> + module will be called bfin_can. >> + >> source "drivers/net/can/mscan/Kconfig" >> >> source "drivers/net/can/sja1000/Kconfig" >> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile >> index 56899fe..7a702f2 100644 >> --- a/drivers/net/can/Makefile >> +++ b/drivers/net/can/Makefile >> @@ -14,5 +14,6 @@ obj-$(CONFIG_CAN_MSCAN) += mscan/ >> obj-$(CONFIG_CAN_AT91) += at91_can.o >> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o >> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o >> +obj-$(CONFIG_CAN_BFIN) += bfin_can.o >> >> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG >> diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c >> new file mode 100644 >> index 0000000..b94169d >> --- /dev/null >> +++ b/drivers/net/can/bfin_can.c >> @@ -0,0 +1,836 @@ >> +/* >> + * Blackfin On-Chip CAN Driver >> + * >> + * Copyright 2004-2009 Analog Devices Inc. > > Consider adding your copyright here, with a name and address. > >> + * >> + * Enter bugs at http://blackfin.uclinux.org/ >> + * >> + * Licensed under the GPL-2 or later. > > Please add the usual "GPL-2 or later" bla-bla here. Here I am not completely sure. But I am sure I am using the head file template of ADI which has been used widely in kernel and should be right. > >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/types.h> > > I think you don't need "types.h" as the code no longer uses "uint*_t". > >> +#include <linux/bitops.h> >> +#include <linux/interrupt.h> >> +#include <linux/errno.h> >> +#include <linux/netdevice.h> >> +#include <linux/skbuff.h> >> +#include <linux/platform_device.h> >> + >> +#include <linux/can.h> >> +#include <linux/can/dev.h> >> +#include <linux/can/error.h> >> + >> +#include <asm/portmux.h> >> + >> +#define DRV_NAME "bfin_can" >> +#define BFIN_CAN_TIMEOUT 100 >> + >> +/* >> + * transmit and receive channels >> + */ >> +#define TRANSMIT_CHL 24 >> +#define RECEIVE_STD_CHL 0 >> +#define RECEIVE_EXT_CHL 4 >> +#define RECEIVE_RTR_CHL 8 >> +#define RECEIVE_EXT_RTR_CHL 12 >> +#define MAX_CHL_NUMBER 32 > > The alignment of the numbers is inconsistant. > >> +/* >> + * bfin can registers layout >> + */ >> +struct bfin_can_mask_regs { >> + u16 aml; >> + u16 dummy1; >> + u16 amh; >> + u16 dummy2; >> +}; >> + >> +struct bfin_can_channel_regs { >> + u16 data[8]; >> + u16 dlc; >> + u16 dummy1; >> + u16 tsv; >> + u16 dummy2; >> + u16 id0; >> + u16 dummy3; >> + u16 id1; >> + u16 dummy4; >> +}; >> + >> +struct bfin_can_regs { >> + /* >> + * global control and status registers >> + */ >> + u16 mc1; /* offset 0 */ >> + u16 dummy1; >> + u16 md1; /* offset 4 */ >> + u16 rsv1[13]; >> + u16 mbtif1; /* offset 0x20 */ >> + u16 dummy2; >> + u16 mbrif1; /* offset 0x24 */ >> + u16 dummy3; >> + u16 mbim1; /* offset 0x28 */ >> + u16 rsv2[11]; > > Ditto. > >> + u16 mc2; /* offset 0x40 */ >> + u16 dummy4; >> + u16 md2; /* offset 0x44 */ >> + u16 dummy5; >> + u16 trs2; /* offset 0x48 */ >> + u16 rsv3[11]; >> + u16 mbtif2; /* offset 0x60 */ >> + u16 dummy6; >> + u16 mbrif2; /* offset 0x64 */ >> + u16 dummy7; >> + u16 mbim2; /* offset 0x68 */ >> + u16 rsv4[11]; >> + u16 clk; /* offset 0x80 */ >> + u16 dummy8; >> + u16 timing; /* offset 0x84 */ >> + u16 rsv5[3]; >> + u16 status; /* offset 0x8c */ >> + u16 dummy9; >> + u16 cec; /* offset 0x90 */ >> + u16 dummy10; >> + u16 gis; /* offset 0x94 */ >> + u16 dummy11; >> + u16 gim; /* offset 0x98 */ >> + u16 rsv6[3]; >> + u16 ctrl; /* offset 0xa0 */ >> + u16 dummy12; >> + u16 intr; /* offset 0xa4 */ >> + u16 rsv7[7]; >> + u16 esr; /* offset 0xb4 */ >> + u16 rsv8[37]; >> + >> + /* >> + * channel(mailbox) mask and message registers >> + */ >> + struct bfin_can_mask_regs msk[MAX_CHL_NUMBER]; /* offset 0x100 */ > > Don't align "msk", please. > >> + struct bfin_can_channel_regs chl[MAX_CHL_NUMBER]; /* offset 0x200 */ >> +}; >> + >> +/* >> + * bfin can private data >> + */ >> +struct bfin_can_priv { >> + struct can_priv can; /* must be the first member */ >> + struct net_device *dev; >> + void __iomem *membase; >> + int rx_irq; >> + int tx_irq; >> + int err_irq; >> + unsigned short *pin_list; >> +}; >> + >> +/* >> + * bfin can timing parameters >> + */ >> +static struct can_bittiming_const bfin_can_bittiming_const = { >> + .name = DRV_NAME, >> + .tseg1_min = 1, >> + .tseg1_max = 16, >> + .tseg2_min = 1, >> + .tseg2_max = 8, >> + .sjw_max = 4, >> + /* Although the BRP field can be set to any value, it is recommended > > Should be: > > /* > * Although the ... > > >> + * that the value be greater than or equal to 4, as restrictions >> + * apply to the bit timing configuration when BRP is less than 4. >> + */ >> + .brp_min = 4, >> + .brp_max = 1024, >> + .brp_inc = 1, >> +}; > > Well, I'm still not a friend of the following inline functions, > especially the *one-liners* which are called just *once*. With the usage > of structs they seem even more useless. > >> +/* >> + * inline functions to read/write ID, data length and payload of CAN frame >> + */ >> +static inline void bfin_can_write_oid(struct bfin_can_priv *priv, int channel, canid_t id) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + writew((id << 2) + AME, ®->chl[channel].id1); >> +} >> + >> +static inline void bfin_can_write_oid_rtr(struct bfin_can_priv *priv, int channel, canid_t id) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + writew((id << 2) + AME + RTR, ®->chl[channel].id1); >> +} >> + >> +static inline canid_t bfin_can_read_oid(struct bfin_can_priv *priv, int channel) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + return (readw(®->chl[channel].id1) & 0x1ffc) >> 2; >> +} >> + >> +static inline void bfin_can_write_xoid(struct bfin_can_priv *priv, int channel, canid_t id) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + writew(id, ®->chl[channel].id0); >> + writew(((id & 0x1FFF0000) >> 16) + IDE + AME, ®->chl[channel].id1); >> +} >> + >> +static inline void bfin_can_write_xoid_rtr(struct bfin_can_priv *priv, int channel, canid_t id) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + writew(id, ®->chl[channel].id0); >> + writew(((id & 0x1FFF0000) >> 16) + IDE + AME + RTR, ®->chl[channel].id1); >> +} >> + >> +static inline canid_t bfin_can_read_xoid(struct bfin_can_priv *priv, int channel) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + return ((readw(®->chl[channel].id1) & 0x1FFF) << 16) + readw(®->chl[channel].id0); >> +} >> + >> +static inline void bfin_can_write_dlc(struct bfin_can_priv *priv, int channel, u8 dlc) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + writew(dlc, ®->chl[channel].dlc); >> +} > > Look, "writew(dlc, ®->chl[channel].dlc)" makes already pretty clear > what the call does. > >> +static inline u8 bfin_can_read_dlc(struct bfin_can_priv *priv, int channel) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + >> + return readw(®->chl[channel].dlc); >> +} > > Ditto. > >> +static inline void bfin_can_write_data(struct bfin_can_priv *priv, int channel, u8 *data, u8 dlc) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + int i; >> + u16 val; >> + >> + for (i = 0; i < 8; i += 2) { >> + val = ((7 - i) < dlc ? (data[7 - i]) : 0) + >> + ((6 - i) < dlc ? (data[6 - i] << 8) : 0); >> + writew(val, ®->chl[channel].data[i]); >> + } >> +} >> + >> +static inline void bfin_can_read_data(struct bfin_can_priv *priv, int channel, u8 *data, u8 dlc) >> +{ >> + struct bfin_can_regs *reg = priv->membase; >> + int i; >> + u16 val; >> + >> + for (i = 0; i < 8; i += 2) { >> + val = readw(®->chl[channel].data[i]); >> + data[7 - i] = (7 - i) < dlc ? val : 0; >> + data[6 - i] = (6 - i) < dlc ? (val >> 8) : 0; >> + } >> +} >> + >> +static int bfin_can_set_bittiming(struct net_device *dev) >> +{ >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + struct can_bittiming *bt = &priv->can.bittiming; >> + u16 clk, timing; >> + >> + clk = bt->brp - 1; >> + timing = ((bt->sjw - 1) << 8) | (bt->prop_seg + bt->phase_seg1 - 1) | >> + ((bt->phase_seg2 - 1) << 4); >> + >> + /* >> + * If the SAM bit is set, the input signal is oversampled three times at the SCLK rate. >> + */ >> + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) >> + timing |= SAM; >> + >> + writew(clk, ®->clk); >> + writew(timing, ®->timing); >> + >> + dev_info(dev->dev.parent, "setting CLOCK=0x%04x TIMING=0x%04x\n", clk, timing); >> + return 0; >> +} >> + >> +static void bfin_can_set_reset_mode(struct net_device *dev) >> +{ >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + int timeout = BFIN_CAN_TIMEOUT; >> + int i; >> + >> + /* disable interrupts */ >> + writew(0, ®->mbim1); >> + writew(0, ®->mbim2); >> + writew(0, ®->gim); >> + >> + /* reset can and enter configuration mode */ >> + writew(SRS | CCR, ®->ctrl); >> + SSYNC(); >> + writew(CCR, ®->ctrl); >> + SSYNC(); >> + while (!(readw(®->ctrl) & CCA)) { >> + udelay(10); >> + if (--timeout == 0) { >> + dev_err(dev->dev.parent, "fail to enter configuration mode\n"); >> + BUG(); >> + } >> + } > > Isn't using BUG() to harsh here? Using and handling a proper return code > might already be sufficient. Due to the hardware design, here timeout will never happen. If it happens, that means hardware component has crashed. > >> + >> + /* >> + * All mailbox configurations are marked as inactive >> + * by writing to CAN Mailbox Configuration Registers 1 and 2 >> + * For all bits: 0 - Mailbox disabled, 1 - Mailbox enabled >> + */ >> + writew(0, ®->mc1); >> + writew(0, ®->mc2); >> + >> + /* Set Mailbox Direction */ >> + writew(0xFFFF, ®->md1); /* mailbox 1-16 are RX */ >> + writew(0, ®->md2); /* mailbox 17-32 are TX */ >> + >> + /* RECEIVE_STD_CHL */ >> + for (i = 0; i < 2; i++) { >> + writew(0, ®->chl[RECEIVE_STD_CHL + i].id0); >> + writew(AME, ®->chl[RECEIVE_STD_CHL + i].id1); >> + writew(0, ®->chl[RECEIVE_STD_CHL + i].dlc); >> + writew(0x1FFF, ®->msk[RECEIVE_STD_CHL + i].amh); >> + writew(0xFFFF, ®->msk[RECEIVE_STD_CHL + i].aml); >> + } >> + >> + /* RECEIVE_EXT_CHL */ >> + for (i = 0; i < 2; i++) { >> + writew(0, ®->chl[RECEIVE_EXT_CHL + i].id0); >> + writew(AME | IDE, ®->chl[RECEIVE_EXT_CHL + i].id1); >> + writew(0, ®->chl[RECEIVE_EXT_CHL + i].dlc); >> + writew(0x1FFF, ®->msk[RECEIVE_EXT_CHL + i].amh); >> + writew(0xFFFF, ®->msk[RECEIVE_EXT_CHL + i].aml); >> + } >> + >> + writew(BIT(TRANSMIT_CHL - 16), ®->mc2); >> + writew(BIT(RECEIVE_STD_CHL) + BIT(RECEIVE_EXT_CHL), ®->mc1); >> + SSYNC(); >> + >> + priv->can.state = CAN_STATE_STOPPED; >> +} >> + >> +static void bfin_can_set_normal_mode(struct net_device *dev) >> +{ >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + int timeout = BFIN_CAN_TIMEOUT; >> + >> + /* >> + * leave configuration mode >> + */ >> + writew(readw(®->ctrl) & ~CCR, ®->ctrl); >> + >> + while (readw(®->status) & CCA) { >> + udelay(10); >> + if (--timeout == 0) { >> + dev_err(dev->dev.parent, "fail to leave configuration mode\n"); >> + BUG(); >> + } >> + } >> + >> + /* >> + * clear _All_ tx and rx interrupts >> + */ >> + writew(0xFFFF, ®->mbtif1); >> + writew(0xFFFF, ®->mbtif2); >> + writew(0xFFFF, ®->mbrif1); >> + writew(0xFFFF, ®->mbrif2); >> + >> + /* >> + * clear global interrupt status register >> + */ >> + writew(0x7FF, ®->gis); /* overwrites with '1' */ >> + >> + /* >> + * Initialize Interrupts >> + * - set bits in the mailbox interrupt mask register >> + * - global interrupt mask >> + */ >> + writew(BIT(RECEIVE_STD_CHL) + BIT(RECEIVE_EXT_CHL), ®->mbim1); >> + writew(BIT(TRANSMIT_CHL - 16), ®->mbim2); >> + >> + writew(EPIM | BOIM | RMLIM, ®->gim); >> + SSYNC(); >> +} >> + >> +static void bfin_can_start(struct net_device *dev) >> +{ >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + >> + /* leave reset mode */ >> + if (priv->can.state != CAN_STATE_STOPPED) >> + bfin_can_set_reset_mode(dev); >> + >> + /* leave reset mode */ >> + bfin_can_set_normal_mode(dev); >> +} >> + >> +static int bfin_can_set_mode(struct net_device *dev, enum can_mode mode) >> +{ >> + switch (mode) { >> + case CAN_MODE_START: >> + bfin_can_start(dev); >> + if (netif_queue_stopped(dev)) >> + netif_wake_queue(dev); >> + break; >> + >> + default: >> + return -EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> + >> +static int bfin_can_start_xmit(struct sk_buff *skb, struct net_device *dev) >> +{ >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + struct can_frame *cf = (struct can_frame *)skb->data; >> + u8 dlc; >> + canid_t id; >> + >> + netif_stop_queue(dev); >> + >> + dlc = cf->can_dlc; >> + id = cf->can_id; >> + >> + /* fill id and data length code */ >> + if (id & CAN_EFF_FLAG) { >> + if (id & CAN_RTR_FLAG) >> + bfin_can_write_xoid_rtr(priv, TRANSMIT_CHL, id); >> + else >> + bfin_can_write_xoid(priv, TRANSMIT_CHL, id); >> + } else { >> + if (id & CAN_RTR_FLAG) >> + bfin_can_write_oid_rtr(priv, TRANSMIT_CHL, id); >> + else >> + bfin_can_write_oid(priv, TRANSMIT_CHL, id); >> + } >> + >> + bfin_can_write_data(priv, TRANSMIT_CHL, cf->data, dlc); >> + >> + bfin_can_write_dlc(priv, TRANSMIT_CHL, dlc); >> + >> + dev->trans_start = jiffies; >> + >> + can_put_echo_skb(skb, dev, 0); >> + >> + /* set transmit request */ >> + writew(BIT(TRANSMIT_CHL - 16), ®->trs2); >> + return 0; >> +} >> + >> +static void bfin_can_rx(struct net_device *dev, u16 isrc) >> +{ >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct net_device_stats *stats = &dev->stats; >> + struct bfin_can_regs *reg = priv->membase; >> + struct can_frame *cf; >> + struct sk_buff *skb; >> + canid_t id; >> + u8 dlc; >> + int obj; >> + >> + skb = alloc_can_skb(dev, &cf); >> + if (skb == NULL) >> + return; >> + >> + /* get id and data length code */ >> + if (isrc & BIT(RECEIVE_EXT_CHL)) { >> + /* extended frame format (EFF) */ >> + id = bfin_can_read_xoid(priv, RECEIVE_EXT_CHL); >> + id |= CAN_EFF_FLAG; >> + obj = RECEIVE_EXT_CHL; >> + } else { >> + /* standard frame format (SFF) */ >> + id = bfin_can_read_oid(priv, RECEIVE_STD_CHL); >> + obj = RECEIVE_STD_CHL; >> + } >> + if (readw(®->chl[obj].id1) & RTR) >> + id |= CAN_RTR_FLAG; >> + dlc = bfin_can_read_dlc(priv, obj); >> + >> + cf->can_id = id; >> + cf->can_dlc = dlc; >> + >> + bfin_can_read_data(priv, obj, cf->data, dlc); >> + >> + netif_rx(skb); >> + >> + stats->rx_packets++; >> + stats->rx_bytes += dlc; >> +} >> + >> +static int bfin_can_err(struct net_device *dev, u16 isrc, u16 status) >> +{ >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + struct net_device_stats *stats = &dev->stats; >> + struct can_frame *cf; >> + struct sk_buff *skb; >> + enum can_state state = priv->can.state; >> + >> + skb = alloc_can_err_skb(dev, &cf); >> + if (skb == NULL) >> + return -ENOMEM; >> + >> + if (isrc & RMLIS) { >> + /* data overrun interrupt */ >> + dev_dbg(dev->dev.parent, "data overrun interrupt\n"); >> + cf->can_id |= CAN_ERR_CRTL; >> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; >> + stats->rx_over_errors++; >> + stats->rx_errors++; >> + } >> + >> + if (isrc & BOIS) { >> + dev_dbg(dev->dev.parent, "bus-off mode interrupt\n"); >> + > > Empty line? > >> + state = CAN_STATE_BUS_OFF; >> + cf->can_id |= CAN_ERR_BUSOFF; >> + can_bus_off(dev); >> + } >> + >> + if (isrc & EPIS) { >> + /* error passive interrupt */ >> + dev_dbg(dev->dev.parent, "error passive interrupt\n"); >> + state = CAN_STATE_ERROR_PASSIVE; >> + } >> + >> + if ((isrc & EWTIS) || (isrc & EWRIS)) { >> + dev_dbg(dev->dev.parent, "Error Warning Transmit/Receive Interrupt\n"); >> + state = CAN_STATE_ERROR_WARNING; >> + } >> + >> + if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING || >> + state == CAN_STATE_ERROR_PASSIVE)) { >> + u16 cec = readw(®->cec); >> + u8 rxerr = cec; >> + u8 txerr = cec >> 8; > > Add an empty line here, please. > >> + cf->can_id |= CAN_ERR_CRTL; >> + if (state == CAN_STATE_ERROR_WARNING) { >> + priv->can.can_stats.error_warning++; >> + cf->data[1] = (txerr > rxerr) ? >> + CAN_ERR_CRTL_TX_WARNING : >> + CAN_ERR_CRTL_RX_WARNING; >> + } else { >> + priv->can.can_stats.error_passive++; >> + cf->data[1] = (txerr > rxerr) ? >> + CAN_ERR_CRTL_TX_PASSIVE : >> + CAN_ERR_CRTL_RX_PASSIVE; >> + } >> + } >> + >> + if (status) { >> + priv->can.can_stats.bus_error++; >> + >> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; >> + >> + if (status & BEF) >> + cf->data[2] |= CAN_ERR_PROT_BIT; >> + else if (status & FER) >> + cf->data[2] |= CAN_ERR_PROT_FORM; >> + else if (status & SER) >> + cf->data[2] |= CAN_ERR_PROT_STUFF; >> + else >> + cf->data[2] |= CAN_ERR_PROT_UNSPEC; >> + } > > Add {} here as well. {} will cause checkpatch warning if it is given to only one line. > >> + priv->can.state = state; >> + >> + netif_rx(skb); >> + >> + stats->rx_packets++; >> + stats->rx_bytes += cf->can_dlc; >> + >> + return 0; >> +} >> + >> +irqreturn_t bfin_can_interrupt(int irq, void *dev_id) >> +{ >> + struct net_device *dev = dev_id; >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + struct net_device_stats *stats = &dev->stats; >> + u16 status, isrc; >> + >> + if ((irq == priv->tx_irq) && readw(®->mbtif2)) { >> + /* transmission complete interrupt */ >> + writew(0xFFFF, ®->mbtif2); >> + stats->tx_packets++; >> + stats->tx_bytes += bfin_can_read_dlc(priv, TRANSMIT_CHL); >> + can_get_echo_skb(dev, 0); >> + netif_wake_queue(dev); >> + } else if ((irq == priv->rx_irq) && readw(®->mbrif1)) { >> + /* receive interrupt */ >> + isrc = readw(®->mbrif1); >> + writew(0xFFFF, ®->mbrif1); >> + bfin_can_rx(dev, isrc); >> + } else if ((irq == priv->err_irq) && readw(®->gis)) { >> + /* error interrupt */ >> + isrc = readw(®->gis); >> + status = readw(®->esr); >> + writew(0x7FF, ®->gis); >> + bfin_can_err(dev, isrc, status); >> + } else >> + return IRQ_NONE; > > Use {} here as well. {} will cause checkpatch warning if it is given to only one line. > >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int bfin_can_open(struct net_device *dev) >> +{ >> + int err; >> + >> + /* set chip into reset mode */ >> + bfin_can_set_reset_mode(dev); >> + >> + /* common open */ >> + err = open_candev(dev); >> + if (err) >> + return err; >> + >> + /* init and start chi */ >> + bfin_can_start(dev); >> + >> + netif_start_queue(dev); >> + >> + return 0; >> +} >> + >> +static int bfin_can_close(struct net_device *dev) >> +{ >> + netif_stop_queue(dev); >> + bfin_can_set_reset_mode(dev); >> + >> + close_candev(dev); >> + >> + return 0; >> +} >> + >> +struct net_device *alloc_bfin_candev(void) >> +{ >> + struct net_device *dev; >> + struct bfin_can_priv *priv; >> + >> + dev = alloc_candev(sizeof(*priv)); >> + if (!dev) >> + return NULL; >> + >> + priv = netdev_priv(dev); >> + >> + priv->dev = dev; >> + priv->can.bittiming_const = &bfin_can_bittiming_const; >> + priv->can.do_set_bittiming = bfin_can_set_bittiming; >> + priv->can.do_set_mode = bfin_can_set_mode; >> + >> + return dev; >> +} >> + >> +static const struct net_device_ops bfin_can_netdev_ops = { >> + .ndo_open = bfin_can_open, >> + .ndo_stop = bfin_can_close, >> + .ndo_start_xmit = bfin_can_start_xmit, >> +}; >> + >> +static int __devinit bfin_can_probe(struct platform_device *pdev) >> +{ >> + int err; >> + struct net_device *dev; >> + struct bfin_can_priv *priv; >> + struct resource *res_mem, *rx_irq, *tx_irq, *err_irq; >> + unsigned short *pdata; >> + >> + pdata = pdev->dev.platform_data; >> + if (!pdata) { >> + dev_err(&pdev->dev, "No platform data provided!\n"); >> + err = -EINVAL; >> + goto exit; >> + } >> + >> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + rx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> + tx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 1); >> + err_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 2); >> + if (!res_mem || !rx_irq || !tx_irq || !err_irq) { >> + err = -EINVAL; >> + goto exit; >> + } >> + >> + if (!request_mem_region(res_mem->start, resource_size(res_mem), >> + dev_name(&pdev->dev))) { >> + err = -EBUSY; >> + goto exit; >> + } >> + >> + /* request peripheral pins */ >> + err = peripheral_request_list(pdata, dev_name(&pdev->dev)); >> + if (err) >> + goto exit_mem_release; >> + >> + dev = alloc_bfin_candev(); >> + if (!dev) { >> + err = -ENOMEM; >> + goto exit_peri_pin_free; >> + } >> + >> + /* register interrupt handler */ >> + err = request_irq(rx_irq->start, &bfin_can_interrupt, 0, >> + "bfin-can-rx", (void *)dev); >> + if (err) >> + goto exit_candev_free; >> + err = request_irq(tx_irq->start, &bfin_can_interrupt, 0, >> + "bfin-can-tx", (void *)dev); >> + if (err) >> + goto exit_rx_irq_free; >> + err = request_irq(err_irq->start, &bfin_can_interrupt, 0, >> + "bfin-can-err", (void *)dev); >> + if (err) >> + goto exit_tx_irq_free; > > If possible, please request/free IRQs in the open and stop functions. > >> + priv = netdev_priv(dev); >> + priv->membase = (void __iomem *)res_mem->start; >> + priv->rx_irq = rx_irq->start; >> + priv->tx_irq = tx_irq->start; >> + priv->err_irq = err_irq->start; >> + priv->pin_list = pdata; >> + priv->can.clock.freq = get_sclk(); >> + >> + dev_set_drvdata(&pdev->dev, dev); >> + SET_NETDEV_DEV(dev, &pdev->dev); >> + >> + dev->flags |= IFF_ECHO; /* we support local echo */ >> + dev->netdev_ops = &bfin_can_netdev_ops; >> + >> + bfin_can_set_reset_mode(dev); >> + >> + err = register_candev(dev); >> + if (err) { >> + dev_err(&pdev->dev, "registering failed (err=%d)\n", err); >> + goto exit_err_irq_free; >> + } >> + >> + dev_info(&pdev->dev, "%s device registered (reg_base=%p, rx_irq=%d, tx_irq=%d, err_irq=%d, sclk=%d)\n", >> + DRV_NAME, (void *)priv->membase, priv->rx_irq, priv->tx_irq, priv->err_irq, >> + priv->can.clock.freq); >> + return 0; >> + >> +exit_err_irq_free: >> + free_irq(err_irq->start, dev); >> +exit_tx_irq_free: >> + free_irq(tx_irq->start, dev); >> +exit_rx_irq_free: >> + free_irq(rx_irq->start, dev); >> +exit_candev_free: >> + free_candev(dev); >> +exit_peri_pin_free: >> + peripheral_free_list(pdata); >> +exit_mem_release: >> + release_mem_region(res_mem->start, resource_size(res_mem)); >> +exit: >> + return err; >> +} >> + >> +static int __devexit bfin_can_remove(struct platform_device *pdev) >> +{ >> + struct net_device *dev = dev_get_drvdata(&pdev->dev); >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct resource *res; >> + >> + bfin_can_set_reset_mode(dev); >> + >> + unregister_candev(dev); >> + >> + dev_set_drvdata(&pdev->dev, NULL); >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + release_mem_region(res->start, resource_size(res)); >> + >> + free_irq(priv->rx_irq, dev); >> + free_irq(priv->tx_irq, dev); >> + free_irq(priv->err_irq, dev); >> + peripheral_free_list(priv->pin_list); >> + >> + free_candev(dev); >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM >> +static int bfin_can_suspend(struct platform_device *pdev, pm_message_t mesg) >> +{ >> + struct net_device *dev = dev_get_drvdata(&pdev->dev); >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + int timeout = BFIN_CAN_TIMEOUT; >> + >> + if (netif_running(dev)) { >> + /* enter sleep mode */ >> + writew(readw(®->ctrl) | SMR, ®->ctrl); >> + SSYNC(); >> + while (!(readw(®->intr) & SMACK)) { >> + udelay(10); >> + if (--timeout == 0) { >> + dev_err(dev->dev.parent, "fail to enter sleep mode\n"); >> + BUG(); >> + } >> + } > > It's already the third time that this timeout check is used. A common > function would make sense. Every time, the check condition is different and print information will change. It is ugly to define only one function. > >> + } >> + >> + return 0; >> +} >> + >> +static int bfin_can_resume(struct platform_device *pdev) >> +{ >> + struct net_device *dev = dev_get_drvdata(&pdev->dev); >> + struct bfin_can_priv *priv = netdev_priv(dev); >> + struct bfin_can_regs *reg = priv->membase; >> + >> + if (netif_running(dev)) { >> + /* leave sleep mode */ >> + writew(0, ®->intr); >> + SSYNC(); >> + } >> + >> + return 0; >> +} >> +#else >> +#define bfin_can_suspend NULL >> +#define bfin_can_resume NULL >> +#endif /* CONFIG_PM */ >> + >> +static struct platform_driver bfin_can_driver = { >> + .probe = bfin_can_probe, >> + .remove = __devexit_p(bfin_can_remove), >> + .suspend = bfin_can_suspend, >> + .resume = bfin_can_resume, >> + .driver = { >> + .name = DRV_NAME, >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +static int __init bfin_can_init(void) >> +{ >> + return platform_driver_register(&bfin_can_driver); >> +} >> + > > Remove empty line above, please. > >> +module_init(bfin_can_init); >> + >> +static void __exit bfin_can_exit(void) >> +{ >> + platform_driver_unregister(&bfin_can_driver); >> +} >> + > > Ditto. > >> +module_exit(bfin_can_exit); >> + >> +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>"); >> +MODULE_LICENSE("GPL"); >> +MODULE_DESCRIPTION("Blackfin on-chip CAN netdevice driver"); > > Wolfgang. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mike Frysinger wrote: > On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >> Barry Song wrote: >>> +#include <linux/module.h> >>> +#include <linux/init.h> >>> +#include <linux/kernel.h> >>> +#include <linux/types.h> >> I think you don't need "types.h" as the code no longer uses "uint*_t". > > linux/types.h declares all types, like u* which this driver still uses I just remember that "linux/types.h" needs to be added for the uint*_t types. At a first glance I do not see __u8/u8 being defined in that header file but I might have missed something. > >> Well, I'm still not a friend of the following inline functions, >> especially the *one-liners* which are called just *once*. With the usage >> of structs they seem even more useless. > > seems like it would make more sense to not even use the read/write > functions either. just declare the regs as volatile and assign/read > the struct directly. Two times no. Don't use volatile and proper accessor functions. See: http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt Wolfgang. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> + if (status) { > >> + priv->can.can_stats.bus_error++; > >> + > >> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > >> + > >> + if (status & BEF) > >> + cf->data[2] |= CAN_ERR_PROT_BIT; > >> + else if (status & FER) > >> + cf->data[2] |= CAN_ERR_PROT_FORM; > >> + else if (status & SER) > >> + cf->data[2] |= CAN_ERR_PROT_STUFF; > >> + else > >> + cf->data[2] |= CAN_ERR_PROT_UNSPEC; > >> + } > > > > Add {} here as well. > {} will cause checkpatch warning if it is given to only one line. Right, no need for braces here, because it is always one statement after if/else. > >> + if ((irq == priv->tx_irq) && readw(®->mbtif2)) { > >> + /* transmission complete interrupt */ > >> + writew(0xFFFF, ®->mbtif2); > >> + stats->tx_packets++; > >> + stats->tx_bytes += bfin_can_read_dlc(priv, TRANSMIT_CHL); > >> + can_get_echo_skb(dev, 0); > >> + netif_wake_queue(dev); > >> + } else if ((irq == priv->rx_irq) && readw(®->mbrif1)) { > >> + /* receive interrupt */ > >> + isrc = readw(®->mbrif1); > >> + writew(0xFFFF, ®->mbrif1); > >> + bfin_can_rx(dev, isrc); > >> + } else if ((irq == priv->err_irq) && readw(®->gis)) { > >> + /* error interrupt */ > >> + isrc = readw(®->gis); > >> + status = readw(®->esr); > >> + writew(0x7FF, ®->gis); > >> + bfin_can_err(dev, isrc, status); > >> + } else > >> + return IRQ_NONE; > > > > Use {} here as well. > {} will cause checkpatch warning if it is given to only one line. Then again, here braces are suggested. If the 'if'-block has braces, the 'else'-block should have braces, too. If checkpatch really complains about it, this is a bug in checkpatch. Regards, Wolfram
On Thu, Dec 10, 2009 at 6:21 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > >> >> + if (status) { >> >> + priv->can.can_stats.bus_error++; >> >> + >> >> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; >> >> + >> >> + if (status & BEF) >> >> + cf->data[2] |= CAN_ERR_PROT_BIT; >> >> + else if (status & FER) >> >> + cf->data[2] |= CAN_ERR_PROT_FORM; >> >> + else if (status & SER) >> >> + cf->data[2] |= CAN_ERR_PROT_STUFF; >> >> + else >> >> + cf->data[2] |= CAN_ERR_PROT_UNSPEC; >> >> + } >> > >> > Add {} here as well. >> {} will cause checkpatch warning if it is given to only one line. > > Right, no need for braces here, because it is always one statement after > if/else. > > >> >> + if ((irq == priv->tx_irq) && readw(®->mbtif2)) { >> >> + /* transmission complete interrupt */ >> >> + writew(0xFFFF, ®->mbtif2); >> >> + stats->tx_packets++; >> >> + stats->tx_bytes += bfin_can_read_dlc(priv, TRANSMIT_CHL); >> >> + can_get_echo_skb(dev, 0); >> >> + netif_wake_queue(dev); >> >> + } else if ((irq == priv->rx_irq) && readw(®->mbrif1)) { >> >> + /* receive interrupt */ >> >> + isrc = readw(®->mbrif1); >> >> + writew(0xFFFF, ®->mbrif1); >> >> + bfin_can_rx(dev, isrc); >> >> + } else if ((irq == priv->err_irq) && readw(®->gis)) { >> >> + /* error interrupt */ >> >> + isrc = readw(®->gis); >> >> + status = readw(®->esr); >> >> + writew(0x7FF, ®->gis); >> >> + bfin_can_err(dev, isrc, status); >> >> + } else >> >> + return IRQ_NONE; >> > >> > Use {} here as well. >> {} will cause checkpatch warning if it is given to only one line. > > Then again, here braces are suggested. If the 'if'-block has braces, the > 'else'-block should have braces, too. If checkpatch really complains about it, > this is a bug in checkpatch. I think you are right. "else {return IRQ_NONE;}" gives no check patch warning. > > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAksgy7kACgkQD27XaX1/VRuNlwCgsSRFNBn9ZOhqnQrNuO8T+jGD > 0IcAnj31td4yugsGcccmKyPPkNTogzGt > =u741 > -----END PGP SIGNATURE----- > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Barry Song wrote: > On Thu, Dec 10, 2009 at 5:11 PM, Wolfgang Grandegger <wg@grandegger.com> wrote: >> Barry Song wrote: >>> Signed-off-by: Barry Song <21cnbao@gmail.com> >>> Signed-off-by: H.J. Oertel <oe@port.de> >>> --- >>> -v3: use structures to describe the layout of registers >> Wow, that was quick, thanks for your effort and patience. >> >> Please use checkpath.pl to detect the obvious coding style problems, >> especially the "WARNING: line over 80 characters". >> >> I also think the declaration of reg should have the __iomem as well: >> >> struct bfin_can_regs __iomem *reg = priv->membase; >> >>> drivers/net/can/Kconfig | 9 + >>> drivers/net/can/Makefile | 1 + >>> drivers/net/can/bfin_can.c | 836 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 846 insertions(+), 0 deletions(-) >>> create mode 100644 drivers/net/can/bfin_can.c >>> [snip] >>> diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c >>> new file mode 100644 >>> index 0000000..b94169d >>> --- /dev/null >>> +++ b/drivers/net/can/bfin_can.c >>> @@ -0,0 +1,836 @@ >>> +/* >>> + * Blackfin On-Chip CAN Driver >>> + * >>> + * Copyright 2004-2009 Analog Devices Inc. >> Consider adding your copyright here, with a name and address. >> >>> + * >>> + * Enter bugs at http://blackfin.uclinux.org/ >>> + * >>> + * Licensed under the GPL-2 or later. >> Please add the usual "GPL-2 or later" bla-bla here. > Here I am not completely sure. But I am sure I am using the head file > template of ADI which has been used widely in kernel and should be > right. Well, at least it's not the usual bla bla. Maybe somebody else could clarify that. David? [snip] >>> +static void bfin_can_set_reset_mode(struct net_device *dev) >>> +{ >>> + struct bfin_can_priv *priv = netdev_priv(dev); >>> + struct bfin_can_regs *reg = priv->membase; >>> + int timeout = BFIN_CAN_TIMEOUT; >>> + int i; >>> + >>> + /* disable interrupts */ >>> + writew(0, ®->mbim1); >>> + writew(0, ®->mbim2); >>> + writew(0, ®->gim); >>> + >>> + /* reset can and enter configuration mode */ >>> + writew(SRS | CCR, ®->ctrl); >>> + SSYNC(); >>> + writew(CCR, ®->ctrl); >>> + SSYNC(); >>> + while (!(readw(®->ctrl) & CCA)) { >>> + udelay(10); >>> + if (--timeout == 0) { >>> + dev_err(dev->dev.parent, "fail to enter configuration mode\n"); >>> + BUG(); >>> + } >>> + } >> Isn't using BUG() to harsh here? Using and handling a proper return code >> might already be sufficient. > Due to the hardware design, here timeout will never happen. If it > happens, that means hardware component has crashed. OK. [snip] >>> +static int bfin_can_err(struct net_device *dev, u16 isrc, u16 status) >>> +{ >>> + struct bfin_can_priv *priv = netdev_priv(dev); >>> + struct bfin_can_regs *reg = priv->membase; >>> + struct net_device_stats *stats = &dev->stats; >>> + struct can_frame *cf; >>> + struct sk_buff *skb; >>> + enum can_state state = priv->can.state; >>> + >>> + skb = alloc_can_err_skb(dev, &cf); >>> + if (skb == NULL) >>> + return -ENOMEM; >>> + >>> + if (isrc & RMLIS) { >>> + /* data overrun interrupt */ >>> + dev_dbg(dev->dev.parent, "data overrun interrupt\n"); >>> + cf->can_id |= CAN_ERR_CRTL; >>> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; >>> + stats->rx_over_errors++; >>> + stats->rx_errors++; >>> + } >>> + >>> + if (isrc & BOIS) { >>> + dev_dbg(dev->dev.parent, "bus-off mode interrupt\n"); >>> + >> Empty line? >> >>> + state = CAN_STATE_BUS_OFF; >>> + cf->can_id |= CAN_ERR_BUSOFF; >>> + can_bus_off(dev); >>> + } >>> + >>> + if (isrc & EPIS) { >>> + /* error passive interrupt */ >>> + dev_dbg(dev->dev.parent, "error passive interrupt\n"); >>> + state = CAN_STATE_ERROR_PASSIVE; >>> + } >>> + >>> + if ((isrc & EWTIS) || (isrc & EWRIS)) { >>> + dev_dbg(dev->dev.parent, "Error Warning Transmit/Receive Interrupt\n"); >>> + state = CAN_STATE_ERROR_WARNING; >>> + } >>> + >>> + if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING || >>> + state == CAN_STATE_ERROR_PASSIVE)) { >>> + u16 cec = readw(®->cec); >>> + u8 rxerr = cec; >>> + u8 txerr = cec >> 8; >> Add an empty line here, please. >> >>> + cf->can_id |= CAN_ERR_CRTL; >>> + if (state == CAN_STATE_ERROR_WARNING) { >>> + priv->can.can_stats.error_warning++; >>> + cf->data[1] = (txerr > rxerr) ? >>> + CAN_ERR_CRTL_TX_WARNING : >>> + CAN_ERR_CRTL_RX_WARNING; >>> + } else { >>> + priv->can.can_stats.error_passive++; >>> + cf->data[1] = (txerr > rxerr) ? >>> + CAN_ERR_CRTL_TX_PASSIVE : >>> + CAN_ERR_CRTL_RX_PASSIVE; >>> + } >>> + } >>> + >>> + if (status) { >>> + priv->can.can_stats.bus_error++; >>> + >>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; >>> + >>> + if (status & BEF) >>> + cf->data[2] |= CAN_ERR_PROT_BIT; >>> + else if (status & FER) >>> + cf->data[2] |= CAN_ERR_PROT_FORM; >>> + else if (status & SER) >>> + cf->data[2] |= CAN_ERR_PROT_STUFF; >>> + else >>> + cf->data[2] |= CAN_ERR_PROT_UNSPEC; >>> + } >> Add {} here as well. > {} will cause checkpatch warning if it is given to only one line. Of course, my fault. Forget it. >>> + priv->can.state = state; >>> + >>> + netif_rx(skb); >>> + >>> + stats->rx_packets++; >>> + stats->rx_bytes += cf->can_dlc; >>> + >>> + return 0; >>> +} >>> + >>> +irqreturn_t bfin_can_interrupt(int irq, void *dev_id) >>> +{ >>> + struct net_device *dev = dev_id; >>> + struct bfin_can_priv *priv = netdev_priv(dev); >>> + struct bfin_can_regs *reg = priv->membase; >>> + struct net_device_stats *stats = &dev->stats; >>> + u16 status, isrc; >>> + >>> + if ((irq == priv->tx_irq) && readw(®->mbtif2)) { >>> + /* transmission complete interrupt */ >>> + writew(0xFFFF, ®->mbtif2); >>> + stats->tx_packets++; >>> + stats->tx_bytes += bfin_can_read_dlc(priv, TRANSMIT_CHL); >>> + can_get_echo_skb(dev, 0); >>> + netif_wake_queue(dev); >>> + } else if ((irq == priv->rx_irq) && readw(®->mbrif1)) { >>> + /* receive interrupt */ >>> + isrc = readw(®->mbrif1); >>> + writew(0xFFFF, ®->mbrif1); >>> + bfin_can_rx(dev, isrc); >>> + } else if ((irq == priv->err_irq) && readw(®->gis)) { >>> + /* error interrupt */ >>> + isrc = readw(®->gis); >>> + status = readw(®->esr); >>> + writew(0x7FF, ®->gis); >>> + bfin_can_err(dev, isrc, status); >>> + } else >>> + return IRQ_NONE; >> Use {} here as well. > {} will cause checkpatch warning if it is given to only one line. But here it should be added as explained here: http://lxr.linux.no/#linux+v2.6.32/Documentation/CodingStyle#L171 Does checkpatch really complain? [snip] >>> +#ifdef CONFIG_PM >>> +static int bfin_can_suspend(struct platform_device *pdev, pm_message_t mesg) >>> +{ >>> + struct net_device *dev = dev_get_drvdata(&pdev->dev); >>> + struct bfin_can_priv *priv = netdev_priv(dev); >>> + struct bfin_can_regs *reg = priv->membase; >>> + int timeout = BFIN_CAN_TIMEOUT; >>> + >>> + if (netif_running(dev)) { >>> + /* enter sleep mode */ >>> + writew(readw(®->ctrl) | SMR, ®->ctrl); >>> + SSYNC(); >>> + while (!(readw(®->intr) & SMACK)) { >>> + udelay(10); >>> + if (--timeout == 0) { >>> + dev_err(dev->dev.parent, "fail to enter sleep mode\n"); >>> + BUG(); >>> + } >>> + } >> It's already the third time that this timeout check is used. A common >> function would make sense. > Every time, the check condition is different and print information > will change. It is ugly to define only one function. OK, I obviously missed that. Sorry for the noise. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 05:20, Wolfgang Grandegger wrote: > Mike Frysinger wrote: >> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>> Barry Song wrote: >>>> +#include <linux/module.h> >>>> +#include <linux/init.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/types.h> >>> I think you don't need "types.h" as the code no longer uses "uint*_t". >> >> linux/types.h declares all types, like u* which this driver still uses > > I just remember that "linux/types.h" needs to be added for the uint*_t > types. At a first glance I do not see __u8/u8 being defined in that > header file but I might have missed something. you need to follow the include paths >>> Well, I'm still not a friend of the following inline functions, >>> especially the *one-liners* which are called just *once*. With the usage >>> of structs they seem even more useless. >> >> seems like it would make more sense to not even use the read/write >> functions either. just declare the regs as volatile and assign/read >> the struct directly. > > Two times no. Don't use volatile and proper accessor functions. See: > > http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt too bad the document is largely irrelevant (all but one paragraph) because this is how volatiles were designed in the first place -- hardware I/O registers. the CAN implementation here is Blackfin specific and not going to be use elsewhere, so other architectures are irrelevant. the resulting C code would certainly look a hell of a lot more natural without the useless I/O accessor functions, and be much tighter. at any rate, the common Blackfin I/O accessor functions force a lot of useless overhead when used here as they're designed for async memory, not MMRs. the driver needs to be switched to the bfin_read/bfin_write MMR functions. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wolfgang Grandegger wrote: > Mike Frysinger wrote: >> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>> Barry Song wrote: >>>> +#include <linux/module.h> >>>> +#include <linux/init.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/types.h> >>> I think you don't need "types.h" as the code no longer uses "uint*_t". >> linux/types.h declares all types, like u* which this driver still uses > > I just remember that "linux/types.h" needs to be added for the uint*_t > types. At a first glance I do not see __u8/u8 being defined in that > header file but I might have missed something. > >>> Well, I'm still not a friend of the following inline functions, >>> especially the *one-liners* which are called just *once*. With the usage >>> of structs they seem even more useless. >> seems like it would make more sense to not even use the read/write >> functions either. just declare the regs as volatile and assign/read >> the struct directly. > > Two times no. Don't use volatile and proper accessor functions. See: > > http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt I was just wondering if bfin_read/write16 would not be the proper accessor functions. readw/writew seems to be implemented differently: http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely shows why accessor functions should be used to access device registers. Well, just curious. I don't really know the blackfin arch. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mike Frysinger wrote: > On Thu, Dec 10, 2009 at 05:20, Wolfgang Grandegger wrote: >> Mike Frysinger wrote: >>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>> Barry Song wrote: >>>>> +#include <linux/module.h> >>>>> +#include <linux/init.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/types.h> >>>> I think you don't need "types.h" as the code no longer uses "uint*_t". >>> linux/types.h declares all types, like u* which this driver still uses >> I just remember that "linux/types.h" needs to be added for the uint*_t >> types. At a first glance I do not see __u8/u8 being defined in that >> header file but I might have missed something. > > you need to follow the include paths I thought I did. Could you point me to the relevant location? >>>> Well, I'm still not a friend of the following inline functions, >>>> especially the *one-liners* which are called just *once*. With the usage >>>> of structs they seem even more useless. >>> seems like it would make more sense to not even use the read/write >>> functions either. just declare the regs as volatile and assign/read >>> the struct directly. >> Two times no. Don't use volatile and proper accessor functions. See: >> >> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt > > too bad the document is largely irrelevant (all but one paragraph) > because this is how volatiles were designed in the first place -- > hardware I/O registers. the CAN implementation here is Blackfin > specific and not going to be use elsewhere, so other architectures are > irrelevant. the resulting C code would certainly look a hell of a lot > more natural without the useless I/O accessor functions, and be much > tighter. Well, so far *no* volatiles have been used in the BFIN CAN driver. But if you tell me that they are really required for blackfin... I can't really judge. > at any rate, the common Blackfin I/O accessor functions force a lot of > useless overhead when used here as they're designed for async memory, > not MMRs. the driver needs to be switched to the bfin_read/bfin_write > MMR functions. I just brought up this issue in another mail. Wolfgang. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 05:48, Wolfgang Grandegger wrote: > Wolfgang Grandegger wrote: >> Mike Frysinger wrote: >>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>> Well, I'm still not a friend of the following inline functions, >>>> especially the *one-liners* which are called just *once*. With the usage >>>> of structs they seem even more useless. >>> seems like it would make more sense to not even use the read/write >>> functions either. just declare the regs as volatile and assign/read >>> the struct directly. >> >> Two times no. Don't use volatile and proper accessor functions. See: >> >> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt > > I was just wondering if bfin_read/write16 would not be the proper > accessor functions. readw/writew seems to be implemented differently: > > http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 > > Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely > shows why accessor functions should be used to access device registers. > > Well, just curious. I don't really know the blackfin arch. the common I/O functions need to account for issues surrounding the bus that has arbitrary devices memory mapped to it. on-chip devices (like what we're talking about here) do not have these issues and so using the common functions is awful overhead. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 05:55, Wolfgang Grandegger wrote: > Mike Frysinger wrote: >> On Thu, Dec 10, 2009 at 05:20, Wolfgang Grandegger wrote: >>> Mike Frysinger wrote: >>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>> Barry Song wrote: >>>>>> +#include <linux/module.h> >>>>>> +#include <linux/init.h> >>>>>> +#include <linux/kernel.h> >>>>>> +#include <linux/types.h> >>>>> I think you don't need "types.h" as the code no longer uses "uint*_t". >>>> linux/types.h declares all types, like u* which this driver still uses >>> I just remember that "linux/types.h" needs to be added for the uint*_t >>> types. At a first glance I do not see __u8/u8 being defined in that >>> header file but I might have missed something. >> >> you need to follow the include paths > > I thought I did. Could you point me to the relevant location? make kernel/printk.i sed -n -e '/^#/p' -e '/ u64;/{p;q}' kernel/printk.i linux/types.h -> asm/types.h -> asm-generic/types.h -> asm-generic/int-ll64.h >>>>> Well, I'm still not a friend of the following inline functions, >>>>> especially the *one-liners* which are called just *once*. With the usage >>>>> of structs they seem even more useless. >>>> seems like it would make more sense to not even use the read/write >>>> functions either. just declare the regs as volatile and assign/read >>>> the struct directly. >>> Two times no. Don't use volatile and proper accessor functions. See: >>> >>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >> >> too bad the document is largely irrelevant (all but one paragraph) >> because this is how volatiles were designed in the first place -- >> hardware I/O registers. the CAN implementation here is Blackfin >> specific and not going to be use elsewhere, so other architectures are >> irrelevant. the resulting C code would certainly look a hell of a lot >> more natural without the useless I/O accessor functions, and be much >> tighter. > > Well, so far *no* volatiles have been used in the BFIN CAN driver. But > if you tell me that they are really required for blackfin... I can't > really judge. i'm not saying they're needed all the time, i'm saying volatile produces more natural coding style than I/O accessors. but after reviewing the bfin_read/bfin_write functions, we still need to use those to workaround a simple anomaly on older parts. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>Wolfgang Grandegger wrote: >> Mike Frysinger wrote: >>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>> Barry Song wrote: >>>>> +#include <linux/module.h> >>>>> +#include <linux/init.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/types.h> >>>> I think you don't need "types.h" as the code no longer uses >"uint*_t". >>> linux/types.h declares all types, like u* which this driver still >uses >> >> I just remember that "linux/types.h" needs to be added for the uint*_t >> types. At a first glance I do not see __u8/u8 being defined in that >> header file but I might have missed something. >> >>>> Well, I'm still not a friend of the following inline functions, >>>> especially the *one-liners* which are called just *once*. With the >usage >>>> of structs they seem even more useless. >>> seems like it would make more sense to not even use the read/write >>> functions either. just declare the regs as volatile and assign/read >>> the struct directly. >> >> Two times no. Don't use volatile and proper accessor functions. See: >> >> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered- >harmful.txt > >I was just wondering if bfin_read/write16 would not be the proper >accessor functions. readw/writew seems to be implemented differently: > >http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 > >Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >shows why accessor functions should be used to access device registers. > >Well, just curious. I don't really know the blackfin arch. Well - on Blackfin its absolutely ok to access System Memory Mapped Registers using structs. At any rate volatile is then required to prevent the compiler to optimize accesses away. IMHO this is a pretty legal use of volatile, and used in hundreds of places all over the kernel. When accessing external controllers accessor functions from io.h must be used. There are two things to consider here: 1) weak ordering of reads and writes 2) killed and reissued reads (especially harmful when reading from FIFOs) -Michael > >Wolfgang. > >_______________________________________________ >Uclinux-dist-devel mailing list >Uclinux-dist-devel@blackfin.uclinux.org >https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mike Frysinger wrote: > On Thu, Dec 10, 2009 at 05:55, Wolfgang Grandegger wrote: >> Mike Frysinger wrote: >>> On Thu, Dec 10, 2009 at 05:20, Wolfgang Grandegger wrote: >>>> Mike Frysinger wrote: >>>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>>> Barry Song wrote: >>>>>>> +#include <linux/module.h> >>>>>>> +#include <linux/init.h> >>>>>>> +#include <linux/kernel.h> >>>>>>> +#include <linux/types.h> >>>>>> I think you don't need "types.h" as the code no longer uses "uint*_t". >>>>> linux/types.h declares all types, like u* which this driver still uses >>>> I just remember that "linux/types.h" needs to be added for the uint*_t >>>> types. At a first glance I do not see __u8/u8 being defined in that >>>> header file but I might have missed something. >>> you need to follow the include paths >> I thought I did. Could you point me to the relevant location? > > make kernel/printk.i > sed -n -e '/^#/p' -e '/ u64;/{p;q}' kernel/printk.i > linux/types.h -> asm/types.h -> asm-generic/types.h -> asm-generic/int-ll64.h Thanks. >>>>>> Well, I'm still not a friend of the following inline functions, >>>>>> especially the *one-liners* which are called just *once*. With the usage >>>>>> of structs they seem even more useless. >>>>> seems like it would make more sense to not even use the read/write >>>>> functions either. just declare the regs as volatile and assign/read >>>>> the struct directly. >>>> Two times no. Don't use volatile and proper accessor functions. See: >>>> >>>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >>> too bad the document is largely irrelevant (all but one paragraph) >>> because this is how volatiles were designed in the first place -- >>> hardware I/O registers. the CAN implementation here is Blackfin >>> specific and not going to be use elsewhere, so other architectures are >>> irrelevant. the resulting C code would certainly look a hell of a lot >>> more natural without the useless I/O accessor functions, and be much >>> tighter. >> Well, so far *no* volatiles have been used in the BFIN CAN driver. But >> if you tell me that they are really required for blackfin... I can't >> really judge. > > i'm not saying they're needed all the time, i'm saying volatile > produces more natural coding style than I/O accessors. but after > reviewing the bfin_read/bfin_write functions, we still need to use > those to workaround a simple anomaly on older parts. I believe that it's good practice to hide such details by using proper accessor functions. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hennerich, Michael wrote: > >> Wolfgang Grandegger wrote: >>> Mike Frysinger wrote: >>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>> Barry Song wrote: >>>>>> +#include <linux/module.h> >>>>>> +#include <linux/init.h> >>>>>> +#include <linux/kernel.h> >>>>>> +#include <linux/types.h> >>>>> I think you don't need "types.h" as the code no longer uses >> "uint*_t". >>>> linux/types.h declares all types, like u* which this driver still >> uses >>> I just remember that "linux/types.h" needs to be added for the > uint*_t >>> types. At a first glance I do not see __u8/u8 being defined in that >>> header file but I might have missed something. >>> >>>>> Well, I'm still not a friend of the following inline functions, >>>>> especially the *one-liners* which are called just *once*. With the >> usage >>>>> of structs they seem even more useless. >>>> seems like it would make more sense to not even use the read/write >>>> functions either. just declare the regs as volatile and assign/read >>>> the struct directly. >>> Two times no. Don't use volatile and proper accessor functions. See: >>> >>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered- >> harmful.txt >> >> I was just wondering if bfin_read/write16 would not be the proper >> accessor functions. readw/writew seems to be implemented differently: >> >> http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 >> >> Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >> shows why accessor functions should be used to access device registers. >> >> Well, just curious. I don't really know the blackfin arch. > > Well - on Blackfin its absolutely ok to access System Memory Mapped > Registers using structs. > At any rate volatile is then required to prevent the compiler to > optimize accesses away. > IMHO this is a pretty legal use of volatile, and used in hundreds of > places all over the kernel. > > When accessing external controllers accessor functions from io.h must be > used. > There are two things to consider here: > 1) weak ordering of reads and writes > 2) killed and reissued reads (especially harmful when reading from > FIFOs) OK, anyway, I believe that it's good practice to hide all such details by using proper accessor functions. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mike Frysinger <vapier.adi@gmail.com> Date: Thu, 10 Dec 2009 05:58:58 -0500 > On Thu, Dec 10, 2009 at 05:48, Wolfgang Grandegger wrote: >> Wolfgang Grandegger wrote: >>> Mike Frysinger wrote: >>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>> Well, I'm still not a friend of the following inline functions, >>>>> especially the *one-liners* which are called just *once*. With the usage >>>>> of structs they seem even more useless. >>>> seems like it would make more sense to not even use the read/write >>>> functions either. ,A just declare the regs as volatile and assign/read >>>> the struct directly. >>> >>> Two times no. Don't use volatile and proper accessor functions. See: >>> >>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >> >> I was just wondering if bfin_read/write16 would not be the proper >> accessor functions. readw/writew seems to be implemented differently: >> >> http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 >> >> Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >> shows why accessor functions should be used to access device registers. >> >> Well, just curious. I don't really know the blackfin arch. > > the common I/O functions need to account for issues surrounding the > bus that has arbitrary devices memory mapped to it. on-chip devices > (like what we're talking about here) do not have these issues and so > using the common functions is awful overhead. Then create special accessors (perhaps with the same names as the existing ones, but with "__" prepended) that lack all of the interrupt disabling, syncs, etc. Really it _is_ cleaner and makes your driver look a lot nicer. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Wolfgang Grandegger <wg@grandegger.com> Date: Thu, 10 Dec 2009 12:16:17 +0100 > I believe that it's good practice to hide such details by using proper > accessor functions. Absolutely, and positively %100 correct. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/12/11 David Miller <davem@davemloft.net>: > From: Mike Frysinger <vapier.adi@gmail.com> > Date: Thu, 10 Dec 2009 05:58:58 -0500 > >> On Thu, Dec 10, 2009 at 05:48, Wolfgang Grandegger wrote: >>> Wolfgang Grandegger wrote: >>>> Mike Frysinger wrote: >>>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>>> Well, I'm still not a friend of the following inline functions, >>>>>> especially the *one-liners* which are called just *once*. With the usage >>>>>> of structs they seem even more useless. >>>>> seems like it would make more sense to not even use the read/write >>>>> functions either. �,A just declare the regs as volatile and assign/read >>>>> the struct directly. >>>> >>>> Two times no. Don't use volatile and proper accessor functions. See: >>>> >>>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >>> >>> I was just wondering if bfin_read/write16 would not be the proper >>> accessor functions. readw/writew seems to be implemented differently: >>> >>> http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 >>> >>> Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >>> shows why accessor functions should be used to access device registers. >>> >>> Well, just curious. I don't really know the blackfin arch. >> >> the common I/O functions need to account for issues surrounding the >> bus that has arbitrary devices memory mapped to it. on-chip devices >> (like what we're talking about here) do not have these issues and so >> using the common functions is awful overhead. > > Then create special accessors (perhaps with the same names as the > existing ones, but with "__" prepended) that lack all of the > interrupt disabling, syncs, etc. > > Really it _is_ cleaner and makes your driver look a lot nicer. I think Mike has said the functions are bfin_read/bfin_write in blackfin arch since those CAN registers are located in memory mapped area but not async memory and have less overhead than common io functions? Is it acceptable to use those functions in this driver? > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2009 at 21:05, Barry Song wrote: > 2009/12/11 David Miller <davem@davemloft.net>: >> From: Mike Frysinger <vapier.adi@gmail.com> >>> On Thu, Dec 10, 2009 at 05:48, Wolfgang Grandegger wrote: >>>> Wolfgang Grandegger wrote: >>>>> Mike Frysinger wrote: >>>>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>>>> Well, I'm still not a friend of the following inline functions, >>>>>>> especially the *one-liners* which are called just *once*. With the usage >>>>>>> of structs they seem even more useless. >>>>>> seems like it would make more sense to not even use the read/write >>>>>> functions either. �,A just declare the regs as volatile and assign/read >>>>>> the struct directly. >>>>> >>>>> Two times no. Don't use volatile and proper accessor functions. See: >>>>> >>>>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >>>> >>>> I was just wondering if bfin_read/write16 would not be the proper >>>> accessor functions. readw/writew seems to be implemented differently: >>>> >>>> http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 >>>> >>>> Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >>>> shows why accessor functions should be used to access device registers. >>>> >>>> Well, just curious. I don't really know the blackfin arch. >>> >>> the common I/O functions need to account for issues surrounding the >>> bus that has arbitrary devices memory mapped to it. on-chip devices >>> (like what we're talking about here) do not have these issues and so >>> using the common functions is awful overhead. >> >> Then create special accessors (perhaps with the same names as the >> existing ones, but with "__" prepended) that lack all of the >> interrupt disabling, syncs, etc. >> >> Really it _is_ cleaner and makes your driver look a lot nicer. > > I think Mike has said the functions are bfin_read/bfin_write in > blackfin arch since those CAN registers are located in memory mapped > area but not async memory and have less overhead than common io > functions? Is it acceptable to use those functions in this driver? yes, bfin_{read,write} should be used -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 11, 2009 at 10:17 AM, Mike Frysinger <vapier.adi@gmail.com> wrote: > On Thu, Dec 10, 2009 at 21:05, Barry Song wrote: >> 2009/12/11 David Miller <davem@davemloft.net>: >>> From: Mike Frysinger <vapier.adi@gmail.com> >>>> On Thu, Dec 10, 2009 at 05:48, Wolfgang Grandegger wrote: >>>>> Wolfgang Grandegger wrote: >>>>>> Mike Frysinger wrote: >>>>>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>>>>> Well, I'm still not a friend of the following inline functions, >>>>>>>> especially the *one-liners* which are called just *once*. With the usage >>>>>>>> of structs they seem even more useless. >>>>>>> seems like it would make more sense to not even use the read/write >>>>>>> functions either. �,A just declare the regs as volatile and assign/read >>>>>>> the struct directly. >>>>>> >>>>>> Two times no. Don't use volatile and proper accessor functions. See: >>>>>> >>>>>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >>>>> >>>>> I was just wondering if bfin_read/write16 would not be the proper >>>>> accessor functions. readw/writew seems to be implemented differently: >>>>> >>>>> http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 >>>>> >>>>> Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >>>>> shows why accessor functions should be used to access device registers. >>>>> >>>>> Well, just curious. I don't really know the blackfin arch. >>>> >>>> the common I/O functions need to account for issues surrounding the >>>> bus that has arbitrary devices memory mapped to it. on-chip devices >>>> (like what we're talking about here) do not have these issues and so >>>> using the common functions is awful overhead. >>> >>> Then create special accessors (perhaps with the same names as the >>> existing ones, but with "__" prepended) that lack all of the >>> interrupt disabling, syncs, etc. >>> >>> Really it _is_ cleaner and makes your driver look a lot nicer. >> >> I think Mike has said the functions are bfin_read/bfin_write in >> blackfin arch since those CAN registers are located in memory mapped >> area but not async memory and have less overhead than common io >> functions? Is it acceptable to use those functions in this driver? > > yes, bfin_{read,write} should be used Wolfgang/David, are you ok with that too? If so, I will send a -v4 patch using bfin_read/write, with all fix according to your other comments. > -mike > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Barry Song wrote: > On Fri, Dec 11, 2009 at 10:17 AM, Mike Frysinger <vapier.adi@gmail.com> wrote: >> On Thu, Dec 10, 2009 at 21:05, Barry Song wrote: >>> 2009/12/11 David Miller <davem@davemloft.net>: >>>> From: Mike Frysinger <vapier.adi@gmail.com> >>>>> On Thu, Dec 10, 2009 at 05:48, Wolfgang Grandegger wrote: >>>>>> Wolfgang Grandegger wrote: >>>>>>> Mike Frysinger wrote: >>>>>>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>>>>>> Well, I'm still not a friend of the following inline functions, >>>>>>>>> especially the *one-liners* which are called just *once*. With the usage >>>>>>>>> of structs they seem even more useless. >>>>>>>> seems like it would make more sense to not even use the read/write >>>>>>>> functions either. �,A just declare the regs as volatile and assign/read >>>>>>>> the struct directly. >>>>>>> Two times no. Don't use volatile and proper accessor functions. See: >>>>>>> >>>>>>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >>>>>> I was just wondering if bfin_read/write16 would not be the proper >>>>>> accessor functions. readw/writew seems to be implemented differently: >>>>>> >>>>>> http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 >>>>>> >>>>>> Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >>>>>> shows why accessor functions should be used to access device registers. >>>>>> >>>>>> Well, just curious. I don't really know the blackfin arch. >>>>> the common I/O functions need to account for issues surrounding the >>>>> bus that has arbitrary devices memory mapped to it. on-chip devices >>>>> (like what we're talking about here) do not have these issues and so >>>>> using the common functions is awful overhead. >>>> Then create special accessors (perhaps with the same names as the >>>> existing ones, but with "__" prepended) that lack all of the >>>> interrupt disabling, syncs, etc. >>>> >>>> Really it _is_ cleaner and makes your driver look a lot nicer. >>> I think Mike has said the functions are bfin_read/bfin_write in >>> blackfin arch since those CAN registers are located in memory mapped >>> area but not async memory and have less overhead than common io >>> functions? Is it acceptable to use those functions in this driver? >> yes, bfin_{read,write} should be used > > Wolfgang/David, are you ok with that too? If so, I will send a -v4 > patch using bfin_read/write, with all fix according to your other > comments. I understood from Mike that these are the proper functions to be used for accessing on-chip registers. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/12/10 David Miller: > From: Mike Frysinger >> On Thu, Dec 10, 2009 at 05:48, Wolfgang Grandegger wrote: >>> Wolfgang Grandegger wrote: >>>> Mike Frysinger wrote: >>>>> On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote: >>>>>> Well, I'm still not a friend of the following inline functions, >>>>>> especially the *one-liners* which are called just *once*. With the usage >>>>>> of structs they seem even more useless. >>>>> seems like it would make more sense to not even use the read/write >>>>> functions either. �,A just declare the regs as volatile and assign/read >>>>> the struct directly. >>>> >>>> Two times no. Don't use volatile and proper accessor functions. See: >>>> >>>> http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-harmful.txt >>> >>> I was just wondering if bfin_read/write16 would not be the proper >>> accessor functions. readw/writew seems to be implemented differently: >>> >>> http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44 >>> >>> Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely >>> shows why accessor functions should be used to access device registers. >>> >>> Well, just curious. I don't really know the blackfin arch. >> >> the common I/O functions need to account for issues surrounding the >> bus that has arbitrary devices memory mapped to it. on-chip devices >> (like what we're talking about here) do not have these issues and so >> using the common functions is awful overhead. > > Then create special accessors (perhaps with the same names as the > existing ones, but with "__" prepended) that lack all of the > interrupt disabling, syncs, etc. > > Really it _is_ cleaner and makes your driver look a lot nicer. it really isnt. more reasons why io accessors suck: - no type checking - easy to screw up register sizes (8/16/32) - have to add shims for each register that changes sizes between parts - no way to mark a register read-only (i.e. const) unfortunately, we cant push the issue with Blackfin MMRs because of a minor anomaly that exists on older parts. -mike -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index bb803fa..8c485aa 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -54,6 +54,15 @@ config CAN_MCP251X ---help--- Driver for the Microchip MCP251x SPI CAN controllers. +config CAN_BFIN + depends on CAN_DEV && (BF534 || BF536 || BF537 || BF538 || BF539 || BF54x) + tristate "Analog Devices Blackfin on-chip CAN" + ---help--- + Driver for the Analog Devices Blackfin on-chip CAN controllers + + To compile this driver as a module, choose M here: the + module will be called bfin_can. + source "drivers/net/can/mscan/Kconfig" source "drivers/net/can/sja1000/Kconfig" diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile index 56899fe..7a702f2 100644 --- a/drivers/net/can/Makefile +++ b/drivers/net/can/Makefile @@ -14,5 +14,6 @@ obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_AT91) += at91_can.o obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o +obj-$(CONFIG_CAN_BFIN) += bfin_can.o ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c new file mode 100644 index 0000000..b94169d --- /dev/null +++ b/drivers/net/can/bfin_can.c @@ -0,0 +1,836 @@ +/* + * Blackfin On-Chip CAN Driver + * + * Copyright 2004-2009 Analog Devices Inc. + * + * Enter bugs at http://blackfin.uclinux.org/ + * + * Licensed under the GPL-2 or later. + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/bitops.h> +#include <linux/interrupt.h> +#include <linux/errno.h> +#include <linux/netdevice.h> +#include <linux/skbuff.h> +#include <linux/platform_device.h> + +#include <linux/can.h> +#include <linux/can/dev.h> +#include <linux/can/error.h> + +#include <asm/portmux.h> + +#define DRV_NAME "bfin_can" +#define BFIN_CAN_TIMEOUT 100 + +/* + * transmit and receive channels + */ +#define TRANSMIT_CHL 24 +#define RECEIVE_STD_CHL 0 +#define RECEIVE_EXT_CHL 4 +#define RECEIVE_RTR_CHL 8 +#define RECEIVE_EXT_RTR_CHL 12 +#define MAX_CHL_NUMBER 32 + +/* + * bfin can registers layout + */ +struct bfin_can_mask_regs { + u16 aml; + u16 dummy1; + u16 amh; + u16 dummy2; +}; + +struct bfin_can_channel_regs { + u16 data[8]; + u16 dlc; + u16 dummy1; + u16 tsv; + u16 dummy2; + u16 id0; + u16 dummy3; + u16 id1; + u16 dummy4; +}; + +struct bfin_can_regs { + /* + * global control and status registers + */ + u16 mc1; /* offset 0 */ + u16 dummy1; + u16 md1; /* offset 4 */ + u16 rsv1[13]; + u16 mbtif1; /* offset 0x20 */ + u16 dummy2; + u16 mbrif1; /* offset 0x24 */ + u16 dummy3; + u16 mbim1; /* offset 0x28 */ + u16 rsv2[11]; + u16 mc2; /* offset 0x40 */ + u16 dummy4; + u16 md2; /* offset 0x44 */ + u16 dummy5; + u16 trs2; /* offset 0x48 */ + u16 rsv3[11]; + u16 mbtif2; /* offset 0x60 */ + u16 dummy6; + u16 mbrif2; /* offset 0x64 */ + u16 dummy7; + u16 mbim2; /* offset 0x68 */ + u16 rsv4[11]; + u16 clk; /* offset 0x80 */ + u16 dummy8; + u16 timing; /* offset 0x84 */ + u16 rsv5[3]; + u16 status; /* offset 0x8c */ + u16 dummy9; + u16 cec; /* offset 0x90 */ + u16 dummy10; + u16 gis; /* offset 0x94 */ + u16 dummy11; + u16 gim; /* offset 0x98 */ + u16 rsv6[3]; + u16 ctrl; /* offset 0xa0 */ + u16 dummy12; + u16 intr; /* offset 0xa4 */ + u16 rsv7[7]; + u16 esr; /* offset 0xb4 */ + u16 rsv8[37]; + + /* + * channel(mailbox) mask and message registers + */ + struct bfin_can_mask_regs msk[MAX_CHL_NUMBER]; /* offset 0x100 */ + struct bfin_can_channel_regs chl[MAX_CHL_NUMBER]; /* offset 0x200 */ +}; + +/* + * bfin can private data + */ +struct bfin_can_priv { + struct can_priv can; /* must be the first member */ + struct net_device *dev; + void __iomem *membase; + int rx_irq; + int tx_irq; + int err_irq; + unsigned short *pin_list; +}; + +/* + * bfin can timing parameters + */ +static struct can_bittiming_const bfin_can_bittiming_const = { + .name = DRV_NAME, + .tseg1_min = 1, + .tseg1_max = 16, + .tseg2_min = 1, + .tseg2_max = 8, + .sjw_max = 4, + /* Although the BRP field can be set to any value, it is recommended + * that the value be greater than or equal to 4, as restrictions + * apply to the bit timing configuration when BRP is less than 4. + */ + .brp_min = 4, + .brp_max = 1024, + .brp_inc = 1, +}; + +/* + * inline functions to read/write ID, data length and payload of CAN frame + */ +static inline void bfin_can_write_oid(struct bfin_can_priv *priv, int channel, canid_t id) +{ + struct bfin_can_regs *reg = priv->membase; + + writew((id << 2) + AME, ®->chl[channel].id1); +} + +static inline void bfin_can_write_oid_rtr(struct bfin_can_priv *priv, int channel, canid_t id) +{ + struct bfin_can_regs *reg = priv->membase; + + writew((id << 2) + AME + RTR, ®->chl[channel].id1); +} + +static inline canid_t bfin_can_read_oid(struct bfin_can_priv *priv, int channel) +{ + struct bfin_can_regs *reg = priv->membase; + + return (readw(®->chl[channel].id1) & 0x1ffc) >> 2; +} + +static inline void bfin_can_write_xoid(struct bfin_can_priv *priv, int channel, canid_t id) +{ + struct bfin_can_regs *reg = priv->membase; + + writew(id, ®->chl[channel].id0); + writew(((id & 0x1FFF0000) >> 16) + IDE + AME, ®->chl[channel].id1); +} + +static inline void bfin_can_write_xoid_rtr(struct bfin_can_priv *priv, int channel, canid_t id) +{ + struct bfin_can_regs *reg = priv->membase; + + writew(id, ®->chl[channel].id0); + writew(((id & 0x1FFF0000) >> 16) + IDE + AME + RTR, ®->chl[channel].id1); +} + +static inline canid_t bfin_can_read_xoid(struct bfin_can_priv *priv, int channel) +{ + struct bfin_can_regs *reg = priv->membase; + + return ((readw(®->chl[channel].id1) & 0x1FFF) << 16) + readw(®->chl[channel].id0); +} + +static inline void bfin_can_write_dlc(struct bfin_can_priv *priv, int channel, u8 dlc) +{ + struct bfin_can_regs *reg = priv->membase; + + writew(dlc, ®->chl[channel].dlc); +} + +static inline u8 bfin_can_read_dlc(struct bfin_can_priv *priv, int channel) +{ + struct bfin_can_regs *reg = priv->membase; + + return readw(®->chl[channel].dlc); +} + +static inline void bfin_can_write_data(struct bfin_can_priv *priv, int channel, u8 *data, u8 dlc) +{ + struct bfin_can_regs *reg = priv->membase; + int i; + u16 val; + + for (i = 0; i < 8; i += 2) { + val = ((7 - i) < dlc ? (data[7 - i]) : 0) + + ((6 - i) < dlc ? (data[6 - i] << 8) : 0); + writew(val, ®->chl[channel].data[i]); + } +} + +static inline void bfin_can_read_data(struct bfin_can_priv *priv, int channel, u8 *data, u8 dlc) +{ + struct bfin_can_regs *reg = priv->membase; + int i; + u16 val; + + for (i = 0; i < 8; i += 2) { + val = readw(®->chl[channel].data[i]); + data[7 - i] = (7 - i) < dlc ? val : 0; + data[6 - i] = (6 - i) < dlc ? (val >> 8) : 0; + } +} + +static int bfin_can_set_bittiming(struct net_device *dev) +{ + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + struct can_bittiming *bt = &priv->can.bittiming; + u16 clk, timing; + + clk = bt->brp - 1; + timing = ((bt->sjw - 1) << 8) | (bt->prop_seg + bt->phase_seg1 - 1) | + ((bt->phase_seg2 - 1) << 4); + + /* + * If the SAM bit is set, the input signal is oversampled three times at the SCLK rate. + */ + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) + timing |= SAM; + + writew(clk, ®->clk); + writew(timing, ®->timing); + + dev_info(dev->dev.parent, "setting CLOCK=0x%04x TIMING=0x%04x\n", clk, timing); + return 0; +} + +static void bfin_can_set_reset_mode(struct net_device *dev) +{ + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + int timeout = BFIN_CAN_TIMEOUT; + int i; + + /* disable interrupts */ + writew(0, ®->mbim1); + writew(0, ®->mbim2); + writew(0, ®->gim); + + /* reset can and enter configuration mode */ + writew(SRS | CCR, ®->ctrl); + SSYNC(); + writew(CCR, ®->ctrl); + SSYNC(); + while (!(readw(®->ctrl) & CCA)) { + udelay(10); + if (--timeout == 0) { + dev_err(dev->dev.parent, "fail to enter configuration mode\n"); + BUG(); + } + } + + /* + * All mailbox configurations are marked as inactive + * by writing to CAN Mailbox Configuration Registers 1 and 2 + * For all bits: 0 - Mailbox disabled, 1 - Mailbox enabled + */ + writew(0, ®->mc1); + writew(0, ®->mc2); + + /* Set Mailbox Direction */ + writew(0xFFFF, ®->md1); /* mailbox 1-16 are RX */ + writew(0, ®->md2); /* mailbox 17-32 are TX */ + + /* RECEIVE_STD_CHL */ + for (i = 0; i < 2; i++) { + writew(0, ®->chl[RECEIVE_STD_CHL + i].id0); + writew(AME, ®->chl[RECEIVE_STD_CHL + i].id1); + writew(0, ®->chl[RECEIVE_STD_CHL + i].dlc); + writew(0x1FFF, ®->msk[RECEIVE_STD_CHL + i].amh); + writew(0xFFFF, ®->msk[RECEIVE_STD_CHL + i].aml); + } + + /* RECEIVE_EXT_CHL */ + for (i = 0; i < 2; i++) { + writew(0, ®->chl[RECEIVE_EXT_CHL + i].id0); + writew(AME | IDE, ®->chl[RECEIVE_EXT_CHL + i].id1); + writew(0, ®->chl[RECEIVE_EXT_CHL + i].dlc); + writew(0x1FFF, ®->msk[RECEIVE_EXT_CHL + i].amh); + writew(0xFFFF, ®->msk[RECEIVE_EXT_CHL + i].aml); + } + + writew(BIT(TRANSMIT_CHL - 16), ®->mc2); + writew(BIT(RECEIVE_STD_CHL) + BIT(RECEIVE_EXT_CHL), ®->mc1); + SSYNC(); + + priv->can.state = CAN_STATE_STOPPED; +} + +static void bfin_can_set_normal_mode(struct net_device *dev) +{ + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + int timeout = BFIN_CAN_TIMEOUT; + + /* + * leave configuration mode + */ + writew(readw(®->ctrl) & ~CCR, ®->ctrl); + + while (readw(®->status) & CCA) { + udelay(10); + if (--timeout == 0) { + dev_err(dev->dev.parent, "fail to leave configuration mode\n"); + BUG(); + } + } + + /* + * clear _All_ tx and rx interrupts + */ + writew(0xFFFF, ®->mbtif1); + writew(0xFFFF, ®->mbtif2); + writew(0xFFFF, ®->mbrif1); + writew(0xFFFF, ®->mbrif2); + + /* + * clear global interrupt status register + */ + writew(0x7FF, ®->gis); /* overwrites with '1' */ + + /* + * Initialize Interrupts + * - set bits in the mailbox interrupt mask register + * - global interrupt mask + */ + writew(BIT(RECEIVE_STD_CHL) + BIT(RECEIVE_EXT_CHL), ®->mbim1); + writew(BIT(TRANSMIT_CHL - 16), ®->mbim2); + + writew(EPIM | BOIM | RMLIM, ®->gim); + SSYNC(); +} + +static void bfin_can_start(struct net_device *dev) +{ + struct bfin_can_priv *priv = netdev_priv(dev); + + /* leave reset mode */ + if (priv->can.state != CAN_STATE_STOPPED) + bfin_can_set_reset_mode(dev); + + /* leave reset mode */ + bfin_can_set_normal_mode(dev); +} + +static int bfin_can_set_mode(struct net_device *dev, enum can_mode mode) +{ + switch (mode) { + case CAN_MODE_START: + bfin_can_start(dev); + if (netif_queue_stopped(dev)) + netif_wake_queue(dev); + break; + + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static int bfin_can_start_xmit(struct sk_buff *skb, struct net_device *dev) +{ + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + struct can_frame *cf = (struct can_frame *)skb->data; + u8 dlc; + canid_t id; + + netif_stop_queue(dev); + + dlc = cf->can_dlc; + id = cf->can_id; + + /* fill id and data length code */ + if (id & CAN_EFF_FLAG) { + if (id & CAN_RTR_FLAG) + bfin_can_write_xoid_rtr(priv, TRANSMIT_CHL, id); + else + bfin_can_write_xoid(priv, TRANSMIT_CHL, id); + } else { + if (id & CAN_RTR_FLAG) + bfin_can_write_oid_rtr(priv, TRANSMIT_CHL, id); + else + bfin_can_write_oid(priv, TRANSMIT_CHL, id); + } + + bfin_can_write_data(priv, TRANSMIT_CHL, cf->data, dlc); + + bfin_can_write_dlc(priv, TRANSMIT_CHL, dlc); + + dev->trans_start = jiffies; + + can_put_echo_skb(skb, dev, 0); + + /* set transmit request */ + writew(BIT(TRANSMIT_CHL - 16), ®->trs2); + return 0; +} + +static void bfin_can_rx(struct net_device *dev, u16 isrc) +{ + struct bfin_can_priv *priv = netdev_priv(dev); + struct net_device_stats *stats = &dev->stats; + struct bfin_can_regs *reg = priv->membase; + struct can_frame *cf; + struct sk_buff *skb; + canid_t id; + u8 dlc; + int obj; + + skb = alloc_can_skb(dev, &cf); + if (skb == NULL) + return; + + /* get id and data length code */ + if (isrc & BIT(RECEIVE_EXT_CHL)) { + /* extended frame format (EFF) */ + id = bfin_can_read_xoid(priv, RECEIVE_EXT_CHL); + id |= CAN_EFF_FLAG; + obj = RECEIVE_EXT_CHL; + } else { + /* standard frame format (SFF) */ + id = bfin_can_read_oid(priv, RECEIVE_STD_CHL); + obj = RECEIVE_STD_CHL; + } + if (readw(®->chl[obj].id1) & RTR) + id |= CAN_RTR_FLAG; + dlc = bfin_can_read_dlc(priv, obj); + + cf->can_id = id; + cf->can_dlc = dlc; + + bfin_can_read_data(priv, obj, cf->data, dlc); + + netif_rx(skb); + + stats->rx_packets++; + stats->rx_bytes += dlc; +} + +static int bfin_can_err(struct net_device *dev, u16 isrc, u16 status) +{ + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + struct net_device_stats *stats = &dev->stats; + struct can_frame *cf; + struct sk_buff *skb; + enum can_state state = priv->can.state; + + skb = alloc_can_err_skb(dev, &cf); + if (skb == NULL) + return -ENOMEM; + + if (isrc & RMLIS) { + /* data overrun interrupt */ + dev_dbg(dev->dev.parent, "data overrun interrupt\n"); + cf->can_id |= CAN_ERR_CRTL; + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; + stats->rx_over_errors++; + stats->rx_errors++; + } + + if (isrc & BOIS) { + dev_dbg(dev->dev.parent, "bus-off mode interrupt\n"); + + state = CAN_STATE_BUS_OFF; + cf->can_id |= CAN_ERR_BUSOFF; + can_bus_off(dev); + } + + if (isrc & EPIS) { + /* error passive interrupt */ + dev_dbg(dev->dev.parent, "error passive interrupt\n"); + state = CAN_STATE_ERROR_PASSIVE; + } + + if ((isrc & EWTIS) || (isrc & EWRIS)) { + dev_dbg(dev->dev.parent, "Error Warning Transmit/Receive Interrupt\n"); + state = CAN_STATE_ERROR_WARNING; + } + + if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING || + state == CAN_STATE_ERROR_PASSIVE)) { + u16 cec = readw(®->cec); + u8 rxerr = cec; + u8 txerr = cec >> 8; + cf->can_id |= CAN_ERR_CRTL; + if (state == CAN_STATE_ERROR_WARNING) { + priv->can.can_stats.error_warning++; + cf->data[1] = (txerr > rxerr) ? + CAN_ERR_CRTL_TX_WARNING : + CAN_ERR_CRTL_RX_WARNING; + } else { + priv->can.can_stats.error_passive++; + cf->data[1] = (txerr > rxerr) ? + CAN_ERR_CRTL_TX_PASSIVE : + CAN_ERR_CRTL_RX_PASSIVE; + } + } + + if (status) { + priv->can.can_stats.bus_error++; + + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + + if (status & BEF) + cf->data[2] |= CAN_ERR_PROT_BIT; + else if (status & FER) + cf->data[2] |= CAN_ERR_PROT_FORM; + else if (status & SER) + cf->data[2] |= CAN_ERR_PROT_STUFF; + else + cf->data[2] |= CAN_ERR_PROT_UNSPEC; + } + + priv->can.state = state; + + netif_rx(skb); + + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; + + return 0; +} + +irqreturn_t bfin_can_interrupt(int irq, void *dev_id) +{ + struct net_device *dev = dev_id; + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + struct net_device_stats *stats = &dev->stats; + u16 status, isrc; + + if ((irq == priv->tx_irq) && readw(®->mbtif2)) { + /* transmission complete interrupt */ + writew(0xFFFF, ®->mbtif2); + stats->tx_packets++; + stats->tx_bytes += bfin_can_read_dlc(priv, TRANSMIT_CHL); + can_get_echo_skb(dev, 0); + netif_wake_queue(dev); + } else if ((irq == priv->rx_irq) && readw(®->mbrif1)) { + /* receive interrupt */ + isrc = readw(®->mbrif1); + writew(0xFFFF, ®->mbrif1); + bfin_can_rx(dev, isrc); + } else if ((irq == priv->err_irq) && readw(®->gis)) { + /* error interrupt */ + isrc = readw(®->gis); + status = readw(®->esr); + writew(0x7FF, ®->gis); + bfin_can_err(dev, isrc, status); + } else + return IRQ_NONE; + + return IRQ_HANDLED; +} + +static int bfin_can_open(struct net_device *dev) +{ + int err; + + /* set chip into reset mode */ + bfin_can_set_reset_mode(dev); + + /* common open */ + err = open_candev(dev); + if (err) + return err; + + /* init and start chi */ + bfin_can_start(dev); + + netif_start_queue(dev); + + return 0; +} + +static int bfin_can_close(struct net_device *dev) +{ + netif_stop_queue(dev); + bfin_can_set_reset_mode(dev); + + close_candev(dev); + + return 0; +} + +struct net_device *alloc_bfin_candev(void) +{ + struct net_device *dev; + struct bfin_can_priv *priv; + + dev = alloc_candev(sizeof(*priv)); + if (!dev) + return NULL; + + priv = netdev_priv(dev); + + priv->dev = dev; + priv->can.bittiming_const = &bfin_can_bittiming_const; + priv->can.do_set_bittiming = bfin_can_set_bittiming; + priv->can.do_set_mode = bfin_can_set_mode; + + return dev; +} + +static const struct net_device_ops bfin_can_netdev_ops = { + .ndo_open = bfin_can_open, + .ndo_stop = bfin_can_close, + .ndo_start_xmit = bfin_can_start_xmit, +}; + +static int __devinit bfin_can_probe(struct platform_device *pdev) +{ + int err; + struct net_device *dev; + struct bfin_can_priv *priv; + struct resource *res_mem, *rx_irq, *tx_irq, *err_irq; + unsigned short *pdata; + + pdata = pdev->dev.platform_data; + if (!pdata) { + dev_err(&pdev->dev, "No platform data provided!\n"); + err = -EINVAL; + goto exit; + } + + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + rx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + tx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 1); + err_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 2); + if (!res_mem || !rx_irq || !tx_irq || !err_irq) { + err = -EINVAL; + goto exit; + } + + if (!request_mem_region(res_mem->start, resource_size(res_mem), + dev_name(&pdev->dev))) { + err = -EBUSY; + goto exit; + } + + /* request peripheral pins */ + err = peripheral_request_list(pdata, dev_name(&pdev->dev)); + if (err) + goto exit_mem_release; + + dev = alloc_bfin_candev(); + if (!dev) { + err = -ENOMEM; + goto exit_peri_pin_free; + } + + /* register interrupt handler */ + err = request_irq(rx_irq->start, &bfin_can_interrupt, 0, + "bfin-can-rx", (void *)dev); + if (err) + goto exit_candev_free; + err = request_irq(tx_irq->start, &bfin_can_interrupt, 0, + "bfin-can-tx", (void *)dev); + if (err) + goto exit_rx_irq_free; + err = request_irq(err_irq->start, &bfin_can_interrupt, 0, + "bfin-can-err", (void *)dev); + if (err) + goto exit_tx_irq_free; + + priv = netdev_priv(dev); + priv->membase = (void __iomem *)res_mem->start; + priv->rx_irq = rx_irq->start; + priv->tx_irq = tx_irq->start; + priv->err_irq = err_irq->start; + priv->pin_list = pdata; + priv->can.clock.freq = get_sclk(); + + dev_set_drvdata(&pdev->dev, dev); + SET_NETDEV_DEV(dev, &pdev->dev); + + dev->flags |= IFF_ECHO; /* we support local echo */ + dev->netdev_ops = &bfin_can_netdev_ops; + + bfin_can_set_reset_mode(dev); + + err = register_candev(dev); + if (err) { + dev_err(&pdev->dev, "registering failed (err=%d)\n", err); + goto exit_err_irq_free; + } + + dev_info(&pdev->dev, "%s device registered (reg_base=%p, rx_irq=%d, tx_irq=%d, err_irq=%d, sclk=%d)\n", + DRV_NAME, (void *)priv->membase, priv->rx_irq, priv->tx_irq, priv->err_irq, + priv->can.clock.freq); + return 0; + +exit_err_irq_free: + free_irq(err_irq->start, dev); +exit_tx_irq_free: + free_irq(tx_irq->start, dev); +exit_rx_irq_free: + free_irq(rx_irq->start, dev); +exit_candev_free: + free_candev(dev); +exit_peri_pin_free: + peripheral_free_list(pdata); +exit_mem_release: + release_mem_region(res_mem->start, resource_size(res_mem)); +exit: + return err; +} + +static int __devexit bfin_can_remove(struct platform_device *pdev) +{ + struct net_device *dev = dev_get_drvdata(&pdev->dev); + struct bfin_can_priv *priv = netdev_priv(dev); + struct resource *res; + + bfin_can_set_reset_mode(dev); + + unregister_candev(dev); + + dev_set_drvdata(&pdev->dev, NULL); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + release_mem_region(res->start, resource_size(res)); + + free_irq(priv->rx_irq, dev); + free_irq(priv->tx_irq, dev); + free_irq(priv->err_irq, dev); + peripheral_free_list(priv->pin_list); + + free_candev(dev); + return 0; +} + +#ifdef CONFIG_PM +static int bfin_can_suspend(struct platform_device *pdev, pm_message_t mesg) +{ + struct net_device *dev = dev_get_drvdata(&pdev->dev); + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + int timeout = BFIN_CAN_TIMEOUT; + + if (netif_running(dev)) { + /* enter sleep mode */ + writew(readw(®->ctrl) | SMR, ®->ctrl); + SSYNC(); + while (!(readw(®->intr) & SMACK)) { + udelay(10); + if (--timeout == 0) { + dev_err(dev->dev.parent, "fail to enter sleep mode\n"); + BUG(); + } + } + } + + return 0; +} + +static int bfin_can_resume(struct platform_device *pdev) +{ + struct net_device *dev = dev_get_drvdata(&pdev->dev); + struct bfin_can_priv *priv = netdev_priv(dev); + struct bfin_can_regs *reg = priv->membase; + + if (netif_running(dev)) { + /* leave sleep mode */ + writew(0, ®->intr); + SSYNC(); + } + + return 0; +} +#else +#define bfin_can_suspend NULL +#define bfin_can_resume NULL +#endif /* CONFIG_PM */ + +static struct platform_driver bfin_can_driver = { + .probe = bfin_can_probe, + .remove = __devexit_p(bfin_can_remove), + .suspend = bfin_can_suspend, + .resume = bfin_can_resume, + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + }, +}; + +static int __init bfin_can_init(void) +{ + return platform_driver_register(&bfin_can_driver); +} + +module_init(bfin_can_init); + +static void __exit bfin_can_exit(void) +{ + platform_driver_unregister(&bfin_can_driver); +} + +module_exit(bfin_can_exit); + +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>"); +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Blackfin on-chip CAN netdevice driver");