Sascha Hauer wrote:

On Fri, Jul 24, 2009 at 04:55:02PM +0200, Wolfgang Grandegger wrote:

quoted

Hi Sascha,

Sascha Hauer wrote:

quoted

Hi,

This patch adds support for the Freescale FlexCAN CAN controller.
The driver has been tested on an i.MX25 SoC with bitrates up to
1Mbit, remote frames and standard and extenden frames.

Please review and consider for inclusion.

See below...

quoted

Sascha

[snip]

quoted

quoted

+static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct can_frame *frame = (struct can_frame *)skb->data;
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_regs __iomem *regs = priv->base;
+	u32 can_id;
+	u32 dlc = MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
+	u32 *can_data;
+
+	netif_stop_queue(dev);
+
+	if (frame->can_id & CAN_EFF_FLAG) {
+		can_id = frame->can_id & MB_ID_EXT;
+		dlc |= MB_CNT_IDE | MB_CNT_SRR;
+	} else {
+		can_id = (frame->can_id & CAN_SFF_MASK) << 18;
+	}
+
+	if (frame->can_id & CAN_RTR_FLAG)
+		dlc |= MB_CNT_RTR;
+
+	writel(dlc, &regs->cantxfg[TX_BUF_ID].can_dlc);
+	writel(can_id, &regs->cantxfg[TX_BUF_ID].can_id);
+
+	can_data = (u32 *)frame->data;
+	writel(cpu_to_be32(*can_data), &regs->cantxfg[TX_BUF_ID].data[0]);
+	writel(cpu_to_be32(*(can_data + 1)), &regs->cantxfg[TX_BUF_ID].data[1]);
+
+	writel(dlc, &regs->cantxfg[TX_BUF_ID].can_dlc);
+
+	kfree_skb(skb);

Support for echo skb using can_put/get_echo_skb() is missing. It should
not be a big deal to add it.

In fact it's not missing, but the hardware is configured to receive its
own packets, so this isn't needed.

Ah, OK, I assume that the message is looped back not before it went out
to the bus to ensure proper time ordering.

quoted

quoted

