| Message ID | 20160726091555.GA26227@awelinux |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2016/07/26 11:16, Andreas Werner wrote: [...] > + > + /* Lock for CTL_BTR register access. > + * This register combines bittiming bits > + * and the operation mode bits. > + * It is also used for bit r/m/w access > + * to all registers. > + */ > + spinlock_t lock; Why not use 80 cols for comments? [...] > + > +static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct can_frame *cf = (struct can_frame *)skb->data; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + struct net_device_stats *stats = &ndev->stats; > + struct men_z192_cf_buf __iomem *cf_buf; > + u32 data[2] = {0, 0}; > + int status; > + u32 id; > + > + if (can_dropped_invalid_skb(ndev, skb)) > + return NETDEV_TX_OK; > + > + status = readl(®s->rx_tx_sts); > + > + if (MEN_Z192_TX_BUF_CNT(status) >= 255) { > + netif_stop_queue(ndev); > + netdev_err(ndev, "not enough space in TX buffer\n"); > + > + return NETDEV_TX_BUSY; > + } > + > + cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START; > + > + if (cf->can_id & CAN_EFF_FLAG) { > + /* Extended frame */ > + id = ((cf->can_id & CAN_EFF_MASK) << > + MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2; > + > + id |= (((cf->can_id & CAN_EFF_MASK) >> > + (CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) << > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > + > + id |= MEN_Z192_CFBUF_IDE; > + id |= MEN_Z192_CFBUF_SRR; > + > + if (cf->can_id & CAN_RTR_FLAG) > + id |= MEN_Z192_CFBUF_E_RTR; > + } else { > + /* Standard frame */ > + id = ((cf->can_id & CAN_SFF_MASK) << > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > + > + if (cf->can_id & CAN_RTR_FLAG) > + id |= MEN_Z192_CFBUF_S_RTR; > + } > + > + if (cf->can_dlc > 0) > + data[0] = be32_to_cpup((__be32 *)(cf->data)); > + if (cf->can_dlc > 3) > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); > + > + writel(id, &cf_buf->can_id); > + writel(cf->can_dlc, &cf_buf->length); > + > + if (!(cf->can_id & CAN_RTR_FLAG)) { > + writel(data[0], &cf_buf->data[0]); > + writel(data[1], &cf_buf->data[1]); > + > + stats->tx_bytes += cf->can_dlc; > + } > + > + /* be sure everything is written to the > + * device before acknowledge the data. > + */ > + mmiowb(); > + > + /* trigger the transmission */ > + men_z192_ack_tx_pkg(priv, 1); > + > + stats->tx_packets++; > + > + kfree_skb(skb); What prevents the skb data to be freed/reused before the device has accessed it? [...] > + > +static int men_z192_probe(struct mcb_device *mdev, > + const struct mcb_device_id *id) > +{ > + struct device *dev = &mdev->dev; > + struct men_z192 *priv; > + struct net_device *ndev; > + void __iomem *dev_base; > + struct resource *mem; > + u32 timebase; > + int ret = 0; > + int irq; > + > + mem = mcb_request_mem(mdev, dev_name(dev)); > + if (IS_ERR(mem)) { > + dev_err(dev, "failed to request device memory"); > + return PTR_ERR(mem); > + } > + > + dev_base = ioremap(mem->start, resource_size(mem)); > + if (!dev_base) { > + dev_err(dev, "failed to ioremap device memory"); > + ret = -ENXIO; > + goto out_release; > + } > + > + irq = mcb_get_irq(mdev); > + if (irq <= 0) { > + ret = -ENODEV; > + goto out_unmap; > + } > + > + ndev = alloc_candev(sizeof(struct men_z192), 1); > + if (!ndev) { > + dev_err(dev, "failed to allocate the can device"); > + ret = -ENOMEM; > + goto out_unmap; > + } > + > + ndev->netdev_ops = &men_z192_netdev_ops; > + ndev->irq = irq; > + > + priv = netdev_priv(ndev); > + priv->ndev = ndev; > + priv->dev = dev; > + > + priv->mem = mem; > + priv->dev_base = dev_base; > + priv->regs = priv->dev_base + MEN_Z192_REGS_OFFS; > + > + timebase = readl(&priv->regs->timebase); > + if (!timebase) { > + dev_err(dev, "invalid timebase configured (timebase=%d)\n", > + timebase); > + ret = -EINVAL; > + goto out_unmap; free_candev is missing in this error path > + } > + > + priv->can.clock.freq = timebase; > + priv->can.bittiming_const = &men_z192_bittiming_const; > + priv->can.do_set_mode = men_z192_set_mode; > + priv->can.do_get_berr_counter = men_z192_get_berr_counter; > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY | > + CAN_CTRLMODE_3_SAMPLES | > + CAN_CTRLMODE_LOOPBACK; > + > + spin_lock_init(&priv->lock); > + > + netif_napi_add(ndev, &priv->napi, men_z192_poll, > + MEN_Z192_NAPI_WEIGHT); > + > + mcb_set_drvdata(mdev, ndev); > + SET_NETDEV_DEV(ndev, dev); > + > + ret = men_z192_register(ndev); > + if (ret) { > + dev_err(dev, "failed to register CAN device"); > + goto out_free_candev; > + } > + > + dev_info(dev, "MEN 16z192 CAN driver successfully registered\n"); > + > + return 0; > + > +out_free_candev: > + netif_napi_del(&priv->napi); > + free_candev(ndev); > +out_unmap: > + iounmap(dev_base); > +out_release: > + mcb_release_mem(mem); > + return ret; > +} > +
On Sun, Aug 07, 2016 at 08:58:14PM -0700, Benjamin Poirier wrote: > On 2016/07/26 11:16, Andreas Werner wrote: > [...] > > + > > + /* Lock for CTL_BTR register access. > > + * This register combines bittiming bits > > + * and the operation mode bits. > > + * It is also used for bit r/m/w access > > + * to all registers. > > + */ > > + spinlock_t lock; > > Why not use 80 cols for comments? > Yes you are right, will changed that. > [...] > > + > > +static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev) > > +{ > > + struct can_frame *cf = (struct can_frame *)skb->data; > > + struct men_z192 *priv = netdev_priv(ndev); > > + struct men_z192_regs __iomem *regs = priv->regs; > > + struct net_device_stats *stats = &ndev->stats; > > + struct men_z192_cf_buf __iomem *cf_buf; > > + u32 data[2] = {0, 0}; > > + int status; > > + u32 id; > > + > > + if (can_dropped_invalid_skb(ndev, skb)) > > + return NETDEV_TX_OK; > > + > > + status = readl(®s->rx_tx_sts); > > + > > + if (MEN_Z192_TX_BUF_CNT(status) >= 255) { > > + netif_stop_queue(ndev); > > + netdev_err(ndev, "not enough space in TX buffer\n"); > > + > > + return NETDEV_TX_BUSY; > > + } > > + > > + cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START; > > + > > + if (cf->can_id & CAN_EFF_FLAG) { > > + /* Extended frame */ > > + id = ((cf->can_id & CAN_EFF_MASK) << > > + MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2; > > + > > + id |= (((cf->can_id & CAN_EFF_MASK) >> > > + (CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) << > > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > > + > > + id |= MEN_Z192_CFBUF_IDE; > > + id |= MEN_Z192_CFBUF_SRR; > > + > > + if (cf->can_id & CAN_RTR_FLAG) > > + id |= MEN_Z192_CFBUF_E_RTR; > > + } else { > > + /* Standard frame */ > > + id = ((cf->can_id & CAN_SFF_MASK) << > > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > > + > > + if (cf->can_id & CAN_RTR_FLAG) > > + id |= MEN_Z192_CFBUF_S_RTR; > > + } > > + > > + if (cf->can_dlc > 0) > > + data[0] = be32_to_cpup((__be32 *)(cf->data)); > > + if (cf->can_dlc > 3) > > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); > > + > > + writel(id, &cf_buf->can_id); > > + writel(cf->can_dlc, &cf_buf->length); > > + > > + if (!(cf->can_id & CAN_RTR_FLAG)) { > > + writel(data[0], &cf_buf->data[0]); > > + writel(data[1], &cf_buf->data[1]); > > + > > + stats->tx_bytes += cf->can_dlc; > > + } > > + > > + /* be sure everything is written to the > > + * device before acknowledge the data. > > + */ > > + mmiowb(); > > + > > + /* trigger the transmission */ > > + men_z192_ack_tx_pkg(priv, 1); > > + > > + stats->tx_packets++; > > + > > + kfree_skb(skb); > > What prevents the skb data to be freed/reused before the device has > accessed it? > I am not sure if I undestand it correctly. Do you mean to free the skb right before the mmiowb? If thats the case, I agree with you. > [...] > > + > > +static int men_z192_probe(struct mcb_device *mdev, > > + const struct mcb_device_id *id) > > +{ > > + struct device *dev = &mdev->dev; > > + struct men_z192 *priv; > > + struct net_device *ndev; > > + void __iomem *dev_base; > > + struct resource *mem; > > + u32 timebase; > > + int ret = 0; > > + int irq; > > + > > + mem = mcb_request_mem(mdev, dev_name(dev)); > > + if (IS_ERR(mem)) { > > + dev_err(dev, "failed to request device memory"); > > + return PTR_ERR(mem); > > + } > > + > > + dev_base = ioremap(mem->start, resource_size(mem)); > > + if (!dev_base) { > > + dev_err(dev, "failed to ioremap device memory"); > > + ret = -ENXIO; > > + goto out_release; > > + } > > + > > + irq = mcb_get_irq(mdev); > > + if (irq <= 0) { > > + ret = -ENODEV; > > + goto out_unmap; > > + } > > + > > + ndev = alloc_candev(sizeof(struct men_z192), 1); > > + if (!ndev) { > > + dev_err(dev, "failed to allocate the can device"); > > + ret = -ENOMEM; > > + goto out_unmap; > > + } > > + > > + ndev->netdev_ops = &men_z192_netdev_ops; > > + ndev->irq = irq; > > + > > + priv = netdev_priv(ndev); > > + priv->ndev = ndev; > > + priv->dev = dev; > > + > > + priv->mem = mem; > > + priv->dev_base = dev_base; > > + priv->regs = priv->dev_base + MEN_Z192_REGS_OFFS; > > + > > + timebase = readl(&priv->regs->timebase); > > + if (!timebase) { > > + dev_err(dev, "invalid timebase configured (timebase=%d)\n", > > + timebase); > > + ret = -EINVAL; > > + goto out_unmap; > > free_candev is missing in this error path argh, yes. Will change that. [...] Thanks for your review Benjamin. I will wait for other comments until I send a v2. Regards Andy
Hello Andreas, a first quick review.... Am 26.07.2016 um 11:16 schrieb Andreas Werner: > This CAN Controller is found on MEN Chameleon FPGAs. > > The driver/device supports the CAN2.0 specification. > There are 255 RX and 255 Tx buffer within the IP. The > pointer for the buffer are handled by HW to make the > access from within the driver as simple as possible. > > The driver also supports parameters to configure the > buffer level interrupt for RX/TX as well as a RX timeout > interrupt. > > With this configuration options, the driver/device > provides flexibility for different types of usecases. > > Signed-off-by: Andreas Werner <andreas.werner@men.de> > --- > drivers/net/can/Kconfig | 10 + > drivers/net/can/Makefile | 1 + > drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 1000 insertions(+) > create mode 100644 drivers/net/can/men_z192_can.c > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index 0d40aef..0fa0387 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -104,6 +104,16 @@ config CAN_JANZ_ICAN3 > This driver can also be built as a module. If so, the module will be > called janz-ican3.ko. > > +config CAN_MEN_Z192 > + tristate "MEN 16Z192-00 CAN Controller" > + depends on MCB > + ---help--- > + Driver for MEN 16Z192-00 CAN Controller IP-Core, which > + is connected to the MEN Chameleon Bus. > + > + This driver can also be built as a module. If so, the module will be > + called men_z192_can.ko. > + > config CAN_RCAR > tristate "Renesas R-Car CAN controller" > depends on ARCH_RENESAS || ARM > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > index e3db0c8..eb206b3 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o > obj-$(CONFIG_CAN_GRCAN) += grcan.o > obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ > obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o > +obj-$(CONFIG_CAN_MEN_Z192) += men_z192_can.o > obj-$(CONFIG_CAN_MSCAN) += mscan/ > obj-$(CONFIG_CAN_M_CAN) += m_can/ > obj-$(CONFIG_CAN_RCAR) += rcar_can.o > diff --git a/drivers/net/can/men_z192_can.c b/drivers/net/can/men_z192_can.c > new file mode 100644 > index 0000000..b39ffee > --- /dev/null > +++ b/drivers/net/can/men_z192_can.c > @@ -0,0 +1,989 @@ > +/* > + * MEN 16Z192 CAN Controller driver > + * > + * Copyright (C) 2016 MEN Mikroelektronik GmbH (www.men.de) > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation; version 2 of the License. > + */ > + > +#include <linux/netdevice.h> > +#include <linux/can/error.h> > +#include <linux/can/dev.h> > +#include <linux/bitops.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/mcb.h> > +#include <linux/can.h> > +#include <linux/io.h> > + > +#define DRV_NAME "z192_can" > + > +#define MEN_Z192_NAPI_WEIGHT 64 > +#define MEN_Z192_MODE_TOUT_US 40 > + > +/* CTL/BTR Register Bits */ > +#define MEN_Z192_CTL0_INITRQ BIT(0) > +#define MEN_Z192_CTL0_SLPRQ BIT(1) > +#define MEN_Z192_CTL1_INITAK BIT(8) > +#define MEN_Z192_CTL1_SLPAK BIT(9) > +#define MEN_Z192_CTL1_LISTEN BIT(12) > +#define MEN_Z192_CTL1_LOOPB BIT(13) > +#define MEN_Z192_CTL1_CANE BIT(15) > +#define MEN_Z192_BTR0_BRP(x) (((x) & 0x3f) << 16) > +#define MEN_Z192_BTR0_SJW(x) (((x) & 0x03) << 22) > +#define MEN_Z192_BTR1_TSEG1(x) (((x) & 0x0f) << 24) > +#define MEN_Z192_BTR1_TSEG2(x) (((x) & 0x07) << 28) > +#define MEN_Z192_BTR1_SAMP BIT(31) > + > +/* IER Interrupt Enable Register bits */ > +#define MEN_Z192_RXIE BIT(0) > +#define MEN_Z192_OVRIE BIT(1) > +#define MEN_Z192_CSCIE BIT(6) > +#define MEN_Z192_TOUTE BIT(7) > +#define MEN_Z192_TXIE BIT(16) > +#define MEN_Z192_ERRIE BIT(17) > + > +#define MEN_Z192_IRQ_ALL \ > + (MEN_Z192_RXIE | MEN_Z192_OVRIE | \ > + MEN_Z192_CSCIE | MEN_Z192_TOUTE | \ > + MEN_Z192_TXIE) > + > +#define MEN_Z192_IRQ_NAPI (MEN_Z192_RXIE | MEN_Z192_TOUTE) > + > +/* RX_TX_STAT RX/TX Status status register bits */ > +#define MEN_Z192_RX_BUF_CNT(x) ((x) & 0xff) > +#define MEN_Z192_TX_BUF_CNT(x) (((x) & 0xff00) >> 8) > +#define MEN_Z192_RFLG_RXIF BIT(16) > +#define MEN_Z192_RFLG_OVRF BIT(17) > +#define MEN_Z192_RFLG_TSTATE GENMASK(19, 18) > +#define MEN_Z192_RFLG_RSTATE GENMASK(21, 20) > +#define MEN_Z192_RFLG_CSCIF BIT(22) > +#define MEN_Z192_RFLG_TOUTF BIT(23) > +#define MEN_Z192_TFLG_TXIF BIT(24) > + > +#define MEN_Z192_GET_TSTATE(x) (((x) & MEN_Z192_RFLG_TSTATE) >> 18) > +#define MEN_Z192_GET_RSTATE(x) (((x) & MEN_Z192_RFLG_RSTATE) >> 20) > + > +#define MEN_Z192_IRQ_FLAGS_ALL \ > + (MEN_Z192_RFLG_RXIF | MEN_Z192_RFLG_OVRF | \ > + MEN_Z192_RFLG_TSTATE | MEN_Z192_RFLG_RSTATE | \ > + MEN_Z192_RFLG_CSCIF | MEN_Z192_RFLG_TOUTF | \ > + MEN_Z192_TFLG_TXIF) > + > +/* RX/TX Error counter bits */ > +#define MEN_Z192_GET_RX_ERR_CNT(x) ((x) & 0xff) > +#define MEN_Z192_GET_TX_ERR_CNT(x) (((x) & 0x00ff0000) >> 16) > + > +/* Buffer level register bits */ > +#define MEN_Z192_RX_BUF_LVL GENMASK(15, 0) > +#define MEN_Z192_TX_BUF_LVL GENMASK(31, 16) > + > +/* RX/TX Buffer register bits */ > +#define MEN_Z192_CFBUF_LEN GENMASK(3, 0) > +#define MEN_Z192_CFBUF_ID1 GENMASK(31, 21) > +#define MEN_Z192_CFBUF_ID2 GENMASK(18, 1) > +#define MEN_Z192_CFBUF_TS GENMASK(31, 8) > +#define MEN_Z192_CFBUF_E_RTR BIT(0) > +#define MEN_Z192_CFBUF_IDE BIT(19) > +#define MEN_Z192_CFBUF_SRR BIT(20) > +#define MEN_Z192_CFBUF_S_RTR BIT(20) > +#define MEN_Z192_CFBUF_ID2_SHIFT 1 > +#define MEN_Z192_CFBUF_ID1_SHIFT 21 > + > +/* Global register offsets */ > +#define MEN_Z192_RX_BUF_START 0x0000 > +#define MEN_Z192_TX_BUF_START 0x1000 > +#define MEN_Z192_REGS_OFFS 0x2000 > + > +/* Buffer level control values */ > +#define MEN_Z192_MIN_BUF_LVL 0 > +#define MEN_Z192_MAX_BUF_LVL 254 > +#define MEN_Z192_RX_BUF_LVL_DEF 5 > +#define MEN_Z192_TX_BUF_LVL_DEF 5 > +#define MEN_Z192_RX_TOUT_MIN 0 > +#define MEN_Z192_RX_TOUT_MAX 65535 > +#define MEN_Z192_RX_TOUT_DEF 1000 > + > +static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; > +module_param(txlvl, int, S_IRUGO); > +MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" > + __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); > + > +static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; > +module_param(rxlvl, int, S_IRUGO); > +MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" > + __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); > + What impact does the level have on the latency? Could you please add some comments. > +static int rx_timeout = MEN_Z192_RX_TOUT_DEF; > +module_param(rx_timeout, int, S_IRUGO); > +MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" > + __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); Ditto. What is "rx_timeout" good for. > + > +struct men_z192_regs { > + u32 ctl_btr; /* Control and bus timing register */ > + u32 ier; /* Interrupt enable register */ > + u32 buf_lvl; /* Buffer level register */ > + u32 rxa; /* RX Data acknowledge register */ > + u32 txa; /* TX data acknowledge register */ > + u32 rx_tx_sts; /* RX/TX flags and buffer level */ > + u32 ovr_ecc_sts; /* Overrun/ECC status register */ > + u32 idac_ver; /* ID acceptance control / version */ > + u32 rx_tx_err; /* RX/TX error counter register */ > + u32 idar_0_to_3; /* ID acceptance register 0...3 */ > + u32 idar_4_to_7; /* ID acceptance register 4...7 */ > + u32 idmr_0_to_3; /* ID mask register 0...3 */ > + u32 idmr_4_to_7; /* ID mask register 4...7 */ > + u32 rx_timeout; /* receive timeout */ > + u32 timebase; /* Base frequency for baudrate calculation */ > +}; > + > +struct men_z192 { > + struct can_priv can; > + struct napi_struct napi; > + struct net_device *ndev; > + struct device *dev; > + > + /* Lock for CTL_BTR register access. > + * This register combines bittiming bits > + * and the operation mode bits. > + * It is also used for bit r/m/w access > + * to all registers. > + */ > + spinlock_t lock; > + struct resource *mem; > + struct men_z192_regs __iomem *regs; > + void __iomem *dev_base; > +}; > + > +struct men_z192_cf_buf { > + u32 can_id; > + u32 data[2]; > + u32 length; > +}; > + > +enum men_z192_int_state { > + MEN_Z192_CAN_DIS = 0, > + MEN_Z192_CAN_EN, > + MEN_Z192_CAN_NAPI_DIS, > + MEN_Z192_CAN_NAPI_EN, > +}; > + > +static enum can_state bus_state_map[] = { > + CAN_STATE_ERROR_ACTIVE, > + CAN_STATE_ERROR_WARNING, > + CAN_STATE_ERROR_PASSIVE, > + CAN_STATE_BUS_OFF > +}; > + > +static const struct can_bittiming_const men_z192_bittiming_const = { > + .name = DRV_NAME, > + .tseg1_min = 4, > + .tseg1_max = 16, > + .tseg2_min = 2, > + .tseg2_max = 8, > + .sjw_max = 4, > + .brp_min = 2, > + .brp_max = 64, > + .brp_inc = 1, > +}; > + > +static inline void men_z192_bit_clr(struct men_z192 *priv, void __iomem *addr, > + u32 mask) > +{ > + unsigned long flags; > + u32 val; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + val = readl(addr); > + val &= ~mask; > + writel(val, addr); > + > + spin_unlock_irqrestore(&priv->lock, flags); > +} > + > +static inline void men_z192_bit_set(struct men_z192 *priv, void __iomem *addr, > + u32 mask) > +{ > + unsigned long flags; > + u32 val; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + val = readl(addr); > + val |= mask; > + writel(val, addr); > + > + spin_unlock_irqrestore(&priv->lock, flags); > +} > + > +static inline void men_z192_ack_rx_pkg(struct men_z192 *priv, > + unsigned int count) > +{ > + writel(count, &priv->regs->rxa); > +} > + > +static inline void men_z192_ack_tx_pkg(struct men_z192 *priv, > + unsigned int count) > +{ > + writel(count, &priv->regs->txa); > +} > + > +static void men_z192_set_int(struct men_z192 *priv, > + enum men_z192_int_state state) > +{ > + struct men_z192_regs __iomem *regs = priv->regs; > + > + switch (state) { > + case MEN_Z192_CAN_DIS: > + men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_ALL); > + break; > + > + case MEN_Z192_CAN_EN: > + men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_ALL); > + break; > + > + case MEN_Z192_CAN_NAPI_DIS: > + men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_NAPI); > + break; > + > + case MEN_Z192_CAN_NAPI_EN: > + men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_NAPI); > + break; > + > + default: > + netdev_err(priv->ndev, "invalid interrupt state\n"); > + break; > + } > +} > + > +static int men_z192_get_berr_counter(const struct net_device *ndev, > + struct can_berr_counter *bec) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + u32 err_cnt; > + > + err_cnt = readl(®s->rx_tx_err); > + > + bec->txerr = MEN_Z192_GET_TX_ERR_CNT(err_cnt); > + bec->rxerr = MEN_Z192_GET_RX_ERR_CNT(err_cnt); > + > + return 0; > +} > + > +static int men_z192_req_run_mode(struct men_z192 *priv) > +{ > + unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; > + struct men_z192_regs __iomem *regs = priv->regs;get > + u32 val; > + > + men_z192_bit_clr(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); > + > + while (timeout--) { > + val = readl(®s->ctl_btr); > + if (!(val & MEN_Z192_CTL1_INITAK)) > + break; > + > + udelay(10); > + } > + > + if (val & MEN_Z192_CTL1_INITAK) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int men_z192_req_init_mode(struct men_z192 *priv) > +{ > + unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; > + struct men_z192_regs __iomem *regs = priv->regs; > + u32 val; > + > + men_z192_bit_set(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); > + > + while (timeout--) { > + val = readl(®s->ctl_btr); > + if (val & MEN_Z192_CTL1_INITAK) > + break; > + > + udelay(10); > + } > + > + if (!(val & MEN_Z192_CTL1_INITAK)) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr) > +{ > + struct net_device_stats *stats = &ndev->stats; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_cf_buf __iomem *cf_buf; > + struct can_frame *cf; > + struct sk_buff *skb; > + u32 cf_offset; > + u32 length; > + u32 data; > + u32 id; > + > + skb = alloc_can_skb(ndev, &cf); > + if (unlikely(!skb)) { > + stats->rx_dropped++; > + return 0; > + } > + > + cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; > + > + cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; > + length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; > + id = readl(&cf_buf->can_id); > + > + if (id & MEN_Z192_CFBUF_IDE) { > + /* Extended frame */ > + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; > + cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> > + MEN_Z192_CFBUF_ID2_SHIFT; > + > + cf->can_id |= CAN_EFF_FLAG; > + > + if (id & MEN_Z192_CFBUF_E_RTR) > + cf->can_id |= CAN_RTR_FLAG; > + } else { > + /* Standard frame */ > + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> > + MEN_Z192_CFBUF_ID1_SHIFT; > + > + if (id & MEN_Z192_CFBUF_S_RTR) > + cf->can_id |= CAN_RTR_FLAG; > + } > + > + cf->can_dlc = get_can_dlc(length); > + > + /* remote transmission request frame > + * contains no data field even if the > + * data length is set to a value > 0 > + */ > + if (!(cf->can_id & CAN_RTR_FLAG)) { > + if (cf->can_dlc > 0) { > + data = readl(&cf_buf->data[0]); > + *(__be32 *)cf->data = cpu_to_be32(data); Do you really need the extra copy? > + } > + if (cf->can_dlc > 4) { > + data = readl(&cf_buf->data[1]); > + *(__be32 *)(cf->data + 4) = cpu_to_be32(data); Ditto. > + } > + } > + > + stats->rx_bytes += cf->can_dlc; > + stats->rx_packets++; > + netif_receive_skb(skb); > + > + return 1; > +} > + > +static int men_z192_poll(struct napi_struct *napi, int quota) > +{ > + struct net_device *ndev = napi->dev; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + int work_done = 0; > + u32 frame_cnt; > + u32 status; > + > + status = readl(®s->rx_tx_sts); > + > + frame_cnt = MEN_Z192_RX_BUF_CNT(status); > + > + while (frame_cnt-- && (work_done < quota)) { > + work_done += men_z192_read_frame(ndev, 0); > + men_z192_ack_rx_pkg(priv, 1); > + } > + > + if (work_done < quota) { > + napi_complete(napi); > + men_z192_set_int(priv, MEN_Z192_CAN_NAPI_EN); > + } > + > + return work_done; > +} > + > +static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct can_frame *cf = (struct can_frame *)skb->data; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + struct net_device_stats *stats = &ndev->stats; > + struct men_z192_cf_buf __iomem *cf_buf; > + u32 data[2] = {0, 0}; > + int status; > + u32 id; > + > + if (can_dropped_invalid_skb(ndev, skb)) > + return NETDEV_TX_OK; > + > + status = readl(®s->rx_tx_sts); > + > + if (MEN_Z192_TX_BUF_CNT(status) >= 255) { > + netif_stop_queue(ndev); > + netdev_err(ndev, "not enough space in TX buffer\n"); > + > + return NETDEV_TX_BUSY; > + } Please try to avoid NETDEV_TX_BUSY by stopping the queue earlier (if buf_cnt == 254). > + cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START; > + > + if (cf->can_id & CAN_EFF_FLAG) { > + /* Extended frame */ > + id = ((cf->can_id & CAN_EFF_MASK) << > + MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2; > + > + id |= (((cf->can_id & CAN_EFF_MASK) >> > + (CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) << > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > + > + id |= MEN_Z192_CFBUF_IDE; > + id |= MEN_Z192_CFBUF_SRR; > + > + if (cf->can_id & CAN_RTR_FLAG) > + id |= MEN_Z192_CFBUF_E_RTR; > + } else { > + /* Standard frame */ > + id = ((cf->can_id & CAN_SFF_MASK) << > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > + > + if (cf->can_id & CAN_RTR_FLAG) > + id |= MEN_Z192_CFBUF_S_RTR; > + } > + > + if (cf->can_dlc > 0) > + data[0] = be32_to_cpup((__be32 *)(cf->data)); > + if (cf->can_dlc > 3) > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); Not necessary if RTR. > + writel(id, &cf_buf->can_id); > + writel(cf->can_dlc, &cf_buf->length); > + > + if (!(cf->can_id & CAN_RTR_FLAG)) { > + writel(data[0], &cf_buf->data[0]); > + writel(data[1], &cf_buf->data[1]); Why do you not check cf->can_dlc here as well. And is the extra copy necessary. > + > + stats->tx_bytes += cf->can_dlc; > + } If I look to other drivers, they write the data even in case of RTR. > + /* be sure everything is written to the > + * device before acknowledge the data. > + */ > + mmiowb(); > + > + /* trigger the transmission */ > + men_z192_ack_tx_pkg(priv, 1); > + > + stats->tx_packets++; > + > + kfree_skb(skb); > + > + return NETDEV_TX_OK; > +} > + > +static void men_z192_err_interrupt(struct net_device *ndev, u32 status) > +{ > + struct net_device_stats *stats = &ndev->stats; > + struct men_z192 *priv = netdev_priv(ndev); > + struct can_berr_counter bec; > + struct can_frame *cf; > + struct sk_buff *skb; > + enum can_state rx_state = 0, tx_state = 0; > + > + skb = alloc_can_err_skb(ndev, &cf); > + if (unlikely(!skb)) > + return; > + > + /* put the rx/tx error counter to > + * the additional controller specific > + * section of the error frame. > + */ > + men_z192_get_berr_counter(ndev, &bec); > + cf->data[6] = bec.txerr; > + cf->data[7] = bec.rxerr; > + > + /* overrun interrupt */ > + if (status & MEN_Z192_RFLG_OVRF) { > + cf->can_id |= CAN_ERR_CRTL; > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > + stats->rx_over_errors++; > + stats->rx_errors++; > + } > + > + /* bus change interrupt */ > + if (status & MEN_Z192_RFLG_CSCIF) { > + rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; > + tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; > + can_change_state(ndev, cf, tx_state, rx_state); > + > + if (priv->can.state == CAN_STATE_BUS_OFF) > + can_bus_off(ndev); > + } Does the controller only provide state change events? What about other errors? > + > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + netif_receive_skb(skb); > +} > + > +static irqreturn_t men_z192_isr(int irq, void *dev_id) > +{ > + struct net_device *ndev = dev_id; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + bool handled = false; > + u32 irq_flags; > + u32 status; > + > + status = readl(®s->rx_tx_sts); > + > + irq_flags = status & MEN_Z192_IRQ_FLAGS_ALL; > + if (!irq_flags) > + goto out; > + > + /* It is save to write to RX_TS_STS[15:0] */ > + writel(irq_flags, ®s->rx_tx_sts); > + > + if (irq_flags & MEN_Z192_TFLG_TXIF) { > + netif_wake_queue(ndev); > + handled = true; > + } > + > + /* handle errors */ > + if ((irq_flags & MEN_Z192_RFLG_OVRF) || > + (irq_flags & MEN_Z192_RFLG_CSCIF)) { > + men_z192_err_interrupt(ndev, status); > + handled = true; > + } > + > + /* schedule NAPI if: > + * - rx IRQ > + * - rx timeout IRQ > + */ > + if ((irq_flags & MEN_Z192_RFLG_RXIF) || > + (irq_flags & MEN_Z192_RFLG_TOUTF)) { > + men_z192_set_int(priv, MEN_Z192_CAN_NAPI_DIS); > + napi_schedule(&priv->napi); > + handled = true; > + } > + > +out: > + return IRQ_RETVAL(handled); > +} > + > +static int men_z192_set_bittiming(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + const struct can_bittiming *bt = &priv->can.bittiming; > + unsigned long flags; > + u32 ctlbtr; > + int ret = 0; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + ctlbtr = readl(&priv->regs->ctl_btr); > + > + if (!(ctlbtr & MEN_Z192_CTL1_INITAK)) { > + netdev_alert(ndev, > + "cannot set bittiminig while in running mode\n"); > + ret = -EPERM; > + goto out_restore; > + } > + > + ctlbtr &= ~(MEN_Z192_BTR0_BRP(0x3f) | > + MEN_Z192_BTR0_SJW(0x03) | > + MEN_Z192_BTR1_TSEG1(0x0f) | > + MEN_Z192_BTR1_TSEG2(0x07) | > + MEN_Z192_CTL1_LISTEN | > + MEN_Z192_CTL1_LOOPB | > + MEN_Z192_BTR1_SAMP); > + > + ctlbtr |= MEN_Z192_BTR0_BRP(bt->brp - 1) | > + MEN_Z192_BTR0_SJW(bt->sjw - 1) | > + MEN_Z192_BTR1_TSEG1(bt->phase_seg1 + bt->prop_seg - 1) | > + MEN_Z192_BTR1_TSEG2(bt->phase_seg2 - 1); > + > + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > + ctlbtr |= MEN_Z192_BTR1_SAMP; > + > + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) > + ctlbtr |= MEN_Z192_CTL1_LISTEN; > + > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) > + ctlbtr |= MEN_Z192_CTL1_LOOPB; > + > + netdev_dbg(ndev, "CTL_BTR=0x%08x\n", ctlbtr); > + > + writel(ctlbtr, &priv->regs->ctl_btr); > + > +out_restore: > + spin_unlock_irqrestore(&priv->lock, flags); > + > + return ret; > +} > + > +static void men_z192_init_idac(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + > + /* hardware filtering (accept everything) */ > + writel(0x00000000, ®s->idar_0_to_3); > + writel(0x00000000, ®s->idar_4_to_7); > + writel(0xffffffff, ®s->idmr_0_to_3); > + writel(0xffffffff, ®s->idmr_4_to_7); > +} > + > +void men_z192_set_can_state(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + enum can_state rx_state, tx_state; > + u32 status; > + > + status = readl(®s->rx_tx_sts); > + > + rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; > + tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; > + > + priv->can.state = max(tx_state, rx_state); > +} > + > +static int men_z192_start(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + int ret; > + > + ret = men_z192_req_init_mode(priv); > + if (ret) > + return ret; > + > + ret = men_z192_set_bittiming(ndev); > + if (ret) > + return ret; > + > + ret = men_z192_req_run_mode(priv); > + if (ret) > + return ret; > + > + men_z192_init_idac(ndev); > + > + /* The 16z192 CAN IP does not reset the can bus state > + * if we enter the init mode. There is also > + * no software reset to reset the state machine. > + * We need to read the current state, and > + * inform the upper layer about the current state. > + */ > + men_z192_set_can_state(ndev); Hm, the application expected the state to be reset. Calling "can_change_state()" in "men_z192_set_can_state()" does make sense. > + > + men_z192_set_int(priv, MEN_Z192_CAN_EN); > + > + return 0; > +} > + > +static int men_z192_open(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + int ret; > + > + ret = open_candev(ndev); > + if (ret) > + return ret; > + > + ret = request_irq(ndev->irq, men_z192_isr, IRQF_SHARED, > + ndev->name, ndev); > + if (ret) > + goto out_close; > + > + ret = men_z192_start(ndev); > + if (ret) > + goto out_free_irq; > + > + napi_enable(&priv->napi); > + netif_start_queue(ndev); > + > + return 0; > + > +out_free_irq: > + free_irq(ndev->irq, ndev); > +out_close: > + close_candev(ndev); > + return ret; > +} > + > +static int men_z192_stop(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + int ret; > + > + men_z192_set_int(priv, MEN_Z192_CAN_DIS); > + > + ret = men_z192_req_init_mode(priv); > + if (ret) > + return ret; > + > + priv->can.state = CAN_STATE_STOPPED; > + > + return 0; > +} > + > +static int men_z192_close(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + int ret; > + > + netif_stop_queue(ndev); > + > + napi_disable(&priv->napi); > + > + ret = men_z192_stop(ndev); > + > + free_irq(ndev->irq, ndev); > + > + close_candev(ndev); > + > + return ret; > +} > + > +static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) > +{ > + int ret; > + > + switch (mode) { > + case CAN_MODE_START: > + ret = men_z192_start(ndev); > + if (ret) > + return ret; "if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and in many other places. > + > + netif_wake_queue(ndev); > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static const struct net_device_ops men_z192_netdev_ops = { > + .ndo_open = men_z192_open, > + .ndo_stop = men_z192_close, > + .ndo_start_xmit = men_z192_xmit, > + .ndo_change_mtu = can_change_mtu, > +}; > + > +static int men_z192_verify_buf_lvl(int buffer_lvl) > +{ > + if (buffer_lvl < MEN_Z192_MIN_BUF_LVL || > + buffer_lvl > MEN_Z192_MAX_BUF_LVL) > + return -EINVAL; > + > + return 0; > +} > + > +static void men_z192_set_buf_lvl_irq(struct net_device *ndev, int rxlvl, > + int txlvl) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + int reg_val; > + > + if (men_z192_verify_buf_lvl(rxlvl)) > + reg_val = MEN_Z192_RX_BUF_LVL_DEF & MEN_Z192_RX_BUF_LVL; > + else > + reg_val = rxlvl & MEN_Z192_RX_BUF_LVL; > + > + if (men_z192_verify_buf_lvl(txlvl)) > + reg_val |= (MEN_Z192_TX_BUF_LVL_DEF << 16) & > + MEN_Z192_TX_BUF_LVL; > + else > + reg_val |= (txlvl << 16) & MEN_Z192_TX_BUF_LVL; > + > + dev_info(priv->dev, "RX IRQ Level: %d TX IRQ Level: %d\n", > + rxlvl, txlvl); > + > + writel(reg_val, ®s->buf_lvl); > +} > + > +static void men_z192_set_rx_tout(struct net_device *ndev, int tout) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + int reg_val; > + > + if (tout < MEN_Z192_RX_TOUT_MIN || tout > MEN_Z192_RX_TOUT_MAX) > + reg_val = MEN_Z192_RX_TOUT_MAX; > + else > + reg_val = tout; > + > + dev_info(priv->dev, "RX IRQ timeout set to: %d\n", reg_val); > + > + writel(reg_val, ®s->rx_timeout); > +} > + > +static int men_z192_register(struct net_device *ndev) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + u32 ctl_btr; > + int ret; > + > + /* The CAN controller should be always enabled. > + * There is no way to enable it if disabled. > + */ > + ctl_btr = readl(®s->ctl_btr); > + if (!(ctl_btr & MEN_Z192_CTL1_CANE)) > + return -ENODEV; > + > + men_z192_set_buf_lvl_irq(ndev, rxlvl, txlvl); > + men_z192_set_rx_tout(ndev, rx_timeout); > + > + ret = men_z192_req_init_mode(priv); > + if (ret) { > + dev_err(priv->dev, "failed to request init mode\n"); > + return ret; > + } > + > + return register_candev(ndev); > +} > + > +static void men_z192_unregister(struct net_device *ndev) > +{ > + return unregister_candev(ndev); > +} > + > +static int men_z192_probe(struct mcb_device *mdev, > + const struct mcb_device_id *id) > +{ > + struct device *dev = &mdev->dev; > + struct men_z192 *priv; > + struct net_device *ndev; > + void __iomem *dev_base; > + struct resource *mem; > + u32 timebase; > + int ret = 0; > + int irq; > + > + mem = mcb_request_mem(mdev, dev_name(dev)); > + if (IS_ERR(mem)) { > + dev_err(dev, "failed to request device memory"); > + return PTR_ERR(mem); > + } > + > + dev_base = ioremap(mem->start, resource_size(mem)); > + if (!dev_base) { > + dev_err(dev, "failed to ioremap device memory"); > + ret = -ENXIO; > + goto out_release; > + } > + > + irq = mcb_get_irq(mdev); > + if (irq <= 0) { > + ret = -ENODEV; > + goto out_unmap; > + } > + > + ndev = alloc_candev(sizeof(struct men_z192), 1); You specify here one echo_skb but it's not used anywhere. Local loopback seems not to be implemented. > + if (!ndev) { > + dev_err(dev, "failed to allocate the can device"); > + ret = -ENOMEM; > + goto out_unmap; > + } > + > + ndev->netdev_ops = &men_z192_netdev_ops; > + ndev->irq = irq; > + > + priv = netdev_priv(ndev); > + priv->ndev = ndev; > + priv->dev = dev; > + > + priv->mem = mem; > + priv->dev_base = dev_base; > + priv->regs = priv->dev_base + MEN_Z192_REGS_OFFS; > + > + timebase = readl(&priv->regs->timebase); > + if (!timebase) { > + dev_err(dev, "invalid timebase configured (timebase=%d)\n", > + timebase); > + ret = -EINVAL; > + goto out_unmap; free_candev(ndev) is missing. > + } > + > + priv->can.clock.freq = timebase; > + priv->can.bittiming_const = &men_z192_bittiming_const; > + priv->can.do_set_mode = men_z192_set_mode; > + priv->can.do_get_berr_counter = men_z192_get_berr_counter; > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY | > + CAN_CTRLMODE_3_SAMPLES | > + CAN_CTRLMODE_LOOPBACK; > + > + spin_lock_init(&priv->lock); > + > + netif_napi_add(ndev, &priv->napi, men_z192_poll, > + MEN_Z192_NAPI_WEIGHT); > + > + mcb_set_drvdata(mdev, ndev); > + SET_NETDEV_DEV(ndev, dev); > + > + ret = men_z192_register(ndev); > + if (ret) { > + dev_err(dev, "failed to register CAN device"); > + goto out_free_candev; > + } > + > + dev_info(dev, "MEN 16z192 CAN driver successfully registered\n"); > + > + return 0; > + > +out_free_candev: > + netif_napi_del(&priv->napi); > + free_candev(ndev); > +out_unmap: > + iounmap(dev_base); > +out_release: > + mcb_release_mem(mem); > + return ret; > +} > + > +static void men_z192_remove(struct mcb_device *mdev) > +{ > + struct net_device *ndev = mcb_get_drvdata(mdev); > + struct men_z192 *priv = netdev_priv(ndev); > + > + men_z192_unregister(ndev); > + netif_napi_del(&priv->napi); > + > + iounmap(priv->dev_base); > + mcb_release_mem(priv->mem); > + > + free_candev(ndev); > +} > + > +static const struct mcb_device_id men_z192_ids[] = { > + { .device = 0xc0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(mcb, men_z192_ids); > + > +static struct mcb_driver men_z192_driver = { > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, > + }, > + .probe = men_z192_probe, > + .remove = men_z192_remove, > + .id_table = men_z192_ids, > +}; > +module_mcb_driver(men_z192_driver); > + > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>"); > +MODULE_DESCRIPTION("MEN 16z192 CAN Controller"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("mcb:16z192"); Wolfgang.
On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote: > Hello Andreas, > > a first quick review.... > > Am 26.07.2016 um 11:16 schrieb Andreas Werner: > >This CAN Controller is found on MEN Chameleon FPGAs. > > > >The driver/device supports the CAN2.0 specification. > >There are 255 RX and 255 Tx buffer within the IP. The > >pointer for the buffer are handled by HW to make the > >access from within the driver as simple as possible. > > > >The driver also supports parameters to configure the > >buffer level interrupt for RX/TX as well as a RX timeout > >interrupt. > > > >With this configuration options, the driver/device > >provides flexibility for different types of usecases. > > > >Signed-off-by: Andreas Werner <andreas.werner@men.de> > >--- > > drivers/net/can/Kconfig | 10 + > > drivers/net/can/Makefile | 1 + > > drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 1000 insertions(+) > > create mode 100644 drivers/net/can/men_z192_can.c > > > >diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > >index 0d40aef..0fa0387 100644 > >--- a/drivers/net/can/Kconfig > >+++ b/drivers/net/can/Kconfig > >@@ -104,6 +104,16 @@ config CAN_JANZ_ICAN3 > > This driver can also be built as a module. If so, the module will be > > called janz-ican3.ko. > > > >+config CAN_MEN_Z192 > >+ tristate "MEN 16Z192-00 CAN Controller" > >+ depends on MCB > >+ ---help--- > >+ Driver for MEN 16Z192-00 CAN Controller IP-Core, which > >+ is connected to the MEN Chameleon Bus. > >+ > >+ This driver can also be built as a module. If so, the module will be > >+ called men_z192_can.ko. > >+ > > config CAN_RCAR > > tristate "Renesas R-Car CAN controller" > > depends on ARCH_RENESAS || ARM > >diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > >index e3db0c8..eb206b3 100644 > >--- a/drivers/net/can/Makefile > >+++ b/drivers/net/can/Makefile > >@@ -22,6 +22,7 @@ obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o > > obj-$(CONFIG_CAN_GRCAN) += grcan.o > > obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ > > obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o > >+obj-$(CONFIG_CAN_MEN_Z192) += men_z192_can.o > > obj-$(CONFIG_CAN_MSCAN) += mscan/ > > obj-$(CONFIG_CAN_M_CAN) += m_can/ > > obj-$(CONFIG_CAN_RCAR) += rcar_can.o > >diff --git a/drivers/net/can/men_z192_can.c b/drivers/net/can/men_z192_can.c > >new file mode 100644 > >index 0000000..b39ffee > >--- /dev/null > >+++ b/drivers/net/can/men_z192_can.c > >@@ -0,0 +1,989 @@ > >+/* > >+ * MEN 16Z192 CAN Controller driver > >+ * > >+ * Copyright (C) 2016 MEN Mikroelektronik GmbH (www.men.de) > >+ * > >+ * This program is free software; you can redistribute it and/or modify it > >+ * under the terms of the GNU General Public License as published by the Free > >+ * Software Foundation; version 2 of the License. > >+ */ > >+ > >+#include <linux/netdevice.h> > >+#include <linux/can/error.h> > >+#include <linux/can/dev.h> > >+#include <linux/bitops.h> > >+#include <linux/kernel.h> > >+#include <linux/module.h> > >+#include <linux/err.h> > >+#include <linux/mcb.h> > >+#include <linux/can.h> > >+#include <linux/io.h> > >+ > >+#define DRV_NAME "z192_can" > >+ > >+#define MEN_Z192_NAPI_WEIGHT 64 > >+#define MEN_Z192_MODE_TOUT_US 40 > >+ > >+/* CTL/BTR Register Bits */ > >+#define MEN_Z192_CTL0_INITRQ BIT(0) > >+#define MEN_Z192_CTL0_SLPRQ BIT(1) > >+#define MEN_Z192_CTL1_INITAK BIT(8) > >+#define MEN_Z192_CTL1_SLPAK BIT(9) > >+#define MEN_Z192_CTL1_LISTEN BIT(12) > >+#define MEN_Z192_CTL1_LOOPB BIT(13) > >+#define MEN_Z192_CTL1_CANE BIT(15) > >+#define MEN_Z192_BTR0_BRP(x) (((x) & 0x3f) << 16) > >+#define MEN_Z192_BTR0_SJW(x) (((x) & 0x03) << 22) > >+#define MEN_Z192_BTR1_TSEG1(x) (((x) & 0x0f) << 24) > >+#define MEN_Z192_BTR1_TSEG2(x) (((x) & 0x07) << 28) > >+#define MEN_Z192_BTR1_SAMP BIT(31) > >+ > >+/* IER Interrupt Enable Register bits */ > >+#define MEN_Z192_RXIE BIT(0) > >+#define MEN_Z192_OVRIE BIT(1) > >+#define MEN_Z192_CSCIE BIT(6) > >+#define MEN_Z192_TOUTE BIT(7) > >+#define MEN_Z192_TXIE BIT(16) > >+#define MEN_Z192_ERRIE BIT(17) > >+ > >+#define MEN_Z192_IRQ_ALL \ > >+ (MEN_Z192_RXIE | MEN_Z192_OVRIE | \ > >+ MEN_Z192_CSCIE | MEN_Z192_TOUTE | \ > >+ MEN_Z192_TXIE) > >+ > >+#define MEN_Z192_IRQ_NAPI (MEN_Z192_RXIE | MEN_Z192_TOUTE) > >+ > >+/* RX_TX_STAT RX/TX Status status register bits */ > >+#define MEN_Z192_RX_BUF_CNT(x) ((x) & 0xff) > >+#define MEN_Z192_TX_BUF_CNT(x) (((x) & 0xff00) >> 8) > >+#define MEN_Z192_RFLG_RXIF BIT(16) > >+#define MEN_Z192_RFLG_OVRF BIT(17) > >+#define MEN_Z192_RFLG_TSTATE GENMASK(19, 18) > >+#define MEN_Z192_RFLG_RSTATE GENMASK(21, 20) > >+#define MEN_Z192_RFLG_CSCIF BIT(22) > >+#define MEN_Z192_RFLG_TOUTF BIT(23) > >+#define MEN_Z192_TFLG_TXIF BIT(24) > >+ > >+#define MEN_Z192_GET_TSTATE(x) (((x) & MEN_Z192_RFLG_TSTATE) >> 18) > >+#define MEN_Z192_GET_RSTATE(x) (((x) & MEN_Z192_RFLG_RSTATE) >> 20) > >+ > >+#define MEN_Z192_IRQ_FLAGS_ALL \ > >+ (MEN_Z192_RFLG_RXIF | MEN_Z192_RFLG_OVRF | \ > >+ MEN_Z192_RFLG_TSTATE | MEN_Z192_RFLG_RSTATE | \ > >+ MEN_Z192_RFLG_CSCIF | MEN_Z192_RFLG_TOUTF | \ > >+ MEN_Z192_TFLG_TXIF) > >+ > >+/* RX/TX Error counter bits */ > >+#define MEN_Z192_GET_RX_ERR_CNT(x) ((x) & 0xff) > >+#define MEN_Z192_GET_TX_ERR_CNT(x) (((x) & 0x00ff0000) >> 16) > >+ > >+/* Buffer level register bits */ > >+#define MEN_Z192_RX_BUF_LVL GENMASK(15, 0) > >+#define MEN_Z192_TX_BUF_LVL GENMASK(31, 16) > >+ > >+/* RX/TX Buffer register bits */ > >+#define MEN_Z192_CFBUF_LEN GENMASK(3, 0) > >+#define MEN_Z192_CFBUF_ID1 GENMASK(31, 21) > >+#define MEN_Z192_CFBUF_ID2 GENMASK(18, 1) > >+#define MEN_Z192_CFBUF_TS GENMASK(31, 8) > >+#define MEN_Z192_CFBUF_E_RTR BIT(0) > >+#define MEN_Z192_CFBUF_IDE BIT(19) > >+#define MEN_Z192_CFBUF_SRR BIT(20) > >+#define MEN_Z192_CFBUF_S_RTR BIT(20) > >+#define MEN_Z192_CFBUF_ID2_SHIFT 1 > >+#define MEN_Z192_CFBUF_ID1_SHIFT 21 > >+ > >+/* Global register offsets */ > >+#define MEN_Z192_RX_BUF_START 0x0000 > >+#define MEN_Z192_TX_BUF_START 0x1000 > >+#define MEN_Z192_REGS_OFFS 0x2000 > >+ > >+/* Buffer level control values */ > >+#define MEN_Z192_MIN_BUF_LVL 0 > >+#define MEN_Z192_MAX_BUF_LVL 254 > >+#define MEN_Z192_RX_BUF_LVL_DEF 5 > >+#define MEN_Z192_TX_BUF_LVL_DEF 5 > >+#define MEN_Z192_RX_TOUT_MIN 0 > >+#define MEN_Z192_RX_TOUT_MAX 65535 > >+#define MEN_Z192_RX_TOUT_DEF 1000 > >+ > >+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; > >+module_param(txlvl, int, S_IRUGO); > >+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" > >+ __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); > >+ > >+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; > >+module_param(rxlvl, int, S_IRUGO); > >+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" > >+ __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); > >+ > > What impact does the level have on the latency? Could you please add some > comments. It has a impact on the latency. rxlvl = 0 -> if one frame got received, a IRQ will be generated rxlvl = 254 -> if 255 frames got received, a IRQ will be generated > > >+static int rx_timeout = MEN_Z192_RX_TOUT_DEF; > >+module_param(rx_timeout, int, S_IRUGO); > >+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" > >+ __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); > > Ditto. What is "rx_timeout" good for. > The rx timeout is used im combination with the rxlvl to assert the if the buffer level is not reached within this timeout. Both, the timeout and the level are used to give the user as much control over the latency and the IRQ handling as possible. With this two options, the driver can be configured for different use cases. I will add this as the comment to make it more clear. > >+ > >+struct men_z192_regs { > >+ u32 ctl_btr; /* Control and bus timing register */ > >+ u32 ier; /* Interrupt enable register */ > >+ u32 buf_lvl; /* Buffer level register */ > >+ u32 rxa; /* RX Data acknowledge register */ > >+ u32 txa; /* TX data acknowledge register */ > >+ u32 rx_tx_sts; /* RX/TX flags and buffer level */ > >+ u32 ovr_ecc_sts; /* Overrun/ECC status register */ > >+ u32 idac_ver; /* ID acceptance control / version */ > >+ u32 rx_tx_err; /* RX/TX error counter register */ > >+ u32 idar_0_to_3; /* ID acceptance register 0...3 */ > >+ u32 idar_4_to_7; /* ID acceptance register 4...7 */ > >+ u32 idmr_0_to_3; /* ID mask register 0...3 */ > >+ u32 idmr_4_to_7; /* ID mask register 4...7 */ > >+ u32 rx_timeout; /* receive timeout */ > >+ u32 timebase; /* Base frequency for baudrate calculation */ > >+}; > >+ > >+struct men_z192 { > >+ struct can_priv can; > >+ struct napi_struct napi; > >+ struct net_device *ndev; > >+ struct device *dev; > >+ > >+ /* Lock for CTL_BTR register access. > >+ * This register combines bittiming bits > >+ * and the operation mode bits. > >+ * It is also used for bit r/m/w access > >+ * to all registers. > >+ */ > >+ spinlock_t lock; > >+ struct resource *mem; > >+ struct men_z192_regs __iomem *regs; > >+ void __iomem *dev_base; > >+}; > >+ > >+struct men_z192_cf_buf { > >+ u32 can_id; > >+ u32 data[2]; > >+ u32 length; > >+}; > >+ > >+enum men_z192_int_state { > >+ MEN_Z192_CAN_DIS = 0, > >+ MEN_Z192_CAN_EN, > >+ MEN_Z192_CAN_NAPI_DIS, > >+ MEN_Z192_CAN_NAPI_EN, > >+}; > >+ > >+static enum can_state bus_state_map[] = { > >+ CAN_STATE_ERROR_ACTIVE, > >+ CAN_STATE_ERROR_WARNING, > >+ CAN_STATE_ERROR_PASSIVE, > >+ CAN_STATE_BUS_OFF > >+}; > >+ > >+static const struct can_bittiming_const men_z192_bittiming_const = { > >+ .name = DRV_NAME, > >+ .tseg1_min = 4, > >+ .tseg1_max = 16, > >+ .tseg2_min = 2, > >+ .tseg2_max = 8, > >+ .sjw_max = 4, > >+ .brp_min = 2, > >+ .brp_max = 64, > >+ .brp_inc = 1, > >+}; > >+ > >+static inline void men_z192_bit_clr(struct men_z192 *priv, void __iomem *addr, > >+ u32 mask) > >+{ > >+ unsigned long flags; > >+ u32 val; > >+ > >+ spin_lock_irqsave(&priv->lock, flags); > >+ > >+ val = readl(addr); > >+ val &= ~mask; > >+ writel(val, addr); > >+ > >+ spin_unlock_irqrestore(&priv->lock, flags); > >+} > >+ > >+static inline void men_z192_bit_set(struct men_z192 *priv, void __iomem *addr, > >+ u32 mask) > >+{ > >+ unsigned long flags; > >+ u32 val; > >+ > >+ spin_lock_irqsave(&priv->lock, flags); > >+ > >+ val = readl(addr); > >+ val |= mask; > >+ writel(val, addr); > >+ > >+ spin_unlock_irqrestore(&priv->lock, flags); > >+} > >+ > >+static inline void men_z192_ack_rx_pkg(struct men_z192 *priv, > >+ unsigned int count) > >+{ > >+ writel(count, &priv->regs->rxa); > >+} > >+ > >+static inline void men_z192_ack_tx_pkg(struct men_z192 *priv, > >+ unsigned int count) > >+{ > >+ writel(count, &priv->regs->txa); > >+} > >+ > >+static void men_z192_set_int(struct men_z192 *priv, > >+ enum men_z192_int_state state) > >+{ > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ > >+ switch (state) { > >+ case MEN_Z192_CAN_DIS: > >+ men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_ALL); > >+ break; > >+ > >+ case MEN_Z192_CAN_EN: > >+ men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_ALL); > >+ break; > >+ > >+ case MEN_Z192_CAN_NAPI_DIS: > >+ men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_NAPI); > >+ break; > >+ > >+ case MEN_Z192_CAN_NAPI_EN: > >+ men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_NAPI); > >+ break; > >+ > >+ default: > >+ netdev_err(priv->ndev, "invalid interrupt state\n"); > >+ break; > >+ } > >+} > >+ > >+static int men_z192_get_berr_counter(const struct net_device *ndev, > >+ struct can_berr_counter *bec) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ u32 err_cnt; > >+ > >+ err_cnt = readl(®s->rx_tx_err); > >+ > >+ bec->txerr = MEN_Z192_GET_TX_ERR_CNT(err_cnt); > >+ bec->rxerr = MEN_Z192_GET_RX_ERR_CNT(err_cnt); > >+ > >+ return 0; > >+} > >+ > >+static int men_z192_req_run_mode(struct men_z192 *priv) > >+{ > >+ unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; > >+ struct men_z192_regs __iomem *regs = priv->regs;get > >+ u32 val; > >+ > >+ men_z192_bit_clr(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); > >+ > >+ while (timeout--) { > >+ val = readl(®s->ctl_btr); > >+ if (!(val & MEN_Z192_CTL1_INITAK)) > >+ break; > >+ > >+ udelay(10); > >+ } > >+ > >+ if (val & MEN_Z192_CTL1_INITAK) > >+ return -ETIMEDOUT; > >+ > >+ return 0; > >+} > >+ > >+static int men_z192_req_init_mode(struct men_z192 *priv) > >+{ > >+ unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ u32 val; > >+ > >+ men_z192_bit_set(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); > >+ > >+ while (timeout--) { > >+ val = readl(®s->ctl_btr); > >+ if (val & MEN_Z192_CTL1_INITAK) > >+ break; > >+ > >+ udelay(10); > >+ } > >+ > >+ if (!(val & MEN_Z192_CTL1_INITAK)) > >+ return -ETIMEDOUT; > >+ > >+ return 0; > >+} > >+ > >+static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr) > >+{ > >+ struct net_device_stats *stats = &ndev->stats; > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_cf_buf __iomem *cf_buf; > >+ struct can_frame *cf; > >+ struct sk_buff *skb; > >+ u32 cf_offset; > >+ u32 length; > >+ u32 data; > >+ u32 id; > >+ > >+ skb = alloc_can_skb(ndev, &cf); > >+ if (unlikely(!skb)) { > >+ stats->rx_dropped++; > >+ return 0; > >+ } > >+ > >+ cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; > >+ > >+ cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; > >+ length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; > >+ id = readl(&cf_buf->can_id); > >+ > >+ if (id & MEN_Z192_CFBUF_IDE) { > >+ /* Extended frame */ > >+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; > >+ cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> > >+ MEN_Z192_CFBUF_ID2_SHIFT; > >+ > >+ cf->can_id |= CAN_EFF_FLAG; > >+ > >+ if (id & MEN_Z192_CFBUF_E_RTR) > >+ cf->can_id |= CAN_RTR_FLAG; > >+ } else { > >+ /* Standard frame */ > >+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> > >+ MEN_Z192_CFBUF_ID1_SHIFT; > >+ > >+ if (id & MEN_Z192_CFBUF_S_RTR) > >+ cf->can_id |= CAN_RTR_FLAG; > >+ } > >+ > >+ cf->can_dlc = get_can_dlc(length); > >+ > >+ /* remote transmission request frame > >+ * contains no data field even if the > >+ * data length is set to a value > 0 > >+ */ > >+ if (!(cf->can_id & CAN_RTR_FLAG)) { > >+ if (cf->can_dlc > 0) { > >+ data = readl(&cf_buf->data[0]); > >+ *(__be32 *)cf->data = cpu_to_be32(data); > > Do you really need the extra copy? > > >+ } > >+ if (cf->can_dlc > 4) { > >+ data = readl(&cf_buf->data[1]); > >+ *(__be32 *)(cf->data + 4) = cpu_to_be32(data); > > Ditto. No its not really needed. I thought its more clean and more readable than putting this in one line withouth the copy. > > >+ } > >+ } > >+ > >+ stats->rx_bytes += cf->can_dlc; > >+ stats->rx_packets++; > >+ netif_receive_skb(skb); > >+ > >+ return 1; > > >+} > >+ > >+static int men_z192_poll(struct napi_struct *napi, int quota) > >+{ > >+ struct net_device *ndev = napi->dev; > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ int work_done = 0; > >+ u32 frame_cnt; > >+ u32 status; > >+ > >+ status = readl(®s->rx_tx_sts); > >+ > >+ frame_cnt = MEN_Z192_RX_BUF_CNT(status); > >+ > >+ while (frame_cnt-- && (work_done < quota)) { > >+ work_done += men_z192_read_frame(ndev, 0); > >+ men_z192_ack_rx_pkg(priv, 1); > >+ } > >+ > >+ if (work_done < quota) { > >+ napi_complete(napi); > >+ men_z192_set_int(priv, MEN_Z192_CAN_NAPI_EN); > >+ } > >+ > >+ return work_done; > >+} > >+ > >+static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev) > >+{ > >+ struct can_frame *cf = (struct can_frame *)skb->data; > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ struct net_device_stats *stats = &ndev->stats; > >+ struct men_z192_cf_buf __iomem *cf_buf; > >+ u32 data[2] = {0, 0}; > >+ int status; > >+ u32 id; > >+ > >+ if (can_dropped_invalid_skb(ndev, skb)) > >+ return NETDEV_TX_OK; > >+ > >+ status = readl(®s->rx_tx_sts); > >+ > >+ if (MEN_Z192_TX_BUF_CNT(status) >= 255) { > >+ netif_stop_queue(ndev); > >+ netdev_err(ndev, "not enough space in TX buffer\n"); > >+ > >+ return NETDEV_TX_BUSY; > >+ } > > Please try to avoid NETDEV_TX_BUSY by stopping the queue earlier (if buf_cnt > == 254). > Agree with you, will fix that. > >+ cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START; > >+ > >+ if (cf->can_id & CAN_EFF_FLAG) { > >+ /* Extended frame */ > >+ id = ((cf->can_id & CAN_EFF_MASK) << > >+ MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2; > >+ > >+ id |= (((cf->can_id & CAN_EFF_MASK) >> > >+ (CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) << > >+ MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > >+ > >+ id |= MEN_Z192_CFBUF_IDE; > >+ id |= MEN_Z192_CFBUF_SRR; > >+ > >+ if (cf->can_id & CAN_RTR_FLAG) > >+ id |= MEN_Z192_CFBUF_E_RTR; > >+ } else { > >+ /* Standard frame */ > >+ id = ((cf->can_id & CAN_SFF_MASK) << > >+ MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > >+ > >+ if (cf->can_id & CAN_RTR_FLAG) > >+ id |= MEN_Z192_CFBUF_S_RTR; > >+ } > >+ > >+ if (cf->can_dlc > 0) > >+ data[0] = be32_to_cpup((__be32 *)(cf->data)); > >+ if (cf->can_dlc > 3) > >+ data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); > > Not necessary if RTR. > Argh, right... > >+ writel(id, &cf_buf->can_id); > >+ writel(cf->can_dlc, &cf_buf->length); > >+ > >+ if (!(cf->can_id & CAN_RTR_FLAG)) { > >+ writel(data[0], &cf_buf->data[0]); > >+ writel(data[1], &cf_buf->data[1]); > > Why do you not check cf->can_dlc here as well. And is the extra copy > necessary. > Yes, I agree with you. The extra copy could be also avoided. > >+ > >+ stats->tx_bytes += cf->can_dlc; > >+ } > > If I look to other drivers, they write the data even in case of RTR. > But why? A RTR does not have any data, therefore there is no need to write the data. Only the length is required as the request size. If there is a reason behind writing the data of a RTR frame, I can change that, but for now there is no reason. > >+ /* be sure everything is written to the > >+ * device before acknowledge the data. > >+ */ > >+ mmiowb(); > >+ > >+ /* trigger the transmission */ > >+ men_z192_ack_tx_pkg(priv, 1); > >+ > >+ stats->tx_packets++; > >+ > >+ kfree_skb(skb); > >+ > >+ return NETDEV_TX_OK; > >+} > >+ > >+static void men_z192_err_interrupt(struct net_device *ndev, u32 status) > >+{ > >+ struct net_device_stats *stats = &ndev->stats; > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct can_berr_counter bec; > >+ struct can_frame *cf; > >+ struct sk_buff *skb; > >+ enum can_state rx_state = 0, tx_state = 0; > >+ > >+ skb = alloc_can_err_skb(ndev, &cf); > >+ if (unlikely(!skb)) > >+ return; > >+ > >+ /* put the rx/tx error counter to > >+ * the additional controller specific > >+ * section of the error frame. > >+ */ > >+ men_z192_get_berr_counter(ndev, &bec); > >+ cf->data[6] = bec.txerr; > >+ cf->data[7] = bec.rxerr; > >+ > >+ /* overrun interrupt */ > >+ if (status & MEN_Z192_RFLG_OVRF) { > >+ cf->can_id |= CAN_ERR_CRTL; > >+ cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > >+ stats->rx_over_errors++; > >+ stats->rx_errors++; > >+ } > >+ > >+ /* bus change interrupt */ > >+ if (status & MEN_Z192_RFLG_CSCIF) { > >+ rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; > >+ tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; > >+ can_change_state(ndev, cf, tx_state, rx_state); > >+ > >+ if (priv->can.state == CAN_STATE_BUS_OFF) > >+ can_bus_off(ndev); > >+ } > > Does the controller only provide state change events? What about other > errors? > I thought that somebody will ask. The controller does only the state change events and the overrun error. Nothing more. I saw the error flags in many other drivers, but they are not existing in my controller. > >+ > >+ stats->rx_packets++; > >+ stats->rx_bytes += cf->can_dlc; > >+ netif_receive_skb(skb); > >+} > >+ > >+static irqreturn_t men_z192_isr(int irq, void *dev_id) > >+{ > >+ struct net_device *ndev = dev_id; > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ bool handled = false; > >+ u32 irq_flags; > >+ u32 status; > >+ > >+ status = readl(®s->rx_tx_sts); > >+ > >+ irq_flags = status & MEN_Z192_IRQ_FLAGS_ALL; > >+ if (!irq_flags) > >+ goto out; > >+ > >+ /* It is save to write to RX_TS_STS[15:0] */ > >+ writel(irq_flags, ®s->rx_tx_sts); > >+ > >+ if (irq_flags & MEN_Z192_TFLG_TXIF) { > >+ netif_wake_queue(ndev); > >+ handled = true; > >+ } > >+ > >+ /* handle errors */ > >+ if ((irq_flags & MEN_Z192_RFLG_OVRF) || > >+ (irq_flags & MEN_Z192_RFLG_CSCIF)) { > >+ men_z192_err_interrupt(ndev, status); > >+ handled = true; > >+ } > >+ > >+ /* schedule NAPI if: > >+ * - rx IRQ > >+ * - rx timeout IRQ > >+ */ > >+ if ((irq_flags & MEN_Z192_RFLG_RXIF) || > >+ (irq_flags & MEN_Z192_RFLG_TOUTF)) { > >+ men_z192_set_int(priv, MEN_Z192_CAN_NAPI_DIS); > >+ napi_schedule(&priv->napi); > >+ handled = true; > >+ } > >+ > >+out: > >+ return IRQ_RETVAL(handled); > >+} > >+ > >+static int men_z192_set_bittiming(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ const struct can_bittiming *bt = &priv->can.bittiming; > >+ unsigned long flags; > >+ u32 ctlbtr; > >+ int ret = 0; > >+ > >+ spin_lock_irqsave(&priv->lock, flags); > >+ > >+ ctlbtr = readl(&priv->regs->ctl_btr); > >+ > >+ if (!(ctlbtr & MEN_Z192_CTL1_INITAK)) { > >+ netdev_alert(ndev, > >+ "cannot set bittiminig while in running mode\n"); > >+ ret = -EPERM; > >+ goto out_restore; > >+ } > >+ > >+ ctlbtr &= ~(MEN_Z192_BTR0_BRP(0x3f) | > >+ MEN_Z192_BTR0_SJW(0x03) | > >+ MEN_Z192_BTR1_TSEG1(0x0f) | > >+ MEN_Z192_BTR1_TSEG2(0x07) | > >+ MEN_Z192_CTL1_LISTEN | > >+ MEN_Z192_CTL1_LOOPB | > >+ MEN_Z192_BTR1_SAMP); > >+ > >+ ctlbtr |= MEN_Z192_BTR0_BRP(bt->brp - 1) | > >+ MEN_Z192_BTR0_SJW(bt->sjw - 1) | > >+ MEN_Z192_BTR1_TSEG1(bt->phase_seg1 + bt->prop_seg - 1) | > >+ MEN_Z192_BTR1_TSEG2(bt->phase_seg2 - 1); > >+ > >+ if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > >+ ctlbtr |= MEN_Z192_BTR1_SAMP; > >+ > >+ if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) > >+ ctlbtr |= MEN_Z192_CTL1_LISTEN; > >+ > >+ if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) > >+ ctlbtr |= MEN_Z192_CTL1_LOOPB; > >+ > >+ netdev_dbg(ndev, "CTL_BTR=0x%08x\n", ctlbtr); > >+ > >+ writel(ctlbtr, &priv->regs->ctl_btr); > >+ > >+out_restore: > >+ spin_unlock_irqrestore(&priv->lock, flags); > >+ > >+ return ret; > >+} > >+ > >+static void men_z192_init_idac(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ > >+ /* hardware filtering (accept everything) */ > >+ writel(0x00000000, ®s->idar_0_to_3); > >+ writel(0x00000000, ®s->idar_4_to_7); > >+ writel(0xffffffff, ®s->idmr_0_to_3); > >+ writel(0xffffffff, ®s->idmr_4_to_7); > >+} > >+ > >+void men_z192_set_can_state(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ enum can_state rx_state, tx_state; > >+ u32 status; > >+ > >+ status = readl(®s->rx_tx_sts); > >+ > >+ rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; > >+ tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; > >+ > >+ priv->can.state = max(tx_state, rx_state); > >+} > >+ > >+static int men_z192_start(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ int ret; > >+ > >+ ret = men_z192_req_init_mode(priv); > >+ if (ret) > >+ return ret; > >+ > >+ ret = men_z192_set_bittiming(ndev); > >+ if (ret) > >+ return ret; > >+ > >+ ret = men_z192_req_run_mode(priv); > >+ if (ret) > >+ return ret; > >+ > >+ men_z192_init_idac(ndev); > >+ > >+ /* The 16z192 CAN IP does not reset the can bus state > >+ * if we enter the init mode. There is also > >+ * no software reset to reset the state machine. > >+ * We need to read the current state, and > >+ * inform the upper layer about the current state. > >+ */ > >+ men_z192_set_can_state(ndev); > > Hm, the application expected the state to be reset. Calling > "can_change_state()" in "men_z192_set_can_state()" does make sense. > Hm yes, but the IP is saving the state, this cannot be avoided. can_change_state() makes sense yes, I will add it. > >+ > >+ men_z192_set_int(priv, MEN_Z192_CAN_EN); > >+ > >+ return 0; > >+} > >+ > >+static int men_z192_open(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ int ret; > >+ > >+ ret = open_candev(ndev); > >+ if (ret) > >+ return ret; > >+ > >+ ret = request_irq(ndev->irq, men_z192_isr, IRQF_SHARED, > >+ ndev->name, ndev); > >+ if (ret) > >+ goto out_close; > >+ > >+ ret = men_z192_start(ndev); > >+ if (ret) > >+ goto out_free_irq; > >+ > >+ napi_enable(&priv->napi); > >+ netif_start_queue(ndev); > >+ > >+ return 0; > >+ > >+out_free_irq: > >+ free_irq(ndev->irq, ndev); > >+out_close: > >+ close_candev(ndev); > >+ return ret; > >+} > >+ > >+static int men_z192_stop(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ int ret; > >+ > >+ men_z192_set_int(priv, MEN_Z192_CAN_DIS); > >+ > >+ ret = men_z192_req_init_mode(priv); > >+ if (ret) > >+ return ret; > >+ > >+ priv->can.state = CAN_STATE_STOPPED; > >+ > >+ return 0; > >+} > >+ > >+static int men_z192_close(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ int ret; > >+ > >+ netif_stop_queue(ndev); > >+ > >+ napi_disable(&priv->napi); > >+ > >+ ret = men_z192_stop(ndev); > >+ > >+ free_irq(ndev->irq, ndev); > >+ > >+ close_candev(ndev); > >+ > >+ return ret; > >+} > >+ > >+static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) > >+{ > >+ int ret; > >+ > >+ switch (mode) { > >+ case CAN_MODE_START: > >+ ret = men_z192_start(ndev); > >+ if (ret) > >+ return ret; > > "if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and > in many other places. > Yes and no. I think its a general question about the naming of those variables. I will check all the variables in the driver if it really makes sense to rename it. For my opinion, "ret" is more generic. But you are right, "err" would be more readable in some places. > >+ > >+ netif_wake_queue(ndev); > >+ break; > >+ default: > >+ return -EOPNOTSUPP; > >+ } > >+ > >+ return 0; > >+} > >+ > >+static const struct net_device_ops men_z192_netdev_ops = { > >+ .ndo_open = men_z192_open, > >+ .ndo_stop = men_z192_close, > >+ .ndo_start_xmit = men_z192_xmit, > >+ .ndo_change_mtu = can_change_mtu, > >+}; > >+ > >+static int men_z192_verify_buf_lvl(int buffer_lvl) > >+{ > >+ if (buffer_lvl < MEN_Z192_MIN_BUF_LVL || > >+ buffer_lvl > MEN_Z192_MAX_BUF_LVL) > >+ return -EINVAL; > >+ > >+ return 0; > >+} > >+ > >+static void men_z192_set_buf_lvl_irq(struct net_device *ndev, int rxlvl, > >+ int txlvl) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ int reg_val; > >+ > >+ if (men_z192_verify_buf_lvl(rxlvl)) > >+ reg_val = MEN_Z192_RX_BUF_LVL_DEF & MEN_Z192_RX_BUF_LVL; > >+ else > >+ reg_val = rxlvl & MEN_Z192_RX_BUF_LVL; > >+ > >+ if (men_z192_verify_buf_lvl(txlvl)) > >+ reg_val |= (MEN_Z192_TX_BUF_LVL_DEF << 16) & > >+ MEN_Z192_TX_BUF_LVL; > >+ else > >+ reg_val |= (txlvl << 16) & MEN_Z192_TX_BUF_LVL; > >+ > >+ dev_info(priv->dev, "RX IRQ Level: %d TX IRQ Level: %d\n", > >+ rxlvl, txlvl); > >+ > >+ writel(reg_val, ®s->buf_lvl); > >+} > >+ > >+static void men_z192_set_rx_tout(struct net_device *ndev, int tout) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ int reg_val; > >+ > >+ if (tout < MEN_Z192_RX_TOUT_MIN || tout > MEN_Z192_RX_TOUT_MAX) > >+ reg_val = MEN_Z192_RX_TOUT_MAX; > >+ else > >+ reg_val = tout; > >+ > >+ dev_info(priv->dev, "RX IRQ timeout set to: %d\n", reg_val); > >+ > >+ writel(reg_val, ®s->rx_timeout); > >+} > >+ > >+static int men_z192_register(struct net_device *ndev) > >+{ > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ struct men_z192_regs __iomem *regs = priv->regs; > >+ u32 ctl_btr; > >+ int ret; > >+ > >+ /* The CAN controller should be always enabled. > >+ * There is no way to enable it if disabled. > >+ */ > >+ ctl_btr = readl(®s->ctl_btr); > >+ if (!(ctl_btr & MEN_Z192_CTL1_CANE)) > >+ return -ENODEV; > >+ > >+ men_z192_set_buf_lvl_irq(ndev, rxlvl, txlvl); > >+ men_z192_set_rx_tout(ndev, rx_timeout); > >+ > >+ ret = men_z192_req_init_mode(priv); > >+ if (ret) { > >+ dev_err(priv->dev, "failed to request init mode\n"); > >+ return ret; > >+ } > >+ > >+ return register_candev(ndev); > >+} > >+ > >+static void men_z192_unregister(struct net_device *ndev) > >+{ > >+ return unregister_candev(ndev); > >+} > >+ > >+static int men_z192_probe(struct mcb_device *mdev, > >+ const struct mcb_device_id *id) > >+{ > >+ struct device *dev = &mdev->dev; > >+ struct men_z192 *priv; > >+ struct net_device *ndev; > >+ void __iomem *dev_base; > >+ struct resource *mem; > >+ u32 timebase; > >+ int ret = 0; > >+ int irq; > >+ > >+ mem = mcb_request_mem(mdev, dev_name(dev)); > >+ if (IS_ERR(mem)) { > >+ dev_err(dev, "failed to request device memory"); > >+ return PTR_ERR(mem); > >+ } > >+ > >+ dev_base = ioremap(mem->start, resource_size(mem)); > >+ if (!dev_base) { > >+ dev_err(dev, "failed to ioremap device memory"); > >+ ret = -ENXIO; > >+ goto out_release; > >+ } > >+ > >+ irq = mcb_get_irq(mdev); > >+ if (irq <= 0) { > >+ ret = -ENODEV; > >+ goto out_unmap; > >+ } > >+ > >+ ndev = alloc_candev(sizeof(struct men_z192), 1); > > You specify here one echo_skb but it's not used anywhere. Local loopback > seems not to be implemented. > Agree with you, will set it to "0". > >+ if (!ndev) { > >+ dev_err(dev, "failed to allocate the can device"); > >+ ret = -ENOMEM; > >+ goto out_unmap; > >+ } > >+ > >+ ndev->netdev_ops = &men_z192_netdev_ops; > >+ ndev->irq = irq; > >+ > >+ priv = netdev_priv(ndev); > >+ priv->ndev = ndev; > >+ priv->dev = dev; > >+ > >+ priv->mem = mem; > >+ priv->dev_base = dev_base; > >+ priv->regs = priv->dev_base + MEN_Z192_REGS_OFFS; > >+ > >+ timebase = readl(&priv->regs->timebase); > >+ if (!timebase) { > >+ dev_err(dev, "invalid timebase configured (timebase=%d)\n", > >+ timebase); > >+ ret = -EINVAL; > >+ goto out_unmap; > > free_candev(ndev) is missing. Argh, will fix that. > > >+ } > >+ > >+ priv->can.clock.freq = timebase; > >+ priv->can.bittiming_const = &men_z192_bittiming_const; > >+ priv->can.do_set_mode = men_z192_set_mode; > >+ priv->can.do_get_berr_counter = men_z192_get_berr_counter; > >+ priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY | > >+ CAN_CTRLMODE_3_SAMPLES | > >+ CAN_CTRLMODE_LOOPBACK; > >+ > >+ spin_lock_init(&priv->lock); > >+ > >+ netif_napi_add(ndev, &priv->napi, men_z192_poll, > >+ MEN_Z192_NAPI_WEIGHT); > >+ > >+ mcb_set_drvdata(mdev, ndev); > >+ SET_NETDEV_DEV(ndev, dev); > >+ > >+ ret = men_z192_register(ndev); > >+ if (ret) { > >+ dev_err(dev, "failed to register CAN device"); > >+ goto out_free_candev; > >+ } > >+ > >+ dev_info(dev, "MEN 16z192 CAN driver successfully registered\n"); > >+ > >+ return 0; > >+ > >+out_free_candev: > >+ netif_napi_del(&priv->napi); > >+ free_candev(ndev); > >+out_unmap: > >+ iounmap(dev_base); > >+out_release: > >+ mcb_release_mem(mem); > >+ return ret; > >+} > >+ > >+static void men_z192_remove(struct mcb_device *mdev) > >+{ > >+ struct net_device *ndev = mcb_get_drvdata(mdev); > >+ struct men_z192 *priv = netdev_priv(ndev); > >+ > >+ men_z192_unregister(ndev); > >+ netif_napi_del(&priv->napi); > >+ > >+ iounmap(priv->dev_base); > >+ mcb_release_mem(priv->mem); > >+ > >+ free_candev(ndev); > >+} > >+ > >+static const struct mcb_device_id men_z192_ids[] = { > >+ { .device = 0xc0 }, > >+ { } > >+}; > >+MODULE_DEVICE_TABLE(mcb, men_z192_ids); > >+ > >+static struct mcb_driver men_z192_driver = { > >+ .driver = { > >+ .name = DRV_NAME, > >+ .owner = THIS_MODULE, > >+ }, > >+ .probe = men_z192_probe, > >+ .remove = men_z192_remove, > >+ .id_table = men_z192_ids, > >+}; > >+module_mcb_driver(men_z192_driver); > >+ > >+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>"); > >+MODULE_DESCRIPTION("MEN 16z192 CAN Controller"); > >+MODULE_LICENSE("GPL v2"); > >+MODULE_ALIAS("mcb:16z192"); > > Wolfgang. > Thanks for the Review Wolfgang. I will send a v2 in the next few days. Regards Andy
Hello, Am 08.08.2016 um 13:39 schrieb Andreas Werner: > On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote: >> Hello Andreas, >> >> a first quick review.... >> >> Am 26.07.2016 um 11:16 schrieb Andreas Werner: >>> This CAN Controller is found on MEN Chameleon FPGAs. >>> >>> The driver/device supports the CAN2.0 specification. >>> There are 255 RX and 255 Tx buffer within the IP. The >>> pointer for the buffer are handled by HW to make the >>> access from within the driver as simple as possible. >>> >>> The driver also supports parameters to configure the >>> buffer level interrupt for RX/TX as well as a RX timeout >>> interrupt. >>> >>> With this configuration options, the driver/device >>> provides flexibility for different types of usecases. >>> >>> Signed-off-by: Andreas Werner <andreas.werner@men.de> >>> --- >>> drivers/net/can/Kconfig | 10 + >>> drivers/net/can/Makefile | 1 + >>> drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 1000 insertions(+) >>> create mode 100644 drivers/net/can/men_z192_can.c ---snip--- >>> +/* Buffer level control values */ >>> +#define MEN_Z192_MIN_BUF_LVL 0 >>> +#define MEN_Z192_MAX_BUF_LVL 254 >>> +#define MEN_Z192_RX_BUF_LVL_DEF 5 >>> +#define MEN_Z192_TX_BUF_LVL_DEF 5 >>> +#define MEN_Z192_RX_TOUT_MIN 0 >>> +#define MEN_Z192_RX_TOUT_MAX 65535 >>> +#define MEN_Z192_RX_TOUT_DEF 1000 >>> + >>> +static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; >>> +module_param(txlvl, int, S_IRUGO); >>> +MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" >>> + __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); >>> + >>> +static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; >>> +module_param(rxlvl, int, S_IRUGO); >>> +MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" >>> + __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); >>> + >> >> What impact does the level have on the latency? Could you please add some >> comments. > > It has a impact on the latency. > rxlvl = 0 -> if one frame got received, a IRQ will be generated > rxlvl = 254 -> if 255 frames got received, a IRQ will be generated Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can be good for. The application usually wants the message as soon as possible. Anyway, the default should be *0*. For RX and TX. >>> +static int rx_timeout = MEN_Z192_RX_TOUT_DEF; >>> +module_param(rx_timeout, int, S_IRUGO); >>> +MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" >>> + __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); >> >> Ditto. What is "rx_timeout" good for. >> > > The rx timeout is used im combination with the rxlvl to assert the > if the buffer level is not reached within this timeout. What event will the application receive in case of a timeout. > Both, the timeout and the level are used to give the user as much > control over the latency and the IRQ handling as possible. > With this two options, the driver can be configured for different > use cases. > > I will add this as the comment to make it more clear. Even a bit more would be appreciated. ---snip--- >>> +static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr) >>> +{ >>> + struct net_device_stats *stats = &ndev->stats; >>> + struct men_z192 *priv = netdev_priv(ndev); >>> + struct men_z192_cf_buf __iomem *cf_buf; >>> + struct can_frame *cf; >>> + struct sk_buff *skb; >>> + u32 cf_offset; >>> + u32 length; >>> + u32 data; >>> + u32 id; >>> + >>> + skb = alloc_can_skb(ndev, &cf); >>> + if (unlikely(!skb)) { >>> + stats->rx_dropped++; >>> + return 0; >>> + } >>> + >>> + cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; >>> + >>> + cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; >>> + length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; >>> + id = readl(&cf_buf->can_id); >>> + >>> + if (id & MEN_Z192_CFBUF_IDE) { >>> + /* Extended frame */ >>> + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; >>> + cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> >>> + MEN_Z192_CFBUF_ID2_SHIFT; >>> + >>> + cf->can_id |= CAN_EFF_FLAG; >>> + >>> + if (id & MEN_Z192_CFBUF_E_RTR) >>> + cf->can_id |= CAN_RTR_FLAG; >>> + } else { >>> + /* Standard frame */ >>> + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> >>> + MEN_Z192_CFBUF_ID1_SHIFT; >>> + >>> + if (id & MEN_Z192_CFBUF_S_RTR) >>> + cf->can_id |= CAN_RTR_FLAG; >>> + } >>> + >>> + cf->can_dlc = get_can_dlc(length); >>> + >>> + /* remote transmission request frame >>> + * contains no data field even if the >>> + * data length is set to a value > 0 >>> + */ >>> + if (!(cf->can_id & CAN_RTR_FLAG)) { >>> + if (cf->can_dlc > 0) { >>> + data = readl(&cf_buf->data[0]); >>> + *(__be32 *)cf->data = cpu_to_be32(data); >> >> Do you really need the extra copy? >> >>> + } >>> + if (cf->can_dlc > 4) { >>> + data = readl(&cf_buf->data[1]); >>> + *(__be32 *)(cf->data + 4) = cpu_to_be32(data); >> >> Ditto. > > No its not really needed. I thought its more clean and more readable than > putting this in one line withouth the copy. It should be fast in the first place. >> >>> + } >>> + } >>> + >>> + stats->rx_bytes += cf->can_dlc; >>> + stats->rx_packets++; >>> + netif_receive_skb(skb); >>> + >>> + return 1; >> >>> +} ---snip--- >>> +static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev) >>> +{ >>> + struct can_frame *cf = (struct can_frame *)skb->data; >>> + struct men_z192 *priv = netdev_priv(ndev); >>> + struct men_z192_regs __iomem *regs = priv->regs; >>> + struct net_device_stats *stats = &ndev->stats; >>> + struct men_z192_cf_buf __iomem *cf_buf; >>> + u32 data[2] = {0, 0}; >>> + int status; >>> + u32 id; >>> + >>> + if (can_dropped_invalid_skb(ndev, skb)) >>> + return NETDEV_TX_OK; >>> + >>> + status = readl(®s->rx_tx_sts); >>> + >>> + if (MEN_Z192_TX_BUF_CNT(status) >= 255) { >>> + netif_stop_queue(ndev); >>> + netdev_err(ndev, "not enough space in TX buffer\n"); >>> + >>> + return NETDEV_TX_BUSY; >>> + } >> >> Please try to avoid NETDEV_TX_BUSY by stopping the queue earlier (if buf_cnt >> == 254). >> > > Agree with you, will fix that. > >>> + cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START; >>> + >>> + if (cf->can_id & CAN_EFF_FLAG) { >>> + /* Extended frame */ >>> + id = ((cf->can_id & CAN_EFF_MASK) << >>> + MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2; >>> + >>> + id |= (((cf->can_id & CAN_EFF_MASK) >> >>> + (CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) << >>> + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; >>> + >>> + id |= MEN_Z192_CFBUF_IDE; >>> + id |= MEN_Z192_CFBUF_SRR; >>> + >>> + if (cf->can_id & CAN_RTR_FLAG) >>> + id |= MEN_Z192_CFBUF_E_RTR; >>> + } else { >>> + /* Standard frame */ >>> + id = ((cf->can_id & CAN_SFF_MASK) << >>> + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; >>> + >>> + if (cf->can_id & CAN_RTR_FLAG) >>> + id |= MEN_Z192_CFBUF_S_RTR; >>> + } >>> + >>> + if (cf->can_dlc > 0) >>> + data[0] = be32_to_cpup((__be32 *)(cf->data)); >>> + if (cf->can_dlc > 3) >>> + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); >> >> Not necessary if RTR. >> > > Argh, right... > >>> + writel(id, &cf_buf->can_id); >>> + writel(cf->can_dlc, &cf_buf->length); >>> + >>> + if (!(cf->can_id & CAN_RTR_FLAG)) { >>> + writel(data[0], &cf_buf->data[0]); >>> + writel(data[1], &cf_buf->data[1]); >> >> Why do you not check cf->can_dlc here as well. And is the extra copy >> necessary. >> > > Yes, I agree with you. The extra copy could be also avoided. > >>> + >>> + stats->tx_bytes += cf->can_dlc; >>> + } >> >> If I look to other drivers, they write the data even in case of RTR. >> > > But why? > > A RTR does not have any data, therefore there is no need to write the data. > Only the length is required as the request size. Yes; I'm wondering as well. > > If there is a reason behind writing the data of a RTR frame, I can > change that, but for now there is no reason. Yep. > >>> + /* be sure everything is written to the >>> + * device before acknowledge the data. >>> + */ >>> + mmiowb(); >>> + >>> + /* trigger the transmission */ >>> + men_z192_ack_tx_pkg(priv, 1); >>> + >>> + stats->tx_packets++; >>> + >>> + kfree_skb(skb); >>> + >>> + return NETDEV_TX_OK; >>> +} >>> + >>> +static void men_z192_err_interrupt(struct net_device *ndev, u32 status) >>> +{ >>> + struct net_device_stats *stats = &ndev->stats; >>> + struct men_z192 *priv = netdev_priv(ndev); >>> + struct can_berr_counter bec; >>> + struct can_frame *cf; >>> + struct sk_buff *skb; >>> + enum can_state rx_state = 0, tx_state = 0; >>> + >>> + skb = alloc_can_err_skb(ndev, &cf); >>> + if (unlikely(!skb)) >>> + return; >>> + >>> + /* put the rx/tx error counter to >>> + * the additional controller specific >>> + * section of the error frame. >>> + */ >>> + men_z192_get_berr_counter(ndev, &bec); >>> + cf->data[6] = bec.txerr; >>> + cf->data[7] = bec.rxerr; >>> + >>> + /* overrun interrupt */ >>> + if (status & MEN_Z192_RFLG_OVRF) { >>> + cf->can_id |= CAN_ERR_CRTL; >>> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; >>> + stats->rx_over_errors++; >>> + stats->rx_errors++; >>> + } >>> + >>> + /* bus change interrupt */ >>> + if (status & MEN_Z192_RFLG_CSCIF) { >>> + rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; >>> + tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; >>> + can_change_state(ndev, cf, tx_state, rx_state); >>> + >>> + if (priv->can.state == CAN_STATE_BUS_OFF) >>> + can_bus_off(ndev); >>> + } >> >> Does the controller only provide state change events? What about other >> errors? >> > > I thought that somebody will ask. The controller does only the state change events > and the overrun error. Nothing more. > > I saw the error flags in many other drivers, but they are not existing > in my controller. No problem. Just to be sure. > >>> + >>> + stats->rx_packets++; >>> + stats->rx_bytes += cf->can_dlc; >>> + netif_receive_skb(skb); >>> +} ---snip--- >>> +void men_z192_set_can_state(struct net_device *ndev) >>> +{ >>> + struct men_z192 *priv = netdev_priv(ndev); >>> + struct men_z192_regs __iomem *regs = priv->regs; >>> + enum can_state rx_state, tx_state; >>> + u32 status; >>> + >>> + status = readl(®s->rx_tx_sts); >>> + >>> + rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; >>> + tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; >>> + >>> + priv->can.state = max(tx_state, rx_state); >>> +} >>> + >>> +static int men_z192_start(struct net_device *ndev) >>> +{ >>> + struct men_z192 *priv = netdev_priv(ndev); >>> + int ret; >>> + >>> + ret = men_z192_req_init_mode(priv); >>> + if (ret) >>> + return ret; >>> + >>> + ret = men_z192_set_bittiming(ndev); >>> + if (ret) >>> + return ret; >>> + >>> + ret = men_z192_req_run_mode(priv); >>> + if (ret) >>> + return ret; >>> + >>> + men_z192_init_idac(ndev); >>> + >>> + /* The 16z192 CAN IP does not reset the can bus state >>> + * if we enter the init mode. There is also >>> + * no software reset to reset the state machine. >>> + * We need to read the current state, and >>> + * inform the upper layer about the current state. >>> + */ >>> + men_z192_set_can_state(ndev); >> >> Hm, the application expected the state to be reset. Calling >> "can_change_state()" in "men_z192_set_can_state()" does make sense. >> > > Hm yes, but the IP is saving the state, this cannot be avoided. > can_change_state() makes sense yes, I will add it. > >>> + >>> + men_z192_set_int(priv, MEN_Z192_CAN_EN); >>> + >>> + return 0; >>> +} >>> + >>> +static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) >>> +{ >>> + int ret; >>> + >>> + switch (mode) { >>> + case CAN_MODE_START: >>> + ret = men_z192_start(ndev); >>> + if (ret) >>> + return ret; >> >> "if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and >> in many other places. >> > > Yes and no. I think its a general question about the naming of those variables. > I will check all the variables in the driver if it really makes sense > to rename it. > > For my opinion, "ret" is more generic. But you are right, "err" would be more > readable in some places. if (err) makes immediately clear that it's an error case. ret is more general, e.g. for the return value of read/write: if (ret < 0) error-case else if (ret == 0) end-of-file else btyes-read Just my personal preference to make the code more readable. >>> + >>> + netif_wake_queue(ndev); >>> + break; >>> + default: >>> + return -EOPNOTSUPP; >>> + } >>> + >>> + return 0; >>> +static int men_z192_probe(struct mcb_device *mdev, >>> + const struct mcb_device_id *id) >>> +{ >>> + struct device *dev = &mdev->dev; >>> + struct men_z192 *priv; >>> + struct net_device *ndev; >>> + void __iomem *dev_base; >>> + struct resource *mem; >>> + u32 timebase; >>> + int ret = 0; >>> + int irq; >>> + >>> + mem = mcb_request_mem(mdev, dev_name(dev)); >>> + if (IS_ERR(mem)) { >>> + dev_err(dev, "failed to request device memory"); >>> + return PTR_ERR(mem); >>> + } >>> + >>> + dev_base = ioremap(mem->start, resource_size(mem)); >>> + if (!dev_base) { >>> + dev_err(dev, "failed to ioremap device memory"); >>> + ret = -ENXIO; >>> + goto out_release; >>> + } >>> + >>> + irq = mcb_get_irq(mdev); >>> + if (irq <= 0) { >>> + ret = -ENODEV; >>> + goto out_unmap; >>> + } >>> + >>> + ndev = alloc_candev(sizeof(struct men_z192), 1); >> >> You specify here one echo_skb but it's not used anywhere. Local loopback >> seems not to be implemented. >> > > Agree with you, will set it to "0". No, the local loopback is mandetory! Wolfgang.
--- Original message --- > Date: Mon, 8 Aug 2016 14:28:39 +0200 > From: Wolfgang Grandegger <wg@grandegger.com> > [...] > >>>+ > >>>+ if (!(cf->can_id & CAN_RTR_FLAG)) { > >>>+ writel(data[0], &cf_buf->data[0]); > >>>+ writel(data[1], &cf_buf->data[1]); > >> > >>Why do you not check cf->can_dlc here as well. And is the extra copy > >>necessary. > >> > > > >Yes, I agree with you. The extra copy could be also avoided. > > > >>>+ > >>>+ stats->tx_bytes += cf->can_dlc; > >>>+ } > >> > >>If I look to other drivers, they write the data even in case of RTR. > >> > > > >But why? > > > >A RTR does not have any data, therefore there is no need to write the data. > >Only the length is required as the request size. > > Yes; I'm wondering as well. > > > > >If there is a reason behind writing the data of a RTR frame, I can > >change that, but for now there is no reason. > > Yep. I _think_ that copying the data without checking the RTR bit clearly avoids a condition and might produce faster code on some machines. In any case, it reads easier. I'm not sure how that interacts with caches etc etc. On the other hand, giving unused data is a bad habit that may reveal security information on some places, so better avoid it. Kurt
On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote: > Hello, > > Am 08.08.2016 um 13:39 schrieb Andreas Werner: > >On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote: > >>Hello Andreas, > >> > >>a first quick review.... > >> > >>Am 26.07.2016 um 11:16 schrieb Andreas Werner: > >>>This CAN Controller is found on MEN Chameleon FPGAs. > >>> > >>>The driver/device supports the CAN2.0 specification. > >>>There are 255 RX and 255 Tx buffer within the IP. The > >>>pointer for the buffer are handled by HW to make the > >>>access from within the driver as simple as possible. > >>> > >>>The driver also supports parameters to configure the > >>>buffer level interrupt for RX/TX as well as a RX timeout > >>>interrupt. > >>> > >>>With this configuration options, the driver/device > >>>provides flexibility for different types of usecases. > >>> > >>>Signed-off-by: Andreas Werner <andreas.werner@men.de> > >>>--- > >>>drivers/net/can/Kconfig | 10 + > >>>drivers/net/can/Makefile | 1 + > >>>drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++ > >>>3 files changed, 1000 insertions(+) > >>>create mode 100644 drivers/net/can/men_z192_can.c > > ---snip--- > > >>>+/* Buffer level control values */ > >>>+#define MEN_Z192_MIN_BUF_LVL 0 > >>>+#define MEN_Z192_MAX_BUF_LVL 254 > >>>+#define MEN_Z192_RX_BUF_LVL_DEF 5 > >>>+#define MEN_Z192_TX_BUF_LVL_DEF 5 > >>>+#define MEN_Z192_RX_TOUT_MIN 0 > >>>+#define MEN_Z192_RX_TOUT_MAX 65535 > >>>+#define MEN_Z192_RX_TOUT_DEF 1000 > >>>+ > >>>+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; > >>>+module_param(txlvl, int, S_IRUGO); > >>>+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" > >>>+ __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); > >>>+ > >>>+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; > >>>+module_param(rxlvl, int, S_IRUGO); > >>>+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" > >>>+ __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); > >>>+ > >> > >>What impact does the level have on the latency? Could you please add some > >>comments. > > > >It has a impact on the latency. > >rxlvl = 0 -> if one frame got received, a IRQ will be generated > >rxlvl = 254 -> if 255 frames got received, a IRQ will be generated > > Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can > be good for. The application usually wants the message as soon as possible. > Anyway, the default should be *0*. For RX and TX. > The HW provides such feature and the driver should be able to control it. It was developed to control the IRQ load (like NAPI) by defining a level of the buffer when the IRQ got asserted. I aggree with you to set the default to "0" which is the main usecase. > >>>+static int rx_timeout = MEN_Z192_RX_TOUT_DEF; > >>>+module_param(rx_timeout, int, S_IRUGO); > >>>+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" > >>>+ __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); > >> > >>Ditto. What is "rx_timeout" good for. > >> > > > >The rx timeout is used im combination with the rxlvl to assert the > >if the buffer level is not reached within this timeout. > > What event will the application receive in case of a timeout. > Its just to control the time when the RX IRQ will be asserted if the buffer level is not reached. Imagine if the rx_timeout is not existing and the rxlvl is set to 50 and only 30 packets are received. The RX IRQ will be never asserted. By defining the rx_timeout, we can control the time when the RX IRQ is asserted if the buffer level is not reached. The application does not receive any special signal, its just the RX IRQ. > >Both, the timeout and the level are used to give the user as much > >control over the latency and the IRQ handling as possible. > >With this two options, the driver can be configured for different > >use cases. > > > >I will add this as the comment to make it more clear. > > Even a bit more would be appreciated. > Sure... > > ---snip--- > > >>>+static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr) > >>>+{ > >>>+ struct net_device_stats *stats = &ndev->stats; > >>>+ struct men_z192 *priv = netdev_priv(ndev); > >>>+ struct men_z192_cf_buf __iomem *cf_buf; > >>>+ struct can_frame *cf; > >>>+ struct sk_buff *skb; > >>>+ u32 cf_offset; > >>>+ u32 length; > >>>+ u32 data; > >>>+ u32 id; > >>>+ > >>>+ skb = alloc_can_skb(ndev, &cf); > >>>+ if (unlikely(!skb)) { > >>>+ stats->rx_dropped++; > >>>+ return 0; > >>>+ } > >>>+ > >>>+ cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; > >>>+ > >>>+ cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; > >>>+ length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; > >>>+ id = readl(&cf_buf->can_id); > >>>+ > >>>+ if (id & MEN_Z192_CFBUF_IDE) { > >>>+ /* Extended frame */ > >>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; > >>>+ cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> > >>>+ MEN_Z192_CFBUF_ID2_SHIFT; > >>>+ > >>>+ cf->can_id |= CAN_EFF_FLAG; > >>>+ > >>>+ if (id & MEN_Z192_CFBUF_E_RTR) > >>>+ cf->can_id |= CAN_RTR_FLAG; > >>>+ } else { > >>>+ /* Standard frame */ > >>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> > >>>+ MEN_Z192_CFBUF_ID1_SHIFT; > >>>+ > >>>+ if (id & MEN_Z192_CFBUF_S_RTR) > >>>+ cf->can_id |= CAN_RTR_FLAG; > >>>+ } > >>>+ > >>>+ cf->can_dlc = get_can_dlc(length); > >>>+ > >>>+ /* remote transmission request frame > >>>+ * contains no data field even if the > >>>+ * data length is set to a value > 0 > >>>+ */ > >>>+ if (!(cf->can_id & CAN_RTR_FLAG)) { > >>>+ if (cf->can_dlc > 0) { > >>>+ data = readl(&cf_buf->data[0]); > >>>+ *(__be32 *)cf->data = cpu_to_be32(data); > >> > >>Do you really need the extra copy? > >> > >>>+ } > >>>+ if (cf->can_dlc > 4) { > >>>+ data = readl(&cf_buf->data[1]); > >>>+ *(__be32 *)(cf->data + 4) = cpu_to_be32(data); > >> > >>Ditto. > > > >No its not really needed. I thought its more clean and more readable than > >putting this in one line withouth the copy. > > It should be fast in the first place. > Ok, will change that. [...] > > >>>+static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) > >>>+{ > >>>+ int ret; > >>>+ > >>>+ switch (mode) { > >>>+ case CAN_MODE_START: > >>>+ ret = men_z192_start(ndev); > >>>+ if (ret) > >>>+ return ret; > >> > >>"if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and > >>in many other places. > >> > > > >Yes and no. I think its a general question about the naming of those variables. > >I will check all the variables in the driver if it really makes sense > >to rename it. > > > >For my opinion, "ret" is more generic. But you are right, "err" would be more > >readable in some places. > > if (err) > > makes immediately clear that it's an error case. ret is more general, e.g. > for the return value of read/write: > > if (ret < 0) > error-case > else if (ret == 0) > end-of-file > else > btyes-read > > Just my personal preference to make the code more readable. Ok, I will think about it. > > >>>+ > >>>+ netif_wake_queue(ndev); > >>>+ break; > >>>+ default: > >>>+ return -EOPNOTSUPP; > >>>+ } > >>>+ > >>>+ return 0; > >>>+static int men_z192_probe(struct mcb_device *mdev, > >>>+ const struct mcb_device_id *id) > >>>+{ > >>>+ struct device *dev = &mdev->dev; > >>>+ struct men_z192 *priv; > >>>+ struct net_device *ndev; > >>>+ void __iomem *dev_base; > >>>+ struct resource *mem; > >>>+ u32 timebase; > >>>+ int ret = 0; > >>>+ int irq; > >>>+ > >>>+ mem = mcb_request_mem(mdev, dev_name(dev)); > >>>+ if (IS_ERR(mem)) { > >>>+ dev_err(dev, "failed to request device memory"); > >>>+ return PTR_ERR(mem); > >>>+ } > >>>+ > >>>+ dev_base = ioremap(mem->start, resource_size(mem)); > >>>+ if (!dev_base) { > >>>+ dev_err(dev, "failed to ioremap device memory"); > >>>+ ret = -ENXIO; > >>>+ goto out_release; > >>>+ } > >>>+ > >>>+ irq = mcb_get_irq(mdev); > >>>+ if (irq <= 0) { > >>>+ ret = -ENODEV; > >>>+ goto out_unmap; > >>>+ } > >>>+ > >>>+ ndev = alloc_candev(sizeof(struct men_z192), 1); > >> > >>You specify here one echo_skb but it's not used anywhere. Local loopback > >>seems not to be implemented. > >> > > > >Agree with you, will set it to "0". > > No, the local loopback is mandetory! > Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c it is not mandatory. In the Documentation/networking/can.txt there is also a "should" and a fallback mechnism if the driver does not support the local loopback. I'm currently ok with this fallback mechanism. Anyway I am not sure that the driver can handle the echo skb correctly. If i understand it correctly, the can_get_echo_skb() is normally called on a "TX done IRQ" to get the skb and loop it back. I do not have such a "TX done IRQ" and have not implemented implemented and added the local looback. May be I can put and get the echo skb within the xmit function? Does this make sense? Regards Andy > Wolfgang.
On Mon, Aug 08, 2016 at 03:06:33PM +0200, Kurt Van Dijck wrote: > > --- Original message --- > > Date: Mon, 8 Aug 2016 14:28:39 +0200 > > From: Wolfgang Grandegger <wg@grandegger.com> > > > [...] > > >>>+ > > >>>+ if (!(cf->can_id & CAN_RTR_FLAG)) { > > >>>+ writel(data[0], &cf_buf->data[0]); > > >>>+ writel(data[1], &cf_buf->data[1]); > > >> > > >>Why do you not check cf->can_dlc here as well. And is the extra copy > > >>necessary. > > >> > > > > > >Yes, I agree with you. The extra copy could be also avoided. > > > > > >>>+ > > >>>+ stats->tx_bytes += cf->can_dlc; > > >>>+ } > > >> > > >>If I look to other drivers, they write the data even in case of RTR. > > >> > > > > > >But why? > > > > > >A RTR does not have any data, therefore there is no need to write the data. > > >Only the length is required as the request size. > > > > Yes; I'm wondering as well. > > > > > > > >If there is a reason behind writing the data of a RTR frame, I can > > >change that, but for now there is no reason. > > > > Yep. > > I _think_ that copying the data without checking the RTR bit clearly > avoids a condition and might produce faster code on some machines. > In any case, it reads easier. > I'm not sure how that interacts with caches etc etc. > > On the other hand, giving unused data is a bad habit that may reveal > security information on some places, so better avoid it. > > Kurt Hi Kurt, thanks for your comment. In my opinion, I really prever to NOT copying such data if the RTR flag ist set. Regards Andy
Am 08.08.2016 um 16:05 schrieb Andreas Werner: > On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote: >> Hello, >> >> Am 08.08.2016 um 13:39 schrieb Andreas Werner: >>> On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote: >>>> Hello Andreas, >>>> >>>> a first quick review.... >>>> >>>> Am 26.07.2016 um 11:16 schrieb Andreas Werner: >>>>> This CAN Controller is found on MEN Chameleon FPGAs. >>>>> >>>>> The driver/device supports the CAN2.0 specification. >>>>> There are 255 RX and 255 Tx buffer within the IP. The >>>>> pointer for the buffer are handled by HW to make the >>>>> access from within the driver as simple as possible. >>>>> >>>>> The driver also supports parameters to configure the >>>>> buffer level interrupt for RX/TX as well as a RX timeout >>>>> interrupt. >>>>> >>>>> With this configuration options, the driver/device >>>>> provides flexibility for different types of usecases. >>>>> >>>>> Signed-off-by: Andreas Werner <andreas.werner@men.de> >>>>> --- >>>>> drivers/net/can/Kconfig | 10 + >>>>> drivers/net/can/Makefile | 1 + >>>>> drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 1000 insertions(+) >>>>> create mode 100644 drivers/net/can/men_z192_can.c >> >> ---snip--- >> >>>>> +/* Buffer level control values */ >>>>> +#define MEN_Z192_MIN_BUF_LVL 0 >>>>> +#define MEN_Z192_MAX_BUF_LVL 254 >>>>> +#define MEN_Z192_RX_BUF_LVL_DEF 5 >>>>> +#define MEN_Z192_TX_BUF_LVL_DEF 5 >>>>> +#define MEN_Z192_RX_TOUT_MIN 0 >>>>> +#define MEN_Z192_RX_TOUT_MAX 65535 >>>>> +#define MEN_Z192_RX_TOUT_DEF 1000 >>>>> + >>>>> +static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; >>>>> +module_param(txlvl, int, S_IRUGO); >>>>> +MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" >>>>> + __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); >>>>> + >>>>> +static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; >>>>> +module_param(rxlvl, int, S_IRUGO); >>>>> +MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" >>>>> + __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); >>>>> + >>>> >>>> What impact does the level have on the latency? Could you please add some >>>> comments. >>> >>> It has a impact on the latency. >>> rxlvl = 0 -> if one frame got received, a IRQ will be generated >>> rxlvl = 254 -> if 255 frames got received, a IRQ will be generated >> >> Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can >> be good for. The application usually wants the message as soon as possible. >> Anyway, the default should be *0*. For RX and TX. >> > > The HW provides such feature and the driver should be able to control it. > It was developed to control the IRQ load (like NAPI) by defining a level of the buffer > when the IRQ got asserted. > > I aggree with you to set the default to "0" which is the main usecase. > >>>>> +static int rx_timeout = MEN_Z192_RX_TOUT_DEF; >>>>> +module_param(rx_timeout, int, S_IRUGO); >>>>> +MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" >>>>> + __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); >>>> >>>> Ditto. What is "rx_timeout" good for. >>>> >>> >>> The rx timeout is used im combination with the rxlvl to assert the >>> if the buffer level is not reached within this timeout. >> >> What event will the application receive in case of a timeout. >> > > Its just to control the time when the RX IRQ will be asserted if the buffer > level is not reached. > Imagine if the rx_timeout is not existing and the rxlvl is set to 50 and > only 30 packets are received. The RX IRQ will be never asserted. > > By defining the rx_timeout, we can control the time when the RX IRQ is asserted > if the buffer level is not reached. > > The application does not receive any special signal, its just the RX IRQ. Now I got it. After timeout an interrupt will be trigger regardless of the thresholds. The default settings should result in minimum latencies. >>> Both, the timeout and the level are used to give the user as much >>> control over the latency and the IRQ handling as possible. >>> With this two options, the driver can be configured for different >>> use cases. >>> >>> I will add this as the comment to make it more clear. >> >> Even a bit more would be appreciated. >> > > Sure... > >> >> ---snip--- >> >>>>> +static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr) >>>>> +{ >>>>> + struct net_device_stats *stats = &ndev->stats; >>>>> + struct men_z192 *priv = netdev_priv(ndev); >>>>> + struct men_z192_cf_buf __iomem *cf_buf; >>>>> + struct can_frame *cf; >>>>> + struct sk_buff *skb; >>>>> + u32 cf_offset; >>>>> + u32 length; >>>>> + u32 data; >>>>> + u32 id; >>>>> + >>>>> + skb = alloc_can_skb(ndev, &cf); >>>>> + if (unlikely(!skb)) { >>>>> + stats->rx_dropped++; >>>>> + return 0; >>>>> + } >>>>> + >>>>> + cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; >>>>> + >>>>> + cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; >>>>> + length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; >>>>> + id = readl(&cf_buf->can_id); >>>>> + >>>>> + if (id & MEN_Z192_CFBUF_IDE) { >>>>> + /* Extended frame */ >>>>> + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; >>>>> + cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> >>>>> + MEN_Z192_CFBUF_ID2_SHIFT; >>>>> + >>>>> + cf->can_id |= CAN_EFF_FLAG; >>>>> + >>>>> + if (id & MEN_Z192_CFBUF_E_RTR) >>>>> + cf->can_id |= CAN_RTR_FLAG; >>>>> + } else { >>>>> + /* Standard frame */ >>>>> + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> >>>>> + MEN_Z192_CFBUF_ID1_SHIFT; >>>>> + >>>>> + if (id & MEN_Z192_CFBUF_S_RTR) >>>>> + cf->can_id |= CAN_RTR_FLAG; >>>>> + } >>>>> + >>>>> + cf->can_dlc = get_can_dlc(length); >>>>> + >>>>> + /* remote transmission request frame >>>>> + * contains no data field even if the >>>>> + * data length is set to a value > 0 >>>>> + */ >>>>> + if (!(cf->can_id & CAN_RTR_FLAG)) { >>>>> + if (cf->can_dlc > 0) { >>>>> + data = readl(&cf_buf->data[0]); >>>>> + *(__be32 *)cf->data = cpu_to_be32(data); >>>> >>>> Do you really need the extra copy? >>>> >>>>> + } >>>>> + if (cf->can_dlc > 4) { >>>>> + data = readl(&cf_buf->data[1]); >>>>> + *(__be32 *)(cf->data + 4) = cpu_to_be32(data); >>>> >>>> Ditto. >>> >>> No its not really needed. I thought its more clean and more readable than >>> putting this in one line withouth the copy. >> >> It should be fast in the first place. >> > > Ok, will change that. > > [...] > >> >>>>> +static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + switch (mode) { >>>>> + case CAN_MODE_START: >>>>> + ret = men_z192_start(ndev); >>>>> + if (ret) >>>>> + return ret; >>>> >>>> "if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and >>>> in many other places. >>>> >>> >>> Yes and no. I think its a general question about the naming of those variables. >>> I will check all the variables in the driver if it really makes sense >>> to rename it. >>> >>> For my opinion, "ret" is more generic. But you are right, "err" would be more >>> readable in some places. >> >> if (err) >> >> makes immediately clear that it's an error case. ret is more general, e.g. >> for the return value of read/write: >> >> if (ret < 0) >> error-case >> else if (ret == 0) >> end-of-file >> else >> btyes-read >> >> Just my personal preference to make the code more readable. > > Ok, I will think about it. > >> >>>>> + >>>>> + netif_wake_queue(ndev); >>>>> + break; >>>>> + default: >>>>> + return -EOPNOTSUPP; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +static int men_z192_probe(struct mcb_device *mdev, >>>>> + const struct mcb_device_id *id) >>>>> +{ >>>>> + struct device *dev = &mdev->dev; >>>>> + struct men_z192 *priv; >>>>> + struct net_device *ndev; >>>>> + void __iomem *dev_base; >>>>> + struct resource *mem; >>>>> + u32 timebase; >>>>> + int ret = 0; >>>>> + int irq; >>>>> + >>>>> + mem = mcb_request_mem(mdev, dev_name(dev)); >>>>> + if (IS_ERR(mem)) { >>>>> + dev_err(dev, "failed to request device memory"); >>>>> + return PTR_ERR(mem); >>>>> + } >>>>> + >>>>> + dev_base = ioremap(mem->start, resource_size(mem)); >>>>> + if (!dev_base) { >>>>> + dev_err(dev, "failed to ioremap device memory"); >>>>> + ret = -ENXIO; >>>>> + goto out_release; >>>>> + } >>>>> + >>>>> + irq = mcb_get_irq(mdev); >>>>> + if (irq <= 0) { >>>>> + ret = -ENODEV; >>>>> + goto out_unmap; >>>>> + } >>>>> + >>>>> + ndev = alloc_candev(sizeof(struct men_z192), 1); >>>> >>>> You specify here one echo_skb but it's not used anywhere. Local loopback >>>> seems not to be implemented. >>>> >>> >>> Agree with you, will set it to "0". >> >> No, the local loopback is mandetory! >> > > Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c > it is not mandatory. In the Documentation/networking/can.txt > there is also a "should" and a fallback mechnism if the driver > does not support the local loopback. Well, s/driver/hardware/ ! Local loopback is the preferred mechanism. > I'm currently ok with this fallback mechanism. > > Anyway I am not sure that the driver can handle the echo skb correctly. > If i understand it correctly, the can_get_echo_skb() is normally called > on a "TX done IRQ" to get the skb and loop it back. > I do not have such a "TX done IRQ" and have not implemented implemented > and added the local looback. What does "MEN_Z192_TFLG_TXIF" signal? > May be I can put and get the echo skb within the xmit function? > Does this make sense? It only makes sense if the driver knows when one or more transfers are done. Wolfgang.
On 2016/08/08 09:26, Andreas Werner wrote: [...] > > > + > > > + if (cf->can_dlc > 0) > > > + data[0] = be32_to_cpup((__be32 *)(cf->data)); > > > + if (cf->can_dlc > 3) > > > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); > > > + > > > + writel(id, &cf_buf->can_id); > > > + writel(cf->can_dlc, &cf_buf->length); > > > + > > > + if (!(cf->can_id & CAN_RTR_FLAG)) { > > > + writel(data[0], &cf_buf->data[0]); > > > + writel(data[1], &cf_buf->data[1]); > > > + > > > + stats->tx_bytes += cf->can_dlc; > > > + } > > > + > > > + /* be sure everything is written to the > > > + * device before acknowledge the data. > > > + */ > > > + mmiowb(); > > > + > > > + /* trigger the transmission */ > > > + men_z192_ack_tx_pkg(priv, 1); > > > + > > > + stats->tx_packets++; > > > + > > > + kfree_skb(skb); > > > > What prevents the skb data to be freed/reused before the device has > > accessed it? I'm sorry, I hadn't realized that all of the data (all 8 bytes of it!) is written directly to the device. I was thinking about ethernet devices that dma packet data.
On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote: > Am 08.08.2016 um 16:05 schrieb Andreas Werner: > >On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote: > >>Hello, > >> > >>Am 08.08.2016 um 13:39 schrieb Andreas Werner: > >>>On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote: > >>>>Hello Andreas, > >>>> > >>>>a first quick review.... > >>>> > >>>>Am 26.07.2016 um 11:16 schrieb Andreas Werner: > >>>>>This CAN Controller is found on MEN Chameleon FPGAs. > >>>>> > >>>>>The driver/device supports the CAN2.0 specification. > >>>>>There are 255 RX and 255 Tx buffer within the IP. The > >>>>>pointer for the buffer are handled by HW to make the > >>>>>access from within the driver as simple as possible. > >>>>> > >>>>>The driver also supports parameters to configure the > >>>>>buffer level interrupt for RX/TX as well as a RX timeout > >>>>>interrupt. > >>>>> > >>>>>With this configuration options, the driver/device > >>>>>provides flexibility for different types of usecases. > >>>>> > >>>>>Signed-off-by: Andreas Werner <andreas.werner@men.de> > >>>>>--- > >>>>>drivers/net/can/Kconfig | 10 + > >>>>>drivers/net/can/Makefile | 1 + > >>>>>drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++ > >>>>>3 files changed, 1000 insertions(+) > >>>>>create mode 100644 drivers/net/can/men_z192_can.c > >> > >>---snip--- > >> > >>>>>+/* Buffer level control values */ > >>>>>+#define MEN_Z192_MIN_BUF_LVL 0 > >>>>>+#define MEN_Z192_MAX_BUF_LVL 254 > >>>>>+#define MEN_Z192_RX_BUF_LVL_DEF 5 > >>>>>+#define MEN_Z192_TX_BUF_LVL_DEF 5 > >>>>>+#define MEN_Z192_RX_TOUT_MIN 0 > >>>>>+#define MEN_Z192_RX_TOUT_MAX 65535 > >>>>>+#define MEN_Z192_RX_TOUT_DEF 1000 > >>>>>+ > >>>>>+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; > >>>>>+module_param(txlvl, int, S_IRUGO); > >>>>>+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" > >>>>>+ __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); > >>>>>+ > >>>>>+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; > >>>>>+module_param(rxlvl, int, S_IRUGO); > >>>>>+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" > >>>>>+ __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); > >>>>>+ > >>>> > >>>>What impact does the level have on the latency? Could you please add some > >>>>comments. > >>> > >>>It has a impact on the latency. > >>>rxlvl = 0 -> if one frame got received, a IRQ will be generated > >>>rxlvl = 254 -> if 255 frames got received, a IRQ will be generated > >> > >>Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can > >>be good for. The application usually wants the message as soon as possible. > >>Anyway, the default should be *0*. For RX and TX. > >> > > > >The HW provides such feature and the driver should be able to control it. > >It was developed to control the IRQ load (like NAPI) by defining a level of the buffer > >when the IRQ got asserted. > > > >I aggree with you to set the default to "0" which is the main usecase. > > > >>>>>+static int rx_timeout = MEN_Z192_RX_TOUT_DEF; > >>>>>+module_param(rx_timeout, int, S_IRUGO); > >>>>>+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" > >>>>>+ __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); > >>>> > >>>>Ditto. What is "rx_timeout" good for. > >>>> > >>> > >>>The rx timeout is used im combination with the rxlvl to assert the > >>>if the buffer level is not reached within this timeout. > >> > >>What event will the application receive in case of a timeout. > >> > > > >Its just to control the time when the RX IRQ will be asserted if the buffer > >level is not reached. > >Imagine if the rx_timeout is not existing and the rxlvl is set to 50 and > >only 30 packets are received. The RX IRQ will be never asserted. > > > >By defining the rx_timeout, we can control the time when the RX IRQ is asserted > >if the buffer level is not reached. > > > >The application does not receive any special signal, its just the RX IRQ. > > Now I got it. After timeout an interrupt will be trigger regardless of the > thresholds. The default settings should result in minimum latencies. > yes :-) I will set the rx_timeout to 0 to get the minimum latency. > >>>Both, the timeout and the level are used to give the user as much > >>>control over the latency and the IRQ handling as possible. > >>>With this two options, the driver can be configured for different > >>>use cases. > >>> > >>>I will add this as the comment to make it more clear. > >> > >>Even a bit more would be appreciated. > >> > > > >Sure... > > > >> > >>---snip--- > >> > >>>>>+static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr) > >>>>>+{ > >>>>>+ struct net_device_stats *stats = &ndev->stats; > >>>>>+ struct men_z192 *priv = netdev_priv(ndev); > >>>>>+ struct men_z192_cf_buf __iomem *cf_buf; > >>>>>+ struct can_frame *cf; > >>>>>+ struct sk_buff *skb; > >>>>>+ u32 cf_offset; > >>>>>+ u32 length; > >>>>>+ u32 data; > >>>>>+ u32 id; > >>>>>+ > >>>>>+ skb = alloc_can_skb(ndev, &cf); > >>>>>+ if (unlikely(!skb)) { > >>>>>+ stats->rx_dropped++; > >>>>>+ return 0; > >>>>>+ } > >>>>>+ > >>>>>+ cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; > >>>>>+ > >>>>>+ cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; > >>>>>+ length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; > >>>>>+ id = readl(&cf_buf->can_id); > >>>>>+ > >>>>>+ if (id & MEN_Z192_CFBUF_IDE) { > >>>>>+ /* Extended frame */ > >>>>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; > >>>>>+ cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> > >>>>>+ MEN_Z192_CFBUF_ID2_SHIFT; > >>>>>+ > >>>>>+ cf->can_id |= CAN_EFF_FLAG; > >>>>>+ > >>>>>+ if (id & MEN_Z192_CFBUF_E_RTR) > >>>>>+ cf->can_id |= CAN_RTR_FLAG; > >>>>>+ } else { > >>>>>+ /* Standard frame */ > >>>>>+ cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> > >>>>>+ MEN_Z192_CFBUF_ID1_SHIFT; > >>>>>+ > >>>>>+ if (id & MEN_Z192_CFBUF_S_RTR) > >>>>>+ cf->can_id |= CAN_RTR_FLAG; > >>>>>+ } > >>>>>+ > >>>>>+ cf->can_dlc = get_can_dlc(length); > >>>>>+ > >>>>>+ /* remote transmission request frame > >>>>>+ * contains no data field even if the > >>>>>+ * data length is set to a value > 0 > >>>>>+ */ > >>>>>+ if (!(cf->can_id & CAN_RTR_FLAG)) { > >>>>>+ if (cf->can_dlc > 0) { > >>>>>+ data = readl(&cf_buf->data[0]); > >>>>>+ *(__be32 *)cf->data = cpu_to_be32(data); > >>>> > >>>>Do you really need the extra copy? > >>>> > >>>>>+ } > >>>>>+ if (cf->can_dlc > 4) { > >>>>>+ data = readl(&cf_buf->data[1]); > >>>>>+ *(__be32 *)(cf->data + 4) = cpu_to_be32(data); > >>>> > >>>>Ditto. > >>> > >>>No its not really needed. I thought its more clean and more readable than > >>>putting this in one line withouth the copy. > >> > >>It should be fast in the first place. > >> > > > >Ok, will change that. > > > >[...] > > > >> > >>>>>+static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) > >>>>>+{ > >>>>>+ int ret; > >>>>>+ > >>>>>+ switch (mode) { > >>>>>+ case CAN_MODE_START: > >>>>>+ ret = men_z192_start(ndev); > >>>>>+ if (ret) > >>>>>+ return ret; > >>>> > >>>>"if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and > >>>>in many other places. > >>>> > >>> > >>>Yes and no. I think its a general question about the naming of those variables. > >>>I will check all the variables in the driver if it really makes sense > >>>to rename it. > >>> > >>>For my opinion, "ret" is more generic. But you are right, "err" would be more > >>>readable in some places. > >> > >> if (err) > >> > >>makes immediately clear that it's an error case. ret is more general, e.g. > >>for the return value of read/write: > >> > >> if (ret < 0) > >> error-case > >> else if (ret == 0) > >> end-of-file > >> else > >> btyes-read > >> > >>Just my personal preference to make the code more readable. > > > >Ok, I will think about it. > > > >> > >>>>>+ > >>>>>+ netif_wake_queue(ndev); > >>>>>+ break; > >>>>>+ default: > >>>>>+ return -EOPNOTSUPP; > >>>>>+ } > >>>>>+ > >>>>>+ return 0; > >>>>>+static int men_z192_probe(struct mcb_device *mdev, > >>>>>+ const struct mcb_device_id *id) > >>>>>+{ > >>>>>+ struct device *dev = &mdev->dev; > >>>>>+ struct men_z192 *priv; > >>>>>+ struct net_device *ndev; > >>>>>+ void __iomem *dev_base; > >>>>>+ struct resource *mem; > >>>>>+ u32 timebase; > >>>>>+ int ret = 0; > >>>>>+ int irq; > >>>>>+ > >>>>>+ mem = mcb_request_mem(mdev, dev_name(dev)); > >>>>>+ if (IS_ERR(mem)) { > >>>>>+ dev_err(dev, "failed to request device memory"); > >>>>>+ return PTR_ERR(mem); > >>>>>+ } > >>>>>+ > >>>>>+ dev_base = ioremap(mem->start, resource_size(mem)); > >>>>>+ if (!dev_base) { > >>>>>+ dev_err(dev, "failed to ioremap device memory"); > >>>>>+ ret = -ENXIO; > >>>>>+ goto out_release; > >>>>>+ } > >>>>>+ > >>>>>+ irq = mcb_get_irq(mdev); > >>>>>+ if (irq <= 0) { > >>>>>+ ret = -ENODEV; > >>>>>+ goto out_unmap; > >>>>>+ } > >>>>>+ > >>>>>+ ndev = alloc_candev(sizeof(struct men_z192), 1); > >>>> > >>>>You specify here one echo_skb but it's not used anywhere. Local loopback > >>>>seems not to be implemented. > >>>> > >>> > >>>Agree with you, will set it to "0". > >> > >>No, the local loopback is mandetory! > >> > > > >Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c > >it is not mandatory. In the Documentation/networking/can.txt > >there is also a "should" and a fallback mechnism if the driver > >does not support the local loopback. > > Well, s/driver/hardware/ ! Local loopback is the preferred mechanism. > Sure... > >I'm currently ok with this fallback mechanism. > > > >Anyway I am not sure that the driver can handle the echo skb correctly. > >If i understand it correctly, the can_get_echo_skb() is normally called > >on a "TX done IRQ" to get the skb and loop it back. > >I do not have such a "TX done IRQ" and have not implemented implemented > >and added the local looback. > > What does "MEN_Z192_TFLG_TXIF" signal? > It is not a "TX Done" IRQ, it is the tx buffer level IRQ. The IRQ is triggered when the number of available tx buffer entries is as configured with txlvl. (after the buffer was full) Example: txlvl = 0 tx buffer has 255 entries. -> The IRQ is triggered as soon as 1 frame got transmitted (254 entries). --- txlvl = 254 tx buffer has 255 entries. -> The IRQ is triggered as soon as the buffer has one entry and it got transmitted > >May be I can put and get the echo skb within the xmit function? > >Does this make sense? > > It only makes sense if the driver knows when one or more transfers are done. > Then i do not think that I can use the txlvl IRQ in this case and need to use the fallback mechanism. > Wolfgang.
On Mon, Aug 08, 2016 at 08:23:55PM -0700, Benjamin Poirier wrote: > On 2016/08/08 09:26, Andreas Werner wrote: > [...] > > > > + > > > > + if (cf->can_dlc > 0) > > > > + data[0] = be32_to_cpup((__be32 *)(cf->data)); > > > > + if (cf->can_dlc > 3) > > > > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); > > > > + > > > > + writel(id, &cf_buf->can_id); > > > > + writel(cf->can_dlc, &cf_buf->length); > > > > + > > > > + if (!(cf->can_id & CAN_RTR_FLAG)) { > > > > + writel(data[0], &cf_buf->data[0]); > > > > + writel(data[1], &cf_buf->data[1]); > > > > + > > > > + stats->tx_bytes += cf->can_dlc; > > > > + } > > > > + > > > > + /* be sure everything is written to the > > > > + * device before acknowledge the data. > > > > + */ > > > > + mmiowb(); > > > > + > > > > + /* trigger the transmission */ > > > > + men_z192_ack_tx_pkg(priv, 1); > > > > + > > > > + stats->tx_packets++; > > > > + > > > > + kfree_skb(skb); > > > > > > What prevents the skb data to be freed/reused before the device has > > > accessed it? > > I'm sorry, I hadn't realized that all of the data (all 8 bytes of it!) > is written directly to the device. I was thinking about ethernet devices > that dma packet data. Ah ok :-) I thought you just want me to free the skb earlier. Regards Andy
Hi Andreas, > Subject: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller > driver > > This CAN Controller is found on MEN Chameleon FPGAs. > > The driver/device supports the CAN2.0 specification. > There are 255 RX and 255 Tx buffer within the IP. The pointer for the > buffer are handled by HW to make the access from within the driver as > simple as possible. > > The driver also supports parameters to configure the buffer level > interrupt for RX/TX as well as a RX timeout interrupt. > > With this configuration options, the driver/device provides flexibility > for different types of use cases. Could you give us some example use case where these two configurations would be useful? What does it bring extra that cannot be achieved by normal NAPI mode itself? As the timeout is an absolute value, how does it fare when configured with say - interface up with 1Mbps bitrate - interface down - interface up with 10Kbps bitrate Should it be a % of bitrate rather than a module parameter if this configuration option is really needed? > > Signed-off-by: Andreas Werner <andreas.werner@men.de> > --- > drivers/net/can/Kconfig | 10 + > drivers/net/can/Makefile | 1 + > drivers/net/can/men_z192_can.c | 989 > +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 1000 insertions(+) > create mode 100644 drivers/net/can/men_z192_can.c > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index > 0d40aef..0fa0387 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -104,6 +104,16 @@ config CAN_JANZ_ICAN3 > This driver can also be built as a module. If so, the module will > be > called janz-ican3.ko. > > +config CAN_MEN_Z192 > + tristate "MEN 16Z192-00 CAN Controller" > + depends on MCB > + ---help--- > + Driver for MEN 16Z192-00 CAN Controller IP-Core, which > + is connected to the MEN Chameleon Bus. > + > + This driver can also be built as a module. If so, the module will > be > + called men_z192_can.ko. > + > config CAN_RCAR > tristate "Renesas R-Car CAN controller" > depends on ARCH_RENESAS || ARM > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile index > e3db0c8..eb206b3 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o > obj-$(CONFIG_CAN_GRCAN) += grcan.o > obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ > obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o > +obj-$(CONFIG_CAN_MEN_Z192) += men_z192_can.o > obj-$(CONFIG_CAN_MSCAN) += mscan/ > obj-$(CONFIG_CAN_M_CAN) += m_can/ > obj-$(CONFIG_CAN_RCAR) += rcar_can.o > diff --git a/drivers/net/can/men_z192_can.c > b/drivers/net/can/men_z192_can.c new file mode 100644 index > 0000000..b39ffee > --- /dev/null > +++ b/drivers/net/can/men_z192_can.c > @@ -0,0 +1,989 @@ > +/* > + * MEN 16Z192 CAN Controller driver > + * > + * Copyright (C) 2016 MEN Mikroelektronik GmbH (www.men.de) > + * > + * This program is free software; you can redistribute it and/or modify > +it > + * under the terms of the GNU General Public License as published by > +the Free > + * Software Foundation; version 2 of the License. > + */ > + > +#include <linux/netdevice.h> > +#include <linux/can/error.h> > +#include <linux/can/dev.h> > +#include <linux/bitops.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/mcb.h> > +#include <linux/can.h> > +#include <linux/io.h> > + > +#define DRV_NAME "z192_can" > + > +#define MEN_Z192_NAPI_WEIGHT 64 > +#define MEN_Z192_MODE_TOUT_US 40 > + > +/* CTL/BTR Register Bits */ > +#define MEN_Z192_CTL0_INITRQ BIT(0) > +#define MEN_Z192_CTL0_SLPRQ BIT(1) > +#define MEN_Z192_CTL1_INITAK BIT(8) > +#define MEN_Z192_CTL1_SLPAK BIT(9) > +#define MEN_Z192_CTL1_LISTEN BIT(12) > +#define MEN_Z192_CTL1_LOOPB BIT(13) > +#define MEN_Z192_CTL1_CANE BIT(15) > +#define MEN_Z192_BTR0_BRP(x) (((x) & 0x3f) << 16) > +#define MEN_Z192_BTR0_SJW(x) (((x) & 0x03) << 22) > +#define MEN_Z192_BTR1_TSEG1(x) (((x) & 0x0f) << 24) > +#define MEN_Z192_BTR1_TSEG2(x) (((x) & 0x07) << 28) > +#define MEN_Z192_BTR1_SAMP BIT(31) > + > +/* IER Interrupt Enable Register bits */ > +#define MEN_Z192_RXIE BIT(0) > +#define MEN_Z192_OVRIE BIT(1) > +#define MEN_Z192_CSCIE BIT(6) > +#define MEN_Z192_TOUTE BIT(7) > +#define MEN_Z192_TXIE BIT(16) > +#define MEN_Z192_ERRIE BIT(17) > + > +#define MEN_Z192_IRQ_ALL \ > + (MEN_Z192_RXIE | MEN_Z192_OVRIE | \ > + MEN_Z192_CSCIE | MEN_Z192_TOUTE | \ > + MEN_Z192_TXIE) > + > +#define MEN_Z192_IRQ_NAPI (MEN_Z192_RXIE | MEN_Z192_TOUTE) > + > +/* RX_TX_STAT RX/TX Status status register bits */ > +#define MEN_Z192_RX_BUF_CNT(x) ((x) & 0xff) > +#define MEN_Z192_TX_BUF_CNT(x) (((x) & 0xff00) >> 8) > +#define MEN_Z192_RFLG_RXIF BIT(16) > +#define MEN_Z192_RFLG_OVRF BIT(17) > +#define MEN_Z192_RFLG_TSTATE GENMASK(19, 18) > +#define MEN_Z192_RFLG_RSTATE GENMASK(21, 20) > +#define MEN_Z192_RFLG_CSCIF BIT(22) > +#define MEN_Z192_RFLG_TOUTF BIT(23) > +#define MEN_Z192_TFLG_TXIF BIT(24) > + > +#define MEN_Z192_GET_TSTATE(x) (((x) & MEN_Z192_RFLG_TSTATE) >> 18) > +#define MEN_Z192_GET_RSTATE(x) (((x) & MEN_Z192_RFLG_RSTATE) >> 20) > + > +#define MEN_Z192_IRQ_FLAGS_ALL \ > + (MEN_Z192_RFLG_RXIF | MEN_Z192_RFLG_OVRF | \ > + MEN_Z192_RFLG_TSTATE | MEN_Z192_RFLG_RSTATE | \ > + MEN_Z192_RFLG_CSCIF | MEN_Z192_RFLG_TOUTF | \ > + MEN_Z192_TFLG_TXIF) > + > +/* RX/TX Error counter bits */ > +#define MEN_Z192_GET_RX_ERR_CNT(x) ((x) & 0xff) > +#define MEN_Z192_GET_TX_ERR_CNT(x) (((x) & 0x00ff0000) >> 16) > + > +/* Buffer level register bits */ > +#define MEN_Z192_RX_BUF_LVL GENMASK(15, 0) > +#define MEN_Z192_TX_BUF_LVL GENMASK(31, 16) > + > +/* RX/TX Buffer register bits */ > +#define MEN_Z192_CFBUF_LEN GENMASK(3, 0) > +#define MEN_Z192_CFBUF_ID1 GENMASK(31, 21) > +#define MEN_Z192_CFBUF_ID2 GENMASK(18, 1) > +#define MEN_Z192_CFBUF_TS GENMASK(31, 8) > +#define MEN_Z192_CFBUF_E_RTR BIT(0) > +#define MEN_Z192_CFBUF_IDE BIT(19) > +#define MEN_Z192_CFBUF_SRR BIT(20) > +#define MEN_Z192_CFBUF_S_RTR BIT(20) > +#define MEN_Z192_CFBUF_ID2_SHIFT 1 > +#define MEN_Z192_CFBUF_ID1_SHIFT 21 > + > +/* Global register offsets */ > +#define MEN_Z192_RX_BUF_START 0x0000 > +#define MEN_Z192_TX_BUF_START 0x1000 > +#define MEN_Z192_REGS_OFFS 0x2000 > + > +/* Buffer level control values */ > +#define MEN_Z192_MIN_BUF_LVL 0 > +#define MEN_Z192_MAX_BUF_LVL 254 > +#define MEN_Z192_RX_BUF_LVL_DEF 5 > +#define MEN_Z192_TX_BUF_LVL_DEF 5 > +#define MEN_Z192_RX_TOUT_MIN 0 > +#define MEN_Z192_RX_TOUT_MAX 65535 > +#define MEN_Z192_RX_TOUT_DEF 1000 > + > +static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; module_param(txlvl, int, > +S_IRUGO); MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) > +0-254, default=" > + __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); > + > +static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; module_param(rxlvl, int, > +S_IRUGO); MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) > +0-254, default=" > + __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); > + > +static int rx_timeout = MEN_Z192_RX_TOUT_DEF; module_param(rx_timeout, > +int, S_IRUGO); MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec > +steps), default=" > + __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); > + > +struct men_z192_regs { > + u32 ctl_btr; /* Control and bus timing register */ > + u32 ier; /* Interrupt enable register */ > + u32 buf_lvl; /* Buffer level register */ > + u32 rxa; /* RX Data acknowledge register */ > + u32 txa; /* TX data acknowledge register */ > + u32 rx_tx_sts; /* RX/TX flags and buffer level */ > + u32 ovr_ecc_sts; /* Overrun/ECC status register */ > + u32 idac_ver; /* ID acceptance control / version */ > + u32 rx_tx_err; /* RX/TX error counter register */ > + u32 idar_0_to_3; /* ID acceptance register 0...3 */ > + u32 idar_4_to_7; /* ID acceptance register 4...7 */ > + u32 idmr_0_to_3; /* ID mask register 0...3 */ > + u32 idmr_4_to_7; /* ID mask register 4...7 */ > + u32 rx_timeout; /* receive timeout */ It would be nice to start all the comments with a capital letter or small letter - but uniformly. > + u32 timebase; /* Base frequency for baudrate calculation Unused variable? > */ > +}; > + > +struct men_z192 { > + struct can_priv can; > + struct napi_struct napi; > + struct net_device *ndev; > + struct device *dev; > + > + /* Lock for CTL_BTR register access. > + * This register combines bittiming bits > + * and the operation mode bits. > + * It is also used for bit r/m/w access > + * to all registers. > + */ > + spinlock_t lock; > + struct resource *mem; > + struct men_z192_regs __iomem *regs; > + void __iomem *dev_base; > +}; > + > +struct men_z192_cf_buf { > + u32 can_id; > + u32 data[2]; > + u32 length; > +}; > + > +enum men_z192_int_state { > + MEN_Z192_CAN_DIS = 0, > + MEN_Z192_CAN_EN, > + MEN_Z192_CAN_NAPI_DIS, > + MEN_Z192_CAN_NAPI_EN, > +}; > + > +static enum can_state bus_state_map[] = { > + CAN_STATE_ERROR_ACTIVE, > + CAN_STATE_ERROR_WARNING, > + CAN_STATE_ERROR_PASSIVE, > + CAN_STATE_BUS_OFF > +}; > + > +static const struct can_bittiming_const men_z192_bittiming_const = { > + .name = DRV_NAME, > + .tseg1_min = 4, > + .tseg1_max = 16, > + .tseg2_min = 2, > + .tseg2_max = 8, > + .sjw_max = 4, > + .brp_min = 2, > + .brp_max = 64, > + .brp_inc = 1, > +}; > + > +static inline void men_z192_bit_clr(struct men_z192 *priv, void __iomem > *addr, > + u32 mask) > +{ > + unsigned long flags; > + u32 val; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + val = readl(addr); > + val &= ~mask; > + writel(val, addr); > + > + spin_unlock_irqrestore(&priv->lock, flags); } > + > +static inline void men_z192_bit_set(struct men_z192 *priv, void __iomem > *addr, > + u32 mask) > +{ > + unsigned long flags; > + u32 val; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + val = readl(addr); > + val |= mask; > + writel(val, addr); > + > + spin_unlock_irqrestore(&priv->lock, flags); } > + > +static inline void men_z192_ack_rx_pkg(struct men_z192 *priv, > + unsigned int count) > +{ > + writel(count, &priv->regs->rxa); > +} > + > +static inline void men_z192_ack_tx_pkg(struct men_z192 *priv, > + unsigned int count) > +{ > + writel(count, &priv->regs->txa); > +} > + > +static void men_z192_set_int(struct men_z192 *priv, > + enum men_z192_int_state state) > +{ > + struct men_z192_regs __iomem *regs = priv->regs; > + > + switch (state) { > + case MEN_Z192_CAN_DIS: > + men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_ALL); > + break; > + > + case MEN_Z192_CAN_EN: > + men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_ALL); > + break; > + > + case MEN_Z192_CAN_NAPI_DIS: > + men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_NAPI); > + break; > + > + case MEN_Z192_CAN_NAPI_EN: > + men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_NAPI); > + break; > + > + default: > + netdev_err(priv->ndev, "invalid interrupt state\n"); > + break; > + } > +} > + > +static int men_z192_get_berr_counter(const struct net_device *ndev, > + struct can_berr_counter *bec) > +{ > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + u32 err_cnt; > + > + err_cnt = readl(®s->rx_tx_err); > + > + bec->txerr = MEN_Z192_GET_TX_ERR_CNT(err_cnt); > + bec->rxerr = MEN_Z192_GET_RX_ERR_CNT(err_cnt); > + > + return 0; > +} > + > +static int men_z192_req_run_mode(struct men_z192 *priv) { > + unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; > + struct men_z192_regs __iomem *regs = priv->regs; > + u32 val; > + > + men_z192_bit_clr(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); > + > + while (timeout--) { > + val = readl(®s->ctl_btr); > + if (!(val & MEN_Z192_CTL1_INITAK)) > + break; > + > + udelay(10); > + } > + Can this loop be replaced with readl_poll_timeout()? > + if (val & MEN_Z192_CTL1_INITAK) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int men_z192_req_init_mode(struct men_z192 *priv) { > + unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; > + struct men_z192_regs __iomem *regs = priv->regs; > + u32 val; > + > + men_z192_bit_set(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); > + > + while (timeout--) { > + val = readl(®s->ctl_btr); > + if (val & MEN_Z192_CTL1_INITAK) > + break; > + > + udelay(10); > + } > + > + if (!(val & MEN_Z192_CTL1_INITAK)) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int men_z192_read_frame(struct net_device *ndev, unsigned int > +frame_nr) { > + struct net_device_stats *stats = &ndev->stats; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_cf_buf __iomem *cf_buf; > + struct can_frame *cf; > + struct sk_buff *skb; > + u32 cf_offset; > + u32 length; > + u32 data; > + u32 id; > + > + skb = alloc_can_skb(ndev, &cf); > + if (unlikely(!skb)) { > + stats->rx_dropped++; > + return 0; > + } > + > + cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; > + > + cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; > + length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; > + id = readl(&cf_buf->can_id); > + > + if (id & MEN_Z192_CFBUF_IDE) { > + /* Extended frame */ > + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; > + cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> > + MEN_Z192_CFBUF_ID2_SHIFT; > + > + cf->can_id |= CAN_EFF_FLAG; > + > + if (id & MEN_Z192_CFBUF_E_RTR) > + cf->can_id |= CAN_RTR_FLAG; > + } else { > + /* Standard frame */ > + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> > + MEN_Z192_CFBUF_ID1_SHIFT; > + > + if (id & MEN_Z192_CFBUF_S_RTR) > + cf->can_id |= CAN_RTR_FLAG; > + } > + > + cf->can_dlc = get_can_dlc(length); > + > + /* remote transmission request frame > + * contains no data field even if the > + * data length is set to a value > 0 > + */ > + if (!(cf->can_id & CAN_RTR_FLAG)) { > + if (cf->can_dlc > 0) { > + data = readl(&cf_buf->data[0]); > + *(__be32 *)cf->data = cpu_to_be32(data); > + } > + if (cf->can_dlc > 4) { > + data = readl(&cf_buf->data[1]); > + *(__be32 *)(cf->data + 4) = cpu_to_be32(data); > + } > + } > + > + stats->rx_bytes += cf->can_dlc; > + stats->rx_packets++; > + netif_receive_skb(skb); > + > + return 1; > +} > + > +static int men_z192_poll(struct napi_struct *napi, int quota) { > + struct net_device *ndev = napi->dev; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + int work_done = 0; > + u32 frame_cnt; > + u32 status; > + > + status = readl(®s->rx_tx_sts); > + > + frame_cnt = MEN_Z192_RX_BUF_CNT(status); > + > + while (frame_cnt-- && (work_done < quota)) { > + work_done += men_z192_read_frame(ndev, 0); Why we need the second arg if we are always going to read from 0th offset? > + men_z192_ack_rx_pkg(priv, 1); > + } > + > + if (work_done < quota) { > + napi_complete(napi); > + men_z192_set_int(priv, MEN_Z192_CAN_NAPI_EN); > + } > + > + return work_done; > +} > + > +static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct can_frame *cf = (struct can_frame *)skb->data; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + struct net_device_stats *stats = &ndev->stats; > + struct men_z192_cf_buf __iomem *cf_buf; > + u32 data[2] = {0, 0}; > + int status; > + u32 id; > + > + if (can_dropped_invalid_skb(ndev, skb)) > + return NETDEV_TX_OK; > + > + status = readl(®s->rx_tx_sts); > + > + if (MEN_Z192_TX_BUF_CNT(status) >= 255) { Can this ever be > 255? > + netif_stop_queue(ndev); > + netdev_err(ndev, "not enough space in TX buffer\n"); > + > + return NETDEV_TX_BUSY; > + } > + > + cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START; > + > + if (cf->can_id & CAN_EFF_FLAG) { > + /* Extended frame */ > + id = ((cf->can_id & CAN_EFF_MASK) << > + MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2; > + > + id |= (((cf->can_id & CAN_EFF_MASK) >> > + (CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) << > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > + > + id |= MEN_Z192_CFBUF_IDE; > + id |= MEN_Z192_CFBUF_SRR; > + > + if (cf->can_id & CAN_RTR_FLAG) > + id |= MEN_Z192_CFBUF_E_RTR; > + } else { > + /* Standard frame */ > + id = ((cf->can_id & CAN_SFF_MASK) << > + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; > + > + if (cf->can_id & CAN_RTR_FLAG) > + id |= MEN_Z192_CFBUF_S_RTR; > + } > + > + if (cf->can_dlc > 0) > + data[0] = be32_to_cpup((__be32 *)(cf->data)); > + if (cf->can_dlc > 3) > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); > + > + writel(id, &cf_buf->can_id); > + writel(cf->can_dlc, &cf_buf->length); > + > + if (!(cf->can_id & CAN_RTR_FLAG)) { > + writel(data[0], &cf_buf->data[0]); > + writel(data[1], &cf_buf->data[1]); > + > + stats->tx_bytes += cf->can_dlc; > + } > + > + /* be sure everything is written to the > + * device before acknowledge the data. > + */ > + mmiowb(); > + > + /* trigger the transmission */ > + men_z192_ack_tx_pkg(priv, 1); > + > + stats->tx_packets++; > + Assuming MEN_Z192_TFLG_TXIF interrupt confirms frame transmission, should the stats we updated on the completion interrupt? You may also want to keep a local echo_skb[txlvl] if you have to implement local loopback. > + kfree_skb(skb); > + > + return NETDEV_TX_OK; > +} > + > +static void men_z192_err_interrupt(struct net_device *ndev, u32 status) > +{ > + struct net_device_stats *stats = &ndev->stats; > + struct men_z192 *priv = netdev_priv(ndev); > + struct can_berr_counter bec; > + struct can_frame *cf; > + struct sk_buff *skb; > + enum can_state rx_state = 0, tx_state = 0; > + > + skb = alloc_can_err_skb(ndev, &cf); > + if (unlikely(!skb)) > + return; > + > + /* put the rx/tx error counter to > + * the additional controller specific > + * section of the error frame. > + */ > + men_z192_get_berr_counter(ndev, &bec); > + cf->data[6] = bec.txerr; > + cf->data[7] = bec.rxerr; > + > + /* overrun interrupt */ > + if (status & MEN_Z192_RFLG_OVRF) { > + cf->can_id |= CAN_ERR_CRTL; > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > + stats->rx_over_errors++; > + stats->rx_errors++; > + } > + > + /* bus change interrupt */ > + if (status & MEN_Z192_RFLG_CSCIF) { > + rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; > + tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; > + can_change_state(ndev, cf, tx_state, rx_state); > + > + if (priv->can.state == CAN_STATE_BUS_OFF) > + can_bus_off(ndev); Should the priv->can.can_stats be updated? > + } > + > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + netif_receive_skb(skb); Should this be netif_rx(skb)? > +} > + > +static irqreturn_t men_z192_isr(int irq, void *dev_id) { > + struct net_device *ndev = dev_id; > + struct men_z192 *priv = netdev_priv(ndev); > + struct men_z192_regs __iomem *regs = priv->regs; > + bool handled = false; > + u32 irq_flags; > + u32 status; > + > + status = readl(®s->rx_tx_sts); > + > + irq_flags = status & MEN_Z192_IRQ_FLAGS_ALL; > + if (!irq_flags) > + goto out; > + > + /* It is save to write to RX_TS_STS[15:0] */ s/save/safe/ ? > + writel(irq_flags, ®s->rx_tx_sts); > + > + if (irq_flags & MEN_Z192_TFLG_TXIF) { > + netif_wake_queue(ndev); > + handled = true; > + } > + > + /* handle errors */ > + if ((irq_flags & MEN_Z192_RFLG_OVRF) || > + (irq_flags & MEN_Z192_RFLG_CSCIF)) { > + men_z192_err_interrupt(ndev, status); > + handled = true; > + } > + > + /* schedule NAPI if: > + * - rx IRQ > + * - rx timeout IRQ > + */ > + if ((irq_flags & MEN_Z192_RFLG_RXIF) || > + (irq_flags & MEN_Z192_RFLG_TOUTF)) { Is it a good idea to check if frames are really there to consume before napi_schedule? It could be just a periodic rx timeout interrupt? Still not convinced by that logic. Thanks, Ramesh
Am 09.08.2016 um 08:10 schrieb Andreas Werner: > On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote: >> Am 08.08.2016 um 16:05 schrieb Andreas Werner: >>> On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote: ---snip--- >>>>>>> + >>>>>>> + ndev = alloc_candev(sizeof(struct men_z192), 1); >>>>>> >>>>>> You specify here one echo_skb but it's not used anywhere. Local loopback >>>>>> seems not to be implemented. >>>>>> >>>>> >>>>> Agree with you, will set it to "0". >>>> >>>> No, the local loopback is mandetory! >>>> >>> >>> Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c >>> it is not mandatory. In the Documentation/networking/can.txt >>> there is also a "should" and a fallback mechnism if the driver >>> does not support the local loopback. >> >> Well, s/driver/hardware/ ! Local loopback is the preferred mechanism. >> > > Sure... > >>> I'm currently ok with this fallback mechanism. >>> >>> Anyway I am not sure that the driver can handle the echo skb correctly. >>> If i understand it correctly, the can_get_echo_skb() is normally called >>> on a "TX done IRQ" to get the skb and loop it back. >>> I do not have such a "TX done IRQ" and have not implemented implemented >>> and added the local looback. >> >> What does "MEN_Z192_TFLG_TXIF" signal? >> > > It is not a "TX Done" IRQ, it is the tx buffer level IRQ. > The IRQ is triggered when the number of available tx buffer entries is as > configured with txlvl. (after the buffer was full) > > Example: > txlvl = 0 > tx buffer has 255 entries. > > -> The IRQ is triggered as soon as 1 frame got transmitted (254 entries). > > --- > > txlvl = 254 > tx buffer has 255 entries. > > -> The IRQ is triggered as soon as the buffer has one entry and it got transmitted > > >>> May be I can put and get the echo skb within the xmit function? >>> Does this make sense? >> >> It only makes sense if the driver knows when one or more transfers are done. >> > > Then i do not think that I can use the txlvl IRQ in this case and need to use > the fallback mechanism. You could store "MEN_Z192_TX_BUF_CNT(readl(®s->rx_tx_sts))" in the start_xmit function and check again in the isr function to find out how much transfers have been transmitted in the meantime. Does it make sense? Wolfgang.
Hi Andreas, On 08/09/2016 08:10 AM, Andreas Werner wrote: > On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote: >>>>>> You specify here one echo_skb but it's not used anywhere. Local loopback >>>>>> seems not to be implemented. >>>>>> >>>>> >>>>> Agree with you, will set it to "0". >>>> >>>> No, the local loopback is mandetory! >>>> >>> >>> Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c >>> it is not mandatory. It is. Even those drivers that show up to use zero echo skbs in alloc_candev() implement the echo functionality correct. Just check 'git grep IFF_ECHO'. Even grcan.c and janz-ican3.c have IFF_ECHO set - but implement it in a different way without using the provided machanism from dev.c . >>> In the Documentation/networking/can.txt >>> there is also a "should" and a fallback mechnism if the driver >>> does not support the local loopback. But this fallback mechanism is bad - really bad! E.g. the slcan.c driver sends a stream of CAN frames without knowing whether the frames ever hit the wire. The slcan driver is more less for hobby users. The CAN frame echo with IFF_ECHO gives a correct representation of the traffic on the wire - including the correct timestamps. You really want to know whether a CAN frame was sent correctly on the bus instead of getting some shortcut info from inside the network layer. . >> >> Well, s/driver/hardware/ ! Local loopback is the preferred mechanism. >> > > Sure... > >>> I'm currently ok with this fallback mechanism. /me not. >>> Anyway I am not sure that the driver can handle the echo skb correctly. >>> If i understand it correctly, the can_get_echo_skb() is normally called >>> on a "TX done IRQ" to get the skb and loop it back. ack. >>> I do not have such a "TX done IRQ" and have not implemented implemented >>> and added the local looback. I'm not really sure how grcan.c and janz-ican3.c implemented the echo functionality but they must have faced a similar situation. A local loopback inside the CAN controller which is generated after successful transmit is an excellent implementation with excellent timestamps. The only problem for you is to detect the looped CAN frames and match them to the skb pointer of the outgoing frame to 'receive' the correct echo skb. When you send CAN frames to an unconnected CAN bus it can't be sent out due to the missing acknowledge from other nodes. So when you send frames and you get echo frames due to the fallback mode your cool CAN controller degrades to slcan level. Regards, Oliver ps. Do you have any URL where one can get the MEN 16Z192 spec?
On Wed, Aug 10, 2016 at 10:28:45PM +0200, Oliver Hartkopp wrote: > Hi Andreas, > > On 08/09/2016 08:10 AM, Andreas Werner wrote: > >On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote: > > >>>>>>You specify here one echo_skb but it's not used anywhere. Local loopback > >>>>>>seems not to be implemented. > >>>>>> > >>>>> > >>>>>Agree with you, will set it to "0". > >>>> > >>>>No, the local loopback is mandetory! > >>>> > >>> > >>>Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c > >>>it is not mandatory. > > It is. > > Even those drivers that show up to use zero echo skbs in alloc_candev() > implement the echo functionality correct. > > Just check 'git grep IFF_ECHO'. Even grcan.c and janz-ican3.c have IFF_ECHO > set - but implement it in a different way without using the provided > machanism from dev.c . > Ok I am with you. > >>>In the Documentation/networking/can.txt > >>>there is also a "should" and a fallback mechnism if the driver > >>>does not support the local loopback. > > But this fallback mechanism is bad - really bad! > > E.g. the slcan.c driver sends a stream of CAN frames without knowing whether > the frames ever hit the wire. The slcan driver is more less for hobby users. > The CAN frame echo with IFF_ECHO gives a correct representation of the > traffic on the wire - including the correct timestamps. > > You really want to know whether a CAN frame was sent correctly on the bus > instead of getting some shortcut info from inside the network layer. > . Thanks for the explanation. I make it more clear why its mandatory. > >> > >>Well, s/driver/hardware/ ! Local loopback is the preferred mechanism. > >> > > > >Sure... > > > >>>I'm currently ok with this fallback mechanism. > > /me not. > > >>>Anyway I am not sure that the driver can handle the echo skb correctly. > >>>If i understand it correctly, the can_get_echo_skb() is normally called > >>>on a "TX done IRQ" to get the skb and loop it back. > > ack. > > >>>I do not have such a "TX done IRQ" and have not implemented implemented > >>>and added the local looback. > > I'm not really sure how grcan.c and janz-ican3.c implemented the echo > functionality but they must have faced a similar situation. > I will check those driver to get more information about the implementation. > A local loopback inside the CAN controller which is generated after > successful transmit is an excellent implementation with excellent > timestamps. The only problem for you is to detect the looped CAN frames and > match them to the skb pointer of the outgoing frame to 'receive' the correct > echo skb. > At the moment, i think there is no way to detect those looped frames. I will talk to our IC designer and discuss this issue with him. Maybe we have the possibility to get a local loopback inside the CAN controller. This seems to be the best way to do it. > When you send CAN frames to an unconnected CAN bus it can't be sent out due > to the missing acknowledge from other nodes. So when you send frames and you > get echo frames due to the fallback mode your cool CAN controller degrades > to slcan level. > I agree with you. This is what we do not want to have. > Regards, > Oliver > > ps. Do you have any URL where one can get the MEN 16Z192 spec? No sorry, its not available. Regards Andy
On 08/11/2016 09:14 AM, Andreas Werner wrote: > On Wed, Aug 10, 2016 at 10:28:45PM +0200, Oliver Hartkopp wrote: >> Just check 'git grep IFF_ECHO'. Even grcan.c and janz-ican3.c have IFF_ECHO >> set - but implement it in a different way without using the provided >> machanism from dev.c . >> > > Ok I am with you. Great :-) >> A local loopback inside the CAN controller which is generated after >> successful transmit is an excellent implementation with excellent >> timestamps. The only problem for you is to detect the looped CAN frames and >> match them to the skb pointer of the outgoing frame to 'receive' the correct >> echo skb. >> > > At the moment, i think there is no way to detect those looped frames. > I will talk to our IC designer and discuss this issue with him. Maybe we > have the possibility to get a local loopback inside the CAN controller. > This seems to be the best way to do it. When you still have the possibility to change the IP core I would suggest to create some kind of 16/32 bit value which you can pass to the CAN controller along with the CAN frame to be sent. And when this frame comes back due to the loopback you can use this non-zero 16/32 bit value to match into a list of tx skb pointers for IFF_ECHO. E.g. when this 16/32 bit value is zero this CAN frame obviously was received from another CAN node. Just an idea. Regards, Oliver
On Thu, Aug 11, 2016 at 10:45:00AM +0200, Oliver Hartkopp wrote: > On 08/11/2016 09:14 AM, Andreas Werner wrote: > >On Wed, Aug 10, 2016 at 10:28:45PM +0200, Oliver Hartkopp wrote: > > >>Just check 'git grep IFF_ECHO'. Even grcan.c and janz-ican3.c have IFF_ECHO > >>set - but implement it in a different way without using the provided > >>machanism from dev.c . > >> > > > >Ok I am with you. > > Great :-) > > >>A local loopback inside the CAN controller which is generated after > >>successful transmit is an excellent implementation with excellent > >>timestamps. The only problem for you is to detect the looped CAN frames and > >>match them to the skb pointer of the outgoing frame to 'receive' the correct > >>echo skb. > >> > > > >At the moment, i think there is no way to detect those looped frames. > >I will talk to our IC designer and discuss this issue with him. Maybe we > >have the possibility to get a local loopback inside the CAN controller. > >This seems to be the best way to do it. > > When you still have the possibility to change the IP core I would suggest to > create some kind of 16/32 bit value which you can pass to the CAN controller > along with the CAN frame to be sent. > > And when this frame comes back due to the loopback you can use this non-zero > 16/32 bit value to match into a list of tx skb pointers for IFF_ECHO. > > E.g. when this 16/32 bit value is zero this CAN frame obviously was received > from another CAN node. > > Just an idea. > I am not sure if we have a way to change the IP but i will try to talk with my IC designer. He will be available next week. Your idea sounds good. I will check a few more driver to get more information how they did the implementation. > Regards, > Oliver Thanks your comments and explanations Oliver. Regards Andy
On 08/11/2016 10:58 AM, Andreas Werner wrote: > On Thu, Aug 11, 2016 at 10:45:00AM +0200, Oliver Hartkopp wrote: >> When you still have the possibility to change the IP core I would suggest to >> create some kind of 16/32 bit value which you can pass to the CAN controller >> along with the CAN frame to be sent. >> >> And when this frame comes back due to the loopback you can use this non-zero >> 16/32 bit value to match into a list of tx skb pointers for IFF_ECHO. >> >> E.g. when this 16/32 bit value is zero this CAN frame obviously was received >> from another CAN node. >> >> Just an idea. >> > > I am not sure if we have a way to change the IP but i will try to talk with > my IC designer. He will be available next week. > > Your idea sounds good. I will check a few more driver to get more information > how they did the implementation. > I just looked into your patch at http://marc.info/?l=linux-can&m=146952497113100&w=2 The struct men_z192_cf_buf { u32 can_id; u32 data[2]; u32 length; }; has a u32 for the length which is masked by #define MEN_Z192_CFBUF_LEN GENMASK(3, 0) in men_z192_read_frame() and is just copied in men_z192_xmit() writel(cf->can_dlc, &cf_buf->length); as only 4 bits are used in the u32 length you probably already can use the upper 16 bits for the discussed IFF_ECHO purpose. Don't know how your IP core handles this u32 length when you enable the loopback - maybe the upper 16 bits are still there in the receive path and you can implement this idea directly :-) Regards, Oliver
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index 0d40aef..0fa0387 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -104,6 +104,16 @@ config CAN_JANZ_ICAN3 This driver can also be built as a module. If so, the module will be called janz-ican3.ko. +config CAN_MEN_Z192 + tristate "MEN 16Z192-00 CAN Controller" + depends on MCB + ---help--- + Driver for MEN 16Z192-00 CAN Controller IP-Core, which + is connected to the MEN Chameleon Bus. + + This driver can also be built as a module. If so, the module will be + called men_z192_can.ko. + config CAN_RCAR tristate "Renesas R-Car CAN controller" depends on ARCH_RENESAS || ARM diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile index e3db0c8..eb206b3 100644 --- a/drivers/net/can/Makefile +++ b/drivers/net/can/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o obj-$(CONFIG_CAN_GRCAN) += grcan.o obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o +obj-$(CONFIG_CAN_MEN_Z192) += men_z192_can.o obj-$(CONFIG_CAN_MSCAN) += mscan/ obj-$(CONFIG_CAN_M_CAN) += m_can/ obj-$(CONFIG_CAN_RCAR) += rcar_can.o diff --git a/drivers/net/can/men_z192_can.c b/drivers/net/can/men_z192_can.c new file mode 100644 index 0000000..b39ffee --- /dev/null +++ b/drivers/net/can/men_z192_can.c @@ -0,0 +1,989 @@ +/* + * MEN 16Z192 CAN Controller driver + * + * Copyright (C) 2016 MEN Mikroelektronik GmbH (www.men.de) + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; version 2 of the License. + */ + +#include <linux/netdevice.h> +#include <linux/can/error.h> +#include <linux/can/dev.h> +#include <linux/bitops.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/mcb.h> +#include <linux/can.h> +#include <linux/io.h> + +#define DRV_NAME "z192_can" + +#define MEN_Z192_NAPI_WEIGHT 64 +#define MEN_Z192_MODE_TOUT_US 40 + +/* CTL/BTR Register Bits */ +#define MEN_Z192_CTL0_INITRQ BIT(0) +#define MEN_Z192_CTL0_SLPRQ BIT(1) +#define MEN_Z192_CTL1_INITAK BIT(8) +#define MEN_Z192_CTL1_SLPAK BIT(9) +#define MEN_Z192_CTL1_LISTEN BIT(12) +#define MEN_Z192_CTL1_LOOPB BIT(13) +#define MEN_Z192_CTL1_CANE BIT(15) +#define MEN_Z192_BTR0_BRP(x) (((x) & 0x3f) << 16) +#define MEN_Z192_BTR0_SJW(x) (((x) & 0x03) << 22) +#define MEN_Z192_BTR1_TSEG1(x) (((x) & 0x0f) << 24) +#define MEN_Z192_BTR1_TSEG2(x) (((x) & 0x07) << 28) +#define MEN_Z192_BTR1_SAMP BIT(31) + +/* IER Interrupt Enable Register bits */ +#define MEN_Z192_RXIE BIT(0) +#define MEN_Z192_OVRIE BIT(1) +#define MEN_Z192_CSCIE BIT(6) +#define MEN_Z192_TOUTE BIT(7) +#define MEN_Z192_TXIE BIT(16) +#define MEN_Z192_ERRIE BIT(17) + +#define MEN_Z192_IRQ_ALL \ + (MEN_Z192_RXIE | MEN_Z192_OVRIE | \ + MEN_Z192_CSCIE | MEN_Z192_TOUTE | \ + MEN_Z192_TXIE) + +#define MEN_Z192_IRQ_NAPI (MEN_Z192_RXIE | MEN_Z192_TOUTE) + +/* RX_TX_STAT RX/TX Status status register bits */ +#define MEN_Z192_RX_BUF_CNT(x) ((x) & 0xff) +#define MEN_Z192_TX_BUF_CNT(x) (((x) & 0xff00) >> 8) +#define MEN_Z192_RFLG_RXIF BIT(16) +#define MEN_Z192_RFLG_OVRF BIT(17) +#define MEN_Z192_RFLG_TSTATE GENMASK(19, 18) +#define MEN_Z192_RFLG_RSTATE GENMASK(21, 20) +#define MEN_Z192_RFLG_CSCIF BIT(22) +#define MEN_Z192_RFLG_TOUTF BIT(23) +#define MEN_Z192_TFLG_TXIF BIT(24) + +#define MEN_Z192_GET_TSTATE(x) (((x) & MEN_Z192_RFLG_TSTATE) >> 18) +#define MEN_Z192_GET_RSTATE(x) (((x) & MEN_Z192_RFLG_RSTATE) >> 20) + +#define MEN_Z192_IRQ_FLAGS_ALL \ + (MEN_Z192_RFLG_RXIF | MEN_Z192_RFLG_OVRF | \ + MEN_Z192_RFLG_TSTATE | MEN_Z192_RFLG_RSTATE | \ + MEN_Z192_RFLG_CSCIF | MEN_Z192_RFLG_TOUTF | \ + MEN_Z192_TFLG_TXIF) + +/* RX/TX Error counter bits */ +#define MEN_Z192_GET_RX_ERR_CNT(x) ((x) & 0xff) +#define MEN_Z192_GET_TX_ERR_CNT(x) (((x) & 0x00ff0000) >> 16) + +/* Buffer level register bits */ +#define MEN_Z192_RX_BUF_LVL GENMASK(15, 0) +#define MEN_Z192_TX_BUF_LVL GENMASK(31, 16) + +/* RX/TX Buffer register bits */ +#define MEN_Z192_CFBUF_LEN GENMASK(3, 0) +#define MEN_Z192_CFBUF_ID1 GENMASK(31, 21) +#define MEN_Z192_CFBUF_ID2 GENMASK(18, 1) +#define MEN_Z192_CFBUF_TS GENMASK(31, 8) +#define MEN_Z192_CFBUF_E_RTR BIT(0) +#define MEN_Z192_CFBUF_IDE BIT(19) +#define MEN_Z192_CFBUF_SRR BIT(20) +#define MEN_Z192_CFBUF_S_RTR BIT(20) +#define MEN_Z192_CFBUF_ID2_SHIFT 1 +#define MEN_Z192_CFBUF_ID1_SHIFT 21 + +/* Global register offsets */ +#define MEN_Z192_RX_BUF_START 0x0000 +#define MEN_Z192_TX_BUF_START 0x1000 +#define MEN_Z192_REGS_OFFS 0x2000 + +/* Buffer level control values */ +#define MEN_Z192_MIN_BUF_LVL 0 +#define MEN_Z192_MAX_BUF_LVL 254 +#define MEN_Z192_RX_BUF_LVL_DEF 5 +#define MEN_Z192_TX_BUF_LVL_DEF 5 +#define MEN_Z192_RX_TOUT_MIN 0 +#define MEN_Z192_RX_TOUT_MAX 65535 +#define MEN_Z192_RX_TOUT_DEF 1000 + +static int txlvl = MEN_Z192_TX_BUF_LVL_DEF; +module_param(txlvl, int, S_IRUGO); +MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default=" + __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")"); + +static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF; +module_param(rxlvl, int, S_IRUGO); +MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default=" + __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")"); + +static int rx_timeout = MEN_Z192_RX_TOUT_DEF; +module_param(rx_timeout, int, S_IRUGO); +MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default=" + __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")"); + +struct men_z192_regs { + u32 ctl_btr; /* Control and bus timing register */ + u32 ier; /* Interrupt enable register */ + u32 buf_lvl; /* Buffer level register */ + u32 rxa; /* RX Data acknowledge register */ + u32 txa; /* TX data acknowledge register */ + u32 rx_tx_sts; /* RX/TX flags and buffer level */ + u32 ovr_ecc_sts; /* Overrun/ECC status register */ + u32 idac_ver; /* ID acceptance control / version */ + u32 rx_tx_err; /* RX/TX error counter register */ + u32 idar_0_to_3; /* ID acceptance register 0...3 */ + u32 idar_4_to_7; /* ID acceptance register 4...7 */ + u32 idmr_0_to_3; /* ID mask register 0...3 */ + u32 idmr_4_to_7; /* ID mask register 4...7 */ + u32 rx_timeout; /* receive timeout */ + u32 timebase; /* Base frequency for baudrate calculation */ +}; + +struct men_z192 { + struct can_priv can; + struct napi_struct napi; + struct net_device *ndev; + struct device *dev; + + /* Lock for CTL_BTR register access. + * This register combines bittiming bits + * and the operation mode bits. + * It is also used for bit r/m/w access + * to all registers. + */ + spinlock_t lock; + struct resource *mem; + struct men_z192_regs __iomem *regs; + void __iomem *dev_base; +}; + +struct men_z192_cf_buf { + u32 can_id; + u32 data[2]; + u32 length; +}; + +enum men_z192_int_state { + MEN_Z192_CAN_DIS = 0, + MEN_Z192_CAN_EN, + MEN_Z192_CAN_NAPI_DIS, + MEN_Z192_CAN_NAPI_EN, +}; + +static enum can_state bus_state_map[] = { + CAN_STATE_ERROR_ACTIVE, + CAN_STATE_ERROR_WARNING, + CAN_STATE_ERROR_PASSIVE, + CAN_STATE_BUS_OFF +}; + +static const struct can_bittiming_const men_z192_bittiming_const = { + .name = DRV_NAME, + .tseg1_min = 4, + .tseg1_max = 16, + .tseg2_min = 2, + .tseg2_max = 8, + .sjw_max = 4, + .brp_min = 2, + .brp_max = 64, + .brp_inc = 1, +}; + +static inline void men_z192_bit_clr(struct men_z192 *priv, void __iomem *addr, + u32 mask) +{ + unsigned long flags; + u32 val; + + spin_lock_irqsave(&priv->lock, flags); + + val = readl(addr); + val &= ~mask; + writel(val, addr); + + spin_unlock_irqrestore(&priv->lock, flags); +} + +static inline void men_z192_bit_set(struct men_z192 *priv, void __iomem *addr, + u32 mask) +{ + unsigned long flags; + u32 val; + + spin_lock_irqsave(&priv->lock, flags); + + val = readl(addr); + val |= mask; + writel(val, addr); + + spin_unlock_irqrestore(&priv->lock, flags); +} + +static inline void men_z192_ack_rx_pkg(struct men_z192 *priv, + unsigned int count) +{ + writel(count, &priv->regs->rxa); +} + +static inline void men_z192_ack_tx_pkg(struct men_z192 *priv, + unsigned int count) +{ + writel(count, &priv->regs->txa); +} + +static void men_z192_set_int(struct men_z192 *priv, + enum men_z192_int_state state) +{ + struct men_z192_regs __iomem *regs = priv->regs; + + switch (state) { + case MEN_Z192_CAN_DIS: + men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_ALL); + break; + + case MEN_Z192_CAN_EN: + men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_ALL); + break; + + case MEN_Z192_CAN_NAPI_DIS: + men_z192_bit_clr(priv, ®s->ier, MEN_Z192_IRQ_NAPI); + break; + + case MEN_Z192_CAN_NAPI_EN: + men_z192_bit_set(priv, ®s->ier, MEN_Z192_IRQ_NAPI); + break; + + default: + netdev_err(priv->ndev, "invalid interrupt state\n"); + break; + } +} + +static int men_z192_get_berr_counter(const struct net_device *ndev, + struct can_berr_counter *bec) +{ + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + u32 err_cnt; + + err_cnt = readl(®s->rx_tx_err); + + bec->txerr = MEN_Z192_GET_TX_ERR_CNT(err_cnt); + bec->rxerr = MEN_Z192_GET_RX_ERR_CNT(err_cnt); + + return 0; +} + +static int men_z192_req_run_mode(struct men_z192 *priv) +{ + unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; + struct men_z192_regs __iomem *regs = priv->regs; + u32 val; + + men_z192_bit_clr(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); + + while (timeout--) { + val = readl(®s->ctl_btr); + if (!(val & MEN_Z192_CTL1_INITAK)) + break; + + udelay(10); + } + + if (val & MEN_Z192_CTL1_INITAK) + return -ETIMEDOUT; + + return 0; +} + +static int men_z192_req_init_mode(struct men_z192 *priv) +{ + unsigned int timeout = MEN_Z192_MODE_TOUT_US / 10; + struct men_z192_regs __iomem *regs = priv->regs; + u32 val; + + men_z192_bit_set(priv, ®s->ctl_btr, MEN_Z192_CTL0_INITRQ); + + while (timeout--) { + val = readl(®s->ctl_btr); + if (val & MEN_Z192_CTL1_INITAK) + break; + + udelay(10); + } + + if (!(val & MEN_Z192_CTL1_INITAK)) + return -ETIMEDOUT; + + return 0; +} + +static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr) +{ + struct net_device_stats *stats = &ndev->stats; + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_cf_buf __iomem *cf_buf; + struct can_frame *cf; + struct sk_buff *skb; + u32 cf_offset; + u32 length; + u32 data; + u32 id; + + skb = alloc_can_skb(ndev, &cf); + if (unlikely(!skb)) { + stats->rx_dropped++; + return 0; + } + + cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr; + + cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset; + length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN; + id = readl(&cf_buf->can_id); + + if (id & MEN_Z192_CFBUF_IDE) { + /* Extended frame */ + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3; + cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >> + MEN_Z192_CFBUF_ID2_SHIFT; + + cf->can_id |= CAN_EFF_FLAG; + + if (id & MEN_Z192_CFBUF_E_RTR) + cf->can_id |= CAN_RTR_FLAG; + } else { + /* Standard frame */ + cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> + MEN_Z192_CFBUF_ID1_SHIFT; + + if (id & MEN_Z192_CFBUF_S_RTR) + cf->can_id |= CAN_RTR_FLAG; + } + + cf->can_dlc = get_can_dlc(length); + + /* remote transmission request frame + * contains no data field even if the + * data length is set to a value > 0 + */ + if (!(cf->can_id & CAN_RTR_FLAG)) { + if (cf->can_dlc > 0) { + data = readl(&cf_buf->data[0]); + *(__be32 *)cf->data = cpu_to_be32(data); + } + if (cf->can_dlc > 4) { + data = readl(&cf_buf->data[1]); + *(__be32 *)(cf->data + 4) = cpu_to_be32(data); + } + } + + stats->rx_bytes += cf->can_dlc; + stats->rx_packets++; + netif_receive_skb(skb); + + return 1; +} + +static int men_z192_poll(struct napi_struct *napi, int quota) +{ + struct net_device *ndev = napi->dev; + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + int work_done = 0; + u32 frame_cnt; + u32 status; + + status = readl(®s->rx_tx_sts); + + frame_cnt = MEN_Z192_RX_BUF_CNT(status); + + while (frame_cnt-- && (work_done < quota)) { + work_done += men_z192_read_frame(ndev, 0); + men_z192_ack_rx_pkg(priv, 1); + } + + if (work_done < quota) { + napi_complete(napi); + men_z192_set_int(priv, MEN_Z192_CAN_NAPI_EN); + } + + return work_done; +} + +static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev) +{ + struct can_frame *cf = (struct can_frame *)skb->data; + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + struct net_device_stats *stats = &ndev->stats; + struct men_z192_cf_buf __iomem *cf_buf; + u32 data[2] = {0, 0}; + int status; + u32 id; + + if (can_dropped_invalid_skb(ndev, skb)) + return NETDEV_TX_OK; + + status = readl(®s->rx_tx_sts); + + if (MEN_Z192_TX_BUF_CNT(status) >= 255) { + netif_stop_queue(ndev); + netdev_err(ndev, "not enough space in TX buffer\n"); + + return NETDEV_TX_BUSY; + } + + cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START; + + if (cf->can_id & CAN_EFF_FLAG) { + /* Extended frame */ + id = ((cf->can_id & CAN_EFF_MASK) << + MEN_Z192_CFBUF_ID2_SHIFT) & MEN_Z192_CFBUF_ID2; + + id |= (((cf->can_id & CAN_EFF_MASK) >> + (CAN_EFF_ID_BITS - CAN_SFF_ID_BITS)) << + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; + + id |= MEN_Z192_CFBUF_IDE; + id |= MEN_Z192_CFBUF_SRR; + + if (cf->can_id & CAN_RTR_FLAG) + id |= MEN_Z192_CFBUF_E_RTR; + } else { + /* Standard frame */ + id = ((cf->can_id & CAN_SFF_MASK) << + MEN_Z192_CFBUF_ID1_SHIFT) & MEN_Z192_CFBUF_ID1; + + if (cf->can_id & CAN_RTR_FLAG) + id |= MEN_Z192_CFBUF_S_RTR; + } + + if (cf->can_dlc > 0) + data[0] = be32_to_cpup((__be32 *)(cf->data)); + if (cf->can_dlc > 3) + data[1] = be32_to_cpup((__be32 *)(cf->data + 4)); + + writel(id, &cf_buf->can_id); + writel(cf->can_dlc, &cf_buf->length); + + if (!(cf->can_id & CAN_RTR_FLAG)) { + writel(data[0], &cf_buf->data[0]); + writel(data[1], &cf_buf->data[1]); + + stats->tx_bytes += cf->can_dlc; + } + + /* be sure everything is written to the + * device before acknowledge the data. + */ + mmiowb(); + + /* trigger the transmission */ + men_z192_ack_tx_pkg(priv, 1); + + stats->tx_packets++; + + kfree_skb(skb); + + return NETDEV_TX_OK; +} + +static void men_z192_err_interrupt(struct net_device *ndev, u32 status) +{ + struct net_device_stats *stats = &ndev->stats; + struct men_z192 *priv = netdev_priv(ndev); + struct can_berr_counter bec; + struct can_frame *cf; + struct sk_buff *skb; + enum can_state rx_state = 0, tx_state = 0; + + skb = alloc_can_err_skb(ndev, &cf); + if (unlikely(!skb)) + return; + + /* put the rx/tx error counter to + * the additional controller specific + * section of the error frame. + */ + men_z192_get_berr_counter(ndev, &bec); + cf->data[6] = bec.txerr; + cf->data[7] = bec.rxerr; + + /* overrun interrupt */ + if (status & MEN_Z192_RFLG_OVRF) { + cf->can_id |= CAN_ERR_CRTL; + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; + stats->rx_over_errors++; + stats->rx_errors++; + } + + /* bus change interrupt */ + if (status & MEN_Z192_RFLG_CSCIF) { + rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; + tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; + can_change_state(ndev, cf, tx_state, rx_state); + + if (priv->can.state == CAN_STATE_BUS_OFF) + can_bus_off(ndev); + } + + stats->rx_packets++; + stats->rx_bytes += cf->can_dlc; + netif_receive_skb(skb); +} + +static irqreturn_t men_z192_isr(int irq, void *dev_id) +{ + struct net_device *ndev = dev_id; + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + bool handled = false; + u32 irq_flags; + u32 status; + + status = readl(®s->rx_tx_sts); + + irq_flags = status & MEN_Z192_IRQ_FLAGS_ALL; + if (!irq_flags) + goto out; + + /* It is save to write to RX_TS_STS[15:0] */ + writel(irq_flags, ®s->rx_tx_sts); + + if (irq_flags & MEN_Z192_TFLG_TXIF) { + netif_wake_queue(ndev); + handled = true; + } + + /* handle errors */ + if ((irq_flags & MEN_Z192_RFLG_OVRF) || + (irq_flags & MEN_Z192_RFLG_CSCIF)) { + men_z192_err_interrupt(ndev, status); + handled = true; + } + + /* schedule NAPI if: + * - rx IRQ + * - rx timeout IRQ + */ + if ((irq_flags & MEN_Z192_RFLG_RXIF) || + (irq_flags & MEN_Z192_RFLG_TOUTF)) { + men_z192_set_int(priv, MEN_Z192_CAN_NAPI_DIS); + napi_schedule(&priv->napi); + handled = true; + } + +out: + return IRQ_RETVAL(handled); +} + +static int men_z192_set_bittiming(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + const struct can_bittiming *bt = &priv->can.bittiming; + unsigned long flags; + u32 ctlbtr; + int ret = 0; + + spin_lock_irqsave(&priv->lock, flags); + + ctlbtr = readl(&priv->regs->ctl_btr); + + if (!(ctlbtr & MEN_Z192_CTL1_INITAK)) { + netdev_alert(ndev, + "cannot set bittiminig while in running mode\n"); + ret = -EPERM; + goto out_restore; + } + + ctlbtr &= ~(MEN_Z192_BTR0_BRP(0x3f) | + MEN_Z192_BTR0_SJW(0x03) | + MEN_Z192_BTR1_TSEG1(0x0f) | + MEN_Z192_BTR1_TSEG2(0x07) | + MEN_Z192_CTL1_LISTEN | + MEN_Z192_CTL1_LOOPB | + MEN_Z192_BTR1_SAMP); + + ctlbtr |= MEN_Z192_BTR0_BRP(bt->brp - 1) | + MEN_Z192_BTR0_SJW(bt->sjw - 1) | + MEN_Z192_BTR1_TSEG1(bt->phase_seg1 + bt->prop_seg - 1) | + MEN_Z192_BTR1_TSEG2(bt->phase_seg2 - 1); + + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) + ctlbtr |= MEN_Z192_BTR1_SAMP; + + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) + ctlbtr |= MEN_Z192_CTL1_LISTEN; + + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) + ctlbtr |= MEN_Z192_CTL1_LOOPB; + + netdev_dbg(ndev, "CTL_BTR=0x%08x\n", ctlbtr); + + writel(ctlbtr, &priv->regs->ctl_btr); + +out_restore: + spin_unlock_irqrestore(&priv->lock, flags); + + return ret; +} + +static void men_z192_init_idac(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + + /* hardware filtering (accept everything) */ + writel(0x00000000, ®s->idar_0_to_3); + writel(0x00000000, ®s->idar_4_to_7); + writel(0xffffffff, ®s->idmr_0_to_3); + writel(0xffffffff, ®s->idmr_4_to_7); +} + +void men_z192_set_can_state(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + enum can_state rx_state, tx_state; + u32 status; + + status = readl(®s->rx_tx_sts); + + rx_state = bus_state_map[MEN_Z192_GET_RSTATE(status)]; + tx_state = bus_state_map[MEN_Z192_GET_TSTATE(status)]; + + priv->can.state = max(tx_state, rx_state); +} + +static int men_z192_start(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + int ret; + + ret = men_z192_req_init_mode(priv); + if (ret) + return ret; + + ret = men_z192_set_bittiming(ndev); + if (ret) + return ret; + + ret = men_z192_req_run_mode(priv); + if (ret) + return ret; + + men_z192_init_idac(ndev); + + /* The 16z192 CAN IP does not reset the can bus state + * if we enter the init mode. There is also + * no software reset to reset the state machine. + * We need to read the current state, and + * inform the upper layer about the current state. + */ + men_z192_set_can_state(ndev); + + men_z192_set_int(priv, MEN_Z192_CAN_EN); + + return 0; +} + +static int men_z192_open(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + int ret; + + ret = open_candev(ndev); + if (ret) + return ret; + + ret = request_irq(ndev->irq, men_z192_isr, IRQF_SHARED, + ndev->name, ndev); + if (ret) + goto out_close; + + ret = men_z192_start(ndev); + if (ret) + goto out_free_irq; + + napi_enable(&priv->napi); + netif_start_queue(ndev); + + return 0; + +out_free_irq: + free_irq(ndev->irq, ndev); +out_close: + close_candev(ndev); + return ret; +} + +static int men_z192_stop(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + int ret; + + men_z192_set_int(priv, MEN_Z192_CAN_DIS); + + ret = men_z192_req_init_mode(priv); + if (ret) + return ret; + + priv->can.state = CAN_STATE_STOPPED; + + return 0; +} + +static int men_z192_close(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + int ret; + + netif_stop_queue(ndev); + + napi_disable(&priv->napi); + + ret = men_z192_stop(ndev); + + free_irq(ndev->irq, ndev); + + close_candev(ndev); + + return ret; +} + +static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode) +{ + int ret; + + switch (mode) { + case CAN_MODE_START: + ret = men_z192_start(ndev); + if (ret) + return ret; + + netif_wake_queue(ndev); + break; + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static const struct net_device_ops men_z192_netdev_ops = { + .ndo_open = men_z192_open, + .ndo_stop = men_z192_close, + .ndo_start_xmit = men_z192_xmit, + .ndo_change_mtu = can_change_mtu, +}; + +static int men_z192_verify_buf_lvl(int buffer_lvl) +{ + if (buffer_lvl < MEN_Z192_MIN_BUF_LVL || + buffer_lvl > MEN_Z192_MAX_BUF_LVL) + return -EINVAL; + + return 0; +} + +static void men_z192_set_buf_lvl_irq(struct net_device *ndev, int rxlvl, + int txlvl) +{ + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + int reg_val; + + if (men_z192_verify_buf_lvl(rxlvl)) + reg_val = MEN_Z192_RX_BUF_LVL_DEF & MEN_Z192_RX_BUF_LVL; + else + reg_val = rxlvl & MEN_Z192_RX_BUF_LVL; + + if (men_z192_verify_buf_lvl(txlvl)) + reg_val |= (MEN_Z192_TX_BUF_LVL_DEF << 16) & + MEN_Z192_TX_BUF_LVL; + else + reg_val |= (txlvl << 16) & MEN_Z192_TX_BUF_LVL; + + dev_info(priv->dev, "RX IRQ Level: %d TX IRQ Level: %d\n", + rxlvl, txlvl); + + writel(reg_val, ®s->buf_lvl); +} + +static void men_z192_set_rx_tout(struct net_device *ndev, int tout) +{ + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + int reg_val; + + if (tout < MEN_Z192_RX_TOUT_MIN || tout > MEN_Z192_RX_TOUT_MAX) + reg_val = MEN_Z192_RX_TOUT_MAX; + else + reg_val = tout; + + dev_info(priv->dev, "RX IRQ timeout set to: %d\n", reg_val); + + writel(reg_val, ®s->rx_timeout); +} + +static int men_z192_register(struct net_device *ndev) +{ + struct men_z192 *priv = netdev_priv(ndev); + struct men_z192_regs __iomem *regs = priv->regs; + u32 ctl_btr; + int ret; + + /* The CAN controller should be always enabled. + * There is no way to enable it if disabled. + */ + ctl_btr = readl(®s->ctl_btr); + if (!(ctl_btr & MEN_Z192_CTL1_CANE)) + return -ENODEV; + + men_z192_set_buf_lvl_irq(ndev, rxlvl, txlvl); + men_z192_set_rx_tout(ndev, rx_timeout); + + ret = men_z192_req_init_mode(priv); + if (ret) { + dev_err(priv->dev, "failed to request init mode\n"); + return ret; + } + + return register_candev(ndev); +} + +static void men_z192_unregister(struct net_device *ndev) +{ + return unregister_candev(ndev); +} + +static int men_z192_probe(struct mcb_device *mdev, + const struct mcb_device_id *id) +{ + struct device *dev = &mdev->dev; + struct men_z192 *priv; + struct net_device *ndev; + void __iomem *dev_base; + struct resource *mem; + u32 timebase; + int ret = 0; + int irq; + + mem = mcb_request_mem(mdev, dev_name(dev)); + if (IS_ERR(mem)) { + dev_err(dev, "failed to request device memory"); + return PTR_ERR(mem); + } + + dev_base = ioremap(mem->start, resource_size(mem)); + if (!dev_base) { + dev_err(dev, "failed to ioremap device memory"); + ret = -ENXIO; + goto out_release; + } + + irq = mcb_get_irq(mdev); + if (irq <= 0) { + ret = -ENODEV; + goto out_unmap; + } + + ndev = alloc_candev(sizeof(struct men_z192), 1); + if (!ndev) { + dev_err(dev, "failed to allocate the can device"); + ret = -ENOMEM; + goto out_unmap; + } + + ndev->netdev_ops = &men_z192_netdev_ops; + ndev->irq = irq; + + priv = netdev_priv(ndev); + priv->ndev = ndev; + priv->dev = dev; + + priv->mem = mem; + priv->dev_base = dev_base; + priv->regs = priv->dev_base + MEN_Z192_REGS_OFFS; + + timebase = readl(&priv->regs->timebase); + if (!timebase) { + dev_err(dev, "invalid timebase configured (timebase=%d)\n", + timebase); + ret = -EINVAL; + goto out_unmap; + } + + priv->can.clock.freq = timebase; + priv->can.bittiming_const = &men_z192_bittiming_const; + priv->can.do_set_mode = men_z192_set_mode; + priv->can.do_get_berr_counter = men_z192_get_berr_counter; + priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY | + CAN_CTRLMODE_3_SAMPLES | + CAN_CTRLMODE_LOOPBACK; + + spin_lock_init(&priv->lock); + + netif_napi_add(ndev, &priv->napi, men_z192_poll, + MEN_Z192_NAPI_WEIGHT); + + mcb_set_drvdata(mdev, ndev); + SET_NETDEV_DEV(ndev, dev); + + ret = men_z192_register(ndev); + if (ret) { + dev_err(dev, "failed to register CAN device"); + goto out_free_candev; + } + + dev_info(dev, "MEN 16z192 CAN driver successfully registered\n"); + + return 0; + +out_free_candev: + netif_napi_del(&priv->napi); + free_candev(ndev); +out_unmap: + iounmap(dev_base); +out_release: + mcb_release_mem(mem); + return ret; +} + +static void men_z192_remove(struct mcb_device *mdev) +{ + struct net_device *ndev = mcb_get_drvdata(mdev); + struct men_z192 *priv = netdev_priv(ndev); + + men_z192_unregister(ndev); + netif_napi_del(&priv->napi); + + iounmap(priv->dev_base); + mcb_release_mem(priv->mem); + + free_candev(ndev); +} + +static const struct mcb_device_id men_z192_ids[] = { + { .device = 0xc0 }, + { } +}; +MODULE_DEVICE_TABLE(mcb, men_z192_ids); + +static struct mcb_driver men_z192_driver = { + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + }, + .probe = men_z192_probe, + .remove = men_z192_remove, + .id_table = men_z192_ids, +}; +module_mcb_driver(men_z192_driver); + +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>"); +MODULE_DESCRIPTION("MEN 16z192 CAN Controller"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("mcb:16z192");
This CAN Controller is found on MEN Chameleon FPGAs. The driver/device supports the CAN2.0 specification. There are 255 RX and 255 Tx buffer within the IP. The pointer for the buffer are handled by HW to make the access from within the driver as simple as possible. The driver also supports parameters to configure the buffer level interrupt for RX/TX as well as a RX timeout interrupt. With this configuration options, the driver/device provides flexibility for different types of usecases. Signed-off-by: Andreas Werner <andreas.werner@men.de> --- drivers/net/can/Kconfig | 10 + drivers/net/can/Makefile | 1 + drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1000 insertions(+) create mode 100644 drivers/net/can/men_z192_can.c