Gole, Anant wrote:

quoted

-----Original Message-----
From: Wolfgang Grandegger [mailto:wg@grandegger.com]
Sent: Saturday, August 29, 2009 12:36 PM
To: Gole, Anant
Cc: Linux Netdev List; Socketcan-core@lists.berlios.de
Subject: Re: [PATCH] net-next:can: add TI CAN (HECC) driver

quoted

TI HECC (High End CAN Controller) module is found on many TI devices. It

has

quoted

32 harwdare mailboxes with full implementation of CAN protocol version

2.0B

quoted

and bus speeds up to 1Mbps. The module specifications are available at TI

web

quoted

<http://www.ti.com>.

This driver is tested on OMAP3517 EVM. Suspend/Resume not tested as yet.

Nice driver, even using the NAPI interface. First some general comments.
Please remove debugging code mainly useful for development, e.g. the
CONFIG_DEBUG_FS block, many dev_info calls and special statistics
counters. Also use dev_dbg for the remaining debug messages useful for
the normal user, like state changes. More comments inline.

Ack on most comments you gave - some good bugs uncovered with this review. Thanks. I am answering those which I have some comment on.

[snip]

quoted

quoted

+#define HECC_MODULE_VERSION     "0.2"

A version number is will usually not maintained. May drivers have it but
it's never changed.

I understand but unless this is a strong objection I would like to keep it as it would be nice to update it when any major change happens.

It's *not* a strong objection but I personally believe that git is doing
a much better job.

[snip]

quoted

quoted

+
+/* CAN Bittiming constants as per HECC specs */
+static struct can_bittiming_const ti_hecc_bittiming_const = {
+       .name = DRV_NAME,
+       .tseg1_min = 1,
+       .tseg1_max = 16,
+       .tseg2_min = 1,

Please check if tseg2_min is a valid value. Usually it's larger.

Yes I did. This controller allows tseg2_min = 1

OK.

[snip]

quoted

quoted

+       struct napi_struct napi;
+       struct net_device *ndev;
+       struct clk *clk;
+       void __iomem *base;
+       unsigned int scc_ram_offset;
+       unsigned int hecc_ram_offset;
+       unsigned int mbox_offset;
+       unsigned int int_line;
+       DECLARE_BITMAP(tx_free_mbx, TI_HECC_MAX_TX_MBOX);
+       spinlock_t tx_lock;

Please document the spinlock tx_lock. What is it used for.

It is to protect the tx_free_mbx bitmap above - will document.

I know, but for kernel inclusion it should be documented.

[snip]

quoted

Right, automatic bus-off recovery should be disabled. To make that clear:

          hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_ABO);

Since CANMC register is set to 0 and ABO bit is part of it I did not make this statement explicitly (it will be redundant). I will update the comment above to state that if it changes in future then enable auto bus off with that statement

OK.

[snip]

quoted

quoted

+
+static int ti_hecc_do_set_mode(struct net_device *ndev, enum can_mode

mode)

quoted

+{
+       struct ti_hecc_priv *priv = netdev_priv(ndev);
+       int ret = 0;
+
+       switch (mode) {
+       case CAN_MODE_SLEEP:
+               dev_info(priv->ndev->dev.parent, "device going to

sleep\n");

quoted

+               if (netif_running(ndev)) {
+                       netif_stop_queue(ndev);
+                       netif_device_detach(ndev);
+                       /* Put HECC in low power mode */
+                       hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_PDR);
+               }
+               priv->can.state = CAN_STATE_SLEEPING;
+               break;

Has sleeping been tested? Actually, we do not have an interface yet to
enter sleep mode. Please remove. If there is a need for it, we should
work togehter to refine the interface and to provide a solution.

No I have not yet tested sleep/suspend/resume. Will remove based upon response to CAN_MODE_STOP comment below

quoted

quoted

+       case CAN_MODE_STOP:
+               dev_info(priv->ndev->dev.parent, "device stopping\n");
+               ti_hecc_stop(ndev);
+               break;

Only CAN_MODE_START is used by the CAN devicde interface for bus-off
recovery.

I saw other drivers (like mscan) also have the code for sleep/stop. Agreed that I have not tested it as you mentioned that the CAN infrastructure does not call it right now. Do you want me to remove it?

Yes, for kernel inclusion it should be removed. We are more relaxed to
keep that "preliminary" and "un-tested" code in the BerliOS SVN
repository. Note that the MSCAN driver is not yet mainline

[snip]

quoted

quoted

+
+       }
+       set_bit(mbxno, priv->tx_free_mbx);
+       spin_unlock_irqrestore(&priv->tx_lock, flags);

Hm, I wonder how the driver ensures that packages go out in order. How
does the CAN hardware schedule pending TX message objects? Vladislav
posted a test programs recently to check message ordering. See:

https://lists.berlios.de/pipermail/socketcan-core/2009-August/002871.html

TI HECC hardware has mailbox priority field and higher mailbox number will transmit if two mailboxes have same priority level. But the code does not use that feature as yet. Let me think of how I can ensure out of order issue.

Have a look to the AT91 driver, it seems to be quite similar.

[snip]

quoted

quoted

+
+       pending_pkts = hecc_read(priv, HECC_CANRMP);
+       while (pending_pkts && (num_pkts < quota)) {
+               mbxno = find_first_bit(&pending_pkts,

HECC_MAX_MAILBOXES);

Here I also wonder if the messages are handled in the correct order.

No I have not taken care of that. Let me look into the pkt handling to see how I can do it based upon hardware capability.

[snip]

quoted

quoted

+
+       ti_hecc_error(ndev, int_status, hecc_read(priv, HECC_CANES));

Hm, you create an error frame for each interrupt!? What do you see with:

# candump any,0:0,#FFFFFFFF

Good catch. I did not observe unnecessary error frames via candump (or I really missed them). But I will correct this anyway.

I see. That's because the error code of can_id is 0 and the mask does
not trigger.

quoted

quoted

+
+       /* Handle Abort acknowledge interrupt */
+       if (int_status & HECC_CANGIF_AAIF) {
+               ack = hecc_read(priv, HECC_CANAA);
+               while (ack) {
+                       mbxno = find_first_bit(&ack, HECC_MAX_MAILBOXES);
+                       if (mbxno == HECC_MAX_MAILBOXES) {
+                               break;
+                       } else {
+                               clear_bit(mbxno, &ack);
+                               /* release echo pkt & update counters */
+                               hecc_set_bit(priv, HECC_CANAA, (1 <<

mbxno));

quoted

+                               hecc_clear_bit(priv, HECC_CANME, (1 <<

mbxno));

quoted

+                               /* FIXME: since net-next tree's dev.h

does not

quoted

+                                * include can_free_echo_skb() doing

equivalent

quoted

+                                * of can_free_echo_skb(ndev, mbxno);
+                                */
+                               if (priv->can.echo_skb[mbxno]) {
+                                       kfree_skb(priv-
can.echo_skb[mbxno]);
+                                       priv->can.echo_skb[mbxno] = NULL;
+                               }
+                               if (netif_queue_stopped(ndev))
+                                       netif_wake_queue(ndev);
+                               spin_lock_irqsave(&priv->tx_lock, flags);
+                               clear_bit(mbxno, priv->tx_free_mbx);
+                               spin_unlock_irqrestore(&priv->tx_lock,

flags);

quoted

+                       }
+               }
+       }

Can that interrupt happen? I have not found any code aborting messages.

It can happen only if a tx mailbox is told to abort. This interrupt is an ack for the same. I am not using the tx abort feature so will remove this.

[snip]

quoted

quoted

+       }
+
+       /* Open common can device */
+       err = open_candev(ndev);
+       if (err) {
+               dev_err(ndev->dev.parent, "open_candev() failed %08X\n",

err);

free_irq?

Good catch. Will fix it.

quoted

quoted

+               return err;
+       }
+

Thanks for your contribution.

Wolfgang.

Appreciate the review. I will send v2 of patch. Not sure still if the original patch reached the socketcan mailing list due to size.

v2 will fit but I'm wondering who is moderating the ML. Oliver?

Wolfgang.