[PATCH v2 0/4] CAST Controller Area Network driver support

[PATCH v2 0/4] CAST Controller Area Network driver support

[Hal Feng]

This patchset adds support for the CAST Controller Area Network Bus
Controller (version fd-7x10N00S00) which is used in StarFive JH7110 SoC.
Note that the CAN FD license for JH7110 has expired, so JH7110 only
supports CAN CC now.

Changes since v1:
Patch 1:
- Add company information in the commit message.
Patch 2:
- Add description for the hardware.
- Move "allOf" stuff down after the property definitions.
- Rename compatible names, clock names, reset names and syscon register
  names.
- Rewrite the example.
Patch 3:
- Reorder all functions for readability.
- Simplify register definitions and register access functions.
- Improve syscon related code.
- Use clk_bulk interface.
- Enable the clocks during .ndo_open() and disable during .ndo_stop().
- Use can_put_echo_skb() and can_get_echo_skb().
- Stop the TX queue when entering .ndo_start_xmit() and restart the TX
  queue after the transmission finished.
- Simplify logic and remove redundant code.
- Improve coding style.
Patch 4:
- Update the nodes according to the new dt-bindings.

History:
v1: https://lore.kernel.org/all/20240129031239.17037-1-william.qiu@starfivetech.com/

William Qiu (4):
  dt-bindings: vendor-prefixes: Add cast vendor prefix
  dt-bindings: can: Add CAST CAN Bus Controller
  can: Add driver for CAST CAN Bus Controller
  riscv: dts: starfive: jh7110: Add CAN nodes

 .../bindings/net/can/cast,can-ctrl.yaml       | 106 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   8 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  32 +
 drivers/net/can/Kconfig                       |   7 +
 drivers/net/can/Makefile                      |   1 +
 drivers/net/can/cast_can.c                    | 936 ++++++++++++++++++
 7 files changed, 1092 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
 create mode 100644 drivers/net/can/cast_can.c


base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
-- 
2.43.2

[PATCH v2 1/4] dt-bindings: vendor-prefixes: Add cast vendor prefix

[Hal Feng]

From: William Qiu <william.qiu@starfivetech.com>

CAST (Computer-Aided Software Technologies, Inc.) is a company developing,
selling, and supporting digital Silicon IP Cores for ASICs or FPGAs.

https://www.cast-inc.com/

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index a70ce43b3dc0..9bfb0156bc8e 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -256,6 +256,8 @@ patternProperties:
     description: Capella Microsystems, Inc
   "^cascoda,.*":
     description: Cascoda, Ltd.
+  "^cast,.*":
+    description: Computer-Aided Software Technologies, Inc.
   "^catalyst,.*":
     description: Catalyst Semiconductor, Inc.
   "^cavium,.*":
-- 
2.43.2

[PATCH v2 2/4] dt-bindings: can: Add CAST CAN Bus Controller

[Hal Feng]

From: William Qiu <william.qiu@starfivetech.com>

Add bindings for CAST CAN Bus Controller.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../bindings/net/can/cast,can-ctrl.yaml       | 106 ++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml

diff --git a/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml b/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
new file mode 100644
index 000000000000..2870cff80164
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/cast,can-ctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CAST CAN Bus Controller
+
+description:
+  This CAN Bus Controller, also called CAN-CTRL, implements a highly
+  featured and reliable CAN bus controller that performs serial
+  communication according to the CAN protocol.
+
+  The CAN-CTRL comes in three variants, they are CC, FD, and XL.
+  The CC variant supports only Classical CAN, the FD variant adds support
+  for CAN FD, and the XL variant supports the Classical CAN, CAN FD, and
+  CAN XL standards.
+
+maintainers:
+  - William Qiu <william.qiu@starfivetech.com>
+  - Hal Feng <hal.feng@starfivetech.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - starfive,jh7110-can
+      - const: cast,can-ctrl-fd-7x10N00S00
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 3
+
+  clock-names:
+    items:
+      - const: apb
+      - const: timer
+      - const: core
+
+  resets:
+    minItems: 3
+
+  reset-names:
+    items:
+      - const: apb
+      - const: timer
+      - const: core
+
+  starfive,syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to System Register Controller syscon node
+          - description: offset of SYS_SYSCONSAIF__SYSCFG register for CAN controller
+          - description: shift of SYS_SYSCONSAIF__SYSCFG register for CAN controller
+          - description: mask of SYS_SYSCONSAIF__SYSCFG register for CAN controller
+    description:
+      Should be four parameters, the phandle to System Register Controller
+      syscon node and the offset/shift/mask of SYS_SYSCONSAIF__SYSCFG register
+      for CAN controller.
+
+allOf:
+  - $ref: can-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-can
+    then:
+      required:
+        - starfive,syscon
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+  - |
+    can@130d0000{
+        compatible = "starfive,jh7110-can", "cast,can-ctrl-fd-7x10N00S00";
+        reg = <0x130d0000 0x1000>;
+        interrupts = <112>;
+        clocks = <&syscrg 115>,
+                 <&syscrg 116>,
+                 <&syscrg 117>;
+        clock-names = "apb", "timer", "core";
+        resets = <&syscrg 111>,
+                 <&syscrg 113>,
+                 <&syscrg 112>;
+        reset-names = "apb", "timer", "core";
+        starfive,syscon = <&sys_syscon 0x10 0x3 0x8>;
+    };
+
+...
-- 
2.43.2

Re: [PATCH v2 2/4] dt-bindings: can: Add CAST CAN Bus Controller

[Rob Herring (Arm)]

On Sun, 22 Sep 2024 22:51:48 +0800, Hal Feng wrote:
> From: William Qiu <william.qiu@starfivetech.com>
> 
> Add bindings for CAST CAN Bus Controller.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../bindings/net/can/cast,can-ctrl.yaml       | 106 ++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml:27:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240922145151.130999-3-hal.feng@starfivetech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 2/4] dt-bindings: can: Add CAST CAN Bus Controller

[Rob Herring]

On Sun, Sep 22, 2024 at 10:51:48PM +0800, Hal Feng wrote:
> From: William Qiu <william.qiu@starfivetech.com>
> 
> Add bindings for CAST CAN Bus Controller.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../bindings/net/can/cast,can-ctrl.yaml       | 106 ++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml b/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> new file mode 100644
> index 000000000000..2870cff80164
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> @@ -0,0 +1,106 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/can/cast,can-ctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CAST CAN Bus Controller
> +
> +description:
> +  This CAN Bus Controller, also called CAN-CTRL, implements a highly
> +  featured and reliable CAN bus controller that performs serial
> +  communication according to the CAN protocol.
> +
> +  The CAN-CTRL comes in three variants, they are CC, FD, and XL.
> +  The CC variant supports only Classical CAN, the FD variant adds support
> +  for CAN FD, and the XL variant supports the Classical CAN, CAN FD, and
> +  CAN XL standards.
> +
> +maintainers:
> +  - William Qiu <william.qiu@starfivetech.com>
> +  - Hal Feng <hal.feng@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - starfive,jh7110-can
> +      - const: cast,can-ctrl-fd-7x10N00S00

What's the 7x10...? Perhaps some explanation on it.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: apb
> +      - const: timer
> +      - const: core
> +
> +  resets:
> +    minItems: 3
> +
> +  reset-names:
> +    items:
> +      - const: apb
> +      - const: timer
> +      - const: core
> +
> +  starfive,syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to System Register Controller syscon node
> +          - description: offset of SYS_SYSCONSAIF__SYSCFG register for CAN controller
> +          - description: shift of SYS_SYSCONSAIF__SYSCFG register for CAN controller
> +          - description: mask of SYS_SYSCONSAIF__SYSCFG register for CAN controller
> +    description:
> +      Should be four parameters, the phandle to System Register Controller
> +      syscon node and the offset/shift/mask of SYS_SYSCONSAIF__SYSCFG register
> +      for CAN controller.

This just repeats what the schema says. More useful would be what you 
need to access/control in this register.

Rob

[PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Hal Feng]

From: William Qiu <william.qiu@starfivetech.com>

Add driver for CAST CAN Bus Controller used on
StarFive JH7110 SoC.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 MAINTAINERS                |   8 +
 drivers/net/can/Kconfig    |   7 +
 drivers/net/can/Makefile   |   1 +
 drivers/net/can/cast_can.c | 936 +++++++++++++++++++++++++++++++++++++
 4 files changed, 952 insertions(+)
 create mode 100644 drivers/net/can/cast_can.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cc40a9d9b8cd..9313b1a69e48 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5010,6 +5010,14 @@ S:	Maintained
 W:	https://wireless.wiki.kernel.org/en/users/Drivers/carl9170
 F:	drivers/net/wireless/ath/carl9170/
 
+CAST CAN DRIVER
+M:	William Qiu <william.qiu@starfivetech.com>
+M:	Hal Feng <hal.feng@starfivetech.com>
+L:	linux-can@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
+F:	drivers/net/can/cast_can.c
+
 CAVIUM I2C DRIVER
 M:	Robert Richter <rric@kernel.org>
 S:	Odd Fixes
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 7f9b60a42d29..a7ae8be5876f 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -124,6 +124,13 @@ config CAN_CAN327
 
 	  If this driver is built as a module, it will be called can327.
 
+config CAN_CASTCAN
+	tristate "CAST CAN"
+	depends on ARCH_STARFIVE || COMPILE_TEST
+	depends on COMMON_CLK && HAS_IOMEM
+	help
+	  CAST CAN driver. This driver supports both CAN and CANFD IP.
+
 config CAN_FLEXCAN
 	tristate "Support for Freescale FLEXCAN based chips"
 	depends on OF || COLDFIRE || COMPILE_TEST
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 4669cd51e7bf..2f1ebd7c0efe 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -17,6 +17,7 @@ obj-y				+= softing/
 obj-$(CONFIG_CAN_AT91)		+= at91_can.o
 obj-$(CONFIG_CAN_BXCAN)		+= bxcan.o
 obj-$(CONFIG_CAN_CAN327)	+= can327.o
+obj-$(CONFIG_CAN_CASTCAN)	+= cast_can.o
 obj-$(CONFIG_CAN_CC770)		+= cc770/
 obj-$(CONFIG_CAN_C_CAN)		+= c_can/
 obj-$(CONFIG_CAN_CTUCANFD)	+= ctucanfd/
