| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOoeyxWbr6jfZjPvYFD+vHKMZ9CpM6SLt+2xo-4E-NnhGinfvg@mail.gmail.com>
Date: Thu, 8 May 2025 11:26:09 +0800
From: Ming Yu <a0282524688@...il.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl,
andi.shyti@...nel.org, mailhol.vincent@...adoo.fr, andrew+netdev@...n.ch,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com,
alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-can@...r.kernel.org, netdev@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-rtc@...r.kernel.org, linux-usb@...r.kernel.org,
Ming Yu <tmyu0@...oton.com>
Subject: Re: [PATCH v10 4/7] can: Add Nuvoton NCT6694 CANFD support
Dear Marc,
Thank you for reviewing.
Marc Kleine-Budde <mkl@...gutronix.de> 於 2025年5月3日 週六 下午9:57寫道：
> > This driver supports Socket CANFD functionality for NCT6694 MFD
> > device based on USB interface.
> >
> > Signed-off-by: Ming Yu <tmyu0@...oton.com>
>
> The destroy functions nct6694_canfd_close() and nct6694_canfd_remove()
> are not the exact inverse of their init functions. Se comments inline.
>
> Please fix and add:
>
> Reviewed-by: Marc Kleine-Budde <mkl@...gutronix.de>
>
> Feel free to mainline this patch as part of the series outside of the
> linux-can-next tree. Better ask the netdev maintainers for their OK, too.
>
> What about transceiver delay compensation for higher CAN-FD bitrates?
> How does you device handle these?
>
In the CAN CMD0's DBTP field, bit 23 is the TDC flag, I will add
support for enabling tdc, and firmware will automatically configure
tdco. Do you think this approach is appropriate?
> > ---
> > MAINTAINERS | 1 +
> > drivers/net/can/usb/Kconfig | 11 +
> > drivers/net/can/usb/Makefile | 1 +
> > drivers/net/can/usb/nct6694_canfd.c | 814 ++++++++++++++++++++++++++++
> > 4 files changed, 827 insertions(+)
> > create mode 100644 drivers/net/can/usb/nct6694_canfd.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 751b9108524a..ee8583edc2d2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17364,6 +17364,7 @@ S: Supported
> > F: drivers/gpio/gpio-nct6694.c
> > F: drivers/i2c/busses/i2c-nct6694.c
> > F: drivers/mfd/nct6694.c
> > +F: drivers/net/can/usb/nct6694_canfd.c
> > F: include/linux/mfd/nct6694.h
> >
> > NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
> > diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> > index 9dae0c71a2e1..759e724a67cf 100644
> > --- a/drivers/net/can/usb/Kconfig
> > +++ b/drivers/net/can/usb/Kconfig
> > @@ -133,6 +133,17 @@ config CAN_MCBA_USB
> > This driver supports the CAN BUS Analyzer interface
> > from Microchip (http://www.microchip.com/development-tools/).
> >
> > +config CAN_NCT6694
> > + tristate "Nuvoton NCT6694 Socket CANfd support"
> > + depends on MFD_NCT6694
> > + select CAN_RX_OFFLOAD
> > + help
> > + If you say yes to this option, support will be included for Nuvoton
> > + NCT6694, a USB device to socket CANfd controller.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called nct6694_canfd.
> > +
> > config CAN_PEAK_USB
> > tristate "PEAK PCAN-USB/USB Pro interfaces for CAN 2.0b/CAN-FD"
> > help
> > diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> > index 8b11088e9a59..fcafb1ac262e 100644
> > --- a/drivers/net/can/usb/Makefile
> > +++ b/drivers/net/can/usb/Makefile
> > @@ -11,5 +11,6 @@ obj-$(CONFIG_CAN_F81604) += f81604.o
> > obj-$(CONFIG_CAN_GS_USB) += gs_usb.o
> > obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/
> > obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
> > +obj-$(CONFIG_CAN_NCT6694) += nct6694_canfd.o
> > obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> > obj-$(CONFIG_CAN_UCAN) += ucan.o
> > diff --git a/drivers/net/can/usb/nct6694_canfd.c b/drivers/net/can/usb/nct6694_canfd.c
> > new file mode 100644
> > index 000000000000..9cf6230ffb7d
> > --- /dev/null
> > +++ b/drivers/net/can/usb/nct6694_canfd.c
> > @@ -0,0 +1,814 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Nuvoton NCT6694 Socket CANfd driver based on USB interface.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
> > + */
> > +
> > +#include <linux/can/dev.h>
> > +#include <linux/can/rx-offload.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/nct6694.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DEVICE_NAME "nct6694-canfd"
> > +
> > +/* USB command module type for NCT6694 CANfd controller.
> > + * This defines the module type used for communication with the NCT6694
> > + * CANfd controller over the USB interface.
> > + */
> > +#define NCT6694_CANFD_MOD 0x05
> > +
> > +/* Command 00h - CAN Setting and Initialization */
> > +#define NCT6694_CANFD_SETTING 0x00
> > +#define NCT6694_CANFD_SETTING_ACTIVE_CTRL1 BIT(0)
> > +#define NCT6694_CANFD_SETTING_ACTIVE_CTRL2 BIT(1)
> > +#define NCT6694_CANFD_SETTING_ACTIVE_NBTP_DBTP BIT(2)
> > +#define NCT6694_CANFD_SETTING_CTRL1_MON BIT(0)
> > +#define NCT6694_CANFD_SETTING_CTRL1_NISO BIT(1)
> > +#define NCT6694_CANFD_SETTING_CTRL1_LBCK BIT(2)
> > +#define NCT6694_CANFD_SETTING_NBTP_NTSEG2 GENMASK(6, 0)
> > +#define NCT6694_CANFD_SETTING_NBTP_NTSEG1 GENMASK(15, 8)
> > +#define NCT6694_CANFD_SETTING_NBTP_NBRP GENMASK(24, 16)
> > +#define NCT6694_CANFD_SETTING_NBTP_NSJW GENMASK(31, 25)
> > +#define NCT6694_CANFD_SETTING_DBTP_DSJW GENMASK(3, 0)
> > +#define NCT6694_CANFD_SETTING_DBTP_DTSEG2 GENMASK(7, 4)
> > +#define NCT6694_CANFD_SETTING_DBTP_DTSEG1 GENMASK(12, 8)
> > +#define NCT6694_CANFD_SETTING_DBTP_DBRP GENMASK(20, 16)
I will add the macro in v11: #define NCT6694_CANFD_SETTING_DBTP_TDC
> > +
> > +/* Command 01h - CAN Information */
> > +#define NCT6694_CANFD_INFORMATION 0x01
> > +#define NCT6694_CANFD_INFORMATION_SEL 0x00
> > +
> > +/* Command 02h - CAN Event */
> > +#define NCT6694_CANFD_EVENT 0x02
> > +#define NCT6694_CANFD_EVENT_SEL(idx, mask) \
> > + ((idx ? 0x80 : 0x00) | ((mask) & 0x7F))
> > +
> > +#define NCT6694_CANFD_EVENT_MASK GENMASK(5, 0)
> > +#define NCT6694_CANFD_EVT_TX_FIFO_EMPTY BIT(7) /* Read-clear */
> > +#define NCT6694_CANFD_EVT_RX_DATA_LOST BIT(5) /* Read-clear */
> > +#define NCT6694_CANFD_EVT_RX_DATA_IN BIT(7) /* Read-clear*/
> ^^
> add a space
>
> > +
...
> > +static int nct6694_canfd_start(struct net_device *ndev)
> > +{
> > + struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > + const struct can_bittiming *d_bt = &priv->can.data_bittiming;
> > + const struct can_bittiming *n_bt = &priv->can.bittiming;
> > + struct nct6694_canfd_setting *setting __free(kfree) = NULL;
> > + const struct nct6694_cmd_header cmd_hd = {
> > + .mod = NCT6694_CANFD_MOD,
> > + .cmd = NCT6694_CANFD_SETTING,
> > + .sel = ndev->dev_port,
> > + .len = cpu_to_le16(sizeof(*setting))
> > + };
> > + int ret;
> > +
> > + setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> > + if (!setting)
> > + return -ENOMEM;
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_MON);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_NISO);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_LBCK);
> > +
> > + /* Disable clock divider */
> > + setting->ctrl2 = 0;
> > +
> > + setting->nbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NSJW,
> > + n_bt->sjw - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NBRP,
> > + n_bt->brp - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG2,
> > + n_bt->phase_seg2 - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG1,
> > + n_bt->prop_seg + n_bt->phase_seg1 - 1));
> > +
> > + setting->dbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DSJW,
> > + d_bt->sjw - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DBRP,
> > + d_bt->brp - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG2,
> > + d_bt->phase_seg2 - 1) |
> > + FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG1,
> > + d_bt->prop_seg + d_bt->phase_seg1 - 1));
I'll update the code to:
setting->dbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DSJW,
d_bt->sjw - 1) |
FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DBRP,
d_bt->brp - 1) |
FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG2,
d_bt->phase_seg2 - 1) |
FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG1,
d_bt->prop_seg +
d_bt->phase_seg1 - 1) |
NCT6694_CANFD_SETTING_DBTP_TDC);
> > +
> > + setting->active = NCT6694_CANFD_SETTING_ACTIVE_CTRL1 |
> > + NCT6694_CANFD_SETTING_ACTIVE_CTRL2 |
> > + NCT6694_CANFD_SETTING_ACTIVE_NBTP_DBTP;
> > +
> > + ret = nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
> > + if (ret)
> > + return ret;
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + return 0;
> > +}
> > +
> > +static void nct6694_canfd_stop(struct net_device *ndev)
> > +{
> > + struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > + struct nct6694_canfd_setting *setting __free(kfree) = NULL;
> > + const struct nct6694_cmd_header cmd_hd = {
> > + .mod = NCT6694_CANFD_MOD,
> > + .cmd = NCT6694_CANFD_SETTING,
> > + .sel = ndev->dev_port,
> > + .len = cpu_to_le16(sizeof(*setting))
> > + };
> > +
> > + /* The NCT6694 cannot be stopped. To ensure safe operation and avoid
> > + * interference, the control mode is set to Listen-Only mode. This
> > + * mode allows the device to monitor bus activity without actively
> > + * participating in communication.
> > + */
> > + setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> > + if (!setting)
> > + return;
> > +
> > + nct6694_read_msg(priv->nct6694, &cmd_hd, setting);
> > + setting->ctrl1 = cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_MON);
> > + setting->active = NCT6694_CANFD_SETTING_ACTIVE_CTRL1;
> > + nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
> > +}
> > +
> > +static int nct6694_canfd_close(struct net_device *ndev)
> > +{
> > + struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > +
>
> make this inverse to nct6694_canfd_open()
>
Fix it in v11.
> > + netif_stop_queue(ndev);
> > + can_rx_offload_disable(&priv->offload);
> > + nct6694_canfd_stop(ndev);
> > + free_irq(ndev->irq, ndev);
> > + destroy_workqueue(priv->wq);
> > + close_candev(ndev);
> > + return 0;
> > +}
> > +
> > +static int nct6694_canfd_set_mode(struct net_device *ndev, enum can_mode mode)
> > +{
> > + int ret;
> > +
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + ret = nct6694_canfd_start(ndev);
> > + if (ret)
> > + return ret;
> > +
> > + netif_wake_queue(ndev);
> > + break;
> > +
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int nct6694_canfd_open(struct net_device *ndev)
> > +{
> > + struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > + int ret;
> > +
> > + ret = open_candev(ndev);
> > + if (ret)
> > + return ret;
> > +
> > + can_rx_offload_enable(&priv->offload);
> > +
> > + ret = request_threaded_irq(ndev->irq, NULL,
> > + nct6694_canfd_irq, IRQF_ONESHOT,
> > + "nct6694_canfd", ndev);
> > + if (ret) {
> > + netdev_err(ndev, "Failed to request IRQ\n");
> > + goto close_candev;
>
> nitpick: rename to can_rx_offload_disable
>
Fix it in v11.
> > + }
> > +
> > + priv->wq = alloc_ordered_workqueue("%s-nct6694_wq",
> > + WQ_FREEZABLE | WQ_MEM_RECLAIM,
> > + ndev->name);
> > + if (!priv->wq) {
> > + ret = -ENOMEM;
> > + goto free_irq;
> > + }
> > +
> > + ret = nct6694_canfd_start(ndev);
> > + if (ret)
> > + goto destroy_wq;
> > +
> > + netif_start_queue(ndev);
> > +
> > + return 0;
> > +
> > +destroy_wq:
> > + destroy_workqueue(priv->wq);
> > +free_irq:
> > + free_irq(ndev->irq, ndev);
> > +close_candev:
> > + can_rx_offload_disable(&priv->offload);
> > + close_candev(ndev);
> > + return ret;
> > +}
> > +
> > +static const struct net_device_ops nct6694_canfd_netdev_ops = {
> > + .ndo_open = nct6694_canfd_open,
> > + .ndo_stop = nct6694_canfd_close,
> > + .ndo_start_xmit = nct6694_canfd_start_xmit,
> > + .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static const struct ethtool_ops nct6694_canfd_ethtool_ops = {
> > + .get_ts_info = ethtool_op_get_ts_info,
> > +};
> > +
> > +static int nct6694_canfd_get_clock(struct nct6694_canfd_priv *priv)
> > +{
> > + struct nct6694_canfd_information *info __free(kfree) = NULL;
> > + static const struct nct6694_cmd_header cmd_hd = {
> > + .mod = NCT6694_CANFD_MOD,
> > + .cmd = NCT6694_CANFD_INFORMATION,
> > + .sel = NCT6694_CANFD_INFORMATION_SEL,
> > + .len = cpu_to_le16(sizeof(*info))
> > + };
> > + int ret, can_clk;
> > +
> > + info = kzalloc(sizeof(*info), GFP_KERNEL);
> > + if (!info)
> > + return -ENOMEM;
> > +
> > + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, info);
> > + if (ret)
> > + return ret;
> > +
> > + can_clk = le32_to_cpu(info->can_clk);
>
> return le32_to_cpu(info->can_clk);
>
Fix it in v11.
> > +
> > + return can_clk;
> > +}
> > +
> > +static int nct6694_canfd_probe(struct platform_device *pdev)
> > +{
> > + const struct mfd_cell *cell = mfd_get_cell(pdev);
> > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> > + struct nct6694_canfd_priv *priv;
> > + struct net_device *ndev;
> > + int ret, irq, can_clk;
> > +
> > + irq = irq_create_mapping(nct6694->domain,
> > + NCT6694_IRQ_CAN0 + cell->id);
> > + if (!irq)
> > + return irq;
> > +
> > + ndev = alloc_candev(sizeof(struct nct6694_canfd_priv), 1);
> > + if (!ndev) {
> > + ret = -ENOMEM;
> > + goto dispose_irq;
> > + }
> > +
> > + ndev->irq = irq;
> > + ndev->flags |= IFF_ECHO;
> > + ndev->dev_port = cell->id;
> > + ndev->netdev_ops = &nct6694_canfd_netdev_ops;
> > + ndev->ethtool_ops = &nct6694_canfd_ethtool_ops;
> > +
> > + priv = netdev_priv(ndev);
> > + priv->nct6694 = nct6694;
> > + priv->ndev = ndev;
> > +
> > + can_clk = nct6694_canfd_get_clock(priv);
> > + if (can_clk < 0) {
> > + ret = dev_err_probe(&pdev->dev, can_clk,
> > + "Failed to get clock\n");
> > + goto free_candev;
> > + }
> > +
> > + INIT_WORK(&priv->tx_work, nct6694_canfd_tx_work);
> > +
> > + priv->can.clock.freq = can_clk;
> > + priv->can.bittiming_const = &nct6694_canfd_bittiming_nominal_const;
> > + priv->can.data_bittiming_const = &nct6694_canfd_bittiming_data_const;
> > + priv->can.do_set_mode = nct6694_canfd_set_mode;
> > + priv->can.do_get_berr_counter = nct6694_canfd_get_berr_counter;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_BERR_REPORTING |
> > + CAN_CTRLMODE_FD_NON_ISO;
> > +
> > + ret = can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
> > + if (ret)
> > + goto free_candev;
> > +
> > + ret = can_rx_offload_add_manual(ndev, &priv->offload,
> > + NCT6694_NAPI_WEIGHT);
> > + if (ret) {
> > + dev_err_probe(&pdev->dev, ret, "Failed to add rx_offload\n");
> > + goto free_candev;
> > + }
> > +
> > + platform_set_drvdata(pdev, priv);
> > + SET_NETDEV_DEV(priv->ndev, &pdev->dev);
> > +
> > + ret = register_candev(priv->ndev);
> > + if (ret)
> > + goto rx_offload_del;
> > +
> > + return 0;
> > +
> > +rx_offload_del:
> > + can_rx_offload_del(&priv->offload);
> > +free_candev:
> > + free_candev(ndev);
> > +dispose_irq:
> > + irq_dispose_mapping(irq);
> > + return ret;
> > +}
> > +
> > +static void nct6694_canfd_remove(struct platform_device *pdev)
> > +{
> > + struct nct6694_canfd_priv *priv = platform_get_drvdata(pdev);
> > + struct net_device *ndev = priv->ndev;
> > +
> > + unregister_candev(ndev);
> > + irq_dispose_mapping(ndev->irq);
> > + can_rx_offload_del(&priv->offload);
> > + free_candev(ndev);
>
> Make the order inverse to the nct6694_canfd_probe() function.
>
Fix it in v11.
Best regards,
Ming
Powered by blists - more mailing lists