+
+	return NETDEV_TX_OK;
+}
+
+static void flexcan_rx_frame(struct net_device *ndev,
+		struct flexcan_mb __iomem *mb)
+{
+	struct net_device_stats *stats = &ndev->stats;
+	struct sk_buff *skb;
+	struct can_frame *frame;
+	int ctrl = readl(&mb->can_dlc);
+	int length = (ctrl >> 16) & 0x0f;
+	u32 id;
+
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (!skb) {
+		stats->rx_dropped++;
+		return;
+	}
+
+	frame = (struct can_frame *)skb_put(skb,
+			sizeof(struct can_frame));
+
+	frame->can_dlc = length;
+	id = readl(&mb->can_id) & MB_ID_EXT;
+
+	if (ctrl & MB_CNT_IDE) {
+		frame->can_id = id & CAN_EFF_MASK;
+		frame->can_id |= CAN_EFF_FLAG;
+		if (ctrl & MB_CNT_RTR)
+			frame->can_id |= CAN_RTR_FLAG;
+	} else {
+		frame->can_id  = id >> 18;
+		if (ctrl & MB_CNT_RTR)
+			frame->can_id |= CAN_RTR_FLAG;
+	}
+
+	*(u32 *)frame->data = be32_to_cpu(readl(&mb->data[0]));
+	*((u32 *)frame->data + 1) = be32_to_cpu(readl(&mb->data[1]));
+
+	stats->rx_packets++;
+	stats->rx_bytes += frame->can_dlc;
+	skb->dev = ndev;
+	skb->protocol = __constant_htons(ETH_P_CAN);
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	netif_rx(skb);
+}
+
+static void flexcan_error(struct net_device *ndev, u32 stat)
+{
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	struct flexcan_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	enum can_state state = priv->can.state;
+	int error_warning = 0, rx_errors = 0;
+
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (!skb)
+		return;
+
+	skb->dev = ndev;
+	skb->protocol = htons(ETH_P_CAN);

	skb->protocol = __constant_htons(ETH_P_CAN);
	skb->ip_summed = CHECKSUM_UNNECESSARY;

as above?!

Ok

quoted

quoted

+
+	cf = (struct can_frame *)skb_put(skb, sizeof(*cf));
+	memset(cf, 0, sizeof(*cf));
+
+	cf->can_id = CAN_ERR_FLAG;
+	cf->can_dlc = CAN_ERR_DLC;
+
+	if (stat & ERRSTAT_RWRNINT) {
+		error_warning = 1;
+		state = CAN_STATE_ERROR_WARNING;
+		cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
+	}
+
+	if (stat & ERRSTAT_TWRNINT) {
+		error_warning = 1;
+		state = CAN_STATE_ERROR_WARNING;
+		cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
+	}
+
+	switch ((stat >> 4) & 0x3) {
+	case 0:
+		state = CAN_STATE_ERROR_ACTIVE;
+		break;

Does the device recover autmatically from the bus-off state? Can
automatic recovery be configured (switched off?).

The device *can* be configured to automatically recover from bus off,
but I didn't use this feature to be more conform to the Linux CAN API.

Good.

quoted

quoted

+	case 1:
+		state = CAN_STATE_ERROR_PASSIVE;
+		break;
+	default:
+		state = CAN_STATE_BUS_OFF;
+		break;
+	}

You seem to handle a state change to error passive like a change to
error warning.

quoted

+	if (stat & ERRSTAT_BOFFINT) {
+		cf->can_id |= CAN_ERR_BUSOFF;
+		can_bus_off(ndev);
+	}
+
+	if (stat & ERRSTAT_BIT1ERR) {

		rx_error = 1; ???

quoted

+		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+		cf->data[2] |= CAN_ERR_PROT_BIT1;
+	}
+
+	if (stat & ERRSTAT_BIT0ERR) {

		rx_error = 1; ???

quoted

+		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+		cf->data[2] |= CAN_ERR_PROT_BIT0;
+	}
+
+	if (stat & ERRSTAT_FRMERR) {
+		rx_errors = 1;
+		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+		cf->data[2] |= CAN_ERR_PROT_FORM;
+	}
+
+	if (stat & ERRSTAT_STFERR) {
+		rx_errors = 1;
+		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+		cf->data[2] |= CAN_ERR_PROT_STUFF;
+	}
+
+
+	if (stat & ERRSTAT_ACKERR) {
+		rx_errors = 1;
+		cf->can_id |= CAN_ERR_ACK;

Is ACK error a RX error?

quoted

+	}
+
+	if (error_warning)
+		priv->can.can_stats.error_warning++;

What about priv->can.can_stats.error_passive;

quoted

+	if (rx_errors)
+		stats->rx_errors++;
+	if (cf->can_id & CAN_ERR_BUSERROR)
+		priv->can.can_stats.bus_error++;

It gets incremented in can_bus_off() already!

ok, I will rework the error handling.

quoted

quoted

+	priv->can.state = state;
+
+	netif_rx(skb);
+
+	ndev->last_rx = jiffies;
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+}
+
+static irqreturn_t flexcan_isr(int irq, void *dev_id)
+{
+	struct net_device *ndev = dev_id;
+	struct net_device_stats *stats = &ndev->stats;
+	struct flexcan_priv *priv = netdev_priv(ndev);
+	struct flexcan_regs __iomem *regs = priv->base;
+	u32 iflags, errstat;
+
+	errstat = readl(&regs->errstat);
+	if (errstat & ERRSTAT_INT) {
+		flexcan_error(ndev, errstat);
+		writel(errstat & ERRSTAT_INT, &regs->errstat);
+	}
+
+	iflags = readl(&regs->iflag1);
+
+	if (iflags & IFLAG_RX_FIFO_OVERFLOW) {
+		writel(IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
+		stats->rx_over_errors++;

		stats->rx_errors++; ???

quoted

+	}
+
+	while (iflags & IFLAG_RX_FIFO_AVAILABLE) {
+		struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
+
+		flexcan_rx_frame(ndev, mb);
+		writel(IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
+		readl(&regs->timer);
+		iflags = readl(&regs->iflag1);
+	}
+
+	if (iflags & (1 << TX_BUF_ID)) {
+		stats->tx_packets++;
+		writel((1 << TX_BUF_ID), &regs->iflag1);
+		netif_wake_queue(ndev);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int flexcan_set_bittiming(struct net_device *ndev)
+{
+	struct flexcan_priv *priv = netdev_priv(ndev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	struct flexcan_regs __iomem *regs = priv->base;
+	int ret = 0;
+	u32 reg;
+
+	dev_dbg(&ndev->dev, "%s: infreq: %ld brp: %d p1: %d p2: %d sample: %d"
+			" sjw: %d prop: %d\n",
+			__func__, clk_get_rate(priv->clk), bt->brp,
+			bt->phase_seg1, bt->phase_seg2, bt->sample_point,
+			bt->sjw, bt->prop_seg);

Could you replace this dev_dbg() in favor of a

        dev_info(dev->dev.parent, "setting CANCTRL=0x%x\n", reg);

before returning.

The dev_dbg is redundant as the output of 'ip' already shows this
information. But shouldn't this be a dev_dbg, too?

For SJA1000 and MSCAN we currently use dev_info() here. I found it
useful to show the bittiming registers as there is no other method to
retrieve them, but that's debatable.

[...]

quoted

quoted

+MODULE_AUTHOR("Sascha Hauer [off-list ref]");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CAN port driver for flexcan based chip");

Apart from that, it already looks quite good.

Thanks for your contribution.

Thanks for review, I will send an updated version soon.

Wolfgang.