Subject: Re: [PATCH v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

From: Marc Kleine-Budde <[email-masked]>
Date: Thu, 31 Mar 2016 22:51:20 +0200

On 03/03/2016 04:38 PM, Ramesh Shanmugasundaram wrote:
> This patch adds support for the CAN FD controller found in Renesas R-Car
> SoCs. The controller operates in CAN FD mode by default.
> 
> CAN FD mode supports both Classical CAN & CAN FD frame formats. The
> controller supports ISO 11898-1:2015 CAN FD format only.
> 
> This controller supports two channels and the driver can enable either
> or both of the channels.
> 
> Driver uses Rx FIFOs (one per channel) for reception & Common FIFOs (one
> per channel) for transmission. Rx filter rules are configured to the
> minimum (one per channel) and it accepts Standard, Extended, Data &
> Remote Frame combinations.
> 
> Note: There are few documentation errors in R-Car Gen3 Hardware User
> Manual v0.5E with respect to CAN FD controller. They are listed below:
> 
> 1. CAN FD interrupt numbers 29 & 30 are listed as per channel
> interrupts. However, they are common to both channels (i.e.) they are
> global and channel interrupts respectively.
> 
> 2. CANFD clock is derived from PLL1. This is not documented.
> 
> 3. CANFD clock is further divided by (1/2) within the CAN FD controller.
> This is not documented.
> 
> 4. The minimum value of NTSEG1 in RSCFDnCFDCmNCFG register is 2 Tq. It
> is mentioned 4 Tq in the manual.
> 
> 5. The maximum number of message RAM area the controller can use is 3584
> bytes. It is specified 10752 bytes in the manual.
> 
> Signed-off-by: Ramesh Shanmugasundaram <[email-masked]>
> ---
> Changes since v1:
> 	* Removed testmodes & debugfs code (suggested by Oliver H)
> 	* Fixed tx path race issue by introducing lock (suggested by Marc K)
> 	* Removed __maybe_unused attribute of rcar_canfd_of_table
> 
> Thanks,
> Ramesh
> ---
>  .../devicetree/bindings/net/can/rcar_canfd.txt     |   86 ++
>  drivers/net/can/Kconfig                            |   10 +
>  drivers/net/can/Makefile                           |    1 +
>  drivers/net/can/rcar_canfd.c                       | 1624 ++++++++++++++++++++
>  4 files changed, 1721 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/can/rcar_canfd.txt
>  create mode 100644 drivers/net/can/rcar_canfd.c
> 
> diff --git a/Documentation/devicetree/bindings/net/can/rcar_canfd.txt b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> new file mode 100644
> index 0000000..4299bd8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/rcar_canfd.txt
> @@ -0,0 +1,86 @@
> +Renesas R-Car CAN FD controller Device Tree Bindings
> +----------------------------------------------------
> +
> +Required properties:
> +- compatible: Must contain one or more of the following:
> +  - "renesas,rcar-gen3-canfd" for R-Car Gen3 compatible controller.
> +  - "renesas,r8a7795-canfd" for R8A7795 (R-Car H3) compatible controller.
> +
> +  When compatible with the generic version, nodes must list the
> +  SoC-specific version corresponding to the platform first, followed by the
> +  family-specific and/or generic versions.
> +
> +- reg: physical base address and size of the R-Car CAN FD register map.
> +- interrupts: interrupt specifier for the Global & Channel interrupts
> +- clocks: phandles and clock specifiers for 3 clock inputs.
> +- clock-names: 3 clock input name strings: "fck", "canfd", "can_clk".
> +- pinctrl-0: pin control group to be used for this controller.
> +- pinctrl-names: must be "default".
> +
> +Required properties for "renesas,r8a7795-canfd" compatible:
> +In R8A7795 SoC, canfd clock is a div6 clock and can be used by both CAN
> +and CAN FD controller at the same time. It needs to be scaled to maximum
> +frequency if any of these controllers use it. This is done using the
> +below properties.
> +
> +- assigned-clocks: phandle of canfd clock.
> +- assigned-clock-rates: maximum frequency of this clock.
> +
> +Each channel is represented as a child node. They can be enabled/disabled
> +using "status" property.
> +
> +Example
> +-------
> +
> +SoC common .dtsi file:
> +
> +		canfd: canfd@e66c0000 {
> +			compatible = "renesas,r8a7795-canfd",
> +				     "renesas,rcar-gen3-canfd";
> +			reg = <0 0xe66c0000 0 0x8000>;
> +			interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>,
> +				   <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 914>,
> +			       <&cpg CPG_CORE R8A7795_CLK_CANFD>,
> +			       <&can_clk>;
> +			clock-names = "fck", "canfd", "can_clk";
> +			assigned-clocks = <&cpg CPG_CORE R8A7795_CLK_CANFD>;
> +			assigned-clock-rates = <40000000>;
> +			power-domains = <&cpg>;
> +			status = "disabled";
> +
> +			channel0 {
> +				status = "disabled";
> +			};
> +
> +			channel1 {
> +				status = "disabled";
> +			};
> +		};
> +
> +Board specific .dts file:
> +
> +E.g. below enables Channel 1 alone in the board.
> +
> +&canfd {
> +        pinctrl-0 = <&canfd1_pins>;
> +        pinctrl-names = "default";
> +        status = "okay";
> +
> +	channel1 {
> +		status = "okay";
> +	};
> +};
> +
> +E.g. below enables Channel 0 alone in the board using External clock
> +as fCAN clock.
> +
> +&canfd {
> +        pinctrl-0 = <&canfd0_pins &can_clk_pins>;
> +        pinctrl-names = "default";
> +        status = "okay";
> +
> +	channel0 {
> +		status = "okay";
> +	};
> +};
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 0d40aef..0ecb4fe 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -114,6 +114,16 @@ config CAN_RCAR
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called rcar_can.
>  
> +config CANFD_RCAR
> +	tristate "Renesas R-Car CAN FD controller"
> +	depends on ARCH_RENESAS || ARM
> +	---help---
> +	  Say Y here if you want to use CAN FD controller found on
> +	  Renesas R-Car SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called rcar_canfd.
> +
>  config CAN_SUN4I
>  	tristate "Allwinner A10 CAN controller"
>  	depends on MACH_SUN4I || MACH_SUN7I || COMPILE_TEST
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index e3db0c8..401b150 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
>  obj-$(CONFIG_CAN_MSCAN)		+= mscan/
>  obj-$(CONFIG_CAN_M_CAN)		+= m_can/
>  obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
> +obj-$(CONFIG_CANFD_RCAR)	+= rcar_canfd.o
>  obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
>  obj-$(CONFIG_CAN_SUN4I)		+= sun4i_can.o
>  obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
> diff --git a/drivers/net/can/rcar_canfd.c b/drivers/net/can/rcar_canfd.c
> new file mode 100644
> index 0000000..56e089d
> --- /dev/null
> +++ b/drivers/net/can/rcar_canfd.c
> @@ -0,0 +1,1624 @@
> +/* Renesas R-Car CAN FD device driver
> + *
> + * Copyright (C) 2015 Renesas Electronics 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/moduleparam.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/of.h>
> +#include <linux/of_device.h>
> +#include <linux/bitmap.h>
> +#include <linux/bitops.h>
> +#include <linux/iopoll.h>
> +
> +#define RCANFD_DRV_NAME			"rcar_canfd"
> +
> +#define RCANFD_FIFO_DEPTH		8	/* Tx FIFO depth */
> +#define RCANFD_NAPI_WEIGHT		8	/* Rx poll quota */
> +
> +#define RCANFD_NUM_CHANNELS		2
> +#define RCANFD_CHANNELS_MASK		0x3	/* Two channels max */

(BIT(RCANFD_NUM_CHANNELS) - 1

> +
> +/* Rx FIFO is a global resource of the controller. There are 8 such FIFOs
> + * available. Each channel gets a dedicated Rx FIFO (i.e.) the channel
> + * number is added to RFFIFO index.
> + */
> +#define RCANFD_RFFIFO_IDX		0
> +
> +/* Tx/Rx or Common FIFO is a per channel resource. Each channel has 3 Common
> + * FIFOs dedicated to them. Use the first (index 0) FIFO out of the 3 for Tx.
> + */
> +#define RCANFD_CFFIFO_IDX		0
> +
> +/* Global register bits */
> +#define RCANFD_GINTF_CANFD		BIT(0)
> +
> +#define RCANFD_GCFG_TPRI		BIT(0)
> +#define RCANFD_GCFG_DCE			BIT(1)
> +#define RCANFD_GCFG_DCS			BIT(4)
> +#define RCANFD_GCFG_CMPOC		BIT(5)
> +#define RCANFD_GCFG_EEFE		BIT(6)
> +
> +#define RCANFD_GCTR_SLPR		BIT(2)
> +#define RCANFD_GCTR_MODEMASK		(0x3)
> +#define RCANFD_GCTR_GOPM		(0x0)
> +#define RCANFD_GCTR_GRESET		(0x1)
> +#define RCANFD_GCTR_GHLT		(0x2)
> +
> +#define RCANFD_GCTR_DEIE		BIT(8)
> +#define RCANFD_GCTR_MEIE		BIT(9)
> +#define RCANFD_GCTR_THLEIE		BIT(10)
> +#define RCANFD_GCTR_CFMPOFIE		BIT(11)
> +#define RCANFD_GCTR_TSRST		BIT(16)
> +
> +#define RCANFD_GSTS_RAMINIT		BIT(3)
> +#define RCANFD_GSTS_SLP			BIT(2)
> +#define RCANFD_GSTS_HLT			BIT(1)
> +#define RCANFD_GSTS_RESET		BIT(0)
> +
> +#define RCANFD_GSTS_GNOPM		(BIT(0) | BIT(1) | BIT(2) | BIT(3))
> +
> +/* Channel register bits */
> +#define RCANFD_CCTR_CSLPR		BIT(2)
> +#define RCANFD_CCTR_MODEMASK		(0x3)
> +#define RCANFD_CCTR_COPM		(0x0)
> +#define RCANFD_CCTR_CRESET		(0x1)
> +#define RCANFD_CCTR_CHLT		(0x2)
> +#define RCANFD_CCTR_CTMASK		(0x3 << 25)
> +#define RCANFD_CCTR_CTMS_ILB		(0x3 << 25)
> +#define RCANFD_CCTR_CTME		BIT(24)
> +#define RCANFD_CCTR_ERRD		BIT(23)
> +#define RCANFD_CCTR_BOMMASK		(0x3 << 21)
> +#define RCANFD_CCTR_BOM_ENTRY		(0x1 << 21)
> +#define RCANFD_CCTR_TDCVFIE		BIT(19)
> +#define RCANFD_CCTR_SOCOIE		BIT(18)
> +#define RCANFD_CCTR_EOCOIE		BIT(17)
> +#define RCANFD_CCTR_TAIE		BIT(16)
> +#define RCANFD_CCTR_ALIE		BIT(15)
> +#define RCANFD_CCTR_BLIE		BIT(14)
> +#define RCANFD_CCTR_OLIE		BIT(13)
> +#define RCANFD_CCTR_BORIE		BIT(12)
> +#define RCANFD_CCTR_BOEIE		BIT(11)
> +#define RCANFD_CCTR_EPIE		BIT(10)
> +#define RCANFD_CCTR_EWIE		BIT(9)
> +#define RCANFD_CCTR_BEIE		BIT(8)
> +
> +#define RCANFD_CSTS_COM			BIT(7)
> +#define RCANFD_CSTS_REC			BIT(6)
> +#define RCANFD_CSTS_TRM			BIT(5)
> +#define RCANFD_CSTS_BO			BIT(4)
> +#define RCANFD_CSTS_EP			BIT(3)
> +#define RCANFD_CSTS_SLP			BIT(2)
> +#define RCANFD_CSTS_HALT		BIT(1)
> +#define RCANFD_CSTS_RESET		BIT(0)
> +
> +#define RCANFD_CSTS_TECCNT(x)		(((x) >> 24) & 0xff)
> +#define RCANFD_CSTS_RECCNT(x)		(((x) >> 16) & 0xff)
> +
> +/* Bit Configuration register */
> +#define RCANFD_BRP(x)			(((x) & 0x3ff) << 0)
> +#define RCANFD_SJW(x)			(((x) & 0x3) << 24)
> +#define RCANFD_TSEG1(x)			(((x) & 0xf) << 16)
> +#define RCANFD_TSEG2(x)			(((x) & 0x7) << 20)
> +
> +#define RCANFD_NR_BRP(x)		(((x) & 0x3ff) << 0)
> +#define RCANFD_NR_SJW(x)		(((x) & 0x1f) << 11)
> +#define RCANFD_NR_TSEG1(x)		(((x) & 0x7f) << 16)
> +#define RCANFD_NR_TSEG2(x)		(((x) & 0x1f) << 24)
> +
> +#define RCANFD_DR_BRP(x)		(((x) & 0xff) << 0)
> +#define RCANFD_DR_SJW(x)		(((x) & 0x7) << 24)
> +#define RCANFD_DR_TSEG1(x)		(((x) & 0xf) << 16)
> +#define RCANFD_DR_TSEG2(x)		(((x) & 0x7) << 20)
> +
> +/* Global Error flag bits */
> +#define RCANFD_GERFL_EEF1		BIT(17)
> +#define RCANFD_GERFL_EEF0		BIT(16)
> +#define RCANFD_GERFL_CMPOF		BIT(3)
> +#define RCANFD_GERFL_THLES		BIT(2)
> +#define RCANFD_GERFL_MES		BIT(1)
> +#define RCANFD_GERFL_DEF		BIT(0)
> +
> +#define RCANFD_GERFL_ERR(x)		((x) & (RCANFD_GERFL_EEF1 |\
> +						RCANFD_GERFL_EEF0 |\
> +						RCANFD_GERFL_MES |\
> +						RCANFD_GERFL_CMPOF))
> +
> +/* Channel Error flag bits */
> +#define RCANFD_CERFL_ADEF		BIT(14)
> +#define RCANFD_CERFL_B0EF		BIT(13)
> +#define RCANFD_CERFL_B1EF		BIT(12)
> +#define RCANFD_CERFL_CEF		BIT(11)
> +#define RCANFD_CERFL_AEF		BIT(10)
> +#define RCANFD_CERFL_FEF		BIT(9)
> +#define RCANFD_CERFL_SEF		BIT(8)
> +#define RCANFD_CERFL_ALEF		BIT(7)
> +#define RCANFD_CERFL_BLEF		BIT(6)
> +#define RCANFD_CERFL_OLEF		BIT(5)
> +#define RCANFD_CERFL_BOREF		BIT(4)
> +#define RCANFD_CERFL_BOEEF		BIT(3)
> +#define RCANFD_CERFL_EPEF		BIT(2)
> +#define RCANFD_CERFL_EWEF		BIT(1)
> +#define RCANFD_CERFL_BEF		BIT(0)
> +
> +#define RCANFD_CERFL_ERR(x)		((x) & (0x7fff)) /* above bits 14:0 */
> +
> +/* CAN FD specific registers */
> +#define RCANFD_DCFG_TDCE		BIT(9)
> +#define RCANFD_DCFG_TDCOC		BIT(8)
> +#define RCANFD_DCFG_TDCO(x)		(((x) & 0x7f) >> 16)
> +
> +#define RCANFD_DCSTS_TDCR(x)		(((x) >> 0) & 0x7f)
> +#define RCANFD_DCSTS_SOCCNT(x)		(((x) >> 24) & 0xff)
> +#define RCANFD_DCSTS_EOCCNT(x)		(((x) >> 16) & 0xff)
> +
> +#define RCANFD_DCSTS_TDCVF		BIT(7)
> +#define RCANFD_DCSTS_EOCO		BIT(8)
> +#define RCANFD_DCSTS_SOCO		BIT(9)
> +
> +/* Rx FIFO bits */
> +#define RCANFD_RFFIFO_RFIF		BIT(3)
> +#define RCANFD_RFFIFO_RFMLT		BIT(2)
> +#define RCANFD_RFFIFO_RFFLL		BIT(1)
> +#define RCANFD_RFFIFO_RFEMP		BIT(0)
> +
> +#define RCANFD_RFFIFO_RFESI		BIT(0)
> +#define RCANFD_RFFIFO_RFBRS		BIT(1)
> +#define RCANFD_RFFIFO_RFFDF		BIT(2)
> +
> +#define RCANFD_RFFIFO_RFIDE		BIT(31)
> +#define RCANFD_RFFIFO_RFRTR		BIT(30)
> +
> +#define RCANFD_RFFIFO_RFDLC(x)		(((x) >> 28) & 0xf)
> +#define RCANFD_RFFIFO_RFPTR(x)		(((x) >> 16) & 0xfff)
> +#define RCANFD_RFFIFO_RFTS(x)		(((x) >> 0) & 0xff)
> +
> +#define RCANFD_RFFIFO_RFIM		BIT(12)
> +#define RCANFD_RFFIFO_RFDC(x)		(((x) & 0x7) << 8)
> +#define RCANFD_RFFIFO_RFPLS(x)		(((x) & 0x7) << 4)
> +#define RCANFD_RFFIFO_RFIE		BIT(1)
> +#define RCANFD_RFFIFO_RFE		BIT(0)
> +
> +/* Common FIFO bits */
> +#define RCANFD_CMFIFO_TML(x)		(((x) & 0xf) << 20)
> +#define RCANFD_CMFIFO_M(x)		(((x) & 0x3) << 16)
> +#define RCANFD_CMFIFO_CFIM		BIT(12)
> +#define RCANFD_CMFIFO_DC(x)		(((x) & 0x7) << 8)
> +#define RCANFD_CMFIFO_PLS(x)		(((x) & 0x7) << 4)
> +#define RCANFD_CMFIFO_CFTXIE		BIT(2)
> +#define RCANFD_CMFIFO_CFE		BIT(0)
> +
> +#define RCANFD_CMFIFO_CFTXIF		BIT(4)
> +#define RCANFD_CMFIFO_CFMLT		BIT(2)
> +#define RCANFD_CMFIFO_CFFLL		BIT(1)
> +#define RCANFD_CMFIFO_CFEMP		BIT(0)
> +#define RCANFD_CMFIFO_CFMC(x)		(((x) >> 8) & 0xff)
> +
> +#define RCANFD_CMFIFO_CFESI		BIT(0)
> +#define RCANFD_CMFIFO_CFBRS		BIT(1)
> +#define RCANFD_CMFIFO_CFFDF		BIT(2)
> +
> +#define RCANFD_CMFIFO_CFIDE		BIT(31)
> +#define RCANFD_CMFIFO_CFRTR		BIT(30)
> +#define RCANFD_CMFIFO_CFID(x)		((x) & 0x1fffffff)
> +
> +#define RCANFD_CMFIFO_CFDLC(x)		(((x) & 0xf) << 28)
> +#define RCANFD_CMFIFO_CFPTR(x)		(((x) & 0xfff) << 16)
> +#define RCANFD_CMFIFO_CFTS(x)		(((x) & 0xff) << 0)
> +
> +/* Global Test Config register */
> +#define RCANFD_GTSTCFG_C0CBCE		BIT(0)
> +#define RCANFD_GTSTCFG_C1CBCE		BIT(1)
> +
> +#define RCANFD_GTSTCTR_ICBCTME		BIT(0)
> +
> +/* AFL Rx rules registers */
> +#define RCANFD_AFLCFG_SETRNC0(x)	(((x) & 0xff) << 24)
> +#define RCANFD_AFLCFG_SETRNC1(x)	(((x) & 0xff) << 16)

What about something like:

#define RCANFD_AFLCFG_SETRNC(n, x)	(((x) & 0xff) << (24 - n * 8))

This will save some if()s in the code

> +#define RCANFD_AFLCFG_GETRNC0(x)	(((x) >> 24) & 0xff)
> +#define RCANFD_AFLCFG_GETRNC1(x)	(((x) >> 16) & 0xff)
> +
> +#define RCANFD_AFL_PAGENUM(entry)	((entry) / 16)
> +
> +#define RCANFD_AFLCTR_AFLDAE		BIT(8)
> +#define RCANFD_AFLCTR_AFLPN(x)		((x) & 0x1f)
> +#define RCANFD_CHANNEL_NUMRULES		1	/* only one rule per channel */
> +#define RCANFD_AFLID_GAFLLB		BIT(29)
> +#define RCANFD_AFLPTR1_RFFIFO(x)	(1 << (x))
> +
> +/* Common register map offsets */
> +
> +/* Nominal rate registers */
> +#define RCANFD_CCFG(m)			(0x0000 + (0x10 * (m)))
> +#define RCANFD_CCTR(m)			(0x0004 + (0x10 * (m)))
> +#define RCANFD_CSTS(m)			(0x0008 + (0x10 * (m)))
> +#define RCANFD_CERFL(m)			(0x000C + (0x10 * (m)))
> +
> +/* Global registers */
> +#define RCANFD_GCFG			(0x0084)
> +#define RCANFD_GCTR			(0x0088)
> +#define RCANFD_GSTS			(0x008c)
> +#define RCANFD_GERFL			(0x0090)
> +#define RCANFD_GTSC			(0x0094)
> +#define RCANFD_GAFLECTR			(0x0098)
> +#define RCANFD_GAFLCFG0			(0x009c)
> +#define RCANFD_GAFLCFG1			(0x00a0)
> +#define RCANFD_RMNB			(0x00a4)
> +
> +#define RCANFD_RMND(y)			(0x00a8 + (0x04 * (y)))
> +
> +/* Rx FIFO Control registers */
> +#define RCANFD_RFCC(x)			(0x00b8 + (0x04 * (x)))
> +#define RCANFD_RFSTS(x)			(0x00d8 + (0x04 * (x)))
> +#define RCANFD_RFPCTR(x)		(0x00f8 + (0x04 * (x)))
> +
> +/* Common FIFO Control registers */
> +#define RCANFD_CFCC(ch, idx)		(0x0118 + (0x0c * (ch)) + \
> +					 (0x04 * (idx)))
> +#define RCANFD_CFSTS(ch, idx)		(0x0178 + (0x0c * (ch)) + \
> +					 (0x04 * (idx)))
> +#define RCANFD_CFPCTR(ch, idx)		(0x01d8 + (0x0c * (ch)) + \
> +					 (0x04 * (idx)))
> +
> +#define RCANFD_FESTS			(0x0238)
> +#define RCANFD_FFSTS			(0x023c)
> +#define RCANFD_FMSTS			(0x0240)
> +#define RCANFD_RFISTS			(0x0244)
> +#define RCANFD_CFRISTS			(0x0248)
> +#define RCANFD_CFTISTS			(0x024c)
> +
> +#define RCANFD_TMC(p)			(0x0250 + (0x01 * (p)))
> +#define RCANFD_TMSTS(p)			(0x02d0 + (0x01 * (p)))
> +
> +#define RCANFD_TMTRSTS(y)		(0x0350 + (0x04 * (y)))
> +#define RCANFD_TMTARSTS(y)		(0x0360 + (0x04 * (y)))
> +#define RCANFD_TMTCSTS(y)		(0x0370 + (0x04 * (y)))
> +#define RCANFD_TMTASTS(y)		(0x0380 + (0x04 * (y)))
> +#define RCANFD_TMIEC(y)			(0x0390 + (0x04 * (y)))
> +
> +#define RCANFD_TXQCC(m)			(0x03a0 + (0x04 * (m)))
> +#define RCANFD_TXQSTS(m)		(0x03c0 + (0x04 * (m)))
> +#define RCANFD_TXQPCTR(m)		(0x03e0 + (0x04 * (m)))
> +
> +#define RCANFD_THLCC(m)			(0x0400 + (0x04 * (m)))
> +#define RCANFD_THLSTS(m)		(0x0420 + (0x04 * (m)))
> +#define RCANFD_THLPCTR(m)		(0x0440 + (0x04 * (m)))
> +
> +#define RCANFD_GTINTSTS0		(0x0460)
> +#define RCANFD_GTINTSTS1		(0x0464)
> +#define RCANFD_GTSTCFG			(0x0468)
> +#define RCANFD_GTSTCTR			(0x046c)
> +#define RCANFD_GLOCKK			(0x047c)
> +#define RCANFD_GRMCFG			(0x04fc)
> +
> +/* Receive rule registers */
> +#define RCANFD_GAFLID(offset, j)	((offset) + (0x10 * (j)))
> +#define RCANFD_GAFLM(offset, j)		((offset) + 0x04 + (0x10 * (j)))
> +#define RCANFD_GAFLP0(offset, j)	((offset) + 0x08 + (0x10 * (j)))
> +#define RCANFD_GAFLP1(offset, j)	((offset) + 0x0c + (0x10 * (j)))
> +
> +/* CAN FD mode specific regsiter map */
> +
> +/* Data bitrate registers */
> +#define RCANFD_F_CDFG(m)		(0x0500 + (0x20 * (m)))
> +#define RCANFD_F_CFDCFG(m)		(0x0504 + (0x20 * (m)))
> +#define RCANFD_F_CFDCTR(m)		(0x0508 + (0x20 * (m)))
> +#define RCANFD_F_CFDSTS(m)		(0x050c + (0x20 * (m)))
> +#define RCANFD_F_CFDCRC(m)		(0x0510 + (0x20 * (m)))
> +
> +#define RCANFD_F_GAFL_OFFSET		(0x1000)
> +
> +#define RCANFD_F_RMID(q)		(0x2000 + (0x10 * (q)))
> +#define RCANFD_F_RMPTR(q)		(0x2004 + (0x10 * (q)))
> +#define RCANFD_F_RMFDSTS(q)		(0x2008 + (0x10 * (q)))
> +#define RCANFD_F_RMDF(q, b)		(0x200c + (0x04 * (b)) + (0x20 * (q)))
> +
> +/* Rx FIFO data registers */
> +#define RCANFD_F_RFOFFSET		(0x3000)
> +#define RCANFD_F_RFID(x)		(RCANFD_F_RFOFFSET + (0x80 * (x)))
> +#define RCANFD_F_RFPTR(x)		(RCANFD_F_RFOFFSET + 0x04 + \
> +					 (0x80 * (x)))
> +#define RCANFD_F_RFFDSTS(x)		(RCANFD_F_RFOFFSET + 0x08 + \
> +					 (0x80 * (x)))
> +#define RCANFD_F_RFDF(x, df)		(RCANFD_F_RFOFFSET + 0x0c + \
> +					 (0x80 * (x)) + (0x04 * (df)))
> +
> +/* Common FIFO data registers */
> +#define RCANFD_F_CFOFFSET		(0x3400)
> +#define RCANFD_F_CFID(ch, idx)		(RCANFD_F_CFOFFSET + (0x180 * (ch)) + \
> +					 (0x80 * (idx)))
> +#define RCANFD_F_CFPTR(ch, idx)		(RCANFD_F_CFOFFSET + 0x04 + \
> +					 (0x180 * (ch)) + (0x80 * (idx)))
> +#define RCANFD_F_CFFDCSTS(ch, idx)	(RCANFD_F_CFOFFSET + 0x08 + \
> +					 (0x180 * (ch)) + (0x80 * (idx)))
> +#define RCANFD_F_CFDF(ch, idx, df)	(RCANFD_F_CFOFFSET + 0x0c + \
> +					 (0x180 * (ch)) + (0x80 * (idx)) + \
> +					 (0x04 * (df)))
> +
> +#define RCANFD_F_TMID(p)		(0x4000 + (0x20 * (p)))
> +#define RCANFD_F_TMPTR(p)		(0x4004 + (0x20 * (p)))
> +#define RCANFD_F_TMFDCTR(p)		(0x4008 + (0x20 * (p)))
> +#define RCANFD_F_TMDF(p, b)		(0x400c + (0x20 * (p)) + (0x04 * (b)))
> +
> +#define RCANFD_F_THLACC(m)		(0x6000 + (0x04 * (m)))
> +#define RCANFD_F_RPGACC(r)		(0x6400 + (0x04 * (r)))
> +
> +struct rcar_canfd_global;
> +
> +/* Channel priv data */
> +struct rcar_canfd_channel {
> +	struct can_priv can;			/* Must be the first member */
> +	struct net_device *ndev;
> +	struct rcar_canfd_global *gpriv;	/* Controller reference */
> +	void __iomem *base;			/* Register base address */
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *dev_root;
> +#endif /* CONFIG_DEBUG_FS */
> +	struct napi_struct napi;
> +	u8  tx_len[RCANFD_FIFO_DEPTH];		/* For net stats */
> +	u32 tx_head;				/* Incremented on xmit */
> +	u32 tx_tail;				/* Incremented on xmit done */
> +	u32 channel;				/* Channel number */
> +	spinlock_t tx_lock;			/* To protect tx path */
> +};
> +
> +/* Global priv data */
> +struct rcar_canfd_global {
> +	struct rcar_canfd_channel *ch[RCANFD_NUM_CHANNELS];
> +	void __iomem *base;		/* Register base address */
> +	struct platform_device *pdev;	/* Respective platform device */
> +	struct clk *clkp;		/* Peripheral clock */
> +	struct clk *can_clk;		/* fCAN clock */
> +	int clock_select;		/* CANFD or Ext clock */
        ^^^
please give the corresponding enum a proper name and use it here.

> +	unsigned long channels_mask;	/* Enabled channels mask */
> +	u32 freq;			/* fCAN clock frequency in Hz */

This value is not used outside of the probe function. You can pass the
freq to the individual channels via an argument.

> +};
> +
> +/* CAN FD mode nominal rate constants */
> +static const struct can_bittiming_const rcar_canfd_nom_bittiming_const = {
> +	.name = RCANFD_DRV_NAME,
> +	.tseg1_min = 2,
> +	.tseg1_max = 128,
> +	.tseg2_min = 2,
> +	.tseg2_max = 32,
> +	.sjw_max = 32,
> +	.brp_min = 1,
> +	.brp_max = 1024,
> +	.brp_inc = 1,
> +};
> +
> +/* CAN FD mode data rate constants */
> +static const struct can_bittiming_const rcar_canfd_data_bittiming_const = {
> +	.name = RCANFD_DRV_NAME,
> +	.tseg1_min = 2,
> +	.tseg1_max = 16,
> +	.tseg2_min = 2,
> +	.tseg2_max = 8,
> +	.sjw_max = 8,
> +	.brp_min = 1,
> +	.brp_max = 256,
> +	.brp_inc = 1,
> +};
> +
> +/* fCAN clock select register settings */
> +enum {
> +	RCANFD_CANFDCLK = 0,	/* CANFD clock */
> +	RCANFD_EXTCLK,		/* Externally input clock */
> +};
> +
> +/* Helper functions */
> +static inline void rcar_canfd_update(u32 mask, u32 val, u32 __iomem *reg)
> +{
> +	u32 data = readl(reg);
> +
> +	data &= ~mask;
> +	data |= (val & mask);
> +	writel(data, reg);
> +}
> +
> +#define rcar_canfd_read(priv, offset)			\
> +	readl(priv->base + (offset))
> +#define rcar_canfd_write(priv, offset, val)		\
> +	writel(val, priv->base + (offset))
> +#define rcar_canfd_set_bit(priv, reg, val)		\
> +	rcar_canfd_update(val, val, priv->base + (reg))
> +#define rcar_canfd_clear_bit(priv, reg, val)		\
> +	rcar_canfd_update(val, 0, priv->base + (reg))
> +#define rcar_canfd_update_bit(priv, reg, mask, val)	\
> +	rcar_canfd_update(mask, val, priv->base + (reg))

