[PATCH v1 0/4] Enable FEC pps ouput

[PATCH v1 0/4] Enable FEC pps ouput

[Luwei Zhou]

This patch does:
	- Replace 32-bit free-running PTP timer with 31-bit.
	- Implement hardware PTP timestamp adjustment.
	- Enable PPS output based on hardware adjustment.


Luwei Zhou (4):
  net: fec: ptp: Use the 31-bit ptp timer.
  net: fec: ptp: Use hardware algorithm to adjust PTP counter.
  net: fec: ptp: Enalbe PPS ouput based on ptp clock
  ARM: Documentation: Update fec dts binding doc

 Documentation/devicetree/bindings/net/fsl-fec.txt |   2 +
 drivers/net/ethernet/freescale/fec.h              |   7 +
 drivers/net/ethernet/freescale/fec_main.c         |   2 +
 drivers/net/ethernet/freescale/fec_ptp.c          | 301 ++++++++++++++++++++--
 4 files changed, 297 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH v1 1/4] net: fec: ptp: Use the 31-bit ptp timer.

[Luwei Zhou]

When ptp switches from software adjustment to hardware ajustment, linux ptp can't converge.
It is caused by the IP limit. Hardware adjustment logcial have issue when ptp counter
runs over 0x80000000(31 bit counter). The internal IP reference manual already remove 32bit
free-running count support. This patch replace the 32-bit PTP timer with 31-bit.

Signed-off-by: Luwei Zhou <b45643@freescale.com>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index cca3617..8016bdd 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -70,6 +70,7 @@
 #define FEC_TS_TIMESTAMP	0x418
 
 #define FEC_CC_MULT	(1 << 31)
+#define FEC_COUNTER_PERIOD	(1 << 31)
 /**
  * fec_ptp_read - read raw cycle counter (to be used by time counter)
  * @cc: the cyclecounter structure
@@ -113,14 +114,15 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
 	/* 1ns counter */
 	writel(inc << FEC_T_INC_OFFSET, fep->hwp + FEC_ATIME_INC);
 
-	/* use free running count */
-	writel(0, fep->hwp + FEC_ATIME_EVT_PERIOD);
+	/* use 31-bit timer counter */
+	writel(FEC_COUNTER_PERIOD, fep->hwp + FEC_ATIME_EVT_PERIOD);
 
-	writel(FEC_T_CTRL_ENABLE, fep->hwp + FEC_ATIME_CTRL);
+	writel(FEC_T_CTRL_ENABLE | FEC_T_CTRL_PERIOD_RST,
+		fep->hwp + FEC_ATIME_CTRL);
 
 	memset(&fep->cc, 0, sizeof(fep->cc));
 	fep->cc.read = fec_ptp_read;
-	fep->cc.mask = CLOCKSOURCE_MASK(32);
+	fep->cc.mask = CLOCKSOURCE_MASK(31);
 	fep->cc.shift = 31;
 	fep->cc.mult = FEC_CC_MULT;
 
-- 
1.9.1

[PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

[Luwei Zhou]

The FEC IP supports hardware adjustment for ptp timer. Refer to the description of
ENET_ATCOR and ENET_ATINC registers in the spec about the hardware adjustment. This
patch uses hardware support to adjust the ptp offset and frequency on the slave side.

Signed-off-by: Luwei Zhou <b45643@freescale.com>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
Signed-off-by: Fugang Duan <b38611@freescale.com>
---
 drivers/net/ethernet/freescale/fec_ptp.c | 68 +++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 8016bdd..e2bf786 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -71,6 +71,7 @@
 
 #define FEC_CC_MULT	(1 << 31)
 #define FEC_COUNTER_PERIOD	(1 << 31)
+#define FEC_T_PERIOD_ONE_SEC	(1000000000UL)
 /**
  * fec_ptp_read - read raw cycle counter (to be used by time counter)
  * @cc: the cyclecounter structure
@@ -145,33 +146,65 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
  */
 static int fec_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
 {
-	u64 diff;
 	unsigned long flags;
 	int neg_adj = 0;
-	u32 mult = FEC_CC_MULT;
+	u32 i, tmp;
+	u32 ptp_ts_clk, ptp_inc;
+	u32 corr_inc, corr_period;
+	u32 corr_ns;
 
 	struct fec_enet_private *fep =
 	    container_of(ptp, struct fec_enet_private, ptp_caps);
 
+	if (ppb == 0)
+		return 0;
+
 	if (ppb < 0) {
 		ppb = -ppb;
 		neg_adj = 1;
 	}
 
-	diff = mult;
-	diff *= ppb;
-	diff = div_u64(diff, 1000000000ULL);
+	ptp_ts_clk = clk_get_rate(fep->clk_ptp);
+	ptp_inc = FEC_T_PERIOD_ONE_SEC / ptp_ts_clk;
+
+	/*
+	 * In theory, corr_inc/corr_period = ppb/FEC_T_PERIOD_ONE_SEC;
+	 * Try to find the corr_inc  between 1 to ptp_inc to meet adjustment
+	 * requirement.
+	 */
+	for (i = 1; i <= ptp_inc; i++) {
+		if (((i * FEC_T_PERIOD_ONE_SEC) / ppb) >= ptp_inc) {
+			corr_inc = i;
+			corr_period = ((i * FEC_T_PERIOD_ONE_SEC) /
+						(ptp_inc * ppb));
+			break;
+		}
+	}
+	/*
+	 * Not found? Set it to high value - double speed
+	 * correct in every clock step.
+	 */
+	if (i > ptp_inc) {
+		corr_inc = ptp_inc;
+		corr_period = 1;
+	}
+
+	if (neg_adj)
+		corr_ns = ptp_inc - corr_inc;
+	else
+		corr_ns = ptp_inc + corr_inc;
 
 	spin_lock_irqsave(&fep->tmreg_lock, flags);
+
+	tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
+	tmp |= corr_ns << FEC_T_INC_CORR_OFFSET;
+	writel(tmp, fep->hwp + FEC_ATIME_INC);
+	writel(corr_period, fep->hwp + FEC_ATIME_CORR);
 	/*
-	 * dummy read to set cycle_last in tc to now.
-	 * So use adjusted mult to calculate when next call
-	 * timercounter_read.
+	 * dummy read to update the timer.
 	 */
 	timecounter_read(&fep->tc);
 
-	fep->cc.mult = neg_adj ? mult - diff : mult + diff;
-
 	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 
 	return 0;
@@ -190,12 +223,20 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	    container_of(ptp, struct fec_enet_private, ptp_caps);
 	unsigned long flags;
 	u64 now;
+	u32 counter;
 
 	spin_lock_irqsave(&fep->tmreg_lock, flags);
 
 	now = timecounter_read(&fep->tc);
 	now += delta;
 
+	/*
+	 * Get the timer value based on adjusted timestamp.
+	 * Update the counter with the masked value.
+	 */
+	counter = now & fep->cc.mask;
+	writel(counter, fep->hwp + FEC_ATIME);
+
 	/* reset the timecounter */
 	timecounter_init(&fep->tc, &fep->cc, now);
 
@@ -246,6 +287,7 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
 
 	u64 ns;
 	unsigned long flags;
+	u32 counter;
 
 	mutex_lock(&fep->ptp_clk_mutex);
 	/* Check the ptp clock */
@@ -256,8 +298,14 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
 
 	ns = ts->tv_sec * 1000000000ULL;
 	ns += ts->tv_nsec;
+	/*
+	 * Get the timer value based on timestamp.
+	 * Update the counter with the masked value.
+	 */
+	counter = ns & fep->cc.mask;
 
 	spin_lock_irqsave(&fep->tmreg_lock, flags);
+	writel(counter, fep->hwp + FEC_ATIME);
 	timecounter_init(&fep->tc, &fep->cc, ns);
 	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 	mutex_unlock(&fep->ptp_clk_mutex);
-- 
1.9.1

[PATCH v1 3/4] net: fec: ptp: Enalbe PPS ouput based on ptp clock

[Luwei Zhou]

FEC ptp timer has 4 channel compare/trigger function. It can be used to enable pps output.
The pulse would be ouput high exactly on N second. The pulse ouput high on compare event mode
is used to produce pulse per second.  The pulse width would be one cycle based on ptp timer clock
source.Since 31-bit ptp hardware timer is used, the timer will wrap more than 2 seconds. We need to
reload the compare compare event about every 1 second.

Signed-off-by: Luwei Zhou <b45643@freescale.com>
---
 drivers/net/ethernet/freescale/fec.h      |   7 +
 drivers/net/ethernet/freescale/fec_main.c |   2 +
 drivers/net/ethernet/freescale/fec_ptp.c  | 223 +++++++++++++++++++++++++++++-
 3 files changed, 231 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 26fb1de..f8e23e6 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -481,12 +481,19 @@ struct fec_enet_private {
 	unsigned int tx_pkts_itr;
 	unsigned int tx_time_itr;
 	unsigned int itr_clk_rate;
+
+	/* pps  */
+	int pps_channel;
+	unsigned int reload_period;
+	int pps_enable;
+	unsigned int next_counter;
 };
 
 void fec_ptp_init(struct platform_device *pdev);
 void fec_ptp_start_cyclecounter(struct net_device *ndev);
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
 int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
+uint fec_ptp_check_pps_event(struct fec_enet_private *fep);
 
 /****************************************************************************/
 #endif /* FEC_H */
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3a4ec0f..5a4ce36 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1558,6 +1558,8 @@ fec_enet_interrupt(int irq, void *dev_id)
 		complete(&fep->mdio_done);
 	}
 
+	fec_ptp_check_pps_event(fep);
+
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index e2bf786..c55239d 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -61,6 +61,24 @@
 #define FEC_T_INC_CORR_MASK             0x00007f00
 #define FEC_T_INC_CORR_OFFSET           8
 
+#define FEC_T_CTRL_PINPER		0x00000080
+#define FEC_T_TF0_MASK			0x00000001
+#define FEC_T_TF0_OFFSET		0
+#define FEC_T_TF1_MASK			0x00000002
+#define FEC_T_TF1_OFFSET		1
+#define FEC_T_TF2_MASK			0x00000004
+#define FEC_T_TF2_OFFSET		2
+#define FEC_T_TF3_MASK			0x00000008
+#define FEC_T_TF3_OFFSET		3
+#define FEC_T_TDRE_MASK			0x00000001
+#define FEC_T_TDRE_OFFSET		0
+#define FEC_T_TMODE_MASK		0x0000003C
+#define FEC_T_TMODE_OFFSET		2
+#define FEC_T_TIE_MASK			0x00000040
+#define FEC_T_TIE_OFFSET		6
+#define FEC_T_TF_MASK			0x00000080
+#define FEC_T_TF_OFFSET			7
+
 #define FEC_ATIME_CTRL		0x400
 #define FEC_ATIME		0x404
 #define FEC_ATIME_EVT_OFFSET	0x408
@@ -69,9 +87,162 @@
 #define FEC_ATIME_INC		0x414
 #define FEC_TS_TIMESTAMP	0x418
 
+#define FEC_TGSR		0x604
+#define FEC_TCSR(n)		(0x608 + n * 0x08)
+#define FEC_TCCR(n)		(0x60C + n * 0x08)
+#define MAX_TIMER_CHANNEL	3
+#define FEC_TMODE_TOGGLE	0x05
+#define FEC_HIGH_PULSE		0x0F
+
 #define FEC_CC_MULT	(1 << 31)
 #define FEC_COUNTER_PERIOD	(1 << 31)
 #define FEC_T_PERIOD_ONE_SEC	(1000000000UL)
+#define PPS_OUPUT_RELOAD_PERIOD	FEC_T_PERIOD_ONE_SEC
+
+/**
+ * fec_ptp_enable_pps
+ * @fep: the fec_enet_private structure handle
+ * @enable: enable the channel pps output
+ *
+ * This function enble the PPS ouput on the timer channel.
+ */
+static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
+{
+	unsigned long flags;
+	u32 val, tempval;
+	int inc;
+	struct timespec ts;
+	u64 ns;
+	u32 remainder;
+	val = 0;
+
+	if (fep->pps_channel == -1 || fep->pps_channel > MAX_TIMER_CHANNEL) {
+		dev_err(&fep->pdev->dev, "Invalid pps channel\n");
+		return -EINVAL;
+	}
+
+	if (!(fep->hwts_tx_en || fep->hwts_rx_en)) {
+		dev_err(&fep->pdev->dev, "No ptp stack is running\n");
+		return -EINVAL;
+	}
+
+	if (fep->pps_enable == enable)
+		return 0;
+
+	fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
+	inc = FEC_T_PERIOD_ONE_SEC / clk_get_rate(fep->clk_ptp);
+
+	spin_lock_irqsave(&fep->tmreg_lock, flags);
+
+	if (enable) {
+		/*
+		 * clear capture or output compare interrupt status if have.
+		 */
+		writel(FEC_T_TF_MASK, fep->hwp + FEC_TCSR(fep->pps_channel));
+
+		/*
+		 * It is recommended to doulbe check the TMODE field in the
+		 * TCSR register to be cleared before the first compare counter
+		 * is written into TCCR register. Just add a double check.
+		 */
+		val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
+		do {
+			val &= ~(FEC_T_TMODE_MASK);
+			writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
+			val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
+		} while (val & FEC_T_TMODE_MASK);
+
+		/*
+		 * Dummy read counter to update the counter
+		 */
+		timecounter_read(&fep->tc);
+		/*
+		 * linux ptp is running. We want to find the first compare event
+		 * in the next second point. So we need to know what the ptp time
+		 * is now and how many nanoseconds is ahead to get next second.
+		 * The remaining nanosecond ahead before the next second would be
+		 * FEC_T_PERIOD_ONE_SEC - ts.tv_nsec. Add the remaining nanoseconds
+		 * to current timer would be next second.
+		 */
+		tempval = readl(fep->hwp + FEC_ATIME_CTRL);
+		tempval |= FEC_T_CTRL_CAPTURE;
+		writel(tempval, fep->hwp + FEC_ATIME_CTRL);
+
+		tempval = readl(fep->hwp + FEC_ATIME);
+		/*
+		 * Converse the ptp local counter to 1588 timestamp
+		 */
+		ns = timecounter_cyc2time(&fep->tc, tempval);
+		ts.tv_sec = div_u64_rem(ns, 1000000000ULL, &remainder);
+		ts.tv_nsec = remainder;
+
+		/*
+		 * The tempval is  less than 3 seconds, and  so val is less than
+		 * 4 seconds. No overflow for 32bit calculation.
+		 */
+		val = FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec + tempval;
+
+		/* Need to consider the situation that the current time is
+		 * very close to the second point, which means FEC_T_PERIOD_ONE_SEC
+		 * - ts.tv_nsec is close to be zero(For example 20ns); Since the timer
+		 * is still running when we calculate the first compare event, it is
+		 * possible that the remaining nanoseonds run out before the compare
+		 * counter is calculated and written into TCCR register. To avoid
+		 * this possibility, we will set the compare event to be the next
+		 * of next second. The current setting is 31-bit timer and wrap
+		 * around over 2 seconds. So it is okay to set the next of next
+		 * seond for the timer.
+		 */
+		val += FEC_T_PERIOD_ONE_SEC;
+
+		/*
+		 * We add (2 *FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec) to current
+		 * ptp counter, which maybe cause 32-bit wrap. Since the
+		 * (FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec) is less than 2 second.
+		 * We can ensure the wrap will not cause issue. If the offset
+		 * is bigger than fep->cc.mask would be a error.
+		 */
+		val &= fep->cc.mask;
+		writel(val, fep->hwp + FEC_TCCR(fep->pps_channel));
+
+		/*
+		 * Calculate the second the compare event timestamp.
+		 */
+		fep->next_counter = (val + fep->reload_period) & fep->cc.mask;
+
+		/*
+		 * Enable compare event when overflow
+		 */
+		val = readl(fep->hwp + FEC_ATIME_CTRL);
+		val |= FEC_T_CTRL_PINPER;
+		writel(val, fep->hwp + FEC_ATIME_CTRL);
+
+		/*
+		 * Compare channel setting.
+		 */
+		val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
+		val |= (1 << FEC_T_TF_OFFSET | 1 << FEC_T_TIE_OFFSET);
+		val &= ~(1 << FEC_T_TDRE_OFFSET);
+		val &= ~(FEC_T_TMODE_MASK);
+		val |= (FEC_HIGH_PULSE << FEC_T_TMODE_OFFSET);
+		writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
+
+		/*
+		 * Write the second compare event timestamp and calculate
+		 * the third timestamp. Refer the TCCR register detail in the spec.
+		 */
+		writel(fep->next_counter, fep->hwp + FEC_TCCR(fep->pps_channel));
+		fep->next_counter = (fep->next_counter + fep->reload_period) & fep->cc.mask;
+	} else {
+		writel(0, fep->hwp + FEC_TCSR(fep->pps_channel));
+	}
+
+	fep->pps_enable = enable;
+	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
+
+	return 0;
+}
+
 /**
  * fec_ptp_read - read raw cycle counter (to be used by time counter)
  * @cc: the cyclecounter structure
@@ -322,6 +493,15 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
 static int fec_ptp_enable(struct ptp_clock_info *ptp,
 			  struct ptp_clock_request *rq, int on)
 {
+	struct fec_enet_private *fep =
+	    container_of(ptp, struct fec_enet_private, ptp_caps);
+	int ret = 0;
+
+	if (rq->type == PTP_CLK_REQ_PPS) {
+		ret = fec_ptp_enable_pps(fep, on);
+
+		return ret;
+	}
 	return -EOPNOTSUPP;
 }
 
@@ -427,6 +607,8 @@ void fec_ptp_init(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct device_node *np = pdev->dev.of_node;
+	int err;
 
 	fep->ptp_caps.owner = THIS_MODULE;
 	snprintf(fep->ptp_caps.name, 16, "fec ptp");
@@ -436,7 +618,7 @@ void fec_ptp_init(struct platform_device *pdev)
 	fep->ptp_caps.n_ext_ts = 0;
 	fep->ptp_caps.n_per_out = 0;
 	fep->ptp_caps.n_pins = 0;
-	fep->ptp_caps.pps = 0;
+	fep->ptp_caps.pps = 1;
 	fep->ptp_caps.adjfreq = fec_ptp_adjfreq;
 	fep->ptp_caps.adjtime = fec_ptp_adjtime;
 	fep->ptp_caps.gettime = fec_ptp_gettime;
@@ -444,6 +626,10 @@ void fec_ptp_init(struct platform_device *pdev)
 	fep->ptp_caps.enable = fec_ptp_enable;
 
 	fep->cycle_speed = clk_get_rate(fep->clk_ptp);
+	err = of_property_read_u32(np, "pps-channel",
+					&fep->pps_channel);
+	if (err)
+		fep->pps_channel = -1;
 
 	spin_lock_init(&fep->tmreg_lock);
 
@@ -459,3 +645,38 @@ void fec_ptp_init(struct platform_device *pdev)
 
 	schedule_delayed_work(&fep->time_keep, HZ);
 }
+
+/**
+ * fec_ptp_check_pps_event
+ * @fep: the fec_enet_private structure handle
+ *
+ * This function check the pps event and reload the timer compare counter.
+ */
+uint fec_ptp_check_pps_event(struct fec_enet_private *fep)
+{
+	u32 val;
+	u8 channel = fep->pps_channel;
+	struct ptp_clock_event event;
+
+	val = readl(fep->hwp + FEC_TCSR(channel));
+	if (val & FEC_T_TF_MASK) {
+		/*
+		 * Write the next next compare(not the next according the spec) value to the register
+		 */
+		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
+		do {
+			writel(val, fep->hwp + FEC_TCSR(channel));
+		} while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
+
+		/*
+		 * Update the counter;
+		 */
+		fep->next_counter = (fep->next_counter + fep->reload_period) & fep->cc.mask;
+
+		event.type = PTP_CLOCK_PPS;
+		ptp_clock_event(fep->ptp_clock, &event);
+		return 1;
+	}
+
+	return 0;
+}
-- 
1.9.1

[PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[Luwei Zhou]

This patch update fec devicetree binding doc that add Optional properties
"pps-channel".

Signed-off-by: Luwei Zhou <b45643@freescale.com>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 0c8775c..14a4fbb 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -22,6 +22,8 @@ Optional properties:
 - fsl,num-rx-queues : The property is valid for enet-avb IP, which supports
   hw multi queues. Should specify the rx queue number, otherwise set rx queue
   number to 1.
+- pps-channel: The FEC ptp support 4 channel timer. Specify the channel number
+  to output PPS signal.
 
 Optional subnodes:
 - mdio : specifies the mdio bus in the FEC, used as a container for phy nodes
-- 
1.9.1

RE: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

[Frank.Li]

> -----Original Message-----
> From: Luwei Zhou [mailto:b45643@freescale.com]
> Sent: Thursday, September 25, 2014 3:10 AM
> To: davem@davemloft.net; richardcochran@gmail.com
> Cc: netdev@vger.kernel.org; shawn.guo@linaro.org; bhutchings@solarflare.com;
> Estevam Fabio-R49496; Duan Fugang-B38611; Li Frank-B20596;
> stephen@networkplumber.org
> Subject: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP
> counter.
> 
> The FEC IP supports hardware adjustment for ptp timer. Refer to the description
> of ENET_ATCOR and ENET_ATINC registers in the spec about the hardware adjustment.
> This patch uses hardware support to adjust the ptp offset and frequency on the
> slave side.
> 
> Signed-off-by: Luwei Zhou <b45643@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> Signed-off-by: Fugang Duan <b38611@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 68 +++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 8016bdd..e2bf786 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -71,6 +71,7 @@
> 
>  #define FEC_CC_MULT	(1 << 31)
>  #define FEC_COUNTER_PERIOD	(1 << 31)
> +#define FEC_T_PERIOD_ONE_SEC	(1000000000UL)
>  /**
>   * fec_ptp_read - read raw cycle counter (to be used by time counter)
>   * @cc: the cyclecounter structure
> @@ -145,33 +146,65 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
>   */
>  static int fec_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)  {
> -	u64 diff;
>  	unsigned long flags;
>  	int neg_adj = 0;
> -	u32 mult = FEC_CC_MULT;
> +	u32 i, tmp;
> +	u32 ptp_ts_clk, ptp_inc;
> +	u32 corr_inc, corr_period;
> +	u32 corr_ns;
> 
>  	struct fec_enet_private *fep =
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
> 
> +	if (ppb == 0)
> +		return 0;
> +
>  	if (ppb < 0) {
>  		ppb = -ppb;
>  		neg_adj = 1;
>  	}
> 
> -	diff = mult;
> -	diff *= ppb;
> -	diff = div_u64(diff, 1000000000ULL);
> +	ptp_ts_clk = clk_get_rate(fep->clk_ptp);
> +	ptp_inc = FEC_T_PERIOD_ONE_SEC / ptp_ts_clk;
> +
> +	/*
> +	 * In theory, corr_inc/corr_period = ppb/FEC_T_PERIOD_ONE_SEC;
> +	 * Try to find the corr_inc  between 1 to ptp_inc to meet adjustment
> +	 * requirement.
> +	 */
> +	for (i = 1; i <= ptp_inc; i++) {
> +		if (((i * FEC_T_PERIOD_ONE_SEC) / ppb) >= ptp_inc) {
> +			corr_inc = i;
> +			corr_period = ((i * FEC_T_PERIOD_ONE_SEC) /
> +						(ptp_inc * ppb));
> +			break;
> +		}
> +	}
> +	/*
> +	 * Not found? Set it to high value - double speed
> +	 * correct in every clock step.
> +	 */
> +	if (i > ptp_inc) {
> +		corr_inc = ptp_inc;
> +		corr_period = 1;
> +	}
> +
> +	if (neg_adj)
> +		corr_ns = ptp_inc - corr_inc;
> +	else
> +		corr_ns = ptp_inc + corr_inc;
> 
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +
> +	tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
> +	tmp |= corr_ns << FEC_T_INC_CORR_OFFSET;
> +	writel(tmp, fep->hwp + FEC_ATIME_INC);
> +	writel(corr_period, fep->hwp + FEC_ATIME_CORR);
>  	/*
> -	 * dummy read to set cycle_last in tc to now.
> -	 * So use adjusted mult to calculate when next call
> -	 * timercounter_read.
> +	 * dummy read to update the timer.
>  	 */
>  	timecounter_read(&fep->tc);
> 
> -	fep->cc.mult = neg_adj ? mult - diff : mult + diff;
> -
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> 
>  	return 0;
> @@ -190,12 +223,20 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64
> delta)
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
>  	unsigned long flags;
>  	u64 now;
> +	u32 counter;
> 
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> 
>  	now = timecounter_read(&fep->tc);
>  	now += delta;
> 
> +	/*
> +	 * Get the timer value based on adjusted timestamp.
> +	 * Update the counter with the masked value.
> +	 */

Comments should like

/* Get the xxx
 * dds
 */

First empty line is not necessary. 
Please check the other place.

Did you run checkpatch script?

Best regards
Frank Li
	
> +	counter = now & fep->cc.mask;
> +	writel(counter, fep->hwp + FEC_ATIME);
> +
>  	/* reset the timecounter */
>  	timecounter_init(&fep->tc, &fep->cc, now);
> 
> @@ -246,6 +287,7 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
> 
>  	u64 ns;
>  	unsigned long flags;
> +	u32 counter;
> 
>  	mutex_lock(&fep->ptp_clk_mutex);
>  	/* Check the ptp clock */
> @@ -256,8 +298,14 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
> 
>  	ns = ts->tv_sec * 1000000000ULL;
>  	ns += ts->tv_nsec;
> +	/*
> +	 * Get the timer value based on timestamp.
> +	 * Update the counter with the masked value.
> +	 */
> +	counter = ns & fep->cc.mask;
> 
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +	writel(counter, fep->hwp + FEC_ATIME);
>  	timecounter_init(&fep->tc, &fep->cc, ns);
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  	mutex_unlock(&fep->ptp_clk_mutex);
> --
> 1.9.1

Re: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

[Richard Cochran]

On Thu, Sep 25, 2014 at 04:10:19PM +0800, Luwei Zhou wrote:
> The FEC IP supports hardware adjustment for ptp timer. Refer to the description of
> ENET_ATCOR and ENET_ATINC registers in the spec about the hardware adjustment. This
> patch uses hardware support to adjust the ptp offset and frequency on the slave side.
> 
> Signed-off-by: Luwei Zhou <b45643@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> Signed-off-by: Fugang Duan <b38611@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 68 +++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 8016bdd..e2bf786 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -71,6 +71,7 @@
>  
>  #define FEC_CC_MULT	(1 << 31)
>  #define FEC_COUNTER_PERIOD	(1 << 31)
> +#define FEC_T_PERIOD_ONE_SEC	(1000000000UL)

Why not use NSEC_PER_USEC?

>  /**
>   * fec_ptp_read - read raw cycle counter (to be used by time counter)
>   * @cc: the cyclecounter structure
> @@ -145,33 +146,65 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
>   */
>  static int fec_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
>  {
> -	u64 diff;
>  	unsigned long flags;
>  	int neg_adj = 0;
> -	u32 mult = FEC_CC_MULT;
> +	u32 i, tmp;
> +	u32 ptp_ts_clk, ptp_inc;
> +	u32 corr_inc, corr_period;
> +	u32 corr_ns;
>  
>  	struct fec_enet_private *fep =
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
>  
> +	if (ppb == 0)
> +		return 0;
> +
>  	if (ppb < 0) {
>  		ppb = -ppb;
>  		neg_adj = 1;
>  	}
>  
> -	diff = mult;
> -	diff *= ppb;
> -	diff = div_u64(diff, 1000000000ULL);
> +	ptp_ts_clk = clk_get_rate(fep->clk_ptp);
> +	ptp_inc = FEC_T_PERIOD_ONE_SEC / ptp_ts_clk;

No need to calculate this every time.

> +
> +	/*
> +	 * In theory, corr_inc/corr_period = ppb/FEC_T_PERIOD_ONE_SEC;
> +	 * Try to find the corr_inc  between 1 to ptp_inc to meet adjustment
> +	 * requirement.
> +	 */
> +	for (i = 1; i <= ptp_inc; i++) {
> +		if (((i * FEC_T_PERIOD_ONE_SEC) / ppb) >= ptp_inc) {

Does this code even work?

(i * FEC_T_PERIOD_ONE_SEC) overflows when i > 4.

Does this code even compile? You don't use div_u64().

Also, remember that this code is a performance critical path. You have
a 64 bit division in every loop iteration. With a high Sync rate, this
function will be called many times per second. You should really
optimize here.

Instead of testing for

	i * FEC_T_PERIOD_ONE_SEC / ppb >= ptp_inc

why not something like

	u64 lhs = NSEC_PER_USEC, rhs = ptp_inc * ppb;

	for (i = 1; i <= ptp_inc; i++) {
		if (lhs >= rhs) {
			...
		}
		lhs += NSEC_PER_USEC;
	}

instead?

> +			corr_inc = i;
> +			corr_period = ((i * FEC_T_PERIOD_ONE_SEC) /
> +						(ptp_inc * ppb));

32 bit overflow again.

> +			break;
> +		}
> +	}
> +	/*
> +	 * Not found? Set it to high value - double speed
> +	 * correct in every clock step.
> +	 */
> +	if (i > ptp_inc) {
> +		corr_inc = ptp_inc;
> +		corr_period = 1;
> +	}
> +
> +	if (neg_adj)
> +		corr_ns = ptp_inc - corr_inc;
> +	else
> +		corr_ns = ptp_inc + corr_inc;
>  
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +
> +	tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
> +	tmp |= corr_ns << FEC_T_INC_CORR_OFFSET;
> +	writel(tmp, fep->hwp + FEC_ATIME_INC);
> +	writel(corr_period, fep->hwp + FEC_ATIME_CORR);
>  	/*
> -	 * dummy read to set cycle_last in tc to now.
> -	 * So use adjusted mult to calculate when next call
> -	 * timercounter_read.
> +	 * dummy read to update the timer.
>  	 */
>  	timecounter_read(&fep->tc);
>  
> -	fep->cc.mult = neg_adj ? mult - diff : mult + diff;
> -
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  
>  	return 0;
> @@ -190,12 +223,20 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
>  	unsigned long flags;
>  	u64 now;
> +	u32 counter;
>  
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
>  
>  	now = timecounter_read(&fep->tc);
>  	now += delta;
>  
> +	/*
> +	 * Get the timer value based on adjusted timestamp.
> +	 * Update the counter with the masked value.
> +	 */
> +	counter = now & fep->cc.mask;
> +	writel(counter, fep->hwp + FEC_ATIME);

Why is this needed?

Thanks,
Richard


> +
>  	/* reset the timecounter */
>  	timecounter_init(&fep->tc, &fep->cc, now);
>  
> @@ -246,6 +287,7 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
>  
>  	u64 ns;
>  	unsigned long flags;
> +	u32 counter;
>  
>  	mutex_lock(&fep->ptp_clk_mutex);
>  	/* Check the ptp clock */
> @@ -256,8 +298,14 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
>  
>  	ns = ts->tv_sec * 1000000000ULL;
>  	ns += ts->tv_nsec;
> +	/*
> +	 * Get the timer value based on timestamp.
> +	 * Update the counter with the masked value.
> +	 */
> +	counter = ns & fep->cc.mask;
>  
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +	writel(counter, fep->hwp + FEC_ATIME);
>  	timecounter_init(&fep->tc, &fep->cc, ns);
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  	mutex_unlock(&fep->ptp_clk_mutex);
> -- 
> 1.9.1
> 

Re: [PATCH v1 3/4] net: fec: ptp: Enalbe PPS ouput based on ptp clock

[Richard Cochran]

On Thu, Sep 25, 2014 at 04:10:20PM +0800, Luwei Zhou wrote:
> FEC ptp timer has 4 channel compare/trigger function. It can be used to enable pps output.
> The pulse would be ouput high exactly on N second. The pulse ouput high on compare event mode
> is used to produce pulse per second.  The pulse width would be one cycle based on ptp timer clock
> source.Since 31-bit ptp hardware timer is used, the timer will wrap more than 2 seconds. We need to
> reload the compare compare event about every 1 second.
> 
> Signed-off-by: Luwei Zhou <b45643@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec.h      |   7 +
>  drivers/net/ethernet/freescale/fec_main.c |   2 +
>  drivers/net/ethernet/freescale/fec_ptp.c  | 223 +++++++++++++++++++++++++++++-
>  3 files changed, 231 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 26fb1de..f8e23e6 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -481,12 +481,19 @@ struct fec_enet_private {
>  	unsigned int tx_pkts_itr;
>  	unsigned int tx_time_itr;
>  	unsigned int itr_clk_rate;
> +
> +	/* pps  */
> +	int pps_channel;
> +	unsigned int reload_period;
> +	int pps_enable;
> +	unsigned int next_counter;
>  };
>  
>  void fec_ptp_init(struct platform_device *pdev);
>  void fec_ptp_start_cyclecounter(struct net_device *ndev);
>  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
>  int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
> +uint fec_ptp_check_pps_event(struct fec_enet_private *fep);
>  
>  /****************************************************************************/
>  #endif /* FEC_H */
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 3a4ec0f..5a4ce36 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1558,6 +1558,8 @@ fec_enet_interrupt(int irq, void *dev_id)
>  		complete(&fep->mdio_done);
>  	}
>  
> +	fec_ptp_check_pps_event(fep);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index e2bf786..c55239d 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -61,6 +61,24 @@
>  #define FEC_T_INC_CORR_MASK             0x00007f00
>  #define FEC_T_INC_CORR_OFFSET           8
>  
> +#define FEC_T_CTRL_PINPER		0x00000080
> +#define FEC_T_TF0_MASK			0x00000001
> +#define FEC_T_TF0_OFFSET		0
> +#define FEC_T_TF1_MASK			0x00000002
> +#define FEC_T_TF1_OFFSET		1
> +#define FEC_T_TF2_MASK			0x00000004
> +#define FEC_T_TF2_OFFSET		2
> +#define FEC_T_TF3_MASK			0x00000008
> +#define FEC_T_TF3_OFFSET		3
> +#define FEC_T_TDRE_MASK			0x00000001
> +#define FEC_T_TDRE_OFFSET		0
> +#define FEC_T_TMODE_MASK		0x0000003C
> +#define FEC_T_TMODE_OFFSET		2
> +#define FEC_T_TIE_MASK			0x00000040
> +#define FEC_T_TIE_OFFSET		6
> +#define FEC_T_TF_MASK			0x00000080
> +#define FEC_T_TF_OFFSET			7
> +
>  #define FEC_ATIME_CTRL		0x400
>  #define FEC_ATIME		0x404
>  #define FEC_ATIME_EVT_OFFSET	0x408
> @@ -69,9 +87,162 @@
>  #define FEC_ATIME_INC		0x414
>  #define FEC_TS_TIMESTAMP	0x418
>  
> +#define FEC_TGSR		0x604
> +#define FEC_TCSR(n)		(0x608 + n * 0x08)
> +#define FEC_TCCR(n)		(0x60C + n * 0x08)
> +#define MAX_TIMER_CHANNEL	3
> +#define FEC_TMODE_TOGGLE	0x05
> +#define FEC_HIGH_PULSE		0x0F
> +
>  #define FEC_CC_MULT	(1 << 31)
>  #define FEC_COUNTER_PERIOD	(1 << 31)
>  #define FEC_T_PERIOD_ONE_SEC	(1000000000UL)
> +#define PPS_OUPUT_RELOAD_PERIOD	FEC_T_PERIOD_ONE_SEC
> +
> +/**
> + * fec_ptp_enable_pps
> + * @fep: the fec_enet_private structure handle
> + * @enable: enable the channel pps output
> + *
> + * This function enble the PPS ouput on the timer channel.
> + */
> +static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
> +{
> +	unsigned long flags;
> +	u32 val, tempval;
> +	int inc;
> +	struct timespec ts;
> +	u64 ns;
> +	u32 remainder;
> +	val = 0;
> +
> +	if (fep->pps_channel == -1 || fep->pps_channel > MAX_TIMER_CHANNEL) {
> +		dev_err(&fep->pdev->dev, "Invalid pps channel\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!(fep->hwts_tx_en || fep->hwts_rx_en)) {
> +		dev_err(&fep->pdev->dev, "No ptp stack is running\n");
> +		return -EINVAL;
> +	}
> +
> +	if (fep->pps_enable == enable)
> +		return 0;
> +
> +	fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
> +	inc = FEC_T_PERIOD_ONE_SEC / clk_get_rate(fep->clk_ptp);
> +
> +	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +
> +	if (enable) {
> +		/*
> +		 * clear capture or output compare interrupt status if have.
> +		 */
> +		writel(FEC_T_TF_MASK, fep->hwp + FEC_TCSR(fep->pps_channel));
> +
> +		/*
> +		 * It is recommended to doulbe check the TMODE field in the
> +		 * TCSR register to be cleared before the first compare counter
> +		 * is written into TCCR register. Just add a double check.
> +		 */
> +		val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> +		do {
> +			val &= ~(FEC_T_TMODE_MASK);
> +			writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
> +			val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> +		} while (val & FEC_T_TMODE_MASK);
> +
> +		/*
> +		 * Dummy read counter to update the counter
> +		 */
> +		timecounter_read(&fep->tc);
> +		/*
> +		 * linux ptp is running. We want to find the first compare event
                   ^^^^^^^^^^^^^^^^^^^^
What does this mean?

> +		 * in the next second point. So we need to know what the ptp time
> +		 * is now and how many nanoseconds is ahead to get next second.
> +		 * The remaining nanosecond ahead before the next second would be
> +		 * FEC_T_PERIOD_ONE_SEC - ts.tv_nsec. Add the remaining nanoseconds
> +		 * to current timer would be next second.
> +		 */
> +		tempval = readl(fep->hwp + FEC_ATIME_CTRL);
> +		tempval |= FEC_T_CTRL_CAPTURE;
> +		writel(tempval, fep->hwp + FEC_ATIME_CTRL);
> +
> +		tempval = readl(fep->hwp + FEC_ATIME);
> +		/*
> +		 * Converse the ptp local counter to 1588 timestamp
                   ^^^^^^^^
Convert?

> +		 */
> +		ns = timecounter_cyc2time(&fep->tc, tempval);
> +		ts.tv_sec = div_u64_rem(ns, 1000000000ULL, &remainder);
> +		ts.tv_nsec = remainder;
> +
> +		/*
> +		 * The tempval is  less than 3 seconds, and  so val is less than
> +		 * 4 seconds. No overflow for 32bit calculation.
> +		 */
> +		val = FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec + tempval;
> +
> +		/* Need to consider the situation that the current time is
> +		 * very close to the second point, which means FEC_T_PERIOD_ONE_SEC
> +		 * - ts.tv_nsec is close to be zero(For example 20ns); Since the timer
> +		 * is still running when we calculate the first compare event, it is
> +		 * possible that the remaining nanoseonds run out before the compare
> +		 * counter is calculated and written into TCCR register. To avoid
> +		 * this possibility, we will set the compare event to be the next
> +		 * of next second. The current setting is 31-bit timer and wrap
> +		 * around over 2 seconds. So it is okay to set the next of next
> +		 * seond for the timer.
> +		 */
> +		val += FEC_T_PERIOD_ONE_SEC;
> +
> +		/*
> +		 * We add (2 *FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec) to current
> +		 * ptp counter, which maybe cause 32-bit wrap. Since the
> +		 * (FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec) is less than 2 second.
> +		 * We can ensure the wrap will not cause issue. If the offset
> +		 * is bigger than fep->cc.mask would be a error.
> +		 */
> +		val &= fep->cc.mask;
> +		writel(val, fep->hwp + FEC_TCCR(fep->pps_channel));
> +
> +		/*
> +		 * Calculate the second the compare event timestamp.
> +		 */
> +		fep->next_counter = (val + fep->reload_period) & fep->cc.mask;
> +
> +		/*
> +		 * Enable compare event when overflow
> +		 */
> +		val = readl(fep->hwp + FEC_ATIME_CTRL);
> +		val |= FEC_T_CTRL_PINPER;
> +		writel(val, fep->hwp + FEC_ATIME_CTRL);
> +
> +		/*
> +		 * Compare channel setting.
> +		 */
> +		val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> +		val |= (1 << FEC_T_TF_OFFSET | 1 << FEC_T_TIE_OFFSET);
> +		val &= ~(1 << FEC_T_TDRE_OFFSET);
> +		val &= ~(FEC_T_TMODE_MASK);
> +		val |= (FEC_HIGH_PULSE << FEC_T_TMODE_OFFSET);
> +		writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
> +
> +		/*
> +		 * Write the second compare event timestamp and calculate
> +		 * the third timestamp. Refer the TCCR register detail in the spec.
> +		 */
> +		writel(fep->next_counter, fep->hwp + FEC_TCCR(fep->pps_channel));
> +		fep->next_counter = (fep->next_counter + fep->reload_period) & fep->cc.mask;
> +	} else {
> +		writel(0, fep->hwp + FEC_TCSR(fep->pps_channel));
> +	}
> +
> +	fep->pps_enable = enable;
> +	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> +
> +	return 0;
> +}
> +
>  /**
>   * fec_ptp_read - read raw cycle counter (to be used by time counter)
>   * @cc: the cyclecounter structure
> @@ -322,6 +493,15 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
>  static int fec_ptp_enable(struct ptp_clock_info *ptp,
>  			  struct ptp_clock_request *rq, int on)
>  {
> +	struct fec_enet_private *fep =
> +	    container_of(ptp, struct fec_enet_private, ptp_caps);
> +	int ret = 0;
> +
> +	if (rq->type == PTP_CLK_REQ_PPS) {
> +		ret = fec_ptp_enable_pps(fep, on);
> +
> +		return ret;
> +	}
>  	return -EOPNOTSUPP;
>  }
>  
> @@ -427,6 +607,8 @@ void fec_ptp_init(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct device_node *np = pdev->dev.of_node;
> +	int err;
>  
>  	fep->ptp_caps.owner = THIS_MODULE;
>  	snprintf(fep->ptp_caps.name, 16, "fec ptp");
> @@ -436,7 +618,7 @@ void fec_ptp_init(struct platform_device *pdev)
>  	fep->ptp_caps.n_ext_ts = 0;
>  	fep->ptp_caps.n_per_out = 0;
>  	fep->ptp_caps.n_pins = 0;
> -	fep->ptp_caps.pps = 0;
> +	fep->ptp_caps.pps = 1;
>  	fep->ptp_caps.adjfreq = fec_ptp_adjfreq;
>  	fep->ptp_caps.adjtime = fec_ptp_adjtime;
>  	fep->ptp_caps.gettime = fec_ptp_gettime;
> @@ -444,6 +626,10 @@ void fec_ptp_init(struct platform_device *pdev)
>  	fep->ptp_caps.enable = fec_ptp_enable;
>  
>  	fep->cycle_speed = clk_get_rate(fep->clk_ptp);
> +	err = of_property_read_u32(np, "pps-channel",
> +					&fep->pps_channel);

Please do not make another DT property for this.

Instead, use the PTP_PIN_SET/GETFUNC interface. You can add a new
enumeration value to ptp_pin_function, like PTP_PF_PPS.

Thanks,
Richard


> +	if (err)
> +		fep->pps_channel = -1;
>  
>  	spin_lock_init(&fep->tmreg_lock);
>  
> @@ -459,3 +645,38 @@ void fec_ptp_init(struct platform_device *pdev)
>  
>  	schedule_delayed_work(&fep->time_keep, HZ);
>  }
> +
> +/**
> + * fec_ptp_check_pps_event
> + * @fep: the fec_enet_private structure handle
> + *
> + * This function check the pps event and reload the timer compare counter.
> + */
> +uint fec_ptp_check_pps_event(struct fec_enet_private *fep)
> +{
> +	u32 val;
> +	u8 channel = fep->pps_channel;
> +	struct ptp_clock_event event;
> +
> +	val = readl(fep->hwp + FEC_TCSR(channel));
> +	if (val & FEC_T_TF_MASK) {
> +		/*
> +		 * Write the next next compare(not the next according the spec) value to the register
> +		 */
> +		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
> +		do {
> +			writel(val, fep->hwp + FEC_TCSR(channel));
> +		} while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
> +
> +		/*
> +		 * Update the counter;
> +		 */
> +		fep->next_counter = (fep->next_counter + fep->reload_period) & fep->cc.mask;
> +
> +		event.type = PTP_CLOCK_PPS;
> +		ptp_clock_event(fep->ptp_clock, &event);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> -- 
> 1.9.1
> 

Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[Richard Cochran]

On Thu, Sep 25, 2014 at 04:10:21PM +0800, Luwei Zhou wrote:
> This patch update fec devicetree binding doc that add Optional properties
> "pps-channel".

Again, use the PTP pin interface. We don't need a random new FEC DT
property for this.

Thanks,
Richard

RE: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

[luwei.zhou]

Hi Frank,

I ran checkpatch and it can pass. I will update it in the next version.

Luwei
Best Regards

-----Original Message-----
From: Li Frank-B20596 
Sent: Thursday, September 25, 2014 9:30 PM
To: Zhou Luwei-B45643; davem@davemloft.net; richardcochran@gmail.com
Cc: netdev@vger.kernel.org; shawn.guo@linaro.org; bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; stephen@networkplumber.org
Subject: RE: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.



> -----Original Message-----
> From: Luwei Zhou [mailto:b45643@freescale.com]
> Sent: Thursday, September 25, 2014 3:10 AM
> To: davem@davemloft.net; richardcochran@gmail.com
> Cc: netdev@vger.kernel.org; shawn.guo@linaro.org; 
> bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; 
> Li Frank-B20596; stephen@networkplumber.org
> Subject: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to 
> adjust PTP counter.
> 
> The FEC IP supports hardware adjustment for ptp timer. Refer to the 
> description of ENET_ATCOR and ENET_ATINC registers in the spec about the hardware adjustment.
> This patch uses hardware support to adjust the ptp offset and 
> frequency on the slave side.
> 
> Signed-off-by: Luwei Zhou <b45643@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> Signed-off-by: Fugang Duan <b38611@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 68 
> +++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 8016bdd..e2bf786 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -71,6 +71,7 @@
> 
>  #define FEC_CC_MULT	(1 << 31)
>  #define FEC_COUNTER_PERIOD	(1 << 31)
> +#define FEC_T_PERIOD_ONE_SEC	(1000000000UL)
>  /**
>   * fec_ptp_read - read raw cycle counter (to be used by time counter)
>   * @cc: the cyclecounter structure
> @@ -145,33 +146,65 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
>   */
>  static int fec_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)  {
> -	u64 diff;
>  	unsigned long flags;
>  	int neg_adj = 0;
> -	u32 mult = FEC_CC_MULT;
> +	u32 i, tmp;
> +	u32 ptp_ts_clk, ptp_inc;
> +	u32 corr_inc, corr_period;
> +	u32 corr_ns;
> 
>  	struct fec_enet_private *fep =
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
> 
> +	if (ppb == 0)
> +		return 0;
> +
>  	if (ppb < 0) {
>  		ppb = -ppb;
>  		neg_adj = 1;
>  	}
> 
> -	diff = mult;
> -	diff *= ppb;
> -	diff = div_u64(diff, 1000000000ULL);
> +	ptp_ts_clk = clk_get_rate(fep->clk_ptp);
> +	ptp_inc = FEC_T_PERIOD_ONE_SEC / ptp_ts_clk;
> +
> +	/*
> +	 * In theory, corr_inc/corr_period = ppb/FEC_T_PERIOD_ONE_SEC;
> +	 * Try to find the corr_inc  between 1 to ptp_inc to meet adjustment
> +	 * requirement.
> +	 */
> +	for (i = 1; i <= ptp_inc; i++) {
> +		if (((i * FEC_T_PERIOD_ONE_SEC) / ppb) >= ptp_inc) {
> +			corr_inc = i;
> +			corr_period = ((i * FEC_T_PERIOD_ONE_SEC) /
> +						(ptp_inc * ppb));
> +			break;
> +		}
> +	}
> +	/*
> +	 * Not found? Set it to high value - double speed
> +	 * correct in every clock step.
> +	 */
> +	if (i > ptp_inc) {
> +		corr_inc = ptp_inc;
> +		corr_period = 1;
> +	}
> +
> +	if (neg_adj)
> +		corr_ns = ptp_inc - corr_inc;
> +	else
> +		corr_ns = ptp_inc + corr_inc;
> 
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +
> +	tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
> +	tmp |= corr_ns << FEC_T_INC_CORR_OFFSET;
> +	writel(tmp, fep->hwp + FEC_ATIME_INC);
> +	writel(corr_period, fep->hwp + FEC_ATIME_CORR);
>  	/*
> -	 * dummy read to set cycle_last in tc to now.
> -	 * So use adjusted mult to calculate when next call
> -	 * timercounter_read.
> +	 * dummy read to update the timer.
>  	 */
>  	timecounter_read(&fep->tc);
> 
> -	fep->cc.mult = neg_adj ? mult - diff : mult + diff;
> -
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> 
>  	return 0;
> @@ -190,12 +223,20 @@ static int fec_ptp_adjtime(struct ptp_clock_info 
> *ptp, s64
> delta)
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
>  	unsigned long flags;
>  	u64 now;
> +	u32 counter;
> 
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> 
>  	now = timecounter_read(&fep->tc);
>  	now += delta;
> 
> +	/*
> +	 * Get the timer value based on adjusted timestamp.
> +	 * Update the counter with the masked value.
> +	 */

Comments should like

/* Get the xxx
 * dds
 */

First empty line is not necessary. 
Please check the other place.

Did you run checkpatch script?

Best regards
Frank Li
	
> +	counter = now & fep->cc.mask;
> +	writel(counter, fep->hwp + FEC_ATIME);
> +
>  	/* reset the timecounter */
>  	timecounter_init(&fep->tc, &fep->cc, now);
> 
> @@ -246,6 +287,7 @@ static int fec_ptp_settime(struct ptp_clock_info 
> *ptp,
> 
>  	u64 ns;
>  	unsigned long flags;
> +	u32 counter;
> 
>  	mutex_lock(&fep->ptp_clk_mutex);
>  	/* Check the ptp clock */
> @@ -256,8 +298,14 @@ static int fec_ptp_settime(struct ptp_clock_info 
> *ptp,
> 
>  	ns = ts->tv_sec * 1000000000ULL;
>  	ns += ts->tv_nsec;
> +	/*
> +	 * Get the timer value based on timestamp.
> +	 * Update the counter with the masked value.
> +	 */
> +	counter = ns & fep->cc.mask;
> 
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +	writel(counter, fep->hwp + FEC_ATIME);
>  	timecounter_init(&fep->tc, &fep->cc, ns);
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  	mutex_unlock(&fep->ptp_clk_mutex);
> --
> 1.9.1

RE: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

[luwei.zhou]

Hi Richard,

See my feedback below. Thanks!

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com] 
Sent: Thursday, September 25, 2014 10:29 PM
To: Zhou Luwei-B45643
Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org; bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li Frank-B20596; stephen@networkplumber.org
Subject: Re: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

On Thu, Sep 25, 2014 at 04:10:19PM +0800, Luwei Zhou wrote:
> The FEC IP supports hardware adjustment for ptp timer. Refer to the 
> description of ENET_ATCOR and ENET_ATINC registers in the spec about 
> the hardware adjustment. This patch uses hardware support to adjust the ptp offset and frequency on the slave side.
> 
> Signed-off-by: Luwei Zhou <b45643@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> Signed-off-by: Fugang Duan <b38611@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec_ptp.c | 68 
> +++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c 
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 8016bdd..e2bf786 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -71,6 +71,7 @@
>  
>  #define FEC_CC_MULT	(1 << 31)
>  #define FEC_COUNTER_PERIOD	(1 << 31)
> +#define FEC_T_PERIOD_ONE_SEC	(1000000000UL)

Why not use NSEC_PER_USEC?
[Zhou Luwei-B45643] Will update in the next version.

>  /**
>   * fec_ptp_read - read raw cycle counter (to be used by time counter)
>   * @cc: the cyclecounter structure
> @@ -145,33 +146,65 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
>   */
>  static int fec_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)  {
> -	u64 diff;
>  	unsigned long flags;
>  	int neg_adj = 0;
> -	u32 mult = FEC_CC_MULT;
> +	u32 i, tmp;
> +	u32 ptp_ts_clk, ptp_inc;
> +	u32 corr_inc, corr_period;
> +	u32 corr_ns;
>  
>  	struct fec_enet_private *fep =
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
>  
> +	if (ppb == 0)
> +		return 0;
> +
>  	if (ppb < 0) {
>  		ppb = -ppb;
>  		neg_adj = 1;
>  	}
>  
> -	diff = mult;
> -	diff *= ppb;
> -	diff = div_u64(diff, 1000000000ULL);
> +	ptp_ts_clk = clk_get_rate(fep->clk_ptp);
> +	ptp_inc = FEC_T_PERIOD_ONE_SEC / ptp_ts_clk;

No need to calculate this every time.
[Zhou Luwei-B45643] Will update in the next version.

> +
> +	/*
> +	 * In theory, corr_inc/corr_period = ppb/FEC_T_PERIOD_ONE_SEC;
> +	 * Try to find the corr_inc  between 1 to ptp_inc to meet adjustment
> +	 * requirement.
> +	 */
> +	for (i = 1; i <= ptp_inc; i++) {
> +		if (((i * FEC_T_PERIOD_ONE_SEC) / ppb) >= ptp_inc) {

Does this code even work?

(i * FEC_T_PERIOD_ONE_SEC) overflows when i > 4.

Does this code even compile? You don't use div_u64().

Also, remember that this code is a performance critical path. You have a 64 bit division in every loop iteration. With a high Sync rate, this function will be called many times per second. You should really optimize here.

Instead of testing for

	i * FEC_T_PERIOD_ONE_SEC / ppb >= ptp_inc

why not something like

	u64 lhs = NSEC_PER_USEC, rhs = ptp_inc * ppb;

	for (i = 1; i <= ptp_inc; i++) {
		if (lhs >= rhs) {
			...
		}
		lhs += NSEC_PER_USEC;
	}

instead?
[Zhou Luwei-B45643] Thanks. It can pass compile without warning and test. In fact I used other tools to increase SYNC rate to 16 per second and it works well. The overflow issue didn't show up maybe because the "i" didn't reach 4 when running the test. I will fix the overflow issue and avoid the div as you mention in the next version.

> +			corr_inc = i;
> +			corr_period = ((i * FEC_T_PERIOD_ONE_SEC) /
> +						(ptp_inc * ppb));

32 bit overflow again.

> +			break;
> +		}
> +	}
> +	/*
> +	 * Not found? Set it to high value - double speed
> +	 * correct in every clock step.
> +	 */
> +	if (i > ptp_inc) {
> +		corr_inc = ptp_inc;
> +		corr_period = 1;
> +	}
> +
> +	if (neg_adj)
> +		corr_ns = ptp_inc - corr_inc;
> +	else
> +		corr_ns = ptp_inc + corr_inc;
>  
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +
> +	tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
> +	tmp |= corr_ns << FEC_T_INC_CORR_OFFSET;
> +	writel(tmp, fep->hwp + FEC_ATIME_INC);
> +	writel(corr_period, fep->hwp + FEC_ATIME_CORR);
>  	/*
> -	 * dummy read to set cycle_last in tc to now.
> -	 * So use adjusted mult to calculate when next call
> -	 * timercounter_read.
> +	 * dummy read to update the timer.
>  	 */
>  	timecounter_read(&fep->tc);
>  
> -	fep->cc.mult = neg_adj ? mult - diff : mult + diff;
> -
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  
>  	return 0;
> @@ -190,12 +223,20 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
>  	unsigned long flags;
>  	u64 now;
> +	u32 counter;
>  
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
>  
>  	now = timecounter_read(&fep->tc);
>  	now += delta;
>  
> +	/*
> +	 * Get the timer value based on adjusted timestamp.
> +	 * Update the counter with the masked value.
> +	 */
> +	counter = now & fep->cc.mask;
> +	writel(counter, fep->hwp + FEC_ATIME);

Why is this needed?

Thanks,
Richard

[Zhou Luwei-B45643] This added to support more accurate pps output on the slave side.  The PTP slave side need to adjust offset to sync with master. But the next compare event counter has
Been written to the hardware register and can't modify when slave side need to be adjustment. I will give a example below:

Assuming the next compare event has been set to be 500ns to trigger a  PPS when global PTP time reach 5 second .  When PTP timer run to 200ns, the running PTP stack on the slave side needs to
Adjust 100ns offset after SYNC. If FEC_ATIME is not adjusted with ptp offset, the next PPS event would be triggered at 5second + 100ns not exactly 5 second. 

What I am trying to is to make "ptp timestamp & cc.mask = PTP timer value".

> +
>  	/* reset the timecounter */
>  	timecounter_init(&fep->tc, &fep->cc, now);
>  
> @@ -246,6 +287,7 @@ static int fec_ptp_settime(struct ptp_clock_info 
> *ptp,
>  
>  	u64 ns;
>  	unsigned long flags;
> +	u32 counter;
>  
>  	mutex_lock(&fep->ptp_clk_mutex);
>  	/* Check the ptp clock */
> @@ -256,8 +298,14 @@ static int fec_ptp_settime(struct ptp_clock_info 
> *ptp,
>  
>  	ns = ts->tv_sec * 1000000000ULL;
>  	ns += ts->tv_nsec;
> +	/*
> +	 * Get the timer value based on timestamp.
> +	 * Update the counter with the masked value.
> +	 */
> +	counter = ns & fep->cc.mask;
>  
>  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> +	writel(counter, fep->hwp + FEC_ATIME);
>  	timecounter_init(&fep->tc, &fep->cc, ns);
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  	mutex_unlock(&fep->ptp_clk_mutex);
> --
> 1.9.1
> 

RE: [PATCH v1 3/4] net: fec: ptp: Enalbe PPS ouput based on ptp clock

[luwei.zhou]

Richard,
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, September 25, 2014 10:42 PM
> To: Zhou Luwei-B45643
> Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org;
> bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li
> Frank-B20596; stephen@networkplumber.org
> Subject: Re: [PATCH v1 3/4] net: fec: ptp: Enalbe PPS ouput based on ptp
> clock
> 
> On Thu, Sep 25, 2014 at 04:10:20PM +0800, Luwei Zhou wrote:
> > FEC ptp timer has 4 channel compare/trigger function. It can be used to
> enable pps output.
> > The pulse would be ouput high exactly on N second. The pulse ouput
> > high on compare event mode is used to produce pulse per second.  The
> > pulse width would be one cycle based on ptp timer clock source.Since
> > 31-bit ptp hardware timer is used, the timer will wrap more than 2
> seconds. We need to reload the compare compare event about every 1 second.
> >
> > Signed-off-by: Luwei Zhou <b45643@freescale.com>
> > ---
> >  drivers/net/ethernet/freescale/fec.h      |   7 +
> >  drivers/net/ethernet/freescale/fec_main.c |   2 +
> >  drivers/net/ethernet/freescale/fec_ptp.c  | 223
> > +++++++++++++++++++++++++++++-
> >  3 files changed, 231 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec.h
> > b/drivers/net/ethernet/freescale/fec.h
> > index 26fb1de..f8e23e6 100644
> > --- a/drivers/net/ethernet/freescale/fec.h
> > +++ b/drivers/net/ethernet/freescale/fec.h
> > @@ -481,12 +481,19 @@ struct fec_enet_private {
> >  	unsigned int tx_pkts_itr;
> >  	unsigned int tx_time_itr;
> >  	unsigned int itr_clk_rate;
> > +
> > +	/* pps  */
> > +	int pps_channel;
> > +	unsigned int reload_period;
> > +	int pps_enable;
> > +	unsigned int next_counter;
> >  };
> >
> >  void fec_ptp_init(struct platform_device *pdev);  void
> > fec_ptp_start_cyclecounter(struct net_device *ndev);  int
> > fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);  int
> > fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
> > +uint fec_ptp_check_pps_event(struct fec_enet_private *fep);
> >
> >
> > /*********************************************************************
> > *******/
> >  #endif /* FEC_H */
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 3a4ec0f..5a4ce36 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1558,6 +1558,8 @@ fec_enet_interrupt(int irq, void *dev_id)
> >  		complete(&fep->mdio_done);
> >  	}
> >
> > +	fec_ptp_check_pps_event(fep);
> > +
> >  	return ret;
> >  }
> >
> > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> > b/drivers/net/ethernet/freescale/fec_ptp.c
> > index e2bf786..c55239d 100644
> > --- a/drivers/net/ethernet/freescale/fec_ptp.c
> > +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> > @@ -61,6 +61,24 @@
> >  #define FEC_T_INC_CORR_MASK             0x00007f00
> >  #define FEC_T_INC_CORR_OFFSET           8
> >
> > +#define FEC_T_CTRL_PINPER		0x00000080
> > +#define FEC_T_TF0_MASK			0x00000001
> > +#define FEC_T_TF0_OFFSET		0
> > +#define FEC_T_TF1_MASK			0x00000002
> > +#define FEC_T_TF1_OFFSET		1
> > +#define FEC_T_TF2_MASK			0x00000004
> > +#define FEC_T_TF2_OFFSET		2
> > +#define FEC_T_TF3_MASK			0x00000008
> > +#define FEC_T_TF3_OFFSET		3
> > +#define FEC_T_TDRE_MASK			0x00000001
> > +#define FEC_T_TDRE_OFFSET		0
> > +#define FEC_T_TMODE_MASK		0x0000003C
> > +#define FEC_T_TMODE_OFFSET		2
> > +#define FEC_T_TIE_MASK			0x00000040
> > +#define FEC_T_TIE_OFFSET		6
> > +#define FEC_T_TF_MASK			0x00000080
> > +#define FEC_T_TF_OFFSET			7
> > +
> >  #define FEC_ATIME_CTRL		0x400
> >  #define FEC_ATIME		0x404
> >  #define FEC_ATIME_EVT_OFFSET	0x408
> > @@ -69,9 +87,162 @@
> >  #define FEC_ATIME_INC		0x414
> >  #define FEC_TS_TIMESTAMP	0x418
> >
> > +#define FEC_TGSR		0x604
> > +#define FEC_TCSR(n)		(0x608 + n * 0x08)
> > +#define FEC_TCCR(n)		(0x60C + n * 0x08)
> > +#define MAX_TIMER_CHANNEL	3
> > +#define FEC_TMODE_TOGGLE	0x05
> > +#define FEC_HIGH_PULSE		0x0F
> > +
> >  #define FEC_CC_MULT	(1 << 31)
> >  #define FEC_COUNTER_PERIOD	(1 << 31)
> >  #define FEC_T_PERIOD_ONE_SEC	(1000000000UL)
> > +#define PPS_OUPUT_RELOAD_PERIOD	FEC_T_PERIOD_ONE_SEC
> > +
> > +/**
> > + * fec_ptp_enable_pps
> > + * @fep: the fec_enet_private structure handle
> > + * @enable: enable the channel pps output
> > + *
> > + * This function enble the PPS ouput on the timer channel.
> > + */
> > +static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint
> > +enable) {
> > +	unsigned long flags;
> > +	u32 val, tempval;
> > +	int inc;
> > +	struct timespec ts;
> > +	u64 ns;
> > +	u32 remainder;
> > +	val = 0;
> > +
> > +	if (fep->pps_channel == -1 || fep->pps_channel > MAX_TIMER_CHANNEL)
> {
> > +		dev_err(&fep->pdev->dev, "Invalid pps channel\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!(fep->hwts_tx_en || fep->hwts_rx_en)) {
> > +		dev_err(&fep->pdev->dev, "No ptp stack is running\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (fep->pps_enable == enable)
> > +		return 0;
> > +
> > +	fep->reload_period = PPS_OUPUT_RELOAD_PERIOD;
> > +	inc = FEC_T_PERIOD_ONE_SEC / clk_get_rate(fep->clk_ptp);
> > +
> > +	spin_lock_irqsave(&fep->tmreg_lock, flags);
> > +
> > +	if (enable) {
> > +		/*
> > +		 * clear capture or output compare interrupt status if have.
> > +		 */
> > +		writel(FEC_T_TF_MASK, fep->hwp + FEC_TCSR(fep->pps_channel));
> > +
> > +		/*
> > +		 * It is recommended to doulbe check the TMODE field in the
> > +		 * TCSR register to be cleared before the first compare
> counter
> > +		 * is written into TCCR register. Just add a double check.
> > +		 */
> > +		val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> > +		do {
> > +			val &= ~(FEC_T_TMODE_MASK);
> > +			writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
> > +			val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> > +		} while (val & FEC_T_TMODE_MASK);
> > +
> > +		/*
> > +		 * Dummy read counter to update the counter
> > +		 */
> > +		timecounter_read(&fep->tc);
> > +		/*
> > +		 * linux ptp is running. We want to find the first compare
> event
>                    ^^^^^^^^^^^^^^^^^^^^
> What does this mean?
Wrong comments.
> 
> > +		 * in the next second point. So we need to know what the ptp
> time
> > +		 * is now and how many nanoseconds is ahead to get next
> second.
> > +		 * The remaining nanosecond ahead before the next second
> would be
> > +		 * FEC_T_PERIOD_ONE_SEC - ts.tv_nsec. Add the remaining
> nanoseconds
> > +		 * to current timer would be next second.
> > +		 */
> > +		tempval = readl(fep->hwp + FEC_ATIME_CTRL);
> > +		tempval |= FEC_T_CTRL_CAPTURE;
> > +		writel(tempval, fep->hwp + FEC_ATIME_CTRL);
> > +
> > +		tempval = readl(fep->hwp + FEC_ATIME);
> > +		/*
> > +		 * Converse the ptp local counter to 1588 timestamp
>                    ^^^^^^^^
> Convert?

Yes
> 
> > +		 */
> > +		ns = timecounter_cyc2time(&fep->tc, tempval);
> > +		ts.tv_sec = div_u64_rem(ns, 1000000000ULL, &remainder);
> > +		ts.tv_nsec = remainder;
> > +
> > +		/*
> > +		 * The tempval is  less than 3 seconds, and  so val is less
> than
> > +		 * 4 seconds. No overflow for 32bit calculation.
> > +		 */
> > +		val = FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec + tempval;
> > +
> > +		/* Need to consider the situation that the current time is
> > +		 * very close to the second point, which means
> FEC_T_PERIOD_ONE_SEC
> > +		 * - ts.tv_nsec is close to be zero(For example 20ns); Since
> the timer
> > +		 * is still running when we calculate the first compare event,
> it is
> > +		 * possible that the remaining nanoseonds run out before the
> compare
> > +		 * counter is calculated and written into TCCR register. To
> avoid
> > +		 * this possibility, we will set the compare event to be the
> next
> > +		 * of next second. The current setting is 31-bit timer and
> wrap
> > +		 * around over 2 seconds. So it is okay to set the next of
> next
> > +		 * seond for the timer.
> > +		 */
> > +		val += FEC_T_PERIOD_ONE_SEC;
> > +
> > +		/*
> > +		 * We add (2 *FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec) to
> current
> > +		 * ptp counter, which maybe cause 32-bit wrap. Since the
> > +		 * (FEC_T_PERIOD_ONE_SEC - (u32)ts.tv_nsec) is less than 2
> second.
> > +		 * We can ensure the wrap will not cause issue. If the offset
> > +		 * is bigger than fep->cc.mask would be a error.
> > +		 */
> > +		val &= fep->cc.mask;
> > +		writel(val, fep->hwp + FEC_TCCR(fep->pps_channel));
> > +
> > +		/*
> > +		 * Calculate the second the compare event timestamp.
> > +		 */
> > +		fep->next_counter = (val + fep->reload_period) & fep->cc.mask;
> > +
> > +		/*
> > +		 * Enable compare event when overflow
> > +		 */
> > +		val = readl(fep->hwp + FEC_ATIME_CTRL);
> > +		val |= FEC_T_CTRL_PINPER;
> > +		writel(val, fep->hwp + FEC_ATIME_CTRL);
> > +
> > +		/*
> > +		 * Compare channel setting.
> > +		 */
> > +		val = readl(fep->hwp + FEC_TCSR(fep->pps_channel));
> > +		val |= (1 << FEC_T_TF_OFFSET | 1 << FEC_T_TIE_OFFSET);
> > +		val &= ~(1 << FEC_T_TDRE_OFFSET);
> > +		val &= ~(FEC_T_TMODE_MASK);
> > +		val |= (FEC_HIGH_PULSE << FEC_T_TMODE_OFFSET);
> > +		writel(val, fep->hwp + FEC_TCSR(fep->pps_channel));
> > +
> > +		/*
> > +		 * Write the second compare event timestamp and calculate
> > +		 * the third timestamp. Refer the TCCR register detail in the
> spec.
> > +		 */
> > +		writel(fep->next_counter, fep->hwp + FEC_TCCR(fep-
> >pps_channel));
> > +		fep->next_counter = (fep->next_counter + fep->reload_period)
> & fep->cc.mask;
> > +	} else {
> > +		writel(0, fep->hwp + FEC_TCSR(fep->pps_channel));
> > +	}
> > +
> > +	fep->pps_enable = enable;
> > +	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * fec_ptp_read - read raw cycle counter (to be used by time counter)
> >   * @cc: the cyclecounter structure
> > @@ -322,6 +493,15 @@ static int fec_ptp_settime(struct ptp_clock_info
> > *ptp,  static int fec_ptp_enable(struct ptp_clock_info *ptp,
> >  			  struct ptp_clock_request *rq, int on)  {
> > +	struct fec_enet_private *fep =
> > +	    container_of(ptp, struct fec_enet_private, ptp_caps);
> > +	int ret = 0;
> > +
> > +	if (rq->type == PTP_CLK_REQ_PPS) {
> > +		ret = fec_ptp_enable_pps(fep, on);
> > +
> > +		return ret;
> > +	}
> >  	return -EOPNOTSUPP;
> >  }
> >
> > @@ -427,6 +607,8 @@ void fec_ptp_init(struct platform_device *pdev)  {
> >  	struct net_device *ndev = platform_get_drvdata(pdev);
> >  	struct fec_enet_private *fep = netdev_priv(ndev);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	int err;
> >
> >  	fep->ptp_caps.owner = THIS_MODULE;
> >  	snprintf(fep->ptp_caps.name, 16, "fec ptp"); @@ -436,7 +618,7 @@
> > void fec_ptp_init(struct platform_device *pdev)
> >  	fep->ptp_caps.n_ext_ts = 0;
> >  	fep->ptp_caps.n_per_out = 0;
> >  	fep->ptp_caps.n_pins = 0;
> > -	fep->ptp_caps.pps = 0;
> > +	fep->ptp_caps.pps = 1;
> >  	fep->ptp_caps.adjfreq = fec_ptp_adjfreq;
> >  	fep->ptp_caps.adjtime = fec_ptp_adjtime;
> >  	fep->ptp_caps.gettime = fec_ptp_gettime; @@ -444,6 +626,10 @@ void
> > fec_ptp_init(struct platform_device *pdev)
> >  	fep->ptp_caps.enable = fec_ptp_enable;
> >
> >  	fep->cycle_speed = clk_get_rate(fep->clk_ptp);
> > +	err = of_property_read_u32(np, "pps-channel",
> > +					&fep->pps_channel);
> 
> Please do not make another DT property for this.
> 
> Instead, use the PTP_PIN_SET/GETFUNC interface. You can add a new
> enumeration value to ptp_pin_function, like PTP_PF_PPS.
> 
> Thanks,
> Richard
You mean to user can use ioctrl interface to set the pin_config? Will update in the next version

Thanks
Luwei
> 
> 
> > +	if (err)
> > +		fep->pps_channel = -1;
> >
> >  	spin_lock_init(&fep->tmreg_lock);
> >
> > @@ -459,3 +645,38 @@ void fec_ptp_init(struct platform_device *pdev)
> >
> >  	schedule_delayed_work(&fep->time_keep, HZ);  }
> > +
> > +/**
> > + * fec_ptp_check_pps_event
> > + * @fep: the fec_enet_private structure handle
> > + *
> > + * This function check the pps event and reload the timer compare
> counter.
> > + */
> > +uint fec_ptp_check_pps_event(struct fec_enet_private *fep) {
> > +	u32 val;
> > +	u8 channel = fep->pps_channel;
> > +	struct ptp_clock_event event;
> > +
> > +	val = readl(fep->hwp + FEC_TCSR(channel));
> > +	if (val & FEC_T_TF_MASK) {
> > +		/*
> > +		 * Write the next next compare(not the next according the
> spec) value to the register
> > +		 */
> > +		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
> > +		do {
> > +			writel(val, fep->hwp + FEC_TCSR(channel));
> > +		} while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
> > +
> > +		/*
> > +		 * Update the counter;
> > +		 */
> > +		fep->next_counter = (fep->next_counter + fep->reload_period)
> &
> > +fep->cc.mask;
> > +
> > +		event.type = PTP_CLOCK_PPS;
> > +		ptp_clock_event(fep->ptp_clock, &event);
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > --
> > 1.9.1
> >

Re: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

[Richard Cochran]

On Fri, Sep 26, 2014 at 05:53:41AM +0000, luwei.zhou@freescale.com wrote:

> [Zhou Luwei-B45643] Thanks. It can pass compile without warning and
> test. In fact I used other tools to increase SYNC rate to 16 per
> second and it works well.

Some PTP profiles use 128 per second.

Thanks,
Richard

RE: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP counter.

[Frank.Li]

> 
> Hi Frank,
> 
> I ran checkpatch and it can pass. I will update it in the next version.
> 

Don't top post message in community mail list.

Frank Li

> Luwei
> Best Regards
> 
> -----Original Message-----
> From: Li Frank-B20596
> Sent: Thursday, September 25, 2014 9:30 PM
> To: Zhou Luwei-B45643; davem@davemloft.net; richardcochran@gmail.com
> Cc: netdev@vger.kernel.org; shawn.guo@linaro.org; bhutchings@solarflare.com;
> Estevam Fabio-R49496; Duan Fugang-B38611; stephen@networkplumber.org
> Subject: RE: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to adjust PTP
> counter.
> 
> 
> 
> > -----Original Message-----
> > From: Luwei Zhou [mailto:b45643@freescale.com]
> > Sent: Thursday, September 25, 2014 3:10 AM
> > To: davem@davemloft.net; richardcochran@gmail.com
> > Cc: netdev@vger.kernel.org; shawn.guo@linaro.org;
> > bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611;
> > Li Frank-B20596; stephen@networkplumber.org
> > Subject: [PATCH v1 2/4] net: fec: ptp: Use hardware algorithm to
> > adjust PTP counter.
> >
> > The FEC IP supports hardware adjustment for ptp timer. Refer to the
> > description of ENET_ATCOR and ENET_ATINC registers in the spec about the
> hardware adjustment.
> > This patch uses hardware support to adjust the ptp offset and
> > frequency on the slave side.
> >
> > Signed-off-by: Luwei Zhou <b45643@freescale.com>
> > Signed-off-by: Frank Li <Frank.Li@freescale.com>
> > Signed-off-by: Fugang Duan <b38611@freescale.com>
> > ---
> >  drivers/net/ethernet/freescale/fec_ptp.c | 68
> > +++++++++++++++++++++++++++-----
> >  1 file changed, 58 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> > b/drivers/net/ethernet/freescale/fec_ptp.c
> > index 8016bdd..e2bf786 100644
> > --- a/drivers/net/ethernet/freescale/fec_ptp.c
> > +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> > @@ -71,6 +71,7 @@
> >
> >  #define FEC_CC_MULT	(1 << 31)
> >  #define FEC_COUNTER_PERIOD	(1 << 31)
> > +#define FEC_T_PERIOD_ONE_SEC	(1000000000UL)
> >  /**
> >   * fec_ptp_read - read raw cycle counter (to be used by time counter)
> >   * @cc: the cyclecounter structure
> > @@ -145,33 +146,65 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
> >   */
> >  static int fec_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)  {
> > -	u64 diff;
> >  	unsigned long flags;
> >  	int neg_adj = 0;
> > -	u32 mult = FEC_CC_MULT;
> > +	u32 i, tmp;
> > +	u32 ptp_ts_clk, ptp_inc;
> > +	u32 corr_inc, corr_period;
> > +	u32 corr_ns;
> >
> >  	struct fec_enet_private *fep =
> >  	    container_of(ptp, struct fec_enet_private, ptp_caps);
> >
> > +	if (ppb == 0)
> > +		return 0;
> > +
> >  	if (ppb < 0) {
> >  		ppb = -ppb;
> >  		neg_adj = 1;
> >  	}
> >
> > -	diff = mult;
> > -	diff *= ppb;
> > -	diff = div_u64(diff, 1000000000ULL);
> > +	ptp_ts_clk = clk_get_rate(fep->clk_ptp);
> > +	ptp_inc = FEC_T_PERIOD_ONE_SEC / ptp_ts_clk;
> > +
> > +	/*
> > +	 * In theory, corr_inc/corr_period = ppb/FEC_T_PERIOD_ONE_SEC;
> > +	 * Try to find the corr_inc  between 1 to ptp_inc to meet adjustment
> > +	 * requirement.
> > +	 */
> > +	for (i = 1; i <= ptp_inc; i++) {
> > +		if (((i * FEC_T_PERIOD_ONE_SEC) / ppb) >= ptp_inc) {
> > +			corr_inc = i;
> > +			corr_period = ((i * FEC_T_PERIOD_ONE_SEC) /
> > +						(ptp_inc * ppb));
> > +			break;
> > +		}
> > +	}
> > +	/*
> > +	 * Not found? Set it to high value - double speed
> > +	 * correct in every clock step.
> > +	 */
> > +	if (i > ptp_inc) {
> > +		corr_inc = ptp_inc;
> > +		corr_period = 1;
> > +	}
> > +
> > +	if (neg_adj)
> > +		corr_ns = ptp_inc - corr_inc;
> > +	else
> > +		corr_ns = ptp_inc + corr_inc;
> >
> >  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> > +
> > +	tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
> > +	tmp |= corr_ns << FEC_T_INC_CORR_OFFSET;
> > +	writel(tmp, fep->hwp + FEC_ATIME_INC);
> > +	writel(corr_period, fep->hwp + FEC_ATIME_CORR);
> >  	/*
> > -	 * dummy read to set cycle_last in tc to now.
> > -	 * So use adjusted mult to calculate when next call
> > -	 * timercounter_read.
> > +	 * dummy read to update the timer.
> >  	 */
> >  	timecounter_read(&fep->tc);
> >
> > -	fep->cc.mult = neg_adj ? mult - diff : mult + diff;
> > -
> >  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> >
> >  	return 0;
> > @@ -190,12 +223,20 @@ static int fec_ptp_adjtime(struct ptp_clock_info
> > *ptp, s64
> > delta)
> >  	    container_of(ptp, struct fec_enet_private, ptp_caps);
> >  	unsigned long flags;
> >  	u64 now;
> > +	u32 counter;
> >
> >  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> >
> >  	now = timecounter_read(&fep->tc);
> >  	now += delta;
> >
> > +	/*
> > +	 * Get the timer value based on adjusted timestamp.
> > +	 * Update the counter with the masked value.
> > +	 */
> 
> Comments should like
> 
> /* Get the xxx
>  * dds
>  */
> 
> First empty line is not necessary.
> Please check the other place.
> 
> Did you run checkpatch script?
> 
> Best regards
> Frank Li
> 
> > +	counter = now & fep->cc.mask;
> > +	writel(counter, fep->hwp + FEC_ATIME);
> > +
> >  	/* reset the timecounter */
> >  	timecounter_init(&fep->tc, &fep->cc, now);
> >
> > @@ -246,6 +287,7 @@ static int fec_ptp_settime(struct ptp_clock_info
> > *ptp,
> >
> >  	u64 ns;
> >  	unsigned long flags;
> > +	u32 counter;
> >
> >  	mutex_lock(&fep->ptp_clk_mutex);
> >  	/* Check the ptp clock */
> > @@ -256,8 +298,14 @@ static int fec_ptp_settime(struct ptp_clock_info
> > *ptp,
> >
> >  	ns = ts->tv_sec * 1000000000ULL;
> >  	ns += ts->tv_nsec;
> > +	/*
> > +	 * Get the timer value based on timestamp.
> > +	 * Update the counter with the masked value.
> > +	 */
> > +	counter = ns & fep->cc.mask;
> >
> >  	spin_lock_irqsave(&fep->tmreg_lock, flags);
> > +	writel(counter, fep->hwp + FEC_ATIME);
> >  	timecounter_init(&fep->tc, &fep->cc, ns);
> >  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> >  	mutex_unlock(&fep->ptp_clk_mutex);
> > --
> > 1.9.1

Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[Richard Cochran]

On Thu, Sep 25, 2014 at 04:43:50PM +0200, Richard Cochran wrote:
> On Thu, Sep 25, 2014 at 04:10:21PM +0800, Luwei Zhou wrote:
> > This patch update fec devicetree binding doc that add Optional properties
> > "pps-channel".
> 
> Again, use the PTP pin interface. We don't need a random new FEC DT
> property for this.

BTW, if the "channel" only means an internal timer resource, please
find a proper way to claim that resource. Do not add another random DT
property. We have enough of those already.
 
Thanks,
Richard

Re: [PATCH v1 0/4] Enable FEC pps ouput

[Richard Cochran]

On Thu, Sep 25, 2014 at 04:10:17PM +0800, Luwei Zhou wrote:
> This patch does:
> 	- Replace 32-bit free-running PTP timer with 31-bit.
> 	- Implement hardware PTP timestamp adjustment.
> 	- Enable PPS output based on hardware adjustment.
> 
> 
> Luwei Zhou (4):
>   net: fec: ptp: Use the 31-bit ptp timer.
>   net: fec: ptp: Use hardware algorithm to adjust PTP counter.
>   net: fec: ptp: Enalbe PPS ouput based on ptp clock
                   ^^^^^^     ^^^^^
                   Enable     output

Also, it looks like you have implemented the wrong feature.

The "pps" capability means that the clock provides a PPS event
(interrupt) to the kernel.

IOW, if .pps==1, then you must call ptp_clock_event() with event type
PTP_CLOCK_PPS.

Your patch set seems to be creating a periodic output and not a PPS
kernel callback.

You should clarify what you are trying to do, and then implement the
appropriate interface in your driver.

Thanks,
Richard

RE: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[luwei.zhou]

On Wed, Oct 1, 2014 at 11:59:00AM, Richard Cochran wrote:
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, October 01, 2014 11:59 AM
> To: Zhou Luwei-B45643
> Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org;
> bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li
> Frank-B20596; stephen@networkplumber.org
> Subject: Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding
> doc
> 
> On Thu, Sep 25, 2014 at 04:43:50PM +0200, Richard Cochran wrote:
> > On Thu, Sep 25, 2014 at 04:10:21PM +0800, Luwei Zhou wrote:
> > > This patch update fec devicetree binding doc that add Optional
> > > properties "pps-channel".
> >
> > Again, use the PTP pin interface. We don't need a random new FEC DT
> > property for this.
> 
> BTW, if the "channel" only means an internal timer resource, please find
> a proper way to claim that resource. Do not add another random DT
> property. We have enough of those already.
> 
> Thanks,
> Richard

Okay. I will use the PTP pin interface to let user set the channel index as V2 patch does. Sorry for the
late reply caused by vocation.

Thanks,
Luwei

RE: [PATCH v1 0/4] Enable FEC pps ouput

[luwei.zhou]

On Wed, Oct 3, 2014 at 04:24:00PM, Richard Cochran wrote:
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Friday, October 03, 2014 4:24 PM
> To: Zhou Luwei-B45643
> Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org;
> bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li
> Frank-B20596; stephen@networkplumber.org
> Subject: Re: [PATCH v1 0/4] Enable FEC pps ouput
> 
> On Thu, Sep 25, 2014 at 04:10:17PM +0800, Luwei Zhou wrote:
> > This patch does:
> > 	- Replace 32-bit free-running PTP timer with 31-bit.
> > 	- Implement hardware PTP timestamp adjustment.
> > 	- Enable PPS output based on hardware adjustment.
> >
> >
> > Luwei Zhou (4):
> >   net: fec: ptp: Use the 31-bit ptp timer.
> >   net: fec: ptp: Use hardware algorithm to adjust PTP counter.
> >   net: fec: ptp: Enalbe PPS ouput based on ptp clock
>                    ^^^^^^     ^^^^^
>                    Enable     output
> 
> Also, it looks like you have implemented the wrong feature.
> 
> The "pps" capability means that the clock provides a PPS event
> (interrupt) to the kernel.
> 
> IOW, if .pps==1, then you must call ptp_clock_event() with event type
> PTP_CLOCK_PPS.
> 
> Your patch set seems to be creating a periodic output and not a PPS
> kernel callback.
> 
> You should clarify what you are trying to do, and then implement the
> appropriate interface in your driver.
> 
> Thanks,
> Richard


Hum..The FEC IP does not support a specified channel for PPS function. So I have to use
a ptp timer output channel to implement the PPS function. That is why the software flow
looks like the periodic output but the function is PPS function. We only want the event to happen
on the  per second. So I think it should be clarified to be PPS event. 

Thanks
Luwei

Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[Richard Cochran]

On Wed, Oct 08, 2014 at 03:15:08AM +0000, luwei.zhou@freescale.com wrote:
> On Wed, Oct 1, 2014 at 11:59:00AM, Richard Cochran wrote:
> > -----Original Message-----
> > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > Sent: Wednesday, October 01, 2014 11:59 AM
> > To: Zhou Luwei-B45643
> > Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org;
> > bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li
> > Frank-B20596; stephen@networkplumber.org
> > Subject: Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding
> > doc
> > 
> > On Thu, Sep 25, 2014 at 04:43:50PM +0200, Richard Cochran wrote:
> > > On Thu, Sep 25, 2014 at 04:10:21PM +0800, Luwei Zhou wrote:
> > > > This patch update fec devicetree binding doc that add Optional
> > > > properties "pps-channel".
> > >
> > > Again, use the PTP pin interface. We don't need a random new FEC DT
> > > property for this.
> > 
> > BTW, if the "channel" only means an internal timer resource, please find
> > a proper way to claim that resource. Do not add another random DT
> > property. We have enough of those already.
> > 
> > Thanks,
> > Richard
> 
> Okay. I will use the PTP pin interface to let user set the channel index as V2 patch does. Sorry for the
> late reply caused by vocation.

Do not use the PTP pin control for an internal PPS event. Instead,
just claim the timer as some kind of internal resource. It looks like
timers are already part of DT, so why not use those?

Thanks,
Richard

RE: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[luwei.zhou]

On Wed, Oct 8, 2014 at 4:06:00AM, Richard Cochran wrote:
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, October 08, 2014 4:06 PM
> To: Zhou Luwei-B45643
> Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org;
> bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li
> Frank-B20596; stephen@networkplumber.org
> Subject: Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding
> doc
> 
> On Wed, Oct 08, 2014 at 03:15:08AM +0000, luwei.zhou@freescale.com wrote:
> > On Wed, Oct 1, 2014 at 11:59:00AM, Richard Cochran wrote:
> > > -----Original Message-----
> > > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > > Sent: Wednesday, October 01, 2014 11:59 AM
> > > To: Zhou Luwei-B45643
> > > Cc: davem@davemloft.net; netdev@vger.kernel.org;
> > > shawn.guo@linaro.org; bhutchings@solarflare.com; Estevam
> > > Fabio-R49496; Duan Fugang-B38611; Li Frank-B20596;
> > > stephen@networkplumber.org
> > > Subject: Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts
> > > binding doc
> > >
> > > On Thu, Sep 25, 2014 at 04:43:50PM +0200, Richard Cochran wrote:
> > > > On Thu, Sep 25, 2014 at 04:10:21PM +0800, Luwei Zhou wrote:
> > > > > This patch update fec devicetree binding doc that add Optional
> > > > > properties "pps-channel".
> > > >
> > > > Again, use the PTP pin interface. We don't need a random new FEC
> > > > DT property for this.
> > >
> > > BTW, if the "channel" only means an internal timer resource, please
> > > find a proper way to claim that resource. Do not add another random
> > > DT property. We have enough of those already.
> > >
> > > Thanks,
> > > Richard
> >
> > Okay. I will use the PTP pin interface to let user set the channel
> > index as V2 patch does. Sorry for the late reply caused by vocation.
> 
> Do not use the PTP pin control for an internal PPS event. Instead, just
> claim the timer as some kind of internal resource. It looks like timers
> are already part of DT, so why not use those?
> 
> Thanks,
> Richard

Hi Richard,

I am not expert in DT.I didn't get your point of using timer resource in the DT.  Yes, the timer resource is
Part of FEC IP hardware. But if you want to choose one channel for using PPS, you have to add another property
to specify. Do you mean the current code already have the DT property to specify the channel?

Thanks
Luwei

Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[Richard Cochran]

On Wed, Oct 08, 2014 at 08:36:11AM +0000, luwei.zhou@freescale.com wrote:
> 
> I am not expert in DT.

(Me neither ;)

> I didn't get your point of using timer resource in the DT.  Yes, the timer resource is
> Part of FEC IP hardware. But if you want to choose one channel for using PPS, you have to add another property
> to specify. Do you mean the current code already have the DT property to specify the channel?

I see some timer bindings in Documentation/devicetree/bindings/timer.
So, you could and should add your SoC timer there and in the DTS
files.

Then, you have something like

	@fec {
		pps_timer = &timer;
	}

in your SoC's DTS file.

Thanks,
Richard

RE: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[luwei.zhou]

On Wed, Oct 8, 2014 at 06:13:00PM, Richard Cochran wrote:
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, October 08, 2014 6:13 PM
> To: Zhou Luwei-B45643
> Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org;
> bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li
> Frank-B20596; stephen@networkplumber.org
> Subject: Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding
> doc
> 
> On Wed, Oct 08, 2014 at 08:36:11AM +0000, luwei.zhou@freescale.com wrote:
> >
> > I am not expert in DT.
> 
> (Me neither ;)
> 
> > I didn't get your point of using timer resource in the DT.  Yes, the
> > timer resource is Part of FEC IP hardware. But if you want to choose
> > one channel for using PPS, you have to add another property to specify.
> Do you mean the current code already have the DT property to specify the
> channel?
> 
> I see some timer bindings in Documentation/devicetree/bindings/timer.
> So, you could and should add your SoC timer there and in the DTS files.
> 
> Then, you have something like
> 
> 	@fec {
> 		pps_timer = &timer;
> 	}
> 
> in your SoC's DTS file.
> 
> Thanks,
> Richard

I know what you meant now. If using similar way, it seems that we still need the new property in the timer node below.

@timer
{
	pps_channel = 0;
}
I didn't find out the exiting channel related property in the Documentation.

Since there are 4 channels in the PTP timer IP and none of 4 channels
Is now used by now. We can use hardcode to define the default PPS output channel not DT such as 
"#define PPS_COUTPUT_CHANNEL FEC_TIMER_CHANNEL0". 

Thanks
Luwei

Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[Richard Cochran]

On Thu, Oct 09, 2014 at 02:19:07AM +0000, luwei.zhou@freescale.com wrote:
> Since there are 4 channels in the PTP timer IP and none of 4 channels
> Is now used by now. We can use hardcode to define the default PPS output channel not DT such as 
> "#define PPS_COUTPUT_CHANNEL FEC_TIMER_CHANNEL0". 

I prefer this to a random new DT property, but how can you be sure
that nothing else in the kernel is using FEC_TIMER_CHANNELx?

Thanks,
Richard

RE: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding doc

[luwei.zhou]

On Thu, Oct 09, 2014 at 02:53:00PM +8000, Richard Cochran wrote:

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, October 09, 2014 2:53 PM
> To: Zhou Luwei-B45643
> Cc: davem@davemloft.net; netdev@vger.kernel.org; shawn.guo@linaro.org;
> bhutchings@solarflare.com; Estevam Fabio-R49496; Duan Fugang-B38611; Li
> Frank-B20596; stephen@networkplumber.org
> Subject: Re: [PATCH v1 4/4] ARM: Documentation: Update fec dts binding
> doc
> 
> On Thu, Oct 09, 2014 at 02:19:07AM +0000, luwei.zhou@freescale.com wrote:
> > Since there are 4 channels in the PTP timer IP and none of 4 channels
> > Is now used by now. We can use hardcode to define the default PPS
> > output channel not DT such as "#define PPS_COUTPUT_CHANNEL
> FEC_TIMER_CHANNEL0".
> 
> I prefer this to a random new DT property, but how can you be sure that
> nothing else in the kernel is using FEC_TIMER_CHANNELx?
> 
> Thanks,
> Richard

The channel related registers in PTP timers are not defined in the Ethernet driver. Current
Code just use ptp timer timestamp function to support linux ptp. So I conclude that we can
Choose one channel as default.

Thanks
Luwei