Message-ID: <CAMZ6Rq+EehdX8Kkg_430tzbE072Fm0PXbzgSqBzeDygTZqzBLA@mail.gmail.com>
Date:   Fri, 12 Aug 2022 23:35:30 +0900
From:   Vincent Mailhol <vincent.mailhol@...il.com>
To:     Pavel Pisa <pisa@....felk.cvut.cz>
Cc:     Matej Vasilevski <matej.vasilevski@...nam.cz>,
        Ondrej Ille <ondrej.ille@...il.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        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>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, Jiri Novak <jnovak@....cvut.cz>,
        Oliver Hartkopp <socketcan@...tkopp.net>
Subject: Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error
 CAN frames

Hi Pavel,

On Tue. 2 Aug. 2022 at 16:38, Pavel Pisa <pisa@....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 :)


Yours sincerely,
Vincent Mailhol

Powered by blists - more mailing lists