[PATCH net-next v3 1/2] ptp: add control over HW timestamp latch point

Arkadiusz Kubalewski posted 2 patches 2 weeks, 4 days ago
[PATCH net-next v3 1/2] ptp: add control over HW timestamp latch point
Posted by Arkadiusz Kubalewski 2 weeks, 4 days ago
Currently HW support of ptp/timesync solutions in network PHY chips can be
implemented with two different approaches, the timestamp maybe latched
either at the beginning or after the Start of Frame Delimiter (SFD) [1].

Allow ptp device drivers to provide user with control over the HW
timestamp latch point with ptp sysfs ABI. Provide a new file under sysfs
ptp device (/sys/class/ptp/ptp<N>/ts_point). The file is available for the
user, if the device driver implements at least one of newly provided
callbacks. If the file is not provided the user shall find a PHY timestamp
latch point within the HW vendor specification.

The file is designed for root user/group access only, as the read for
regular user could impact performance of the ptp device.

Usage, examples:

** Obtain current state:
$ cat /sys/class/ptp/ptp<N>/ts_point
Command returns enum/integer:
* 1 - timestamp latched by PHY at the beginning of SFD,
* 2 - timestamp latched by PHY after the SFD,
* None - callback returns error to the user.

** Configure timestamp latch point at the beginning of SFD:
$ echo 1 > /sys/class/ptp/ptp<N>/ts_point

** Configure timestamp latch point after the SFD:
$ echo 2 > /sys/class/ptp/ptp<N>/ts_point

[1] https://www.ieee802.org/3/cx/public/april20/tse_3cx_01_0420.pdf

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
v3:
- max value of enum ptp_ts_point is also enumerated,
- move enum ptp_ts_point to uapi,
- add NONE value to enum ptp_ts_point, to make clear that value was
  not provided, thus allow further extension of ethtool netlink.
---
 Documentation/ABI/testing/sysfs-ptp | 12 ++++++++
 drivers/ptp/ptp_sysfs.c             | 44 +++++++++++++++++++++++++++++
 include/linux/ptp_clock_kernel.h    | 12 ++++++++
 include/uapi/linux/ptp_clock.h      | 18 ++++++++++++
 4 files changed, 86 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-ptp b/Documentation/ABI/testing/sysfs-ptp
index 9c317ac7c47a..063b3e20386e 100644
--- a/Documentation/ABI/testing/sysfs-ptp
+++ b/Documentation/ABI/testing/sysfs-ptp
@@ -140,3 +140,15 @@ Description:
 		PPS events to the Linux PPS subsystem. To enable PPS
 		events, write a "1" into the file. To disable events,
 		write a "0" into the file.
+
+What:		/sys/class/ptp/ptp<N>/ts_point
+Date:		October 2024
+Contact:	Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
+Description:
+		This file provides control over the point in time in
+		which the HW timestamp is latched. As specified in IEEE
+		802.3cx, the latch point can be either at the beginning
+		or after the end of Start of Frame Delimiter (SFD).
+		Value "1" means the timestamp is latched at the
+		beginning of the SFD. Value "2" means that timestamp is
+		latched after the end of SFD.
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 6b1b8f57cd95..2f3f28edbbfd 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -28,6 +28,46 @@ static ssize_t max_phase_adjustment_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(max_phase_adjustment);
 
+static ssize_t ts_point_show(struct device *dev, struct device_attribute *attr,
+			     char *page)
+{
+	struct ptp_clock *ptp = dev_get_drvdata(dev);
+	enum ptp_ts_point point;
+	int err;
+
+	if (!ptp->info->get_ts_point)
+		return -EOPNOTSUPP;
+	err = ptp->info->get_ts_point(ptp->info, &point);
+	if (err)
+		return err;
+
+	return sysfs_emit(page, "%d\n", point);
+}
+
+static ssize_t ts_point_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct ptp_clock *ptp = dev_get_drvdata(dev);
+	enum ptp_ts_point point;
+	int err;
+	u8 val;
+
+	if (!ptp->info->set_ts_point)
+		return -EOPNOTSUPP;
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+	if (val <= PTP_TS_POINT_NONE || val > PTP_TS_POINT_MAX)
+		return -EINVAL;
+	point = val;
+
+	err = ptp->info->set_ts_point(ptp->info, point);
+	if (err)
+		return err;
+
+	return count;
+}
+static DEVICE_ATTR(ts_point, 0660, ts_point_show, ts_point_store);
+
 #define PTP_SHOW_INT(name, var)						\
 static ssize_t var##_show(struct device *dev,				\
 			   struct device_attribute *attr, char *page)	\
@@ -335,6 +375,7 @@ static struct attribute *ptp_attrs[] = {
 	&dev_attr_pps_enable.attr,
 	&dev_attr_n_vclocks.attr,
 	&dev_attr_max_vclocks.attr,
+	&dev_attr_ts_point.attr,
 	NULL
 };
 
