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

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

[Marc Kleine-Budde]

Hello,

I'm posting John's patches from github [1] here for an initial review.

regards,
Marc

[1] https://github.com/candle-usb/candleLight_fw/issues/100

[PATCH 1/2] can: gs_usb: hardware timestamp support

[Marc Kleine-Budde]

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

Adds support for hardware timestamps if the firmware includes it as a
feature via the GS_CAN_FEATURE_HW_TIMESTAMP flag. Checks for this
feature during probe, extends the RX expected length if it is and
enables it during open.

`classic_can_ts` and `canfd_ts` structs are copied and extended to
include the us timestamp following data as defined in the firmware. The
timestamp is then captured and set using `skb_hwtstamps` on each RX.

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

Hardware timestamps are added to capabilities as b1f6b93e678fb ("can:
mcp251xfd: advertise timestamping capabilities")

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

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index baf749c8cda3..0d6729f12946 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -16,6 +16,9 @@
 #include <linux/netdevice.h>
 #include <linux/signal.h>
 #include <linux/usb.h>
+#include <linux/timecounter.h>
+#include <linux/workqueue.h>
+#include <linux/clocksource.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -37,6 +40,13 @@
 #define GSUSB_ENDPOINT_IN 1
 #define GSUSB_ENDPOINT_OUT 2
 
+/* Timestamp 32 bit timer runs at 1 MHz (1 us tick). Worker accounts for timer
+ * overflow (will be after ~71 minutes) */
+#define GSUSB_TIMESTAMP_TIMER_HZ 1e6
+#define GSUSB_TIMESTAMP_WORK_DELAY_SEC 1800
+static_assert(GSUSB_TIMESTAMP_WORK_DELAY_SEC <
+	      CYCLECOUNTER_MASK(32) / GSUSB_TIMESTAMP_TIMER_HZ / 2);
+
 /* Device specific constants */
 enum gs_usb_breq {
 	GS_USB_BREQ_HOST_FORMAT = 0,
@@ -199,6 +209,11 @@ struct classic_can {
 	u8 data[8];
 } __packed;
 
+struct classic_can_ts {
+	u8 data[8];
+	u32 timestamp_us;
+} __packed;
+
 struct classic_can_quirk {
 	u8 data[8];
 	u8 quirk;
@@ -208,6 +223,11 @@ struct canfd {
 	u8 data[64];
 } __packed;
 
+struct canfd_ts {
+	u8 data[64];
+	u32 timestamp_us;
+} __packed;
+
 struct canfd_quirk {
 	u8 data[64];
 	u8 quirk;
@@ -225,7 +245,9 @@ struct gs_host_frame {
 	union {
 		DECLARE_FLEX_ARRAY(struct classic_can, classic_can);
 		DECLARE_FLEX_ARRAY(struct classic_can_quirk, classic_can_quirk);
+		DECLARE_FLEX_ARRAY(struct classic_can_ts, classic_can_ts);
 		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 +281,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 +378,83 @@ static int gs_cmd_reset(struct gs_can *gsdev)
 	return rc;
 }
 
+static int gs_usb_get_sof_timestamp(struct net_device *netdev,
+				    u32 *sof_timestamp_us)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+	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,
+			     sof_timestamp_us, sizeof(*sof_timestamp_us),
+			     USB_CTRL_GET_TIMEOUT,
+			     GFP_KERNEL);
+
+	return (rc > 0) ? 0 : rc;
+}
+
+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_sof_timestamp(dev->netdev, &timestamp);
+	if (err)
+		netdev_err(dev->netdev,
+			   "Error %d while reading timestamp. HW timestamps may be inaccurate.",
+			   err);
+
+	return timestamp;
+}
+
+static void gs_usb_set_timestamp(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_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,
+			      GSUSB_TIMESTAMP_WORK_DELAY_SEC * HZ);
+}
+
+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 = 1;
+	cc->mult = clocksource_hz2mult(GSUSB_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,
+			      GSUSB_TIMESTAMP_WORK_DELAY_SEC * HZ);
+}
+
+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;
@@ -428,6 +532,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 				cfd->flags |= CANFD_ESI;
 
 			memcpy(cfd->data, hf->canfd->data, cfd->len);
+
+			/* set hardware timestamp if supported */
+			if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
+				gs_usb_set_timestamp(dev, skb, le32_to_cpu(hf->canfd_ts->timestamp_us));
+			}
 		} else {
 			skb = alloc_can_skb(dev->netdev, &cf);
 			if (!skb)
@@ -438,6 +547,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 			memcpy(cf->data, hf->classic_can->data, 8);
 
+			/* set hardware timestamp if supported */
+			if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
+				gs_usb_set_timestamp(dev, skb, le32_to_cpu(hf->classic_can_ts->timestamp_us));
+			}
+
 			/* ERROR frames tell us information about the controller */
 			if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
 				gs_update_state(dev, cf);
@@ -814,6 +928,10 @@ static int gs_can_open(struct net_device *netdev)
 	else if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
 		flags |= GS_CAN_MODE_LISTEN_ONLY;
 
+	/* if hardware supports timestamps, enable it */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		flags |= GS_CAN_MODE_HW_TIMESTAMP;
+
 	/* Controller is not allowed to retry TX
 	 * this mode is unavailable on atmels uc3c hardware
 	 */
@@ -838,6 +956,10 @@ static int gs_can_open(struct net_device *netdev)
 		return rc;
 	}
 
+	/* start polling sof timestamp */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		gs_usb_timestamp_init(dev);
+
 	kfree(dm);
 
 	dev->can.state = CAN_STATE_ERROR_ACTIVE;
@@ -869,6 +991,10 @@ static int gs_can_close(struct net_device *netdev)
 					  dev->rxbuf_dma[i]);
 	}
 
+	/* Stop polling sof timestamp */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		gs_usb_timestamp_stop(dev);
+
 	/* Stop sending URBs */
 	usb_kill_anchored_urbs(&dev->tx_submitted);
 	atomic_set(&dev->active_tx_urbs, 0);
@@ -895,6 +1021,7 @@ static const struct net_device_ops gs_usb_netdev_ops = {
 	.ndo_stop = gs_can_close,
 	.ndo_start_xmit = gs_can_start_xmit,
 	.ndo_change_mtu = can_change_mtu,
+	.ndo_eth_ioctl = can_eth_ioctl_hwts,
 };
 
 static int gs_usb_set_identify(struct net_device *netdev, bool do_identify)
@@ -946,7 +1073,7 @@ static int gs_usb_set_phys_id(struct net_device *dev,
 
 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 = can_ethtool_op_get_ts_info_hwts,
 };
 
 static struct gs_can *gs_make_candev(unsigned int channel,
@@ -1228,8 +1355,17 @@ 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)
+				dev->hf_size_rx = struct_size(hf, canfd_ts, 1);
+			else
+				dev->hf_size_rx = struct_size(hf, canfd, 1);
+		} else if (dev->canch[i]->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
+			dev->hf_size_rx = struct_size(hf, classic_can_ts, 1);
+		}
 	}
 
 	kfree(dconf);
-- 
2.35.1

[PATCH 2/2] can: gs_usb: advertise timestamping capabilities and add ioctl support

[Marc Kleine-Budde]

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

