Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1656295 > unrolled thread

[PATCH v2 4/4] net: macb: Add hardware PTP support

Started byRafal Ozieblo <rafalo@cadence.com>
First post2017-06-02 16:30 +0200
Last post2017-06-08 14:50 +0200
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 4/4] net: macb: Add hardware PTP support Rafal Ozieblo <rafalo@cadence.com> - 2017-06-02 16:30 +0200
    Re: [PATCH v2 4/4] net: macb: Add hardware PTP support Richard Cochran <richardcochran@gmail.com> - 2017-06-04 23:00 +0200
      RE: [PATCH v2 4/4] net: macb: Add hardware PTP support Rafal Ozieblo <rafalo@cadence.com> - 2017-06-06 11:00 +0200
        Re: [PATCH v2 4/4] net: macb: Add hardware PTP support Richard Cochran <richardcochran@gmail.com> - 2017-06-06 20:40 +0200
    Re: [PATCH v2 4/4] net: macb: Add hardware PTP support Richard Cochran <richardcochran@gmail.com> - 2017-06-06 20:40 +0200
      RE: [PATCH v2 4/4] net: macb: Add hardware PTP support Rafal Ozieblo <rafalo@cadence.com> - 2017-06-07 13:20 +0200
        Re: [PATCH v2 4/4] net: macb: Add hardware PTP support Richard Cochran <richardcochran@gmail.com> - 2017-06-07 15:30 +0200
          RE: [PATCH v2 4/4] net: macb: Add hardware PTP support Rafal Ozieblo <rafalo@cadence.com> - 2017-06-08 14:50 +0200

#1656295 — [PATCH v2 4/4] net: macb: Add hardware PTP support

FromRafal Ozieblo <rafalo@cadence.com>
Date2017-06-02 16:30 +0200
Subject[PATCH v2 4/4] net: macb: Add hardware PTP support
Message-ID<tNVKr-2l4-53@gated-at.bofh.it>
This patch is based on original Harini's patch and Andrei's patch,
implemented in a separate file to ease the review/maintanance
and integration with other platforms.

This driver supports GEM-GXL:
- Register ptp clock framework
- Initialize PTP related registers
- HW time stamp on the PTP Ethernet packets are received using the
  SO_TIMESTAMPING API. Time stamps are obtained from the dma buffer
  descriptors
- add macb_ptp to compilation chain

Signed-off-by: Rafal Ozieblo <rafalo@cadence.com>
---
 drivers/net/ethernet/cadence/Makefile    |   4 +
 drivers/net/ethernet/cadence/macb.h      | 126 ++++++++
 drivers/net/ethernet/cadence/macb_main.c |  91 +++++-
 drivers/net/ethernet/cadence/macb_ptp.c  | 512 +++++++++++++++++++++++++++++++
 4 files changed, 727 insertions(+), 6 deletions(-)
 create mode 100755 drivers/net/ethernet/cadence/macb_ptp.c

diff --git a/drivers/net/ethernet/cadence/Makefile b/drivers/net/ethernet/cadence/Makefile
index 31ea6e3..1d66ddb 100644
--- a/drivers/net/ethernet/cadence/Makefile
+++ b/drivers/net/ethernet/cadence/Makefile
@@ -3,5 +3,9 @@
 #
 macb-y	:= macb_main.o
 
+ifeq ($(CONFIG_MACB_USE_HWSTAMP),y)
+macb-y	+= macb_ptp.o
+endif
+
 obj-$(CONFIG_MACB) += macb.o
 obj-$(CONFIG_MACB_PCI) += macb_pci.o
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 4359b08..4285daa6 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -11,6 +11,8 @@
 #define _MACB_H
 
 #include <linux/phy.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/net_tstamp.h>
 
 #if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
 #define MACB_EXT_DESC
@@ -90,6 +92,10 @@
 #define GEM_SA3T		0x009C /* Specific3 Top */
 #define GEM_SA4B		0x00A0 /* Specific4 Bottom */
 #define GEM_SA4T		0x00A4 /* Specific4 Top */
+#define GEM_EFTSH		0x00e8 /* PTP Event Frame Transmitted Seconds Register 47:32 */
+#define GEM_EFRSH		0x00ec /* PTP Event Frame Received Seconds Register 47:32 */
+#define GEM_PEFTSH		0x00f0 /* PTP Peer Event Frame Transmitted Seconds Register 47:32 */
+#define GEM_PEFRSH		0x00f4 /* PTP Peer Event Frame Received Seconds Register 47:32 */
 #define GEM_OTX			0x0100 /* Octets transmitted */
 #define GEM_OCTTXL		0x0100 /* Octets transmitted [31:0] */
 #define GEM_OCTTXH		0x0104 /* Octets transmitted [47:32] */
@@ -159,6 +165,9 @@
 #define GEM_DCFG6		0x0294 /* Design Config 6 */
 #define GEM_DCFG7		0x0298 /* Design Config 7 */
 
+#define GEM_TXBDCTRL	0x04cc /* TX Buffer Descriptor control register */
+#define GEM_RXBDCTRL	0x04d0 /* RX Buffer Descriptor control register */
+
 #define GEM_ISR(hw_q)		(0x0400 + ((hw_q) << 2))
 #define GEM_TBQP(hw_q)		(0x0440 + ((hw_q) << 2))
 #define GEM_TBQPH(hw_q)		(0x04C8)
@@ -195,6 +204,8 @@
 #define MACB_TZQ_OFFSET		12 /* Transmit zero quantum pause frame */
 #define MACB_TZQ_SIZE		1
 #define MACB_SRTSM_OFFSET	15
+#define MACB_OSSMODE_OFFSET 24 /* Enable One Step Synchro Mode */
+#define MACB_OSSMODE_SIZE	1
 
 /* Bitfields in NCFGR */
 #define MACB_SPD_OFFSET		0 /* Speed */
@@ -452,6 +463,52 @@
 #define GEM_NSINCR_OFFSET			0
 #define GEM_NSINCR_SIZE				8
 
+/* Bitfields in TSH */
+#define GEM_TSH_OFFSET				0 /* TSU timer value (s). MSB [47:32] of seconds timer count */
+#define GEM_TSH_SIZE				16
+
+/* Bitfields in TSL */
+#define GEM_TSL_OFFSET				0 /* TSU timer value (s). LSB [31:0] of seconds timer count */
+#define GEM_TSL_SIZE				32
+
+/* Bitfields in TN */
+#define GEM_TN_OFFSET				0 /* TSU timer value (ns) */
+#define GEM_TN_SIZE					30
+
+/* Bitfields in TXBDCTRL */
+#define GEM_TXTSMODE_OFFSET			4 /* TX Descriptor Timestamp Insertion mode */
+#define GEM_TXTSMODE_SIZE			2
+
+/* Bitfields in RXBDCTRL */
+#define GEM_RXTSMODE_OFFSET			4 /* RX Descriptor Timestamp Insertion mode */
+#define GEM_RXTSMODE_SIZE			2
+
+/* Transmit DMA buffer descriptor Word 1 */
+#define GEM_DMA_TXVALID_OFFSET		23 /* timestamp has been captured in the Buffer Descriptor */
+#define GEM_DMA_TXVALID_SIZE		1
+
+/* Receive DMA buffer descriptor Word 0 */
+#define GEM_DMA_RXVALID_OFFSET		2 /* indicates a valid timestamp in the Buffer Descriptor */
+#define GEM_DMA_RXVALID_SIZE		1
+
+/* DMA buffer descriptor Word 2 (32 bit addressing) or Word 4 (64 bit addressing) */
+#define GEM_DMA_SECL_OFFSET			30 /* Timestamp seconds[1:0]  */
+#define GEM_DMA_SECL_SIZE			2
+#define GEM_DMA_NSEC_OFFSET			0 /* Timestamp nanosecs [29:0] */
+#define GEM_DMA_NSEC_SIZE			30
+
+/* DMA buffer descriptor Word 3 (32 bit addressing) or Word 5 (64 bit addressing) */
+
+/* New hardware supports 12 bit precision of timestamp in DMA buffer descriptor.
+ * Old hardware supports only 6 bit precision but it is enough for PTP.
+ * Less accuracy is used always instead of checking hardware version.
+ */
+#define GEM_DMA_SECH_OFFSET			0 /* Timestamp seconds[5:2] */
+#define GEM_DMA_SECH_SIZE			4
+#define GEM_DMA_SEC_WIDTH			(GEM_DMA_SECH_SIZE + GEM_DMA_SECL_SIZE)
+#define GEM_DMA_SEC_TOP				(1 << GEM_DMA_SEC_WIDTH)
+#define GEM_DMA_SEC_MASK			(GEM_DMA_SEC_TOP - 1)
+
 /* Bitfields in ADJ */
 #define GEM_ADDSUB_OFFSET			31
 #define GEM_ADDSUB_SIZE				1
