All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
@ 2020-05-14 10:28 Olivier Dautricourt
  2020-05-14 10:28 ` [PATCH 1/3] net: stmmac: gmac3: add auxiliary snapshot support Olivier Dautricourt
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Olivier Dautricourt @ 2020-05-14 10:28 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S . Miller
  Cc: Richard Cochran, netdev, Olivier Dautricourt

This patch series covers a use case where an embedded system is
disciplining an internal clock to a GNSS signal, which provides a
stable frequency, and wants to act as a PTP Grandmaster by disciplining
a ptp clock to this internal clock.

In our setup a 10Mhz oscillator is frequency adjusted so that a derived
pps from that oscillator is in phase with the pps generated by 
a gnss receiver.

An other derived clock from the same disciplined oscillator is used as
ptp_clock for the ethernet mac.

The internal pps of the system is forwarded to one of the auxiliary inputs
of the MAC.

Initially the mac time registers are considered random.
We want the mac nanosecond field to be 0 on the auxiliary pps input edge.


PATCH 1/3: 
	The stmmac gmac3 version used in the setup is patched to retrieve a
	timestamp at the rising edge of the aux input and to forward
	it to userspace.

* What matters here is that we get the subsecond offset between the aux 
edge and the edge of the PHC's pps. *


PATCH 2,3/3:

	We want the ptp clock to be in time with our aux pps input.
	Since the ptp clock is derived from the system oscillator, we
	don't want to do frequency adjustements.

	The stmmac driver is patched to allow to set the coarse correction
	mode which avoid to adjust the frequency of the mac continuously
	(the default behavior), but instead, have just one
	time adjustment.


We calculate the time difference between the mac and the internal
clock, and adust the ptp clock time with clock_adjtime syscall.


To summarize this in a user-space program:

****
#include <time.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <stdio.h>

#include <arpa/inet.h>
#include <net/if.h>

#include <sys/ioctl.h>
#include <sys/syscall.h>
#include <sys/timex.h>

#include <linux/ptp_clock.h>
#include <linux/net_tstamp.h>
#include <linux/sockios.h>

#define NS_PER_SEC 1000000000LL

#define CLOCKFD 3

#define FD_TO_CLOCKID(fd) \
	((clockid_t) ((((unsigned int) ~fd) << 3) | CLOCKFD))


static inline int clock_adjtime(clockid_t id, struct timex *tx)
{
	return syscall(__NR_clock_adjtime, id, tx);
}

int main(void)
{
	int fd;
	struct timex tx = {0};
	struct ifreq ifreq = {0};
	struct hwtstamp_config cfg = {0};
	struct ptp_extts_event event = {0};
	struct ptp_extts_request extts_request = {
		.index = 0,
		.flags = PTP_RISING_EDGE | PTP_ENABLE_FEATURE
	};

	const char *iface = "eth0";
	const char *ptp_dev = "/dev/ptp2";

	strncpy(ifreq.ifr_name, iface, sizeof(ifreq.ifr_name) - 1);
	ifreq.ifr_data = (void *) &cfg;
	fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);

	if (fd < 0)
		return 1;

	if (ioctl(fd, SIOCGHWTSTAMP, &ifreq) < 0)
		return 1;

	// Activate coarse mode for stmmac
	cfg.flags |= HWTSTAMP_FLAGS_ADJ_COARSE;
	cfg.flags &= ~HWTSTAMP_FLAGS_ADJ_FINE;

	if (ioctl(fd, SIOCSHWTSTAMP, &ifreq) < 0)
		return 1;

	fd = open(ptp_dev, O_RDWR);

	if (fd < 0)
		return 1;

	// Enable extts input index 0
	if (ioctl(fd, PTP_EXTTS_REQUEST, &extts_request) < 0)
		return 1;

	// Read extts
	if (read(fd, &event, sizeof(event)) != sizeof(event))
		return 1;

	// Correct phc time subsecond: note that this does not correct the phc
	// second count for concision. The delta is (event.t.nsec - NS_PER_SEC).
	tx.modes = ADJ_SETOFFSET | ADJ_NANO;
	tx.time.tv_sec = -1;
	tx.time.tv_usec = event.t.nsec;

	if (clock_adjtime(FD_TO_CLOCKID(fd), &tx))
		return 1;

	// Disable extts index 0
	extts_request.index = 0;
	extts_request.flags = 0;

	if (ioctl(fd, PTP_EXTTS_REQUEST, &extts_request) < 0)
		return 1;

	return 0;
}
****

Artem Panfilov (1):
  net: stmmac: GMAC3: add auxiliary snapshot support

Olivier Dautricourt (2):
  net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE
  net: stmmac: Support coarse mode through ioctl

 .../net/ethernet/stmicro/stmmac/dwmac1000.h   |  3 +-
 .../ethernet/stmicro/stmmac/dwmac1000_core.c  | 24 ++++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  9 ++--
 .../ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 10 +++-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 ++++++---
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 47 +++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.h  | 20 ++++++++
 include/uapi/linux/net_tstamp.h               | 12 +++++
 net/core/dev_ioctl.c                          |  3 --
 9 files changed, 133 insertions(+), 16 deletions(-)

-- 
2.17.1


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

* [PATCH 1/3] net: stmmac: gmac3: add auxiliary snapshot support
  2020-05-14 10:28 [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Olivier Dautricourt
@ 2020-05-14 10:28 ` Olivier Dautricourt
  2020-05-14 10:28 ` [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE Olivier Dautricourt
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 17+ messages in thread
From: Olivier Dautricourt @ 2020-05-14 10:28 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S . Miller
  Cc: Richard Cochran, netdev, Artem Panfilov, Olivier Dautricourt

From: Artem Panfilov <panfilov.artyom@gmail.com>

This patch adds support for time stamping external inputs for GMAC3.

The documentation defines 4 auxiliary snapshots ATSEN0 to ATSEN3 which
can be toggled by writing the Timestamp Control Register.

When the gmac detects a pps rising edge on one of it's auxiliary inputs,
an isr of type GMAC_INT_STATUS_TSTAMP will be triggered.
We use this isr to generate a ptp clock event of type PTP_CLOCK_EXTTS
with the following content:

  - Time of which the event occurred in ns.
  - All the extts for which the event was generated (0000 - 1111)

Note from the documentation:
"When more than one bit is set at the same time, it means
that corresponding auxiliary triggers were sampled at the same clock"

When the GMAC writes it's auxiliary snapshots on it's fifo
and that fifo is full, it will discard any new auxiliary snapshot until
we read the fifo. By reading on each isr, it is unlikely that
we will loose the 1pps external timestamps.

Events for one auxiliary input can be requested through the
PTP_EXTTS_REQUEST ioctl and read as already implemented in the uapi.

This patch introduces 2 functions:

    stmmac_set_hw_tstamping and stmmac_get_hw_tstamping

Each time we initialize the timestamping, we read the current
value of PTP_TCR and patch with new configuration without setting
the ATSENX flags again, which are set independently by the user
through the ioctl.
This allows to not loose the activated external events between each
initialization of the timestamping, and not force the user to redo
ioctl.

Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
Signed-off-by: Artem Panfilov <panfilov.artyom@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac1000.h   |  3 +-
 .../ethernet/stmicro/stmmac/dwmac1000_core.c  | 24 ++++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  9 ++--
 .../ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 10 ++++-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  7 +--
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 44 +++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.h  | 20 +++++++++
 7 files changed, 107 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
index b70d44ac0990..5cff6c100258 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
@@ -41,8 +41,7 @@
 #define	GMAC_INT_DISABLE_PCS	(GMAC_INT_DISABLE_RGMII | \
 				 GMAC_INT_DISABLE_PCSLINK | \
 				 GMAC_INT_DISABLE_PCSAN)
-#define	GMAC_INT_DEFAULT_MASK	(GMAC_INT_DISABLE_TIMESTAMP | \
-				 GMAC_INT_DISABLE_PCS)
+#define	GMAC_INT_DEFAULT_MASK	GMAC_INT_DISABLE_PCS
 
 /* PMT Control and Status */
 #define GMAC_PMT		0x0000002c
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
index efc6ec1b8027..3895fe9396e5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -20,6 +20,7 @@
 #include "stmmac.h"
 #include "stmmac_pcs.h"
 #include "dwmac1000.h"
+#include "stmmac_ptp.h"
 
 static void dwmac1000_core_init(struct mac_device_info *hw,
 				struct net_device *dev)
@@ -300,9 +301,29 @@ static void dwmac1000_rgsmii(void __iomem *ioaddr, struct stmmac_extra_stats *x)
 	}
 }
 
+static void dwmac1000_ptp_isr(struct stmmac_priv *priv)
+{
+	struct ptp_clock_event event;
+	u32 reg_value;
+	u64 ns;
+
+	reg_value = readl(priv->ioaddr + PTP_GMAC3_TSR);
+
+	ns = readl(priv->ioaddr + PTP_GMAC3_AUXTSLO);
+	ns += readl(priv->ioaddr + PTP_GMAC3_AUXTSHI) * 1000000000ULL;
+
+	event.timestamp = ns;
+	event.type = PTP_CLOCK_EXTTS;
+	event.index = (reg_value & PTP_GMAC3_ATSSTN_MASK) >>
+			PTP_GMAC3_ATSSTN_SHIFT;
+	ptp_clock_event(priv->ptp_clock, &event);
+}
+
 static int dwmac1000_irq_status(struct mac_device_info *hw,
 				struct stmmac_extra_stats *x)
 {
+	struct stmmac_priv *priv =
+		container_of(x, struct stmmac_priv, xstats);
 	void __iomem *ioaddr = hw->pcsr;
 	u32 intr_status = readl(ioaddr + GMAC_INT_STATUS);
 	u32 intr_mask = readl(ioaddr + GMAC_INT_MASK);
@@ -324,6 +345,9 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
 		x->irq_receive_pmt_irq_n++;
 	}
 
+	if (intr_status & GMAC_INT_STATUS_TSTAMP)
+		dwmac1000_ptp_isr(priv);
+
 	/* MAC tx/rx EEE LPI entry/exit interrupts */
 	if (intr_status & GMAC_INT_STATUS_LPIIS) {
 		/* Clean LPI interrupt by reading the Reg 12 */
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index ffe2d63389b8..8fa63d059231 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -492,7 +492,8 @@ struct stmmac_ops {
 
 /* PTP and HW Timer helpers */
 struct stmmac_hwtimestamp {
-	void (*config_hw_tstamping) (void __iomem *ioaddr, u32 data);
+	void (*get_hw_tstamping)(void __iomem *ioaddr, u32 *data);
+	void (*set_hw_tstamping)(void __iomem *ioaddr, u32 data);
 	void (*config_sub_second_increment)(void __iomem *ioaddr, u32 ptp_clock,
 					   int gmac4, u32 *ssinc);
 	int (*init_systime) (void __iomem *ioaddr, u32 sec, u32 nsec);
@@ -502,8 +503,10 @@ struct stmmac_hwtimestamp {
 	void (*get_systime) (void __iomem *ioaddr, u64 *systime);
 };
 
-#define stmmac_config_hw_tstamping(__priv, __args...) \
-	stmmac_do_void_callback(__priv, ptp, config_hw_tstamping, __args)
+#define stmmac_get_hw_tstamping(__priv, __args...) \
+	stmmac_do_void_callback(__priv, ptp, get_hw_tstamping, __args)
+#define stmmac_set_hw_tstamping(__priv, __args...) \
+	stmmac_do_void_callback(__priv, ptp, set_hw_tstamping, __args)
 #define stmmac_config_sub_second_increment(__priv, __args...) \
 	stmmac_do_void_callback(__priv, ptp, config_sub_second_increment, __args)
 #define stmmac_init_systime(__priv, __args...) \
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index d291612eeafb..b974d83afe67 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -15,7 +15,12 @@
 #include "common.h"
 #include "stmmac_ptp.h"
 
-static void config_hw_tstamping(void __iomem *ioaddr, u32 data)
+static void get_hw_tstamping(void __iomem *ioaddr, u32 *data)
+{
+	*data = readl(ioaddr + PTP_TCR);
+}
+
+static void set_hw_tstamping(void __iomem *ioaddr, u32 data)
 {
 	writel(data, ioaddr + PTP_TCR);
 }
@@ -154,7 +159,8 @@ static void get_systime(void __iomem *ioaddr, u64 *systime)
 }
 
 const struct stmmac_hwtimestamp stmmac_ptp = {
-	.config_hw_tstamping = config_hw_tstamping,
+	.get_hw_tstamping = get_hw_tstamping,
+	.set_hw_tstamping = set_hw_tstamping,
 	.init_systime = init_systime,
 	.config_sub_second_increment = config_sub_second_increment,
 	.config_addend = config_addend,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a999d6b33a64..c39fafe69b12 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -686,13 +686,14 @@ static int stmmac_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
 	priv->hwts_tx_en = config.tx_type == HWTSTAMP_TX_ON;
 
 	if (!priv->hwts_tx_en && !priv->hwts_rx_en)
-		stmmac_config_hw_tstamping(priv, priv->ptpaddr, 0);
+		stmmac_set_hw_tstamping(priv, priv->ptpaddr, 0);
 	else {
-		value = (PTP_TCR_TSENA | PTP_TCR_TSCFUPDT | PTP_TCR_TSCTRLSSR |
+		stmmac_get_hw_tstamping(priv, priv->ptpaddr, &value);
+		value |= (PTP_TCR_TSENA | PTP_TCR_TSCFUPDT | PTP_TCR_TSCTRLSSR |
 			 tstamp_all | ptp_v2 | ptp_over_ethernet |
 			 ptp_over_ipv6_udp | ptp_over_ipv4_udp | ts_event_en |
 			 ts_master_en | snap_type_sel);
-		stmmac_config_hw_tstamping(priv, priv->ptpaddr, value);
+		stmmac_set_hw_tstamping(priv, priv->ptpaddr, value);
 
 		/* program Sub Second Increment reg */
 		stmmac_config_sub_second_increment(priv,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 0989e2bb6ee3..920f0f3ebbca 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -45,6 +45,43 @@ static int stmmac_adjust_freq(struct ptp_clock_info *ptp, s32 ppb)
 	return 0;
 }
 
+static int stmmac_extts_configure(struct stmmac_priv *priv,
+				  struct ptp_clock_request *rq,
+				  int on)
+{
+	u32 tsauxc, tsauxc_mask;
+
+	if (priv->ptp_clock_ops.n_ext_ts <= rq->extts.index)
+		return -ERANGE;
+
+	switch (rq->extts.index) {
+	case 0:
+		tsauxc_mask = PTP_TCR_ATSEN0;
+		break;
+	case 1:
+		tsauxc_mask = PTP_TCR_ATSEN1;
+		break;
+	case 2:
+		tsauxc_mask = PTP_TCR_ATSEN2;
+		break;
+	case 3:
+		tsauxc_mask = PTP_TCR_ATSEN3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	tsauxc = readl(priv->ptpaddr);
+
+	if (on)
+		tsauxc |= tsauxc_mask;
+	else
+		tsauxc &= ~tsauxc_mask;
+
+	writel(tsauxc, priv->ptpaddr);
+	return 0;
+}
+
 /**
  * stmmac_adjust_time
  *
@@ -158,6 +195,11 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 					     priv->systime_flags);
 		spin_unlock_irqrestore(&priv->ptp_lock, flags);
 		break;
+	case PTP_CLK_REQ_EXTTS:
+		spin_lock_irqsave(&priv->ptp_lock, flags);
+		ret = stmmac_extts_configure(priv, rq, on);
+		spin_unlock_irqrestore(&priv->ptp_lock, flags);
+		break;
 	default:
 		break;
 	}
@@ -202,6 +244,8 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
 		stmmac_ptp_clock_ops.max_adj = priv->plat->ptp_max_adj;
 
 	stmmac_ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num;
+	if (priv->plat->has_gmac)
+		stmmac_ptp_clock_ops.n_ext_ts = 4;
 
 	spin_lock_init(&priv->ptp_lock);
 	priv->ptp_clock_ops = stmmac_ptp_clock_ops;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
index 7abb1d47e7da..26bea23f04ac 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
@@ -59,9 +59,29 @@
 #define	PTP_TCR_SNAPTYPSEL_1	BIT(16)
 /* Enable MAC address for PTP Frame Filtering */
 #define	PTP_TCR_TSENMACADDR	BIT(18)
+/* Clear Auxiliary Snapshot FIFO */
+#define PTP_TCR_ATSFC		BIT(24)
+/* Enable Auxiliary Snapshot 0 */
+#define PTP_TCR_ATSEN0		BIT(25)
+/* Enable Auxiliary Snapshot 1 */
+#define PTP_TCR_ATSEN1		BIT(26)
+/* Enable Auxiliary Snapshot 2 */
+#define PTP_TCR_ATSEN2		BIT(27)
+/* Enable Auxiliary Snapshot 3 */
+#define PTP_TCR_ATSEN3		BIT(28)
 
 /* SSIR defines */
 #define	PTP_SSIR_SSINC_MASK		0xff
 #define	GMAC4_PTP_SSIR_SSINC_SHIFT	16
 
+/* Auxiliary Timestamp Snapshot defines */
+
+#define PTP_GMAC3_TSR		0x0728	/* Timestamp Status */
+#define PTP_GMAC3_AUXTSLO	0x0730	/* Aux Timestamp - Nanoseconds Reg */
+#define PTP_GMAC3_AUXTSHI	0x0734	/* Aux Timestamp - Seconds Reg */
+
+#define PTP_GMAC3_TSR		0x0728	/* Timestamp Status */
+#define PTP_GMAC3_ATSSTN_MASK	GENMASK(19, 16)
+#define PTP_GMAC3_ATSSTN_SHIFT	16
+
 #endif	/* __STMMAC_PTP_H__ */
-- 
2.17.1


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

* [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE
  2020-05-14 10:28 [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Olivier Dautricourt
  2020-05-14 10:28 ` [PATCH 1/3] net: stmmac: gmac3: add auxiliary snapshot support Olivier Dautricourt
@ 2020-05-14 10:28 ` Olivier Dautricourt
  2020-05-14 13:38   ` Richard Cochran
  2020-05-14 10:28 ` [PATCH 3/3] net: stmmac: Support coarse mode through ioctl Olivier Dautricourt
  2020-05-14 13:53 ` [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Richard Cochran
  3 siblings, 1 reply; 17+ messages in thread
From: Olivier Dautricourt @ 2020-05-14 10:28 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S . Miller
  Cc: Richard Cochran, netdev, Olivier Dautricourt

This commit allows a user to specify a flag value for configuring
timestamping through hwtsamp_config structure.

New flags introduced:

HWTSTAMP_FLAGS_NONE = 0
	No flag specified: as it is of value 0, this will selects the
	default behavior for all the existing drivers and should not
	break existing userland programs.

HWTSTAMP_FLAGS_ADJ_FINE = 1
	Use the fine adjustment mode.
	Fine adjustment mode is usually used for precise frequency adjustments.

HWTSTAMP_FLAGS_ADJ_COARSE = 2
	Use the coarse adjustment mode
	Coarse adjustment mode is usually used for direct phase correction.

Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
---
 include/uapi/linux/net_tstamp.h | 12 ++++++++++++
 net/core/dev_ioctl.c            |  3 ---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index 7ed0b3d1c00a..0cfcd490228f 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -65,6 +65,18 @@ struct hwtstamp_config {
 	int rx_filter;
 };
 
+/* possible values for hwtstamp_config->flags */
+enum hwtsamp_flags {
+	/* No special flag specified */
+	HWTSTAMP_FLAGS_NONE,
+
+	/* Enable fine adjustment mode if the driver supports it */
+	HWTSTAMP_FLAGS_ADJ_FINE,
+
+	/* Enable coarse adjustment mode if the driver supports it */
+	HWTSTAMP_FLAGS_ADJ_COARSE,
+};
+
 /* possible values for hwtstamp_config->tx_type */
 enum hwtstamp_tx_types {
 	/*
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 547b587c1950..017671545d45 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -177,9 +177,6 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
 	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
 		return -EFAULT;
 
-	if (cfg.flags) /* reserved for future extensions */
-		return -EINVAL;
-
 	tx_type = cfg.tx_type;
 	rx_filter = cfg.rx_filter;
 
-- 
2.17.1


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

* [PATCH 3/3] net: stmmac: Support coarse mode through ioctl
  2020-05-14 10:28 [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Olivier Dautricourt
  2020-05-14 10:28 ` [PATCH 1/3] net: stmmac: gmac3: add auxiliary snapshot support Olivier Dautricourt
  2020-05-14 10:28 ` [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE Olivier Dautricourt
@ 2020-05-14 10:28 ` Olivier Dautricourt
  2020-05-27  3:55   ` Richard Cochran
  2020-05-14 13:53 ` [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Richard Cochran
  3 siblings, 1 reply; 17+ messages in thread
From: Olivier Dautricourt @ 2020-05-14 10:28 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S . Miller
  Cc: Richard Cochran, netdev, Olivier Dautricourt

This commit enables coarse correction mode for stmmac driver.

The coarse mode allows to update the system time in one process.
The required time adjustment is written in the Timestamp Update registers
while the Sub-second increment register is programmed with the period
of the clock, which is the precision of our correction.

The fine adjutment mode is always the default behavior of the driver.
One should use the HWTSAMP_FLAG_ADJ_COARSE flag while calling
SIOCGHWTSTAMP ioctl to enable coarse mode for stmmac driver.

Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c   | 17 +++++++++++++----
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c    |  3 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index c39fafe69b12..f46503b086f4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -541,9 +541,12 @@ static int stmmac_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
 	netdev_dbg(priv->dev, "%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
 		   __func__, config.flags, config.tx_type, config.rx_filter);
 
-	/* reserved for future extensions */
-	if (config.flags)
-		return -EINVAL;
+	if (config.flags != HWTSTAMP_FLAGS_ADJ_COARSE) {
+		/* Defaulting to fine adjustment for compatibility */
+		netdev_dbg(priv->dev, "%s defaulting to fine adjustment mode\n",
+			   __func__);
+		config.flags = HWTSTAMP_FLAGS_ADJ_FINE;
+	}
 
 	if (config.tx_type != HWTSTAMP_TX_OFF &&
 	    config.tx_type != HWTSTAMP_TX_ON)
@@ -689,10 +692,16 @@ static int stmmac_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
 		stmmac_set_hw_tstamping(priv, priv->ptpaddr, 0);
 	else {
 		stmmac_get_hw_tstamping(priv, priv->ptpaddr, &value);
-		value |= (PTP_TCR_TSENA | PTP_TCR_TSCFUPDT | PTP_TCR_TSCTRLSSR |
+		value |= (PTP_TCR_TSENA |  PTP_TCR_TSCTRLSSR |
 			 tstamp_all | ptp_v2 | ptp_over_ethernet |
 			 ptp_over_ipv6_udp | ptp_over_ipv4_udp | ts_event_en |
 			 ts_master_en | snap_type_sel);
+
+		if (config.flags == HWTSTAMP_FLAGS_ADJ_FINE)
+			value |= PTP_TCR_TSCFUPDT;
+		else
+			value &= ~PTP_TCR_TSCFUPDT;
+
 		stmmac_set_hw_tstamping(priv, priv->ptpaddr, value);
 
 		/* program Sub Second Increment reg */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 920f0f3ebbca..7fb318441015 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -27,6 +27,9 @@ static int stmmac_adjust_freq(struct ptp_clock_info *ptp, s32 ppb)
 	int neg_adj = 0;
 	u64 adj;
 
+	if (priv->tstamp_config.flags != HWTSTAMP_FLAGS_ADJ_FINE)
+		return -EPERM;
+
 	if (ppb < 0) {
 		neg_adj = 1;
 		ppb = -ppb;
-- 
2.17.1


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

* Re: [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE
  2020-05-14 10:28 ` [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE Olivier Dautricourt
@ 2020-05-14 13:38   ` Richard Cochran
  2020-05-14 15:20     ` Olivier Dautricourt
  0 siblings, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2020-05-14 13:38 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

On Thu, May 14, 2020 at 12:28:07PM +0200, Olivier Dautricourt wrote:
> This commit allows a user to specify a flag value for configuring
> timestamping through hwtsamp_config structure.
> 
> New flags introduced:
> 
> HWTSTAMP_FLAGS_NONE = 0
> 	No flag specified: as it is of value 0, this will selects the
> 	default behavior for all the existing drivers and should not
> 	break existing userland programs.
> 
> HWTSTAMP_FLAGS_ADJ_FINE = 1
> 	Use the fine adjustment mode.
> 	Fine adjustment mode is usually used for precise frequency adjustments.
> 
> HWTSTAMP_FLAGS_ADJ_COARSE = 2
> 	Use the coarse adjustment mode
> 	Coarse adjustment mode is usually used for direct phase correction.

I'll have to NAK this change.  The purpose of HWTSTAMP ioctl is to
configure the hardware time stamp settings.  This patch would misuse
it for some other purpose.

Sorry,
Richard

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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-14 10:28 [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Olivier Dautricourt
                   ` (2 preceding siblings ...)
  2020-05-14 10:28 ` [PATCH 3/3] net: stmmac: Support coarse mode through ioctl Olivier Dautricourt
@ 2020-05-14 13:53 ` Richard Cochran
  2020-05-14 15:09   ` Olivier Dautricourt
  3 siblings, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2020-05-14 13:53 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

On Thu, May 14, 2020 at 12:28:05PM +0200, Olivier Dautricourt wrote:
> This patch series covers a use case where an embedded system is
> disciplining an internal clock to a GNSS signal, which provides a
> stable frequency, and wants to act as a PTP Grandmaster by disciplining
> a ptp clock to this internal clock.

Have you seen the new GM patch series on the linuxptp-devel mailing
list?  You may find it interesting...
 
> In our setup a 10Mhz oscillator is frequency adjusted so that a derived
> pps from that oscillator is in phase with the pps generated by 
> a gnss receiver.
> 
> An other derived clock from the same disciplined oscillator is used as
> ptp_clock for the ethernet mac.
> 
> The internal pps of the system is forwarded to one of the auxiliary inputs
> of the MAC.
> 
> Initially the mac time registers are considered random.
> We want the mac nanosecond field to be 0 on the auxiliary pps input edge.
> 
> 
> PATCH 1/3: 
> 	The stmmac gmac3 version used in the setup is patched to retrieve a
> 	timestamp at the rising edge of the aux input and to forward
> 	it to userspace.
> 
> * What matters here is that we get the subsecond offset between the aux 
> edge and the edge of the PHC's pps. *
> 
> 
> PATCH 2,3/3:
> 
> 	We want the ptp clock to be in time with our aux pps input.
> 	Since the ptp clock is derived from the system oscillator, we
> 	don't want to do frequency adjustements.
> 
> 	The stmmac driver is patched to allow to set the coarse correction
> 	mode which avoid to adjust the frequency of the mac continuously
> 	(the default behavior), but instead, have just one
> 	time adjustment.

You can use the new ts2phc program (in the linuxptp-devel patch
series, soon to be merged) by configuring it to use the nullf servo.

Thanks,
Richard


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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-14 13:53 ` [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Richard Cochran
@ 2020-05-14 15:09   ` Olivier Dautricourt
  2020-05-15  0:37     ` Richard Cochran
  0 siblings, 1 reply; 17+ messages in thread
From: Olivier Dautricourt @ 2020-05-14 15:09 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

The 05/14/2020 06:53, Richard Cochran wrote:
> On Thu, May 14, 2020 at 12:28:05PM +0200, Olivier Dautricourt wrote:
> > This patch series covers a use case where an embedded system is
> > disciplining an internal clock to a GNSS signal, which provides a
> > stable frequency, and wants to act as a PTP Grandmaster by disciplining
> > a ptp clock to this internal clock.
> 
> Have you seen the new GM patch series on the linuxptp-devel mailing
> list?  You may find it interesting...
  
  I have not, but i'll look deeper in the way this use case is handled
  in this series..

> 
> > In our setup a 10Mhz oscillator is frequency adjusted so that a derived
> > pps from that oscillator is in phase with the pps generated by
> > a gnss receiver.
> >
> > An other derived clock from the same disciplined oscillator is used as
> > ptp_clock for the ethernet mac.
> >
> > The internal pps of the system is forwarded to one of the auxiliary inputs
> > of the MAC.
> >
> > Initially the mac time registers are considered random.
> > We want the mac nanosecond field to be 0 on the auxiliary pps input edge.
> >
> >
> > PATCH 1/3:
> >       The stmmac gmac3 version used in the setup is patched to retrieve a
> >       timestamp at the rising edge of the aux input and to forward
> >       it to userspace.
> >
> > * What matters here is that we get the subsecond offset between the aux
> > edge and the edge of the PHC's pps. *
> >
> >
> > PATCH 2,3/3:
> >
> >       We want the ptp clock to be in time with our aux pps input.
> >       Since the ptp clock is derived from the system oscillator, we
> >       don't want to do frequency adjustements.
> >
> >       The stmmac driver is patched to allow to set the coarse correction
> >       mode which avoid to adjust the frequency of the mac continuously
> >       (the default behavior), but instead, have just one
> >       time adjustment.
> 
> You can use the new ts2phc program (in the linuxptp-devel patch
> series, soon to be merged) by configuring it to use the nullf servo.

  My issue is that the default behavior of the stmmac driver is to
  set the mac into fine mode which implies to continuously do frequency
  adjustment, So if i'm not mistaken using the nullf servo will not address
  that issue even if we are not doing explicit clock_adjtime syscalls.
  
  Patches 2/3 and 3/3 are reponsible for preventing this.
> 
> Thanks,
> Richard
> 
> ATTENTION: This email came from an external source.
> Do not open attachments or click on links from unknown senders or unexpected emails.

Regards,

-- 
Olivier Dautricourt


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

* Re: [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE
  2020-05-14 13:38   ` Richard Cochran
@ 2020-05-14 15:20     ` Olivier Dautricourt
  2020-05-15  0:29       ` Richard Cochran
  0 siblings, 1 reply; 17+ messages in thread
From: Olivier Dautricourt @ 2020-05-14 15:20 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

The 05/14/2020 06:38, Richard Cochran wrote:
> On Thu, May 14, 2020 at 12:28:07PM +0200, Olivier Dautricourt wrote:
> > This commit allows a user to specify a flag value for configuring
> > timestamping through hwtsamp_config structure.
> >
> > New flags introduced:
> >
> > HWTSTAMP_FLAGS_NONE = 0
> >       No flag specified: as it is of value 0, this will selects the
> >       default behavior for all the existing drivers and should not
> >       break existing userland programs.
> >
> > HWTSTAMP_FLAGS_ADJ_FINE = 1
> >       Use the fine adjustment mode.
> >       Fine adjustment mode is usually used for precise frequency adjustments.
> >
> > HWTSTAMP_FLAGS_ADJ_COARSE = 2
> >       Use the coarse adjustment mode
> >       Coarse adjustment mode is usually used for direct phase correction.
> 
> I'll have to NAK this change.  The purpose of HWTSTAMP ioctl is to
> configure the hardware time stamp settings.  This patch would misuse
> it for some other purpose.

Hello,
 
Can't we consider this as a time stamp settings ?
I don't see where we could put those driver-specific flags.
That flag field was reserved for futher improvements so i found
it acceptable to specify that here.

This ioctl structure is used in the driver initialization
function which is reponsible for configuring the mac.
Adding an ioctl for that may be redundant.
But i'd like your opinion on that.

> 
> Sorry,
> Richard


Regards,

-- 
Olivier Dautricourt


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

* Re: [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE
  2020-05-14 15:20     ` Olivier Dautricourt
@ 2020-05-15  0:29       ` Richard Cochran
  0 siblings, 0 replies; 17+ messages in thread
From: Richard Cochran @ 2020-05-15  0:29 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

On Thu, May 14, 2020 at 05:20:41PM +0200, Olivier Dautricourt wrote:
> Can't we consider this as a time stamp settings ?

No.  It really is not a time stamp setting at all.

> I don't see where we could put those driver-specific flags.
> That flag field was reserved for futher improvements so i found
> it acceptable to specify that here.

This field is for possible future changes in time stamps, not clock
inputs or servo modes.

Thanks,
Richard

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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-14 15:09   ` Olivier Dautricourt
@ 2020-05-15  0:37     ` Richard Cochran
  2020-05-15 13:26       ` Julien Beraud
  0 siblings, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2020-05-15  0:37 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

On Thu, May 14, 2020 at 05:09:01PM +0200, Olivier Dautricourt wrote:
>   My issue is that the default behavior of the stmmac driver is to
>   set the mac into fine mode which implies to continuously do frequency
>   adjustment, So if i'm not mistaken using the nullf servo will not address
>   that issue even if we are not doing explicit clock_adjtime syscalls.

Why not use the external time stamps from your GPS to do one of the
following?

A) Feed them into a user space PI controller and do frequency
   correction, or

B) Feed the offsets into the hardware PLL (if that is present) using
   the new PHC ADJ_OFFSET support (found in net-next).

I don't see a need for changing any kernel interfaces at all.  If you
think there is such a need, then you will have to make some kind of
argument or justification for it.

BTW, support for B has been implemented in linuxptp, and the patches
will appear soon.  We can also add it into the ts2phc program if you
like.

Thanks,
Richard

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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-15  0:37     ` Richard Cochran
@ 2020-05-15 13:26       ` Julien Beraud
  2020-05-15 23:30         ` Richard Cochran
  2020-05-27  4:05         ` Richard Cochran
  0 siblings, 2 replies; 17+ messages in thread
From: Julien Beraud @ 2020-05-15 13:26 UTC (permalink / raw)
  To: Richard Cochran, Olivier Dautricourt
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

Sorry to jump in, just wanted to add a few details,

On 15/05/2020 02:37, Richard Cochran wrote:
> On Thu, May 14, 2020 at 05:09:01PM +0200, Olivier Dautricourt wrote:
>>    My issue is that the default behavior of the stmmac driver is to
>>    set the mac into fine mode which implies to continuously do frequency
>>    adjustment, So if i'm not mistaken using the nullf servo will not address
>>    that issue even if we are not doing explicit clock_adjtime syscalls.
> 
> Why not use the external time stamps from your GPS to do one of the
> following?
> 
> A) Feed them into a user space PI controller and do frequency
>     correction, or
> 
> B) Feed the offsets into the hardware PLL (if that is present) using
>     the new PHC ADJ_OFFSET support (found in net-next).
> 
> I don't see a need for changing any kernel interfaces at all.  If you
> think there is such a need, then you will have to make some kind of
> argument or justification for it.

I think that B could work, but it doesn't solve the issue with the adjustment
mode of the mac's clock.
I understand this may be very specific to stmmac but the problem is still
there.

This MAC's Timestamping Clock has 2 adjustment modes :
- Fine
- Coarse

It has the following registers :
- sub-second-increment (32bits)
- addend (32bits)
- timestamp-control (to set the mode to fine or coarse)

It also has an accumulator (32bits)

-> When in fine mode, which is the current default,
- The sub-second-increment (in ns) is added to the clock's time when the
accumulator overflows
- At each clock cycle of ptp_ref_clock, the value of addend is added to the
the accumulator

The frequency adjustments of the mac's clock are done by changing the value of
addend, so that the number of clock cycles that make the accumulator overflow
slightly change.

The value of sub-second-increment is set to 2 / ptp_clk_freq, because using an
accumulator with the same number of bits as the addend register makes it
impossible to overflow at every addition.

This mode allows to implement a PTP Slave, constantly adjusting the clock's freq
to match the master.

-> In coarse mode, the accumulator and the addend register are not used and the
value of sub-second-increment is added to the clock at every ptp_ref_clock
cycle.
So the value of sub-second-increment can be set to 1 / ptp_clk_freq but fine
adjustments of the clocks frequency are not possible.

This mode allows to implement a Grand Master where we can feed the stmmac's ptp
ref clock input with a frequency that's controlled externally, making it
useless to do frequency adjustments with the MAC.

We want our devices to be able to switch from Master to Slave at runtime.

To make things short it implies that :

1- In coarse mode, the accuracy of the timestamps is twice what it is in fine
mode (loosing precision on the capability to adjust the mac clock's frequency)

2- I don't think modes can be switched gracefully while the timestamping clk is
active. (2 registers need to be modified : sub-second-increment and control)

1 makes it quite valuable to be able to switch mode. 2 means that this
cannot be done when doing the clock adjustment (offset vs. freq adj. for
instance).

So the question is what interface could we use to configure a timestamping
clock that has more than one functioning mode and which mode can be changed at
runtime, but not while timestamping is running ?

Another thing to note is that based on your comments, I understand that the way
the stmmac's clock is handled now, i.e configuring and enabling the clock when
enabling hardware timestamping may not be right, and we may want to split
enabling the clock and enabling hardware timestamping, but we'd still
need to be able to switch the mode of adjustement of the clock.

I hope this helps a bit,
Julien

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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-15 13:26       ` Julien Beraud
@ 2020-05-15 23:30         ` Richard Cochran
  2020-05-25 10:00           ` Olivier Dautricourt
  2020-05-27  4:05         ` Richard Cochran
  1 sibling, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2020-05-15 23:30 UTC (permalink / raw)
  To: Julien Beraud
  Cc: Olivier Dautricourt, Giuseppe Cavallaro, Alexandre Torgue,
	Jose Abreu, David S . Miller, netdev

On Fri, May 15, 2020 at 03:26:47PM +0200, Julien Beraud wrote:
> So the question is what interface could we use to configure a timestamping
> clock that has more than one functioning mode and which mode can be changed at
> runtime, but not while timestamping is running ?

Thanks for your detailed response.  Let me digest that and see what I
can come up with...

Thanks,
Richard

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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-15 23:30         ` Richard Cochran
@ 2020-05-25 10:00           ` Olivier Dautricourt
  0 siblings, 0 replies; 17+ messages in thread
From: Olivier Dautricourt @ 2020-05-25 10:00 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Julien Beraud, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

The 05/15/2020 16:30, Richard Cochran wrote:
> On Fri, May 15, 2020 at 03:26:47PM +0200, Julien Beraud wrote:
> > So the question is what interface could we use to configure a timestamping
> > clock that has more than one functioning mode and which mode can be changed at
> > runtime, but not while timestamping is running ?
> 
> Thanks for your detailed response.  Let me digest that and see what I
> can come up with...
> 
> Thanks,
> Richard

Hello Richard,

Did you get a chance to look into this issue ?


Thanks,

-- 
Olivier


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

* Re: [PATCH 3/3] net: stmmac: Support coarse mode through ioctl
  2020-05-14 10:28 ` [PATCH 3/3] net: stmmac: Support coarse mode through ioctl Olivier Dautricourt
@ 2020-05-27  3:55   ` Richard Cochran
  2020-06-03 16:12     ` Olivier Dautricourt
  0 siblings, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2020-05-27  3:55 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

On Thu, May 14, 2020 at 12:28:08PM +0200, Olivier Dautricourt wrote:
> The required time adjustment is written in the Timestamp Update registers
> while the Sub-second increment register is programmed with the period
> of the clock, which is the precision of our correction.

I don't see in this patch where the "required time adjustment is
written in the Timestamp Update registers".

What am I missing?

Thanks,
Richard


> 
> The fine adjutment mode is always the default behavior of the driver.
> One should use the HWTSAMP_FLAG_ADJ_COARSE flag while calling
> SIOCGHWTSTAMP ioctl to enable coarse mode for stmmac driver.
> 
> Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c   | 17 +++++++++++++----
>  .../net/ethernet/stmicro/stmmac/stmmac_ptp.c    |  3 +++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index c39fafe69b12..f46503b086f4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -541,9 +541,12 @@ static int stmmac_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
>  	netdev_dbg(priv->dev, "%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
>  		   __func__, config.flags, config.tx_type, config.rx_filter);
>  
> -	/* reserved for future extensions */
> -	if (config.flags)
> -		return -EINVAL;
> +	if (config.flags != HWTSTAMP_FLAGS_ADJ_COARSE) {
> +		/* Defaulting to fine adjustment for compatibility */
> +		netdev_dbg(priv->dev, "%s defaulting to fine adjustment mode\n",
> +			   __func__);
> +		config.flags = HWTSTAMP_FLAGS_ADJ_FINE;
> +	}
>  
>  	if (config.tx_type != HWTSTAMP_TX_OFF &&
>  	    config.tx_type != HWTSTAMP_TX_ON)
> @@ -689,10 +692,16 @@ static int stmmac_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
>  		stmmac_set_hw_tstamping(priv, priv->ptpaddr, 0);
>  	else {
>  		stmmac_get_hw_tstamping(priv, priv->ptpaddr, &value);
> -		value |= (PTP_TCR_TSENA | PTP_TCR_TSCFUPDT | PTP_TCR_TSCTRLSSR |
> +		value |= (PTP_TCR_TSENA |  PTP_TCR_TSCTRLSSR |
>  			 tstamp_all | ptp_v2 | ptp_over_ethernet |
>  			 ptp_over_ipv6_udp | ptp_over_ipv4_udp | ts_event_en |
>  			 ts_master_en | snap_type_sel);
> +
> +		if (config.flags == HWTSTAMP_FLAGS_ADJ_FINE)
> +			value |= PTP_TCR_TSCFUPDT;
> +		else
> +			value &= ~PTP_TCR_TSCFUPDT;
> +
>  		stmmac_set_hw_tstamping(priv, priv->ptpaddr, value);
>  
>  		/* program Sub Second Increment reg */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 920f0f3ebbca..7fb318441015 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -27,6 +27,9 @@ static int stmmac_adjust_freq(struct ptp_clock_info *ptp, s32 ppb)
>  	int neg_adj = 0;
>  	u64 adj;
>  
> +	if (priv->tstamp_config.flags != HWTSTAMP_FLAGS_ADJ_FINE)
> +		return -EPERM;
> +
>  	if (ppb < 0) {
>  		neg_adj = 1;
>  		ppb = -ppb;
> -- 
> 2.17.1
> 

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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-15 13:26       ` Julien Beraud
  2020-05-15 23:30         ` Richard Cochran
@ 2020-05-27  4:05         ` Richard Cochran
  2020-06-03 15:17           ` Olivier Dautricourt
  1 sibling, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2020-05-27  4:05 UTC (permalink / raw)
  To: Julien Beraud
  Cc: Olivier Dautricourt, Giuseppe Cavallaro, Alexandre Torgue,
	Jose Abreu, David S . Miller, netdev

On Fri, May 15, 2020 at 03:26:47PM +0200, Julien Beraud wrote:
> The frequency adjustments of the mac's clock are done by changing the value of
> addend, so that the number of clock cycles that make the accumulator overflow
> slightly change.

This is typical.
 
> The value of sub-second-increment is set to 2 / ptp_clk_freq, because using an
> accumulator with the same number of bits as the addend register makes it
> impossible to overflow at every addition.
> 
> This mode allows to implement a PTP Slave, constantly adjusting the clock's freq
> to match the master.

Right.  And I would stick with this.  With the ts2phc program
(linuxptp master branch) you can use the EXTTS to discipline the clock
to match the external time source.

> -> In coarse mode, the accumulator and the addend register are not used and the
> value of sub-second-increment is added to the clock at every ptp_ref_clock
> cycle.

That sounds like simply configuring a fixed rate.

> This mode allows to implement a Grand Master where we can feed the stmmac's ptp
> ref clock input with a frequency that's controlled externally, making it
> useless to do frequency adjustments with the MAC.
> 
> We want our devices to be able to switch from Master to Slave at runtime.

If I understand correctly, what you are really asking for is the
ability to switch clock sources.  This normally done through the
device tree, and changing the device tree at run time is done with
overlays (which I think is still a WIP).

> So the question is what interface could we use to configure a timestamping
> clock that has more than one functioning mode and which mode can be changed at
> runtime, but not while timestamping is running ?

I think this must be decided at boot time using the DTS.  In any case,
the PHC time stamping API is not the right place for this.  If you end
up making a custom method (via debugfs for example), then your PHC
should then return an error when user space attempts to adjust it.
 
Thanks,
Richard

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

* Re: [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock
  2020-05-27  4:05         ` Richard Cochran
@ 2020-06-03 15:17           ` Olivier Dautricourt
  0 siblings, 0 replies; 17+ messages in thread
From: Olivier Dautricourt @ 2020-06-03 15:17 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Julien Beraud, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

The 05/26/2020 21:05, Richard Cochran wrote:
> On Fri, May 15, 2020 at 03:26:47PM +0200, Julien Beraud wrote:
> > The frequency adjustments of the mac's clock are done by changing the value of
> > addend, so that the number of clock cycles that make the accumulator overflow
> > slightly change.
> 
> This is typical.
> 
> > The value of sub-second-increment is set to 2 / ptp_clk_freq, because using an
> > accumulator with the same number of bits as the addend register makes it
> > impossible to overflow at every addition.
> >
> > This mode allows to implement a PTP Slave, constantly adjusting the clock's freq
> > to match the master.
> 
> Right.  And I would stick with this.  With the ts2phc program
> (linuxptp master branch) you can use the EXTTS to discipline the clock
> to match the external time source.

The advantage for us here is that the coarse mode allows to set the
sub-second-increment to 1 / ptp_clk_freq and increase the precision of
the timestamps in that mode. Because we don't have to deal with the accumulator
overflow anymore.

> 
> > -> In coarse mode, the accumulator and the addend register are not used and the
> > value of sub-second-increment is added to the clock at every ptp_ref_clock
> > cycle.
> 
> That sounds like simply configuring a fixed rate.

Yes it is. But we want that rate not to be the same that in fine mode, by
configuring it at runtime.

> 
> > This mode allows to implement a Grand Master where we can feed the stmmac's ptp
> > ref clock input with a frequency that's controlled externally, making it
> > useless to do frequency adjustments with the MAC.
> >
> > We want our devices to be able to switch from Master to Slave at runtime.
> 
> If I understand correctly, what you are really asking for is the
> ability to switch clock sources.  This normally done through the
> device tree, and changing the device tree at run time is done with
> overlays (which I think is still a WIP).

In our setup the source clock switch is actually done in an fpga, so the
device tree node is still the same in both cases and we configure the input clock
through a gpio control from the ptp client.

> > So the question is what interface could we use to configure a timestamping
> > clock that has more than one functioning mode and which mode can be changed at
> > runtime, but not while timestamping is running ?
> 
> I think this must be decided at boot time using the DTS.  In any case,
> the PHC time stamping API is not the right place for this.  If you end
> up making a custom method (via debugfs for example), then your PHC
> should then return an error when user space attempts to adjust it.

I think the problem here relies on the fact that the configuration of the mac
is done in the stmmac_hwstamp_set function while what we are really
configuring is the time stamping + ptp_clock functionning.
In the end there is only one register where both are configured
(Control register). But this also makes sense because if there is no
active timestamping the ptp clock can't work. Also as Julien explained
it in his previous mail, the mode switch (writing 2 registers) needs
the timestamping to be reinitialized in any case.

So for something upstreamable i'm kind of stuck here.
This kind of mode configuration does not seems to fit in the current kernel API
because it affects both timestamping (sub-second-increment value) and the ptp clock
functionning (no fine adjustment possible while in coarse mode).

If we don't find a solution i'll at least resubmit my first patch
which is independent of the other two.

Thanks,
-- 
Olivier Dautricourt


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

* Re: [PATCH 3/3] net: stmmac: Support coarse mode through ioctl
  2020-05-27  3:55   ` Richard Cochran
@ 2020-06-03 16:12     ` Olivier Dautricourt
  0 siblings, 0 replies; 17+ messages in thread
From: Olivier Dautricourt @ 2020-06-03 16:12 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S . Miller, netdev

The 05/26/2020 20:55, Richard Cochran wrote:
> On Thu, May 14, 2020 at 12:28:08PM +0200, Olivier Dautricourt wrote:
> > The required time adjustment is written in the Timestamp Update registers
> > while the Sub-second increment register is programmed with the period
> > of the clock, which is the precision of our correction.
> 
> I don't see in this patch where the "required time adjustment is
> written in the Timestamp Update registers".
> 
> What am I missing?
> 
> Thanks,
> Richard

This routine already exists in the driver (adjust_systime function).
This one is called on ADJ_SETOFFSET in both functionning modes.
So in both modes the phase jump is done by setting the target time in those
update registers, set a flag and waiting for this flag to be cleared, that
would mean that the correction is effective.


Regards,
-- 
Olivier Dautricourt


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

end of thread, other threads:[~2020-06-03 16:12 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-14 10:28 [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Olivier Dautricourt
2020-05-14 10:28 ` [PATCH 1/3] net: stmmac: gmac3: add auxiliary snapshot support Olivier Dautricourt
2020-05-14 10:28 ` [PATCH 2/3] net: uapi: Add HWTSTAMP_FLAGS_ADJ_FINE/ADJ_COARSE Olivier Dautricourt
2020-05-14 13:38   ` Richard Cochran
2020-05-14 15:20     ` Olivier Dautricourt
2020-05-15  0:29       ` Richard Cochran
2020-05-14 10:28 ` [PATCH 3/3] net: stmmac: Support coarse mode through ioctl Olivier Dautricourt
2020-05-27  3:55   ` Richard Cochran
2020-06-03 16:12     ` Olivier Dautricourt
2020-05-14 13:53 ` [PATCH 0/3] Patch series for a PTP Grandmaster use case using stmmac/gmac3 ptp clock Richard Cochran
2020-05-14 15:09   ` Olivier Dautricourt
2020-05-15  0:37     ` Richard Cochran
2020-05-15 13:26       ` Julien Beraud
2020-05-15 23:30         ` Richard Cochran
2020-05-25 10:00           ` Olivier Dautricourt
2020-05-27  4:05         ` Richard Cochran
2020-06-03 15:17           ` Olivier Dautricourt

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.