Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[Marc Kleine-Budde]

Hello,

after noticing that the gs_usb firmware sends timestamps on the TX,
too, I updated the driver and squashed the 2nd patch. Also added
proper endianness handling to gs_usb_get_timestamp(). I allowed to add
myself as Co-developed-by.

regards,
Marc

Changes since v2: https://lore.kernel.org/all/20220827092545.2971240-1-mkl@pengutronix.de
- new patch 1/2: use common spelling of GS_USB in macros
- squashed both old patches into now 2/2
- use GS_USB instead of GSUSB in macros
- gs_usb_get_timestamp(): renamed from gs_usb_get_sof_timestamp()
- gs_usb_get_timestamp(): take care of endianness
- gs_usb_skb_set_timestamp(): renamed from gs_usb_set_timestamp()
- gs_usb_set_timestamp(): add new function to add timestamp to skb
  from struct gs_host_frame
- add support for TX timestamps
- gs_can_eth_ioctl(): return -EOPNOTSUPP if device doesn't support
  GS_CAN_FEATURE_HW_TIMESTAMP
- gs_usb_get_ts_info(): renamed from gs_usb_get_ts_info_hwts()
- gs_usb_get_ts_info(): call can_ethtool_op_get_ts_info_hwts() if
  device supports GS_CAN_FEATURE_HW_TIMESTAMP, call
  ethtool_op_get_ts_info() otherwise

Changes since v1: https://lore.kernel.org/all/20220826104629.2837024-1-mkl@pengutronix.de
- add new includes sorted
- adjust multi line comment style
- don't use 1e6, but 1 * HZ_PER_MHZ, to fix sparse warning
- use __le32 instead of u32 in struct classic_can_ts and canfd_ts
- place _ts in front of _quirk in union in struct gs_host_frame
- gs_usb_get_sof_timestamp(): pass "const struct gs_can *dev" as 1st
  argument, not struct net_device *netdev, simplify return handling
- gs_usb_timestamp_init(), gs_usb_timestamp_stop(),
  gs_usb_get_ts_info_hwts(): make static
- gs_usb_timestamp_init(): increase cc->shift to maximize precision
- fix long lines, remove braces {} for single statement blocks
- gs_can_open(): move check for GS_CAN_FEATURE_HW_TIMESTAMP after all
  ctrlmode checks
- gs_can_open(): move start polling sof timestamp after kfree(dm)
- gs_can_close(): move stop polling sof timestamp to be symmetric with
  respect to gs_can_open()
- gs_usb_probe(): make calculation of dev->hf_size_rx more robost

[PATCH v3 1/2] can: gs_usb: use common spelling of GS_USB in macros

[Marc Kleine-Budde]

There are a few macros in the driver which use GSUSB in the name of
the macro, while the majority uses GS_USB. Convert all macros to
GS_USB.

Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Fixes: b00ca070e022 ("can: gs_usb: activate quirks for CANtact Pro unconditionally")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index baf749c8cda3..532510902f60 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -22,8 +22,8 @@
 #include <linux/can/error.h>
 
 /* Device specific constants */
-#define USB_GSUSB_1_VENDOR_ID 0x1d50
-#define USB_GSUSB_1_PRODUCT_ID 0x606f
+#define USB_GS_USB_1_VENDOR_ID 0x1d50
+#define USB_GS_USB_1_PRODUCT_ID 0x606f
 
 #define USB_CANDLELIGHT_VENDOR_ID 0x1209
 #define USB_CANDLELIGHT_PRODUCT_ID 0x2323
@@ -34,8 +34,8 @@
 #define USB_ABE_CANDEBUGGER_FD_VENDOR_ID 0x16d0
 #define USB_ABE_CANDEBUGGER_FD_PRODUCT_ID 0x10b8
 
