This patch add support for Fintek USB to 2CAN controller support.
Signed-off-by: Ji-Ze Hong (Peter Hong) <[email protected]>
---
Changelog:
v3:
1. Change CAN clock to using MEGA units.
2. Remove USB set/get retry, only remain SJA1000 reset/operation retry.
3. Fix all numberic constant to define.
4. Add terminator control. (only 0 & 120 ohm)
5. Using struct data to represent INT/TX/RX endpoints data instead byte
arrays.
6. Error message reports changed from %d to %pe for mnemotechnic values.
7. Some bit operations are changed to FIELD_PREP().
8. Separate TX functions from f81604_read_int_callback().
9. cf->can_id |= CAN_ERR_CNT in f81604_read_int_callback to report valid
TX/RX error counts.
10. Move f81604_prepare_urbs/f81604_remove_urbs() from CAN open/close() to
USB probe/disconnect().
11. coding style refactoring.
v2:
1. coding style refactoring.
2. some const number are defined to describe itself.
3. fix wrong usage for can_get_echo_skb() in f81604_write_bulk_callback().
drivers/net/can/usb/Kconfig | 12 +
drivers/net/can/usb/Makefile | 1 +
drivers/net/can/usb/f81604.c | 1232 ++++++++++++++++++++++++++++++++++
3 files changed, 1245 insertions(+)
create mode 100644 drivers/net/can/usb/f81604.c
diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index 445504ababce..06ab92cd2ea7 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -147,4 +147,16 @@ config CAN_UCAN
from Theobroma Systems like the A31-µQ7 and the RK3399-Q7
(https://www.theobroma-systems.com/rk3399-q7)
+config CAN_F81604
+ tristate "Fintek F81604 USB to 2CAN interface"
+ help
+ This driver supports the Fintek F81604 USB to 2CAN interface.
+ The device can support CAN2.0A/B protocol and also support
+ 2 output pins to control external terminator (optional).
+
+ To compile this driver as a module, choose M here: the module will
+ be called f81604.
+
+ (see also https://www.fintek.com.tw).
+
endmenu
diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
index 1ea16be5743b..9de0305a5cce 100644
--- a/drivers/net/can/usb/Makefile
+++ b/drivers/net/can/usb/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/
obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
obj-$(CONFIG_CAN_UCAN) += ucan.o
+obj-$(CONFIG_CAN_F81604) += f81604.o
diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
new file mode 100644
index 000000000000..06cd5d95c44d
--- /dev/null
+++ b/drivers/net/can/usb/f81604.c
@@ -0,0 +1,1232 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Fintek F81604 USB-to-2CAN controller driver.
+ *
+ * Copyright (C) 2023 Ji-Ze Hong (Peter Hong) <[email protected]>
+ */
+#include <linux/netdevice.h>
+#include <linux/usb.h>
+#include <linux/bitfield.h>
+#include <linux/units.h>
+
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+#include <linux/can/platform/sja1000.h>
+
+/* vendor and product id */
+#define F81604_VENDOR_ID 0x2c42
+#define F81604_PRODUCT_ID 0x1709
+#define F81604_CAN_CLOCK (24 * MEGA / 2)
+#define F81604_MAX_DEV 2
+#define F81604_SET_DEVICE_RETRY 100
+
+#define F81604_USB_TIMEOUT 2000
+#define F81604_SET_GET_REGISTER 0xA0
+#define F81604_PORT_OFFSET 0x1000
+
+#define F81604_BULK_SIZE 64
+#define F81604_INT_SIZE 16
+#define F81604_DATA_SIZE 14
+#define F81604_MAX_RX_URBS 4
+
+#define F81604_CMD_OFFSET 0x00
+#define F81604_CMD_DATA 0x00
+
+#define F81604_DLC_LEN_MASK 0x0f
+#define F81604_DLC_EFF_BIT BIT(7)
+#define F81604_DLC_RTR_BIT BIT(6)
+
+/* device setting */
+#define F81604_CTRL_MODE_REG 0x80
+#define F81604_TX_ONESHOT (0x03 << 3)
+#define F81604_TX_NORMAL (0x01 << 3)
+#define F81604_RX_AUTO_RELEASE_BUF BIT(1)
+#define F81604_INT_WHEN_CHANGE BIT(0)
+
+#define F81604_TERMINATOR_REG 0x105
+#define F81604_CAN0_TERM BIT(2)
+#define F81604_CAN1_TERM BIT(3)
+
+#define F81604_TERMINATION_DISABLED CAN_TERMINATION_DISABLED
+#define F81604_TERMINATION_ENABLED 120
+
+/* SJA1000 registers - manual section 6.4 (Pelican Mode) */
+#define SJA1000_MOD 0x00
+#define SJA1000_CMR 0x01
+#define SJA1000_IR 0x03
+#define SJA1000_IER 0x04
+#define SJA1000_ALC 0x0B
+#define SJA1000_ECC 0x0C
+#define SJA1000_RXERR 0x0E
+#define SJA1000_TXERR 0x0F
+#define SJA1000_ACCC0 0x10
+#define SJA1000_ACCC1 0x11
+#define SJA1000_ACCC2 0x12
+#define SJA1000_ACCC3 0x13
+#define SJA1000_ACCM0 0x14
+#define SJA1000_ACCM1 0x15
+#define SJA1000_ACCM2 0x16
+#define SJA1000_ACCM3 0x17
+#define SJA1000_MAX_FILTER_CNT 4
+
+/* Common registers - manual section 6.5 */
+#define SJA1000_BTR0 0x06
+#define SJA1000_BTR1 0x07
+#define SJA1000_BTR1_SAMPLE_TRIPLE BIT(7)
+#define SJA1000_OCR 0x08
+#define SJA1000_CDR 0x1F
+
+/* mode register */
+#define MOD_RM 0x01
+#define MOD_LOM 0x02
+#define MOD_STM 0x04
+
+/* commands */
+#define CMD_CDO 0x08
+
+/* interrupt sources */
+#define IRQ_BEI 0x80
+#define IRQ_ALI 0x40
+#define IRQ_EPI 0x20
+#define IRQ_DOI 0x08
+#define IRQ_EI 0x04
+#define IRQ_TI 0x02
+#define IRQ_RI 0x01
+#define IRQ_ALL 0xFF
+#define IRQ_OFF 0x00
+
+/* status register content */
+#define SR_BS 0x80
+#define SR_ES 0x40
+#define SR_TS 0x20
+#define SR_RS 0x10
+#define SR_TCS 0x08
+#define SR_TBS 0x04
+#define SR_DOS 0x02
+#define SR_RBS 0x01
+
+/* ECC register */
+#define ECC_SEG 0x1F
+#define ECC_DIR 0x20
+#define ECC_BIT 0x00
+#define ECC_FORM 0x40
+#define ECC_STUFF 0x80
+#define ECC_MASK 0xc0
+
+/* ALC register */
+#define ALC_MASK 0x1f
+
+/* table of devices that work with this driver */
+static const struct usb_device_id f81604_table[] = {
+ { USB_DEVICE(F81604_VENDOR_ID, F81604_PRODUCT_ID) },
+ {} /* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(usb, f81604_table);
+
+static const struct ethtool_ops f81604_ethtool_ops = {
+ .get_ts_info = ethtool_op_get_ts_info,
+};
+
+static const u16 f81604_termination[] = { F81604_TERMINATION_DISABLED,
+ F81604_TERMINATION_ENABLED };
+
+struct f81604_priv {
+ struct net_device *netdev[F81604_MAX_DEV];
+ struct mutex mutex; /* for terminator setting */
+};
+
+struct f81604_port_priv {
+ struct can_priv can;
+ struct net_device *netdev;
+ struct sk_buff *echo_skb;
+
+ /* For synchronize need_clear_alc/need_clear_ecc in worker & interrupt
+ * callback.
+ */
+ spinlock_t lock;
+ bool need_clear_alc;
+ bool need_clear_ecc;
+
+ struct work_struct handle_clear_reg_work;
+ struct work_struct handle_clear_overrun_work;
+
+ struct usb_device *dev;
+ struct usb_interface *intf;
+
+ struct urb *int_urb;
+ u8 int_read_buffer[F81604_INT_SIZE];
+
+ struct urb *read_urb[F81604_MAX_RX_URBS];
+ u8 bulk_read_buffer[F81604_MAX_RX_URBS][F81604_BULK_SIZE];
+
+ struct urb *write_urb;
+ u8 bulk_write_buffer[F81604_DATA_SIZE];
+
+ u8 ocr;
+ u8 cdr;
+};
+
+/* Interrupt endpoint data format: SR/IR/IER/ALC/ECC/EWLR/RXERR/TXERR/VAL */
+struct f81604_int_data {
+ u8 sr;
+ u8 isrc;
+ u8 ier;
+ u8 alc;
+ u8 ecc;
+ u8 ewlr;
+ u8 rxerr;
+ u8 txerr;
+ u8 val;
+} __packed;
+
+struct f81604_sff {
+ __be16 id;
+ u8 data[CAN_MAX_DLEN];
+} __packed;
+
+struct f81604_eff {
+ __be32 id;
+ u8 data[CAN_MAX_DLEN];
+} __packed;
+
+struct f81604_bulk_data {
+ u8 cmd;
+
+ /* According for F81604 DLC define:
+ * #define F81604_DLC_LEN_MASK 0x0f
+ * #define F81604_DLC_EFF_BIT BIT(7)
+ * #define F81604_DLC_RTR_BIT BIT(6)
+ */
+ u8 dlc;
+
+ union {
+ struct f81604_sff sff;
+ struct f81604_eff eff;
+ };
+} __packed;
+
+static int f81604_set_register(struct usb_device *dev, u16 reg, u8 data)
+{
+ int ret;
+
+ ret = usb_control_msg_send(dev, 0, F81604_SET_GET_REGISTER,
+ USB_TYPE_VENDOR | USB_DIR_OUT, 0, reg,
+ &data, sizeof(data), F81604_USB_TIMEOUT,
+ GFP_KERNEL);
+ if (ret)
+ dev_err(&dev->dev, "%s: reg: %x data: %x failed: %pe\n",
+ __func__, reg, data, ERR_PTR(ret));
+
+ return ret;
+}
+
+static int f81604_get_register(struct usb_device *dev, u16 reg, u8 *data)
+{
+ int ret;
+
+ ret = usb_control_msg_recv(dev, 0, F81604_SET_GET_REGISTER,
+ USB_TYPE_VENDOR | USB_DIR_IN, 0, reg, data,
+ sizeof(*data), F81604_USB_TIMEOUT,
+ GFP_KERNEL);
+
+ if (ret < 0)
+ dev_err(&dev->dev, "%s: reg: %x failed: %pe\n", __func__, reg,
+ ERR_PTR(ret));
+
+ return ret;
+}
+
+static int f81604_mask_set_register(struct usb_device *dev, u16 reg, u8 mask,
+ u8 data)
+{
+ int status;
+ u8 tmp;
+
+ status = f81604_get_register(dev, reg, &tmp);
+ if (status)
+ return status;
+
+ tmp &= ~mask;
+ tmp |= (mask & data);
+
+ return f81604_set_register(dev, reg, tmp);
+}
+
+static int f81604_set_sja1000_register(struct usb_device *dev, u8 port,
+ u16 reg, u8 data)
+{
+ int real_reg;
+
+ real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
+ return f81604_set_register(dev, real_reg, data);
+}
+
+static int f81604_get_sja1000_register(struct usb_device *dev, u8 port,
+ u16 reg, u8 *data)
+{
+ int real_reg;
+
+ real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
+ return f81604_get_register(dev, real_reg, data);
+}
+
+static int f81604_set_reset_mode(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int status, i;
+ u8 tmp;
+
+ /* disable interrupts */
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_IER, IRQ_OFF);
+ if (status)
+ return status;
+
+ for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
+ status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_MOD, &tmp);
+ if (status)
+ return status;
+
+ /* check reset bit */
+ if (tmp & MOD_RM) {
+ priv->can.state = CAN_STATE_STOPPED;
+ return 0;
+ }
+
+ /* reset chip */
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_MOD, MOD_RM);
+ if (status)
+ return status;
+ }
+
+ return -EPERM;
+}
+
+static int f81604_set_normal_mode(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ u8 mod_reg = 0x00;
+ u8 tmp, ier = 0;
+ int status, i;
+
+ for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
+ status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_MOD, &tmp);
+ if (status)
+ return status;
+
+ /* check reset bit */
+ if ((tmp & MOD_RM) == 0) {
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ /* enable interrupts, RI handled by bulk-in */
+ ier = IRQ_ALL & ~IRQ_RI;
+ if (!(priv->can.ctrlmode &
+ CAN_CTRLMODE_BERR_REPORTING))
+ ier &= ~IRQ_BEI;
+
+ return f81604_set_sja1000_register(priv->dev,
+ netdev->dev_id,
+ SJA1000_IER, ier);
+ }
+
+ /* set chip to normal mode */
+ if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+ mod_reg |= MOD_LOM;
+ if (priv->can.ctrlmode & CAN_CTRLMODE_PRESUME_ACK)
+ mod_reg |= MOD_STM;
+
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_MOD, mod_reg);
+ if (status)
+ return status;
+ }
+
+ return -EPERM;
+}
+
+static int f81604_chipset_init(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int status;
+ int i;
+
+ /* set clock divider and output control register */
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_CDR,
+ priv->cdr | CDR_PELICAN);
+ if (status)
+ return status;
+
+ /* set acceptance filter (accept all) */
+ for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_ACCC0 + i, 0);
+ if (status)
+ return status;
+ }
+
+ for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_ACCM0 + i, 0xFF);
+ if (status)
+ return status;
+ }
+
+ return f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_OCR,
+ priv->ocr | OCR_MODE_NORMAL);
+}
+
+static void f81604_unregister_urbs(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int i;
+
+ for (i = 0; i < F81604_MAX_RX_URBS; ++i)
+ usb_kill_urb(priv->read_urb[i]);
+
+ usb_kill_urb(priv->write_urb);
+ usb_kill_urb(priv->int_urb);
+}
+
+static int f81604_register_urbs(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int status, i;
+
+ for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
+ status = usb_submit_urb(priv->read_urb[i], GFP_KERNEL);
+ if (status) {
+ netdev_warn(netdev, "%s: submit rx urb failed: %pe\n",
+ __func__, ERR_PTR(status));
+ return status;
+ }
+ }
+
+ status = usb_submit_urb(priv->int_urb, GFP_KERNEL);
+ if (status) {
+ netdev_warn(netdev, "%s: submit int urb failed: %pe\n",
+ __func__, ERR_PTR(status));
+ return status;
+ }
+
+ return 0;
+}
+
+static int f81604_start(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int status;
+ u8 mode;
+ u8 tmp;
+
+ f81604_unregister_urbs(netdev);
+
+ mode = F81604_RX_AUTO_RELEASE_BUF | F81604_INT_WHEN_CHANGE;
+
+ /* Set TR/AT mode */
+ if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+ mode |= F81604_TX_ONESHOT;
+ else
+ mode |= F81604_TX_NORMAL;
+
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ F81604_CTRL_MODE_REG, mode);
+ if (status)
+ return status;
+
+ /* set reset mode */
+ status = f81604_set_reset_mode(netdev);
+ if (status)
+ return status;
+
+ status = f81604_chipset_init(netdev);
+ if (status)
+ return status;
+
+ /* Clear error counters and error code capture */
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_TXERR, 0);
+ if (status)
+ return status;
+
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_RXERR, 0);
+ if (status)
+ return status;
+
+ /* Read clear for ECC/ALC/IR register */
+ status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_ECC, &tmp);
+ if (status)
+ return status;
+
+ status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_ALC, &tmp);
+ if (status)
+ return status;
+
+ status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_IR, &tmp);
+ if (status)
+ return status;
+
+ status = f81604_register_urbs(netdev);
+ if (status)
+ return status;
+
+ return f81604_set_normal_mode(netdev);
+}
+
+static int f81604_set_bittiming(struct net_device *dev)
+{
+ struct f81604_port_priv *priv = netdev_priv(dev);
+ struct can_bittiming *bt = &priv->can.bittiming;
+ int status = 0;
+ u8 btr0, btr1;
+
+ btr0 = FIELD_PREP(GENMASK(5, 0), bt->brp - 1) |
+ FIELD_PREP(GENMASK(7, 6), bt->sjw - 1);
+
+ btr1 = FIELD_PREP(GENMASK(3, 0), bt->prop_seg + bt->phase_seg1 - 1) |
+ FIELD_PREP(GENMASK(6, 4), bt->phase_seg2 - 1);
+
+ if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+ btr1 |= SJA1000_BTR1_SAMPLE_TRIPLE;
+
+ status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
+ SJA1000_BTR0, btr0);
+ if (status) {
+ netdev_warn(dev, "%s: Set BTR0 failed: %pe\n", __func__,
+ ERR_PTR(status));
+ return status;
+ }
+
+ status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
+ SJA1000_BTR1, btr1);
+ if (status) {
+ netdev_warn(dev, "%s: Set BTR1 failed: %pe\n", __func__,
+ ERR_PTR(status));
+ return status;
+ }
+
+ return 0;
+}
+
+static int f81604_set_mode(struct net_device *netdev, enum can_mode mode)
+{
+ int err;
+
+ switch (mode) {
+ case CAN_MODE_START:
+ err = f81604_start(netdev);
+ if (!err && netif_queue_stopped(netdev))
+ netif_wake_queue(netdev);
+ break;
+
+ default:
+ err = -EOPNOTSUPP;
+ }
+
+ return err;
+}
+
+static void f81604_process_rx_packet(struct urb *urb)
+{
+ struct net_device *netdev = urb->context;
+ struct net_device_stats *stats;
+ struct f81604_bulk_data *ptr;
+ struct can_frame *cf;
+ struct sk_buff *skb;
+ int i, count;
+ u8 *data;
+
+ WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
+
+ data = urb->transfer_buffer;
+ stats = &netdev->stats;
+
+ if (urb->actual_length % F81604_DATA_SIZE)
+ netdev_warn(netdev, "actual_length %% %d != 0 (%d)\n",
+ F81604_DATA_SIZE, urb->actual_length);
+ else if (!urb->actual_length)
+ netdev_warn(netdev, "actual_length = 0 (%d)\n",
+ urb->actual_length);
+
+ count = urb->actual_length / F81604_DATA_SIZE;
+
+ for (i = 0; i < count; ++i) {
+ ptr = (struct f81604_bulk_data *)&data[i * F81604_DATA_SIZE];
+
+ if (ptr->cmd != F81604_CMD_DATA)
+ continue;
+
+ skb = alloc_can_skb(netdev, &cf);
+ if (!skb) {
+ netdev_warn(netdev, "%s: not enough memory", __func__);
+ continue;
+ }
+
+ cf->len = can_cc_dlc2len(ptr->dlc & F81604_DLC_LEN_MASK);
+
+ if (ptr->dlc & F81604_DLC_EFF_BIT) {
+ cf->can_id = be32_to_cpu(ptr->eff.id) >> 3;
+ cf->can_id |= CAN_EFF_FLAG;
+ } else {
+ cf->can_id = be16_to_cpu(ptr->sff.id) >> 5;
+ }
+
+ if (ptr->dlc & F81604_DLC_RTR_BIT)
+ cf->can_id |= CAN_RTR_FLAG;
+ else if (ptr->dlc & F81604_DLC_EFF_BIT)
+ memcpy(cf->data, ptr->eff.data, cf->len);
+ else
+ memcpy(cf->data, ptr->sff.data, cf->len);
+
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
+
+ stats->rx_packets++;
+ netif_rx(skb);
+ }
+}
+
+static void f81604_read_bulk_callback(struct urb *urb)
+{
+ struct net_device *netdev = urb->context;
+ int status;
+
+ if (!netif_device_present(netdev))
+ return;
+
+ if (urb->status)
+ netdev_info(netdev, "%s: URB aborted %pe\n", __func__,
+ ERR_PTR(urb->status));
+
+ switch (urb->status) {
+ case 0: /* success */
+ break;
+
+ case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
+ case -ESHUTDOWN:
+ return;
+
+ default:
+ goto resubmit_urb;
+ }
+
+ f81604_process_rx_packet(urb);
+
+resubmit_urb:
+ status = usb_submit_urb(urb, GFP_ATOMIC);
+ if (status == -ENODEV)
+ netif_device_detach(netdev);
+ else if (status)
+ netdev_err(netdev,
+ "%s: failed to resubmit read bulk urb: %pe\n",
+ __func__, ERR_PTR(status));
+}
+
+static void f81604_write_bulk_callback(struct urb *urb)
+{
+ struct net_device *netdev = urb->context;
+
+ if (!netif_device_present(netdev))
+ return;
+
+ if (urb->status)
+ netdev_info(netdev, "%s: Tx URB error: %pe\n", __func__,
+ ERR_PTR(urb->status));
+}
+
+static void f81604_handle_clear_overrun_work(struct work_struct *work)
+{
+ struct f81604_port_priv *priv;
+ struct net_device *netdev;
+
+ priv = container_of(work, struct f81604_port_priv,
+ handle_clear_overrun_work);
+ netdev = priv->netdev;
+
+ f81604_set_sja1000_register(priv->dev, netdev->dev_id, SJA1000_CMR,
+ CMD_CDO);
+}
+
+static void f81604_handle_clear_reg_work(struct work_struct *work)
+{
+ struct f81604_port_priv *priv;
+ struct net_device *netdev;
+ bool clear_ecc, clear_alc;
+ unsigned long flags;
+ u8 tmp;
+
+ priv = container_of(work, struct f81604_port_priv,
+ handle_clear_reg_work);
+ netdev = priv->netdev;
+
+ spin_lock_irqsave(&priv->lock, flags);
+ clear_alc = priv->need_clear_alc;
+ clear_ecc = priv->need_clear_ecc;
+ priv->need_clear_alc = false;
+ priv->need_clear_ecc = false;
+ spin_unlock_irqrestore(&priv->lock, flags);
+
+ if (clear_alc)
+ f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_ALC, &tmp);
+
+ if (clear_ecc)
+ f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_ECC, &tmp);
+}
+
+static void f81604_handle_tx(struct urb *urb)
+{
+ struct f81604_int_data *data = urb->transfer_buffer;
+ struct net_device *netdev = urb->context;
+ struct net_device_stats *stats;
+ struct f81604_port_priv *priv;
+
+ priv = netdev_priv(netdev);
+ stats = &netdev->stats;
+
+ if (!(data->isrc & IRQ_TI))
+ return;
+
+ /* transmission buffer released */
+ if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
+ !(data->sr & SR_TCS)) {
+ stats->tx_errors++;
+ can_free_echo_skb(netdev, 0, NULL);
+ } else {
+ /* transmission complete */
+ stats->tx_bytes += can_get_echo_skb(netdev, 0, NULL);
+ stats->tx_packets++;
+ }
+
+ netif_wake_queue(netdev);
+}
+
+static void f81604_read_int_callback(struct urb *urb)
+{
+ struct f81604_int_data *data = urb->transfer_buffer;
+ struct net_device *netdev = urb->context;
+ struct net_device_stats *stats;
+ struct f81604_port_priv *priv;
+ enum can_state can_state;
+ enum can_state rx_state;
+ enum can_state tx_state;
+ struct can_frame *cf;
+ struct sk_buff *skb;
+ unsigned long flags;
+ int r;
+
+ priv = netdev_priv(netdev);
+ can_state = priv->can.state;
+ stats = &netdev->stats;
+
+ if (!netif_device_present(netdev))
+ return;
+
+ if (urb->status)
+ netdev_info(netdev, "%s: Int URB aborted: %pe\n", __func__,
+ ERR_PTR(urb->status));
+
+ switch (urb->status) {
+ case 0: /* success */
+ break;
+
+ case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
+ case -ESHUTDOWN:
+ return;
+
+ default:
+ goto resubmit_urb;
+ }
+
+ /* Note: ALC/ECC will not auto clear by read here, must to clear by
+ * read register (via handle_clear_reg_work).
+ */
+
+ /* handle can bus errors */
+ if (data->isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
+ skb = alloc_can_err_skb(netdev, &cf);
+ if (!skb) {
+ netdev_warn(netdev,
+ "no memory to alloc_can_err_skb\n");
+ goto resubmit_urb;
+ }
+
+ cf->can_id |= CAN_ERR_CNT;
+ cf->data[6] = data->txerr;
+ cf->data[7] = data->rxerr;
+
+ if (data->isrc & IRQ_DOI) {
+ /* data overrun interrupt */
+ netdev_dbg(netdev, "data overrun interrupt\n");
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+ stats->rx_over_errors++;
+ stats->rx_errors++;
+
+ schedule_work(&priv->handle_clear_overrun_work);
+ }
+
+ if (data->isrc & IRQ_EI) {
+ /* error warning interrupt */
+ netdev_dbg(netdev, "error warning interrupt\n");
+
+ if (data->sr & SR_BS)
+ can_state = CAN_STATE_BUS_OFF;
+ else if (data->sr & SR_ES)
+ can_state = CAN_STATE_ERROR_WARNING;
+ else
+ can_state = CAN_STATE_ERROR_ACTIVE;
+ }
+
+ if (data->isrc & IRQ_BEI) {
+ /* bus error interrupt */
+ netdev_dbg(netdev, "bus error interrupt\n");
+
+ priv->can.can_stats.bus_error++;
+ stats->rx_errors++;
+
+ cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+ /* set error type */
+ switch (data->ecc & ECC_MASK) {
+ case ECC_BIT:
+ cf->data[2] |= CAN_ERR_PROT_BIT;
+ break;
+ case ECC_FORM:
+ cf->data[2] |= CAN_ERR_PROT_FORM;
+ break;
+ case ECC_STUFF:
+ cf->data[2] |= CAN_ERR_PROT_STUFF;
+ break;
+ default:
+ break;
+ }
+
+ /* set error location */
+ cf->data[3] = data->ecc & ECC_SEG;
+
+ /* Error occurred during transmission? */
+ if ((data->ecc & ECC_DIR) == 0)
+ cf->data[2] |= CAN_ERR_PROT_TX;
+
+ spin_lock_irqsave(&priv->lock, flags);
+ priv->need_clear_ecc = true;
+ spin_unlock_irqrestore(&priv->lock, flags);
+
+ schedule_work(&priv->handle_clear_reg_work);
+ }
+
+ if (data->isrc & IRQ_EPI) {
+ if (can_state == CAN_STATE_ERROR_PASSIVE)
+ can_state = CAN_STATE_ERROR_WARNING;
+ else
+ can_state = CAN_STATE_ERROR_PASSIVE;
+
+ /* error passive interrupt */
+ netdev_dbg(netdev, "error passive interrupt: %d\n",
+ can_state);
+ }
+
+ if (data->isrc & IRQ_ALI) {
+ /* arbitration lost interrupt */
+ netdev_dbg(netdev, "arbitration lost interrupt\n");
+
+ priv->can.can_stats.arbitration_lost++;
+ stats->tx_errors++;
+ cf->can_id |= CAN_ERR_LOSTARB;
+ cf->data[0] = data->alc & ALC_MASK;
+
+ spin_lock_irqsave(&priv->lock, flags);
+ priv->need_clear_alc = true;
+ spin_unlock_irqrestore(&priv->lock, flags);
+
+ schedule_work(&priv->handle_clear_reg_work);
+ }
+
+ if (can_state != priv->can.state) {
+ tx_state = data->txerr >= data->rxerr ? can_state : 0;
+ rx_state = data->txerr <= data->rxerr ? can_state : 0;
+
+ can_change_state(netdev, cf, tx_state, rx_state);
+
+ if (can_state == CAN_STATE_BUS_OFF)
+ can_bus_off(netdev);
+ }
+
+ netif_rx(skb);
+ }
+
+ /* handle TX */
+ if (can_state != CAN_STATE_BUS_OFF)
+ f81604_handle_tx(urb);
+
+resubmit_urb:
+ r = usb_submit_urb(urb, GFP_ATOMIC);
+ if (r) {
+ netdev_err(netdev,
+ "%s: failed resubmitting int bulk urb: %pe\n",
+ __func__, ERR_PTR(r));
+
+ if (r == -ENODEV)
+ netif_device_detach(netdev);
+ }
+}
+
+static netdev_tx_t f81604_start_xmit(struct sk_buff *skb,
+ struct net_device *netdev)
+{
+ struct can_frame *cf = (struct can_frame *)skb->data;
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ struct net_device_stats *stats = &netdev->stats;
+ struct f81604_bulk_data *ptr;
+ int status, len;
+
+ if (can_dropped_invalid_skb(netdev, skb))
+ return NETDEV_TX_OK;
+
+ netif_stop_queue(netdev);
+
+ WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
+
+ ptr = (struct f81604_bulk_data *)priv->bulk_write_buffer;
+ memset(ptr, 0, F81604_DATA_SIZE);
+
+ len = can_get_cc_dlc(cf, priv->can.ctrlmode);
+ ptr->cmd = F81604_CMD_DATA;
+ ptr->dlc = len;
+
+ if (cf->can_id & CAN_RTR_FLAG)
+ ptr->dlc |= F81604_DLC_RTR_BIT;
+
+ if (cf->can_id & CAN_EFF_FLAG) {
+ ptr->eff.id = cpu_to_be32((cf->can_id & CAN_EFF_MASK) << 3);
+ ptr->dlc |= F81604_DLC_EFF_BIT;
+
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ memcpy(&ptr->eff.data, cf->data, len);
+ } else {
+ ptr->sff.id = cpu_to_be16((cf->can_id & CAN_SFF_MASK) << 5);
+
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ memcpy(&ptr->sff.data, cf->data, len);
+ }
+
+ can_put_echo_skb(skb, netdev, 0, 0);
+
+ status = usb_submit_urb(priv->write_urb, GFP_ATOMIC);
+ if (status) {
+ netdev_err(netdev, "%s: failed to resubmit tx bulk urb: %pe\n",
+ __func__, ERR_PTR(status));
+
+ can_free_echo_skb(netdev, 0, NULL);
+ stats->tx_dropped++;
+
+ if (status == -ENODEV)
+ netif_device_detach(netdev);
+ }
+
+ return NETDEV_TX_OK;
+}
+
+static int f81604_get_berr_counter(const struct net_device *netdev,
+ struct can_berr_counter *bec)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int status;
+ u8 txerr;
+ u8 rxerr;
+
+ status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_TXERR, &txerr);
+ if (status)
+ return status;
+
+ status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_RXERR, &rxerr);
+ if (status)
+ return status;
+
+ bec->txerr = txerr;
+ bec->rxerr = rxerr;
+
+ return 0;
+}
+
+static void f81604_remove_urbs(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int i;
+
+ for (i = 0; i < F81604_MAX_RX_URBS; ++i)
+ usb_free_urb(priv->read_urb[i]);
+
+ usb_free_urb(priv->write_urb);
+ usb_free_urb(priv->int_urb);
+}
+
+static int f81604_prepare_urbs(struct net_device *netdev)
+{
+ static const u8 bulk_in_addr[F81604_MAX_DEV] = { 0x82, 0x84 };
+ static const u8 bulk_out_addr[F81604_MAX_DEV] = { 0x01, 0x03 };
+ static const u8 int_in_addr[F81604_MAX_DEV] = { 0x81, 0x83 };
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int id = netdev->dev_id;
+ int i;
+
+ for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
+ priv->read_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
+ if (!priv->read_urb[i])
+ goto error;
+
+ usb_fill_bulk_urb(priv->read_urb[i], priv->dev,
+ usb_rcvbulkpipe(priv->dev, bulk_in_addr[id]),
+ priv->bulk_read_buffer[i], F81604_BULK_SIZE,
+ f81604_read_bulk_callback, netdev);
+ }
+
+ priv->write_urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!priv->write_urb)
+ goto error;
+
+ usb_fill_bulk_urb(priv->write_urb, priv->dev,
+ usb_sndbulkpipe(priv->dev, bulk_out_addr[id]),
+ priv->bulk_write_buffer, F81604_DATA_SIZE,
+ f81604_write_bulk_callback, netdev);
+
+ priv->int_urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!priv->int_urb)
+ goto error;
+
+ usb_fill_int_urb(priv->int_urb, priv->dev,
+ usb_rcvintpipe(priv->dev, int_in_addr[id]),
+ priv->int_read_buffer, F81604_INT_SIZE,
+ f81604_read_int_callback, netdev, 1);
+
+ return 0;
+
+error:
+ f81604_remove_urbs(netdev);
+ return -ENOMEM;
+}
+
+/* Open USB device */
+static int f81604_open(struct net_device *netdev)
+{
+ int err;
+
+ err = open_candev(netdev);
+ if (err)
+ return err;
+
+ err = f81604_start(netdev);
+ if (err)
+ goto start_failed;
+
+ netif_start_queue(netdev);
+ return 0;
+
+start_failed:
+ if (err == -ENODEV)
+ netif_device_detach(netdev);
+
+ close_candev(netdev);
+
+ return err;
+}
+
+/* Close USB device */
+static int f81604_close(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+
+ f81604_set_reset_mode(netdev);
+
+ netif_stop_queue(netdev);
+ cancel_work_sync(&priv->handle_clear_overrun_work);
+ cancel_work_sync(&priv->handle_clear_reg_work);
+ close_candev(netdev);
+
+ f81604_unregister_urbs(netdev);
+
+ return 0;
+}
+
+static const struct net_device_ops f81604_netdev_ops = {
+ .ndo_open = f81604_open,
+ .ndo_stop = f81604_close,
+ .ndo_start_xmit = f81604_start_xmit,
+ .ndo_change_mtu = can_change_mtu,
+};
+
+static const struct can_bittiming_const f81604_bittiming_const = {
+ .name = "f81604",
+ .tseg1_min = 1,
+ .tseg1_max = 16,
+ .tseg2_min = 1,
+ .tseg2_max = 8,
+ .sjw_max = 4,
+ .brp_min = 1,
+ .brp_max = 64,
+ .brp_inc = 1,
+};
+
+/* Called by the usb core when driver is unloaded or device is removed */
+static void f81604_disconnect(struct usb_interface *intf)
+{
+ struct f81604_priv *priv = usb_get_intfdata(intf);
+ int i;
+
+ for (i = 0; i < F81604_MAX_DEV; ++i) {
+ if (!priv->netdev[i])
+ continue;
+
+ unregister_netdev(priv->netdev[i]);
+ f81604_remove_urbs(priv->netdev[i]);
+ free_candev(priv->netdev[i]);
+ }
+}
+
+static int f81604_set_termination(struct net_device *netdev, u16 term)
+{
+ struct f81604_port_priv *port_priv = netdev_priv(netdev);
+ struct f81604_priv *priv;
+ u8 mask, data = 0;
+ int r;
+
+ priv = usb_get_intfdata(port_priv->intf);
+
+ if (netdev->dev_id == 0)
+ mask = F81604_CAN0_TERM;
+ else
+ mask = F81604_CAN1_TERM;
+
+ if (term == F81604_TERMINATION_ENABLED)
+ data = mask;
+
+ mutex_lock(&priv->mutex);
+
+ r = f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
+ mask, data);
+
+ mutex_unlock(&priv->mutex);
+
+ return r;
+}
+
+static int f81604_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ struct usb_device *dev = interface_to_usbdev(intf);
+ struct f81604_port_priv *port_priv;
+ struct net_device *netdev;
+ struct f81604_priv *priv;
+ int i, err;
+
+ priv = devm_kzalloc(&intf->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ usb_set_intfdata(intf, priv);
+
+ for (i = 0; i < F81604_MAX_DEV; ++i) {
+ netdev = alloc_candev(sizeof(*port_priv), 1);
+ if (!netdev) {
+ dev_err(&intf->dev, "Couldn't alloc candev: %d\n", i);
+ err = -ENOMEM;
+
+ goto failure_cleanup;
+ }
+
+ port_priv = netdev_priv(netdev);
+ netdev->dev_id = i;
+
+ mutex_init(&priv->mutex);
+ spin_lock_init(&port_priv->lock);
+
+ INIT_WORK(&port_priv->handle_clear_overrun_work,
+ f81604_handle_clear_overrun_work);
+ INIT_WORK(&port_priv->handle_clear_reg_work,
+ f81604_handle_clear_reg_work);
+
+ port_priv->intf = intf;
+ port_priv->dev = dev;
+ port_priv->ocr = OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL;
+ port_priv->cdr = CDR_CBP;
+ port_priv->can.state = CAN_STATE_STOPPED;
+ port_priv->can.clock.freq = F81604_CAN_CLOCK;
+
+ port_priv->can.termination_const = f81604_termination;
+ port_priv->can.termination_const_cnt =
+ ARRAY_SIZE(f81604_termination);
+ port_priv->can.bittiming_const = &f81604_bittiming_const;
+ port_priv->can.do_set_bittiming = f81604_set_bittiming;
+ port_priv->can.do_set_mode = f81604_set_mode;
+ port_priv->can.do_set_termination = f81604_set_termination;
+ port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
+ port_priv->can.ctrlmode_supported =
+ CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
+ CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
+ CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
+
+ netdev->ethtool_ops = &f81604_ethtool_ops;
+ netdev->netdev_ops = &f81604_netdev_ops;
+ netdev->flags |= IFF_ECHO;
+
+ SET_NETDEV_DEV(netdev, &intf->dev);
+
+ err = f81604_set_termination(netdev,
+ F81604_TERMINATION_DISABLED);
+ if (err)
+ goto clean_candev;
+
+ err = f81604_prepare_urbs(netdev);
+ if (err)
+ goto clean_candev;
+
+ err = register_candev(netdev);
+ if (err)
+ goto clean_candev;
+
+ port_priv->netdev = netdev;
+ priv->netdev[i] = netdev;
+
+ dev_info(&intf->dev, "Channel #%d registered as %s\n", i,
+ netdev->name);
+ }
+
+ return 0;
+
+clean_candev:
+ netdev_err(netdev, "couldn't enable CAN device: %pe\n", ERR_PTR(err));
+ free_candev(netdev);
+
+failure_cleanup:
+ f81604_disconnect(intf);
+ return err;
+}
+
+static struct usb_driver f81604_driver = {
+ .name = "f81604",
+ .probe = f81604_probe,
+ .disconnect = f81604_disconnect,
+ .id_table = f81604_table,
+};
+
+module_usb_driver(f81604_driver);
+
+MODULE_AUTHOR("Ji-Ze Hong (Peter Hong) <[email protected]>");
+MODULE_DESCRIPTION("Fintek F81604 USB to 2xCANBUS");
+MODULE_LICENSE("GPL");
--
2.17.1
Hi Peter
This looks good to me.
On Mon Mar 27, 2023 at 7:10 AM CEST, Ji-Ze Hong (Peter Hong) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> This patch add support for Fintek USB to 2CAN controller support.
>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <[email protected]>
> ---
> Changelog:
> v3:
> 1. Change CAN clock to using MEGA units.
> 2. Remove USB set/get retry, only remain SJA1000 reset/operation retry.
> 3. Fix all numberic constant to define.
> 4. Add terminator control. (only 0 & 120 ohm)
> 5. Using struct data to represent INT/TX/RX endpoints data instead byte
> arrays.
> 6. Error message reports changed from %d to %pe for mnemotechnic values.
> 7. Some bit operations are changed to FIELD_PREP().
> 8. Separate TX functions from f81604_read_int_callback().
> 9. cf->can_id |= CAN_ERR_CNT in f81604_read_int_callback to report valid
> TX/RX error counts.
> 10. Move f81604_prepare_urbs/f81604_remove_urbs() from CAN open/close() to
> USB probe/disconnect().
> 11. coding style refactoring.
>
> v2:
> 1. coding style refactoring.
> 2. some const number are defined to describe itself.
> 3. fix wrong usage for can_get_echo_skb() in f81604_write_bulk_callback().
>
> drivers/net/can/usb/Kconfig | 12 +
> drivers/net/can/usb/Makefile | 1 +
> drivers/net/can/usb/f81604.c | 1232 ++++++++++++++++++++++++++++++++++
> 3 files changed, 1245 insertions(+)
> create mode 100644 drivers/net/can/usb/f81604.c
>
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index 445504ababce..06ab92cd2ea7 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -147,4 +147,16 @@ config CAN_UCAN
> from Theobroma Systems like the A31-µQ7 and the RK3399-Q7
> (https://www.theobroma-systems.com/rk3399-q7)
>
> +config CAN_F81604
> + tristate "Fintek F81604 USB to 2CAN interface"
> + help
> + This driver supports the Fintek F81604 USB to 2CAN interface.
> + The device can support CAN2.0A/B protocol and also support
> + 2 output pins to control external terminator (optional).
> +
> + To compile this driver as a module, choose M here: the module will
> + be called f81604.
> +
> + (see also https://www.fintek.com.tw).
> +
> endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 1ea16be5743b..9de0305a5cce 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/
> obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> obj-$(CONFIG_CAN_UCAN) += ucan.o
> +obj-$(CONFIG_CAN_F81604) += f81604.o
> diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
> new file mode 100644
> index 000000000000..06cd5d95c44d
> --- /dev/null
> +++ b/drivers/net/can/usb/f81604.c
> @@ -0,0 +1,1232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Fintek F81604 USB-to-2CAN controller driver.
> + *
> + * Copyright (C) 2023 Ji-Ze Hong (Peter Hong) <[email protected]>
> + */
> +#include <linux/netdevice.h>
> +#include <linux/usb.h>
> +#include <linux/bitfield.h>
> +#include <linux/units.h>
> +
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/platform/sja1000.h>
> +
> +/* vendor and product id */
> +#define F81604_VENDOR_ID 0x2c42
> +#define F81604_PRODUCT_ID 0x1709
> +#define F81604_CAN_CLOCK (24 * MEGA / 2)
> +#define F81604_MAX_DEV 2
> +#define F81604_SET_DEVICE_RETRY 100
> +
> +#define F81604_USB_TIMEOUT 2000
> +#define F81604_SET_GET_REGISTER 0xA0
> +#define F81604_PORT_OFFSET 0x1000
> +
> +#define F81604_BULK_SIZE 64
> +#define F81604_INT_SIZE 16
> +#define F81604_DATA_SIZE 14
> +#define F81604_MAX_RX_URBS 4
> +
> +#define F81604_CMD_OFFSET 0x00
> +#define F81604_CMD_DATA 0x00
> +
> +#define F81604_DLC_LEN_MASK 0x0f
> +#define F81604_DLC_EFF_BIT BIT(7)
> +#define F81604_DLC_RTR_BIT BIT(6)
> +
> +/* device setting */
> +#define F81604_CTRL_MODE_REG 0x80
> +#define F81604_TX_ONESHOT (0x03 << 3)
> +#define F81604_TX_NORMAL (0x01 << 3)
> +#define F81604_RX_AUTO_RELEASE_BUF BIT(1)
> +#define F81604_INT_WHEN_CHANGE BIT(0)
> +
> +#define F81604_TERMINATOR_REG 0x105
> +#define F81604_CAN0_TERM BIT(2)
> +#define F81604_CAN1_TERM BIT(3)
> +
> +#define F81604_TERMINATION_DISABLED CAN_TERMINATION_DISABLED
> +#define F81604_TERMINATION_ENABLED 120
> +
> +/* SJA1000 registers - manual section 6.4 (Pelican Mode) */
> +#define SJA1000_MOD 0x00
> +#define SJA1000_CMR 0x01
> +#define SJA1000_IR 0x03
> +#define SJA1000_IER 0x04
> +#define SJA1000_ALC 0x0B
> +#define SJA1000_ECC 0x0C
> +#define SJA1000_RXERR 0x0E
> +#define SJA1000_TXERR 0x0F
> +#define SJA1000_ACCC0 0x10
> +#define SJA1000_ACCC1 0x11
> +#define SJA1000_ACCC2 0x12
> +#define SJA1000_ACCC3 0x13
> +#define SJA1000_ACCM0 0x14
> +#define SJA1000_ACCM1 0x15
> +#define SJA1000_ACCM2 0x16
> +#define SJA1000_ACCM3 0x17
> +#define SJA1000_MAX_FILTER_CNT 4
> +
> +/* Common registers - manual section 6.5 */
> +#define SJA1000_BTR0 0x06
> +#define SJA1000_BTR1 0x07
> +#define SJA1000_BTR1_SAMPLE_TRIPLE BIT(7)
> +#define SJA1000_OCR 0x08
> +#define SJA1000_CDR 0x1F
> +
> +/* mode register */
> +#define MOD_RM 0x01
> +#define MOD_LOM 0x02
> +#define MOD_STM 0x04
> +
> +/* commands */
> +#define CMD_CDO 0x08
> +
> +/* interrupt sources */
> +#define IRQ_BEI 0x80
> +#define IRQ_ALI 0x40
> +#define IRQ_EPI 0x20
> +#define IRQ_DOI 0x08
> +#define IRQ_EI 0x04
> +#define IRQ_TI 0x02
> +#define IRQ_RI 0x01
> +#define IRQ_ALL 0xFF
> +#define IRQ_OFF 0x00
> +
> +/* status register content */
> +#define SR_BS 0x80
> +#define SR_ES 0x40
> +#define SR_TS 0x20
> +#define SR_RS 0x10
> +#define SR_TCS 0x08
> +#define SR_TBS 0x04
> +#define SR_DOS 0x02
> +#define SR_RBS 0x01
> +
> +/* ECC register */
> +#define ECC_SEG 0x1F
> +#define ECC_DIR 0x20
> +#define ECC_BIT 0x00
> +#define ECC_FORM 0x40
> +#define ECC_STUFF 0x80
> +#define ECC_MASK 0xc0
> +
> +/* ALC register */
> +#define ALC_MASK 0x1f
> +
> +/* table of devices that work with this driver */
> +static const struct usb_device_id f81604_table[] = {
> + { USB_DEVICE(F81604_VENDOR_ID, F81604_PRODUCT_ID) },
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, f81604_table);
> +
> +static const struct ethtool_ops f81604_ethtool_ops = {
> + .get_ts_info = ethtool_op_get_ts_info,
> +};
> +
> +static const u16 f81604_termination[] = { F81604_TERMINATION_DISABLED,
> + F81604_TERMINATION_ENABLED };
> +
> +struct f81604_priv {
> + struct net_device *netdev[F81604_MAX_DEV];
> + struct mutex mutex; /* for terminator setting */
> +};
> +
> +struct f81604_port_priv {
> + struct can_priv can;
> + struct net_device *netdev;
> + struct sk_buff *echo_skb;
> +
> + /* For synchronize need_clear_alc/need_clear_ecc in worker & interrupt
> + * callback.
> + */
> + spinlock_t lock;
> + bool need_clear_alc;
> + bool need_clear_ecc;
> +
> + struct work_struct handle_clear_reg_work;
> + struct work_struct handle_clear_overrun_work;
> +
> + struct usb_device *dev;
> + struct usb_interface *intf;
> +
> + struct urb *int_urb;
> + u8 int_read_buffer[F81604_INT_SIZE];
> +
> + struct urb *read_urb[F81604_MAX_RX_URBS];
> + u8 bulk_read_buffer[F81604_MAX_RX_URBS][F81604_BULK_SIZE];
> +
> + struct urb *write_urb;
> + u8 bulk_write_buffer[F81604_DATA_SIZE];
> +
> + u8 ocr;
> + u8 cdr;
> +};
> +
> +/* Interrupt endpoint data format: SR/IR/IER/ALC/ECC/EWLR/RXERR/TXERR/VAL */
> +struct f81604_int_data {
> + u8 sr;
> + u8 isrc;
> + u8 ier;
> + u8 alc;
> + u8 ecc;
> + u8 ewlr;
> + u8 rxerr;
> + u8 txerr;
> + u8 val;
> +} __packed;
> +
> +struct f81604_sff {
> + __be16 id;
> + u8 data[CAN_MAX_DLEN];
> +} __packed;
> +
> +struct f81604_eff {
> + __be32 id;
> + u8 data[CAN_MAX_DLEN];
> +} __packed;
> +
> +struct f81604_bulk_data {
> + u8 cmd;
> +
> + /* According for F81604 DLC define:
> + * #define F81604_DLC_LEN_MASK 0x0f
> + * #define F81604_DLC_EFF_BIT BIT(7)
> + * #define F81604_DLC_RTR_BIT BIT(6)
> + */
> + u8 dlc;
> +
> + union {
> + struct f81604_sff sff;
> + struct f81604_eff eff;
> + };
> +} __packed;
> +
> +static int f81604_set_register(struct usb_device *dev, u16 reg, u8 data)
> +{
> + int ret;
> +
> + ret = usb_control_msg_send(dev, 0, F81604_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_OUT, 0, reg,
> + &data, sizeof(data), F81604_USB_TIMEOUT,
> + GFP_KERNEL);
> + if (ret)
> + dev_err(&dev->dev, "%s: reg: %x data: %x failed: %pe\n",
> + __func__, reg, data, ERR_PTR(ret));
> +
> + return ret;
> +}
> +
> +static int f81604_get_register(struct usb_device *dev, u16 reg, u8 *data)
> +{
> + int ret;
> +
> + ret = usb_control_msg_recv(dev, 0, F81604_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_IN, 0, reg, data,
> + sizeof(*data), F81604_USB_TIMEOUT,
> + GFP_KERNEL);
> +
> + if (ret < 0)
> + dev_err(&dev->dev, "%s: reg: %x failed: %pe\n", __func__, reg,
> + ERR_PTR(ret));
> +
> + return ret;
> +}
> +
> +static int f81604_mask_set_register(struct usb_device *dev, u16 reg, u8 mask,
> + u8 data)
> +{
> + int status;
> + u8 tmp;
> +
> + status = f81604_get_register(dev, reg, &tmp);
> + if (status)
> + return status;
> +
> + tmp &= ~mask;
> + tmp |= (mask & data);
> +
> + return f81604_set_register(dev, reg, tmp);
> +}
> +
> +static int f81604_set_sja1000_register(struct usb_device *dev, u8 port,
> + u16 reg, u8 data)
> +{
> + int real_reg;
> +
> + real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
> + return f81604_set_register(dev, real_reg, data);
> +}
> +
> +static int f81604_get_sja1000_register(struct usb_device *dev, u8 port,
> + u16 reg, u8 *data)
> +{
> + int real_reg;
> +
> + real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
> + return f81604_get_register(dev, real_reg, data);
> +}
> +
> +static int f81604_set_reset_mode(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status, i;
> + u8 tmp;
> +
> + /* disable interrupts */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_IER, IRQ_OFF);
> + if (status)
> + return status;
> +
> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, &tmp);
> + if (status)
> + return status;
> +
> + /* check reset bit */
> + if (tmp & MOD_RM) {
> + priv->can.state = CAN_STATE_STOPPED;
> + return 0;
> + }
> +
> + /* reset chip */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, MOD_RM);
> + if (status)
> + return status;
> + }
> +
> + return -EPERM;
> +}
> +
> +static int f81604_set_normal_mode(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + u8 mod_reg = 0x00;
> + u8 tmp, ier = 0;
> + int status, i;
> +
> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, &tmp);
> + if (status)
> + return status;
> +
> + /* check reset bit */
> + if ((tmp & MOD_RM) == 0) {
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> + /* enable interrupts, RI handled by bulk-in */
> + ier = IRQ_ALL & ~IRQ_RI;
> + if (!(priv->can.ctrlmode &
> + CAN_CTRLMODE_BERR_REPORTING))
> + ier &= ~IRQ_BEI;
> +
> + return f81604_set_sja1000_register(priv->dev,
> + netdev->dev_id,
> + SJA1000_IER, ier);
> + }
> +
> + /* set chip to normal mode */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> + mod_reg |= MOD_LOM;
> + if (priv->can.ctrlmode & CAN_CTRLMODE_PRESUME_ACK)
> + mod_reg |= MOD_STM;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, mod_reg);
> + if (status)
> + return status;
> + }
> +
> + return -EPERM;
> +}
> +
> +static int f81604_chipset_init(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + int i;
> +
> + /* set clock divider and output control register */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_CDR,
> + priv->cdr | CDR_PELICAN);
> + if (status)
> + return status;
> +
> + /* set acceptance filter (accept all) */
> + for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ACCC0 + i, 0);
> + if (status)
> + return status;
> + }
> +
> + for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ACCM0 + i, 0xFF);
> + if (status)
> + return status;
> + }
> +
> + return f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_OCR,
> + priv->ocr | OCR_MODE_NORMAL);
> +}
> +
> +static void f81604_unregister_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i)
> + usb_kill_urb(priv->read_urb[i]);
> +
> + usb_kill_urb(priv->write_urb);
> + usb_kill_urb(priv->int_urb);
> +}
> +
> +static int f81604_register_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status, i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
> + status = usb_submit_urb(priv->read_urb[i], GFP_KERNEL);
> + if (status) {
> + netdev_warn(netdev, "%s: submit rx urb failed: %pe\n",
> + __func__, ERR_PTR(status));
> + return status;
> + }
> + }
> +
> + status = usb_submit_urb(priv->int_urb, GFP_KERNEL);
> + if (status) {
> + netdev_warn(netdev, "%s: submit int urb failed: %pe\n",
> + __func__, ERR_PTR(status));
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +static int f81604_start(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + u8 mode;
> + u8 tmp;
> +
> + f81604_unregister_urbs(netdev);
> +
> + mode = F81604_RX_AUTO_RELEASE_BUF | F81604_INT_WHEN_CHANGE;
> +
> + /* Set TR/AT mode */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> + mode |= F81604_TX_ONESHOT;
> + else
> + mode |= F81604_TX_NORMAL;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + F81604_CTRL_MODE_REG, mode);
> + if (status)
> + return status;
> +
> + /* set reset mode */
> + status = f81604_set_reset_mode(netdev);
> + if (status)
> + return status;
> +
> + status = f81604_chipset_init(netdev);
> + if (status)
> + return status;
> +
> + /* Clear error counters and error code capture */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_TXERR, 0);
> + if (status)
> + return status;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_RXERR, 0);
> + if (status)
> + return status;
> +
> + /* Read clear for ECC/ALC/IR register */
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ECC, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ALC, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_IR, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_register_urbs(netdev);
> + if (status)
> + return status;
> +
> + return f81604_set_normal_mode(netdev);
> +}
> +
> +static int f81604_set_bittiming(struct net_device *dev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(dev);
> + struct can_bittiming *bt = &priv->can.bittiming;
> + int status = 0;
> + u8 btr0, btr1;
> +
> + btr0 = FIELD_PREP(GENMASK(5, 0), bt->brp - 1) |
> + FIELD_PREP(GENMASK(7, 6), bt->sjw - 1);
> +
> + btr1 = FIELD_PREP(GENMASK(3, 0), bt->prop_seg + bt->phase_seg1 - 1) |
> + FIELD_PREP(GENMASK(6, 4), bt->phase_seg2 - 1);
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> + btr1 |= SJA1000_BTR1_SAMPLE_TRIPLE;
> +
> + status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
> + SJA1000_BTR0, btr0);
> + if (status) {
> + netdev_warn(dev, "%s: Set BTR0 failed: %pe\n", __func__,
> + ERR_PTR(status));
> + return status;
> + }
> +
> + status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
> + SJA1000_BTR1, btr1);
> + if (status) {
> + netdev_warn(dev, "%s: Set BTR1 failed: %pe\n", __func__,
> + ERR_PTR(status));
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +static int f81604_set_mode(struct net_device *netdev, enum can_mode mode)
> +{
> + int err;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + err = f81604_start(netdev);
> + if (!err && netif_queue_stopped(netdev))
> + netif_wake_queue(netdev);
> + break;
> +
> + default:
> + err = -EOPNOTSUPP;
> + }
> +
> + return err;
> +}
> +
> +static void f81604_process_rx_packet(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_bulk_data *ptr;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + int i, count;
> + u8 *data;
> +
> + WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
> +
> + data = urb->transfer_buffer;
> + stats = &netdev->stats;
> +
> + if (urb->actual_length % F81604_DATA_SIZE)
> + netdev_warn(netdev, "actual_length %% %d != 0 (%d)\n",
> + F81604_DATA_SIZE, urb->actual_length);
> + else if (!urb->actual_length)
> + netdev_warn(netdev, "actual_length = 0 (%d)\n",
> + urb->actual_length);
> +
> + count = urb->actual_length / F81604_DATA_SIZE;
> +
> + for (i = 0; i < count; ++i) {
> + ptr = (struct f81604_bulk_data *)&data[i * F81604_DATA_SIZE];
> +
> + if (ptr->cmd != F81604_CMD_DATA)
> + continue;
> +
> + skb = alloc_can_skb(netdev, &cf);
> + if (!skb) {
> + netdev_warn(netdev, "%s: not enough memory", __func__);
> + continue;
> + }
> +
> + cf->len = can_cc_dlc2len(ptr->dlc & F81604_DLC_LEN_MASK);
> +
> + if (ptr->dlc & F81604_DLC_EFF_BIT) {
> + cf->can_id = be32_to_cpu(ptr->eff.id) >> 3;
> + cf->can_id |= CAN_EFF_FLAG;
> + } else {
> + cf->can_id = be16_to_cpu(ptr->sff.id) >> 5;
> + }
> +
> + if (ptr->dlc & F81604_DLC_RTR_BIT)
> + cf->can_id |= CAN_RTR_FLAG;
> + else if (ptr->dlc & F81604_DLC_EFF_BIT)
> + memcpy(cf->data, ptr->eff.data, cf->len);
> + else
> + memcpy(cf->data, ptr->sff.data, cf->len);
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> +
> + stats->rx_packets++;
> + netif_rx(skb);
> + }
> +}
> +
> +static void f81604_read_bulk_callback(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> + int status;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: URB aborted %pe\n", __func__,
> + ERR_PTR(urb->status));
> +
> + switch (urb->status) {
> + case 0: /* success */
> + break;
> +
> + case -ENOENT:
> + case -EPIPE:
> + case -EPROTO:
> + case -ESHUTDOWN:
> + return;
> +
> + default:
> + goto resubmit_urb;
> + }
> +
> + f81604_process_rx_packet(urb);
> +
> +resubmit_urb:
> + status = usb_submit_urb(urb, GFP_ATOMIC);
> + if (status == -ENODEV)
> + netif_device_detach(netdev);
> + else if (status)
> + netdev_err(netdev,
> + "%s: failed to resubmit read bulk urb: %pe\n",
> + __func__, ERR_PTR(status));
> +}
> +
> +static void f81604_write_bulk_callback(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: Tx URB error: %pe\n", __func__,
> + ERR_PTR(urb->status));
> +}
> +
> +static void f81604_handle_clear_overrun_work(struct work_struct *work)
> +{
> + struct f81604_port_priv *priv;
> + struct net_device *netdev;
> +
> + priv = container_of(work, struct f81604_port_priv,
> + handle_clear_overrun_work);
> + netdev = priv->netdev;
> +
> + f81604_set_sja1000_register(priv->dev, netdev->dev_id, SJA1000_CMR,
> + CMD_CDO);
> +}
> +
> +static void f81604_handle_clear_reg_work(struct work_struct *work)
> +{
> + struct f81604_port_priv *priv;
> + struct net_device *netdev;
> + bool clear_ecc, clear_alc;
> + unsigned long flags;
> + u8 tmp;
> +
> + priv = container_of(work, struct f81604_port_priv,
> + handle_clear_reg_work);
> + netdev = priv->netdev;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + clear_alc = priv->need_clear_alc;
> + clear_ecc = priv->need_clear_ecc;
> + priv->need_clear_alc = false;
> + priv->need_clear_ecc = false;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + if (clear_alc)
> + f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ALC, &tmp);
> +
> + if (clear_ecc)
> + f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ECC, &tmp);
> +}
> +
> +static void f81604_handle_tx(struct urb *urb)
> +{
> + struct f81604_int_data *data = urb->transfer_buffer;
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_port_priv *priv;
> +
> + priv = netdev_priv(netdev);
> + stats = &netdev->stats;
> +
> + if (!(data->isrc & IRQ_TI))
> + return;
> +
> + /* transmission buffer released */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
> + !(data->sr & SR_TCS)) {
> + stats->tx_errors++;
> + can_free_echo_skb(netdev, 0, NULL);
> + } else {
> + /* transmission complete */
> + stats->tx_bytes += can_get_echo_skb(netdev, 0, NULL);
> + stats->tx_packets++;
> + }
> +
> + netif_wake_queue(netdev);
> +}
> +
> +static void f81604_read_int_callback(struct urb *urb)
> +{
> + struct f81604_int_data *data = urb->transfer_buffer;
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_port_priv *priv;
> + enum can_state can_state;
> + enum can_state rx_state;
> + enum can_state tx_state;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + unsigned long flags;
> + int r;
> +
> + priv = netdev_priv(netdev);
> + can_state = priv->can.state;
> + stats = &netdev->stats;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: Int URB aborted: %pe\n", __func__,
> + ERR_PTR(urb->status));
> +
> + switch (urb->status) {
> + case 0: /* success */
> + break;
> +
> + case -ENOENT:
> + case -EPIPE:
> + case -EPROTO:
> + case -ESHUTDOWN:
> + return;
> +
> + default:
> + goto resubmit_urb;
> + }
> +
> + /* Note: ALC/ECC will not auto clear by read here, must to clear by
> + * read register (via handle_clear_reg_work).
> + */
> +
> + /* handle can bus errors */
> + if (data->isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
> + skb = alloc_can_err_skb(netdev, &cf);
> + if (!skb) {
> + netdev_warn(netdev,
> + "no memory to alloc_can_err_skb\n");
> + goto resubmit_urb;
> + }
> +
> + cf->can_id |= CAN_ERR_CNT;
> + cf->data[6] = data->txerr;
> + cf->data[7] = data->rxerr;
> +
> + if (data->isrc & IRQ_DOI) {
> + /* data overrun interrupt */
> + netdev_dbg(netdev, "data overrun interrupt\n");
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> + stats->rx_over_errors++;
> + stats->rx_errors++;
> +
> + schedule_work(&priv->handle_clear_overrun_work);
> + }
> +
> + if (data->isrc & IRQ_EI) {
> + /* error warning interrupt */
> + netdev_dbg(netdev, "error warning interrupt\n");
> +
> + if (data->sr & SR_BS)
> + can_state = CAN_STATE_BUS_OFF;
> + else if (data->sr & SR_ES)
> + can_state = CAN_STATE_ERROR_WARNING;
> + else
> + can_state = CAN_STATE_ERROR_ACTIVE;
> + }
> +
> + if (data->isrc & IRQ_BEI) {
> + /* bus error interrupt */
> + netdev_dbg(netdev, "bus error interrupt\n");
> +
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> +
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + /* set error type */
> + switch (data->ecc & ECC_MASK) {
> + case ECC_BIT:
> + cf->data[2] |= CAN_ERR_PROT_BIT;
> + break;
> + case ECC_FORM:
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> + case ECC_STUFF:
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> + default:
> + break;
> + }
> +
> + /* set error location */
> + cf->data[3] = data->ecc & ECC_SEG;
> +
> + /* Error occurred during transmission? */
> + if ((data->ecc & ECC_DIR) == 0)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->need_clear_ecc = true;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + schedule_work(&priv->handle_clear_reg_work);
> + }
> +
> + if (data->isrc & IRQ_EPI) {
> + if (can_state == CAN_STATE_ERROR_PASSIVE)
> + can_state = CAN_STATE_ERROR_WARNING;
> + else
> + can_state = CAN_STATE_ERROR_PASSIVE;
> +
> + /* error passive interrupt */
> + netdev_dbg(netdev, "error passive interrupt: %d\n",
> + can_state);
> + }
> +
> + if (data->isrc & IRQ_ALI) {
> + /* arbitration lost interrupt */
> + netdev_dbg(netdev, "arbitration lost interrupt\n");
> +
> + priv->can.can_stats.arbitration_lost++;
> + stats->tx_errors++;
> + cf->can_id |= CAN_ERR_LOSTARB;
> + cf->data[0] = data->alc & ALC_MASK;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->need_clear_alc = true;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + schedule_work(&priv->handle_clear_reg_work);
> + }
> +
> + if (can_state != priv->can.state) {
> + tx_state = data->txerr >= data->rxerr ? can_state : 0;
> + rx_state = data->txerr <= data->rxerr ? can_state : 0;
> +
> + can_change_state(netdev, cf, tx_state, rx_state);
> +
> + if (can_state == CAN_STATE_BUS_OFF)
> + can_bus_off(netdev);
> + }
> +
> + netif_rx(skb);
> + }
> +
> + /* handle TX */
> + if (can_state != CAN_STATE_BUS_OFF)
> + f81604_handle_tx(urb);
> +
> +resubmit_urb:
> + r = usb_submit_urb(urb, GFP_ATOMIC);
> + if (r) {
> + netdev_err(netdev,
> + "%s: failed resubmitting int bulk urb: %pe\n",
> + __func__, ERR_PTR(r));
> +
> + if (r == -ENODEV)
> + netif_device_detach(netdev);
> + }
> +}
> +
> +static netdev_tx_t f81604_start_xmit(struct sk_buff *skb,
> + struct net_device *netdev)
> +{
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + struct net_device_stats *stats = &netdev->stats;
> + struct f81604_bulk_data *ptr;
> + int status, len;
> +
> + if (can_dropped_invalid_skb(netdev, skb))
> + return NETDEV_TX_OK;
> +
> + netif_stop_queue(netdev);
> +
> + WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
> +
> + ptr = (struct f81604_bulk_data *)priv->bulk_write_buffer;
> + memset(ptr, 0, F81604_DATA_SIZE);
> +
> + len = can_get_cc_dlc(cf, priv->can.ctrlmode);
> + ptr->cmd = F81604_CMD_DATA;
> + ptr->dlc = len;
> +
> + if (cf->can_id & CAN_RTR_FLAG)
> + ptr->dlc |= F81604_DLC_RTR_BIT;
> +
> + if (cf->can_id & CAN_EFF_FLAG) {
> + ptr->eff.id = cpu_to_be32((cf->can_id & CAN_EFF_MASK) << 3);
> + ptr->dlc |= F81604_DLC_EFF_BIT;
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + memcpy(&ptr->eff.data, cf->data, len);
> + } else {
> + ptr->sff.id = cpu_to_be16((cf->can_id & CAN_SFF_MASK) << 5);
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + memcpy(&ptr->sff.data, cf->data, len);
> + }
> +
> + can_put_echo_skb(skb, netdev, 0, 0);
> +
> + status = usb_submit_urb(priv->write_urb, GFP_ATOMIC);
> + if (status) {
> + netdev_err(netdev, "%s: failed to resubmit tx bulk urb: %pe\n",
> + __func__, ERR_PTR(status));
> +
> + can_free_echo_skb(netdev, 0, NULL);
> + stats->tx_dropped++;
> +
> + if (status == -ENODEV)
> + netif_device_detach(netdev);
> + }
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static int f81604_get_berr_counter(const struct net_device *netdev,
> + struct can_berr_counter *bec)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + u8 txerr;
> + u8 rxerr;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_TXERR, &txerr);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_RXERR, &rxerr);
> + if (status)
> + return status;
> +
> + bec->txerr = txerr;
> + bec->rxerr = rxerr;
> +
> + return 0;
> +}
> +
> +static void f81604_remove_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i)
> + usb_free_urb(priv->read_urb[i]);
> +
> + usb_free_urb(priv->write_urb);
> + usb_free_urb(priv->int_urb);
> +}
> +
> +static int f81604_prepare_urbs(struct net_device *netdev)
> +{
> + static const u8 bulk_in_addr[F81604_MAX_DEV] = { 0x82, 0x84 };
> + static const u8 bulk_out_addr[F81604_MAX_DEV] = { 0x01, 0x03 };
> + static const u8 int_in_addr[F81604_MAX_DEV] = { 0x81, 0x83 };
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int id = netdev->dev_id;
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
> + priv->read_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->read_urb[i])
> + goto error;
> +
> + usb_fill_bulk_urb(priv->read_urb[i], priv->dev,
> + usb_rcvbulkpipe(priv->dev, bulk_in_addr[id]),
> + priv->bulk_read_buffer[i], F81604_BULK_SIZE,
> + f81604_read_bulk_callback, netdev);
> + }
> +
> + priv->write_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->write_urb)
> + goto error;
> +
> + usb_fill_bulk_urb(priv->write_urb, priv->dev,
> + usb_sndbulkpipe(priv->dev, bulk_out_addr[id]),
> + priv->bulk_write_buffer, F81604_DATA_SIZE,
> + f81604_write_bulk_callback, netdev);
> +
> + priv->int_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->int_urb)
> + goto error;
> +
> + usb_fill_int_urb(priv->int_urb, priv->dev,
> + usb_rcvintpipe(priv->dev, int_in_addr[id]),
> + priv->int_read_buffer, F81604_INT_SIZE,
> + f81604_read_int_callback, netdev, 1);
> +
> + return 0;
> +
> +error:
> + f81604_remove_urbs(netdev);
> + return -ENOMEM;
> +}
> +
> +/* Open USB device */
> +static int f81604_open(struct net_device *netdev)
> +{
> + int err;
> +
> + err = open_candev(netdev);
> + if (err)
> + return err;
> +
> + err = f81604_start(netdev);
> + if (err)
> + goto start_failed;
> +
> + netif_start_queue(netdev);
> + return 0;
> +
> +start_failed:
> + if (err == -ENODEV)
> + netif_device_detach(netdev);
> +
> + close_candev(netdev);
> +
> + return err;
> +}
> +
> +/* Close USB device */
> +static int f81604_close(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> +
> + f81604_set_reset_mode(netdev);
> +
> + netif_stop_queue(netdev);
> + cancel_work_sync(&priv->handle_clear_overrun_work);
> + cancel_work_sync(&priv->handle_clear_reg_work);
> + close_candev(netdev);
> +
> + f81604_unregister_urbs(netdev);
> +
> + return 0;
> +}
> +
> +static const struct net_device_ops f81604_netdev_ops = {
> + .ndo_open = f81604_open,
> + .ndo_stop = f81604_close,
> + .ndo_start_xmit = f81604_start_xmit,
> + .ndo_change_mtu = can_change_mtu,
> +};
> +
> +static const struct can_bittiming_const f81604_bittiming_const = {
> + .name = "f81604",
> + .tseg1_min = 1,
> + .tseg1_max = 16,
> + .tseg2_min = 1,
> + .tseg2_max = 8,
> + .sjw_max = 4,
> + .brp_min = 1,
> + .brp_max = 64,
> + .brp_inc = 1,
> +};
> +
> +/* Called by the usb core when driver is unloaded or device is removed */
> +static void f81604_disconnect(struct usb_interface *intf)
> +{
> + struct f81604_priv *priv = usb_get_intfdata(intf);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_DEV; ++i) {
> + if (!priv->netdev[i])
> + continue;
> +
> + unregister_netdev(priv->netdev[i]);
> + f81604_remove_urbs(priv->netdev[i]);
> + free_candev(priv->netdev[i]);
> + }
> +}
> +
> +static int f81604_set_termination(struct net_device *netdev, u16 term)
> +{
> + struct f81604_port_priv *port_priv = netdev_priv(netdev);
> + struct f81604_priv *priv;
> + u8 mask, data = 0;
> + int r;
> +
> + priv = usb_get_intfdata(port_priv->intf);
> +
> + if (netdev->dev_id == 0)
> + mask = F81604_CAN0_TERM;
> + else
> + mask = F81604_CAN1_TERM;
> +
> + if (term == F81604_TERMINATION_ENABLED)
> + data = mask;
> +
> + mutex_lock(&priv->mutex);
> +
> + r = f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
> + mask, data);
> +
> + mutex_unlock(&priv->mutex);
> +
> + return r;
> +}
> +
> +static int f81604_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *dev = interface_to_usbdev(intf);
> + struct f81604_port_priv *port_priv;
> + struct net_device *netdev;
> + struct f81604_priv *priv;
> + int i, err;
> +
> + priv = devm_kzalloc(&intf->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + usb_set_intfdata(intf, priv);
> +
> + for (i = 0; i < F81604_MAX_DEV; ++i) {
> + netdev = alloc_candev(sizeof(*port_priv), 1);
> + if (!netdev) {
> + dev_err(&intf->dev, "Couldn't alloc candev: %d\n", i);
> + err = -ENOMEM;
> +
> + goto failure_cleanup;
> + }
> +
> + port_priv = netdev_priv(netdev);
> + netdev->dev_id = i;
> +
> + mutex_init(&priv->mutex);
> + spin_lock_init(&port_priv->lock);
> +
> + INIT_WORK(&port_priv->handle_clear_overrun_work,
> + f81604_handle_clear_overrun_work);
> + INIT_WORK(&port_priv->handle_clear_reg_work,
> + f81604_handle_clear_reg_work);
> +
> + port_priv->intf = intf;
> + port_priv->dev = dev;
> + port_priv->ocr = OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL;
> + port_priv->cdr = CDR_CBP;
> + port_priv->can.state = CAN_STATE_STOPPED;
> + port_priv->can.clock.freq = F81604_CAN_CLOCK;
> +
> + port_priv->can.termination_const = f81604_termination;
> + port_priv->can.termination_const_cnt =
> + ARRAY_SIZE(f81604_termination);
> + port_priv->can.bittiming_const = &f81604_bittiming_const;
> + port_priv->can.do_set_bittiming = f81604_set_bittiming;
> + port_priv->can.do_set_mode = f81604_set_mode;
> + port_priv->can.do_set_termination = f81604_set_termination;
> + port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
> + port_priv->can.ctrlmode_supported =
> + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
> + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
> + CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
> +
> + netdev->ethtool_ops = &f81604_ethtool_ops;
> + netdev->netdev_ops = &f81604_netdev_ops;
> + netdev->flags |= IFF_ECHO;
> +
> + SET_NETDEV_DEV(netdev, &intf->dev);
> +
> + err = f81604_set_termination(netdev,
> + F81604_TERMINATION_DISABLED);
> + if (err)
> + goto clean_candev;
> +
> + err = f81604_prepare_urbs(netdev);
> + if (err)
> + goto clean_candev;
> +
> + err = register_candev(netdev);
> + if (err)
> + goto clean_candev;
> +
> + port_priv->netdev = netdev;
> + priv->netdev[i] = netdev;
> +
> + dev_info(&intf->dev, "Channel #%d registered as %s\n", i,
> + netdev->name);
> + }
> +
> + return 0;
> +
> +clean_candev:
> + netdev_err(netdev, "couldn't enable CAN device: %pe\n", ERR_PTR(err));
> + free_candev(netdev);
> +
> +failure_cleanup:
> + f81604_disconnect(intf);
> + return err;
> +}
> +
> +static struct usb_driver f81604_driver = {
> + .name = "f81604",
> + .probe = f81604_probe,
> + .disconnect = f81604_disconnect,
> + .id_table = f81604_table,
> +};
> +
> +module_usb_driver(f81604_driver);
> +
> +MODULE_AUTHOR("Ji-Ze Hong (Peter Hong) <[email protected]>");
> +MODULE_DESCRIPTION("Fintek F81604 USB to 2xCANBUS");
> +MODULE_LICENSE("GPL");
> --
> 2.17.1
Reviewed-by: Steen Hegelund <[email protected]>
BR
Steen
Hi Peter,
Thanks for addressing my comments. Here is my feedback for v2.
Compiling with W=12 throws:
drivers/net/can/usb/f81604.c:105: warning: macro "SR_DOS" is not
used [-Wunused-macros]
105 | #define SR_DOS 0x02
|
drivers/net/can/usb/f81604.c:102: warning: macro "SR_RS" is not used
[-Wunused-macros]
102 | #define SR_RS 0x10
|
drivers/net/can/usb/f81604.c:101: warning: macro "SR_TS" is not used
[-Wunused-macros]
101 | #define SR_TS 0x20
|
drivers/net/can/usb/f81604.c:32: warning: macro "F81604_CMD_OFFSET"
is not used [-Wunused-macros]
32 | #define F81604_CMD_OFFSET 0x00
|
drivers/net/can/usb/f81604.c:63: warning: macro "SJA1000_ACCC1" is
not used [-Wunused-macros]
63 | #define SJA1000_ACCC1 0x11
|
drivers/net/can/usb/f81604.c:65: warning: macro "SJA1000_ACCC3" is
not used [-Wunused-macros]
65 | #define SJA1000_ACCC3 0x13
|
drivers/net/can/usb/f81604.c:67: warning: macro "SJA1000_ACCM1" is
not used [-Wunused-macros]
67 | #define SJA1000_ACCM1 0x15
|
drivers/net/can/usb/f81604.c:68: warning: macro "SJA1000_ACCM2" is
not used [-Wunused-macros]
68 | #define SJA1000_ACCM2 0x16
|
drivers/net/can/usb/f81604.c:69: warning: macro "SJA1000_ACCM3" is
not used [-Wunused-macros]
69 | #define SJA1000_ACCM3 0x17
|
drivers/net/can/usb/f81604.c:64: warning: macro "SJA1000_ACCC2" is
not used [-Wunused-macros]
64 | #define SJA1000_ACCC2 0x12
|
drivers/net/can/usb/f81604.c:106: warning: macro "SR_RBS" is not
used [-Wunused-macros]
106 | #define SR_RBS 0x01
|
drivers/net/can/usb/f81604.c:104: warning: macro "SR_TBS" is not
used [-Wunused-macros]
104 | #define SR_TBS 0x04
|
Please consider removing the unused macro.
On Mon. 27 Mar 2023 at 14:37, Ji-Ze Hong (Peter Hong)
<[email protected]> wrote:
> This patch add support for Fintek USB to 2CAN controller support.
^^^^^^^ ^^^^^^^
Do not repeat "support".
Also "add" -> "adds".
>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <[email protected]>
> ---
> Changelog:
> v3:
> 1. Change CAN clock to using MEGA units.
> 2. Remove USB set/get retry, only remain SJA1000 reset/operation retry.
> 3. Fix all numberic constant to define.
> 4. Add terminator control. (only 0 & 120 ohm)
> 5. Using struct data to represent INT/TX/RX endpoints data instead byte
> arrays.
> 6. Error message reports changed from %d to %pe for mnemotechnic values.
> 7. Some bit operations are changed to FIELD_PREP().
> 8. Separate TX functions from f81604_read_int_callback().
> 9. cf->can_id |= CAN_ERR_CNT in f81604_read_int_callback to report valid
> TX/RX error counts.
> 10. Move f81604_prepare_urbs/f81604_remove_urbs() from CAN open/close() to
> USB probe/disconnect().
> 11. coding style refactoring.
>
> v2:
> 1. coding style refactoring.
> 2. some const number are defined to describe itself.
> 3. fix wrong usage for can_get_echo_skb() in f81604_write_bulk_callback().
>
> drivers/net/can/usb/Kconfig | 12 +
> drivers/net/can/usb/Makefile | 1 +
> drivers/net/can/usb/f81604.c | 1232 ++++++++++++++++++++++++++++++++++
> 3 files changed, 1245 insertions(+)
> create mode 100644 drivers/net/can/usb/f81604.c
>
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index 445504ababce..06ab92cd2ea7 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -147,4 +147,16 @@ config CAN_UCAN
> from Theobroma Systems like the A31-µQ7 and the RK3399-Q7
> (https://www.theobroma-systems.com/rk3399-q7)
>
> +config CAN_F81604
Please sort the entries by alphabetical order.
> + tristate "Fintek F81604 USB to 2CAN interface"
> + help
> + This driver supports the Fintek F81604 USB to 2CAN interface.
> + The device can support CAN2.0A/B protocol and also support
> + 2 output pins to control external terminator (optional).
> +
> + To compile this driver as a module, choose M here: the module will
> + be called f81604.
> +
> + (see also https://www.fintek.com.tw).
> +
> endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 1ea16be5743b..9de0305a5cce 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/
> obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> obj-$(CONFIG_CAN_UCAN) += ucan.o
> +obj-$(CONFIG_CAN_F81604) += f81604.o
Please sort the targets by alphabetical order.
> diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
> new file mode 100644
> index 000000000000..06cd5d95c44d
> --- /dev/null
> +++ b/drivers/net/can/usb/f81604.c
> @@ -0,0 +1,1232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Fintek F81604 USB-to-2CAN controller driver.
> + *
> + * Copyright (C) 2023 Ji-Ze Hong (Peter Hong) <[email protected]>
> + */
> +#include <linux/netdevice.h>
> +#include <linux/usb.h>
> +#include <linux/bitfield.h>
> +#include <linux/units.h>
Please sort the includes by alphabetical order.
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/platform/sja1000.h>
> +
> +/* vendor and product id */
> +#define F81604_VENDOR_ID 0x2c42
> +#define F81604_PRODUCT_ID 0x1709
> +#define F81604_CAN_CLOCK (24 * MEGA / 2)
> +#define F81604_MAX_DEV 2
> +#define F81604_SET_DEVICE_RETRY 100
> +
> +#define F81604_USB_TIMEOUT 2000
> +#define F81604_SET_GET_REGISTER 0xA0
> +#define F81604_PORT_OFFSET 0x1000
> +
> +#define F81604_BULK_SIZE 64
> +#define F81604_INT_SIZE 16
> +#define F81604_DATA_SIZE 14
> +#define F81604_MAX_RX_URBS 4
> +
> +#define F81604_CMD_OFFSET 0x00
> +#define F81604_CMD_DATA 0x00
> +
> +#define F81604_DLC_LEN_MASK 0x0f
> +#define F81604_DLC_EFF_BIT BIT(7)
> +#define F81604_DLC_RTR_BIT BIT(6)
> +
> +/* device setting */
> +#define F81604_CTRL_MODE_REG 0x80
> +#define F81604_TX_ONESHOT (0x03 << 3)
> +#define F81604_TX_NORMAL (0x01 << 3)
> +#define F81604_RX_AUTO_RELEASE_BUF BIT(1)
> +#define F81604_INT_WHEN_CHANGE BIT(0)
> +
> +#define F81604_TERMINATOR_REG 0x105
> +#define F81604_CAN0_TERM BIT(2)
> +#define F81604_CAN1_TERM BIT(3)
> +
> +#define F81604_TERMINATION_DISABLED CAN_TERMINATION_DISABLED
> +#define F81604_TERMINATION_ENABLED 120
> +
> +/* SJA1000 registers - manual section 6.4 (Pelican Mode) */
> +#define SJA1000_MOD 0x00
> +#define SJA1000_CMR 0x01
> +#define SJA1000_IR 0x03
> +#define SJA1000_IER 0x04
> +#define SJA1000_ALC 0x0B
> +#define SJA1000_ECC 0x0C
> +#define SJA1000_RXERR 0x0E
> +#define SJA1000_TXERR 0x0F
> +#define SJA1000_ACCC0 0x10
> +#define SJA1000_ACCC1 0x11
> +#define SJA1000_ACCC2 0x12
> +#define SJA1000_ACCC3 0x13
> +#define SJA1000_ACCM0 0x14
> +#define SJA1000_ACCM1 0x15
> +#define SJA1000_ACCM2 0x16
> +#define SJA1000_ACCM3 0x17
> +#define SJA1000_MAX_FILTER_CNT 4
> +
> +/* Common registers - manual section 6.5 */
> +#define SJA1000_BTR0 0x06
> +#define SJA1000_BTR1 0x07
> +#define SJA1000_BTR1_SAMPLE_TRIPLE BIT(7)
> +#define SJA1000_OCR 0x08
> +#define SJA1000_CDR 0x1F
> +
> +/* mode register */
> +#define MOD_RM 0x01
> +#define MOD_LOM 0x02
> +#define MOD_STM 0x04
> +
> +/* commands */
> +#define CMD_CDO 0x08
> +
> +/* interrupt sources */
> +#define IRQ_BEI 0x80
> +#define IRQ_ALI 0x40
> +#define IRQ_EPI 0x20
> +#define IRQ_DOI 0x08
> +#define IRQ_EI 0x04
> +#define IRQ_TI 0x02
> +#define IRQ_RI 0x01
> +#define IRQ_ALL 0xFF
> +#define IRQ_OFF 0x00
> +
> +/* status register content */
> +#define SR_BS 0x80
> +#define SR_ES 0x40
> +#define SR_TS 0x20
> +#define SR_RS 0x10
> +#define SR_TCS 0x08
> +#define SR_TBS 0x04
> +#define SR_DOS 0x02
> +#define SR_RBS 0x01
> +
> +/* ECC register */
> +#define ECC_SEG 0x1F
> +#define ECC_DIR 0x20
> +#define ECC_BIT 0x00
> +#define ECC_FORM 0x40
> +#define ECC_STUFF 0x80
> +#define ECC_MASK 0xc0
> +
> +/* ALC register */
> +#define ALC_MASK 0x1f
> +
> +/* table of devices that work with this driver */
> +static const struct usb_device_id f81604_table[] = {
> + { USB_DEVICE(F81604_VENDOR_ID, F81604_PRODUCT_ID) },
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, f81604_table);
> +
> +static const struct ethtool_ops f81604_ethtool_ops = {
> + .get_ts_info = ethtool_op_get_ts_info,
> +};
> +
> +static const u16 f81604_termination[] = { F81604_TERMINATION_DISABLED,
> + F81604_TERMINATION_ENABLED };
> +
> +struct f81604_priv {
> + struct net_device *netdev[F81604_MAX_DEV];
> + struct mutex mutex; /* for terminator setting */
> +};
> +
> +struct f81604_port_priv {
> + struct can_priv can;
> + struct net_device *netdev;
> + struct sk_buff *echo_skb;
> +
> + /* For synchronize need_clear_alc/need_clear_ecc in worker & interrupt
> + * callback.
> + */
> + spinlock_t lock;
> + bool need_clear_alc;
> + bool need_clear_ecc;
Did you consider using an atomic bitmask instead of the spinlock?
https://www.kernel.org/doc/html/latest/kernel-hacking/hacking.html?highlight=test_and_clear_bit#atomic-operations
> + struct work_struct handle_clear_reg_work;
> + struct work_struct handle_clear_overrun_work;
> +
> + struct usb_device *dev;
> + struct usb_interface *intf;
> +
> + struct urb *int_urb;
> + u8 int_read_buffer[F81604_INT_SIZE];
> +
> + struct urb *read_urb[F81604_MAX_RX_URBS];
> + u8 bulk_read_buffer[F81604_MAX_RX_URBS][F81604_BULK_SIZE];
> +
> + struct urb *write_urb;
> + u8 bulk_write_buffer[F81604_DATA_SIZE];
> +
> + u8 ocr;
> + u8 cdr;
> +};
> +
> +/* Interrupt endpoint data format: SR/IR/IER/ALC/ECC/EWLR/RXERR/TXERR/VAL */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This part of the comment is not helpful. It would be better to explain
the different acronyms (SR, IR, IER...)
> +struct f81604_int_data {
> + u8 sr;
> + u8 isrc;
> + u8 ier;
> + u8 alc;
> + u8 ecc;
> + u8 ewlr;
> + u8 rxerr;
> + u8 txerr;
> + u8 val;
> +} __packed;
> +
> +struct f81604_sff {
> + __be16 id;
> + u8 data[CAN_MAX_DLEN];
> +} __packed;
> +
> +struct f81604_eff {
> + __be32 id;
> + u8 data[CAN_MAX_DLEN];
> +} __packed;
> +
> +struct f81604_bulk_data {
Choose something more explicit than bulk_data (e.g. can_frame).
> + u8 cmd;
> +
> + /* According for F81604 DLC define:
> + * #define F81604_DLC_LEN_MASK 0x0f
> + * #define F81604_DLC_EFF_BIT BIT(7)
> + * #define F81604_DLC_RTR_BIT BIT(6)
> + */
> + u8 dlc;
> +
> + union {
This union is aligned on 16 bits.
> + struct f81604_sff sff;
sff->id is a 16 bit value so it is aligned (as long as struct
f81604_bulk_data is aligned).
> + struct f81604_eff eff;
eff->id is a 32 bit value. It is not aligned. So, you must always use
{get|set}_unaligned_be32() to manipulate this value.
N.B. on x86 architecture, unaligned access is fine, but some other
architecture may throw a fault. Read this for more details:
https://docs.kernel.org/arm/mem_alignment.html
> + };
> +} __packed;
> +
> +static int f81604_set_register(struct usb_device *dev, u16 reg, u8 data)
> +{
> + int ret;
> +
> + ret = usb_control_msg_send(dev, 0, F81604_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_OUT, 0, reg,
> + &data, sizeof(data), F81604_USB_TIMEOUT,
> + GFP_KERNEL);
> + if (ret)
> + dev_err(&dev->dev, "%s: reg: %x data: %x failed: %pe\n",
> + __func__, reg, data, ERR_PTR(ret));
> +
> + return ret;
> +}
> +
> +static int f81604_get_register(struct usb_device *dev, u16 reg, u8 *data)
> +{
> + int ret;
> +
> + ret = usb_control_msg_recv(dev, 0, F81604_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_IN, 0, reg, data,
> + sizeof(*data), F81604_USB_TIMEOUT,
> + GFP_KERNEL);
> +
> + if (ret < 0)
> + dev_err(&dev->dev, "%s: reg: %x failed: %pe\n", __func__, reg,
> + ERR_PTR(ret));
> +
> + return ret;
> +}
> +
> +static int f81604_mask_set_register(struct usb_device *dev, u16 reg, u8 mask,
> + u8 data)
> +{
> + int status;
> + u8 tmp;
> +
> + status = f81604_get_register(dev, reg, &tmp);
> + if (status)
> + return status;
> +
> + tmp &= ~mask;
> + tmp |= (mask & data);
> +
> + return f81604_set_register(dev, reg, tmp);
> +}
> +
> +static int f81604_set_sja1000_register(struct usb_device *dev, u8 port,
> + u16 reg, u8 data)
> +{
> + int real_reg;
> +
> + real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
> + return f81604_set_register(dev, real_reg, data);
> +}
> +
> +static int f81604_get_sja1000_register(struct usb_device *dev, u8 port,
> + u16 reg, u8 *data)
> +{
> + int real_reg;
> +
> + real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
> + return f81604_get_register(dev, real_reg, data);
> +}
> +
> +static int f81604_set_reset_mode(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status, i;
> + u8 tmp;
> +
> + /* disable interrupts */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_IER, IRQ_OFF);
> + if (status)
> + return status;
> +
> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
Thanks for removing F81604_USB_MAX_RETRY.
Yet, I still would like to understand why you need one hundred tries?
Is this some paranoiac safenet? Or does the device really need so many
attempts to operate reliably? If those are needed, I would like to
understand the root cause.
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, &tmp);
> + if (status)
> + return status;
> +
> + /* check reset bit */
> + if (tmp & MOD_RM) {
> + priv->can.state = CAN_STATE_STOPPED;
> + return 0;
> + }
> +
> + /* reset chip */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, MOD_RM);
> + if (status)
> + return status;
> + }
> +
> + return -EPERM;
> +}
> +
> +static int f81604_set_normal_mode(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + u8 mod_reg = 0x00;
> + u8 tmp, ier = 0;
> + int status, i;
> +
> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, &tmp);
> + if (status)
> + return status;
> +
> + /* check reset bit */
> + if ((tmp & MOD_RM) == 0) {
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> + /* enable interrupts, RI handled by bulk-in */
> + ier = IRQ_ALL & ~IRQ_RI;
> + if (!(priv->can.ctrlmode &
> + CAN_CTRLMODE_BERR_REPORTING))
> + ier &= ~IRQ_BEI;
> +
> + return f81604_set_sja1000_register(priv->dev,
> + netdev->dev_id,
> + SJA1000_IER, ier);
> + }
> +
> + /* set chip to normal mode */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> + mod_reg |= MOD_LOM;
> + if (priv->can.ctrlmode & CAN_CTRLMODE_PRESUME_ACK)
> + mod_reg |= MOD_STM;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, mod_reg);
> + if (status)
> + return status;
> + }
> +
> + return -EPERM;
> +}
> +
> +static int f81604_chipset_init(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + int i;
> +
> + /* set clock divider and output control register */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_CDR,
> + priv->cdr | CDR_PELICAN);
> + if (status)
> + return status;
> +
> + /* set acceptance filter (accept all) */
> + for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ACCC0 + i, 0);
> + if (status)
> + return status;
> + }
> +
> + for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ACCM0 + i, 0xFF);
> + if (status)
> + return status;
> + }
> +
> + return f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_OCR,
> + priv->ocr | OCR_MODE_NORMAL);
> +}
> +
> +static void f81604_unregister_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i)
> + usb_kill_urb(priv->read_urb[i]);
> +
> + usb_kill_urb(priv->write_urb);
> + usb_kill_urb(priv->int_urb);
> +}
> +
> +static int f81604_register_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status, i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
> + status = usb_submit_urb(priv->read_urb[i], GFP_KERNEL);
> + if (status) {
> + netdev_warn(netdev, "%s: submit rx urb failed: %pe\n",
> + __func__, ERR_PTR(status));
> + return status;
> + }
> + }
> +
> + status = usb_submit_urb(priv->int_urb, GFP_KERNEL);
> + if (status) {
> + netdev_warn(netdev, "%s: submit int urb failed: %pe\n",
> + __func__, ERR_PTR(status));
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +static int f81604_start(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + u8 mode;
> + u8 tmp;
> +
> + f81604_unregister_urbs(netdev);
> +
> + mode = F81604_RX_AUTO_RELEASE_BUF | F81604_INT_WHEN_CHANGE;
> +
> + /* Set TR/AT mode */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> + mode |= F81604_TX_ONESHOT;
> + else
> + mode |= F81604_TX_NORMAL;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + F81604_CTRL_MODE_REG, mode);
> + if (status)
> + return status;
> +
> + /* set reset mode */
> + status = f81604_set_reset_mode(netdev);
> + if (status)
> + return status;
> +
> + status = f81604_chipset_init(netdev);
> + if (status)
> + return status;
> +
> + /* Clear error counters and error code capture */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_TXERR, 0);
> + if (status)
> + return status;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_RXERR, 0);
> + if (status)
> + return status;
> +
> + /* Read clear for ECC/ALC/IR register */
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ECC, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ALC, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_IR, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_register_urbs(netdev);
> + if (status)
> + return status;
> +
> + return f81604_set_normal_mode(netdev);
> +}
> +
> +static int f81604_set_bittiming(struct net_device *dev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(dev);
> + struct can_bittiming *bt = &priv->can.bittiming;
> + int status = 0;
> + u8 btr0, btr1;
> +
> + btr0 = FIELD_PREP(GENMASK(5, 0), bt->brp - 1) |
> + FIELD_PREP(GENMASK(7, 6), bt->sjw - 1);
Add a #define for all your GENMASK(). e.g.
#define GENMASK(5, 0) F81604_BRP
> + btr1 = FIELD_PREP(GENMASK(3, 0), bt->prop_seg + bt->phase_seg1 - 1) |
> + FIELD_PREP(GENMASK(6, 4), bt->phase_seg2 - 1);
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> + btr1 |= SJA1000_BTR1_SAMPLE_TRIPLE;
> +
> + status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
> + SJA1000_BTR0, btr0);
> + if (status) {
> + netdev_warn(dev, "%s: Set BTR0 failed: %pe\n", __func__,
> + ERR_PTR(status));
> + return status;
> + }
> +
> + status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
> + SJA1000_BTR1, btr1);
> + if (status) {
> + netdev_warn(dev, "%s: Set BTR1 failed: %pe\n", __func__,
> + ERR_PTR(status));
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +static int f81604_set_mode(struct net_device *netdev, enum can_mode mode)
> +{
> + int err;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + err = f81604_start(netdev);
> + if (!err && netif_queue_stopped(netdev))
> + netif_wake_queue(netdev);
> + break;
> +
> + default:
> + err = -EOPNOTSUPP;
> + }
> +
> + return err;
> +}
> +
> +static void f81604_process_rx_packet(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_bulk_data *ptr;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + int i, count;
> + u8 *data;
> +
> + WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
> +
> + data = urb->transfer_buffer;
> + stats = &netdev->stats;
> +
> + if (urb->actual_length % F81604_DATA_SIZE)
> + netdev_warn(netdev, "actual_length %% %d != 0 (%d)\n",
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
> + F81604_DATA_SIZE, urb->actual_length);
> + else if (!urb->actual_length)
> + netdev_warn(netdev, "actual_length = 0 (%d)\n",
> + urb->actual_length);
The log messages will be:
actual_length = 0 (0)
?!
Please fix this.
> + count = urb->actual_length / F81604_DATA_SIZE;
> +
> + for (i = 0; i < count; ++i) {
> + ptr = (struct f81604_bulk_data *)&data[i * F81604_DATA_SIZE];
> +
> + if (ptr->cmd != F81604_CMD_DATA)
> + continue;
> +
> + skb = alloc_can_skb(netdev, &cf);
> + if (!skb) {
> + netdev_warn(netdev, "%s: not enough memory", __func__);
> + continue;
> + }
> +
> + cf->len = can_cc_dlc2len(ptr->dlc & F81604_DLC_LEN_MASK);
> +
> + if (ptr->dlc & F81604_DLC_EFF_BIT) {
> + cf->can_id = be32_to_cpu(ptr->eff.id) >> 3;
> + cf->can_id |= CAN_EFF_FLAG;
> + } else {
> + cf->can_id = be16_to_cpu(ptr->sff.id) >> 5;
> + }
> +
> + if (ptr->dlc & F81604_DLC_RTR_BIT)
> + cf->can_id |= CAN_RTR_FLAG;
> + else if (ptr->dlc & F81604_DLC_EFF_BIT)
> + memcpy(cf->data, ptr->eff.data, cf->len);
You can group this together with the if (ptr->dlc & F81604_DLC_EFF_BIT) above.
> + else
> + memcpy(cf->data, ptr->sff.data, cf->len);
Idem, you can group this with the else above.
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
You can group this together with the if (ptr->dlc & F81604_DLC_RTR_BIT) above.
> + stats->rx_packets++;
> + netif_rx(skb);
> + }
> +}
> +
> +static void f81604_read_bulk_callback(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> + int status;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: URB aborted %pe\n", __func__,
> + ERR_PTR(urb->status));
> +
> + switch (urb->status) {
> + case 0: /* success */
> + break;
> +
> + case -ENOENT:
> + case -EPIPE:
> + case -EPROTO:
> + case -ESHUTDOWN:
> + return;
> +
> + default:
> + goto resubmit_urb;
> + }
> +
> + f81604_process_rx_packet(urb);
> +
> +resubmit_urb:
> + status = usb_submit_urb(urb, GFP_ATOMIC);
> + if (status == -ENODEV)
> + netif_device_detach(netdev);
> + else if (status)
> + netdev_err(netdev,
> + "%s: failed to resubmit read bulk urb: %pe\n",
> + __func__, ERR_PTR(status));
> +}
> +
> +static void f81604_write_bulk_callback(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: Tx URB error: %pe\n", __func__,
> + ERR_PTR(urb->status));
> +}
> +
> +static void f81604_handle_clear_overrun_work(struct work_struct *work)
> +{
> + struct f81604_port_priv *priv;
> + struct net_device *netdev;
> +
> + priv = container_of(work, struct f81604_port_priv,
> + handle_clear_overrun_work);
> + netdev = priv->netdev;
> +
> + f81604_set_sja1000_register(priv->dev, netdev->dev_id, SJA1000_CMR,
> + CMD_CDO);
> +}
> +
> +static void f81604_handle_clear_reg_work(struct work_struct *work)
> +{
> + struct f81604_port_priv *priv;
> + struct net_device *netdev;
> + bool clear_ecc, clear_alc;
> + unsigned long flags;
> + u8 tmp;
> +
> + priv = container_of(work, struct f81604_port_priv,
> + handle_clear_reg_work);
> + netdev = priv->netdev;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + clear_alc = priv->need_clear_alc;
> + clear_ecc = priv->need_clear_ecc;
> + priv->need_clear_alc = false;
> + priv->need_clear_ecc = false;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + if (clear_alc)
> + f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ALC, &tmp);
> +
> + if (clear_ecc)
> + f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ECC, &tmp);
> +}
> +
> +static void f81604_handle_tx(struct urb *urb)
> +{
> + struct f81604_int_data *data = urb->transfer_buffer;
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_port_priv *priv;
> +
> + priv = netdev_priv(netdev);
> + stats = &netdev->stats;
> +
> + if (!(data->isrc & IRQ_TI))
> + return;
> +
> + /* transmission buffer released */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
> + !(data->sr & SR_TCS)) {
> + stats->tx_errors++;
> + can_free_echo_skb(netdev, 0, NULL);
> + } else {
> + /* transmission complete */
> + stats->tx_bytes += can_get_echo_skb(netdev, 0, NULL);
> + stats->tx_packets++;
> + }
> +
> + netif_wake_queue(netdev);
> +}
> +
> +static void f81604_read_int_callback(struct urb *urb)
> +{
> + struct f81604_int_data *data = urb->transfer_buffer;
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_port_priv *priv;
> + enum can_state can_state;
> + enum can_state rx_state;
> + enum can_state tx_state;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + unsigned long flags;
> + int r;
Be more consistent throughout your driver. The return value is
sometimes named "r", sometimes "status" and sometimes "ret". My
suggestion is to use "ret" everytime.
> + priv = netdev_priv(netdev);
> + can_state = priv->can.state;
> + stats = &netdev->stats;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: Int URB aborted: %pe\n", __func__,
> + ERR_PTR(urb->status));
> +
> + switch (urb->status) {
> + case 0: /* success */
> + break;
> +
> + case -ENOENT:
> + case -EPIPE:
> + case -EPROTO:
> + case -ESHUTDOWN:
> + return;
> +
> + default:
> + goto resubmit_urb;
> + }
> +
> + /* Note: ALC/ECC will not auto clear by read here, must to clear by
> + * read register (via handle_clear_reg_work).
> + */
> +
> + /* handle can bus errors */
> + if (data->isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
The function f81604_read_int_callback() is too long. Can you split it
and add a f81604_handle_can_bus_errors() which will contain below
code?
> + skb = alloc_can_err_skb(netdev, &cf);
> + if (!skb) {
> + netdev_warn(netdev,
> + "no memory to alloc_can_err_skb\n");
> + goto resubmit_urb;
If the allocation fails, please continue to increment struct net_device_stats.
> + }
> +
> + cf->can_id |= CAN_ERR_CNT;
> + cf->data[6] = data->txerr;
> + cf->data[7] = data->rxerr;
> +
> + if (data->isrc & IRQ_DOI) {
> + /* data overrun interrupt */
> + netdev_dbg(netdev, "data overrun interrupt\n");
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> + stats->rx_over_errors++;
> + stats->rx_errors++;
> +
> + schedule_work(&priv->handle_clear_overrun_work);
> + }
> +
> + if (data->isrc & IRQ_EI) {
> + /* error warning interrupt */
> + netdev_dbg(netdev, "error warning interrupt\n");
> +
> + if (data->sr & SR_BS)
> + can_state = CAN_STATE_BUS_OFF;
> + else if (data->sr & SR_ES)
> + can_state = CAN_STATE_ERROR_WARNING;
> + else
> + can_state = CAN_STATE_ERROR_ACTIVE;
> + }
> +
> + if (data->isrc & IRQ_BEI) {
> + /* bus error interrupt */
> + netdev_dbg(netdev, "bus error interrupt\n");
> +
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> +
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + /* set error type */
> + switch (data->ecc & ECC_MASK) {
> + case ECC_BIT:
> + cf->data[2] |= CAN_ERR_PROT_BIT;
> + break;
> + case ECC_FORM:
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> + case ECC_STUFF:
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> + default:
> + break;
> + }
> +
> + /* set error location */
> + cf->data[3] = data->ecc & ECC_SEG;
> +
> + /* Error occurred during transmission? */
> + if ((data->ecc & ECC_DIR) == 0)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->need_clear_ecc = true;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + schedule_work(&priv->handle_clear_reg_work);
> + }
> +
> + if (data->isrc & IRQ_EPI) {
> + if (can_state == CAN_STATE_ERROR_PASSIVE)
> + can_state = CAN_STATE_ERROR_WARNING;
> + else
> + can_state = CAN_STATE_ERROR_PASSIVE;
> +
> + /* error passive interrupt */
> + netdev_dbg(netdev, "error passive interrupt: %d\n",
> + can_state);
> + }
> +
> + if (data->isrc & IRQ_ALI) {
> + /* arbitration lost interrupt */
> + netdev_dbg(netdev, "arbitration lost interrupt\n");
> +
> + priv->can.can_stats.arbitration_lost++;
> + stats->tx_errors++;
> + cf->can_id |= CAN_ERR_LOSTARB;
> + cf->data[0] = data->alc & ALC_MASK;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->need_clear_alc = true;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + schedule_work(&priv->handle_clear_reg_work);
> + }
> +
> + if (can_state != priv->can.state) {
> + tx_state = data->txerr >= data->rxerr ? can_state : 0;
> + rx_state = data->txerr <= data->rxerr ? can_state : 0;
> +
> + can_change_state(netdev, cf, tx_state, rx_state);
> +
> + if (can_state == CAN_STATE_BUS_OFF)
> + can_bus_off(netdev);
> + }
> +
> + netif_rx(skb);
> + }
> +
> + /* handle TX */
> + if (can_state != CAN_STATE_BUS_OFF)
> + f81604_handle_tx(urb);
Use data, not urb:
f81604_handle_tx(data);
> +
> +resubmit_urb:
> + r = usb_submit_urb(urb, GFP_ATOMIC);
> + if (r) {
> + netdev_err(netdev,
> + "%s: failed resubmitting int bulk urb: %pe\n",
> + __func__, ERR_PTR(r));
> +
> + if (r == -ENODEV)
> + netif_device_detach(netdev);
> + }
> +}
> +
> +static netdev_tx_t f81604_start_xmit(struct sk_buff *skb,
> + struct net_device *netdev)
> +{
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + struct net_device_stats *stats = &netdev->stats;
> + struct f81604_bulk_data *ptr;
Choose a more explicit name than ptr (e.g. frame).
> + int status, len;
> +
> + if (can_dropped_invalid_skb(netdev, skb))
> + return NETDEV_TX_OK;
> +
> + netif_stop_queue(netdev);
In your driver, you send the CAN frames one at a time and wait for the
rx_handler to restart the queue. This approach dramatically degrades
the throughput. Is this a device limitation? Is the device not able to
manage more than one frame at a time?
> + WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
This is a compile time constant. Use static_assert() just after the
declaration of struct f81604_bulk_data instead.
> + ptr = (struct f81604_bulk_data *)priv->bulk_write_buffer;
> + memset(ptr, 0, F81604_DATA_SIZE);
> +
> + len = can_get_cc_dlc(cf, priv->can.ctrlmode);
> + ptr->cmd = F81604_CMD_DATA;
> + ptr->dlc = len;
> +
> + if (cf->can_id & CAN_RTR_FLAG)
> + ptr->dlc |= F81604_DLC_RTR_BIT;
> +
> + if (cf->can_id & CAN_EFF_FLAG) {
> + ptr->eff.id = cpu_to_be32((cf->can_id & CAN_EFF_MASK) << 3);
> + ptr->dlc |= F81604_DLC_EFF_BIT;
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + memcpy(&ptr->eff.data, cf->data, len);
> + } else {
> + ptr->sff.id = cpu_to_be16((cf->can_id & CAN_SFF_MASK) << 5);
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + memcpy(&ptr->sff.data, cf->data, len);
> + }
> +
> + can_put_echo_skb(skb, netdev, 0, 0);
> +
> + status = usb_submit_urb(priv->write_urb, GFP_ATOMIC);
> + if (status) {
> + netdev_err(netdev, "%s: failed to resubmit tx bulk urb: %pe\n",
> + __func__, ERR_PTR(status));
> +
> + can_free_echo_skb(netdev, 0, NULL);
> + stats->tx_dropped++;
> +
> + if (status == -ENODEV)
> + netif_device_detach(netdev);
> + }
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static int f81604_get_berr_counter(const struct net_device *netdev,
> + struct can_berr_counter *bec)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + u8 txerr;
> + u8 rxerr;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_TXERR, &txerr);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_RXERR, &rxerr);
> + if (status)
> + return status;
> +
> + bec->txerr = txerr;
> + bec->rxerr = rxerr;
> +
> + return 0;
> +}
> +
> +static void f81604_remove_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i)
> + usb_free_urb(priv->read_urb[i]);
> +
> + usb_free_urb(priv->write_urb);
> + usb_free_urb(priv->int_urb);
> +}
> +
> +static int f81604_prepare_urbs(struct net_device *netdev)
> +{
> + static const u8 bulk_in_addr[F81604_MAX_DEV] = { 0x82, 0x84 };
> + static const u8 bulk_out_addr[F81604_MAX_DEV] = { 0x01, 0x03 };
> + static const u8 int_in_addr[F81604_MAX_DEV] = { 0x81, 0x83 };
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int id = netdev->dev_id;
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
> + priv->read_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->read_urb[i])
> + goto error;
> +
> + usb_fill_bulk_urb(priv->read_urb[i], priv->dev,
> + usb_rcvbulkpipe(priv->dev, bulk_in_addr[id]),
> + priv->bulk_read_buffer[i], F81604_BULK_SIZE,
> + f81604_read_bulk_callback, netdev);
> + }
> +
> + priv->write_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->write_urb)
> + goto error;
> +
> + usb_fill_bulk_urb(priv->write_urb, priv->dev,
> + usb_sndbulkpipe(priv->dev, bulk_out_addr[id]),
> + priv->bulk_write_buffer, F81604_DATA_SIZE,
> + f81604_write_bulk_callback, netdev);
> +
> + priv->int_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->int_urb)
> + goto error;
> +
> + usb_fill_int_urb(priv->int_urb, priv->dev,
> + usb_rcvintpipe(priv->dev, int_in_addr[id]),
> + priv->int_read_buffer, F81604_INT_SIZE,
> + f81604_read_int_callback, netdev, 1);
> +
> + return 0;
> +
> +error:
> + f81604_remove_urbs(netdev);
> + return -ENOMEM;
> +}
> +
> +/* Open USB device */
> +static int f81604_open(struct net_device *netdev)
> +{
> + int err;
> +
> + err = open_candev(netdev);
> + if (err)
> + return err;
> +
> + err = f81604_start(netdev);
> + if (err)
> + goto start_failed;
> +
> + netif_start_queue(netdev);
> + return 0;
> +
> +start_failed:
> + if (err == -ENODEV)
> + netif_device_detach(netdev);
> +
> + close_candev(netdev);
> +
> + return err;
> +}
> +
> +/* Close USB device */
> +static int f81604_close(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> +
> + f81604_set_reset_mode(netdev);
> +
> + netif_stop_queue(netdev);
> + cancel_work_sync(&priv->handle_clear_overrun_work);
> + cancel_work_sync(&priv->handle_clear_reg_work);
> + close_candev(netdev);
> +
> + f81604_unregister_urbs(netdev);
> +
> + return 0;
> +}
> +
> +static const struct net_device_ops f81604_netdev_ops = {
> + .ndo_open = f81604_open,
> + .ndo_stop = f81604_close,
> + .ndo_start_xmit = f81604_start_xmit,
> + .ndo_change_mtu = can_change_mtu,
> +};
> +
> +static const struct can_bittiming_const f81604_bittiming_const = {
> + .name = "f81604",
.name = KBUILD_MODNAME,
> + .tseg1_min = 1,
> + .tseg1_max = 16,
> + .tseg2_min = 1,
> + .tseg2_max = 8,
> + .sjw_max = 4,
> + .brp_min = 1,
> + .brp_max = 64,
> + .brp_inc = 1,
> +};
> +
> +/* Called by the usb core when driver is unloaded or device is removed */
> +static void f81604_disconnect(struct usb_interface *intf)
> +{
> + struct f81604_priv *priv = usb_get_intfdata(intf);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_DEV; ++i) {
> + if (!priv->netdev[i])
> + continue;
> +
> + unregister_netdev(priv->netdev[i]);
> + f81604_remove_urbs(priv->netdev[i]);
> + free_candev(priv->netdev[i]);
> + }
> +}
> +
> +static int f81604_set_termination(struct net_device *netdev, u16 term)
> +{
> + struct f81604_port_priv *port_priv = netdev_priv(netdev);
> + struct f81604_priv *priv;
> + u8 mask, data = 0;
> + int r;
> +
> + priv = usb_get_intfdata(port_priv->intf);
> +
> + if (netdev->dev_id == 0)
> + mask = F81604_CAN0_TERM;
> + else
> + mask = F81604_CAN1_TERM;
> +
> + if (term == F81604_TERMINATION_ENABLED)
> + data = mask;
> +
> + mutex_lock(&priv->mutex);
Did you witness a race condition?
As far as I know, this call back is only called while the network
stack big kernel lock (a.k.a. rtnl_lock) is being hold.
If you have doubt, try adding a:
ASSERT_RTNL()
If this assert works, then another mutex is not needed.
> + r = f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
> + mask, data);
> +
> + mutex_unlock(&priv->mutex);
> +
> + return r;
> +}
> +
> +static int f81604_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *dev = interface_to_usbdev(intf);
> + struct f81604_port_priv *port_priv;
> + struct net_device *netdev;
> + struct f81604_priv *priv;
> + int i, err;
> +
> + priv = devm_kzalloc(&intf->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + usb_set_intfdata(intf, priv);
> +
> + for (i = 0; i < F81604_MAX_DEV; ++i) {
> + netdev = alloc_candev(sizeof(*port_priv), 1);
> + if (!netdev) {
> + dev_err(&intf->dev, "Couldn't alloc candev: %d\n", i);
> + err = -ENOMEM;
> +
> + goto failure_cleanup;
> + }
> +
> + port_priv = netdev_priv(netdev);
> + netdev->dev_id = i;
> +
> + mutex_init(&priv->mutex);
> + spin_lock_init(&port_priv->lock);
> +
> + INIT_WORK(&port_priv->handle_clear_overrun_work,
> + f81604_handle_clear_overrun_work);
> + INIT_WORK(&port_priv->handle_clear_reg_work,
> + f81604_handle_clear_reg_work);
> +
> + port_priv->intf = intf;
> + port_priv->dev = dev;
> + port_priv->ocr = OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL;
> + port_priv->cdr = CDR_CBP;
> + port_priv->can.state = CAN_STATE_STOPPED;
> + port_priv->can.clock.freq = F81604_CAN_CLOCK;
> +
> + port_priv->can.termination_const = f81604_termination;
> + port_priv->can.termination_const_cnt =
> + ARRAY_SIZE(f81604_termination);
> + port_priv->can.bittiming_const = &f81604_bittiming_const;
> + port_priv->can.do_set_bittiming = f81604_set_bittiming;
> + port_priv->can.do_set_mode = f81604_set_mode;
> + port_priv->can.do_set_termination = f81604_set_termination;
> + port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
> + port_priv->can.ctrlmode_supported =
> + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
> + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
> + CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
Did you test the CAN_CTRLMODE_CC_LEN8_DLC feature? Did you confirm
that you can send and receive DLC greater than 8?
> + netdev->ethtool_ops = &f81604_ethtool_ops;
> + netdev->netdev_ops = &f81604_netdev_ops;
> + netdev->flags |= IFF_ECHO;
> +
> + SET_NETDEV_DEV(netdev, &intf->dev);
> +
> + err = f81604_set_termination(netdev,
> + F81604_TERMINATION_DISABLED);
> + if (err)
> + goto clean_candev;
> +
> + err = f81604_prepare_urbs(netdev);
> + if (err)
> + goto clean_candev;
> +
> + err = register_candev(netdev);
> + if (err)
> + goto clean_candev;
> +
> + port_priv->netdev = netdev;
> + priv->netdev[i] = netdev;
> +
> + dev_info(&intf->dev, "Channel #%d registered as %s\n", i,
> + netdev->name);
> + }
> +
> + return 0;
> +
> +clean_candev:
> + netdev_err(netdev, "couldn't enable CAN device: %pe\n", ERR_PTR(err));
> + free_candev(netdev);
> +
> +failure_cleanup:
> + f81604_disconnect(intf);
> + return err;
> +}
> +
> +static struct usb_driver f81604_driver = {
> + .name = "f81604",
.name = KBUILD_MODNAME,
> + .probe = f81604_probe,
> + .disconnect = f81604_disconnect,
> + .id_table = f81604_table,
> +};
> +
> +module_usb_driver(f81604_driver);
> +
> +MODULE_AUTHOR("Ji-Ze Hong (Peter Hong) <[email protected]>");
> +MODULE_DESCRIPTION("Fintek F81604 USB to 2xCANBUS");
> +MODULE_LICENSE("GPL");
Yours sincerely,
Vincent Mailhol
On 27.03.2023 13:10:48, Ji-Ze Hong (Peter Hong) wrote:
> This patch add support for Fintek USB to 2CAN controller support.
In addition to Vincent's comment, please describe your change
declarative, e.g.:
| Add support for the Fintek USB to 2CAN controller.
Please add an entry in the MAINTAINERS file.
> Signed-off-by: Ji-Ze Hong (Peter Hong) <[email protected]>
[...]
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 1ea16be5743b..9de0305a5cce 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/
> obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> obj-$(CONFIG_CAN_UCAN) += ucan.o
> +obj-$(CONFIG_CAN_F81604) += f81604.o
As Vincent said: Please sort the targets by alphabetical order, move the
entry in the Kconfig to the corresponding place.
> diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
> new file mode 100644
> index 000000000000..06cd5d95c44d
> --- /dev/null
> +++ b/drivers/net/can/usb/f81604.c
> @@ -0,0 +1,1232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Fintek F81604 USB-to-2CAN controller driver.
> + *
> + * Copyright (C) 2023 Ji-Ze Hong (Peter Hong) <[email protected]>
> + */
> +#include <linux/netdevice.h>
> +#include <linux/usb.h>
> +#include <linux/bitfield.h>
> +#include <linux/units.h>
> +
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/platform/sja1000.h>
> +
> +/* vendor and product id */
> +#define F81604_VENDOR_ID 0x2c42
> +#define F81604_PRODUCT_ID 0x1709
> +#define F81604_CAN_CLOCK (24 * MEGA / 2)
> +#define F81604_MAX_DEV 2
> +#define F81604_SET_DEVICE_RETRY 100
> +
> +#define F81604_USB_TIMEOUT 2000
> +#define F81604_SET_GET_REGISTER 0xA0
> +#define F81604_PORT_OFFSET 0x1000
> +
> +#define F81604_BULK_SIZE 64
> +#define F81604_INT_SIZE 16
> +#define F81604_DATA_SIZE 14
> +#define F81604_MAX_RX_URBS 4
> +
> +#define F81604_CMD_OFFSET 0x00
> +#define F81604_CMD_DATA 0x00
> +
> +#define F81604_DLC_LEN_MASK 0x0f
> +#define F81604_DLC_EFF_BIT BIT(7)
> +#define F81604_DLC_RTR_BIT BIT(6)
> +
> +/* device setting */
> +#define F81604_CTRL_MODE_REG 0x80
> +#define F81604_TX_ONESHOT (0x03 << 3)
> +#define F81604_TX_NORMAL (0x01 << 3)
> +#define F81604_RX_AUTO_RELEASE_BUF BIT(1)
> +#define F81604_INT_WHEN_CHANGE BIT(0)
> +
> +#define F81604_TERMINATOR_REG 0x105
> +#define F81604_CAN0_TERM BIT(2)
> +#define F81604_CAN1_TERM BIT(3)
> +
> +#define F81604_TERMINATION_DISABLED CAN_TERMINATION_DISABLED
> +#define F81604_TERMINATION_ENABLED 120
> +
Please add the common prefix "F81604_" to these defines, too.
> +/* SJA1000 registers - manual section 6.4 (Pelican Mode) */
> +#define SJA1000_MOD 0x00
> +#define SJA1000_CMR 0x01
> +#define SJA1000_IR 0x03
> +#define SJA1000_IER 0x04
> +#define SJA1000_ALC 0x0B
> +#define SJA1000_ECC 0x0C
> +#define SJA1000_RXERR 0x0E
> +#define SJA1000_TXERR 0x0F
> +#define SJA1000_ACCC0 0x10
> +#define SJA1000_ACCC1 0x11
> +#define SJA1000_ACCC2 0x12
> +#define SJA1000_ACCC3 0x13
> +#define SJA1000_ACCM0 0x14
> +#define SJA1000_ACCM1 0x15
> +#define SJA1000_ACCM2 0x16
> +#define SJA1000_ACCM3 0x17
> +#define SJA1000_MAX_FILTER_CNT 4
If these are SJA1000 registers, bits, also add SJA1000 as prefix.
> +
> +/* Common registers - manual section 6.5 */
> +#define SJA1000_BTR0 0x06
> +#define SJA1000_BTR1 0x07
> +#define SJA1000_BTR1_SAMPLE_TRIPLE BIT(7)
> +#define SJA1000_OCR 0x08
> +#define SJA1000_CDR 0x1F
> +
> +/* mode register */
> +#define MOD_RM 0x01
> +#define MOD_LOM 0x02
> +#define MOD_STM 0x04
> +
> +/* commands */
> +#define CMD_CDO 0x08
> +
> +/* interrupt sources */
> +#define IRQ_BEI 0x80
> +#define IRQ_ALI 0x40
> +#define IRQ_EPI 0x20
> +#define IRQ_DOI 0x08
> +#define IRQ_EI 0x04
> +#define IRQ_TI 0x02
> +#define IRQ_RI 0x01
> +#define IRQ_ALL 0xFF
> +#define IRQ_OFF 0x00
> +
> +/* status register content */
> +#define SR_BS 0x80
> +#define SR_ES 0x40
> +#define SR_TS 0x20
> +#define SR_RS 0x10
> +#define SR_TCS 0x08
> +#define SR_TBS 0x04
> +#define SR_DOS 0x02
> +#define SR_RBS 0x01
> +
> +/* ECC register */
> +#define ECC_SEG 0x1F
> +#define ECC_DIR 0x20
> +#define ECC_BIT 0x00
> +#define ECC_FORM 0x40
> +#define ECC_STUFF 0x80
> +#define ECC_MASK 0xc0
> +
> +/* ALC register */
> +#define ALC_MASK 0x1f
> +
> +/* table of devices that work with this driver */
> +static const struct usb_device_id f81604_table[] = {
> + { USB_DEVICE(F81604_VENDOR_ID, F81604_PRODUCT_ID) },
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, f81604_table);
> +
> +static const struct ethtool_ops f81604_ethtool_ops = {
> + .get_ts_info = ethtool_op_get_ts_info,
> +};
> +
> +static const u16 f81604_termination[] = { F81604_TERMINATION_DISABLED,
> + F81604_TERMINATION_ENABLED };
> +
> +struct f81604_priv {
> + struct net_device *netdev[F81604_MAX_DEV];
> + struct mutex mutex; /* for terminator setting */
> +};
> +
> +struct f81604_port_priv {
> + struct can_priv can;
> + struct net_device *netdev;
> + struct sk_buff *echo_skb;
> +
> + /* For synchronize need_clear_alc/need_clear_ecc in worker & interrupt
> + * callback.
> + */
> + spinlock_t lock;
> + bool need_clear_alc;
> + bool need_clear_ecc;
What about creating a u8 with 2 flags, use set_bit() and
test_and_clear_bit().
> +
> + struct work_struct handle_clear_reg_work;
> + struct work_struct handle_clear_overrun_work;
Can you combine both workers? Add a 3rd flag to the u8 you created
above.
> +
> + struct usb_device *dev;
> + struct usb_interface *intf;
> +
> + struct urb *int_urb;
> + u8 int_read_buffer[F81604_INT_SIZE];
> +
> + struct urb *read_urb[F81604_MAX_RX_URBS];
Please use an struct usb_anchor for this.
> + u8 bulk_read_buffer[F81604_MAX_RX_URBS][F81604_BULK_SIZE];
allocate the URB dynamic with kmalloc() and use urb->transfer_flags |=
URB_FREE_BUFFER for automatic free()ing.
> +
> + struct urb *write_urb;
> + u8 bulk_write_buffer[F81604_DATA_SIZE];
With just 1 TX URB the TX would be quite slow. Does your hardware
support more than 1 TX URB at a time? USB devices usually answer with
NAK it they are busy, automatic and transparent retry is handled by the
USB host controller.
> +
> + u8 ocr;
> + u8 cdr;
These are compile time constant, please remove.
> +};
> +
> +/* Interrupt endpoint data format: SR/IR/IER/ALC/ECC/EWLR/RXERR/TXERR/VAL */
> +struct f81604_int_data {
> + u8 sr;
> + u8 isrc;
> + u8 ier;
> + u8 alc;
> + u8 ecc;
> + u8 ewlr;
> + u8 rxerr;
> + u8 txerr;
> + u8 val;
> +} __packed;
I think this struct is always aligned to 4 bytes, so add a __aligned(4)
behind __packed.
> +
> +struct f81604_sff {
> + __be16 id;
> + u8 data[CAN_MAX_DLEN];
> +} __packed;
__aligned(2)
> +
> +struct f81604_eff {
> + __be32 id;
> + u8 data[CAN_MAX_DLEN];
> +} __packed;
__aligned(2)
> +
> +struct f81604_bulk_data {
> + u8 cmd;
> +
> + /* According for F81604 DLC define:
> + * #define F81604_DLC_LEN_MASK 0x0f
> + * #define F81604_DLC_EFF_BIT BIT(7)
> + * #define F81604_DLC_RTR_BIT BIT(6)
no need to duplicate code
> + */
> + u8 dlc;
> +
> + union {
> + struct f81604_sff sff;
> + struct f81604_eff eff;
> + };
> +} __packed;
__aligned(4)
> +
> +static int f81604_set_register(struct usb_device *dev, u16 reg, u8 data)
What about renaming these to f81604_write(), f81604_read().
I would change the 1st argument to struct f81604_port_priv *priv.
> +{
> + int ret;
> +
> + ret = usb_control_msg_send(dev, 0, F81604_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_OUT, 0, reg,
> + &data, sizeof(data), F81604_USB_TIMEOUT,
> + GFP_KERNEL);
> + if (ret)
> + dev_err(&dev->dev, "%s: reg: %x data: %x failed: %pe\n",
> + __func__, reg, data, ERR_PTR(ret));
> +
> + return ret;
> +}
> +
> +static int f81604_get_register(struct usb_device *dev, u16 reg, u8 *data)
> +{
> + int ret;
> +
> + ret = usb_control_msg_recv(dev, 0, F81604_SET_GET_REGISTER,
> + USB_TYPE_VENDOR | USB_DIR_IN, 0, reg, data,
> + sizeof(*data), F81604_USB_TIMEOUT,
> + GFP_KERNEL);
> +
> + if (ret < 0)
> + dev_err(&dev->dev, "%s: reg: %x failed: %pe\n", __func__, reg,
> + ERR_PTR(ret));
> +
> + return ret;
> +}
> +
> +static int f81604_mask_set_register(struct usb_device *dev, u16 reg, u8 mask,
> + u8 data)
...to f81604_update_bits().
> +{
> + int status;
> + u8 tmp;
> +
> + status = f81604_get_register(dev, reg, &tmp);
> + if (status)
> + return status;
> +
> + tmp &= ~mask;
> + tmp |= (mask & data);
> +
> + return f81604_set_register(dev, reg, tmp);
> +}
> +
> +static int f81604_set_sja1000_register(struct usb_device *dev, u8 port,
> + u16 reg, u8 data)
...to f81604_sja1000_read(), f81604_sja1000_write().
Make the first argument struct f81604_port_priv *priv, too, then you
don't need to pass port.
> +{
> + int real_reg;
> +
> + real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
> + return f81604_set_register(dev, real_reg, data);
> +}
> +
> +static int f81604_get_sja1000_register(struct usb_device *dev, u8 port,
> + u16 reg, u8 *data)
> +{
> + int real_reg;
> +
> + real_reg = reg + F81604_PORT_OFFSET * port + F81604_PORT_OFFSET;
> + return f81604_get_register(dev, real_reg, data);
> +}
> +
> +static int f81604_set_reset_mode(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status, i;
> + u8 tmp;
> +
> + /* disable interrupts */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_IER, IRQ_OFF);
> + if (status)
> + return status;
> +
> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, &tmp);
> + if (status)
> + return status;
> +
> + /* check reset bit */
> + if (tmp & MOD_RM) {
> + priv->can.state = CAN_STATE_STOPPED;
> + return 0;
> + }
> +
> + /* reset chip */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, MOD_RM);
> + if (status)
> + return status;
> + }
> +
> + return -EPERM;
> +}
> +
> +static int f81604_set_normal_mode(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + u8 mod_reg = 0x00;
> + u8 tmp, ier = 0;
> + int status, i;
> +
> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, &tmp);
> + if (status)
> + return status;
> +
> + /* check reset bit */
> + if ((tmp & MOD_RM) == 0) {
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> + /* enable interrupts, RI handled by bulk-in */
> + ier = IRQ_ALL & ~IRQ_RI;
> + if (!(priv->can.ctrlmode &
> + CAN_CTRLMODE_BERR_REPORTING))
> + ier &= ~IRQ_BEI;
> +
> + return f81604_set_sja1000_register(priv->dev,
> + netdev->dev_id,
> + SJA1000_IER, ier);
> + }
> +
> + /* set chip to normal mode */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> + mod_reg |= MOD_LOM;
> + if (priv->can.ctrlmode & CAN_CTRLMODE_PRESUME_ACK)
> + mod_reg |= MOD_STM;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_MOD, mod_reg);
> + if (status)
> + return status;
> + }
> +
> + return -EPERM;
> +}
> +
> +static int f81604_chipset_init(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + int i;
> +
> + /* set clock divider and output control register */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_CDR,
> + priv->cdr | CDR_PELICAN);
> + if (status)
> + return status;
> +
> + /* set acceptance filter (accept all) */
> + for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ACCC0 + i, 0);
> + if (status)
> + return status;
> + }
> +
> + for (i = 0; i < SJA1000_MAX_FILTER_CNT; ++i) {
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ACCM0 + i, 0xFF);
> + if (status)
> + return status;
> + }
> +
> + return f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_OCR,
> + priv->ocr | OCR_MODE_NORMAL);
> +}
> +
> +static void f81604_unregister_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i)
> + usb_kill_urb(priv->read_urb[i]);
> +
> + usb_kill_urb(priv->write_urb);
> + usb_kill_urb(priv->int_urb);
> +}
> +
> +static int f81604_register_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status, i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
> + status = usb_submit_urb(priv->read_urb[i], GFP_KERNEL);
> + if (status) {
> + netdev_warn(netdev, "%s: submit rx urb failed: %pe\n",
> + __func__, ERR_PTR(status));
> + return status;
> + }
> + }
> +
> + status = usb_submit_urb(priv->int_urb, GFP_KERNEL);
> + if (status) {
> + netdev_warn(netdev, "%s: submit int urb failed: %pe\n",
> + __func__, ERR_PTR(status));
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +static int f81604_start(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + u8 mode;
> + u8 tmp;
> +
> + f81604_unregister_urbs(netdev);
> +
> + mode = F81604_RX_AUTO_RELEASE_BUF | F81604_INT_WHEN_CHANGE;
> +
> + /* Set TR/AT mode */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> + mode |= F81604_TX_ONESHOT;
> + else
> + mode |= F81604_TX_NORMAL;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + F81604_CTRL_MODE_REG, mode);
> + if (status)
> + return status;
> +
> + /* set reset mode */
> + status = f81604_set_reset_mode(netdev);
I would make port_priv the first argument of all (internal) functions.
> + if (status)
> + return status;
> +
> + status = f81604_chipset_init(netdev);
> + if (status)
> + return status;
> +
> + /* Clear error counters and error code capture */
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_TXERR, 0);
> + if (status)
> + return status;
> +
> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_RXERR, 0);
> + if (status)
> + return status;
> +
> + /* Read clear for ECC/ALC/IR register */
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ECC, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ALC, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_IR, &tmp);
> + if (status)
> + return status;
> +
> + status = f81604_register_urbs(netdev);
> + if (status)
> + return status;
> +
> + return f81604_set_normal_mode(netdev);
> +}
> +
> +static int f81604_set_bittiming(struct net_device *dev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(dev);
> + struct can_bittiming *bt = &priv->can.bittiming;
> + int status = 0;
> + u8 btr0, btr1;
> +
> + btr0 = FIELD_PREP(GENMASK(5, 0), bt->brp - 1) |
> + FIELD_PREP(GENMASK(7, 6), bt->sjw - 1);
> +
> + btr1 = FIELD_PREP(GENMASK(3, 0), bt->prop_seg + bt->phase_seg1 - 1) |
> + FIELD_PREP(GENMASK(6, 4), bt->phase_seg2 - 1);
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> + btr1 |= SJA1000_BTR1_SAMPLE_TRIPLE;
> +
> + status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
> + SJA1000_BTR0, btr0);
> + if (status) {
> + netdev_warn(dev, "%s: Set BTR0 failed: %pe\n", __func__,
> + ERR_PTR(status));
> + return status;
> + }
> +
> + status = f81604_set_sja1000_register(priv->dev, dev->dev_id,
> + SJA1000_BTR1, btr1);
> + if (status) {
> + netdev_warn(dev, "%s: Set BTR1 failed: %pe\n", __func__,
> + ERR_PTR(status));
> + return status;
> + }
> +
> + return 0;
> +}
> +
> +static int f81604_set_mode(struct net_device *netdev, enum can_mode mode)
> +{
> + int err;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + err = f81604_start(netdev);
> + if (!err && netif_queue_stopped(netdev))
> + netif_wake_queue(netdev);
> + break;
> +
> + default:
> + err = -EOPNOTSUPP;
> + }
> +
> + return err;
> +}
> +
> +static void f81604_process_rx_packet(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_bulk_data *ptr;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + int i, count;
both should be unsigned.
> + u8 *data;
> +
> + WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
WARN_ON is a runtime check and makes no sense on compile time constants.
> +
> + data = urb->transfer_buffer;
> + stats = &netdev->stats;
> +
> + if (urb->actual_length % F81604_DATA_SIZE)
> + netdev_warn(netdev, "actual_length %% %d != 0 (%d)\n",
> + F81604_DATA_SIZE, urb->actual_length);
Please rephrase, so that the normal user understands the problem, e.g.
"URB length %u not multiple of %u".
> + else if (!urb->actual_length)
> + netdev_warn(netdev, "actual_length = 0 (%d)\n",
> + urb->actual_length);
same here.
> +
> + count = urb->actual_length / F81604_DATA_SIZE;
> +
> + for (i = 0; i < count; ++i) {
> + ptr = (struct f81604_bulk_data *)&data[i * F81604_DATA_SIZE];
> +
> + if (ptr->cmd != F81604_CMD_DATA)
> + continue;
> +
> + skb = alloc_can_skb(netdev, &cf);
> + if (!skb) {
stats->rx_dropped++;
Don't write to the log, in OOM situations, other parts of the kernel
usually will write something.
> + netdev_warn(netdev, "%s: not enough memory", __func__);
> + continue;
> + }
> +
> + cf->len = can_cc_dlc2len(ptr->dlc & F81604_DLC_LEN_MASK);
If you claim to support CAN_CTRLMODE_CC_LEN8_DLC:
can_frame_set_cc_len((cf, dlc, priv->can.ctrlmode);
> +
> + if (ptr->dlc & F81604_DLC_EFF_BIT) {
> + cf->can_id = be32_to_cpu(ptr->eff.id) >> 3;
> + cf->can_id |= CAN_EFF_FLAG;
> + } else {
> + cf->can_id = be16_to_cpu(ptr->sff.id) >> 5;
> + }
> +
> + if (ptr->dlc & F81604_DLC_RTR_BIT)
> + cf->can_id |= CAN_RTR_FLAG;
> + else if (ptr->dlc & F81604_DLC_EFF_BIT)
> + memcpy(cf->data, ptr->eff.data, cf->len);
> + else
> + memcpy(cf->data, ptr->sff.data, cf->len);
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> +
> + stats->rx_packets++;
> + netif_rx(skb);
> + }
> +}
> +
> +static void f81604_read_bulk_callback(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> + int status;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: URB aborted %pe\n", __func__,
> + ERR_PTR(urb->status));
> +
> + switch (urb->status) {
> + case 0: /* success */
> + break;
> +
> + case -ENOENT:
> + case -EPIPE:
> + case -EPROTO:
> + case -ESHUTDOWN:
> + return;
> +
> + default:
> + goto resubmit_urb;
> + }
> +
> + f81604_process_rx_packet(urb);
> +
> +resubmit_urb:
> + status = usb_submit_urb(urb, GFP_ATOMIC);
> + if (status == -ENODEV)
> + netif_device_detach(netdev);
> + else if (status)
> + netdev_err(netdev,
> + "%s: failed to resubmit read bulk urb: %pe\n",
> + __func__, ERR_PTR(status));
> +}
> +
> +static void f81604_write_bulk_callback(struct urb *urb)
> +{
> + struct net_device *netdev = urb->context;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: Tx URB error: %pe\n", __func__,
> + ERR_PTR(urb->status));
> +}
> +
> +static void f81604_handle_clear_overrun_work(struct work_struct *work)
> +{
> + struct f81604_port_priv *priv;
> + struct net_device *netdev;
> +
> + priv = container_of(work, struct f81604_port_priv,
> + handle_clear_overrun_work);
> + netdev = priv->netdev;
> +
> + f81604_set_sja1000_register(priv->dev, netdev->dev_id, SJA1000_CMR,
> + CMD_CDO);
> +}
> +
> +static void f81604_handle_clear_reg_work(struct work_struct *work)
> +{
> + struct f81604_port_priv *priv;
> + struct net_device *netdev;
> + bool clear_ecc, clear_alc;
> + unsigned long flags;
> + u8 tmp;
> +
> + priv = container_of(work, struct f81604_port_priv,
> + handle_clear_reg_work);
> + netdev = priv->netdev;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + clear_alc = priv->need_clear_alc;
> + clear_ecc = priv->need_clear_ecc;
> + priv->need_clear_alc = false;
> + priv->need_clear_ecc = false;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + if (clear_alc)
> + f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ALC, &tmp);
> +
> + if (clear_ecc)
> + f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_ECC, &tmp);
> +}
> +
> +static void f81604_handle_tx(struct urb *urb)
> +{
> + struct f81604_int_data *data = urb->transfer_buffer;
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_port_priv *priv;
> +
> + priv = netdev_priv(netdev);
> + stats = &netdev->stats;
> +
> + if (!(data->isrc & IRQ_TI))
> + return;
> +
> + /* transmission buffer released */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
> + !(data->sr & SR_TCS)) {
> + stats->tx_errors++;
> + can_free_echo_skb(netdev, 0, NULL);
> + } else {
> + /* transmission complete */
> + stats->tx_bytes += can_get_echo_skb(netdev, 0, NULL);
> + stats->tx_packets++;
> + }
> +
> + netif_wake_queue(netdev);
> +}
> +
> +static void f81604_read_int_callback(struct urb *urb)
> +{
> + struct f81604_int_data *data = urb->transfer_buffer;
> + struct net_device *netdev = urb->context;
> + struct net_device_stats *stats;
> + struct f81604_port_priv *priv;
> + enum can_state can_state;
> + enum can_state rx_state;
> + enum can_state tx_state;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + unsigned long flags;
> + int r;
> +
> + priv = netdev_priv(netdev);
> + can_state = priv->can.state;
> + stats = &netdev->stats;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + netdev_info(netdev, "%s: Int URB aborted: %pe\n", __func__,
> + ERR_PTR(urb->status));
> +
> + switch (urb->status) {
> + case 0: /* success */
> + break;
> +
> + case -ENOENT:
> + case -EPIPE:
> + case -EPROTO:
> + case -ESHUTDOWN:
> + return;
> +
> + default:
> + goto resubmit_urb;
> + }
> +
> + /* Note: ALC/ECC will not auto clear by read here, must to clear by
^^^^^^^^^^^^^
must be cleared
> + * read register (via handle_clear_reg_work).
> + */
> +
> + /* handle can bus errors */
> + if (data->isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
Move this in a function....
> + skb = alloc_can_err_skb(netdev, &cf);
Please continue the error handling, even in case of OOM.
> + if (!skb) {
> + netdev_warn(netdev,
> + "no memory to alloc_can_err_skb\n");
> + goto resubmit_urb;
> + }
> +
> + cf->can_id |= CAN_ERR_CNT;
> + cf->data[6] = data->txerr;
> + cf->data[7] = data->rxerr;
> +
> + if (data->isrc & IRQ_DOI) {
> + /* data overrun interrupt */
> + netdev_dbg(netdev, "data overrun interrupt\n");
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> + stats->rx_over_errors++;
> + stats->rx_errors++;
> +
> + schedule_work(&priv->handle_clear_overrun_work);
> + }
> +
> + if (data->isrc & IRQ_EI) {
> + /* error warning interrupt */
> + netdev_dbg(netdev, "error warning interrupt\n");
> +
> + if (data->sr & SR_BS)
> + can_state = CAN_STATE_BUS_OFF;
> + else if (data->sr & SR_ES)
> + can_state = CAN_STATE_ERROR_WARNING;
> + else
> + can_state = CAN_STATE_ERROR_ACTIVE;
> + }
> +
> + if (data->isrc & IRQ_BEI) {
> + /* bus error interrupt */
> + netdev_dbg(netdev, "bus error interrupt\n");
> +
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> +
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + /* set error type */
> + switch (data->ecc & ECC_MASK) {
> + case ECC_BIT:
> + cf->data[2] |= CAN_ERR_PROT_BIT;
> + break;
> + case ECC_FORM:
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> + case ECC_STUFF:
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> + default:
> + break;
> + }
> +
> + /* set error location */
> + cf->data[3] = data->ecc & ECC_SEG;
> +
> + /* Error occurred during transmission? */
> + if ((data->ecc & ECC_DIR) == 0)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->need_clear_ecc = true;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + schedule_work(&priv->handle_clear_reg_work);
> + }
> +
> + if (data->isrc & IRQ_EPI) {
> + if (can_state == CAN_STATE_ERROR_PASSIVE)
> + can_state = CAN_STATE_ERROR_WARNING;
> + else
> + can_state = CAN_STATE_ERROR_PASSIVE;
> +
> + /* error passive interrupt */
> + netdev_dbg(netdev, "error passive interrupt: %d\n",
> + can_state);
> + }
> +
> + if (data->isrc & IRQ_ALI) {
> + /* arbitration lost interrupt */
> + netdev_dbg(netdev, "arbitration lost interrupt\n");
> +
> + priv->can.can_stats.arbitration_lost++;
> + stats->tx_errors++;
Don't increase tx_errors, see bd0ccb92efb0 ("can: sja1000:
sja1000_err(): don't count arbitration lose as an error").
> + cf->can_id |= CAN_ERR_LOSTARB;
> + cf->data[0] = data->alc & ALC_MASK;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + priv->need_clear_alc = true;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + schedule_work(&priv->handle_clear_reg_work);
I'd move the schedule work call to the end of
this..:
if (priv->work_flags)
schedule_work()
> + }
> +
> + if (can_state != priv->can.state) {
> + tx_state = data->txerr >= data->rxerr ? can_state : 0;
> + rx_state = data->txerr <= data->rxerr ? can_state : 0;
> +
> + can_change_state(netdev, cf, tx_state, rx_state);
> +
> + if (can_state == CAN_STATE_BUS_OFF)
> + can_bus_off(netdev);
> + }
> +
> + netif_rx(skb);
> + }
> +
> + /* handle TX */
> + if (can_state != CAN_STATE_BUS_OFF)
you might move the data->isrc & IRQ_TI check here
> + f81604_handle_tx(urb);
I'd change the signature to struct f81604_port_priv *, struct f81604_int_data *data.
> +
> +resubmit_urb:
> + r = usb_submit_urb(urb, GFP_ATOMIC);
> + if (r) {
> + netdev_err(netdev,
> + "%s: failed resubmitting int bulk urb: %pe\n",
> + __func__, ERR_PTR(r));
> +
> + if (r == -ENODEV)
> + netif_device_detach(netdev);
> + }
> +}
> +
> +static netdev_tx_t f81604_start_xmit(struct sk_buff *skb,
> + struct net_device *netdev)
> +{
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + struct net_device_stats *stats = &netdev->stats;
> + struct f81604_bulk_data *ptr;
> + int status, len;
> +
> + if (can_dropped_invalid_skb(netdev, skb))
> + return NETDEV_TX_OK;
> +
> + netif_stop_queue(netdev);
> +
> + WARN_ON(sizeof(*ptr) != F81604_DATA_SIZE);
WARN_ON is a runtime check and makes no sense on compile time constants.
> +
> + ptr = (struct f81604_bulk_data *)priv->bulk_write_buffer;
> + memset(ptr, 0, F81604_DATA_SIZE);
> +
> + len = can_get_cc_dlc(cf, priv->can.ctrlmode);
This functions returns the "dlc" not the len. dlc might be > 8.
> + ptr->cmd = F81604_CMD_DATA;
> + ptr->dlc = len;
> +
> + if (cf->can_id & CAN_RTR_FLAG)
> + ptr->dlc |= F81604_DLC_RTR_BIT;
> +
> + if (cf->can_id & CAN_EFF_FLAG) {
> + ptr->eff.id = cpu_to_be32((cf->can_id & CAN_EFF_MASK) << 3);
> + ptr->dlc |= F81604_DLC_EFF_BIT;
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + memcpy(&ptr->eff.data, cf->data, len);
You must use cf->len instead of len here.
> + } else {
> + ptr->sff.id = cpu_to_be16((cf->can_id & CAN_SFF_MASK) << 5);
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + memcpy(&ptr->sff.data, cf->data, len);
You must use cf->len instead of len here.
> + }
> +
> + can_put_echo_skb(skb, netdev, 0, 0);
> +
> + status = usb_submit_urb(priv->write_urb, GFP_ATOMIC);
> + if (status) {
> + netdev_err(netdev, "%s: failed to resubmit tx bulk urb: %pe\n",
> + __func__, ERR_PTR(status));
> +
> + can_free_echo_skb(netdev, 0, NULL);
> + stats->tx_dropped++;
> +
> + if (status == -ENODEV)
> + netif_device_detach(netdev);
In the else path you probably want to netif_wake_queue()?
> + }
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static int f81604_get_berr_counter(const struct net_device *netdev,
> + struct can_berr_counter *bec)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int status;
> + u8 txerr;
> + u8 rxerr;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_TXERR, &txerr);
> + if (status)
> + return status;
> +
> + status = f81604_get_sja1000_register(priv->dev, netdev->dev_id,
> + SJA1000_RXERR, &rxerr);
> + if (status)
> + return status;
> +
> + bec->txerr = txerr;
> + bec->rxerr = rxerr;
> +
> + return 0;
> +}
> +
> +static void f81604_remove_urbs(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i)
> + usb_free_urb(priv->read_urb[i]);
usb_kill_anchored_urbs()
> +
> + usb_free_urb(priv->write_urb);
> + usb_free_urb(priv->int_urb);
> +}
> +
> +static int f81604_prepare_urbs(struct net_device *netdev)
> +{
> + static const u8 bulk_in_addr[F81604_MAX_DEV] = { 0x82, 0x84 };
> + static const u8 bulk_out_addr[F81604_MAX_DEV] = { 0x01, 0x03 };
> + static const u8 int_in_addr[F81604_MAX_DEV] = { 0x81, 0x83 };
Remove the 0x80 from the in endpoints, we need the proper endpoint
number here.
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> + int id = netdev->dev_id;
> + int i;
> +
> + for (i = 0; i < F81604_MAX_RX_URBS; ++i) {
> + priv->read_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->read_urb[i])
> + goto error;
> +
usb_anchor_urb()
> + usb_fill_bulk_urb(priv->read_urb[i], priv->dev,
> + usb_rcvbulkpipe(priv->dev, bulk_in_addr[id]),
> + priv->bulk_read_buffer[i], F81604_BULK_SIZE,
> + f81604_read_bulk_callback, netdev);
> + }
> +
> + priv->write_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->write_urb)
> + goto error;
> +
> + usb_fill_bulk_urb(priv->write_urb, priv->dev,
> + usb_sndbulkpipe(priv->dev, bulk_out_addr[id]),
> + priv->bulk_write_buffer, F81604_DATA_SIZE,
> + f81604_write_bulk_callback, netdev);
> +
> + priv->int_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->int_urb)
> + goto error;
> +
> + usb_fill_int_urb(priv->int_urb, priv->dev,
> + usb_rcvintpipe(priv->dev, int_in_addr[id]),
> + priv->int_read_buffer, F81604_INT_SIZE,
> + f81604_read_int_callback, netdev, 1);
Does the endpoint descriptor specify a proper interval?
> +
> + return 0;
> +
> +error:
> + f81604_remove_urbs(netdev);
> + return -ENOMEM;
> +}
> +
> +/* Open USB device */
> +static int f81604_open(struct net_device *netdev)
> +{
> + int err;
> +
> + err = open_candev(netdev);
> + if (err)
> + return err;
> +
> + err = f81604_start(netdev);
> + if (err)
> + goto start_failed;
> +
> + netif_start_queue(netdev);
> + return 0;
> +
> +start_failed:
> + if (err == -ENODEV)
> + netif_device_detach(netdev);
> +
> + close_candev(netdev);
> +
> + return err;
> +}
> +
> +/* Close USB device */
> +static int f81604_close(struct net_device *netdev)
> +{
> + struct f81604_port_priv *priv = netdev_priv(netdev);
> +
> + f81604_set_reset_mode(netdev);
> +
> + netif_stop_queue(netdev);
> + cancel_work_sync(&priv->handle_clear_overrun_work);
> + cancel_work_sync(&priv->handle_clear_reg_work);
> + close_candev(netdev);
> +
> + f81604_unregister_urbs(netdev);
> +
> + return 0;
> +}
> +
> +static const struct net_device_ops f81604_netdev_ops = {
> + .ndo_open = f81604_open,
> + .ndo_stop = f81604_close,
> + .ndo_start_xmit = f81604_start_xmit,
> + .ndo_change_mtu = can_change_mtu,
> +};
> +
> +static const struct can_bittiming_const f81604_bittiming_const = {
> + .name = "f81604",
> + .tseg1_min = 1,
> + .tseg1_max = 16,
> + .tseg2_min = 1,
> + .tseg2_max = 8,
> + .sjw_max = 4,
> + .brp_min = 1,
> + .brp_max = 64,
> + .brp_inc = 1,
> +};
> +
> +/* Called by the usb core when driver is unloaded or device is removed */
> +static void f81604_disconnect(struct usb_interface *intf)
> +{
> + struct f81604_priv *priv = usb_get_intfdata(intf);
> + int i;
> +
> + for (i = 0; i < F81604_MAX_DEV; ++i) {
^^^^^^^^^^^^^^
ARRAY_SIZE(priv->netdev)
> + if (!priv->netdev[i])
> + continue;
> +
> + unregister_netdev(priv->netdev[i]);
> + f81604_remove_urbs(priv->netdev[i]);
> + free_candev(priv->netdev[i]);
> + }
> +}
> +
> +static int f81604_set_termination(struct net_device *netdev, u16 term)
> +{
> + struct f81604_port_priv *port_priv = netdev_priv(netdev);
> + struct f81604_priv *priv;
> + u8 mask, data = 0;
> + int r;
> +
> + priv = usb_get_intfdata(port_priv->intf);
> +
> + if (netdev->dev_id == 0)
> + mask = F81604_CAN0_TERM;
> + else
> + mask = F81604_CAN1_TERM;
> +
> + if (term == F81604_TERMINATION_ENABLED)
> + data = mask;
> +
> + mutex_lock(&priv->mutex);
> +
> + r = f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
> + mask, data);
> +
> + mutex_unlock(&priv->mutex);
> +
> + return r;
> +}
> +
> +static int f81604_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *dev = interface_to_usbdev(intf);
> + struct f81604_port_priv *port_priv;
> + struct net_device *netdev;
> + struct f81604_priv *priv;
> + int i, err;
> +
> + priv = devm_kzalloc(&intf->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + usb_set_intfdata(intf, priv);
> +
> + for (i = 0; i < F81604_MAX_DEV; ++i) {
^^^^^^^^^^^^^^^^^^
same here
> + netdev = alloc_candev(sizeof(*port_priv), 1);
> + if (!netdev) {
> + dev_err(&intf->dev, "Couldn't alloc candev: %d\n", i);
> + err = -ENOMEM;
> +
> + goto failure_cleanup;
> + }
> +
> + port_priv = netdev_priv(netdev);
> + netdev->dev_id = i;
> +
> + mutex_init(&priv->mutex);
> + spin_lock_init(&port_priv->lock);
> +
> + INIT_WORK(&port_priv->handle_clear_overrun_work,
> + f81604_handle_clear_overrun_work);
> + INIT_WORK(&port_priv->handle_clear_reg_work,
> + f81604_handle_clear_reg_work);
> +
> + port_priv->intf = intf;
> + port_priv->dev = dev;
> + port_priv->ocr = OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL;
> + port_priv->cdr = CDR_CBP;
> + port_priv->can.state = CAN_STATE_STOPPED;
not need to assign
> + port_priv->can.clock.freq = F81604_CAN_CLOCK;
> +
> + port_priv->can.termination_const = f81604_termination;
> + port_priv->can.termination_const_cnt =
> + ARRAY_SIZE(f81604_termination);
> + port_priv->can.bittiming_const = &f81604_bittiming_const;
> + port_priv->can.do_set_bittiming = f81604_set_bittiming;
> + port_priv->can.do_set_mode = f81604_set_mode;
> + port_priv->can.do_set_termination = f81604_set_termination;
> + port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
> + port_priv->can.ctrlmode_supported =
> + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
> + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
> + CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
> +
> + netdev->ethtool_ops = &f81604_ethtool_ops;
> + netdev->netdev_ops = &f81604_netdev_ops;
> + netdev->flags |= IFF_ECHO;
> +
> + SET_NETDEV_DEV(netdev, &intf->dev);
> +
> + err = f81604_set_termination(netdev,
> + F81604_TERMINATION_DISABLED);
> + if (err)
> + goto clean_candev;
> +
> + err = f81604_prepare_urbs(netdev);
> + if (err)
> + goto clean_candev;
In case of a failure, the driver leaks urb, as priv->netdev[i] is not
assigned.
> +
> + err = register_candev(netdev);
> + if (err)
> + goto clean_candev;
> +
> + port_priv->netdev = netdev;
> + priv->netdev[i] = netdev;
> +
> + dev_info(&intf->dev, "Channel #%d registered as %s\n", i,
> + netdev->name);
> + }
> +
> + return 0;
> +
> +clean_candev:
> + netdev_err(netdev, "couldn't enable CAN device: %pe\n", ERR_PTR(err));
> + free_candev(netdev);
> +
> +failure_cleanup:
> + f81604_disconnect(intf);
> + return err;
> +}
> +
> +static struct usb_driver f81604_driver = {
> + .name = "f81604",
> + .probe = f81604_probe,
> + .disconnect = f81604_disconnect,
> + .id_table = f81604_table,
> +};
> +
> +module_usb_driver(f81604_driver);
> +
> +MODULE_AUTHOR("Ji-Ze Hong (Peter Hong) <[email protected]>");
> +MODULE_DESCRIPTION("Fintek F81604 USB to 2xCANBUS");
> +MODULE_LICENSE("GPL");
> --
> 2.17.1
>
>
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Vincent,
Vincent MAILHOL 於 2023/3/27 下午 06:27 寫道:
> eff->id is a 32 bit value. It is not aligned. So, you must always use
> {get|set}_unaligned_be32() to manipulate this value.
> N.B. on x86 architecture, unaligned access is fine, but some other
> architecture may throw a fault. Read this for more details:
>
> https://docs.kernel.org/arm/mem_alignment.html
for the consistency of the code, could I also add get/put_unaligned_be16
in SFF
sections ?
>> +static int f81604_set_reset_mode(struct net_device *netdev)
>> +{
>> + struct f81604_port_priv *priv = netdev_priv(netdev);
>> + int status, i;
>> + u8 tmp;
>> +
>> + /* disable interrupts */
>> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
>> + SJA1000_IER, IRQ_OFF);
>> + if (status)
>> + return status;
>> +
>> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> Thanks for removing F81604_USB_MAX_RETRY.
>
> Yet, I still would like to understand why you need one hundred tries?
> Is this some paranoiac safenet? Or does the device really need so many
> attempts to operate reliably? If those are needed, I would like to
> understand the root cause.
This section is copy from sja1000.c. In my test, the operation/reset may
retry 1 times.
I'll reduce it from 100 to 10 times.
>> + int status, len;
>> +
>> + if (can_dropped_invalid_skb(netdev, skb))
>> + return NETDEV_TX_OK;
>> +
>> + netif_stop_queue(netdev);
> In your driver, you send the CAN frames one at a time and wait for the
> rx_handler to restart the queue. This approach dramatically degrades
> the throughput. Is this a device limitation? Is the device not able to
> manage more than one frame at a time?
>
This device will not NAK on TX frame not complete, it only NAK on TX
endpoint
memory not processed, so we'll send next frame unitl TX complete(TI)
interrupt
received.
The device can polling status register via TX/RX endpoint, but it's more
complex.
We'll plan to do it when first driver landing in mainstream.
>> +static int f81604_set_termination(struct net_device *netdev, u16 term)
>> +{
>> + struct f81604_port_priv *port_priv = netdev_priv(netdev);
>> + struct f81604_priv *priv;
>> + u8 mask, data = 0;
>> + int r;
>> +
>> + priv = usb_get_intfdata(port_priv->intf);
>> +
>> + if (netdev->dev_id == 0)
>> + mask = F81604_CAN0_TERM;
>> + else
>> + mask = F81604_CAN1_TERM;
>> +
>> + if (term == F81604_TERMINATION_ENABLED)
>> + data = mask;
>> +
>> + mutex_lock(&priv->mutex);
> Did you witness a race condition?
>
> As far as I know, this call back is only called while the network
> stack big kernel lock (a.k.a. rtnl_lock) is being hold.
> If you have doubt, try adding a:
>
> ASSERT_RTNL()
>
> If this assert works, then another mutex is not needed.
It had added ASSERT_RTNL() into f81604_set_termination(). It only assert
in f81604_probe() -> f81604_set_termination(), not called via ip command:
ip link set dev can0 type can termination 120
ip link set dev can0 type can termination 0
so I'll still use mutex on here.
>> + port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
>> + port_priv->can.ctrlmode_supported =
>> + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
>> + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
>> + CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
> Did you test the CAN_CTRLMODE_CC_LEN8_DLC feature? Did you confirm
> that you can send and receive DLC greater than 8?
Sorry, I had misunderstand the define. This device is only support 0~8
data length,
so I'll remove CAN_CTRLMODE_CC_LEN8_DLC in future patch.
Thanks,
On Tue 28 Mar 2023 at 12:32, Peter Hong <[email protected]> wrote:
> Hi Vincent,
>
> Vincent MAILHOL 於 2023/3/27 下午 06:27 寫道:
> > eff->id is a 32 bit value. It is not aligned. So, you must always use
> > {get|set}_unaligned_be32() to manipulate this value.
> > N.B. on x86 architecture, unaligned access is fine, but some other
> > architecture may throw a fault. Read this for more details:
> >
> > https://docs.kernel.org/arm/mem_alignment.html
>
> for the consistency of the code, could I also add get/put_unaligned_be16
> in SFF
> sections ?
It is not needed. OK to mix.
> >> +static int f81604_set_reset_mode(struct net_device *netdev)
> >> +{
> >> + struct f81604_port_priv *priv = netdev_priv(netdev);
> >> + int status, i;
> >> + u8 tmp;
> >> +
> >> + /* disable interrupts */
> >> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> >> + SJA1000_IER, IRQ_OFF);
> >> + if (status)
> >> + return status;
> >> +
> >> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> > Thanks for removing F81604_USB_MAX_RETRY.
> >
> > Yet, I still would like to understand why you need one hundred tries?
> > Is this some paranoiac safenet? Or does the device really need so many
> > attempts to operate reliably? If those are needed, I would like to
> > understand the root cause.
>
> This section is copy from sja1000.c. In my test, the operation/reset may
> retry 1 times.
> I'll reduce it from 100 to 10 times.
Is it because the device is not ready? Does this only appear at
startup or at random?
>
> >> + int status, len;
> >> +
> >> + if (can_dropped_invalid_skb(netdev, skb))
> >> + return NETDEV_TX_OK;
> >> +
> >> + netif_stop_queue(netdev);
> > In your driver, you send the CAN frames one at a time and wait for the
> > rx_handler to restart the queue. This approach dramatically degrades
> > the throughput. Is this a device limitation? Is the device not able to
> > manage more than one frame at a time?
> >
>
> This device will not NAK on TX frame not complete, it only NAK on TX
> endpoint
> memory not processed, so we'll send next frame unitl TX complete(TI)
> interrupt
> received.
>
> The device can polling status register via TX/RX endpoint, but it's more
> complex.
> We'll plan to do it when first driver landing in mainstream.
OK for me to have this as a next step. Marc, what do you think?
> >> +static int f81604_set_termination(struct net_device *netdev, u16 term)
> >> +{
> >> + struct f81604_port_priv *port_priv = netdev_priv(netdev);
> >> + struct f81604_priv *priv;
> >> + u8 mask, data = 0;
> >> + int r;
> >> +
> >> + priv = usb_get_intfdata(port_priv->intf);
> >> +
> >> + if (netdev->dev_id == 0)
> >> + mask = F81604_CAN0_TERM;
> >> + else
> >> + mask = F81604_CAN1_TERM;
> >> +
> >> + if (term == F81604_TERMINATION_ENABLED)
> >> + data = mask;
> >> +
> >> + mutex_lock(&priv->mutex);
> > Did you witness a race condition?
> >
> > As far as I know, this call back is only called while the network
> > stack big kernel lock (a.k.a. rtnl_lock) is being hold.
> > If you have doubt, try adding a:
> >
> > ASSERT_RTNL()
> >
> > If this assert works, then another mutex is not needed.
>
> It had added ASSERT_RTNL() into f81604_set_termination(). It only assert
> in f81604_probe() -> f81604_set_termination(), not called via ip command:
> ip link set dev can0 type can termination 120
> ip link set dev can0 type can termination 0
>
> so I'll still use mutex on here.
Sorry, do you mean that the assert throws warnings for f81604_probe()
-> f81604_set_termination() but that it is OK (no warning) for ip
command?
I did not see that you called f81604_set_termination() internally.
Indeed, rtnl_lock is not held in probe(). But I think it is still OK.
In f81604_probe() you call f81604_set_termination() before
register_candev(). If the device is not yet registered,
f81604_set_termination() can not yet be called via ip command. Can you
describe more precisely where you think there is a concurrency issue?
I still do not see it.
> >> + port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
> >> + port_priv->can.ctrlmode_supported =
> >> + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
> >> + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
> >> + CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
> > Did you test the CAN_CTRLMODE_CC_LEN8_DLC feature? Did you confirm
> > that you can send and receive DLC greater than 8?
>
> Sorry, I had misunderstand the define. This device is only support 0~8
> data length,
^^^^^^^^^^^
Data length or Data Length Code (DLC)? Classical CAN maximum data
length is 8 but maximum DLC is 15 (and DLC 8 to 15 mean a data length
of 8).
> so I'll remove CAN_CTRLMODE_CC_LEN8_DLC in future patch.
On 28.03.2023 13:49:05, Vincent MAILHOL wrote:
[...]
> > >> + int status, len;
> > >> +
> > >> + if (can_dropped_invalid_skb(netdev, skb))
> > >> + return NETDEV_TX_OK;
> > >> +
> > >> + netif_stop_queue(netdev);
> > > In your driver, you send the CAN frames one at a time and wait for the
> > > rx_handler to restart the queue. This approach dramatically degrades
> > > the throughput. Is this a device limitation? Is the device not able to
> > > manage more than one frame at a time?
> > >
> >
> > This device will not NAK on TX frame not complete, it only NAK on TX
> > endpoint
> > memory not processed, so we'll send next frame unitl TX complete(TI)
> > interrupt
> > received.
> >
> > The device can polling status register via TX/RX endpoint, but it's more
> > complex.
> > We'll plan to do it when first driver landing in mainstream.
>
> OK for me to have this as a next step. Marc, what do you think?
Fine with me. First make it work, then make it fast.
But I think this will never be a fast and resource-efficient USB CAN
adapter. There are exiting drivers with an open and documented USB
interface (gs_usb) and Open Source µC implementations (candlelight) with
better performance.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Marc,
Marc Kleine-Budde 於 2023/3/27 下午 11:09 寫道:
> On 27.03.2023 13:10:48, Ji-Ze Hong (Peter Hong) wrote:
>> This patch add support for Fintek USB to 2CAN controller support.
> In addition to Vincent's comment, please describe your change
> declarative, e.g.:
>
> | Add support for the Fintek USB to 2CAN controller.
>
> Please add an entry in the MAINTAINERS file.
>
We'll add description to MAINTAINERS file as following:
FINTEK F81604 USB to 2CAN DEVICE DRIVERS
M: Ji-Ze Hong (Peter Hong) <[email protected]>
L: [email protected]
S: Maintained
Is this OK?
>> + u8 bulk_read_buffer[F81604_MAX_RX_URBS][F81604_BULK_SIZE];
> allocate the URB dynamic with kmalloc() and use urb->transfer_flags |=
> URB_FREE_BUFFER for automatic free()ing.
Could I use kmalloc on device probe() and kfree on disconnect() ?
It may reduce a lot malloc/free times.
>> +
>> + struct urb *write_urb;
>> + u8 bulk_write_buffer[F81604_DATA_SIZE];
> With just 1 TX URB the TX would be quite slow. Does your hardware
> support more than 1 TX URB at a time? USB devices usually answer with
> NAK it they are busy, automatic and transparent retry is handled by the
> USB host controller.
This device only NAK when TX EP buf full. It maybe malfunction when sending
TX URB while CAN transmitting.
>> +};
>> +
>> +/* Interrupt endpoint data format: SR/IR/IER/ALC/ECC/EWLR/RXERR/TXERR/VAL */
>> +struct f81604_int_data {
>> + u8 sr;
>> + u8 isrc;
>> + u8 ier;
>> + u8 alc;
>> + u8 ecc;
>> + u8 ewlr;
>> + u8 rxerr;
>> + u8 txerr;
>> + u8 val;
>> +} __packed;
> I think this struct is always aligned to 4 bytes, so add a __aligned(4)
> behind __packed.
>> +
>> +struct f81604_sff {
>> + __be16 id;
>> + u8 data[CAN_MAX_DLEN];
>> +} __packed;
> __aligned(2)
>
>> +
>> +struct f81604_eff {
>> + __be32 id;
>> + u8 data[CAN_MAX_DLEN];
>> +} __packed;
> __aligned(2)
>
>> +
>> +struct f81604_bulk_data {
>> + u8 cmd;
>> +
>> + /* According for F81604 DLC define:
>> + * #define F81604_DLC_LEN_MASK 0x0f
>> + * #define F81604_DLC_EFF_BIT BIT(7)
>> + * #define F81604_DLC_RTR_BIT BIT(6)
> no need to duplicate code
>
>> + */
>> + u8 dlc;
>> +
>> + union {
>> + struct f81604_sff sff;
>> + struct f81604_eff eff;
>> + };
>> +} __packed;
> __aligned(4)
I had add following aligned is OK.
struct f81604_int_data; __aligned(4)
struct f81604_sff; __aligned(2)
struct f81604_eff; __aligned(2)
But when I add __aligned(4) to struct f81604_bulk_data, It'll generate
error about size != 14.
./include/linux/build_bug.h:78:41: error: static assertion failed:
"sizeof(struct f81604_bulk_data) == F81604_DATA_SIZE"
So I'll add align to these structs without f81604_bulk_data. Is it OK?
>> + usb_fill_bulk_urb(priv->read_urb[i], priv->dev,
>> + usb_rcvbulkpipe(priv->dev, bulk_in_addr[id]),
>> + priv->bulk_read_buffer[i], F81604_BULK_SIZE,
>> + f81604_read_bulk_callback, netdev);
>> + }
>> +
>> + priv->write_urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!priv->write_urb)
>> + goto error;
>> +
>> + usb_fill_bulk_urb(priv->write_urb, priv->dev,
>> + usb_sndbulkpipe(priv->dev, bulk_out_addr[id]),
>> + priv->bulk_write_buffer, F81604_DATA_SIZE,
>> + f81604_write_bulk_callback, netdev);
>> +
>> + priv->int_urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!priv->int_urb)
>> + goto error;
>> +
>> + usb_fill_int_urb(priv->int_urb, priv->dev,
>> + usb_rcvintpipe(priv->dev, int_in_addr[id]),
>> + priv->int_read_buffer, F81604_INT_SIZE,
>> + f81604_read_int_callback, netdev, 1);
> Does the endpoint descriptor specify a proper interval?
The interval of interrupt endpoint is 1 (1ms).
Bus 001 Device 010: ID 2c42:1709 FINTEK USB TO CANBUS BRIDGE
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 1.10
bDeviceClass 0
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 64
idVendor 0x2c42
idProduct 0x1709
bcdDevice 0.01
iManufacturer 1 FINTEK
iProduct 2 USB TO CANBUS BRIDGE
iSerial 3 88635600168801
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 0x003c
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0xa0
(Bus Powered)
Remote Wakeup
MaxPower 100mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 6
bInterfaceClass 0
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0010 1x 16 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0010 1x 16 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x03 EP 3 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 0
Device Status: 0xa780
(Bus Powered)
Thanks
Hi Vincent,
Vincent MAILHOL 於 2023/3/28 下午 12:49 寫道:
>>>> +static int f81604_set_reset_mode(struct net_device *netdev)
>>>> +{
>>>> + struct f81604_port_priv *priv = netdev_priv(netdev);
>>>> + int status, i;
>>>> + u8 tmp;
>>>> +
>>>> + /* disable interrupts */
>>>> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
>>>> + SJA1000_IER, IRQ_OFF);
>>>> + if (status)
>>>> + return status;
>>>> +
>>>> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
>>> Thanks for removing F81604_USB_MAX_RETRY.
>>>
>>> Yet, I still would like to understand why you need one hundred tries?
>>> Is this some paranoiac safenet? Or does the device really need so many
>>> attempts to operate reliably? If those are needed, I would like to
>>> understand the root cause.
>> This section is copy from sja1000.c. In my test, the operation/reset may
>> retry 1 times.
>> I'll reduce it from 100 to 10 times.
> Is it because the device is not ready? Does this only appear at
> startup or at random?
I'm using ip link up/down to test open/close(). It's may not ready so fast.
but the maximum retry is only 1 for test 10000 times.
>>>> +static int f81604_set_termination(struct net_device *netdev, u16 term)
>>>> +{
>>>> + struct f81604_port_priv *port_priv = netdev_priv(netdev);
>>>> + struct f81604_priv *priv;
>>>> + u8 mask, data = 0;
>>>> + int r;
>>>> +
>>>> + priv = usb_get_intfdata(port_priv->intf);
>>>> +
>>>> + if (netdev->dev_id == 0)
>>>> + mask = F81604_CAN0_TERM;
>>>> + else
>>>> + mask = F81604_CAN1_TERM;
>>>> +
>>>> + if (term == F81604_TERMINATION_ENABLED)
>>>> + data = mask;
>>>> +
>>>> + mutex_lock(&priv->mutex);
>>> Did you witness a race condition?
>>>
>>> As far as I know, this call back is only called while the network
>>> stack big kernel lock (a.k.a. rtnl_lock) is being hold.
>>> If you have doubt, try adding a:
>>>
>>> ASSERT_RTNL()
>>>
>>> If this assert works, then another mutex is not needed.
>> It had added ASSERT_RTNL() into f81604_set_termination(). It only assert
>> in f81604_probe() -> f81604_set_termination(), not called via ip command:
>> ip link set dev can0 type can termination 120
>> ip link set dev can0 type can termination 0
>>
>> so I'll still use mutex on here.
> Sorry, do you mean that the assert throws warnings for f81604_probe()
> -> f81604_set_termination() but that it is OK (no warning) for ip
> command?
>
> I did not see that you called f81604_set_termination() internally.
> Indeed, rtnl_lock is not held in probe(). But I think it is still OK.
> In f81604_probe() you call f81604_set_termination() before
> register_candev(). If the device is not yet registered,
> f81604_set_termination() can not yet be called via ip command. Can you
> describe more precisely where you think there is a concurrency issue?
> I still do not see it.
Sorry, I had inverse the mean of ASSERT_RTNL(). It like you said.
f81604_probe() not held rtnl_lock.
ip set terminator will held rtnl_lock.
Due to f81604_set_termination() will called by f81604_probe() to
initialize, it may need mutex in
situation as following:
User is setting can0 terminator when f81604_probe() complete generate
can0 and generating can1.
So IMO, the mutex may needed.
>>>> + port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
>>>> + port_priv->can.ctrlmode_supported =
>>>> + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
>>>> + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
>>>> + CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
>>> Did you test the CAN_CTRLMODE_CC_LEN8_DLC feature? Did you confirm
>>> that you can send and receive DLC greater than 8?
>> Sorry, I had misunderstand the define. This device is only support 0~8
>> data length,
> ^^^^^^^^^^^
>
> Data length or Data Length Code (DLC)? Classical CAN maximum data
> length is 8 but maximum DLC is 15 (and DLC 8 to 15 mean a data length
> of 8).
>
This device can't support DLC > 8. It's only support 0~8.
Thanks
On Thu. 30 Mars 2023 at 15:49, Peter Hong <[email protected]> wrote:
> Hi Vincent,
>
> Vincent MAILHOL 於 2023/3/28 下午 12:49 寫道:
> >>>> +static int f81604_set_reset_mode(struct net_device *netdev)
> >>>> +{
> >>>> + struct f81604_port_priv *priv = netdev_priv(netdev);
> >>>> + int status, i;
> >>>> + u8 tmp;
> >>>> +
> >>>> + /* disable interrupts */
> >>>> + status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> >>>> + SJA1000_IER, IRQ_OFF);
> >>>> + if (status)
> >>>> + return status;
> >>>> +
> >>>> + for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> >>> Thanks for removing F81604_USB_MAX_RETRY.
> >>>
> >>> Yet, I still would like to understand why you need one hundred tries?
> >>> Is this some paranoiac safenet? Or does the device really need so many
> >>> attempts to operate reliably? If those are needed, I would like to
> >>> understand the root cause.
> >> This section is copy from sja1000.c. In my test, the operation/reset may
> >> retry 1 times.
> >> I'll reduce it from 100 to 10 times.
> > Is it because the device is not ready? Does this only appear at
> > startup or at random?
>
> I'm using ip link up/down to test open/close(). It's may not ready so fast.
> but the maximum retry is only 1 for test 10000 times.
Ack, thanks for the explanation.
> >>>> +static int f81604_set_termination(struct net_device *netdev, u16 term)
> >>>> +{
> >>>> + struct f81604_port_priv *port_priv = netdev_priv(netdev);
> >>>> + struct f81604_priv *priv;
> >>>> + u8 mask, data = 0;
> >>>> + int r;
> >>>> +
> >>>> + priv = usb_get_intfdata(port_priv->intf);
> >>>> +
> >>>> + if (netdev->dev_id == 0)
> >>>> + mask = F81604_CAN0_TERM;
> >>>> + else
> >>>> + mask = F81604_CAN1_TERM;
> >>>> +
> >>>> + if (term == F81604_TERMINATION_ENABLED)
> >>>> + data = mask;
> >>>> +
> >>>> + mutex_lock(&priv->mutex);
> >>> Did you witness a race condition?
> >>>
> >>> As far as I know, this call back is only called while the network
> >>> stack big kernel lock (a.k.a. rtnl_lock) is being hold.
> >>> If you have doubt, try adding a:
> >>>
> >>> ASSERT_RTNL()
> >>>
> >>> If this assert works, then another mutex is not needed.
> >> It had added ASSERT_RTNL() into f81604_set_termination(). It only assert
> >> in f81604_probe() -> f81604_set_termination(), not called via ip command:
> >> ip link set dev can0 type can termination 120
> >> ip link set dev can0 type can termination 0
> >>
> >> so I'll still use mutex on here.
> > Sorry, do you mean that the assert throws warnings for f81604_probe()
> > -> f81604_set_termination() but that it is OK (no warning) for ip
> > command?
> >
> > I did not see that you called f81604_set_termination() internally.
> > Indeed, rtnl_lock is not held in probe(). But I think it is still OK.
> > In f81604_probe() you call f81604_set_termination() before
> > register_candev(). If the device is not yet registered,
> > f81604_set_termination() can not yet be called via ip command. Can you
> > describe more precisely where you think there is a concurrency issue?
> > I still do not see it.
>
> Sorry, I had inverse the mean of ASSERT_RTNL(). It like you said.
> f81604_probe() not held rtnl_lock.
> ip set terminator will held rtnl_lock.
>
> Due to f81604_set_termination() will called by f81604_probe() to
> initialize, it may need mutex in
> situation as following:
>
> User is setting can0 terminator when f81604_probe() complete generate
> can0 and generating can1.
> So IMO, the mutex may needed.
Hmm, I am still not a fan of setting a mutex for a single concurrency
issue which can only happen during probing.
What about this:
static int __f81604_set_termination(struct net_device *netdev, u16 term)
{
struct f81604_port_priv *port_priv = netdev_priv(netdev);
u8 mask, data = 0;
if (netdev->dev_id == 0)
mask = F81604_CAN0_TERM;
else
mask = F81604_CAN1_TERM;
if (term == F81604_TERMINATION_ENABLED)
data = mask;
return f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
mask, data);
}
static int f81604_set_termination(struct net_device *netdev, u16 term)
{
ASSERT_RTNL();
return __f81604_set_termination(struct net_device *netdev, u16 term);
}
static int f81604_init_termination(struct f81604_priv *priv)
{
int i, ret;
for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
ret = __f81604_set_termination(f81604_priv->netdev[i],
F81604_TERMINATION_DISABLED);
if (ret)
return ret;
}
}
static int f81604_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
/* ... */
err = f81604_init_termination(priv);
if (err)
goto failure_cleanup;
for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
/* ... */
}
/* ... */
}
Initialise all resistors with __f81604_set_termination() in probe()
before registering any network device. Use f81604_set_termination()
which has the lock assert elsewhere.
Also, looking at your probe() function, in label clean_candev:, if the
second can channel fails its initialization, you do not clean the
first can channel. I suggest adding a f81604_init_netdev() and
handling the netdev issue and cleanup in that function.
> >>>> + port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
> >>>> + port_priv->can.ctrlmode_supported =
> >>>> + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
> >>>> + CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
> >>>> + CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
> >>> Did you test the CAN_CTRLMODE_CC_LEN8_DLC feature? Did you confirm
> >>> that you can send and receive DLC greater than 8?
> >> Sorry, I had misunderstand the define. This device is only support 0~8
> >> data length,
> > ^^^^^^^^^^^
> >
> > Data length or Data Length Code (DLC)? Classical CAN maximum data
> > length is 8 but maximum DLC is 15 (and DLC 8 to 15 mean a data length
> > of 8).
> >
>
> This device can't support DLC > 8. It's only support 0~8.
Ack.
Hi Vincent,
Vincent MAILHOL 於 2023/3/30 下午 09:11 寫道:
> Hmm, I am still not a fan of setting a mutex for a single concurrency
> issue which can only happen during probing.
>
> What about this:
>
> static int __f81604_set_termination(struct net_device *netdev, u16 term)
> {
> struct f81604_port_priv *port_priv = netdev_priv(netdev);
> u8 mask, data = 0;
>
> if (netdev->dev_id == 0)
> mask = F81604_CAN0_TERM;
> else
> mask = F81604_CAN1_TERM;
>
> if (term == F81604_TERMINATION_ENABLED)
> data = mask;
>
> return f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
> mask, data);
> }
>
> static int f81604_set_termination(struct net_device *netdev, u16 term)
> {
> ASSERT_RTNL();
>
> return __f81604_set_termination(struct net_device *netdev, u16 term);
> }
>
> static int f81604_init_termination(struct f81604_priv *priv)
> {
> int i, ret;
>
> for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
> ret = __f81604_set_termination(f81604_priv->netdev[i],
> F81604_TERMINATION_DISABLED);
> if (ret)
> return ret;
> }
> }
>
> static int f81604_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> /* ... */
>
> err = f81604_init_termination(priv);
> if (err)
> goto failure_cleanup;
>
> for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
> /* ... */
> }
>
> /* ... */
> }
>
> Initialise all resistors with __f81604_set_termination() in probe()
> before registering any network device. Use f81604_set_termination()
> which has the lock assert elsewhere.
The f81604_set_termination() will transform into the following code:
static int f81604_write(struct usb_device *dev, u16 reg, u8 data);
static int f81604_read(struct usb_device *dev, u16 reg, u8 *data);
static int f81604_update_bits(struct usb_device *dev, u16 reg, u8 mask,
u8 data);
static int __f81604_set_termination(struct usb_device *dev, int idx, u16
term)
{
u8 mask, data = 0;
if (idx == 0)
mask = F81604_CAN0_TERM;
else
mask = F81604_CAN1_TERM;
if (term)
data = mask;
return f81604_update_bits(dev, F81604_TERMINATOR_REG, mask, data);
}
static int f81604_set_termination(struct net_device *netdev, u16 term)
{
struct f81604_port_priv *port_priv = netdev_priv(netdev);
struct f81604_priv *priv;
ASSERT_RTNL();
priv = usb_get_intfdata(port_priv->intf);
return __f81604_set_termination(port_priv->dev, netdev->dev_id, term);
}
and also due to f81604_write() / f81604_read() / f81604_update_bits()
may use
in f81604_probe() without port private data, so we'll change their first
parameter
from "struct f81604_port_priv *priv" to "struct usb_device *dev". Is it OK ?
> Also, looking at your probe() function, in label clean_candev:, if the
> second can channel fails its initialization, you do not clean the
> first can channel. I suggest adding a f81604_init_netdev() and
> handling the netdev issue and cleanup in that function.
When the second can channel failed its initialization, the label
"clean_candev" will
clear second "netdev" object and the first "netdev" will cleanup in
f81604_disconnect().
Could I remain this section of code ?
Thanks
Hi Peter,
On Mon. 10 Apr 2023 at 14:52, Peter Hong <[email protected]> wrote:
> Hi Vincent,
>
> Vincent MAILHOL 於 2023/3/30 下午 09:11 寫道:
> > Hmm, I am still not a fan of setting a mutex for a single concurrency
> > issue which can only happen during probing.
> >
> > What about this:
> >
> > static int __f81604_set_termination(struct net_device *netdev, u16 term)
> > {
> > struct f81604_port_priv *port_priv = netdev_priv(netdev);
> > u8 mask, data = 0;
> >
> > if (netdev->dev_id == 0)
> > mask = F81604_CAN0_TERM;
> > else
> > mask = F81604_CAN1_TERM;
> >
> > if (term == F81604_TERMINATION_ENABLED)
> > data = mask;
> >
> > return f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG,
> > mask, data);
> > }
> >
> > static int f81604_set_termination(struct net_device *netdev, u16 term)
> > {
> > ASSERT_RTNL();
> >
> > return __f81604_set_termination(struct net_device *netdev, u16 term);
> > }
> >
> > static int f81604_init_termination(struct f81604_priv *priv)
> > {
> > int i, ret;
> >
> > for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
> > ret = __f81604_set_termination(f81604_priv->netdev[i],
> > F81604_TERMINATION_DISABLED);
> > if (ret)
> > return ret;
> > }
> > }
> >
> > static int f81604_probe(struct usb_interface *intf,
> > const struct usb_device_id *id)
> > {
> > /* ... */
> >
> > err = f81604_init_termination(priv);
> > if (err)
> > goto failure_cleanup;
> >
> > for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) {
> > /* ... */
> > }
> >
> > /* ... */
> > }
> >
> > Initialise all resistors with __f81604_set_termination() in probe()
> > before registering any network device. Use f81604_set_termination()
> > which has the lock assert elsewhere.
>
> The f81604_set_termination() will transform into the following code:
>
> static int f81604_write(struct usb_device *dev, u16 reg, u8 data);
> static int f81604_read(struct usb_device *dev, u16 reg, u8 *data);
> static int f81604_update_bits(struct usb_device *dev, u16 reg, u8 mask,
> u8 data);
>
> static int __f81604_set_termination(struct usb_device *dev, int idx, u16
> term)
> {
> u8 mask, data = 0;
>
> if (idx == 0)
> mask = F81604_CAN0_TERM;
> else
> mask = F81604_CAN1_TERM;
>
> if (term)
> data = mask;
>
> return f81604_update_bits(dev, F81604_TERMINATOR_REG, mask, data);
> }
>
> static int f81604_set_termination(struct net_device *netdev, u16 term)
> {
> struct f81604_port_priv *port_priv = netdev_priv(netdev);
> struct f81604_priv *priv;
>
> ASSERT_RTNL();
>
> priv = usb_get_intfdata(port_priv->intf);
^^^^
Why do you need priv here? I do not see it used in the next line.
> return __f81604_set_termination(port_priv->dev, netdev->dev_id, term);
> }
>
> and also due to f81604_write() / f81604_read() / f81604_update_bits()
> may use
> in f81604_probe() without port private data, so we'll change their first
> parameter
> from "struct f81604_port_priv *priv" to "struct usb_device *dev". Is it OK ?
Right now, it is hard to visualize the final result. Please send what
you think is the best and we will review.
> > Also, looking at your probe() function, in label clean_candev:, if the
> > second can channel fails its initialization, you do not clean the
> > first can channel. I suggest adding a f81604_init_netdev() and
> > handling the netdev issue and cleanup in that function.
>
> When the second can channel failed its initialization, the label
> "clean_candev" will
> clear second "netdev" object and the first "netdev" will cleanup in
> f81604_disconnect().
>
> Could I remain this section of code ?
Oh! I was not aware that disconnect() would be called on a failed
probe. Overall, I prefer the use of subfunctions because it makes it
easier to understand the logic, especially for the cleanup after
failure. Let's say that it is acceptable as-is. OK to keep.