[v1] net/axgbe: enable IEEE 1588 PTP support for axgbe

  From: Selwin Sebastian <selwin.sebastian@amd.com>

Add ethdev APIs to support PTP timestamping

Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
---
 drivers/net/axgbe/axgbe_common.h |   8 +
 drivers/net/axgbe/axgbe_ethdev.c | 327 ++++++++++++++++++++++++++++++-
 drivers/net/axgbe/axgbe_ethdev.h |  14 ++
 drivers/net/axgbe/axgbe_rxtx.c   |  11 +-
 4 files changed, 358 insertions(+), 2 deletions(-)
  

[AMD Public Use]

Acked-by: Somalapuram Amaranath <asomalap@amd.com>

-----Original Message-----
From: Sebastian, Selwin <Selwin.Sebastian@amd.com>
Sent: Monday, June 1, 2020 6:28 PM
To: dev@dpdk.org
Cc: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>
Subject: [PATCH v1] net/axgbe: enable IEEE 1588 PTP support for axgbe

From: Selwin Sebastian <selwin.sebastian@amd.com>

Add ethdev APIs to support PTP timestamping

Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
---
 drivers/net/axgbe/axgbe_common.h |   8 +
 drivers/net/axgbe/axgbe_ethdev.c | 327 ++++++++++++++++++++++++++++++-  drivers/net/axgbe/axgbe_ethdev.h |  14 ++
 drivers/net/axgbe/axgbe_rxtx.c   |  11 +-
 4 files changed, 358 insertions(+), 2 deletions(-)

diff --git a/drivers/net/axgbe/axgbe_common.h b/drivers/net/axgbe/axgbe_common.h
index f48117180..e0a0b0fbb 100644
--- a/drivers/net/axgbe/axgbe_common.h
+++ b/drivers/net/axgbe/axgbe_common.h
@@ -45,6 +45,7 @@
 #endif

 #define AXGBE_HZ250
+#define NSEC_PER_SEC    1000000000L

 /* DMA register offsets */
 #define DMA_MR0x3000
@@ -491,6 +492,8 @@
 #define MAC_TSCR_TSEVNTENA_WIDTH1
 #define MAC_TSCR_TSINIT_INDEX2
 #define MAC_TSCR_TSINIT_WIDTH1
+#define MAC_TSCR_TSUPDT_INDEX3
+#define MAC_TSCR_TSUPDT_WIDTH1
 #define MAC_TSCR_TSIPENA_INDEX11
 #define MAC_TSCR_TSIPENA_WIDTH1
 #define MAC_TSCR_TSIPV4ENA_INDEX13
@@ -505,6 +508,8 @@
 #define MAC_TSCR_TXTSSTSM_WIDTH1
 #define MAC_TSSR_TXTSC_INDEX15
 #define MAC_TSSR_TXTSC_WIDTH1
+#define MAC_STNUR_ADDSUB_INDEX          31
+#define MAC_STNUR_ADDSUB_WIDTH          1
 #define MAC_TXSNR_TXTSSTSMIS_INDEX31
 #define MAC_TXSNR_TXTSSTSMIS_WIDTH1
 #define MAC_VLANHTR_VLHT_INDEX0
@@ -538,6 +543,7 @@
 #define MAC_VR_USERVER_INDEX16
 #define MAC_VR_USERVER_WIDTH8

+
 /* MMC register offsets */
 #define MMC_CR0x0800
 #define MMC_RISR0x0804
@@ -1170,6 +1176,8 @@
 #define RX_CONTEXT_DESC3_TSA_WIDTH1
 #define RX_CONTEXT_DESC3_TSD_INDEX6
 #define RX_CONTEXT_DESC3_TSD_WIDTH1
+#define RX_CONTEXT_DESC3_PMT_INDEX0
+#define RX_CONTEXT_DESC3_PMT_WIDTH4

 #define TX_PACKET_ATTRIBUTES_CSUM_ENABLE_INDEX0
 #define TX_PACKET_ATTRIBUTES_CSUM_ENABLE_WIDTH1
diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 867058845..18c44404c 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -8,6 +8,7 @@
 #include "axgbe_common.h"
 #include "axgbe_phy.h"
 #include "axgbe_regs.h"
+#include "rte_time.h"

 static int eth_axgbe_dev_init(struct rte_eth_dev *eth_dev);  static int eth_axgbe_dev_uninit(struct rte_eth_dev *eth_dev); @@ -74,6 +75,24 @@ static void axgbe_txq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 struct rte_eth_txq_info *qinfo);
 const uint32_t *axgbe_dev_supported_ptypes_get(struct rte_eth_dev *dev);

