netdev - RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

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>] [day] [month] [year] [list]
Message-ID:
 <ZQ2PR01MB130747EE09D3CF9B2290B08FE64F2@ZQ2PR01MB1307.CHNPR01.prod.partner.outlook.cn>
Date: Fri, 25 Oct 2024 02:49:10 +0000
From: Hal Feng <hal.feng@...rfivetech.com>
To: Vincent MAILHOL <mailhol.vincent@...adoo.fr>, Hal Feng
	<hal.feng@...ux.starfivetech.com>
CC: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Marc Kleine-Budde <mkl@...gutronix.de>,
	"David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Philipp
 Zabel <p.zabel@...gutronix.de>, Palmer Dabbelt <palmer@...belt.com>, Paul
 Walmsley <paul.walmsley@...ive.com>, Albert Ou <aou@...s.berkeley.edu>, Emil
 Renner Berthing <emil.renner.berthing@...onical.com>, William Qiu
	<william.qiu@...rfivetech.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-can@...r.kernel.org"
	<linux-can@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-riscv@...ts.infradead.org"
	<linux-riscv@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

> On 16.10.24 22:16, Vincent MAILHOL wrote:
> On Wed. 16 Oct. 2024 at 14:05, Vincent MAILHOL
> <mailhol.vincent@...adoo.fr> wrote:
> > On Tue. 15 Oct. 2024 at 18:33, Hal Feng <hal.feng@...ux.starfivetech.com>
> wrote:
> > > On 9/23/2024 11:41 AM, Vincent MAILHOL wrote:
> > > > Hi Hal,
> > > >
> > > > A few more comments on top of what Andrew already wrote.
> > > >
> > > > On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng@...rfivetech.com>
> wrote:
> > > >> From: William Qiu <william.qiu@...rfivetech.com>
> > > >>
> > > >> Add driver for CAST CAN Bus Controller used on StarFive JH7110
> > > >> SoC.
> > > >>
> > > >> Signed-off-by: William Qiu <william.qiu@...rfivetech.com>
> > > >> Co-developed-by: Hal Feng <hal.feng@...rfivetech.com>
> > > >> Signed-off-by: Hal Feng <hal.feng@...rfivetech.com>
> > > >> ---
> 
> (...)
> 
> > > >> +
> > > >> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> > > >> +               priv->can.ctrlmode_supported =
> CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
> > > >> +               priv->can.data_bittiming_const =
> &ccan_data_bittiming_const_canfd;
> > > >> +       } else {
> > > >> +               priv->can.ctrlmode_supported =
> CAN_CTRLMODE_LOOPBACK;
> > > >> +       }
> > > >
> > > > Nitpick, consider doing this:
> > > >
> > > >   priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> > > >   if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> > > >           priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
> > > >           priv->can.data_bittiming_const =
> &ccan_data_bittiming_const_canfd;
> > > >   }
> > >
> > > OK.
> > >
> > > >
> > > > Also, does you hardware support dlc greater than 8 (c.f.
> > > > CAN_CTRLMODE_CC_LEN8_DLC)?
> > >
> > > The class CAN (CC) mode does not support, but the CAN FD mode
> supports.
> >
> > So, CAN_CTRLMODE_CC_LEN8_DLC is a Classical CAN feature. Strictly
> > speaking, this does not exist in CAN FD. Do you mean that only the
> > CAST_CAN_TYPE_CANFD supports sending Classical CAN frames with a DLC
> > greater than 8?
> >
> > If none of the Classical CAN or CAN FD variants of your device is able
> > to send Classical CAN frames with a DLC greater than 8, then this is
> > just not supported by your device.
> >
> > Could you share the datasheet so that I can double check this?
> 
> I received the datasheet from a good samaritan. With this, I was able to
> confirm a few things.
> 
> 1/ Your device can support CAN_CTRLMODE_CC_LEN8_DLC:
> 
> This is shown in the datasheet at:
> 
>   Table 3-52 Definition of the DLC (according to the CAN 2.0 / FD specification)
> 
> DLC values 9 to 15 (binary 1001 to 1111) are accepted by the device.
> When sending and receiving such frames, can_frame->len is set to 8 and
> can_frame->len8_dlc is set to the actual DLC value. Use the
> can_cc_dlc2len() and can_get_cc_dlc() helpers for this.
> 
> 
> 2/ Your device can support CAN_CTRLMODE_TDC_AUTO:
> 
> This is documented in the datasheet at:
> 
>   8.8 TDC and RDC
> 
> This will allow the use of higher bitrates (e.g. 4 Mbits/s) in CAN-FD.
> You can refer to this commit for an example of how to implement it:
> 
>   https://git.kernel.org/torvalds/c/1010a8fa9608
> 
> 
> 3/ Your device can support CAN_CTRLMODE_3_SAMPLES:
> 
> This is called triple mode redundancy (TMR) in your datasheet.
> 
> 
> 4/ Your device can support CAN_CTRLMODE_LISTENONLY:
> 
> This is documented in the datasheet at:
> 
>   3.9.10.2. Listen Only Mode (LOM)
> 
> 
> 5/ Your device can support CAN_CTRLMODE_ONE_SHOT:
> 
> This is documented in the datasheet at:
> 
>   6.5.3 Single Shot Transmit Trigger
> 
> 
> 6/ Your device can support CAN_CTRLMODE_BERR_REPORTING:
> 
> This is shown in the datasheet at:
> 
>   Table 3-24 Error Counter Registers RECNT (0xb2) and TECNT (0xb3)
> 
> 
> 7/ Your device can support CAN_CTRLMODE_PRESUME_ACK:
> 
> c.f. the SACK (self acknowledge) register
> 
> 
> So your device comes with MANY features. I would like to see those
> implemented in your driver. Most of the time, adding a feature just means
> writing one value to a register.
> 
> Please let me know if any of this is unclear.

I will confirm the above features with my colleagues. If these features can
really be supported, let's implement them. Thanks.

Best regards,
Hal