Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames

[Vincent Mailhol <vincent.mailhol@gmail.com>]

On Mon. 13 Aug. 2022 at 00:20, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> Hello Vincent,
>
> On Friday 12 of August 2022 16:35:30 Vincent Mailhol wrote:
> > Hi Pavel,
> >
> > On Tue. 2 Aug. 2022 at 16:38, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> > > Hello Vincent,
> > >
> > > thanks much for review. I am adding some notices to Tx timestamps
> > > after your comments
> > >
> > > On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> > > > I just send a series last week which a significant amount of changes
> > > > for CAN timestamping tree-wide:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > > >comm it/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6
> > > >
> > > > I suggest you have a look at this series and harmonize it with the new
> > > > features (e.g. Hardware TX timestamp).
> > > >
> > > > On Tue. 2 Aug. 2022 at 03:52, Matej Vasilevski
> > >
> > > ...
> > >
> > > > > +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq
> > > > > *ifr) +{
> > > > > +       struct ctucan_priv *priv = netdev_priv(dev);
> > > > > +       struct hwtstamp_config cfg;
> > > > > +
> > > > > +       if (!priv->timestamp_possible)
> > > > > +               return -EOPNOTSUPP;
> > > > > +
> > > > > +       if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> > > > > +               return -EFAULT;
> > > > > +
> > > > > +       if (cfg.flags)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       if (cfg.tx_type != HWTSTAMP_TX_OFF)
> > > > > +               return -ERANGE;
> > > >
> > > > I have a great news: your driver now also support hardware TX
> > > > timestamps:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > > >comm it/?id=8bdd1112edcd3edce2843e03826204a84a61042d
> > > >
> > > > > +
> > > > > +       switch (cfg.rx_filter) {
> > > > > +       case HWTSTAMP_FILTER_NONE:
> > > > > +               priv->timestamp_enabled = false;
> > >
> > > ...
> > >
> > > > > +
> > > > > +       cfg.flags = 0;
> > > > > +       cfg.tx_type = HWTSTAMP_TX_OFF;
> > > >
> > > > Hardware TX timestamps are now supported (c.f. supra).
> > > >
> > > > > +       cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL
> > > > > : HWTSTAMP_FILTER_NONE; +       return copy_to_user(ifr->ifr_data,
> > > > > &cfg, sizeof(cfg)) ? -EFAULT : 0; +}
> > > > > +
> > > > > +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr,
> > > > > int cmd)
> > > >
> > > > Please consider using the generic function can_eth_ioctl_hwts()
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > > >comm it/?id=90f942c5a6d775bad1be33ba214755314105da4a
> > > >
> > > > > +{
> > >
> > > ...
> > >
> > > > > +       info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> > > > > +                                SOF_TIMESTAMPING_RAW_HARDWARE;
> > > > > +       info->tx_types = BIT(HWTSTAMP_TX_OFF);
> > > >
> > > > Hardware TX timestamps are now supported (c.f. supra).
> > > >
> > > > > +       info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > > > > +                          BIT(HWTSTAMP_FILTER_ALL);
> > >
> > > I am not sure if it is good idea to report support for hardware
> > > TX timestamps by all drivers. Precise hardware Tx timestamps
> > > are important for some CAN applications but they require to be
> > > exactly/properly aligned with Rx timestamps.
> > >
> > > Only some CAN (FD) controllers really support that feature.
> > > For M-CAN and some others it is realized as another event
> > > FIFO in addition to Tx and Rx FIFOs.
> > >
> > > For CTU CAN FD, we have decided that we do not complicate design
> > > and driver by separate events channel. We have configurable
> > > and possibly large Rx FIFO depth which is logical to use for
> > > analyzer mode and we can use loopback to receive own messages
> > > timestamped same way as external received ones.
> > >
> > > See 2.14.1 Loopback mode
> > > SETTINGS[ILBP]=1.
> > >
> > > in the datasheet
> > >
> > >   http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/doc/Datasheet.pdf
> > >
> > > There is still missing information which frames are received
> > > locally and from which buffer they are in the Rx message format,
> > > but we plan to add that into VHDL design.
> > >
> > > In such case, we can switch driver mode and release Tx buffers
> > > only after corresponding message is read from Rx FIFO and
> > > fill exact finegrain (10 ns in our current design) timestamps
> > > to the echo skb. The order of received messages will be seen
> > > exactly mathing the wire order for both transmitted and received
> > > messages then. Which I consider as proper solution for the
> > > most applications including CAN bus analyzers.
> > >
> > > So I consider to report HW Tx timestamps for cases where exact,
> > > precise timestamping is not available for loopback messages
> > > as problematic because you cannot distinguish if you talk
> > > with driver and HW with real/precise timestamps support
> > > or only dummy implementation to make some tools happy.
> >
> > Thank you for the explanation. I did not know about the nuance about
> > those different hardware timestamps.
> >
> > So if I understand correctly, your hardware can deliver two types of
> > hardware timestamps:
> >
> >   - The "real" one: fine grained with 10 ns precision when the frame
> > is actually sent
> >
> >   - The "dummy" one: less precise timestamp generated when there is an
> > event on the device’s Rx or Tx FIFO.
> >
> > Is this correct?
> >
> > Are the "real" and the "dummy" timestamps synced on the same quartz?
> >
> > What is the precision of the "dummy" timestamp? If it in the order of
> > magnitude of 10µs? For many use cases, this is enough. 10µs represents
> > roughly a dozen of time quata (more or less depending on the bitrate
> > and its prescaler).
> > Actually, I never saw hardware with a timestamp precision below 1µs
> > (not saying those don't exist, just that I never encountered them).
> >
> > I am not against what you propose. But my suggestion would be rather
> > to report both TX and RX timestamps and just document the precision
> > (i.e. TX has precision with an order of magnitude of 10µs and RX has
> > precision of 10 ns).
> >
> > At the end, I let you decide what works the best for you. Just keep in
> > mind that the micro second precision is already a great achievement
> > and not many people need the 10 nano second (especially for CAN).
> >
> > P.S.: I am on holiday, my answers might be delayed :)
>
> I am leaving off the Internet for next week as well now...
>
> My discussion has been reaction to your information about your
> CTU CAN FD change, but may it be I have lost the track.
>
> > > On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> > > > I have a great news: your driver now also support hardware TX
> > > > timestamps:
>
> Our actual/mainline driver actually does not support neither Rx nor Tx
> timestamps. Matej Vasilevski has prepared and sent to review patches
> adding Rx timestamping (10 ns resolution for our actual designs).
> He has rebased his changes above yours... CTU CAN FD hardware
> supports such timestamping for many years... probably preceding 2.0
> IP core version.
>
> But even when this patch is clean up and accepted into mainline,
> CTU CAN FD driver will not support hardware Tx timestams, may it
> be software ones are implemented in generic CAN echo code, not checked
> now... So if your change add report of HW Tx stamps then it would be
> problem to distinguish situation when we implement hardware Tx timestamps.
>
> The rest of the previous text is how to implement precise Tx timestamps
> on other and our controller design. We do not have separate queue
> to report Tx timestamps for successfully sent frames. But we can
> enable copy of sent Tx frames to Rx FIFO so there is a way how
> to achieve that. But there is still minor design detail that
> we need to mark such frames as echo of local Tx in Rx FIFO queue
> and ideally add there even number of the Tx buffer or even some
> user provided information from some Tx buffer filed to distinguish
> that such frames should be reported through echo and ensure that
> they are not reported to that client who has sent them etc...
> But there are our implementation details...
>
> But what worries me, is your statement that HW Tx timestamps
> are already reported as available on CTU CAN FD by your patch,
> if I understood your reply well.

I read again the full exchange, and you were right from the beginning.
Please forget my comments on the hardware TX timestamps, I just
misread you.


Yours sincerely,
Vincent Mailhol