Following dc0d8e068f837 ("can: gs_usb: hardware timestamp support")
this correctly advertises the support for RX HW timestamps if the device
does support it. Similar to bedd94835a352 ("can: peak_usb: advertise
timestamping capabilities...") since PCAN devices also only support RX
and not TX HW timestamping.

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

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 0d6729f12946..da14bdc63281 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1016,12 +1016,37 @@ 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)
+{
+	struct hwtstamp_config hwts_cfg = { 0 };
+
+	switch (cmd) {
+	case SIOCSHWTSTAMP: /* set */
+		if (copy_from_user(&hwts_cfg, ifr->ifr_data, sizeof(hwts_cfg)))
+			return -EFAULT;
+		if (hwts_cfg.tx_type == HWTSTAMP_TX_OFF &&
+		    hwts_cfg.rx_filter == HWTSTAMP_FILTER_ALL)
+			return 0;
+		return -ERANGE;
+
+	case SIOCGHWTSTAMP: /* get */
+		hwts_cfg.tx_type = HWTSTAMP_TX_OFF;
+		hwts_cfg.rx_filter = HWTSTAMP_FILTER_ALL;
+		if (copy_to_user(ifr->ifr_data, &hwts_cfg, sizeof(hwts_cfg)))
+			return -EFAULT;
+		return 0;
+
+	default:
+		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 = can_eth_ioctl_hwts,
+	.ndo_eth_ioctl = gs_can_eth_ioctl,
 };
 
 static int gs_usb_set_identify(struct net_device *netdev, bool do_identify)
@@ -1071,9 +1096,31 @@ static int gs_usb_set_phys_id(struct net_device *dev,
 	return rc;
 }
 
+int  gs_usb_get_ts_info_hwts(struct net_device *netdev,
+				    struct ethtool_ts_info *info)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_SOFTWARE |
+		SOF_TIMESTAMPING_RX_SOFTWARE |
+		SOF_TIMESTAMPING_SOFTWARE;
+	info->phc_index = -1;
+	info->tx_types = BIT(HWTSTAMP_TX_OFF);
+	info->rx_filters = BIT(HWTSTAMP_FILTER_ALL);
+
+	/* report if device supports HW timestamps but only RX */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		info->so_timestamping |=
+			SOF_TIMESTAMPING_RX_HARDWARE |
+			SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	return 0;
+}
+
 static const struct ethtool_ops gs_usb_ethtool_ops = {
 	.set_phys_id = gs_usb_set_phys_id,
-	.get_ts_info = can_ethtool_op_get_ts_info_hwts,
+	.get_ts_info = gs_usb_get_ts_info_hwts,
 };
 
 static struct gs_can *gs_make_candev(unsigned int channel,
-- 
2.35.1

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

[Marc Kleine-Budde]

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

On 26.08.2022 12:46:28, Marc Kleine-Budde wrote:
> From: John Whittington <git@jbrengineering.co.uk>
> 
> Adds support for hardware timestamps if the firmware includes it as a
> feature via the GS_CAN_FEATURE_HW_TIMESTAMP flag. Checks for this
> feature during probe, extends the RX expected length if it is and
> enables it during open.
> 
> `classic_can_ts` and `canfd_ts` structs are copied and extended to
> include the us timestamp following data as defined in the firmware. The
> timestamp is then captured and set using `skb_hwtstamps` on each RX.
> 
> The frame us timestamp is provided from a 32 bit 1 MHz timer which would
> roll over after 4294 seconds so a cyclecounter, timecounter and delayed
> worker are used to convert the timer into a proper ns timestamp - same
> implementation as efd8d98dfb900 ("can: mcp251xfd: add HW timestamp
> infrastructure").
> 
> Hardware timestamps are added to capabilities as b1f6b93e678fb ("can:
> mcp251xfd: advertise timestamping capabilities")
> 
> Signed-off-by: John Whittington <git@jbrengineering.co.uk>
> Link: https://github.com/candle-usb/candleLight_fw/issues/100
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Please compile the driver with C=1, i.e. "make C=1" and fix the
following errors:

| drivers/net/can/usb/gs_usb.c:438:6: warning: symbol 'gs_usb_timestamp_init' was not declared. Should it be static?
| drivers/net/can/usb/gs_usb.c:454:6: warning: symbol 'gs_usb_timestamp_stop' was not declared. Should it be static?
| drivers/net/can/usb/gs_usb.c:539:64: warning: cast to restricted __le32
| drivers/net/can/usb/gs_usb.c:553:64: warning: cast to restricted __le32
| drivers/net/can/usb/gs_usb.c:1100:6: warning: symbol 'gs_usb_get_ts_info_hwts' was not declared. Should it be static?

> ---
>  drivers/net/can/usb/gs_usb.c | 142 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index baf749c8cda3..0d6729f12946 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -16,6 +16,9 @@
>  #include <linux/netdevice.h>
>  #include <linux/signal.h>
>  #include <linux/usb.h>
> +#include <linux/timecounter.h>
> +#include <linux/workqueue.h>
> +#include <linux/clocksource.h>

please keep includes sorted

>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -37,6 +40,13 @@
>  #define GSUSB_ENDPOINT_IN 1
>  #define GSUSB_ENDPOINT_OUT 2
>  
> +/* Timestamp 32 bit timer runs at 1 MHz (1 us tick). Worker accounts for timer
> + * overflow (will be after ~71 minutes) */

Please move the closing */ to a new line.

> +#define GSUSB_TIMESTAMP_TIMER_HZ 1e6

1e6 is not an int and sparse will complain about it:

|   CHECK   drivers/net/can/usb/gs_usb.c                                                                                                           
| drivers/net/can/usb/gs_usb.c:47:1: error: bad integer constant expression                                                                        
| drivers/net/can/usb/gs_usb.c:47:1: error: static assertion failed: "GSUSB_TIMESTAMP_WORK_DELAY_SEC < CYCLECOUNTER_MASK(32) / GSUSB_TIMESTAMP_TIMER_HZ / 2"

use:

#define GSUSB_TIMESTAMP_TIMER_HZ (1 * HZ_PER_MHZ)

instead and add "linux/units.h" to includes.

> +#define GSUSB_TIMESTAMP_WORK_DELAY_SEC 1800
> +static_assert(GSUSB_TIMESTAMP_WORK_DELAY_SEC <
> +	      CYCLECOUNTER_MASK(32) / GSUSB_TIMESTAMP_TIMER_HZ / 2);
> +
>  /* Device specific constants */
>  enum gs_usb_breq {
>  	GS_USB_BREQ_HOST_FORMAT = 0,
> @@ -199,6 +209,11 @@ struct classic_can {
>  	u8 data[8];
>  } __packed;
>  
> +struct classic_can_ts {
> +	u8 data[8];
> +	u32 timestamp_us;

Make this __le32, that will fix the sparse warning, too.

> +} __packed;
> +
>  struct classic_can_quirk {
>  	u8 data[8];
>  	u8 quirk;
> @@ -208,6 +223,11 @@ struct canfd {
>  	u8 data[64];
>  } __packed;
>  
> +struct canfd_ts {
> +	u8 data[64];
> +	u32 timestamp_us;

Make this __le32, that will fix the sparse warning, too.

> +} __packed;
> +
>  struct canfd_quirk {
>  	u8 data[64];
>  	u8 quirk;
> @@ -225,7 +245,9 @@ struct gs_host_frame {
>  	union {
>  		DECLARE_FLEX_ARRAY(struct classic_can, classic_can);
>  		DECLARE_FLEX_ARRAY(struct classic_can_quirk, classic_can_quirk);
> +		DECLARE_FLEX_ARRAY(struct classic_can_ts, classic_can_ts);
>  		DECLARE_FLEX_ARRAY(struct canfd, canfd);
> +		DECLARE_FLEX_ARRAY(struct canfd_ts, canfd_ts);
>  		DECLARE_FLEX_ARRAY(struct canfd_quirk, canfd_quirk);

Please make the order uniform, make _ts 2nd and _quirk 3rd.

>  	};
>  } __packed;
> @@ -259,6 +281,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 +378,83 @@ static int gs_cmd_reset(struct gs_can *gsdev)
>  	return rc;
>  }
>  
> +static int gs_usb_get_sof_timestamp(struct net_device *netdev,
> +				    u32 *sof_timestamp_us)
> +{
> +	struct gs_can *dev = netdev_priv(netdev);
> +	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,
> +			     sof_timestamp_us, sizeof(*sof_timestamp_us),
> +			     USB_CTRL_GET_TIMEOUT,
> +			     GFP_KERNEL);

CHECK: Alignment should match open parenthesis
#125: FILE: drivers/net/can/usb/gs_usb.c:390:
+       rc = usb_control_msg_recv(interface_to_usbdev(dev->iface),
+                            usb_sndctrlpipe(interface_to_usbdev(dev->iface), 0),

> +
> +	return (rc > 0) ? 0 : rc;

The doc says:

| Return: If successful, 0 is returned, Otherwise, a negative error number.

"return usb_control_msg_recv()" should be sufficient.

> +}
> +
> +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_sof_timestamp(dev->netdev, &timestamp);
> +	if (err)
> +		netdev_err(dev->netdev,
> +			   "Error %d while reading timestamp. HW timestamps may be inaccurate.",
> +			   err);
> +
> +	return timestamp;
> +}
> +
> +static void gs_usb_set_timestamp(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_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,
> +			      GSUSB_TIMESTAMP_WORK_DELAY_SEC * HZ);
> +}
> +
> +void gs_usb_timestamp_init(struct gs_can *dev)

static

> +{
> +	struct cyclecounter *cc = &dev->cc;
> +
> +	cc->read = gs_usb_timestamp_read;
> +	cc->mask = CYCLECOUNTER_MASK(32);
> +	cc->shift = 1;
> +	cc->mult = clocksource_hz2mult(GSUSB_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,
> +			      GSUSB_TIMESTAMP_WORK_DELAY_SEC * HZ);
> +}
> +
> +void gs_usb_timestamp_stop(struct gs_can *dev)

static

> +{
> +	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;
> @@ -428,6 +532,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  				cfd->flags |= CANFD_ESI;
>  
>  			memcpy(cfd->data, hf->canfd->data, cfd->len);
> +
> +			/* set hardware timestamp if supported */
> +			if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
> +				gs_usb_set_timestamp(dev, skb, le32_to_cpu(hf->canfd_ts->timestamp_us));
> +			}

braces {} are not necessary for single statement blocks

>  		} else {
>  			skb = alloc_can_skb(dev->netdev, &cf);
>  			if (!skb)
> @@ -438,6 +547,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  
>  			memcpy(cf->data, hf->classic_can->data, 8);
>  
> +			/* set hardware timestamp if supported */
> +			if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
> +				gs_usb_set_timestamp(dev, skb, le32_to_cpu(hf->classic_can_ts->timestamp_us));
> +			}
> +

braces {} are not necessary for single statement blocks

>  			/* ERROR frames tell us information about the controller */
>  			if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
>  				gs_update_state(dev, cf);
> @@ -814,6 +928,10 @@ static int gs_can_open(struct net_device *netdev)
>  	else if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
>  		flags |= GS_CAN_MODE_LISTEN_ONLY;
>  
> +	/* if hardware supports timestamps, enable it */
> +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> +		flags |= GS_CAN_MODE_HW_TIMESTAMP;
> +
>  	/* Controller is not allowed to retry TX
>  	 * this mode is unavailable on atmels uc3c hardware
>  	 */
> @@ -838,6 +956,10 @@ static int gs_can_open(struct net_device *netdev)
>  		return rc;
>  	}
>  
> +	/* start polling sof timestamp */
> +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> +		gs_usb_timestamp_init(dev);
> +
>  	kfree(dm);
>  
>  	dev->can.state = CAN_STATE_ERROR_ACTIVE;
> @@ -869,6 +991,10 @@ static int gs_can_close(struct net_device *netdev)
>  					  dev->rxbuf_dma[i]);
>  	}
>  
> +	/* Stop polling sof timestamp */
> +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> +		gs_usb_timestamp_stop(dev);
> +
>  	/* Stop sending URBs */
>  	usb_kill_anchored_urbs(&dev->tx_submitted);
>  	atomic_set(&dev->active_tx_urbs, 0);
> @@ -895,6 +1021,7 @@ static const struct net_device_ops gs_usb_netdev_ops = {
>  	.ndo_stop = gs_can_close,
>  	.ndo_start_xmit = gs_can_start_xmit,
>  	.ndo_change_mtu = can_change_mtu,
> +	.ndo_eth_ioctl = can_eth_ioctl_hwts,
>  };
>  
>  static int gs_usb_set_identify(struct net_device *netdev, bool do_identify)
> @@ -946,7 +1073,7 @@ static int gs_usb_set_phys_id(struct net_device *dev,
>  
>  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 = can_ethtool_op_get_ts_info_hwts,
>  };
>  
>  static struct gs_can *gs_make_candev(unsigned int channel,
> @@ -1228,8 +1355,17 @@ 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)
> +				dev->hf_size_rx = struct_size(hf, canfd_ts, 1);
> +			else
> +				dev->hf_size_rx = struct_size(hf, canfd, 1);
> +		} else if (dev->canch[i]->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
> +			dev->hf_size_rx = struct_size(hf, classic_can_ts, 1);
> +		}

