[PATCH v2 net-next 0/3] ptp: Add adjust phase to support phase offset.

[PATCH v2 net-next 0/3] ptp: Add adjust phase to support phase offset.

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

This series adds adjust phase to the PTP Hardware Clock device interface.

Some PTP hardware clocks have a write phase mode that has
a built-in hardware filtering capability.  The write phase mode
utilizes a phase offset control word instead of a frequency offset 
control word.  Add adjust phase function to take advantage of this
capability.

Changes since v1:
- As suggested by Richard Cochran:
  1. ops->adjphase is new so need to check for non-null function pointer.
  2. Kernel coding style uses lower_case_underscores.
  3. Use existing PTP clock API for delayed worker.

Vincent Cheng (3):
  ptp: Add adjphase function to support phase offset control.
  ptp: Add adjust_phase to ptp_clock_caps capability.
  ptp: ptp_clockmatrix: Add adjphase() to support PHC write phase mode.

 drivers/ptp/ptp_chardev.c             |  1 +
 drivers/ptp/ptp_clock.c               |  3 ++
 drivers/ptp/ptp_clockmatrix.c         | 92 +++++++++++++++++++++++++++++++++++
 drivers/ptp/ptp_clockmatrix.h         |  8 ++-
 include/linux/ptp_clock_kernel.h      |  6 ++-
 include/uapi/linux/ptp_clock.h        |  4 +-
 tools/testing/selftests/ptp/testptp.c |  6 ++-
 7 files changed, 114 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v2 net-next 1/3] ptp: Add adjphase function to support phase offset control.

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

Adds adjust phase function to take advantage of a PHC
clock's hardware filtering capability that uses phase offset
control word instead of frequency offset control word.

Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
---
 drivers/ptp/ptp_clock.c          | 3 +++
 include/linux/ptp_clock_kernel.h | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index acabbe7..fc984a8 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -146,6 +146,9 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 		else
 			err = ops->adjfreq(ops, ppb);
 		ptp->dialed_frequency = tx->freq;
+	} else if (tx->modes & ADJ_OFFSET) {
+		if (ops->adjphase)
+			err = ops->adjphase(ops, tx->offset);
 	} else if (tx->modes == 0) {
 		tx->freq = ptp->dialed_frequency;
 		err = 0;
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 121a7ed..31144d9 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -36,7 +36,7 @@ struct ptp_system_timestamp {
 };
 
 /**
- * struct ptp_clock_info - decribes a PTP hardware clock
+ * struct ptp_clock_info - describes a PTP hardware clock
  *
  * @owner:     The clock driver should set to THIS_MODULE.
  * @name:      A short "friendly name" to identify the clock and to
@@ -65,6 +65,9 @@ struct ptp_system_timestamp {
  *            parameter delta: Desired frequency offset from nominal frequency
  *            in parts per billion
  *
+ * @adjphase:  Adjusts the phase offset of the hardware clock.
+ *             parameter delta: Desired change in nanoseconds.
+ *
  * @adjtime:  Shifts the time of the hardware clock.
  *            parameter delta: Desired change in nanoseconds.
  *
@@ -128,6 +131,7 @@ struct ptp_clock_info {
 	struct ptp_pin_desc *pin_config;
 	int (*adjfine)(struct ptp_clock_info *ptp, long scaled_ppm);
 	int (*adjfreq)(struct ptp_clock_info *ptp, s32 delta);
+	int (*adjphase)(struct ptp_clock_info *ptp, s32 phase);
 	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,
-- 
2.7.4

[PATCH v2 net-next 2/3] ptp: Add adjust_phase to ptp_clock_caps capability.

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

Add adjust_phase to ptp_clock_caps capability to allow
user to query if a PHC driver supports adjust phase with
ioctl PTP_CLOCK_GETCAPS command.

Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
Reviewed-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_chardev.c             | 1 +
 include/uapi/linux/ptp_clock.h        | 4 +++-
 tools/testing/selftests/ptp/testptp.c | 6 ++++--
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 93d574f..375cd6e 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -136,6 +136,7 @@ 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;
 		if (copy_to_user((void __user *)arg, &caps, sizeof(caps)))
 			err = -EFAULT;
 		break;
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 9dc9d00..ff070aa 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -89,7 +89,9 @@ struct ptp_clock_caps {
 	int n_pins;    /* Number of input/output pins. */
 	/* Whether the clock supports precise system-device cross timestamps */
 	int cross_timestamping;
-	int rsv[13];   /* Reserved for future use. */
+	/* Whether the clock supports adjust phase */
+	int adjust_phase;
+	int rsv[12];   /* 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 c0dd102..da7a9dd 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -269,14 +269,16 @@ int main(int argc, char *argv[])
 			       "  %d programmable periodic signals\n"
 			       "  %d pulse per second\n"
 			       "  %d programmable pins\n"
-			       "  %d cross timestamping\n",
+			       "  %d cross timestamping\n"
+			       "  %d adjust_phase\n",
 			       caps.max_adj,
 			       caps.n_alarm,
 			       caps.n_ext_ts,
 			       caps.n_per_out,
 			       caps.pps,
 			       caps.n_pins,
-			       caps.cross_timestamping);
+			       caps.cross_timestamping,
+			       caps.adjust_phase);
 		}
 	}
 
-- 
2.7.4

[PATCH v2 net-next 3/3] ptp: ptp_clockmatrix: Add adjphase() to support PHC write phase mode.

[vincent.cheng.xh]

From: Vincent Cheng <vincent.cheng.xh@renesas.com>

Add idtcm_adjphase() to support PHC write phase mode.

Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
---
 drivers/ptp/ptp_clockmatrix.c | 92 +++++++++++++++++++++++++++++++++++++++++++
 drivers/ptp/ptp_clockmatrix.h |  8 +++-
 2 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
index a3f6088..ceb6bc5 100644
--- a/drivers/ptp/ptp_clockmatrix.c
+++ b/drivers/ptp/ptp_clockmatrix.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/delay.h>
+#include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/timekeeping.h>
 
@@ -24,6 +25,16 @@ MODULE_LICENSE("GPL");
 
 #define SETTIME_CORRECTION (0)
 
+static long set_write_phase_ready(struct ptp_clock_info *ptp)
+{
+	struct idtcm_channel *channel =
+		container_of(ptp, struct idtcm_channel, caps);
+
+	channel->write_phase_ready = 1;
+
+	return 0;
+}
+
 static int char_array_to_timespec(u8 *buf,
 				  u8 count,
 				  struct timespec64 *ts)
@@ -871,6 +882,64 @@ static int idtcm_set_pll_mode(struct idtcm_channel *channel,
 
 /* PTP Hardware Clock interface */
 
+/**
+ * @brief Maximum absolute value for write phase offset in picoseconds
+ *
+ * Destination signed register is 32-bit register in resolution of 50ps
+ *
+ * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350
+ */
+static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns)
+{
+	struct idtcm *idtcm = channel->idtcm;
+
+	int err;
+	u8 i;
+	u8 buf[4] = {0};
+	s32 phase_50ps;
+	s64 offset_ps;
+
+	if (channel->pll_mode != PLL_MODE_WRITE_PHASE) {
+
+		err = idtcm_set_pll_mode(channel, PLL_MODE_WRITE_PHASE);
+
+		if (err)
+			return err;
+
+		channel->write_phase_ready = 0;
+
+		ptp_schedule_worker(channel->ptp_clock,
+				    msecs_to_jiffies(WR_PHASE_SETUP_MS));
+	}
+
+	if (!channel->write_phase_ready)
+		delta_ns = 0;
+
+	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_ROUND_CLOSEST(div64_s64(offset_ps, 50), 1);
+
+	for (i = 0; i < 4; i++) {
+		buf[i] = phase_50ps & 0xff;
+		phase_50ps >>= 8;
+	}
+
+	err = idtcm_write(idtcm, channel->dpll_phase, DPLL_WR_PHASE,
+			  buf, sizeof(buf));
+
+	return err;
+}
+
 static int idtcm_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
 {
 	struct idtcm_channel *channel =
@@ -977,6 +1046,24 @@ static int idtcm_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	return err;
 }
 