please use static inline functions instead of defines.

> +
> +static void rcar_canfd_get_data(struct canfd_frame *cf,
> +				struct rcar_canfd_channel *priv, u32 off)

Please use "struct rcar_canfd_channel *priv" as first argument, struct
canfd_frame *cf as second. Remove off, as the offset is already defined
by the channel.

> +{
> +	u32 i, lwords;
> +
> +	lwords = cf->len / sizeof(u32);
> +	if (cf->len % sizeof(u32))
> +		lwords++;

Use DIV_ROUND_UP() instead of open coding it.

> +	for (i = 0; i < lwords; i++)
> +		*((u32 *)cf->data + i) =
> +			rcar_canfd_read(priv, off + (i * sizeof(u32)));
> +}
> +
> +static void rcar_canfd_put_data(struct canfd_frame *cf,
> +				struct rcar_canfd_channel *priv, u32 off)

same here

> +{
> +	u32 i, j, lwords, leftover;
> +	u32 data = 0;
> +
> +	lwords = cf->len / sizeof(u32);
> +	leftover = cf->len % sizeof(u32);
> +	for (i = 0; i < lwords; i++)
> +		rcar_canfd_write(priv, off + (i * sizeof(u32)),
> +				 *((u32 *)cf->data + i));

Here you don't convert the endianess...
> +
> +	if (leftover) {
> +		u8 *p = (u8 *)((u32 *)cf->data + i);
> +
> +		for (j = 0; j < leftover; j++)
> +			data |= p[j] << (j * 8);

...here you do an implicit endianess conversion. "data" is little
endian, while p[j] is big endian.

> +		rcar_canfd_write(priv, off + (i * sizeof(u32)), data);
> +	}

Have you tested to send CAN frames with len != n*4 against a different
controller?

> +}
> +
> +static void rcar_canfd_tx_failure_cleanup(struct net_device *ndev)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < RCANFD_FIFO_DEPTH; i++)
> +		can_free_echo_skb(ndev, i);
> +}
> +
> +static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
> +{
> +	u32 sts, ch;
> +	int err;
> +
> +	/* Check RAMINIT flag as CAN RAM initialization takes place
> +	 * after the MCU reset
> +	 */
> +	err = readl_poll_timeout((gpriv->base + RCANFD_GSTS), sts,
> +				 !(sts & RCANFD_GSTS_RAMINIT), 2, 500000);
> +	if (err) {
> +		dev_dbg(&gpriv->pdev->dev, "global raminit failed\n");
> +		return err;
> +	}
> +
> +	/* Transition to Global Reset mode */
> +	rcar_canfd_clear_bit(gpriv, RCANFD_GCTR, RCANFD_GCTR_SLPR);
> +	rcar_canfd_update_bit(gpriv, RCANFD_GCTR,
> +			      RCANFD_GCTR_MODEMASK, RCANFD_GCTR_GRESET);
> +
> +	/* Ensure Global reset mode */
> +	err = readl_poll_timeout((gpriv->base + RCANFD_GSTS), sts,
> +				 (sts & RCANFD_GSTS_RESET), 2, 500000);
> +	if (err) {
> +		dev_dbg(&gpriv->pdev->dev, "global reset failed\n");
> +		return err;
> +	}
> +
> +	/* Reset Global error flags */
> +	rcar_canfd_write(gpriv, RCANFD_GERFL, 0x0);
> +
> +	/* Set the controller into FD mode */
> +	rcar_canfd_set_bit(gpriv, RCANFD_GRMCFG, RCANFD_GINTF_CANFD);
> +
> +	/* Transition all Channels to reset mode */
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +		rcar_canfd_clear_bit(gpriv, RCANFD_CCTR(ch), RCANFD_CCTR_CSLPR);
> +
> +		rcar_canfd_update_bit(gpriv, RCANFD_CCTR(ch),
> +				      RCANFD_CCTR_MODEMASK,
> +				      RCANFD_CCTR_CRESET);
> +
> +		/* Ensure Channel reset mode */
> +		err = readl_poll_timeout((gpriv->base + RCANFD_CSTS(ch)), sts,
> +					 (sts & RCANFD_CSTS_RESET), 2, 500000);
> +		if (err) {
> +			dev_dbg(&gpriv->pdev->dev,
> +				"channel %u reset failed\n", ch);
> +			return err;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void rcar_canfd_configure_controller(struct rcar_canfd_global *gpriv)
> +{
> +	u32 cfg, ch;
> +
> +	/* Global Configuration settings */
> +	cfg = RCANFD_GCFG_EEFE;		/* ECC error flag enabled */
> +
> +	/* Set External Clock if selected */
> +	if (gpriv->clock_select != RCANFD_CANFDCLK)
> +		cfg |= RCANFD_GCFG_DCS;
> +
> +	/* Truncate payload to configured message size RFPLS */
> +	cfg |= RCANFD_GCFG_CMPOC;
> +
> +	rcar_canfd_set_bit(gpriv, RCANFD_GCFG, cfg);
> +
> +	/* Channel configuration settings */
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +		rcar_canfd_set_bit(gpriv, RCANFD_CCTR(ch), RCANFD_CCTR_ERRD);
> +		rcar_canfd_update_bit(gpriv, RCANFD_CCTR(ch),
> +				      RCANFD_CCTR_BOMMASK,
> +				      RCANFD_CCTR_BOM_ENTRY);
> +	}
> +}
> +
> +static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
> +					   u32 ch)
> +{
> +	u32 cfg;
> +	int start, page, num_rules = RCANFD_CHANNEL_NUMRULES;
> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +	if (ch == 0) {
> +		start = 0; /* Start from 0th rule */
> +	} else {
> +		/* Get number of existing rules and adjust */
> +		cfg = rcar_canfd_read(gpriv, RCANFD_GAFLCFG0);
> +		start = RCANFD_AFLCFG_GETRNC0(cfg);
> +	}
> +
> +	/* Enable write access to entry */
> +	page = RCANFD_AFL_PAGENUM(start);
> +	rcar_canfd_set_bit(gpriv, RCANFD_GAFLECTR,
> +			   (RCANFD_AFLCTR_AFLPN(page) | RCANFD_AFLCTR_AFLDAE));
> +
> +	/* Write number of rules for channel */
> +	if (ch == 0)
> +		rcar_canfd_set_bit(gpriv, RCANFD_GAFLCFG0,
> +				   RCANFD_AFLCFG_SETRNC0(num_rules));
> +	else
> +		rcar_canfd_set_bit(gpriv, RCANFD_GAFLCFG0,
> +				   RCANFD_AFLCFG_SETRNC1(num_rules));
> +
> +	/* Accept all IDs */
> +	rcar_canfd_write(gpriv, RCANFD_GAFLID(RCANFD_F_GAFL_OFFSET, start), 0);
> +	/* IDE or RTR is not considered for matching */
> +	rcar_canfd_write(gpriv, RCANFD_GAFLM(RCANFD_F_GAFL_OFFSET, start), 0);
> +	/* Any data length accepted */
> +	rcar_canfd_write(gpriv, RCANFD_GAFLP0(RCANFD_F_GAFL_OFFSET, start), 0);
> +	/* Place the msg in corresponding Rx FIFO entry */
> +	rcar_canfd_write(gpriv, RCANFD_GAFLP1(RCANFD_F_GAFL_OFFSET, start),
> +			 RCANFD_AFLPTR1_RFFIFO(ridx));
> +
> +	/* Disable write access to page */
> +	rcar_canfd_clear_bit(gpriv, RCANFD_GAFLECTR, RCANFD_AFLCTR_AFLDAE);
> +}
> +
> +static void rcar_canfd_configure_rx(struct rcar_canfd_global *gpriv, u32 ch)
> +{
> +	/* Rx FIFO is used for reception */
> +	u32 cfg;
> +	u16 rfdc, rfpls;
> +
> +	/* Select Rx FIFO based on channel */
> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +	rfdc = 2;		/* b010 - 8 messages Rx FIFO depth */
> +	rfpls = 7;		/* b111 - Max 64 bytes payload */
> +
> +	cfg = (RCANFD_RFFIFO_RFIM | RCANFD_RFFIFO_RFDC(rfdc) |
> +		RCANFD_RFFIFO_RFPLS(rfpls) | RCANFD_RFFIFO_RFIE);
> +	rcar_canfd_write(gpriv, RCANFD_RFCC(ridx), cfg);
> +}
> +
> +static void rcar_canfd_configure_tx(struct rcar_canfd_global *gpriv, u32 ch)
> +{
> +	/* Tx/Rx(Common) FIFO configured in Tx mode is
> +	 * used for transmission
> +	 *
> +	 * Each channel has 3 Common FIFO dedicated to them.
> +	 * Use the 1st (index 0) out of 3
> +	 */
> +	u32 cfg;
> +	u16 cftml, cfm, cfdc, cfpls;
> +
> +	cftml = 0;		/* 0th buffer */
> +	cfm = 1;		/* b01 - Transmit mode */
> +	cfdc = 2;		/* b010 - 8 messages Tx FIFO depth */
> +	cfpls = 7;		/* b111 - Max 64 bytes payload */
> +
> +	cfg = (RCANFD_CMFIFO_TML(cftml) | RCANFD_CMFIFO_M(cfm) |
> +		RCANFD_CMFIFO_CFIM | RCANFD_CMFIFO_DC(cfdc) |
> +		RCANFD_CMFIFO_PLS(cfpls) | RCANFD_CMFIFO_CFTXIE);
> +	rcar_canfd_write(gpriv, RCANFD_CFCC(ch, RCANFD_CFFIFO_IDX), cfg);
> +
> +	/* Clear FD mode specific control/status register */
> +	rcar_canfd_write(gpriv, RCANFD_F_CFFDCSTS(ch, RCANFD_CFFIFO_IDX), 0);
> +}
> +
> +static void rcar_canfd_enable_global_interrupts(struct rcar_canfd_global *gpriv)
> +{
> +	u32 ctr;
> +
> +	/* Clear any stray error interrupt flags */
> +	rcar_canfd_write(gpriv, RCANFD_GERFL, 0);
> +
> +	/* Global interrupts setup */
> +	ctr = RCANFD_GCTR_MEIE;
> +	ctr |= RCANFD_GCTR_CFMPOFIE;
> +
> +	rcar_canfd_set_bit(gpriv, RCANFD_GCTR, ctr);
> +}
> +
> +static void rcar_canfd_disable_global_interrupts(struct rcar_canfd_global
> +						 *gpriv)
> +{
> +	/* Disable all interrupts */
> +	rcar_canfd_write(gpriv, RCANFD_GCTR, 0);
> +
> +	/* Clear any stray error interrupt flags */
> +	rcar_canfd_write(gpriv, RCANFD_GERFL, 0);
> +}
> +
> +static void rcar_canfd_enable_channel_interrupts(struct rcar_canfd_channel
> +						 *priv)
> +{
> +	u32 ctr, ch = priv->channel;
> +
> +	/* Clear any stray error flags */
> +	rcar_canfd_write(priv, RCANFD_CERFL(ch), 0);
> +
> +	/* Channel interrupts setup */
> +	ctr = (RCANFD_CCTR_TAIE |
> +	       RCANFD_CCTR_ALIE | RCANFD_CCTR_BLIE |
> +	       RCANFD_CCTR_OLIE | RCANFD_CCTR_BORIE |
> +	       RCANFD_CCTR_BOEIE | RCANFD_CCTR_EPIE |
> +	       RCANFD_CCTR_EWIE | RCANFD_CCTR_BEIE);
> +	rcar_canfd_set_bit(priv, RCANFD_CCTR(ch), ctr);
> +}
> +
> +static void rcar_canfd_disable_channel_interrupts(struct rcar_canfd_channel
> +						  *priv)
> +{
> +	u32 ctr, ch = priv->channel;
> +
> +	ctr = (RCANFD_CCTR_TAIE |
> +	       RCANFD_CCTR_ALIE | RCANFD_CCTR_BLIE |
> +	       RCANFD_CCTR_OLIE | RCANFD_CCTR_BORIE |
> +	       RCANFD_CCTR_BOEIE | RCANFD_CCTR_EPIE |
> +	       RCANFD_CCTR_EWIE | RCANFD_CCTR_BEIE);
> +	rcar_canfd_clear_bit(priv, RCANFD_CCTR(ch), ctr);
> +
> +	/* Clear any stray error flags */
> +	rcar_canfd_write(priv, RCANFD_CERFL(ch), 0);
> +}
> +
> +static void rcar_canfd_global_error(struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	u32 ch = priv->channel;
> +	u32 gerfl, sts;
> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +	gerfl = rcar_canfd_read(priv, RCANFD_GERFL);
> +	if ((gerfl & RCANFD_GERFL_EEF0) && (ch == 0)) {
> +		netdev_dbg(ndev, "Ch0: ECC Error flag\n");
> +		stats->tx_dropped++;
> +	}
> +	if ((gerfl & RCANFD_GERFL_EEF1) && (ch == 1)) {
> +		netdev_dbg(ndev, "Ch1: ECC Error flag\n");
> +		stats->tx_dropped++;
> +	}
> +	if (gerfl & RCANFD_GERFL_MES) {
> +		sts = rcar_canfd_read(priv,
> +				      RCANFD_CFSTS(ch, RCANFD_CFFIFO_IDX));
> +		if (sts & RCANFD_CMFIFO_CFMLT) {
> +			netdev_dbg(ndev, "Tx Message Lost flag\n");
> +			stats->tx_dropped++;
> +			rcar_canfd_write(priv,
> +					 RCANFD_CFSTS(ch, RCANFD_CFFIFO_IDX),
> +					 sts & ~RCANFD_CMFIFO_CFMLT);
> +		}
> +
> +		sts = rcar_canfd_read(priv, RCANFD_RFSTS(ridx));
> +		if (sts & RCANFD_RFFIFO_RFMLT) {
> +			netdev_dbg(ndev, "Rx Message Lost flag\n");
> +			stats->rx_dropped++;
> +			rcar_canfd_write(priv, RCANFD_RFSTS(ridx),
> +					 sts & ~RCANFD_RFFIFO_RFMLT);
> +		}
> +	}
> +	if (gerfl & RCANFD_GERFL_CMPOF) {
> +		/* Message Lost flag will be set for respective channel
> +		 * when this condition happens with counters and flags
> +		 * already updated.
> +		 */
> +		netdev_dbg(ndev, "global payload overflow interrupt\n");
> +	}
> +
> +	/* Clear all global error interrupts. Only affected channels bits
> +	 * get cleared
> +	 */
> +	rcar_canfd_write(priv, RCANFD_GERFL, 0);
> +}
> +
> +static void rcar_canfd_error(struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	u32 cerfl, csts;
> +	u32 txerr = 0, rxerr = 0;
> +	u32 ch = priv->channel;
> +
> +	/* Propagate the error condition to the CAN stack */
> +	skb = alloc_can_err_skb(ndev, &cf);
> +	if (!skb) {
> +		stats->rx_dropped++;
> +		return;
> +	}
> +
> +	/* Channel error interrupt */
> +	cerfl = rcar_canfd_read(priv, RCANFD_CERFL(ch));
> +	csts = rcar_canfd_read(priv, RCANFD_CSTS(ch));
> +	txerr = RCANFD_CSTS_TECCNT(csts);
> +	rxerr = RCANFD_CSTS_RECCNT(csts);
> +
> +	netdev_dbg(ndev, "ch erfl %x sts %x txerr %u rxerr %u\n",
> +		   cerfl, csts, txerr, rxerr);
> +
> +	if (cerfl & RCANFD_CERFL_BEF) {
> +		netdev_dbg(ndev, "Bus error\n");
> +		cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> +		cf->data[2] = CAN_ERR_PROT_UNSPEC;
> +		priv->can.can_stats.bus_error++;
> +	}
> +	if (cerfl & RCANFD_CERFL_ADEF) {
> +		netdev_dbg(ndev, "ACK Delimiter Error\n");
> +		stats->tx_errors++;
> +		cf->data[3] |= CAN_ERR_PROT_LOC_ACK_DEL;
> +	}
> +	if (cerfl & RCANFD_CERFL_B0EF) {
> +		netdev_dbg(ndev, "Bit Error (dominant)\n");
> +		stats->tx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_BIT0;
> +	}
> +	if (cerfl & RCANFD_CERFL_B1EF) {
> +		netdev_dbg(ndev, "Bit Error (recessive)\n");
> +		stats->tx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_BIT1;
> +	}
> +	if (cerfl & RCANFD_CERFL_CEF) {
> +		netdev_dbg(ndev, "CRC Error\n");
> +		stats->rx_errors++;
> +		cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> +	}
> +	if (cerfl & RCANFD_CERFL_AEF) {
> +		netdev_dbg(ndev, "ACK Error\n");
> +		stats->tx_errors++;
> +		cf->can_id |= CAN_ERR_ACK;
> +		cf->data[3] |= CAN_ERR_PROT_LOC_ACK;
> +	}
> +	if (cerfl & RCANFD_CERFL_FEF) {
> +		netdev_dbg(ndev, "Form Error\n");
> +		stats->rx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_FORM;
> +	}
> +	if (cerfl & RCANFD_CERFL_SEF) {
> +		netdev_dbg(ndev, "Stuff Error\n");
> +		stats->rx_errors++;
> +		cf->data[2] |= CAN_ERR_PROT_STUFF;
> +	}
> +	if (cerfl & RCANFD_CERFL_ALEF) {
> +		netdev_dbg(ndev, "Arbitration lost Error\n");
> +		priv->can.can_stats.arbitration_lost++;
> +		cf->can_id |= CAN_ERR_LOSTARB;
> +		cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
> +	}
> +	if (cerfl & RCANFD_CERFL_BLEF) {
> +		netdev_dbg(ndev, "Bus Lock Error\n");
> +		stats->rx_errors++;
> +		cf->can_id |= CAN_ERR_BUSERROR;
> +	}
> +	if (cerfl & RCANFD_CERFL_EWEF) {
> +		netdev_dbg(ndev, "Error warning interrupt\n");
> +		priv->can.state = CAN_STATE_ERROR_WARNING;
> +		priv->can.can_stats.error_warning++;
> +		cf->can_id |= CAN_ERR_CRTL;
> +		cf->data[1] = txerr > rxerr ? CAN_ERR_CRTL_TX_WARNING :
> +			CAN_ERR_CRTL_RX_WARNING;
> +		cf->data[6] = txerr;
> +		cf->data[7] = rxerr;
> +	}
> +	if (cerfl & RCANFD_CERFL_EPEF) {
> +		netdev_dbg(ndev, "Error passive interrupt\n");
> +		priv->can.state = CAN_STATE_ERROR_PASSIVE;
> +		priv->can.can_stats.error_passive++;
> +		cf->can_id |= CAN_ERR_CRTL;
> +		cf->data[1] = txerr > rxerr ? CAN_ERR_CRTL_TX_PASSIVE :
> +			CAN_ERR_CRTL_RX_PASSIVE;
> +		cf->data[6] = txerr;
> +		cf->data[7] = rxerr;
> +	}
> +	if (cerfl & RCANFD_CERFL_BOEEF) {
> +		netdev_dbg(ndev, "Bus-off entry interrupt\n");
> +		rcar_canfd_tx_failure_cleanup(ndev);
> +		priv->can.state = CAN_STATE_BUS_OFF;
> +		priv->can.can_stats.bus_off++;
> +		can_bus_off(ndev);
> +		cf->can_id |= CAN_ERR_BUSOFF;
> +	}
> +	if (cerfl & RCANFD_CERFL_OLEF) {
> +		netdev_dbg(ndev,
> +			   "Overload Frame Transmission error interrupt\n");
> +		stats->tx_errors++;
> +		cf->can_id |= CAN_ERR_PROT;
> +		cf->data[2] |= CAN_ERR_PROT_OVERLOAD;
> +	}
> +
> +	/* Clear all channel error interrupts */
> +	rcar_canfd_write(priv, RCANFD_CERFL(ch), 0);
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;
> +	netif_rx(skb);
> +}
> +
> +static void rcar_canfd_tx_done(struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	u32 sts;
> +	unsigned long flags;
> +	u32 ch = priv->channel;
> +
> +	do {

You should iterare over all pending CAN frames:

> 	for (/* nix */; (priv->tx_head - priv->tx_tail) > 0; priv->tx_tail++) {

> +		u8 unsent, sent;
> +
> +		sent = priv->tx_tail % RCANFD_FIFO_DEPTH;

and check here, if that packet has really been tramsitted. Exit the loop
otherweise.

> +		stats->tx_packets++;
> +		stats->tx_bytes += priv->tx_len[sent];
> +		priv->tx_len[sent] = 0;
> +		can_get_echo_skb(ndev, sent);
> +
> +		spin_lock_irqsave(&priv->tx_lock, flags);

What does the tx_lock protect? The tx path is per channel, isn't it?

> +		priv->tx_tail++;
> +		sts = rcar_canfd_read(priv,
> +				      RCANFD_CFSTS(ch, RCANFD_CFFIFO_IDX));
> +		unsent = RCANFD_CMFIFO_CFMC(sts);
> +
> +		/* Wake producer only when there is room */
> +		if (unsent != RCANFD_FIFO_DEPTH)
> +			netif_wake_queue(ndev);

Move the netif_wake_queue() out of the loop.

> +
> +		if (priv->tx_head - priv->tx_tail <= unsent) {
> +			spin_unlock_irqrestore(&priv->tx_lock, flags);
> +			break;
> +		}
> +		spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> +	} while (1);
> +
> +	/* Clear interrupt */
> +	rcar_canfd_write(priv, RCANFD_CFSTS(ch, RCANFD_CFFIFO_IDX),
> +			 sts & ~RCANFD_CMFIFO_CFTXIF);
> +	can_led_event(ndev, CAN_LED_EVENT_TX);
> +}
> +
> +static irqreturn_t rcar_canfd_global_interrupt(int irq, void *dev_id)
> +{
> +	struct rcar_canfd_global *gpriv = dev_id;
> +	struct net_device *ndev;
> +	struct rcar_canfd_channel *priv;
> +	u32 sts, gerfl;
> +	u32 ch, ridx;
> +
> +	/* Global error interrupts still indicate a condition specific
> +	 * to a channel. RxFIFO interrupt is a global interrupt.
> +	 */
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +		priv = gpriv->ch[ch];
> +		ndev = priv->ndev;
> +		ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +		/* Global error interrupts */
> +		gerfl = rcar_canfd_read(priv, RCANFD_GERFL);
> +		if (RCANFD_GERFL_ERR(gerfl))
> +			rcar_canfd_global_error(ndev);
> +
> +		/* Handle Rx interrupts */
> +		sts = rcar_canfd_read(priv, RCANFD_RFSTS(ridx));
> +		if (sts & RCANFD_RFFIFO_RFIF) {
> +			if (napi_schedule_prep(&priv->napi)) {
> +				/* Disable Rx FIFO interrupts */
> +				rcar_canfd_clear_bit(priv,
> +						     RCANFD_RFCC(ridx),
> +						     RCANFD_RFFIFO_RFIE);
> +				__napi_schedule(&priv->napi);
> +			}
> +		}
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
> +{
> +	struct rcar_canfd_global *gpriv = dev_id;
> +	struct net_device *ndev;
> +	struct rcar_canfd_channel *priv;
> +	u32 sts, cerfl, ch;
> +
> +	/* Common FIFO is a per channel resource */
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +		priv = gpriv->ch[ch];
> +		ndev = priv->ndev;
> +
> +		/* Channel error interrupts */
> +		cerfl = rcar_canfd_read(priv, RCANFD_CERFL(ch));
> +		if (RCANFD_CERFL_ERR(cerfl))
> +			rcar_canfd_error(ndev);
> +
> +		/* Handle Tx interrupts */
> +		sts = rcar_canfd_read(priv,
> +				      RCANFD_CFSTS(ch, RCANFD_CFFIFO_IDX));
> +		if (sts & RCANFD_CMFIFO_CFTXIF)
> +			rcar_canfd_tx_done(ndev);
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +static void rcar_canfd_set_bittiming(struct net_device *dev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(dev);
> +	const struct can_bittiming *bt = &priv->can.bittiming;
> +	const struct can_bittiming *dbt = &priv->can.data_bittiming;
> +	u16 brp, sjw, tseg1, tseg2;
> +	u32 cfg;
> +	u32 ch = priv->channel;
> +
> +	/* Nominal bit timing settings */
> +	brp = bt->brp - 1;
> +	sjw = bt->sjw - 1;
> +	tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
> +	tseg2 = bt->phase_seg2 - 1;
> +
> +	cfg = (RCANFD_NR_TSEG1(tseg1) | RCANFD_NR_BRP(brp) |
> +	       RCANFD_NR_SJW(sjw) | RCANFD_NR_TSEG2(tseg2));
> +
> +	rcar_canfd_write(priv, RCANFD_CCFG(ch), cfg);
> +	netdev_dbg(priv->ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
> +		   brp, sjw, tseg1, tseg2);
> +
> +	/* Data bit timing settings */
> +	brp = dbt->brp - 1;
> +	sjw = dbt->sjw - 1;
> +	tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
> +	tseg2 = dbt->phase_seg2 - 1;
> +
> +	cfg = (RCANFD_DR_TSEG1(tseg1) | RCANFD_DR_BRP(brp) |
> +	       RCANFD_DR_SJW(sjw) | RCANFD_DR_TSEG2(tseg2));
> +
> +	rcar_canfd_write(priv, RCANFD_F_CDFG(ch), cfg);
> +	netdev_dbg(priv->ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
> +		   brp, sjw, tseg1, tseg2);
> +}
> +
> +static int rcar_canfd_start(struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	int err = -EOPNOTSUPP;
> +	u32 sts, ch = priv->channel;
> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +	/* Ensure channel starts in FD mode */
> +	if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD)) {
> +		netdev_err(ndev, "enable can fd mode for channel %d\n", ch);
> +		goto fail_mode;
> +	}
> +
> +	rcar_canfd_set_bittiming(ndev);
> +
> +	rcar_canfd_enable_channel_interrupts(priv);
> +
> +	/* Set channel to Operational mode */
> +	rcar_canfd_update_bit(priv, RCANFD_CCTR(ch),
> +			      RCANFD_CCTR_MODEMASK, RCANFD_CCTR_COPM);
> +
> +	/* Verify channel mode change */
> +	err = readl_poll_timeout((priv->base + RCANFD_CSTS(ch)), sts,
> +				 (sts & RCANFD_CSTS_COM), 2, 500000);
> +	if (err) {
> +		netdev_err(ndev, "channel %u communication state failed\n", ch);
> +		goto fail_mode_change;
> +	}
> +
> +	/* Enable Common & Rx FIFO */
> +	rcar_canfd_set_bit(priv, RCANFD_CFCC(ch, RCANFD_CFFIFO_IDX),
> +			   RCANFD_CMFIFO_CFE);
> +	rcar_canfd_set_bit(priv, RCANFD_RFCC(ridx), RCANFD_RFFIFO_RFE);
> +
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +	return 0;
> +
> +fail_mode_change:
> +	rcar_canfd_disable_channel_interrupts(priv);
> +fail_mode:
> +	return err;
> +}
> +
> +static int rcar_canfd_open(struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	struct rcar_canfd_global *gpriv = priv->gpriv;
> +	int err;
> +
> +	/* Peripheral clock is already enabled in probe */
> +	err = clk_prepare_enable(gpriv->can_clk);
> +	if (err) {
> +		netdev_err(ndev, "failed to enable CAN clock, error %d\n", err);
> +		goto out_clock;
> +	}
> +
> +	err = open_candev(ndev);
> +	if (err) {
> +		netdev_err(ndev, "open_candev() failed, error %d\n", err);
> +		goto out_can_clock;
> +	}
> +
> +	napi_enable(&priv->napi);
> +	err = rcar_canfd_start(ndev);
> +	if (err)
> +		goto out_close;
> +	netif_start_queue(ndev);
> +	can_led_event(ndev, CAN_LED_EVENT_OPEN);
> +	return 0;
> +out_close:
> +	napi_disable(&priv->napi);
> +	close_candev(ndev);
> +out_can_clock:
> +	clk_disable_unprepare(gpriv->can_clk);
> +out_clock:
> +	return err;
> +}
> +
> +static void rcar_canfd_stop(struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	int err;
> +	u32 sts, ch = priv->channel;
> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +	/* Transition to channel reset mode  */
> +	rcar_canfd_update_bit(priv, RCANFD_CCTR(ch),
> +			      RCANFD_CCTR_MODEMASK, RCANFD_CCTR_CRESET);
> +
> +	/* Check Channel reset mode */
> +	err = readl_poll_timeout((priv->base + RCANFD_CSTS(ch)), sts,
> +				 (sts & RCANFD_CSTS_RESET), 2, 500000);
> +	if (err)
> +		netdev_err(ndev, "channel %u reset failed\n", ch);
> +
> +	rcar_canfd_disable_channel_interrupts(priv);
> +
> +	/* Disable Common & Rx FIFO */
> +	rcar_canfd_clear_bit(priv, RCANFD_CFCC(ch, RCANFD_CFFIFO_IDX),
> +			     RCANFD_CMFIFO_CFE);
> +	rcar_canfd_clear_bit(priv, RCANFD_RFCC(ridx), RCANFD_RFFIFO_RFE);
> +
> +	/* Set the state as STOPPED */
> +	priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +static int rcar_canfd_close(struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	struct rcar_canfd_global *gpriv = priv->gpriv;
> +
> +	netif_stop_queue(ndev);
> +	rcar_canfd_stop(ndev);
> +	napi_disable(&priv->napi);
> +	clk_disable_unprepare(gpriv->can_clk);
> +	close_candev(ndev);
> +	can_led_event(ndev, CAN_LED_EVENT_STOP);
> +	return 0;
> +}
> +
> +static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> +					 struct net_device *ndev)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +	u32 sts, id, ptr;
> +	unsigned long flags;
> +	u32 ch = priv->channel;
> +
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +
> +	if (cf->can_id & CAN_EFF_FLAG) {
> +		id = cf->can_id & CAN_EFF_MASK;
> +		id |= RCANFD_CMFIFO_CFIDE;
> +	} else {
> +		id = cf->can_id & CAN_SFF_MASK;
> +	}
> +
> +	if (cf->can_id & CAN_RTR_FLAG)
> +		id |= RCANFD_CMFIFO_CFRTR;
> +
> +	rcar_canfd_write(priv, RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX),
> +			 id);
> +	ptr = RCANFD_CMFIFO_CFDLC(can_len2dlc(cf->len));

ptr usually means pointer, better call it dlc.

> +	rcar_canfd_write(priv, RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX),
> +			 ptr);
> +
> +	sts = rcar_canfd_read(priv, RCANFD_F_CFFDCSTS(ch, RCANFD_CFFIFO_IDX));
> +	if (can_is_canfd_skb(skb)) {
> +		/* CAN FD frame format */
> +		sts |= RCANFD_CMFIFO_CFFDF;
> +		if (cf->flags & CANFD_BRS)
> +			sts |= RCANFD_CMFIFO_CFBRS;
> +		else
> +			sts &= ~RCANFD_CMFIFO_CFBRS;
> +	} else {
> +		sts &= ~RCANFD_CMFIFO_CFFDF;
> +	}
> +	rcar_canfd_write(priv, RCANFD_F_CFFDCSTS(ch, RCANFD_CFFIFO_IDX), sts);
> +
> +	rcar_canfd_put_data(cf, priv,
> +			    RCANFD_F_CFDF(ch, RCANFD_CFFIFO_IDX, 0));
> +
> +	priv->tx_len[priv->tx_head % RCANFD_FIFO_DEPTH] = cf->len;
> +	can_put_echo_skb(skb, ndev, priv->tx_head % RCANFD_FIFO_DEPTH);
> +
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +	priv->tx_head++;
> +
> +	/* Start Tx: Write 0xff to CFPC to increment the CPU-side
> +	 * pointer for the Common FIFO
> +	 */
> +	rcar_canfd_write(priv, RCANFD_CFPCTR(ch, RCANFD_CFFIFO_IDX), 0xff);
> +
> +	/* Stop the queue if we've filled all FIFO entries */
> +	if (priv->tx_head - priv->tx_tail >= RCANFD_FIFO_DEPTH)
> +		netif_stop_queue(ndev);

Please move the check of stop_queue, before starting the send.

> +
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +	return NETDEV_TX_OK;
> +}
> +
> +static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
> +{
> +	struct net_device_stats *stats = &priv->ndev->stats;
> +	struct canfd_frame *cf;
> +	struct sk_buff *skb;
> +	u32 sts = 0, id, ptr;
> +	u32 ch = priv->channel;
> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +	id = rcar_canfd_read(priv, RCANFD_F_RFID(ridx));
> +	ptr = rcar_canfd_read(priv, RCANFD_F_RFPTR(ridx));
> +
> +	sts = rcar_canfd_read(priv, RCANFD_F_RFFDSTS(ridx));
> +	if (sts & RCANFD_RFFIFO_RFFDF)
> +		skb = alloc_canfd_skb(priv->ndev, &cf);
> +	else
> +		skb = alloc_can_skb(priv->ndev,
> +				    (struct can_frame **)&cf);
> +
> +	if (!skb) {
> +		stats->rx_dropped++;
> +		return;
> +	}
> +
> +	if (sts & RCANFD_RFFIFO_RFFDF)
> +		cf->len = can_dlc2len(RCANFD_RFFIFO_RFDLC(ptr));
> +	else
> +		cf->len = get_can_dlc(RCANFD_RFFIFO_RFDLC(ptr));
> +
> +	if (sts & RCANFD_RFFIFO_RFESI) {
> +		cf->flags |= CANFD_ESI;
> +		netdev_dbg(priv->ndev, "ESI Error\n");
> +	}
> +
> +	if (id & RCANFD_RFFIFO_RFIDE)
> +		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +	else
> +		cf->can_id = id & CAN_SFF_MASK;
> +
> +	if (!(sts & RCANFD_RFFIFO_RFFDF) && (id & RCANFD_RFFIFO_RFRTR)) {
> +		cf->can_id |= CAN_RTR_FLAG;
> +	} else {
> +		if (sts & RCANFD_RFFIFO_RFBRS)
> +			cf->flags |= CANFD_BRS;
> +
> +		rcar_canfd_get_data(cf, priv, RCANFD_F_RFDF(ridx, 0));
> +	}
> +
> +	/* Write 0xff to RFPC to increment the CPU-side
> +	 * pointer of the Rx FIFO
> +	 */
> +	rcar_canfd_write(priv, RCANFD_RFPCTR(ridx), 0xff);
> +
> +	can_led_event(priv->ndev, CAN_LED_EVENT_RX);
> +
> +	stats->rx_bytes += cf->len;
> +	stats->rx_packets++;
> +	netif_receive_skb(skb);
> +}
> +
> +static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
> +{
> +	struct rcar_canfd_channel *priv =
> +		container_of(napi, struct rcar_canfd_channel, napi);
> +	int num_pkts;
> +	u32 sts;
> +	u32 ch = priv->channel;
> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> +
> +	for (num_pkts = 0; num_pkts < quota; num_pkts++) {
> +		sts = rcar_canfd_read(priv, RCANFD_RFSTS(ridx));
> +		/* Clear interrupt bit */
> +		if (sts & RCANFD_RFFIFO_RFIF)
> +			rcar_canfd_write(priv, RCANFD_RFSTS(ridx),
> +					 sts & ~RCANFD_RFFIFO_RFIF);
> +
> +		/* Check FIFO empty condition */
> +		if (sts & RCANFD_RFFIFO_RFEMP)
> +			break;
> +
> +		rcar_canfd_rx_pkt(priv);

This sequence looks strange. You first conditionally ack the interrupt
then you check for empty fifo, then read the CAN frame. I would assume
that you first check if there's a CAN frame, read it and then clear the
interrupt.

> +	}
> +
> +	/* All packets processed */
> +	if (num_pkts < quota) {
> +		napi_complete(napi);
> +		/* Enable Rx FIFO interrupts */
> +		rcar_canfd_set_bit(priv, RCANFD_RFCC(ridx), RCANFD_RFFIFO_RFIE);
> +	}
> +	return num_pkts;
> +}
> +
> +static int rcar_canfd_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +	int err;
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		err = rcar_canfd_start(ndev);
> +		if (err)
> +			return err;
> +		netif_wake_queue(ndev);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int rcar_canfd_get_berr_counter(const struct net_device *dev,
> +				       struct can_berr_counter *bec)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(dev);
> +	u32 val, ch = priv->channel;
> +
> +	/* Peripheral clock is already enabled in probe */
> +	val = rcar_canfd_read(priv, RCANFD_CSTS(ch));
> +	bec->txerr = RCANFD_CSTS_TECCNT(val);
> +	bec->rxerr = RCANFD_CSTS_RECCNT(val);
> +	return 0;
> +}
> +
> +static const struct net_device_ops rcar_canfd_netdev_ops = {
> +	.ndo_open = rcar_canfd_open,
> +	.ndo_stop = rcar_canfd_close,
> +	.ndo_start_xmit = rcar_canfd_start_xmit,
> +	.ndo_change_mtu = can_change_mtu,
> +};
> +
> +static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch)
> +{
> +	struct platform_device *pdev = gpriv->pdev;
> +	struct rcar_canfd_channel *priv;
> +	struct net_device *ndev;
> +	int err = -ENODEV;
> +
> +	ndev = alloc_candev(sizeof(*priv), RCANFD_FIFO_DEPTH);
> +	if (!ndev) {
> +		dev_err(&pdev->dev, "alloc_candev() failed\n");
> +		err = -ENOMEM;
> +		goto fail;
> +	}
> +	priv = netdev_priv(ndev);
> +
> +	ndev->netdev_ops = &rcar_canfd_netdev_ops;
> +	ndev->flags |= IFF_ECHO;
> +	priv->ndev = ndev;
> +	priv->base = gpriv->base;
> +	priv->channel = ch;
> +	priv->can.clock.freq = gpriv->freq;
> +	dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
> +
> +	priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
> +	priv->can.data_bittiming_const =
> +		&rcar_canfd_data_bittiming_const;
> +
> +	/* Controller starts in CAN FD mode, which supports classical CAN and
> +	 * CAN FD frames. For classical CAN frames only configurations, data
> +	 * bitrate should be configured the same as nominal bitrate.
> +	 */
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
> +		CAN_CTRLMODE_FD;
> +
> +	priv->can.do_set_mode = rcar_canfd_do_set_mode;
> +	priv->can.do_get_berr_counter = rcar_canfd_get_berr_counter;
> +	priv->gpriv = gpriv;
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> +	netif_napi_add(ndev, &priv->napi, rcar_canfd_rx_poll,
> +		       RCANFD_NAPI_WEIGHT);
> +	err = register_candev(ndev);
> +	if (err) {
> +		dev_err(&pdev->dev,
> +			"register_candev() failed, error %d\n", err);
> +		goto fail_candev;
> +	}
> +	spin_lock_init(&priv->tx_lock);
> +	devm_can_led_init(ndev);
> +	gpriv->ch[priv->channel] = priv;
> +	dev_info(&pdev->dev, "device registered (channel %u)\n", priv->channel);
> +	return 0;
> +
> +fail_candev:
> +	netif_napi_del(&priv->napi);
> +	free_candev(ndev);
> +fail:
> +	return err;
> +}
> +
> +static void rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch)
> +{
> +	struct rcar_canfd_channel *priv = gpriv->ch[ch];
> +
> +	if (priv) {
> +		unregister_candev(priv->ndev);
> +		netif_napi_del(&priv->napi);
> +		free_candev(priv->ndev);
> +	}
> +}
> +
> +static int rcar_canfd_probe(struct platform_device *pdev)
> +{
> +	struct resource *mem;
> +	void __iomem *addr;
> +	u32 sts, ch;
> +	struct rcar_canfd_global *gpriv;
> +	struct device_node *of_child;
> +	unsigned long channels_mask = 0;
> +	int err, ch_irq, g_irq;
> +
> +	of_child = of_get_child_by_name(pdev->dev.of_node, "channel0");
> +	if (of_child && of_device_is_available(of_child))
> +		channels_mask |= BIT(0);	/* Channel 0 */
> +
> +	of_child = of_get_child_by_name(pdev->dev.of_node, "channel1");
> +	if (of_child && of_device_is_available(of_child))
> +		channels_mask |= BIT(1);	/* Channel 1 */
> +
> +	ch_irq = platform_get_irq(pdev, 0);
> +	if (ch_irq < 0) {
> +		dev_err(&pdev->dev, "no Channel IRQ resource\n");
> +		err = ch_irq;
> +		goto fail_dev;
> +	}
> +
> +	g_irq = platform_get_irq(pdev, 1);
> +	if (g_irq < 0) {
> +		dev_err(&pdev->dev, "no Global IRQ resource\n");
> +		err = g_irq;
> +		goto fail_dev;
> +	}
> +
> +	/* Global controller context */
> +	gpriv = devm_kzalloc(&pdev->dev, sizeof(*gpriv), GFP_KERNEL);
> +	if (!gpriv) {
> +		err = -ENOMEM;
> +		goto fail_dev;
> +	}
> +	gpriv->pdev = pdev;
> +	gpriv->channels_mask = channels_mask;
> +
> +	/* Peripheral clock */
> +	gpriv->clkp = devm_clk_get(&pdev->dev, "fck");
> +	if (IS_ERR(gpriv->clkp)) {
> +		err = PTR_ERR(gpriv->clkp);
> +		dev_err(&pdev->dev, "cannot get peripheral clock, error %d\n",
> +			err);
> +		goto fail_dev;
> +	}
> +
> +	/* fCAN clock: Pick External clock. If not available fallback to
> +	 * CANFD clock
> +	 */
> +	gpriv->can_clk = devm_clk_get(&pdev->dev, "can_clk");
> +	if (IS_ERR(gpriv->can_clk)) {
> +		gpriv->can_clk = devm_clk_get(&pdev->dev, "canfd");
> +		if (IS_ERR(gpriv->can_clk)) {
> +			err = PTR_ERR(gpriv->can_clk);
> +			dev_err(&pdev->dev,
> +				"cannot get canfd clock, error %d\n", err);
> +			goto fail_dev;
> +		}
> +		gpriv->clock_select = RCANFD_CANFDCLK;
> +
> +	} else {
> +		gpriv->clock_select = RCANFD_EXTCLK;
> +	}
> +	gpriv->freq = clk_get_rate(gpriv->can_clk);
> +
> +	if (gpriv->clock_select == RCANFD_CANFDCLK)
> +		/* CANFD clock is further divided by (1/2) within the IP */
> +		gpriv->freq /= 2;
> +
> +	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_dev;
> +	}
> +	gpriv->base = addr;
> +
> +	/* Request IRQ that's common for both channels */
> +	err = devm_request_irq(&pdev->dev, ch_irq,
> +			       rcar_canfd_channel_interrupt, 0,
> +			       "canfd.chn", gpriv);
> +	if (err) {
> +		dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> +			ch_irq, err);
> +		goto fail_dev;
> +	}
> +	err = devm_request_irq(&pdev->dev, g_irq,
> +			       rcar_canfd_global_interrupt, 0,
> +			       "canfd.gbl", gpriv);
> +	if (err) {
> +		dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> +			g_irq, err);
> +		goto fail_dev;
> +	}
> +
> +	/* Enable peripheral clock for register access */
> +	err = clk_prepare_enable(gpriv->clkp);
> +	if (err) {
> +		dev_err(&pdev->dev,
> +			"failed to enable peripheral clock, error %d\n", err);
> +		goto fail_dev;
> +	}
> +
> +	err = rcar_canfd_reset_controller(gpriv);
> +	if (err) {
> +		dev_err(&pdev->dev, "reset controller failed\n");
> +		goto fail_clk;
> +	}
> +
> +	/* Controller in Global reset & Channel reset mode */
> +	rcar_canfd_configure_controller(gpriv);
> +
> +	/* Configure per channel attributes */
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +		/* Configure Channel's Rx fifo */
> +		rcar_canfd_configure_rx(gpriv, ch);
> +
> +		/* Configure Channel's Tx (Common) fifo */
> +		rcar_canfd_configure_tx(gpriv, ch);
> +
> +		/* Configure receive rules */
> +		rcar_canfd_configure_afl_rules(gpriv, ch);
> +	}
> +
> +	/* Configure common interrupts */
> +	rcar_canfd_enable_global_interrupts(gpriv);
> +
> +	/* Start Global operation mode */
> +	rcar_canfd_update_bit(gpriv, RCANFD_GCTR, RCANFD_GCTR_MODEMASK,
> +			      RCANFD_GCTR_GOPM);
> +
> +	/* Verify mode change */
> +	err = readl_poll_timeout((gpriv->base + RCANFD_GSTS), sts,
> +				 !(sts & RCANFD_GSTS_GNOPM), 2, 500000);
> +	if (err) {
> +		dev_err(&pdev->dev, "global operational mode failed\n");
> +		goto fail_mode;
> +	}
> +
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +		err = rcar_canfd_channel_probe(gpriv, ch);
> +		if (err)
> +			goto fail_channel;
> +	}