diff --git a/drivers/net/can/cast_can.c b/drivers/net/can/cast_can.c
new file mode 100644
index 000000000000..020a2eaa236b
--- /dev/null
+++ b/drivers/net/can/cast_can.c
@@ -0,0 +1,936 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CAST Controller Area Network Bus Controller Driver
+ *
+ * Copyright (c) 2022-2024 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/skbuff.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#define DRIVER_NAME "cast_can"
+
+enum ccan_reg {
+	CCAN_RUBF	= 0x00,	/* Receive Buffer Registers 0x00-0x4f */
+	CCAN_RUBF_ID	= 0x00,
+	CCAN_RBUF_CTL	= 0x04,
+	CCAN_RBUF_DATA	= 0x08,
+	CCAN_TBUF	= 0x50,	/* Transmit Buffer Registers 0x50-0x97 */
+	CCAN_TBUF_ID	= 0x50,
+	CCAN_TBUF_CTL	= 0x54,
+	CCAN_TBUF_DATA	= 0x58,
+	CCAN_TTS	= 0x98,	/* Transmission Time Stamp 0x98-0x9f */
+	CCAN_CFG_STAT	= 0xa0,
+	CCAN_TCMD	= 0xa1,
+	CCAN_TCTRL	= 0xa2,
+	CCAN_RCTRL	= 0xa3,
+	CCAN_RTIE	= 0xa4,
+	CCAN_RTIF	= 0xa5,
+	CCAN_ERRINT	= 0xa6,
+	CCAN_LIMIT	= 0xa7,
+	CCAN_S_SEG_1	= 0xa8,
+	CCAN_S_SEG_2	= 0xa9,
+	CCAN_S_SJW	= 0xaa,
+	CCAN_S_PRESC	= 0xab,
+	CCAN_F_SEG_1	= 0xac,
+	CCAN_F_SEG_2	= 0xad,
+	CCAN_F_SJW	= 0xae,
+	CCAN_F_PRESC	= 0xaf,
+	CCAN_EALCAP	= 0xb0,
+	CCAN_RECNT	= 0xb2,
+	CCAN_TECNT	= 0xb3,
+};
+
+enum ccan_reg_bit_mask {
+	CCAN_RST_MASK		  = BIT(7), /* Set Reset Bit */
+	CCAN_FULLCAN_MASK	  = BIT(4),
+	CCAN_FIFO_MASK		  = BIT(5),
+	CCAN_TSONE_MASK		  = BIT(2),
+	CCAN_TSALL_MASK		  = BIT(1),
+	CCAN_LBMEMOD_MASK	  = BIT(6), /* Set loopback external mode */
+	CCAN_LBMIMOD_MASK	  = BIT(5), /* Set loopback internal mode */
+	CCAN_BUSOFF_MASK	  = BIT(0),
+	CCAN_TTSEN_MASK		  = BIT(7),
+	CCAN_BRS_MASK		  = BIT(4), /* CAN-FD Bit Rate Switch mask */
+	CCAN_EDL_MASK		  = BIT(5), /* Extended Data Length */
+	CCAN_DLC_MASK		  = GENMASK(3, 0),
+	CCAN_TENEXT_MASK	  = BIT(6),
+	CCAN_IDE_MASK		  = BIT(7),
+	CCAN_RTR_MASK		  = BIT(6),
+	CCAN_INTR_ALL_MASK	  = GENMASK(7, 0), /* All interrupts enable mask */
+	CCAN_RIE_MASK		  = BIT(7),
+	CCAN_RFIE_MASK		  = BIT(5),
+	CCAN_RAFIE_MASK		  = BIT(4),
+	CCAN_EIE_MASK		  = BIT(1),
+	CCAN_TASCTIVE_MASK	  = BIT(1),
+	CCAN_RASCTIVE_MASK	  = BIT(2),
+	CCAN_TBSEL_MASK		  = BIT(7), /* Message writen in STB */
+	CCAN_STBY_MASK		  = BIT(5),
+	CCAN_TPE_MASK		  = BIT(4), /* Transmit primary enable */
+	CCAN_TPA_MASK		  = BIT(3),
+	CCAN_SACK_MASK		  = BIT(7),
+	CCAN_RREL_MASK		  = BIT(4),
+	CCAN_RSTAT_NOT_EMPTY_MASK = GENMASK(1, 0),
+	CCAN_RIF_MASK		  = BIT(7),
+	CCAN_RAFIF_MASK		  = BIT(4),
+	CCAN_RFIF_MASK		  = BIT(5),
+	CCAN_TPIF_MASK		  = BIT(3), /* Transmission Primary Interrupt Flag */
+	CCAN_TSIF_MASK		  = BIT(2),
+	CCAN_EIF_MASK		  = BIT(1),
+	CCAN_AIF_MASK		  = BIT(0),
+	CCAN_EWARN_MASK		  = BIT(7),
+	CCAN_EPASS_MASK		  = BIT(6),
+	CCAN_EPIE_MASK		  = BIT(5),
+	CCAN_EPIF_MASK		  = BIT(4),
+	CCAN_ALIE_MASK		  = BIT(3),
+	CCAN_ALIF_MASK		  = BIT(2),
+	CCAN_BEIE_MASK		  = BIT(1),
+	CCAN_BEIF_MASK		  = BIT(0),
+	CCAN_AFWL_MASK		  = BIT(6),
+	CCAN_EWL_MASK		  = (BIT(3) | GENMASK(1, 0)),
+	CCAN_KOER_MASK		  = GENMASK(7, 5),
+	CCAN_BIT_ERROR_MASK	  = BIT(5),
+	CCAN_FORM_ERROR_MASK	  = BIT(6),
+	CCAN_STUFF_ERROR_MASK	  = GENMASK(6, 5),
+	CCAN_ACK_ERROR_MASK	  = BIT(7),
+	CCAN_CRC_ERROR_MASK	  = (BIT(7) | BIT(5)),
+	CCAN_OTH_ERROR_MASK	  = GENMASK(7, 6),
+};
+
+/* CCAN_S/F_SEG_1 bitfield shift */
+#define SEG_1_SHIFT		0
+#define SEG_2_SHIFT		8
+#define SJW_SHIFT		16
+#define PRESC_SHIFT		24
+
+enum cast_can_type {
+	CAST_CAN_TYPE_CAN = 0,
+	CAST_CAN_TYPE_CANFD,
+};
+
+struct ccan_priv {
+	struct can_priv can;
+	struct napi_struct napi;
+	struct device *dev;
+	void __iomem *reg_base;
+	struct clk_bulk_data clks[3];
+	struct reset_control *resets;
+	u32 cantype;
+};
+
+struct cast_can_data {
+	enum cast_can_type cantype;
+	const struct can_bittiming_const *bittime_const;
+	int (*syscon_update)(struct ccan_priv *priv);
+};
+
+static struct can_bittiming_const ccan_bittiming_const = {
+	.name = DRIVER_NAME,
+	.tseg1_min = 2,
+	.tseg1_max = 16,
+	.tseg2_min = 2,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 256,
+	.brp_inc = 1,
+};
+
+static struct can_bittiming_const ccan_bittiming_const_canfd = {
+	.name = DRIVER_NAME,
+	.tseg1_min = 2,
+	.tseg1_max = 64,
+	.tseg2_min = 2,
+	.tseg2_max = 16,
+	.sjw_max = 16,
+	.brp_min = 1,
+	.brp_max = 256,
+	.brp_inc = 1,
+};
+
+static struct can_bittiming_const ccan_data_bittiming_const_canfd = {
+	.name = DRIVER_NAME,
+	.tseg1_min = 1,
+	.tseg1_max = 16,
+	.tseg2_min = 2,
+	.tseg2_max = 8,
+	.sjw_max = 8,
+	.brp_min = 1,
+	.brp_max = 256,
+	.brp_inc = 1,
+};
+
+static inline u32 ccan_read_reg(const struct ccan_priv *priv, u8 reg)
+{
+	return ioread32(priv->reg_base + reg);
+}
+
+static inline void ccan_write_reg(const struct ccan_priv *priv, u8 reg, u32 value)
+{
+	iowrite32(value, priv->reg_base + reg);
+}
+
+static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
+				    enum ccan_reg reg)
+{
+	u8 reg_down;
+	union val {
+		u8 val_8[4];
+		u32 val_32;
+	} val;
+
+	reg_down = ALIGN_DOWN(reg, 4);
+	val.val_32 = ccan_read_reg(priv, reg_down);
+	return val.val_8[reg - reg_down];
+}
+
+static inline void ccan_write_reg_8bit(const struct ccan_priv *priv,
+				       enum ccan_reg reg, u8 value)
+{
+	u8 reg_down;
+	union val {
+		u8 val_8[4];
+		u32 val_32;
+	} val;
+
+	reg_down = ALIGN_DOWN(reg, 4);
+	val.val_32 = ccan_read_reg(priv, reg_down);
+	val.val_8[reg - reg_down] = value;
+	ccan_write_reg(priv, reg_down, val.val_32);
+}
+
+static void ccan_reg_set_bits(const struct ccan_priv *priv,
+			      enum ccan_reg reg,
+			      enum ccan_reg_bit_mask bits)
+{
+	u8 val;
+
+	val = ccan_read_reg_8bit(priv, reg);
+	val |= bits;
+	ccan_write_reg_8bit(priv, reg, val);
+}
+
+static void ccan_reg_clear_bits(const struct ccan_priv *priv,
+				enum ccan_reg reg,
+				enum ccan_reg_bit_mask bits)
+{
+	u8 val;
+
+	val = ccan_read_reg_8bit(priv, reg);
+	val &= ~bits;
+	ccan_write_reg_8bit(priv, reg, val);
+}
+
+static void ccan_set_reset_mode(struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+
+	ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
+}
+
+static int ccan_bittime_configuration(struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	struct can_bittiming *dbt = &priv->can.data_bittiming;
+	u32 bittiming, data_bittiming;
+	u8 reset_test;
+
+	reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
+
+	if (!(reset_test & CCAN_RST_MASK)) {
+		netdev_alert(ndev, "Not in reset mode, cannot set bit timing\n");
+		return -EPERM;
+	}
+
+	/* Check the bittime parameter */
+	if ((((int)(bt->phase_seg1 + bt->prop_seg + 1) - 2) < 0) ||
+	    (((int)(bt->phase_seg2) - 1) < 0) ||
+	    (((int)(bt->sjw) - 1) < 0) ||
+	    (((int)(bt->brp) - 1) < 0))
+		return -EINVAL;
+
+	bittiming = ((bt->phase_seg1 + bt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
+			 ((bt->phase_seg2 - 1) << SEG_2_SHIFT) |
+			 ((bt->sjw - 1) << SJW_SHIFT) |
+			 ((bt->brp - 1) << PRESC_SHIFT);
+
+	ccan_write_reg(priv, CCAN_S_SEG_1, bittiming);
+
+	if (priv->cantype == CAST_CAN_TYPE_CANFD) {
+		if ((((int)(dbt->phase_seg1 + dbt->prop_seg + 1) - 2) < 0) ||
+		    (((int)(dbt->phase_seg2) - 1) < 0) ||
+		    (((int)(dbt->sjw) - 1) < 0) ||
+		    (((int)(dbt->brp) - 1) < 0))
+			return -EINVAL;
+
+		data_bittiming = ((dbt->phase_seg1 + dbt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
+				 ((dbt->phase_seg2 - 1) << SEG_2_SHIFT) |
+				 ((dbt->sjw - 1) << SJW_SHIFT) |
+				 ((dbt->brp - 1) << PRESC_SHIFT);
+
+		ccan_write_reg(priv, CCAN_F_SEG_1, data_bittiming);
+	}
+
+	ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
+
+	netdev_dbg(ndev, "Slow bit rate: %08x\n", ccan_read_reg(priv, CCAN_S_SEG_1));
+	netdev_dbg(ndev, "Fast bit rate: %08x\n", ccan_read_reg(priv, CCAN_F_SEG_1));
+
+	return 0;
+}
+
+static int ccan_chip_start(struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+	int err;
+
+	ccan_set_reset_mode(ndev);
+
+	err = ccan_bittime_configuration(ndev);
+	if (err) {
+		netdev_err(ndev, "Bittime setting failed!\n");
+		return err;
+	}
+
+	/* Set Almost Full Warning Limit */
+	ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_AFWL_MASK);
+
+	/* Programmable Error Warning Limit = (EWL+1)*8. Set EWL=11->Error Warning=96 */
+	ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_EWL_MASK);
+
+	/* Interrupts enable */
+	ccan_write_reg_8bit(priv, CCAN_RTIE, CCAN_INTR_ALL_MASK);
+
+	/* Error Interrupts enable(Error Passive and Bus Error) */
+	ccan_reg_set_bits(priv, CCAN_ERRINT, CCAN_EPIE_MASK);
+
+	/* Check whether it is loopback mode or normal mode */
+	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
+		ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_LBMIMOD_MASK);
+	else
+		ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_LBMEMOD_MASK | CCAN_LBMIMOD_MASK);
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	return 0;
+}
+
+static int ccan_do_set_mode(struct net_device *ndev, enum can_mode mode)
+{
+	int ret;
+
+	switch (mode) {
+	case CAN_MODE_START:
+		ret = ccan_chip_start(ndev);
+		if (ret) {
+			netdev_err(ndev, "Could not start CAN device !\n");
+			return ret;
+		}
+		netif_wake_queue(ndev);
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+	return ret;
+}
+
+static void ccan_tx_interrupt(struct net_device *ndev, u8 isr)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+
+	/* wait till transmission of the PTB or STB finished */
+	while (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
+		if (isr & CCAN_TPIF_MASK)
+			ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TPIF_MASK);
+
+		if (isr & CCAN_TSIF_MASK)
+			ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TSIF_MASK);
+
+		isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
+	}
+
+	ndev->stats.tx_bytes += can_get_echo_skb(ndev, 0, NULL);
+	ndev->stats.tx_packets++;
+	netif_wake_queue(ndev);
+}
+
+static void ccan_rxfull_interrupt(struct net_device *ndev, u8 isr)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+
+	if (isr & CCAN_RAFIF_MASK)
+		ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK);
+
+	if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK))
+		ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
+}
+
+static enum can_state ccan_get_chip_status(struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+	u8 can_stat, eir;
+
+	can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
+	eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
+
+	if (can_stat & CCAN_BUSOFF_MASK)
+		return CAN_STATE_BUS_OFF;
+
+	if (eir & CCAN_EPASS_MASK)
+		return CAN_STATE_ERROR_PASSIVE;
+
+	if (eir & CCAN_EWARN_MASK)
+		return CAN_STATE_ERROR_WARNING;
+
+	return CAN_STATE_ERROR_ACTIVE;
+}
+
+static void ccan_error_interrupt(struct net_device *ndev, u8 isr, u8 eir)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u8 koer, recnt = 0, tecnt = 0, can_stat = 0;
+
+	skb = alloc_can_err_skb(ndev, &cf);
+
+	koer = ccan_read_reg_8bit(priv, CCAN_EALCAP) & CCAN_KOER_MASK;
+	recnt = ccan_read_reg_8bit(priv, CCAN_RECNT);
+	tecnt = ccan_read_reg_8bit(priv, CCAN_TECNT);
+
+	/* Read CAN status */
+	can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
+
+	/* Bus off ---> active error mode */
+	if ((isr & CCAN_EIF_MASK) && priv->can.state == CAN_STATE_BUS_OFF)
+		priv->can.state = ccan_get_chip_status(ndev);
+
+	/* State selection */
+	if (can_stat & CCAN_BUSOFF_MASK) {
+		priv->can.state = ccan_get_chip_status(ndev);
+		priv->can.can_stats.bus_off++;
+		ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_BUSOFF_MASK);
+		can_bus_off(ndev);
+		if (skb)
+			cf->can_id |= CAN_ERR_BUSOFF;
+	} else if (eir & CCAN_EPASS_MASK) {
+		priv->can.state = ccan_get_chip_status(ndev);
+		priv->can.can_stats.error_passive++;
+		if (skb) {
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] |= (recnt > 127) ? CAN_ERR_CRTL_RX_PASSIVE : 0;
+			cf->data[1] |= (tecnt > 127) ? CAN_ERR_CRTL_TX_PASSIVE : 0;
+			cf->data[6] = tecnt;
+			cf->data[7] = recnt;
+		}
+	} else if (eir & CCAN_EWARN_MASK) {
+		priv->can.state = ccan_get_chip_status(ndev);
+		priv->can.can_stats.error_warning++;
+		if (skb) {
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] |= (recnt > 95) ? CAN_ERR_CRTL_RX_WARNING : 0;
+			cf->data[1] |= (tecnt > 95) ? CAN_ERR_CRTL_TX_WARNING : 0;
+			cf->data[6] = tecnt;
+			cf->data[7] = recnt;
+		}
+	}
+
+	/* Check for in protocol defined error interrupt */
+	if (eir & CCAN_BEIF_MASK) {
+		if (skb)
+			cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
+
+		if (koer == CCAN_BIT_ERROR_MASK) {
+			stats->tx_errors++;
+			if (skb)
+				cf->data[2] = CAN_ERR_PROT_BIT;
+		} else if (koer == CCAN_FORM_ERROR_MASK) {
+			stats->rx_errors++;
+			if (skb)
+				cf->data[2] = CAN_ERR_PROT_FORM;
+		} else if (koer == CCAN_STUFF_ERROR_MASK) {
+			stats->rx_errors++;
+			if (skb)
+				cf->data[3] = CAN_ERR_PROT_STUFF;
+		} else if (koer == CCAN_ACK_ERROR_MASK) {
+			stats->tx_errors++;
+			if (skb)
+				cf->data[2] = CAN_ERR_PROT_LOC_ACK;
+		} else if (koer == CCAN_CRC_ERROR_MASK) {
+			stats->rx_errors++;
+			if (skb)
+				cf->data[2] = CAN_ERR_PROT_LOC_CRC_SEQ;
+		}
+		priv->can.can_stats.bus_error++;
+	}
+
+	if (skb) {
+		stats->rx_packets++;
+		stats->rx_bytes += cf->can_dlc;
+		netif_rx(skb);
+	}
+
+	netdev_dbg(ndev, "Recnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_RECNT));
+	netdev_dbg(ndev, "Tecnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_TECNT));
+}
+
+static irqreturn_t ccan_interrupt(int irq, void *dev_id)
+{
+	struct net_device *ndev = (struct net_device *)dev_id;
+	struct ccan_priv *priv = netdev_priv(ndev);
+	u8 isr, eir;
+	u8 isr_handled = 0, eir_handled = 0;
+
+	/* Read the value of interrupt status register */
+	isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
+
+	/* Read the value of error interrupt register */
+	eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
+
+	/* Check for Tx interrupt and processing it */
+	if (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
+		ccan_tx_interrupt(ndev, isr);
+		isr_handled |= (CCAN_TPIF_MASK | CCAN_TSIF_MASK);
+	}
+
+	if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK)) {
+		ccan_rxfull_interrupt(ndev, isr);
+		isr_handled |= (CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
+	}
+
+	/* Check Rx interrupt and processing the receive interrupt routine */
+	if (isr & CCAN_RIF_MASK) {
+		ccan_reg_clear_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
+		ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RIF_MASK);
+
+		napi_schedule(&priv->napi);
+		isr_handled |= CCAN_RIF_MASK;
+	}
+
+	if ((isr & CCAN_EIF_MASK) | (eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK))) {
+		/* Reset EPIF and BEIF. Reset EIF */
+		ccan_reg_set_bits(priv, CCAN_ERRINT, eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK));
+		ccan_reg_set_bits(priv, CCAN_RTIF, isr & CCAN_EIF_MASK);
+
+		ccan_error_interrupt(ndev, isr, eir);
+
+		isr_handled |= CCAN_EIF_MASK;
+		eir_handled |= (CCAN_EPIF_MASK | CCAN_BEIF_MASK);
+	}
+
+	if (isr_handled == 0 && eir_handled == 0) {
+		netdev_err(ndev, "Unhandled interrupt!\n");
+		return IRQ_NONE;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ccan_open(struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
+	if (ret) {
+		netdev_err(ndev, "Failed to enable CAN clocks\n");
+		return ret;
+	}
+
+	/* Set chip into reset mode */
+	ccan_set_reset_mode(ndev);
+
+	/* Common open */
+	ret = open_candev(ndev);
+	if (ret)
+		goto clk_exit;
+
+	/* Register interrupt handler */
+	ret = devm_request_irq(priv->dev, ndev->irq, ccan_interrupt, IRQF_SHARED,
+			       ndev->name, ndev);
+	if (ret) {
+		netdev_err(ndev, "Request_irq err: %d\n", ret);
+		goto candev_exit;
+	}
+
+	ret = ccan_chip_start(ndev);
+	if (ret) {
+		netdev_err(ndev, "Could not start CAN device !\n");
+		goto candev_exit;
+	}
+
+	napi_enable(&priv->napi);
+	netif_start_queue(ndev);
+
+	return 0;
+
+candev_exit:
+	close_candev(ndev);
+clk_exit:
+	clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
+	return ret;
+}
+
+static int ccan_close(struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+
+	netif_stop_queue(ndev);
+	napi_disable(&priv->napi);
+
+	ccan_set_reset_mode(ndev);
+	priv->can.state = CAN_STATE_STOPPED;
+
+	close_candev(ndev);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
+
+	return 0;
+}
+
+static netdev_tx_t ccan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	u32 id, ctl, addr_off = CCAN_TBUF_DATA;
+	int i;
+
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+
+	netif_stop_queue(ndev);
+
+	/* Work in XMIT_PTB mode */
+	ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_TBSEL_MASK);
+
+	ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_STBY_MASK);
+
+	id = cf->can_id & ((cf->can_id & CAN_EFF_FLAG) ? CAN_EFF_MASK : CAN_SFF_MASK);
+
+	ctl = can_fd_len2dlc(cf->len);
+	ctl = (cf->can_id & CAN_EFF_FLAG) ? (ctl | CCAN_IDE_MASK) : (ctl & ~CCAN_IDE_MASK);
+
+	if (priv->cantype == CAST_CAN_TYPE_CANFD && can_is_canfd_skb(skb)) {
+		ctl |= (cf->flags & CANFD_BRS) ? (CCAN_BRS_MASK | CCAN_EDL_MASK) : CCAN_EDL_MASK;
+
+		for (i = 0; i < cf->len; i += 4) {
+			ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + i)));
+			addr_off += 4;
+		}
+	} else {
+		ctl &= ~(CCAN_EDL_MASK | CCAN_BRS_MASK);
+
+		if (cf->can_id & CAN_RTR_FLAG) {
+			ctl |= CCAN_RTR_MASK;
+		} else {
+			ctl &= ~CCAN_RTR_MASK;
+			ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + 0)));
+			ccan_write_reg(priv, addr_off + 4, *((u32 *)(cf->data + 4)));
+		}
+	}
+
+	ccan_write_reg(priv, CCAN_TBUF_ID, id);
+	ccan_write_reg(priv, CCAN_TBUF_CTL, ctl);
+	ccan_reg_set_bits(priv, CCAN_TCMD, CCAN_TPE_MASK);
+
+	can_put_echo_skb(skb, ndev, 0, 0);
+
+	return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops ccan_netdev_ops = {
+	.ndo_open = ccan_open,
+	.ndo_stop = ccan_close,
+	.ndo_start_xmit = ccan_start_xmit,
+	.ndo_change_mtu = can_change_mtu,
+};
+
+static int ccan_rx(struct net_device *ndev)
+{
+	struct ccan_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &ndev->stats;
+	struct canfd_frame *cf_fd;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u32 can_id;
+	u8 dlc, control;
+	int i;
+
+	control = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL);
+	can_id = ccan_read_reg(priv, CCAN_RUBF_ID);
+	dlc = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL) & CCAN_DLC_MASK;
+
+	if (control & CCAN_EDL_MASK)
+		/* allocate sk_buffer for canfd frame */
+		skb = alloc_canfd_skb(ndev, &cf_fd);
+	else
+		/* allocate sk_buffer for can frame */
+		skb = alloc_can_skb(ndev, &cf);
+
+	if (!skb) {
+		stats->rx_dropped++;
+		return 0;
+	}
+
+	/* Change the CANFD or CAN2.0 data into socketcan data format */
+	if (control & CCAN_EDL_MASK)
+		cf_fd->len = can_fd_dlc2len(dlc);
+	else
+		cf->can_dlc = can_cc_dlc2len(dlc);
+
+	/* Change the CANFD or CAN2.0 id into socketcan id format */
+	if (control & CCAN_EDL_MASK) {
+		cf_fd->can_id = can_id;
+		cf_fd->can_id = (control & CCAN_IDE_MASK) ? (cf_fd->can_id | CAN_EFF_FLAG) :
+			     (cf_fd->can_id & ~CAN_EFF_FLAG);
+	} else {
+		cf->can_id = can_id;
+		cf->can_id = (control & CCAN_IDE_MASK) ? (cf->can_id | CAN_EFF_FLAG) :
+			     (cf->can_id & ~CAN_EFF_FLAG);
+	}
+
+	if (!(control & CCAN_EDL_MASK))
+		if (control & CCAN_RTR_MASK)
+			cf->can_id |= CAN_RTR_FLAG;
+
+	if (control & CCAN_EDL_MASK) {
+		for (i = 0; i < cf_fd->len; i += 4)
+			*((u32 *)(cf_fd->data + i)) = ccan_read_reg(priv, CCAN_RBUF_DATA + i);
+	} else {
+		/* skb reads the received datas, if the RTR bit not set */
+		if (!(control & CCAN_RTR_MASK)) {
+			*((u32 *)(cf->data + 0)) = ccan_read_reg(priv, CCAN_RBUF_DATA);
+			*((u32 *)(cf->data + 4)) = ccan_read_reg(priv, CCAN_RBUF_DATA + 4);
+		}
+	}
+
+	ccan_reg_set_bits(priv, CCAN_RCTRL, CCAN_RREL_MASK);
+
+	stats->rx_bytes += (control & CCAN_EDL_MASK) ? cf_fd->len : cf->can_dlc;
+	stats->rx_packets++;
+	netif_receive_skb(skb);
+
+	return 1;
+}
+
+static int ccan_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct net_device *ndev = napi->dev;
+	struct ccan_priv *priv = netdev_priv(ndev);
+	int work_done = 0;
+	u8 rx_status = 0;
+
+	rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
+
+	/* Clear receive interrupt and deal with all the received frames */
+	while ((rx_status & CCAN_RSTAT_NOT_EMPTY_MASK) && (work_done < quota)) {
+		work_done += ccan_rx(ndev);
+
+		rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
+	}
+
+	napi_complete(napi);
+	ccan_reg_set_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
+
+	return work_done;
+}
+
+static int ccan_driver_probe(struct platform_device *pdev)
+{
+	struct net_device *ndev;
+	struct ccan_priv *priv;
+	const struct cast_can_data *ddata;
+	void __iomem *addr;
+	int ret;
+
+	addr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(addr)) {
+		ret = PTR_ERR(addr);
+		goto exit;
+	}
+
+	ddata = of_device_get_match_data(&pdev->dev);
+	if (!ddata)
+		return -ENODEV;
+
+	ndev = alloc_candev(sizeof(struct ccan_priv), 1);
+	if (!ndev) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+
+	priv = netdev_priv(ndev);
+	priv->dev = &pdev->dev;
+	priv->cantype = ddata->cantype;
+	priv->can.bittiming_const = ddata->bittime_const;
+
+	if (ddata->syscon_update) {
+		ret = ddata->syscon_update(priv);
+		if (ret)
+			goto free_exit;
+	}
+
+	priv->clks[0].id = "apb";
+	priv->clks[1].id = "timer";
+	priv->clks[2].id = "core";
+
+	ret = devm_clk_bulk_get(&pdev->dev, ARRAY_SIZE(priv->clks), priv->clks);
+	if (ret) {
+		ret = dev_err_probe(&pdev->dev, ret, "Failed to get CAN clocks\n");
+		goto free_exit;
+	}
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
+	if (ret) {
+		ret = dev_err_probe(&pdev->dev, ret, "Failed to enable CAN clocks\n");
+		goto free_exit;
+	}
+
+	priv->resets = devm_reset_control_array_get_exclusive(&pdev->dev);
+	if (IS_ERR(priv->resets)) {
+		ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->resets),
+				    "Failed to get CAN resets");
+		goto clk_exit;
+	}
+
+	ret = reset_control_deassert(priv->resets);
+	if (ret)
+		goto clk_exit;
+
+	if (priv->cantype == CAST_CAN_TYPE_CANFD) {
+		priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
+		priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
+	} else {
+		priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
+	}
+
+	priv->reg_base = addr;
+	priv->can.clock.freq = clk_get_rate(priv->clks[2].clk);
+	priv->can.do_set_mode = ccan_do_set_mode;
+	ndev->irq = platform_get_irq(pdev, 0);
+
+	/* We support local echo */
+	ndev->flags |= IFF_ECHO;
+	ndev->netdev_ops = &ccan_netdev_ops;
+
+	platform_set_drvdata(pdev, ndev);
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	netif_napi_add_tx_weight(ndev, &priv->napi, ccan_rx_poll, 16);
+	ret = register_candev(ndev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register (err=%d)\n", ret);
+		goto reset_exit;
+	}
+
+	dev_dbg(&pdev->dev, "Driver registered: regs=%p, irp=%d, clock=%d\n",
+		priv->reg_base, ndev->irq, priv->can.clock.freq);
+
+	return 0;
+
+reset_exit:
+	reset_control_assert(priv->resets);
+clk_exit:
+	clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
+free_exit:
+	free_candev(ndev);
+exit:
+	return ret;
+}
+
+static void ccan_driver_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct ccan_priv *priv = netdev_priv(ndev);
+
+	reset_control_assert(priv->resets);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
+
+	unregister_candev(ndev);
+	netif_napi_del(&priv->napi);
+	free_candev(ndev);
+}
+
+static const struct cast_can_data ccan_canfd_data = {
+	.cantype = CAST_CAN_TYPE_CANFD,
+	.bittime_const = &ccan_bittiming_const_canfd,
+};
+
+static int sf_jh7110_syscon_update(struct ccan_priv *priv)
+{
+	struct of_phandle_args args;
+	struct regmap *syscon;
+	u32 syscon_offset, syscon_shift, syscon_mask, regval;
+	int ret;
+
+	ret = of_parse_phandle_with_fixed_args(priv->dev->of_node,
+					       "starfive,syscon", 3, 0, &args);
+	if (ret) {
+		dev_err(priv->dev, "Failed to parse starfive,syscon\n");
+		return -EINVAL;
+	}
+
+	syscon = syscon_node_to_regmap(args.np);
+	of_node_put(args.np);
+	if (IS_ERR(syscon))
+		return PTR_ERR(syscon);
+
+	syscon_offset = args.args[0];
+	syscon_shift  = args.args[1];
+	syscon_mask   = args.args[2];
+
+	/* Enable can2.0/canfd function */
+	regval = priv->cantype << syscon_shift;
+	ret = regmap_update_bits(syscon, syscon_offset, syscon_mask, regval);
+
+	return ret;
+}
+
+static const struct cast_can_data sf_jh7110_can_data = {
+	.cantype = CAST_CAN_TYPE_CAN,
+	.bittime_const = &ccan_bittiming_const,
+	.syscon_update = sf_jh7110_syscon_update,
+};
+
+static const struct of_device_id ccan_of_match[] = {
+	{ .compatible = "cast,can-ctrl-fd-7x10N00S00", .data = &ccan_canfd_data },
+	{ .compatible = "starfive,jh7110-can", .data = &sf_jh7110_can_data },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, ccan_of_match);
+
+static struct platform_driver ccan_driver = {
+	.probe          = ccan_driver_probe,
+	.remove         = ccan_driver_remove,
+	.driver = {
+		.name  = DRIVER_NAME,
+		.of_match_table = ccan_of_match,
+	},
+};
+module_platform_driver(ccan_driver);
+
+MODULE_DESCRIPTION("CAST CAN Bus Controller Driver");
+MODULE_AUTHOR("Fraunhofer IPMS");
+MODULE_AUTHOR("William Qiu <william.qiu@starfivetech.com>");
+MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
+MODULE_LICENSE("GPL");
-- 
2.43.2

Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Andrew Lunn]

