[PATCH v5] can: add Renesas R-Car CAN driver

[PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

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

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

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

Changes in version 5:
- removed the code specific to the mailbox mode handling and added the code
  for the FIFO mode, dropping support for CAN_CTRLMODE_ONE_SHOT;
- stop accumulating errors in the byte 1 of an error frame;
- added #define RCAR_CAN_CTLR_CANM for CTLR.CANM field, added 'CANM_' infix to
  all its values;
- replaced ~RCAR_CAN_CTLR_CANM_FORCE_RESET with ~RCAR_CAN_CTLR_CANM; 
- added polling for the CAN reset status bit when going from/to CAN reset.

Changes in version 4:
- added 'RCAR_CAN_' prefix to all #define's;
- replaced all BIT(N) invocations with (1 << N) which allowed to remove casts
  to 'u8';
- called rcar_can_set_bittiming() from rcar_can_start()  again and  stopped
  registering it as do_set_bittiming() method which allowed to remove clock
  API calls and control register writes from this function and make it *void*;
- added #define RCAR_CAN_CTLR_IDFM for more clarity;
- clarified comment to #define RCAR_CAN_IER_TXMIE;
- did some whitespace cleanups.

Changes in version 3:
- replaced the register #define's with 'struct rcar_can_regs' fields, replaced
  rcar_can_{read|write}[bwl]() with mere {read|write}[bwl]();
- removed hardware bus-off recovery support which allowed to also remove
  rcar_can_start() prototype;
- added RX/TX error count to error data frame for error warning/passive;
- moved TX completion interrupt handling into separate function;
- added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
- removed unneeded type cast in the probe() method.

Changes in version 2:
- added function to clean up TX mailboxes after bus error and bus-off;
- added module parameter to enable hardware recovery from bus-off, added handler
  for the bus-off recovery interrupt, and set the control register according to
  the parameter value and the restart timer setting in rcar_can_start();
- changed the way CAN_ERR_CRTL_[RT]X_{PASSIVE|WARNING} flags are set to a more
  realistic one;
- replaced MBX_* macros and rcar_can_mbx_{read|write}[bl]() functions with
  'struct rcar_can_mbox_regs', 'struct rcar_can_regs', and {read|write}[bl](),
  replaced 'reg_base' field of 'struct rcar_can_priv' with 'struct rcar_can_regs
  __iomem *regs';
- added 'ier' field to 'struct rcar_can_priv' to cache the current value of the
  interrupt enable register;
- added a check for enabled interrupts on entry to rcar_can_interrupt();
- limited transmit mailbox search loop in rcar_can_interrupt();
- decoupled  TX byte count increment from can_get_echo_skb() call;
- removed netif_queue_stopped() call from rcar_can_interrupt();
- added clk_prepare_enable()/clk_disable_unprepare() to ndo_{open|close}(),
  do_set_bittiming(), and do_get_berr_counter() methods, removed clk_enable()
  call from the probe() method and clk_disable() call from the remove() method;
- allowed rcar_can_set_bittiming() to be called when the device is closed and
  remove  explicit call to it from rcar_can_start();
- switched to using mailbox number priority transmit mode, and switched to the
  sequential mailbox use in ndo_start_xmit() method;
- stopped reading the message control registers in ndo_start_xmit() method;
- avoided returning NETDEV_TX_BUSY from ndo_start_xmit() method;
- stopped reading data when RTR bit is set in the CAN frame;
- made 'num_pkts' variable *int* and moved its check to the *while* condition in
  rcar_can_rx_poll();
- used dev_get_platdata() in the probe() method;
- enabled bus error interrupt only if CAN_CTRLMODE_BERR_REPORTING flag is set;
- started reporting CAN_CTRLMODE_BERR_REPORTING support and stopped reporting
  CAN_CTRLMODE_3_SAMPLES support;
- set CAN_ERR_ACK flag on ACK error;
- switched to incrementing bus error counter only once per bus error interrupt;
- started switching to CAN sleep mode in rcar_can_stop() and stopped switching
  to it in the remove() method;
- removed netdev_err() calls on allocation failure in rcar_can_error() and
  rcar_can_rx_pkt();
- removed "CANi" from the register offset macro comments.

 drivers/net/can/Kconfig               |    9 
 drivers/net/can/Makefile              |    1 
 drivers/net/can/rcar_can.c            |  857 ++++++++++++++++++++++++++++++++++
 include/linux/can/platform/rcar_can.h |   15 
 4 files changed, 882 insertions(+)

Index: linux-can-next/drivers/net/can/Kconfig
=================================--- linux-can-next.orig/drivers/net/can/Kconfig
+++ linux-can-next/drivers/net/can/Kconfig
@@ -125,6 +125,15 @@ config CAN_GRCAN
 	  endian syntheses of the cores would need some modifications on
 	  the hardware level to work.
 
+config CAN_RCAR
+	tristate "Renesas R-Car CAN controller"
+	---help---
+	  Say Y here if you want to use CAN controller found on Renesas R-Car
+	  SoCs.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called rcar_can.
+
 source "drivers/net/can/mscan/Kconfig"
 
 source "drivers/net/can/sja1000/Kconfig"
Index: linux-can-next/drivers/net/can/Makefile
=================================--- linux-can-next.orig/drivers/net/can/Makefile
+++ linux-can-next/drivers/net/can/Makefile
@@ -25,5 +25,6 @@ obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ica
 obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
 obj-$(CONFIG_PCH_CAN)		+= pch_can.o
 obj-$(CONFIG_CAN_GRCAN)		+= grcan.o
+obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
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 @@
+/*
+ * Renesas R-Car CAN device driver
+ *
+ * Copyright (C) 2013 Cogent Embedded, Inc. <source@cogentembedded.com>
+ * Copyright (C) 2013 Renesas Solutions Corp.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/can/led.h>
+#include <linux/can/dev.h>
+#include <linux/clk.h>
+#include <linux/can/platform/rcar_can.h>
+
+#define RCAR_CAN_DRV_NAME	"rcar_can"
+
+/* Mailbox configuration:
+ * mailbox 60 - 63 - Rx FIFO mailboxes
+ * mailbox 56 - 59 - Tx FIFO mailboxes
+ * non-FIFO mailboxes are not used
+ */
+#define RCAR_CAN_N_MBX		64 /* Number of mailboxes in non-FIFO mode */
+#define RCAR_CAN_RX_FIFO_MBX	60 /* Mailbox - window to Rx FIFO */
+#define RCAR_CAN_TX_FIFO_MBX	56 /* Mailbox - window to Tx FIFO */
+#define RCAR_CAN_N_ECHO_SKB	10
+
+/* 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;
+
+struct rcar_can_regs {
+	struct rcar_can_mbox_regs mb[RCAR_CAN_N_MBX]; /* Mailbox registers */
+	u32 mkr_2_9[8];	/* Mask Registers 2-9 */
+	u32 fidcr[2];	/* FIFO Received ID Compare Register */
+	u32 mkivlr1;	/* Mask Invalid Register 1 */
+	u32 mier1;	/* Mailbox Interrupt Enable Register 1 */
+	u32 mkr_0_1[2];	/* Mask Registers 0-1 */
+	u32 mkivlr0;    /* Mask Invalid Register 0*/
+	u32 mier0;      /* Mailbox Interrupt Enable Register 0 */
+	u8 pad_440[0x3c0];
+	u8 mctl[64];	/* Message Control Registers */
+	u16 ctlr;	/* Control Register */
+	u16 str;	/* Status register */
+	u8 bcr[3];	/* Bit Configuration Register */
+	u8 clkr;	/* Clock Select Register */
+	u8 rfcr;	/* Receive FIFO Control Register */
+	u8 rfpcr;	/* Receive FIFO Pointer Control Register */
+	u8 tfcr;	/* Transmit FIFO Control Register */
+	u8 tfpcr;       /* Transmit FIFO Pointer Control Register */
+	u8 eier;	/* Error Interrupt Enable Register */
+	u8 eifr;	/* Error Interrupt Factor Judge Register */
+	u8 recr;	/* Receive Error Count Register */
+	u8 tecr;        /* Transmit Error Count Register */
+	u8 ecsr;	/* Error Code Store Register */
+	u8 cssr;	/* Channel Search Support Register */
+	u8 mssr;	/* Mailbox Search Status Register */
+	u8 msmr;	/* Mailbox Search Mode Register */
+	u16 tsr;	/* Time Stamp Register */
+	u8 afsr;	/* Acceptance Filter Support Register */
+	u8 pad_857;
+	u8 tcr;		/* Test Control Register */
+	u8 pad_859[7];
+	u8 ier;		/* Interrupt Enable Register */
+	u8 isr;		/* Interrupt Status Register */
+	u8 pad_862;
+	u8 mbsmr;	/* Mailbox Search Mask Register */
+} __packed;
+
+struct rcar_can_priv {
+	struct can_priv can;	/* Must be the first member! */
+	struct net_device *ndev;
+	struct napi_struct napi;
+	struct rcar_can_regs __iomem *regs;
+	struct clk *clk;
+	spinlock_t skb_lock;
+	u32 frames_queued;
+	u32 bytes_queued;
+	u8 clock_select;
+	u8 ier;
+};
+
+static const struct can_bittiming_const rcar_can_bittiming_const = {
+	.name = RCAR_CAN_DRV_NAME,
+	.tseg1_min = 4,
+	.tseg1_max = 16,
+	.tseg2_min = 2,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 1024,
+	.brp_inc = 1,
+};
+
+/* Control Register bits */
+#define RCAR_CAN_CTLR_BOM	(3 << 11) /* Bus-Off Recovery Mode Bits */
+#define RCAR_CAN_CTLR_BOM_ENT	(1 << 11) /* Entry to halt mode */
+					/* at bus-off entry */
+#define RCAR_CAN_CTLR_SLPM	(1 << 10)
+#define RCAR_CAN_CTLR_CANM	(3 << 8) /* Operating Mode Select Bit */
+#define RCAR_CAN_CTLR_CANM_HALT	(1 << 9)
+#define RCAR_CAN_CTLR_CANM_RESET (1 << 8)
+#define RCAR_CAN_CTLR_CANM_FORCE_RESET (3 << 8)
+#define RCAR_CAN_CTLR_MLM	(1 << 3) /* Message Lost Mode Select */
+#define RCAR_CAN_CTLR_IDFM	(3 << 1) /* ID Format Mode Select Bits */
+#define RCAR_CAN_CTLR_IDFM_MIXED (1 << 2) /* Mixed ID mode */
+#define RCAR_CAN_CTLR_MBM	(1 << 0) /* Mailbox Mode select */
+
+/* Status Register bits */
+#define RCAR_CAN_STR_RSTST	(1 << 8) /* Reset Status Bit */
+
+/* FIFO Received ID Compare Registers 0 and 1 bits */
+#define RCAR_CAN_FIDCR_IDE	(1 << 31) /* ID Extension Bit */
+#define RCAR_CAN_FIDCR_RTR	(1 << 30) /* Remote Transmission Request Bit */
+
+/* Receive FIFO Control Register bits */
+#define RCAR_CAN_RFCR_RFEST	(1 << 7) /* Receive FIFO Empty Status Flag */
+#define RCAR_CAN_RFCR_RFUST	(7 << 1) /* Receive FIFO Unread Message */
+					/* Number Status Bits */
+#define RCAR_CAN_RFCR_RFUST_SHIFT 1	/* Offset of Receive FIFO Unread */
+					/* Message Number Status Bits */
+#define RCAR_CAN_RFCR_RFE	(1 << 0) /* Receive FIFO Enable */
+
+/* Transmit FIFO Control Register bits */
+#define RCAR_CAN_TFCR_TFUST	(7 << 1) /* Transmit FIFO Unsent Message */
+					/* Number Status Bits */
+#define RCAR_CAN_TFCR_TFUST_SHIFT 1	/* Offset of Transmit FIFO Unsent */
+					/* Message Number Status Bits */
+#define RCAR_CAN_TFCR_TFE	(1 << 0) /* Transmit FIFO Enable */
+
+#define RCAR_CAN_N_RX_MKREGS1	2	/* Number of mask registers */
+					/* for Rx mailboxes 0-31 */
+#define RCAR_CAN_N_RX_MKREGS2	8
+
+/* Bit Configuration Register settings */
+#define RCAR_CAN_BCR_TSEG1(x)	(((x) & 0x0f) << 28)
+#define RCAR_CAN_BCR_BPR(x)	(((x) & 0x3ff) << 16)
+#define RCAR_CAN_BCR_SJW(x)	(((x) & 0x3) << 12)
+#define RCAR_CAN_BCR_TSEG2(x)	(((x) & 0x07) << 8)
+
+/* Mailbox and Mask Registers bits */
+#define RCAR_CAN_IDE		(1 << 31)
+#define RCAR_CAN_RTR		(1 << 30)
+#define RCAR_CAN_SID_SHIFT	18
+
+/* Mailbox Interrupt Enable Register 1 bits */
+#define RCAR_CAN_MIER1_RXFIE	(1 << 28) /* Receive  FIFO Interrupt Enable */
+#define RCAR_CAN_MIER1_TXFIT	(1 << 25) /* Transmit FIFO Interrupt Timing */
+					/* Generate interrupt when */
+					/* Tx FIFO becomes empty due to */
+					/* completion of transmission */
+#define RCAR_CAN_MIER1_TXFIE	(1 << 24) /* Transmit FIFO Interrupt Enable */
+
+/* Interrupt Enable Register bits */
+#define RCAR_CAN_IER_ERSIE	(1 << 5) /* Error (ERS) Interrupt Enable Bit */
+#define RCAR_CAN_IER_RXFIE	(1 << 4) /* Reception FIFO Interrupt */
+					/* Enable Bit */
+#define RCAR_CAN_IER_TXFIE	(1 << 3) /* Transmission FIFO Interrupt */
+					/* Enable Bit */
+/* Interrupt Status Register bits */
+#define RCAR_CAN_ISR_ERSF	(1 << 5) /* Error (ERS) Interrupt Status Bit */
+#define RCAR_CAN_ISR_RXFF	(1 << 4) /* Reception FIFO Interrupt */
+					/* Status Bit */
+#define RCAR_CAN_ISR_TXFF	(1 << 3) /* Transmission FIFO Interrupt */
+					/* Status Bit */
+
+/* Error Interrupt Enable Register bits */
+#define RCAR_CAN_EIER_BLIE	(1 << 7) /* Bus Lock Interrupt Enable */
+#define RCAR_CAN_EIER_OLIE	(1 << 6) /* Overload Frame Transmit */
+					/* Interrupt Enable */
+#define RCAR_CAN_EIER_ORIE	(1 << 5) /* Receive Overrun  Interrupt Enable */
+#define RCAR_CAN_EIER_BORIE	(1 << 4) /* Bus-Off Recovery Interrupt Enable */
+#define RCAR_CAN_EIER_BOEIE	(1 << 3) /* Bus-Off Entry Interrupt Enable */
+#define RCAR_CAN_EIER_EPIE	(1 << 2) /* Error Passive Interrupt Enable */
+#define RCAR_CAN_EIER_EWIE	(1 << 1) /* Error Warning Interrupt Enable */
+#define RCAR_CAN_EIER_BEIE	(1 << 0) /* Bus Error Interrupt Enable */
+
+/* Error Interrupt Factor Judge Register bits */
+#define RCAR_CAN_EIFR_BLIF	(1 << 7) /* Bus Lock Detect Flag */
+#define RCAR_CAN_EIFR_OLIF	(1 << 6) /* Overload Frame Transmission */
+					 /* Detect Flag */
+#define RCAR_CAN_EIFR_ORIF	(1 << 5) /* Receive Overrun Detect Flag */
+#define RCAR_CAN_EIFR_BORIF	(1 << 4) /* Bus-Off Recovery Detect Flag */
+#define RCAR_CAN_EIFR_BOEIF	(1 << 3) /* Bus-Off Entry Detect Flag */
+#define RCAR_CAN_EIFR_EPIF	(1 << 2) /* Error Passive Detect Flag */
+#define RCAR_CAN_EIFR_EWIF	(1 << 1) /* Error Warning Detect Flag */
+#define RCAR_CAN_EIFR_BEIF	(1 << 0) /* Bus Error Detect Flag */
+
+/* Error Code Store Register bits */
+#define RCAR_CAN_ECSR_EDPM	(1 << 7) /* Error Display Mode Select Bit */
+#define RCAR_CAN_ECSR_ADEF	(1 << 6) /* ACK Delimiter Error Flag */
+#define RCAR_CAN_ECSR_BE0F	(1 << 5) /* Bit Error (dominant) Flag */
+#define RCAR_CAN_ECSR_BE1F	(1 << 4) /* Bit Error (recessive) Flag */
+#define RCAR_CAN_ECSR_CEF	(1 << 3) /* CRC Error Flag */
+#define RCAR_CAN_ECSR_AEF	(1 << 2) /* ACK Error Flag */
+#define RCAR_CAN_ECSR_FEF	(1 << 1) /* Form Error Flag */
+#define RCAR_CAN_ECSR_SEF	(1 << 0) /* Stuff Error Flag */
+
+#define RCAR_CAN_NAPI_WEIGHT 4
+
+static void tx_failure_cleanup(struct net_device *ndev)
+{
+	u32 i;
+
+	for (i = 0; i < RCAR_CAN_N_ECHO_SKB; i++)
+		can_free_echo_skb(ndev, i);
+}
+
+static void rcar_can_error(struct net_device *ndev)
+{
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u8 eifr, txerr = 0, rxerr = 0;
+
+	/* Propagate the error condition to the CAN stack */
+	skb = alloc_can_err_skb(ndev, &cf);
+	if (!skb)
+		return;
+
+	eifr = readb(&priv->regs->eifr);
+	if (eifr & (RCAR_CAN_EIFR_EWIF | RCAR_CAN_EIFR_EPIF)) {
+		cf->can_id |= CAN_ERR_CRTL;
+		txerr = readb(&priv->regs->tecr);
+		rxerr = readb(&priv->regs->recr);
+		cf->data[6] = txerr;
+		cf->data[7] = rxerr;
+	}
+	if (eifr & RCAR_CAN_EIFR_BEIF) {
+		int rx_errors = 0, tx_errors = 0;
+		u8 ecsr;
+
+		netdev_dbg(priv->ndev, "Bus error interrupt:\n");
+		cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
+		cf->data[2] = CAN_ERR_PROT_UNSPEC;
+
+		ecsr = readb(&priv->regs->ecsr);
+		if (ecsr & RCAR_CAN_ECSR_ADEF) {
+			netdev_dbg(priv->ndev, "ACK Delimiter Error\n");
+			cf->data[3] |= CAN_ERR_PROT_LOC_ACK_DEL;
+			tx_errors++;
+			writeb(~RCAR_CAN_ECSR_ADEF, &priv->regs->ecsr);
+		}
+		if (ecsr & RCAR_CAN_ECSR_BE0F) {
+			netdev_dbg(priv->ndev, "Bit Error (dominant)\n");
+			cf->data[2] |= CAN_ERR_PROT_BIT0;
+			tx_errors++;
+			writeb(~RCAR_CAN_ECSR_BE0F, &priv->regs->ecsr);
+		}
+		if (ecsr & RCAR_CAN_ECSR_BE1F) {
+			netdev_dbg(priv->ndev, "Bit Error (recessive)\n");
+			cf->data[2] |= CAN_ERR_PROT_BIT1;
+			tx_errors++;
+			writeb(~RCAR_CAN_ECSR_BE1F, &priv->regs->ecsr);
+		}
+		if (ecsr & RCAR_CAN_ECSR_CEF) {
+			netdev_dbg(priv->ndev, "CRC Error\n");
+			cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
+			rx_errors++;
+			writeb(~RCAR_CAN_ECSR_CEF, &priv->regs->ecsr);
+		}
+		if (ecsr & RCAR_CAN_ECSR_AEF) {
+			netdev_dbg(priv->ndev, "ACK Error\n");
+			cf->can_id |= CAN_ERR_ACK;
+			cf->data[3] |= CAN_ERR_PROT_LOC_ACK;
+			tx_errors++;
+			writeb(~RCAR_CAN_ECSR_AEF, &priv->regs->ecsr);
+		}
+		if (ecsr & RCAR_CAN_ECSR_FEF) {
+			netdev_dbg(priv->ndev, "Form Error\n");
+			cf->data[2] |= CAN_ERR_PROT_FORM;
+			rx_errors++;
+			writeb(~RCAR_CAN_ECSR_FEF, &priv->regs->ecsr);
+		}
+		if (ecsr & RCAR_CAN_ECSR_SEF) {
+			netdev_dbg(priv->ndev, "Stuff Error\n");
+			cf->data[2] |= CAN_ERR_PROT_STUFF;
+			rx_errors++;
+			writeb(~RCAR_CAN_ECSR_SEF, &priv->regs->ecsr);
+		}
+
+		priv->can.can_stats.bus_error++;
+		ndev->stats.rx_errors += rx_errors;
+		ndev->stats.tx_errors += tx_errors;
+		writeb(~RCAR_CAN_EIFR_BEIF, &priv->regs->eifr);
+	}
+	if (eifr & RCAR_CAN_EIFR_EWIF) {
+		netdev_dbg(priv->ndev, "Error warning interrupt\n");
+		priv->can.state = CAN_STATE_ERROR_WARNING;
+		priv->can.can_stats.error_warning++;
+		cf->data[1] = txerr > rxerr ? CAN_ERR_CRTL_TX_WARNING :
+					      CAN_ERR_CRTL_RX_WARNING;
+		/* Clear interrupt condition */
+		writeb(~RCAR_CAN_EIFR_EWIF, &priv->regs->eifr);
+	}
+	if (eifr & RCAR_CAN_EIFR_EPIF) {
+		netdev_dbg(priv->ndev, "Error passive interrupt\n");
+		priv->can.state = CAN_STATE_ERROR_PASSIVE;
+		priv->can.can_stats.error_passive++;
+		cf->data[1] = txerr > rxerr ? CAN_ERR_CRTL_TX_PASSIVE :
+					      CAN_ERR_CRTL_RX_PASSIVE;
+		/* Clear interrupt condition */
+		writeb(~RCAR_CAN_EIFR_EPIF, &priv->regs->eifr);
+	}
+	if (eifr & RCAR_CAN_EIFR_BOEIF) {
+		netdev_dbg(priv->ndev, "Bus-off entry interrupt\n");
+		tx_failure_cleanup(ndev);
+		priv->ier = RCAR_CAN_IER_ERSIE;
+		writeb(priv->ier, &priv->regs->ier);
+		priv->can.state = CAN_STATE_BUS_OFF;
+		cf->can_id |= CAN_ERR_BUSOFF;
+		/* Clear interrupt condition */
+		writeb(~RCAR_CAN_EIFR_BOEIF, &priv->regs->eifr);
+		can_bus_off(ndev);
+	}
+	if (eifr & RCAR_CAN_EIFR_ORIF) {
+		netdev_dbg(priv->ndev, "Receive overrun error interrupt\n");
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+		ndev->stats.rx_over_errors++;
+		ndev->stats.rx_errors++;
+		writeb(~RCAR_CAN_EIFR_ORIF, &priv->regs->eifr);
+	}
+	if (eifr & RCAR_CAN_EIFR_OLIF) {
+		netdev_dbg(priv->ndev,
+			   "Overload Frame Transmission error interrupt\n");
+		cf->can_id |= CAN_ERR_PROT;
+		cf->data[2] |= CAN_ERR_PROT_OVERLOAD;
+		ndev->stats.rx_over_errors++;
+		ndev->stats.rx_errors++;
+		writeb(~RCAR_CAN_EIFR_OLIF, &priv->regs->eifr);
+	}
+
+	netif_rx(skb);
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+}
+
+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);
+	can_led_event(ndev, CAN_LED_EVENT_TX);
+	netif_wake_queue(ndev);
+}
+
+static irqreturn_t rcar_can_interrupt(int irq, void *dev_id)
+{
+	struct net_device *ndev = (struct net_device *)dev_id;
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	u8 isr;
+
+	isr = readb(&priv->regs->isr);
+	if (!(isr & priv->ier))
+		return IRQ_NONE;
+
+	if (isr & RCAR_CAN_ISR_ERSF)
+		rcar_can_error(ndev);
+
+	if (isr & RCAR_CAN_ISR_TXFF) {
+		/* Clear interrupt */
+		writeb(isr & ~RCAR_CAN_ISR_TXFF, &priv->regs->isr);
+		rcar_can_tx_done(ndev);
+	}
+
+	if (isr & RCAR_CAN_ISR_RXFF) {
+		if (napi_schedule_prep(&priv->napi)) {
+			/* Disable Rx FIFO interrupts */
+			priv->ier &= ~RCAR_CAN_IER_RXFIE;
+			writeb(priv->ier, &priv->regs->ier);
+			__napi_schedule(&priv->napi);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+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 */
+	clkr = readb(&priv->regs->clkr);
+	bcr = RCAR_CAN_BCR_TSEG1(bt->phase_seg1 + bt->prop_seg - 1) |
+	      RCAR_CAN_BCR_BPR(bt->brp - 1) | RCAR_CAN_BCR_SJW(bt->sjw - 1) |
+	      RCAR_CAN_BCR_TSEG2(bt->phase_seg2 - 1);
+	writel(bcr, &priv->regs->bcr);
+	writeb(clkr, &priv->regs->clkr);
+}
+
+static void rcar_can_start(struct net_device *ndev)
+{
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	u16 ctlr, str;
+
+	/* Set controller to known mode:
+	 * - FIFO mailbox mode
+	 * - accept all messages
+	 * - overrun mode
+	 * CAN is in sleep mode after MCU hardware or software reset.
+	 */
+	ctlr = readw(&priv->regs->ctlr);
+	ctlr &= ~RCAR_CAN_CTLR_SLPM;
+	writew(ctlr, &priv->regs->ctlr);
+	/* Go to reset mode */
+	ctlr |= RCAR_CAN_CTLR_CANM_FORCE_RESET;
+	writew(ctlr, &priv->regs->ctlr);
+	do {
+		str = readw(&priv->regs->str);
+	} while (!(str & RCAR_CAN_STR_RSTST));
+	rcar_can_set_bittiming(ndev);
+	ctlr |= RCAR_CAN_CTLR_IDFM_MIXED; /* Select mixed ID mode */
+	ctlr |= RCAR_CAN_CTLR_BOM_ENT;	/* Entry to halt mode automatically */
+					/* at bus-off */
+	ctlr |= RCAR_CAN_CTLR_MBM;	/* Select FIFO mailbox mode */
+	ctlr |= RCAR_CAN_CTLR_MLM;	/* Overrun mode */
+	writew(ctlr, &priv->regs->ctlr);
+
+	writeb(priv->clock_select, &priv->regs->clkr);
+
+	/* Accept all SID and EID */
+	writel(0, &priv->regs->mkr_2_9[6]);
+	writel(0, &priv->regs->mkr_2_9[7]);
+	/* In FIFO mailbox mode, write "0" to bits 24 to 31 */
+	writel(0, &priv->regs->mkivlr1);
+	/* Accept all frames */
+	writel(0, &priv->regs->fidcr[0]);
+	writel(RCAR_CAN_FIDCR_IDE | RCAR_CAN_FIDCR_RTR, &priv->regs->fidcr[1]);
+	/* Enable and configure FIFO mailbox interrupts */
+	writel(RCAR_CAN_MIER1_RXFIE | RCAR_CAN_MIER1_TXFIT |
+	       RCAR_CAN_MIER1_TXFIE, &priv->regs->mier1);
+
+	priv->ier = RCAR_CAN_IER_ERSIE | RCAR_CAN_IER_RXFIE |
+		    RCAR_CAN_IER_TXFIE;
+	writeb(priv->ier, &priv->regs->ier);
+
+	/* Accumulate error codes */
+	writeb(RCAR_CAN_ECSR_EDPM, &priv->regs->ecsr);
+	/* Enable error interrupts */
+	writeb(RCAR_CAN_EIER_EWIE | RCAR_CAN_EIER_EPIE | RCAR_CAN_EIER_BOEIE |
+	       (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING ?
+	       RCAR_CAN_EIER_BEIE : 0) | RCAR_CAN_EIER_ORIE |
+	       RCAR_CAN_EIER_OLIE, &priv->regs->eier);
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	/* Go to operation mode */
+	writew(ctlr & ~RCAR_CAN_CTLR_CANM, &priv->regs->ctlr);
+	do {
+		str = readw(&priv->regs->str);
+	} while (str & RCAR_CAN_STR_RSTST);
+	/* Enable Rx and Tx FIFO */
+	writeb(RCAR_CAN_RFCR_RFE, &priv->regs->rfcr);
+	writeb(RCAR_CAN_TFCR_TFE, &priv->regs->tfcr);
+}
+
+static int rcar_can_open(struct net_device *ndev)
+{
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	int err;
+
+	clk_prepare_enable(priv->clk);
+	err = open_candev(ndev);
+	if (err) {
+		netdev_err(ndev, "open_candev() failed %d\n", err);
+		goto out;
+	}
+	napi_enable(&priv->napi);
+	err = request_irq(ndev->irq, rcar_can_interrupt, 0, ndev->name, ndev);
+	if (err) {
+		netdev_err(ndev, "error requesting interrupt %x\n", ndev->irq);
+		goto out_close;
+	}
+	can_led_event(ndev, CAN_LED_EVENT_OPEN);
+	rcar_can_start(ndev);
+	netif_start_queue(ndev);
+	return 0;
+out_close:
+	napi_disable(&priv->napi);
+	close_candev(ndev);
+	clk_disable_unprepare(priv->clk);
+out:
+	return err;
+}
+
+static void rcar_can_stop(struct net_device *ndev)
+{
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	u16 ctlr, str;
+
+	/* Go to (force) reset mode */
+	ctlr = readw(&priv->regs->ctlr);
+	ctlr |= RCAR_CAN_CTLR_CANM_FORCE_RESET;
+	writew(ctlr, &priv->regs->ctlr);
+	do {
+		str = readw(&priv->regs->str);
+	} while (!(str & RCAR_CAN_STR_RSTST));
+	writel(0, &priv->regs->mier0);
+	writel(0, &priv->regs->mier1);
+	writeb(0, &priv->regs->ier);
+	writeb(0, &priv->regs->eier);
+	/* Go to sleep mode */
+	ctlr |= RCAR_CAN_CTLR_SLPM;
+	writew(ctlr, &priv->regs->ctlr);
+	priv->can.state = CAN_STATE_STOPPED;
+}
+
+static int rcar_can_close(struct net_device *ndev)
+{
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+
+	netif_stop_queue(ndev);
+	rcar_can_stop(ndev);
+	free_irq(ndev->irq, ndev);
+	napi_disable(&priv->napi);
+	clk_disable_unprepare(priv->clk);
+	close_candev(ndev);
+	can_led_event(ndev, CAN_LED_EVENT_STOP);
+	return 0;
+}
+
+static netdev_tx_t rcar_can_start_xmit(struct sk_buff *skb,
+				       struct net_device *ndev)
+{
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	u32 data, i;
+	unsigned long flags;
+	u8 tfcr;
+
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+	tfcr = readb(&priv->regs->tfcr);
+	if ((tfcr & RCAR_CAN_TFCR_TFUST) >> RCAR_CAN_TFCR_TFUST_SHIFT > 2)
+		netif_stop_queue(ndev);
+
+	if (cf->can_id & CAN_EFF_FLAG) {
+		/* Extended frame format */
+		data = (cf->can_id & CAN_EFF_MASK) | RCAR_CAN_IDE;
+	} else {
+		/* Standard frame format */
+		data = (cf->can_id & CAN_SFF_MASK) << RCAR_CAN_SID_SHIFT;
+	}
+	if (cf->can_id & CAN_RTR_FLAG) {
+		/* Remote transmission request */
+		data |= RCAR_CAN_RTR;
+	}
+	writel(data, &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].id);
+
+	writeb(cf->can_dlc, &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].dlc);
+
+	for (i = 0; i < cf->can_dlc; i++)
+		writeb(cf->data[i],
+		       &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].data[i]);
+
+	spin_lock_irqsave(&priv->skb_lock, flags);
+	can_put_echo_skb(skb, ndev, priv->frames_queued++);
+	priv->bytes_queued += cf->can_dlc;
+	spin_unlock_irqrestore(&priv->skb_lock, flags);
+	/* Start Tx: write 0xFF to the TFPCR register to increment
+	 * the CPU-side pointer for the transmit FIFO to the next
+	 * mailbox location
+	 */
+	writeb(0xFF, &priv->regs->tfpcr);
+
+	return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops rcar_can_netdev_ops = {
+	.ndo_open = rcar_can_open,
+	.ndo_stop = rcar_can_close,
+	.ndo_start_xmit = rcar_can_start_xmit,
+};
+
+static void rcar_can_rx_pkt(struct rcar_can_priv *priv)
+{
+	struct net_device_stats *stats = &priv->ndev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u32 data;
+	u8 dlc;
+
+	skb = alloc_can_skb(priv->ndev, &cf);
+	if (!skb) {
+		stats->rx_dropped++;
+		return;
+	}
+
+	data = readl(&priv->regs->mb[RCAR_CAN_RX_FIFO_MBX].id);
+	if (data & RCAR_CAN_IDE)
+		cf->can_id = (data & CAN_EFF_MASK) | CAN_EFF_FLAG;
+	else
+		cf->can_id = (data >> RCAR_CAN_SID_SHIFT) & CAN_SFF_MASK;
+
+	dlc = readb(&priv->regs->mb[RCAR_CAN_RX_FIFO_MBX].dlc);
+	cf->can_dlc = get_can_dlc(dlc);
+	if (data & RCAR_CAN_RTR) {
+		cf->can_id |= CAN_RTR_FLAG;
+	} else {
+		for (dlc = 0; dlc < cf->can_dlc; dlc++)
+			cf->data[dlc] +			readb(&priv->regs->mb[RCAR_CAN_RX_FIFO_MBX].data[dlc]);
+	}
+
+	can_led_event(priv->ndev, CAN_LED_EVENT_RX);
+
+	netif_receive_skb(skb);
+	stats->rx_bytes += cf->can_dlc;
+	stats->rx_packets++;
+}
+
+static int rcar_can_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct rcar_can_priv *priv = container_of(napi,
+						  struct rcar_can_priv, napi);
+	int num_pkts = 0;
+
+	while (num_pkts < quota) {
+		u8 i, rfcr, nframes, isr;
+
+		isr = readb(&priv->regs->isr);
+		/* Clear interrupt bit */
+		if (isr & RCAR_CAN_ISR_RXFF)
+			writeb(isr & ~RCAR_CAN_ISR_RXFF, &priv->regs->isr);
+		rfcr = readb(&priv->regs->rfcr);
+		if (rfcr & RCAR_CAN_RFCR_RFEST)
+			break;
+		nframes = (rfcr & RCAR_CAN_RFCR_RFUST) >>
+			  RCAR_CAN_RFCR_RFUST_SHIFT;
+		for (i = 0; i < nframes; i++) {
+			rcar_can_rx_pkt(priv);
+			/* Write 0xFF to the RFPCR register to increment
+			 * the CPU-side pointer for the receive FIFO
+			 * to the next mailbox location
+			 */
+			writeb(0xFF, &priv->regs->rfpcr);
+			++num_pkts;
+		}
+	}
+	/* All packets processed */
+	if (num_pkts < quota) {
+		napi_complete(napi);
+		priv->ier |= RCAR_CAN_IER_RXFIE;
+		writeb(priv->ier, &priv->regs->ier);
+	}
+	return num_pkts;
+}
+
+static int rcar_can_do_set_mode(struct net_device *ndev, enum can_mode mode)
+{
+	switch (mode) {
+	case CAN_MODE_START:
+		rcar_can_start(ndev);
+		netif_wake_queue(ndev);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int rcar_can_get_berr_counter(const struct net_device *dev,
+				     struct can_berr_counter *bec)
+{
+	struct rcar_can_priv *priv = netdev_priv(dev);
+
+	clk_prepare_enable(priv->clk);
+	bec->txerr = readb(&priv->regs->tecr);
+	bec->rxerr = readb(&priv->regs->recr);
+	clk_disable_unprepare(priv->clk);
+	return 0;
+}
+
+static int rcar_can_probe(struct platform_device *pdev)
+{
+	struct rcar_can_platform_data *pdata;
+	struct rcar_can_priv *priv;
+	struct net_device *ndev;
+	struct resource *mem;
+	void __iomem *addr;
+	int err = -ENODEV;
+	int irq;
+
+	pdata = dev_get_platdata(&pdev->dev);
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data provided!\n");
+		goto fail;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "No IRQ resource\n");
+		goto fail;
+	}
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	addr = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(addr)) {
+		err = PTR_ERR(addr);
+		goto fail;
+	}
+
+	ndev = alloc_candev(sizeof(struct rcar_can_priv), RCAR_CAN_N_ECHO_SKB);
+	if (!ndev) {
+		dev_err(&pdev->dev, "alloc_candev failed\n");
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	priv = netdev_priv(ndev);
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		err = PTR_ERR(priv->clk);
+		dev_err(&pdev->dev, "cannot get clock: %d\n", err);
+		goto fail_clk;
+	}
+
+	ndev->netdev_ops = &rcar_can_netdev_ops;
+	ndev->irq = irq;
+	ndev->flags |= IFF_ECHO;
+	priv->ndev = ndev;
+	priv->regs = addr;
+	priv->clock_select = pdata->clock_select;
+	priv->can.clock.freq = clk_get_rate(priv->clk);
+	priv->can.bittiming_const = &rcar_can_bittiming_const;
+	priv->can.do_set_mode = rcar_can_do_set_mode;
+	priv->can.do_get_berr_counter = rcar_can_get_berr_counter;
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
+	platform_set_drvdata(pdev, ndev);
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+	spin_lock_init(&priv->skb_lock);
+
+	netif_napi_add(ndev, &priv->napi, rcar_can_rx_poll,
+		       RCAR_CAN_NAPI_WEIGHT);
+	err = register_candev(ndev);
+	if (err) {
+		dev_err(&pdev->dev, "register_candev() failed\n");
+		goto fail_candev;
+	}
+
+	devm_can_led_init(ndev);
+
+	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%u)\n",
+		 priv->regs, ndev->irq);
+
+	return 0;
+fail_candev:
+	netif_napi_del(&priv->napi);
+fail_clk:
+	free_candev(ndev);
+fail:
+	return err;
+}
+
+static int rcar_can_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+
+	unregister_candev(ndev);
+	netif_napi_del(&priv->napi);
+	free_candev(ndev);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int rcar_can_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	u16 ctlr;
+
+	if (netif_running(ndev)) {
+		netif_stop_queue(ndev);
+		netif_device_detach(ndev);
+	}
+	ctlr = readw(&priv->regs->ctlr);
+	ctlr |= RCAR_CAN_CTLR_CANM_HALT;
+	writew(ctlr, &priv->regs->ctlr);
+	ctlr |= RCAR_CAN_CTLR_SLPM;
+	writew(ctlr, &priv->regs->ctlr);
+	priv->can.state = CAN_STATE_SLEEPING;
+
+	clk_disable(priv->clk);
+	return 0;
+}
+
+static int rcar_can_resume(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct rcar_can_priv *priv = netdev_priv(ndev);
+	u16 ctlr;
+
+	clk_enable(priv->clk);
+
+	ctlr = readw(&priv->regs->ctlr);
+	ctlr &= ~RCAR_CAN_CTLR_SLPM;
+	writew(ctlr, &priv->regs->ctlr);
+	ctlr &= ~RCAR_CAN_CTLR_CANM;
+	writew(ctlr, &priv->regs->ctlr);
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	if (netif_running(ndev)) {
+		netif_device_attach(ndev);
+		netif_start_queue(ndev);
+	}
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
+
+static struct platform_driver rcar_can_driver = {
+	.driver = {
+		.name = RCAR_CAN_DRV_NAME,
+		.owner = THIS_MODULE,
+		.pm = &rcar_can_pm_ops,
+	},
+	.probe = rcar_can_probe,
+	.remove = rcar_can_remove,
+};
+
+module_platform_driver(rcar_can_driver);
+
+MODULE_AUTHOR("Cogent Embedded, Inc.");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("CAN driver for Renesas R-Car SoC");
+MODULE_ALIAS("platform:" RCAR_CAN_DRV_NAME);
Index: linux-can-next/include/linux/can/platform/rcar_can.h
=================================--- /dev/null
+++ linux-can-next/include/linux/can/platform/rcar_can.h
@@ -0,0 +1,15 @@
+#ifndef _CAN_PLATFORM_RCAR_CAN_H_
+#define _CAN_PLATFORM_RCAR_CAN_H_
+
+#include <linux/types.h>
+
+/* Clock Select Register settings */
+#define CLKR_CLKEXT	3	/* Externally input clock */
+#define CLKR_CLKP2	1	/* Peripheral clock (clkp2) */
+#define CLKR_CLKP1	0	/* Peripheral clock (clkp1) */
+
+struct rcar_can_platform_data {
+	u8 clock_select;	/* Clock source select */
+};
+
+#endif	/* !_CAN_PLATFORM_RCAR_CAN_H_ */

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

On 27-12-2013 1:37, Sergei Shtylyov wrote:

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

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

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

> Changes in version 5:
> - removed the code specific to the mailbox mode handling and added the code
>    for the FIFO mode, dropping support for CAN_CTRLMODE_ONE_SHOT;
> - stop accumulating errors in the byte 1 of an error frame;
> - added #define RCAR_CAN_CTLR_CANM for CTLR.CANM field, added 'CANM_' infix to
>    all its values;
> - replaced ~RCAR_CAN_CTLR_CANM_FORCE_RESET with ~RCAR_CAN_CTLR_CANM;
> - added polling for the CAN reset status bit when going from/to CAN reset.

    Wolfgang, Marc, there has been no feedback from you on the last version, 
working in FIFO mode. Ping!

WBR, Sergei

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 37958 bytes --]

On 12/26/2013 10:37 PM, Sergei Shtylyov wrote:
> Add support for the CAN controller found in Renesas R-Car SoCs. 
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'linux-can-next.git' repo.
> 
> Changes in version 5:
> - removed the code specific to the mailbox mode handling and added the code
>   for the FIFO mode, dropping support for CAN_CTRLMODE_ONE_SHOT;
> - stop accumulating errors in the byte 1 of an error frame;
> - added #define RCAR_CAN_CTLR_CANM for CTLR.CANM field, added 'CANM_' infix to
>   all its values;
> - replaced ~RCAR_CAN_CTLR_CANM_FORCE_RESET with ~RCAR_CAN_CTLR_CANM; 
> - added polling for the CAN reset status bit when going from/to CAN reset.
> 
> Changes in version 4:
> - added 'RCAR_CAN_' prefix to all #define's;
> - replaced all BIT(N) invocations with (1 << N) which allowed to remove casts
>   to 'u8';
> - called rcar_can_set_bittiming() from rcar_can_start()  again and  stopped
>   registering it as do_set_bittiming() method which allowed to remove clock
>   API calls and control register writes from this function and make it *void*;
> - added #define RCAR_CAN_CTLR_IDFM for more clarity;
> - clarified comment to #define RCAR_CAN_IER_TXMIE;
> - did some whitespace cleanups.
> 
> Changes in version 3:
> - replaced the register #define's with 'struct rcar_can_regs' fields, replaced
>   rcar_can_{read|write}[bwl]() with mere {read|write}[bwl]();
> - removed hardware bus-off recovery support which allowed to also remove
>   rcar_can_start() prototype;
> - added RX/TX error count to error data frame for error warning/passive;
> - moved TX completion interrupt handling into separate function;
> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
> - removed unneeded type cast in the probe() method.
> 
> Changes in version 2:
> - added function to clean up TX mailboxes after bus error and bus-off;
> - added module parameter to enable hardware recovery from bus-off, added handler
>   for the bus-off recovery interrupt, and set the control register according to
>   the parameter value and the restart timer setting in rcar_can_start();
> - changed the way CAN_ERR_CRTL_[RT]X_{PASSIVE|WARNING} flags are set to a more
>   realistic one;
> - replaced MBX_* macros and rcar_can_mbx_{read|write}[bl]() functions with
>   'struct rcar_can_mbox_regs', 'struct rcar_can_regs', and {read|write}[bl](),
>   replaced 'reg_base' field of 'struct rcar_can_priv' with 'struct rcar_can_regs
>   __iomem *regs';
> - added 'ier' field to 'struct rcar_can_priv' to cache the current value of the
>   interrupt enable register;
> - added a check for enabled interrupts on entry to rcar_can_interrupt();
> - limited transmit mailbox search loop in rcar_can_interrupt();
> - decoupled  TX byte count increment from can_get_echo_skb() call;
> - removed netif_queue_stopped() call from rcar_can_interrupt();
> - added clk_prepare_enable()/clk_disable_unprepare() to ndo_{open|close}(),
>   do_set_bittiming(), and do_get_berr_counter() methods, removed clk_enable()
>   call from the probe() method and clk_disable() call from the remove() method;
> - allowed rcar_can_set_bittiming() to be called when the device is closed and
>   remove  explicit call to it from rcar_can_start();
> - switched to using mailbox number priority transmit mode, and switched to the
>   sequential mailbox use in ndo_start_xmit() method;
> - stopped reading the message control registers in ndo_start_xmit() method;
> - avoided returning NETDEV_TX_BUSY from ndo_start_xmit() method;
> - stopped reading data when RTR bit is set in the CAN frame;
> - made 'num_pkts' variable *int* and moved its check to the *while* condition in
>   rcar_can_rx_poll();
> - used dev_get_platdata() in the probe() method;
> - enabled bus error interrupt only if CAN_CTRLMODE_BERR_REPORTING flag is set;
> - started reporting CAN_CTRLMODE_BERR_REPORTING support and stopped reporting
>   CAN_CTRLMODE_3_SAMPLES support;
> - set CAN_ERR_ACK flag on ACK error;
> - switched to incrementing bus error counter only once per bus error interrupt;
> - started switching to CAN sleep mode in rcar_can_stop() and stopped switching
>   to it in the remove() method;
> - removed netdev_err() calls on allocation failure in rcar_can_error() and
>   rcar_can_rx_pkt();
> - removed "CANi" from the register offset macro comments.
> 
>  drivers/net/can/Kconfig               |    9 
>  drivers/net/can/Makefile              |    1 
>  drivers/net/can/rcar_can.c            |  857 ++++++++++++++++++++++++++++++++++
>  include/linux/can/platform/rcar_can.h |   15 
>  4 files changed, 882 insertions(+)
> 
> Index: linux-can-next/drivers/net/can/Kconfig
> ===================================================================
> --- linux-can-next.orig/drivers/net/can/Kconfig
> +++ linux-can-next/drivers/net/can/Kconfig
> @@ -125,6 +125,15 @@ config CAN_GRCAN
>  	  endian syntheses of the cores would need some modifications on
>  	  the hardware level to work.
>  
> +config CAN_RCAR
> +	tristate "Renesas R-Car CAN controller"
> +	---help---
> +	  Say Y here if you want to use CAN controller found on Renesas R-Car
> +	  SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called rcar_can.
> +
>  source "drivers/net/can/mscan/Kconfig"
>  
>  source "drivers/net/can/sja1000/Kconfig"
> Index: linux-can-next/drivers/net/can/Makefile
> ===================================================================
> --- linux-can-next.orig/drivers/net/can/Makefile
> +++ linux-can-next/drivers/net/can/Makefile
> @@ -25,5 +25,6 @@ obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ica
>  obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
>  obj-$(CONFIG_PCH_CAN)		+= pch_can.o
>  obj-$(CONFIG_CAN_GRCAN)		+= grcan.o
> +obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> 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 @@
> +/*
> + * Renesas R-Car CAN device driver
> + *
> + * Copyright (C) 2013 Cogent Embedded, Inc. <source@cogentembedded.com>
> + * Copyright (C) 2013 Renesas Solutions Corp.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/errno.h>
> +#include <linux/netdevice.h>
> +#include <linux/platform_device.h>
> +#include <linux/can/led.h>
> +#include <linux/can/dev.h>
> +#include <linux/clk.h>
> +#include <linux/can/platform/rcar_can.h>
> +
> +#define RCAR_CAN_DRV_NAME	"rcar_can"
> +
> +/* Mailbox configuration:
> + * mailbox 60 - 63 - Rx FIFO mailboxes
> + * mailbox 56 - 59 - Tx FIFO mailboxes
> + * non-FIFO mailboxes are not used
> + */
> +#define RCAR_CAN_N_MBX		64 /* Number of mailboxes in non-FIFO mode */
> +#define RCAR_CAN_RX_FIFO_MBX	60 /* Mailbox - window to Rx FIFO */
> +#define RCAR_CAN_TX_FIFO_MBX	56 /* Mailbox - window to Tx FIFO */
> +#define RCAR_CAN_N_ECHO_SKB	10
> +
> +/* 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;

If you have contact to the hardware designer please blame him for
placing the data register unaligned into the register space. :)

> +
> +struct rcar_can_regs {
> +	struct rcar_can_mbox_regs mb[RCAR_CAN_N_MBX]; /* Mailbox registers */
> +	u32 mkr_2_9[8];	/* Mask Registers 2-9 */
> +	u32 fidcr[2];	/* FIFO Received ID Compare Register */
> +	u32 mkivlr1;	/* Mask Invalid Register 1 */
> +	u32 mier1;	/* Mailbox Interrupt Enable Register 1 */
> +	u32 mkr_0_1[2];	/* Mask Registers 0-1 */
> +	u32 mkivlr0;    /* Mask Invalid Register 0*/
> +	u32 mier0;      /* Mailbox Interrupt Enable Register 0 */
> +	u8 pad_440[0x3c0];
> +	u8 mctl[64];	/* Message Control Registers */
> +	u16 ctlr;	/* Control Register */
> +	u16 str;	/* Status register */
> +	u8 bcr[3];	/* Bit Configuration Register */
> +	u8 clkr;	/* Clock Select Register */

bcr[3] and clkr looks broken, see comment where the register is used.

> +	u8 rfcr;	/* Receive FIFO Control Register */
> +	u8 rfpcr;	/* Receive FIFO Pointer Control Register */
> +	u8 tfcr;	/* Transmit FIFO Control Register */
> +	u8 tfpcr;       /* Transmit FIFO Pointer Control Register */
> +	u8 eier;	/* Error Interrupt Enable Register */
> +	u8 eifr;	/* Error Interrupt Factor Judge Register */
> +	u8 recr;	/* Receive Error Count Register */
> +	u8 tecr;        /* Transmit Error Count Register */
> +	u8 ecsr;	/* Error Code Store Register */
> +	u8 cssr;	/* Channel Search Support Register */
> +	u8 mssr;	/* Mailbox Search Status Register */
> +	u8 msmr;	/* Mailbox Search Mode Register */
> +	u16 tsr;	/* Time Stamp Register */
> +	u8 afsr;	/* Acceptance Filter Support Register */
> +	u8 pad_857;
> +	u8 tcr;		/* Test Control Register */
> +	u8 pad_859[7];
> +	u8 ier;		/* Interrupt Enable Register */
> +	u8 isr;		/* Interrupt Status Register */
> +	u8 pad_862;
> +	u8 mbsmr;	/* Mailbox Search Mask Register */
> +} __packed;
> +
> +struct rcar_can_priv {
> +	struct can_priv can;	/* Must be the first member! */
> +	struct net_device *ndev;
> +	struct napi_struct napi;
> +	struct rcar_can_regs __iomem *regs;
> +	struct clk *clk;
> +	spinlock_t skb_lock;
> +	u32 frames_queued;
> +	u32 bytes_queued;
> +	u8 clock_select;
> +	u8 ier;
> +};
> +
> +static const struct can_bittiming_const rcar_can_bittiming_const = {
> +	.name = RCAR_CAN_DRV_NAME,
> +	.tseg1_min = 4,
> +	.tseg1_max = 16,
> +	.tseg2_min = 2,
> +	.tseg2_max = 8,
> +	.sjw_max = 4,
> +	.brp_min = 1,
> +	.brp_max = 1024,
> +	.brp_inc = 1,
> +};
> +
> +/* Control Register bits */
> +#define RCAR_CAN_CTLR_BOM	(3 << 11) /* Bus-Off Recovery Mode Bits */
> +#define RCAR_CAN_CTLR_BOM_ENT	(1 << 11) /* Entry to halt mode */
> +					/* at bus-off entry */
> +#define RCAR_CAN_CTLR_SLPM	(1 << 10)
> +#define RCAR_CAN_CTLR_CANM	(3 << 8) /* Operating Mode Select Bit */
> +#define RCAR_CAN_CTLR_CANM_HALT	(1 << 9)
> +#define RCAR_CAN_CTLR_CANM_RESET (1 << 8)
> +#define RCAR_CAN_CTLR_CANM_FORCE_RESET (3 << 8)
> +#define RCAR_CAN_CTLR_MLM	(1 << 3) /* Message Lost Mode Select */
> +#define RCAR_CAN_CTLR_IDFM	(3 << 1) /* ID Format Mode Select Bits */
> +#define RCAR_CAN_CTLR_IDFM_MIXED (1 << 2) /* Mixed ID mode */
> +#define RCAR_CAN_CTLR_MBM	(1 << 0) /* Mailbox Mode select */
> +
> +/* Status Register bits */
> +#define RCAR_CAN_STR_RSTST	(1 << 8) /* Reset Status Bit */
> +
> +/* FIFO Received ID Compare Registers 0 and 1 bits */
> +#define RCAR_CAN_FIDCR_IDE	(1 << 31) /* ID Extension Bit */
> +#define RCAR_CAN_FIDCR_RTR	(1 << 30) /* Remote Transmission Request Bit */
> +
> +/* Receive FIFO Control Register bits */
> +#define RCAR_CAN_RFCR_RFEST	(1 << 7) /* Receive FIFO Empty Status Flag */
> +#define RCAR_CAN_RFCR_RFUST	(7 << 1) /* Receive FIFO Unread Message */
> +					/* Number Status Bits */
> +#define RCAR_CAN_RFCR_RFUST_SHIFT 1	/* Offset of Receive FIFO Unread */
> +					/* Message Number Status Bits */
> +#define RCAR_CAN_RFCR_RFE	(1 << 0) /* Receive FIFO Enable */
> +
> +/* Transmit FIFO Control Register bits */
> +#define RCAR_CAN_TFCR_TFUST	(7 << 1) /* Transmit FIFO Unsent Message */
> +					/* Number Status Bits */
> +#define RCAR_CAN_TFCR_TFUST_SHIFT 1	/* Offset of Transmit FIFO Unsent */
> +					/* Message Number Status Bits */
> +#define RCAR_CAN_TFCR_TFE	(1 << 0) /* Transmit FIFO Enable */
> +
> +#define RCAR_CAN_N_RX_MKREGS1	2	/* Number of mask registers */
> +					/* for Rx mailboxes 0-31 */
> +#define RCAR_CAN_N_RX_MKREGS2	8
> +
> +/* Bit Configuration Register settings */
> +#define RCAR_CAN_BCR_TSEG1(x)	(((x) & 0x0f) << 28)
> +#define RCAR_CAN_BCR_BPR(x)	(((x) & 0x3ff) << 16)
> +#define RCAR_CAN_BCR_SJW(x)	(((x) & 0x3) << 12)
> +#define RCAR_CAN_BCR_TSEG2(x)	(((x) & 0x07) << 8)
> +
> +/* Mailbox and Mask Registers bits */
> +#define RCAR_CAN_IDE		(1 << 31)
> +#define RCAR_CAN_RTR		(1 << 30)
> +#define RCAR_CAN_SID_SHIFT	18
> +
> +/* Mailbox Interrupt Enable Register 1 bits */
> +#define RCAR_CAN_MIER1_RXFIE	(1 << 28) /* Receive  FIFO Interrupt Enable */
> +#define RCAR_CAN_MIER1_TXFIT	(1 << 25) /* Transmit FIFO Interrupt Timing */
> +					/* Generate interrupt when */
> +					/* Tx FIFO becomes empty due to */
> +					/* completion of transmission */
> +#define RCAR_CAN_MIER1_TXFIE	(1 << 24) /* Transmit FIFO Interrupt Enable */
> +
> +/* Interrupt Enable Register bits */
> +#define RCAR_CAN_IER_ERSIE	(1 << 5) /* Error (ERS) Interrupt Enable Bit */
> +#define RCAR_CAN_IER_RXFIE	(1 << 4) /* Reception FIFO Interrupt */
> +					/* Enable Bit */
> +#define RCAR_CAN_IER_TXFIE	(1 << 3) /* Transmission FIFO Interrupt */
> +					/* Enable Bit */
> +/* Interrupt Status Register bits */
> +#define RCAR_CAN_ISR_ERSF	(1 << 5) /* Error (ERS) Interrupt Status Bit */
> +#define RCAR_CAN_ISR_RXFF	(1 << 4) /* Reception FIFO Interrupt */
> +					/* Status Bit */
> +#define RCAR_CAN_ISR_TXFF	(1 << 3) /* Transmission FIFO Interrupt */
> +					/* Status Bit */
> +
> +/* Error Interrupt Enable Register bits */
> +#define RCAR_CAN_EIER_BLIE	(1 << 7) /* Bus Lock Interrupt Enable */
> +#define RCAR_CAN_EIER_OLIE	(1 << 6) /* Overload Frame Transmit */
> +					/* Interrupt Enable */
> +#define RCAR_CAN_EIER_ORIE	(1 << 5) /* Receive Overrun  Interrupt Enable */
> +#define RCAR_CAN_EIER_BORIE	(1 << 4) /* Bus-Off Recovery Interrupt Enable */
> +#define RCAR_CAN_EIER_BOEIE	(1 << 3) /* Bus-Off Entry Interrupt Enable */
> +#define RCAR_CAN_EIER_EPIE	(1 << 2) /* Error Passive Interrupt Enable */
> +#define RCAR_CAN_EIER_EWIE	(1 << 1) /* Error Warning Interrupt Enable */
> +#define RCAR_CAN_EIER_BEIE	(1 << 0) /* Bus Error Interrupt Enable */
> +
> +/* Error Interrupt Factor Judge Register bits */
> +#define RCAR_CAN_EIFR_BLIF	(1 << 7) /* Bus Lock Detect Flag */
> +#define RCAR_CAN_EIFR_OLIF	(1 << 6) /* Overload Frame Transmission */
> +					 /* Detect Flag */
> +#define RCAR_CAN_EIFR_ORIF	(1 << 5) /* Receive Overrun Detect Flag */
> +#define RCAR_CAN_EIFR_BORIF	(1 << 4) /* Bus-Off Recovery Detect Flag */
> +#define RCAR_CAN_EIFR_BOEIF	(1 << 3) /* Bus-Off Entry Detect Flag */
> +#define RCAR_CAN_EIFR_EPIF	(1 << 2) /* Error Passive Detect Flag */
> +#define RCAR_CAN_EIFR_EWIF	(1 << 1) /* Error Warning Detect Flag */
> +#define RCAR_CAN_EIFR_BEIF	(1 << 0) /* Bus Error Detect Flag */
> +
> +/* Error Code Store Register bits */
> +#define RCAR_CAN_ECSR_EDPM	(1 << 7) /* Error Display Mode Select Bit */
> +#define RCAR_CAN_ECSR_ADEF	(1 << 6) /* ACK Delimiter Error Flag */
> +#define RCAR_CAN_ECSR_BE0F	(1 << 5) /* Bit Error (dominant) Flag */
> +#define RCAR_CAN_ECSR_BE1F	(1 << 4) /* Bit Error (recessive) Flag */
> +#define RCAR_CAN_ECSR_CEF	(1 << 3) /* CRC Error Flag */
> +#define RCAR_CAN_ECSR_AEF	(1 << 2) /* ACK Error Flag */
> +#define RCAR_CAN_ECSR_FEF	(1 << 1) /* Form Error Flag */
> +#define RCAR_CAN_ECSR_SEF	(1 << 0) /* Stuff Error Flag */
> +
> +#define RCAR_CAN_NAPI_WEIGHT 4
> +
> +static void tx_failure_cleanup(struct net_device *ndev)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < RCAR_CAN_N_ECHO_SKB; i++)
> +		can_free_echo_skb(ndev, i);
> +}
> +
> +static void rcar_can_error(struct net_device *ndev)
> +{
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u8 eifr, txerr = 0, rxerr = 0;
> +
> +	/* Propagate the error condition to the CAN stack */
> +	skb = alloc_can_err_skb(ndev, &cf);
> +	if (!skb)
> +		return;
> +
> +	eifr = readb(&priv->regs->eifr);
> +	if (eifr & (RCAR_CAN_EIFR_EWIF | RCAR_CAN_EIFR_EPIF)) {
> +		cf->can_id |= CAN_ERR_CRTL;
> +		txerr = readb(&priv->regs->tecr);
> +		rxerr = readb(&priv->regs->recr);
> +		cf->data[6] = txerr;
> +		cf->data[7] = rxerr;
> +	}
> +	if (eifr & RCAR_CAN_EIFR_BEIF) {
> +		int rx_errors = 0, tx_errors = 0;
> +		u8 ecsr;
> +
> +		netdev_dbg(priv->ndev, "Bus error interrupt:\n");
> +		cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> +		cf->data[2] = CAN_ERR_PROT_UNSPEC;
> +
> +		ecsr = readb(&priv->regs->ecsr);
> +		if (ecsr & RCAR_CAN_ECSR_ADEF) {
> +			netdev_dbg(priv->ndev, "ACK Delimiter Error\n");
> +			cf->data[3] |= CAN_ERR_PROT_LOC_ACK_DEL;
> +			tx_errors++;
> +			writeb(~RCAR_CAN_ECSR_ADEF, &priv->regs->ecsr);
> +		}
> +		if (ecsr & RCAR_CAN_ECSR_BE0F) {
> +			netdev_dbg(priv->ndev, "Bit Error (dominant)\n");
> +			cf->data[2] |= CAN_ERR_PROT_BIT0;
> +			tx_errors++;
> +			writeb(~RCAR_CAN_ECSR_BE0F, &priv->regs->ecsr);
> +		}
> +		if (ecsr & RCAR_CAN_ECSR_BE1F) {
> +			netdev_dbg(priv->ndev, "Bit Error (recessive)\n");
> +			cf->data[2] |= CAN_ERR_PROT_BIT1;
> +			tx_errors++;
> +			writeb(~RCAR_CAN_ECSR_BE1F, &priv->regs->ecsr);
> +		}
> +		if (ecsr & RCAR_CAN_ECSR_CEF) {
> +			netdev_dbg(priv->ndev, "CRC Error\n");
> +			cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> +			rx_errors++;
> +			writeb(~RCAR_CAN_ECSR_CEF, &priv->regs->ecsr);
> +		}
> +		if (ecsr & RCAR_CAN_ECSR_AEF) {
> +			netdev_dbg(priv->ndev, "ACK Error\n");
> +			cf->can_id |= CAN_ERR_ACK;
> +			cf->data[3] |= CAN_ERR_PROT_LOC_ACK;
> +			tx_errors++;
> +			writeb(~RCAR_CAN_ECSR_AEF, &priv->regs->ecsr);
> +		}
> +		if (ecsr & RCAR_CAN_ECSR_FEF) {
> +			netdev_dbg(priv->ndev, "Form Error\n");
> +			cf->data[2] |= CAN_ERR_PROT_FORM;
> +			rx_errors++;
> +			writeb(~RCAR_CAN_ECSR_FEF, &priv->regs->ecsr);
> +		}
> +		if (ecsr & RCAR_CAN_ECSR_SEF) {
> +			netdev_dbg(priv->ndev, "Stuff Error\n");
> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
> +			rx_errors++;
> +			writeb(~RCAR_CAN_ECSR_SEF, &priv->regs->ecsr);
> +		}
> +
> +		priv->can.can_stats.bus_error++;
> +		ndev->stats.rx_errors += rx_errors;
> +		ndev->stats.tx_errors += tx_errors;
> +		writeb(~RCAR_CAN_EIFR_BEIF, &priv->regs->eifr);
> +	}
> +	if (eifr & RCAR_CAN_EIFR_EWIF) {
> +		netdev_dbg(priv->ndev, "Error warning interrupt\n");
> +		priv->can.state = CAN_STATE_ERROR_WARNING;
> +		priv->can.can_stats.error_warning++;
> +		cf->data[1] = txerr > rxerr ? CAN_ERR_CRTL_TX_WARNING :
> +					      CAN_ERR_CRTL_RX_WARNING;
> +		/* Clear interrupt condition */
> +		writeb(~RCAR_CAN_EIFR_EWIF, &priv->regs->eifr);
> +	}
> +	if (eifr & RCAR_CAN_EIFR_EPIF) {
> +		netdev_dbg(priv->ndev, "Error passive interrupt\n");
> +		priv->can.state = CAN_STATE_ERROR_PASSIVE;
> +		priv->can.can_stats.error_passive++;
> +		cf->data[1] = txerr > rxerr ? CAN_ERR_CRTL_TX_PASSIVE :
> +					      CAN_ERR_CRTL_RX_PASSIVE;
> +		/* Clear interrupt condition */
> +		writeb(~RCAR_CAN_EIFR_EPIF, &priv->regs->eifr);
> +	}
> +	if (eifr & RCAR_CAN_EIFR_BOEIF) {
> +		netdev_dbg(priv->ndev, "Bus-off entry interrupt\n");
> +		tx_failure_cleanup(ndev);
> +		priv->ier = RCAR_CAN_IER_ERSIE;
> +		writeb(priv->ier, &priv->regs->ier);
> +		priv->can.state = CAN_STATE_BUS_OFF;
> +		cf->can_id |= CAN_ERR_BUSOFF;
> +		/* Clear interrupt condition */
> +		writeb(~RCAR_CAN_EIFR_BOEIF, &priv->regs->eifr);
> +		can_bus_off(ndev);
> +	}
> +	if (eifr & RCAR_CAN_EIFR_ORIF) {
> +		netdev_dbg(priv->ndev, "Receive overrun error interrupt\n");
> +		cf->can_id |= CAN_ERR_CRTL;
> +		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +		ndev->stats.rx_over_errors++;
> +		ndev->stats.rx_errors++;
> +		writeb(~RCAR_CAN_EIFR_ORIF, &priv->regs->eifr);
> +	}
> +	if (eifr & RCAR_CAN_EIFR_OLIF) {
> +		netdev_dbg(priv->ndev,
> +			   "Overload Frame Transmission error interrupt\n");
> +		cf->can_id |= CAN_ERR_PROT;
> +		cf->data[2] |= CAN_ERR_PROT_OVERLOAD;
> +		ndev->stats.rx_over_errors++;
> +		ndev->stats.rx_errors++;
> +		writeb(~RCAR_CAN_EIFR_OLIF, &priv->regs->eifr);
> +	}
> +
> +	netif_rx(skb);
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;
> +}
> +
> +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);

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.

> +	can_led_event(ndev, CAN_LED_EVENT_TX);
> +	netif_wake_queue(ndev);
> +}
> +
> +static irqreturn_t rcar_can_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = (struct net_device *)dev_id;
the cast is not needed
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	u8 isr;
> +
> +	isr = readb(&priv->regs->isr);
> +	if (!(isr & priv->ier))
> +		return IRQ_NONE;
> +
> +	if (isr & RCAR_CAN_ISR_ERSF)
> +		rcar_can_error(ndev);
> +
> +	if (isr & RCAR_CAN_ISR_TXFF) {
> +		/* Clear interrupt */
> +		writeb(isr & ~RCAR_CAN_ISR_TXFF, &priv->regs->isr);
> +		rcar_can_tx_done(ndev);
> +	}
> +
> +	if (isr & RCAR_CAN_ISR_RXFF) {
> +		if (napi_schedule_prep(&priv->napi)) {
> +			/* Disable Rx FIFO interrupts */
> +			priv->ier &= ~RCAR_CAN_IER_RXFIE;
> +			writeb(priv->ier, &priv->regs->ier);
> +			__napi_schedule(&priv->napi);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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 */

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?

> +	clkr = readb(&priv->regs->clkr);
> +	bcr = RCAR_CAN_BCR_TSEG1(bt->phase_seg1 + bt->prop_seg - 1) |
> +	      RCAR_CAN_BCR_BPR(bt->brp - 1) | RCAR_CAN_BCR_SJW(bt->sjw - 1) |
> +	      RCAR_CAN_BCR_TSEG2(bt->phase_seg2 - 1);
> +	writel(bcr, &priv->regs->bcr);
> +	writeb(clkr, &priv->regs->clkr);
> +}
> +
> +static void rcar_can_start(struct net_device *ndev)
> +{
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	u16 ctlr, str;
> +
> +	/* Set controller to known mode:
> +	 * - FIFO mailbox mode
> +	 * - accept all messages
> +	 * - overrun mode
> +	 * CAN is in sleep mode after MCU hardware or software reset.
> +	 */
> +	ctlr = readw(&priv->regs->ctlr);
> +	ctlr &= ~RCAR_CAN_CTLR_SLPM;
> +	writew(ctlr, &priv->regs->ctlr);
> +	/* Go to reset mode */
> +	ctlr |= RCAR_CAN_CTLR_CANM_FORCE_RESET;
> +	writew(ctlr, &priv->regs->ctlr);
> +	do {
> +		str = readw(&priv->regs->str);
> +	} while (!(str & RCAR_CAN_STR_RSTST));

Please add a timeout for this loop and the loop below.

> +	rcar_can_set_bittiming(ndev);
> +	ctlr |= RCAR_CAN_CTLR_IDFM_MIXED; /* Select mixed ID mode */
> +	ctlr |= RCAR_CAN_CTLR_BOM_ENT;	/* Entry to halt mode automatically */
> +					/* at bus-off */
> +	ctlr |= RCAR_CAN_CTLR_MBM;	/* Select FIFO mailbox mode */
> +	ctlr |= RCAR_CAN_CTLR_MLM;	/* Overrun mode */
> +	writew(ctlr, &priv->regs->ctlr);
> +
> +	writeb(priv->clock_select, &priv->regs->clkr);
> +
> +	/* Accept all SID and EID */
> +	writel(0, &priv->regs->mkr_2_9[6]);
> +	writel(0, &priv->regs->mkr_2_9[7]);
> +	/* In FIFO mailbox mode, write "0" to bits 24 to 31 */
> +	writel(0, &priv->regs->mkivlr1);
> +	/* Accept all frames */
> +	writel(0, &priv->regs->fidcr[0]);
> +	writel(RCAR_CAN_FIDCR_IDE | RCAR_CAN_FIDCR_RTR, &priv->regs->fidcr[1]);
> +	/* Enable and configure FIFO mailbox interrupts */
> +	writel(RCAR_CAN_MIER1_RXFIE | RCAR_CAN_MIER1_TXFIT |
> +	       RCAR_CAN_MIER1_TXFIE, &priv->regs->mier1);
> +
> +	priv->ier = RCAR_CAN_IER_ERSIE | RCAR_CAN_IER_RXFIE |
> +		    RCAR_CAN_IER_TXFIE;
> +	writeb(priv->ier, &priv->regs->ier);
> +
> +	/* Accumulate error codes */
> +	writeb(RCAR_CAN_ECSR_EDPM, &priv->regs->ecsr);
> +	/* Enable error interrupts */
> +	writeb(RCAR_CAN_EIER_EWIE | RCAR_CAN_EIER_EPIE | RCAR_CAN_EIER_BOEIE |
> +	       (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING ?
> +	       RCAR_CAN_EIER_BEIE : 0) | RCAR_CAN_EIER_ORIE |
> +	       RCAR_CAN_EIER_OLIE, &priv->regs->eier);
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +	/* Go to operation mode */
> +	writew(ctlr & ~RCAR_CAN_CTLR_CANM, &priv->regs->ctlr);
> +	do {
> +		str = readw(&priv->regs->str);
> +	} while (str & RCAR_CAN_STR_RSTST);
> +	/* Enable Rx and Tx FIFO */
> +	writeb(RCAR_CAN_RFCR_RFE, &priv->regs->rfcr);
> +	writeb(RCAR_CAN_TFCR_TFE, &priv->regs->tfcr);
> +}
> +
> +static int rcar_can_open(struct net_device *ndev)
> +{
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	int err;
> +
> +	clk_prepare_enable(priv->clk);

clk_prepare_enable can fail

> +	err = open_candev(ndev);
> +	if (err) {
> +		netdev_err(ndev, "open_candev() failed %d\n", err);
> +		goto out;

please adjust the jump label, you have to disable the clock.

> +	}
> +	napi_enable(&priv->napi);
> +	err = request_irq(ndev->irq, rcar_can_interrupt, 0, ndev->name, ndev);
> +	if (err) {
> +		netdev_err(ndev, "error requesting interrupt %x\n", ndev->irq);
> +		goto out_close;
> +	}
> +	can_led_event(ndev, CAN_LED_EVENT_OPEN);
> +	rcar_can_start(ndev);
> +	netif_start_queue(ndev);
> +	return 0;
> +out_close:
> +	napi_disable(&priv->napi);
> +	close_candev(ndev);
> +	clk_disable_unprepare(priv->clk);
> +out:
> +	return err;
> +}
> +
> +static void rcar_can_stop(struct net_device *ndev)
> +{
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	u16 ctlr, str;
> +
> +	/* Go to (force) reset mode */
> +	ctlr = readw(&priv->regs->ctlr);
> +	ctlr |= RCAR_CAN_CTLR_CANM_FORCE_RESET;
> +	writew(ctlr, &priv->regs->ctlr);
> +	do {
> +		str = readw(&priv->regs->str);
> +	} while (!(str & RCAR_CAN_STR_RSTST));

please add a timeout to the loop

> +	writel(0, &priv->regs->mier0);
> +	writel(0, &priv->regs->mier1);
> +	writeb(0, &priv->regs->ier);
> +	writeb(0, &priv->regs->eier);
> +	/* Go to sleep mode */
> +	ctlr |= RCAR_CAN_CTLR_SLPM;
> +	writew(ctlr, &priv->regs->ctlr);
> +	priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +static int rcar_can_close(struct net_device *ndev)
> +{
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +
> +	netif_stop_queue(ndev);
> +	rcar_can_stop(ndev);
> +	free_irq(ndev->irq, ndev);
> +	napi_disable(&priv->napi);
> +	clk_disable_unprepare(priv->clk);
> +	close_candev(ndev);
> +	can_led_event(ndev, CAN_LED_EVENT_STOP);
> +	return 0;
> +}
> +
> +static netdev_tx_t rcar_can_start_xmit(struct sk_buff *skb,
> +				       struct net_device *ndev)
> +{
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	u32 data, i;
> +	unsigned long flags;
> +	u8 tfcr;
> +
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +	tfcr = readb(&priv->regs->tfcr);
> +	if ((tfcr & RCAR_CAN_TFCR_TFUST) >> RCAR_CAN_TFCR_TFUST_SHIFT > 2)
> +		netif_stop_queue(ndev);

Can you explain what's checked here?

> +
> +	if (cf->can_id & CAN_EFF_FLAG) {
> +		/* Extended frame format */
> +		data = (cf->can_id & CAN_EFF_MASK) | RCAR_CAN_IDE;
> +	} else {
> +		/* Standard frame format */
> +		data = (cf->can_id & CAN_SFF_MASK) << RCAR_CAN_SID_SHIFT;
> +	}
> +	if (cf->can_id & CAN_RTR_FLAG) {
> +		/* Remote transmission request */
> +		data |= RCAR_CAN_RTR;
> +	}

You can move the comments into the line of if and else and remove the {
& } as there is only one line after if and else.

> +	writel(data, &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].id);
> +
> +	writeb(cf->can_dlc, &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].dlc);
> +
> +	for (i = 0; i < cf->can_dlc; i++)
> +		writeb(cf->data[i],
> +		       &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].data[i]);
> +
> +	spin_lock_irqsave(&priv->skb_lock, flags);
> +	can_put_echo_skb(skb, ndev, priv->frames_queued++);
> +	priv->bytes_queued += cf->can_dlc;

How does the frames_queued and bytes_queued mechanism work?

> +	spin_unlock_irqrestore(&priv->skb_lock, flags);
> +	/* Start Tx: write 0xFF to the TFPCR register to increment
> +	 * the CPU-side pointer for the transmit FIFO to the next
> +	 * mailbox location
> +	 */
> +	writeb(0xFF, &priv->regs->tfpcr);

please use lowercase for hex.

> +
> +	return NETDEV_TX_OK;

I'm missing flow control here. You have to stop the queue if there isn't
any room in the tx fifo.

> +}
> +
> +static const struct net_device_ops rcar_can_netdev_ops = {
> +	.ndo_open = rcar_can_open,
> +	.ndo_stop = rcar_can_close,
> +	.ndo_start_xmit = rcar_can_start_xmit,
> +};
> +
> +static void rcar_can_rx_pkt(struct rcar_can_priv *priv)
> +{
> +	struct net_device_stats *stats = &priv->ndev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u32 data;
> +	u8 dlc;
> +
> +	skb = alloc_can_skb(priv->ndev, &cf);
> +	if (!skb) {
> +		stats->rx_dropped++;
> +		return;
> +	}
> +
> +	data = readl(&priv->regs->mb[RCAR_CAN_RX_FIFO_MBX].id);
> +	if (data & RCAR_CAN_IDE)
> +		cf->can_id = (data & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +	else
> +		cf->can_id = (data >> RCAR_CAN_SID_SHIFT) & CAN_SFF_MASK;
> +
> +	dlc = readb(&priv->regs->mb[RCAR_CAN_RX_FIFO_MBX].dlc);
> +	cf->can_dlc = get_can_dlc(dlc);
> +	if (data & RCAR_CAN_RTR) {
> +		cf->can_id |= CAN_RTR_FLAG;
> +	} else {
> +		for (dlc = 0; dlc < cf->can_dlc; dlc++)
> +			cf->data[dlc] =
> +			readb(&priv->regs->mb[RCAR_CAN_RX_FIFO_MBX].data[dlc]);
> +	}
> +
> +	can_led_event(priv->ndev, CAN_LED_EVENT_RX);
> +
> +	netif_receive_skb(skb);
> +	stats->rx_bytes += cf->can_dlc;
> +	stats->rx_packets++;
> +}
> +
> +static int rcar_can_rx_poll(struct napi_struct *napi, int quota)
> +{
> +	struct rcar_can_priv *priv = container_of(napi,
> +						  struct rcar_can_priv, napi);
> +	int num_pkts = 0;
> +
> +	while (num_pkts < quota) {
> +		u8 i, rfcr, nframes, isr;
> +
> +		isr = readb(&priv->regs->isr);
> +		/* Clear interrupt bit */
> +		if (isr & RCAR_CAN_ISR_RXFF)
> +			writeb(isr & ~RCAR_CAN_ISR_RXFF, &priv->regs->isr);
> +		rfcr = readb(&priv->regs->rfcr);
> +		if (rfcr & RCAR_CAN_RFCR_RFEST)
> +			break;
> +		nframes = (rfcr & RCAR_CAN_RFCR_RFUST) >>
> +			  RCAR_CAN_RFCR_RFUST_SHIFT;
> +		for (i = 0; i < nframes; i++) {
> +			rcar_can_rx_pkt(priv);
> +			/* Write 0xFF to the RFPCR register to increment
> +			 * the CPU-side pointer for the receive FIFO
> +			 * to the next mailbox location
> +			 */
> +			writeb(0xFF, &priv->regs->rfpcr);
> +			++num_pkts;
> +		}

The for loop inside the while loop makes no sense if you increment
num_pkts. You are not allowed to receive more than quota CAN frames.

> +	}
> +	/* All packets processed */
> +	if (num_pkts < quota) {
> +		napi_complete(napi);
> +		priv->ier |= RCAR_CAN_IER_RXFIE;
> +		writeb(priv->ier, &priv->regs->ier);
> +	}
> +	return num_pkts;
> +}
> +
> +static int rcar_can_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		rcar_can_start(ndev);
> +		netif_wake_queue(ndev);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int rcar_can_get_berr_counter(const struct net_device *dev,
> +				     struct can_berr_counter *bec)
> +{
> +	struct rcar_can_priv *priv = netdev_priv(dev);
> +
> +	clk_prepare_enable(priv->clk);

clk_prepare_enable can fail

> +	bec->txerr = readb(&priv->regs->tecr);
> +	bec->rxerr = readb(&priv->regs->recr);
> +	clk_disable_unprepare(priv->clk);
> +	return 0;
> +}
> +
> +static int rcar_can_probe(struct platform_device *pdev)
> +{
> +	struct rcar_can_platform_data *pdata;
> +	struct rcar_can_priv *priv;
> +	struct net_device *ndev;
> +	struct resource *mem;
> +	void __iomem *addr;
> +	int err = -ENODEV;
> +	int irq;
> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "No platform data provided!\n");
> +		goto fail;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (!irq) {
> +		dev_err(&pdev->dev, "No IRQ resource\n");
> +		goto fail;
> +	}
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	addr = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(addr)) {
> +		err = PTR_ERR(addr);
> +		goto fail;
> +	}
> +
> +	ndev = alloc_candev(sizeof(struct rcar_can_priv), RCAR_CAN_N_ECHO_SKB);
> +	if (!ndev) {
> +		dev_err(&pdev->dev, "alloc_candev failed\n");
> +		err = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	priv = netdev_priv(ndev);
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		err = PTR_ERR(priv->clk);
> +		dev_err(&pdev->dev, "cannot get clock: %d\n", err);
> +		goto fail_clk;
> +	}
> +
> +	ndev->netdev_ops = &rcar_can_netdev_ops;
> +	ndev->irq = irq;
> +	ndev->flags |= IFF_ECHO;
> +	priv->ndev = ndev;
> +	priv->regs = addr;
> +	priv->clock_select = pdata->clock_select;
> +	priv->can.clock.freq = clk_get_rate(priv->clk);
> +	priv->can.bittiming_const = &rcar_can_bittiming_const;
> +	priv->can.do_set_mode = rcar_can_do_set_mode;
> +	priv->can.do_get_berr_counter = rcar_can_get_berr_counter;
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
> +	platform_set_drvdata(pdev, ndev);
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +	spin_lock_init(&priv->skb_lock);
> +
> +	netif_napi_add(ndev, &priv->napi, rcar_can_rx_poll,
> +		       RCAR_CAN_NAPI_WEIGHT);
> +	err = register_candev(ndev);
> +	if (err) {
> +		dev_err(&pdev->dev, "register_candev() failed\n");
> +		goto fail_candev;
> +	}
> +
> +	devm_can_led_init(ndev);
> +
> +	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%u)\n",
> +		 priv->regs, ndev->irq);
> +
> +	return 0;
> +fail_candev:
> +	netif_napi_del(&priv->napi);
> +fail_clk:
> +	free_candev(ndev);
> +fail:
> +	return err;
> +}
> +
> +static int rcar_can_remove(struct platform_device *pdev)
> +{
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +
> +	unregister_candev(ndev);
> +	netif_napi_del(&priv->napi);
> +	free_candev(ndev);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rcar_can_suspend(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	u16 ctlr;
> +
> +	if (netif_running(ndev)) {
> +		netif_stop_queue(ndev);
> +		netif_device_detach(ndev);
> +	}
> +	ctlr = readw(&priv->regs->ctlr);
> +	ctlr |= RCAR_CAN_CTLR_CANM_HALT;
> +	writew(ctlr, &priv->regs->ctlr);
> +	ctlr |= RCAR_CAN_CTLR_SLPM;
> +	writew(ctlr, &priv->regs->ctlr);
> +	priv->can.state = CAN_STATE_SLEEPING;
> +
> +	clk_disable(priv->clk);
> +	return 0;
> +}
> +
> +static int rcar_can_resume(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct rcar_can_priv *priv = netdev_priv(ndev);
> +	u16 ctlr;
> +
> +	clk_enable(priv->clk);
> +
> +	ctlr = readw(&priv->regs->ctlr);
> +	ctlr &= ~RCAR_CAN_CTLR_SLPM;
> +	writew(ctlr, &priv->regs->ctlr);
> +	ctlr &= ~RCAR_CAN_CTLR_CANM;
> +	writew(ctlr, &priv->regs->ctlr);
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +	if (netif_running(ndev)) {
> +		netif_device_attach(ndev);
> +		netif_start_queue(ndev);
> +	}
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
> +
> +static struct platform_driver rcar_can_driver = {
> +	.driver = {
> +		.name = RCAR_CAN_DRV_NAME,
> +		.owner = THIS_MODULE,
> +		.pm = &rcar_can_pm_ops,
> +	},
> +	.probe = rcar_can_probe,
> +	.remove = rcar_can_remove,
> +};
> +
> +module_platform_driver(rcar_can_driver);
> +
> +MODULE_AUTHOR("Cogent Embedded, Inc.");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CAN driver for Renesas R-Car SoC");
> +MODULE_ALIAS("platform:" RCAR_CAN_DRV_NAME);
> Index: linux-can-next/include/linux/can/platform/rcar_can.h
> ===================================================================
> --- /dev/null
> +++ linux-can-next/include/linux/can/platform/rcar_can.h
> @@ -0,0 +1,15 @@
> +#ifndef _CAN_PLATFORM_RCAR_CAN_H_
> +#define _CAN_PLATFORM_RCAR_CAN_H_
> +
> +#include <linux/types.h>
> +
> +/* Clock Select Register settings */
> +#define CLKR_CLKEXT	3	/* Externally input clock */
> +#define CLKR_CLKP2	1	/* Peripheral clock (clkp2) */
> +#define CLKR_CLKP1	0	/* Peripheral clock (clkp1) */

Please make it an enum

> +
> +struct rcar_can_platform_data {
> +	u8 clock_select;	/* Clock source select */
> +};
> +
> +#endif	/* !_CAN_PLATFORM_RCAR_CAN_H_ */
> 

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

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

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

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

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

[...]
>> 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 @@
[...]
>> +/* 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;

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

    Unfortunately, we don't.

> 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.

[...]
>> +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);

> 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.

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

> the cast is not needed

    Removed.

[...]
>> +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 */

> 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.

>> +static void rcar_can_start(struct net_device *ndev)
>> +{
>> +	struct rcar_can_priv *priv = netdev_priv(ndev);
>> +	u16 ctlr, str;
>> +
>> +	/* Set controller to known mode:
>> +	 * - FIFO mailbox mode
>> +	 * - accept all messages
>> +	 * - overrun mode
>> +	 * CAN is in sleep mode after MCU hardware or software reset.
>> +	 */
>> +	ctlr = readw(&priv->regs->ctlr);
>> +	ctlr &= ~RCAR_CAN_CTLR_SLPM;
>> +	writew(ctlr, &priv->regs->ctlr);
>> +	/* Go to reset mode */
>> +	ctlr |= RCAR_CAN_CTLR_CANM_FORCE_RESET;
>> +	writew(ctlr, &priv->regs->ctlr);
>> +	do {
>> +		str = readw(&priv->regs->str);
>> +	} while (!(str & RCAR_CAN_STR_RSTST));

> Please add a timeout for this loop and the loop below.

    Added a counter, converted the loop to *for*.

>> +static int rcar_can_open(struct net_device *ndev)
>> +{
>> +	struct rcar_can_priv *priv = netdev_priv(ndev);
>> +	int err;
>> +
>> +	clk_prepare_enable(priv->clk);

> clk_prepare_enable can fail

    Added check.

>> +	err = open_candev(ndev);
>> +	if (err) {
>> +		netdev_err(ndev, "open_candev() failed %d\n", err);
>> +		goto out;

> please adjust the jump label, you have to disable the clock.

    Fixed.

[...]
>> +static void rcar_can_stop(struct net_device *ndev)
>> +{
>> +	struct rcar_can_priv *priv = netdev_priv(ndev);
>> +	u16 ctlr, str;
>> +
>> +	/* Go to (force) reset mode */
>> +	ctlr = readw(&priv->regs->ctlr);
>> +	ctlr |= RCAR_CAN_CTLR_CANM_FORCE_RESET;
>> +	writew(ctlr, &priv->regs->ctlr);
>> +	do {
>> +		str = readw(&priv->regs->str);
>> +	} while (!(str & RCAR_CAN_STR_RSTST));

> please add a timeout to the loop

    Added a counter and converted to *for*.

[...]
>> +static netdev_tx_t rcar_can_start_xmit(struct sk_buff *skb,
>> +				       struct net_device *ndev)
>> +{
>> +	struct rcar_can_priv *priv = netdev_priv(ndev);
>> +	struct can_frame *cf = (struct can_frame *)skb->data;
>> +	u32 data, i;
>> +	unsigned long flags;
>> +	u8 tfcr;
>> +
>> +	if (can_dropped_invalid_skb(ndev, skb))
>> +		return NETDEV_TX_OK;
>> +	tfcr = readb(&priv->regs->tfcr);
>> +	if ((tfcr & RCAR_CAN_TFCR_TFUST) >> RCAR_CAN_TFCR_TFUST_SHIFT > 2)
>> +		netif_stop_queue(ndev);

> Can you explain what's checked here?

    if (<Number of unsent massages in Transmit FIFO> > 2)

FIFO depth = 4.

    Added a comment. Changed to >= 3.

>> +
>> +	if (cf->can_id & CAN_EFF_FLAG) {
>> +		/* Extended frame format */
>> +		data = (cf->can_id & CAN_EFF_MASK) | RCAR_CAN_IDE;
>> +	} else {
>> +		/* Standard frame format */
>> +		data = (cf->can_id & CAN_SFF_MASK) << RCAR_CAN_SID_SHIFT;
>> +	}
>> +	if (cf->can_id & CAN_RTR_FLAG) {
>> +		/* Remote transmission request */
>> +		data |= RCAR_CAN_RTR;
>> +	}

> You can move the comments into the line of if and else and remove the {
> & } as there is only one line after if and else.

    Done.

>> +	writel(data, &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].id);
>> +
>> +	writeb(cf->can_dlc, &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].dlc);
>> +
>> +	for (i = 0; i < cf->can_dlc; i++)
>> +		writeb(cf->data[i],
>> +		       &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].data[i]);
>> +
>> +	spin_lock_irqsave(&priv->skb_lock, flags);
>> +	can_put_echo_skb(skb, ndev, priv->frames_queued++);
>> +	priv->bytes_queued += cf->can_dlc;

> How does the frames_queued and bytes_queued mechanism work?

    Explained above, we get TX interrupt only after all queued packets are sent.

>> +	spin_unlock_irqrestore(&priv->skb_lock, flags);
>> +	/* Start Tx: write 0xFF to the TFPCR register to increment
>> +	 * the CPU-side pointer for the transmit FIFO to the next
>> +	 * mailbox location
>> +	 */
>> +	writeb(0xFF, &priv->regs->tfpcr);

> please use lowercase for hex.

    Done here ind in comment above.

>> +
>> +	return NETDEV_TX_OK;

> I'm missing flow control here. You have to stop the queue if there isn't
> any room in the tx fifo.

    You've seen it above.

[...]
>> +static int rcar_can_rx_poll(struct napi_struct *napi, int quota)
>> +{
>> +	struct rcar_can_priv *priv = container_of(napi,
>> +						  struct rcar_can_priv, napi);
>> +	int num_pkts = 0;
>> +
>> +	while (num_pkts < quota) {
>> +		u8 i, rfcr, nframes, isr;
>> +
>> +		isr = readb(&priv->regs->isr);
>> +		/* Clear interrupt bit */
>> +		if (isr & RCAR_CAN_ISR_RXFF)
>> +			writeb(isr & ~RCAR_CAN_ISR_RXFF, &priv->regs->isr);
>> +		rfcr = readb(&priv->regs->rfcr);
>> +		if (rfcr & RCAR_CAN_RFCR_RFEST)
>> +			break;
>> +		nframes = (rfcr & RCAR_CAN_RFCR_RFUST) >>
>> +			  RCAR_CAN_RFCR_RFUST_SHIFT;
>> +		for (i = 0; i < nframes; i++) {
>> +			rcar_can_rx_pkt(priv);
>> +			/* Write 0xFF to the RFPCR register to increment
>> +			 * the CPU-side pointer for the receive FIFO
>> +			 * to the next mailbox location
>> +			 */
>> +			writeb(0xFF, &priv->regs->rfpcr);
>> +			++num_pkts;
>> +		}

> The for loop inside the while loop makes no sense if you increment
> num_pkts. You are not allowed to receive more than quota CAN frames.

    Removed the *for* loop. Stupid me. :-)

>> +static int rcar_can_get_berr_counter(const struct net_device *dev,
>> +				     struct can_berr_counter *bec)
>> +{
>> +	struct rcar_can_priv *priv = netdev_priv(dev);
>> +
>> +	clk_prepare_enable(priv->clk);

> clk_prepare_enable can fail

    Fixed.

[...]
>> +static int rcar_can_resume(struct device *dev)
>> +{
>> +	struct net_device *ndev = dev_get_drvdata(dev);
>> +	struct rcar_can_priv *priv = netdev_priv(ndev);
>> +	u16 ctlr;
>> +
>> +	clk_enable(priv->clk);

    Added error check.

[...]
>> Index: linux-can-next/include/linux/can/platform/rcar_can.h
>> =================================>> --- /dev/null
>> +++ linux-can-next/include/linux/can/platform/rcar_can.h
>> @@ -0,0 +1,15 @@
>> +#ifndef _CAN_PLATFORM_RCAR_CAN_H_
>> +#define _CAN_PLATFORM_RCAR_CAN_H_
>> +
>> +#include <linux/types.h>
>> +
>> +/* Clock Select Register settings */
>> +#define CLKR_CLKEXT	3	/* Externally input clock */
>> +#define CLKR_CLKP2	1	/* Peripheral clock (clkp2) */
>> +#define CLKR_CLKP1	0	/* Peripheral clock (clkp1) */

> Please make it an enum

    Done.

WBR, Sergei

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3825 bytes --]

On 01/25/2014 02:34 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 01/20/2014 12:18 PM, Marc Kleine-Budde wrote:
> 
>>> Add support for the CAN controller found in Renesas R-Car SoCs.
> 
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
>>> ---
>>> The patch is against the 'linux-can-next.git' repo.
> 
> [...]
>>> 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 @@
> [...]
>>> +/* 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;
> 
>> If you have contact to the hardware designer please blame him for
> 
>    Unfortunately, we don't.
> 
>> 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.
> 
> [...]
>>> +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);
> 
>> 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.

>>> +static irqreturn_t rcar_can_interrupt(int irq, void *dev_id)
>>> +{
>>> +    struct net_device *ndev = (struct net_device *)dev_id;
> 
>> the cast is not needed
> 
>    Removed.
> 
> [...]
>>> +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 */
> 
>> 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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

On 02/13/2014 03:12 PM, Marc Kleine-Budde wrote:

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

>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

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

>> [...]
>>>> 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 @@
>> [...]
>>>> +/* 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;

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

>>     Unfortunately, we don't.

>>> 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.

    Moreover, there are certainly strange issues with the host bus.

>> [...]
>>>> +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);

>>> 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?

    Yes, there's a mode where TX interrupt signals send completion. I rewrote
the driver to make use of this mode now.

> If you only have TX FIFO
> empty, you have to limit the FIFO depth to 1.

    Not quite clear why...

[...]
>>>> +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 */

>>> 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.

    IMO, reading 32-bits is futile, as we're going to completely overwrite 
those 24 bits that constitute BCR. So I kept the 8-bit CLKR read but removed 
the CLKR write in the end. I've also added a comment clarifying why CLKR is 
positioned in the LSBs of 32-bit word (while it's address would assume MSBs).
The host bus is big-endian but byte-swaps at least 16- and 32-bit accesses, so 
that read[wl]()/write[wl]() work. 8-bit accesses are not byte swapped, despite 
what the figure in the manual shows.

> Marc

WBR, Sergei

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On 02/21/2014 12:48 AM, Sergei Shtylyov wrote:

>>> 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.
> 
>    IMO, reading 32-bits is futile, as we're going to completely
> overwrite those 24 bits that constitute BCR. So I kept the 8-bit CLKR
> read but removed the CLKR write in the end. I've also added a comment
> clarifying why CLKR is positioned in the LSBs of 32-bit word (while it's
> address would assume MSBs).
> The host bus is big-endian but byte-swaps at least 16- and 32-bit
> accesses, so that read[wl]()/write[wl]() work. 8-bit accesses are not
> byte swapped, despite what the figure in the manual shows.

A 32 bit read/modify/write is a standard operation, nothing special, no
need to worry about byte swapping or anything like this.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

On 28-02-2014 13:08, Marc Kleine-Budde wrote:

>>>> 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.

>>     IMO, reading 32-bits is futile, as we're going to completely
>> overwrite those 24 bits that constitute BCR. So I kept the 8-bit CLKR
>> read but removed the CLKR write in the end. I've also added a comment
>> clarifying why CLKR is positioned in the LSBs of 32-bit word (while it's
>> address would assume MSBs).
>> The host bus is big-endian but byte-swaps at least 16- and 32-bit
>> accesses, so that read[wl]()/write[wl]() work. 8-bit accesses are not
>> byte swapped, despite what the figure in the manual shows.

> A 32 bit read/modify/write is a standard operation, nothing special, no
> need to worry about byte swapping or anything like this.

    Oh, really? 8-)
    Don't you know that read[bwlq]() assume little-endian memory layout and to 
read from big-endian 32-bit register one normally needs readl_be()?

> Marc

WBR, Sergei

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On 02/28/2014 12:16 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 28-02-2014 13:08, Marc Kleine-Budde wrote:
> 
>>>>> 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.
> 
>>>     IMO, reading 32-bits is futile, as we're going to completely
>>> overwrite those 24 bits that constitute BCR. So I kept the 8-bit CLKR
>>> read but removed the CLKR write in the end. I've also added a comment
>>> clarifying why CLKR is positioned in the LSBs of 32-bit word (while it's
>>> address would assume MSBs).
>>> The host bus is big-endian but byte-swaps at least 16- and 32-bit
>>> accesses, so that read[wl]()/write[wl]() work. 8-bit accesses are not
>>> byte swapped, despite what the figure in the manual shows.
> 
>> A 32 bit read/modify/write is a standard operation, nothing special, no
>> need to worry about byte swapping or anything like this.
> 
>    Oh, really? 8-)
>    Don't you know that read[bwlq]() assume little-endian memory layout
> and to read from big-endian 32-bit register one normally needs readl_be()?

I assume you are on little endian ARM only (for now).

If you use a standard 32 bit read, then modify the correct bits in that
32 bit word and write it back, with the corresponding 32 bit write
everything should be fine. For this usecase you just have yo figure out
which 24 of the 32 bit are the one you have to change and which are the
8 that must not be modified.

Looking at the register layout:

> +	u8 bcr[3];	/* Bit Configuration Register */
> +	u8 clkr;	/* Clock Select Register */

I think clkr would be the lowest 8 bit and bcr[] are the upper 24.
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Geert Uytterhoeven]

On Fri, Feb 28, 2014 at 12:37 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>> A 32 bit read/modify/write is a standard operation, nothing special, no
>>> need to worry about byte swapping or anything like this.
>>
>>    Oh, really? 8-)
>>    Don't you know that read[bwlq]() assume little-endian memory layout
>> and to read from big-endian 32-bit register one normally needs readl_be()?
>
> I assume you are on little endian ARM only (for now).
>
> If you use a standard 32 bit read, then modify the correct bits in that
> 32 bit word and write it back, with the corresponding 32 bit write
> everything should be fine. For this usecase you just have yo figure out
> which 24 of the 32 bit are the one you have to change and which are the
> 8 that must not be modified.
>
> Looking at the register layout:
>
>> +     u8 bcr[3];      /* Bit Configuration Register */
>> +     u8 clkr;        /* Clock Select Register */
>
> I think clkr would be the lowest 8 bit and bcr[] are the upper 24.

That would be the outcome on big endian ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v5] can: add Renesas R-Car CAN driver

[David Laight]

RnJvbTogR2VlcnQgVXl0dGVyaG9ldmVuDQo+IE9uIEZyaSwgRmViIDI4LCAyMDE0IGF0IDEyOjM3
IFBNLCBNYXJjIEtsZWluZS1CdWRkZSA8bWtsQHBlbmd1dHJvbml4LmRlPiB3cm90ZToNCj4gPj4+
IEEgMzIgYml0IHJlYWQvbW9kaWZ5L3dyaXRlIGlzIGEgc3RhbmRhcmQgb3BlcmF0aW9uLCBub3Ro
aW5nIHNwZWNpYWwsIG5vDQo+ID4+PiBuZWVkIHRvIHdvcnJ5IGFib3V0IGJ5dGUgc3dhcHBpbmcg
b3IgYW55dGhpbmcgbGlrZSB0aGlzLg0KPiA+Pg0KPiA+PiAgICBPaCwgcmVhbGx5PyA4LSkNCj4g
Pj4gICAgRG9uJ3QgeW91IGtub3cgdGhhdCByZWFkW2J3bHFdKCkgYXNzdW1lIGxpdHRsZS1lbmRp
YW4gbWVtb3J5IGxheW91dA0KPiA+PiBhbmQgdG8gcmVhZCBmcm9tIGJpZy1lbmRpYW4gMzItYml0
IHJlZ2lzdGVyIG9uZSBub3JtYWxseSBuZWVkcyByZWFkbF9iZSgpPw0KPiA+DQo+ID4gSSBhc3N1
bWUgeW91IGFyZSBvbiBsaXR0bGUgZW5kaWFuIEFSTSBvbmx5IChmb3Igbm93KS4NCj4gPg0KPiA+
IElmIHlvdSB1c2UgYSBzdGFuZGFyZCAzMiBiaXQgcmVhZCwgdGhlbiBtb2RpZnkgdGhlIGNvcnJl
Y3QgYml0cyBpbiB0aGF0DQo+ID4gMzIgYml0IHdvcmQgYW5kIHdyaXRlIGl0IGJhY2ssIHdpdGgg
dGhlIGNvcnJlc3BvbmRpbmcgMzIgYml0IHdyaXRlDQo+ID4gZXZlcnl0aGluZyBzaG91bGQgYmUg
ZmluZS4gRm9yIHRoaXMgdXNlY2FzZSB5b3UganVzdCBoYXZlIHlvIGZpZ3VyZSBvdXQNCj4gPiB3
aGljaCAyNCBvZiB0aGUgMzIgYml0IGFyZSB0aGUgb25lIHlvdSBoYXZlIHRvIGNoYW5nZSBhbmQg
d2hpY2ggYXJlIHRoZQ0KPiA+IDggdGhhdCBtdXN0IG5vdCBiZSBtb2RpZmllZC4NCj4gPg0KPiA+
IExvb2tpbmcgYXQgdGhlIHJlZ2lzdGVyIGxheW91dDoNCj4gPg0KPiA+PiArICAgICB1OCBiY3Jb
M107ICAgICAgLyogQml0IENvbmZpZ3VyYXRpb24gUmVnaXN0ZXIgKi8NCj4gPj4gKyAgICAgdTgg
Y2xrcjsgICAgICAgIC8qIENsb2NrIFNlbGVjdCBSZWdpc3RlciAqLw0KPiA+DQo+ID4gSSB0aGlu
ayBjbGtyIHdvdWxkIGJlIHRoZSBsb3dlc3QgOCBiaXQgYW5kIGJjcltdIGFyZSB0aGUgdXBwZXIg
MjQuDQo+IA0KPiBUaGF0IHdvdWxkIGJlIHRoZSBvdXRjb21lIG9uIGJpZyBlbmRpYW4gOy0pDQoN
Ckxvb2tzIHRvIG1lIGFzIHRob3VnaCBpdCBzaG91bGQgYmUgZGVmaW5lZCBhcyBhIDMyYml0IGZp
ZWxkIGFuZCB0aGVuDQp0aGUgYXBwcm9wcmlhdGUgYml0IGRlZmluaXRpb25zIGFuZCBtYXNrcyBh
cHBsaWVkLg0KDQoJRGF2aWQNCg0K

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

On 02/28/2014 12:47 PM, David Laight wrote:
> From: Geert Uytterhoeven
>> On Fri, Feb 28, 2014 at 12:37 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>>> A 32 bit read/modify/write is a standard operation, nothing special, no
>>>>> need to worry about byte swapping or anything like this.
>>>>
>>>>    Oh, really? 8-)
>>>>    Don't you know that read[bwlq]() assume little-endian memory layout
>>>> and to read from big-endian 32-bit register one normally needs readl_be()?
>>>
>>> I assume you are on little endian ARM only (for now).
>>>
>>> If you use a standard 32 bit read, then modify the correct bits in that
>>> 32 bit word and write it back, with the corresponding 32 bit write
>>> everything should be fine. For this usecase you just have yo figure out
>>> which 24 of the 32 bit are the one you have to change and which are the
>>> 8 that must not be modified.
>>>
>>> Looking at the register layout:
>>>
>>>> +     u8 bcr[3];      /* Bit Configuration Register */
>>>> +     u8 clkr;        /* Clock Select Register */
>>>
>>> I think clkr would be the lowest 8 bit and bcr[] are the upper 24.
>>
>> That would be the outcome on big endian ;-)
> 
> Looks to me as though it should be defined as a 32bit field and then
> the appropriate bit definitions and masks applied.

Ack, 32bit yes, but no field (as in http://en.wikipedia.org/wiki/Bit_field).

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

RE: [PATCH v5] can: add Renesas R-Car CAN driver

[David Laight]

RnJvbTogTWFyYyBLbGVpbmUtQnVkZGUNCj4gT24gMDIvMjgvMjAxNCAxMjo0NyBQTSwgRGF2aWQg
TGFpZ2h0IHdyb3RlOg0KPiA+IEZyb206IEdlZXJ0IFV5dHRlcmhvZXZlbg0KPiA+PiBPbiBGcmks
IEZlYiAyOCwgMjAxNCBhdCAxMjozNyBQTSwgTWFyYyBLbGVpbmUtQnVkZGUgPG1rbEBwZW5ndXRy
b25peC5kZT4gd3JvdGU6DQo+ID4+Pj4+IEEgMzIgYml0IHJlYWQvbW9kaWZ5L3dyaXRlIGlzIGEg
c3RhbmRhcmQgb3BlcmF0aW9uLCBub3RoaW5nIHNwZWNpYWwsIG5vDQo+ID4+Pj4+IG5lZWQgdG8g
d29ycnkgYWJvdXQgYnl0ZSBzd2FwcGluZyBvciBhbnl0aGluZyBsaWtlIHRoaXMuDQo+ID4+Pj4N
Cj4gPj4+PiAgICBPaCwgcmVhbGx5PyA4LSkNCj4gPj4+PiAgICBEb24ndCB5b3Uga25vdyB0aGF0
IHJlYWRbYndscV0oKSBhc3N1bWUgbGl0dGxlLWVuZGlhbiBtZW1vcnkgbGF5b3V0DQo+ID4+Pj4g
YW5kIHRvIHJlYWQgZnJvbSBiaWctZW5kaWFuIDMyLWJpdCByZWdpc3RlciBvbmUgbm9ybWFsbHkg
bmVlZHMgcmVhZGxfYmUoKT8NCj4gPj4+DQo+ID4+PiBJIGFzc3VtZSB5b3UgYXJlIG9uIGxpdHRs
ZSBlbmRpYW4gQVJNIG9ubHkgKGZvciBub3cpLg0KPiA+Pj4NCj4gPj4+IElmIHlvdSB1c2UgYSBz
dGFuZGFyZCAzMiBiaXQgcmVhZCwgdGhlbiBtb2RpZnkgdGhlIGNvcnJlY3QgYml0cyBpbiB0aGF0
DQo+ID4+PiAzMiBiaXQgd29yZCBhbmQgd3JpdGUgaXQgYmFjaywgd2l0aCB0aGUgY29ycmVzcG9u
ZGluZyAzMiBiaXQgd3JpdGUNCj4gPj4+IGV2ZXJ5dGhpbmcgc2hvdWxkIGJlIGZpbmUuIEZvciB0
aGlzIHVzZWNhc2UgeW91IGp1c3QgaGF2ZSB5byBmaWd1cmUgb3V0DQo+ID4+PiB3aGljaCAyNCBv
ZiB0aGUgMzIgYml0IGFyZSB0aGUgb25lIHlvdSBoYXZlIHRvIGNoYW5nZSBhbmQgd2hpY2ggYXJl
IHRoZQ0KPiA+Pj4gOCB0aGF0IG11c3Qgbm90IGJlIG1vZGlmaWVkLg0KPiA+Pj4NCj4gPj4+IExv
b2tpbmcgYXQgdGhlIHJlZ2lzdGVyIGxheW91dDoNCj4gPj4+DQo+ID4+Pj4gKyAgICAgdTggYmNy
WzNdOyAgICAgIC8qIEJpdCBDb25maWd1cmF0aW9uIFJlZ2lzdGVyICovDQo+ID4+Pj4gKyAgICAg
dTggY2xrcjsgICAgICAgIC8qIENsb2NrIFNlbGVjdCBSZWdpc3RlciAqLw0KPiA+Pj4NCj4gPj4+
IEkgdGhpbmsgY2xrciB3b3VsZCBiZSB0aGUgbG93ZXN0IDggYml0IGFuZCBiY3JbXSBhcmUgdGhl
IHVwcGVyIDI0Lg0KPiA+Pg0KPiA+PiBUaGF0IHdvdWxkIGJlIHRoZSBvdXRjb21lIG9uIGJpZyBl
bmRpYW4gOy0pDQo+ID4NCj4gPiBMb29rcyB0byBtZSBhcyB0aG91Z2ggaXQgc2hvdWxkIGJlIGRl
ZmluZWQgYXMgYSAzMmJpdCBmaWVsZCBhbmQgdGhlbg0KPiA+IHRoZSBhcHByb3ByaWF0ZSBiaXQg
ZGVmaW5pdGlvbnMgYW5kIG1hc2tzIGFwcGxpZWQuDQo+IA0KPiBBY2ssIDMyYml0IHllcywgYnV0
IG5vIGZpZWxkIChhcyBpbiBodHRwOi8vZW4ud2lraXBlZGlhLm9yZy93aWtpL0JpdF9maWVsZCku
DQoNClllcywgSSBtZWFudCAnZmllbGQnIGFzIGluICdzdHJ1Y3R1cmUgbWVtYmVyJywgbm90IGJp
dC1maWVsZC4NCkkgbGVhcm50IGEgbG9uZyB0aW1lIGFnbyB0aGF0IGJpdC1maWVsZHMgYXJlIHJh
cmVseSwgaWYgZXZlciwgdGhlIGNvbnN0cnVjdA0KeW91IGFyZSBsb29raW5nIGZvci4NCjEpIEEg
cGFja2VkIGxvb2t1cCB0YWJsZSB3aGVyZSB0aGUgY29kZSBmb3Igb25lIGFjY2VzcyB3YXMgbGFy
Z2VyIHRoYW4gdGhlIHRhYmxlLg0KMikgZm9vLT50d29fYml0X2ZpZWxkID0gMzsgY29tcGlsZWQg
aW50byBjb2RlIHRoYXQgZmlyc3QgemVyb2VkIHRoZSB0d28gYml0cw0KICAgKGNhdXNlZCBhbiBJ
U1IgdG8gY29ycnVwdCBhIGxpbmtlZCBsaXN0KS4NCk5ldmVyIG1pbmQgdGhlIGlzc3VlcyBhYm91
dCB0aGUgYWN0dWFsIG9yZGVyIG9mIHRoZSBiaXRzLg0KDQoJRGF2aWQNCg0K

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

On 02/28/2014 12:41 PM, Geert Uytterhoeven wrote:
> On Fri, Feb 28, 2014 at 12:37 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>> A 32 bit read/modify/write is a standard operation, nothing special, no
>>>> need to worry about byte swapping or anything like this.
>>>
>>>    Oh, really? 8-)
>>>    Don't you know that read[bwlq]() assume little-endian memory layout
>>> and to read from big-endian 32-bit register one normally needs readl_be()?
>>
>> I assume you are on little endian ARM only (for now).
>>
>> If you use a standard 32 bit read, then modify the correct bits in that
>> 32 bit word and write it back, with the corresponding 32 bit write
>> everything should be fine. For this usecase you just have yo figure out
>> which 24 of the 32 bit are the one you have to change and which are the
>> 8 that must not be modified.
>>
>> Looking at the register layout:
>>
>>> +     u8 bcr[3];      /* Bit Configuration Register */
>>> +     u8 clkr;        /* Clock Select Register */
>>
>> I think clkr would be the lowest 8 bit and bcr[] are the upper 24.
> 
> That would be the outcome on big endian ;-)

Doh! Yes, correct.
The point is, just read/modify the correct bits/write, should just work.
The driver has to be tested on BE ARM anyways, as this isn't the only 32
bit reg.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

On 28-02-2014 15:49, Marc Kleine-Budde wrote:

>>>>> A 32 bit read/modify/write is a standard operation, nothing special, no
>>>>> need to worry about byte swapping or anything like this.

>>>>     Oh, really? 8-)
>>>>     Don't you know that read[bwlq]() assume little-endian memory layout
>>>> and to read from big-endian 32-bit register one normally needs readl_be()?

>>> I assume you are on little endian ARM only (for now).

    That doesn't matter but yes.

>>> If you use a standard 32 bit read, then modify the correct bits in that
>>> 32 bit word and write it back, with the corresponding 32 bit write
>>> everything should be fine. For this usecase you just have yo figure out
>>> which 24 of the 32 bit are the one you have to change and which are the
>>> 8 that must not be modified.

    It seems you can't figure that out yourself. :-)

>>> Looking at the register layout:

>>>> +     u8 bcr[3];      /* Bit Configuration Register */
>>>> +     u8 clkr;        /* Clock Select Register */

>>> I think clkr would be the lowest 8 bit and bcr[] are the upper 24.

>> That would be the outcome on big endian ;-)

> Doh! Yes, correct.
> The point is, just read/modify the correct bits/write, should just work.

    That's what I do. But I completely fail to see the point of reading BCR 
which is completely overwritten. I would like to consider the pointless 
discussion about 32-bit read complete now.

> The driver has to be tested on BE ARM anyways, as this isn't the only 32
> bit reg.

    This is not a 32-bit register but 24- and 8-bit one. I'm afraid we won't 
be able to test on BE soon as the machine we're debugging on is LE only. 
Anyway, for the big-endian configured Superhyway bus the big-endian HPB bus 
the CAN controller resides on shouldn't swap bytes.

> Marc

WBR, Sergei

RE: [PATCH v5] can: add Renesas R-Car CAN driver

[David Laight]