@@ -527,6 +584,8 @@
 #define queue_readl(queue, reg)		(queue)->bp->macb_reg_readl((queue)->bp, (queue)->reg)
 #define queue_writel(queue, reg, value)	(queue)->bp->macb_reg_writel((queue)->bp, (queue)->reg, (value))
 
+#define PTP_TS_BUFFER_SIZE		128 /* must be power of 2 */
+
 /* Conditional GEM/MACB macros.  These perform the operation to the correct
  * register dependent on whether the device is a GEM or a MACB.  For registers
  * and bitfields that are common across both devices, use macb_{read,write}l
@@ -574,6 +633,11 @@ struct macb_dma_desc_ptp {
 	u32	ts_1;
 	u32	ts_2;
 };
+
+struct gem_tx_ts {
+	struct sk_buff *skb;
+	struct macb_dma_desc_ptp desc_ptp;
+};
 #endif
 
 /* DMA descriptor bitfields */
@@ -889,6 +953,11 @@ struct macb_config {
 	int	jumbo_max_len;
 };
 
+struct tsu_incr {
+	u32 sub_ns;
+	u32 ns;
+};
+
 struct macb_queue {
 	struct macb		*bp;
 	int			irq;
@@ -905,6 +974,12 @@ struct macb_queue {
 	struct macb_tx_skb	*tx_skb;
 	dma_addr_t		tx_ring_dma;
 	struct work_struct	tx_error_task;
+
+#ifdef CONFIG_MACB_USE_HWSTAMP
+	struct work_struct	tx_ts_task;
+	unsigned int		tx_ts_head, tx_ts_tail;
+	struct gem_tx_ts	tx_timestamps[PTP_TS_BUFFER_SIZE];
+#endif
 };
 
 struct macb {
@@ -975,8 +1050,59 @@ struct macb {
 #ifdef MACB_EXT_DESC
 	uint8_t hw_dma_cap;
 #endif
+	spinlock_t tsu_clk_lock; /* gem tsu clock locking */
+	unsigned int tsu_rate;
+	struct ptp_clock *ptp_clock;
+	struct ptp_clock_info ptp_clock_info;
+	struct tsu_incr tsu_incr;
+	struct hwtstamp_config tstamp_config;
 };
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+#define GEM_TSEC_SIZE  (GEM_TSH_SIZE + GEM_TSL_SIZE)
+#define TSU_SEC_MAX_VAL (((u64)1 << GEM_TSEC_SIZE) - 1)
+#define TSU_NSEC_MAX_VAL ((1 << GEM_TN_SIZE) - 1)
+
+enum macb_bd_control {
+	TSTAMP_DISABLED,
+	TSTAMP_FRAME_PTP_EVENT_ONLY,
+	TSTAMP_ALL_PTP_FRAMES,
+	TSTAMP_ALL_FRAMES,
+};
+
+void gem_ptp_init(struct net_device *ndev);
+void gem_ptp_remove(struct net_device *ndev);
+int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *des);
+void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
+static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
+{
+	if (queue->bp->tstamp_config.tx_type == TSTAMP_DISABLED)
+		return -ENOTSUPP;
+
+	return gem_ptp_txstamp(queue, skb, desc);
+}
+
+static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
+{
+	if (bp->tstamp_config.rx_filter == TSTAMP_DISABLED)
+		return;
+
+	gem_ptp_rxstamp(bp, skb, desc);
+}
+int gem_get_hwtst(struct net_device *dev, struct ifreq *rq);
+int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd);
+#else
+static inline void gem_ptp_init(struct net_device *ndev) { }
+static inline void gem_ptp_remove(struct net_device *ndev) { }
+
+static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
+{
+	return -1;
+}
+
+static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
+#endif
+
 static inline bool macb_is_gem(struct macb *bp)
 {
 	return !!(bp->caps & MACB_CAPS_MACB_IS_GEM);
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 3151429..87a6580 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -828,6 +828,12 @@ static void macb_tx_interrupt(struct macb_queue *queue)
 
 			/* First, update TX stats if needed */
 			if (skb) {
+				if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
+					/* skb now belongs to timestamp buffer
+					 * and will be removed later
+					 */
+					tx_skb->skb = NULL;
+				}
 				netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
 					    macb_tx_ring_wrap(bp, tail),
 					    skb->data);
@@ -994,6 +1000,8 @@ static int gem_rx(struct macb *bp, int budget)
 		bp->dev->stats.rx_packets++;
 		bp->dev->stats.rx_bytes += skb->len;
 
+		gem_ptp_do_rxstamp(bp, skb, desc);
+
 #if defined(DEBUG) && defined(VERBOSE_DEBUG)
 		netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
 			    skb->len, skb->csum);
@@ -1315,7 +1323,6 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
 				queue_writel(queue, ISR, MACB_BIT(HRESP));
 		}
-
 		status = queue_readl(queue, ISR);
 	}
 
@@ -1645,7 +1652,6 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Make newly initialized descriptor visible to hardware */
 	wmb();
