[PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping

[PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping

[Yangbo Lu]

From: Ioana Radulescu <ruxandra.radulescu@nxp.com>

Hardware timestamping is supported both on Rx and Tx paths.
On Rx, timestamping is enabled for all frames. On Tx, we
only instruct the hardware to timestamp the frames marked
accordingly by the stack.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c |  109 +++++++++++++++++++++++-
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h |   48 ++++++++++-
 2 files changed, 151 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 553678d..d9febba 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -38,8 +38,11 @@
 #include <linux/msi.h>
 #include <linux/kthread.h>
 #include <linux/iommu.h>
-
+#include <linux/net_tstamp.h>
 #include <linux/fsl/mc.h>
+
+#include <net/sock.h>
+
 #include "dpaa2-eth.h"
 
 /* CREATE_TRACE_POINTS only needs to be defined once. Other dpa files
@@ -275,6 +278,16 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
 
 	prefetch(skb->data);
 
+	/* Get the timestamp value */
+	if (priv->ts_rx_en) {
+		struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
+		u64 *ns = dpaa2_get_ts(vaddr, false);
+
+		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
+		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+		shhwtstamps->hwtstamp = ns_to_ktime(*ns);
+	}
+
 	/* Check if we need to validate the L4 csum */
 	if (likely(dpaa2_fd_get_frc(fd) & DPAA2_FD_FRC_FASV)) {
 		status = le32_to_cpu(fas->status);
@@ -334,6 +347,28 @@ static int consume_frames(struct dpaa2_eth_channel *ch)
 	return cleaned;
 }
 
+/* Configure the egress frame annotation for timestamp update */
+static void enable_tx_tstamp(struct dpaa2_fd *fd, void *buf_start)
+{
+	struct dpaa2_faead *faead;
+	u32 ctrl, frc;
+
+	/* Mark the egress frame annotation area as valid */
+	frc = dpaa2_fd_get_frc(fd);
+	dpaa2_fd_set_frc(fd, frc | DPAA2_FD_FRC_FAEADV);
+
+	/* Set hardware annotation size */
+	ctrl = dpaa2_fd_get_ctrl(fd);
+	dpaa2_fd_set_ctrl(fd, ctrl | DPAA2_FD_CTRL_ASAL);
+
+	/* enable UPD (update prepanded data) bit in FAEAD field of
+	 * hardware frame annotation area
+	 */
+	ctrl = DPAA2_FAEAD_A2V | DPAA2_FAEAD_UPDV | DPAA2_FAEAD_UPD;
+	faead = dpaa2_get_faead(buf_start, true);
+	faead->ctrl = cpu_to_le32(ctrl);
+}
+
 /* Create a frame descriptor based on a fragmented skb */
 static int build_sg_fd(struct dpaa2_eth_priv *priv,
 		       struct sk_buff *skb,
@@ -420,6 +455,9 @@ static int build_sg_fd(struct dpaa2_eth_priv *priv,
 	dpaa2_fd_set_len(fd, skb->len);
 	dpaa2_fd_set_ctrl(fd, DPAA2_FD_CTRL_PTA | DPAA2_FD_CTRL_PTV1);
 
+	if (priv->ts_tx_en && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
+		enable_tx_tstamp(fd, sgt_buf);
+
 	return 0;
 
 dma_map_single_failed:
@@ -470,6 +508,9 @@ static int build_single_fd(struct dpaa2_eth_priv *priv,
 	dpaa2_fd_set_format(fd, dpaa2_fd_single);
 	dpaa2_fd_set_ctrl(fd, DPAA2_FD_CTRL_PTA | DPAA2_FD_CTRL_PTV1);
 
+	if (priv->ts_tx_en && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
+		enable_tx_tstamp(fd, buffer_start);
+
 	return 0;
 }
 
@@ -520,6 +561,19 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv,
 		return;
 	}
 
+	/* Get the timestamp value */
+	if (priv->ts_tx_en && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
+		struct skb_shared_hwtstamps shhwtstamps;
+		u64 *ns;
+
+		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+
+		ns = dpaa2_get_ts(skbh, true);
+		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
+		shhwtstamps.hwtstamp = ns_to_ktime(*ns);
+		skb_tstamp_tx(skb, &shhwtstamps);
+	}
+
 	/* Free SGT buffer allocated on tx */
 	if (fd_format != dpaa2_fd_single)
 		skb_free_frag(skbh);
@@ -552,6 +606,10 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev)
 			goto err_alloc_headroom;
 		}
 		percpu_extras->tx_reallocs++;
+
+		if (skb->sk)
+			skb_set_owner_w(ns, skb->sk);
+
 		dev_kfree_skb(skb);
 		skb = ns;
 	}
@@ -1365,6 +1423,45 @@ static int dpaa2_eth_set_features(struct net_device *net_dev,
 	return 0;
 }
 
+static int dpaa2_eth_ts_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
+{
+	struct dpaa2_eth_priv *priv = netdev_priv(dev);
+	struct hwtstamp_config config;
+
+	if (copy_from_user(&config, rq->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	switch (config.tx_type) {
+	case HWTSTAMP_TX_OFF:
+		priv->ts_tx_en = false;
+		break;
+	case HWTSTAMP_TX_ON:
+		priv->ts_tx_en = true;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	if (config.rx_filter == HWTSTAMP_FILTER_NONE) {
+		priv->ts_rx_en = false;
+	} else {
+		priv->ts_rx_en = true;
+		/* TS is set for all frame types, not only those requested */
+		config.rx_filter = HWTSTAMP_FILTER_ALL;
+	}
+
+	return copy_to_user(rq->ifr_data, &config, sizeof(config)) ?
+			-EFAULT : 0;
+}
+
+static int dpaa2_eth_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
+{
+	if (cmd == SIOCSHWTSTAMP)
+		return dpaa2_eth_ts_ioctl(dev, rq, cmd);
+
+	return -EINVAL;
+}
+
 static const struct net_device_ops dpaa2_eth_ops = {
 	.ndo_open = dpaa2_eth_open,
 	.ndo_start_xmit = dpaa2_eth_tx,
@@ -1375,6 +1472,7 @@ static int dpaa2_eth_set_features(struct net_device *net_dev,
 	.ndo_change_mtu = dpaa2_eth_change_mtu,
 	.ndo_set_rx_mode = dpaa2_eth_set_rx_mode,
 	.ndo_set_features = dpaa2_eth_set_features,
+	.ndo_do_ioctl = dpaa2_eth_ioctl,
 };
 
 static void cdan_cb(struct dpaa2_io_notification_ctx *ctx)
@@ -1770,7 +1868,9 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv)
 
 	/* tx buffer */
 	buf_layout.private_data_size = DPAA2_ETH_SWA_SIZE;
-	buf_layout.options = DPNI_BUF_LAYOUT_OPT_PRIVATE_DATA_SIZE;
+	buf_layout.pass_timestamp = true;
+	buf_layout.options = DPNI_BUF_LAYOUT_OPT_PRIVATE_DATA_SIZE |
+			     DPNI_BUF_LAYOUT_OPT_TIMESTAMP;
 	err = dpni_set_buffer_layout(priv->mc_io, 0, priv->mc_token,
 				     DPNI_QUEUE_TX, &buf_layout);
 	if (err) {
@@ -1779,7 +1879,7 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv)
 	}
 
 	/* tx-confirm buffer */
-	buf_layout.options = 0;
+	buf_layout.options = DPNI_BUF_LAYOUT_OPT_TIMESTAMP;
 	err = dpni_set_buffer_layout(priv->mc_io, 0, priv->mc_token,
 				     DPNI_QUEUE_TX_CONFIRM, &buf_layout);
 	if (err) {
@@ -1810,7 +1910,8 @@ static int set_buffer_layout(struct dpaa2_eth_priv *priv)
 	buf_layout.options = DPNI_BUF_LAYOUT_OPT_PARSER_RESULT |
 			     DPNI_BUF_LAYOUT_OPT_FRAME_STATUS |
 			     DPNI_BUF_LAYOUT_OPT_DATA_ALIGN |
-			     DPNI_BUF_LAYOUT_OPT_DATA_HEAD_ROOM;
+			     DPNI_BUF_LAYOUT_OPT_DATA_HEAD_ROOM |
+			     DPNI_BUF_LAYOUT_OPT_TIMESTAMP;
 	err = dpni_set_buffer_layout(priv->mc_io, 0, priv->mc_token,
 				     DPNI_QUEUE_RX, &buf_layout);
 	if (err) {
diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
index 54cea2f..1300f28 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
@@ -88,8 +88,12 @@
 #define DPAA2_ETH_SKB_SIZE \
 	(DPAA2_ETH_RX_BUF_SIZE + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
 
-/* Hardware annotation area in RX  buffers */
+/* Hardware annotation area in RX/TX buffers */
 #define DPAA2_ETH_RX_HWA_SIZE		64
+#define DPAA2_ETH_TX_HWA_SIZE		128
+
+/* PTP nominal frequency 1GHz */
+#define DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS 1
 
 /* Due to a limitation in WRIOP 1.0.0, the RX buffer data must be aligned
  * to 256B. For newer revisions, the requirement is only for 64B alignment
@@ -135,6 +139,7 @@ struct dpaa2_eth_swa {
 /* Annotation bits in FD CTRL */
 #define DPAA2_FD_CTRL_PTA		0x00800000
 #define DPAA2_FD_CTRL_PTV1		0x00400000
+#define DPAA2_FD_CTRL_ASAL		0x00020000	/* ASAL = 128B */
 
 /* Frame annotation status */
 struct dpaa2_fas {
@@ -150,6 +155,23 @@ struct dpaa2_fas {
 #define DPAA2_FAS_OFFSET		0
 #define DPAA2_FAS_SIZE			(sizeof(struct dpaa2_fas))
 
+/* Timestamp is located in the next 8 bytes of the buffer's
+ * hardware annotation area
+ */
+#define DPAA2_TS_OFFSET			0x8
+
+/* Frame annotation egress action descriptor */
+#define DPAA2_FAEAD_OFFSET		0x58
+
+struct dpaa2_faead {
+	__le32 conf_fqid;
+	__le32 ctrl;
+};
+
+#define DPAA2_FAEAD_A2V			0x20000000
+#define DPAA2_FAEAD_UPDV		0x00001000
+#define DPAA2_FAEAD_UPD			0x00000010
+
 /* Accessors for the hardware annotation fields that we use */
 static inline void *dpaa2_get_hwa(void *buf_addr, bool swa)
 {
@@ -161,6 +183,16 @@ struct dpaa2_fas {
 	return dpaa2_get_hwa(buf_addr, swa) + DPAA2_FAS_OFFSET;
 }
 
+static inline u64 *dpaa2_get_ts(void *buf_addr, bool swa)
+{
+	return dpaa2_get_hwa(buf_addr, swa) + DPAA2_TS_OFFSET;
+}
+
+static inline struct dpaa2_faead *dpaa2_get_faead(void *buf_addr, bool swa)
+{
+	return dpaa2_get_hwa(buf_addr, swa) + DPAA2_FAEAD_OFFSET;
+}
+
 /* Error and status bits in the frame annotation status word */
 /* Debug frame, otherwise supposed to be discarded */
 #define DPAA2_FAS_DISC			0x80000000
@@ -319,6 +351,9 @@ struct dpaa2_eth_priv {
 	u16 bpid;
 	struct iommu_domain *iommu_domain;
 
+	bool ts_tx_en; /* Tx timestamping enabled */
+	bool ts_rx_en; /* Rx timestamping enabled */
+
 	u16 tx_qdid;
 	u16 rx_buf_align;
 	struct fsl_mc_io *mc_io;
@@ -377,10 +412,19 @@ static inline unsigned int dpaa2_eth_buf_raw_size(struct dpaa2_eth_priv *priv)
 unsigned int dpaa2_eth_needed_headroom(struct dpaa2_eth_priv *priv,
 				       struct sk_buff *skb)
 {
+	unsigned int headroom = DPAA2_ETH_SWA_SIZE;
+
+	/* For non-linear skbs we have no headroom requirement, as we build a
+	 * SG frame with a newly allocated SGT buffer
+	 */
 	if (skb_is_nonlinear(skb))
 		return 0;
 
-	return DPAA2_ETH_SWA_SIZE;
+	/* If we have Tx timestamping, need 128B hardware annotation */
+	if (priv->ts_tx_en && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
+		headroom += DPAA2_ETH_TX_HWA_SIZE;
+
+	return headroom;
 }
 
 /* Extra headroom space requested to hardware, in order to make sure there's
-- 
1.7.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping

[Dan Carpenter]

On Wed, Apr 25, 2018 at 05:17:49PM +0800, Yangbo Lu wrote:
> @@ -275,6 +278,16 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
>  
>  	prefetch(skb->data);
>  
> +	/* Get the timestamp value */
> +	if (priv->ts_rx_en) {
> +		struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> +		u64 *ns = dpaa2_get_ts(vaddr, false);
> +
> +		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);

This will cause Sparse endianess warnings.

I don't totally understand why we're writing to *ns.  Do we access *ns
again or not?  Either way, this doesn't seem right.  In other words, why
don't we do this:

		__le64 *period = dpaa2_get_ts(vaddr, false);
		u64 ns;

		ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(period);
		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
		shhwtstamps->hwtstamp = ns_to_ktime(ns);

Then if we need to save a munged *ns then we can do this at the end:

		/* we need this because blah blah blah */
		*period = (__le64)ns;


> +		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> +		shhwtstamps->hwtstamp = ns_to_ktime(*ns);
> +	}
> +
>  	/* Check if we need to validate the L4 csum */
>  	if (likely(dpaa2_fd_get_frc(fd) & DPAA2_FD_FRC_FASV)) {
>  		status = le32_to_cpu(fas->status);

[ snip ]

> @@ -520,6 +561,19 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv,
>  		return;
>  	}
>  
> +	/* Get the timestamp value */
> +	if (priv->ts_tx_en && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> +		struct skb_shared_hwtstamps shhwtstamps;
> +		u64 *ns;
> +
> +		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +
> +		ns = dpaa2_get_ts(skbh, true);
> +		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
> +		shhwtstamps.hwtstamp = ns_to_ktime(*ns);
> +		skb_tstamp_tx(skb, &shhwtstamps);

Sparse issues here also.

> +	}
> +
>  	/* Free SGT buffer allocated on tx */
>  	if (fd_format != dpaa2_fd_single)
>  		skb_free_frag(skbh);
> @@ -552,6 +606,10 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev)
>  			goto err_alloc_headroom;
>  		}
>  		percpu_extras->tx_reallocs++;
> +
> +		if (skb->sk)
> +			skb_set_owner_w(ns, skb->sk);

Is this really related?  (I have not looked at this code).

> +
>  		dev_kfree_skb(skb);
>  		skb = ns;
>  	}

[ snip ]

> @@ -319,6 +351,9 @@ struct dpaa2_eth_priv {
>  	u16 bpid;
>  	struct iommu_domain *iommu_domain;
>  
> +	bool ts_tx_en; /* Tx timestamping enabled */
> +	bool ts_rx_en; /* Rx timestamping enabled */

These variable names are not great.  I wouldn't have understood "ts_"
without the comment.  "tx_" is good.  "en" is confusing until you read
the comment.  But really it should just be left out because "enable" is
assumed, generally.  Last week I asked someone to rewrite a patch that
had a _disable variable because negative variables lead to double
negatives which screw with my tiny head.

	if (blah_disable != 0) {

OH MY BLASTED WORD MY BRIAN ESPLODED!!!1!

So let's just name these "tx_timestamps" or something.


> +
>  	u16 tx_qdid;
>  	u16 rx_buf_align;
>  	struct fsl_mc_io *mc_io;

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping

[Richard Cochran]

On Wed, Apr 25, 2018 at 05:17:49PM +0800, Yangbo Lu wrote:

> +/* PTP nominal frequency 1GHz */
> +#define DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS 1

Nit: Frequency is the inverse of the period.  It can be one or the
other, not both.

Why not call it simply DPAA2_PTP_CLK_PERIOD_NS?

You haven't implemented the ethtool_get_ts_info() method, but you need
to do so.  Show us how the user can related these MAC time stamps to
the PHC device in your other patch series.

Thanks,
Richard
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping

[Y.b. Lu]

Hi Dan,

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, April 25, 2018 6:04 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; Richard Cochran
> <richardcochran@gmail.com>; Ruxandra Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>
> Subject: Re: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware
> timestamping
> 
> On Wed, Apr 25, 2018 at 05:17:49PM +0800, Yangbo Lu wrote:
> > @@ -275,6 +278,16 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv
> > *priv,
> >
> >  	prefetch(skb->data);
> >
> > +	/* Get the timestamp value */
> > +	if (priv->ts_rx_en) {
> > +		struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> > +		u64 *ns = dpaa2_get_ts(vaddr, false);
> > +
> > +		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
> 
> This will cause Sparse endianess warnings.
> 
> I don't totally understand why we're writing to *ns.  Do we access *ns again
> or not?  Either way, this doesn't seem right.  In other words, why don't we
> do this:
> 
> 		__le64 *period = dpaa2_get_ts(vaddr, false);
> 		u64 ns;
> 
> 		ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS *
> le64_to_cpup(period);
> 		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> 		shhwtstamps->hwtstamp = ns_to_ktime(ns);
> 
> Then if we need to save a munged *ns then we can do this at the end:
> 
> 		/* we need this because blah blah blah */
> 		*period = (__le64)ns;
> 

[Y.b. Lu] You're right. I will modify the code according to your suggestion.

> 
> > +		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > +		shhwtstamps->hwtstamp = ns_to_ktime(*ns);
> > +	}
> > +
> >  	/* Check if we need to validate the L4 csum */
> >  	if (likely(dpaa2_fd_get_frc(fd) & DPAA2_FD_FRC_FASV)) {
> >  		status = le32_to_cpu(fas->status);
> 
> [ snip ]
> 
> > @@ -520,6 +561,19 @@ static void free_tx_fd(const struct dpaa2_eth_priv
> *priv,
> >  		return;
> >  	}
> >
> > +	/* Get the timestamp value */
> > +	if (priv->ts_tx_en && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> > +		struct skb_shared_hwtstamps shhwtstamps;
> > +		u64 *ns;
> > +
> > +		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> > +
> > +		ns = dpaa2_get_ts(skbh, true);
> > +		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
> > +		shhwtstamps.hwtstamp = ns_to_ktime(*ns);
> > +		skb_tstamp_tx(skb, &shhwtstamps);
> 
> Sparse issues here also.

[Y.b. Lu] Will modify the code according to your suggestion.

> 
> > +	}
> > +
> >  	/* Free SGT buffer allocated on tx */
> >  	if (fd_format != dpaa2_fd_single)
> >  		skb_free_frag(skbh);
> > @@ -552,6 +606,10 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff
> *skb, struct net_device *net_dev)
> >  			goto err_alloc_headroom;
> >  		}
> >  		percpu_extras->tx_reallocs++;
> > +
> > +		if (skb->sk)
> > +			skb_set_owner_w(ns, skb->sk);
> 
> Is this really related?  (I have not looked at this code).

[Y.b. Lu] Yes. The skb_tstamp_tx() function will check that.

> 
> > +
> >  		dev_kfree_skb(skb);
> >  		skb = ns;
> >  	}
> 
> [ snip ]
> 
> > @@ -319,6 +351,9 @@ struct dpaa2_eth_priv {
> >  	u16 bpid;
> >  	struct iommu_domain *iommu_domain;
> >
> > +	bool ts_tx_en; /* Tx timestamping enabled */
> > +	bool ts_rx_en; /* Rx timestamping enabled */
> 
> These variable names are not great.  I wouldn't have understood "ts_"
> without the comment.  "tx_" is good.  "en" is confusing until you read the
> comment.  But really it should just be left out because "enable" is assumed,
> generally.  Last week I asked someone to rewrite a patch that had a _disable
> variable because negative variables lead to double negatives which screw with
> my tiny head.
> 
> 	if (blah_disable != 0) {
> 
> OH MY BLASTED WORD MY BRIAN ESPLODED!!!1!
> 
> So let's just name these "tx_timestamps" or something.

[Y.b. Lu] Ok. Let me use tx_tstamp/rx_tstamp instead. The tstamp is common used in driver.

> 
> 
> > +
> >  	u16 tx_qdid;
> >  	u16 rx_buf_align;
> >  	struct fsl_mc_io *mc_io;
> 
> regards,
> dan carpenter

RE: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping

[Y.b. Lu]

Hi Richard,


> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, April 26, 2018 10:33 AM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; dan.carpenter@oracle.com;
> Ruxandra Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>
> Subject: Re: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware
> timestamping
> 
> On Wed, Apr 25, 2018 at 05:17:49PM +0800, Yangbo Lu wrote:
> 
> > +/* PTP nominal frequency 1GHz */
> > +#define DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS 1
> 
> Nit: Frequency is the inverse of the period.  It can be one or the other, not
> both.
> 
> Why not call it simply DPAA2_PTP_CLK_PERIOD_NS?

[Y.b. Lu] Thanks. Let me use DPAA2_PTP_CLK_PERIOD_NS in next version.

> 
> You haven't implemented the ethtool_get_ts_info() method, but you need to
> do so.  Show us how the user can related these MAC time stamps to the PHC
> device in your other patch series.

[Y.b. Lu] I will add patch for ethtool_get_ts_info in next version. Thanks!
But I'm not sure whether ethtool should show hw timestamping always ignoring phc enabled or not.
Actually the hw timestamping is supported always since ptp clock had been initialized in MC firmware.
Let me just send out the patch for reviewing.

> 
> Thanks,
> Richard
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel