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

Marc Kleine-Budde
2016-03-31T20:51:20Z

[-- Attachment #1.1: Type: text/plain, Size: 62051 bytes --]

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 <ramesh.shanmugasundaram@bp.renesas.com>
> ---
> 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 <ramesh.shanmugasundaram@bp.renesas.com>");
> +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 |

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