All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/9] ptp .adjphase cleanups
@ 2023-05-10 20:52 Rahul Rameshbabu
  2023-05-10 20:52 ` [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used Rahul Rameshbabu
                   ` (9 more replies)
  0 siblings, 10 replies; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:52 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

The goal of this patch series is to improve documentation of .adjphase, add
a new callback .getmaxphase to enable advertising the max phase offset a
device PHC can support, and support invoking .adjphase from the testptp
kselftest.

Link: https://lore.kernel.org/netdev/20230120160609.19160723@kernel.org/

Rahul Rameshbabu (9):
  ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be
    used
  docs: ptp.rst: Add information about NVIDIA Mellanox devices
  testptp: Remove magic numbers related to nanosecond to second
    conversion
  testptp: Add support for testing ptp_clock_info .adjphase callback
  ptp: Add .getmaxphase callback to ptp_clock_info
  net/mlx5: Add .getmaxphase ptp_clock_info callback
  ptp: ptp_clockmatrix: Add .getmaxphase ptp_clock_info callback
  ptp: idt82p33: Add .getmaxphase ptp_clock_info callback
  ptp: ocp: Add .getmaxphase ptp_clock_info callback

 Documentation/driver-api/ptp.rst              | 30 ++++++++++++++++
 .../ethernet/mellanox/mlx5/core/lib/clock.c   | 31 ++++++++--------
 drivers/ptp/ptp_chardev.c                     |  5 ++-
 drivers/ptp/ptp_clock.c                       |  4 +++
 drivers/ptp/ptp_clockmatrix.c                 | 36 +++++++++----------
 drivers/ptp/ptp_clockmatrix.h                 |  2 +-
 drivers/ptp/ptp_idt82p33.c                    | 18 +++++-----
 drivers/ptp/ptp_idt82p33.h                    |  4 +--
 drivers/ptp/ptp_ocp.c                         |  7 ++++
 drivers/ptp/ptp_sysfs.c                       | 12 +++++++
 include/linux/ptp_clock_kernel.h              | 11 ++++--
 include/uapi/linux/ptp_clock.h                |  3 +-
 tools/testing/selftests/ptp/testptp.c         | 29 ++++++++++++---
 13 files changed, 136 insertions(+), 56 deletions(-)