-
 	skb_tx_timestamp(skb);
 
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
@@ -2503,6 +2509,71 @@ static int macb_set_ringparam(struct net_device *netdev,
 	return 0;
 }
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+static unsigned int gem_get_tsu_rate(struct macb *bp)
+{
+	struct clk *tsu_clk;
+	unsigned int tsu_rate;
+
+	tsu_clk = devm_clk_get(&bp->pdev->dev, "tsu_clk");
+	if (!IS_ERR(tsu_clk))
+		tsu_rate = clk_get_rate(tsu_clk);
+	/* try pclk instead */
+	else if (!IS_ERR(bp->pclk)) {
+		tsu_clk = bp->pclk;
+		tsu_rate = clk_get_rate(tsu_clk);
+	} else
+		return -ENOTSUPP;
+	return tsu_rate;
+}
+
+static s32 gem_get_ptp_max_adj(void)
+{
+	return 64E6;
+}
+
+static int gem_get_ts_info(struct net_device *dev,
+			   struct ethtool_ts_info *info)
+{
+	struct macb *bp = netdev_priv(dev);
+
+	ethtool_op_get_ts_info(dev, info);
+	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
+		return 0;
+
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_SOFTWARE |
+		SOF_TIMESTAMPING_RX_SOFTWARE |
+		SOF_TIMESTAMPING_SOFTWARE |
+		SOF_TIMESTAMPING_TX_HARDWARE |
+		SOF_TIMESTAMPING_RX_HARDWARE |
+		SOF_TIMESTAMPING_RAW_HARDWARE;
+	info->tx_types =
+		(1 << HWTSTAMP_TX_ONESTEP_SYNC) |
+		(1 << HWTSTAMP_TX_OFF) |
+		(1 << HWTSTAMP_TX_ON);
+	info->rx_filters =
+		(1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_ALL);
+	info->phc_index = -1;
+
+	if (bp->ptp_clock)
+		info->phc_index = ptp_clock_index(bp->ptp_clock);
+
+	return 0;
+}
+
+static struct macb_ptp_info gem_ptp_info = {
+	.ptp_init	 = gem_ptp_init,
+	.ptp_remove	 = gem_ptp_remove,
+	.get_ptp_max_adj = gem_get_ptp_max_adj,
+	.get_tsu_rate	 = gem_get_tsu_rate,
+	.get_ts_info	 = gem_get_ts_info,
+	.get_hwtst	 = gem_get_hwtst,
+	.set_hwtst	 = gem_set_hwtst,
+};
+#endif
+
 static int macb_get_ts_info(struct net_device *netdev,
 			    struct ethtool_ts_info *info)
 {
@@ -2636,12 +2707,16 @@ static void macb_configure_caps(struct macb *bp,
 		dcfg = gem_readl(bp, DCFG2);
 		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
 			bp->caps |= MACB_CAPS_FIFO_MODE;
-		if (IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) && gem_has_ptp(bp)) {
+#ifdef CONFIG_MACB_USE_HWSTAMP
+		if (gem_has_ptp(bp)) {
 			if (!GEM_BFEXT(TSU, gem_readl(bp, DCFG5)))
 				pr_err("GEM doesn't support hardware ptp.\n");
-			else
+			else {
 				bp->hw_dma_cap |= HW_DMA_CAP_PTP;
+				bp->ptp_info = &gem_ptp_info;
+			}
 		}
+#endif
 	}
 
 	dev_dbg(&bp->pdev->dev, "Cadence caps 0x%08x\n", bp->caps);
@@ -3247,7 +3322,9 @@ static const struct macb_config np4_config = {
 };
 
 static const struct macb_config zynqmp_config = {
-	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
+			MACB_CAPS_JUMBO |
+			MACB_CAPS_GEM_HAS_PTP,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -3281,7 +3358,9 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
 #endif /* CONFIG_OF */
 
 static const struct macb_config default_gem_config = {
-	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
+			MACB_CAPS_JUMBO |
+			MACB_CAPS_GEM_HAS_PTP,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
new file mode 100755
index 0000000..d536970
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -0,0 +1,512 @@
+/**
+ * 1588 PTP support for Cadence GEM device.
+ *
+ * Copyright (C) 2017 Cadence Design Systems - http://www.cadence.com
+ *
+ * Authors: Rafal Ozieblo <rafalo@cadence.com>
+ *          Bartosz Folta <bfolta@cadence.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/etherdevice.h>
+#include <linux/platform_device.h>
+#include <linux/time64.h>
+#include <linux/ptp_classify.h>
+#include <linux/if_ether.h>
+#include <linux/if_vlan.h>
+#include <linux/net_tstamp.h>
+#include <linux/circ_buf.h>
+#include <linux/spinlock.h>
+
+#include "macb.h"
+
+#define  GEM_PTP_TIMER_NAME "gem-ptp-timer"
+
+static struct macb_dma_desc_ptp *macb_ptp_desc(struct macb *bp,
+					       struct macb_dma_desc *desc)
+{
+	if (bp->hw_dma_cap == HW_DMA_CAP_PTP)
+		return (struct macb_dma_desc_ptp *)
+				((u8 *)desc + sizeof(struct macb_dma_desc));
+	if (bp->hw_dma_cap == HW_DMA_CAP_64B_PTP)
+		return (struct macb_dma_desc_ptp *)
+				((u8 *)desc + sizeof(struct macb_dma_desc)
+				+ sizeof(struct macb_dma_desc_64));
+	return NULL;
+}
+
+static int gem_tsu_get_time(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+	long first, second;
+	u32 secl, sech;
+	unsigned long flags;
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+
+	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
+	first = gem_readl(bp, TN);
+	secl = gem_readl(bp, TSL);
+	sech = gem_readl(bp, TSH);
+	second = gem_readl(bp, TN);
+
+	/* test for nsec rollover */
+	if (first > second) {
+		/* if so, use later read & re-read seconds
+		 * (assume all done within 1s)
+		 */
+		ts->tv_nsec = gem_readl(bp, TN);
+		secl = gem_readl(bp, TSL);
+		sech = gem_readl(bp, TSH);
+	} else {
+		ts->tv_nsec = first;
+	}
+
+	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
+	ts->tv_sec = (((u64)sech << GEM_TSL_SIZE) | secl)
+			& TSU_SEC_MAX_VAL;
+	return 0;
+}
+
+static int gem_tsu_set_time(struct ptp_clock_info *ptp,
+			    const struct timespec64 *ts)
+{
+	u32 ns, sech, secl;
+	unsigned long flags;
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+
+	secl = (u32)ts->tv_sec;
+	sech = (ts->tv_sec >> GEM_TSL_SIZE) & ((1 << GEM_TSH_SIZE) - 1);
+	ns = ts->tv_nsec;
+
+	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
+
+	/* TSH doesn't latch the time and no atomicity! */
+	gem_writel(bp, TN, 0); /* clear to avoid overflow */
+	gem_writel(bp, TSH, sech);
+	/* write lower bits 2nd, for synchronized secs update */
+	gem_writel(bp, TSL, secl);
+	gem_writel(bp, TN, ns);
+
+	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
+
+	return 0;
+}
+
+static int gem_tsu_incr_set(struct macb *bp, struct tsu_incr *incr_spec)
+{
+	unsigned long flags;
+
+	/* tsu_timer_incr register must be written after
+	 * the tsu_timer_incr_sub_ns register and the write operation
+	 * will cause the value written to the tsu_timer_incr_sub_ns register
+	 * to take effect.
+	 */
+	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
+	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, incr_spec->sub_ns));
+	gem_writel(bp, TI, GEM_BF(NSINCR, incr_spec->ns));
+	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
+
+	return 0;
+}
+
+static int gem_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	struct tsu_incr incr_spec;
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+	u64 adj;
+	u32 word;
+	bool neg_adj = false;
+
+	if (scaled_ppm < 0) {
+		neg_adj = true;
+		scaled_ppm = -scaled_ppm;
+	}
+
+	/* Adjustment is relative to base frequency */
+	incr_spec.sub_ns = bp->tsu_incr.sub_ns;
+	incr_spec.ns = bp->tsu_incr.ns;
+
+	/* scaling: unused(8bit) | ns(8bit) | fractions(16bit) */
+	word = ((u64)incr_spec.ns << GEM_SUBNSINCR_SIZE) + incr_spec.sub_ns;
+	adj = (u64)scaled_ppm * word;
+	/* Divide with rounding, equivalent to floating dividing:
+	 * (temp / USEC_PER_SEC) + 0.5
+	 */
+	adj += (USEC_PER_SEC >> 1);
+	adj >>= GEM_SUBNSINCR_SIZE; /* remove fractions */
+	adj = div_u64(adj, USEC_PER_SEC);
+	adj = neg_adj ? (word - adj) : (word + adj);
+
+	incr_spec.ns = (adj >> GEM_SUBNSINCR_SIZE)
+			& ((1 << GEM_NSINCR_SIZE) - 1);
+	incr_spec.sub_ns = adj & ((1 << GEM_SUBNSINCR_SIZE) - 1);
+	gem_tsu_incr_set(bp, &incr_spec);
+	return 0;
+}
+
+static int gem_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct timespec64 now, then = ns_to_timespec64(delta);
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+	u32 adj, sign = 0;
+
+	if (delta < 0) {
+		sign = 1;
+		delta = -delta;
+	}
+
+	if (delta > TSU_NSEC_MAX_VAL) {
+		gem_tsu_get_time(&bp->ptp_clock_info, &now);
+		if (sign)
+			now = timespec64_sub(now, then);
+		else
+			now = timespec64_add(now, then);
+
+		gem_tsu_set_time(&bp->ptp_clock_info,
+				 (const struct timespec64 *)&now);
+	} else {
+		adj = (sign << GEM_ADDSUB_OFFSET) | delta;
+
+		gem_writel(bp, TA, adj);
+	}
+
+	return 0;
+}
+
+static int gem_ptp_enable(struct ptp_clock_info *ptp,
+			  struct ptp_clock_request *rq, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static struct ptp_clock_info gem_ptp_caps_template = {
+	.owner		= THIS_MODULE,
+	.name		= GEM_PTP_TIMER_NAME,
+	.max_adj	= 0,
+	.n_alarm	= 0,
+	.n_ext_ts	= 0,
+	.n_per_out	= 0,
+	.n_pins		= 0,
+	.pps		= 1,
+	.adjfine	= gem_ptp_adjfine,
+	.adjtime	= gem_ptp_adjtime,
+	.gettime64	= gem_tsu_get_time,
+	.settime64	= gem_tsu_set_time,
+	.enable		= gem_ptp_enable,
+};
+
+static void gem_ptp_init_timer(struct macb *bp)
+{
+	u32 rem = 0;
+
+	bp->tsu_incr.ns = div_u64_rem(NSEC_PER_SEC, bp->tsu_rate, &rem);
+	if (rem) {
+		u64 adj = rem;
+
+		adj <<= GEM_SUBNSINCR_SIZE;
+		bp->tsu_incr.sub_ns = div_u64(adj, bp->tsu_rate);
+	} else {
+		bp->tsu_incr.sub_ns = 0;
+	}
+}
+
+static void gem_ptp_init_tsu(struct macb *bp)
+{
+	struct timespec64 ts;
+
+	/* 1. get current system time */
+	ts = ns_to_timespec64(ktime_to_ns(ktime_get_real()));
+
+	/* 2. set ptp timer */
+	gem_tsu_set_time(&bp->ptp_clock_info, &ts);
+
+	/* 3. set PTP timer increment value to BASE_INCREMENT */
+	gem_tsu_incr_set(bp, &bp->tsu_incr);
+
+	gem_writel(bp, TA, 0);
+}
+
+static void gem_ptp_clear_timer(struct macb *bp)
+{
+	bp->tsu_incr.ns = 0;
+	bp->tsu_incr.sub_ns = 0;
+
+	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, 0));
+	gem_writel(bp, TI, GEM_BF(NSINCR, 0));
+	gem_writel(bp, TA, 0);
+}
+
+static int gem_hw_timestamp(struct macb *bp, u32 dma_desc_ts_1,
+			    u32 dma_desc_ts_2, struct timespec64 *ts)
+{
+	struct timespec64 tsu;
+
+	ts->tv_sec = (GEM_BFEXT(DMA_SECH, dma_desc_ts_2) << GEM_DMA_SECL_SIZE) |
+			GEM_BFEXT(DMA_SECL, dma_desc_ts_1);
+	ts->tv_nsec = GEM_BFEXT(DMA_NSEC, dma_desc_ts_1);
+
+	/* TSU overlapping workaround
+	 * The timestamp only contains lower few bits of seconds,
+	 * so add value from 1588 timer
+	 */
+	gem_tsu_get_time(&bp->ptp_clock_info, &tsu);
+
+	/* If the top bit is set in the timestamp,
+	 * but not in 1588 timer, it has rolled over,
+	 * so subtract max size
+	 */
+	if ((ts->tv_sec & (GEM_DMA_SEC_TOP >> 1)) &&
+	    !(tsu.tv_sec & (GEM_DMA_SEC_TOP >> 1)))
+		ts->tv_sec -= GEM_DMA_SEC_TOP;
+
+	ts->tv_sec += ((~GEM_DMA_SEC_MASK) & tsu.tv_sec);
+
+	return 0;
+}
+
+void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb,
+		     struct macb_dma_desc *desc)
+{
+	struct timespec64 ts;
+	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
+	struct macb_dma_desc_ptp *desc_ptp;
+
+	if (GEM_BFEXT(DMA_RXVALID, desc->addr)) {
+		desc_ptp = macb_ptp_desc(bp, desc);
+		gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
+		memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
+		shhwtstamps->hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
+	}
+}
+
+static void gem_tstamp_tx(struct macb *bp, struct sk_buff *skb,
+			  struct macb_dma_desc_ptp *desc_ptp)
+{
+	struct skb_shared_hwtstamps shhwtstamps;
+	struct timespec64 ts;
+
+	gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
+	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+	shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
+	skb_tstamp_tx(skb, &shhwtstamps);
+}
+
+int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb,
+		    struct macb_dma_desc *desc)
+{
+	struct gem_tx_ts *tx_timestamp;
+	struct macb_dma_desc_ptp *desc_ptp;
+	unsigned long head = queue->tx_ts_head;
+	unsigned long tail = READ_ONCE(queue->tx_ts_tail);
+
+	if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl))
+		return -EINVAL;
+
+	if (CIRC_SPACE(head, tail, PTP_TS_BUFFER_SIZE) == 0)
+		return -ENOMEM;
+
+	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+	desc_ptp = macb_ptp_desc(queue->bp, desc);
+	tx_timestamp = &queue->tx_timestamps[head];
+	tx_timestamp->skb = skb;
+	tx_timestamp->desc_ptp.ts_1 = desc_ptp->ts_1;
+	tx_timestamp->desc_ptp.ts_2 = desc_ptp->ts_2;
+	/* move head */
+	smp_store_release(&queue->tx_ts_head,
+			  (head + 1) & (PTP_TS_BUFFER_SIZE - 1));
+
+	schedule_work(&queue->tx_ts_task);
+	return 0;
+}
+
+static void gem_tx_timestamp_flush(struct work_struct *work)
+{
+	struct macb_queue *queue =
+			container_of(work, struct macb_queue, tx_ts_task);
+	struct gem_tx_ts *tx_ts;
+	unsigned long head, tail;
+
+	/* take current head */
+	head = smp_load_acquire(&queue->tx_ts_head);
+	tail = queue->tx_ts_tail;
+
+	while (CIRC_CNT(head, tail, PTP_TS_BUFFER_SIZE)) {
+		tx_ts = &queue->tx_timestamps[tail];
+		gem_tstamp_tx(queue->bp, tx_ts->skb, &tx_ts->desc_ptp);
+		/* cleanup */
+		dev_kfree_skb_any(tx_ts->skb);
+		/* remove old tail */
+		smp_store_release(&queue->tx_ts_tail,
+				  (tail + 1) & (PTP_TS_BUFFER_SIZE - 1));
+		tail = queue->tx_ts_tail;
+	}
+}
+
+void gem_ptp_init(struct net_device *dev)
+{
+	struct macb *bp = netdev_priv(dev);
+	unsigned int q;
+	struct macb_queue *queue;
+
+	bp->ptp_clock_info = gem_ptp_caps_template;
+
+	/* nominal frequency and maximum adjustment in ppb */
+	bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
+	bp->ptp_clock_info.max_adj = bp->ptp_info->get_ptp_max_adj();
+	gem_ptp_init_timer(bp);
+	bp->ptp_clock = ptp_clock_register(&bp->ptp_clock_info, &dev->dev);
+	if (IS_ERR(&bp->ptp_clock)) {
+		bp->ptp_clock = NULL;
+		pr_err("ptp clock register failed\n");
+		return;
+	}
+
+	spin_lock_init(&bp->tsu_clk_lock);
+	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
+		queue->tx_ts_head = 0;
+		queue->tx_ts_tail = 0;
+		INIT_WORK(&queue->tx_ts_task, gem_tx_timestamp_flush);
+	}
+
+	gem_ptp_init_tsu(bp);
+
+	dev_info(&bp->pdev->dev, "%s ptp clock registered.\n",
+		 GEM_PTP_TIMER_NAME);
+}
+
+void gem_ptp_remove(struct net_device *ndev)
+{
+	struct macb *bp = netdev_priv(ndev);
+
+	if (bp->ptp_clock)
+		ptp_clock_unregister(bp->ptp_clock);
+
+	gem_ptp_clear_timer(bp);
+
+	dev_info(&bp->pdev->dev, "%s ptp clock unregistered.\n",
+		 GEM_PTP_TIMER_NAME);
+}
+
+static int gem_ptp_set_ts_mode(struct macb *bp,
+			       enum macb_bd_control tx_bd_control,
+			     enum macb_bd_control rx_bd_control)
+{
+	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
+	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));
+
+	return 0;
+}
+
+int gem_get_hwtst(struct net_device *dev, struct ifreq *rq)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
+
+	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
+		return -EFAULT;
+
+	if (copy_to_user(rq->ifr_data, tstamp_config, sizeof(*tstamp_config)))
+		return -EFAULT;
+	else
+		return 0;
+}
+
+static int gem_ptp_set_one_step_sync(struct macb *bp, u8 enable)
+{
+	u32 reg_val;
+
+	reg_val = macb_readl(bp, NCR);
+
+	if (enable)
+		macb_writel(bp, NCR, reg_val | MACB_BIT(OSSMODE));
+	else
+		macb_writel(bp, NCR, reg_val & ~MACB_BIT(OSSMODE));
+
+	return 0;
+}
+
+int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
+	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
+	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
+	u32 regval;
+
+	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
+		return -EFAULT;
+
+	if (copy_from_user(tstamp_config, ifr->ifr_data,
+			   sizeof(*tstamp_config)))
+		return -EFAULT;
+
+	/* reserved for future extensions */
+	if (tstamp_config->flags)
+		return -EINVAL;
+
+	switch (tstamp_config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+		break;
+	case HWTSTAMP_TX_ONESTEP_SYNC:
+		if (gem_ptp_set_one_step_sync(bp, 1) != 0)
+			return -ERANGE;
+	case HWTSTAMP_TX_ON:
+		tx_bd_control = TSTAMP_ALL_FRAMES;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (tstamp_config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		rx_bd_control =  TSTAMP_ALL_PTP_FRAMES;
+		tstamp_config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		regval = macb_readl(bp, NCR);
+		macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_ALL:
+		rx_bd_control = TSTAMP_ALL_FRAMES;
+		tstamp_config->rx_filter = HWTSTAMP_FILTER_ALL;
+		break;
+	default:
+		tstamp_config->rx_filter = HWTSTAMP_FILTER_NONE;
+		return -ERANGE;
+	}
+
+	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)
+		return -ERANGE;
+
+	if (copy_to_user(ifr->ifr_data, tstamp_config, sizeof(*tstamp_config)))
+		return -EFAULT;
+	else
+		return 0;
+}
+
-- 
2.4.5