@@ -363,6 +404,9 @@ static umode_t ptp_is_attribute_visible(struct kobject *kobj,
 	} else if (attr == &dev_attr_max_phase_adjustment.attr) {
 		if (!info->adjphase || !info->getmaxphase)
 			mode = 0;
+	} else if (attr == &dev_attr_ts_point.attr) {
+		if (!info->get_ts_point && !info->set_ts_point)
+			mode = 0;
 	}
 
 	return mode;
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index c892d22ce0a7..d48619c7c60a 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -159,6 +159,14 @@ struct ptp_system_timestamp {
  *                scheduling time (>=0) or negative value in case further
  *                scheduling is not required.
  *
+ * @set_ts_point: Request change of timestamp latch point, as the timestamp
+ *                could be latched at the beginning or after the end of start
+ *                frame delimiter (SFD), as described in IEEE 802.3cx
+ *                specification.
+ *
+ * @get_ts_point: Obtain the timestamp measurement latch point, counterpart of
+ *                .set_ts_point() for getting currently configured value.
+ *
  * Drivers should embed their ptp_clock_info within a private
  * structure, obtaining a reference to it using container_of().
  *
@@ -195,6 +203,10 @@ struct ptp_clock_info {
 	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
 		      enum ptp_pin_function func, unsigned int chan);
 	long (*do_aux_work)(struct ptp_clock_info *ptp);
+	int (*set_ts_point)(struct ptp_clock_info *ptp,
+			    enum ptp_ts_point point);
+	int (*get_ts_point)(struct ptp_clock_info *ptp,
+			    enum ptp_ts_point *point);
 };
 
 struct ptp_clock;
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 18eefa6d93d6..11a9dad9db00 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -253,4 +253,22 @@ struct ptp_extts_event {
 	unsigned int rsv[2];     /* Reserved for future use. */
 };
 
+/**
+ * enum ptp_ts_point - possible timestamp latch points (IEEE 802.3cx)
+ *
+ * @PTP_TS_POINT_NONE: no timestamp latch point was provided
+ * @PTP_TS_POINT_SFD: timestamp latched at the beginning of sending Start
+ *		      of Frame Delimiter (SFD)
+ * @PTP_TS_POINT_POST_SFD: timestamp latched after the end of sending Start
+ *			   of Frame Delimiter (SFD)
+ */
+enum ptp_ts_point {
+	PTP_TS_POINT_NONE = 0,
+	PTP_TS_POINT_SFD,
+	PTP_TS_POINT_POST_SFD,
+
+	/* private: */
+	PTP_TS_POINT_MAX
+};
+
 #endif
-- 
2.38.1
Re: [PATCH net-next v3 1/2] ptp: add control over HW timestamp latch point
Posted by Andrew Lunn 2 weeks, 3 days ago
On Wed, Nov 06, 2024 at 02:07:55AM +0100, Arkadiusz Kubalewski wrote:
> Currently HW support of ptp/timesync solutions in network PHY chips can be
> implemented with two different approaches, the timestamp maybe latched
> either at the beginning or after the Start of Frame Delimiter (SFD) [1].
> 
> Allow ptp device drivers to provide user with control over the HW
> timestamp latch point with ptp sysfs ABI. Provide a new file under sysfs
> ptp device (/sys/class/ptp/ptp<N>/ts_point). The file is available for the
> user, if the device driver implements at least one of newly provided
> callbacks. If the file is not provided the user shall find a PHY timestamp
> latch point within the HW vendor specification.
> 
> The file is designed for root user/group access only, as the read for
> regular user could impact performance of the ptp device.
> 
> Usage, examples:
> 
> ** Obtain current state:
> $ cat /sys/class/ptp/ptp<N>/ts_point
> Command returns enum/integer:
> * 1 - timestamp latched by PHY at the beginning of SFD,
> * 2 - timestamp latched by PHY after the SFD,
> * None - callback returns error to the user.

-EOPNOTSUPP would be more conventional, not None.

> 
> ** Configure timestamp latch point at the beginning of SFD:
> $ echo 1 > /sys/class/ptp/ptp<N>/ts_point
> 
> ** Configure timestamp latch point after the SFD:
> $ echo 2 > /sys/class/ptp/ptp<N>/ts_point

and i assume these also return -EOPNOTSUPP if it is not supported.

And a dumb question, why should i care where the latch point is? Why
would i want to change it? Richard always says that you cannot trust
the kernel to have the same latency from kernel to kernel version
because driver developers like to tweak the latency, invalidating all
measurements the user has done when setting up their ptp4l
daemon. This just seems like yet another way to break my ptp4l
configuration.

	Andrew
