>From: Paul Menzel <pmen...@molgen.mpg.de> >Sent: Monday, October 21, 2024 5:21 PM > >Dear Arkadiusz, > > >Thank you for your patch.
Thank you for the review! > >Am 21.10.24 um 16:19 schrieb Arkadiusz Kubalewski: >> 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. > >Please describe, that it’s done using `/sys` filesystem. > >How can this be tested? Sure, will add some example/description. > >> [1] https://www.ieee802.org/3/cx/public/april20/tse_3cx_01_0420.pdf >> >> Reviewed-by: Aleksandr Loktionov <aleksandr.loktio...@intel.com> >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalew...@intel.com> >> --- >> Documentation/ABI/testing/sysfs-ptp | 12 ++++++++ >> drivers/ptp/ptp_sysfs.c | 44 +++++++++++++++++++++++++++++ >> include/linux/ptp_clock_kernel.h | 29 +++++++++++++++++++ >> 3 files changed, 85 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-ptp >> b/Documentation/ABI/testing/sysfs-ptp >> index 9c317ac7c47a..a0d89e0fd72e 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.kubalew...@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 "0" means the timestamp is latched at the >> + beginning of the SFD. Value "1" means that timestamp is >> + latched after the end of SFD. > >Would it make sense to let it be configured by strings, so it’s clear, >what the values mean? > >1. beginning_of_sfd >2. end_of_sfd Actually I don't have strong opinion here. I don't know much sysfs files which take strings as arguments, thus started with numeric values. And from 'consistency' perspective it is much more common to use numeric enum values. But, as I said, I could change it, just not sure if that is actually better. Any other opinions? > >> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c >> index 6b1b8f57cd95..7e9f6ef368b6 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_MAX) >> + return -EINVAL; >> + point = val; >> + >> + err = ptp->info->set_ts_point(ptp->info, point); >> + if (err) >> + return err; >> + >> + return count; >> +} >> +static DEVICE_ATTR_RW(ts_point); >> + >> #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..921d6615bd39 100644 >> --- a/include/linux/ptp_clock_kernel.h >> +++ b/include/linux/ptp_clock_kernel.h >> @@ -55,6 +55,23 @@ struct ptp_system_timestamp { >> clockid_t clockid; >> }; >> >> +/** >> + * enum ptp_ts_point - possible timestamp latch points (IEEE 802.3cx) >> + * @PTP_TS_POINT_SFD: timestamp latched at the beginning of sending >> Start > >The alignment of the start of the description looks strange with the >second line being further right. > True, will try to fix it. >> + * 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_SFD, >> + PTP_TS_POINT_POST_SFD, >> + >> + /* private: */ >> + __PTP_TS_POINT_MAX >> +}; >> + >> +#define PTP_TS_POINT_MAX (__PTP_TS_POINT_MAX - 1) >> + >> /** >> * struct ptp_clock_info - describes a PTP hardware clock >> * >> @@ -159,6 +176,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 +220,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; > > >Kind regards, > >Paul Thank you! Arkadiusz
