netdev - Re: [PATCH v9 4/7] can: Add Nuvoton NCT6694 CANFD support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250410-artichoke-swan-of-abracadabra-33c1dc-mkl@pengutronix.de>
Date: Thu, 10 Apr 2025 08:04:26 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Ming Yu <a0282524688@...il.com>
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 v9 4/7] can: Add Nuvoton NCT6694 CANFD support

On 10.04.2025 10:40:34, Ming Yu wrote:
> Dear Marc,
> 
> Thank you for reviewing.
> 
> Marc Kleine-Budde <mkl@...gutronix.de> 於 2025年4月9日 週三 下午6:21寫道：
> ...
> > > +static void nct6694_canfd_handle_state_change(struct net_device *ndev, u8 status)
> > > +{
> > > +     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
> > > +     enum can_state new_state, rx_state, tx_state;
> > > +     struct can_berr_counter bec;
> > > +     struct can_frame *cf;
> > > +     struct sk_buff *skb;
> > > +
> > > +     nct6694_canfd_get_berr_counter(ndev, &bec);
> > > +     can_state_get_by_berr_counter(ndev, &bec, &tx_state, &rx_state);
> > > +
> > > +     new_state = max(tx_state, rx_state);
> > > +
> > > +     /* state hasn't changed */
> > > +     if (new_state == priv->can.state)
> > > +             return;
> > > +
> > > +     skb = alloc_can_err_skb(ndev, &cf);
> > > +
> > > +     can_change_state(ndev, cf, tx_state, rx_state);
> > > +
> > > +     if (new_state == CAN_STATE_BUS_OFF) {
> > > +             can_bus_off(ndev);
> >
> > What does your device do when it goes into bus off? Does it recover itself?
> >
> 
> No, the device does not support automatic bus-off recovery. It
> requires an explicit CAN Setting and Initialization(CMD0) command to
> re-initialize the controller after entering bus-off state.

Ok

[...]

> > > +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;
> > > +
> > > +     setting->nbr = cpu_to_le32(n_bt->bitrate);
> > > +     setting->dbr = cpu_to_le32(d_bt->bitrate);
> > > +
> > > +     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);
> > > +
> > > +     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));
> >
> > What does your device do, if you set the bitrates _and_ the bit timing
> > parameters? They are redundant.
> 
> The firmware calculates the default bit timing parameters when it
> receives the bitrates, and then overwrites them if it later receives
> explicit bit timing parameters.
> 
> To avoid confusion and ensure consistent behavior, I will remove the
> bitrate setting logic in next patch. Instead, the bit timing will be
> determined solely based on the provided bit timing parameters.

Sounds good.

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-9   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)