On Mon. 13 Aug. 2022 at 00:20, Pavel Pisa <pisa@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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