+static int axgbe_timesync_enable(struct rte_eth_dev *dev); static int
+axgbe_timesync_disable(struct rte_eth_dev *dev); static int
+axgbe_timesync_read_rx_timestamp(struct rte_eth_dev *dev,
+struct timespec *timestamp, uint32_t flags); static int
+axgbe_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
+struct timespec *timestamp);
+static int axgbe_timesync_adjust_time(struct rte_eth_dev *dev, int64_t
+delta); static int axgbe_timesync_read_time(struct rte_eth_dev *dev,
+struct timespec *timestamp);
+static int axgbe_timesync_write_time(struct rte_eth_dev *dev,
+const struct timespec *timestamp);
+static void axgbe_set_tstamp_time(struct axgbe_port *pdata, unsigned int sec,
+unsigned int nsec);
+static void axgbe_update_tstamp_addend(struct axgbe_port *pdata,
+unsigned int addend);
+
 struct axgbe_xstats {
 char name[RTE_ETH_XSTATS_NAME_SIZE];
 int offset;
@@ -214,6 +233,14 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
 .dev_supported_ptypes_get     = axgbe_dev_supported_ptypes_get,
 .rx_descriptor_status         = axgbe_dev_rx_descriptor_status,
 .tx_descriptor_status         = axgbe_dev_tx_descriptor_status,
+.timesync_enable              = axgbe_timesync_enable,
+.timesync_disable             = axgbe_timesync_disable,
+.timesync_read_rx_timestamp   = axgbe_timesync_read_rx_timestamp,
+.timesync_read_tx_timestamp   = axgbe_timesync_read_tx_timestamp,
+.timesync_adjust_time         = axgbe_timesync_adjust_time,
+.timesync_read_time           = axgbe_timesync_read_time,
+.timesync_write_time          = axgbe_timesync_write_time,
+
 };

 static int axgbe_phy_reset(struct axgbe_port *pdata) @@ -1000,12 +1027,16 @@ axgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 DEV_RX_OFFLOAD_TCP_CKSUM  |
 DEV_RX_OFFLOAD_JUMBO_FRAME|
 DEV_RX_OFFLOAD_SCATTER  |
+PKT_RX_IEEE1588_PTP |
 DEV_RX_OFFLOAD_KEEP_CRC;

 dev_info->tx_offload_capa =
 DEV_TX_OFFLOAD_IPV4_CKSUM  |
 DEV_TX_OFFLOAD_UDP_CKSUM   |
-DEV_TX_OFFLOAD_TCP_CKSUM;
+DEV_TX_OFFLOAD_TCP_CKSUM   |
+PKT_TX_IEEE1588_TMST |
+DEV_TX_OFFLOAD_MULTI_SEGS;
+

 if (pdata->hw_feat.rss) {
 dev_info->flow_type_rss_offloads = AXGBE_RSS_OFFLOAD; @@ -1255,6 +1286,300 @@ axgbe_dev_supported_ptypes_get(struct rte_eth_dev *dev)
 return NULL;
 }

+static void axgbe_update_tstamp_time(struct axgbe_port *pdata,
+unsigned int sec, unsigned int nsec, int addsub) {
+unsigned int count = 100;
+uint32_t sub_val = 0;
+uint32_t sub_val_sec = 0xFFFFFFFF;
+uint32_t sub_val_nsec = 0x3B9ACA00;
+
+if (addsub) {
+if (sec)
+sub_val = sub_val_sec - (sec - 1);
+else
+sub_val = sec;
+
+AXGMAC_IOWRITE(pdata, MAC_STSUR, sub_val);
+sub_val = sub_val_nsec - nsec;
+AXGMAC_IOWRITE(pdata, MAC_STNUR, sub_val);
+AXGMAC_IOWRITE_BITS(pdata, MAC_STNUR, ADDSUB, 1);
+} else {
+AXGMAC_IOWRITE(pdata, MAC_STSUR, sec);
+AXGMAC_IOWRITE_BITS(pdata, MAC_STNUR, ADDSUB, 0);
+AXGMAC_IOWRITE(pdata, MAC_STNUR, nsec);
+}
+AXGMAC_IOWRITE_BITS(pdata, MAC_TSCR, TSUPDT, 1);
+/* Wait for time update to complete */
+while (--count && AXGMAC_IOREAD_BITS(pdata, MAC_TSCR, TSUPDT))
+rte_delay_ms(1);
+}
+
+static inline uint64_t
+div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder) {
+*remainder = dividend % divisor;
+return dividend / divisor;
+}
+
+static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor) {
+uint32_t remainder;
+return div_u64_rem(dividend, divisor, &remainder); }
+
+static int axgbe_adjfreq(struct axgbe_port *pdata, int64_t delta) {
+uint64_t adjust;
+uint32_t addend, diff;
+unsigned int neg_adjust = 0;
+
+if (delta < 0) {
+neg_adjust = 1;
+delta = -delta;
+}
+adjust = (uint64_t)pdata->tstamp_addend;
+adjust *= delta;
+diff = (uint32_t)div_u64(adjust, 1000000000UL);
+addend = (neg_adjust) ? pdata->tstamp_addend - diff :
+pdata->tstamp_addend + diff;
+pdata->tstamp_addend = addend;
+axgbe_update_tstamp_addend(pdata, addend);
+return 0;
+}
+
+static int axgbe_timesync_adjust_time(struct rte_eth_dev *dev, int64_t
+delta) {
+struct axgbe_port *pdata = dev->data->dev_private;
+struct timespec timestamp_delta;
+
+axgbe_adjfreq(pdata, delta);
+pdata->systime_tc.nsec += delta;
+
+if (delta < 0) {
+delta = -delta;
+timestamp_delta = rte_ns_to_timespec(delta);
+axgbe_update_tstamp_time(pdata, timestamp_delta.tv_sec,
+timestamp_delta.tv_nsec, 1);
+} else {
+timestamp_delta = rte_ns_to_timespec(delta);
+axgbe_update_tstamp_time(pdata, timestamp_delta.tv_sec,
+timestamp_delta.tv_nsec, 0);
+}
+return 0;
+}
+
+static int axgbe_timesync_read_time(struct rte_eth_dev *dev,
+struct timespec *timestamp)
+{
+uint64_t nsec;
+struct axgbe_port *pdata = dev->data->dev_private;
+
+nsec = AXGMAC_IOREAD(pdata, MAC_STSR);
+nsec *= NSEC_PER_SEC;
+nsec += AXGMAC_IOREAD(pdata, MAC_STNR);
+*timestamp = rte_ns_to_timespec(nsec);
+return 0;
+}
+static int axgbe_timesync_write_time(struct rte_eth_dev *dev,
+    const struct timespec *timestamp) {
+unsigned int count = 100;
+struct axgbe_port *pdata = dev->data->dev_private;
+
+AXGMAC_IOWRITE(pdata, MAC_STSUR, timestamp->tv_sec);
+AXGMAC_IOWRITE(pdata, MAC_STNUR, timestamp->tv_nsec);
+AXGMAC_IOWRITE_BITS(pdata, MAC_TSCR, TSUPDT, 1);
+/* Wait for time update to complete */
+while (--count && AXGMAC_IOREAD_BITS(pdata, MAC_TSCR, TSUPDT))
+rte_delay_ms(1);
+if (!count)
+PMD_DRV_LOG(ERR, "Timed out update timestamp\n");
+return 0;
+}
+
+static void axgbe_update_tstamp_addend(struct axgbe_port *pdata,
+uint32_t addend)
+{
+unsigned int count = 100;
+
+AXGMAC_IOWRITE(pdata, MAC_TSAR, addend);
+AXGMAC_IOWRITE_BITS(pdata, MAC_TSCR, TSADDREG, 1);
+
+/* Wait for addend update to complete */
+while (--count && AXGMAC_IOREAD_BITS(pdata, MAC_TSCR, TSADDREG))
+rte_delay_ms(1);
+if (!count)
+PMD_DRV_LOG(ERR, "Timed out updating timestamp addend register\n"); }
+
+static void axgbe_set_tstamp_time(struct axgbe_port *pdata, unsigned int sec,
+unsigned int nsec)
+{
+unsigned int count = 100;
+
+/*System Time Sec Update*/
+AXGMAC_IOWRITE(pdata, MAC_STSUR, sec);
+/*System Time nanoSec Update*/
+AXGMAC_IOWRITE(pdata, MAC_STNUR, nsec);
+/*Initialize Timestamp*/
+AXGMAC_IOWRITE_BITS(pdata, MAC_TSCR, TSINIT, 1);
+
+/* Wait for time update to complete */
+while (--count && AXGMAC_IOREAD_BITS(pdata, MAC_TSCR, TSINIT))
+rte_delay_ms(1);
+if (!count)
+PMD_DRV_LOG(ERR, "Timed out initializing timestamp\n"); }
+
+static int
+axgbe_timesync_enable(struct rte_eth_dev *dev) {
+struct axgbe_port *pdata = dev->data->dev_private;
+unsigned int mac_tscr = 0;
+uint64_t dividend;
+struct timespec timestamp;
+uint64_t nsec;
+
+/* Set one nano-second accuracy */
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSCTRLSSR, 1);
+
+/* Set fine timestamp update */
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSCFUPDT, 1);
+
+/* Overwrite earlier timestamps */
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TXTSSTSM, 1);
+
+AXGMAC_IOWRITE(pdata, MAC_TSCR, mac_tscr);
+
+/* Enabling processing of ptp over eth pkt */
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSIPENA, 1);
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSVER2ENA, 1);
+/* Enable timestamp for all pkts*/
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSENALL, 1);
+
+/* enabling timestamp */
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSENA, 1);
+AXGMAC_IOWRITE(pdata, MAC_TSCR, mac_tscr);
+
+/* Exit if timestamping is not enabled */
+if (!AXGMAC_GET_BITS(mac_tscr, MAC_TSCR, TSENA)) {
+PMD_DRV_LOG(ERR, "Exiting as timestamp is not enabled\n");
+return 0;
+}
+
+/* Sub-second Increment Value*/
+AXGMAC_IOWRITE_BITS(pdata, MAC_SSIR, SSINC, AXGBE_TSTAMP_SSINC);
+/* Sub-nanosecond Increment Value */
+AXGMAC_IOWRITE_BITS(pdata, MAC_SSIR, SNSINC, AXGBE_TSTAMP_SNSINC);
+
+pdata->ptpclk_rate = AXGBE_V2_PTP_CLOCK_FREQ;
+dividend = 50000000;
+dividend <<= 32;
+pdata->tstamp_addend = div_u64(dividend, pdata->ptpclk_rate);
+
+axgbe_update_tstamp_addend(pdata, pdata->tstamp_addend);
+axgbe_set_tstamp_time(pdata, 0, 0);
+
+/* Initialize the timecounter */
+memset(&pdata->systime_tc, 0, sizeof(struct rte_timecounter));
+
+pdata->systime_tc.cc_mask = AXGBE_CYCLECOUNTER_MASK;
+pdata->systime_tc.cc_shift = 0;
+pdata->systime_tc.nsec_mask = 0;
+
+PMD_DRV_LOG(DEBUG, "Intializing system time counter with realtime\n");
+
+/* Updating the counter once with clock real time */
+clock_gettime(CLOCK_REALTIME, &timestamp);
+nsec = rte_timespec_to_ns(&timestamp);
+nsec = rte_timecounter_update(&pdata->systime_tc, nsec);
+axgbe_set_tstamp_time(pdata, timestamp.tv_sec, timestamp.tv_nsec);
+return 0;
+}
+
+static int
+axgbe_timesync_disable(struct rte_eth_dev *dev) {
+struct axgbe_port *pdata = dev->data->dev_private;
+unsigned int mac_tscr = 0;
+
+/*disable timestamp for all pkts*/
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSENALL, 0);
+/*disable the addened register*/
+AXGMAC_IOWRITE_BITS(pdata, MAC_TSCR, TSADDREG, 0);
+/* disable timestamp update */
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSCFUPDT, 0);
+/*disable time stamp*/
+AXGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSENA, 0);
+return 0;
+}
+
+static int
+axgbe_timesync_read_rx_timestamp(struct rte_eth_dev *dev,
+struct timespec *timestamp, uint32_t flags) {
+uint64_t nsec = 0;
+volatile union axgbe_rx_desc *desc;
+uint16_t idx, pmt;
+struct axgbe_rx_queue *rxq = *dev->data->rx_queues;
+
+idx = AXGBE_GET_DESC_IDX(rxq, rxq->cur);
+desc = &rxq->desc[idx];
+
+while (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3, OWN))
+rte_delay_ms(1);
+if (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3, CTXT)) {
+if (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_CONTEXT_DESC3, TSA)
+&& !AXGMAC_GET_BITS_LE(desc->write.desc3,
+RX_CONTEXT_DESC3, TSD)) {
+pmt = AXGMAC_GET_BITS_LE(desc->write.desc3,
+RX_CONTEXT_DESC3, PMT);
+nsec = rte_le_to_cpu_32(desc->write.desc1);
+nsec *= NSEC_PER_SEC;
+nsec += rte_le_to_cpu_32(desc->write.desc0);
+if (nsec != 0xffffffffffffffffULL) {
+if (pmt == 0x01)
+*timestamp = rte_ns_to_timespec(nsec);
+PMD_DRV_LOG(DEBUG, "flags = 0x%x nsec = %ld\n",
+flags, nsec);
+}
+}
+}
+
+return 0;
+}
+
+static int
+axgbe_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
+struct timespec *timestamp)
+{
+uint64_t nsec;
+struct axgbe_port *pdata = dev->data->dev_private;
+unsigned int tx_snr, tx_ssr;
+
+rte_delay_us(5);
+if (pdata->vdata->tx_tstamp_workaround) {
+tx_snr = AXGMAC_IOREAD(pdata, MAC_TXSNR);
+tx_ssr = AXGMAC_IOREAD(pdata, MAC_TXSSR);
+
+} else {
+tx_ssr = AXGMAC_IOREAD(pdata, MAC_TXSSR);
+tx_snr = AXGMAC_IOREAD(pdata, MAC_TXSNR);
+}
+if (AXGMAC_GET_BITS(tx_snr, MAC_TXSNR, TXTSSTSMIS)) {
+PMD_DRV_LOG(DEBUG, "Waiting for TXTSSTSMIS\n");
+return 0;
+}
+nsec = tx_ssr;
+nsec *= NSEC_PER_SEC;
+nsec += tx_snr;
+PMD_DRV_LOG(DEBUG, "nsec = %ld tx_ssr = %d tx_snr = %d\n",
+nsec, tx_ssr, tx_snr);
+*timestamp = rte_ns_to_timespec(nsec);
+return 0;
+}
+
 static void axgbe_get_all_hw_features(struct axgbe_port *pdata)  {
 unsigned int mac_hfr0, mac_hfr1, mac_hfr2; diff --git a/drivers/net/axgbe/axgbe_ethdev.h b/drivers/net/axgbe/axgbe_ethdev.h
index f10ec4a40..8a6bbbb22 100644
--- a/drivers/net/axgbe/axgbe_ethdev.h
+++ b/drivers/net/axgbe/axgbe_ethdev.h
@@ -9,6 +9,7 @@
 #include <rte_mempool.h>
 #include <rte_lcore.h>
 #include "axgbe_common.h"
+#include "rte_time.h"

 #define IRQ0xff
 #define VLAN_HLEN4
@@ -63,6 +64,13 @@
 #define AXGBE_V2_DMA_CLOCK_FREQ500000000
 #define AXGBE_V2_PTP_CLOCK_FREQ125000000

+/* Timestamp support - values based on 50MHz PTP clock
+ *   50MHz => 20 nsec
+ */
+#define AXGBE_TSTAMP_SSINC       20
+#define AXGBE_TSTAMP_SNSINC      0
+#define AXGBE_CYCLECOUNTER_MASK 0xffffffffffffffffULL
+
 #define AXGMAC_FIFO_MIN_ALLOC2048
 #define AXGMAC_FIFO_UNIT256
 #define AXGMAC_FIFO_ALIGN(_x)                            \
@@ -644,6 +652,12 @@ struct axgbe_port {
 unsigned int hash_table_count;
 unsigned int uc_hash_mac_addr;
 unsigned int uc_hash_table[AXGBE_MAC_HASH_TABLE_SIZE];
+
+/* For IEEE1588 PTP */
+struct rte_timecounter systime_tc;
+struct rte_timecounter tx_tstamp;
+unsigned int tstamp_addend;
+
 };

 void axgbe_init_function_ptrs_dev(struct axgbe_hw_if *hw_if); diff --git a/drivers/net/axgbe/axgbe_rxtx.c b/drivers/net/axgbe/axgbe_rxtx.c index 30c467db7..19195109b 100644
--- a/drivers/net/axgbe/axgbe_rxtx.c
+++ b/drivers/net/axgbe/axgbe_rxtx.c
@@ -275,6 +275,9 @@ axgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 /* Get the RSS hash */
 if (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3, RSV))
 mbuf->hash.rss = rte_le_to_cpu_32(desc->write.desc1);
+/* Indicate if a Context Descriptor is next */
+if (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3, CDA))
+mbuf->ol_flags |= PKT_RX_IEEE1588_PTP;
 pkt_len = AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3,
      PL) - rxq->crc_len;
 /* Mbuf populate */
@@ -487,6 +490,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 struct axgbe_tx_queue *txq;
 unsigned int tsize;
 const struct rte_memzone *tz;
+struct rte_eth_dev_data *dev_data;

 tx_desc = nb_desc;
 pdata = dev->data->dev_private;
@@ -507,6 +511,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 return -ENOMEM;
 txq->pdata = pdata;

+dev_data = pdata->eth_dev->data;
 txq->nb_desc = tx_desc;
 txq->free_thresh = tx_conf->tx_free_thresh ?
 tx_conf->tx_free_thresh : AXGBE_TX_FREE_THRESH; @@ -518,7 +523,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 if (txq->nb_desc % txq->free_thresh != 0)
 txq->vector_disable = 1;

-if (tx_conf->offloads != 0)
+if ((tx_conf->offloads != 0) || dev_data->dev_conf.txmode.offloads)
 txq->vector_disable = 1;

 /* Allocate TX ring hardware descriptors */ @@ -722,6 +727,10 @@ static int axgbe_xmit_hw(struct axgbe_tx_queue *txq,
 /* Total msg length to transmit */
 AXGMAC_SET_BITS_LE(desc->desc3, TX_NORMAL_DESC3, FL,
    mbuf->pkt_len);
+/* Timestamp enablement check */
+if (mbuf->ol_flags & PKT_TX_IEEE1588_TMST)
+AXGMAC_SET_BITS_LE(desc->desc2, TX_NORMAL_DESC2, TTSE, 1);
+rte_wmb();
 /* Mark it as First and Last Descriptor */
 AXGMAC_SET_BITS_LE(desc->desc3, TX_NORMAL_DESC3, FD, 1);
 AXGMAC_SET_BITS_LE(desc->desc3, TX_NORMAL_DESC3, LD, 1);
--
2.17.1
  

On 6/4/2020 5:46 AM, Somalapuram, Amaranath wrote:
> [AMD Public Use]
> 
> Acked-by: Somalapuram Amaranath <asomalap@amd.com>

Hi Somalapuram,

Can you please pay attention to following two things while replying,

1) Don't top post, post only bottom of the email.
This helps while reading from archives, the conversation flows from top to
bottom. If you mix top posting and bottom posting the archive becomes very hard
to understand and useless.

2) Snip the unrelated part from the email you are replying.
This makes your reply easier to read and focuses to your point.
Also this helps again to follow the issue from email archives as well as makes
easy to follow status of the patch from patchwork
(https://patches.dpdk.org/patch/70721/)

It may be easier for you just add the ack to the top and send but it makes life
harder for many others.

btw, is 'Somalapuram' your name or surname? According other contributors, amd
mail address convention seems as <name.surname@amd.com>, according that
'Somalapuram' is your surname. If so can you please update the tag to be in
following format:
"Acked-by: Name Surname <email@address.com>"


> 
> -----Original Message-----
> From: Sebastian, Selwin <Selwin.Sebastian@amd.com>
> Sent: Monday, June 1, 2020 6:28 PM
> To: dev@dpdk.org
> Cc: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>
> Subject: [PATCH v1] net/axgbe: enable IEEE 1588 PTP support for axgbe
> 
> From: Selwin Sebastian <selwin.sebastian@amd.com>
> 
> Add ethdev APIs to support PTP timestamping
> 
> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>

<...>
  

On 6/1/2020 1:57 PM, selwin.sebastian@amd.com wrote:
> From: Selwin Sebastian <selwin.sebastian@amd.com>
> 
> Add ethdev APIs to support PTP timestamping

For the patch title, "net/axgbe: " already says the change is in the 'axgbe'
driver, no need to duplicate " ..  support for axgbe".

<...>

> +static inline uint64_t
> +div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
> +{
> +	*remainder = dividend % divisor;
> +	return dividend / divisor;
> +}
> +
> +static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
> +{

The coding convention [1] we have says return type will be on seperate line, as
already done in some of these functions. Since this is new code, better to start
good, can you please apply the coding convention to all fucntions, like:

 static inline uint64_t
 div_u64(uint64_t dividend, uint32_t divisor)

[1]
https://doc.dpdk.org/guides/contributing/coding_style.html
(I definitly suggest reading it if you didn't already)

<...>

> @@ -487,6 +490,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>  	struct axgbe_tx_queue *txq;
>  	unsigned int tsize;
>  	const struct rte_memzone *tz;
> +	struct rte_eth_dev_data *dev_data;
>  
>  	tx_desc = nb_desc;
>  	pdata = dev->data->dev_private;
> @@ -507,6 +511,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>  		return -ENOMEM;
>  	txq->pdata = pdata;
>  
> +	dev_data = pdata->eth_dev->data;
>  	txq->nb_desc = tx_desc;
>  	txq->free_thresh = tx_conf->tx_free_thresh ?
>  		tx_conf->tx_free_thresh : AXGBE_TX_FREE_THRESH;
> @@ -518,7 +523,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>  	if (txq->nb_desc % txq->free_thresh != 0)
>  		txq->vector_disable = 1;
>  
> -	if (tx_conf->offloads != 0)
> +	if ((tx_conf->offloads != 0) || dev_data->dev_conf.txmode.offloads)
>  		txq->vector_disable = 1;


This change seems unrelated with the rest of the patch, and I far as I remember
this was in the another patch too. What do you think making this seperate patch
with the proper description it deserves?
  

[AMD Official Use Only - Internal Distribution Only]

Hi Ferruh,
	Added recommended modifications and resubmitted the patch.  Removed offloads handling part and "DEV_TX_OFFLOAD_MULTI_SEGS" Flag also as it is not yet supported by driver. 
 Commit 0625a29f42c62998318ee3e05b2420e436318678 forces the usage of  DEV_TX_OFFLOAD_MULTI_SEGS for using ptpclient test application. I had to remove this commit for my test.  Any inputs on how this can be handled ?
 
Regards
Selwin
 
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Friday, June 5, 2020 8:34 PM
To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
Cc: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>
Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: enable IEEE 1588 PTP support for axgbe

[CAUTION: External Email]

On 6/1/2020 1:57 PM, selwin.sebastian@amd.com wrote:
> From: Selwin Sebastian <selwin.sebastian@amd.com>
>
> Add ethdev APIs to support PTP timestamping

For the patch title, "net/axgbe: " already says the change is in the 'axgbe'
driver, no need to duplicate " ..  support for axgbe".

<...>

> +static inline uint64_t
> +div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder) 
> +{
> +     *remainder = dividend % divisor;
> +     return dividend / divisor;
> +}
> +
> +static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor) {

The coding convention [1] we have says return type will be on seperate line, as already done in some of these functions. Since this is new code, better to start good, can you please apply the coding convention to all fucntions, like:

 static inline uint64_t
 div_u64(uint64_t dividend, uint32_t divisor)

[1]
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fcontributing%2Fcoding_style.html&amp;data=02%7C01%7Cselwin.sebastian%40amd.com%7C66b9bd3bd4ca48fb8d5c08d80961a079%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637269662196393667&amp;sdata=XbS0AFCJAZ69RzcG23v0sOGNetZqEKQpxvGqAG%2B7Crw%3D&amp;reserved=0
(I definitly suggest reading it if you didn't already)

<...>

> @@ -487,6 +490,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>       struct axgbe_tx_queue *txq;
>       unsigned int tsize;
>       const struct rte_memzone *tz;
> +     struct rte_eth_dev_data *dev_data;
>
>       tx_desc = nb_desc;
>       pdata = dev->data->dev_private;
> @@ -507,6 +511,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>               return -ENOMEM;
>       txq->pdata = pdata;
>
> +     dev_data = pdata->eth_dev->data;
>       txq->nb_desc = tx_desc;
>       txq->free_thresh = tx_conf->tx_free_thresh ?
>               tx_conf->tx_free_thresh : AXGBE_TX_FREE_THRESH; @@ 
> -518,7 +523,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>       if (txq->nb_desc % txq->free_thresh != 0)
>               txq->vector_disable = 1;
>
> -     if (tx_conf->offloads != 0)
> +     if ((tx_conf->offloads != 0) || 
> + dev_data->dev_conf.txmode.offloads)
>               txq->vector_disable = 1;


This change seems unrelated with the rest of the patch, and I far as I remember this was in the another patch too. What do you think making this seperate patch with the proper description it deserves?
  

On 6/9/2020 4:42 PM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Ferruh,
> 	Added recommended modifications and resubmitted the patch.  Removed offloads handling part and "DEV_TX_OFFLOAD_MULTI_SEGS" Flag also as it is not yet supported by driver. 
>  Commit 0625a29f42c62998318ee3e05b2420e436318678 forces the usage of  DEV_TX_OFFLOAD_MULTI_SEGS for using ptpclient test application. I had to remove this commit for my test.  Any inputs on how this can be handled ?

Cc'ed Pablo.

According to the commit log "full Tx path" required for IEEE1588.

Is the DEV_TX_OFFLOAD_MULTI_SEGS requirement for IEEE1588, if so axgbe driver
needs to implement it before claiming the IEEE1588 support.

Or 'DEV_TX_OFFLOAD_MULTI_SEGS' may be used to force the underlying PMD to the
use the scalar data path. Pablo can answer this better.

>  
> Regards
> Selwin
>  
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com> 
> Sent: Friday, June 5, 2020 8:34 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Cc: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: enable IEEE 1588 PTP support for axgbe
> 
> [CAUTION: External Email]
> 
> On 6/1/2020 1:57 PM, selwin.sebastian@amd.com wrote:
>> From: Selwin Sebastian <selwin.sebastian@amd.com>
>>
>> Add ethdev APIs to support PTP timestamping
> 
> For the patch title, "net/axgbe: " already says the change is in the 'axgbe'
> driver, no need to duplicate " ..  support for axgbe".
> 
> <...>
> 
>> +static inline uint64_t
>> +div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder) 
>> +{
>> +     *remainder = dividend % divisor;
>> +     return dividend / divisor;
>> +}
>> +
>> +static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor) {
> 
> The coding convention [1] we have says return type will be on seperate line, as already done in some of these functions. Since this is new code, better to start good, can you please apply the coding convention to all fucntions, like:
> 
>  static inline uint64_t
>  div_u64(uint64_t dividend, uint32_t divisor)
> 
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fcontributing%2Fcoding_style.html&amp;data=02%7C01%7Cselwin.sebastian%40amd.com%7C66b9bd3bd4ca48fb8d5c08d80961a079%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637269662196393667&amp;sdata=XbS0AFCJAZ69RzcG23v0sOGNetZqEKQpxvGqAG%2B7Crw%3D&amp;reserved=0
> (I definitly suggest reading it if you didn't already)
> 
> <...>
> 
>> @@ -487,6 +490,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>       struct axgbe_tx_queue *txq;
>>       unsigned int tsize;
>>       const struct rte_memzone *tz;
>> +     struct rte_eth_dev_data *dev_data;
>>
>>       tx_desc = nb_desc;
>>       pdata = dev->data->dev_private;
>> @@ -507,6 +511,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>               return -ENOMEM;
>>       txq->pdata = pdata;
>>
>> +     dev_data = pdata->eth_dev->data;
>>       txq->nb_desc = tx_desc;
>>       txq->free_thresh = tx_conf->tx_free_thresh ?
>>               tx_conf->tx_free_thresh : AXGBE_TX_FREE_THRESH; @@ 
>> -518,7 +523,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>       if (txq->nb_desc % txq->free_thresh != 0)
>>               txq->vector_disable = 1;
>>
>> -     if (tx_conf->offloads != 0)
>> +     if ((tx_conf->offloads != 0) || 
>> + dev_data->dev_conf.txmode.offloads)
>>               txq->vector_disable = 1;
> 
> 
> This change seems unrelated with the rest of the patch, and I far as I remember this was in the another patch too. What do you think making this seperate patch with the proper description it deserves?
>
  

[AMD Official Use Only - Internal Distribution Only]

Hi Pablo,
	Can you please help ? 

Thanks and Regards
Selwin Sebastian
 
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Tuesday, June 9, 2020 9:34 PM
To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
Cc: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>
Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: enable IEEE 1588 PTP support for axgbe

[CAUTION: External Email]

On 6/9/2020 4:42 PM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Ferruh,
>       Added recommended modifications and resubmitted the patch.  Removed offloads handling part and "DEV_TX_OFFLOAD_MULTI_SEGS" Flag also as it is not yet supported by driver.
>  Commit 0625a29f42c62998318ee3e05b2420e436318678 forces the usage of  DEV_TX_OFFLOAD_MULTI_SEGS for using ptpclient test application. I had to remove this commit for my test.  Any inputs on how this can be handled ?

Cc'ed Pablo.

According to the commit log "full Tx path" required for IEEE1588.

Is the DEV_TX_OFFLOAD_MULTI_SEGS requirement for IEEE1588, if so axgbe driver needs to implement it before claiming the IEEE1588 support.

Or 'DEV_TX_OFFLOAD_MULTI_SEGS' may be used to force the underlying PMD to the use the scalar data path. Pablo can answer this better.

>
> Regards
> Selwin
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, June 5, 2020 8:34 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Cc: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: enable IEEE 1588 PTP 
> support for axgbe
>
> [CAUTION: External Email]
>
> On 6/1/2020 1:57 PM, selwin.sebastian@amd.com wrote:
>> From: Selwin Sebastian <selwin.sebastian@amd.com>
>>
>> Add ethdev APIs to support PTP timestamping
>
> For the patch title, "net/axgbe: " already says the change is in the 'axgbe'
> driver, no need to duplicate " ..  support for axgbe".
>
> <...>
>
>> +static inline uint64_t
>> +div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t 
>> +*remainder) {
>> +     *remainder = dividend % divisor;
>> +     return dividend / divisor;
>> +}
>> +
>> +static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor) 
>> +{
>
> The coding convention [1] we have says return type will be on seperate line, as already done in some of these functions. Since this is new code, better to start good, can you please apply the coding convention to all fucntions, like:
>
>  static inline uint64_t
>  div_u64(uint64_t dividend, uint32_t divisor)
>
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.
> dpdk.org%2Fguides%2Fcontributing%2Fcoding_style.html&amp;data=02%7C01%
> 7CSelwin.Sebastian%40amd.com%7Cf68e63a441694a33637c08d80c8ec76f%7C3dd8
> 961fe4884e608e11a82d994e183d%7C0%7C0%7C637273154657245807&amp;sdata=P6
> vU9bOzwKTBSJZFWQXEnnb6Wu%2FCViJ6kM1Cm2faeJU%3D&amp;reserved=0
> (I definitly suggest reading it if you didn't already)
>
> <...>
>
>> @@ -487,6 +490,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>       struct axgbe_tx_queue *txq;
>>       unsigned int tsize;
>>       const struct rte_memzone *tz;
>> +     struct rte_eth_dev_data *dev_data;
>>
>>       tx_desc = nb_desc;
>>       pdata = dev->data->dev_private; @@ -507,6 +511,7 @@ int 
>> axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>               return -ENOMEM;
>>       txq->pdata = pdata;
>>
>> +     dev_data = pdata->eth_dev->data;
>>       txq->nb_desc = tx_desc;
>>       txq->free_thresh = tx_conf->tx_free_thresh ?
>>               tx_conf->tx_free_thresh : AXGBE_TX_FREE_THRESH; @@
>> -518,7 +523,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>       if (txq->nb_desc % txq->free_thresh != 0)
>>               txq->vector_disable = 1;
>>
>> -     if (tx_conf->offloads != 0)
>> +     if ((tx_conf->offloads != 0) ||
>> + dev_data->dev_conf.txmode.offloads)
>>               txq->vector_disable = 1;
>
>
> This change seems unrelated with the rest of the patch, and I far as I remember this was in the another patch too. What do you think making this seperate patch with the proper description it deserves?
>