-- 
2.38.4


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
@ 2023-05-10 20:52 ` Rahul Rameshbabu
  2023-05-11  2:09   ` Richard Cochran
  2023-05-10 20:52 ` [PATCH net-next 2/9] docs: ptp.rst: Add information about NVIDIA Mellanox devices Rahul Rameshbabu
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:52 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

.adjphase expects a PHC to use an internal servo algorithm to correct the
provided phase offset target in the callback. Implementation of the
internal servo algorithm are defined by the individual devices.

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 Documentation/driver-api/ptp.rst | 17 +++++++++++++++++
 include/linux/ptp_clock_kernel.h |  6 ++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/ptp.rst b/Documentation/driver-api/ptp.rst
index 664838ae7776..c6ef41cf6130 100644
--- a/Documentation/driver-api/ptp.rst
+++ b/Documentation/driver-api/ptp.rst
@@ -73,6 +73,23 @@ Writing clock drivers
    class driver, since the lock may also be needed by the clock
    driver's interrupt service routine.
 
+PTP hardware clock requirements for '.adjphase'
+-----------------------------------------------
+
+   The 'struct ptp_clock_info' interface has a '.adjphase' function.
+   This function has a set of requirements from the PHC in order to be
+   implemented.
+
+     * The PHC implements a servo algorithm internally that is used to
+       correct the offset passed in the '.adjphase' call.
+     * When other PTP adjustment functions are called, the PHC servo
+       algorithm is disabled, and the frequency prior to the '.adjphase'
+       call is restored internally in the PHC.
+
+   **NOTE:** '.adjphase' is not a simple time adjustment functionality
+   that 'jumps' the PHC clock time based on the provided offset. It
+   should correct the offset provided using an internal algorithm.
+
 Supported hardware
 ==================
 
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index fdffa6a98d79..f8e8443a8b35 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -77,8 +77,10 @@ struct ptp_system_timestamp {
  *            nominal frequency in parts per million, but with a
  *            16 bit binary fractional field.
  *
- * @adjphase:  Adjusts the phase offset of the hardware clock.
- *             parameter delta: Desired change in nanoseconds.
+ * @adjphase:  Indicates that the PHC should use an internal servo
+ *             algorithm to correct the provided phase offset.
+ *             parameter delta: PHC servo phase adjustment target
+ *                              in nanoseconds.
  *
  * @adjtime:  Shifts the time of the hardware clock.
  *            parameter delta: Desired change in nanoseconds.
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 2/9] docs: ptp.rst: Add information about NVIDIA Mellanox devices
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
  2023-05-10 20:52 ` [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used Rahul Rameshbabu
@ 2023-05-10 20:52 ` Rahul Rameshbabu
  2023-05-11  2:10   ` Richard Cochran
  2023-05-10 20:53 ` [PATCH net-next 3/9] testptp: Remove magic numbers related to nanosecond to second conversion Rahul Rameshbabu
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:52 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

The mlx5_core driver has implemented ptp clock driver functionality but
lacked documentation about the PTP devices. This patch adds information
about the Mellanox device family.

Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 Documentation/driver-api/ptp.rst | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/driver-api/ptp.rst b/Documentation/driver-api/ptp.rst
index c6ef41cf6130..6eab5253b800 100644
--- a/Documentation/driver-api/ptp.rst
+++ b/Documentation/driver-api/ptp.rst
@@ -123,3 +123,16 @@ Supported hardware
           - LPF settings (bandwidth, phase limiting, automatic holdover, physical layer assist (per ITU-T G.8273.2))
           - Programmable output PTP clocks, any frequency up to 1GHz (to other PHY/MAC time stampers, refclk to ASSPs/SoCs/FPGAs)
           - Lock to GNSS input, automatic switching between GNSS and user-space PHC control (optional)
+
+   * NVIDIA Mellanox
+
+     - GPIO
+          - Certain variants of ConnectX-6 Dx and later products support one
+            GPIO which can time stamp external triggers and one GPIO to produce
+            periodic signals.
+          - Certain variants of ConnectX-5 and older products support one GPIO,
+            configured to either time stamp external triggers or produce
+            periodic signals.
+     - PHC instances
+          - All ConnectX devices have a free-running counter
+          - ConnectX-6 Dx and later devices have a UTC format counter
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 3/9] testptp: Remove magic numbers related to nanosecond to second conversion
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
  2023-05-10 20:52 ` [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used Rahul Rameshbabu
  2023-05-10 20:52 ` [PATCH net-next 2/9] docs: ptp.rst: Add information about NVIDIA Mellanox devices Rahul Rameshbabu
@ 2023-05-10 20:53 ` Rahul Rameshbabu
  2023-05-11  2:14   ` Richard Cochran
  2023-05-10 20:53 ` [PATCH net-next 4/9] testptp: Add support for testing ptp_clock_info .adjphase callback Rahul Rameshbabu
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:53 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

Use existing NSEC_PER_SEC declaration in place of hardcoded magic numbers.

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Maciek Machnikowski <maciek@machnikowski.net>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 tools/testing/selftests/ptp/testptp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index 198ad5f32187..ca2b03d57aef 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -110,7 +110,7 @@ static long ppb_to_scaled_ppm(int ppb)
 
 static int64_t pctns(struct ptp_clock_time *t)
 {
-	return t->sec * 1000000000LL + t->nsec;
+	return t->sec * NSEC_PER_SEC + t->nsec;
 }
 
 static void usage(char *progname)
@@ -317,7 +317,7 @@ int main(int argc, char *argv[])
 		tx.time.tv_usec = adjns;
 		while (tx.time.tv_usec < 0) {
 			tx.time.tv_sec  -= 1;
-			tx.time.tv_usec += 1000000000;
+			tx.time.tv_usec += NSEC_PER_SEC;
 		}
 
 		if (clock_adjtime(clkid, &tx) < 0) {
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 4/9] testptp: Add support for testing ptp_clock_info .adjphase callback
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
                   ` (2 preceding siblings ...)
  2023-05-10 20:53 ` [PATCH net-next 3/9] testptp: Remove magic numbers related to nanosecond to second conversion Rahul Rameshbabu
@ 2023-05-10 20:53 ` Rahul Rameshbabu
  2023-05-11  2:15   ` Richard Cochran
  2023-05-10 20:53 ` [PATCH net-next 5/9] ptp: Add .getmaxphase callback to ptp_clock_info Rahul Rameshbabu
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:53 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

Invoke clock_adjtime syscall with tx.modes set with ADJ_OFFSET when testptp
is invoked with a phase adjustment offset value. Support seconds and
nanoseconds for the offset value.

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Maciek Machnikowski <maciek@machnikowski.net>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 tools/testing/selftests/ptp/testptp.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index ca2b03d57aef..ae23ef51f198 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -134,6 +134,7 @@ static void usage(char *progname)
 		"            1 - external time stamp\n"
 		"            2 - periodic output\n"
 		" -n val     shift the ptp clock time by 'val' nanoseconds\n"
+		" -o val     phase offset (in nanoseconds) to be provided to the PHC servo\n"
 		" -p val     enable output with a period of 'val' nanoseconds\n"
 		" -H val     set output phase to 'val' nanoseconds (requires -p)\n"
 		" -w val     set output pulse width to 'val' nanoseconds (requires -p)\n"
@@ -167,6 +168,7 @@ int main(int argc, char *argv[])
 	int adjfreq = 0x7fffffff;
 	int adjtime = 0;
 	int adjns = 0;
+	int adjphase = 0;
 	int capabilities = 0;
 	int extts = 0;
 	int flagtest = 0;
@@ -188,7 +190,7 @@ int main(int argc, char *argv[])
 
 	progname = strrchr(argv[0], '/');
 	progname = progname ? 1+progname : argv[0];
-	while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:p:P:sSt:T:w:z"))) {
+	while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:o:p:P:sSt:T:w:z"))) {
 		switch (c) {
 		case 'c':
 			capabilities = 1;
@@ -228,6 +230,9 @@ int main(int argc, char *argv[])
 		case 'n':
 			adjns = atoi(optarg);
 			break;
+		case 'o':
+			adjphase = atoi(optarg);
+			break;
 		case 'p':
 			perout = atoll(optarg);
 			break;
@@ -327,6 +332,18 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (adjphase) {
+		memset(&tx, 0, sizeof(tx));
+		tx.modes = ADJ_OFFSET | ADJ_NANO;
+		tx.offset = adjphase;
+
+		if (clock_adjtime(clkid, &tx) < 0) {
+			perror("clock_adjtime");
+		} else {
+			puts("phase adjustment okay");
+		}
+	}
+
 	if (gettime) {
 		if (clock_gettime(clkid, &ts)) {
 			perror("clock_gettime");
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 5/9] ptp: Add .getmaxphase callback to ptp_clock_info
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
                   ` (3 preceding siblings ...)
  2023-05-10 20:53 ` [PATCH net-next 4/9] testptp: Add support for testing ptp_clock_info .adjphase callback Rahul Rameshbabu
@ 2023-05-10 20:53 ` Rahul Rameshbabu
  2023-05-11  2:20   ` Richard Cochran
  2023-05-10 20:53 ` [PATCH net-next 6/9] net/mlx5: Add .getmaxphase ptp_clock_info callback Rahul Rameshbabu
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:53 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

Enables advertisement of the maximum offset supported by the phase control
functionality of PHCs. The callback is used to return an error if an offset
not supported by the PHC is used in ADJ_OFFSET. The ioctls
PTP_CLOCK_GETCAPS and PTP_CLOCK_GETCAPS2 now advertise the maximum offset a
PHC's phase control functionality is capable of supporting. Introduce new
sysfs node, max_phase_adjustment.

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Maciek Machnikowski <maciek@machnikowski.net>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/ptp/ptp_chardev.c             |  5 ++++-
 drivers/ptp/ptp_clock.c               |  4 ++++
 drivers/ptp/ptp_sysfs.c               | 12 ++++++++++++
 include/linux/ptp_clock_kernel.h      |  5 +++++
 include/uapi/linux/ptp_clock.h        |  3 ++-
 tools/testing/selftests/ptp/testptp.c |  6 ++++--
 6 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index af3bc65c4595..362bf756e6b7 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -136,7 +136,10 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		caps.pps = ptp->info->pps;
 		caps.n_pins = ptp->info->n_pins;
 		caps.cross_timestamping = ptp->info->getcrosststamp != NULL;
-		caps.adjust_phase = ptp->info->adjphase != NULL;
+		caps.adjust_phase = ptp->info->adjphase != NULL &&
+				    ptp->info->getmaxphase != NULL;
+		if (caps.adjust_phase)
+			caps.max_phase_adj = ptp->info->getmaxphase(ptp->info);
 		if (copy_to_user((void __user *)arg, &caps, sizeof(caps)))
 			err = -EFAULT;
 		break;
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 790f9250b381..80f74e38c2da 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -135,11 +135,15 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 		ptp->dialed_frequency = tx->freq;
 	} else if (tx->modes & ADJ_OFFSET) {
 		if (ops->adjphase) {
+			s32 max_phase_adj = ops->getmaxphase(ops);
 			s32 offset = tx->offset;
 
 			if (!(tx->modes & ADJ_NANO))
 				offset *= NSEC_PER_USEC;
 
+			if (offset > max_phase_adj || offset < -max_phase_adj)
+				return -ERANGE;
+
 			err = ops->adjphase(ops, offset);
 		}
 	} else if (tx->modes == 0) {
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index f30b0a439470..62a5fafcd407 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -27,7 +27,18 @@ static ssize_t var##_show(struct device *dev,				\
 }									\
 static DEVICE_ATTR(name, 0444, var##_show, NULL);
 
+#define PTP_SHOW_CB_INT(name, cb)					\
+static ssize_t cb##_show(struct device *dev,				\
+			  struct device_attribute *attr, char *page)	\
+{									\
+	struct ptp_clock *ptp = dev_get_drvdata(dev);			\
+	return snprintf(page, PAGE_SIZE-1, "%d\n",			\
+			ptp->info->cb(ptp->info));			\
+}									\
+static DEVICE_ATTR(name, 0444, cb##_show, NULL);
+
 PTP_SHOW_INT(max_adjustment, max_adj);
+PTP_SHOW_CB_INT(max_phase_adjustment, getmaxphase);
 PTP_SHOW_INT(n_alarms, n_alarm);
 PTP_SHOW_INT(n_external_timestamps, n_ext_ts);
 PTP_SHOW_INT(n_periodic_outputs, n_per_out);
@@ -309,6 +320,7 @@ static struct attribute *ptp_attrs[] = {
 	&dev_attr_clock_name.attr,
 
 	&dev_attr_max_adjustment.attr,
+	&dev_attr_max_phase_adjustment.attr,
 	&dev_attr_n_alarms.attr,
 	&dev_attr_n_external_timestamps.attr,
 	&dev_attr_n_periodic_outputs.attr,
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index f8e8443a8b35..1ef4e0f9bd2a 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -82,6 +82,10 @@ struct ptp_system_timestamp {
  *             parameter delta: PHC servo phase adjustment target
  *                              in nanoseconds.
  *
+ * @getmaxphase:  Advertises maximum offset that can be provided
+ *                to the hardware clock's phase control functionality
+ *                through adjphase.
+ *
  * @adjtime:  Shifts the time of the hardware clock.
  *            parameter delta: Desired change in nanoseconds.
  *
@@ -171,6 +175,7 @@ struct ptp_clock_info {
 	struct ptp_pin_desc *pin_config;
 	int (*adjfine)(struct ptp_clock_info *ptp, long scaled_ppm);
 	int (*adjphase)(struct ptp_clock_info *ptp, s32 phase);
+	s32 (*getmaxphase)(struct ptp_clock_info *ptp);
 	int (*adjtime)(struct ptp_clock_info *ptp, s64 delta);
 	int (*gettime64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
 	int (*gettimex64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 1d108d597f66..05cc35fc94ac 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -95,7 +95,8 @@ struct ptp_clock_caps {
 	int cross_timestamping;
 	/* Whether the clock supports adjust phase */
 	int adjust_phase;
-	int rsv[12];   /* Reserved for future use. */
+	int max_phase_adj; /* Maximum phase adjustment in nanoseconds. */
+	int rsv[11];       /* Reserved for future use. */
 };
 
 struct ptp_extts_request {
diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index ae23ef51f198..a162a3e15c29 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -292,7 +292,8 @@ int main(int argc, char *argv[])
 			       "  %d pulse per second\n"
 			       "  %d programmable pins\n"
 			       "  %d cross timestamping\n"
-			       "  %d adjust_phase\n",
+			       "  %d adjust_phase\n"
+			       "  %d maximum phase adjustment (ns)\n",
 			       caps.max_adj,
 			       caps.n_alarm,
 			       caps.n_ext_ts,
@@ -300,7 +301,8 @@ int main(int argc, char *argv[])
 			       caps.pps,
 			       caps.n_pins,
 			       caps.cross_timestamping,
-			       caps.adjust_phase);
+			       caps.adjust_phase,
+			       caps.max_phase_adj);
 		}
 	}
 
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 6/9] net/mlx5: Add .getmaxphase ptp_clock_info callback
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
                   ` (4 preceding siblings ...)
  2023-05-10 20:53 ` [PATCH net-next 5/9] ptp: Add .getmaxphase callback to ptp_clock_info Rahul Rameshbabu
@ 2023-05-10 20:53 ` Rahul Rameshbabu
  2023-05-11  2:20   ` Richard Cochran
  2023-05-10 20:53 ` [PATCH net-next 7/9] ptp: ptp_clockmatrix: " Rahul Rameshbabu
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:53 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

Implement .getmaxphase callback of ptp_clock_info in mlx5 driver. No longer
do a range check in .adjphase callback implementation. Handled by the ptp
stack.

Cc: Saeed Mahameed <saeedm@nvidia.com>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/lib/clock.c   | 31 +++++++++----------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
index 932fbc843c69..973babfaff25 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c
@@ -93,17 +93,23 @@ static bool mlx5_modify_mtutc_allowed(struct mlx5_core_dev *mdev)
 	return MLX5_CAP_MCAM_FEATURE(mdev, ptpcyc2realtime_modify);
 }
 
-static bool mlx5_is_mtutc_time_adj_cap(struct mlx5_core_dev *mdev, s64 delta)
+static s32 mlx5_ptp_getmaxphase(struct ptp_clock_info *ptp)
 {
-	s64 min = MLX5_MTUTC_OPERATION_ADJUST_TIME_MIN;
-	s64 max = MLX5_MTUTC_OPERATION_ADJUST_TIME_MAX;
+	struct mlx5_clock *clock = container_of(ptp, struct mlx5_clock, ptp_info);
+	struct mlx5_core_dev *mdev;
 
-	if (MLX5_CAP_MCAM_FEATURE(mdev, mtutc_time_adjustment_extended_range)) {
-		min = MLX5_MTUTC_OPERATION_ADJUST_TIME_EXTENDED_MIN;
-		max = MLX5_MTUTC_OPERATION_ADJUST_TIME_EXTENDED_MAX;
-	}
+	mdev = container_of(clock, struct mlx5_core_dev, clock);
+
+	return MLX5_CAP_MCAM_FEATURE(mdev, mtutc_time_adjustment_extended_range) ?
+		       MLX5_MTUTC_OPERATION_ADJUST_TIME_EXTENDED_MAX :
+			     MLX5_MTUTC_OPERATION_ADJUST_TIME_MAX;
+}
+
+static bool mlx5_is_mtutc_time_adj_cap(struct mlx5_core_dev *mdev, s64 delta)
+{
+	s64 max = mlx5_ptp_getmaxphase(&mdev->clock.ptp_info);
 
-	if (delta < min || delta > max)
+	if (delta < -max || delta > max)
 		return false;
 
 	return true;
@@ -351,14 +357,6 @@ static int mlx5_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 
 static int mlx5_ptp_adjphase(struct ptp_clock_info *ptp, s32 delta)
 {
-	struct mlx5_clock *clock = container_of(ptp, struct mlx5_clock, ptp_info);
-	struct mlx5_core_dev *mdev;
-
-	mdev = container_of(clock, struct mlx5_core_dev, clock);
-
-	if (!mlx5_is_mtutc_time_adj_cap(mdev, delta))
-		return -ERANGE;
-
 	return mlx5_ptp_adjtime(ptp, delta);
 }
 
@@ -734,6 +732,7 @@ static const struct ptp_clock_info mlx5_ptp_clock_info = {
 	.pps		= 0,
 	.adjfine	= mlx5_ptp_adjfine,
 	.adjphase	= mlx5_ptp_adjphase,
+	.getmaxphase    = mlx5_ptp_getmaxphase,
 	.adjtime	= mlx5_ptp_adjtime,
 	.gettimex64	= mlx5_ptp_gettimex,
 	.settime64	= mlx5_ptp_settime,
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 7/9] ptp: ptp_clockmatrix: Add .getmaxphase ptp_clock_info callback
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
                   ` (5 preceding siblings ...)
  2023-05-10 20:53 ` [PATCH net-next 6/9] net/mlx5: Add .getmaxphase ptp_clock_info callback Rahul Rameshbabu
@ 2023-05-10 20:53 ` Rahul Rameshbabu
  2023-05-10 20:53 ` [PATCH net-next 8/9] ptp: idt82p33: " Rahul Rameshbabu
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:53 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

Advertise the maximum offset the .adjphase callback is capable of
supporting in nanoseconds for IDT ClockMatrix devices.

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Vincent Cheng <vincent.cheng.xh@renesas.com>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/ptp/ptp_clockmatrix.c | 36 +++++++++++++++++------------------
 drivers/ptp/ptp_clockmatrix.h |  2 +-
 2 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index c9d451bf89e2..f6f9d4adce04 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel)
 /* PTP Hardware Clock interface */
 
 /*
- * Maximum absolute value for write phase offset in picoseconds
- *
- * @channel:  channel
- * @delta_ns: delta in nanoseconds
+ * Maximum absolute value for write phase offset in nanoseconds
  *
  * Destination signed register is 32-bit register in resolution of 50ps
  *
- * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350
+ * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350 ps
+ * Represent 107374182350 ps as 107374182 ns
+ */
+static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused)
+{
+	return MAX_ABS_WRITE_PHASE_NANOSECONDS;
+}
+
+/*
+ * Internal function for implementing support for write phase offset
+ *
+ * @channel:  channel
+ * @delta_ns: delta in nanoseconds
  */
 static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns)
 {
@@ -1708,7 +1717,6 @@ static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns)
 	u8 i;
 	u8 buf[4] = {0};
 	s32 phase_50ps;
-	s64 offset_ps;
 
 	if (channel->mode != PTP_PLL_MODE_WRITE_PHASE) {
 		err = channel->configure_write_phase(channel);
@@ -1716,19 +1724,7 @@ static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns)
 			return err;
 	}
 
-	offset_ps = (s64)delta_ns * 1000;
-
-	/*
-	 * Check for 32-bit signed max * 50:
-	 *
-	 * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350
-	 */
-	if (offset_ps > MAX_ABS_WRITE_PHASE_PICOSECONDS)
-		offset_ps = MAX_ABS_WRITE_PHASE_PICOSECONDS;
-	else if (offset_ps < -MAX_ABS_WRITE_PHASE_PICOSECONDS)
-		offset_ps = -MAX_ABS_WRITE_PHASE_PICOSECONDS;
-
-	phase_50ps = div_s64(offset_ps, 50);
+	phase_50ps = div_s64((s64)delta_ns * 1000, 50);
 
 	for (i = 0; i < 4; i++) {
 		buf[i] = phase_50ps & 0xff;
@@ -2048,6 +2044,7 @@ static const struct ptp_clock_info idtcm_caps = {
 	.n_ext_ts	= MAX_TOD,
 	.n_pins		= MAX_REF_CLK,
 	.adjphase	= &idtcm_adjphase,
+	.getmaxphase	= &idtcm_getmaxphase,
 	.adjfine	= &idtcm_adjfine,
 	.adjtime	= &idtcm_adjtime,
 	.gettime64	= &idtcm_gettime,
@@ -2064,6 +2061,7 @@ static const struct ptp_clock_info idtcm_caps_deprecated = {
 	.n_ext_ts	= MAX_TOD,
 	.n_pins		= MAX_REF_CLK,
 	.adjphase	= &idtcm_adjphase,
+	.getmaxphase    = &idtcm_getmaxphase,
 	.adjfine	= &idtcm_adjfine,
 	.adjtime	= &idtcm_adjtime_deprecated,
 	.gettime64	= &idtcm_gettime,
diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
index bf1e49409844..7c17c4f7f573 100644
--- a/drivers/ptp/ptp_clockmatrix.h
+++ b/drivers/ptp/ptp_clockmatrix.h
@@ -18,7 +18,7 @@
 #define MAX_PLL		(8)
 #define MAX_REF_CLK	(16)
 
-#define MAX_ABS_WRITE_PHASE_PICOSECONDS (107374182350LL)
+#define MAX_ABS_WRITE_PHASE_NANOSECONDS (107374182L)
 
 #define TOD_MASK_ADDR		(0xFFA5)
 #define DEFAULT_TOD_MASK	(0x04)
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 8/9] ptp: idt82p33: Add .getmaxphase ptp_clock_info callback
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
                   ` (6 preceding siblings ...)
  2023-05-10 20:53 ` [PATCH net-next 7/9] ptp: ptp_clockmatrix: " Rahul Rameshbabu
@ 2023-05-10 20:53 ` Rahul Rameshbabu
  2023-05-10 20:53 ` [PATCH net-next 9/9] ptp: ocp: " Rahul Rameshbabu
  2023-05-11  2:23 ` [PATCH net-next 0/9] ptp .adjphase cleanups Richard Cochran
  9 siblings, 0 replies; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:53 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

Advertise the maximum offset the .adjphase callback is capable of
supporting in nanoseconds for IDT devices.

This change refactors the negation of the offset stored in the register to
be after the boundary check of the offset value rather than before.
Boundary checking is done based on the intended value rather than its
device-specific representation.

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Min Li <min.li.xe@renesas.com>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/ptp/ptp_idt82p33.c | 18 +++++++++---------
 drivers/ptp/ptp_idt82p33.h |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
index afc76c22271a..057190b9cd3d 100644
--- a/drivers/ptp/ptp_idt82p33.c
+++ b/drivers/ptp/ptp_idt82p33.c
@@ -978,24 +978,23 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
 	return err;
 }
 
+static s32 idt82p33_getmaxphase(__always_unused struct ptp_clock_info *ptp)
+{
+	return WRITE_PHASE_OFFSET_LIMIT;
+}
+
 static int idt82p33_adjwritephase(struct ptp_clock_info *ptp, s32 offset_ns)
 {
 	struct idt82p33_channel *channel =
 		container_of(ptp, struct idt82p33_channel, caps);
 	struct idt82p33 *idt82p33 = channel->idt82p33;
-	s64 offset_regval, offset_fs;
+	s64 offset_regval;
 	u8 val[4] = {0};
 	int err;
 
-	offset_fs = (s64)(-offset_ns) * 1000000;
-
-	if (offset_fs > WRITE_PHASE_OFFSET_LIMIT)
-		offset_fs = WRITE_PHASE_OFFSET_LIMIT;
-	else if (offset_fs < -WRITE_PHASE_OFFSET_LIMIT)
-		offset_fs = -WRITE_PHASE_OFFSET_LIMIT;
-
 	/* Convert from phaseoffset_fs to register value */
-	offset_regval = div_s64(offset_fs * 1000, IDT_T0DPLL_PHASE_RESOL);
+	offset_regval = div_s64((s64)(-offset_ns) * 1000000000ll,
+				IDT_T0DPLL_PHASE_RESOL);
 
 	val[0] = offset_regval & 0xFF;
 	val[1] = (offset_regval >> 8) & 0xFF;
@@ -1175,6 +1174,7 @@ static void idt82p33_caps_init(u32 index, struct ptp_clock_info *caps,
 	caps->n_ext_ts = MAX_PHC_PLL,
 	caps->n_pins = max_pins,
 	caps->adjphase = idt82p33_adjwritephase,
+	caps->getmaxphase = idt82p33_getmaxphase,
 	caps->adjfine = idt82p33_adjfine;
 	caps->adjtime = idt82p33_adjtime;
 	caps->gettime64 = idt82p33_gettime;
diff --git a/drivers/ptp/ptp_idt82p33.h b/drivers/ptp/ptp_idt82p33.h
index 8fcb0b17d207..6a63c14b6966 100644
--- a/drivers/ptp/ptp_idt82p33.h
+++ b/drivers/ptp/ptp_idt82p33.h
@@ -43,9 +43,9 @@
 #define DEFAULT_OUTPUT_MASK_PLL1	DEFAULT_OUTPUT_MASK_PLL0
 
 /**
- * @brief Maximum absolute value for write phase offset in femtoseconds
+ * @brief Maximum absolute value for write phase offset in nanoseconds
  */
-#define WRITE_PHASE_OFFSET_LIMIT (20000052084ll)
+#define WRITE_PHASE_OFFSET_LIMIT (20000l)
 
 /** @brief Phase offset resolution
  *
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH net-next 9/9] ptp: ocp: Add .getmaxphase ptp_clock_info callback
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
                   ` (7 preceding siblings ...)
  2023-05-10 20:53 ` [PATCH net-next 8/9] ptp: idt82p33: " Rahul Rameshbabu
@ 2023-05-10 20:53 ` Rahul Rameshbabu
  2023-05-11 11:12   ` Vadim Fedorenko
  2023-05-11  2:23 ` [PATCH net-next 0/9] ptp .adjphase cleanups Richard Cochran
  9 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-10 20:53 UTC (permalink / raw)
  To: netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller

Add a function that advertises a maximum offset of zero supported by
ptp_clock_info .adjphase in the OCP null ptp implementation.

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jonathan Lemon <jonathan.lemon@gmail.com>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/ptp/ptp_ocp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index ab8cab4d1560..20a974ced8d6 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -1124,6 +1124,12 @@ ptp_ocp_null_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
 	return -EOPNOTSUPP;
 }
 
+static s32
+ptp_ocp_null_getmaxphase(struct ptp_clock_info *ptp_info)
+{
+	return 0;
+}
+
 static int
 ptp_ocp_null_adjphase(struct ptp_clock_info *ptp_info, s32 phase_ns)
 {
@@ -1239,6 +1245,7 @@ static const struct ptp_clock_info ptp_ocp_clock_info = {
 	.adjtime	= ptp_ocp_adjtime,
 	.adjfine	= ptp_ocp_null_adjfine,
 	.adjphase	= ptp_ocp_null_adjphase,
+	.getmaxphase	= ptp_ocp_null_getmaxphase,
 	.enable		= ptp_ocp_enable,
 	.verify		= ptp_ocp_verify,
 	.pps		= true,
-- 
2.38.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used
  2023-05-10 20:52 ` [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used Rahul Rameshbabu
@ 2023-05-11  2:09   ` Richard Cochran
  2023-05-11 20:20     ` Rahul Rameshbabu
  0 siblings, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2023-05-11  2:09 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, May 10, 2023 at 01:52:58PM -0700, Rahul Rameshbabu wrote:
> +PTP hardware clock requirements for '.adjphase'
> +-----------------------------------------------
> +
> +   The 'struct ptp_clock_info' interface has a '.adjphase' function.
> +   This function has a set of requirements from the PHC in order to be
> +   implemented.
> +
> +     * The PHC implements a servo algorithm internally that is used to
> +       correct the offset passed in the '.adjphase' call.
> +     * When other PTP adjustment functions are called, the PHC servo
> +       algorithm is disabled,

I disagree with this part:

> and the frequency prior to the '.adjphase'
> +       call is restored internally in the PHC.

That seems like an arbitrary rule that doesn't make much sense.

Thanks,
Richard



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 2/9] docs: ptp.rst: Add information about NVIDIA Mellanox devices
  2023-05-10 20:52 ` [PATCH net-next 2/9] docs: ptp.rst: Add information about NVIDIA Mellanox devices Rahul Rameshbabu
@ 2023-05-11  2:10   ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2023-05-11  2:10 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, May 10, 2023 at 01:52:59PM -0700, Rahul Rameshbabu wrote:
> The mlx5_core driver has implemented ptp clock driver functionality but
> lacked documentation about the PTP devices. This patch adds information
> about the Mellanox device family.
> 
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 3/9] testptp: Remove magic numbers related to nanosecond to second conversion
  2023-05-10 20:53 ` [PATCH net-next 3/9] testptp: Remove magic numbers related to nanosecond to second conversion Rahul Rameshbabu
@ 2023-05-11  2:14   ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2023-05-11  2:14 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, May 10, 2023 at 01:53:00PM -0700, Rahul Rameshbabu wrote:
> Use existing NSEC_PER_SEC declaration in place of hardcoded magic numbers.
> 
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Maciek Machnikowski <maciek@machnikowski.net>
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 4/9] testptp: Add support for testing ptp_clock_info .adjphase callback
  2023-05-10 20:53 ` [PATCH net-next 4/9] testptp: Add support for testing ptp_clock_info .adjphase callback Rahul Rameshbabu
@ 2023-05-11  2:15   ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2023-05-11  2:15 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, May 10, 2023 at 01:53:01PM -0700, Rahul Rameshbabu wrote:
> Invoke clock_adjtime syscall with tx.modes set with ADJ_OFFSET when testptp
> is invoked with a phase adjustment offset value. Support seconds and
> nanoseconds for the offset value.
> 
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Maciek Machnikowski <maciek@machnikowski.net>
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 5/9] ptp: Add .getmaxphase callback to ptp_clock_info
  2023-05-10 20:53 ` [PATCH net-next 5/9] ptp: Add .getmaxphase callback to ptp_clock_info Rahul Rameshbabu
@ 2023-05-11  2:20   ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2023-05-11  2:20 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, May 10, 2023 at 01:53:02PM -0700, Rahul Rameshbabu wrote:

> +#define PTP_SHOW_CB_INT(name, cb)					\
> +static ssize_t cb##_show(struct device *dev,				\
> +			  struct device_attribute *attr, char *page)	\
> +{									\
> +	struct ptp_clock *ptp = dev_get_drvdata(dev);			\
> +	return snprintf(page, PAGE_SIZE-1, "%d\n",			\
> +			ptp->info->cb(ptp->info));			\
> +}									\
> +static DEVICE_ATTR(name, 0444, cb##_show, NULL);
> +
>  PTP_SHOW_INT(max_adjustment, max_adj);
> +PTP_SHOW_CB_INT(max_phase_adjustment, getmaxphase);

Ugh, please no macro here.  Just add it plainly like clock_name_show.
Rest of patch LGTM.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 6/9] net/mlx5: Add .getmaxphase ptp_clock_info callback
  2023-05-10 20:53 ` [PATCH net-next 6/9] net/mlx5: Add .getmaxphase ptp_clock_info callback Rahul Rameshbabu
@ 2023-05-11  2:20   ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2023-05-11  2:20 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, May 10, 2023 at 01:53:03PM -0700, Rahul Rameshbabu wrote:
> Implement .getmaxphase callback of ptp_clock_info in mlx5 driver. No longer
> do a range check in .adjphase callback implementation. Handled by the ptp
> stack.
> 
> Cc: Saeed Mahameed <saeedm@nvidia.com>
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 0/9] ptp .adjphase cleanups
  2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
                   ` (8 preceding siblings ...)
  2023-05-10 20:53 ` [PATCH net-next 9/9] ptp: ocp: " Rahul Rameshbabu
@ 2023-05-11  2:23 ` Richard Cochran
  2023-05-11 20:26   ` Rahul Rameshbabu
  9 siblings, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2023-05-11  2:23 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, May 10, 2023 at 01:52:57PM -0700, Rahul Rameshbabu wrote:
> The goal of this patch series is to improve documentation of .adjphase, add
> a new callback .getmaxphase to enable advertising the max phase offset a
> device PHC can support, and support invoking .adjphase from the testptp
> kselftest.

Let's make sure to get ACKs on these three...

3600   C 10.May'23 Rahul Rameshbab      ├─>[PATCH net-next 7/9] ptp: ptp_clockmatrix: Add .getmaxphase ptp_clock_info callback
3601   C 10.May'23 Rahul Rameshbab      ├─>[PATCH net-next 8/9] ptp: idt82p33: Add .getmaxphase ptp_clock_info callback
3602   C 10.May'23 Rahul Rameshbab      └─>[PATCH net-next 9/9] ptp: ocp: Add .getmaxphase ptp_clock_info callback

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 9/9] ptp: ocp: Add .getmaxphase ptp_clock_info callback
  2023-05-10 20:53 ` [PATCH net-next 9/9] ptp: ocp: " Rahul Rameshbabu
@ 2023-05-11 11:12   ` Vadim Fedorenko
  2023-05-11 20:35     ` Rahul Rameshbabu
  0 siblings, 1 reply; 24+ messages in thread
From: Vadim Fedorenko @ 2023-05-11 11:12 UTC (permalink / raw)
  To: Rahul Rameshbabu, netdev
  Cc: Saeed Mahameed, Gal Pressman, Tariq Toukan, David S. Miller,
	Jakub Kicinski, Richard Cochran, Jacob Keller, Jonathan Lemon

On 10/05/2023 21:53, Rahul Rameshbabu wrote:
> Add a function that advertises a maximum offset of zero supported by
> ptp_clock_info .adjphase in the OCP null ptp implementation.
>
> Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Jonathan Lemon <jonathan.lemon@gmail.com>

Neither Jonathan, nor myself are in the actual Cc list.

> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> ---
>   drivers/ptp/ptp_ocp.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index ab8cab4d1560..20a974ced8d6 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -1124,6 +1124,12 @@ ptp_ocp_null_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
>   	return -EOPNOTSUPP;
>   }
>   
> +static s32
> +ptp_ocp_null_getmaxphase(struct ptp_clock_info *ptp_info)
> +{
> +	return 0;
> +}
> +
>   static int
>   ptp_ocp_null_adjphase(struct ptp_clock_info *ptp_info, s32 phase_ns)
>   {
> @@ -1239,6 +1245,7 @@ static const struct ptp_clock_info ptp_ocp_clock_info = {
>   	.adjtime	= ptp_ocp_adjtime,
>   	.adjfine	= ptp_ocp_null_adjfine,
>   	.adjphase	= ptp_ocp_null_adjphase,
> +	.getmaxphase	= ptp_ocp_null_getmaxphase,
>   	.enable		= ptp_ocp_enable,
>   	.verify		= ptp_ocp_verify,
>   	.pps		= true,

Acked-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used
  2023-05-11  2:09   ` Richard Cochran
@ 2023-05-11 20:20     ` Rahul Rameshbabu
  2023-05-12  0:51       ` Richard Cochran
  0 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-11 20:20 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, 10 May, 2023 19:09:40 -0700 Richard Cochran <richardcochran@gmail.com> wrote:
> On Wed, May 10, 2023 at 01:52:58PM -0700, Rahul Rameshbabu wrote:
>> +PTP hardware clock requirements for '.adjphase'
>> +-----------------------------------------------
>> +
>> +   The 'struct ptp_clock_info' interface has a '.adjphase' function.
>> +   This function has a set of requirements from the PHC in order to be
>> +   implemented.
>> +
>> +     * The PHC implements a servo algorithm internally that is used to
>> +       correct the offset passed in the '.adjphase' call.
>> +     * When other PTP adjustment functions are called, the PHC servo
>> +       algorithm is disabled,
>
> I disagree with this part:
>
>> and the frequency prior to the '.adjphase'
>> +       call is restored internally in the PHC.
>
> That seems like an arbitrary rule that doesn't make much sense.

If the PHC does not restore the frequency, won't the value cached in the
ptp stack in the kernel become inaccurate compared to the frequency
change induced by the '.adjphase' call? This concern is why I added this
clause in the documentation. Let me know if my understanding is off with
regards to this. I think we had a similar conversation on this
previously in the mailing list.

https://lore.kernel.org/netdev/Y88L6EPtgvW4tSA+@hoboy.vegasvil.org/

>
> Thanks,
> Richard

-- Rahul Rameshbabu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 0/9] ptp .adjphase cleanups
  2023-05-11  2:23 ` [PATCH net-next 0/9] ptp .adjphase cleanups Richard Cochran
@ 2023-05-11 20:26   ` Rahul Rameshbabu
  0 siblings, 0 replies; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-11 20:26 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Wed, 10 May, 2023 19:23:37 -0700 Richard Cochran <richardcochran@gmail.com> wrote:
> On Wed, May 10, 2023 at 01:52:57PM -0700, Rahul Rameshbabu wrote:
>> The goal of this patch series is to improve documentation of .adjphase, add
>> a new callback .getmaxphase to enable advertising the max phase offset a
>> device PHC can support, and support invoking .adjphase from the testptp
>> kselftest.
>
> Let's make sure to get ACKs on these three...
>
> 3600   C 10.May'23 Rahul Rameshbab      ├─>[PATCH net-next 7/9] ptp: ptp_clockmatrix: Add .getmaxphase ptp_clock_info callback
> 3601   C 10.May'23 Rahul Rameshbab      ├─>[PATCH net-next 8/9] ptp: idt82p33: Add .getmaxphase ptp_clock_info callback
> 3602   C 10.May'23 Rahul Rameshbab      └─>[PATCH net-next 9/9] ptp: ocp: Add .getmaxphase ptp_clock_info callback

Yeah, I had a small PEBKAC issue when the git-send-email command was
invoked (still had --suppress-cc in the commandline from test runs in my
shell history)... Will be sure to verify the CCs on each patch when
sending out the v2 for this series.

>
> Thanks,
> Richard
p
-- Rahul Rameshbabu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 9/9] ptp: ocp: Add .getmaxphase ptp_clock_info callback
  2023-05-11 11:12   ` Vadim Fedorenko
@ 2023-05-11 20:35     ` Rahul Rameshbabu
  0 siblings, 0 replies; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-11 20:35 UTC (permalink / raw)
  To: Vadim Fedorenko
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Richard Cochran, Jacob Keller,
	Jonathan Lemon

On Thu, 11 May, 2023 12:12:20 +0100 Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:
> On 10/05/2023 21:53, Rahul Rameshbabu wrote:
>> Add a function that advertises a maximum offset of zero supported by
>> ptp_clock_info .adjphase in the OCP null ptp implementation.
>>
>> Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Jonathan Lemon <jonathan.lemon@gmail.com>
>
> Neither Jonathan, nor myself are in the actual Cc list.
>

Accidentally had --suppress-cc in the invocation when this patch series
was sent and figured it would not be worth a RESEND since a v2 would
likely be needed. Will check that the v2 properly has the needed CCs for
each patch in the series.

-- Rahul Rameshbabu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used
  2023-05-11 20:20     ` Rahul Rameshbabu
@ 2023-05-12  0:51       ` Richard Cochran
  2023-05-22 17:03         ` Rahul Rameshbabu
  0 siblings, 1 reply; 24+ messages in thread
From: Richard Cochran @ 2023-05-12  0:51 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Thu, May 11, 2023 at 01:20:45PM -0700, Rahul Rameshbabu wrote:

> If the PHC does not restore the frequency, won't the value cached in the
> ptp stack in the kernel become inaccurate compared to the frequency
> change induced by the '.adjphase' call?

If the HW implements a PI controller, and if it has converged, then
the current frequency will be close to the remote time server's.

> This concern is why I added this
> clause in the documentation. Let me know if my understanding is off with
> regards to this. I think we had a similar conversation on this
> previously in the mailing list.
> 
> https://lore.kernel.org/netdev/Y88L6EPtgvW4tSA+@hoboy.vegasvil.org/

I guess it depends on the HW algorithm and the situation.  But I don't
think there is a "rule" that always gets the best result.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used
  2023-05-12  0:51       ` Richard Cochran
@ 2023-05-22 17:03         ` Rahul Rameshbabu
  2023-05-22 20:07           ` Richard Cochran
  0 siblings, 1 reply; 24+ messages in thread
From: Rahul Rameshbabu @ 2023-05-22 17:03 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

Hi Richard,

I have a v2 prepared. I have just one question left before sending that
it out.

On Thu, 11 May, 2023 17:51:47 -0700 Richard Cochran <richardcochran@gmail.com> wrote:
> On Thu, May 11, 2023 at 01:20:45PM -0700, Rahul Rameshbabu wrote:
>
>> If the PHC does not restore the frequency, won't the value cached in the
>> ptp stack in the kernel become inaccurate compared to the frequency
>> change induced by the '.adjphase' call?
>
> If the HW implements a PI controller, and if it has converged, then
> the current frequency will be close to the remote time server's.

This point makes sense to me. However, I have a concern about the case
where the linuxptp servo has not had a chance to make a single frequency
adjustment (0 ppb) and .adjphase/LOCKED_STABLE state is initially
called/reached. After converging, the frequency will be close to the
remote time's server's frequency, but that frequency will likely not be
0 ppb. If .adjfine had been called previously, the difference between
the remote time server's frequency and the cached frequency in the ptp
stack would likely be significantly closer. That said, do you think it
makes sense to have some kind of API that gives information about the
in-HW controller such as the frequency offset it operated? Or maybe in
general an API in the future for introspecting the state of this in-HW
servo?

>
>> This concern is why I added this
>> clause in the documentation. Let me know if my understanding is off with
>> regards to this. I think we had a similar conversation on this
>> previously in the mailing list.
>> 
>> https://lore.kernel.org/netdev/Y88L6EPtgvW4tSA+@hoboy.vegasvil.org/
>
> I guess it depends on the HW algorithm and the situation.  But I don't
> think there is a "rule" that always gets the best result.

Agreed. I do not think enforcing the PHC to restore the frequency to the
value before .adjphase is called would be helpful. This preserves the
integrity of the cached value in the kernel stack, but that is not
helpful since we can potentially see an initial growing error in the
offset between the remote server's time and the PHC's time after making
this frequency reversion.

>
> Thanks,
> Richard

-- Rahul Rameshbabu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used
  2023-05-22 17:03         ` Rahul Rameshbabu
@ 2023-05-22 20:07           ` Richard Cochran
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Cochran @ 2023-05-22 20:07 UTC (permalink / raw)
  To: Rahul Rameshbabu
  Cc: netdev, Saeed Mahameed, Gal Pressman, Tariq Toukan,
	David S. Miller, Jakub Kicinski, Jacob Keller

On Mon, May 22, 2023 at 10:03:23AM -0700, Rahul Rameshbabu wrote:

> This point makes sense to me. However, I have a concern about the case
> where the linuxptp servo has not had a chance to make a single frequency
> adjustment (0 ppb) and .adjphase/LOCKED_STABLE state is initially
> called/reached. After converging, the frequency will be close to the
> remote time's server's frequency, but that frequency will likely not be
> 0 ppb. If .adjfine had been called previously, the difference between
> the remote time server's frequency and the cached frequency in the ptp
> stack would likely be significantly closer. That said, do you think it
> makes sense to have some kind of API that gives information about the
> in-HW controller such as the frequency offset it operated? Or maybe in
> general an API in the future for introspecting the state of this in-HW
> servo?

The whole point of hardware vendors doing this is to hide their
implementation from prying eyes.  So they are not going to provide
introspection at all.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2023-05-22 20:07 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-10 20:52 [PATCH net-next 0/9] ptp .adjphase cleanups Rahul Rameshbabu
2023-05-10 20:52 ` [PATCH net-next 1/9] ptp: Clarify ptp_clock_info .adjphase expects an internal servo to be used Rahul Rameshbabu
2023-05-11  2:09   ` Richard Cochran
2023-05-11 20:20     ` Rahul Rameshbabu
2023-05-12  0:51       ` Richard Cochran
2023-05-22 17:03         ` Rahul Rameshbabu
2023-05-22 20:07           ` Richard Cochran
2023-05-10 20:52 ` [PATCH net-next 2/9] docs: ptp.rst: Add information about NVIDIA Mellanox devices Rahul Rameshbabu
2023-05-11  2:10   ` Richard Cochran
2023-05-10 20:53 ` [PATCH net-next 3/9] testptp: Remove magic numbers related to nanosecond to second conversion Rahul Rameshbabu
2023-05-11  2:14   ` Richard Cochran
2023-05-10 20:53 ` [PATCH net-next 4/9] testptp: Add support for testing ptp_clock_info .adjphase callback Rahul Rameshbabu
2023-05-11  2:15   ` Richard Cochran
2023-05-10 20:53 ` [PATCH net-next 5/9] ptp: Add .getmaxphase callback to ptp_clock_info Rahul Rameshbabu
2023-05-11  2:20   ` Richard Cochran
2023-05-10 20:53 ` [PATCH net-next 6/9] net/mlx5: Add .getmaxphase ptp_clock_info callback Rahul Rameshbabu
2023-05-11  2:20   ` Richard Cochran
2023-05-10 20:53 ` [PATCH net-next 7/9] ptp: ptp_clockmatrix: " Rahul Rameshbabu
2023-05-10 20:53 ` [PATCH net-next 8/9] ptp: idt82p33: " Rahul Rameshbabu
2023-05-10 20:53 ` [PATCH net-next 9/9] ptp: ocp: " Rahul Rameshbabu
2023-05-11 11:12   ` Vadim Fedorenko
2023-05-11 20:35     ` Rahul Rameshbabu
2023-05-11  2:23 ` [PATCH net-next 0/9] ptp .adjphase cleanups Richard Cochran
2023-05-11 20:26   ` Rahul Rameshbabu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.