-#define GSUSB_ENDPOINT_IN 1
-#define GSUSB_ENDPOINT_OUT 2
+#define GS_USB_ENDPOINT_IN 1
+#define GS_USB_ENDPOINT_OUT 2
 
 /* Device specific constants */
 enum gs_usb_breq {
@@ -491,7 +491,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
  resubmit_urb:
 	usb_fill_bulk_urb(urb, usbcan->udev,
-			  usb_rcvbulkpipe(usbcan->udev, GSUSB_ENDPOINT_IN),
+			  usb_rcvbulkpipe(usbcan->udev, GS_USB_ENDPOINT_IN),
 			  hf, dev->parent->hf_size_rx,
 			  gs_usb_receive_bulk_callback, usbcan);
 
@@ -659,7 +659,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 	}
 
 	usb_fill_bulk_urb(urb, dev->udev,
-			  usb_sndbulkpipe(dev->udev, GSUSB_ENDPOINT_OUT),
+			  usb_sndbulkpipe(dev->udev, GS_USB_ENDPOINT_OUT),
 			  hf, dev->hf_size_tx,
 			  gs_usb_xmit_callback, txc);
 
@@ -769,7 +769,7 @@ static int gs_can_open(struct net_device *netdev)
 			usb_fill_bulk_urb(urb,
 					  dev->udev,
 					  usb_rcvbulkpipe(dev->udev,
-							  GSUSB_ENDPOINT_IN),
+							  GS_USB_ENDPOINT_IN),
 					  buf,
 					  dev->parent->hf_size_rx,
 					  gs_usb_receive_bulk_callback, parent);
@@ -1063,8 +1063,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	 * GS_CAN_FEATURE_QUIRK_BREQ_CANTACT_PRO to workaround this
 	 * issue.
 	 */