RnJvbTogU2VyZ2VpIFNodHlseW92DQo+ICAgICBUaGlzIGlzIG5vdCBhIDMyLWJpdCByZWdpc3Rl
ciBidXQgMjQtIGFuZCA4LWJpdCBvbmUuIEknbSBhZnJhaWQgd2Ugd29uJ3QNCj4gYmUgYWJsZSB0
byB0ZXN0IG9uIEJFIHNvb24gYXMgdGhlIG1hY2hpbmUgd2UncmUgZGVidWdnaW5nIG9uIGlzIExF
IG9ubHkuDQo+IEFueXdheSwgZm9yIHRoZSBiaWctZW5kaWFuIGNvbmZpZ3VyZWQgU3VwZXJoeXdh
eSBidXMgdGhlIGJpZy1lbmRpYW4gSFBCIGJ1cw0KPiB0aGUgQ0FOIGNvbnRyb2xsZXIgcmVzaWRl
cyBvbiBzaG91bGRuJ3Qgc3dhcCBieXRlcy4NCg0KRXhjZXB0IHRoYXQgeW91IGFyZW4ndCBnb2lu
ZyB0byBnZXQgYW55IGNwdSBJIGtub3cgYWJvdXQgdG8gZG8gYSBidXMNCmFjY2VzcyB3aXRoIG9u
bHkgdGhyZWUgb2YgdGhlIGZvdXIgYnl0ZSBlbmFibGVzIGFzc2VydGVkIChleGNlcHQgYXMgcGFy
dA0Kb2YgYSBtaXNhbGlnbmVkIDMyIGJpdCB0cmFuc2ZlcikuDQoNClNvIHlvdSBlaXRoZXIgaGF2
ZSBmb3VyIDgtYml0IHJlZ2lzdGVycywgb3Igb25lIDMyLWJpdCBvbmUuDQoNCkknZCBzdWdnZXN0
IGtlZXBpbmcgYm90aCBwYXJ0cyBpbiB0aGUgZHJpdmVyIGRhdGEgYXJlYSBhbmQgb3JpbmcNCnRo
ZW0gdG9nZXRoZXIgYmVmb3JlIHdyaXRpbmcgdG8gdGhlIGRldmljZS4NCg0KCURhdmlkDQoNCg=

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

On 28-02-2014 16:17, David Laight wrote:

>>      This is not a 32-bit register but 24- and 8-bit one. I'm afraid we won't
>> be able to test on BE soon as the machine we're debugging on is LE only.
>> Anyway, for the big-endian configured Superhyway bus the big-endian HPB bus
>> the CAN controller resides on shouldn't swap bytes.

> Except that you aren't going to get any cpu I know about to do a bus
> access with only three of the four byte enables asserted (except as part
> of a misaligned 32 bit transfer).

> So you either have four 8-bit registers, or one 32-bit one.

    I have what is described in the manual, 24-bit register allowing 8-, 16-, 
and 32-bit access.

> I'd suggest keeping both parts in the driver data area

    Well, I'm keeping CLKR value in the platform data. But there is absolutely 
no need to keep BCR anywhere because I'd never use the saved value.

> and oring them together before writing to the device.

    That's what I'm doing.

> 	David

WBR, Sergei

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Geert Uytterhoeven]

Hi Sergei,

On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Changes in version 3:

> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
> - removed unneeded type cast in the probe() method.

> +/* 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;

Sorry, I missed the earlier discussion, but why the _packed?
One u32 and 12 bytes makes a nice multiple of 4.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
> Hi Sergei,
> 
> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
>> Changes in version 3:
> 
>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>> - removed unneeded type cast in the probe() method.
> 
>> +/* 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;
> 
> Sorry, I missed the earlier discussion, but why the _packed?
> One u32 and 12 bytes makes a nice multiple of 4.

Better safe than sorry, it's the layout of the registers in hardware,
don't let the compiler optimize here.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Geert Uytterhoeven]

On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>> <sergei.shtylyov@cogentembedded.com> wrote:
>>> Changes in version 3:
>>
>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>>> - removed unneeded type cast in the probe() method.
>>
>>> +/* 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;
>>
>> Sorry, I missed the earlier discussion, but why the _packed?
>> One u32 and 12 bytes makes a nice multiple of 4.
>
> Better safe than sorry, it's the layout of the registers in hardware,
> don't let the compiler optimize here.

Actually __packed makes it less safe, as the compiler now assumes
the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
byte accesses.

Fortunately it won't happen in this case as the code uses writel/readl to
acces the id member.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1925 bytes --]

On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>>> <sergei.shtylyov@cogentembedded.com> wrote:
>>>> Changes in version 3:
>>>
>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>>>> - removed unneeded type cast in the probe() method.
>>>
>>>> +/* 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;
>>>
>>> Sorry, I missed the earlier discussion, but why the _packed?
>>> One u32 and 12 bytes makes a nice multiple of 4.
>>
>> Better safe than sorry, it's the layout of the registers in hardware,
>> don't let the compiler optimize here.
> 
> Actually __packed makes it less safe, as the compiler now assumes
> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
> byte accesses.
> 
> Fortunately it won't happen in this case as the code uses writel/readl to
> acces the id member.

Yes, as this are registers they must not be accessed directly. However
we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
that the base address of this struct is always aligned to 4 bytes.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Ben Dooks]

On 20/01/14 11:58, Marc Kleine-Budde wrote:
> On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
>> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>>>> <sergei.shtylyov@cogentembedded.com> wrote:
>>>>> Changes in version 3:
>>>>
>>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>>>>> - removed unneeded type cast in the probe() method.
>>>>
>>>>> +/* 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;
>>>>
>>>> Sorry, I missed the earlier discussion, but why the _packed?
>>>> One u32 and 12 bytes makes a nice multiple of 4.
>>>
>>> Better safe than sorry, it's the layout of the registers in hardware,
>>> don't let the compiler optimize here.
>>
>> Actually __packed makes it less safe, as the compiler now assumes
>> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
>> byte accesses.
>>
>> Fortunately it won't happen in this case as the code uses writel/readl to
>> acces the id member.
>
> Yes, as this are registers they must not be accessed directly. However
> we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
> that the base address of this struct is always aligned to 4 bytes.

I thought we'd gotten past the stage of ever writing register
definitions as structures?

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Geert Uytterhoeven]

On Mon, Jan 20, 2014 at 1:02 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 20/01/14 11:58, Marc Kleine-Budde wrote:
>>
>> On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
>>>
>>> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de>
>>> wrote:
>>>>
>>>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>>>>>
>>>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>>>>> <sergei.shtylyov@cogentembedded.com> wrote:
>>>>>>
>>>>>> Changes in version 3:
>>>>>
>>>>>
>>>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct
>>>>>> rcar_can_regs';
>>>>>> - removed unneeded type cast in the probe() method.
>>>>>
>>>>>
>>>>>> +/* 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;
>>>>>
>>>>>
>>>>> Sorry, I missed the earlier discussion, but why the _packed?
>>>>> One u32 and 12 bytes makes a nice multiple of 4.
>>>>
>>>>
>>>> Better safe than sorry, it's the layout of the registers in hardware,
>>>> don't let the compiler optimize here.
>>>
>>>
>>> Actually __packed makes it less safe, as the compiler now assumes
>>> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
>>> byte accesses.
>>>
>>> Fortunately it won't happen in this case as the code uses writel/readl to
>>> acces the id member.
>>
>>
>> Yes, as this are registers they must not be accessed directly. However
>> we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
>> that the base address of this struct is always aligned to 4 bytes.
>
> I thought we'd gotten past the stage of ever writing register
> definitions as structures?

It allows to use e.g. "readl(&base->field)", which is used all over the place.

And sparse will inform you (read: the person who runs it) if you access
it directly.

Note that this driver does not use direct accesses.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2702 bytes --]

On 01/20/2014 01:05 PM, Geert Uytterhoeven wrote:
> On Mon, Jan 20, 2014 at 1:02 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> On 20/01/14 11:58, Marc Kleine-Budde wrote:
>>>
>>> On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
>>>>
>>>> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de>
>>>> wrote:
>>>>>
>>>>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>>>>>>
>>>>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>>>>>> <sergei.shtylyov@cogentembedded.com> wrote:
>>>>>>>
>>>>>>> Changes in version 3:
>>>>>>
>>>>>>
>>>>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct
>>>>>>> rcar_can_regs';
>>>>>>> - removed unneeded type cast in the probe() method.
>>>>>>
>>>>>>
>>>>>>> +/* 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;
>>>>>>
>>>>>>
>>>>>> Sorry, I missed the earlier discussion, but why the _packed?
>>>>>> One u32 and 12 bytes makes a nice multiple of 4.
>>>>>
>>>>>
>>>>> Better safe than sorry, it's the layout of the registers in hardware,
>>>>> don't let the compiler optimize here.
>>>>
>>>>
>>>> Actually __packed makes it less safe, as the compiler now assumes
>>>> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
>>>> byte accesses.
>>>>
>>>> Fortunately it won't happen in this case as the code uses writel/readl to
>>>> acces the id member.
>>>
>>>
>>> Yes, as this are registers they must not be accessed directly. However
>>> we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
>>> that the base address of this struct is always aligned to 4 bytes.
>>
>> I thought we'd gotten past the stage of ever writing register
>> definitions as structures?
> 
> It allows to use e.g. "readl(&base->field)", which is used all over the place.
> 
> And sparse will inform you (read: the person who runs it) if you access
> it directly.

I'm doing this (however just started using additional -D__CHECK_FOO___),
before applying patches to my tree.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

On 01/20/2014 01:02 PM, Ben Dooks wrote:
> On 20/01/14 11:58, Marc Kleine-Budde wrote:
>> On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde
>>> <mkl@pengutronix.de> wrote:
>>>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>>>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>>>>> <sergei.shtylyov@cogentembedded.com> wrote:
>>>>>> Changes in version 3:
>>>>>
>>>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct
>>>>>> rcar_can_regs';
>>>>>> - removed unneeded type cast in the probe() method.
>>>>>
>>>>>> +/* 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;
>>>>>
>>>>> Sorry, I missed the earlier discussion, but why the _packed?
>>>>> One u32 and 12 bytes makes a nice multiple of 4.
>>>>
>>>> Better safe than sorry, it's the layout of the registers in hardware,
>>>> don't let the compiler optimize here.
>>>
>>> Actually __packed makes it less safe, as the compiler now assumes
>>> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
>>> byte accesses.
>>>
>>> Fortunately it won't happen in this case as the code uses
>>> writel/readl to
>>> acces the id member.
>>
>> Yes, as this are registers they must not be accessed directly. However
>> we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
>> that the base address of this struct is always aligned to 4 bytes.
> 
> I thought we'd gotten past the stage of ever writing register
> definitions as structures?

I don't care whether to use offsets defined by a #define or an enum or a
struct. What's the "official" preference? Is it documented somewhere?

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

RE: [PATCH v5] can: add Renesas R-Car CAN driver

[David Laight]

RnJvbTogDQo+IE9uIDIwLzAxLzE0IDExOjU4LCBNYXJjIEtsZWluZS1CdWRkZSB3cm90ZToNCj4g
PiBPbiAwMS8yMC8yMDE0IDEyOjUyIFBNLCBHZWVydCBVeXR0ZXJob2V2ZW4gd3JvdGU6DQo+ID4+
IE9uIE1vbiwgSmFuIDIwLCAyMDE0IGF0IDEyOjQ3IFBNLCBNYXJjIEtsZWluZS1CdWRkZSA8bWts
QHBlbmd1dHJvbml4LmRlPiB3cm90ZToNCj4gPj4+IE9uIDAxLzIwLzIwMTQgMTI6NDMgUE0sIEdl
ZXJ0IFV5dHRlcmhvZXZlbiB3cm90ZToNCj4gPj4+PiBPbiBUaHUsIERlYyAyNiwgMjAxMyBhdCAx
MDozNyBQTSwgU2VyZ2VpIFNodHlseW92DQo+ID4+Pj4gPHNlcmdlaS5zaHR5bHlvdkBjb2dlbnRl
bWJlZGRlZC5jb20+IHdyb3RlOg0KPiA+Pj4+PiBDaGFuZ2VzIGluIHZlcnNpb24gMzoNCj4gPj4+
Pg0KPiA+Pj4+PiAtIGFkZGVkICdfX3BhY2tlZCcgdG8gJ3N0cnVjdCByY2FyX2Nhbl9tYm94X3Jl
Z3MnIGFuZCAnc3RydWN0IHJjYXJfY2FuX3JlZ3MnOw0KPiA+Pj4+PiAtIHJlbW92ZWQgdW5uZWVk
ZWQgdHlwZSBjYXN0IGluIHRoZSBwcm9iZSgpIG1ldGhvZC4NCj4gPj4+Pg0KPiA+Pj4+PiArLyog
TWFpbGJveCByZWdpc3RlcnMgc3RydWN0dXJlICovDQo+ID4+Pj4+ICtzdHJ1Y3QgcmNhcl9jYW5f
bWJveF9yZWdzIHsNCj4gPj4+Pj4gKyAgICAgICB1MzIgaWQ7ICAgICAgICAgLyogSURFIGFuZCBS
VFIgYml0cywgU0lEIGFuZCBFSUQgKi8NCj4gPj4+Pj4gKyAgICAgICB1OCBzdHViOyAgICAgICAg
LyogTm90IHVzZWQgKi8NCj4gPj4+Pj4gKyAgICAgICB1OCBkbGM7ICAgICAgICAgLyogRGF0YSBM
ZW5ndGggQ29kZSAtIGJpdHMgWzAuLjNdICovDQo+ID4+Pj4+ICsgICAgICAgdTggZGF0YVs4XTsg
ICAgIC8qIERhdGEgQnl0ZXMgKi8NCj4gPj4+Pj4gKyAgICAgICB1OCB0c2g7ICAgICAgICAgLyog
VGltZSBTdGFtcCBIaWdoZXIgQnl0ZSAqLw0KPiA+Pj4+PiArICAgICAgIHU4IHRzbDsgICAgICAg
ICAvKiBUaW1lIFN0YW1wIExvd2VyIEJ5dGUgKi8NCj4gPj4+Pj4gK30gX19wYWNrZWQ7DQo+ID4+
Pj4NCj4gPj4+PiBTb3JyeSwgSSBtaXNzZWQgdGhlIGVhcmxpZXIgZGlzY3Vzc2lvbiwgYnV0IHdo
eSB0aGUgX3BhY2tlZD8NCj4gPj4+PiBPbmUgdTMyIGFuZCAxMiBieXRlcyBtYWtlcyBhIG5pY2Ug
bXVsdGlwbGUgb2YgNC4NCj4gPj4+DQo+ID4+PiBCZXR0ZXIgc2FmZSB0aGFuIHNvcnJ5LCBpdCdz
IHRoZSBsYXlvdXQgb2YgdGhlIHJlZ2lzdGVycyBpbiBoYXJkd2FyZSwNCj4gPj4+IGRvbid0IGxl
dCB0aGUgY29tcGlsZXIgb3B0aW1pemUgaGVyZS4NCg0KV2h5IG5vdCBqdXN0IGFkZCBhIGNvbXBp
bGUgdGltZSBjaGVjayBhZ2FpbnN0IHRoZSBzaXplIG9mIHRoZQ0Kc3RydWN0dXJlIC0gdGhhdCB3
aWxsIGVuc3VyZSB0aGF0IG5vIHBhZGRpbmcgaXMgYWNjaWRlbnRhbGx5IGFkZGVkLg0KDQo+ID4+
IEFjdHVhbGx5IF9fcGFja2VkIG1ha2VzIGl0IGxlc3Mgc2FmZSwgYXMgdGhlIGNvbXBpbGVyIG5v
dyBhc3N1bWVzDQo+ID4+IHRoZSB1MzIgaWQgbWVtYmVyIGlzIHVuYWxpZ25lZCwgYW5kIHRodXMg
bWF5IHR1cm4gMzItYml0IGFjY2Vzc2VzIGludG8gNA0KPiA+PiBieXRlIGFjY2Vzc2VzLg0KPiA+
Pg0KPiA+PiBGb3J0dW5hdGVseSBpdCB3b24ndCBoYXBwZW4gaW4gdGhpcyBjYXNlIGFzIHRoZSBj
b2RlIHVzZXMgd3JpdGVsL3JlYWRsIHRvDQo+ID4+IGFjY2VzIHRoZSBpZCBtZW1iZXIuDQoNCldo
aWNoIG1lYW5zIHRoYXQgaXQgd2lsbCBiZSBhbGlnbmVkIChhbmQgbXVzdCBiZSBhbGlnbmVkKS4N
ClNvIHRoZSBwYWNrZWQgaXMgY29tcGxldGVseSB1c2VsZXNzIGFuZCBwb2ludGxlc3MuDQoNCj4g
PiBZZXMsIGFzIHRoaXMgYXJlIHJlZ2lzdGVycyB0aGV5IG11c3Qgbm90IGJlIGFjY2Vzc2VkIGRp
cmVjdGx5LiBIb3dldmVyDQo+ID4gd2UgY2FuIHVzZSAiX19hdHRyaWJ1dGVfXyAoKHBhY2tlZCwg
YWxpZ25lZCg0KSkpIiB0byB0ZWxsIHRoZSBjb21waWxlcg0KPiA+IHRoYXQgdGhlIGJhc2UgYWRk
cmVzcyBvZiB0aGlzIHN0cnVjdCBpcyBhbHdheXMgYWxpZ25lZCB0byA0IGJ5dGVzLg0KPiANCj4g
SSB0aG91Z2h0IHdlJ2QgZ290dGVuIHBhc3QgdGhlIHN0YWdlIG9mIGV2ZXIgd3JpdGluZyByZWdp
c3Rlcg0KPiBkZWZpbml0aW9ucyBhcyBzdHJ1Y3R1cmVzPw0KDQpBbnkgb3RoZXIgd2F5IGlzIGxp
a2VseSB0byBiZSBtb3JlIGVycm9yIHByb25lIHNpbmNlIG5vdGhpbmcNCnRpZXMgdGhlIG9mZnNl
dHMgd2l0aCB0aGUgY29ycmVjdCBiYXNlIGFkZHJlc3MuDQoNCklNSE8gaXQgaXMgYmVzdCB0byBv
bmx5IHNwZWNpZnkgcGFja2VkIGlmIHRoZSBzdHJ1Y3R1cmUgaXMgZXhwZWN0ZWQNCnRvIGJlIG1p
c2FsaWduZWQuIE1pc2FsaWduZWQgbWVtYmVycyBjYW4gYmUgJ2ZpeGVkJyBieSBtYXJraW5nIHRo
ZW0NCmFsaWduZWQoKS4NCg0KRXZlbiB0aGVuIHlvdSBtaWdodCB3YW50IHRvIGRlZmluZSB0aGUg
c3RydWN0dXJlIGl0c2VsZiB3aXRob3V0DQp0aGUgJ3BhY2tlZCcgYW5kIHRoZSBkZWZpbmUgYSBz
ZWNvbmQgcGFja2VkIHN0cnVjdHVyZSBjb250YWluaW5nDQphIHNpbmdsZSBtZW1iZXIuDQoNCmVn
OiB5b3UgbWlnaHQgaGF2ZToNCg0Kc3RydWN0IGEgew0KCXUzMglhXzE7DQoJdTY0CWFfMiBfX2F0
dHJpYnV0ZV9fKChhbGlnbmVkKDQpKSk7DQoJdTMyCWFfMzsNCn07DQoNCnN0cnVjdCBhX3BhY2tl
ZCB7DQoJc3RydWN0IGEgcDsNCn0gX19hdHRyaWJ1dGVfXygocGFja2VkKSk7DQoNCllvdSBtaWdo
dCBldmVuIG5lZWQgYSB1bmlvbiBvZiB0aGUgdHdvIHR5cGVzIQ0KDQoJRGF2aWQNCg0K

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

On 01/20/2014 01:13 PM, David Laight wrote:
> From: 
>> On 20/01/14 11:58, Marc Kleine-Budde wrote:
>>> On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
>>>> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>>>>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>>>>>> <sergei.shtylyov@cogentembedded.com> wrote:
>>>>>>> Changes in version 3:
>>>>>>
>>>>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>>>>>>> - removed unneeded type cast in the probe() method.
>>>>>>
>>>>>>> +/* 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;
>>>>>>
>>>>>> Sorry, I missed the earlier discussion, but why the _packed?
>>>>>> One u32 and 12 bytes makes a nice multiple of 4.
>>>>>
>>>>> Better safe than sorry, it's the layout of the registers in hardware,
>>>>> don't let the compiler optimize here.
> 
> Why not just add a compile time check against the size of the
> structure - that will ensure that no padding is accidentally added.

And what do when the check fails? Add an the __packed (again)? :D

>>>> Actually __packed makes it less safe, as the compiler now assumes
>>>> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
>>>> byte accesses.
>>>>
>>>> Fortunately it won't happen in this case as the code uses writel/readl to
>>>> acces the id member.
> 
> Which means that it will be aligned (and must be aligned).
> So the packed is completely useless and pointless.

Then we'll remove the packed.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[David Miller]

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Mon, 20 Jan 2014 12:58:42 +0100

> On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
>> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
>>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
>>>> <sergei.shtylyov@cogentembedded.com> wrote:
>>>>> Changes in version 3:
>>>>
>>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>>>>> - removed unneeded type cast in the probe() method.
>>>>
>>>>> +/* 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;
>>>>
>>>> Sorry, I missed the earlier discussion, but why the _packed?
>>>> One u32 and 12 bytes makes a nice multiple of 4.
>>>
>>> Better safe than sorry, it's the layout of the registers in hardware,
>>> don't let the compiler optimize here.
>> 
>> Actually __packed makes it less safe, as the compiler now assumes
>> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
>> byte accesses.
>> 
>> Fortunately it won't happen in this case as the code uses writel/readl to
>> acces the id member.
> 
> Yes, as this are registers they must not be accessed directly. However
> we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
> that the base address of this struct is always aligned to 4 bytes.

I truly think using packed here is rediculous, please remove it unless
you can prove that things won't work without it.

Thanks.

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

On 01/20/2014 10:16 PM, David Miller wrote:

>>>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>>>>>> - removed unneeded type cast in the probe() method.

>>>>>> +/* 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;

>>>>> Sorry, I missed the earlier discussion, but why the _packed?
>>>>> One u32 and 12 bytes makes a nice multiple of 4.

>>>> Better safe than sorry, it's the layout of the registers in hardware,
>>>> don't let the compiler optimize here.

>>> Actually __packed makes it less safe, as the compiler now assumes
>>> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
>>> byte accesses.

>>> Fortunately it won't happen in this case as the code uses writel/readl to
>>> acces the id member.

>> Yes, as this are registers they must not be accessed directly. However
>> we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
>> that the base address of this struct is always aligned to 4 bytes.

> I truly think using packed here is rediculous, please remove it unless
> you can prove that things won't work without it.

    They will. But how about the following 'struct rcar_can_regs'?

> Thanks.

WBR, Sergei

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

On 01/20/2014 11:12 PM, Sergei Shtylyov wrote:
[...]

>> I truly think using packed here is rediculous, please remove it unless
>> you can prove that things won't work without it.
> 
>    They will. But how about the following 'struct rcar_can_regs'?

The strcuts in question are:

> +/* 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;
> +
> +struct rcar_can_regs {
> +	struct rcar_can_mbox_regs mb[RCAR_CAN_N_MBX]; /* Mailbox registers */
> +	u32 mkr_2_9[8];	/* Mask Registers 2-9 */
> +	u32 fidcr[2];	/* FIFO Received ID Compare Register */
> +	u32 mkivlr1;	/* Mask Invalid Register 1 */
> +	u32 mier1;	/* Mailbox Interrupt Enable Register 1 */
> +	u32 mkr_0_1[2];	/* Mask Registers 0-1 */
> +	u32 mkivlr0;    /* Mask Invalid Register 0*/
> +	u32 mier0;      /* Mailbox Interrupt Enable Register 0 */
> +	u8 pad_440[0x3c0];
> +	u8 mctl[64];	/* Message Control Registers */
> +	u16 ctlr;	/* Control Register */
> +	u16 str;	/* Status register */
> +	u8 bcr[3];	/* Bit Configuration Register */
> +	u8 clkr;	/* Clock Select Register */
> +	u8 rfcr;	/* Receive FIFO Control Register */
> +	u8 rfpcr;	/* Receive FIFO Pointer Control Register */
> +	u8 tfcr;	/* Transmit FIFO Control Register */
> +	u8 tfpcr;       /* Transmit FIFO Pointer Control Register */
> +	u8 eier;	/* Error Interrupt Enable Register */
> +	u8 eifr;	/* Error Interrupt Factor Judge Register */
> +	u8 recr;	/* Receive Error Count Register */
> +	u8 tecr;        /* Transmit Error Count Register */
> +	u8 ecsr;	/* Error Code Store Register */
> +	u8 cssr;	/* Channel Search Support Register */
> +	u8 mssr;	/* Mailbox Search Status Register */
> +	u8 msmr;	/* Mailbox Search Mode Register */
> +	u16 tsr;	/* Time Stamp Register */
> +	u8 afsr;	/* Acceptance Filter Support Register */
> +	u8 pad_857;
> +	u8 tcr;		/* Test Control Register */
> +	u8 pad_859[7];
> +	u8 ier;		/* Interrupt Enable Register */
> +	u8 isr;		/* Interrupt Status Register */
> +	u8 pad_862;
> +	u8 mbsmr;	/* Mailbox Search Mask Register */
> +} __packed;

