From: Wolfgang Grandegger <wg@grandegger•com>
To: Sascha Hauer <s.hauer@pengutronix•de>
Cc: Socketcan-core@lists•berlios.de,
Linux Netdev List <netdev@vger•kernel.org>
Subject: Re: [PATCH] Add Support for Freescale FlexCAN CAN controller
Date: Mon, 27 Jul 2009 10:30:31 +0200 [thread overview]
Message-ID: <4A6D65A7.4080805@grandegger.com> (raw)
In-Reply-To: <20090727062559.GP2714@pengutronix.de>
Sascha Hauer wrote:
> On Fri, Jul 24, 2009 at 04:55:02PM +0200, Wolfgang Grandegger wrote:
>> Hi Sascha,
>>
>> Sascha Hauer wrote:
>>> 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...
>>
>>> Sascha
[snip]
>>> +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, ®s->cantxfg[TX_BUF_ID].can_dlc);
>>> + writel(can_id, ®s->cantxfg[TX_BUF_ID].can_id);
>>> +
>>> + can_data = (u32 *)frame->data;
>>> + writel(cpu_to_be32(*can_data), ®s->cantxfg[TX_BUF_ID].data[0]);
>>> + writel(cpu_to_be32(*(can_data + 1)), ®s->cantxfg[TX_BUF_ID].data[1]);
>>> +
>>> + writel(dlc, ®s->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.
>>> +
>>> + 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
>
>>> +
>>> + 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.
>>> + 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.
>>
>>> + if (stat & ERRSTAT_BOFFINT) {
>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>> + can_bus_off(ndev);
>>> + }
>>> +
>>> + if (stat & ERRSTAT_BIT1ERR) {
>> rx_error = 1; ???
>>
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> + cf->data[2] |= CAN_ERR_PROT_BIT1;
>>> + }
>>> +
>>> + if (stat & ERRSTAT_BIT0ERR) {
>> rx_error = 1; ???
>>
>>> + 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?
>>
>>> + }
>>> +
>>> + if (error_warning)
>>> + priv->can.can_stats.error_warning++;
>> What about priv->can.can_stats.error_passive;
>>
>>> + 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.
>
>>> + 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(®s->errstat);
>>> + if (errstat & ERRSTAT_INT) {
>>> + flexcan_error(ndev, errstat);
>>> + writel(errstat & ERRSTAT_INT, ®s->errstat);
>>> + }
>>> +
>>> + iflags = readl(®s->iflag1);
>>> +
>>> + if (iflags & IFLAG_RX_FIFO_OVERFLOW) {
>>> + writel(IFLAG_RX_FIFO_OVERFLOW, ®s->iflag1);
>>> + stats->rx_over_errors++;
>> stats->rx_errors++; ???
>>
>>> + }
>>> +
>>> + while (iflags & IFLAG_RX_FIFO_AVAILABLE) {
>>> + struct flexcan_mb __iomem *mb = ®s->cantxfg[0];
>>> +
>>> + flexcan_rx_frame(ndev, mb);
>>> + writel(IFLAG_RX_FIFO_AVAILABLE, ®s->iflag1);
>>> + readl(®s->timer);
>>> + iflags = readl(®s->iflag1);
>>> + }
>>> +
>>> + if (iflags & (1 << TX_BUF_ID)) {
>>> + stats->tx_packets++;
>>> + writel((1 << TX_BUF_ID), ®s->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.
[...]
>>> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix•de>");
>>> +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.
next prev parent reply other threads:[~2009-07-27 8:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-07-24 13:19 [PATCH] Add Support for Freescale FlexCAN CAN controller Sascha Hauer
2009-07-24 14:55 ` Wolfgang Grandegger
2009-07-27 6:25 ` Sascha Hauer
2009-07-27 8:30 ` Wolfgang Grandegger [this message]
2009-07-27 9:25 ` Wolfgang Grandegger
2009-07-27 9:43 ` Oliver Hartkopp
2009-07-27 10:07 ` Wolfgang Grandegger
2009-07-27 10:52 ` Oliver Hartkopp
2009-07-27 12:12 ` Wolfgang Grandegger
2009-07-28 7:40 ` Sascha Hauer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A6D65A7.4080805@grandegger.com \
--to=wg@grandegger$(echo .)com \
--cc=Socketcan-core@lists$(echo .)berlios.de \
--cc=netdev@vger$(echo .)kernel.org \
--cc=s.hauer@pengutronix$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox