[PATCH RFC] mlx4_en: Add PTP hardware clock

[PATCH RFC] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

From: Shawn Bohrer <sbohrer@rgmadvisors.com>

This adds a PHC to the mlx4_en driver.  This is largely based off of the
e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
very similar.  One thing I am unsure about is that in the e1000e driver
they protect the timecounter code with a spinlock because the hardware
reports the time in two 32bit registers.  The Mellanox code looks
similar however the existing timecounter code in the mlx4_en driver
wasn't protected with a lock so I left the lock out here as well.

Additionally here the mlx4_en_phc_adjfreq() method simply returns
-EOPNOTSUPP because I don't have the relevant hardware documentation on
how to do this.  I'm hoping one of the Mellanox developers can either
provide this documentation or provide a patch to implement that
function.

This is minimally tested at this point but Documentation/ptp/testptp
appears to work on a Mellanox ConnectX-3 card.

Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  156 +++++++++++++++++++++-
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
 drivers/net/ethernet/mellanox/mlx4/en_main.c    |   11 +-
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    4 +
 4 files changed, 162 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
index fd64410..4405b90 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
@@ -111,10 +111,142 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
 	hwts->hwtstamp = ns_to_ktime(nsec);
 }
 
+/**
+ * mlx4_en_remove_timestamp - disable PTP device
+ * @mdev: board private structure
+ *
+ * Stop the PTP support.
+ **/
+void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
+{
+	if (mdev->ptp_clock) {
+		ptp_clock_unregister(mdev->ptp_clock);
+		mdev->ptp_clock = NULL;
+		mlx4_info(mdev, "removed PHC\n");
+	}
+}
+
+void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
+{
+	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
+					      mdev->overflow_period);
+
+	if (timeout) {
+		timecounter_read(&mdev->clock);
+		mdev->last_overflow_check = jiffies;
+	}
+}
+
+/**
+ * mlx4_en_phc_adjfreq - adjust the frequency of the hardware clock
+ * @ptp: ptp clock structure
+ * @delta: Desired frequency change in parts per billion
+ *
+ * Adjust the frequency of the PHC cycle counter by the indicated delta from
+ * the base frequency.
+ **/
+static int mlx4_en_phc_adjfreq(struct ptp_clock_info __always_unused *ptp,
+			       s32 __always_unused delta)
+{
+	return -EOPNOTSUPP;
+}
+
+/**
+ * mlx4_en_phc_adjtime - Shift the time of the hardware clock
+ * @ptp: ptp clock structure
+ * @delta: Desired change in nanoseconds
+ *
+ * Adjust the timer by resetting the timecounter structure.
+ **/
+static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	s64 now;
+
+	now = timecounter_read(&mdev->clock);
+	now += delta;
+	timecounter_init(&mdev->clock, &mdev->cycles, now);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_gettime - Reads the current time from the hardware clock
+ * @ptp: ptp clock structure
+ * @ts: timespec structure to hold the current time value
+ *
+ * Read the timecounter and return the correct value in ns after converting
+ * it into a struct timespec.
+ **/
+static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	u32 remainder;
+	u64 ns = timecounter_read(&mdev->clock);
+
+	ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
+	ts->tv_nsec = remainder;
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_settime - Set the current time on the hardware clock
+ * @ptp: ptp clock structure
+ * @ts: timespec containing the new time for the cycle counter
+ *
+ * Reset the timecounter to use a new base value instead of the kernel
+ * wall timer value.
+ **/
+static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
+			       const struct timespec *ts)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	u64 ns = timespec_to_ns(ts);
+
+	/* reset the timecounter */
+	timecounter_init(&mdev->clock, &mdev->cycles, ns);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_enable - enable or disable an ancillary feature
+ * @ptp: ptp clock structure
+ * @request: Desired resource to enable or disable
+ * @on: Caller passes one to enable or zero to disable
+ *
+ * Enable (or disable) ancillary features of the PHC subsystem.
+ * Currently, no ancillary features are supported.
+ **/
+static int mlx4_en_phc_enable(struct ptp_clock_info __always_unused *ptp,
+			      struct ptp_clock_request __always_unused *request,
+			      int __always_unused on)
+{
+	return -EOPNOTSUPP;
+}
+
+static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
+	.owner		= THIS_MODULE,
+	.n_alarm	= 0,
+	.n_ext_ts	= 0,
+	.n_per_out	= 0,
+	.pps		= 0,
+	.adjfreq	= mlx4_en_phc_adjfreq,
+	.adjtime	= mlx4_en_phc_adjtime,
+	.gettime	= mlx4_en_phc_gettime,
+	.settime	= mlx4_en_phc_settime,
+	.enable		= mlx4_en_phc_enable,
+};
+
 void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 {
 	struct mlx4_dev *dev = mdev->dev;
 	u64 ns;
+	int i;
 
 	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
 	mdev->cycles.read = mlx4_en_read_clock;
@@ -137,15 +269,23 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 	ns = cyclecounter_cyc2ns(&mdev->cycles, mdev->cycles.mask);
 	do_div(ns, NSEC_PER_SEC / 2 / HZ);
 	mdev->overflow_period = ns;
-}
 
-void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
-{
-	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
-					      mdev->overflow_period);
+	/* Configure the PHC */
+	mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
+	mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH) {
+		snprintf(mdev->ptp_clock_info.name,
+			 sizeof(mdev->ptp_clock_info.name), "%pm",
+			 mdev->pndev[i]->perm_addr);
+		break;
+	}
 
-	if (timeout) {
-		timecounter_read(&mdev->clock);
-		mdev->last_overflow_check = jiffies;
+	mdev->ptp_clock = ptp_clock_register(&mdev->ptp_clock_info,
+					     &mdev->pdev->dev);
+	if (IS_ERR(mdev->ptp_clock)) {
+		mdev->ptp_clock = NULL;
+		mlx4_err(mdev, "ptp_clock_register failed\n");
+	} else {
+		mlx4_info(mdev, "registered PHC clock\n");
 	}
+
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 0596f9f..3e8d336 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1193,6 +1193,9 @@ static int mlx4_en_get_ts_info(struct net_device *dev,
 		info->rx_filters =
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_ALL);
+
+		if (mdev->ptp_clock)
+			info->phc_index = ptp_clock_index(mdev->ptp_clock);
 	}
 
 	return ret;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
index 0d087b0..51bed5d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
@@ -196,6 +196,9 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
 		if (mdev->pndev[i])
 			mlx4_en_destroy_netdev(mdev->pndev[i]);
 
+	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
+		mlx4_en_remove_timestamp(mdev);
+
 	flush_workqueue(mdev->workqueue);
 	destroy_workqueue(mdev->workqueue);
 	(void) mlx4_mr_free(dev, &mdev->mr);
@@ -264,10 +267,6 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
 	mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH)
 		mdev->port_cnt++;
 
-	/* Initialize time stamp mechanism */
-	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
-		mlx4_en_init_timestamp(mdev);
-
 	mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH) {
 		if (!dev->caps.comp_pool) {
 			mdev->profile.prof[i].rx_ring_num =
@@ -305,6 +304,10 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
 			mdev->pndev[i] = NULL;
 	}
 
+	/* Initialize time stamp mechanism */
+	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
+		mlx4_en_init_timestamp(mdev);
+
 	return mdev;
 
 err_mr:
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index f3758de..fc35801 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -45,6 +45,7 @@
 #include <linux/dcbnl.h>
 #endif
 #include <linux/cpu_rmap.h>
+#include <linux/ptp_clock_kernel.h>
 
 #include <linux/mlx4/device.h>
 #include <linux/mlx4/qp.h>
@@ -377,6 +378,8 @@ struct mlx4_en_dev {
 	struct timecounter	clock;
 	unsigned long		last_overflow_check;
 	unsigned long		overflow_period;
+	struct ptp_clock	*ptp_clock;
+	struct ptp_clock_info	ptp_clock_info;
 };
 
 
@@ -786,6 +789,7 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
 			    struct skb_shared_hwtstamps *hwts,
 			    u64 timestamp);
 void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev);
+void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev);
 int mlx4_en_timestamp_config(struct net_device *dev,
 			     int tx_type,
 			     int rx_filter);
-- 
1.7.7.6

Re: [PATCH RFC] mlx4_en: Add PTP hardware clock

[Richard Cochran]

On Wed, Dec 11, 2013 at 12:24:25PM -0600, Shawn Bohrer wrote:
> From: Shawn Bohrer <sbohrer@rgmadvisors.com>
> 
> This adds a PHC to the mlx4_en driver.  This is largely based off of the
> e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> very similar.  One thing I am unsure about is that in the e1000e driver
> they protect the timecounter code with a spinlock because the hardware
> reports the time in two 32bit registers.  The Mellanox code looks
> similar however the existing timecounter code in the mlx4_en driver
> wasn't protected with a lock so I left the lock out here as well.

Before, there were only three call sites,

grep -nH -e timecounter_ *
en_clock.c:108:	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
en_clock.c:131:	timecounter_init(&mdev->clock, &mdev->cycles,
en_clock.c:148:		timecounter_read(&mdev->clock);

and so perhaps the locking was unnecessary (but maybe not).
In any case, the code that you added definitely needs locks.

> Additionally here the mlx4_en_phc_adjfreq() method simply returns
> -EOPNOTSUPP because I don't have the relevant hardware documentation on
> how to do this.  I'm hoping one of the Mellanox developers can either
> provide this documentation or provide a patch to implement that
> function.

Since the code already uses timecounter_read to convert clock ticks
into nanoseconds, why not simply adjust the 'mult' as other drivers
do?

[ If the clock is easily adjustable in hardware, then, by all means,
  do it that way. ]
 
> This is minimally tested at this point but Documentation/ptp/testptp
> appears to work on a Mellanox ConnectX-3 card.

Once you have the adjustment in place, then you can try it with
linuxptp.

Thanks,
Richard

Re: [PATCH RFC] mlx4_en: Add PTP hardware clock

[Or Gerlitz]

On Wed, Dec 11, 2013 Shawn Bohrer <shawn.bohrer@gmail.com> wrote:

> @@ -264,10 +267,6 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
>         mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH)
>                 mdev->port_cnt++;
>
> -       /* Initialize time stamp mechanism */
> -       if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> -               mlx4_en_init_timestamp(mdev);
> -
>         mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH) {
>                 if (!dev->caps.comp_pool) {
>                         mdev->profile.prof[i].rx_ring_num =
> @@ -305,6 +304,10 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
>                         mdev->pndev[i] = NULL;
>         }
>
> +       /* Initialize time stamp mechanism */
> +       if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> +               mlx4_en_init_timestamp(mdev);
> +
>         return mdev;

so you are reverting here commit 1ec4864b1017 "net/mlx4_en: Fixed
crash when port type is changed" -- why?

Re: [PATCH RFC] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

On Wed, Dec 11, 2013 at 10:47:51PM +0200, Or Gerlitz wrote:
> On Wed, Dec 11, 2013 Shawn Bohrer <shawn.bohrer@gmail.com> wrote:
> 
> > @@ -264,10 +267,6 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
> >         mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH)
> >                 mdev->port_cnt++;
> >
> > -       /* Initialize time stamp mechanism */
> > -       if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> > -               mlx4_en_init_timestamp(mdev);
> > -
> >         mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH) {
> >                 if (!dev->caps.comp_pool) {
> >                         mdev->profile.prof[i].rx_ring_num =
> > @@ -305,6 +304,10 @@ static void *mlx4_en_add(struct mlx4_dev *dev)
> >                         mdev->pndev[i] = NULL;
> >         }
> >
> > +       /* Initialize time stamp mechanism */
> > +       if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> > +               mlx4_en_init_timestamp(mdev);
> > +
> >         return mdev;
> 
> so you are reverting here commit 1ec4864b1017 "net/mlx4_en: Fixed
> crash when port type is changed" -- why?

For stupid reasons.  I'm using mdev->pndev[i]->perm_addr to initialize
the ptp_clock_info.name field in mlx4_en_init_timestamp().  I'll look
around and see if I can find something better to use and put this
back.

--
Shawn

Re: [PATCH RFC] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

On Wed, Dec 11, 2013 at 07:54:39PM +0100, Richard Cochran wrote:
> On Wed, Dec 11, 2013 at 12:24:25PM -0600, Shawn Bohrer wrote:
> > From: Shawn Bohrer <sbohrer@rgmadvisors.com>
> > 
> > This adds a PHC to the mlx4_en driver.  This is largely based off of the
> > e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> > very similar.  One thing I am unsure about is that in the e1000e driver
> > they protect the timecounter code with a spinlock because the hardware
> > reports the time in two 32bit registers.  The Mellanox code looks
> > similar however the existing timecounter code in the mlx4_en driver
> > wasn't protected with a lock so I left the lock out here as well.
> 
> Before, there were only three call sites,
> 
> grep -nH -e timecounter_ *
> en_clock.c:108:	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
> en_clock.c:131:	timecounter_init(&mdev->clock, &mdev->cycles,
> en_clock.c:148:		timecounter_read(&mdev->clock);
> 
> and so perhaps the locking was unnecessary (but maybe not).
> In any case, the code that you added definitely needs locks.
> 

Yeah, that looked sketchy to me.  I've added the locks.

> > Additionally here the mlx4_en_phc_adjfreq() method simply returns
> > -EOPNOTSUPP because I don't have the relevant hardware documentation on
> > how to do this.  I'm hoping one of the Mellanox developers can either
> > provide this documentation or provide a patch to implement that
> > function.
> 
> Since the code already uses timecounter_read to convert clock ticks
> into nanoseconds, why not simply adjust the 'mult' as other drivers
> do?
>
> [ If the clock is easily adjustable in hardware, then, by all means,
>   do it that way. ]

Awesome, I totally missed this.  I've updated my code to do this for
now and if there is a better way the Mellanox guys can chime in.

>  
> > This is minimally tested at this point but Documentation/ptp/testptp
> > appears to work on a Mellanox ConnectX-3 card.
> 
> Once you have the adjustment in place, then you can try it with
> linuxptp.

Yep, that is the goal of this exercise.  I should have a v2 patch with
these changes after I do some more testing.

Thanks,
Shawn

Re: [PATCH RFC] mlx4_en: Add PTP hardware clock

[Ben Hutchings]

On Wed, 2013-12-11 at 12:24 -0600, Shawn Bohrer wrote:
[...]
> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
[...]
> -void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> -{
> -	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> -					      mdev->overflow_period);
> +	/* Configure the PHC */
> +	mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
> +	mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_ETH) {
> +		snprintf(mdev->ptp_clock_info.name,
> +			 sizeof(mdev->ptp_clock_info.name), "%pm",
[...]

Name should be "mlx4", I think.

Does each function have an independent clock or is this shared between
functions?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH RFC] mlx4_en: Add PTP hardware clock

[Hadar Hen Zion]

On 12/11/2013 11:28 PM, Shawn Bohrer wrote:
> On Wed, Dec 11, 2013 at 07:54:39PM +0100, Richard Cochran wrote:
>> On Wed, Dec 11, 2013 at 12:24:25PM -0600, Shawn Bohrer wrote:
>>> From: Shawn Bohrer <sbohrer@rgmadvisors.com>
>>>
>>> This adds a PHC to the mlx4_en driver.  This is largely based off of the
>>> e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
>>> very similar.  One thing I am unsure about is that in the e1000e driver
>>> they protect the timecounter code with a spinlock because the hardware
>>> reports the time in two 32bit registers.  The Mellanox code looks
>>> similar however the existing timecounter code in the mlx4_en driver
>>> wasn't protected with a lock so I left the lock out here as well.
>>
>> Before, there were only three call sites,
>>
>> grep -nH -e timecounter_ *
>> en_clock.c:108:	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
>> en_clock.c:131:	timecounter_init(&mdev->clock, &mdev->cycles,
>> en_clock.c:148:		timecounter_read(&mdev->clock);
>>
>> and so perhaps the locking was unnecessary (but maybe not).
>> In any case, the code that you added definitely needs locks.
>>
>
> Yeah, that looked sketchy to me.  I've added the locks.
>
>>> Additionally here the mlx4_en_phc_adjfreq() method simply returns
>>> -EOPNOTSUPP because I don't have the relevant hardware documentation on
>>> how to do this.  I'm hoping one of the Mellanox developers can either
>>> provide this documentation or provide a patch to implement that
>>> function.
>>
>> Since the code already uses timecounter_read to convert clock ticks
>> into nanoseconds, why not simply adjust the 'mult' as other drivers
>> do?
>>
>> [ If the clock is easily adjustable in hardware, then, by all means,
>>    do it that way. ]
>
> Awesome, I totally missed this.  I've updated my code to do this for
> now and if there is a better way the Mellanox guys can chime in.
>
>>
>>> This is minimally tested at this point but Documentation/ptp/testptp
>>> appears to work on a Mellanox ConnectX-3 card.
>>
>> Once you have the adjustment in place, then you can try it with
>> linuxptp.
>
> Yep, that is the goal of this exercise.  I should have a v2 patch with
> these changes after I do some more testing.
>
> Thanks,
> Shawn
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Hi Shawn,

I'm a SW developer working in Mellanox SW team which is responsible for 
upstream related tasks.
I already had this task assigned to me and I was planning to start 
working on it soon. Anyway now when you already started working on it I 
would like to work closely with you and complete the missing parts. 
Please send me your v2 with the relevant fixes as discussed in the above 
mails. I'll also do some testing.

Thanks,
Hadar

Re: [PATCH RFC] mlx4_en: Add PTP hardware clock

[Shawn Bohrer]

On Thu, Dec 12, 2013 at 11:50:07AM +0200, Hadar Hen Zion wrote:
> On 12/11/2013 11:28 PM, Shawn Bohrer wrote:
> >On Wed, Dec 11, 2013 at 07:54:39PM +0100, Richard Cochran wrote:
> >>On Wed, Dec 11, 2013 at 12:24:25PM -0600, Shawn Bohrer wrote:
> >>>From: Shawn Bohrer <sbohrer@rgmadvisors.com>
> >>>
> >>>This adds a PHC to the mlx4_en driver.  This is largely based off of the
> >>>e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> >>>very similar.  One thing I am unsure about is that in the e1000e driver
> >>>they protect the timecounter code with a spinlock because the hardware
> >>>reports the time in two 32bit registers.  The Mellanox code looks
> >>>similar however the existing timecounter code in the mlx4_en driver
> >>>wasn't protected with a lock so I left the lock out here as well.
> >>
> >>Before, there were only three call sites,
> >>
> >>grep -nH -e timecounter_ *
> >>en_clock.c:108:	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
> >>en_clock.c:131:	timecounter_init(&mdev->clock, &mdev->cycles,
> >>en_clock.c:148:		timecounter_read(&mdev->clock);
> >>
> >>and so perhaps the locking was unnecessary (but maybe not).
> >>In any case, the code that you added definitely needs locks.
> >>
> >
> >Yeah, that looked sketchy to me.  I've added the locks.
> >
> >>>Additionally here the mlx4_en_phc_adjfreq() method simply returns
> >>>-EOPNOTSUPP because I don't have the relevant hardware documentation on
> >>>how to do this.  I'm hoping one of the Mellanox developers can either
> >>>provide this documentation or provide a patch to implement that
> >>>function.
> >>
> >>Since the code already uses timecounter_read to convert clock ticks
> >>into nanoseconds, why not simply adjust the 'mult' as other drivers
> >>do?
> >>
> >>[ If the clock is easily adjustable in hardware, then, by all means,
> >>   do it that way. ]
> >
> >Awesome, I totally missed this.  I've updated my code to do this for
> >now and if there is a better way the Mellanox guys can chime in.
> >
> >>
> >>>This is minimally tested at this point but Documentation/ptp/testptp
> >>>appears to work on a Mellanox ConnectX-3 card.
> >>
> >>Once you have the adjustment in place, then you can try it with
> >>linuxptp.
> >
> >Yep, that is the goal of this exercise.  I should have a v2 patch with
> >these changes after I do some more testing.
> >
> >Thanks,
> >Shawn
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> Hi Shawn,
> 
> I'm a SW developer working in Mellanox SW team which is responsible
> for upstream related tasks.
> I already had this task assigned to me and I was planning to start
> working on it soon. Anyway now when you already started working on
> it I would like to work closely with you and complete the missing
> parts. Please send me your v2 with the relevant fixes as discussed
> in the above mails. I'll also do some testing.

Below is v2.  It compiles, and boots, but linuxptp doesn't work yet
and I haven't looked into why.  I think linuxptp is actually
complaining about SIOCSHWTSTAMP not working as expected, but again I
haven't really even started debugging yet.


This adds a PHC to the mlx4_en driver.  This is largely based off of the
e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
very similar.

Additionally here the mlx4_en_phc_adjfreq() method is implemented in
software, but it may be possible to do this in hardware.

This is minimally tested at this point but Documentation/ptp/testptp
appears to work on a Mellanox ConnectX-3 card.

Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
 drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
 4 files changed, 196 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
index fd64410..9b0d515 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
@@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
 			    struct skb_shared_hwtstamps *hwts,
 			    u64 timestamp)
 {
+	unsigned long flags;
 	u64 nsec;
 
+	spin_lock_irqsave(&mdev->clock_lock, flags);
 	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
 
 	memset(hwts, 0, sizeof(struct skb_shared_hwtstamps));
 	hwts->hwtstamp = ns_to_ktime(nsec);
 }
 
+/**
+ * mlx4_en_remove_timestamp - disable PTP device
+ * @mdev: board private structure
+ *
+ * Stop the PTP support.
+ **/
+void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
+{
+	if (mdev->ptp_clock) {
+		ptp_clock_unregister(mdev->ptp_clock);
+		mdev->ptp_clock = NULL;
+		mlx4_info(mdev, "removed PHC\n");
+	}
+}
+
+void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
+{
+	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
+					      mdev->overflow_period);
+	unsigned long flags;
+
+	if (timeout) {
+		spin_lock_irqsave(&mdev->clock_lock, flags);
+		timecounter_read(&mdev->clock);
+		spin_unlock_irqrestore(&mdev->clock_lock, flags);
+		mdev->last_overflow_check = jiffies;
+	}
+}
+
+/**
+ * mlx4_en_phc_adjfreq - adjust the frequency of the hardware clock
+ * @ptp: ptp clock structure
+ * @delta: Desired frequency change in parts per billion
+ *
+ * Adjust the frequency of the PHC cycle counter by the indicated delta from
+ * the base frequency.
+ **/
+static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
+{
+	u64 adj;
+	u32 diff, mult;
+	int neg_adj = 0;
+	unsigned long flags;
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+
+	if (delta < 0) {
+		neg_adj = 1;
+		delta = -delta;
+	}
+	mult = mdev->nominal_c_mult;
+	adj = mult;
+	adj *= delta;
+	diff = div_u64(adj, 1000000000ULL);
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	timecounter_read(&mdev->clock);
+	mdev->cycles.mult = neg_adj ? mult - diff : mult + diff;
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_adjtime - Shift the time of the hardware clock
+ * @ptp: ptp clock structure
+ * @delta: Desired change in nanoseconds
+ *
+ * Adjust the timer by resetting the timecounter structure.
+ **/
+static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	unsigned long flags;
+	s64 now;
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	now = timecounter_read(&mdev->clock);
+	now += delta;
+	timecounter_init(&mdev->clock, &mdev->cycles, now);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_gettime - Reads the current time from the hardware clock
+ * @ptp: ptp clock structure
+ * @ts: timespec structure to hold the current time value
+ *
+ * Read the timecounter and return the correct value in ns after converting
+ * it into a struct timespec.
+ **/
+static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	unsigned long flags;
+	u32 remainder;
+	u64 ns;
+
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	ns = timecounter_read(&mdev->clock);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
+	ts->tv_nsec = remainder;
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_settime - Set the current time on the hardware clock
+ * @ptp: ptp clock structure
+ * @ts: timespec containing the new time for the cycle counter
+ *
+ * Reset the timecounter to use a new base value instead of the kernel
+ * wall timer value.
+ **/
+static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
+			       const struct timespec *ts)
+{
+	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
+						ptp_clock_info);
+	u64 ns = timespec_to_ns(ts);
+	unsigned long flags;
+
+	/* reset the timecounter */
+	spin_lock_irqsave(&mdev->clock_lock, flags);
+	timecounter_init(&mdev->clock, &mdev->cycles, ns);
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
+
+	return 0;
+}
+
+/**
+ * mlx4_en_phc_enable - enable or disable an ancillary feature
+ * @ptp: ptp clock structure
+ * @request: Desired resource to enable or disable
+ * @on: Caller passes one to enable or zero to disable
+ *
+ * Enable (or disable) ancillary features of the PHC subsystem.
+ * Currently, no ancillary features are supported.
+ **/
+static int mlx4_en_phc_enable(struct ptp_clock_info __always_unused *ptp,
+			      struct ptp_clock_request __always_unused *request,
+			      int __always_unused on)
+{
+	return -EOPNOTSUPP;
+}
+
+static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
+	.owner		= THIS_MODULE,
+	.max_adj	= 100000000,
+	.n_alarm	= 0,
+	.n_ext_ts	= 0,
+	.n_per_out	= 0,
+	.pps		= 0,
+	.adjfreq	= mlx4_en_phc_adjfreq,
+	.adjtime	= mlx4_en_phc_adjtime,
+	.gettime	= mlx4_en_phc_gettime,
+	.settime	= mlx4_en_phc_settime,
+	.enable		= mlx4_en_phc_enable,
+};
+
 void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 {
 	struct mlx4_dev *dev = mdev->dev;
+	unsigned long flags;
 	u64 ns;
 
 	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
@@ -127,9 +297,12 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 	mdev->cycles.shift = 14;
 	mdev->cycles.mult =
 		clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
+	mdev->nominal_c_mult = mdev->cycles.mult;
 
+	spin_lock_irqsave(&mdev->clock_lock, flags);
 	timecounter_init(&mdev->clock, &mdev->cycles,
 			 ktime_to_ns(ktime_get_real()));
+	spin_unlock_irqrestore(&mdev->clock_lock, flags);
 
 	/* Calculate period in seconds to call the overflow watchdog - to make
 	 * sure counter is checked at least once every wrap around.
@@ -137,15 +310,18 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
 	ns = cyclecounter_cyc2ns(&mdev->cycles, mdev->cycles.mask);
 	do_div(ns, NSEC_PER_SEC / 2 / HZ);
 	mdev->overflow_period = ns;
-}
 
-void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
-{
-	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
-					      mdev->overflow_period);
+	/* Configure the PHC */
+	mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
+	snprintf(mdev->ptp_clock_info.name, 16, "mlx4 ptp");
 
-	if (timeout) {
-		timecounter_read(&mdev->clock);
-		mdev->last_overflow_check = jiffies;
+	mdev->ptp_clock = ptp_clock_register(&mdev->ptp_clock_info,
+					     &mdev->pdev->dev);
+	if (IS_ERR(mdev->ptp_clock)) {
+		mdev->ptp_clock = NULL;
+		mlx4_err(mdev, "ptp_clock_register failed\n");
+	} else {
+		mlx4_info(mdev, "registered PHC clock\n");
 	}
+
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 0596f9f..3e8d336 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1193,6 +1193,9 @@ static int mlx4_en_get_ts_info(struct net_device *dev,
 		info->rx_filters =
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_ALL);
+
+		if (mdev->ptp_clock)
+			info->phc_index = ptp_clock_index(mdev->ptp_clock);
 	}
 
 	return ret;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
index 0d087b0..6268057 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
@@ -196,6 +196,9 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
 		if (mdev->pndev[i])
 			mlx4_en_destroy_netdev(mdev->pndev[i]);
 
+	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
+		mlx4_en_remove_timestamp(mdev);
+
 	flush_workqueue(mdev->workqueue);
 	destroy_workqueue(mdev->workqueue);
 	(void) mlx4_mr_free(dev, &mdev->mr);
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index f3758de..2b4083f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -45,6 +45,7 @@
 #include <linux/dcbnl.h>
 #endif
 #include <linux/cpu_rmap.h>
+#include <linux/ptp_clock_kernel.h>
 
 #include <linux/mlx4/device.h>
 #include <linux/mlx4/qp.h>
@@ -373,10 +374,14 @@ struct mlx4_en_dev {
 	u32                     priv_pdn;
 	spinlock_t              uar_lock;
 	u8			mac_removed[MLX4_MAX_PORTS + 1];
+	spinlock_t		clock_lock;
+	u32			nominal_c_mult;
 	struct cyclecounter	cycles;
 	struct timecounter	clock;
 	unsigned long		last_overflow_check;
 	unsigned long		overflow_period;
+	struct ptp_clock	*ptp_clock;
+	struct ptp_clock_info	ptp_clock_info;
 };
 
 
@@ -786,6 +791,7 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
 			    struct skb_shared_hwtstamps *hwts,
 			    u64 timestamp);
 void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev);
+void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev);
 int mlx4_en_timestamp_config(struct net_device *dev,
 			     int tx_type,
 			     int rx_filter);
-- 
1.7.7.6

mlx4_en SIOCSHWTSTAMP cycles the link

[Shawn Bohrer]

On Thu, Dec 12, 2013 at 09:28:15AM -0600, Shawn Bohrer wrote:
> Below is v2.  It compiles, and boots, but linuxptp doesn't work yet
> and I haven't looked into why.  I think linuxptp is actually
> complaining about SIOCSHWTSTAMP not working as expected, but again I
> haven't really even started debugging yet.

So performing a SIOCSHWTSTAMP on the mlx4_en driver cycles the link:

[ 9463.081330] mlx4_en: p2p1: Changing Time Stamp configuration
[ 9463.084637] mlx4_en: p2p1:   frag:0 - size:1526 prefix:0 align:0 stride:1536
[ 9463.155533] mlx4_en: p2p1: Link Down
[ 9466.471169] mlx4_en: p2p1: Link Up

This breaks linuxptp (ptp4l) since it causes all of the already open
sockets to silently loose their multicast joins.  Thus ptp4l sits
there waiting for multicast PTP packets that aren't going to come.
I hacked around this in ptp4l by performing the ioctl once with the
hwstamp_ctl program from linuxptp and skipping the ioctl in ptp4l,
which works, but quite frankly this sucks.  I'd love to know if anyone
has any better ideas on how to make SIOCSHWTSTAMP usable.

After hacking around the SIOCSHWTSTAMP issue I've noticed that ptp4l
synchronizes the PHC, however there must be another bug somewhere
because it creates a 35 second offset from the PTP master.  When I use
ptp4l with software timestamps I only have a couple of microseconds
offset from the master.  I'll spend some more time debugging tomorrow,
but if anyone has any ideas why ptp4l is creating a 35 second offset
when using the PHC I'd love to hear them.  I did compare the
timestamps returned by the packets with a call to timecounter_read()
and those values are only ever 1-2us apart.  It is possible that there
is a bug somewhere in linuxptp but in general I like to assume my code
is broken.

--
Shawn

> This adds a PHC to the mlx4_en driver.  This is largely based off of the
> e1000e driver (drivers/net/ethernet/intel/e1000e/ptp.c) which seemed
> very similar.
> 
> Additionally here the mlx4_en_phc_adjfreq() method is implemented in
> software, but it may be possible to do this in hardware.
> 
> This is minimally tested at this point but Documentation/ptp/testptp
> appears to work on a Mellanox ConnectX-3 card.
> 
> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_clock.c   |  192 ++++++++++++++++++++++-
>  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |    3 +
>  drivers/net/ethernet/mellanox/mlx4/en_main.c    |    3 +
>  drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |    6 +
>  4 files changed, 196 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> index fd64410..9b0d515 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> @@ -103,17 +103,187 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
>  			    struct skb_shared_hwtstamps *hwts,
>  			    u64 timestamp)
>  {
> +	unsigned long flags;
>  	u64 nsec;
>  
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
>  	nsec = timecounter_cyc2time(&mdev->clock, timestamp);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
>  
>  	memset(hwts, 0, sizeof(struct skb_shared_hwtstamps));
>  	hwts->hwtstamp = ns_to_ktime(nsec);
>  }
>  
> +/**
> + * mlx4_en_remove_timestamp - disable PTP device
> + * @mdev: board private structure
> + *
> + * Stop the PTP support.
> + **/
> +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev)
> +{
> +	if (mdev->ptp_clock) {
> +		ptp_clock_unregister(mdev->ptp_clock);
> +		mdev->ptp_clock = NULL;
> +		mlx4_info(mdev, "removed PHC\n");
> +	}
> +}
> +
> +void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> +{
> +	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> +					      mdev->overflow_period);
> +	unsigned long flags;
> +
> +	if (timeout) {
> +		spin_lock_irqsave(&mdev->clock_lock, flags);
> +		timecounter_read(&mdev->clock);
> +		spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +		mdev->last_overflow_check = jiffies;
> +	}
> +}
> +
> +/**
> + * mlx4_en_phc_adjfreq - adjust the frequency of the hardware clock
> + * @ptp: ptp clock structure
> + * @delta: Desired frequency change in parts per billion
> + *
> + * Adjust the frequency of the PHC cycle counter by the indicated delta from
> + * the base frequency.
> + **/
> +static int mlx4_en_phc_adjfreq(struct ptp_clock_info *ptp, s32 delta)
> +{
> +	u64 adj;
> +	u32 diff, mult;
> +	int neg_adj = 0;
> +	unsigned long flags;
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +
> +	if (delta < 0) {
> +		neg_adj = 1;
> +		delta = -delta;
> +	}
> +	mult = mdev->nominal_c_mult;
> +	adj = mult;
> +	adj *= delta;
> +	diff = div_u64(adj, 1000000000ULL);
> +
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	timecounter_read(&mdev->clock);
> +	mdev->cycles.mult = neg_adj ? mult - diff : mult + diff;
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_adjtime - Shift the time of the hardware clock
> + * @ptp: ptp clock structure
> + * @delta: Desired change in nanoseconds
> + *
> + * Adjust the timer by resetting the timecounter structure.
> + **/
> +static int mlx4_en_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +	unsigned long flags;
> +	s64 now;
> +
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	now = timecounter_read(&mdev->clock);
> +	now += delta;
> +	timecounter_init(&mdev->clock, &mdev->cycles, now);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_gettime - Reads the current time from the hardware clock
> + * @ptp: ptp clock structure
> + * @ts: timespec structure to hold the current time value
> + *
> + * Read the timecounter and return the correct value in ns after converting
> + * it into a struct timespec.
> + **/
> +static int mlx4_en_phc_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
> +{
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +	unsigned long flags;
> +	u32 remainder;
> +	u64 ns;
> +
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	ns = timecounter_read(&mdev->clock);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	ts->tv_sec = div_u64_rem(ns, NSEC_PER_SEC, &remainder);
> +	ts->tv_nsec = remainder;
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_settime - Set the current time on the hardware clock
> + * @ptp: ptp clock structure
> + * @ts: timespec containing the new time for the cycle counter
> + *
> + * Reset the timecounter to use a new base value instead of the kernel
> + * wall timer value.
> + **/
> +static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
> +			       const struct timespec *ts)
> +{
> +	struct mlx4_en_dev *mdev = container_of(ptp, struct mlx4_en_dev,
> +						ptp_clock_info);
> +	u64 ns = timespec_to_ns(ts);
> +	unsigned long flags;
> +
> +	/* reset the timecounter */
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
> +	timecounter_init(&mdev->clock, &mdev->cycles, ns);
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
> +
> +	return 0;
> +}
> +
> +/**
> + * mlx4_en_phc_enable - enable or disable an ancillary feature
> + * @ptp: ptp clock structure
> + * @request: Desired resource to enable or disable
> + * @on: Caller passes one to enable or zero to disable
> + *
> + * Enable (or disable) ancillary features of the PHC subsystem.
> + * Currently, no ancillary features are supported.
> + **/
> +static int mlx4_en_phc_enable(struct ptp_clock_info __always_unused *ptp,
> +			      struct ptp_clock_request __always_unused *request,
> +			      int __always_unused on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct ptp_clock_info mlx4_en_ptp_clock_info = {
> +	.owner		= THIS_MODULE,
> +	.max_adj	= 100000000,
> +	.n_alarm	= 0,
> +	.n_ext_ts	= 0,
> +	.n_per_out	= 0,
> +	.pps		= 0,
> +	.adjfreq	= mlx4_en_phc_adjfreq,
> +	.adjtime	= mlx4_en_phc_adjtime,
> +	.gettime	= mlx4_en_phc_gettime,
> +	.settime	= mlx4_en_phc_settime,
> +	.enable		= mlx4_en_phc_enable,
> +};
> +
>  void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>  {
>  	struct mlx4_dev *dev = mdev->dev;
> +	unsigned long flags;
>  	u64 ns;
>  
>  	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
> @@ -127,9 +297,12 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>  	mdev->cycles.shift = 14;
>  	mdev->cycles.mult =
>  		clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
> +	mdev->nominal_c_mult = mdev->cycles.mult;
>  
> +	spin_lock_irqsave(&mdev->clock_lock, flags);
>  	timecounter_init(&mdev->clock, &mdev->cycles,
>  			 ktime_to_ns(ktime_get_real()));
> +	spin_unlock_irqrestore(&mdev->clock_lock, flags);
>  
>  	/* Calculate period in seconds to call the overflow watchdog - to make
>  	 * sure counter is checked at least once every wrap around.
> @@ -137,15 +310,18 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>  	ns = cyclecounter_cyc2ns(&mdev->cycles, mdev->cycles.mask);
>  	do_div(ns, NSEC_PER_SEC / 2 / HZ);
>  	mdev->overflow_period = ns;
> -}
>  
> -void mlx4_en_ptp_overflow_check(struct mlx4_en_dev *mdev)
> -{
> -	bool timeout = time_is_before_jiffies(mdev->last_overflow_check +
> -					      mdev->overflow_period);
> +	/* Configure the PHC */
> +	mdev->ptp_clock_info = mlx4_en_ptp_clock_info;
> +	snprintf(mdev->ptp_clock_info.name, 16, "mlx4 ptp");
>  
> -	if (timeout) {
> -		timecounter_read(&mdev->clock);
> -		mdev->last_overflow_check = jiffies;
> +	mdev->ptp_clock = ptp_clock_register(&mdev->ptp_clock_info,
> +					     &mdev->pdev->dev);
> +	if (IS_ERR(mdev->ptp_clock)) {
> +		mdev->ptp_clock = NULL;
> +		mlx4_err(mdev, "ptp_clock_register failed\n");
> +	} else {
> +		mlx4_info(mdev, "registered PHC clock\n");
>  	}
> +
>  }
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 0596f9f..3e8d336 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -1193,6 +1193,9 @@ static int mlx4_en_get_ts_info(struct net_device *dev,
>  		info->rx_filters =
>  			(1 << HWTSTAMP_FILTER_NONE) |
>  			(1 << HWTSTAMP_FILTER_ALL);
> +
> +		if (mdev->ptp_clock)
> +			info->phc_index = ptp_clock_index(mdev->ptp_clock);
>  	}
>  
>  	return ret;
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_main.c b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> index 0d087b0..6268057 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_main.c
> @@ -196,6 +196,9 @@ static void mlx4_en_remove(struct mlx4_dev *dev, void *endev_ptr)
>  		if (mdev->pndev[i])
>  			mlx4_en_destroy_netdev(mdev->pndev[i]);
>  
> +	if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_TS)
> +		mlx4_en_remove_timestamp(mdev);
> +
>  	flush_workqueue(mdev->workqueue);
>  	destroy_workqueue(mdev->workqueue);
>  	(void) mlx4_mr_free(dev, &mdev->mr);
> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> index f3758de..2b4083f 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> @@ -45,6 +45,7 @@
>  #include <linux/dcbnl.h>
>  #endif
>  #include <linux/cpu_rmap.h>
> +#include <linux/ptp_clock_kernel.h>
>  
>  #include <linux/mlx4/device.h>
>  #include <linux/mlx4/qp.h>
> @@ -373,10 +374,14 @@ struct mlx4_en_dev {
>  	u32                     priv_pdn;
>  	spinlock_t              uar_lock;
>  	u8			mac_removed[MLX4_MAX_PORTS + 1];
> +	spinlock_t		clock_lock;
> +	u32			nominal_c_mult;
>  	struct cyclecounter	cycles;
>  	struct timecounter	clock;
>  	unsigned long		last_overflow_check;
>  	unsigned long		overflow_period;
> +	struct ptp_clock	*ptp_clock;
> +	struct ptp_clock_info	ptp_clock_info;
>  };
>  
>  
> @@ -786,6 +791,7 @@ void mlx4_en_fill_hwtstamps(struct mlx4_en_dev *mdev,
>  			    struct skb_shared_hwtstamps *hwts,
>  			    u64 timestamp);
>  void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev);
> +void mlx4_en_remove_timestamp(struct mlx4_en_dev *mdev);
>  int mlx4_en_timestamp_config(struct net_device *dev,
>  			     int tx_type,
>  			     int rx_filter);
> -- 
> 1.7.7.6
> 

Re: mlx4_en SIOCSHWTSTAMP cycles the link

[Richard Cochran]

On Mon, Dec 16, 2013 at 05:34:15PM -0600, Shawn Bohrer wrote:
> On Thu, Dec 12, 2013 at 09:28:15AM -0600, Shawn Bohrer wrote:
> > Below is v2.  It compiles, and boots, but linuxptp doesn't work yet
> > and I haven't looked into why.  I think linuxptp is actually
> > complaining about SIOCSHWTSTAMP not working as expected, but again I
> > haven't really even started debugging yet.
> 
> So performing a SIOCSHWTSTAMP on the mlx4_en driver cycles the link:
> 
> [ 9463.081330] mlx4_en: p2p1: Changing Time Stamp configuration
> [ 9463.084637] mlx4_en: p2p1:   frag:0 - size:1526 prefix:0 align:0 stride:1536
> [ 9463.155533] mlx4_en: p2p1: Link Down
> [ 9466.471169] mlx4_en: p2p1: Link Up
> 
> This breaks linuxptp (ptp4l) since it causes all of the already open
> sockets to silently loose their multicast joins.  Thus ptp4l sits
> there waiting for multicast PTP packets that aren't going to come.

The ptp4l program should notice this and recover. This happens when
catching the transmission error on the next sync or delay request. (Or
in slave only E2E mode the sockets are closed and reopened after every
announce interval). If this isn't happening, please post to the
linuxptp-users list the details of your setup.

> I hacked around this in ptp4l by performing the ioctl once with the
> hwstamp_ctl program from linuxptp and skipping the ioctl in ptp4l,
> which works, but quite frankly this sucks.  I'd love to know if anyone
> has any better ideas on how to make SIOCSHWTSTAMP usable.
> 
> After hacking around the SIOCSHWTSTAMP issue I've noticed that ptp4l
> synchronizes the PHC, however there must be another bug somewhere
> because it creates a 35 second offset from the PTP master.

This is the current TAI-UTC offset. Maybe your grand master is serving
UTC instead of the PTP timescale?

Thanks,
Richard

Re: mlx4_en SIOCSHWTSTAMP cycles the link

[Richard Cochran]

On Mon, Dec 16, 2013 at 05:34:15PM -0600, Shawn Bohrer wrote:

> I hacked around this in ptp4l by performing the ioctl once with the
> hwstamp_ctl program from linuxptp and skipping the ioctl in ptp4l,
> which works, but quite frankly this sucks.  I'd love to know if anyone
> has any better ideas on how to make SIOCSHWTSTAMP usable.

So, are you talking about the vlan issue? That would deserve its own
thread.

Thanks,
Richard

Re: mlx4_en SIOCSHWTSTAMP cycles the link

[tedheadster]

> After hacking around the SIOCSHWTSTAMP issue I've noticed that ptp4l
> synchronizes the PHC, however there must be another bug somewhere
> because it creates a 35 second offset from the PTP master.

There is an option to ptp4l to account for this. You would use "-O 35"
when starting ptp4, or perhaps '-O -35' (I forget exactly).

- Matthew

Re: mlx4_en SIOCSHWTSTAMP cycles the link

[Shawn Bohrer]

On Tue, Dec 17, 2013 at 12:26:08PM +0100, Richard Cochran wrote:
> On Mon, Dec 16, 2013 at 05:34:15PM -0600, Shawn Bohrer wrote:
> > On Thu, Dec 12, 2013 at 09:28:15AM -0600, Shawn Bohrer wrote:
> > > Below is v2.  It compiles, and boots, but linuxptp doesn't work yet
> > > and I haven't looked into why.  I think linuxptp is actually
> > > complaining about SIOCSHWTSTAMP not working as expected, but again I
> > > haven't really even started debugging yet.
> > 
> > So performing a SIOCSHWTSTAMP on the mlx4_en driver cycles the link:
> > 
> > [ 9463.081330] mlx4_en: p2p1: Changing Time Stamp configuration
> > [ 9463.084637] mlx4_en: p2p1:   frag:0 - size:1526 prefix:0 align:0 stride:1536
> > [ 9463.155533] mlx4_en: p2p1: Link Down
> > [ 9466.471169] mlx4_en: p2p1: Link Up
> > 
> > This breaks linuxptp (ptp4l) since it causes all of the already open
> > sockets to silently loose their multicast joins.  Thus ptp4l sits
> > there waiting for multicast PTP packets that aren't going to come.
> 
> The ptp4l program should notice this and recover. This happens when
> catching the transmission error on the next sync or delay request. (Or
> in slave only E2E mode the sockets are closed and reopened after every
> announce interval). If this isn't happening, please post to the
> linuxptp-users list the details of your setup.

ptp4l does notice that it isn't receiving packets so it closes its
sockets, opens new ones, and issues the ioctl again, which cycles the
link and the loop repeats.  The real problem is every time you issue
the ioctl the link cycles, which is pointless when the device settings
haven't changed.  I have a patch to mlx4_en that I'll send in a minute
which changes the driver to only cycle the link if the timestamp
options have changed which fixes the issue for me in ptp4l.
 
> > I hacked around this in ptp4l by performing the ioctl once with the
> > hwstamp_ctl program from linuxptp and skipping the ioctl in ptp4l,
> > which works, but quite frankly this sucks.  I'd love to know if anyone
> > has any better ideas on how to make SIOCSHWTSTAMP usable.
> > 
> > After hacking around the SIOCSHWTSTAMP issue I've noticed that ptp4l
> > synchronizes the PHC, however there must be another bug somewhere
> > because it creates a 35 second offset from the PTP master.
> 
> This is the current TAI-UTC offset. Maybe your grand master is serving
> UTC instead of the PTP timescale?

Ah, thanks for point this out.  I just looked over the ptp4l and
phc2sys man page and noticed that it is documented that ptp4l uses the
PTP time scale when run in hardware timestamping mode.  Taking this
into account it appears my mlx4_en PHC driver is working fine so I'll
resend that patch as well.

--
Shawn