I think they should work without packed, too.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Ben Dooks]

On 20/01/14 21:17, Marc Kleine-Budde wrote:
> On 01/20/2014 11:12 PM, Sergei Shtylyov wrote:
> [...]
>
>>> I truly think using packed here is rediculous, please remove it unless
>>> you can prove that things won't work without it.
>>
>>     They will. But how about the following 'struct rcar_can_regs'?
>
> The strcuts in question are:
>
>> +/* 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;
>> +
>> +struct rcar_can_regs {
>> +	struct rcar_can_mbox_regs mb[RCAR_CAN_N_MBX]; /* Mailbox registers */
>> +	u32 mkr_2_9[8];	/* Mask Registers 2-9 */
>> +	u32 fidcr[2];	/* FIFO Received ID Compare Register */
>> +	u32 mkivlr1;	/* Mask Invalid Register 1 */
>> +	u32 mier1;	/* Mailbox Interrupt Enable Register 1 */
>> +	u32 mkr_0_1[2];	/* Mask Registers 0-1 */
>> +	u32 mkivlr0;    /* Mask Invalid Register 0*/
>> +	u32 mier0;      /* Mailbox Interrupt Enable Register 0 */
>> +	u8 pad_440[0x3c0];
>> +	u8 mctl[64];	/* Message Control Registers */
>> +	u16 ctlr;	/* Control Register */
>> +	u16 str;	/* Status register */
>> +	u8 bcr[3];	/* Bit Configuration Register */
>> +	u8 clkr;	/* Clock Select Register */
>> +	u8 rfcr;	/* Receive FIFO Control Register */
>> +	u8 rfpcr;	/* Receive FIFO Pointer Control Register */
>> +	u8 tfcr;	/* Transmit FIFO Control Register */
>> +	u8 tfpcr;       /* Transmit FIFO Pointer Control Register */
>> +	u8 eier;	/* Error Interrupt Enable Register */
>> +	u8 eifr;	/* Error Interrupt Factor Judge Register */
>> +	u8 recr;	/* Receive Error Count Register */
>> +	u8 tecr;        /* Transmit Error Count Register */
>> +	u8 ecsr;	/* Error Code Store Register */
>> +	u8 cssr;	/* Channel Search Support Register */
>> +	u8 mssr;	/* Mailbox Search Status Register */
>> +	u8 msmr;	/* Mailbox Search Mode Register */
>> +	u16 tsr;	/* Time Stamp Register */
>> +	u8 afsr;	/* Acceptance Filter Support Register */
>> +	u8 pad_857;
>> +	u8 tcr;		/* Test Control Register */
>> +	u8 pad_859[7];
>> +	u8 ier;		/* Interrupt Enable Register */
>> +	u8 isr;		/* Interrupt Status Register */
>> +	u8 pad_862;
>> +	u8 mbsmr;	/* Mailbox Search Mask Register */
>> +} __packed;
>
> I think they should work without packed, too.

I think this discussion proves why this is not a good idea.

IIRC, a compiler has the right to pad, or re-order structure
elements as it sees fit depending on the architecture and options.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Geert Uytterhoeven]

On Wed, Jan 22, 2014 at 12:52 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> IIRC, a compiler has the right to pad, or re-order structure
> elements as it sees fit depending on the architecture and options.

Pad: yes, reorder: no.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v5] can: add Renesas R-Car CAN driver

[David Laight]

RnJvbTogQmVuIERvb2tzDQo+IE9uIDIwLzAxLzE0IDIxOjE3LCBNYXJjIEtsZWluZS1CdWRkZSB3
cm90ZToNCj4gPiBPbiAwMS8yMC8yMDE0IDExOjEyIFBNLCBTZXJnZWkgU2h0eWx5b3Ygd3JvdGU6
DQo+ID4gWy4uLl0NCj4gPg0KPiA+Pj4gSSB0cnVseSB0aGluayB1c2luZyBwYWNrZWQgaGVyZSBp
cyByZWRpY3Vsb3VzLCBwbGVhc2UgcmVtb3ZlIGl0IHVubGVzcw0KPiA+Pj4geW91IGNhbiBwcm92
ZSB0aGF0IHRoaW5ncyB3b24ndCB3b3JrIHdpdGhvdXQgaXQuDQo+ID4+DQo+ID4+ICAgICBUaGV5
IHdpbGwuIEJ1dCBob3cgYWJvdXQgdGhlIGZvbGxvd2luZyAnc3RydWN0IHJjYXJfY2FuX3JlZ3Mn
Pw0KPiA+DQo+ID4gVGhlIHN0cmN1dHMgaW4gcXVlc3Rpb24gYXJlOg0KPiA+DQo+ID4+ICsvKiBN
YWlsYm94IHJlZ2lzdGVycyBzdHJ1Y3R1cmUgKi8NCj4gPj4gK3N0cnVjdCByY2FyX2Nhbl9tYm94
X3JlZ3Mgew0KPiA+PiArCXUzMiBpZDsJCS8qIElERSBhbmQgUlRSIGJpdHMsIFNJRCBhbmQgRUlE
ICovDQo+ID4+ICsJdTggc3R1YjsJLyogTm90IHVzZWQgKi8NCj4gPj4gKwl1OCBkbGM7CQkvKiBE
YXRhIExlbmd0aCBDb2RlIC0gYml0cyBbMC4uM10gKi8NCj4gPj4gKwl1OCBkYXRhWzhdOwkvKiBE
YXRhIEJ5dGVzICovDQo+ID4+ICsJdTggdHNoOwkJLyogVGltZSBTdGFtcCBIaWdoZXIgQnl0ZSAq
Lw0KPiA+PiArCXU4IHRzbDsJCS8qIFRpbWUgU3RhbXAgTG93ZXIgQnl0ZSAqLw0KPiA+PiArfSBf
X3BhY2tlZDsNCj4gPj4gKw0KPiA+PiArc3RydWN0IHJjYXJfY2FuX3JlZ3Mgew0KPiA+PiArCXN0
cnVjdCByY2FyX2Nhbl9tYm94X3JlZ3MgbWJbUkNBUl9DQU5fTl9NQlhdOyAvKiBNYWlsYm94IHJl
Z2lzdGVycyAqLw0KPiA+PiArCXUzMiBta3JfMl85WzhdOwkvKiBNYXNrIFJlZ2lzdGVycyAyLTkg
Ki8NCj4gPj4gKwl1MzIgZmlkY3JbMl07CS8qIEZJRk8gUmVjZWl2ZWQgSUQgQ29tcGFyZSBSZWdp
c3RlciAqLw0KPiA+PiArCXUzMiBta2l2bHIxOwkvKiBNYXNrIEludmFsaWQgUmVnaXN0ZXIgMSAq
Lw0KPiA+PiArCXUzMiBtaWVyMTsJLyogTWFpbGJveCBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVy
IDEgKi8NCj4gPj4gKwl1MzIgbWtyXzBfMVsyXTsJLyogTWFzayBSZWdpc3RlcnMgMC0xICovDQo+
ID4+ICsJdTMyIG1raXZscjA7ICAgIC8qIE1hc2sgSW52YWxpZCBSZWdpc3RlciAwKi8NCj4gPj4g
Kwl1MzIgbWllcjA7ICAgICAgLyogTWFpbGJveCBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVyIDAg
Ki8NCj4gPj4gKwl1OCBwYWRfNDQwWzB4M2MwXTsNCj4gPj4gKwl1OCBtY3RsWzY0XTsJLyogTWVz
c2FnZSBDb250cm9sIFJlZ2lzdGVycyAqLw0KPiA+PiArCXUxNiBjdGxyOwkvKiBDb250cm9sIFJl
Z2lzdGVyICovDQo+ID4+ICsJdTE2IHN0cjsJLyogU3RhdHVzIHJlZ2lzdGVyICovDQo+ID4+ICsJ
dTggYmNyWzNdOwkvKiBCaXQgQ29uZmlndXJhdGlvbiBSZWdpc3RlciAqLw0KPiA+PiArCXU4IGNs
a3I7CS8qIENsb2NrIFNlbGVjdCBSZWdpc3RlciAqLw0KPiA+PiArCXU4IHJmY3I7CS8qIFJlY2Vp
dmUgRklGTyBDb250cm9sIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggcmZwY3I7CS8qIFJlY2VpdmUg
RklGTyBQb2ludGVyIENvbnRyb2wgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCB0ZmNyOwkvKiBUcmFu
c21pdCBGSUZPIENvbnRyb2wgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCB0ZnBjcjsgICAgICAgLyog
VHJhbnNtaXQgRklGTyBQb2ludGVyIENvbnRyb2wgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCBlaWVy
OwkvKiBFcnJvciBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggZWlmcjsJ
LyogRXJyb3IgSW50ZXJydXB0IEZhY3RvciBKdWRnZSBSZWdpc3RlciAqLw0KPiA+PiArCXU4IHJl
Y3I7CS8qIFJlY2VpdmUgRXJyb3IgQ291bnQgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCB0ZWNyOyAg
ICAgICAgLyogVHJhbnNtaXQgRXJyb3IgQ291bnQgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCBlY3Ny
OwkvKiBFcnJvciBDb2RlIFN0b3JlIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggY3NzcjsJLyogQ2hh
bm5lbCBTZWFyY2ggU3VwcG9ydCBSZWdpc3RlciAqLw0KPiA+PiArCXU4IG1zc3I7CS8qIE1haWxi
b3ggU2VhcmNoIFN0YXR1cyBSZWdpc3RlciAqLw0KPiA+PiArCXU4IG1zbXI7CS8qIE1haWxib3gg
U2VhcmNoIE1vZGUgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1MTYgdHNyOwkvKiBUaW1lIFN0YW1wIFJl
Z2lzdGVyICovDQo+ID4+ICsJdTggYWZzcjsJLyogQWNjZXB0YW5jZSBGaWx0ZXIgU3VwcG9ydCBS
ZWdpc3RlciAqLw0KPiA+PiArCXU4IHBhZF84NTc7DQo+ID4+ICsJdTggdGNyOwkJLyogVGVzdCBD
b250cm9sIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggcGFkXzg1OVs3XTsNCj4gPj4gKwl1OCBpZXI7
CQkvKiBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggaXNyOwkJLyogSW50
ZXJydXB0IFN0YXR1cyBSZWdpc3RlciAqLw0KPiA+PiArCXU4IHBhZF84NjI7DQo+ID4+ICsJdTgg
bWJzbXI7CS8qIE1haWxib3ggU2VhcmNoIE1hc2sgUmVnaXN0ZXIgKi8NCj4gPj4gK30gX19wYWNr
ZWQ7DQo+ID4NCj4gPiBJIHRoaW5rIHRoZXkgc2hvdWxkIHdvcmsgd2l0aG91dCBwYWNrZWQsIHRv
by4NCj4gDQo+IEkgdGhpbmsgdGhpcyBkaXNjdXNzaW9uIHByb3ZlcyB3aHkgdGhpcyBpcyBub3Qg
YSBnb29kIGlkZWEuDQoNCllvdSBuZWVkIHRoZSBzZWNvbmQgc3RydWN0dXJlIHRvIGJlIHRoZSBj
b3JyZWN0IHNpemUsIHdpdGggb3Igd2l0aG91dA0KX19wYWNrZWQgdGhlcmUgY291bGQgZWFzaWx5
IGJlIGEgdHlwbyAtIHNvIGFkZCBhIGNvbXBpbGUtdHlwZSBhc3NlcnQNCm9uIHRoZSBzaXplLg0K
DQpJZiB0aGlzIG1hdGNoZXMgc29tZSBoYXJkd2FyZSBzcGVjLCBhbmQgdGhlIGhhcmR3YXJlIHNw
ZWMgZG9lc24ndA0KaGF2ZSBhbnl0aGluZyBtaXNhbGlnbmVkLCB0aGVuIHlvdSBkb24ndCB3YW50
IHRvIHNwZWNpZnkgX19wYWNrZWQuDQpXaXRob3V0IHRoZSBfX3BhY2tlZCB0aGUgc2l6ZSBjaGVj
ayB3aWxsIGRldGVjdCBtb3JlIHR5cG9zLg0KDQo+IElJUkMsIGEgY29tcGlsZXIgaGFzIHRoZSBy
aWdodCB0byBwYWQsIG9yIHJlLW9yZGVyIHN0cnVjdHVyZQ0KPiBlbGVtZW50cyBhcyBpdCBzZWVz
IGZpdCBkZXBlbmRpbmcgb24gdGhlIGFyY2hpdGVjdHVyZSBhbmQgb3B0aW9ucy4NCg0KVGhlIGxh
bmd1YWdlIG1pZ2h0IGFsbG93IG1hbnkgdGhpbmdzLCB0aGUgQUJJIGlzIG11Y2ggbW9yZSBleHBs
aWNpdC4NCkluIHRoaXMgY2FzZSB5b3UgcmVhbGx5IGNhcmUgYWJvdXQgdGhlIEFCSS4NCg0KCURh
dmlkDQoNCg=

Re: [PATCH v5] can: add Renesas R-Car CAN driver

[Sergei Shtylyov]

Hello.

On 20-01-2014 15:43, Geert Uytterhoeven wrote:

>> Changes in version 3:

>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
>> - removed unneeded type cast in the probe() method.

>> +/* 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;

> Sorry, I missed the earlier discussion, but why the _packed?
> One u32 and 12 bytes makes a nice multiple of 4.

    Mainly for consistency as this is a structure representing the hardware 
registers.

> Gr{oetje,eeting}s,

>                          Geert

WBR, Sergei