[toc] | [next] | [standalone]

#1657170

FromRichard Cochran <richardcochran@gmail.com>
Date2017-06-04 23:00 +0200
Message-ID<tOKMV-1G2-1@gated-at.bofh.it>
In reply to#1656295
On Fri, Jun 02, 2017 at 03:28:10PM +0100, Rafal Ozieblo wrote:

> +int gem_get_hwtst(struct net_device *dev, struct ifreq *rq)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
> +
> +	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
> +		return -EFAULT;

Segmentation fault?  Really?

...

> +int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
> +	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
> +	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
> +	u32 regval;
> +
> +	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
> +		return -EFAULT;

here again

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1658491

FromRafal Ozieblo <rafalo@cadence.com>
Date2017-06-06 11:00 +0200
Message-ID<tPivg-6qB-29@gated-at.bofh.it>
In reply to#1657170
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: 4 czerwca 2017 22:55
> To: Rafal Ozieblo <rafalo@cadence.com>
> Cc: David Miller <davem@davemloft.net>; nicolas.ferre@atmel.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> harini.katakam@xilinx.com; andrei.pistirica@microchip.com
> Subject: Re: [PATCH v2 4/4] net: macb: Add hardware PTP support
> 
> On Fri, Jun 02, 2017 at 03:28:10PM +0100, Rafal Ozieblo wrote:
> 
> > +int gem_get_hwtst(struct net_device *dev, struct ifreq *rq)
> > +{
> > +	struct macb *bp = netdev_priv(dev);
> > +	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
> > +
> > +	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
> > +		return -EFAULT;
> 
> Segmentation fault?  Really?
> 
> ...
> 
> > +int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
> > +{
> > +	struct macb *bp = netdev_priv(dev);
> > +	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
> > +	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
> > +	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
> > +	u32 regval;
> > +
> > +	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
> > +		return -EFAULT;
> 
> here again
Would "ENOTSUP" be sufficient ?

> 
> Thanks,
> Richard

Regards,
Rafal

[toc] | [prev] | [next] | [standalone]

#1659048

FromRichard Cochran <richardcochran@gmail.com>
Date2017-06-06 20:40 +0200
Message-ID<tPryy-41R-21@gated-at.bofh.it>
In reply to#1658491
On Tue, Jun 06, 2017 at 08:54:50AM +0000, Rafal Ozieblo wrote:
> Would "ENOTSUP" be sufficient ?

You mean EOPNOTSUPP?  Yes, sounds ok to me.  EFAULT is surely wrong.

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1659045

FromRichard Cochran <richardcochran@gmail.com>
Date2017-06-06 20:40 +0200
Message-ID<tPryy-41R-19@gated-at.bofh.it>
In reply to#1656295
On Fri, Jun 02, 2017 at 03:28:10PM +0100, Rafal Ozieblo wrote:
> +static s32 gem_get_ptp_max_adj(void)
> +{
> +	return 64E6;
> +}

This is a floating point constant.  Please use integer instead.

> +
> +static int gem_get_ts_info(struct net_device *dev,
> +			   struct ethtool_ts_info *info)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +
> +	ethtool_op_get_ts_info(dev, info);

This default is misguided.

> +	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0)
> +		return 0;

Try this: 

	if ((bp->hw_dma_cap & HW_DMA_CAP_PTP) == 0) {
		ethtool_op_get_ts_info(dev, info);
		return 0;
	}

> +	info->so_timestamping =
> +		SOF_TIMESTAMPING_TX_SOFTWARE |
> +		SOF_TIMESTAMPING_RX_SOFTWARE |
> +		SOF_TIMESTAMPING_SOFTWARE |
> +		SOF_TIMESTAMPING_TX_HARDWARE |
> +		SOF_TIMESTAMPING_RX_HARDWARE |
> +		SOF_TIMESTAMPING_RAW_HARDWARE;
> +	info->tx_types =
> +		(1 << HWTSTAMP_TX_ONESTEP_SYNC) |
> +		(1 << HWTSTAMP_TX_OFF) |
> +		(1 << HWTSTAMP_TX_ON);
> +	info->rx_filters =
> +		(1 << HWTSTAMP_FILTER_NONE) |
> +		(1 << HWTSTAMP_FILTER_ALL);
> +	info->phc_index = -1;
> +
> +	if (bp->ptp_clock)
> +		info->phc_index = ptp_clock_index(bp->ptp_clock);

Like this please:

	info->phc_index = bp->ptp_clock ? ptp_clock_index(bp->ptp_clock) : -1;

> +
> +	return 0;
> +}
> +
> +static struct macb_ptp_info gem_ptp_info = {
> +	.ptp_init	 = gem_ptp_init,
> +	.ptp_remove	 = gem_ptp_remove,
> +	.get_ptp_max_adj = gem_get_ptp_max_adj,
> +	.get_tsu_rate	 = gem_get_tsu_rate,
> +	.get_ts_info	 = gem_get_ts_info,
> +	.get_hwtst	 = gem_get_hwtst,
> +	.set_hwtst	 = gem_set_hwtst,
> +};
> +#endif
> +
>  static int macb_get_ts_info(struct net_device *netdev,
>  			    struct ethtool_ts_info *info)
>  {
> @@ -2636,12 +2707,16 @@ static void macb_configure_caps(struct macb *bp,
>  		dcfg = gem_readl(bp, DCFG2);
>  		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
>  			bp->caps |= MACB_CAPS_FIFO_MODE;
> -		if (IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) && gem_has_ptp(bp)) {
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +		if (gem_has_ptp(bp)) {
>  			if (!GEM_BFEXT(TSU, gem_readl(bp, DCFG5)))
>  				pr_err("GEM doesn't support hardware ptp.\n");
> -			else
> +			else {
>  				bp->hw_dma_cap |= HW_DMA_CAP_PTP;
> +				bp->ptp_info = &gem_ptp_info;
> +			}
>  		}
> +#endif
>  	}
>  
>  	dev_dbg(&bp->pdev->dev, "Cadence caps 0x%08x\n", bp->caps);
> @@ -3247,7 +3322,9 @@ static const struct macb_config np4_config = {
>  };
>  
>  static const struct macb_config zynqmp_config = {
> -	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
> +	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
> +			MACB_CAPS_JUMBO |
> +			MACB_CAPS_GEM_HAS_PTP,
>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = macb_init,
> @@ -3281,7 +3358,9 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
>  #endif /* CONFIG_OF */
>  
>  static const struct macb_config default_gem_config = {
> -	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
> +	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
> +			MACB_CAPS_JUMBO |
> +			MACB_CAPS_GEM_HAS_PTP,
>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = macb_init,

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> new file mode 100755
> index 0000000..d536970
> --- /dev/null
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -0,0 +1,512 @@
> +/**
> + * 1588 PTP support for Cadence GEM device.
> + *
> + * Copyright (C) 2017 Cadence Design Systems - http://www.cadence.com
> + *
> + * Authors: Rafal Ozieblo <rafalo@cadence.com>
> + *          Bartosz Folta <bfolta@cadence.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/etherdevice.h>
> +#include <linux/platform_device.h>
> +#include <linux/time64.h>
> +#include <linux/ptp_classify.h>
> +#include <linux/if_ether.h>
> +#include <linux/if_vlan.h>
> +#include <linux/net_tstamp.h>
> +#include <linux/circ_buf.h>
> +#include <linux/spinlock.h>
> +
> +#include "macb.h"
> +
> +#define  GEM_PTP_TIMER_NAME "gem-ptp-timer"
> +
> +static struct macb_dma_desc_ptp *macb_ptp_desc(struct macb *bp,
> +					       struct macb_dma_desc *desc)
> +{
> +	if (bp->hw_dma_cap == HW_DMA_CAP_PTP)
> +		return (struct macb_dma_desc_ptp *)
> +				((u8 *)desc + sizeof(struct macb_dma_desc));
> +	if (bp->hw_dma_cap == HW_DMA_CAP_64B_PTP)
> +		return (struct macb_dma_desc_ptp *)
> +				((u8 *)desc + sizeof(struct macb_dma_desc)
> +				+ sizeof(struct macb_dma_desc_64));
> +	return NULL;
> +}
> +
> +static int gem_tsu_get_time(struct ptp_clock_info *ptp, struct timespec64 *ts)
> +{
> +	long first, second;
> +	u32 secl, sech;
> +	unsigned long flags;
> +	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);

Please list locals in "upside down Christmas tree" style:

	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
	unsigned long flags;
	long first, second;
	u32 secl, sech;

> +	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
> +	first = gem_readl(bp, TN);
> +	secl = gem_readl(bp, TSL);
> +	sech = gem_readl(bp, TSH);
> +	second = gem_readl(bp, TN);
> +
> +	/* test for nsec rollover */
> +	if (first > second) {
> +		/* if so, use later read & re-read seconds
> +		 * (assume all done within 1s)
> +		 */
> +		ts->tv_nsec = gem_readl(bp, TN);
> +		secl = gem_readl(bp, TSL);
> +		sech = gem_readl(bp, TSH);
> +	} else {
> +		ts->tv_nsec = first;
> +	}
> +
> +	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
> +	ts->tv_sec = (((u64)sech << GEM_TSL_SIZE) | secl)
> +			& TSU_SEC_MAX_VAL;
> +	return 0;
> +}
> +
> +static int gem_tsu_set_time(struct ptp_clock_info *ptp,
> +			    const struct timespec64 *ts)
> +{
> +	u32 ns, sech, secl;
> +	unsigned long flags;
> +	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);

here too ...

> +	secl = (u32)ts->tv_sec;
> +	sech = (ts->tv_sec >> GEM_TSL_SIZE) & ((1 << GEM_TSH_SIZE) - 1);
> +	ns = ts->tv_nsec;
> +
> +	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
> +
> +	/* TSH doesn't latch the time and no atomicity! */
> +	gem_writel(bp, TN, 0); /* clear to avoid overflow */
> +	gem_writel(bp, TSH, sech);
> +	/* write lower bits 2nd, for synchronized secs update */
> +	gem_writel(bp, TSL, secl);
> +	gem_writel(bp, TN, ns);
> +
> +	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int gem_tsu_incr_set(struct macb *bp, struct tsu_incr *incr_spec)
> +{
> +	unsigned long flags;
> +
> +	/* tsu_timer_incr register must be written after
> +	 * the tsu_timer_incr_sub_ns register and the write operation
> +	 * will cause the value written to the tsu_timer_incr_sub_ns register
> +	 * to take effect.
> +	 */
> +	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
> +	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, incr_spec->sub_ns));
> +	gem_writel(bp, TI, GEM_BF(NSINCR, incr_spec->ns));
> +	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int gem_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{
> +	struct tsu_incr incr_spec;
> +	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
> +	u64 adj;
> +	u32 word;
> +	bool neg_adj = false;

and here ...

> +
> +	if (scaled_ppm < 0) {
> +		neg_adj = true;
> +		scaled_ppm = -scaled_ppm;
> +	}
> +
> +	/* Adjustment is relative to base frequency */
> +	incr_spec.sub_ns = bp->tsu_incr.sub_ns;
> +	incr_spec.ns = bp->tsu_incr.ns;
> +
> +	/* scaling: unused(8bit) | ns(8bit) | fractions(16bit) */
> +	word = ((u64)incr_spec.ns << GEM_SUBNSINCR_SIZE) + incr_spec.sub_ns;
> +	adj = (u64)scaled_ppm * word;
> +	/* Divide with rounding, equivalent to floating dividing:
> +	 * (temp / USEC_PER_SEC) + 0.5
> +	 */
> +	adj += (USEC_PER_SEC >> 1);
> +	adj >>= GEM_SUBNSINCR_SIZE; /* remove fractions */
> +	adj = div_u64(adj, USEC_PER_SEC);
> +	adj = neg_adj ? (word - adj) : (word + adj);
> +
> +	incr_spec.ns = (adj >> GEM_SUBNSINCR_SIZE)
> +			& ((1 << GEM_NSINCR_SIZE) - 1);
> +	incr_spec.sub_ns = adj & ((1 << GEM_SUBNSINCR_SIZE) - 1);
> +	gem_tsu_incr_set(bp, &incr_spec);
> +	return 0;
> +}
> +
> +static int gem_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	struct timespec64 now, then = ns_to_timespec64(delta);
> +	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
> +	u32 adj, sign = 0;

and here too.  Please check the other functions...

> +	if (delta < 0) {
> +		sign = 1;
> +		delta = -delta;
> +	}
> +
> +	if (delta > TSU_NSEC_MAX_VAL) {
> +		gem_tsu_get_time(&bp->ptp_clock_info, &now);
> +		if (sign)
> +			now = timespec64_sub(now, then);
> +		else
> +			now = timespec64_add(now, then);
> +
> +		gem_tsu_set_time(&bp->ptp_clock_info,
> +				 (const struct timespec64 *)&now);
> +	} else {
> +		adj = (sign << GEM_ADDSUB_OFFSET) | delta;
> +
> +		gem_writel(bp, TA, adj);
> +	}
> +
> +	return 0;
> +}
> +
> +static int gem_ptp_enable(struct ptp_clock_info *ptp,
> +			  struct ptp_clock_request *rq, int on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static struct ptp_clock_info gem_ptp_caps_template = {
> +	.owner		= THIS_MODULE,
> +	.name		= GEM_PTP_TIMER_NAME,
> +	.max_adj	= 0,
> +	.n_alarm	= 0,
> +	.n_ext_ts	= 0,
> +	.n_per_out	= 0,
> +	.n_pins		= 0,
> +	.pps		= 1,
> +	.adjfine	= gem_ptp_adjfine,
> +	.adjtime	= gem_ptp_adjtime,
> +	.gettime64	= gem_tsu_get_time,
> +	.settime64	= gem_tsu_set_time,
> +	.enable		= gem_ptp_enable,
> +};
> +
> +static void gem_ptp_init_timer(struct macb *bp)
> +{
> +	u32 rem = 0;
> +
> +	bp->tsu_incr.ns = div_u64_rem(NSEC_PER_SEC, bp->tsu_rate, &rem);
> +	if (rem) {
> +		u64 adj = rem;

This variable belongs above, not in the if-block.

> +		adj <<= GEM_SUBNSINCR_SIZE;
> +		bp->tsu_incr.sub_ns = div_u64(adj, bp->tsu_rate);
> +	} else {
> +		bp->tsu_incr.sub_ns = 0;
> +	}
> +}
> +
> +static void gem_ptp_init_tsu(struct macb *bp)
> +{
> +	struct timespec64 ts;
> +
> +	/* 1. get current system time */
> +	ts = ns_to_timespec64(ktime_to_ns(ktime_get_real()));
> +
> +	/* 2. set ptp timer */
> +	gem_tsu_set_time(&bp->ptp_clock_info, &ts);
> +
> +	/* 3. set PTP timer increment value to BASE_INCREMENT */
> +	gem_tsu_incr_set(bp, &bp->tsu_incr);
> +
> +	gem_writel(bp, TA, 0);
> +}
> +
> +static void gem_ptp_clear_timer(struct macb *bp)
> +{
> +	bp->tsu_incr.ns = 0;
> +	bp->tsu_incr.sub_ns = 0;

What is the point of this function?

> +	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, 0));
> +	gem_writel(bp, TI, GEM_BF(NSINCR, 0));
> +	gem_writel(bp, TA, 0);
> +}
> +
> +static int gem_hw_timestamp(struct macb *bp, u32 dma_desc_ts_1,
> +			    u32 dma_desc_ts_2, struct timespec64 *ts)
> +{
> +	struct timespec64 tsu;
> +
> +	ts->tv_sec = (GEM_BFEXT(DMA_SECH, dma_desc_ts_2) << GEM_DMA_SECL_SIZE) |
> +			GEM_BFEXT(DMA_SECL, dma_desc_ts_1);
> +	ts->tv_nsec = GEM_BFEXT(DMA_NSEC, dma_desc_ts_1);
> +
> +	/* TSU overlapping workaround
> +	 * The timestamp only contains lower few bits of seconds,
> +	 * so add value from 1588 timer
> +	 */
> +	gem_tsu_get_time(&bp->ptp_clock_info, &tsu);
> +
> +	/* If the top bit is set in the timestamp,
> +	 * but not in 1588 timer, it has rolled over,
> +	 * so subtract max size
> +	 */
> +	if ((ts->tv_sec & (GEM_DMA_SEC_TOP >> 1)) &&
> +	    !(tsu.tv_sec & (GEM_DMA_SEC_TOP >> 1)))
> +		ts->tv_sec -= GEM_DMA_SEC_TOP;
> +
> +	ts->tv_sec += ((~GEM_DMA_SEC_MASK) & tsu.tv_sec);
> +
> +	return 0;
> +}
> +
> +void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb,
> +		     struct macb_dma_desc *desc)
> +{
> +	struct timespec64 ts;
> +	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> +	struct macb_dma_desc_ptp *desc_ptp;
> +
> +	if (GEM_BFEXT(DMA_RXVALID, desc->addr)) {
> +		desc_ptp = macb_ptp_desc(bp, desc);
> +		gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
> +		memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
> +		shhwtstamps->hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> +	}
> +}
> +
> +static void gem_tstamp_tx(struct macb *bp, struct sk_buff *skb,
> +			  struct macb_dma_desc_ptp *desc_ptp)
> +{
> +	struct skb_shared_hwtstamps shhwtstamps;
> +	struct timespec64 ts;
> +
> +	gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
> +	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +	shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> +	skb_tstamp_tx(skb, &shhwtstamps);
> +}
> +
> +int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb,
> +		    struct macb_dma_desc *desc)
> +{
> +	struct gem_tx_ts *tx_timestamp;
> +	struct macb_dma_desc_ptp *desc_ptp;
> +	unsigned long head = queue->tx_ts_head;
> +	unsigned long tail = READ_ONCE(queue->tx_ts_tail);
> +
> +	if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl))
> +		return -EINVAL;
> +
> +	if (CIRC_SPACE(head, tail, PTP_TS_BUFFER_SIZE) == 0)
> +		return -ENOMEM;
> +
> +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	desc_ptp = macb_ptp_desc(queue->bp, desc);
> +	tx_timestamp = &queue->tx_timestamps[head];
> +	tx_timestamp->skb = skb;
> +	tx_timestamp->desc_ptp.ts_1 = desc_ptp->ts_1;
> +	tx_timestamp->desc_ptp.ts_2 = desc_ptp->ts_2;
> +	/* move head */
> +	smp_store_release(&queue->tx_ts_head,
> +			  (head + 1) & (PTP_TS_BUFFER_SIZE - 1));
> +
> +	schedule_work(&queue->tx_ts_task);

