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