Date:   Fri, 12 Aug 2022 17:19:58 +0200
From:   Pavel Pisa <pisa@....felk.cvut.cz>
To:     Vincent Mailhol <vincent.mailhol@...il.com>,
        Matej Vasilevski <matej.vasilevski@...nam.cz>,
        Pavel Hronek <hronepa1@....cvut.cz>
Cc:     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

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

Best wishes,

                Pavel
-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@....felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Powered by blists - more mailing lists