Should the CAN IP core be clocked the whole time? What about shuting
down the clock and enabling it on ifup?

> +
> +	platform_set_drvdata(pdev, gpriv);
> +	dev_info(&pdev->dev, "global operational state (clk %d)\n",
> +		 gpriv->clock_select);
> +	return 0;
> +
> +fail_channel:
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS)
> +		rcar_canfd_channel_remove(gpriv, ch);
> +fail_mode:
> +	rcar_canfd_disable_global_interrupts(gpriv);
> +fail_clk:
> +	clk_disable_unprepare(gpriv->clkp);
> +fail_dev:
> +	return err;
> +}
> +
> +static int rcar_canfd_remove(struct platform_device *pdev)
> +{
> +	struct rcar_canfd_global *gpriv = platform_get_drvdata(pdev);
> +	struct rcar_canfd_channel *priv;
> +	u32 ch;
> +
> +	rcar_canfd_reset_controller(gpriv);
> +	rcar_canfd_disable_global_interrupts(gpriv);
> +
> +	for_each_set_bit(ch, &gpriv->channels_mask, RCANFD_NUM_CHANNELS) {
> +		priv = gpriv->ch[ch];
> +		if (priv) {

This should always be true.

> +			rcar_canfd_disable_channel_interrupts(priv);
> +			unregister_candev(priv->ndev);
> +			netif_napi_del(&priv->napi);
> +			free_candev(priv->ndev);

Please make use of rcar_canfd_channel_remove(), as you already have the
function.

> +		}
> +	}
> +
> +	/* Enter global sleep mode */
> +	rcar_canfd_set_bit(gpriv, RCANFD_GCTR, RCANFD_GCTR_SLPR);
> +	clk_disable_unprepare(gpriv->clkp);
> +	return 0;
> +}
> +
> +static int __maybe_unused rcar_canfd_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int __maybe_unused rcar_canfd_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
> +			 rcar_canfd_resume);
> +
> +static const struct of_device_id rcar_canfd_of_table[] = {
> +	{ .compatible = "renesas,rcar-gen3-canfd" },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, rcar_canfd_of_table);
> +
> +static struct platform_driver rcar_canfd_driver = {
> +	.driver = {
> +		.name = RCANFD_DRV_NAME,
> +		.of_match_table = of_match_ptr(rcar_canfd_of_table),
> +		.pm = &rcar_canfd_pm_ops,
> +	},
> +	.probe = rcar_canfd_probe,
> +	.remove = rcar_canfd_remove,
> +};
> +
> +module_platform_driver(rcar_canfd_driver);
> +
> +MODULE_AUTHOR("Ramesh Shanmugasundaram <[email-masked]>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CAN FD driver for Renesas R-Car SoC");
> +MODULE_ALIAS("platform:" RCANFD_DRV_NAME);

regards,
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   |