RE: [Intel-wired-lan] [PATCH net-next v3 1/2] ptp: add control over HW timestamp latch point
Posted by Kubalewski, Arkadiusz 2 weeks, 3 days ago
>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>Andrew Lunn
>Sent: Wednesday, November 6, 2024 6:45 PM
>
>On Wed, Nov 06, 2024 at 02:07:55AM +0100, Arkadiusz Kubalewski wrote:
>> Currently HW support of ptp/timesync solutions in network PHY chips can
>>be
>> implemented with two different approaches, the timestamp maybe latched
>> either at the beginning or after the Start of Frame Delimiter (SFD) [1].
>>
>> Allow ptp device drivers to provide user with control over the HW
>> timestamp latch point with ptp sysfs ABI. Provide a new file under sysfs
>> ptp device (/sys/class/ptp/ptp<N>/ts_point). The file is available for
>>the
>> user, if the device driver implements at least one of newly provided
>> callbacks. If the file is not provided the user shall find a PHY
>>timestamp
>> latch point within the HW vendor specification.
>>
>> The file is designed for root user/group access only, as the read for
>> regular user could impact performance of the ptp device.
>>
>> Usage, examples:
>>
>> ** Obtain current state:
>> $ cat /sys/class/ptp/ptp<N>/ts_point
>> Command returns enum/integer:
>> * 1 - timestamp latched by PHY at the beginning of SFD,
>> * 2 - timestamp latched by PHY after the SFD,
>> * None - callback returns error to the user.
>
>-EOPNOTSUPP would be more conventional, not None.
>

Sure, I can change it if new version is needed (Kuba's other thread on this)

>>
>> ** Configure timestamp latch point at the beginning of SFD:
>> $ echo 1 > /sys/class/ptp/ptp<N>/ts_point
>>
>> ** Configure timestamp latch point after the SFD:
>> $ echo 2 > /sys/class/ptp/ptp<N>/ts_point
>
>and i assume these also return -EOPNOTSUPP if it is not supported.
>
>And a dumb question, why should i care where the latch point is? Why
>would i want to change it? Richard always says that you cannot trust
>the kernel to have the same latency from kernel to kernel version
>because driver developers like to tweak the latency, invalidating all
>measurements the user has done when setting up their ptp4l
>daemon. This just seems like yet another way to break my ptp4l
>configuration.
>
>	Andrew

Well, making control knob available to the users.

The explanation/rationale/problem statement is available under provided
Link, and patch allows part of IEEE 802_3cx standard to be controlled.

Thank you!
Arkadiusz
Re: [PATCH net-next v3 1/2] ptp: add control over HW timestamp latch point
Posted by Jakub Kicinski 2 weeks, 4 days ago
On Wed,  6 Nov 2024 02:07:55 +0100 Arkadiusz Kubalewski wrote:
> +What:		/sys/class/ptp/ptp<N>/ts_point
> +Date:		October 2024
> +Contact:	Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> +Description:
> +		This file provides control over the point in time in
> +		which the HW timestamp is latched. As specified in IEEE
> +		802.3cx, the latch point can be either at the beginning
> +		or after the end of Start of Frame Delimiter (SFD).
> +		Value "1" means the timestamp is latched at the
> +		beginning of the SFD. Value "2" means that timestamp is
> +		latched after the end of SFD.

Richard has the final say but IMO packet timestamping config does not
belong to the PHC, rather ndo_hwtstamp_set.
RE: [PATCH net-next v3 1/2] ptp: add control over HW timestamp latch point
Posted by Kubalewski, Arkadiusz 2 weeks, 3 days ago
>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, November 6, 2024 3:05 AM
>
>On Wed,  6 Nov 2024 02:07:55 +0100 Arkadiusz Kubalewski wrote:
>> +What:		/sys/class/ptp/ptp<N>/ts_point
>> +Date:		October 2024
>> +Contact:	Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> +Description:
>> +		This file provides control over the point in time in
>> +		which the HW timestamp is latched. As specified in IEEE
>> +		802.3cx, the latch point can be either at the beginning
>> +		or after the end of Start of Frame Delimiter (SFD).
>> +		Value "1" means the timestamp is latched at the
>> +		beginning of the SFD. Value "2" means that timestamp is
>> +		latched after the end of SFD.
>
>Richard has the final say but IMO packet timestamping config does not
>belong to the PHC, rather ndo_hwtstamp_set.

Ok, thank you for feedback.

Richard do you agree with Kuba?

Thank you!
Arkadiusz
Re: [PATCH net-next v3 1/2] ptp: add control over HW timestamp latch point
Posted by Richard Cochran 2 weeks, 2 days ago
On Thu, Nov 07, 2024 at 09:01:41AM +0000, Kubalewski, Arkadiusz wrote:
> >From: Jakub Kicinski <kuba@kernel.org>
> >Sent: Wednesday, November 6, 2024 3:05 AM
> >Richard has the final say but IMO packet timestamping config does not
> >belong to the PHC, rather ndo_hwtstamp_set.

Right, PHC means the hardware clock.  That is distinct from the time
stamping settings.  These belong to the network device, and are
contolled by using the SIOCSHWTSTAMP ioctl.

> Ok, thank you for feedback.
> 
> Richard do you agree with Kuba?

IMO, setting the time stamp point should be with an ioctl in a way
similar way to SIOCSHWTSTAMP.

Thanks,
Richard