> +static inline u32 ccan_read_reg(const struct ccan_priv *priv, u8 reg)
> +{
> +	return ioread32(priv->reg_base + reg);
> +}
> +
> +static inline void ccan_write_reg(const struct ccan_priv *priv, u8 reg, u32 value)
> +{
> +	iowrite32(value, priv->reg_base + reg);
> +}

No inline functions in .c files please. Let the compiler decide.

> +static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
> +				    enum ccan_reg reg)
> +{
> +	u8 reg_down;
> +	union val {
> +		u8 val_8[4];
> +		u32 val_32;
> +	} val;
> +
> +	reg_down = ALIGN_DOWN(reg, 4);
> +	val.val_32 = ccan_read_reg(priv, reg_down);
> +	return val.val_8[reg - reg_down];

There is an ioread8(). Is it invalid to do a byte read for this
hardware? If so, it is probably worth a comment.

> +static int ccan_bittime_configuration(struct net_device *ndev)
> +{
> +	struct ccan_priv *priv = netdev_priv(ndev);
> +	struct can_bittiming *bt = &priv->can.bittiming;
> +	struct can_bittiming *dbt = &priv->can.data_bittiming;
> +	u32 bittiming, data_bittiming;
> +	u8 reset_test;
> +
> +	reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> +
> +	if (!(reset_test & CCAN_RST_MASK)) {
> +		netdev_alert(ndev, "Not in reset mode, cannot set bit timing\n");
> +		return -EPERM;
> +	}


You don't see nedev_alert() used very often. If this is fatal then
netdev_err().

Also, EPERM? man 3 errno say:

       EPERM           Operation not permitted (POSIX.1-2001).

Why is this a permission issue?

> +static void ccan_tx_interrupt(struct net_device *ndev, u8 isr)
> +{
> +	struct ccan_priv *priv = netdev_priv(ndev);
> +
> +	/* wait till transmission of the PTB or STB finished */
> +	while (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
> +		if (isr & CCAN_TPIF_MASK)
> +			ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TPIF_MASK);
> +
> +		if (isr & CCAN_TSIF_MASK)
> +			ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TSIF_MASK);
> +
> +		isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
> +	}

Potentially endless loops like this are a bad idea. If the firmware
crashes, you are never getting out of here. Please use one of the
macros from iopoll.h

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

dev_id is a void *, so you don't need the cast.

	Andrew

RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Hal Feng]

> On 23.09.24 00:34, Andrew Lunn wrote: 
> > +static inline u32 ccan_read_reg(const struct ccan_priv *priv, u8 reg)
> > +{
> > +	return ioread32(priv->reg_base + reg); }
> > +
> > +static inline void ccan_write_reg(const struct ccan_priv *priv, u8
> > +reg, u32 value) {
> > +	iowrite32(value, priv->reg_base + reg); }
> 
> No inline functions in .c files please. Let the compiler decide.

OK. Drop them.

> 
> > +static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
> > +				    enum ccan_reg reg)
> > +{
> > +	u8 reg_down;
> > +	union val {
> > +		u8 val_8[4];
> > +		u32 val_32;
> > +	} val;
> > +
> > +	reg_down = ALIGN_DOWN(reg, 4);
> > +	val.val_32 = ccan_read_reg(priv, reg_down);
> > +	return val.val_8[reg - reg_down];
> 
> There is an ioread8(). Is it invalid to do a byte read for this hardware? If so, it is
> probably worth a comment.

The hardware has been initially developed as peripheral component for 8 bit systems
and therefore control and status registers defined as 8 bit groups. Nevertheless
the hardware is designed as a 32 bit component finally. It prefers 32-bit read/write
interfaces. I will add a comment later.

> 
> > +static int ccan_bittime_configuration(struct net_device *ndev) {
> > +	struct ccan_priv *priv = netdev_priv(ndev);
> > +	struct can_bittiming *bt = &priv->can.bittiming;
> > +	struct can_bittiming *dbt = &priv->can.data_bittiming;
> > +	u32 bittiming, data_bittiming;
> > +	u8 reset_test;
> > +
> > +	reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> > +
> > +	if (!(reset_test & CCAN_RST_MASK)) {
> > +		netdev_alert(ndev, "Not in reset mode, cannot set bit
> timing\n");
> > +		return -EPERM;
> > +	}
> 
> 
> You don't see nedev_alert() used very often. If this is fatal then netdev_err().
> 
> Also, EPERM? man 3 errno say:
> 
>        EPERM           Operation not permitted (POSIX.1-2001).
> 
> Why is this a permission issue?

Will use netdev_err() and return -EWOULDBLOCK instead.

> 
> > +static void ccan_tx_interrupt(struct net_device *ndev, u8 isr) {
> > +	struct ccan_priv *priv = netdev_priv(ndev);
> > +
> > +	/* wait till transmission of the PTB or STB finished */
> > +	while (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
> > +		if (isr & CCAN_TPIF_MASK)
> > +			ccan_reg_set_bits(priv, CCAN_RTIF,
> CCAN_TPIF_MASK);
> > +
> > +		if (isr & CCAN_TSIF_MASK)
> > +			ccan_reg_set_bits(priv, CCAN_RTIF,
> CCAN_TSIF_MASK);
> > +
> > +		isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
> > +	}
> 
> Potentially endless loops like this are a bad idea. If the firmware crashes, you
> are never getting out of here. Please use one of the macros from iopoll.h

Agree with you. Will modify accordingly.

> 
> > +static irqreturn_t ccan_interrupt(int irq, void *dev_id) {
> > +	struct net_device *ndev = (struct net_device *)dev_id;
> 
> dev_id is a void *, so you don't need the cast.

OK, drop it.

Thanks for you review.

Best regards,
Hal

Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Andrew Lunn]

> > > +	reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> > > +
> > > +	if (!(reset_test & CCAN_RST_MASK)) {
> > > +		netdev_alert(ndev, "Not in reset mode, cannot set bit
> > timing\n");
> > > +		return -EPERM;
> > > +	}
> > 
> > 
> > You don't see nedev_alert() used very often. If this is fatal then netdev_err().
> > 
> > Also, EPERM? man 3 errno say:
> > 
> >        EPERM           Operation not permitted (POSIX.1-2001).
> > 
> > Why is this a permission issue?
> 
> Will use netdev_err() and return -EWOULDBLOCK instead.

I'm not sure that is any better.

       EAGAIN          Resource  temporarily unavailable (may be the same value
                       as EWOULDBLOCK) (POSIX.1-2001).

This is generally used when the kernel expects user space to try a
system call again, and it might then work. Is that what you expect
here?

> > > +static irqreturn_t ccan_interrupt(int irq, void *dev_id) {
> > > +	struct net_device *ndev = (struct net_device *)dev_id;
> > 
> > dev_id is a void *, so you don't need the cast.
> 
> OK, drop it.

Please look at the whole patch. There might be other instances where a
void * is used with a cast, which can be removed. This was just the
first i spotted.

	Andrew

Re: RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Marc Kleine-Budde]

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

On 23.09.2024 07:53:24, Hal Feng wrote:
> > > +static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
> > > +				    enum ccan_reg reg)
> > > +{
> > > +	u8 reg_down;
> > > +	union val {
> > > +		u8 val_8[4];
> > > +		u32 val_32;
> > > +	} val;
> > > +
> > > +	reg_down = ALIGN_DOWN(reg, 4);
> > > +	val.val_32 = ccan_read_reg(priv, reg_down);
> > > +	return val.val_8[reg - reg_down];
> > 
> > There is an ioread8(). Is it invalid to do a byte read for this hardware? If so, it is
> > probably worth a comment.
> 
> The hardware has been initially developed as peripheral component for 8 bit systems
> and therefore control and status registers defined as 8 bit groups. Nevertheless
> the hardware is designed as a 32 bit component finally. It prefers 32-bit read/write
> interfaces. I will add a comment later.

As mentioned in my v1 review, you are doing proper u32 accesses.

> > > +static int ccan_bittime_configuration(struct net_device *ndev) {
> > > +	struct ccan_priv *priv = netdev_priv(ndev);
> > > +	struct can_bittiming *bt = &priv->can.bittiming;
> > > +	struct can_bittiming *dbt = &priv->can.data_bittiming;
> > > +	u32 bittiming, data_bittiming;
> > > +	u8 reset_test;
> > > +
> > > +	reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> > > +
> > > +	if (!(reset_test & CCAN_RST_MASK)) {
> > > +		netdev_alert(ndev, "Not in reset mode, cannot set bit
> > timing\n");
> > > +		return -EPERM;
> > > +	}
> > 
> > 
> > You don't see nedev_alert() used very often. If this is fatal then netdev_err().
> > 
> > Also, EPERM? man 3 errno say:
> > 
> >        EPERM           Operation not permitted (POSIX.1-2001).
> > 
> > Why is this a permission issue?
> 
> Will use netdev_err() and return -EWOULDBLOCK instead.

You have a dedicated function to put the IP core into reset
"ccan_set_reset_mode()". If you don't trust you IP core or it needs some
time, add a poll to that function and return an error.

Then there's no need on ccan_bittime_configuration() to check for reset
mode.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Marc Kleine-Budde]

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

On 22.09.2024 22:51:49, Hal Feng wrote:
> From: William Qiu <william.qiu@starfivetech.com>
> 
> Add driver for CAST CAN Bus Controller used on
> StarFive JH7110 SoC.

Have you read me review of the v1 of this series?

https://lore.kernel.org/all/20240129-zone-defame-c5580e596f72-mkl@pengutronix.de/

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Hal Feng]

On 9/23/2024 5:14 AM, Marc Kleine-Budde wrote: 
> On 22.09.2024 22:51:49, Hal Feng wrote:
> > From: William Qiu <william.qiu@starfivetech.com>
> >
> > Add driver for CAST CAN Bus Controller used on StarFive JH7110 SoC.
> 
> Have you read me review of the v1 of this series?
> 
> https://lore.kernel.org/all/20240129-zone-defame-c5580e596f72-
> mkl@pengutronix.de/

Yes, I modify accordingly except using FIELD_GET() / FIELD_PREP(), using
rx_offload helper and the shared interrupt flag. I found FIELD_GET() / FIELD_PREP()
can only be used when the mask is a constant, and the CAN module won't
work normally if I change the interrupt flag to 0. I will try to using rx_offload helper
in the next version.

Best regards,
Hal

Re: RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Marc Kleine-Budde]

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

On 25.10.2024 01:45:30, Hal Feng wrote:
> On 9/23/2024 5:14 AM, Marc Kleine-Budde wrote: 
> > On 22.09.2024 22:51:49, Hal Feng wrote:
> > > From: William Qiu <william.qiu@starfivetech.com>
> > >
> > > Add driver for CAST CAN Bus Controller used on StarFive JH7110 SoC.
> > 
> > Have you read me review of the v1 of this series?
> > 
> > https://lore.kernel.org/all/20240129-zone-defame-c5580e596f72-
> > mkl@pengutronix.de/
> 
> Yes, I modify accordingly except using FIELD_GET() / FIELD_PREP(), using
> rx_offload helper and the shared interrupt flag. I found FIELD_GET() / FIELD_PREP()
> can only be used when the mask is a constant,

Do you have a non constant mask?

> and the CAN module won't
> work normally if I change the interrupt flag to 0.

What do you mean by "won't work normally"?
It makes no sense to claim that you support shared interrupts, but
print an error message, if your IP core had no active interrupt.

> I will try to using rx_offload helper
> in the next version.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Vincent MAILHOL]

Hi Hal,

A few more comments on top of what Andrew already wrote.

On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng@starfivetech.com> wrote:
> From: William Qiu <william.qiu@starfivetech.com>
>
> Add driver for CAST CAN Bus Controller used on
> StarFive JH7110 SoC.
>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  MAINTAINERS                |   8 +
>  drivers/net/can/Kconfig    |   7 +
>  drivers/net/can/Makefile   |   1 +
>  drivers/net/can/cast_can.c | 936 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 952 insertions(+)
>  create mode 100644 drivers/net/can/cast_can.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cc40a9d9b8cd..9313b1a69e48 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5010,6 +5010,14 @@ S:       Maintained
>  W:     https://wireless.wiki.kernel.org/en/users/Drivers/carl9170
>  F:     drivers/net/wireless/ath/carl9170/
>
> +CAST CAN DRIVER
> +M:     William Qiu <william.qiu@starfivetech.com>
> +M:     Hal Feng <hal.feng@starfivetech.com>
> +L:     linux-can@vger.kernel.org
> +S:     Supported
> +F:     Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> +F:     drivers/net/can/cast_can.c
> +
>  CAVIUM I2C DRIVER
>  M:     Robert Richter <rric@kernel.org>
>  S:     Odd Fixes
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 7f9b60a42d29..a7ae8be5876f 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -124,6 +124,13 @@ config CAN_CAN327
>
>           If this driver is built as a module, it will be called can327.
>
> +config CAN_CASTCAN
> +       tristate "CAST CAN"
> +       depends on ARCH_STARFIVE || COMPILE_TEST
> +       depends on COMMON_CLK && HAS_IOMEM
> +       help
> +         CAST CAN driver. This driver supports both CAN and CANFD IP.

Nitpick, maybe briefly mention the module name:

  If built as a module, it will be named cast_can.

>  config CAN_FLEXCAN
>         tristate "Support for Freescale FLEXCAN based chips"
>         depends on OF || COLDFIRE || COMPILE_TEST
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 4669cd51e7bf..2f1ebd7c0efe 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -17,6 +17,7 @@ obj-y                         += softing/
>  obj-$(CONFIG_CAN_AT91)         += at91_can.o
>  obj-$(CONFIG_CAN_BXCAN)                += bxcan.o
>  obj-$(CONFIG_CAN_CAN327)       += can327.o
> +obj-$(CONFIG_CAN_CASTCAN)      += cast_can.o
>  obj-$(CONFIG_CAN_CC770)                += cc770/
>  obj-$(CONFIG_CAN_C_CAN)                += c_can/
>  obj-$(CONFIG_CAN_CTUCANFD)     += ctucanfd/
> diff --git a/drivers/net/can/cast_can.c b/drivers/net/can/cast_can.c
> new file mode 100644
> index 000000000000..020a2eaa236b
> --- /dev/null
> +++ b/drivers/net/can/cast_can.c
> @@ -0,0 +1,936 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CAST Controller Area Network Bus Controller Driver
> + *
> + * Copyright (c) 2022-2024 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/skbuff.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#define DRIVER_NAME "cast_can"
> +
> +enum ccan_reg {
> +       CCAN_RUBF       = 0x00, /* Receive Buffer Registers 0x00-0x4f */
> +       CCAN_RUBF_ID    = 0x00,
> +       CCAN_RBUF_CTL   = 0x04,
> +       CCAN_RBUF_DATA  = 0x08,
> +       CCAN_TBUF       = 0x50, /* Transmit Buffer Registers 0x50-0x97 */
> +       CCAN_TBUF_ID    = 0x50,
> +       CCAN_TBUF_CTL   = 0x54,
> +       CCAN_TBUF_DATA  = 0x58,
> +       CCAN_TTS        = 0x98, /* Transmission Time Stamp 0x98-0x9f */
> +       CCAN_CFG_STAT   = 0xa0,
> +       CCAN_TCMD       = 0xa1,
> +       CCAN_TCTRL      = 0xa2,
> +       CCAN_RCTRL      = 0xa3,
> +       CCAN_RTIE       = 0xa4,
> +       CCAN_RTIF       = 0xa5,
> +       CCAN_ERRINT     = 0xa6,
> +       CCAN_LIMIT      = 0xa7,
> +       CCAN_S_SEG_1    = 0xa8,
> +       CCAN_S_SEG_2    = 0xa9,
> +       CCAN_S_SJW      = 0xaa,
> +       CCAN_S_PRESC    = 0xab,
> +       CCAN_F_SEG_1    = 0xac,
> +       CCAN_F_SEG_2    = 0xad,
> +       CCAN_F_SJW      = 0xae,
> +       CCAN_F_PRESC    = 0xaf,
> +       CCAN_EALCAP     = 0xb0,
> +       CCAN_RECNT      = 0xb2,
> +       CCAN_TECNT      = 0xb3,
> +};
> +
> +enum ccan_reg_bit_mask {
> +       CCAN_RST_MASK             = BIT(7), /* Set Reset Bit */
> +       CCAN_FULLCAN_MASK         = BIT(4),
> +       CCAN_FIFO_MASK            = BIT(5),
> +       CCAN_TSONE_MASK           = BIT(2),
> +       CCAN_TSALL_MASK           = BIT(1),
> +       CCAN_LBMEMOD_MASK         = BIT(6), /* Set loopback external mode */
> +       CCAN_LBMIMOD_MASK         = BIT(5), /* Set loopback internal mode */
> +       CCAN_BUSOFF_MASK          = BIT(0),
> +       CCAN_TTSEN_MASK           = BIT(7),
> +       CCAN_BRS_MASK             = BIT(4), /* CAN-FD Bit Rate Switch mask */
> +       CCAN_EDL_MASK             = BIT(5), /* Extended Data Length */
> +       CCAN_DLC_MASK             = GENMASK(3, 0),
> +       CCAN_TENEXT_MASK          = BIT(6),
> +       CCAN_IDE_MASK             = BIT(7),
> +       CCAN_RTR_MASK             = BIT(6),
> +       CCAN_INTR_ALL_MASK        = GENMASK(7, 0), /* All interrupts enable mask */
> +       CCAN_RIE_MASK             = BIT(7),
> +       CCAN_RFIE_MASK            = BIT(5),
> +       CCAN_RAFIE_MASK           = BIT(4),
> +       CCAN_EIE_MASK             = BIT(1),
> +       CCAN_TASCTIVE_MASK        = BIT(1),
> +       CCAN_RASCTIVE_MASK        = BIT(2),
> +       CCAN_TBSEL_MASK           = BIT(7), /* Message writen in STB */
                                                         ^^^^^^

Typo: writen -> written

> +       CCAN_STBY_MASK            = BIT(5),
> +       CCAN_TPE_MASK             = BIT(4), /* Transmit primary enable */
> +       CCAN_TPA_MASK             = BIT(3),
> +       CCAN_SACK_MASK            = BIT(7),
> +       CCAN_RREL_MASK            = BIT(4),
> +       CCAN_RSTAT_NOT_EMPTY_MASK = GENMASK(1, 0),
> +       CCAN_RIF_MASK             = BIT(7),
> +       CCAN_RAFIF_MASK           = BIT(4),
> +       CCAN_RFIF_MASK            = BIT(5),
> +       CCAN_TPIF_MASK            = BIT(3), /* Transmission Primary Interrupt Flag */
> +       CCAN_TSIF_MASK            = BIT(2),
> +       CCAN_EIF_MASK             = BIT(1),
> +       CCAN_AIF_MASK             = BIT(0),
> +       CCAN_EWARN_MASK           = BIT(7),
> +       CCAN_EPASS_MASK           = BIT(6),
> +       CCAN_EPIE_MASK            = BIT(5),
> +       CCAN_EPIF_MASK            = BIT(4),
> +       CCAN_ALIE_MASK            = BIT(3),
> +       CCAN_ALIF_MASK            = BIT(2),
> +       CCAN_BEIE_MASK            = BIT(1),
> +       CCAN_BEIF_MASK            = BIT(0),
> +       CCAN_AFWL_MASK            = BIT(6),
> +       CCAN_EWL_MASK             = (BIT(3) | GENMASK(1, 0)),
> +       CCAN_KOER_MASK            = GENMASK(7, 5),
> +       CCAN_BIT_ERROR_MASK       = BIT(5),
> +       CCAN_FORM_ERROR_MASK      = BIT(6),
> +       CCAN_STUFF_ERROR_MASK     = GENMASK(6, 5),
> +       CCAN_ACK_ERROR_MASK       = BIT(7),
> +       CCAN_CRC_ERROR_MASK       = (BIT(7) | BIT(5)),
> +       CCAN_OTH_ERROR_MASK       = GENMASK(7, 6),
> +};
> +
> +/* CCAN_S/F_SEG_1 bitfield shift */
> +#define SEG_1_SHIFT            0
> +#define SEG_2_SHIFT            8
> +#define SJW_SHIFT              16
> +#define PRESC_SHIFT            24
> +
> +enum cast_can_type {
> +       CAST_CAN_TYPE_CAN = 0,
> +       CAST_CAN_TYPE_CANFD,
> +};
> +
> +struct ccan_priv {
> +       struct can_priv can;
> +       struct napi_struct napi;
> +       struct device *dev;
> +       void __iomem *reg_base;
> +       struct clk_bulk_data clks[3];
> +       struct reset_control *resets;
> +       u32 cantype;
> +};
> +
> +struct cast_can_data {
> +       enum cast_can_type cantype;
> +       const struct can_bittiming_const *bittime_const;
> +       int (*syscon_update)(struct ccan_priv *priv);
> +};
> +
> +static struct can_bittiming_const ccan_bittiming_const = {
> +       .name = DRIVER_NAME,
> +       .tseg1_min = 2,
> +       .tseg1_max = 16,
> +       .tseg2_min = 2,
> +       .tseg2_max = 8,
> +       .sjw_max = 4,
> +       .brp_min = 1,
> +       .brp_max = 256,
> +       .brp_inc = 1,
> +};
> +
> +static struct can_bittiming_const ccan_bittiming_const_canfd = {
> +       .name = DRIVER_NAME,
> +       .tseg1_min = 2,
> +       .tseg1_max = 64,
> +       .tseg2_min = 2,
> +       .tseg2_max = 16,
> +       .sjw_max = 16,
> +       .brp_min = 1,
> +       .brp_max = 256,
> +       .brp_inc = 1,
> +};
> +
> +static struct can_bittiming_const ccan_data_bittiming_const_canfd = {
> +       .name = DRIVER_NAME,
> +       .tseg1_min = 1,
> +       .tseg1_max = 16,
> +       .tseg2_min = 2,
> +       .tseg2_max = 8,
> +       .sjw_max = 8,
> +       .brp_min = 1,
> +       .brp_max = 256,
> +       .brp_inc = 1,
> +};
> +
> +static inline u32 ccan_read_reg(const struct ccan_priv *priv, u8 reg)
> +{
> +       return ioread32(priv->reg_base + reg);
> +}
> +
> +static inline void ccan_write_reg(const struct ccan_priv *priv, u8 reg, u32 value)
> +{
> +       iowrite32(value, priv->reg_base + reg);
> +}
> +
> +static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
> +                                   enum ccan_reg reg)
> +{
> +       u8 reg_down;
> +       union val {
> +               u8 val_8[4];
> +               u32 val_32;
> +       } val;
> +
> +       reg_down = ALIGN_DOWN(reg, 4);
> +       val.val_32 = ccan_read_reg(priv, reg_down);
> +       return val.val_8[reg - reg_down];
> +}
> +
> +static inline void ccan_write_reg_8bit(const struct ccan_priv *priv,
> +                                      enum ccan_reg reg, u8 value)
> +{
> +       u8 reg_down;
> +       union val {
> +               u8 val_8[4];
> +               u32 val_32;
> +       } val;
> +
> +       reg_down = ALIGN_DOWN(reg, 4);
> +       val.val_32 = ccan_read_reg(priv, reg_down);
> +       val.val_8[reg - reg_down] = value;
> +       ccan_write_reg(priv, reg_down, val.val_32);
> +}
> +
> +static void ccan_reg_set_bits(const struct ccan_priv *priv,
> +                             enum ccan_reg reg,
> +                             enum ccan_reg_bit_mask bits)
> +{
> +       u8 val;
> +
> +       val = ccan_read_reg_8bit(priv, reg);
> +       val |= bits;
> +       ccan_write_reg_8bit(priv, reg, val);
> +}
> +
> +static void ccan_reg_clear_bits(const struct ccan_priv *priv,
> +                               enum ccan_reg reg,
> +                               enum ccan_reg_bit_mask bits)
> +{
> +       u8 val;
> +
> +       val = ccan_read_reg_8bit(priv, reg);
> +       val &= ~bits;
> +       ccan_write_reg_8bit(priv, reg, val);
> +}
> +
> +static void ccan_set_reset_mode(struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +
> +       ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
> +}
> +
> +static int ccan_bittime_configuration(struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       struct can_bittiming *bt = &priv->can.bittiming;
> +       struct can_bittiming *dbt = &priv->can.data_bittiming;
> +       u32 bittiming, data_bittiming;
> +       u8 reset_test;
> +
> +       reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> +
> +       if (!(reset_test & CCAN_RST_MASK)) {
> +               netdev_alert(ndev, "Not in reset mode, cannot set bit timing\n");
> +               return -EPERM;
> +       }
> +
> +       /* Check the bittime parameter */
> +       if ((((int)(bt->phase_seg1 + bt->prop_seg + 1) - 2) < 0) ||
> +           (((int)(bt->phase_seg2) - 1) < 0) ||
> +           (((int)(bt->sjw) - 1) < 0) ||
> +           (((int)(bt->brp) - 1) < 0))
> +               return -EINVAL;

Here, you are checking for wraparounds. But can this really happen? We
know for sure that:

  - bt->phase_seg1 + bt->prop_seg1 <= ccan_bittiming_const->tseg1_max
  - bt->phase_seg1 >= ccan_bittiming_const->tseg2_min

and so on.

Unless there is a condition under which this issue can show up, remove
the check.

> +       bittiming = ((bt->phase_seg1 + bt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
> +                        ((bt->phase_seg2 - 1) << SEG_2_SHIFT) |
> +                        ((bt->sjw - 1) << SJW_SHIFT) |
> +                        ((bt->brp - 1) << PRESC_SHIFT);
> +
> +       ccan_write_reg(priv, CCAN_S_SEG_1, bittiming);
> +
> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> +               if ((((int)(dbt->phase_seg1 + dbt->prop_seg + 1) - 2) < 0) ||
> +                   (((int)(dbt->phase_seg2) - 1) < 0) ||
> +                   (((int)(dbt->sjw) - 1) < 0) ||
> +                   (((int)(dbt->brp) - 1) < 0))
> +                       return -EINVAL;
> +
> +               data_bittiming = ((dbt->phase_seg1 + dbt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
> +                                ((dbt->phase_seg2 - 1) << SEG_2_SHIFT) |
> +                                ((dbt->sjw - 1) << SJW_SHIFT) |
> +                                ((dbt->brp - 1) << PRESC_SHIFT);

Nitpick: the calculation for the bittiming and the data_bittiming is
the same. Can you factorize this calculation in a helper function?

> +               ccan_write_reg(priv, CCAN_F_SEG_1, data_bittiming);
> +       }
> +
> +       ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
> +
> +       netdev_dbg(ndev, "Slow bit rate: %08x\n", ccan_read_reg(priv, CCAN_S_SEG_1));
> +       netdev_dbg(ndev, "Fast bit rate: %08x\n", ccan_read_reg(priv, CCAN_F_SEG_1));
> +
> +       return 0;
> +}
> +
> +static int ccan_chip_start(struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       int err;
> +
> +       ccan_set_reset_mode(ndev);
> +
> +       err = ccan_bittime_configuration(ndev);
> +       if (err) {
> +               netdev_err(ndev, "Bittime setting failed!\n");
> +               return err;
> +       }
> +
> +       /* Set Almost Full Warning Limit */
> +       ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_AFWL_MASK);
> +
> +       /* Programmable Error Warning Limit = (EWL+1)*8. Set EWL=11->Error Warning=96 */
> +       ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_EWL_MASK);
> +
> +       /* Interrupts enable */
> +       ccan_write_reg_8bit(priv, CCAN_RTIE, CCAN_INTR_ALL_MASK);
> +
> +       /* Error Interrupts enable(Error Passive and Bus Error) */
> +       ccan_reg_set_bits(priv, CCAN_ERRINT, CCAN_EPIE_MASK);
> +
> +       /* Check whether it is loopback mode or normal mode */
> +       if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +               ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_LBMIMOD_MASK);
> +       else
> +               ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_LBMEMOD_MASK | CCAN_LBMIMOD_MASK);
> +
> +       priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +       return 0;
> +}
> +
> +static int ccan_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +       int ret;
> +
> +       switch (mode) {
> +       case CAN_MODE_START:
> +               ret = ccan_chip_start(ndev);
> +               if (ret) {
> +                       netdev_err(ndev, "Could not start CAN device !\n");
> +                       return ret;
> +               }
> +               netif_wake_queue(ndev);
> +               break;
> +       default:
> +               ret = -EOPNOTSUPP;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +static void ccan_tx_interrupt(struct net_device *ndev, u8 isr)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +
> +       /* wait till transmission of the PTB or STB finished */
> +       while (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
> +               if (isr & CCAN_TPIF_MASK)
> +                       ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TPIF_MASK);
> +
> +               if (isr & CCAN_TSIF_MASK)
> +                       ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TSIF_MASK);
> +
> +               isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
> +       }
> +
> +       ndev->stats.tx_bytes += can_get_echo_skb(ndev, 0, NULL);
> +       ndev->stats.tx_packets++;
> +       netif_wake_queue(ndev);
> +}
> +
> +static void ccan_rxfull_interrupt(struct net_device *ndev, u8 isr)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +
> +       if (isr & CCAN_RAFIF_MASK)
> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK);
> +
> +       if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK))
> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
> +}
> +
> +static enum can_state ccan_get_chip_status(struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       u8 can_stat, eir;
> +
> +       can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> +       eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
> +
> +       if (can_stat & CCAN_BUSOFF_MASK)
> +               return CAN_STATE_BUS_OFF;
> +
> +       if (eir & CCAN_EPASS_MASK)
> +               return CAN_STATE_ERROR_PASSIVE;
> +
> +       if (eir & CCAN_EWARN_MASK)
> +               return CAN_STATE_ERROR_WARNING;
> +
> +       return CAN_STATE_ERROR_ACTIVE;
> +}
> +
> +static void ccan_error_interrupt(struct net_device *ndev, u8 isr, u8 eir)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct can_frame *cf;
> +       struct sk_buff *skb;
> +       u8 koer, recnt = 0, tecnt = 0, can_stat = 0;
> +
> +       skb = alloc_can_err_skb(ndev, &cf);
> +
> +       koer = ccan_read_reg_8bit(priv, CCAN_EALCAP) & CCAN_KOER_MASK;
> +       recnt = ccan_read_reg_8bit(priv, CCAN_RECNT);
> +       tecnt = ccan_read_reg_8bit(priv, CCAN_TECNT);
> +
> +       /* Read CAN status */
> +       can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> +
> +       /* Bus off ---> active error mode */
> +       if ((isr & CCAN_EIF_MASK) && priv->can.state == CAN_STATE_BUS_OFF)
> +               priv->can.state = ccan_get_chip_status(ndev);
> +
> +       /* State selection */
> +       if (can_stat & CCAN_BUSOFF_MASK) {
> +               priv->can.state = ccan_get_chip_status(ndev);
> +               priv->can.can_stats.bus_off++;
> +               ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_BUSOFF_MASK);
> +               can_bus_off(ndev);
> +               if (skb)
> +                       cf->can_id |= CAN_ERR_BUSOFF;
> +       } else if (eir & CCAN_EPASS_MASK) {
> +               priv->can.state = ccan_get_chip_status(ndev);
> +               priv->can.can_stats.error_passive++;
> +               if (skb) {
> +                       cf->can_id |= CAN_ERR_CRTL;
> +                       cf->data[1] |= (recnt > 127) ? CAN_ERR_CRTL_RX_PASSIVE : 0;
> +                       cf->data[1] |= (tecnt > 127) ? CAN_ERR_CRTL_TX_PASSIVE : 0;

Use the CAN state thresholds from include/uapi/linux/can/error.h:

  recnt >= CAN_ERROR_PASSIVE_THRESHOLD

and so on.

Replace the ternary operator by an if.

> +                       cf->data[6] = tecnt;
> +                       cf->data[7] = recnt;
> +               }
> +       } else if (eir & CCAN_EWARN_MASK) {
> +               priv->can.state = ccan_get_chip_status(ndev);
> +               priv->can.can_stats.error_warning++;
> +               if (skb) {
> +                       cf->can_id |= CAN_ERR_CRTL;
> +                       cf->data[1] |= (recnt > 95) ? CAN_ERR_CRTL_RX_WARNING : 0;
> +                       cf->data[1] |= (tecnt > 95) ? CAN_ERR_CRTL_TX_WARNING : 0;

Ditto with CAN_ERROR_WARNING_THRESHOLD.

> +                       cf->data[6] = tecnt;
> +                       cf->data[7] = recnt;
> +               }
> +       }
> +
> +       /* Check for in protocol defined error interrupt */
> +       if (eir & CCAN_BEIF_MASK) {
> +               if (skb)
> +                       cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> +
> +               if (koer == CCAN_BIT_ERROR_MASK) {
> +                       stats->tx_errors++;
> +                       if (skb)
> +                               cf->data[2] = CAN_ERR_PROT_BIT;
> +               } else if (koer == CCAN_FORM_ERROR_MASK) {
> +                       stats->rx_errors++;
> +                       if (skb)
> +                               cf->data[2] = CAN_ERR_PROT_FORM;
> +               } else if (koer == CCAN_STUFF_ERROR_MASK) {
> +                       stats->rx_errors++;
> +                       if (skb)
> +                               cf->data[3] = CAN_ERR_PROT_STUFF;
> +               } else if (koer == CCAN_ACK_ERROR_MASK) {
> +                       stats->tx_errors++;
> +                       if (skb)
> +                               cf->data[2] = CAN_ERR_PROT_LOC_ACK;
> +               } else if (koer == CCAN_CRC_ERROR_MASK) {
> +                       stats->rx_errors++;
> +                       if (skb)
> +                               cf->data[2] = CAN_ERR_PROT_LOC_CRC_SEQ;
> +               }
> +               priv->can.can_stats.bus_error++;
> +       }
> +
> +       if (skb) {
> +               stats->rx_packets++;
> +               stats->rx_bytes += cf->can_dlc;

Do not increase the reception statistics for an error frame. Refer to:

  https://git.kernel.org/torvalds/c/676068db69b8

for the reason why.

> +               netif_rx(skb);
> +       }
> +
> +       netdev_dbg(ndev, "Recnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_RECNT));
> +       netdev_dbg(ndev, "Tecnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_TECNT));

Either remove this error message or print something more explicit. The
end-user may not know what a Recnt is. Something like this is better:

  netdev_dbg(ndev, "Rx error count: 0x%02x", ccan_read_reg_8bit(priv,
CCAN_RECNT));

If there are a lot of errors on the but, this can create a lot of
spam. If you decide to keep, encapsulate this in a:

  if (net_ratelimit())

> +}
> +
> +static irqreturn_t ccan_interrupt(int irq, void *dev_id)
> +{
> +       struct net_device *ndev = (struct net_device *)dev_id;
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       u8 isr, eir;
> +       u8 isr_handled = 0, eir_handled = 0;
> +
> +       /* Read the value of interrupt status register */
> +       isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
> +
> +       /* Read the value of error interrupt register */
> +       eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
> +
> +       /* Check for Tx interrupt and processing it */
> +       if (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
> +               ccan_tx_interrupt(ndev, isr);
> +               isr_handled |= (CCAN_TPIF_MASK | CCAN_TSIF_MASK);
> +       }
> +
> +       if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK)) {
> +               ccan_rxfull_interrupt(ndev, isr);
> +               isr_handled |= (CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
> +       }
> +
> +       /* Check Rx interrupt and processing the receive interrupt routine */
> +       if (isr & CCAN_RIF_MASK) {
> +               ccan_reg_clear_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RIF_MASK);
> +
> +               napi_schedule(&priv->napi);
> +               isr_handled |= CCAN_RIF_MASK;
> +       }
> +
> +       if ((isr & CCAN_EIF_MASK) | (eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK))) {
> +               /* Reset EPIF and BEIF. Reset EIF */
> +               ccan_reg_set_bits(priv, CCAN_ERRINT, eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK));
> +               ccan_reg_set_bits(priv, CCAN_RTIF, isr & CCAN_EIF_MASK);
> +
> +               ccan_error_interrupt(ndev, isr, eir);
> +
> +               isr_handled |= CCAN_EIF_MASK;
> +               eir_handled |= (CCAN_EPIF_MASK | CCAN_BEIF_MASK);
> +       }
> +
> +       if (isr_handled == 0 && eir_handled == 0) {
> +               netdev_err(ndev, "Unhandled interrupt!\n");
> +               return IRQ_NONE;
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int ccan_open(struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       int ret;
> +
> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
> +       if (ret) {
> +               netdev_err(ndev, "Failed to enable CAN clocks\n");
> +               return ret;
> +       }
> +
> +       /* Set chip into reset mode */
> +       ccan_set_reset_mode(ndev);
> +
> +       /* Common open */
> +       ret = open_candev(ndev);
> +       if (ret)
> +               goto clk_exit;
> +
> +       /* Register interrupt handler */
> +       ret = devm_request_irq(priv->dev, ndev->irq, ccan_interrupt, IRQF_SHARED,
> +                              ndev->name, ndev);
> +       if (ret) {
> +               netdev_err(ndev, "Request_irq err: %d\n", ret);
> +               goto candev_exit;
> +       }
> +
> +       ret = ccan_chip_start(ndev);
> +       if (ret) {
> +               netdev_err(ndev, "Could not start CAN device !\n");

Nipick: in English punctuation rules, there an no spaces before the
exclamation mark:

  netdev_err(ndev, "Could not start CAN device!\n");

(or you may consider just removing the exclamation mark. No need to be
so emotional for an error message).

> +               goto candev_exit;
> +       }
> +
> +       napi_enable(&priv->napi);
> +       netif_start_queue(ndev);
> +
> +       return 0;
> +
> +candev_exit:
> +       close_candev(ndev);
> +clk_exit:
> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
> +       return ret;
> +}
> +
> +static int ccan_close(struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +
> +       netif_stop_queue(ndev);
> +       napi_disable(&priv->napi);
> +
> +       ccan_set_reset_mode(ndev);
> +       priv->can.state = CAN_STATE_STOPPED;
> +
> +       close_candev(ndev);
> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
> +
> +       return 0;
> +}
> +
> +static netdev_tx_t ccan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +       u32 id, ctl, addr_off = CCAN_TBUF_DATA;
> +       int i;
> +
> +       if (can_dropped_invalid_skb(ndev, skb))
> +               return NETDEV_TX_OK;
> +
> +       netif_stop_queue(ndev);

Does your device only allow sending one frame at a time? Doesn't it
have a transmission queue?

> +       /* Work in XMIT_PTB mode */
> +       ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_TBSEL_MASK);
> +
> +       ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_STBY_MASK);
> +
> +       id = cf->can_id & ((cf->can_id & CAN_EFF_FLAG) ? CAN_EFF_MASK : CAN_SFF_MASK);
> +
> +       ctl = can_fd_len2dlc(cf->len);
> +       ctl = (cf->can_id & CAN_EFF_FLAG) ? (ctl | CCAN_IDE_MASK) : (ctl & ~CCAN_IDE_MASK);
> +
> +       if (priv->cantype == CAST_CAN_TYPE_CANFD && can_is_canfd_skb(skb)) {
> +               ctl |= (cf->flags & CANFD_BRS) ? (CCAN_BRS_MASK | CCAN_EDL_MASK) : CCAN_EDL_MASK;

Replace those long ternary operations with some il/else. It is easier to read.

> +               for (i = 0; i < cf->len; i += 4) {
> +                       ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + i)));
> +                       addr_off += 4;

Nitpick, what about:

  ccan_write_reg(priv, CCAN_TBUF_DATA + i, *((u32 *)(cf->data + i)));

and then remove the declaration of addr_off.

> +               }
> +       } else {
> +               ctl &= ~(CCAN_EDL_MASK | CCAN_BRS_MASK);
> +
> +               if (cf->can_id & CAN_RTR_FLAG) {
> +                       ctl |= CCAN_RTR_MASK;
> +               } else {
> +                       ctl &= ~CCAN_RTR_MASK;
> +                       ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + 0)));
> +                       ccan_write_reg(priv, addr_off + 4, *((u32 *)(cf->data + 4)));

In this else block, addr_off is just an alias of CCAN_TBUF_DATA. So,
directly do:

  ccan_write_reg(priv, CCAN_TBUF_DATA, *((u32 *)(cf->data + 0)));
  ccan_write_reg(priv, CCAN_TBUF_DATA + 4, *((u32 *)(cf->data + 4)));

> +               }
> +       }
> +
> +       ccan_write_reg(priv, CCAN_TBUF_ID, id);
> +       ccan_write_reg(priv, CCAN_TBUF_CTL, ctl);
> +       ccan_reg_set_bits(priv, CCAN_TCMD, CCAN_TPE_MASK);
> +
> +       can_put_echo_skb(skb, ndev, 0, 0);
> +
> +       return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops ccan_netdev_ops = {
> +       .ndo_open = ccan_open,
> +       .ndo_stop = ccan_close,
> +       .ndo_start_xmit = ccan_start_xmit,
> +       .ndo_change_mtu = can_change_mtu,
> +};

Also add a struct ethtool_ops for the default timestamps:

  static const struct ethtool_ops ccan_ethtool_ops = {
          .get_ts_info = ethtool_op_get_ts_info,
  };

This assumes that your device does not support hardware timestamps. If
you do have hardware timestamping support, please adjust accordingly.

> +static int ccan_rx(struct net_device *ndev)
> +{
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct canfd_frame *cf_fd;
> +       struct can_frame *cf;
> +       struct sk_buff *skb;
> +       u32 can_id;
> +       u8 dlc, control;
> +       int i;
> +
> +       control = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL);
> +       can_id = ccan_read_reg(priv, CCAN_RUBF_ID);
> +       dlc = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL) & CCAN_DLC_MASK;
> +
> +       if (control & CCAN_EDL_MASK)
> +               /* allocate sk_buffer for canfd frame */
> +               skb = alloc_canfd_skb(ndev, &cf_fd);
> +       else
> +               /* allocate sk_buffer for can frame */
> +               skb = alloc_can_skb(ndev, &cf);
> +
> +       if (!skb) {
> +               stats->rx_dropped++;
> +               return 0;
> +       }
> +
> +       /* Change the CANFD or CAN2.0 data into socketcan data format */
> +       if (control & CCAN_EDL_MASK)
> +               cf_fd->len = can_fd_dlc2len(dlc);
> +       else
> +               cf->can_dlc = can_cc_dlc2len(dlc);

cf->can_dlc is deprecated. Use cf->len instead.

> +       /* Change the CANFD or CAN2.0 id into socketcan id format */
> +       if (control & CCAN_EDL_MASK) {
> +               cf_fd->can_id = can_id;
> +               cf_fd->can_id = (control & CCAN_IDE_MASK) ? (cf_fd->can_id | CAN_EFF_FLAG) :
> +                            (cf_fd->can_id & ~CAN_EFF_FLAG);
> +       } else {
> +               cf->can_id = can_id;
> +               cf->can_id = (control & CCAN_IDE_MASK) ? (cf->can_id | CAN_EFF_FLAG) :
> +                            (cf->can_id & ~CAN_EFF_FLAG);
> +       }

struct can_frame and struct canfd_frame have overlapping fields. You
can just have one stack variable for both and then, no need for an
if/else here.

> +       if (!(control & CCAN_EDL_MASK))
> +               if (control & CCAN_RTR_MASK)
> +                       cf->can_id |= CAN_RTR_FLAG;
> +
> +       if (control & CCAN_EDL_MASK) {

You are checking for if (!(control & CCAN_EDL_MASK)) and just after
for if (control & CCAN_EDL_MASK). Factorize those two together.

> +               for (i = 0; i < cf_fd->len; i += 4)
> +                       *((u32 *)(cf_fd->data + i)) = ccan_read_reg(priv, CCAN_RBUF_DATA + i);
> +       } else {
> +               /* skb reads the received datas, if the RTR bit not set */
> +               if (!(control & CCAN_RTR_MASK)) {
> +                       *((u32 *)(cf->data + 0)) = ccan_read_reg(priv, CCAN_RBUF_DATA);
> +                       *((u32 *)(cf->data + 4)) = ccan_read_reg(priv, CCAN_RBUF_DATA + 4);
> +               }
> +       }
> +
> +       ccan_reg_set_bits(priv, CCAN_RCTRL, CCAN_RREL_MASK);
> +
> +       stats->rx_bytes += (control & CCAN_EDL_MASK) ? cf_fd->len : cf->can_dlc;

No, cf_fd->len and cf->can_dlc are the same byte (these are parts of
an union). It is just that cf->can_dlc is deprecated and is kept to
not break the UAPI. In the kernel it should never be used.

Here, what you have to do is just to not increase stats->rx_bytes for
RTR frames. Try to factorize this with the other checks which you did
above.

> +       stats->rx_packets++;
> +       netif_receive_skb(skb);
> +
> +       return 1;

Why return 1 on success and 0 on failure? The convention in the kernel
is that 0 means success. If you really want to keep 0 for failure, at
least make this return boolean true or boolean false, but overall, try
to follow the return conventions.

> +}
> +
> +static int ccan_rx_poll(struct napi_struct *napi, int quota)
> +{
> +       struct net_device *ndev = napi->dev;
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +       int work_done = 0;
> +       u8 rx_status = 0;
> +
> +       rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
> +
> +       /* Clear receive interrupt and deal with all the received frames */
> +       while ((rx_status & CCAN_RSTAT_NOT_EMPTY_MASK) && (work_done < quota)) {
> +               work_done += ccan_rx(ndev);
> +
> +               rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
> +       }
> +
> +       napi_complete(napi);
> +       ccan_reg_set_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
> +
> +       return work_done;
> +}
> +
> +static int ccan_driver_probe(struct platform_device *pdev)
> +{
> +       struct net_device *ndev;
> +       struct ccan_priv *priv;
> +       const struct cast_can_data *ddata;
> +       void __iomem *addr;
> +       int ret;
> +
> +       addr = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(addr)) {
> +               ret = PTR_ERR(addr);
> +               goto exit;

No need for goto here:

  return PTR_ERR(addr);

> +       }
> +
> +       ddata = of_device_get_match_data(&pdev->dev);
> +       if (!ddata)
> +               return -ENODEV;
> +
> +       ndev = alloc_candev(sizeof(struct ccan_priv), 1);
> +       if (!ndev) {
> +               ret = -ENOMEM;
> +               goto exit;

Same:

  return -ENOMEM;

(this done, remove the exit label).

> +       }
> +
> +       priv = netdev_priv(ndev);
> +       priv->dev = &pdev->dev;
> +       priv->cantype = ddata->cantype;
> +       priv->can.bittiming_const = ddata->bittime_const;
> +
> +       if (ddata->syscon_update) {
> +               ret = ddata->syscon_update(priv);
> +               if (ret)
> +                       goto free_exit;
> +       }
> +
> +       priv->clks[0].id = "apb";
> +       priv->clks[1].id = "timer";
> +       priv->clks[2].id = "core";
> +
> +       ret = devm_clk_bulk_get(&pdev->dev, ARRAY_SIZE(priv->clks), priv->clks);
> +       if (ret) {
> +               ret = dev_err_probe(&pdev->dev, ret, "Failed to get CAN clocks\n");
> +               goto free_exit;
> +       }
> +
> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
> +       if (ret) {
> +               ret = dev_err_probe(&pdev->dev, ret, "Failed to enable CAN clocks\n");
> +               goto free_exit;
> +       }
> +
> +       priv->resets = devm_reset_control_array_get_exclusive(&pdev->dev);
> +       if (IS_ERR(priv->resets)) {
> +               ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->resets),
> +                                   "Failed to get CAN resets");
> +               goto clk_exit;
> +       }
> +
> +       ret = reset_control_deassert(priv->resets);
> +       if (ret)
> +               goto clk_exit;
> +
> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
> +               priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
> +       } else {
> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> +       }

Nitpick, consider doing this:

  priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
  if (priv->cantype == CAST_CAN_TYPE_CANFD) {
          priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
          priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
  }

Also, does you hardware support dlc greater than 8 (c.f.
CAN_CTRLMODE_CC_LEN8_DLC)?

> +       priv->reg_base = addr;
> +       priv->can.clock.freq = clk_get_rate(priv->clks[2].clk);
> +       priv->can.do_set_mode = ccan_do_set_mode;
> +       ndev->irq = platform_get_irq(pdev, 0);
> +
> +       /* We support local echo */
> +       ndev->flags |= IFF_ECHO;
> +       ndev->netdev_ops = &ccan_netdev_ops;
> +
> +       platform_set_drvdata(pdev, ndev);
> +       SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> +       netif_napi_add_tx_weight(ndev, &priv->napi, ccan_rx_poll, 16);
> +       ret = register_candev(ndev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to register (err=%d)\n", ret);

From the Linux coding style:

  Printing numbers in parentheses (%d) adds no value and should be avoided.

Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#printing-kernel-messages

> +               goto reset_exit;
> +       }
> +
> +       dev_dbg(&pdev->dev, "Driver registered: regs=%p, irp=%d, clock=%d\n",
> +               priv->reg_base, ndev->irq, priv->can.clock.freq);
> +
> +       return 0;
> +
> +reset_exit:
> +       reset_control_assert(priv->resets);
> +clk_exit:
> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
> +free_exit:
> +       free_candev(ndev);
> +exit:
> +       return ret;
> +}
> +
> +static void ccan_driver_remove(struct platform_device *pdev)
> +{
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct ccan_priv *priv = netdev_priv(ndev);
> +
> +       reset_control_assert(priv->resets);
> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
> +
> +       unregister_candev(ndev);
> +       netif_napi_del(&priv->napi);
> +       free_candev(ndev);
> +}
> +
> +static const struct cast_can_data ccan_canfd_data = {
> +       .cantype = CAST_CAN_TYPE_CANFD,
> +       .bittime_const = &ccan_bittiming_const_canfd,
> +};
> +
> +static int sf_jh7110_syscon_update(struct ccan_priv *priv)
> +{
> +       struct of_phandle_args args;
> +       struct regmap *syscon;
> +       u32 syscon_offset, syscon_shift, syscon_mask, regval;
> +       int ret;
> +
> +       ret = of_parse_phandle_with_fixed_args(priv->dev->of_node,
> +                                              "starfive,syscon", 3, 0, &args);
> +       if (ret) {
> +               dev_err(priv->dev, "Failed to parse starfive,syscon\n");
> +               return -EINVAL;
> +       }
> +
> +       syscon = syscon_node_to_regmap(args.np);
> +       of_node_put(args.np);
> +       if (IS_ERR(syscon))
> +               return PTR_ERR(syscon);
> +
> +       syscon_offset = args.args[0];
> +       syscon_shift  = args.args[1];
> +       syscon_mask   = args.args[2];
> +
> +       /* Enable can2.0/canfd function */
> +       regval = priv->cantype << syscon_shift;
> +       ret = regmap_update_bits(syscon, syscon_offset, syscon_mask, regval);
> +
> +       return ret;
> +}
> +
> +static const struct cast_can_data sf_jh7110_can_data = {
> +       .cantype = CAST_CAN_TYPE_CAN,
> +       .bittime_const = &ccan_bittiming_const,
> +       .syscon_update = sf_jh7110_syscon_update,
> +};
> +
> +static const struct of_device_id ccan_of_match[] = {
> +       { .compatible = "cast,can-ctrl-fd-7x10N00S00", .data = &ccan_canfd_data },
> +       { .compatible = "starfive,jh7110-can", .data = &sf_jh7110_can_data },
> +       { /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, ccan_of_match);
> +
> +static struct platform_driver ccan_driver = {
> +       .probe          = ccan_driver_probe,
> +       .remove         = ccan_driver_remove,
> +       .driver = {
> +               .name  = DRIVER_NAME,
> +               .of_match_table = ccan_of_match,
> +       },
> +};
> +module_platform_driver(ccan_driver);
> +
> +MODULE_DESCRIPTION("CAST CAN Bus Controller Driver");
> +MODULE_AUTHOR("Fraunhofer IPMS");
> +MODULE_AUTHOR("William Qiu <william.qiu@starfivetech.com>");
> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
> +MODULE_LICENSE("GPL");


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Hal Feng]

On 9/23/2024 11:41 AM, Vincent MAILHOL wrote:
> Hi Hal,
> 
> A few more comments on top of what Andrew already wrote.
> 
> On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng@starfivetech.com> wrote:
>> From: William Qiu <william.qiu@starfivetech.com>
>>
>> Add driver for CAST CAN Bus Controller used on
>> StarFive JH7110 SoC.
>>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  MAINTAINERS                |   8 +
>>  drivers/net/can/Kconfig    |   7 +
>>  drivers/net/can/Makefile   |   1 +
>>  drivers/net/can/cast_can.c | 936 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 952 insertions(+)
>>  create mode 100644 drivers/net/can/cast_can.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index cc40a9d9b8cd..9313b1a69e48 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5010,6 +5010,14 @@ S:       Maintained
>>  W:     https://wireless.wiki.kernel.org/en/users/Drivers/carl9170
>>  F:     drivers/net/wireless/ath/carl9170/
>>
>> +CAST CAN DRIVER
>> +M:     William Qiu <william.qiu@starfivetech.com>
>> +M:     Hal Feng <hal.feng@starfivetech.com>
>> +L:     linux-can@vger.kernel.org
>> +S:     Supported
>> +F:     Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
>> +F:     drivers/net/can/cast_can.c
>> +
>>  CAVIUM I2C DRIVER
>>  M:     Robert Richter <rric@kernel.org>
>>  S:     Odd Fixes
>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>> index 7f9b60a42d29..a7ae8be5876f 100644
>> --- a/drivers/net/can/Kconfig
>> +++ b/drivers/net/can/Kconfig
>> @@ -124,6 +124,13 @@ config CAN_CAN327
>>
>>           If this driver is built as a module, it will be called can327.
>>
>> +config CAN_CASTCAN
>> +       tristate "CAST CAN"
>> +       depends on ARCH_STARFIVE || COMPILE_TEST
>> +       depends on COMMON_CLK && HAS_IOMEM
>> +       help
>> +         CAST CAN driver. This driver supports both CAN and CANFD IP.
> 
> Nitpick, maybe briefly mention the module name:
> 
>   If built as a module, it will be named cast_can.

OK.

> 
>>  config CAN_FLEXCAN
>>         tristate "Support for Freescale FLEXCAN based chips"
>>         depends on OF || COLDFIRE || COMPILE_TEST
>> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
>> index 4669cd51e7bf..2f1ebd7c0efe 100644
>> --- a/drivers/net/can/Makefile
>> +++ b/drivers/net/can/Makefile
>> @@ -17,6 +17,7 @@ obj-y                         += softing/
>>  obj-$(CONFIG_CAN_AT91)         += at91_can.o
>>  obj-$(CONFIG_CAN_BXCAN)                += bxcan.o
>>  obj-$(CONFIG_CAN_CAN327)       += can327.o
>> +obj-$(CONFIG_CAN_CASTCAN)      += cast_can.o
>>  obj-$(CONFIG_CAN_CC770)                += cc770/
>>  obj-$(CONFIG_CAN_C_CAN)                += c_can/
>>  obj-$(CONFIG_CAN_CTUCANFD)     += ctucanfd/
>> diff --git a/drivers/net/can/cast_can.c b/drivers/net/can/cast_can.c
>> new file mode 100644
>> index 000000000000..020a2eaa236b
>> --- /dev/null
>> +++ b/drivers/net/can/cast_can.c
>> @@ -0,0 +1,936 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * CAST Controller Area Network Bus Controller Driver
>> + *
>> + * Copyright (c) 2022-2024 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/can/dev.h>
>> +#include <linux/can/error.h>
>> +#include <linux/clk.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +#define DRIVER_NAME "cast_can"
>> +
>> +enum ccan_reg {
>> +       CCAN_RUBF       = 0x00, /* Receive Buffer Registers 0x00-0x4f */
>> +       CCAN_RUBF_ID    = 0x00,
>> +       CCAN_RBUF_CTL   = 0x04,
>> +       CCAN_RBUF_DATA  = 0x08,
>> +       CCAN_TBUF       = 0x50, /* Transmit Buffer Registers 0x50-0x97 */
>> +       CCAN_TBUF_ID    = 0x50,
>> +       CCAN_TBUF_CTL   = 0x54,
>> +       CCAN_TBUF_DATA  = 0x58,
>> +       CCAN_TTS        = 0x98, /* Transmission Time Stamp 0x98-0x9f */
>> +       CCAN_CFG_STAT   = 0xa0,
>> +       CCAN_TCMD       = 0xa1,
>> +       CCAN_TCTRL      = 0xa2,
>> +       CCAN_RCTRL      = 0xa3,
>> +       CCAN_RTIE       = 0xa4,
>> +       CCAN_RTIF       = 0xa5,
>> +       CCAN_ERRINT     = 0xa6,
>> +       CCAN_LIMIT      = 0xa7,
>> +       CCAN_S_SEG_1    = 0xa8,
>> +       CCAN_S_SEG_2    = 0xa9,
>> +       CCAN_S_SJW      = 0xaa,
>> +       CCAN_S_PRESC    = 0xab,
>> +       CCAN_F_SEG_1    = 0xac,
>> +       CCAN_F_SEG_2    = 0xad,
>> +       CCAN_F_SJW      = 0xae,
>> +       CCAN_F_PRESC    = 0xaf,
>> +       CCAN_EALCAP     = 0xb0,
>> +       CCAN_RECNT      = 0xb2,
>> +       CCAN_TECNT      = 0xb3,
>> +};
>> +
>> +enum ccan_reg_bit_mask {
>> +       CCAN_RST_MASK             = BIT(7), /* Set Reset Bit */
>> +       CCAN_FULLCAN_MASK         = BIT(4),
>> +       CCAN_FIFO_MASK            = BIT(5),
>> +       CCAN_TSONE_MASK           = BIT(2),
>> +       CCAN_TSALL_MASK           = BIT(1),
>> +       CCAN_LBMEMOD_MASK         = BIT(6), /* Set loopback external mode */
>> +       CCAN_LBMIMOD_MASK         = BIT(5), /* Set loopback internal mode */
>> +       CCAN_BUSOFF_MASK          = BIT(0),
>> +       CCAN_TTSEN_MASK           = BIT(7),
>> +       CCAN_BRS_MASK             = BIT(4), /* CAN-FD Bit Rate Switch mask */
>> +       CCAN_EDL_MASK             = BIT(5), /* Extended Data Length */
>> +       CCAN_DLC_MASK             = GENMASK(3, 0),
>> +       CCAN_TENEXT_MASK          = BIT(6),
>> +       CCAN_IDE_MASK             = BIT(7),
>> +       CCAN_RTR_MASK             = BIT(6),
>> +       CCAN_INTR_ALL_MASK        = GENMASK(7, 0), /* All interrupts enable mask */
>> +       CCAN_RIE_MASK             = BIT(7),
>> +       CCAN_RFIE_MASK            = BIT(5),
>> +       CCAN_RAFIE_MASK           = BIT(4),
>> +       CCAN_EIE_MASK             = BIT(1),
>> +       CCAN_TASCTIVE_MASK        = BIT(1),
>> +       CCAN_RASCTIVE_MASK        = BIT(2),
>> +       CCAN_TBSEL_MASK           = BIT(7), /* Message writen in STB */
>                                                          ^^^^^^
> 
> Typo: writen -> written

Will fix it later.

> 
>> +       CCAN_STBY_MASK            = BIT(5),
>> +       CCAN_TPE_MASK             = BIT(4), /* Transmit primary enable */
>> +       CCAN_TPA_MASK             = BIT(3),
>> +       CCAN_SACK_MASK            = BIT(7),
>> +       CCAN_RREL_MASK            = BIT(4),
>> +       CCAN_RSTAT_NOT_EMPTY_MASK = GENMASK(1, 0),
>> +       CCAN_RIF_MASK             = BIT(7),
>> +       CCAN_RAFIF_MASK           = BIT(4),
>> +       CCAN_RFIF_MASK            = BIT(5),
>> +       CCAN_TPIF_MASK            = BIT(3), /* Transmission Primary Interrupt Flag */
>> +       CCAN_TSIF_MASK            = BIT(2),
>> +       CCAN_EIF_MASK             = BIT(1),
>> +       CCAN_AIF_MASK             = BIT(0),
>> +       CCAN_EWARN_MASK           = BIT(7),
>> +       CCAN_EPASS_MASK           = BIT(6),
>> +       CCAN_EPIE_MASK            = BIT(5),
>> +       CCAN_EPIF_MASK            = BIT(4),
>> +       CCAN_ALIE_MASK            = BIT(3),
>> +       CCAN_ALIF_MASK            = BIT(2),
>> +       CCAN_BEIE_MASK            = BIT(1),
>> +       CCAN_BEIF_MASK            = BIT(0),
>> +       CCAN_AFWL_MASK            = BIT(6),
>> +       CCAN_EWL_MASK             = (BIT(3) | GENMASK(1, 0)),
>> +       CCAN_KOER_MASK            = GENMASK(7, 5),
>> +       CCAN_BIT_ERROR_MASK       = BIT(5),
>> +       CCAN_FORM_ERROR_MASK      = BIT(6),
>> +       CCAN_STUFF_ERROR_MASK     = GENMASK(6, 5),
>> +       CCAN_ACK_ERROR_MASK       = BIT(7),
>> +       CCAN_CRC_ERROR_MASK       = (BIT(7) | BIT(5)),
>> +       CCAN_OTH_ERROR_MASK       = GENMASK(7, 6),
>> +};
>> +
>> +/* CCAN_S/F_SEG_1 bitfield shift */
>> +#define SEG_1_SHIFT            0
>> +#define SEG_2_SHIFT            8
>> +#define SJW_SHIFT              16
>> +#define PRESC_SHIFT            24
>> +
>> +enum cast_can_type {
>> +       CAST_CAN_TYPE_CAN = 0,
>> +       CAST_CAN_TYPE_CANFD,
>> +};
>> +
>> +struct ccan_priv {
>> +       struct can_priv can;
>> +       struct napi_struct napi;
>> +       struct device *dev;
>> +       void __iomem *reg_base;
>> +       struct clk_bulk_data clks[3];
>> +       struct reset_control *resets;
>> +       u32 cantype;
>> +};
>> +
>> +struct cast_can_data {
>> +       enum cast_can_type cantype;
>> +       const struct can_bittiming_const *bittime_const;
>> +       int (*syscon_update)(struct ccan_priv *priv);
>> +};
>> +
>> +static struct can_bittiming_const ccan_bittiming_const = {
>> +       .name = DRIVER_NAME,
>> +       .tseg1_min = 2,
>> +       .tseg1_max = 16,
>> +       .tseg2_min = 2,
>> +       .tseg2_max = 8,
>> +       .sjw_max = 4,
>> +       .brp_min = 1,
>> +       .brp_max = 256,
>> +       .brp_inc = 1,
>> +};
>> +
>> +static struct can_bittiming_const ccan_bittiming_const_canfd = {
>> +       .name = DRIVER_NAME,
>> +       .tseg1_min = 2,
>> +       .tseg1_max = 64,
>> +       .tseg2_min = 2,
>> +       .tseg2_max = 16,
>> +       .sjw_max = 16,
>> +       .brp_min = 1,
>> +       .brp_max = 256,
>> +       .brp_inc = 1,
>> +};
>> +
>> +static struct can_bittiming_const ccan_data_bittiming_const_canfd = {
>> +       .name = DRIVER_NAME,
>> +       .tseg1_min = 1,
>> +       .tseg1_max = 16,
>> +       .tseg2_min = 2,
>> +       .tseg2_max = 8,
>> +       .sjw_max = 8,
>> +       .brp_min = 1,
>> +       .brp_max = 256,
>> +       .brp_inc = 1,
>> +};
>> +
>> +static inline u32 ccan_read_reg(const struct ccan_priv *priv, u8 reg)
>> +{
>> +       return ioread32(priv->reg_base + reg);
>> +}
>> +
>> +static inline void ccan_write_reg(const struct ccan_priv *priv, u8 reg, u32 value)
>> +{
>> +       iowrite32(value, priv->reg_base + reg);
>> +}
>> +
>> +static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
>> +                                   enum ccan_reg reg)
>> +{
>> +       u8 reg_down;
>> +       union val {
>> +               u8 val_8[4];
>> +               u32 val_32;
>> +       } val;
>> +
>> +       reg_down = ALIGN_DOWN(reg, 4);
>> +       val.val_32 = ccan_read_reg(priv, reg_down);
>> +       return val.val_8[reg - reg_down];
>> +}
>> +
>> +static inline void ccan_write_reg_8bit(const struct ccan_priv *priv,
>> +                                      enum ccan_reg reg, u8 value)
>> +{
>> +       u8 reg_down;
>> +       union val {
>> +               u8 val_8[4];
>> +               u32 val_32;
>> +       } val;
>> +
>> +       reg_down = ALIGN_DOWN(reg, 4);
>> +       val.val_32 = ccan_read_reg(priv, reg_down);
>> +       val.val_8[reg - reg_down] = value;
>> +       ccan_write_reg(priv, reg_down, val.val_32);
>> +}
>> +
>> +static void ccan_reg_set_bits(const struct ccan_priv *priv,
>> +                             enum ccan_reg reg,
>> +                             enum ccan_reg_bit_mask bits)
>> +{
>> +       u8 val;
>> +
>> +       val = ccan_read_reg_8bit(priv, reg);
>> +       val |= bits;
>> +       ccan_write_reg_8bit(priv, reg, val);
>> +}
>> +
>> +static void ccan_reg_clear_bits(const struct ccan_priv *priv,
>> +                               enum ccan_reg reg,
>> +                               enum ccan_reg_bit_mask bits)
>> +{
>> +       u8 val;
>> +
>> +       val = ccan_read_reg_8bit(priv, reg);
>> +       val &= ~bits;
>> +       ccan_write_reg_8bit(priv, reg, val);
>> +}
>> +
>> +static void ccan_set_reset_mode(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
>> +}
>> +
>> +static int ccan_bittime_configuration(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct can_bittiming *bt = &priv->can.bittiming;
>> +       struct can_bittiming *dbt = &priv->can.data_bittiming;
>> +       u32 bittiming, data_bittiming;
>> +       u8 reset_test;
>> +
>> +       reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
>> +
>> +       if (!(reset_test & CCAN_RST_MASK)) {
>> +               netdev_alert(ndev, "Not in reset mode, cannot set bit timing\n");
>> +               return -EPERM;
>> +       }
>> +
>> +       /* Check the bittime parameter */
>> +       if ((((int)(bt->phase_seg1 + bt->prop_seg + 1) - 2) < 0) ||
>> +           (((int)(bt->phase_seg2) - 1) < 0) ||
>> +           (((int)(bt->sjw) - 1) < 0) ||
>> +           (((int)(bt->brp) - 1) < 0))
>> +               return -EINVAL;
> 
> Here, you are checking for wraparounds. But can this really happen? We
> know for sure that:
> 
>   - bt->phase_seg1 + bt->prop_seg1 <= ccan_bittiming_const->tseg1_max
>   - bt->phase_seg1 >= ccan_bittiming_const->tseg2_min
> 
> and so on.
> 
> Unless there is a condition under which this issue can show up, remove
> the check.

OK, will drop it.

> 
>> +       bittiming = ((bt->phase_seg1 + bt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
>> +                        ((bt->phase_seg2 - 1) << SEG_2_SHIFT) |
>> +                        ((bt->sjw - 1) << SJW_SHIFT) |
>> +                        ((bt->brp - 1) << PRESC_SHIFT);
>> +
>> +       ccan_write_reg(priv, CCAN_S_SEG_1, bittiming);
>> +
>> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
>> +               if ((((int)(dbt->phase_seg1 + dbt->prop_seg + 1) - 2) < 0) ||
>> +                   (((int)(dbt->phase_seg2) - 1) < 0) ||
>> +                   (((int)(dbt->sjw) - 1) < 0) ||
>> +                   (((int)(dbt->brp) - 1) < 0))
>> +                       return -EINVAL;
>> +
>> +               data_bittiming = ((dbt->phase_seg1 + dbt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
>> +                                ((dbt->phase_seg2 - 1) << SEG_2_SHIFT) |
>> +                                ((dbt->sjw - 1) << SJW_SHIFT) |
>> +                                ((dbt->brp - 1) << PRESC_SHIFT);
> 
> Nitpick: the calculation for the bittiming and the data_bittiming is
> the same. Can you factorize this calculation in a helper function?

Yes, will add a helper function to calculate.

> 
>> +               ccan_write_reg(priv, CCAN_F_SEG_1, data_bittiming);
>> +       }
>> +
>> +       ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
>> +
>> +       netdev_dbg(ndev, "Slow bit rate: %08x\n", ccan_read_reg(priv, CCAN_S_SEG_1));
>> +       netdev_dbg(ndev, "Fast bit rate: %08x\n", ccan_read_reg(priv, CCAN_F_SEG_1));
>> +
>> +       return 0;
>> +}
>> +
>> +static int ccan_chip_start(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       int err;
>> +
>> +       ccan_set_reset_mode(ndev);
>> +
>> +       err = ccan_bittime_configuration(ndev);
>> +       if (err) {
>> +               netdev_err(ndev, "Bittime setting failed!\n");
>> +               return err;
>> +       }
>> +
>> +       /* Set Almost Full Warning Limit */
>> +       ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_AFWL_MASK);
>> +
>> +       /* Programmable Error Warning Limit = (EWL+1)*8. Set EWL=11->Error Warning=96 */
>> +       ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_EWL_MASK);
>> +
>> +       /* Interrupts enable */
>> +       ccan_write_reg_8bit(priv, CCAN_RTIE, CCAN_INTR_ALL_MASK);
>> +
>> +       /* Error Interrupts enable(Error Passive and Bus Error) */
>> +       ccan_reg_set_bits(priv, CCAN_ERRINT, CCAN_EPIE_MASK);
>> +
>> +       /* Check whether it is loopback mode or normal mode */
>> +       if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
>> +               ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_LBMIMOD_MASK);
>> +       else
>> +               ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_LBMEMOD_MASK | CCAN_LBMIMOD_MASK);
>> +
>> +       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +
>> +       return 0;
>> +}
>> +
>> +static int ccan_do_set_mode(struct net_device *ndev, enum can_mode mode)
>> +{
>> +       int ret;
>> +
>> +       switch (mode) {
>> +       case CAN_MODE_START:
>> +               ret = ccan_chip_start(ndev);
>> +               if (ret) {
>> +                       netdev_err(ndev, "Could not start CAN device !\n");
>> +                       return ret;
>> +               }
>> +               netif_wake_queue(ndev);
>> +               break;
>> +       default:
>> +               ret = -EOPNOTSUPP;
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static void ccan_tx_interrupt(struct net_device *ndev, u8 isr)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       /* wait till transmission of the PTB or STB finished */
>> +       while (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
>> +               if (isr & CCAN_TPIF_MASK)
>> +                       ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TPIF_MASK);
>> +
>> +               if (isr & CCAN_TSIF_MASK)
>> +                       ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TSIF_MASK);
>> +
>> +               isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
>> +       }
>> +
>> +       ndev->stats.tx_bytes += can_get_echo_skb(ndev, 0, NULL);
>> +       ndev->stats.tx_packets++;
>> +       netif_wake_queue(ndev);
>> +}
>> +
>> +static void ccan_rxfull_interrupt(struct net_device *ndev, u8 isr)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       if (isr & CCAN_RAFIF_MASK)
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK);
>> +
>> +       if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK))
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
>> +}
>> +
>> +static enum can_state ccan_get_chip_status(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       u8 can_stat, eir;
>> +
>> +       can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
>> +       eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
>> +
>> +       if (can_stat & CCAN_BUSOFF_MASK)
>> +               return CAN_STATE_BUS_OFF;
>> +
>> +       if (eir & CCAN_EPASS_MASK)
>> +               return CAN_STATE_ERROR_PASSIVE;
>> +
>> +       if (eir & CCAN_EWARN_MASK)
>> +               return CAN_STATE_ERROR_WARNING;
>> +
>> +       return CAN_STATE_ERROR_ACTIVE;
>> +}
>> +
>> +static void ccan_error_interrupt(struct net_device *ndev, u8 isr, u8 eir)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct net_device_stats *stats = &ndev->stats;
>> +       struct can_frame *cf;
>> +       struct sk_buff *skb;
>> +       u8 koer, recnt = 0, tecnt = 0, can_stat = 0;
>> +
>> +       skb = alloc_can_err_skb(ndev, &cf);
>> +
>> +       koer = ccan_read_reg_8bit(priv, CCAN_EALCAP) & CCAN_KOER_MASK;
>> +       recnt = ccan_read_reg_8bit(priv, CCAN_RECNT);
>> +       tecnt = ccan_read_reg_8bit(priv, CCAN_TECNT);
>> +
>> +       /* Read CAN status */
>> +       can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
>> +
>> +       /* Bus off ---> active error mode */
>> +       if ((isr & CCAN_EIF_MASK) && priv->can.state == CAN_STATE_BUS_OFF)
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +
>> +       /* State selection */
>> +       if (can_stat & CCAN_BUSOFF_MASK) {
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +               priv->can.can_stats.bus_off++;
>> +               ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_BUSOFF_MASK);
>> +               can_bus_off(ndev);
>> +               if (skb)
>> +                       cf->can_id |= CAN_ERR_BUSOFF;
>> +       } else if (eir & CCAN_EPASS_MASK) {
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +               priv->can.can_stats.error_passive++;
>> +               if (skb) {
>> +                       cf->can_id |= CAN_ERR_CRTL;
>> +                       cf->data[1] |= (recnt > 127) ? CAN_ERR_CRTL_RX_PASSIVE : 0;
>> +                       cf->data[1] |= (tecnt > 127) ? CAN_ERR_CRTL_TX_PASSIVE : 0;
> 
> Use the CAN state thresholds from include/uapi/linux/can/error.h:
> 
>   recnt >= CAN_ERROR_PASSIVE_THRESHOLD
> 
> and so on.

Get it.

> 
> Replace the ternary operator by an if.

OK. It will be more readable.

> 
>> +                       cf->data[6] = tecnt;
>> +                       cf->data[7] = recnt;
>> +               }
>> +       } else if (eir & CCAN_EWARN_MASK) {
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +               priv->can.can_stats.error_warning++;
>> +               if (skb) {
>> +                       cf->can_id |= CAN_ERR_CRTL;
>> +                       cf->data[1] |= (recnt > 95) ? CAN_ERR_CRTL_RX_WARNING : 0;
>> +                       cf->data[1] |= (tecnt > 95) ? CAN_ERR_CRTL_TX_WARNING : 0;
> 
> Ditto with CAN_ERROR_WARNING_THRESHOLD.

Get it.

> 
>> +                       cf->data[6] = tecnt;
>> +                       cf->data[7] = recnt;
>> +               }
>> +       }
>> +
>> +       /* Check for in protocol defined error interrupt */
>> +       if (eir & CCAN_BEIF_MASK) {
>> +               if (skb)
>> +                       cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
>> +
>> +               if (koer == CCAN_BIT_ERROR_MASK) {
>> +                       stats->tx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_BIT;
>> +               } else if (koer == CCAN_FORM_ERROR_MASK) {
>> +                       stats->rx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_FORM;
>> +               } else if (koer == CCAN_STUFF_ERROR_MASK) {
>> +                       stats->rx_errors++;
>> +                       if (skb)
>> +                               cf->data[3] = CAN_ERR_PROT_STUFF;
>> +               } else if (koer == CCAN_ACK_ERROR_MASK) {
>> +                       stats->tx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_LOC_ACK;
>> +               } else if (koer == CCAN_CRC_ERROR_MASK) {
>> +                       stats->rx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_LOC_CRC_SEQ;
>> +               }
>> +               priv->can.can_stats.bus_error++;
>> +       }
>> +
>> +       if (skb) {
>> +               stats->rx_packets++;
>> +               stats->rx_bytes += cf->can_dlc;
> 
> Do not increase the reception statistics for an error frame. Refer to:
> 
>   https://git.kernel.org/torvalds/c/676068db69b8
> 
> for the reason why.

Will fix it accordingly.

> 
>> +               netif_rx(skb);
>> +       }
>> +
>> +       netdev_dbg(ndev, "Recnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_RECNT));
>> +       netdev_dbg(ndev, "Tecnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_TECNT));
> 
> Either remove this error message or print something more explicit. The
> end-user may not know what a Recnt is. Something like this is better:
> 
>   netdev_dbg(ndev, "Rx error count: 0x%02x", ccan_read_reg_8bit(priv,
> CCAN_RECNT));
> 
> If there are a lot of errors on the but, this can create a lot of
> spam. If you decide to keep, encapsulate this in a:
> 
>   if (net_ratelimit())

I prefer removing this error message. Thanks for your suggestions.

> 
>> +}
>> +
>> +static irqreturn_t ccan_interrupt(int irq, void *dev_id)
>> +{
>> +       struct net_device *ndev = (struct net_device *)dev_id;
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       u8 isr, eir;
>> +       u8 isr_handled = 0, eir_handled = 0;
>> +
>> +       /* Read the value of interrupt status register */
>> +       isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
>> +
>> +       /* Read the value of error interrupt register */
>> +       eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
>> +
>> +       /* Check for Tx interrupt and processing it */
>> +       if (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
>> +               ccan_tx_interrupt(ndev, isr);
>> +               isr_handled |= (CCAN_TPIF_MASK | CCAN_TSIF_MASK);
>> +       }
>> +
>> +       if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK)) {
>> +               ccan_rxfull_interrupt(ndev, isr);
>> +               isr_handled |= (CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
>> +       }
>> +
>> +       /* Check Rx interrupt and processing the receive interrupt routine */
>> +       if (isr & CCAN_RIF_MASK) {
>> +               ccan_reg_clear_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RIF_MASK);
>> +
>> +               napi_schedule(&priv->napi);
>> +               isr_handled |= CCAN_RIF_MASK;
>> +       }
>> +
>> +       if ((isr & CCAN_EIF_MASK) | (eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK))) {
>> +               /* Reset EPIF and BEIF. Reset EIF */
>> +               ccan_reg_set_bits(priv, CCAN_ERRINT, eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK));
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, isr & CCAN_EIF_MASK);
>> +
>> +               ccan_error_interrupt(ndev, isr, eir);
>> +
>> +               isr_handled |= CCAN_EIF_MASK;
>> +               eir_handled |= (CCAN_EPIF_MASK | CCAN_BEIF_MASK);
>> +       }
>> +
>> +       if (isr_handled == 0 && eir_handled == 0) {
>> +               netdev_err(ndev, "Unhandled interrupt!\n");
>> +               return IRQ_NONE;
>> +       }
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static int ccan_open(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       int ret;
>> +
>> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
>> +       if (ret) {
>> +               netdev_err(ndev, "Failed to enable CAN clocks\n");
>> +               return ret;
>> +       }
>> +
>> +       /* Set chip into reset mode */
>> +       ccan_set_reset_mode(ndev);
>> +
>> +       /* Common open */
>> +       ret = open_candev(ndev);
>> +       if (ret)
>> +               goto clk_exit;
>> +
>> +       /* Register interrupt handler */
>> +       ret = devm_request_irq(priv->dev, ndev->irq, ccan_interrupt, IRQF_SHARED,
>> +                              ndev->name, ndev);
>> +       if (ret) {
>> +               netdev_err(ndev, "Request_irq err: %d\n", ret);
>> +               goto candev_exit;
>> +       }
>> +
>> +       ret = ccan_chip_start(ndev);
>> +       if (ret) {
>> +               netdev_err(ndev, "Could not start CAN device !\n");
> 
> Nipick: in English punctuation rules, there an no spaces before the
> exclamation mark:
> 
>   netdev_err(ndev, "Could not start CAN device!\n");
> 
> (or you may consider just removing the exclamation mark. No need to be
> so emotional for an error message).

Yeah, just drop the space and the exclamation.

> 
>> +               goto candev_exit;
>> +       }
>> +
>> +       napi_enable(&priv->napi);
>> +       netif_start_queue(ndev);
>> +
>> +       return 0;
>> +
>> +candev_exit:
>> +       close_candev(ndev);
>> +clk_exit:
>> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
>> +       return ret;
>> +}
>> +
>> +static int ccan_close(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       netif_stop_queue(ndev);
>> +       napi_disable(&priv->napi);
>> +
>> +       ccan_set_reset_mode(ndev);
>> +       priv->can.state = CAN_STATE_STOPPED;
>> +
>> +       close_candev(ndev);
>> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
>> +
>> +       return 0;
>> +}
>> +
>> +static netdev_tx_t ccan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct canfd_frame *cf = (struct canfd_frame *)skb->data;
>> +       u32 id, ctl, addr_off = CCAN_TBUF_DATA;
>> +       int i;
>> +
>> +       if (can_dropped_invalid_skb(ndev, skb))
>> +               return NETDEV_TX_OK;
>> +
>> +       netif_stop_queue(ndev);
> 
> Does your device only allow sending one frame at a time? Doesn't it
> have a transmission queue?

The hardware has a transmission queue, but the current driver doesn't
use it. Let me use the transmission queue in the next version.

> 
>> +       /* Work in XMIT_PTB mode */
>> +       ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_TBSEL_MASK);
>> +
>> +       ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_STBY_MASK);
>> +
>> +       id = cf->can_id & ((cf->can_id & CAN_EFF_FLAG) ? CAN_EFF_MASK : CAN_SFF_MASK);
>> +
>> +       ctl = can_fd_len2dlc(cf->len);
>> +       ctl = (cf->can_id & CAN_EFF_FLAG) ? (ctl | CCAN_IDE_MASK) : (ctl & ~CCAN_IDE_MASK);
>> +
>> +       if (priv->cantype == CAST_CAN_TYPE_CANFD && can_is_canfd_skb(skb)) {
>> +               ctl |= (cf->flags & CANFD_BRS) ? (CCAN_BRS_MASK | CCAN_EDL_MASK) : CCAN_EDL_MASK;
> 
> Replace those long ternary operations with some il/else. It is easier to read.

OK.

> 
>> +               for (i = 0; i < cf->len; i += 4) {
>> +                       ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + i)));
>> +                       addr_off += 4;
> 
> Nitpick, what about:
> 
>   ccan_write_reg(priv, CCAN_TBUF_DATA + i, *((u32 *)(cf->data + i)));
> 
> and then remove the declaration of addr_off.

Yeah, it will be clearer.

> 
>> +               }
>> +       } else {
>> +               ctl &= ~(CCAN_EDL_MASK | CCAN_BRS_MASK);
>> +
>> +               if (cf->can_id & CAN_RTR_FLAG) {
>> +                       ctl |= CCAN_RTR_MASK;
>> +               } else {
>> +                       ctl &= ~CCAN_RTR_MASK;
>> +                       ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + 0)));
>> +                       ccan_write_reg(priv, addr_off + 4, *((u32 *)(cf->data + 4)));
> 
> In this else block, addr_off is just an alias of CCAN_TBUF_DATA. So,
> directly do:
> 
>   ccan_write_reg(priv, CCAN_TBUF_DATA, *((u32 *)(cf->data + 0)));
>   ccan_write_reg(priv, CCAN_TBUF_DATA + 4, *((u32 *)(cf->data + 4)));

OK.

> 
>> +               }
>> +       }
>> +
>> +       ccan_write_reg(priv, CCAN_TBUF_ID, id);
>> +       ccan_write_reg(priv, CCAN_TBUF_CTL, ctl);
>> +       ccan_reg_set_bits(priv, CCAN_TCMD, CCAN_TPE_MASK);
>> +
>> +       can_put_echo_skb(skb, ndev, 0, 0);
>> +
>> +       return NETDEV_TX_OK;
>> +}
>> +
>> +static const struct net_device_ops ccan_netdev_ops = {
>> +       .ndo_open = ccan_open,
>> +       .ndo_stop = ccan_close,
>> +       .ndo_start_xmit = ccan_start_xmit,
>> +       .ndo_change_mtu = can_change_mtu,
>> +};
> 
> Also add a struct ethtool_ops for the default timestamps:
> 
>   static const struct ethtool_ops ccan_ethtool_ops = {
>           .get_ts_info = ethtool_op_get_ts_info,
>   };
> 
> This assumes that your device does not support hardware timestamps. If
> you do have hardware timestamping support, please adjust accordingly.

Will add this struct ethtool_ops later.

> 
>> +static int ccan_rx(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct net_device_stats *stats = &ndev->stats;
>> +       struct canfd_frame *cf_fd;
>> +       struct can_frame *cf;
>> +       struct sk_buff *skb;
>> +       u32 can_id;
>> +       u8 dlc, control;
>> +       int i;
>> +
>> +       control = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL);
>> +       can_id = ccan_read_reg(priv, CCAN_RUBF_ID);
>> +       dlc = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL) & CCAN_DLC_MASK;
>> +
>> +       if (control & CCAN_EDL_MASK)
>> +               /* allocate sk_buffer for canfd frame */
>> +               skb = alloc_canfd_skb(ndev, &cf_fd);
>> +       else
>> +               /* allocate sk_buffer for can frame */
>> +               skb = alloc_can_skb(ndev, &cf);
>> +
>> +       if (!skb) {
>> +               stats->rx_dropped++;
>> +               return 0;
>> +       }
>> +
>> +       /* Change the CANFD or CAN2.0 data into socketcan data format */
>> +       if (control & CCAN_EDL_MASK)
>> +               cf_fd->len = can_fd_dlc2len(dlc);
>> +       else
>> +               cf->can_dlc = can_cc_dlc2len(dlc);
> 
> cf->can_dlc is deprecated. Use cf->len instead.

