On 01/25/2014 02:34 AM, Sergei Shtylyov wrote:

Hello.

On 01/20/2014 12:18 PM, Marc Kleine-Budde wrote:

quoted

quoted

Add support for the CAN controller found in Renesas R-Car SoCs.

quoted

quoted

Signed-off-by: Sergei Shtylyov <redacted>

quoted

quoted

---
The patch is against the 'linux-can-next.git' repo.

[...]

quoted

quoted

Index: linux-can-next/drivers/net/can/rcar_can.c
===================================================================

--- /dev/null
+++ linux-can-next/drivers/net/can/rcar_can.c

@@ -0,0 +1,857 @@

[...]

quoted

quoted

+/* Mailbox registers structure */
+struct rcar_can_mbox_regs {
+    u32 id;        /* IDE and RTR bits, SID and EID */
+    u8 stub;    /* Not used */
+    u8 dlc;        /* Data Length Code - bits [0..3] */
+    u8 data[8];    /* Data Bytes */
+    u8 tsh;        /* Time Stamp Higher Byte */
+    u8 tsl;        /* Time Stamp Lower Byte */
+} __packed;

quoted

If you have contact to the hardware designer please blame him for

   Unfortunately, we don't.

quoted

placing the data register unaligned into the register space. :)

   It's not even the only one or worst example of questionable register
design in this module IMO.

[...]

quoted

quoted

+static void rcar_can_tx_done(struct net_device *ndev)
+{
+    struct rcar_can_priv *priv = netdev_priv(ndev);
+    struct net_device_stats *stats = &ndev->stats;
+    int i;
+
+    spin_lock(&priv->skb_lock);
+    for (i = 0; i < priv->frames_queued; i++)
+        can_get_echo_skb(ndev, i);
+    stats->tx_bytes += priv->bytes_queued;
+    stats->tx_packets += priv->frames_queued;
+    priv->bytes_queued = 0;
+    priv->frames_queued = 0;
+    spin_unlock(&priv->skb_lock);

quoted

This looks broken. What happens if you send 2 CAN frames in a row, the
first one is send, a TX complete interrupt is issued and you handle it
here? You assume, that all CAN frames have been sent.

   TX interrupt will be issued only when TX FIFO gets empty (all 2 frames
have been transmitted in this case). Please see the comment to the
RCAR_CAN_MIER1_TXFIT bit.

Does the hardware have a TX complete interrupt? If you only have TX FIFO
empty, you have to limit the FIFO depth to 1.

quoted

quoted

+static irqreturn_t rcar_can_interrupt(int irq, void *dev_id)
+{
+    struct net_device *ndev = (struct net_device *)dev_id;

quoted

the cast is not needed

   Removed.

[...]

quoted

quoted

+static void rcar_can_set_bittiming(struct net_device *dev)
+{
+    struct rcar_can_priv *priv = netdev_priv(dev);
+    struct can_bittiming *bt = &priv->can.bittiming;
+    u32 bcr;
+    u8 clkr;
+
+    /* Don't overwrite CLKR with 32-bit BCR access */
+    /* CLKR has 8-bit access */

quoted

Can you explain the register layout here? Why do you access BCR with 32
bits when the register is defined as 3x8 bit? Can't you make it a
standard 32 bit register?

1. According to documentation BCR is the 24-bit register.
Actually we can consider some 32-bit register that combines BCR and
CLKR but according to documentation there are two separate registers.
2. BCR has 8- ,16-, and 32-bit access (according to documentation).
3. This is the algorithm that the documentation suggests.
4. We had a driver version with byte access but 32-bit access seems
shorter.

Please use a normal read-modify-write 32 bit access.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |