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

[Vincent MAILHOL]

On Wed. 16 Oct. 2024 at 14:05, Vincent MAILHOL
<mailhol.vincent@xxxxxxxxxx> wrote:
> On Tue. 15 Oct. 2024 at 18:33, Hal Feng <hal.feng@xxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> wrote:
> > >> From: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> > >>
> > >> Add driver for CAST CAN Bus Controller used on
> > >> StarFive JH7110 SoC.
> > >>
> > >> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> > >> Co-developed-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> > >> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> > >> ---

(...)

> > >> +
> > >> +       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.


Yours sincerely,
Vincent Mailhol