+static int idtcm_adjphase(struct ptp_clock_info *ptp, s32 delta)
+{
+	struct idtcm_channel *channel =
+		container_of(ptp, struct idtcm_channel, caps);
+
+	struct idtcm *idtcm = channel->idtcm;
+
+	int err;
+
+	mutex_lock(&idtcm->reg_lock);
+
+	err = _idtcm_adjphase(channel, delta);
+
+	mutex_unlock(&idtcm->reg_lock);
+
+	return err;
+}
+
 static int idtcm_enable(struct ptp_clock_info *ptp,
 			struct ptp_clock_request *rq, int on)
 {
@@ -1055,13 +1142,16 @@ static const struct ptp_clock_info idtcm_caps = {
 	.owner		= THIS_MODULE,
 	.max_adj	= 244000,
 	.n_per_out	= 1,
+	.adjphase	= &idtcm_adjphase,
 	.adjfreq	= &idtcm_adjfreq,
 	.adjtime	= &idtcm_adjtime,
 	.gettime64	= &idtcm_gettime,
 	.settime64	= &idtcm_settime,
 	.enable		= &idtcm_enable,
+	.do_aux_work	= &set_write_phase_ready,
 };
 
+
 static int idtcm_enable_channel(struct idtcm *idtcm, u32 index)
 {
 	struct idtcm_channel *channel;
@@ -1146,6 +1236,8 @@ static int idtcm_enable_channel(struct idtcm *idtcm, u32 index)
 	if (!channel->ptp_clock)
 		return -ENOTSUPP;
 
+	channel->write_phase_ready = 0;
+
 	dev_info(&idtcm->client->dev, "PLL%d registered as ptp%d\n",
 		 index, channel->ptp_clock->index);
 
diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
index 6c1f93a..3de0eb7 100644
--- a/drivers/ptp/ptp_clockmatrix.h
+++ b/drivers/ptp/ptp_clockmatrix.h
@@ -15,6 +15,8 @@
 #define FW_FILENAME	"idtcm.bin"
 #define MAX_PHC_PLL	4
 
+#define MAX_ABS_WRITE_PHASE_PICOSECONDS (107374182350LL)
+
 #define PLL_MASK_ADDR		(0xFFA5)
 #define DEFAULT_PLL_MASK	(0x04)
 
@@ -33,8 +35,9 @@
 
 #define POST_SM_RESET_DELAY_MS		(3000)
 #define PHASE_PULL_IN_THRESHOLD_NS	(150000)
-#define TOD_WRITE_OVERHEAD_COUNT_MAX    (5)
-#define TOD_BYTE_COUNT                  (11)
+#define TOD_WRITE_OVERHEAD_COUNT_MAX	(5)
+#define TOD_BYTE_COUNT			(11)
+#define WR_PHASE_SETUP_MS		(5000)
 
 /* Values of DPLL_N.DPLL_MODE.PLL_MODE */
 enum pll_mode {
@@ -77,6 +80,7 @@ struct idtcm_channel {
 	u16			hw_dpll_n;
 	enum pll_mode		pll_mode;
 	u16			output_mask;
+	int			write_phase_ready;
 };
 
 struct idtcm {
-- 
2.7.4

Re: [PATCH v2 net-next 1/3] ptp: Add adjphase function to support phase offset control.

[Richard Cochran]

On Fri, May 01, 2020 at 11:35:36PM -0400, vincent.cheng.xh@renesas.com wrote:
> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
> 
> Adds adjust phase function to take advantage of a PHC
> clock's hardware filtering capability that uses phase offset
> control word instead of frequency offset control word.
> 
> Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>

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

Re: [PATCH v2 net-next 3/3] ptp: ptp_clockmatrix: Add adjphase() to support PHC write phase mode.

[Richard Cochran]

On Fri, May 01, 2020 at 11:35:38PM -0400, vincent.cheng.xh@renesas.com wrote:
> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
> 
> Add idtcm_adjphase() to support PHC write phase mode.
> 
> Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>

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

Re: [PATCH v2 net-next 0/3] ptp: Add adjust phase to support phase offset.

[David Miller]

From: <vincent.cheng.xh@renesas.com>
Date: Fri, 1 May 2020 23:35:35 -0400

> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
> 
> This series adds adjust phase to the PTP Hardware Clock device interface.
> 
> Some PTP hardware clocks have a write phase mode that has
> a built-in hardware filtering capability.  The write phase mode
> utilizes a phase offset control word instead of a frequency offset 
> control word.  Add adjust phase function to take advantage of this
> capability.
> 
> Changes since v1:
> - As suggested by Richard Cochran:
>   1. ops->adjphase is new so need to check for non-null function pointer.
>   2. Kernel coding style uses lower_case_underscores.
>   3. Use existing PTP clock API for delayed worker.

Series applied.

Re: [PATCH v2 net-next 1/3] ptp: Add adjphase function to support phase offset control.

[Aya Levin]

On 5/2/2020 6:35 AM, vincent.cheng.xh@renesas.com wrote:
> From: Vincent Cheng <vincent.cheng.xh@renesas.com>
> 
> Adds adjust phase function to take advantage of a PHC
> clock's hardware filtering capability that uses phase offset
> control word instead of frequency offset control word.
> 
> Signed-off-by: Vincent Cheng <vincent.cheng.xh@renesas.com>
> ---
>   drivers/ptp/ptp_clock.c          | 3 +++
>   include/linux/ptp_clock_kernel.h | 6 +++++-
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index acabbe7..fc984a8 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -146,6 +146,9 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
>   		else
>   			err = ops->adjfreq(ops, ppb);
>   		ptp->dialed_frequency = tx->freq;
> +	} else if (tx->modes & ADJ_OFFSET) {
> +		if (ops->adjphase)
> +			err = ops->adjphase(ops, tx->offset);
>   	} else if (tx->modes == 0) {
>   		tx->freq = ptp->dialed_frequency;
>   		err = 0;
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 121a7ed..31144d9 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -36,7 +36,7 @@ struct ptp_system_timestamp {
>   };
>   
>   /**
> - * struct ptp_clock_info - decribes a PTP hardware clock
> + * struct ptp_clock_info - describes a PTP hardware clock
>    *
>    * @owner:     The clock driver should set to THIS_MODULE.
>    * @name:      A short "friendly name" to identify the clock and to
> @@ -65,6 +65,9 @@ struct ptp_system_timestamp {
>    *            parameter delta: Desired frequency offset from nominal frequency
>    *            in parts per billion
>    *
> + * @adjphase:  Adjusts the phase offset of the hardware clock.
> + *             parameter delta: Desired change in nanoseconds.
> + *
>    * @adjtime:  Shifts the time of the hardware clock.
>    *            parameter delta: Desired change in nanoseconds.
>    *
> @@ -128,6 +131,7 @@ struct ptp_clock_info {
>   	struct ptp_pin_desc *pin_config;
>   	int (*adjfine)(struct ptp_clock_info *ptp, long scaled_ppm);
>   	int (*adjfreq)(struct ptp_clock_info *ptp, s32 delta);
> +	int (*adjphase)(struct ptp_clock_info *ptp, s32 phase);
Hi,

Please explain the difference in the output between adjphase and 
adjtime. I'd expect both to add delta to current time. Am I missing 
something?

Thanks,
Aya
>   	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,

Re: [PATCH v2 net-next 1/3] ptp: Add adjphase function to support phase offset control.

[Vincent Cheng]

Hi Aya,

>>+ * @adjphase:  Adjusts the phase offset of the hardware clock.
>>+ *             parameter delta: Desired change in nanoseconds.
>>+ *
>>   * @adjtime:  Shifts the time of the hardware clock.
>>   *            parameter delta: Desired change in nanoseconds.
>>   *
>>@@ -128,6 +131,7 @@ struct ptp_clock_info {
>>  	struct ptp_pin_desc *pin_config;
>>  	int (*adjfine)(struct ptp_clock_info *ptp, long scaled_ppm);
>>  	int (*adjfreq)(struct ptp_clock_info *ptp, s32 delta);
>>+	int (*adjphase)(struct ptp_clock_info *ptp, s32 phase);
>Hi,
>
>Please explain the difference in the output between adjphase and adjtime. I'd
>expect both to add delta to current time. Am I missing something?

Yes, both add delta to the current time and the 1 PPS should arrive at the same location.

adjtime modifies HW counter with a value to move the 1 PPS abruptly to new location.
adjphase modifies the frequency to quickly nudge the 1 PPS to new location and also includes a HW filter to smooth out the adjustments and fine tune frequency.

Continuous small offset adjustments using adjtime, likley see sudden shifts of the 1 PPS.  The 1 PPS probably disappears and re-appears.
Continuous small offset adjustments using adjphase, should see continuous 1 PPS.

adjtime is good for large offset corrections
adjphase is good for small offset corrections to allow HW filter to control the frequency instead of relying on SW filter.

On interruption of timestamps, adjphase HW can hold the frequency steady better since it has history of corrections.

ptp4l will switch to adjphase only if servo_offset_threshold and servo_num_offset_values conditions are met, ie. when offsets are stabalized below servo_offset_threshold.

adjphase is most useful for scnearios with minimal PDV.

Hope that helps.

Thanks,
Vincent

>
>Thanks, Aya
>>  	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,