There are several gs_usb compatible firmware implementations out there,
we cannot test them all. Better use max(), to get the true maximum
needed USB rx len.

>  	}
>  
>  	kfree(dconf);

I'll fix all these and send a v2.

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 2/2] can: gs_usb: advertise timestamping capabilities and add ioctl support

[Marc Kleine-Budde]

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

On 26.08.2022 12:46:29, Marc Kleine-Budde wrote:
> From: John Whittington <git@jbrengineering.co.uk>
> 
> Following dc0d8e068f837 ("can: gs_usb: hardware timestamp support")
> this correctly advertises the support for RX HW timestamps if the device
> does support it. Similar to bedd94835a352 ("can: peak_usb: advertise
> timestamping capabilities...") since PCAN devices also only support RX
> and not TX HW timestamping.
> 
> Not-Signed-off-by: John Whittington <git@jbrengineering.co.uk>

Can I get the S-o-b for this patch, too?

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 1/2] can: gs_usb: hardware timestamp support

[Marc Kleine-Budde]

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

On 26.08.2022 12:46:28, Marc Kleine-Budde wrote:
> +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 = 1;

Using

| cc->shift = 32 - bits_per(NSEC_PER_SEC / GSUSB_TIMESTAMP_TIMER_HZ);

gives the max precision. If you want for example to tune the gs_usb's
clock against the Linux system clock (which needs some extra code
though...).

> +	cc->mult = clocksource_hz2mult(GSUSB_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,
> +			      GSUSB_TIMESTAMP_WORK_DELAY_SEC * HZ);
> +}

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 2/2] can: gs_usb: advertise timestamping capabilities and add ioctl support

[Marc Kleine-Budde]

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

On 26.08.2022 12:46:29, Marc Kleine-Budde wrote:
> From: John Whittington <git@jbrengineering.co.uk>
> 
> Following dc0d8e068f837 ("can: gs_usb: hardware timestamp support")
> this correctly advertises the support for RX HW timestamps if the device
> does support it. Similar to bedd94835a352 ("can: peak_usb: advertise
> timestamping capabilities...") since PCAN devices also only support RX
> and not TX HW timestamping.

While all peak_usb devices support RX HW timestamps, not all gs_usb
devices do. Please adjust the gs_can_eth_ioctl() accordingly.

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 --]