-	if (dev->udev->descriptor.idVendor == cpu_to_le16(USB_GSUSB_1_VENDOR_ID) &&
-	    dev->udev->descriptor.idProduct == cpu_to_le16(USB_GSUSB_1_PRODUCT_ID) &&
+	if (dev->udev->descriptor.idVendor == cpu_to_le16(USB_GS_USB_1_VENDOR_ID) &&
+	    dev->udev->descriptor.idProduct == cpu_to_le16(USB_GS_USB_1_PRODUCT_ID) &&
 	    dev->udev->manufacturer && dev->udev->product &&
 	    !strcmp(dev->udev->manufacturer, "LinkLayer Labs") &&
 	    !strcmp(dev->udev->product, "CANtact Pro") &&
@@ -1258,8 +1258,8 @@ static void gs_usb_disconnect(struct usb_interface *intf)
 }
 
 static const struct usb_device_id gs_usb_table[] = {
-	{ USB_DEVICE_INTERFACE_NUMBER(USB_GSUSB_1_VENDOR_ID,
-				      USB_GSUSB_1_PRODUCT_ID, 0) },
+	{ USB_DEVICE_INTERFACE_NUMBER(USB_GS_USB_1_VENDOR_ID,
+				      USB_GS_USB_1_PRODUCT_ID, 0) },
 	{ USB_DEVICE_INTERFACE_NUMBER(USB_CANDLELIGHT_VENDOR_ID,
 				      USB_CANDLELIGHT_PRODUCT_ID, 0) },
 	{ USB_DEVICE_INTERFACE_NUMBER(USB_CES_CANEXT_FD_VENDOR_ID,
-- 
2.35.1

[PATCH v3 2/2] can: gs_usb: add RX and TX hardware timestamp support

[Marc Kleine-Budde]

From: John Whittington <git@jbrengineering.co.uk>

Add support for hardware timestamps, if the firmware includes it as a
feature via the GS_CAN_FEATURE_HW_TIMESTAMP flag. Check for this
feature during probe, extend the RX expected length if it is and
enable it during open.

The struct classic_can_ts and struct canfd_ts are extended to include
the µs timestamp following data as defined in the firmware. The
timestamp is then captured and set using skb_hwtstamps() on each RX
and TX.

The frame µs timestamp is provided from a 32 bit 1 MHz timer which
rolls over every 4294 seconds, so a cyclecounter, timecounter, and
delayed worker are used to convert the timer into a proper ns
timestamp - same implementation as commit efd8d98dfb900 ("can:
mcp251xfd: add HW timestamp infrastructure").

Hardware timestamps are added to capabilities as commit
b1f6b93e678f ("can: mcp251xfd: advertise timestamping capabilities and
add ioctl support").

Signed-off-by: John Whittington <git@jbrengineering.co.uk>
Link: https://github.com/candle-usb/candleLight_fw/issues/100
Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 193 +++++++++++++++++++++++++++++++++--
 1 file changed, 186 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 532510902f60..cc363f1935ce 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -10,12 +10,16 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/clocksource.h>
 #include <linux/ethtool.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/signal.h>
+#include <linux/timecounter.h>
+#include <linux/units.h>
 #include <linux/usb.h>
+#include <linux/workqueue.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -37,6 +41,14 @@
 #define GS_USB_ENDPOINT_IN 1
 #define GS_USB_ENDPOINT_OUT 2
 
+/* Timestamp 32 bit timer runs at 1 MHz (1 µs tick). Worker accounts
+ * for timer overflow (will be after ~71 minutes)
+ */
+#define GS_USB_TIMESTAMP_TIMER_HZ (1 * HZ_PER_MHZ)
+#define GS_USB_TIMESTAMP_WORK_DELAY_SEC 1800
+static_assert(GS_USB_TIMESTAMP_WORK_DELAY_SEC <
+	      CYCLECOUNTER_MASK(32) / GS_USB_TIMESTAMP_TIMER_HZ / 2);
+
 /* Device specific constants */
 enum gs_usb_breq {
 	GS_USB_BREQ_HOST_FORMAT = 0,
@@ -199,6 +211,11 @@ struct classic_can {
 	u8 data[8];
 } __packed;
 
+struct classic_can_ts {
+	u8 data[8];
+	__le32 timestamp_us;
+} __packed;
+
 struct classic_can_quirk {
 	u8 data[8];
 	u8 quirk;
@@ -208,6 +225,11 @@ struct canfd {
 	u8 data[64];
 } __packed;
 
+struct canfd_ts {
+	u8 data[64];
+	__le32 timestamp_us;
+} __packed;
+
 struct canfd_quirk {
 	u8 data[64];
 	u8 quirk;
@@ -224,8 +246,10 @@ struct gs_host_frame {
 
 	union {
 		DECLARE_FLEX_ARRAY(struct classic_can, classic_can);
+		DECLARE_FLEX_ARRAY(struct classic_can_ts, classic_can_ts);
 		DECLARE_FLEX_ARRAY(struct classic_can_quirk, classic_can_quirk);
 		DECLARE_FLEX_ARRAY(struct canfd, canfd);
+		DECLARE_FLEX_ARRAY(struct canfd_ts, canfd_ts);
 		DECLARE_FLEX_ARRAY(struct canfd_quirk, canfd_quirk);
 	};
 } __packed;
@@ -259,6 +283,11 @@ struct gs_can {
 	struct can_bittiming_const bt_const, data_bt_const;
 	unsigned int channel;	/* channel number */
 
+	/* time counter for hardware timestamps */
+	struct cyclecounter cc;
+	struct timecounter tc;
+	struct delayed_work timestamp;
+
 	u32 feature;
 	unsigned int hf_size_tx;
 
@@ -351,6 +380,87 @@ static int gs_cmd_reset(struct gs_can *gsdev)
 	return rc;
 }
 
+static inline int gs_usb_get_timestamp(const struct gs_can *dev,
+				       u32 *timestamp_p)
+{
+	__le32 timestamp;
+	int rc;
+
+	rc = usb_control_msg_recv(interface_to_usbdev(dev->iface),
+				  usb_sndctrlpipe(interface_to_usbdev(dev->iface), 0),
+				  GS_USB_BREQ_TIMESTAMP,
+				  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+				  dev->channel, 0,
+				  &timestamp, sizeof(timestamp),
+				  USB_CTRL_GET_TIMEOUT,
+				  GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	*timestamp_p = le32_to_cpu(timestamp);
+
+	return 0;
+}
+
+static u64 gs_usb_timestamp_read(const struct cyclecounter *cc)
+{
+	const struct gs_can *dev;
+	u32 timestamp = 0;
+	int err;
+
+	dev = container_of(cc, struct gs_can, cc);
+	err = gs_usb_get_timestamp(dev, &timestamp);
+	if (err)
+		netdev_err(dev->netdev,
+			   "Error %d while reading timestamp. HW timestamps may be inaccurate.",
+			   err);
+
+	return timestamp;
+}
+
+static void gs_usb_timestamp_work(struct work_struct *work)
+{
+	struct delayed_work *delayed_work = to_delayed_work(work);
+	struct gs_can *dev;
+
+	dev = container_of(delayed_work, struct gs_can, timestamp);
+	timecounter_read(&dev->tc);
+
+	schedule_delayed_work(&dev->timestamp,
+			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
+}
+
+static void gs_usb_skb_set_timestamp(const struct gs_can *dev,
+				     struct sk_buff *skb, u32 timestamp)
+{
+	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+	u64 ns;
+
+	ns = timecounter_cyc2time(&dev->tc, timestamp);
+	hwtstamps->hwtstamp = ns_to_ktime(ns);
+}
+
+static void gs_usb_timestamp_init(struct gs_can *dev)
+{
+	struct cyclecounter *cc = &dev->cc;
+
+	cc->read = gs_usb_timestamp_read;
+	cc->mask = CYCLECOUNTER_MASK(32);
+	cc->shift = 32 - bits_per(NSEC_PER_SEC / GS_USB_TIMESTAMP_TIMER_HZ);
+	cc->mult = clocksource_hz2mult(GS_USB_TIMESTAMP_TIMER_HZ, cc->shift);
+
+	timecounter_init(&dev->tc, &dev->cc, ktime_get_real_ns());
+
+	INIT_DELAYED_WORK(&dev->timestamp, gs_usb_timestamp_work);
+	schedule_delayed_work(&dev->timestamp,
+			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
+}
+
+static void gs_usb_timestamp_stop(struct gs_can *dev)
+{
+	cancel_delayed_work_sync(&dev->timestamp);
+}
+
 static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
 {
 	struct can_device_stats *can_stats = &dev->can.can_stats;
@@ -376,6 +486,24 @@ static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
 	}
 }
 
+static void gs_usb_set_timestamp(const struct gs_can *dev, struct sk_buff *skb,
+				 const struct gs_host_frame *hf)
+{
+	u32 timestamp;
+
+	if (!(dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP))
+		return;
+
+	if (hf->flags & GS_CAN_FLAG_FD)
+		timestamp = le32_to_cpu(hf->canfd_ts->timestamp_us);
+	else
+		timestamp = le32_to_cpu(hf->classic_can_ts->timestamp_us);
+
+	gs_usb_skb_set_timestamp(dev, skb, timestamp);
+
+	return;
+}
+
 static void gs_usb_receive_bulk_callback(struct urb *urb)
 {
 	struct gs_usb *usbcan = urb->context;
@@ -443,6 +571,8 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 				gs_update_state(dev, cf);
 		}
 
+		gs_usb_set_timestamp(dev, skb, hf);
+
 		netdev->stats.rx_packets++;
 		netdev->stats.rx_bytes += hf->can_dlc;
 
@@ -465,6 +595,9 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			goto resubmit_urb;
 		}
 
+		skb = dev->can.echo_skb[hf->echo_id];
+		gs_usb_set_timestamp(dev, skb, hf);
+
 		netdev->stats.tx_packets++;
 		netdev->stats.tx_bytes += can_get_echo_skb(netdev, hf->echo_id,
 							   NULL);
@@ -823,6 +956,10 @@ static int gs_can_open(struct net_device *netdev)
 	if (ctrlmode & CAN_CTRLMODE_3_SAMPLES)
 		flags |= GS_CAN_MODE_TRIPLE_SAMPLE;
 
+	/* if hardware supports timestamps, enable it */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		flags |= GS_CAN_MODE_HW_TIMESTAMP;
+
 	/* finally start device */
 	dm->mode = cpu_to_le32(GS_CAN_MODE_START);
 	dm->flags = cpu_to_le32(flags);
@@ -840,6 +977,10 @@ static int gs_can_open(struct net_device *netdev)
 
 	kfree(dm);
 
+	/* start polling timestamp */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		gs_usb_timestamp_init(dev);
+
 	dev->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	parent->active_channels++;
@@ -858,6 +999,10 @@ static int gs_can_close(struct net_device *netdev)
 
 	netif_stop_queue(netdev);
 
+	/* stop polling timestamp */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		gs_usb_timestamp_stop(dev);
+
 	/* Stop polling */
 	parent->active_channels--;
 	if (!parent->active_channels) {
@@ -890,11 +1035,22 @@ static int gs_can_close(struct net_device *netdev)
 	return 0;
 }
 
+static int gs_can_eth_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
+{
+	const struct gs_can *dev = netdev_priv(netdev);
+
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		return can_eth_ioctl_hwts(netdev, ifr, cmd);
+
+	return -EOPNOTSUPP;
+}
+
 static const struct net_device_ops gs_usb_netdev_ops = {
 	.ndo_open = gs_can_open,
 	.ndo_stop = gs_can_close,
 	.ndo_start_xmit = gs_can_start_xmit,
 	.ndo_change_mtu = can_change_mtu,
+	.ndo_eth_ioctl = gs_can_eth_ioctl,
 };
 
 static int gs_usb_set_identify(struct net_device *netdev, bool do_identify)
@@ -944,9 +1100,21 @@ static int gs_usb_set_phys_id(struct net_device *dev,
 	return rc;
 }
 
+static int gs_usb_get_ts_info(struct net_device *netdev,
+			      struct ethtool_ts_info *info)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+
+	/* report if device supports HW timestamps */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		return can_ethtool_op_get_ts_info_hwts(netdev, info);
+
+	return ethtool_op_get_ts_info(netdev, info);
+}
+
 static const struct ethtool_ops gs_usb_ethtool_ops = {
 	.set_phys_id = gs_usb_set_phys_id,
-	.get_ts_info = ethtool_op_get_ts_info,
+	.get_ts_info = gs_usb_get_ts_info,
 };
 
 static struct gs_can *gs_make_candev(unsigned int channel,
@@ -1202,15 +1370,13 @@ static int gs_usb_probe(struct usb_interface *intf,
 	}
 
 	init_usb_anchor(&dev->rx_submitted);
-	/* default to classic CAN, switch to CAN-FD if at least one of
-	 * our channels support CAN-FD.
-	 */
-	dev->hf_size_rx = struct_size(hf, classic_can, 1);
 
 	usb_set_intfdata(intf, dev);
 	dev->udev = udev;
 
 	for (i = 0; i < icount; i++) {
+		unsigned int hf_size_rx = 0;
+
 		dev->canch[i] = gs_make_candev(i, intf, dconf);
 		if (IS_ERR_OR_NULL(dev->canch[i])) {
 			/* save error code to return later */
@@ -1228,8 +1394,21 @@ static int gs_usb_probe(struct usb_interface *intf,
 		}
 		dev->canch[i]->parent = dev;
 
-		if (dev->canch[i]->can.ctrlmode_supported & CAN_CTRLMODE_FD)
-			dev->hf_size_rx = struct_size(hf, canfd, 1);
+		/* set RX packet size based on FD and if hardware
+                * timestamps are supported.
+		*/
+		if (dev->canch[i]->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
+			if (dev->canch[i]->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+				hf_size_rx = struct_size(hf, canfd_ts, 1);
+			else
+				hf_size_rx = struct_size(hf, canfd, 1);
+		} else {
+			if (dev->canch[i]->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+				hf_size_rx = struct_size(hf, classic_can_ts, 1);
+			else
+				hf_size_rx = struct_size(hf, classic_can, 1);
+		}
+		dev->hf_size_rx = max(dev->hf_size_rx, hf_size_rx);
 	}
 
 	kfree(dconf);
-- 
2.35.1

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

Adding Peter and Christoph on Cc.

On 28.08.2022 00:15:46, Marc Kleine-Budde wrote:
> after noticing that the gs_usb firmware sends timestamps on the TX,
> too, I updated the driver and squashed the 2nd patch. Also added
> proper endianness handling to gs_usb_get_timestamp(). I allowed to add
> myself as Co-developed-by.

Peter, Christoph, does the CES CANext FD support timestamps? Are you
planing to add TS support?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[john]

Hi Marc,

Thanks for the support on this. I was following the threads but away so unable to make changes. With the patch now squashed and your updates, it looks like no further input is required from me on this.

Kind regards,
John.


------- Original Message -------
On Saturday, August 27th, 2022 at 23:15, Marc Kleine-Budde <mkl@pengutronix.de> wrote:


>
>
> Hello,
>
> after noticing that the gs_usb firmware sends timestamps on the TX,
> too, I updated the driver and squashed the 2nd patch. Also added
> proper endianness handling to gs_usb_get_timestamp(). I allowed to add
> myself as Co-developed-by.
>
> regards,
> Marc
>
> Changes since v2: https://lore.kernel.org/all/20220827092545.2971240-1-mkl@pengutronix.de
> - new patch 1/2: use common spelling of GS_USB in macros
> - squashed both old patches into now 2/2
> - use GS_USB instead of GSUSB in macros
> - gs_usb_get_timestamp(): renamed from gs_usb_get_sof_timestamp()
> - gs_usb_get_timestamp(): take care of endianness
> - gs_usb_skb_set_timestamp(): renamed from gs_usb_set_timestamp()
> - gs_usb_set_timestamp(): add new function to add timestamp to skb
> from struct gs_host_frame
> - add support for TX timestamps
> - gs_can_eth_ioctl(): return -EOPNOTSUPP if device doesn't support
> GS_CAN_FEATURE_HW_TIMESTAMP
> - gs_usb_get_ts_info(): renamed from gs_usb_get_ts_info_hwts()
> - gs_usb_get_ts_info(): call can_ethtool_op_get_ts_info_hwts() if
> device supports GS_CAN_FEATURE_HW_TIMESTAMP, call
> ethtool_op_get_ts_info() otherwise
>
> Changes since v1: https://lore.kernel.org/all/20220826104629.2837024-1-mkl@pengutronix.de
> - add new includes sorted
> - adjust multi line comment style
> - don't use 1e6, but 1 * HZ_PER_MHZ, to fix sparse warning
> - use __le32 instead of u32 in struct classic_can_ts and canfd_ts
> - place _ts in front of _quirk in union in struct gs_host_frame
> - gs_usb_get_sof_timestamp(): pass "const struct gs_can *dev" as 1st
> argument, not struct net_device *netdev, simplify return handling
> - gs_usb_timestamp_init(), gs_usb_timestamp_stop(),
> gs_usb_get_ts_info_hwts(): make static
> - gs_usb_timestamp_init(): increase cc->shift to maximize precision
>
> - fix long lines, remove braces {} for single statement blocks
> - gs_can_open(): move check for GS_CAN_FEATURE_HW_TIMESTAMP after all
> ctrlmode checks
> - gs_can_open(): move start polling sof timestamp after kfree(dm)
> - gs_can_close(): move stop polling sof timestamp to be symmetric with
> respect to gs_can_open()
> - gs_usb_probe(): make calculation of dev->hf_size_rx more robost
>
>

AW: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[Fink, Peter]

Hi Marc,

currently CANext FD does not support timestamps.
We also have no active plans on adding timestamp support, but I wouldn't rule it out completely that this might come up at some point and we will add it.

Best regards,
Peter

> -----Ursprüngliche Nachricht-----
> Betreff: Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support
>
> Adding Peter and Christoph on Cc.
>
> On 28.08.2022 00:15:46, Marc Kleine-Budde wrote:
> > after noticing that the gs_usb firmware sends timestamps on the TX,
> > too, I updated the driver and squashed the 2nd patch. Also added
> > proper endianness handling to gs_usb_get_timestamp(). I allowed to add
> > myself as Co-developed-by.
>
> Peter, Christoph, does the CES CANext FD support timestamps? Are you
> planing to add TS support?
>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Christ Electronic Systems GmbH
http://www.christ-es.de/

Geschäftsführer: Franz Reichle
Hauptsitz: Alpenstraße 34, 87700 Memmingen
Amtsgericht Memmingen HRB 9102

Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail ist nicht gestattet.
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure or distribution of the material in this e-mail is strictly forbidden.

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On 31.08.2022 09:16:10, Fink, Peter wrote:
> Hi Marc,
> 
> currently CANext FD does not support timestamps. We also have no
> active plans on adding timestamp support, but I wouldn't rule it out
> completely that this might come up at some point and we will add it.

Thanks for the feedback,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On 29.08.2022 13:25:00, john@jbrengineering.co.uk wrote:
> Thanks for the support on this. I was following the threads but away
> so unable to make changes. With the patch now squashed and your
> updates, it looks like no further input is required from me on this.

Can you test the v3 patch?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[john]

Yes, I have tested the v3 patch with candleLight_fw (c19f3a). `candump -H` works
as intended and I have actually used it in anger so to speak, debugging a timing
tolerance issue with a CAN node.

I've also confirmed that `ethtool --show-time-stamping can0` returns the correct
timestamping info for both devices which do and don't include the
`GS_CAN_FEATURE_HW_TIMESTAMP` flag. Also that devices without that flag continue
to operate as before with the SocketCAN tools.

John.

------- Original Message -------
On Monday, September 5th, 2022 at 18:13, Marc Kleine-Budde <mkl@pengutronix.de> wrote:


> 
> 
> On 29.08.2022 13:25:00, john@jbrengineering.co.uk wrote:
> 
> > Thanks for the support on this. I was following the threads but away
> > so unable to make changes. With the patch now squashed and your
> > updates, it looks like no further input is required from me on this.
> 
> 
> Can you test the v3 patch?
> 
> Marc
> 
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On 07.09.2022 07:13:02, john@jbrengineering.co.uk wrote:
> Yes, I have tested the v3 patch with candleLight_fw (c19f3a). `candump -H` works
> as intended and I have actually used it in anger so to speak, debugging a timing
> tolerance issue with a CAN node.

\o/ Have you found your issue?
 
> I've also confirmed that `ethtool --show-time-stamping can0` returns the correct
> timestamping info for both devices which do and don't include the
> `GS_CAN_FEATURE_HW_TIMESTAMP` flag. Also that devices without that flag continue
> to operate as before with the SocketCAN tools.

Thanks for testing.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/2] can: gs_usb: hardware timestamp support

[john]

On Wednesday, September 7th, 2022 at 11:04, Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> \o/ Have you found your issue?

Yes - a cyclical message period was outside of tolerance. First using hardware timestamps was important to rule out any delay from host machine capturing the messages and confirm the device was indeed deviating. Then once the device was patched, confirming it was within spec required hardware timestamps too. I did find that host software timestamps would at times introduce some delays not present on the bus (running in a VM with other tasks so understandable and they looked more like system delays than the periodic ones created by un-patched device). The end result though was a fixed device and happy outcome!

John.