Re: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller
From: Hal Feng
Date: Tue Oct 15 2024 - 05:33:41 EST
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@xxxxxxxxxxxxxxxx> wrote:
>> From: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
>>
>> Add driver for CAST CAN Bus Controller used on
>> StarFive JH7110 SoC.
>>
>> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
>> Co-developed-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxx>
>> +M: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
>> +L: linux-can@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxxx>
>> 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