Get it.

> 
>> +       /* Change the CANFD or CAN2.0 id into socketcan id format */
>> +       if (control & CCAN_EDL_MASK) {
>> +               cf_fd->can_id = can_id;
>> +               cf_fd->can_id = (control & CCAN_IDE_MASK) ? (cf_fd->can_id | CAN_EFF_FLAG) :
>> +                            (cf_fd->can_id & ~CAN_EFF_FLAG);
>> +       } else {
>> +               cf->can_id = can_id;
>> +               cf->can_id = (control & CCAN_IDE_MASK) ? (cf->can_id | CAN_EFF_FLAG) :
>> +                            (cf->can_id & ~CAN_EFF_FLAG);
>> +       }
> 
> struct can_frame and struct canfd_frame have overlapping fields. You
> can just have one stack variable for both and then, no need for an
> if/else here.

Get it.

> 
>> +       if (!(control & CCAN_EDL_MASK))
>> +               if (control & CCAN_RTR_MASK)
>> +                       cf->can_id |= CAN_RTR_FLAG;
>> +
>> +       if (control & CCAN_EDL_MASK) {
> 
> You are checking for if (!(control & CCAN_EDL_MASK)) and just after
> for if (control & CCAN_EDL_MASK). Factorize those two together.

OK.

> 
>> +               for (i = 0; i < cf_fd->len; i += 4)
>> +                       *((u32 *)(cf_fd->data + i)) = ccan_read_reg(priv, CCAN_RBUF_DATA + i);
>> +       } else {
>> +               /* skb reads the received datas, if the RTR bit not set */
>> +               if (!(control & CCAN_RTR_MASK)) {
>> +                       *((u32 *)(cf->data + 0)) = ccan_read_reg(priv, CCAN_RBUF_DATA);
>> +                       *((u32 *)(cf->data + 4)) = ccan_read_reg(priv, CCAN_RBUF_DATA + 4);
>> +               }
>> +       }
>> +
>> +       ccan_reg_set_bits(priv, CCAN_RCTRL, CCAN_RREL_MASK);
>> +
>> +       stats->rx_bytes += (control & CCAN_EDL_MASK) ? cf_fd->len : cf->can_dlc;
> 
> No, cf_fd->len and cf->can_dlc are the same byte (these are parts of
> an union). It is just that cf->can_dlc is deprecated and is kept to
> not break the UAPI. In the kernel it should never be used.
> 
> Here, what you have to do is just to not increase stats->rx_bytes for
> RTR frames. Try to factorize this with the other checks which you did
> above.

Get it.

> 
>> +       stats->rx_packets++;
>> +       netif_receive_skb(skb);
>> +
>> +       return 1;
> 
> Why return 1 on success and 0 on failure? The convention in the kernel
> is that 0 means success. If you really want to keep 0 for failure, at
> least make this return boolean true or boolean false, but overall, try
> to follow the return conventions.

The return value here represents the number of successfully received packets.
It is used in ccan_rx_poll() for counting the number of successfully
received packets.

> 
>> +}
>> +
>> +static int ccan_rx_poll(struct napi_struct *napi, int quota)
>> +{
>> +       struct net_device *ndev = napi->dev;
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       int work_done = 0;
>> +       u8 rx_status = 0;
>> +
>> +       rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
>> +
>> +       /* Clear receive interrupt and deal with all the received frames */
>> +       while ((rx_status & CCAN_RSTAT_NOT_EMPTY_MASK) && (work_done < quota)) {
>> +               work_done += ccan_rx(ndev);
>> +
>> +               rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
>> +       }
>> +
>> +       napi_complete(napi);
>> +       ccan_reg_set_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
>> +
>> +       return work_done;
>> +}
>> +
>> +static int ccan_driver_probe(struct platform_device *pdev)
>> +{
>> +       struct net_device *ndev;
>> +       struct ccan_priv *priv;
>> +       const struct cast_can_data *ddata;
>> +       void __iomem *addr;
>> +       int ret;
>> +
>> +       addr = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(addr)) {
>> +               ret = PTR_ERR(addr);
>> +               goto exit;
> 
> No need for goto here:
> 
>   return PTR_ERR(addr);

OK.

> 
>> +       }
>> +
>> +       ddata = of_device_get_match_data(&pdev->dev);
>> +       if (!ddata)
>> +               return -ENODEV;
>> +
>> +       ndev = alloc_candev(sizeof(struct ccan_priv), 1);
>> +       if (!ndev) {
>> +               ret = -ENOMEM;
>> +               goto exit;
> 
> Same:
> 
>   return -ENOMEM;
> 
> (this done, remove the exit label).

OK.

> 
>> +       }
>> +
>> +       priv = netdev_priv(ndev);
>> +       priv->dev = &pdev->dev;
>> +       priv->cantype = ddata->cantype;
>> +       priv->can.bittiming_const = ddata->bittime_const;
>> +
>> +       if (ddata->syscon_update) {
>> +               ret = ddata->syscon_update(priv);
>> +               if (ret)
>> +                       goto free_exit;
>> +       }
>> +
>> +       priv->clks[0].id = "apb";
>> +       priv->clks[1].id = "timer";
>> +       priv->clks[2].id = "core";
>> +
>> +       ret = devm_clk_bulk_get(&pdev->dev, ARRAY_SIZE(priv->clks), priv->clks);
>> +       if (ret) {
>> +               ret = dev_err_probe(&pdev->dev, ret, "Failed to get CAN clocks\n");
>> +               goto free_exit;
>> +       }
>> +
>> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
>> +       if (ret) {
>> +               ret = dev_err_probe(&pdev->dev, ret, "Failed to enable CAN clocks\n");
>> +               goto free_exit;
>> +       }
>> +
>> +       priv->resets = devm_reset_control_array_get_exclusive(&pdev->dev);
>> +       if (IS_ERR(priv->resets)) {
>> +               ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->resets),
>> +                                   "Failed to get CAN resets");
>> +               goto clk_exit;
>> +       }
>> +
>> +       ret = reset_control_deassert(priv->resets);
>> +       if (ret)
>> +               goto clk_exit;
>> +
>> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
>> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
>> +               priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
>> +       } else {
>> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
>> +       }
> 
> Nitpick, consider doing this:
> 
>   priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
>   if (priv->cantype == CAST_CAN_TYPE_CANFD) {
>           priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
>           priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
>   }

OK.

> 
> Also, does you hardware support dlc greater than 8 (c.f.
> CAN_CTRLMODE_CC_LEN8_DLC)?

The class CAN (CC) mode does not support, but the CAN FD mode supports.

> 
>> +       priv->reg_base = addr;
>> +       priv->can.clock.freq = clk_get_rate(priv->clks[2].clk);
>> +       priv->can.do_set_mode = ccan_do_set_mode;
>> +       ndev->irq = platform_get_irq(pdev, 0);
>> +
>> +       /* We support local echo */
>> +       ndev->flags |= IFF_ECHO;
>> +       ndev->netdev_ops = &ccan_netdev_ops;
>> +
>> +       platform_set_drvdata(pdev, ndev);
>> +       SET_NETDEV_DEV(ndev, &pdev->dev);
>> +
>> +       netif_napi_add_tx_weight(ndev, &priv->napi, ccan_rx_poll, 16);
>> +       ret = register_candev(ndev);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Failed to register (err=%d)\n", ret);
> 
>>From the Linux coding style:
> 
>   Printing numbers in parentheses (%d) adds no value and should be avoided.
> 
> Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#printing-kernel-messages

Will fix it accordingly.

Sorry for the late reply. Thank you for your detailed review.

Best regards,
Hal

Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Vincent MAILHOL]

On Tue. 15 Oct. 2024 at 18:33, Hal Feng <hal.feng@linux.starfivetech.com> wrote:
> On 9/23/2024 11:41 AM, Vincent MAILHOL wrote:
> > Hi Hal,
> >
> > A few more comments on top of what Andrew already wrote.
> >
> > On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng@starfivetech.com> wrote:
> >> From: William Qiu <william.qiu@starfivetech.com>
> >>
> >> Add driver for CAST CAN Bus Controller used on
> >> StarFive JH7110 SoC.
> >>
> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >> ---

(...)

> >> +       stats->rx_packets++;
> >> +       netif_receive_skb(skb);
> >> +
> >> +       return 1;
> >
> > Why return 1 on success and 0 on failure? The convention in the kernel
> > is that 0 means success. If you really want to keep 0 for failure, at
> > least make this return boolean true or boolean false, but overall, try
> > to follow the return conventions.
>
> The return value here represents the number of successfully received packets.
> It is used in ccan_rx_poll() for counting the number of successfully
> received packets.

Ack. I guess this will become more clear after you implement the queue logic.

(...)

> >> +
> >> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> >> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
> >> +               priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
> >> +       } else {
> >> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> >> +       }
> >
> > Nitpick, consider doing this:
> >
> >   priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> >   if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> >           priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
> >           priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
> >   }
>
> OK.
>
> >
> > Also, does you hardware support dlc greater than 8 (c.f.
> > CAN_CTRLMODE_CC_LEN8_DLC)?
>
> The class CAN (CC) mode does not support, but the CAN FD mode supports.

So, CAN_CTRLMODE_CC_LEN8_DLC is a Classical CAN feature. Strictly
speaking, this does not exist in CAN FD. Do you mean that only the
CAST_CAN_TYPE_CANFD supports sending Classical CAN frames with a DLC
greater than 8?

If none of the Classical CAN or CAN FD variants of your device is able
to send Classical CAN frames with a DLC greater than 8, then this is
just not supported by your device.

Could you share the datasheet so that I can double check this?

(...)

> Sorry for the late reply. Thank you for your detailed review.

No problem, take your time!


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Vincent MAILHOL]

On Wed. 16 Oct. 2024 at 14:05, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Tue. 15 Oct. 2024 at 18:33, Hal Feng <hal.feng@linux.starfivetech.com> wrote:
> > On 9/23/2024 11:41 AM, Vincent MAILHOL wrote:
> > > Hi Hal,
> > >
> > > A few more comments on top of what Andrew already wrote.
> > >
> > > On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng@starfivetech.com> wrote:
> > >> From: William Qiu <william.qiu@starfivetech.com>
> > >>
> > >> Add driver for CAST CAN Bus Controller used on
> > >> StarFive JH7110 SoC.
> > >>
> > >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> > >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > >> ---

(...)

> > >> +
> > >> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> > >> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
> > >> +               priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
> > >> +       } else {
> > >> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> > >> +       }
> > >
> > > Nitpick, consider doing this:
> > >
> > >   priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> > >   if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> > >           priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
> > >           priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
> > >   }
> >
> > OK.
> >
> > >
> > > Also, does you hardware support dlc greater than 8 (c.f.
> > > CAN_CTRLMODE_CC_LEN8_DLC)?
> >
> > The class CAN (CC) mode does not support, but the CAN FD mode supports.
>
> So, CAN_CTRLMODE_CC_LEN8_DLC is a Classical CAN feature. Strictly
> speaking, this does not exist in CAN FD. Do you mean that only the
> CAST_CAN_TYPE_CANFD supports sending Classical CAN frames with a DLC
> greater than 8?
>
> If none of the Classical CAN or CAN FD variants of your device is able
> to send Classical CAN frames with a DLC greater than 8, then this is
> just not supported by your device.
>
> Could you share the datasheet so that I can double check this?

I received the datasheet from a good samaritan. With this, I was able
to confirm a few things.

1/ Your device can support CAN_CTRLMODE_CC_LEN8_DLC:

This is shown in the datasheet at:

  Table 3-52 Definition of the DLC (according to the CAN 2.0 / FD specification)

DLC values 9 to 15 (binary 1001 to 1111) are accepted by the device.
When sending and receiving such frames, can_frame->len is set to 8 and
can_frame->len8_dlc is set to the actual DLC value. Use the
can_cc_dlc2len() and can_get_cc_dlc() helpers for this.


2/ Your device can support CAN_CTRLMODE_TDC_AUTO:

This is documented in the datasheet at:

  8.8 TDC and RDC

This will allow the use of higher bitrates (e.g. 4 Mbits/s) in CAN-FD.
You can refer to this commit for an example of how to implement it:

  https://git.kernel.org/torvalds/c/1010a8fa9608


3/ Your device can support CAN_CTRLMODE_3_SAMPLES:

This is called triple mode redundancy (TMR) in your datasheet.


4/ Your device can support CAN_CTRLMODE_LISTENONLY:

This is documented in the datasheet at:

  3.9.10.2. Listen Only Mode (LOM)


5/ Your device can support CAN_CTRLMODE_ONE_SHOT:

This is documented in the datasheet at:

  6.5.3 Single Shot Transmit Trigger


6/ Your device can support CAN_CTRLMODE_BERR_REPORTING:

This is shown in the datasheet at:

  Table 3-24 Error Counter Registers RECNT (0xb2) and TECNT (0xb3)


7/ Your device can support CAN_CTRLMODE_PRESUME_ACK:

c.f. the SACK (self acknowledge) register


So your device comes with MANY features. I would like to see those
implemented in your driver. Most of the time, adding a feature just
means writing one value to a register.

Please let me know if any of this is unclear.


Yours sincerely,
Vincent Mailhol

[PATCH v2 4/4] riscv: dts: starfive: jh7110: Add CAN nodes

[Hal Feng]

From: William Qiu <william.qiu@starfivetech.com>

Add can0/1 support for StarFive JH7110 SoC.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 32 ++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 0d8339357bad..368cc40829f9 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -929,6 +929,38 @@ watchdog@13070000 {
 				 <&syscrg JH7110_SYSRST_WDT_CORE>;
 		};
 
+		can0: can@130d0000 {
+			compatible = "starfive,jh7110-can", "cast,can-ctrl-fd-7x10N00S00";
+			reg = <0x0 0x130d0000 0x0 0x1000>;
+			interrupts = <112>;
+			clocks = <&syscrg JH7110_SYSCLK_CAN0_APB>,
+				 <&syscrg JH7110_SYSCLK_CAN0_TIMER>,
+				 <&syscrg JH7110_SYSCLK_CAN0_CAN>;
+			clock-names = "apb", "timer", "core";
+			resets = <&syscrg JH7110_SYSRST_CAN0_APB>,
+				 <&syscrg JH7110_SYSRST_CAN0_TIMER>,
+				 <&syscrg JH7110_SYSRST_CAN0_CORE>;
+			reset-names = "apb", "timer", "core";
+			starfive,syscon = <&sys_syscon 0x10 0x3 0x8>;
+			status = "disabled";
+		};
+
+		can1: can@130e0000 {
+			compatible = "starfive,jh7110-can", "cast,can-ctrl-fd-7x10N00S00";
+			reg = <0x0 0x130e0000 0x0 0x1000>;
+			interrupts = <113>;
+			clocks = <&syscrg JH7110_SYSCLK_CAN1_APB>,
+				 <&syscrg JH7110_SYSCLK_CAN1_TIMER>,
+				 <&syscrg JH7110_SYSCLK_CAN1_CAN>;
+			clock-names = "apb", "timer", "core";
+			resets = <&syscrg JH7110_SYSRST_CAN1_APB>,
+				 <&syscrg JH7110_SYSRST_CAN1_TIMER>,
+				 <&syscrg JH7110_SYSRST_CAN1_CORE>;
+			reset-names = "apb", "timer", "core";
+			starfive,syscon = <&sys_syscon 0x88 0x12 0x40000>;
+			status = "disabled";
+		};
+
 		crypto: crypto@16000000 {
 			compatible = "starfive,jh7110-crypto";
 			reg = <0x0 0x16000000 0x0 0x4000>;
-- 
2.43.2

RE: [PATCH v2 2/4] dt-bindings: can: Add CAST CAN Bus Controller

[Hal Feng]

> On 25.9.24 04:03, Rob Herring wrote:
> On Sun, Sep 22, 2024 at 10:51:48PM +0800, Hal Feng wrote:
> > From: William Qiu <william.qiu@starfivetech.com>
> >
> > Add bindings for CAST CAN Bus Controller.
> >
> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  .../bindings/net/can/cast,can-ctrl.yaml       | 106 ++++++++++++++++++
> >  1 file changed, 106 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> > b/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> > new file mode 100644
> > index 000000000000..2870cff80164
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
> > @@ -0,0 +1,106 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/can/cast,can-ctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: CAST CAN Bus Controller
> > +
> > +description:
> > +  This CAN Bus Controller, also called CAN-CTRL, implements a highly
> > +  featured and reliable CAN bus controller that performs serial
> > +  communication according to the CAN protocol.
> > +
> > +  The CAN-CTRL comes in three variants, they are CC, FD, and XL.
> > +  The CC variant supports only Classical CAN, the FD variant adds
> > + support  for CAN FD, and the XL variant supports the Classical CAN,
> > + CAN FD, and  CAN XL standards.
> > +
> > +maintainers:
> > +  - William Qiu <william.qiu@starfivetech.com>
> > +  - Hal Feng <hal.feng@starfivetech.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +        - starfive,jh7110-can
> > +      - const: cast,can-ctrl-fd-7x10N00S00
> 
> What's the 7x10...? Perhaps some explanation on it.

7x10N00S00 is the CAN IP product version.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 3
> > +
> > +  clock-names:
> > +    items:
> > +      - const: apb
> > +      - const: timer
> > +      - const: core
> > +
> > +  resets:
> > +    minItems: 3
> > +
> > +  reset-names:
> > +    items:
> > +      - const: apb
> > +      - const: timer
> > +      - const: core
> > +
> > +  starfive,syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - items:
> > +          - description: phandle to System Register Controller syscon node
> > +          - description: offset of SYS_SYSCONSAIF__SYSCFG register for CAN
> controller
> > +          - description: shift of SYS_SYSCONSAIF__SYSCFG register for CAN
> controller
> > +          - description: mask of SYS_SYSCONSAIF__SYSCFG register for CAN
> controller
> > +    description:
> > +      Should be four parameters, the phandle to System Register Controller
> > +      syscon node and the offset/shift/mask of SYS_SYSCONSAIF__SYSCFG
> register
> > +      for CAN controller.
> 
> This just repeats what the schema says. More useful would be what you need
> to access/control in this register.

OK, will improve the description here. Thanks.

Best regards,
Hal