Message-Id: <202208020937.54675.pisa@cmp.felk.cvut.cz>
Date:   Tue, 2 Aug 2022 09:37:54 +0200
From:   Pavel Pisa <pisa@....felk.cvut.cz>
To:     Vincent Mailhol <vincent.mailhol@...il.com>
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

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.

 
Best wishes and thanks for consideration about altrenatives,

                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