diff mbox series

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

Message ID 20240922145151.130999-4-hal.feng@starfivetech.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series CAST Controller Area Network driver support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 14 of 14 maintainers
netdev/build_clang success Errors and warnings before: 17 this patch: 17
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 22 this patch: 22
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns WARNING: please write a help paragraph that fully describes the config symbol
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline fail Was 0 now: 4
netdev/contest success net-next-2024-09-26--21-00 (tests: 768)

Commit Message

Hal Feng Sept. 22, 2024, 2:51 p.m. UTC
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

Comments

Andrew Lunn Sept. 22, 2024, 4:33 p.m. UTC | #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);
> +}

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
Marc Kleine-Budde Sept. 22, 2024, 9:13 p.m. UTC | #2
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
Vincent Mailhol Sept. 23, 2024, 3:41 a.m. UTC | #3
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
Hal Feng Sept. 23, 2024, 7:53 a.m. UTC | #4
> 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
Andrew Lunn Sept. 23, 2024, 12:12 p.m. UTC | #5
> > > +	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
Hal Feng Oct. 15, 2024, 9:30 a.m. UTC | #6
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
Vincent Mailhol Oct. 16, 2024, 5:05 a.m. UTC | #7
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
Vincent Mailhol Oct. 16, 2024, 2:16 p.m. UTC | #8
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
Marc Kleine-Budde Oct. 28, 2024, 2:18 p.m. UTC | #9
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
Marc Kleine-Budde Oct. 28, 2024, 3:28 p.m. UTC | #10
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
diff mbox series

Patch

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");