Since the time stamp is in the buffer descriptor, why delay the
delivery via the work item?

> +	return 0;
> +}
> +
> +static void gem_tx_timestamp_flush(struct work_struct *work)
> +{
> +	struct macb_queue *queue =
> +			container_of(work, struct macb_queue, tx_ts_task);
> +	struct gem_tx_ts *tx_ts;
> +	unsigned long head, tail;
> +
> +	/* take current head */
> +	head = smp_load_acquire(&queue->tx_ts_head);
> +	tail = queue->tx_ts_tail;
> +
> +	while (CIRC_CNT(head, tail, PTP_TS_BUFFER_SIZE)) {
> +		tx_ts = &queue->tx_timestamps[tail];
> +		gem_tstamp_tx(queue->bp, tx_ts->skb, &tx_ts->desc_ptp);
> +		/* cleanup */
> +		dev_kfree_skb_any(tx_ts->skb);
> +		/* remove old tail */
> +		smp_store_release(&queue->tx_ts_tail,
> +				  (tail + 1) & (PTP_TS_BUFFER_SIZE - 1));
> +		tail = queue->tx_ts_tail;
> +	}
> +}
> +
> +void gem_ptp_init(struct net_device *dev)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +	unsigned int q;
> +	struct macb_queue *queue;
> +
> +	bp->ptp_clock_info = gem_ptp_caps_template;
> +
> +	/* nominal frequency and maximum adjustment in ppb */
> +	bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
> +	bp->ptp_clock_info.max_adj = bp->ptp_info->get_ptp_max_adj();
> +	gem_ptp_init_timer(bp);
> +	bp->ptp_clock = ptp_clock_register(&bp->ptp_clock_info, &dev->dev);
> +	if (IS_ERR(&bp->ptp_clock)) {
> +		bp->ptp_clock = NULL;
> +		pr_err("ptp clock register failed\n");
> +		return;
> +	}

ptp_clock_register() can also return NULL, and so you can avoid the
following code in that case, right?

> +
> +	spin_lock_init(&bp->tsu_clk_lock);
> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> +		queue->tx_ts_head = 0;
> +		queue->tx_ts_tail = 0;
> +		INIT_WORK(&queue->tx_ts_task, gem_tx_timestamp_flush);
> +	}
> +
> +	gem_ptp_init_tsu(bp);
> +
> +	dev_info(&bp->pdev->dev, "%s ptp clock registered.\n",
> +		 GEM_PTP_TIMER_NAME);
> +}
> +
> +void gem_ptp_remove(struct net_device *ndev)
> +{
> +	struct macb *bp = netdev_priv(ndev);
> +
> +	if (bp->ptp_clock)
> +		ptp_clock_unregister(bp->ptp_clock);
> +
> +	gem_ptp_clear_timer(bp);

Why is this 'clear' needed?

> +	dev_info(&bp->pdev->dev, "%s ptp clock unregistered.\n",
> +		 GEM_PTP_TIMER_NAME);
> +}

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1659687

FromRafal Ozieblo <rafalo@cadence.com>
Date2017-06-07 13:20 +0200
Message-ID<tPHah-5UP-5@gated-at.bofh.it>
In reply to#1659045
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: 6 czerwca 2017 20:34
> To: Rafal Ozieblo <rafalo@cadence.com>
> Cc: David Miller <davem@davemloft.net>; nicolas.ferre@atmel.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> harini.katakam@xilinx.com; andrei.pistirica@microchip.com
> Subject: Re: [PATCH v2 4/4] net: macb: Add hardware PTP support
> 
> On Fri, Jun 02, 2017 at 03:28:10PM +0100, Rafal Ozieblo wrote:
> > +static void gem_ptp_clear_timer(struct macb *bp)
> > +{
> > +	bp->tsu_incr.ns = 0;
> > +	bp->tsu_incr.sub_ns = 0;
> 
> What is the point of this function?
Cleaning all bellow registers will stop hardware PTP clock.

> 
> > +	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, 0));
> > +	gem_writel(bp, TI, GEM_BF(NSINCR, 0));
> > +	gem_writel(bp, TA, 0);
> > +}
(...)
> > +int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb,
> > +		    struct macb_dma_desc *desc)
> > +{
> > +	struct gem_tx_ts *tx_timestamp;
> > +	struct macb_dma_desc_ptp *desc_ptp;
> > +	unsigned long head = queue->tx_ts_head;
> > +	unsigned long tail = READ_ONCE(queue->tx_ts_tail);
> > +
> > +	if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl))
> > +		return -EINVAL;
> > +
> > +	if (CIRC_SPACE(head, tail, PTP_TS_BUFFER_SIZE) == 0)
> > +		return -ENOMEM;
> > +
> > +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > +	desc_ptp = macb_ptp_desc(queue->bp, desc);
> > +	tx_timestamp = &queue->tx_timestamps[head];
> > +	tx_timestamp->skb = skb;
> > +	tx_timestamp->desc_ptp.ts_1 = desc_ptp->ts_1;
> > +	tx_timestamp->desc_ptp.ts_2 = desc_ptp->ts_2;
> > +	/* move head */
> > +	smp_store_release(&queue->tx_ts_head,
> > +			  (head + 1) & (PTP_TS_BUFFER_SIZE - 1));
> > +
> > +	schedule_work(&queue->tx_ts_task);
> 
> Since the time stamp is in the buffer descriptor, why delay the
> delivery via the work item?

I put comment about that a few months ago:
https://patchwork.ozlabs.org/patch/705629/
Let me quote part about not doing it via worker:

" I think, you can not do it in that way. 
It will hold two locks. If you enable appropriate option in kernel (as far as I 
remember CONFIG_DEBUG_SPINLOCK) you will get a warning here.

Please look at following call-stack:

1. macb_interrupt()   // spin_lock(&bp->lock) is taken
2. macb_tx_interrupt()
3. macb_handle_txtstamp()
4. skb_tstamp_tx()
5. __skb_tstamp_tx()
6. skb_may_tx_timestamp()
7. read_lock_bh() // second lock is taken

I know that those are different locks and different types. But this could lead 
to deadlocks. This is the reason of warning I could see.
And this is the reason why I get timestamp in interrupt routine but pass it to 
skb outside interrupt (using circular buffer).

Please, refer to this:
https://lkml.org/lkml/2016/11/18/168

1. macb_tx_interrupt()
2. macb_tx_timestamp_add() and schedule_work(&queue->tx_timestamp_task)

Then, outside interrupt (without holding a lock) :
1. macb_tx_timestamp_flush()
2. macb_tstamp_tx()
3. skb_tstamp_tx()"
> 
> > +	return 0;
> > +}
(...)
> > +void gem_ptp_remove(struct net_device *ndev)
> > +{
> > +	struct macb *bp = netdev_priv(ndev);
> > +
> > +	if (bp->ptp_clock)
> > +		ptp_clock_unregister(bp->ptp_clock);
> > +
> > +	gem_ptp_clear_timer(bp);
> 
> Why is this 'clear' needed?
To stop hardware PTP clock.
> 
> > +	dev_info(&bp->pdev->dev, "%s ptp clock unregistered.\n",
> > +		 GEM_PTP_TIMER_NAME);
> > +}
> 
> Thanks,
> Richard

I'll correct all other issues.

Regards,
Rafal

[toc] | [prev] | [next] | [standalone]

#1659793

FromRichard Cochran <richardcochran@gmail.com>
Date2017-06-07 15:30 +0200
Message-ID<tPJc6-7aN-15@gated-at.bofh.it>
In reply to#1659687
On Wed, Jun 07, 2017 at 11:13:36AM +0000, Rafal Ozieblo wrote:
> Please look at following call-stack:
> 
> 1. macb_interrupt()   // spin_lock(&bp->lock) is taken
> 2. macb_tx_interrupt()
> 3. macb_handle_txtstamp()
> 4. skb_tstamp_tx()
> 5. __skb_tstamp_tx()
> 6. skb_may_tx_timestamp()
> 7. read_lock_bh() // second lock is taken

Well, you can always drop the lock, or postpone the call to
skb_tstamp_tx() until after the spin lock is released...

> I know that those are different locks and different types. But this could lead 
> to deadlocks. This is the reason of warning I could see.

Can you please post the lockdep splat?

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1661136

