From patchwork Fri Jun 18 11:53:03 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Wolfgang Grandegger X-Patchwork-Id: 56176 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 4764F1007D4 for ; Fri, 18 Jun 2010 21:53:20 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761032Ab0FRLxK (ORCPT ); Fri, 18 Jun 2010 07:53:10 -0400 Received: from mail-out.m-online.net ([212.18.0.10]:33993 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758537Ab0FRLxI (ORCPT ); Fri, 18 Jun 2010 07:53:08 -0400 Received: from frontend1.mail.m-online.net (unknown [192.168.8.180]) by mail-out.m-online.net (Postfix) with ESMTP id 853181C0080C; Fri, 18 Jun 2010 13:53:04 +0200 (CEST) X-Auth-Info: QAsJBp5E+TREOukX2LWfCz+ykhd4p/B/oCQG8eM39kI= Received: from lancy.mylan.de (p4FF0410F.dip.t-dialin.net [79.240.65.15]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by smtp-auth.mnet-online.de (Postfix) with ESMTPSA id 5A4411C0024A; Fri, 18 Jun 2010 13:53:04 +0200 (CEST) Message-ID: <4C1B5E1F.5080702@grandegger.com> Date: Fri, 18 Jun 2010 13:53:03 +0200 From: Wolfgang Grandegger User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100430 Fedora/3.0.4-2.fc12 Thunderbird/3.0.4 MIME-Version: 1.0 To: Marc Kleine-Budde CC: socketcan-core@lists.berlios.de, netdev@vger.kernel.org Subject: Re: [PATCH] socketcan: add a driver for FlexCAN controllers. References: <20100617105201.GA2015@bluebox.local> <4C1B4098.3090800@grandegger.com> <4C1B4796.3060506@pengutronix.de> <4C1B4B85.3010905@grandegger.com> <4C1B4DF0.2090103@pengutronix.de> <4C1B52A7.3060607@grandegger.com> <4C1B56BC.1050303@pengutronix.de> In-Reply-To: <4C1B56BC.1050303@pengutronix.de> X-Enigmail-Version: 1.0.1 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 06/18/2010 01:21 PM, Marc Kleine-Budde wrote: > Wolfgang Grandegger wrote: >> On 06/18/2010 12:44 PM, Marc Kleine-Budde wrote: >>> Wolfgang Grandegger wrote: >>>> On 06/18/2010 12:16 PM, Marc Kleine-Budde wrote: >>>>> Wolfgang Grandegger wrote: >>>>>> Hi Hans-Jürgen, >>>>>> >>>>>> On 06/17/2010 12:52 PM, Hans J. Koch wrote: >>>>>>> This adds a driver for FlexCAN based CAN controllers, >>>>>>> e.g. found in Freescale i.MX35 SoCs. >>>>>>> >>>>>>> The original version of this driver was posted by Sascha Hauer in July 2009: >>>>>>> http://kerneltrap.org/mailarchive/linux-netdev/2009/7/29/6251621 >>>>>>> >>>>>>> I took this version, added NAPI support, and fixed some problems found >>>>>>> during testing. Well, here is the result. Please review. >>>>>> I briefly browsed the patch and various bits and pieces are missing or >>>>>> not correctly implemented. Marc already pointed out a few of them: >>>>>> >>>>>> - I do not find can_put/get_echo_skb functions in the code. How is >>>>>> IFF_ECHO supposed to work? >>>>> the driver uses hardware loopback >>>> OK, then >>>> >>>> dev = alloc_candev(sizeof(struct flexcan_priv), 0); >>>> >>>> should be used (and TX_ECHO_SKB_MAX removed) in Hans-Jürgens driver. >>>> >>>>>> - Support for CAN_CTRLMODE_BERR_REPORTING and do_get_berr_counter() >>>>>> seems to be missing. >>>>>> >>>>>> - Make use of alloc_can_skb() and alloc_can_err_skb(). >>>>> the last two points are already addressed in my version of the driver. >>>> I do not see support for CAN_CTRLMODE_BERR_REPORTING in your driver >>>> (which has nothing to do with do_get_berr_counter). >>> oh yes...sorry, got confused. >>> >>> However I implemented CAN_CTRLMODE_BERR_REPORTING, i.e. turning of the >>> bit error interrupts by default. This has the effect that no bus warning >>> and bus passive interrupt was signalled. >>> >>> I should add a comment to my driver. >> >> I'm confused, CAN_CTRLMODE_BERR_REPORTING means that the user can enable >> bus error reporting, which seems not be handled in the driver your sent. >> See: >> >> http://lxr.linux.no/linux/drivers/net/can/sja1000/sja1000.c#L134 >> http://lxr.linux.no/linux/drivers/net/can/sja1000/sja1000.c#L588 > > Which interrupts does "IRQ_BEI" include? What should > CAN_CTRLMODE_BERR_REPORTING do? > > Looking at: > http://lxr.linux.no/linux+v2.6.34/drivers/net/can/sja1000/sja1000.c#L393 > it seems that BEI on the SJA just effects bit, form and stuff errors. Yes, it effects *bus errors*... actually the one you handle in at91_poll_err_frame(). Especially the AT91_IRQ_AERR does cause the infamous interrupt flooding (which is, btw, not treated correctly as bus error in your driver). > If I disable the corresponding interrupt in the flexcan. This is > ERR_MSK, (1 << 14 in CTRL), I don't get error warning or error passive > interrupts either. I'm not sure about the bus off interrupt. Hm, my understanding is that you can disable those bus error interrupts individually via AT91_IRQ_ERR_FRAME. > From my point of view this is not that what CAN_CTRLMODE_BERR_REPORTING > should do, is it? It should *not* disable state change interrupts, of course. I have started to fix some issues in the at91 driver some time ago, which I have attached below. Wolfgang. --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c index a2f29a3..ad36b59 100644 --- a/drivers/net/can/at91_can.c +++ b/drivers/net/can/at91_can.c @@ -164,6 +164,7 @@ struct at91_priv { void __iomem *reg_base; u32 reg_sr; + u32 reg_ie_napi; unsigned int tx_next; unsigned int tx_echo; unsigned int rx_next; @@ -273,7 +274,7 @@ static int at91_set_bittiming(struct net_device *dev) static void at91_chip_start(struct net_device *dev) { struct at91_priv *priv = netdev_priv(dev); - u32 reg_mr, reg_ier; + u32 reg_mr; /* disable interrupts */ at91_write(priv, AT91_IDR, AT91_IRQ_ALL); @@ -290,10 +291,12 @@ static void at91_chip_start(struct net_device *dev) priv->can.state = CAN_STATE_ERROR_ACTIVE; - /* Enable interrupts */ - reg_ier = AT91_IRQ_MB_RX | AT91_IRQ_ERRP | AT91_IRQ_ERR_FRAME; + /* enable interrupts */ + priv->reg_ir_napi = AT91_IRQ_MB_RX; + if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) + priv->reg_ir_napi |= AT91_IRQ_ERR_FRAME; at91_write(priv, AT91_IDR, AT91_IRQ_ALL); - at91_write(priv, AT91_IER, reg_ier); + at91_write(priv, AT91_IER, priv->reg_ir_napi | AT91_IRQ_ERRP); } static void at91_chip_stop(struct net_device *dev, enum can_state state) @@ -604,20 +607,21 @@ static void at91_poll_err_frame(struct net_device *dev, { struct at91_priv *priv = netdev_priv(dev); + priv->can.can_stats.bus_error++; + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + /* CRC error */ if (reg_sr & AT91_IRQ_CERR) { dev_dbg(dev->dev.parent, "CERR irq\n"); dev->stats.rx_errors++; - priv->can.can_stats.bus_error++; - cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ | + CAN_ERR_PROT_LOC_CRC_DEL; } /* Stuffing Error */ if (reg_sr & AT91_IRQ_SERR) { dev_dbg(dev->dev.parent, "SERR irq\n"); dev->stats.rx_errors++; - priv->can.can_stats.bus_error++; - cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; cf->data[2] |= CAN_ERR_PROT_STUFF; } @@ -626,14 +630,13 @@ static void at91_poll_err_frame(struct net_device *dev, dev_dbg(dev->dev.parent, "AERR irq\n"); dev->stats.tx_errors++; cf->can_id |= CAN_ERR_ACK; + cf->data[3] |= CAN_ERR_PROT_LOC_ACK; } /* Form error */ if (reg_sr & AT91_IRQ_FERR) { dev_dbg(dev->dev.parent, "FERR irq\n"); dev->stats.rx_errors++; - priv->can.can_stats.bus_error++; - cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; cf->data[2] |= CAN_ERR_PROT_FORM; } @@ -641,9 +644,7 @@ static void at91_poll_err_frame(struct net_device *dev, if (reg_sr & AT91_IRQ_BERR) { dev_dbg(dev->dev.parent, "BERR irq\n"); dev->stats.tx_errors++; - priv->can.can_stats.bus_error++; - cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; - cf->data[2] |= CAN_ERR_PROT_BIT; + cf->data[2] |= CAN_ERR_PROT_BIT0 | CAN_ERR_PROT_BIT1; } } @@ -662,7 +663,6 @@ static int at91_poll_err(struct net_device *dev, int quota, u32 reg_sr) at91_poll_err_frame(dev, cf, reg_sr); netif_receive_skb(skb); - dev->last_rx = jiffies; dev->stats.rx_packets++; dev->stats.rx_bytes += cf->can_dlc; @@ -688,12 +688,10 @@ static int at91_poll(struct napi_struct *napi, int quota) work_done += at91_poll_err(dev, quota - work_done, reg_sr); if (work_done < quota) { - /* enable IRQs for frame errors and all mailboxes >= rx_next */ - u32 reg_ier = AT91_IRQ_ERR_FRAME; - reg_ier |= AT91_IRQ_MB_RX & ~AT91_MB_RX_MASK(priv->rx_next); - napi_complete(napi); - at91_write(priv, AT91_IER, reg_ier); + /* enable IRQs for frame errors and all mailboxes >= rx_next */ + at91_write(priv, AT91_IER, priv->reg_ir_napi & + ~AT91_MB_RX_MASK(priv->rx_next)); } return work_done; @@ -899,7 +897,6 @@ static void at91_irq_err(struct net_device *dev) at91_irq_err_state(dev, cf, new_state); netif_rx(skb); - dev->last_rx = jiffies; dev->stats.rx_packets++; dev->stats.rx_bytes += cf->can_dlc; @@ -933,8 +930,7 @@ static irqreturn_t at91_irq(int irq, void *dev_id) * save for later use. */ priv->reg_sr = reg_sr; - at91_write(priv, AT91_IDR, - AT91_IRQ_MB_RX | AT91_IRQ_ERR_FRAME); + at91_write(priv, AT91_IDR, priv->reg_ir_napi); napi_schedule(&priv->napi); } @@ -1073,7 +1069,8 @@ static int __init at91_can_probe(struct platform_device *pdev) priv->can.bittiming_const = &at91_bittiming_const; priv->can.do_set_bittiming = at91_set_bittiming; priv->can.do_set_mode = at91_set_mode; - priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | + CAN_CTRLMODE_BERR_REPORTING; priv->reg_base = addr; priv->dev = dev; priv->clk = clk; diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c index 145b1a7..77b5fbc 100644 --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -89,8 +89,8 @@ static int sja1000_probe_chip(struct net_device *dev) struct sja1000_priv *priv = netdev_priv(dev); if (priv->reg_base && (priv->read_reg(priv, 0) == 0xFF)) { - printk(KERN_INFO "%s: probing @0x%lX failed\n", - DRV_NAME, dev->base_addr); + dev_info(dev->dev.parent, "probing @0x%p failed\n", + priv->reg_base); return 0; } return -1; @@ -254,7 +254,7 @@ static void chipset_init(struct net_device *dev) * [ can-id ] [flags] [len] [can data (up to 8 bytes] */ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb, - struct net_device *dev) + struct net_device *dev) { struct sja1000_priv *priv = netdev_priv(dev); struct can_frame *cf = (struct can_frame *)skb->data; @@ -602,9 +602,9 @@ void free_sja1000dev(struct net_device *dev) EXPORT_SYMBOL_GPL(free_sja1000dev); static const struct net_device_ops sja1000_netdev_ops = { - .ndo_open = sja1000_open, - .ndo_stop = sja1000_close, - .ndo_start_xmit = sja1000_start_xmit, + .ndo_open = sja1000_open, + .ndo_stop = sja1000_close, + .ndo_start_xmit = sja1000_start_xmit, }; int register_sja1000dev(struct net_device *dev)