FromRafal Ozieblo <rafalo@cadence.com>
Date2017-06-08 14:50 +0200
Message-ID<tQ52W-4l3-11@gated-at.bofh.it>
In reply to#1659793
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: 7 czerwca 2017 15:28
> To: Rafal Ozieblo <rafalo@cadence.com>
> Cc: David Miller <davem@davemloft.net>; nicolas.ferre@atmel.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> harini.katakam@xilinx.com; andrei.pistirica@microchip.com
> Subject: Re: [PATCH v2 4/4] net: macb: Add hardware PTP support
> 
> ********** EXTERNAL MAIL. PLEASE USE CAUTION WHEN CLICKING LINKS
> OR OPENING ATTACHMENTS **********
> 
> On Wed, Jun 07, 2017 at 11:13:36AM +0000, Rafal Ozieblo wrote:
> > Please look at following call-stack:
> >
> > 1. macb_interrupt()   // spin_lock(&bp->lock) is taken
> > 2. macb_tx_interrupt()
> > 3. macb_handle_txtstamp()
> > 4. skb_tstamp_tx()
> > 5. __skb_tstamp_tx()
> > 6. skb_may_tx_timestamp()
> > 7. read_lock_bh() // second lock is taken
> 
> Well, you can always drop the lock, or postpone the call to
> skb_tstamp_tx() until after the spin lock is released...
Postponing is done using worker. Another pros is that skb_tstamp_tx() is called
outside interrupt routine.
> 
> > I know that those are different locks and different types. But this could
> lead
> > to deadlocks. This is the reason of warning I could see.
> 
> Can you please post the lockdep splat?
> 
[ 1264.352242] =================================
[ 1264.356719] [ INFO: inconsistent lock state ]
[ 1264.361242] 4.4.0-42139-g418b7a0-dirty #16 Not tainted
[ 1264.366502] ---------------------------------
[ 1264.371004] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
[ 1264.377204] ptp4l/105 [HC1[1]:SC0[4]:HE0:SE0] takes:
[ 1264.382286]  (&(&list->lock)->rlock){?.-...}, at: [<d03250f8>] skb_queue_tail+0x18/0x34
[ 1264.390622] {HARDIRQ-ON-W} state was registered at:
[ 1264.395618]   [<d0039d50>] lock_acquire+0x124/0x158
[ 1264.400730]   [<d03e81f8>] _raw_spin_lock_bh+0x50/0x5c
[ 1264.406095]   [<d0381745>] first_packet_length+0x35/0x1b4
[ 1264.411710]   [<d0381f80>] udp_poll+0x30/0x3c
[ 1264.416267]   [<d031743d>] sock_poll+0x141/0x148
[ 1264.421092]   [<d00ea20d>] do_sys_poll+0x171/0x300
[ 1264.426111]   [<d00ea41d>] SyS_poll+0x45/0x80
[ 1264.430676]   [<d000481c>] system_call+0x40/0x50
[ 1264.435492]   [<d00043d1>] common_exception_return+0x0/0x83
[ 1264.441274] irq event stamp: 23325
[ 1264.444802] hardirqs last  enabled at (23324): [<d03e84da>] _raw_spin_unlock_irqrestore+0x4e/0x78
[ 1264.453929] hardirqs last disabled at (23325): [<d00065af>] do_interrupt+0x13/0x90
[ 1264.461732] softirqs last  enabled at (23292): [<d0362cf8>] ip_finish_output2+0x80/0x43c
[ 1264.470073] softirqs last disabled at (23294): [<d0362da6>] ip_finish_output2+0x12e/0x43c
[ 1264.478480] 
[ 1264.478480] other info that might help us debug this:
[ 1264.485211]  Possible unsafe locking scenario:
[ 1264.485211] 
[ 1264.491323]        CPU0
[ 1264.493883]        ----
[ 1264.496439]   lock(&(&list->lock)->rlock);
[ 1264.500665]   <Interrupt>
[ 1264.503392]     lock(&(&list->lock)->rlock);
[ 1264.507793] 
[ 1264.507793]  *** DEADLOCK ***
[ 1264.507793] 
[ 1264.514016] 4 locks held by ptp4l/105:
[ 1264.517886]  #0:  (rcu_read_lock_bh){......}, at: [<d0362da6>] ip_finish_output2+0x12e/0x43c
[ 1264.526675]  #1:  (rcu_read_lock_bh){......}, at: [<d03330f4>] __dev_queue_xmit+0x38/0x5f8
[ 1264.535287]  #2:  (_xmit_ETHER#2){+.-...}, at: [<d0348381>] sch_direct_xmit+0x51/0x138
[ 1264.543593]  #3:  (&(&bp->lock)->rlock){-.-...}, at: [<d02caeb2>] macb_interrupt+0x36/0x564
[ 1264.552316] 
[ 1264.552316] stack backtrace:
[ 1264.556962] CPU: 0 PID: 105 Comm: ptp4l Not tainted 4.4.0-42139-g418b7a0-dirty #16
[ 1264.564654] 
[ 1264.564654] Stack: d1786130 d75815e3 00001ffc d75815e0 9003811c d7581620 d7558c00 d1c0bd60 
[ 1264.564654]        00000004 00000001 00000001 d7581600 900389fc d7581650 d7558c00 d7559160 
[ 1264.564654]        00000004 00000000 00000000 d7559160 d04ecc34 d1bf2de0 00000179 d7581620 
[ 1264.588339] Call Trace:
[ 1264.590894]  [<d009a8af>] print_usage_bug.part.33+0x1c7/0x1cc
[ 1264.596883]  [<d003811c>] mark_lock+0x2f4/0x44c
[ 1264.601639]  [<d00389fc>] __lock_acquire+0x460/0x1228
[ 1264.606907]  [<d0039d50>] lock_acquire+0x124/0x158
[ 1264.611907]  [<d03e8335>] _raw_spin_lock_irqsave+0x45/0x54
[ 1264.617612]  [<d03250f8>] skb_queue_tail+0x18/0x34
[ 1264.622594]  [<d03251f0>] sock_queue_err_skb+0xdc/0x108
[ 1264.628017]  [<d0326705>] __skb_complete_tx_timestamp+0x55/0x60
[ 1264.634146]  [<d0326810>] __skb_tstamp_tx+0x94/0x98
[ 1264.639215]  [<d032682c>] skb_tstamp_tx+0x18/0x1c
[ 1264.644110]  [<d02cb065>] macb_interrupt+0x1e9/0x564
[ 1264.649302]  [<d0041af5>] handle_irq_event_percpu+0xe9/0x2a4
[ 1264.655179]  [<d0041ce6>] handle_irq_event+0x36/0x50
[ 1264.660343]  [<d0043965>] handle_level_irq+0x99/0xb8
[ 1264.665525]  [<d00415c5>] generic_handle_irq+0x19/0x24
[ 1264.670863]  [<d000491d>] do_IRQ+0x35/0x38
[ 1264.675141]  [<d000660c>] do_interrupt+0x70/0x90
[ 1264.679959]  [<d00043d1>] common_exception_return+0x0/0x83
[ 1264.685635]  [<d02c8eb4>] macb_start_xmit+0x414/0x418
[ 1264.690905]  [<d0332eb1>] dev_hard_start_xmit+0x2ed/0x450
[ 1264.696521]  [<d034839e>] sch_direct_xmit+0x6e/0x138
[ 1264.701720]  [<d0333449>] __dev_queue_xmit+0x38d/0x5f8
[ 1264.707058]  [<d03336c8>] dev_queue_xmit+0x14/0x18
[ 1264.712049]  [<d033ab64>] neigh_resolve_output+0x180/0x190
[ 1264.717734]  [<d0362ff2>] ip_finish_output2+0x37a/0x43c
[ 1264.723186]  [<d0364168>] ip_finish_output+0x1b8/0x200
[ 1264.728532]  [<d0364969>] ip_mc_output+0x179/0x1cc
[ 1264.733532]  [<d03642e5>] ip_local_out+0x45/0x5c
[ 1264.738359]  [<d03651ae>] ip_send_skb+0x16/0x64
[ 1264.743097]  [<d03815f4>] udp_send_skb+0x160/0x1c8
[ 1264.748098]  [<d0382741>] udp_sendmsg+0x45d/0x5a8
[ 1264.753009]  [<d038a7f9>] inet_sendmsg+0x21/0x3c
[ 1264.757801]  [<d0317de5>] sock_sendmsg+0x1d/0x44
[ 1264.762629]  [<d03188e1>] SyS_sendto+0x89/0xa4
[ 1264.767288]  [<d000481c>] system_call+0x40/0x50
[ 1264.772010]  [<d00043d1>] common_exception_return+0x0/0x83

> Thanks,
> Richard

Regards,
Rafal

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web