All of lore.kernel.org
 help / color / mirror / Atom feed
* Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface
@ 2022-04-20 14:03 Lasse Johnsen
  2022-04-20 19:19 ` Andrew Lunn
  2022-04-21 14:48 ` Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Richard Cochran
  0 siblings, 2 replies; 15+ messages in thread
From: Lasse Johnsen @ 2022-04-20 14:03 UTC (permalink / raw)
  To: netdev; +Cc: richardcochran, Gordon Hollingworth, Ahmad Byagowi

[-- Attachment #1: Type: text/plain, Size: 2117 bytes --]

Hello,


The attached set of patches adds support for the IEEE1588 functionality on the BCM54210PE PHY using the Linux Kernel mii_timestamper interface. The BCM54210PE PHY can be found in the Raspberry PI Compute Module 4 and the work has been undertaken by Timebeat.app on behalf of Raspberry PI with help and support from the nice engineers at Broadcom.


Design:

After the ethernet frames are identified using the ptp_classify_raw function, they are parsed through the mii_timestamper interface to the bcm54210pe_rxtstamp and bcm54210pe_txtstamp functions via the skb_defer_rx_timestamp and skb_clone_tx_timestamp kernel functions. 

In both cases these functions enqueue the sk_buff ptrs and schedules a work struct thread that attaches timestamps and forward the skbs upstream via the netif_rx_ni and skb_complete_tx_timestamp in the bcm54210pe_run_rx_timestamp_match_thread and bcm54210pe_run_tx_timestamp_match_thread functions in a non-interrupt context. 

The driver uses poll style behaviour triggered by the rx or tx of frames, but does not use a formal interrupt handler.

In addition to gettime, settime and adjtime and adjfine, I've implemented the gettimex64 function to provide the best possible sync of the kernel clock from the PHC. However, as the PHY is separated from the MAC by the MDIO bus, I cannot lock and prevent the scheduler from interrupting the 3-timestamp process, but performance is nonetheless reasonable and the kernel clock sees offset variations in the 1-2us range.


Features:

In addition the driver add support for perout and extts functionality using ptp_clock_request structs as provided for by the standard SO_TIMESTAMPING API.


Test & Performance:

We have tested the features and accuracy in our lab and are able as a client to run at -7 intervals without significant performance impact on a Raspberry PI CM4 on an IO board. We are able to maintain synchronisation of the PHC within +/-10ns of its grandmaster PTP source and the system clock within 1-2us of the PHC.


I look forward to receiving your feedback.


All the best,

Lasse


[-- Attachment #2: bcm54210pe-1588.patch.txt --]
[-- Type: text/plain, Size: 51963 bytes --]

From bf2ec31ad461f2213e4f3850649e1d00953cf86d Mon Sep 17 00:00:00 2001
From: Lasse Johnsen <l@ssejohnsen.me>
Date: Sat, 5 Feb 2022 09:34:19 -0500
Subject: [PATCH] Added support for IEEE1588 timestamping for the BCM54210PE
 PHY using the kernel mii_timestamper interface

---
 arch/arm/configs/bcm2711_defconfig            |    1 +
 arch/arm64/configs/bcm2711_defconfig          |    1 +
 .../net/ethernet/broadcom/genet/bcmgenet.c    |   16 +-
 drivers/net/phy/Makefile                      |    1 +
 drivers/net/phy/bcm54210pe_ptp.c              | 1406 +++++++++++++++++
 drivers/net/phy/bcm54210pe_ptp.h              |  111 ++
 drivers/net/phy/broadcom.c                    |   21 +-
 drivers/ptp/Kconfig                           |   17 +
 8 files changed, 1572 insertions(+), 2 deletions(-)
 create mode 100755 drivers/net/phy/bcm54210pe_ptp.c
 create mode 100755 drivers/net/phy/bcm54210pe_ptp.h

diff --git a/arch/arm/configs/bcm2711_defconfig b/arch/arm/configs/bcm2711_defconfig
index 8f4ae82cade4f..c7f3cce0024b7 100644
--- a/arch/arm/configs/bcm2711_defconfig
+++ b/arch/arm/configs/bcm2711_defconfig
@@ -1402,6 +1402,7 @@ CONFIG_MAXIM_THERMOCOUPLE=m
 CONFIG_MAX31856=m
 CONFIG_PWM_BCM2835=m
 CONFIG_PWM_PCA9685=m
+CONFIG_GENERIC_PHY=y
 CONFIG_RPI_AXIPERF=m
 CONFIG_NVMEM_RMEM=m
 CONFIG_EXT4_FS=y
diff --git a/arch/arm64/configs/bcm2711_defconfig b/arch/arm64/configs/bcm2711_defconfig
index 75333e69ef741..2af6b2fc5dcda 100644
--- a/arch/arm64/configs/bcm2711_defconfig
+++ b/arch/arm64/configs/bcm2711_defconfig
@@ -1409,6 +1409,7 @@ CONFIG_MAXIM_THERMOCOUPLE=m
 CONFIG_MAX31856=m
 CONFIG_PWM_BCM2835=m
 CONFIG_PWM_PCA9685=m
+CONFIG_GENERIC_PHY=y
 CONFIG_RPI_AXIPERF=m
 CONFIG_ANDROID=y
 CONFIG_ANDROID_BINDER_IPC=y
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index bd1f419bc47ae..2fa6258103025 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -39,8 +39,11 @@
 
 #include <asm/unaligned.h>
 
+#include <linux/ptp_classify.h>
+
 #include "bcmgenet.h"
 
+
 /* Maximum number of hardware queues, downsized if needed */
 #define GENET_MAX_MQ_CNT	4
 
@@ -2096,7 +2099,18 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	GENET_CB(skb)->last_cb = tx_cb_ptr;
-	skb_tx_timestamp(skb);
+
+	// Timestamping
+	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
+	{
+		//skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		skb_pull(skb, skb_mac_offset(skb)); // Feels like this pull should really be part of ptp_classify_raw...
+		skb_clone_tx_timestamp(skb);
+	}
+	else if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP))
+	{
+		skb_tstamp_tx(skb, NULL);
+	}
 
 	/* Decrement total BD count and advance our write pointer */
 	ring->free_bds -= nr_frags + 1;
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf8..528192d59d793 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_BCM84881_PHY)	+= bcm84881.o
 obj-$(CONFIG_BCM87XX_PHY)	+= bcm87xx.o
 obj-$(CONFIG_BCM_CYGNUS_PHY)	+= bcm-cygnus.o
 obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o
+obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o
 obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
 obj-$(CONFIG_CICADA_PHY)	+= cicada.o
 obj-$(CONFIG_CORTINA_PHY)	+= cortina.o
diff --git a/drivers/net/phy/bcm54210pe_ptp.c b/drivers/net/phy/bcm54210pe_ptp.c
new file mode 100755
index 0000000000000..c4882c84229f9
--- /dev/null
+++ b/drivers/net/phy/bcm54210pe_ptp.c
@@ -0,0 +1,1406 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  drivers/net/phy/bcm54210pe_ptp.c
+ *
+ * IEEE1588 (PTP), perout and extts for BCM54210PE PHY
+ *
+ * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
+ * License: GPL
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/ip.h>                                                                                
+#include <linux/net_tstamp.h>
+#include <linux/mii.h>
+#include <linux/phy.h>                                                                               
+#include <linux/ptp_classify.h>
+#include <linux/ptp_clock_kernel.h>                                                                  
+#include <linux/udp.h>
+#include <asm/unaligned.h> 
+#include <linux/brcmphy.h>
+#include <linux/irq.h>
+#include <linux/workqueue.h>
+#include <linux/gpio.h>
+#include <linux/if_ether.h>
+#include <linux/delay.h>
+#include <linux/sched.h>
+
+#include "bcm54210pe_ptp.h"
+#include "bcm-phy-lib.h"
+
+#define PTP_CONTROL_OFFSET		32
+#define PTP_TSMT_OFFSET 		0
+#define PTP_SEQUENCE_ID_OFFSET		30
+#define PTP_CLOCK_ID_OFFSET		20
+#define PTP_CLOCK_ID_SIZE		8
+#define PTP_SEQUENCE_PORT_NUMER_OFFSET  (PTP_CLOCK_ID_OFFSET + PTP_CLOCK_ID_SIZE)
+
+#define EXT_SELECT_REG			0x17
+#define EXT_DATA_REG			0x15
+
+#define EXT_ENABLE_REG1			0x17
+#define EXT_ENABLE_DATA1		0x0F7E
+#define EXT_ENABLE_REG2			0x15
+#define EXT_ENABLE_DATA2		0x0000
+
+#define EXT_1588_SLICE_REG		0x0810
+#define EXT_1588_SLICE_DATA		0x0101
+
+#define ORIGINAL_TIME_CODE_0 		0x0854
+#define ORIGINAL_TIME_CODE_1 		0x0855
+#define ORIGINAL_TIME_CODE_2 		0x0856
+#define ORIGINAL_TIME_CODE_3 		0x0857
+#define ORIGINAL_TIME_CODE_4 		0x0858
+
+#define TIME_STAMP_REG_0		0x0889
+#define TIME_STAMP_REG_1		0x088A
+#define TIME_STAMP_REG_2		0x088B
+#define TIME_STAMP_REG_3		0x08C4
+#define TIME_STAMP_INFO_1		0x088C
+#define TIME_STAMP_INFO_2		0x088D
+#define INTERRUPT_STATUS_REG		0x085F
+#define INTERRUPT_MASK_REG		0x085E
+#define EXT_SOFTWARE_RESET		0x0F70
+#define EXT_RESET1			0x0001 //RESET
+#define EXT_RESET2			0x0000 //NORMAL OPERATION
+#define GLOBAL_TIMESYNC_REG		0x0FF5
+
+#define TX_EVENT_MODE_REG		0x0811
+#define RX_EVENT_MODE_REG		0x0819
+#define TX_TSCAPTURE_ENABLE_REG		0x0821
+#define RX_TSCAPTURE_ENABLE_REG		0x0822
+#define TXRX_1588_OPTION_REG		0x0823
+
+#define TX_TS_OFFSET_LSB		0x0834
+#define TX_TS_OFFSET_MSB		0x0835
+#define RX_TS_OFFSET_LSB		0x0844
+#define RX_TS_OFFSET_MSB		0x0845
+#define NSE_DPPL_NCO_1_LSB_REG		0x0873
+#define NSE_DPPL_NCO_1_MSB_REG		0x0874
+
+#define NSE_DPPL_NCO_2_0_REG		0x0875
+#define NSE_DPPL_NCO_2_1_REG		0x0876
+#define NSE_DPPL_NCO_2_2_REG		0x0877
+
+#define NSE_DPPL_NCO_3_0_REG		0x0878
+#define NSE_DPPL_NCO_3_1_REG		0x0879
+#define NSE_DPPL_NCO_3_2_REG		0x087A
+
+#define NSE_DPPL_NCO_4_REG		0x087B
+
+#define NSE_DPPL_NCO_5_0_REG		0x087C
+#define NSE_DPPL_NCO_5_1_REG		0x087D
+#define NSE_DPPL_NCO_5_2_REG		0x087E
+
+#define NSE_DPPL_NCO_6_REG		0x087F
+
+#define NSE_DPPL_NCO_7_0_REG		0x0880
+#define NSE_DPPL_NCO_7_1_REG		0x0881
+
+#define DPLL_SELECT_REG			0x085b
+#define TIMECODE_SEL_REG		0x08C3
+#define SHADOW_REG_CONTROL		0x085C
+#define SHADOW_REG_LOAD			0x085D
+
+#define PTP_INTERRUPT_REG		0x0D0C
+
+#define CTR_DBG_REG			0x088E
+#define HEART_BEAT_REG4			0x08ED
+#define HEART_BEAT_REG3			0x08EC
+#define HEART_BEAT_REG2			0x0888
+#define	HEART_BEAT_REG1			0x0887
+#define	HEART_BEAT_REG0			0x0886
+	
+#define READ_END_REG			0x0885
+
+static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp)
+{
+	struct bcm54210pe_circular_buffer_item *item; 
+	struct list_head *this, *next;
+
+	u8 index = (txrx * 4) + message_type;
+
+	if(index >= CIRCULAR_BUFFER_COUNT)
+	{
+		return false;
+	}
+
+	list_for_each_safe(this, next, &private->circular_buffers[index])
+	{
+		item = list_entry(this, struct bcm54210pe_circular_buffer_item, list);
+
+		if(item->sequence_id == seq_id && item->is_valid)
+		{
+			item->is_valid = false;
+			*timestamp = item->time_stamp;
+			mutex_unlock(&private->timestamp_buffer_lock);
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private)
+{
+	struct phy_device *phydev = private->phydev;
+	struct bcm54210pe_circular_buffer_item *item;
+	u16 fifo_info_1, fifo_info_2;
+	u8 tx_or_rx, msg_type, index;
+	u16 sequence_id;
+	u64 timestamp;
+	u16 Time[4];
+
+	mutex_lock(&private->timestamp_buffer_lock);
+
+	while(bcm_phy_read_exp(phydev, INTERRUPT_STATUS_REG) & 2)
+	{
+		mutex_lock(&private->clock_lock);
+
+		// Flush out the FIFO
+		bcm_phy_write_exp(phydev, READ_END_REG, 1);
+
+		Time[3] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_3);
+		Time[2] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_2);
+		Time[1] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_1);
+		Time[0] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_0);
+
+		fifo_info_1 = bcm_phy_read_exp(phydev, TIME_STAMP_INFO_1);
+		fifo_info_2 = bcm_phy_read_exp(phydev, TIME_STAMP_INFO_2);
+
+		bcm_phy_write_exp(phydev, READ_END_REG, 2);
+		bcm_phy_write_exp(phydev, READ_END_REG, 0);
+
+		mutex_unlock(&private->clock_lock);
+
+		msg_type = (u8) ((fifo_info_2 & 0xF000) >> 12);
+		tx_or_rx = (u8) ((fifo_info_2 & 0x0800) >> 11); // 1 = TX, 0 = RX
+		sequence_id = fifo_info_1;
+
+		timestamp = four_u16_to_ns(Time);
+
+		index = (tx_or_rx * 4) + msg_type;
+
+		if(index < CIRCULAR_BUFFER_COUNT)
+		{
+			item = list_first_entry_or_null(&private->circular_buffers[index], struct bcm54210pe_circular_buffer_item, list);
+		}
+
+		if(item == NULL) {
+			continue;
+		}
+
+		list_del_init(&item->list);
+
+		item->msg_type = msg_type;
+		item->sequence_id = sequence_id;
+		item->time_stamp = timestamp;
+		item->is_valid = true;
+
+		list_add_tail(&item->list, &private->circular_buffers[index]);
+	}
+
+	mutex_unlock(&private->timestamp_buffer_lock);
+}
+
+static void bcm54210pe_run_rx_timestamp_match_thread(struct work_struct *w)
+{
+	struct bcm54210pe_private *private =
+		container_of(w, struct bcm54210pe_private, rxts_work);
+
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct ptp_header *hdr;
+	struct sk_buff *skb;
+
+	u8 msg_type;
+	u16 sequence_id;
+	u64 timestamp;
+	int x, type;
+
+	skb = skb_dequeue(&private->rx_skb_queue);
+
+	while (skb != NULL) {
+
+		// Yes....  skb_defer_rx_timestamp just did this but <ZZZzzz>....
+		skb_push(skb, ETH_HLEN);
+		type = ptp_classify_raw(skb);
+		skb_pull(skb, ETH_HLEN);
+
+		hdr = ptp_parse_header(skb, type);
+
+		if (hdr == NULL) {
+			goto dequeue;
+		}
+
+		msg_type = ptp_get_msgtype(hdr, type);
+		sequence_id = be16_to_cpu(hdr->sequence_id);
+
+		timestamp = 0;
+
+		for (x = 0; x < 10; x++) {
+			bcm54210pe_read_sop_time_register(private);
+			if (bcm54210pe_fetch_timestamp(0, msg_type, sequence_id,
+						       private, &timestamp)) {
+				break;
+			}
+
+			udelay(private->fib_sequence[x] *
+			       private->fib_factor_rx);
+		}
+
+		shhwtstamps = skb_hwtstamps(skb);
+
+		if (!shhwtstamps) {
+			goto dequeue;
+		}
+
+		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+		shhwtstamps->hwtstamp = ns_to_ktime(timestamp);
+
+	dequeue:
+		netif_rx_ni(skb);
+		skb = skb_dequeue(&private->rx_skb_queue);
+	}
+}
+
+static void bcm54210pe_run_tx_timestamp_match_thread(struct work_struct *w)
+{
+	struct bcm54210pe_private *private =
+		container_of(w, struct bcm54210pe_private, txts_work);
+
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct sk_buff *skb;
+
+	struct ptp_header *hdr;
+	u8 msg_type;
+	u16 sequence_id;
+	u64 timestamp;
+	int x;
+
+	timestamp = 0;
+	skb = skb_dequeue(&private->tx_skb_queue);
+
+	while (skb != NULL) {
+		int type = ptp_classify_raw(skb);
+		hdr = ptp_parse_header(skb, type);
+
+		if (!hdr) {
+			return;
+		}
+
+		msg_type = ptp_get_msgtype(hdr, type);
+		sequence_id = be16_to_cpu(hdr->sequence_id);
+
+		for (x = 0; x < 10; x++) {
+			bcm54210pe_read_sop_time_register(private);
+			if (bcm54210pe_fetch_timestamp(1, msg_type, sequence_id,
+						       private, &timestamp)) {
+				break;
+			}
+			udelay(private->fib_sequence[x] * private->fib_factor_tx);
+		}
+		shhwtstamps = skb_hwtstamps(skb);
+
+		if (!shhwtstamps) {
+			kfree_skb(skb);
+			goto dequeue;
+		}
+
+		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+		shhwtstamps->hwtstamp = ns_to_ktime(timestamp);
+
+		skb_complete_tx_timestamp(skb, shhwtstamps);
+
+	dequeue:
+		skb = skb_dequeue(&private->tx_skb_queue);
+	}
+}
+
+static int bcm54210pe_config_1588(struct phy_device *phydev)
+{
+	int err;
+
+	err = bcm_phy_write_exp(phydev, PTP_INTERRUPT_REG, 0x3c02 );
+
+	err |=  bcm_phy_write_exp(phydev, GLOBAL_TIMESYNC_REG, 0x0001); //Enable global timesync register.
+	err |=  bcm_phy_write_exp(phydev, EXT_1588_SLICE_REG, 0x0101); //ENABLE TX and RX slice 1588
+	err |=  bcm_phy_write_exp(phydev, TX_EVENT_MODE_REG, 0xFF00); //Add 80bit timestamp + NO CPU MODE in TX
+	err |=  bcm_phy_write_exp(phydev, RX_EVENT_MODE_REG, 0xFF00); //Add 32+32 bits timestamp + NO CPU mode in RX
+	err |=  bcm_phy_write_exp(phydev, TIMECODE_SEL_REG, 0x0101); //Select 80 bit counter
+	err |=  bcm_phy_write_exp(phydev, TX_TSCAPTURE_ENABLE_REG, 0x0001); //Enable timestamp capture in TX
+	err |=  bcm_phy_write_exp(phydev, RX_TSCAPTURE_ENABLE_REG, 0x0001); //Enable timestamp capture in RX
+
+	//Enable shadow register
+	err |= bcm_phy_write_exp(phydev, SHADOW_REG_CONTROL, 0x0000);
+	err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x07c0);
+
+
+	// Set global mode and trigger immediate framesync to load shaddow registers
+	err |=  bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xC020);
+
+	//15, 33 or 41 - experimental
+	printk("DEBUG: GPIO %d IRQ %d\n", 15, gpio_to_irq(41));
+
+	// Enable Interrupt behaviour (eventhough we get no interrupts)
+	err |= bcm54210pe_interrupts_enable(phydev,true, false);
+	
+	return err; 
+}
+
+static int bcm54210pe_gettimex(struct ptp_clock_info *info,
+			       struct timespec64 *ts,
+			       struct ptp_system_timestamp *sts)
+{
+
+	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
+	return bcm54210pe_get80bittime(ptp->chosen, ts, sts);
+}
+
+// Must be called under clock_lock
+static void bcm54210pe_read80bittime_register(struct phy_device *phydev, u64 *time_stamp_80, u64 *time_stamp_48)
+{
+	u16 Time[5];
+	u64 ts[3];
+	u64 cumulative;
+
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x400);
+	Time[4] = bcm_phy_read_exp(phydev, HEART_BEAT_REG4);
+	Time[3] = bcm_phy_read_exp(phydev, HEART_BEAT_REG3);
+	Time[2] = bcm_phy_read_exp(phydev, HEART_BEAT_REG2);
+	Time[1] = bcm_phy_read_exp(phydev, HEART_BEAT_REG1);
+	Time[0] = bcm_phy_read_exp(phydev, HEART_BEAT_REG0);
+
+	// Set read end bit
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x800);
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x000);
+
+	*time_stamp_80 = four_u16_to_ns(Time);
+
+	if (time_stamp_48 != NULL) {
+
+
+		ts[2] = (((u64)Time[2]) << 32);
+		ts[1] = (((u64)Time[1]) << 16);
+		ts[0] = ((u64)Time[0]);
+
+		cumulative = 0;
+		cumulative |= ts[0];
+		cumulative |= ts[1];
+		cumulative |= ts[2];
+
+		*time_stamp_48 = cumulative;
+	}
+}
+
+// Must be called under clock_lock
+static void bcm54210pe_read48bittime_register(struct phy_device *phydev, u64 *time_stamp)
+{
+	u16 Time[3];
+	u64 ts[3];
+	u64 cumulative;
+
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x400);
+	Time[2] = bcm_phy_read_exp(phydev, HEART_BEAT_REG2);
+	Time[1] = bcm_phy_read_exp(phydev, HEART_BEAT_REG1);
+	Time[0] = bcm_phy_read_exp(phydev, HEART_BEAT_REG0);
+
+	// Set read end bit
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x800);
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x000);
+
+
+	ts[2] = (((u64)Time[2]) << 32);
+	ts[1] = (((u64)Time[1]) << 16);
+	ts[0] = ((u64)Time[0]);
+
+	cumulative = 0;
+	cumulative |= ts[0];
+	cumulative |= ts[1];
+	cumulative |= ts[2];
+
+	*time_stamp = cumulative;
+}
+
+static int bcm54210pe_get80bittime(struct bcm54210pe_private *private,
+				   struct timespec64 *ts,
+				   struct ptp_system_timestamp *sts)
+{
+	struct phy_device *phydev;
+	u16 nco_6_register_value;
+	int i;
+	u64 time_stamp_48, time_stamp_80, control_ts;
+
+	phydev = private->phydev;
+
+	// Capture timestamp on next framesync
+	nco_6_register_value = 0x2000;
+
+	// Lock
+	mutex_lock(&private->clock_lock);
+
+	// We share frame sync events with extts, so we need to ensure no event has occurred as we are about to boot the registers, so....
+
+	// If extts is enabled
+	if (private->extts_en) {
+
+		// Halt framesyncs generated by the sync in pin
+		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0000);
+
+		// Read what's in the 8- bit register
+		bcm54210pe_read48bittime_register(phydev, &control_ts);
+
+		// If it matches neither the last gettime or extts timestamp
+		if (control_ts != private->last_extts_ts && control_ts != private->last_immediate_ts[0]) { // FIXME: This is a bug
+
+			// Odds are this is a extts not yet logged as an event
+			//printk("extts triggered by get80bittime\n");
+			bcm54210pe_trigger_extts_event(private, control_ts);
+		}
+	}
+
+	// Heartbeat register selection. Latch 80 bit Original time coude counter into Heartbeat register
+	// (this is undocumented)
+	bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0040);
+
+	// Amend to base register
+	nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+	// Set the NCO register
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+	// Trigger framesync
+	if (sts != NULL) {
+
+		// If we are doing a gettimex call
+		ptp_read_system_prets(sts);
+		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+		ptp_read_system_postts(sts);
+
+	} else {
+
+		// or if we are doing a gettime call
+		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+	}
+
+	for (i = 0; i < 5;i++) {
+
+		bcm54210pe_read80bittime_register(phydev, &time_stamp_80, &time_stamp_48);
+
+		if (time_stamp_80 != 0) {
+			break;
+		}
+	}
+
+	// Convert to timespec64
+	ns_to_ts(time_stamp_80, ts);
+
+	// If we are using extts
+	if(private->extts_en) {
+
+		// Commit last timestamp
+	   	private->last_immediate_ts[0] = time_stamp_48;
+	    	private->last_immediate_ts[1] = time_stamp_80;
+
+		// Heartbeat register selection. Latch 48 bit Original time coude counter into Heartbeat register
+		// (this is undocumented)
+		bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0000);
+
+		// Rearm framesync for sync in pin
+		nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+	}
+
+	mutex_unlock(&private->clock_lock);
+
+	return 0;
+}
+
+static int bcm54210pe_gettime(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+	int err;
+	err = bcm54210pe_gettimex(info, ts, NULL);
+	return err;
+}
+
+static int bcm54210pe_get48bittime(struct bcm54210pe_private *private, u64 *timestamp)
+{
+	u16 nco_6_register_value;
+	int i, err;
+
+	struct phy_device *phydev = private->phydev;
+
+	// Capture timestamp on next framesync
+	nco_6_register_value = 0x2000;
+
+	mutex_lock(&private->clock_lock);
+
+	// Heartbeat register selection. Latch 48 bit Original time coude counter into Heartbeat register
+	// (this is undocumented)
+	err = bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0000);
+
+	// Amend to base register
+	nco_6_register_value =
+		bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+	// Set the NCO register
+	err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+	// Trigger framesync
+	err |= bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	for (i = 0; i < 5; i++) {
+
+		bcm54210pe_read48bittime_register(phydev,timestamp);
+
+		if (*timestamp != 0) {
+			break;
+		}
+	}
+	mutex_unlock(&private->clock_lock);
+
+	return err;
+}
+
+static int bcm54210pe_settime(struct ptp_clock_info *info, const struct timespec64 *ts)
+{
+	u16 shadow_load_register, nco_6_register_value;
+	u16 original_time_codes[5], local_time_codes[3];
+	struct bcm54210pe_ptp *ptp;
+	struct phy_device *phydev;
+
+	ptp = container_of(info, struct bcm54210pe_ptp, caps);
+	phydev = ptp->chosen->phydev;
+
+	shadow_load_register = 0;
+	nco_6_register_value = 0;
+
+	// Assign original time codes (80 bit)
+	original_time_codes[4] = (u16) ((ts->tv_sec & 0x0000FFFF00000000) >> 32);
+	original_time_codes[3] = (u16) ((ts->tv_sec  & 0x00000000FFFF0000) >> 16);
+	original_time_codes[2] = (u16) (ts->tv_sec  & 0x000000000000FFFF);
+	original_time_codes[1] = (u16) ((ts->tv_nsec & 0x00000000FFFF0000) >> 16);
+	original_time_codes[0] = (u16) (ts->tv_nsec & 0x000000000000FFFF);
+
+	// Assign original time codes (48 bit)
+	local_time_codes[2] = 0x4000;
+	local_time_codes[1] = (u16) (ts->tv_nsec >> 20);
+	local_time_codes[0] = (u16) (ts->tv_nsec >> 4);
+
+	// Set Time Code load bit in the shadow load register
+	shadow_load_register |= 0x0400;
+
+	// Set Local Time load bit in the shadow load register
+	shadow_load_register |= 0x0080;
+
+	mutex_lock(&ptp->chosen->clock_lock);
+
+	// Write Original Time Code Register
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_0, original_time_codes[0]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_1, original_time_codes[1]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_2, original_time_codes[2]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_3, original_time_codes[3]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_4, original_time_codes[4]);
+
+	// Write Local Time Code Register
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_0_REG, local_time_codes[0]);
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_1_REG, local_time_codes[1]);
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_2_REG, local_time_codes[2]);
+
+	// Write Shadow register
+	bcm_phy_write_exp(phydev, SHADOW_REG_CONTROL, 0x0000);
+	bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, shadow_load_register);
+
+	// Set global mode and nse_init
+	nco_6_register_value = bcm54210pe_get_base_nco6_reg(ptp->chosen, nco_6_register_value, true);
+
+	// Write to register
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+	// Trigger framesync
+	bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	// Set the second on set
+	ptp->chosen->second_on_set = ts->tv_sec;
+
+	mutex_unlock(&ptp->chosen->clock_lock);
+
+	return 0;
+}
+
+static int bcm54210pe_adjfine(struct ptp_clock_info *info, long scaled_ppm)
+{	
+	int err;
+	u16 lo, hi;
+	u32 corrected_8ns_interval, base_8ns_interval;
+	bool negative;
+
+	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
+	struct phy_device *phydev = ptp->chosen->phydev;
+
+	negative = false;
+        if ( scaled_ppm < 0 ) {
+		negative = true;
+		scaled_ppm = -scaled_ppm;
+	}
+
+	// This is not completely accurate but very fast
+	scaled_ppm >>= 7;
+
+	// Nominal counter increment is 8ns
+	base_8ns_interval = 1 << 31;
+
+	// Add or subtract differential
+	if (negative) {
+		corrected_8ns_interval = base_8ns_interval - scaled_ppm;
+	} else {
+		corrected_8ns_interval = base_8ns_interval + scaled_ppm;
+	}
+
+	// Load up registers
+	hi = (corrected_8ns_interval & 0xFFFF0000) >> 16;
+	lo = (corrected_8ns_interval & 0x0000FFFF);
+
+	mutex_lock(&ptp->chosen->clock_lock);
+
+	// Set freq_mdio_sel to 1
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_2_REG, 0x4000);
+
+	// Load 125MHz frequency reqcntrl
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_1_MSB_REG, hi);
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_1_LSB_REG, lo);
+
+	// On next framesync load freq from freqcntrl
+	bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x0040);
+
+	// Trigger framesync
+	err = bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	mutex_unlock(&ptp->chosen->clock_lock);
+
+	return err;
+}
+
+static int bcm54210pe_adjtime(struct ptp_clock_info *info, s64 delta)
+{
+	int err;
+	struct timespec64 ts;
+	u64 now;
+
+	err = bcm54210pe_gettime(info, &ts);
+	if (err < 0)
+		return err;
+
+	now = ktime_to_ns(timespec64_to_ktime(ts));
+	ts = ns_to_timespec64(now + delta);
+
+	err = bcm54210pe_settime(info, &ts);
+
+	return err;
+}
+
+
+static int bcm54210pe_extts_enable(struct bcm54210pe_private *private, int enable)
+{
+	int err;
+	struct phy_device *phydev;
+	u16 nco_6_register_value;
+
+	phydev = private->phydev;
+
+	if (enable) {
+
+		if (!private->extts_en) {
+
+			// Set enable per_out
+			private->extts_en = true;
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_4_REG, 0x0001);
+
+			nco_6_register_value = 0;
+			nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_7_0_REG, 0x0100);
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_7_1_REG, 0x0200);
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+			schedule_delayed_work(&private->extts_ws, msecs_to_jiffies(1));
+		}
+
+	} else {
+		private->extts_en = false;
+		err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_4_REG, 0x0000);
+
+	}
+
+	return err;
+}
+
+static void bcm54210pe_run_extts_thread(struct work_struct *extts_ws)
+{
+	struct bcm54210pe_private *private;
+	struct phy_device *phydev;
+	u64 interval, time_stamp_48, time_stamp_80;
+
+	private = container_of((struct delayed_work *)extts_ws, struct bcm54210pe_private, extts_ws);
+	phydev = private->phydev;
+
+	interval = 10;	// in ms - long after we are gone from this earth, discussions will be had and
+	  		// songs will be sung about whether this interval is short enough....
+			// Before you complain let me say that in Timebeat.app up to ~150ms allows
+			// single digit ns servo accuracy. If your client / servo is not as cool: Do better :-)
+
+	mutex_lock(&private->clock_lock);
+
+	bcm54210pe_read80bittime_register(phydev, &time_stamp_80, &time_stamp_48);
+
+
+	if (private->last_extts_ts != time_stamp_48 &&
+	    private->last_immediate_ts[0] != time_stamp_48 &&
+	    private->last_immediate_ts[1] != time_stamp_80) {
+
+		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xE000);
+		bcm54210pe_trigger_extts_event(private, time_stamp_48);
+		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xE004);
+	}
+
+	mutex_unlock(&private->clock_lock);
+
+	// Do we need to reschedule
+	if (private->extts_en) {
+		schedule_delayed_work(&private->extts_ws, msecs_to_jiffies(interval));
+	}
+}
+
+// Must be called under clock_lock
+static void bcm54210pe_trigger_extts_event(struct bcm54210pe_private *private, u64 time_stamp)
+{
+
+	struct ptp_clock_event event;
+	struct timespec64 ts;
+
+	event.type = PTP_CLOCK_EXTTS;
+	event.timestamp = convert_48bit_to_80bit(private->second_on_set, time_stamp);
+	event.index = 0;
+
+	ptp_clock_event(private->ptp->ptp_clock, &event);
+
+    	private->last_extts_ts = time_stamp;
+
+	ns_to_ts(time_stamp, &ts);
+}
+
+static int bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period, s64 pulsewidth, int enable)
+{
+	struct phy_device *phydev;
+	u16 nco_6_register_value, frequency_hi, frequency_lo, pulsewidth_reg, pulse_start_hi, pulse_start_lo;
+	int err;
+
+	phydev = private->phydev;
+
+	if (enable) {
+		frequency_hi = 0;
+		frequency_lo = 0;
+		pulsewidth_reg = 0;
+		pulse_start_hi = 0;
+		pulse_start_lo = 0;
+
+		// Convert interval pulse spacing (period) and pulsewidth to 8 ns units
+		period /= 8;
+		pulsewidth /= 8;
+
+		// Mode 2 only: If pulsewidth is not explicitly set with PTP_PEROUT_DUTY_CYCLE
+		if (pulsewidth == 0) {
+			if (period < 2500) {
+				// At a frequency at less than 20us (2500 x 8ns) set pulse length to 1/10th of the interval pulse spacing
+				pulsewidth = period / 10;
+
+				// Where the interval pulse spacing is short, ensure we set a pulse length of 8ns
+				if (pulsewidth == 0) {
+					pulsewidth = 1;
+				}
+
+			} else {
+				// Otherwise set pulse with to 4us (8ns x 500 = 4us)
+				pulsewidth = 500;
+			}
+		}
+
+		if (private->perout_mode == SYNC_OUT_MODE_1) {
+
+			// Set period
+			private->perout_period = period;
+
+			if (!private->perout_en) {
+
+				// Set enable per_out
+				private->perout_en = true;
+				schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(1));
+			}
+
+			err = 0;
+
+		} else if (private->perout_mode == SYNC_OUT_MODE_2) {
+
+			// Set enable per_out
+			private->perout_en = true;
+
+			// Calculate registers
+			frequency_lo 	 = (u16)period; 			// Lowest 16 bits of 8ns interval pulse spacing [15:0]
+			frequency_hi	 = (u16) (0x3FFF & (period >> 16));	// Highest 14 bits of 8ns interval pulse spacing [29:16]
+			frequency_hi	|= (u16) pulsewidth << 14; 		// 2 lowest bits of 8ns pulse length [1:0]
+			pulsewidth_reg	 = (u16) (0x7F & (pulsewidth >> 2));	// 7 highest bit  of 8 ns pulse length [8:2]
+
+			// Get base value
+			nco_6_register_value = bcm54210pe_get_base_nco6_reg(
+				private, nco_6_register_value, true);
+
+			mutex_lock(&private->clock_lock);
+
+			// Write to register
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG,
+						nco_6_register_value);
+
+			// Set sync out pulse interval spacing and pulse length
+			err |= bcm_phy_write_exp(
+				phydev, NSE_DPPL_NCO_3_0_REG, frequency_lo);
+			err |= bcm_phy_write_exp(
+				phydev, NSE_DPPL_NCO_3_1_REG, frequency_hi);
+			err |= bcm_phy_write_exp(phydev,
+						 NSE_DPPL_NCO_3_2_REG,
+						 pulsewidth_reg);
+
+			// On next framesync load sync out frequency
+			err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD,
+						 0x0200);
+
+			// Trigger immediate framesync framesync
+			err |= bcm_phy_modify_exp(
+				phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+			mutex_unlock(&private->clock_lock);
+		}
+	} else {
+
+		// Set disable pps
+		private->perout_en = false;
+
+		// Get base value
+		nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+		mutex_lock(&private->clock_lock);
+
+		// Write to register
+		err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+		mutex_unlock(&private->clock_lock);
+	}
+
+	return err;
+}
+
+static void bcm54210pe_run_perout_mode_one_thread(struct work_struct *perout_ws)
+{
+	struct bcm54210pe_private *private;
+	u64 local_time_stamp_48bits; //, local_time_stamp_80bits;
+	u64 next_event, time_before_next_pulse, period;
+	u16 nco_6_register_value, pulsewidth_nco3_hack;
+	u64 wait_one, wait_two;
+
+	private = container_of((struct delayed_work *)perout_ws, struct bcm54210pe_private, perout_ws);
+	period = private->perout_period * 8;
+	pulsewidth_nco3_hack = 250; // The BCM chip is broken. It does not respect this in sync out mode 1
+
+	nco_6_register_value = 0;
+
+	// Get base value
+	nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+	// Get 48 bit local time
+	bcm54210pe_get48bittime(private, &local_time_stamp_48bits);
+
+	// Calculate time before next event and next event time
+	time_before_next_pulse =  period - (local_time_stamp_48bits % period);
+	next_event = local_time_stamp_48bits + time_before_next_pulse;
+
+	// Lock
+	mutex_lock(&private->clock_lock);
+
+	// Set pulsewidth (test reveal this does not work), but registers need content or no pulse will exist
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_3_1_REG, pulsewidth_nco3_hack << 14);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_3_2_REG, pulsewidth_nco3_hack >> 2);
+
+	// Set sync out pulse interval spacing and pulse length
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_0_REG, next_event & 0xFFF0);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_1_REG, next_event >> 16);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_2_REG, next_event >> 32);
+
+	// On next framesync load sync out frequency
+	bcm_phy_write_exp(private->phydev, SHADOW_REG_LOAD, 0x0200);
+
+	// Write to register with mode one set for sync out
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value || 0x0001);
+
+	// Trigger immediate framesync framesync
+	bcm_phy_modify_exp(private->phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	// Unlock
+	mutex_unlock(&private->clock_lock);
+
+	// Wait until 1/10 period after the next pulse
+	wait_one = (time_before_next_pulse / 1000000) + (period / 1000000 / 10);
+	mdelay(wait_one);
+
+	// Lock
+	mutex_lock(&private->clock_lock);
+
+	// Clear pulse by bumping sync_out_match to max (this pulls sync out down)
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_0_REG, 0xFFF0);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_1_REG, 0xFFFF);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_2_REG, 0xFFFF);
+
+	// On next framesync load sync out frequency
+	bcm_phy_write_exp(private->phydev, SHADOW_REG_LOAD, 0x0200);
+
+	// Trigger immediate framesync framesync
+	bcm_phy_modify_exp(private->phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	// Unlock
+	mutex_unlock(&private->clock_lock);
+
+	// Calculate wait before we reschedule the next pulse
+	wait_two = (period / 1000000) - (2 * (period / 10000000));
+
+	// Do we need to reschedule
+	if (private->perout_en) {
+		schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(wait_two));
+	}
+}
+
+
+bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
+{
+	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
+
+	if (private->hwts_rx_en) {
+		skb_queue_tail(&private->rx_skb_queue, skb);
+		schedule_work(&private->rxts_work);
+		return true;
+	}
+
+	return false;
+}
+
+void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
+{
+	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
+
+	switch (private->hwts_tx_en)
+	{
+		case HWTSTAMP_TX_ON:
+		{	
+			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+			skb_queue_tail(&private->tx_skb_queue, skb);
+			schedule_work(&private->txts_work);
+			break;
+		}
+		case HWTSTAMP_TX_OFF:
+		{	
+		
+		}
+		default:
+		{
+			kfree_skb(skb);
+			break;
+		}
+	}
+}
+
+int bcm54210pe_ts_info(struct mii_timestamper *mii_ts, struct ethtool_ts_info *info)
+{
+	struct bcm54210pe_private *bcm54210pe = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
+
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_HARDWARE |
+		SOF_TIMESTAMPING_RX_HARDWARE |
+		SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	info->phc_index = ptp_clock_index(bcm54210pe->ptp->ptp_clock);
+	info->tx_types =
+		(1 << HWTSTAMP_TX_OFF) |
+		(1 << HWTSTAMP_TX_ON) ;
+      	info->rx_filters =
+                (1 << HWTSTAMP_FILTER_NONE) |
+                (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+                (1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT);
+	return 0;
+}
+
+int bcm54210pe_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+{
+	struct bcm54210pe_private *device = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
+
+	struct hwtstamp_config cfg;
+
+	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
+		return -EFAULT;
+
+	if (cfg.flags) /* reserved for future extensions */
+		return -EINVAL;
+
+	if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
+		return -ERANGE;
+
+	device->hwts_tx_en = cfg.tx_type;
+
+	switch (cfg.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		device->hwts_rx_en = 0;
+		device->layer = 0;
+		device->version = 0;
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L4;
+		device->version = PTP_CLASS_V1;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L4;
+		device->version = PTP_CLASS_V2;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L2;
+		device->version = PTP_CLASS_V2;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L4 | PTP_CLASS_L2;
+		device->version = PTP_CLASS_V2;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		break;
+	default:
+		return -ERANGE;
+	}
+	
+	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+}
+
+static int bcm54210pe_feature_enable(struct ptp_clock_info *info, struct ptp_clock_request *req, int on)
+{
+	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
+	s64 period, pulsewidth;
+	struct timespec64 ts;
+
+	switch (req->type) {
+
+	case PTP_CLK_REQ_PEROUT :
+
+		period = 0;
+		pulsewidth = 0;
+
+		// Check if pin func is set correctly
+		if (ptp->chosen->sdp_config[SYNC_OUT_PIN].func != PTP_PF_PEROUT) {
+			return -EOPNOTSUPP;
+		}
+
+		// No other flags supported
+		if (req->perout.flags & ~PTP_PEROUT_DUTY_CYCLE) {
+			return -EOPNOTSUPP;
+		}
+
+		// Check if a specific pulsewidth is set
+		if ((req->perout.flags & PTP_PEROUT_DUTY_CYCLE) > 0) {
+
+			if (ptp->chosen->perout_mode == SYNC_OUT_MODE_1) {
+				return -EOPNOTSUPP;
+			}
+
+			// Extract pulsewidth
+			ts.tv_sec = req->perout.on.sec;
+			ts.tv_nsec = req->perout.on.nsec;
+			pulsewidth = timespec64_to_ns(&ts);
+
+			// 9 bits in 8ns units, so max = 4,088ns
+			if (pulsewidth > 511 * 8) {
+				return -ERANGE;
+			}
+		}
+
+		// Extract pulse spacing interval (period)
+		ts.tv_sec = req->perout.period.sec;
+		ts.tv_nsec = req->perout.period.nsec;
+		period = timespec64_to_ns(&ts);
+
+		// 16ns is minimum pulse spacing interval (a value of 16 will result in 8ns high followed by 8 ns low)
+		if (period != 0 && period < 16) {
+			return -ERANGE;
+		}
+
+		return bcm54210pe_perout_enable(ptp->chosen, period, pulsewidth, on);
+
+	case PTP_CLK_REQ_EXTTS:
+
+		if (ptp->chosen->sdp_config[SYNC_IN_PIN].func != PTP_PF_EXTTS) {
+			return -EOPNOTSUPP;
+		}
+
+		return bcm54210pe_extts_enable(ptp->chosen, on);
+
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+
+static int bcm54210pe_ptp_verify_pin(struct ptp_clock_info *info, unsigned int pin,
+			      enum ptp_pin_function func, unsigned int chan)
+{
+	switch (func) {
+	case PTP_PF_NONE:
+		return 0;
+		break;
+	case PTP_PF_EXTTS:
+		if (pin == SYNC_IN_PIN)
+			return 0;
+		break;
+	case PTP_PF_PEROUT:
+		if (pin == SYNC_OUT_PIN)
+			return 0;
+		break;
+	case PTP_PF_PHYSYNC:
+		break;
+	}
+	return -1;
+}
+
+static const struct ptp_clock_info bcm54210pe_clk_caps = {
+        .owner          = THIS_MODULE,
+        .name           = "BCM54210PE_PHC",
+        .max_adj        = 100000000,
+        .n_alarm        = 0,
+        .n_pins         = 2,
+        .n_ext_ts       = 1,
+        .n_per_out      = 1,
+        .pps            = 0,
+        .adjtime        = &bcm54210pe_adjtime,
+        .adjfine        = &bcm54210pe_adjfine,
+        .gettime64      = &bcm54210pe_gettime,
+	.gettimex64	= &bcm54210pe_gettimex,
+        .settime64      = &bcm54210pe_settime,
+	.enable		= &bcm54210pe_feature_enable,
+	.verify		= &bcm54210pe_ptp_verify_pin,
+};
+
+static int bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en)
+{
+	u16 interrupt_mask;
+
+	interrupt_mask = 0;
+
+	if (fsync_en) {
+		interrupt_mask |= 0x0001;
+	}
+
+	if (sop_en) {
+		interrupt_mask |= 0x0002;
+	}
+
+	return bcm_phy_write_exp(phydev, INTERRUPT_MASK_REG, interrupt_mask);
+}
+
+static int bcm54210pe_sw_reset(struct phy_device *phydev)
+{
+	u16 err;
+	u16 aux;
+        
+	err =  bcm_phy_write_exp(phydev, EXT_SOFTWARE_RESET, EXT_RESET1);
+	err = bcm_phy_read_exp(phydev, EXT_ENABLE_REG1);
+        if (err < 0)
+                return err;
+
+        err = bcm_phy_write_exp(phydev, EXT_SOFTWARE_RESET, EXT_RESET2);
+	aux = bcm_phy_read_exp(phydev, EXT_SOFTWARE_RESET);
+        return err;
+}
+
+int bcm54210pe_probe(struct phy_device *phydev)
+{
+	int x, y;
+	struct bcm54210pe_ptp *ptp;
+        struct bcm54210pe_private *bcm54210pe;
+	struct ptp_pin_desc *sync_in_pin_desc, *sync_out_pin_desc;
+
+	bcm54210pe_sw_reset(phydev);
+	bcm54210pe_config_1588(phydev);
+
+	bcm54210pe = kzalloc(sizeof(struct bcm54210pe_private), GFP_KERNEL);
+        if (!bcm54210pe) {
+		return -ENOMEM;
+	}
+
+	ptp = kzalloc(sizeof(struct bcm54210pe_ptp), GFP_KERNEL);
+        if (!ptp) {
+		return -ENOMEM;
+	}
+
+	bcm54210pe->phydev = phydev;
+	bcm54210pe->ptp = ptp;
+
+	bcm54210pe->mii_ts.rxtstamp = bcm54210pe_rxtstamp;
+	bcm54210pe->mii_ts.txtstamp = bcm54210pe_txtstamp;
+	bcm54210pe->mii_ts.hwtstamp = bcm54210pe_hwtstamp;
+	bcm54210pe->mii_ts.ts_info  = bcm54210pe_ts_info;
+
+
+	phydev->mii_ts = &bcm54210pe->mii_ts;
+
+	// Initialisation of work_structs and similar
+	INIT_WORK(&bcm54210pe->txts_work, bcm54210pe_run_tx_timestamp_match_thread);
+	INIT_WORK(&bcm54210pe->rxts_work, bcm54210pe_run_rx_timestamp_match_thread);
+	INIT_DELAYED_WORK(&bcm54210pe->perout_ws, bcm54210pe_run_perout_mode_one_thread);
+	INIT_DELAYED_WORK(&bcm54210pe->extts_ws, bcm54210pe_run_extts_thread);
+
+	// SKB queues
+	skb_queue_head_init(&bcm54210pe->tx_skb_queue);
+	skb_queue_head_init(&bcm54210pe->rx_skb_queue);
+
+	for (x = 0; x < CIRCULAR_BUFFER_COUNT; x++)
+	{ 
+		INIT_LIST_HEAD(&bcm54210pe->circular_buffers[x]);
+	
+		for (y = 0; y < CIRCULAR_BUFFER_ITEM_COUNT; y++)
+		{ list_add(&bcm54210pe->circular_buffer_items[x][y].list, &bcm54210pe->circular_buffers[x]); }
+	}
+
+	// Caps
+	memcpy(&bcm54210pe->ptp->caps, &bcm54210pe_clk_caps, sizeof(bcm54210pe_clk_caps));
+	bcm54210pe->ptp->caps.pin_config = bcm54210pe->sdp_config;
+
+	// Mutex
+	mutex_init(&bcm54210pe->clock_lock);
+	mutex_init(&bcm54210pe->timestamp_buffer_lock);
+
+	// Features
+	bcm54210pe->one_step = false;
+	bcm54210pe->extts_en = false;
+	bcm54210pe->perout_en = false;
+	bcm54210pe->perout_mode = SYNC_OUT_MODE_1;
+
+	// Fibonacci RSewoke style progressive backoff scheme
+	bcm54210pe->fib_sequence[0] = 1;
+	bcm54210pe->fib_sequence[1] = 1;
+	bcm54210pe->fib_sequence[2] = 2;
+	bcm54210pe->fib_sequence[3] = 3;
+	bcm54210pe->fib_sequence[4] = 5;
+	bcm54210pe->fib_sequence[5] = 8;
+	bcm54210pe->fib_sequence[6] = 13;
+	bcm54210pe->fib_sequence[7] = 21;
+	bcm54210pe->fib_sequence[8] = 34;
+	bcm54210pe->fib_sequence[9] = 55;
+
+	//bcm54210pe->fib_sequence = {1, 1, 2, 3, 5, 8, 13, 21, 34, 55};
+	bcm54210pe->fib_factor_rx = 10;
+	bcm54210pe->fib_factor_tx = 10;
+
+	// Pin descriptions
+	sync_in_pin_desc = &bcm54210pe->sdp_config[SYNC_IN_PIN];
+	snprintf(sync_in_pin_desc->name, sizeof(sync_in_pin_desc->name), "SYNC_IN");
+	sync_in_pin_desc->index = SYNC_IN_PIN;
+	sync_in_pin_desc->func = PTP_PF_NONE;
+
+	sync_out_pin_desc = &bcm54210pe->sdp_config[SYNC_OUT_PIN];
+	snprintf(sync_out_pin_desc->name, sizeof(sync_out_pin_desc->name), "SYNC_OUT");
+	sync_out_pin_desc->index = SYNC_OUT_PIN;
+	sync_out_pin_desc->func = PTP_PF_NONE;
+
+	ptp->chosen = bcm54210pe;
+	phydev->priv = bcm54210pe;
+	ptp->caps.owner = THIS_MODULE;
+
+	bcm54210pe->ptp->ptp_clock = ptp_clock_register(&bcm54210pe->ptp->caps, &phydev->mdio.dev);
+
+	if (IS_ERR(bcm54210pe->ptp->ptp_clock)) {
+                        return PTR_ERR(bcm54210pe->ptp->ptp_clock);
+	}
+
+	return 0;
+}
+
+static u16 bcm54210pe_get_base_nco6_reg(struct bcm54210pe_private *private, u16 val, bool do_nse_init)
+{
+
+	// Set Global mode to CPU system
+	val |= 0xC000;
+
+	// NSE init
+	if (do_nse_init) {
+		val |= 0x1000;
+	}
+
+	if (private->extts_en) {
+		val |= 0x2004;
+	}
+
+	if(private->perout_en) {
+		if (private->perout_mode == SYNC_OUT_MODE_1) {
+			val |= 0x0001;
+		} else if (private->perout_mode == SYNC_OUT_MODE_2) {
+			val |= 0x0002;
+		}
+	}
+
+	return val;
+}
+
+
+static u64 convert_48bit_to_80bit(u64 second_on_set, u64 ts)
+{
+	return (second_on_set * 1000000000) + ts;
+}
+
+static u64 four_u16_to_ns(u16 *four_u16)
+{
+	u32 seconds;
+	u32 nanoseconds;
+	struct timespec64 ts;
+	u16 *ptr;
+
+	nanoseconds = 0;
+	seconds = 0;
+
+
+	ptr = (u16 *)&nanoseconds;
+	*ptr = four_u16[0]; ptr++; *ptr = four_u16[1];
+
+	ptr = (u16 *)&seconds;
+	*ptr = four_u16[2]; ptr++; *ptr = four_u16[3];
+
+	ts.tv_sec = seconds;
+	ts.tv_nsec = nanoseconds;
+
+	return ts_to_ns(&ts);
+}
+
+static u64 ts_to_ns(struct timespec64 *ts)
+{
+	return ((u64)ts->tv_sec * (u64)1000000000) + ts->tv_nsec;
+}
+
+static void ns_to_ts(u64 time_stamp, struct timespec64 *ts)
+{
+	ts->tv_sec  = ( (u64)time_stamp / (u64)1000000000 );
+	ts->tv_nsec = ( (u64)time_stamp % (u64)1000000000 );
+}
diff --git a/drivers/net/phy/bcm54210pe_ptp.h b/drivers/net/phy/bcm54210pe_ptp.h
new file mode 100755
index 0000000000000..483dafc2d4514
--- /dev/null
+++ b/drivers/net/phy/bcm54210pe_ptp.h
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  drivers/net/phy/bcm54210pe_ptp.h
+ *
+* IEEE1588 (PTP), perout and extts for BCM54210PE PHY
+ *
+ * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
+ * License: GPL
+ */
+
+#include <linux/ptp_clock_kernel.h>
+#include <linux/list.h>
+
+#define CIRCULAR_BUFFER_COUNT 8
+#define CIRCULAR_BUFFER_ITEM_COUNT 32
+
+#define SYNC_IN_PIN 0
+#define SYNC_OUT_PIN 1
+
+#define SYNC_OUT_MODE_1 1
+#define SYNC_OUT_MODE_2 2
+
+#define DIRECTION_RX 0
+#define DIRECTION_TX 1
+
+struct bcm54210pe_ptp {
+	struct ptp_clock_info caps;
+	struct ptp_clock *ptp_clock;
+	struct bcm54210pe_private *chosen;
+};
+
+struct bcm54210pe_circular_buffer_item {
+	struct list_head list;
+
+	u8 msg_type;
+	u16 sequence_id;
+	u64 time_stamp;
+	bool is_valid;
+};
+
+struct bcm54210pe_private {
+	struct phy_device *phydev;
+	struct bcm54210pe_ptp *ptp;
+	struct mii_timestamper mii_ts;
+	struct ptp_pin_desc sdp_config[2];
+
+	int ts_tx_config;
+	int tx_rx_filter;
+
+	bool one_step;
+	bool perout_en;
+	bool extts_en;
+
+	int second_on_set;
+
+	int perout_mode;
+	int perout_period;
+	int perout_pulsewidth;
+
+	u64 last_extts_ts;
+	u64 last_immediate_ts[2];
+
+	struct sk_buff_head tx_skb_queue;
+	struct sk_buff_head rx_skb_queue;
+
+	struct bcm54210pe_circular_buffer_item
+		circular_buffer_items[CIRCULAR_BUFFER_COUNT]
+				     [CIRCULAR_BUFFER_ITEM_COUNT];
+	struct list_head circular_buffers[CIRCULAR_BUFFER_COUNT];
+
+	struct work_struct txts_work, rxts_work;
+	struct delayed_work perout_ws, extts_ws;
+	struct mutex clock_lock, timestamp_buffer_lock;
+
+	int fib_sequence[10];
+
+	int fib_factor_rx;
+	int fib_factor_tx;
+
+	int hwts_tx_en;
+	int hwts_rx_en;
+	int layer;
+	int version;
+};
+
+static bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
+static void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
+static void bcm54210pe_run_rx_timestamp_match_thread(struct work_struct *w);
+static void bcm54210pe_run_tx_timestamp_match_thread(struct work_struct *w);
+static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private);
+static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp);
+
+static u16  bcm54210pe_get_base_nco6_reg(struct bcm54210pe_private *private, u16 val, bool do_nse_init);
+static int  bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en);
+static int  bcm54210pe_gettimex(struct ptp_clock_info *info, struct timespec64 *ts, struct ptp_system_timestamp *sts);
+static int  bcm54210pe_get80bittime(struct bcm54210pe_private *private, struct timespec64 *ts, struct ptp_system_timestamp *sts);
+static int  bcm54210pe_get48bittime(struct bcm54210pe_private *private, u64 *time_stamp);
+static void bcm54210pe_read80bittime_register(struct phy_device *phydev, u64 *time_stamp_80, u64 *time_stamp_48);
+static void bcm54210pe_read48bittime_register(struct phy_device *phydev, u64 *time_stamp);
+
+static int  bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period, s64 pulsewidth, int on);
+static void bcm54210pe_run_perout_mode_one_thread(struct work_struct *perout_ws);
+
+static int  bcm54210pe_extts_enable(struct bcm54210pe_private *private, int enable);
+static void bcm54210pe_run_extts_thread(struct work_struct *extts_ws);
+static void bcm54210pe_trigger_extts_event(struct bcm54210pe_private *private, u64 timestamp);
+
+static u64  convert_48bit_to_80bit(u64 second_on_set, u64 ts);
+static u64  four_u16_to_ns(u16 *four_u16);
+static u64  ts_to_ns(struct timespec64 *ts);
+static void ns_to_ts(u64 time_stamp, struct timespec64 *ts);
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 8b0ac38742d06..c8b79522cf3ad 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -15,6 +15,11 @@
 #include <linux/phy.h>
 #include <linux/brcmphy.h>
 #include <linux/of.h>
+#include <linux/irq.h>
+
+#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
+extern int bcm54210pe_probe(struct phy_device *phydev);
+#endif
 
 #define BRCM_PHY_MODEL(phydev) \
 	((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)
@@ -778,7 +783,20 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
-}, {
+},
+
+#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
+{
+	.phy_id		= PHY_ID_BCM54213PE,
+	.phy_id_mask	= 0xffffffff,
+        .name           = "Broadcom BCM54210PE",
+        /* PHY_GBIT_FEATURES */
+        .config_init    = bcm54xx_config_init,
+        .ack_interrupt  = bcm_phy_ack_intr,
+        .config_intr    = bcm_phy_config_intr,
+	.probe		= bcm54210pe_probe,
+#elif
+{ 
 	.phy_id		= PHY_ID_BCM54213PE,
 	.phy_id_mask	= 0xffffffff,
 	.name		= "Broadcom BCM54213PE",
@@ -786,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+#endif
 }, {
 	.phy_id		= PHY_ID_BCM5461,
 	.phy_id_mask	= 0xfffffff0,
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 3e377f3c69e5d..975a62286a9c6 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -87,6 +87,23 @@ config PTP_1588_CLOCK_INES
 	  core.  This clock is only useful if the MII bus of your MAC
 	  is wired up to the core.
 
+ config BCM54120PE_PHY
+	tristate "Add suport for ptp in bcm54210pe PHYs"
+	depends on NETWORK_PHY_TIMESTAMPING
+	depends on PHYLIB
+	depends on PTP_1588_CLOCK
+	depends on BCM_NET_PHYLIB
+        select NET_PTP_CLASSIFY
+	help
+	  This driver adds support for using the BCM54210PE as a PTP
+	  clock. This clock is only useful if your PTP programs are
+	  getting hardware time stamps on the PTP Ethernet packets
+	  using the SO_TIMESTAMPING API.
+
+	  In order for this to work, your MAC driver must also
+	  implement the skb_tx_timestamp() function.
+
+
 config PTP_1588_CLOCK_PCH
 	tristate "Intel PCH EG20T as PTP clock"
 	depends on X86_32 || COMPILE_TEST

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

* Re: Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface
  2022-04-20 14:03 Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Lasse Johnsen
@ 2022-04-20 19:19 ` Andrew Lunn
  2022-04-21 17:32   ` Florian Fainelli
  2022-04-22 14:21   ` [PATCH net-next] 1588 support on bcm54210pe Lasse Johnsen
  2022-04-21 14:48 ` Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Richard Cochran
  1 sibling, 2 replies; 15+ messages in thread
From: Andrew Lunn @ 2022-04-20 19:19 UTC (permalink / raw)
  To: Lasse Johnsen; +Cc: netdev, richardcochran, Gordon Hollingworth, Ahmad Byagowi

On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:
> Hello,
> 
> 
> The attached set of patches adds support for the IEEE1588 functionality on the BCM54210PE PHY using the Linux Kernel mii_timestamper interface. The BCM54210PE PHY can be found in the Raspberry PI Compute Module 4 and the work has been undertaken by Timebeat.app on behalf of Raspberry PI with help and support from the nice engineers at Broadcom.

Hi Lasse

There are a few process issues you should address.

Please wrap your email at about 80 characters.

Please take a read of

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

and

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

It is a bit of a learning curve getting patches accepted, and you have
to follow the processes defined in these documents.

>  arch/arm/configs/bcm2711_defconfig            |    1 +
>  arch/arm64/configs/bcm2711_defconfig          |    1 +

You will need to split these changes up. defconfg changes go via the
Broadcom maintainers. PHY driver changes go via netdev. You can
initially post them as a series, but in the end you might need to send
them to different people/lists.

> +obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o

How specific is this code to the bcm54210pe? Should it work for any
bcm54xxx PHY? You might want to name this file broadcom_ptp.c if it
will work with any PHY supported by broadcom.c.

> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp)
> +{
> +	struct bcm54210pe_circular_buffer_item *item; 
> +	struct list_head *this, *next;
> +
> +	u8 index = (txrx * 4) + message_type;
> +
> +	if(index >= CIRCULAR_BUFFER_COUNT)
> +	{
> +		return false;
> +	}

Please run your code through ./scripts/checkpatch.pl. You will find
the code has a number of code style issues which need cleaning up.

> +#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
> +{
> +	.phy_id		= PHY_ID_BCM54213PE,
> +	.phy_id_mask	= 0xffffffff,
> +        .name           = "Broadcom BCM54210PE",
> +        /* PHY_GBIT_FEATURES */
> +        .config_init    = bcm54xx_config_init,
> +        .ack_interrupt  = bcm_phy_ack_intr,
> +        .config_intr    = bcm_phy_config_intr,
> +	.probe		= bcm54210pe_probe,
> +#elif
> +{ 
>  	.phy_id		= PHY_ID_BCM54213PE,
>  	.phy_id_mask	= 0xffffffff,
>  	.name		= "Broadcom BCM54213PE",
> @@ -786,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
>  	.config_init	= bcm54xx_config_init,
>  	.ack_interrupt	= bcm_phy_ack_intr,
>  	.config_intr	= bcm_phy_config_intr,
> +#endif

Don't replace the existing entry, extend it with your new
functionality.

	Andrew

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

* Re: Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface
  2022-04-20 14:03 Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Lasse Johnsen
  2022-04-20 19:19 ` Andrew Lunn
@ 2022-04-21 14:48 ` Richard Cochran
  2022-04-22 15:08   ` [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe Lasse Johnsen
  1 sibling, 1 reply; 15+ messages in thread
From: Richard Cochran @ 2022-04-21 14:48 UTC (permalink / raw)
  To: Lasse Johnsen; +Cc: netdev, Gordon Hollingworth, Ahmad Byagowi

On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index bd1f419bc47ae..2fa6258103025 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

This change is unrelated to the PHY driver and should be in its own
patch series.

> @@ -39,8 +39,11 @@
>  
>  #include <asm/unaligned.h>
>  
> +#include <linux/ptp_classify.h>
> +
>  #include "bcmgenet.h"
>  
> +

Don't add extra blank lines.  As Andrew commented, run your code
through checkpatch.pl and fix the coding style.

>  /* Maximum number of hardware queues, downsized if needed */
>  #define GENET_MAX_MQ_CNT	4
>  
> @@ -2096,7 +2099,18 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>  	}
>  
>  	GENET_CB(skb)->last_cb = tx_cb_ptr;
> -	skb_tx_timestamp(skb);
> +
> +	// Timestamping
> +	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +	{
> +		//skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +		skb_pull(skb, skb_mac_offset(skb)); // Feels like this pull should really be part of ptp_classify_raw...
> +		skb_clone_tx_timestamp(skb);
> +	}
> +	else if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP))
> +	{
> +		skb_tstamp_tx(skb, NULL);
> +	}

This is totally wrong.  skb_tx_timestamp() is the correct MAC driver API.

>  
>  	/* Decrement total BD count and advance our write pointer */
>  	ring->free_bds -= nr_frags + 1;
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index a13e402074cf8..528192d59d793 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_BCM84881_PHY)	+= bcm84881.o
>  obj-$(CONFIG_BCM87XX_PHY)	+= bcm87xx.o
>  obj-$(CONFIG_BCM_CYGNUS_PHY)	+= bcm-cygnus.o
>  obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o
> +obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o
>  obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
>  obj-$(CONFIG_CICADA_PHY)	+= cicada.o
>  obj-$(CONFIG_CORTINA_PHY)	+= cortina.o

> diff --git a/drivers/net/phy/bcm54210pe_ptp.c b/drivers/net/phy/bcm54210pe_ptp.c
> new file mode 100755
> index 0000000000000..c4882c84229f9
> --- /dev/null
> +++ b/drivers/net/phy/bcm54210pe_ptp.c
> @@ -0,0 +1,1406 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  drivers/net/phy/bcm54210pe_ptp.c
> + *
> + * IEEE1588 (PTP), perout and extts for BCM54210PE PHY
> + *
> + * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
> + * License: GPL

Use the proper MODULE macros for this info.

> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/ip.h>                                                                                
> +#include <linux/net_tstamp.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>                                                                               
> +#include <linux/ptp_classify.h>
> +#include <linux/ptp_clock_kernel.h>                                                                  
> +#include <linux/udp.h>
> +#include <asm/unaligned.h> 
> +#include <linux/brcmphy.h>
> +#include <linux/irq.h>
> +#include <linux/workqueue.h>
> +#include <linux/gpio.h>
> +#include <linux/if_ether.h>
> +#include <linux/delay.h>
> +#include <linux/sched.h>

Suggest keeping include directives in alphabetical order.

> +
> +#include "bcm54210pe_ptp.h"
> +#include "bcm-phy-lib.h"
> +
> +#define PTP_CONTROL_OFFSET		32
> +#define PTP_TSMT_OFFSET 		0
> +#define PTP_SEQUENCE_ID_OFFSET		30
> +#define PTP_CLOCK_ID_OFFSET		20
> +#define PTP_CLOCK_ID_SIZE		8
> +#define PTP_SEQUENCE_PORT_NUMER_OFFSET  (PTP_CLOCK_ID_OFFSET + PTP_CLOCK_ID_SIZE)
> +
> +#define EXT_SELECT_REG			0x17
> +#define EXT_DATA_REG			0x15
> +
> +#define EXT_ENABLE_REG1			0x17
> +#define EXT_ENABLE_DATA1		0x0F7E
> +#define EXT_ENABLE_REG2			0x15
> +#define EXT_ENABLE_DATA2		0x0000
> +
> +#define EXT_1588_SLICE_REG		0x0810
> +#define EXT_1588_SLICE_DATA		0x0101

EXT_1588_SLICE_DATA is not used anywhere.

> +
> +#define ORIGINAL_TIME_CODE_0 		0x0854
> +#define ORIGINAL_TIME_CODE_1 		0x0855
> +#define ORIGINAL_TIME_CODE_2 		0x0856
> +#define ORIGINAL_TIME_CODE_3 		0x0857
> +#define ORIGINAL_TIME_CODE_4 		0x0858
> +
> +#define TIME_STAMP_REG_0		0x0889
> +#define TIME_STAMP_REG_1		0x088A
> +#define TIME_STAMP_REG_2		0x088B
> +#define TIME_STAMP_REG_3		0x08C4
> +#define TIME_STAMP_INFO_1		0x088C
> +#define TIME_STAMP_INFO_2		0x088D
> +#define INTERRUPT_STATUS_REG		0x085F
> +#define INTERRUPT_MASK_REG		0x085E
> +#define EXT_SOFTWARE_RESET		0x0F70
> +#define EXT_RESET1			0x0001 //RESET
> +#define EXT_RESET2			0x0000 //NORMAL OPERATION
> +#define GLOBAL_TIMESYNC_REG		0x0FF5
> +
> +#define TX_EVENT_MODE_REG		0x0811
> +#define RX_EVENT_MODE_REG		0x0819
> +#define TX_TSCAPTURE_ENABLE_REG		0x0821
> +#define RX_TSCAPTURE_ENABLE_REG		0x0822
> +#define TXRX_1588_OPTION_REG		0x0823
> +
> +#define TX_TS_OFFSET_LSB		0x0834
> +#define TX_TS_OFFSET_MSB		0x0835
> +#define RX_TS_OFFSET_LSB		0x0844
> +#define RX_TS_OFFSET_MSB		0x0845
> +#define NSE_DPPL_NCO_1_LSB_REG		0x0873
> +#define NSE_DPPL_NCO_1_MSB_REG		0x0874
> +
> +#define NSE_DPPL_NCO_2_0_REG		0x0875
> +#define NSE_DPPL_NCO_2_1_REG		0x0876
> +#define NSE_DPPL_NCO_2_2_REG		0x0877
> +
> +#define NSE_DPPL_NCO_3_0_REG		0x0878
> +#define NSE_DPPL_NCO_3_1_REG		0x0879
> +#define NSE_DPPL_NCO_3_2_REG		0x087A
> +
> +#define NSE_DPPL_NCO_4_REG		0x087B
> +
> +#define NSE_DPPL_NCO_5_0_REG		0x087C
> +#define NSE_DPPL_NCO_5_1_REG		0x087D
> +#define NSE_DPPL_NCO_5_2_REG		0x087E
> +
> +#define NSE_DPPL_NCO_6_REG		0x087F
> +
> +#define NSE_DPPL_NCO_7_0_REG		0x0880
> +#define NSE_DPPL_NCO_7_1_REG		0x0881
> +
> +#define DPLL_SELECT_REG			0x085b
> +#define TIMECODE_SEL_REG		0x08C3
> +#define SHADOW_REG_CONTROL		0x085C
> +#define SHADOW_REG_LOAD			0x085D
> +
> +#define PTP_INTERRUPT_REG		0x0D0C
> +
> +#define CTR_DBG_REG			0x088E
> +#define HEART_BEAT_REG4			0x08ED
> +#define HEART_BEAT_REG3			0x08EC
> +#define HEART_BEAT_REG2			0x0888
> +#define	HEART_BEAT_REG1			0x0887
> +#define	HEART_BEAT_REG0			0x0886
> +	
> +#define READ_END_REG			0x0885
> +
> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp)
> +{
> +	struct bcm54210pe_circular_buffer_item *item; 
> +	struct list_head *this, *next;
> +
> +	u8 index = (txrx * 4) + message_type;
> +
> +	if(index >= CIRCULAR_BUFFER_COUNT)
> +	{
> +		return false;
> +	}
> +
> +	list_for_each_safe(this, next, &private->circular_buffers[index])
> +	{
> +		item = list_entry(this, struct bcm54210pe_circular_buffer_item, list);
> +
> +		if(item->sequence_id == seq_id && item->is_valid)
> +		{
> +			item->is_valid = false;

Instead of using this flag, just remove matched items from list.

> +			*timestamp = item->time_stamp;
> +			mutex_unlock(&private->timestamp_buffer_lock);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private)
> +{
> +	struct phy_device *phydev = private->phydev;
> +	struct bcm54210pe_circular_buffer_item *item;
> +	u16 fifo_info_1, fifo_info_2;
> +	u8 tx_or_rx, msg_type, index;
> +	u16 sequence_id;
> +	u64 timestamp;
> +	u16 Time[4];
> +
> +	mutex_lock(&private->timestamp_buffer_lock);
> +
> +	while(bcm_phy_read_exp(phydev, INTERRUPT_STATUS_REG) & 2)

Replace magic number 2 proper macro.

Also, this loop is potentially infinite.  Add a sanity check to break out.

> +	{
> +		mutex_lock(&private->clock_lock);
> +
> +		// Flush out the FIFO
> +		bcm_phy_write_exp(phydev, READ_END_REG, 1);
> +
> +		Time[3] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_3);
> +		Time[2] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_2);
> +		Time[1] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_1);
> +		Time[0] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_0);
> +
> +		fifo_info_1 = bcm_phy_read_exp(phydev, TIME_STAMP_INFO_1);
> +		fifo_info_2 = bcm_phy_read_exp(phydev, TIME_STAMP_INFO_2);
> +
> +		bcm_phy_write_exp(phydev, READ_END_REG, 2);
> +		bcm_phy_write_exp(phydev, READ_END_REG, 0);
> +
> +		mutex_unlock(&private->clock_lock);
> +
> +		msg_type = (u8) ((fifo_info_2 & 0xF000) >> 12);
> +		tx_or_rx = (u8) ((fifo_info_2 & 0x0800) >> 11); // 1 = TX, 0 = RX
> +		sequence_id = fifo_info_1;
> +
> +		timestamp = four_u16_to_ns(Time);
> +
> +		index = (tx_or_rx * 4) + msg_type;
> +
> +		if(index < CIRCULAR_BUFFER_COUNT)
> +		{
> +			item = list_first_entry_or_null(&private->circular_buffers[index], struct bcm54210pe_circular_buffer_item, list);
> +		}
> +
> +		if(item == NULL) {
> +			continue;
> +		}
> +
> +		list_del_init(&item->list);
> +
> +		item->msg_type = msg_type;
> +		item->sequence_id = sequence_id;
> +		item->time_stamp = timestamp;
> +		item->is_valid = true;
> +
> +		list_add_tail(&item->list, &private->circular_buffers[index]);
> +	}
> +
> +	mutex_unlock(&private->timestamp_buffer_lock);
> +}
> +
> +static void bcm54210pe_run_rx_timestamp_match_thread(struct work_struct *w)
> +{
> +	struct bcm54210pe_private *private =
> +		container_of(w, struct bcm54210pe_private, rxts_work);
> +
> +	struct skb_shared_hwtstamps *shhwtstamps;
> +	struct ptp_header *hdr;
> +	struct sk_buff *skb;
> +
> +	u8 msg_type;
> +	u16 sequence_id;
> +	u64 timestamp;
> +	int x, type;
> +
> +	skb = skb_dequeue(&private->rx_skb_queue);
> +
> +	while (skb != NULL) {
> +
> +		// Yes....  skb_defer_rx_timestamp just did this but <ZZZzzz>....
> +		skb_push(skb, ETH_HLEN);
> +		type = ptp_classify_raw(skb);
> +		skb_pull(skb, ETH_HLEN);
> +
> +		hdr = ptp_parse_header(skb, type);
> +
> +		if (hdr == NULL) {
> +			goto dequeue;
> +		}
> +
> +		msg_type = ptp_get_msgtype(hdr, type);
> +		sequence_id = be16_to_cpu(hdr->sequence_id);
> +
> +		timestamp = 0;
> +
> +		for (x = 0; x < 10; x++) {

Ten times? and ...

> +			bcm54210pe_read_sop_time_register(private);
> +			if (bcm54210pe_fetch_timestamp(0, msg_type, sequence_id,
> +						       private, &timestamp)) {
> +				break;
> +			}
> +
> +			udelay(private->fib_sequence[x] *
> +			       private->fib_factor_rx);

with a cute udelay?  No, use a proper deferral mechanism.

> +		}
> +
> +		shhwtstamps = skb_hwtstamps(skb);
> +
> +		if (!shhwtstamps) {
> +			goto dequeue;
> +		}
> +
> +		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> +		shhwtstamps->hwtstamp = ns_to_ktime(timestamp);
> +
> +	dequeue:
> +		netif_rx_ni(skb);
> +		skb = skb_dequeue(&private->rx_skb_queue);
> +	}
> +}
> +
> +static void bcm54210pe_run_tx_timestamp_match_thread(struct work_struct *w)
> +{
> +	struct bcm54210pe_private *private =
> +		container_of(w, struct bcm54210pe_private, txts_work);
> +
> +	struct skb_shared_hwtstamps *shhwtstamps;
> +	struct sk_buff *skb;
> +
> +	struct ptp_header *hdr;
> +	u8 msg_type;
> +	u16 sequence_id;
> +	u64 timestamp;
> +	int x;
> +
> +	timestamp = 0;
> +	skb = skb_dequeue(&private->tx_skb_queue);
> +
> +	while (skb != NULL) {
> +		int type = ptp_classify_raw(skb);
> +		hdr = ptp_parse_header(skb, type);
> +
> +		if (!hdr) {
> +			return;
> +		}
> +
> +		msg_type = ptp_get_msgtype(hdr, type);
> +		sequence_id = be16_to_cpu(hdr->sequence_id);
> +
> +		for (x = 0; x < 10; x++) {
> +			bcm54210pe_read_sop_time_register(private);
> +			if (bcm54210pe_fetch_timestamp(1, msg_type, sequence_id,
> +						       private, &timestamp)) {
> +				break;
> +			}
> +			udelay(private->fib_sequence[x] * private->fib_factor_tx);
> +		}
> +		shhwtstamps = skb_hwtstamps(skb);
> +
> +		if (!shhwtstamps) {
> +			kfree_skb(skb);
> +			goto dequeue;
> +		}
> +
> +		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> +		shhwtstamps->hwtstamp = ns_to_ktime(timestamp);
> +
> +		skb_complete_tx_timestamp(skb, shhwtstamps);
> +
> +	dequeue:
> +		skb = skb_dequeue(&private->tx_skb_queue);
> +	}
> +}
> +
> +static int bcm54210pe_config_1588(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	err = bcm_phy_write_exp(phydev, PTP_INTERRUPT_REG, 0x3c02 );
> +
> +	err |=  bcm_phy_write_exp(phydev, GLOBAL_TIMESYNC_REG, 0x0001); //Enable global timesync register.
> +	err |=  bcm_phy_write_exp(phydev, EXT_1588_SLICE_REG, 0x0101); //ENABLE TX and RX slice 1588

Does this device support multiple ports?  If so, the driver should
support that.

> +	err |=  bcm_phy_write_exp(phydev, TX_EVENT_MODE_REG, 0xFF00); //Add 80bit timestamp + NO CPU MODE in TX
> +	err |=  bcm_phy_write_exp(phydev, RX_EVENT_MODE_REG, 0xFF00); //Add 32+32 bits timestamp + NO CPU mode in RX
> +	err |=  bcm_phy_write_exp(phydev, TIMECODE_SEL_REG, 0x0101); //Select 80 bit counter
> +	err |=  bcm_phy_write_exp(phydev, TX_TSCAPTURE_ENABLE_REG, 0x0001); //Enable timestamp capture in TX
> +	err |=  bcm_phy_write_exp(phydev, RX_TSCAPTURE_ENABLE_REG, 0x0001); //Enable timestamp capture in RX
> +
> +	//Enable shadow register
> +	err |= bcm_phy_write_exp(phydev, SHADOW_REG_CONTROL, 0x0000);
> +	err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x07c0);
> +
> +
> +	// Set global mode and trigger immediate framesync to load shaddow registers
> +	err |=  bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xC020);
> +
> +	//15, 33 or 41 - experimental
> +	printk("DEBUG: GPIO %d IRQ %d\n", 15, gpio_to_irq(41));
> +
> +	// Enable Interrupt behaviour (eventhough we get no interrupts)
> +	err |= bcm54210pe_interrupts_enable(phydev,true, false);
> +	
> +	return err; 
> +}
> +
> +static int bcm54210pe_gettimex(struct ptp_clock_info *info,
> +			       struct timespec64 *ts,
> +			       struct ptp_system_timestamp *sts)
> +{
> +
> +	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
> +	return bcm54210pe_get80bittime(ptp->chosen, ts, sts);
> +}
> +
> +// Must be called under clock_lock
> +static void bcm54210pe_read80bittime_register(struct phy_device *phydev, u64 *time_stamp_80, u64 *time_stamp_48)
> +{
> +	u16 Time[5];
> +	u64 ts[3];
> +	u64 cumulative;
> +
> +	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x400);
> +	Time[4] = bcm_phy_read_exp(phydev, HEART_BEAT_REG4);
> +	Time[3] = bcm_phy_read_exp(phydev, HEART_BEAT_REG3);
> +	Time[2] = bcm_phy_read_exp(phydev, HEART_BEAT_REG2);
> +	Time[1] = bcm_phy_read_exp(phydev, HEART_BEAT_REG1);
> +	Time[0] = bcm_phy_read_exp(phydev, HEART_BEAT_REG0);
> +
> +	// Set read end bit
> +	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x800);
> +	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x000);
> +
> +	*time_stamp_80 = four_u16_to_ns(Time);
> +
> +	if (time_stamp_48 != NULL) {
> +
> +
> +		ts[2] = (((u64)Time[2]) << 32);
> +		ts[1] = (((u64)Time[1]) << 16);
> +		ts[0] = ((u64)Time[0]);
> +
> +		cumulative = 0;
> +		cumulative |= ts[0];
> +		cumulative |= ts[1];
> +		cumulative |= ts[2];
> +
> +		*time_stamp_48 = cumulative;
> +	}
> +}
> +
> +// Must be called under clock_lock
> +static void bcm54210pe_read48bittime_register(struct phy_device *phydev, u64 *time_stamp)
> +{
> +	u16 Time[3];
> +	u64 ts[3];
> +	u64 cumulative;
> +
> +	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x400);
> +	Time[2] = bcm_phy_read_exp(phydev, HEART_BEAT_REG2);
> +	Time[1] = bcm_phy_read_exp(phydev, HEART_BEAT_REG1);
> +	Time[0] = bcm_phy_read_exp(phydev, HEART_BEAT_REG0);
> +
> +	// Set read end bit
> +	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x800);
> +	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x000);
> +
> +
> +	ts[2] = (((u64)Time[2]) << 32);
> +	ts[1] = (((u64)Time[1]) << 16);
> +	ts[0] = ((u64)Time[0]);
> +
> +	cumulative = 0;
> +	cumulative |= ts[0];
> +	cumulative |= ts[1];
> +	cumulative |= ts[2];
> +
> +	*time_stamp = cumulative;
> +}
> +
> +static int bcm54210pe_get80bittime(struct bcm54210pe_private *private,
> +				   struct timespec64 *ts,
> +				   struct ptp_system_timestamp *sts)
> +{
> +	struct phy_device *phydev;
> +	u16 nco_6_register_value;
> +	int i;
> +	u64 time_stamp_48, time_stamp_80, control_ts;
> +
> +	phydev = private->phydev;
> +
> +	// Capture timestamp on next framesync
> +	nco_6_register_value = 0x2000;
> +
> +	// Lock
> +	mutex_lock(&private->clock_lock);
> +
> +	// We share frame sync events with extts, so we need to ensure no event has occurred as we are about to boot the registers, so....
> +
> +	// If extts is enabled
> +	if (private->extts_en) {
> +
> +		// Halt framesyncs generated by the sync in pin
> +		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0000);
> +
> +		// Read what's in the 8- bit register
> +		bcm54210pe_read48bittime_register(phydev, &control_ts);
> +
> +		// If it matches neither the last gettime or extts timestamp
> +		if (control_ts != private->last_extts_ts && control_ts != private->last_immediate_ts[0]) { // FIXME: This is a bug
> +
> +			// Odds are this is a extts not yet logged as an event
> +			//printk("extts triggered by get80bittime\n");
> +			bcm54210pe_trigger_extts_event(private, control_ts);
> +		}
> +	}
> +
> +	// Heartbeat register selection. Latch 80 bit Original time coude counter into Heartbeat register
> +	// (this is undocumented)
> +	bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0040);
> +
> +	// Amend to base register
> +	nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
> +
> +	// Set the NCO register
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> +	// Trigger framesync
> +	if (sts != NULL) {
> +
> +		// If we are doing a gettimex call
> +		ptp_read_system_prets(sts);
> +		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +		ptp_read_system_postts(sts);
> +
> +	} else {
> +
> +		// or if we are doing a gettime call
> +		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +	}
> +
> +	for (i = 0; i < 5;i++) {
> +
> +		bcm54210pe_read80bittime_register(phydev, &time_stamp_80, &time_stamp_48);
> +
> +		if (time_stamp_80 != 0) {
> +			break;
> +		}
> +	}
> +
> +	// Convert to timespec64
> +	ns_to_ts(time_stamp_80, ts);
> +
> +	// If we are using extts
> +	if(private->extts_en) {
> +
> +		// Commit last timestamp
> +	   	private->last_immediate_ts[0] = time_stamp_48;
> +	    	private->last_immediate_ts[1] = time_stamp_80;
> +
> +		// Heartbeat register selection. Latch 48 bit Original time coude counter into Heartbeat register
> +		// (this is undocumented)
> +		bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0000);
> +
> +		// Rearm framesync for sync in pin
> +		nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
> +		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +	}
> +
> +	mutex_unlock(&private->clock_lock);
> +
> +	return 0;
> +}
> +
> +static int bcm54210pe_gettime(struct ptp_clock_info *info, struct timespec64 *ts)
> +{
> +	int err;
> +	err = bcm54210pe_gettimex(info, ts, NULL);
> +	return err;
> +}
> +
> +static int bcm54210pe_get48bittime(struct bcm54210pe_private *private, u64 *timestamp)
> +{
> +	u16 nco_6_register_value;
> +	int i, err;
> +
> +	struct phy_device *phydev = private->phydev;
> +
> +	// Capture timestamp on next framesync
> +	nco_6_register_value = 0x2000;
> +
> +	mutex_lock(&private->clock_lock);
> +
> +	// Heartbeat register selection. Latch 48 bit Original time coude counter into Heartbeat register
> +	// (this is undocumented)
> +	err = bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0000);
> +
> +	// Amend to base register
> +	nco_6_register_value =
> +		bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
> +
> +	// Set the NCO register
> +	err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> +	// Trigger framesync
> +	err |= bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> +	for (i = 0; i < 5; i++) {
> +
> +		bcm54210pe_read48bittime_register(phydev,timestamp);
> +
> +		if (*timestamp != 0) {
> +			break;
> +		}
> +	}
> +	mutex_unlock(&private->clock_lock);
> +
> +	return err;
> +}
> +
> +static int bcm54210pe_settime(struct ptp_clock_info *info, const struct timespec64 *ts)
> +{
> +	u16 shadow_load_register, nco_6_register_value;
> +	u16 original_time_codes[5], local_time_codes[3];
> +	struct bcm54210pe_ptp *ptp;
> +	struct phy_device *phydev;
> +
> +	ptp = container_of(info, struct bcm54210pe_ptp, caps);
> +	phydev = ptp->chosen->phydev;
> +
> +	shadow_load_register = 0;
> +	nco_6_register_value = 0;
> +
> +	// Assign original time codes (80 bit)
> +	original_time_codes[4] = (u16) ((ts->tv_sec & 0x0000FFFF00000000) >> 32);
> +	original_time_codes[3] = (u16) ((ts->tv_sec  & 0x00000000FFFF0000) >> 16);
> +	original_time_codes[2] = (u16) (ts->tv_sec  & 0x000000000000FFFF);
> +	original_time_codes[1] = (u16) ((ts->tv_nsec & 0x00000000FFFF0000) >> 16);
> +	original_time_codes[0] = (u16) (ts->tv_nsec & 0x000000000000FFFF);
> +
> +	// Assign original time codes (48 bit)
> +	local_time_codes[2] = 0x4000;
> +	local_time_codes[1] = (u16) (ts->tv_nsec >> 20);
> +	local_time_codes[0] = (u16) (ts->tv_nsec >> 4);
> +
> +	// Set Time Code load bit in the shadow load register
> +	shadow_load_register |= 0x0400;
> +
> +	// Set Local Time load bit in the shadow load register
> +	shadow_load_register |= 0x0080;
> +
> +	mutex_lock(&ptp->chosen->clock_lock);
> +
> +	// Write Original Time Code Register
> +	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_0, original_time_codes[0]);
> +	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_1, original_time_codes[1]);
> +	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_2, original_time_codes[2]);
> +	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_3, original_time_codes[3]);
> +	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_4, original_time_codes[4]);
> +
> +	// Write Local Time Code Register
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_0_REG, local_time_codes[0]);
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_1_REG, local_time_codes[1]);
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_2_REG, local_time_codes[2]);
> +
> +	// Write Shadow register
> +	bcm_phy_write_exp(phydev, SHADOW_REG_CONTROL, 0x0000);
> +	bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, shadow_load_register);
> +
> +	// Set global mode and nse_init
> +	nco_6_register_value = bcm54210pe_get_base_nco6_reg(ptp->chosen, nco_6_register_value, true);
> +
> +	// Write to register
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> +	// Trigger framesync
> +	bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> +	// Set the second on set
> +	ptp->chosen->second_on_set = ts->tv_sec;
> +
> +	mutex_unlock(&ptp->chosen->clock_lock);
> +
> +	return 0;
> +}
> +
> +static int bcm54210pe_adjfine(struct ptp_clock_info *info, long scaled_ppm)
> +{	
> +	int err;
> +	u16 lo, hi;
> +	u32 corrected_8ns_interval, base_8ns_interval;
> +	bool negative;
> +
> +	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
> +	struct phy_device *phydev = ptp->chosen->phydev;
> +
> +	negative = false;
> +        if ( scaled_ppm < 0 ) {
> +		negative = true;
> +		scaled_ppm = -scaled_ppm;
> +	}
> +
> +	// This is not completely accurate but very fast
> +	scaled_ppm >>= 7;
> +
> +	// Nominal counter increment is 8ns
> +	base_8ns_interval = 1 << 31;
> +
> +	// Add or subtract differential
> +	if (negative) {
> +		corrected_8ns_interval = base_8ns_interval - scaled_ppm;
> +	} else {
> +		corrected_8ns_interval = base_8ns_interval + scaled_ppm;
> +	}
> +
> +	// Load up registers
> +	hi = (corrected_8ns_interval & 0xFFFF0000) >> 16;
> +	lo = (corrected_8ns_interval & 0x0000FFFF);
> +
> +	mutex_lock(&ptp->chosen->clock_lock);
> +
> +	// Set freq_mdio_sel to 1
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_2_REG, 0x4000);
> +
> +	// Load 125MHz frequency reqcntrl
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_1_MSB_REG, hi);
> +	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_1_LSB_REG, lo);
> +
> +	// On next framesync load freq from freqcntrl
> +	bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x0040);
> +
> +	// Trigger framesync
> +	err = bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> +	mutex_unlock(&ptp->chosen->clock_lock);
> +
> +	return err;
> +}
> +
> +static int bcm54210pe_adjtime(struct ptp_clock_info *info, s64 delta)
> +{
> +	int err;
> +	struct timespec64 ts;
> +	u64 now;
> +
> +	err = bcm54210pe_gettime(info, &ts);
> +	if (err < 0)
> +		return err;
> +
> +	now = ktime_to_ns(timespec64_to_ktime(ts));
> +	ts = ns_to_timespec64(now + delta);
> +
> +	err = bcm54210pe_settime(info, &ts);
> +
> +	return err;
> +}
> +
> +
> +static int bcm54210pe_extts_enable(struct bcm54210pe_private *private, int enable)
> +{
> +	int err;
> +	struct phy_device *phydev;
> +	u16 nco_6_register_value;
> +
> +	phydev = private->phydev;
> +
> +	if (enable) {
> +
> +		if (!private->extts_en) {
> +
> +			// Set enable per_out
> +			private->extts_en = true;
> +			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_4_REG, 0x0001);
> +
> +			nco_6_register_value = 0;
> +			nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
> +
> +			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_7_0_REG, 0x0100);
> +			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_7_1_REG, 0x0200);
> +			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> +			schedule_delayed_work(&private->extts_ws, msecs_to_jiffies(1));
> +		}
> +
> +	} else {
> +		private->extts_en = false;
> +		err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_4_REG, 0x0000);
> +
> +	}
> +
> +	return err;
> +}
> +
> +static void bcm54210pe_run_extts_thread(struct work_struct *extts_ws)
> +{
> +	struct bcm54210pe_private *private;
> +	struct phy_device *phydev;
> +	u64 interval, time_stamp_48, time_stamp_80;
> +
> +	private = container_of((struct delayed_work *)extts_ws, struct bcm54210pe_private, extts_ws);
> +	phydev = private->phydev;
> +
> +	interval = 10;	// in ms - long after we are gone from this earth, discussions will be had and
> +	  		// songs will be sung about whether this interval is short enough....
> +			// Before you complain let me say that in Timebeat.app up to ~150ms allows
> +			// single digit ns servo accuracy. If your client / servo is not as cool: Do better :-)
> +
> +	mutex_lock(&private->clock_lock);
> +
> +	bcm54210pe_read80bittime_register(phydev, &time_stamp_80, &time_stamp_48);
> +
> +
> +	if (private->last_extts_ts != time_stamp_48 &&
> +	    private->last_immediate_ts[0] != time_stamp_48 &&
> +	    private->last_immediate_ts[1] != time_stamp_80) {
> +
> +		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xE000);
> +		bcm54210pe_trigger_extts_event(private, time_stamp_48);
> +		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xE004);
> +	}
> +
> +	mutex_unlock(&private->clock_lock);
> +
> +	// Do we need to reschedule
> +	if (private->extts_en) {
> +		schedule_delayed_work(&private->extts_ws, msecs_to_jiffies(interval));
> +	}
> +}
> +
> +// Must be called under clock_lock
> +static void bcm54210pe_trigger_extts_event(struct bcm54210pe_private *private, u64 time_stamp)
> +{
> +
> +	struct ptp_clock_event event;
> +	struct timespec64 ts;
> +
> +	event.type = PTP_CLOCK_EXTTS;
> +	event.timestamp = convert_48bit_to_80bit(private->second_on_set, time_stamp);
> +	event.index = 0;
> +
> +	ptp_clock_event(private->ptp->ptp_clock, &event);
> +
> +    	private->last_extts_ts = time_stamp;
> +
> +	ns_to_ts(time_stamp, &ts);
> +}
> +
> +static int bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period, s64 pulsewidth, int enable)
> +{
> +	struct phy_device *phydev;
> +	u16 nco_6_register_value, frequency_hi, frequency_lo, pulsewidth_reg, pulse_start_hi, pulse_start_lo;
> +	int err;
> +
> +	phydev = private->phydev;
> +
> +	if (enable) {
> +		frequency_hi = 0;
> +		frequency_lo = 0;
> +		pulsewidth_reg = 0;
> +		pulse_start_hi = 0;
> +		pulse_start_lo = 0;
> +
> +		// Convert interval pulse spacing (period) and pulsewidth to 8 ns units
> +		period /= 8;
> +		pulsewidth /= 8;
> +
> +		// Mode 2 only: If pulsewidth is not explicitly set with PTP_PEROUT_DUTY_CYCLE
> +		if (pulsewidth == 0) {
> +			if (period < 2500) {
> +				// At a frequency at less than 20us (2500 x 8ns) set pulse length to 1/10th of the interval pulse spacing
> +				pulsewidth = period / 10;
> +
> +				// Where the interval pulse spacing is short, ensure we set a pulse length of 8ns
> +				if (pulsewidth == 0) {
> +					pulsewidth = 1;
> +				}
> +
> +			} else {
> +				// Otherwise set pulse with to 4us (8ns x 500 = 4us)
> +				pulsewidth = 500;
> +			}
> +		}
> +
> +		if (private->perout_mode == SYNC_OUT_MODE_1) {
> +
> +			// Set period
> +			private->perout_period = period;
> +
> +			if (!private->perout_en) {
> +
> +				// Set enable per_out
> +				private->perout_en = true;
> +				schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(1));
> +			}
> +
> +			err = 0;
> +
> +		} else if (private->perout_mode == SYNC_OUT_MODE_2) {
> +
> +			// Set enable per_out
> +			private->perout_en = true;
> +
> +			// Calculate registers
> +			frequency_lo 	 = (u16)period; 			// Lowest 16 bits of 8ns interval pulse spacing [15:0]
> +			frequency_hi	 = (u16) (0x3FFF & (period >> 16));	// Highest 14 bits of 8ns interval pulse spacing [29:16]
> +			frequency_hi	|= (u16) pulsewidth << 14; 		// 2 lowest bits of 8ns pulse length [1:0]
> +			pulsewidth_reg	 = (u16) (0x7F & (pulsewidth >> 2));	// 7 highest bit  of 8 ns pulse length [8:2]
> +
> +			// Get base value
> +			nco_6_register_value = bcm54210pe_get_base_nco6_reg(
> +				private, nco_6_register_value, true);
> +
> +			mutex_lock(&private->clock_lock);
> +
> +			// Write to register
> +			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG,
> +						nco_6_register_value);
> +
> +			// Set sync out pulse interval spacing and pulse length
> +			err |= bcm_phy_write_exp(
> +				phydev, NSE_DPPL_NCO_3_0_REG, frequency_lo);
> +			err |= bcm_phy_write_exp(
> +				phydev, NSE_DPPL_NCO_3_1_REG, frequency_hi);
> +			err |= bcm_phy_write_exp(phydev,
> +						 NSE_DPPL_NCO_3_2_REG,
> +						 pulsewidth_reg);
> +
> +			// On next framesync load sync out frequency
> +			err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD,
> +						 0x0200);
> +
> +			// Trigger immediate framesync framesync
> +			err |= bcm_phy_modify_exp(
> +				phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> +			mutex_unlock(&private->clock_lock);
> +		}
> +	} else {
> +
> +		// Set disable pps
> +		private->perout_en = false;
> +
> +		// Get base value
> +		nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
> +
> +		mutex_lock(&private->clock_lock);
> +
> +		// Write to register
> +		err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> +		mutex_unlock(&private->clock_lock);
> +	}
> +
> +	return err;
> +}
> +
> +static void bcm54210pe_run_perout_mode_one_thread(struct work_struct *perout_ws)
> +{
> +	struct bcm54210pe_private *private;
> +	u64 local_time_stamp_48bits; //, local_time_stamp_80bits;
> +	u64 next_event, time_before_next_pulse, period;
> +	u16 nco_6_register_value, pulsewidth_nco3_hack;
> +	u64 wait_one, wait_two;
> +
> +	private = container_of((struct delayed_work *)perout_ws, struct bcm54210pe_private, perout_ws);
> +	period = private->perout_period * 8;
> +	pulsewidth_nco3_hack = 250; // The BCM chip is broken. It does not respect this in sync out mode 1
> +
> +	nco_6_register_value = 0;
> +
> +	// Get base value
> +	nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
> +
> +	// Get 48 bit local time
> +	bcm54210pe_get48bittime(private, &local_time_stamp_48bits);
> +
> +	// Calculate time before next event and next event time
> +	time_before_next_pulse =  period - (local_time_stamp_48bits % period);
> +	next_event = local_time_stamp_48bits + time_before_next_pulse;
> +
> +	// Lock
> +	mutex_lock(&private->clock_lock);
> +
> +	// Set pulsewidth (test reveal this does not work), but registers need content or no pulse will exist
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_3_1_REG, pulsewidth_nco3_hack << 14);
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_3_2_REG, pulsewidth_nco3_hack >> 2);
> +
> +	// Set sync out pulse interval spacing and pulse length
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_0_REG, next_event & 0xFFF0);
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_1_REG, next_event >> 16);
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_2_REG, next_event >> 32);
> +
> +	// On next framesync load sync out frequency
> +	bcm_phy_write_exp(private->phydev, SHADOW_REG_LOAD, 0x0200);
> +
> +	// Write to register with mode one set for sync out
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value || 0x0001);
> +
> +	// Trigger immediate framesync framesync
> +	bcm_phy_modify_exp(private->phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> +	// Unlock
> +	mutex_unlock(&private->clock_lock);
> +
> +	// Wait until 1/10 period after the next pulse
> +	wait_one = (time_before_next_pulse / 1000000) + (period / 1000000 / 10);
> +	mdelay(wait_one);
> +
> +	// Lock
> +	mutex_lock(&private->clock_lock);
> +
> +	// Clear pulse by bumping sync_out_match to max (this pulls sync out down)
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_0_REG, 0xFFF0);
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_1_REG, 0xFFFF);
> +	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_2_REG, 0xFFFF);
> +
> +	// On next framesync load sync out frequency
> +	bcm_phy_write_exp(private->phydev, SHADOW_REG_LOAD, 0x0200);
> +
> +	// Trigger immediate framesync framesync
> +	bcm_phy_modify_exp(private->phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> +	// Unlock
> +	mutex_unlock(&private->clock_lock);
> +
> +	// Calculate wait before we reschedule the next pulse
> +	wait_two = (period / 1000000) - (2 * (period / 10000000));
> +
> +	// Do we need to reschedule
> +	if (private->perout_en) {
> +		schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(wait_two));
> +	}
> +}
> +
> +
> +bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
> +{
> +	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
> +
> +	if (private->hwts_rx_en) {
> +		skb_queue_tail(&private->rx_skb_queue, skb);

This is clunky.  The time stamp item may already be in the list.  Code
should check the list and deliver the skb immediately on match.  Queue
the skb only when time stamp not ready yet.

It appears that time stamps generate an interrupt, no?  If so, then
why not use the ISR to trigger reading of time stamps?

See dp83640.c for an example of how to handle asynchronous Rx time stamps.

Moreover: Does this device provide in-band Rx time stamps?  If so, why
not use them?

> +		schedule_work(&private->rxts_work);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
> +{
> +	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
> +
> +	switch (private->hwts_tx_en)
> +	{
> +		case HWTSTAMP_TX_ON:
> +		{	
> +			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +			skb_queue_tail(&private->tx_skb_queue, skb);
> +			schedule_work(&private->txts_work);
> +			break;
> +		}
> +		case HWTSTAMP_TX_OFF:
> +		{	
> +		
> +		}
> +		default:
> +		{
> +			kfree_skb(skb);
> +			break;
> +		}
> +	}
> +}
> +
> +int bcm54210pe_ts_info(struct mii_timestamper *mii_ts, struct ethtool_ts_info *info)
> +{
> +	struct bcm54210pe_private *bcm54210pe = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
> +
> +	info->so_timestamping =
> +		SOF_TIMESTAMPING_TX_HARDWARE |
> +		SOF_TIMESTAMPING_RX_HARDWARE |
> +		SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +	info->phc_index = ptp_clock_index(bcm54210pe->ptp->ptp_clock);
> +	info->tx_types =
> +		(1 << HWTSTAMP_TX_OFF) |
> +		(1 << HWTSTAMP_TX_ON) ;
> +      	info->rx_filters =
> +                (1 << HWTSTAMP_FILTER_NONE) |
> +                (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
> +                (1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT);
> +	return 0;
> +}
> +
> +int bcm54210pe_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
> +{
> +	struct bcm54210pe_private *device = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
> +
> +	struct hwtstamp_config cfg;
> +
> +	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> +		return -EFAULT;
> +
> +	if (cfg.flags) /* reserved for future extensions */
> +		return -EINVAL;
> +
> +	if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
> +		return -ERANGE;
> +
> +	device->hwts_tx_en = cfg.tx_type;
> +
> +	switch (cfg.rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +		device->hwts_rx_en = 0;
> +		device->layer = 0;
> +		device->version = 0;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> +		device->hwts_rx_en = 1;
> +		device->layer = PTP_CLASS_L4;
> +		device->version = PTP_CLASS_V1;
> +		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> +		device->hwts_rx_en = 1;
> +		device->layer = PTP_CLASS_L4;
> +		device->version = PTP_CLASS_V2;
> +		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> +		device->hwts_rx_en = 1;
> +		device->layer = PTP_CLASS_L2;
> +		device->version = PTP_CLASS_V2;
> +		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> +		device->hwts_rx_en = 1;
> +		device->layer = PTP_CLASS_L4 | PTP_CLASS_L2;
> +		device->version = PTP_CLASS_V2;
> +		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +	
> +	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +}
> +
> +static int bcm54210pe_feature_enable(struct ptp_clock_info *info, struct ptp_clock_request *req, int on)
> +{
> +	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
> +	s64 period, pulsewidth;
> +	struct timespec64 ts;
> +
> +	switch (req->type) {
> +
> +	case PTP_CLK_REQ_PEROUT :
> +
> +		period = 0;
> +		pulsewidth = 0;
> +
> +		// Check if pin func is set correctly
> +		if (ptp->chosen->sdp_config[SYNC_OUT_PIN].func != PTP_PF_PEROUT) {
> +			return -EOPNOTSUPP;
> +		}
> +
> +		// No other flags supported
> +		if (req->perout.flags & ~PTP_PEROUT_DUTY_CYCLE) {
> +			return -EOPNOTSUPP;
> +		}
> +
> +		// Check if a specific pulsewidth is set
> +		if ((req->perout.flags & PTP_PEROUT_DUTY_CYCLE) > 0) {
> +
> +			if (ptp->chosen->perout_mode == SYNC_OUT_MODE_1) {
> +				return -EOPNOTSUPP;
> +			}
> +
> +			// Extract pulsewidth
> +			ts.tv_sec = req->perout.on.sec;
> +			ts.tv_nsec = req->perout.on.nsec;
> +			pulsewidth = timespec64_to_ns(&ts);
> +
> +			// 9 bits in 8ns units, so max = 4,088ns
> +			if (pulsewidth > 511 * 8) {
> +				return -ERANGE;
> +			}
> +		}
> +
> +		// Extract pulse spacing interval (period)
> +		ts.tv_sec = req->perout.period.sec;
> +		ts.tv_nsec = req->perout.period.nsec;
> +		period = timespec64_to_ns(&ts);
> +
> +		// 16ns is minimum pulse spacing interval (a value of 16 will result in 8ns high followed by 8 ns low)
> +		if (period != 0 && period < 16) {
> +			return -ERANGE;
> +		}
> +
> +		return bcm54210pe_perout_enable(ptp->chosen, period, pulsewidth, on);
> +
> +	case PTP_CLK_REQ_EXTTS:
> +
> +		if (ptp->chosen->sdp_config[SYNC_IN_PIN].func != PTP_PF_EXTTS) {
> +			return -EOPNOTSUPP;
> +		}
> +
> +		return bcm54210pe_extts_enable(ptp->chosen, on);
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +
> +static int bcm54210pe_ptp_verify_pin(struct ptp_clock_info *info, unsigned int pin,
> +			      enum ptp_pin_function func, unsigned int chan)
> +{
> +	switch (func) {
> +	case PTP_PF_NONE:
> +		return 0;
> +		break;
> +	case PTP_PF_EXTTS:
> +		if (pin == SYNC_IN_PIN)
> +			return 0;
> +		break;
> +	case PTP_PF_PEROUT:
> +		if (pin == SYNC_OUT_PIN)
> +			return 0;
> +		break;
> +	case PTP_PF_PHYSYNC:
> +		break;
> +	}
> +	return -1;
> +}
> +
> +static const struct ptp_clock_info bcm54210pe_clk_caps = {
> +        .owner          = THIS_MODULE,
> +        .name           = "BCM54210PE_PHC",
> +        .max_adj        = 100000000,
> +        .n_alarm        = 0,
> +        .n_pins         = 2,
> +        .n_ext_ts       = 1,
> +        .n_per_out      = 1,
> +        .pps            = 0,
> +        .adjtime        = &bcm54210pe_adjtime,
> +        .adjfine        = &bcm54210pe_adjfine,
> +        .gettime64      = &bcm54210pe_gettime,
> +	.gettimex64	= &bcm54210pe_gettimex,
> +        .settime64      = &bcm54210pe_settime,
> +	.enable		= &bcm54210pe_feature_enable,
> +	.verify		= &bcm54210pe_ptp_verify_pin,
> +};
> +
> +static int bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en)
> +{
> +	u16 interrupt_mask;
> +
> +	interrupt_mask = 0;
> +
> +	if (fsync_en) {
> +		interrupt_mask |= 0x0001;
> +	}
> +
> +	if (sop_en) {
> +		interrupt_mask |= 0x0002;
> +	}
> +
> +	return bcm_phy_write_exp(phydev, INTERRUPT_MASK_REG, interrupt_mask);
> +}
> +
> +static int bcm54210pe_sw_reset(struct phy_device *phydev)
> +{
> +	u16 err;
> +	u16 aux;
> +        
> +	err =  bcm_phy_write_exp(phydev, EXT_SOFTWARE_RESET, EXT_RESET1);
> +	err = bcm_phy_read_exp(phydev, EXT_ENABLE_REG1);
> +        if (err < 0)
> +                return err;
> +
> +        err = bcm_phy_write_exp(phydev, EXT_SOFTWARE_RESET, EXT_RESET2);
> +	aux = bcm_phy_read_exp(phydev, EXT_SOFTWARE_RESET);
> +        return err;
> +}
> +
> +int bcm54210pe_probe(struct phy_device *phydev)
> +{
> +	int x, y;
> +	struct bcm54210pe_ptp *ptp;
> +        struct bcm54210pe_private *bcm54210pe;
> +	struct ptp_pin_desc *sync_in_pin_desc, *sync_out_pin_desc;
> +
> +	bcm54210pe_sw_reset(phydev);
> +	bcm54210pe_config_1588(phydev);
> +
> +	bcm54210pe = kzalloc(sizeof(struct bcm54210pe_private), GFP_KERNEL);
> +        if (!bcm54210pe) {
> +		return -ENOMEM;
> +	}
> +
> +	ptp = kzalloc(sizeof(struct bcm54210pe_ptp), GFP_KERNEL);
> +        if (!ptp) {
> +		return -ENOMEM;
> +	}
> +
> +	bcm54210pe->phydev = phydev;
> +	bcm54210pe->ptp = ptp;
> +
> +	bcm54210pe->mii_ts.rxtstamp = bcm54210pe_rxtstamp;
> +	bcm54210pe->mii_ts.txtstamp = bcm54210pe_txtstamp;
> +	bcm54210pe->mii_ts.hwtstamp = bcm54210pe_hwtstamp;
> +	bcm54210pe->mii_ts.ts_info  = bcm54210pe_ts_info;
> +
> +
> +	phydev->mii_ts = &bcm54210pe->mii_ts;
> +
> +	// Initialisation of work_structs and similar
> +	INIT_WORK(&bcm54210pe->txts_work, bcm54210pe_run_tx_timestamp_match_thread);
> +	INIT_WORK(&bcm54210pe->rxts_work, bcm54210pe_run_rx_timestamp_match_thread);
> +	INIT_DELAYED_WORK(&bcm54210pe->perout_ws, bcm54210pe_run_perout_mode_one_thread);
> +	INIT_DELAYED_WORK(&bcm54210pe->extts_ws, bcm54210pe_run_extts_thread);

Don't use generic work.  Instead, implement ptp_clock_info::do_aux_work()

That way, you get a kthread that may be given appropriate scheduling priority.

> +
> +	// SKB queues
> +	skb_queue_head_init(&bcm54210pe->tx_skb_queue);
> +	skb_queue_head_init(&bcm54210pe->rx_skb_queue);
> +
> +	for (x = 0; x < CIRCULAR_BUFFER_COUNT; x++)
> +	{ 
> +		INIT_LIST_HEAD(&bcm54210pe->circular_buffers[x]);
> +	
> +		for (y = 0; y < CIRCULAR_BUFFER_ITEM_COUNT; y++)
> +		{ list_add(&bcm54210pe->circular_buffer_items[x][y].list, &bcm54210pe->circular_buffers[x]); }
> +	}
> +
> +	// Caps
> +	memcpy(&bcm54210pe->ptp->caps, &bcm54210pe_clk_caps, sizeof(bcm54210pe_clk_caps));
> +	bcm54210pe->ptp->caps.pin_config = bcm54210pe->sdp_config;
> +
> +	// Mutex
> +	mutex_init(&bcm54210pe->clock_lock);
> +	mutex_init(&bcm54210pe->timestamp_buffer_lock);
> +
> +	// Features
> +	bcm54210pe->one_step = false;
> +	bcm54210pe->extts_en = false;
> +	bcm54210pe->perout_en = false;
> +	bcm54210pe->perout_mode = SYNC_OUT_MODE_1;
> +
> +	// Fibonacci RSewoke style progressive backoff scheme
> +	bcm54210pe->fib_sequence[0] = 1;
> +	bcm54210pe->fib_sequence[1] = 1;
> +	bcm54210pe->fib_sequence[2] = 2;
> +	bcm54210pe->fib_sequence[3] = 3;
> +	bcm54210pe->fib_sequence[4] = 5;
> +	bcm54210pe->fib_sequence[5] = 8;
> +	bcm54210pe->fib_sequence[6] = 13;
> +	bcm54210pe->fib_sequence[7] = 21;
> +	bcm54210pe->fib_sequence[8] = 34;
> +	bcm54210pe->fib_sequence[9] = 55;
> +
> +	//bcm54210pe->fib_sequence = {1, 1, 2, 3, 5, 8, 13, 21, 34, 55};
> +	bcm54210pe->fib_factor_rx = 10;
> +	bcm54210pe->fib_factor_tx = 10;
> +
> +	// Pin descriptions
> +	sync_in_pin_desc = &bcm54210pe->sdp_config[SYNC_IN_PIN];
> +	snprintf(sync_in_pin_desc->name, sizeof(sync_in_pin_desc->name), "SYNC_IN");
> +	sync_in_pin_desc->index = SYNC_IN_PIN;
> +	sync_in_pin_desc->func = PTP_PF_NONE;
> +
> +	sync_out_pin_desc = &bcm54210pe->sdp_config[SYNC_OUT_PIN];
> +	snprintf(sync_out_pin_desc->name, sizeof(sync_out_pin_desc->name), "SYNC_OUT");
> +	sync_out_pin_desc->index = SYNC_OUT_PIN;
> +	sync_out_pin_desc->func = PTP_PF_NONE;
> +
> +	ptp->chosen = bcm54210pe;
> +	phydev->priv = bcm54210pe;
> +	ptp->caps.owner = THIS_MODULE;
> +
> +	bcm54210pe->ptp->ptp_clock = ptp_clock_register(&bcm54210pe->ptp->caps, &phydev->mdio.dev);
> +
> +	if (IS_ERR(bcm54210pe->ptp->ptp_clock)) {
> +                        return PTR_ERR(bcm54210pe->ptp->ptp_clock);
> +	}
> +
> +	return 0;
> +}
> +
> +static u16 bcm54210pe_get_base_nco6_reg(struct bcm54210pe_private *private, u16 val, bool do_nse_init)
> +{
> +
> +	// Set Global mode to CPU system
> +	val |= 0xC000;
> +
> +	// NSE init
> +	if (do_nse_init) {
> +		val |= 0x1000;
> +	}
> +
> +	if (private->extts_en) {
> +		val |= 0x2004;
> +	}
> +
> +	if(private->perout_en) {
> +		if (private->perout_mode == SYNC_OUT_MODE_1) {
> +			val |= 0x0001;
> +		} else if (private->perout_mode == SYNC_OUT_MODE_2) {
> +			val |= 0x0002;
> +		}
> +	}
> +
> +	return val;
> +}
> +
> +
> +static u64 convert_48bit_to_80bit(u64 second_on_set, u64 ts)
> +{
> +	return (second_on_set * 1000000000) + ts;
> +}
> +
> +static u64 four_u16_to_ns(u16 *four_u16)
> +{
> +	u32 seconds;
> +	u32 nanoseconds;
> +	struct timespec64 ts;
> +	u16 *ptr;
> +
> +	nanoseconds = 0;
> +	seconds = 0;
> +
> +
> +	ptr = (u16 *)&nanoseconds;
> +	*ptr = four_u16[0]; ptr++; *ptr = four_u16[1];
> +
> +	ptr = (u16 *)&seconds;
> +	*ptr = four_u16[2]; ptr++; *ptr = four_u16[3];
> +
> +	ts.tv_sec = seconds;
> +	ts.tv_nsec = nanoseconds;
> +
> +	return ts_to_ns(&ts);
> +}
> +
> +static u64 ts_to_ns(struct timespec64 *ts)
> +{
> +	return ((u64)ts->tv_sec * (u64)1000000000) + ts->tv_nsec;
> +}

Use timespec64_to_ns()

> +static void ns_to_ts(u64 time_stamp, struct timespec64 *ts)
> +{
> +	ts->tv_sec  = ( (u64)time_stamp / (u64)1000000000 );
> +	ts->tv_nsec = ( (u64)time_stamp % (u64)1000000000 );
> +}

Use ns_to_timespec64()

> diff --git a/drivers/net/phy/bcm54210pe_ptp.h b/drivers/net/phy/bcm54210pe_ptp.h
> new file mode 100755
> index 0000000000000..483dafc2d4514
> --- /dev/null
> +++ b/drivers/net/phy/bcm54210pe_ptp.h
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  drivers/net/phy/bcm54210pe_ptp.h
> + *
> +* IEEE1588 (PTP), perout and extts for BCM54210PE PHY
> + *
> + * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
> + * License: GPL
> + */
> +
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/list.h>
> +
> +#define CIRCULAR_BUFFER_COUNT 8
> +#define CIRCULAR_BUFFER_ITEM_COUNT 32
> +
> +#define SYNC_IN_PIN 0
> +#define SYNC_OUT_PIN 1
> +
> +#define SYNC_OUT_MODE_1 1
> +#define SYNC_OUT_MODE_2 2
> +
> +#define DIRECTION_RX 0
> +#define DIRECTION_TX 1
> +
> +struct bcm54210pe_ptp {
> +	struct ptp_clock_info caps;
> +	struct ptp_clock *ptp_clock;
> +	struct bcm54210pe_private *chosen;
> +};
> +
> +struct bcm54210pe_circular_buffer_item {
> +	struct list_head list;
> +
> +	u8 msg_type;
> +	u16 sequence_id;
> +	u64 time_stamp;
> +	bool is_valid;
> +};
> +
> +struct bcm54210pe_private {
> +	struct phy_device *phydev;
> +	struct bcm54210pe_ptp *ptp;
> +	struct mii_timestamper mii_ts;
> +	struct ptp_pin_desc sdp_config[2];
> +
> +	int ts_tx_config;
> +	int tx_rx_filter;
> +
> +	bool one_step;
> +	bool perout_en;
> +	bool extts_en;
> +
> +	int second_on_set;
> +
> +	int perout_mode;
> +	int perout_period;
> +	int perout_pulsewidth;
> +
> +	u64 last_extts_ts;
> +	u64 last_immediate_ts[2];
> +
> +	struct sk_buff_head tx_skb_queue;
> +	struct sk_buff_head rx_skb_queue;
> +
> +	struct bcm54210pe_circular_buffer_item
> +		circular_buffer_items[CIRCULAR_BUFFER_COUNT]
> +				     [CIRCULAR_BUFFER_ITEM_COUNT];
> +	struct list_head circular_buffers[CIRCULAR_BUFFER_COUNT];
> +
> +	struct work_struct txts_work, rxts_work;
> +	struct delayed_work perout_ws, extts_ws;
> +	struct mutex clock_lock, timestamp_buffer_lock;
> +
> +	int fib_sequence[10];
> +
> +	int fib_factor_rx;
> +	int fib_factor_tx;
> +
> +	int hwts_tx_en;
> +	int hwts_rx_en;
> +	int layer;
> +	int version;
> +};
> +
> +static bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
> +static void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
> +static void bcm54210pe_run_rx_timestamp_match_thread(struct work_struct *w);
> +static void bcm54210pe_run_tx_timestamp_match_thread(struct work_struct *w);
> +static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private);
> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp);
> +
> +static u16  bcm54210pe_get_base_nco6_reg(struct bcm54210pe_private *private, u16 val, bool do_nse_init);
> +static int  bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en);
> +static int  bcm54210pe_gettimex(struct ptp_clock_info *info, struct timespec64 *ts, struct ptp_system_timestamp *sts);
> +static int  bcm54210pe_get80bittime(struct bcm54210pe_private *private, struct timespec64 *ts, struct ptp_system_timestamp *sts);
> +static int  bcm54210pe_get48bittime(struct bcm54210pe_private *private, u64 *time_stamp);
> +static void bcm54210pe_read80bittime_register(struct phy_device *phydev, u64 *time_stamp_80, u64 *time_stamp_48);
> +static void bcm54210pe_read48bittime_register(struct phy_device *phydev, u64 *time_stamp);
> +
> +static int  bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period, s64 pulsewidth, int on);
> +static void bcm54210pe_run_perout_mode_one_thread(struct work_struct *perout_ws);
> +
> +static int  bcm54210pe_extts_enable(struct bcm54210pe_private *private, int enable);
> +static void bcm54210pe_run_extts_thread(struct work_struct *extts_ws);
> +static void bcm54210pe_trigger_extts_event(struct bcm54210pe_private *private, u64 timestamp);
> +
> +static u64  convert_48bit_to_80bit(u64 second_on_set, u64 ts);
> +static u64  four_u16_to_ns(u16 *four_u16);
> +static u64  ts_to_ns(struct timespec64 *ts);
> +static void ns_to_ts(u64 time_stamp, struct timespec64 *ts);

Never put "static" prototypes into a header file.

Avoid static prototypes altogether.  Instead, order the functions
within the source file so that sub-functions appear before their call
sites.

> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 8b0ac38742d06..c8b79522cf3ad 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -15,6 +15,11 @@
>  #include <linux/phy.h>
>  #include <linux/brcmphy.h>
>  #include <linux/of.h>
> +#include <linux/irq.h>
> +
> +#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
> +extern int bcm54210pe_probe(struct phy_device *phydev);
> +#endif
>  
>  #define BRCM_PHY_MODEL(phydev) \
>  	((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)
> @@ -778,7 +783,20 @@ static struct phy_driver broadcom_drivers[] = {
>  	.config_init	= bcm54xx_config_init,
>  	.ack_interrupt	= bcm_phy_ack_intr,
>  	.config_intr	= bcm_phy_config_intr,
> -}, {
> +},
> +
> +#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
> +{
> +	.phy_id		= PHY_ID_BCM54213PE,
> +	.phy_id_mask	= 0xffffffff,
> +        .name           = "Broadcom BCM54210PE",
> +        /* PHY_GBIT_FEATURES */
> +        .config_init    = bcm54xx_config_init,
> +        .ack_interrupt  = bcm_phy_ack_intr,
> +        .config_intr    = bcm_phy_config_intr,
> +	.probe		= bcm54210pe_probe,
> +#elif
> +{ 
>  	.phy_id		= PHY_ID_BCM54213PE,
>  	.phy_id_mask	= 0xffffffff,
>  	.name		= "Broadcom BCM54213PE",
> @@ -786,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
>  	.config_init	= bcm54xx_config_init,
>  	.ack_interrupt	= bcm_phy_ack_intr,
>  	.config_intr	= bcm_phy_config_intr,
> +#endif
>  }, {
>  	.phy_id		= PHY_ID_BCM5461,
>  	.phy_id_mask	= 0xfffffff0,
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 3e377f3c69e5d..975a62286a9c6 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -87,6 +87,23 @@ config PTP_1588_CLOCK_INES
>  	  core.  This clock is only useful if the MII bus of your MAC
>  	  is wired up to the core.
>  
> + config BCM54120PE_PHY
> +	tristate "Add suport for ptp in bcm54210pe PHYs"
> +	depends on NETWORK_PHY_TIMESTAMPING
> +	depends on PHYLIB
> +	depends on PTP_1588_CLOCK
> +	depends on BCM_NET_PHYLIB
> +        select NET_PTP_CLASSIFY
> +	help
> +	  This driver adds support for using the BCM54210PE as a PTP
> +	  clock. This clock is only useful if your PTP programs are
> +	  getting hardware time stamps on the PTP Ethernet packets
> +	  using the SO_TIMESTAMPING API.
> +
> +	  In order for this to work, your MAC driver must also
> +	  implement the skb_tx_timestamp() function.
> +
> +
>  config PTP_1588_CLOCK_PCH
>  	tristate "Intel PCH EG20T as PTP clock"
>  	depends on X86_32 || COMPILE_TEST


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

* Re: Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface
  2022-04-20 19:19 ` Andrew Lunn
@ 2022-04-21 17:32   ` Florian Fainelli
  2022-04-22 14:21   ` [PATCH net-next] 1588 support on bcm54210pe Lasse Johnsen
  1 sibling, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2022-04-21 17:32 UTC (permalink / raw)
  To: Andrew Lunn, Lasse Johnsen
  Cc: netdev, richardcochran, Gordon Hollingworth, Ahmad Byagowi

On 4/20/22 12:19, Andrew Lunn wrote:
> On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:
>> Hello,
>>
>>
>> The attached set of patches adds support for the IEEE1588 functionality on the BCM54210PE PHY using the Linux Kernel mii_timestamper interface. The BCM54210PE PHY can be found in the Raspberry PI Compute Module 4 and the work has been undertaken by Timebeat.app on behalf of Raspberry PI with help and support from the nice engineers at Broadcom.

Lasse, can you copy the maintainers listed for 
drivers/net/phy/broadcom.c in the future? Thank you.
-- 
Florian

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

* [PATCH net-next] 1588 support on bcm54210pe
  2022-04-20 19:19 ` Andrew Lunn
  2022-04-21 17:32   ` Florian Fainelli
@ 2022-04-22 14:21   ` Lasse Johnsen
  2022-04-22 15:00     ` Andrew Lunn
  1 sibling, 1 reply; 15+ messages in thread
From: Lasse Johnsen @ 2022-04-22 14:21 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, richardcochran, Gordon Hollingworth, Ahmad Byagowi,
	Heiner Kallweit, Russell King, bcm-kernel-feedback-list,
	Florian Fainelli

Hi Andrew,

> On 20 Apr 2022, at 20:19, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:
>> Hello,
>> 
>> 
>> The attached set of patches adds support for the IEEE1588 functionality on the BCM54210PE PHY using the Linux Kernel mii_timestamper interface. The BCM54210PE PHY can be found in the Raspberry PI Compute Module 4 and the work has been undertaken by Timebeat.app on behalf of Raspberry PI with help and support from the nice engineers at Broadcom.
> 
> Hi Lasse
> 
> There are a few process issues you should address.
> 
> Please wrap your email at about 80 characters.
> 
> Please take a read of
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> 
> and
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq
> 
> It is a bit of a learning curve getting patches accepted, and you have
> to follow the processes defined in these documents.

I have read the documents, I understand about 10% of them and I am considering jumping off a tall building :-)

I’ve changed the subject of the email. How did I do?

> 
>> arch/arm/configs/bcm2711_defconfig            |    1 +
>> arch/arm64/configs/bcm2711_defconfig          |    1 +
> 
> You will need to split these changes up. defconfg changes go via the
> Broadcom maintainers. PHY driver changes go via netdev. You can
> initially post them as a series, but in the end you might need to send
> them to different people/lists.
> 

Ok. I was asked by Florian to put the Broadcom maintainers in Cc so I will do this to begin with.

>> +obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o
> 
> How specific is this code to the bcm54210pe? Should it work for any
> bcm54xxx PHY? You might want to name this file broadcom_ptp.c if it
> will work with any PHY supported by broadcom.c.

I am confident that this code is relevant exclusively to the BCM54210PE. It will not even work with the BCM54210, BCM54210S and BCM54210SE PHYs.

> 
>> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp)
>> +{
>> +	struct bcm54210pe_circular_buffer_item *item; 
>> +	struct list_head *this, *next;
>> +
>> +	u8 index = (txrx * 4) + message_type;
>> +
>> +	if(index >= CIRCULAR_BUFFER_COUNT)
>> +	{
>> +		return false;
>> +	}
> 
> Please run your code through ./scripts/checkpatch.pl. You will find
> the code has a number of code style issues which need cleaning up.

I am about to respond to Richard's mail with an amended set of patches which is much cleaner. checkpatch now complains only about a Signed-off line and asks if Maintainers needs updating because I’ve added a file (I guess it probably does). 

> 
>> +#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
>> +{
>> +	.phy_id		= PHY_ID_BCM54213PE,
>> +	.phy_id_mask	= 0xffffffff,
>> +        .name           = "Broadcom BCM54210PE",
>> +        /* PHY_GBIT_FEATURES */
>> +        .config_init    = bcm54xx_config_init,
>> +        .ack_interrupt  = bcm_phy_ack_intr,
>> +        .config_intr    = bcm_phy_config_intr,
>> +	.probe		= bcm54210pe_probe,
>> +#elif
>> +{ 
>> 	.phy_id		= PHY_ID_BCM54213PE,
>> 	.phy_id_mask	= 0xffffffff,
>> 	.name		= "Broadcom BCM54213PE",
>> @@ -786,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
>> 	.config_init	= bcm54xx_config_init,
>> 	.ack_interrupt	= bcm_phy_ack_intr,
>> 	.config_intr	= bcm_phy_config_intr,
>> +#endif
> 
> Don't replace the existing entry, extend it with your new
> functionality.

Is what you propose possible? Isn’t the issue here that the two PHYs (54213PE and 54210PE) present themselves with the same phy ID? If there is a way to seperate them then I will need your instruction on how to do it. 

> 
> 	Andrew

All the best,

Lasse

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

* Re: [PATCH net-next] 1588 support on bcm54210pe
  2022-04-22 14:21   ` [PATCH net-next] 1588 support on bcm54210pe Lasse Johnsen
@ 2022-04-22 15:00     ` Andrew Lunn
  2022-04-22 19:48       ` Richard Cochran
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Lunn @ 2022-04-22 15:00 UTC (permalink / raw)
  To: Lasse Johnsen
  Cc: netdev, richardcochran, Gordon Hollingworth, Ahmad Byagowi,
	Heiner Kallweit, Russell King, bcm-kernel-feedback-list,
	Florian Fainelli

On Fri, Apr 22, 2022 at 03:21:16PM +0100, Lasse Johnsen wrote:
> Hi Andrew,
> 
> > On 20 Apr 2022, at 20:19, Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:
> >> Hello,
> >> 
> >> 
> >> The attached set of patches adds support for the IEEE1588 functionality on the BCM54210PE PHY using the Linux Kernel mii_timestamper interface. The BCM54210PE PHY can be found in the Raspberry PI Compute Module 4 and the work has been undertaken by Timebeat.app on behalf of Raspberry PI with help and support from the nice engineers at Broadcom.
> > 
> > Hi Lasse
> > 
> > There are a few process issues you should address.
> > 
> > Please wrap your email at about 80 characters.

Still not wrapped. Kernel developers tend to be old school, still
believe a terminal is 80 characters wide, and has 25 lines, just like
the VT100 they grew up with. It can be hard to get some email clients
to do this correctly, which is why most use mutt.

> > Please take a read of
> > 
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> > 
> > and
> > 
> > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq
> > 
> > It is a bit of a learning curve getting patches accepted, and you have
> > to follow the processes defined in these documents.
> 
> I have read the documents, I understand about 10% of them and I am considering jumping off a tall building :-)

As i said, it is a learning curve, but we are here to help.

> I’ve changed the subject of the email. How did I do?

I step in the right direction.

git log --oneline drivers/net/phy/broadcom.c
bf8bfc4336f7 net: phy: broadcom: Fix brcm_fet_config_init()
d15c7e875d44 net: phy: broadcom: hook up soft_reset for BCM54616S
72e78d22e152 net: phy: broadcom: Utilize appropriate suspend for BCM54810/11
38b6a9073007 net: phy: broadcom: Wire suspend/resume for BCM50610 and BCM50610M
d6da08ed1425 net: phy: broadcom: Add IDDQ-SR mode
8dc84dcd7f74 net: phy: broadcom: Enable 10BaseT DAC early wake
ad4e1e48a629 net: phy: broadcom: re-add check for PHY_BRCM_DIS_TXCRXC_NOENRGY on the BCM54811 PHY
5a32fcdb1e68 net: phy: broadcom: Add statistics for all Gigabit PHYs
b1dd9bf688b0 net: phy: broadcom: Fix RGMII delays for BCM50160 and BCM50610M

The prefix "net: phy: broadcom" helps get the right people to review
your patch. Florian will be looking for anything "broadcom". I look
for anything "phy".

> Ok. I was asked by Florian to put the Broadcom maintainers in Cc so I will do this to begin with.

There is a tool to help you get the correct people to send patches to:

./scripts/get_maintainer.pl <FILENAME>.patch

and it will give you something like:

Florian Fainelli <f.fainelli@gmail.com> (supporter:BROADCOM ETHERNET PHY DRIVERS)
Andrew Lunn <andrew@lunn.ch> (maintainer:ETHERNET PHY LIBRARY)
Heiner Kallweit <hkallweit1@gmail.com> (maintainer:ETHERNET PHY LIBRARY)
Russell King <linux@armlinux.org.uk> (reviewer:ETHERNET PHY LIBRARY)
"David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS)
bcm-kernel-feedback-list@broadcom.com (open list:BROADCOM ETHERNET PHY DRIVERS)
netdev@vger.kernel.org (open list:BROADCOM ETHERNET PHY DRIVERS)
linux-kernel@vger.kernel.org (open list)

> >> +obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o
> > 
> > How specific is this code to the bcm54210pe? Should it work for any
> > bcm54xxx PHY? You might want to name this file broadcom_ptp.c if it
> > will work with any PHY supported by broadcom.c.
> 

> I am confident that this code is relevant exclusively to the
> BCM54210PE. It will not even work with the BCM54210, BCM54210S and
> BCM54210SE PHYs.

Florian can probably tell us more, but often hardware like this is
shared by multiple devices. If it is, you might want to use a more
generic prefix.

> >> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp)
> >> +{
> >> +	struct bcm54210pe_circular_buffer_item *item; 
> >> +	struct list_head *this, *next;
> >> +
> >> +	u8 index = (txrx * 4) + message_type;
> >> +
> >> +	if(index >= CIRCULAR_BUFFER_COUNT)
> >> +	{
> >> +		return false;
> >> +	}
> > 
> > Please run your code through ./scripts/checkpatch.pl. You will find
> > the code has a number of code style issues which need cleaning up.
 
> I am about to respond to Richard's mail with an amended set of
> patches which is much cleaner. checkpatch now complains only about a
> Signed-off line and asks if Maintainers needs updating because I’ve
> added a file (I guess it probably does).

Signed-off-by is important. Without it, your patch will not get
accepted. Did a number of people write the code? You might need
Signed-off-by: from each of them, or you need to use
Co-Developed-by.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Please try to use

git format-patch

and

git send-email

when sending your updated patches. It is a good idea to pass
--cover-letter to git format-patch, and give a 'big picture'
explanation in patch 0/X, along with a list of what you have changed
since the last version. Please also remember to put "v2: in the
subject, "patch net-next v2", so we can keep track of the different
versions.

> >> +#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
> >> +{
> >> +	.phy_id		= PHY_ID_BCM54213PE,
> >> +	.phy_id_mask	= 0xffffffff,
> >> +        .name           = "Broadcom BCM54210PE",
> >> +        /* PHY_GBIT_FEATURES */
> >> +        .config_init    = bcm54xx_config_init,
> >> +        .ack_interrupt  = bcm_phy_ack_intr,
> >> +        .config_intr    = bcm_phy_config_intr,
> >> +	.probe		= bcm54210pe_probe,
> >> +#elif
> >> +{ 
> >> 	.phy_id		= PHY_ID_BCM54213PE,
> >> 	.phy_id_mask	= 0xffffffff,
> >> 	.name		= "Broadcom BCM54213PE",
> >> @@ -786,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
> >> 	.config_init	= bcm54xx_config_init,
> >> 	.ack_interrupt	= bcm_phy_ack_intr,
> >> 	.config_intr	= bcm_phy_config_intr,
> >> +#endif
> > 
> > Don't replace the existing entry, extend it with your new
> > functionality.
 

> Is what you propose possible? Isn’t the issue here that the two PHYs
> (54213PE and 54210PE) present themselves with the same phy ID?

Ah, they should not do that. There are solutions to this, but lets
leave this as is for the moment. Lets get other issues solved first.

      Andrew

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

* [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe
  2022-04-21 14:48 ` Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Richard Cochran
@ 2022-04-22 15:08   ` Lasse Johnsen
  2022-04-22 15:22     ` Jonathan Lemon
  2022-04-22 15:42     ` Andrew Lunn
  0 siblings, 2 replies; 15+ messages in thread
From: Lasse Johnsen @ 2022-04-22 15:08 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev, Gordon Hollingworth, Ahmad Byagowi, Florian Fainelli,
	Andrew Lunn, Heiner Kallweit, Russell King,
	bcm-kernel-feedback-list, David S. Miller, Jakub Kicinski,
	Paolo Abeni

[-- Attachment #1: Type: text/plain, Size: 15308 bytes --]

Hi Richard,

Firstly, off topic, please allow me the indulgence this once. If it was not for your work, then I’m pretty sure my career would have looked significantly different. It’s a real privilege to interact with you. Respect… :-)

(To anyone not too familiar with the real world consequences of what accurate clock sync has provided, in finance as an example, Richard’s work prevents brokerages from front-running their clients, it’s a bulwark against fraud and is the foundation for numerous pieces of financial regulation protecting the average Joe all over the world).


> On 21 Apr 2022, at 15:48, Richard Cochran <richardcochran@gmail.com> wrote:
> 
> On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index bd1f419bc47ae..2fa6258103025 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> 
> This change is unrelated to the PHY driver and should be in its own
> patch series.
> 

Noted, I have everything in a single commit after I flattened 86 previous commits - what is a good way to do what you propose?

>> @@ -39,8 +39,11 @@
>> 
>> #include <asm/unaligned.h>
>> 
>> +#include <linux/ptp_classify.h>
>> +
>> #include "bcmgenet.h"
>> 
>> +
> 
> Don't add extra blank lines.  As Andrew commented, run your code
> through checkpatch.pl and fix the coding style.

I have done so in the new patches below.

> 
>> /* Maximum number of hardware queues, downsized if needed */
>> #define GENET_MAX_MQ_CNT	4
>> 
>> @@ -2096,7 +2099,18 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>> 	}
>> 
>> 	GENET_CB(skb)->last_cb = tx_cb_ptr;
>> -	skb_tx_timestamp(skb);
>> +
>> +	// Timestamping
>> +	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
>> +	{
>> +		//skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>> +		skb_pull(skb, skb_mac_offset(skb)); // Feels like this pull should really be part of ptp_classify_raw...
>> +		skb_clone_tx_timestamp(skb);
>> +	}
>> +	else if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP))
>> +	{
>> +		skb_tstamp_tx(skb, NULL);
>> +	}
> 
> This is totally wrong.  skb_tx_timestamp() is the correct MAC driver API.

Thank you. What is a good if condition in the instance then please? In the new patches I have gone with:

(unlikely(skb_shinfo(skb)->tx_flags & (SKBTX_HW_TSTAMP | SKBTX_SW_TSTAMP)) > 0)


> 
>> 
>> 	/* Decrement total BD count and advance our write pointer */
>> 	ring->free_bds -= nr_frags + 1;
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index a13e402074cf8..528192d59d793 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_BCM84881_PHY)	+= bcm84881.o
>> obj-$(CONFIG_BCM87XX_PHY)	+= bcm87xx.o
>> obj-$(CONFIG_BCM_CYGNUS_PHY)	+= bcm-cygnus.o
>> obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o
>> +obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o
>> obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
>> obj-$(CONFIG_CICADA_PHY)	+= cicada.o
>> obj-$(CONFIG_CORTINA_PHY)	+= cortina.o
> 
>> diff --git a/drivers/net/phy/bcm54210pe_ptp.c b/drivers/net/phy/bcm54210pe_ptp.c
>> new file mode 100755
>> index 0000000000000..c4882c84229f9
>> --- /dev/null
>> +++ b/drivers/net/phy/bcm54210pe_ptp.c
>> @@ -0,0 +1,1406 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + *  drivers/net/phy/bcm54210pe_ptp.c
>> + *
>> + * IEEE1588 (PTP), perout and extts for BCM54210PE PHY
>> + *
>> + * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
>> + * License: GPL
> 
> Use the proper MODULE macros for this info.

I have added the module macros in the new patches.

> 
>> + */
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/ip.h>                                                                                
>> +#include <linux/net_tstamp.h>
>> +#include <linux/mii.h>
>> +#include <linux/phy.h>                                                                               
>> +#include <linux/ptp_classify.h>
>> +#include <linux/ptp_clock_kernel.h>                                                                  
>> +#include <linux/udp.h>
>> +#include <asm/unaligned.h> 
>> +#include <linux/brcmphy.h>
>> +#include <linux/irq.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/gpio.h>
>> +#include <linux/if_ether.h>
>> +#include <linux/delay.h>
>> +#include <linux/sched.h>
> 
> Suggest keeping include directives in alphabetical order.

Noted, and included in new version of patches.

> 
>> +
>> +#include "bcm54210pe_ptp.h"
>> +#include "bcm-phy-lib.h"
>> +
>> +#define PTP_CONTROL_OFFSET		32
>> +#define PTP_TSMT_OFFSET 		0
>> +#define PTP_SEQUENCE_ID_OFFSET		30
>> +#define PTP_CLOCK_ID_OFFSET		20
>> +#define PTP_CLOCK_ID_SIZE		8
>> +#define PTP_SEQUENCE_PORT_NUMER_OFFSET  (PTP_CLOCK_ID_OFFSET + PTP_CLOCK_ID_SIZE)
>> +
>> +#define EXT_SELECT_REG			0x17
>> +#define EXT_DATA_REG			0x15
>> +
>> +#define EXT_ENABLE_REG1			0x17
>> +#define EXT_ENABLE_DATA1		0x0F7E
>> +#define EXT_ENABLE_REG2			0x15
>> +#define EXT_ENABLE_DATA2		0x0000
>> +
>> +#define EXT_1588_SLICE_REG		0x0810
>> +#define EXT_1588_SLICE_DATA		0x0101
> 
> EXT_1588_SLICE_DATA is not used anywhere.

Noted and removed. EXT_SELECT_REG removed for same reason.

>> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp)
>> +{
>> +	struct bcm54210pe_circular_buffer_item *item; 
>> +	struct list_head *this, *next;
>> +
>> +	u8 index = (txrx * 4) + message_type;
>> +
>> +	if(index >= CIRCULAR_BUFFER_COUNT)
>> +	{
>> +		return false;
>> +	}
>> +
>> +	list_for_each_safe(this, next, &private->circular_buffers[index])
>> +	{
>> +		item = list_entry(this, struct bcm54210pe_circular_buffer_item, list);
>> +
>> +		if(item->sequence_id == seq_id && item->is_valid)
>> +		{
>> +			item->is_valid = false;
> 
> Instead of using this flag, just remove matched items from list.

It’s a circular buffer of a predefined size. I could be wrong, but does this not preclude what you suggest? If not, I would humbly ask for guidance.

>> +static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private)
>> +{
>> +	struct phy_device *phydev = private->phydev;
>> +	struct bcm54210pe_circular_buffer_item *item;
>> +	u16 fifo_info_1, fifo_info_2;
>> +	u8 tx_or_rx, msg_type, index;
>> +	u16 sequence_id;
>> +	u64 timestamp;
>> +	u16 Time[4];
>> +
>> +	mutex_lock(&private->timestamp_buffer_lock);
>> +
>> +	while(bcm_phy_read_exp(phydev, INTERRUPT_STATUS_REG) & 2)
> 
> Replace magic number 2 proper macro.

The magic is gone and replaced with INTC_SOP. INTC_FSYNC has also been defined in the header file in preparation for future interrupt driven behaviour. It is currently unused. If this is not agreeable I can remove. 

> 
> Also, this loop is potentially infinite.  Add a sanity check to break out.

I have added an iteration check to prevent this.

>> +	while (skb != NULL) {
>> +
>> +		// Yes....  skb_defer_rx_timestamp just did this but <ZZZzzz>....
>> +		skb_push(skb, ETH_HLEN);
>> +		type = ptp_classify_raw(skb);
>> +		skb_pull(skb, ETH_HLEN);
>> +
>> +		hdr = ptp_parse_header(skb, type);
>> +
>> +		if (hdr == NULL) {
>> +			goto dequeue;
>> +		}
>> +
>> +		msg_type = ptp_get_msgtype(hdr, type);
>> +		sequence_id = be16_to_cpu(hdr->sequence_id);
>> +
>> +		timestamp = 0;
>> +
>> +		for (x = 0; x < 10; x++) {
> 
> Ten times? and ...

If we have not matched the timestamp in the PHY fifo by then then a timestamp of 0 is returned. This strikes me as reasonable behaviour, but I can omit returning the packet in the err queue completely if that is the desired behaviour. I will rely on your guidance for the desired behaviour.

> 
>> +			bcm54210pe_read_sop_time_register(private);
>> +			if (bcm54210pe_fetch_timestamp(0, msg_type, sequence_id,
>> +						       private, &timestamp)) {
>> +				break;
>> +			}
>> +
>> +			udelay(private->fib_sequence[x] *
>> +			       private->fib_factor_rx);
> 
> with a cute udelay?  No, use a proper deferral mechanism.

Hehe… The proposed method strikes me as cheap and reasonable in the instance. I am happy to change, but will you propose a satisfactory deferral mechanism in the instance? I just don’t have sufficient kernel experience to identify the best approach and your input would be much appreciated. 

I have read this: https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

I think on the rx side it is unlikely that we get to the udelay as that timestamp “should be”(tm) available on the first iteration. On the corresponding tx side the amount of udelaying is also minimal, but again, I will take the direction you give me.  

>> +static int bcm54210pe_config_1588(struct phy_device *phydev)
>> +{
>> +	int err;
>> +
>> +	err = bcm_phy_write_exp(phydev, PTP_INTERRUPT_REG, 0x3c02 );
>> +
>> +	err |=  bcm_phy_write_exp(phydev, GLOBAL_TIMESYNC_REG, 0x0001); //Enable global timesync register.
>> +	err |=  bcm_phy_write_exp(phydev, EXT_1588_SLICE_REG, 0x0101); //ENABLE TX and RX slice 1588
> 
> Does this device support multiple ports?  If so, the driver should
> support that.

It does not no. It is a single port PHY.

>> 
>> +bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
>> +{
>> +	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
>> +
>> +	if (private->hwts_rx_en) {
>> +		skb_queue_tail(&private->rx_skb_queue, skb);
> 
> This is clunky.  The time stamp item may already be in the list.  Code
> should check the list and deliver the skb immediately on match.  Queue
> the skb only when time stamp not ready yet.
> 

No, that strikes me as unlikely both when acting as a source or consumer of PTP. The updating of the list is triggered by an tx or rx frame event. So in order for an rx timestamp to be already available, a tx event must have occurred at more or less the exact same time. This is not impossible, but probably sufficiently improbable to warrant the check you suggest. However, your experience far outweighs mine and I can modify to implement the behaviour you suggest if you insist.

> It appears that time stamps generate an interrupt, no?  If so, then
> why not use the ISR to trigger reading of time stamps?
> 

No, as I indicated in my email we trigger poll style behaviour on the receipt or transmit of 1588 frames. This is not ideal, and I hope to change in the future time permitting, but in the current iteration, that is the behaviour and it performs well.

> See dp83640.c for an example of how to handle asynchronous Rx time stamps.
> 
> Moreover: Does this device provide in-band Rx time stamps?  If so, why
> not use them?

This is the first generation PHY and it does not do in-band RX. I asked BCM and studied the documentation. I’m sure I’m allowed to say, that the second generation 40nm BCM PHY (which - "I am not making this up" is available in 3 versions: BCM54210, BCM54210S and BCM54210SE - not “PE”) - supports in-band rx timestamps. However, as a matter of curiosity, BCM utilise the field in the header now used for minor versioning in 1588-2019, so in due course using this silicon feature will be a significant challenge.

>> +	// Initialisation of work_structs and similar
>> +	INIT_WORK(&bcm54210pe->txts_work, bcm54210pe_run_tx_timestamp_match_thread);
>> +	INIT_WORK(&bcm54210pe->rxts_work, bcm54210pe_run_rx_timestamp_match_thread);
>> +	INIT_DELAYED_WORK(&bcm54210pe->perout_ws, bcm54210pe_run_perout_mode_one_thread);
>> +	INIT_DELAYED_WORK(&bcm54210pe->extts_ws, bcm54210pe_run_extts_thread);
> 
> Don't use generic work.  Instead, implement ptp_clock_info::do_aux_work()

No periodic work is undertaken. Rather rx and tx frames schedule work. With this in mind are you confident your recommendation is sound? If so I might need additional instruction on how to implement.

> 
> That way, you get a kthread that may be given appropriate scheduling priority.

Given the currently scheme if you do not propose to change it, can I give the theads an alternative priority such as it is?

>> 
>> +static u64 ts_to_ns(struct timespec64 *ts)
>> +{
>> +	return ((u64)ts->tv_sec * (u64)1000000000) + ts->tv_nsec;
>> +}
> 
> Use timespec64_to_ns()

Thank you. Noted with thanks and updated.

> 
>> +static void ns_to_ts(u64 time_stamp, struct timespec64 *ts)
>> +{
>> +	ts->tv_sec  = ( (u64)time_stamp / (u64)1000000000 );
>> +	ts->tv_nsec = ( (u64)time_stamp % (u64)1000000000 );
>> +}
> 
> Use ns_to_timespec64()

Also noted with thanks and updated.

>> 
>> +static bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
>> +static void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
>> +static void bcm54210pe_run_rx_timestamp_match_thread(struct work_struct *w);
>> +static void bcm54210pe_run_tx_timestamp_match_thread(struct work_struct *w);
>> +static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private);
>> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp);
>> +
>> +static u16  bcm54210pe_get_base_nco6_reg(struct bcm54210pe_private *private, u16 val, bool do_nse_init);
>> +static int  bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en);
>> +static int  bcm54210pe_gettimex(struct ptp_clock_info *info, struct timespec64 *ts, struct ptp_system_timestamp *sts);
>> +static int  bcm54210pe_get80bittime(struct bcm54210pe_private *private, struct timespec64 *ts, struct ptp_system_timestamp *sts);
>> +static int  bcm54210pe_get48bittime(struct bcm54210pe_private *private, u64 *time_stamp);
>> +static void bcm54210pe_read80bittime_register(struct phy_device *phydev, u64 *time_stamp_80, u64 *time_stamp_48);
>> +static void bcm54210pe_read48bittime_register(struct phy_device *phydev, u64 *time_stamp);
>> +
>> +static int  bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period, s64 pulsewidth, int on);
>> +static void bcm54210pe_run_perout_mode_one_thread(struct work_struct *perout_ws);
>> +
>> +static int  bcm54210pe_extts_enable(struct bcm54210pe_private *private, int enable);
>> +static void bcm54210pe_run_extts_thread(struct work_struct *extts_ws);
>> +static void bcm54210pe_trigger_extts_event(struct bcm54210pe_private *private, u64 timestamp);
>> +
>> +static u64  convert_48bit_to_80bit(u64 second_on_set, u64 ts);
>> +static u64  four_u16_to_ns(u16 *four_u16);
>> +static u64  ts_to_ns(struct timespec64 *ts);
>> +static void ns_to_ts(u64 time_stamp, struct timespec64 *ts);
> 
> Never put "static" prototypes into a header file.
> 
> Avoid static prototypes altogether.  Instead, order the functions
> within the source file so that sub-functions appear before their call
> sites.

Noted with thanks and updated.




[-- Attachment #2: bcm54210pe-1588.v2.patch.txt --]
[-- Type: text/plain, Size: 48668 bytes --]

From 3fcbbac9fe85909877f15d95f7a1e64dd6d06ab7 Mon Sep 17 00:00:00 2001
From: Lasse Johnsen <l@ssejohnsen.me>
Date: Sat, 5 Feb 2022 09:34:19 -0500
Subject: [PATCH] Added support for IEEE1588 timestamping for the BCM54210PE
 PHY using the kernel mii_timestamper interface

---
 arch/arm/configs/bcm2711_defconfig            |    1 +
 arch/arm64/configs/bcm2711_defconfig          |    1 +
 .../net/ethernet/broadcom/genet/bcmgenet.c    |    8 +-
 drivers/net/phy/Makefile                      |    1 +
 drivers/net/phy/bcm54210pe_ptp.c              | 1382 +++++++++++++++++
 drivers/net/phy/bcm54210pe_ptp.h              |   85 +
 drivers/net/phy/broadcom.c                    |   21 +-
 drivers/ptp/Kconfig                           |   17 +
 8 files changed, 1514 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/bcm54210pe_ptp.c
 create mode 100644 drivers/net/phy/bcm54210pe_ptp.h

diff --git a/arch/arm/configs/bcm2711_defconfig b/arch/arm/configs/bcm2711_defconfig
index 8f4ae82cade4f..c7f3cce0024b7 100644
--- a/arch/arm/configs/bcm2711_defconfig
+++ b/arch/arm/configs/bcm2711_defconfig
@@ -1402,6 +1402,7 @@ CONFIG_MAXIM_THERMOCOUPLE=m
 CONFIG_MAX31856=m
 CONFIG_PWM_BCM2835=m
 CONFIG_PWM_PCA9685=m
+CONFIG_GENERIC_PHY=y
 CONFIG_RPI_AXIPERF=m
 CONFIG_NVMEM_RMEM=m
 CONFIG_EXT4_FS=y
diff --git a/arch/arm64/configs/bcm2711_defconfig b/arch/arm64/configs/bcm2711_defconfig
index 75333e69ef741..2af6b2fc5dcda 100644
--- a/arch/arm64/configs/bcm2711_defconfig
+++ b/arch/arm64/configs/bcm2711_defconfig
@@ -1409,6 +1409,7 @@ CONFIG_MAXIM_THERMOCOUPLE=m
 CONFIG_MAX31856=m
 CONFIG_PWM_BCM2835=m
 CONFIG_PWM_PCA9685=m
+CONFIG_GENERIC_PHY=y
 CONFIG_RPI_AXIPERF=m
 CONFIG_ANDROID=y
 CONFIG_ANDROID_BINDER_IPC=y
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index bd1f419bc47ae..264bcb5b08b24 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -36,6 +36,7 @@
 #include <linux/ipv6.h>
 #include <linux/phy.h>
 #include <linux/platform_data/bcmgenet.h>
+#include <linux/ptp_classify.h>
 
 #include <asm/unaligned.h>
 
@@ -2096,7 +2097,12 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	GENET_CB(skb)->last_cb = tx_cb_ptr;
-	skb_tx_timestamp(skb);
+
+	// Timestamping
+	if (unlikely(skb_shinfo(skb)->tx_flags & (SKBTX_HW_TSTAMP | SKBTX_SW_TSTAMP)) > 0) {
+		skb_pull(skb, skb_mac_offset(skb));
+		skb_tx_timestamp(skb);
+	}
 
 	/* Decrement total BD count and advance our write pointer */
 	ring->free_bds -= nr_frags + 1;
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf8..528192d59d793 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_BCM84881_PHY)	+= bcm84881.o
 obj-$(CONFIG_BCM87XX_PHY)	+= bcm87xx.o
 obj-$(CONFIG_BCM_CYGNUS_PHY)	+= bcm-cygnus.o
 obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o
+obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o
 obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
 obj-$(CONFIG_CICADA_PHY)	+= cicada.o
 obj-$(CONFIG_CORTINA_PHY)	+= cortina.o
diff --git a/drivers/net/phy/bcm54210pe_ptp.c b/drivers/net/phy/bcm54210pe_ptp.c
new file mode 100644
index 0000000000000..d35f98d1403c7
--- /dev/null
+++ b/drivers/net/phy/bcm54210pe_ptp.c
@@ -0,0 +1,1382 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *
+ * IEEE1588 (PTP), perout and extts for BCM54210PE PHY
+ *
+ * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
+ * License: GPL
+ */
+
+#include <linux/brcmphy.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/if_ether.h>
+#include <linux/ip.h>
+#include <linux/irq.h>
+#include <linux/mii.h>
+#include <linux/net_tstamp.h>
+#include <linux/phy.h>
+#include <linux/ptp_classify.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/sched.h>
+#include <linux/udp.h>
+#include <linux/workqueue.h>
+
+#include <asm/unaligned.h>
+
+#include "bcm54210pe_ptp.h"
+#include "bcm-phy-lib.h"
+
+MODULE_DESCRIPTION("Broadcom BCM54210PE PHY driver");
+MODULE_AUTHOR("Lasse L. Johnsen");
+MODULE_LICENSE("GPL");
+
+#define PTP_CONTROL_OFFSET		32
+#define PTP_TSMT_OFFSET			0
+#define PTP_SEQUENCE_ID_OFFSET	30
+#define PTP_CLOCK_ID_OFFSET		20
+#define PTP_CLOCK_ID_SIZE		8
+#define PTP_SEQUENCE_PORT_NUMER_OFFSET  (PTP_CLOCK_ID_OFFSET + PTP_CLOCK_ID_SIZE)
+
+#define EXT_ENABLE_REG1			0x17
+#define EXT_ENABLE_DATA1		0x0F7E
+#define EXT_ENABLE_REG2			0x15
+#define EXT_ENABLE_DATA2		0x0000
+
+#define EXT_1588_SLICE_REG		0x0810
+#define EXT_1588_SLICE_DATA		0x0101
+
+#define ORIGINAL_TIME_CODE_0	0x0854
+#define ORIGINAL_TIME_CODE_1	0x0855
+#define ORIGINAL_TIME_CODE_2	0x0856
+#define ORIGINAL_TIME_CODE_3	0x0857
+#define ORIGINAL_TIME_CODE_4	0x0858
+
+#define TIME_STAMP_REG_0		0x0889
+#define TIME_STAMP_REG_1		0x088A
+#define TIME_STAMP_REG_2		0x088B
+#define TIME_STAMP_REG_3		0x08C4
+#define TIME_STAMP_INFO_1		0x088C
+#define TIME_STAMP_INFO_2		0x088D
+#define INTERRUPT_STATUS_REG	0x085F
+#define INTERRUPT_MASK_REG		0x085E
+#define EXT_SOFTWARE_RESET		0x0F70
+#define EXT_RESET1				0x0001 //RESET
+#define EXT_RESET2				0x0000 //NORMAL OPERATION
+#define GLOBAL_TIMESYNC_REG		0x0FF5
+
+#define TX_EVENT_MODE_REG		0x0811
+#define RX_EVENT_MODE_REG		0x0819
+#define TX_TSCAPTURE_ENABLE_REG	0x0821
+#define RX_TSCAPTURE_ENABLE_REG	0x0822
+#define TXRX_1588_OPTION_REG	0x0823
+
+#define TX_TS_OFFSET_LSB		0x0834
+#define TX_TS_OFFSET_MSB		0x0835
+#define RX_TS_OFFSET_LSB		0x0844
+#define RX_TS_OFFSET_MSB		0x0845
+#define NSE_DPPL_NCO_1_LSB_REG	0x0873
+#define NSE_DPPL_NCO_1_MSB_REG	0x0874
+
+#define NSE_DPPL_NCO_2_0_REG	0x0875
+#define NSE_DPPL_NCO_2_1_REG	0x0876
+#define NSE_DPPL_NCO_2_2_REG	0x0877
+
+#define NSE_DPPL_NCO_3_0_REG	0x0878
+#define NSE_DPPL_NCO_3_1_REG	0x0879
+#define NSE_DPPL_NCO_3_2_REG	0x087A
+
+#define NSE_DPPL_NCO_4_REG		0x087B
+
+#define NSE_DPPL_NCO_5_0_REG	0x087C
+#define NSE_DPPL_NCO_5_1_REG	0x087D
+#define NSE_DPPL_NCO_5_2_REG	0x087E
+
+#define NSE_DPPL_NCO_6_REG		0x087F
+
+#define NSE_DPPL_NCO_7_0_REG	0x0880
+#define NSE_DPPL_NCO_7_1_REG	0x0881
+
+#define DPLL_SELECT_REG			0x085b
+#define TIMECODE_SEL_REG		0x08C3
+#define SHADOW_REG_CONTROL		0x085C
+#define SHADOW_REG_LOAD			0x085D
+
+#define PTP_INTERRUPT_REG		0x0D0C
+
+#define CTR_DBG_REG				0x088E
+#define HEART_BEAT_REG4			0x08ED
+#define HEART_BEAT_REG3			0x08EC
+#define HEART_BEAT_REG2			0x0888
+#define	HEART_BEAT_REG1			0x0887
+#define	HEART_BEAT_REG0			0x0886
+
+#define READ_END_REG			0x0885
+
+static u64 convert_48bit_to_80bit(u64 second_on_set, u64 ts)
+{
+	return (second_on_set * 1000000000) + ts;
+}
+
+static u64 four_u16_to_ns(u16 *four_u16)
+{
+	u32 seconds;
+	u32 nanoseconds;
+	struct timespec64 ts;
+	u16 *ptr;
+
+	nanoseconds = 0;
+	seconds = 0;
+
+	ptr = (u16 *)&nanoseconds;
+	*ptr = four_u16[0]; ptr++; *ptr = four_u16[1];
+
+	ptr = (u16 *)&seconds;
+	*ptr = four_u16[2]; ptr++; *ptr = four_u16[3];
+
+	ts.tv_sec = seconds;
+	ts.tv_nsec = nanoseconds;
+
+	return timespec64_to_ns(&ts);
+}
+
+static int bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en)
+{
+	u16 interrupt_mask;
+
+	interrupt_mask = 0;
+
+	if (fsync_en)
+		interrupt_mask |= 0x0001;
+
+	if (sop_en)
+		interrupt_mask |= 0x0002;
+
+	return bcm_phy_write_exp(phydev, INTERRUPT_MASK_REG, interrupt_mask);
+}
+
+static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id,
+				       struct bcm54210pe_private *private, u64 *timestamp)
+{
+	struct bcm54210pe_circular_buffer_item *item;
+	struct list_head *this, *next;
+
+	u8 index = (txrx * 4) + message_type;
+
+	if (index >= CIRCULAR_BUFFER_COUNT)
+		return false;
+
+	list_for_each_safe(this, next, &private->circular_buffers[index]) {
+		item = list_entry(this, struct bcm54210pe_circular_buffer_item, list);
+
+		if (item->sequence_id == seq_id && item->is_valid) {
+			item->is_valid = false;
+			*timestamp = item->time_stamp;
+			mutex_unlock(&private->timestamp_buffer_lock);
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static u16 bcm54210pe_get_base_nco6_reg(struct bcm54210pe_private *private,
+					u16 val, bool do_nse_init)
+{
+	// Set Global mode to CPU system
+	val |= 0xC000;
+
+	// NSE init
+	if (do_nse_init)
+		val |= 0x1000;
+
+	if (private->extts_en)
+		val |= 0x2004;
+
+	if (private->perout_en) {
+		if (private->perout_mode == SYNC_OUT_MODE_1)
+			val |= 0x0001;
+		else if (private->perout_mode == SYNC_OUT_MODE_2)
+			val |= 0x0002;
+	}
+
+	return val;
+}
+
+static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private)
+{
+	struct phy_device *phydev = private->phydev;
+	struct bcm54210pe_circular_buffer_item *item;
+	u16 fifo_info_1, fifo_info_2;
+	u8 tx_or_rx, msg_type, index;
+	u16 sequence_id;
+	u64 timestamp;
+	u16 time[4];
+	int deadlock_check;
+
+	deadlock_check = 0;
+
+	mutex_lock(&private->timestamp_buffer_lock);
+
+	while (bcm_phy_read_exp(phydev, INTERRUPT_STATUS_REG) & INTC_SOP) {
+		mutex_lock(&private->clock_lock);
+
+		// Flush out the FIFO
+		bcm_phy_write_exp(phydev, READ_END_REG, 1);
+
+		time[3] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_3);
+		time[2] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_2);
+		time[1] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_1);
+		time[0] = bcm_phy_read_exp(phydev, TIME_STAMP_REG_0);
+
+		fifo_info_1 = bcm_phy_read_exp(phydev, TIME_STAMP_INFO_1);
+		fifo_info_2 = bcm_phy_read_exp(phydev, TIME_STAMP_INFO_2);
+
+		bcm_phy_write_exp(phydev, READ_END_REG, 2);
+		bcm_phy_write_exp(phydev, READ_END_REG, 0);
+
+		mutex_unlock(&private->clock_lock);
+
+		msg_type = (u8)((fifo_info_2 & 0xF000) >> 12);
+		tx_or_rx = (u8)((fifo_info_2 & 0x0800) >> 11); // 1 = TX, 0 = RX
+		sequence_id = fifo_info_1;
+
+		timestamp = four_u16_to_ns(time);
+
+		index = (tx_or_rx * 4) + msg_type;
+
+		if (index < CIRCULAR_BUFFER_COUNT)
+			item = list_first_entry_or_null(&private->circular_buffers[index],
+							struct bcm54210pe_circular_buffer_item,
+							list);
+
+		if (!item)
+			continue;
+
+		list_del_init(&item->list);
+
+		item->msg_type = msg_type;
+		item->sequence_id = sequence_id;
+		item->time_stamp = timestamp;
+		item->is_valid = true;
+
+		list_add_tail(&item->list, &private->circular_buffers[index]);
+
+		deadlock_check++;
+		if (deadlock_check > 100)
+			break;
+	}
+
+	mutex_unlock(&private->timestamp_buffer_lock);
+}
+
+static void bcm54210pe_run_rx_timestamp_match_thread(struct work_struct *w)
+{
+	struct bcm54210pe_private *private =
+		container_of(w, struct bcm54210pe_private, rxts_work);
+
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct ptp_header *hdr;
+	struct sk_buff *skb;
+
+	u8 msg_type;
+	u16 sequence_id;
+	u64 timestamp;
+	int x, type;
+
+	skb = skb_dequeue(&private->rx_skb_queue);
+
+	while (skb) {
+		// Yes....  skb_defer_rx_timestamp just did this but <ZZZzzz>....
+		skb_push(skb, ETH_HLEN);
+		type = ptp_classify_raw(skb);
+		skb_pull(skb, ETH_HLEN);
+
+		hdr = ptp_parse_header(skb, type);
+
+		if (!hdr)
+			goto dequeue;
+
+		msg_type = ptp_get_msgtype(hdr, type);
+		sequence_id = be16_to_cpu(hdr->sequence_id);
+
+		timestamp = 0;
+
+		for (x = 0; x < 10; x++) {
+			bcm54210pe_read_sop_time_register(private);
+			if (bcm54210pe_fetch_timestamp(0, msg_type, sequence_id,
+						       private, &timestamp)) {
+				break;
+			}
+
+			udelay(private->fib_sequence[x] *
+			       private->fib_factor_rx);
+		}
+
+		shhwtstamps = skb_hwtstamps(skb);
+
+		if (!shhwtstamps)
+			goto dequeue;
+
+		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+		shhwtstamps->hwtstamp = ns_to_ktime(timestamp);
+
+dequeue:
+		netif_rx_ni(skb);
+		skb = skb_dequeue(&private->rx_skb_queue);
+	}
+}
+
+static void bcm54210pe_run_tx_timestamp_match_thread(struct work_struct *w)
+{
+	struct bcm54210pe_private *private =
+		container_of(w, struct bcm54210pe_private, txts_work);
+
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct sk_buff *skb;
+
+	struct ptp_header *hdr;
+	u8 msg_type;
+	u16 sequence_id;
+	u64 timestamp;
+	int x, type;
+
+	timestamp = 0;
+	skb = skb_dequeue(&private->tx_skb_queue);
+
+	while (skb) {
+		type = ptp_classify_raw(skb);
+		hdr = ptp_parse_header(skb, type);
+
+		if (!hdr)
+			goto dequeue;
+
+		msg_type = ptp_get_msgtype(hdr, type);
+		sequence_id = be16_to_cpu(hdr->sequence_id);
+
+		for (x = 0; x < 10; x++) {
+			bcm54210pe_read_sop_time_register(private);
+			if (bcm54210pe_fetch_timestamp(1, msg_type, sequence_id,
+						       private, &timestamp)) {
+				break;
+			}
+			udelay(private->fib_sequence[x] * private->fib_factor_tx);
+		}
+		shhwtstamps = skb_hwtstamps(skb);
+
+		if (!shhwtstamps) {
+			kfree_skb(skb);
+			goto dequeue;
+		}
+
+		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+		shhwtstamps->hwtstamp = ns_to_ktime(timestamp);
+
+		skb_complete_tx_timestamp(skb, shhwtstamps);
+
+dequeue:
+		skb = skb_dequeue(&private->tx_skb_queue);
+	}
+}
+
+static int bcm54210pe_config_1588(struct phy_device *phydev)
+{
+	int err;
+
+	err = bcm_phy_write_exp(phydev, PTP_INTERRUPT_REG, 0x3c02);
+
+	//Enable global timesync register
+	err |=  bcm_phy_write_exp(phydev, GLOBAL_TIMESYNC_REG, 0x0001);
+
+	//ENABLE TX and RX slice 1588
+	err |=  bcm_phy_write_exp(phydev, EXT_1588_SLICE_REG, 0x0101);
+
+	//Add 80bit timestamp + NO CPU MODE in TX
+	err |=  bcm_phy_write_exp(phydev, TX_EVENT_MODE_REG, 0xFF00);
+
+	//Add 32+32 bits timestamp + NO CPU mode in RX
+	err |=  bcm_phy_write_exp(phydev, RX_EVENT_MODE_REG, 0xFF00);
+
+	//Select 80 bit counter
+	err |=  bcm_phy_write_exp(phydev, TIMECODE_SEL_REG, 0x0101);
+
+	//Enable timestamp capture in TX
+	err |=  bcm_phy_write_exp(phydev, TX_TSCAPTURE_ENABLE_REG, 0x0001);
+
+	//Enable timestamp capture in RX
+	err |=  bcm_phy_write_exp(phydev, RX_TSCAPTURE_ENABLE_REG, 0x0001);
+
+	//Enable shadow register
+	err |= bcm_phy_write_exp(phydev, SHADOW_REG_CONTROL, 0x0000);
+	err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x07c0);
+
+	// Set global mode and trigger immediate framesync to load shaddow registers
+	err |=  bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xC020);
+
+	// Enable Interrupt behaviour (eventhough we get no interrupts)
+	err |= bcm54210pe_interrupts_enable(phydev, true, false);
+
+	return err;
+}
+
+// Must be called under clock_lock
+static void bcm54210pe_trigger_extts_event(struct bcm54210pe_private *private, u64 timestamp)
+{
+	struct ptp_clock_event event;
+	struct timespec64 ts;
+
+	event.type = PTP_CLOCK_EXTTS;
+	event.timestamp = convert_48bit_to_80bit(private->second_on_set, timestamp);
+	event.index = 0;
+
+	ptp_clock_event(private->ptp->ptp_clock, &event);
+
+	private->last_extts_ts = timestamp;
+
+	ts = ns_to_timespec64(timestamp);
+}
+
+// Must be called under clock_lock
+static void bcm54210pe_read80bittime_register(struct phy_device *phydev,
+					      u64 *time_stamp_80, u64 *time_stamp_48)
+{
+	u16 time[5];
+	u64 ts[3];
+	u64 cumulative;
+
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x400);
+	time[4] = bcm_phy_read_exp(phydev, HEART_BEAT_REG4);
+	time[3] = bcm_phy_read_exp(phydev, HEART_BEAT_REG3);
+	time[2] = bcm_phy_read_exp(phydev, HEART_BEAT_REG2);
+	time[1] = bcm_phy_read_exp(phydev, HEART_BEAT_REG1);
+	time[0] = bcm_phy_read_exp(phydev, HEART_BEAT_REG0);
+
+	// Set read end bit
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x800);
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x000);
+
+	*time_stamp_80 = four_u16_to_ns(time);
+
+	if (time_stamp_48) {
+		ts[2] = (((u64)time[2]) << 32);
+		ts[1] = (((u64)time[1]) << 16);
+		ts[0] = ((u64)time[0]);
+
+		cumulative = 0;
+		cumulative |= ts[0];
+		cumulative |= ts[1];
+		cumulative |= ts[2];
+
+		*time_stamp_48 = cumulative;
+	}
+}
+
+// Must be called under clock_lock
+static void bcm54210pe_read48bittime_register(struct phy_device *phydev, u64 *time_stamp)
+{
+	u16 time[3];
+	u64 ts[3];
+	u64 cumulative;
+
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x400);
+	time[2] = bcm_phy_read_exp(phydev, HEART_BEAT_REG2);
+	time[1] = bcm_phy_read_exp(phydev, HEART_BEAT_REG1);
+	time[0] = bcm_phy_read_exp(phydev, HEART_BEAT_REG0);
+
+	// Set read end bit
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x800);
+	bcm_phy_write_exp(phydev, CTR_DBG_REG, 0x000);
+
+	ts[2] = (((u64)time[2]) << 32);
+	ts[1] = (((u64)time[1]) << 16);
+	ts[0] = ((u64)time[0]);
+
+	cumulative = 0;
+	cumulative |= ts[0];
+	cumulative |= ts[1];
+	cumulative |= ts[2];
+
+	*time_stamp = cumulative;
+}
+
+static int bcm54210pe_get80bittime(struct bcm54210pe_private *private,
+				   struct timespec64 *ts,
+				   struct ptp_system_timestamp *sts)
+{
+	struct phy_device *phydev;
+	u16 nco_6_register_value;
+	int i;
+	u64 time_stamp_48, time_stamp_80, control_ts;
+
+	phydev = private->phydev;
+
+	// Capture timestamp on next framesync
+	nco_6_register_value = 0x2000;
+
+	// Lock
+	mutex_lock(&private->clock_lock);
+
+	// We share frame sync events with extts, so we need to ensure no event
+	// has occurred as we are about to boot the registers, so....
+
+	// If extts is enabled
+	if (private->extts_en) {
+		// Halt framesyncs generated by the sync in pin
+		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0000);
+
+		// Read what's in the 8- bit register
+		bcm54210pe_read48bittime_register(phydev, &control_ts);
+
+		// If it matches neither the last gettime or extts timestamp
+		if (control_ts != private->last_extts_ts &&
+		    control_ts != private->last_immediate_ts[0]) {
+			// Odds are this is a extts not yet logged as an event
+			//printk("extts triggered by get80bittime\n");
+			bcm54210pe_trigger_extts_event(private, control_ts);
+		}
+	}
+
+	// Heartbeat register selection. Latch 80 bit Original time counter
+	// into Heartbeat register (this is undocumented)
+	bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0040);
+
+	// Amend to base register
+	nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+	// Set the NCO register
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+	// Trigger framesync
+	if (sts) {
+		// If we are doing a gettimex call
+		ptp_read_system_prets(sts);
+		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+		ptp_read_system_postts(sts);
+
+	} else {
+		// or if we are doing a gettime call
+		bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+	}
+
+	for (i = 0; i < 5; i++) {
+		bcm54210pe_read80bittime_register(phydev, &time_stamp_80, &time_stamp_48);
+
+		if (time_stamp_80 != 0)
+			break;
+	}
+
+	// Convert to timespec64
+	*ts = ns_to_timespec64(time_stamp_80);
+
+	// If we are using extts
+	if (private->extts_en) {
+		// Commit last timestamp
+		private->last_immediate_ts[0] = time_stamp_48;
+		private->last_immediate_ts[1] = time_stamp_80;
+
+		// Heartbeat register selection. Latch 48 bit Original time counter
+		// into Heartbeat register (this is undocumented)
+		bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0000);
+
+		// Rearm framesync for sync in pin
+		nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
+								    nco_6_register_value, false);
+		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+	}
+
+	mutex_unlock(&private->clock_lock);
+
+	return 0;
+}
+
+static int bcm54210pe_gettimex(struct ptp_clock_info *info,
+			       struct timespec64 *ts,
+			       struct ptp_system_timestamp *sts)
+{
+	struct bcm54210pe_ptp *ptp;
+
+	ptp = container_of(info, struct bcm54210pe_ptp, caps);
+
+	return bcm54210pe_get80bittime(ptp->chosen, ts, sts);
+}
+
+static int bcm54210pe_gettime(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+	return bcm54210pe_gettimex(info, ts, NULL);
+}
+
+static int bcm54210pe_get48bittime(struct bcm54210pe_private *private, u64 *timestamp)
+{
+	u16 nco_6_register_value;
+	int i, err;
+
+	struct phy_device *phydev = private->phydev;
+
+	// Capture timestamp on next framesync
+	nco_6_register_value = 0x2000;
+
+	mutex_lock(&private->clock_lock);
+
+	// Heartbeat register selection. Latch 48 bit Original time counter
+	// into Heartbeat register (this is undocumented)
+	err = bcm_phy_write_exp(phydev, DPLL_SELECT_REG, 0x0000);
+
+	// Amend to base register
+	nco_6_register_value =
+		bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+	// Set the NCO register
+	err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+	// Trigger framesync
+	err |= bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	for (i = 0; i < 5; i++) {
+		bcm54210pe_read48bittime_register(phydev, timestamp);
+
+		if (*timestamp != 0)
+			break;
+	}
+
+	mutex_unlock(&private->clock_lock);
+
+	return err;
+}
+
+static int bcm54210pe_settime(struct ptp_clock_info *info, const struct timespec64 *ts)
+{
+	u16 shadow_load_register, nco_6_register_value;
+	u16 original_time_codes[5], local_time_codes[3];
+	struct bcm54210pe_ptp *ptp;
+	struct phy_device *phydev;
+
+	ptp = container_of(info, struct bcm54210pe_ptp, caps);
+	phydev = ptp->chosen->phydev;
+
+	shadow_load_register = 0;
+	nco_6_register_value = 0;
+
+	// Assign original time codes (80 bit)
+	original_time_codes[4] = (u16)((ts->tv_sec & 0x0000FFFF00000000) >> 32);
+	original_time_codes[3] = (u16)((ts->tv_sec  & 0x00000000FFFF0000) >> 16);
+	original_time_codes[2] = (u16)(ts->tv_sec  & 0x000000000000FFFF);
+	original_time_codes[1] = (u16)((ts->tv_nsec & 0x00000000FFFF0000) >> 16);
+	original_time_codes[0] = (u16)(ts->tv_nsec & 0x000000000000FFFF);
+
+	// Assign original time codes (48 bit)
+	local_time_codes[2] = 0x4000;
+	local_time_codes[1] = (u16)(ts->tv_nsec >> 20);
+	local_time_codes[0] = (u16)(ts->tv_nsec >> 4);
+
+	// Set Time Code load bit in the shadow load register
+	shadow_load_register |= 0x0400;
+
+	// Set Local Time load bit in the shadow load register
+	shadow_load_register |= 0x0080;
+
+	mutex_lock(&ptp->chosen->clock_lock);
+
+	// Write Original Time Code Register
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_0, original_time_codes[0]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_1, original_time_codes[1]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_2, original_time_codes[2]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_3, original_time_codes[3]);
+	bcm_phy_write_exp(phydev, ORIGINAL_TIME_CODE_4, original_time_codes[4]);
+
+	// Write Local Time Code Register
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_0_REG, local_time_codes[0]);
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_1_REG, local_time_codes[1]);
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_2_REG, local_time_codes[2]);
+
+	// Write Shadow register
+	bcm_phy_write_exp(phydev, SHADOW_REG_CONTROL, 0x0000);
+	bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, shadow_load_register);
+
+	// Set global mode and nse_init
+	nco_6_register_value = bcm54210pe_get_base_nco6_reg(ptp->chosen,
+							    nco_6_register_value, true);
+
+	// Write to register
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+	// Trigger framesync
+	bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	// Set the second on set
+	ptp->chosen->second_on_set = ts->tv_sec;
+
+	mutex_unlock(&ptp->chosen->clock_lock);
+
+	return 0;
+}
+
+static int bcm54210pe_adjfine(struct ptp_clock_info *info, long scaled_ppm)
+{
+	int err;
+	u16 lo, hi;
+	u32 corrected_8ns_interval, base_8ns_interval;
+	bool negative;
+
+	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
+	struct phy_device *phydev = ptp->chosen->phydev;
+
+	negative = false;
+	if (scaled_ppm < 0) {
+		negative = true;
+		scaled_ppm = -scaled_ppm;
+	}
+
+	// This is not completely accurate but very fast
+	scaled_ppm >>= 7;
+
+	// Nominal counter increment is 8ns
+	base_8ns_interval = 1 << 31;
+
+	// Add or subtract differential
+	if (negative)
+		corrected_8ns_interval = base_8ns_interval - scaled_ppm;
+	else
+		corrected_8ns_interval = base_8ns_interval + scaled_ppm;
+
+	// Load up registers
+	hi = (corrected_8ns_interval & 0xFFFF0000) >> 16;
+	lo = (corrected_8ns_interval & 0x0000FFFF);
+
+	mutex_lock(&ptp->chosen->clock_lock);
+
+	// Set freq_mdio_sel to 1
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_2_2_REG, 0x4000);
+
+	// Load 125MHz frequency reqcntrl
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_1_MSB_REG, hi);
+	bcm_phy_write_exp(phydev, NSE_DPPL_NCO_1_LSB_REG, lo);
+
+	// On next framesync load freq from freqcntrl
+	bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x0040);
+
+	// Trigger framesync
+	err = bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	mutex_unlock(&ptp->chosen->clock_lock);
+
+	return err;
+}
+
+static int bcm54210pe_adjtime(struct ptp_clock_info *info, s64 delta)
+{
+	int err;
+	struct timespec64 ts;
+	u64 now;
+
+	err = bcm54210pe_gettime(info, &ts);
+	if (err < 0)
+		return err;
+
+	now = ktime_to_ns(timespec64_to_ktime(ts));
+	ts = ns_to_timespec64(now + delta);
+
+	err = bcm54210pe_settime(info, &ts);
+
+	return err;
+}
+
+static int bcm54210pe_extts_enable(struct bcm54210pe_private *private, int enable)
+{
+	int err;
+	struct phy_device *phydev;
+	u16 nco_6_register_value;
+
+	phydev = private->phydev;
+
+	if (enable) {
+		if (!private->extts_en) {
+			// Set enable per_out
+			private->extts_en = true;
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_4_REG, 0x0001);
+
+			nco_6_register_value = 0;
+			nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
+									    nco_6_register_value,
+									    false);
+
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_7_0_REG, 0x0100);
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_7_1_REG, 0x0200);
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+			schedule_delayed_work(&private->extts_ws, msecs_to_jiffies(1));
+		}
+
+	} else {
+		private->extts_en = false;
+		err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_4_REG, 0x0000);
+	}
+
+	return err;
+}
+
+static void bcm54210pe_run_extts_thread(struct work_struct *extts_ws)
+{
+	struct bcm54210pe_private *private;
+	struct phy_device *phydev;
+	u64 interval, time_stamp_48, time_stamp_80;
+
+	private = container_of((struct delayed_work *)extts_ws,
+			       struct bcm54210pe_private, extts_ws);
+	phydev = private->phydev;
+
+	interval = 10;	// in ms - long after we are gone from this earth, discussions will be had
+			// and songs will be sung about whether this interval is short enough....
+			// Before you complain let me say that in Timebeat.app up to ~150ms allows
+			// single digit ns servo accuracy. If your client / servo is not as cool:
+			// Do better :-)
+
+	mutex_lock(&private->clock_lock);
+
+	bcm54210pe_read80bittime_register(phydev, &time_stamp_80, &time_stamp_48);
+
+	if (private->last_extts_ts != time_stamp_48 &&
+	    private->last_immediate_ts[0] != time_stamp_48 &&
+	    private->last_immediate_ts[1] != time_stamp_80) {
+		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xE000);
+		bcm54210pe_trigger_extts_event(private, time_stamp_48);
+		bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, 0xE004);
+	}
+
+	mutex_unlock(&private->clock_lock);
+
+	// Do we need to reschedule
+	if (private->extts_en)
+		schedule_delayed_work(&private->extts_ws, msecs_to_jiffies(interval));
+}
+
+static int bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period,
+				    s64 pulsewidth, int enable)
+{
+	struct phy_device *phydev;
+	u16 nco_6_register_value, frequency_hi, frequency_lo,
+		pulsewidth_reg, pulse_start_hi, pulse_start_lo;
+	int err;
+
+	phydev = private->phydev;
+
+	if (enable) {
+		frequency_hi = 0;
+		frequency_lo = 0;
+		pulsewidth_reg = 0;
+		pulse_start_hi = 0;
+		pulse_start_lo = 0;
+
+		// Convert interval pulse spacing (period) and pulsewidth to 8 ns units
+		period /= 8;
+		pulsewidth /= 8;
+
+		// Mode 2 only: If pulsewidth is not explicitly set with PTP_PEROUT_DUTY_CYCLE
+		if (pulsewidth == 0) {
+			if (period < 2500) {
+				// At a frequency at less than 20us (2500 x 8ns) set
+				// pulse length to 1/10th of the interval pulse spacing
+				pulsewidth = period / 10;
+
+				// Where the interval pulse spacing is short,
+				// ensure we set a pulse length of 8ns
+				if (pulsewidth == 0)
+					pulsewidth = 1;
+
+			} else {
+				// Otherwise set pulse with to 4us (8ns x 500 = 4us)
+				pulsewidth = 500;
+			}
+		}
+
+		if (private->perout_mode == SYNC_OUT_MODE_1) {
+			// Set period
+			private->perout_period = period;
+
+			if (!private->perout_en) {
+				// Set enable per_out
+				private->perout_en = true;
+				schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(1));
+			}
+
+			err = 0;
+
+		} else if (private->perout_mode == SYNC_OUT_MODE_2) {
+			// Set enable per_out
+			private->perout_en = true;
+
+			// Calculate registers
+
+			// Lowest 16 bits of 8ns interval pulse spacing [15:0]
+			frequency_lo	= (u16)period;
+
+			// Highest 14 bits of 8ns interval pulse spacing [29:16]
+			frequency_hi	= (u16)(0x3FFF & (period >> 16));
+
+			// 2 lowest bits of 8ns pulse length [1:0]
+			frequency_hi   |= (u16)pulsewidth << 14;
+
+			// 7 highest bit  of 8 ns pulse length [8:2]
+			pulsewidth_reg	= (u16)(0x7F & (pulsewidth >> 2));
+
+			// Get base value
+			nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
+									    nco_6_register_value,
+									    true);
+
+			mutex_lock(&private->clock_lock);
+
+			// Write to register
+			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG,
+						nco_6_register_value);
+
+			// Set sync out pulse interval spacing and pulse length
+			err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_0_REG, frequency_lo);
+			err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_1_REG, frequency_hi);
+			err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_2_REG, pulsewidth_reg);
+
+			// On next framesync load sync out frequency
+			err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x0200);
+
+			// Trigger immediate framesync
+			err |= bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+			mutex_unlock(&private->clock_lock);
+		}
+	} else {
+		// Set disable pps
+		private->perout_en = false;
+
+		// Get base value
+		nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
+								    nco_6_register_value,
+								    false);
+
+		mutex_lock(&private->clock_lock);
+
+		// Write to register
+		err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
+
+		mutex_unlock(&private->clock_lock);
+	}
+
+	return err;
+}
+
+static void bcm54210pe_run_perout_mode_one_thread(struct work_struct *perout_ws)
+{
+	struct bcm54210pe_private *private;
+	u64 local_time_stamp_48bits; //, local_time_stamp_80bits;
+	u64 next_event, time_before_next_pulse, period;
+	u16 nco_6_register_value, pulsewidth_nco3_hack;
+	u64 wait_one, wait_two;
+
+	private = container_of((struct delayed_work *)perout_ws,
+			       struct bcm54210pe_private, perout_ws);
+	period = private->perout_period * 8;
+	pulsewidth_nco3_hack = 250; // The BCM chip is broken.
+				    // It does not respect this in sync out mode 1
+
+	nco_6_register_value = 0;
+
+	// Get base value
+	nco_6_register_value = bcm54210pe_get_base_nco6_reg(private, nco_6_register_value, false);
+
+	// Get 48 bit local time
+	bcm54210pe_get48bittime(private, &local_time_stamp_48bits);
+
+	// Calculate time before next event and next event time
+	time_before_next_pulse =  period - (local_time_stamp_48bits % period);
+	next_event = local_time_stamp_48bits + time_before_next_pulse;
+
+	// Lock
+	mutex_lock(&private->clock_lock);
+
+	// Set pulsewidth (test reveal this does not work),
+	// but registers need content or no pulse will exist
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_3_1_REG, pulsewidth_nco3_hack << 14);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_3_2_REG, pulsewidth_nco3_hack >> 2);
+
+	// Set sync out pulse interval spacing and pulse length
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_0_REG, next_event & 0xFFF0);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_1_REG, next_event >> 16);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_2_REG, next_event >> 32);
+
+	// On next framesync load sync out frequency
+	bcm_phy_write_exp(private->phydev, SHADOW_REG_LOAD, 0x0200);
+
+	// Write to register with mode one set for sync out
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value | 0x0001);
+
+	// Trigger immediate framesync
+	bcm_phy_modify_exp(private->phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	// Unlock
+	mutex_unlock(&private->clock_lock);
+
+	// Wait until 1/10 period after the next pulse
+	wait_one = (time_before_next_pulse / 1000000) + (period / 1000000 / 10);
+	mdelay(wait_one);
+
+	// Lock
+	mutex_lock(&private->clock_lock);
+
+	// Clear pulse by bumping sync_out_match to max (this pulls sync out down)
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_0_REG, 0xFFF0);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_1_REG, 0xFFFF);
+	bcm_phy_write_exp(private->phydev, NSE_DPPL_NCO_5_2_REG, 0xFFFF);
+
+	// On next framesync load sync out frequency
+	bcm_phy_write_exp(private->phydev, SHADOW_REG_LOAD, 0x0200);
+
+	// Trigger immediate framesync
+	bcm_phy_modify_exp(private->phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
+
+	// Unlock
+	mutex_unlock(&private->clock_lock);
+
+	// Calculate wait before we reschedule the next pulse
+	wait_two = (period / 1000000) - (2 * (period / 10000000));
+
+	// Do we need to reschedule
+	if (private->perout_en)
+		schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(wait_two));
+}
+
+bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
+{
+	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private,
+							  mii_ts);
+
+	if (private->hwts_rx_en) {
+		skb_queue_tail(&private->rx_skb_queue, skb);
+		schedule_work(&private->rxts_work);
+		return true;
+	}
+
+	return false;
+}
+
+void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
+{
+	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private,
+							  mii_ts);
+
+	switch (private->hwts_tx_en) {
+	case HWTSTAMP_TX_ON:
+	{
+		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		skb_queue_tail(&private->tx_skb_queue, skb);
+		schedule_work(&private->txts_work);
+		break;
+	}
+
+	case HWTSTAMP_TX_OFF:
+	{
+	}
+
+	default:
+	{
+		kfree_skb(skb);
+		break;
+	}
+	}
+}
+
+int bcm54210pe_ts_info(struct mii_timestamper *mii_ts, struct ethtool_ts_info *info)
+{
+	struct bcm54210pe_private *bcm54210pe = container_of(mii_ts, struct bcm54210pe_private,
+							     mii_ts);
+
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_HARDWARE |
+		SOF_TIMESTAMPING_RX_HARDWARE |
+		SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	info->phc_index = ptp_clock_index(bcm54210pe->ptp->ptp_clock);
+	info->tx_types =
+		(1 << HWTSTAMP_TX_OFF) |
+		(1 << HWTSTAMP_TX_ON);
+	info->rx_filters =
+		(1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT);
+	return 0;
+}
+
+int bcm54210pe_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
+{
+	struct bcm54210pe_private *device = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
+
+	struct hwtstamp_config cfg;
+
+	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
+		return -EFAULT;
+
+	if (cfg.flags) /* reserved for future extensions */
+		return -EINVAL;
+
+	if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
+		return -ERANGE;
+
+	device->hwts_tx_en = cfg.tx_type;
+
+	switch (cfg.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		device->hwts_rx_en = 0;
+		device->layer = 0;
+		device->version = 0;
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L4;
+		device->version = PTP_CLASS_V1;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L4;
+		device->version = PTP_CLASS_V2;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L2;
+		device->version = PTP_CLASS_V2;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+		device->hwts_rx_en = 1;
+		device->layer = PTP_CLASS_L4 | PTP_CLASS_L2;
+		device->version = PTP_CLASS_V2;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+}
+
+static int bcm54210pe_feature_enable(struct ptp_clock_info *info,
+				     struct ptp_clock_request *req, int on)
+{
+	struct bcm54210pe_ptp *ptp = container_of(info, struct bcm54210pe_ptp, caps);
+	s64 period, pulsewidth;
+	struct timespec64 ts;
+
+	switch (req->type) {
+	case PTP_CLK_REQ_PEROUT:
+
+		period = 0;
+		pulsewidth = 0;
+
+		// Check if pin func is set correctly
+		if (ptp->chosen->sdp_config[SYNC_OUT_PIN].func != PTP_PF_PEROUT)
+			return -EOPNOTSUPP;
+
+		// No other flags supported
+		if (req->perout.flags & ~PTP_PEROUT_DUTY_CYCLE)
+			return -EOPNOTSUPP;
+
+		// Check if a specific pulsewidth is set
+		if ((req->perout.flags & PTP_PEROUT_DUTY_CYCLE) > 0) {
+			if (ptp->chosen->perout_mode == SYNC_OUT_MODE_1)
+				return -EOPNOTSUPP;
+
+			// Extract pulsewidth
+			ts.tv_sec = req->perout.on.sec;
+			ts.tv_nsec = req->perout.on.nsec;
+			pulsewidth = timespec64_to_ns(&ts);
+
+			// 9 bits in 8ns units, so max = 4,088ns
+			if (pulsewidth > 511 * 8)
+				return -ERANGE;
+		}
+
+		// Extract pulse spacing interval (period)
+		ts.tv_sec = req->perout.period.sec;
+		ts.tv_nsec = req->perout.period.nsec;
+		period = timespec64_to_ns(&ts);
+
+		// 16ns is minimum pulse spacing interval (a value of
+		// 16 will result in 8ns high followed by 8 ns low)
+		if (period != 0 && period < 16)
+			return -ERANGE;
+
+		return bcm54210pe_perout_enable(ptp->chosen, period, pulsewidth, on);
+
+	case PTP_CLK_REQ_EXTTS:
+
+		if (ptp->chosen->sdp_config[SYNC_IN_PIN].func != PTP_PF_EXTTS)
+			return -EOPNOTSUPP;
+
+		return bcm54210pe_extts_enable(ptp->chosen, on);
+
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int bcm54210pe_ptp_verify_pin(struct ptp_clock_info *info, unsigned int pin,
+				     enum ptp_pin_function func, unsigned int chan)
+{
+	switch (func) {
+	case PTP_PF_NONE:
+		return 0;
+	case PTP_PF_EXTTS:
+		if (pin == SYNC_IN_PIN)
+			return 0;
+		break;
+	case PTP_PF_PEROUT:
+		if (pin == SYNC_OUT_PIN)
+			return 0;
+		break;
+	case PTP_PF_PHYSYNC:
+		break;
+	}
+	return -1;
+}
+
+static const struct ptp_clock_info bcm54210pe_clk_caps = {
+	.owner		= THIS_MODULE,
+	.name		= "BCM54210PE_PHC",
+	.max_adj	= 100000000,
+	.n_alarm	= 0,
+	.n_pins		= 2,
+	.n_ext_ts	= 1,
+	.n_per_out	= 1,
+	.pps		= 0,
+	.adjtime	= &bcm54210pe_adjtime,
+	.adjfine	= &bcm54210pe_adjfine,
+	.gettime64	= &bcm54210pe_gettime,
+	.gettimex64	= &bcm54210pe_gettimex,
+	.settime64	= &bcm54210pe_settime,
+	.enable		= &bcm54210pe_feature_enable,
+	.verify		= &bcm54210pe_ptp_verify_pin,
+};
+
+static int bcm54210pe_sw_reset(struct phy_device *phydev)
+{
+	u16 err;
+	u16 aux;
+
+	err =  bcm_phy_write_exp(phydev, EXT_SOFTWARE_RESET, EXT_RESET1);
+	err |= bcm_phy_read_exp(phydev, EXT_ENABLE_REG1);
+
+	if (err < 0)
+		return err;
+
+	err |= bcm_phy_write_exp(phydev, EXT_SOFTWARE_RESET, EXT_RESET2);
+	aux = bcm_phy_read_exp(phydev, EXT_SOFTWARE_RESET);
+	return err;
+}
+
+int bcm54210pe_probe(struct phy_device *phydev)
+{
+	int x, y;
+	struct bcm54210pe_ptp *ptp;
+	struct bcm54210pe_private *bcm54210pe;
+	struct ptp_pin_desc *sync_in_pin_desc, *sync_out_pin_desc;
+
+	bcm54210pe_sw_reset(phydev);
+	bcm54210pe_config_1588(phydev);
+
+	bcm54210pe = kzalloc(sizeof(*bcm54210pe), GFP_KERNEL);
+	if (!bcm54210pe)
+		return -ENOMEM;
+
+	ptp = kzalloc(sizeof(*ptp), GFP_KERNEL);
+	if (!ptp)
+		return -ENOMEM;
+
+	bcm54210pe->phydev = phydev;
+	bcm54210pe->ptp = ptp;
+
+	bcm54210pe->mii_ts.rxtstamp = bcm54210pe_rxtstamp;
+	bcm54210pe->mii_ts.txtstamp = bcm54210pe_txtstamp;
+	bcm54210pe->mii_ts.hwtstamp = bcm54210pe_hwtstamp;
+	bcm54210pe->mii_ts.ts_info  = bcm54210pe_ts_info;
+
+	phydev->mii_ts = &bcm54210pe->mii_ts;
+
+	// Initialisation of work_structs and similar
+	INIT_WORK(&bcm54210pe->txts_work, bcm54210pe_run_tx_timestamp_match_thread);
+	INIT_WORK(&bcm54210pe->rxts_work, bcm54210pe_run_rx_timestamp_match_thread);
+	INIT_DELAYED_WORK(&bcm54210pe->perout_ws, bcm54210pe_run_perout_mode_one_thread);
+	INIT_DELAYED_WORK(&bcm54210pe->extts_ws, bcm54210pe_run_extts_thread);
+
+	// SKB queues
+	skb_queue_head_init(&bcm54210pe->tx_skb_queue);
+	skb_queue_head_init(&bcm54210pe->rx_skb_queue);
+
+	for (x = 0; x < CIRCULAR_BUFFER_COUNT; x++) {
+		INIT_LIST_HEAD(&bcm54210pe->circular_buffers[x]);
+
+		for (y = 0; y < CIRCULAR_BUFFER_ITEM_COUNT; y++)
+			list_add(&bcm54210pe->circular_buffer_items[x][y].list,
+				 &bcm54210pe->circular_buffers[x]);
+	}
+
+	// Caps
+	memcpy(&bcm54210pe->ptp->caps, &bcm54210pe_clk_caps, sizeof(bcm54210pe_clk_caps));
+	bcm54210pe->ptp->caps.pin_config = bcm54210pe->sdp_config;
+
+	// Mutex
+	mutex_init(&bcm54210pe->clock_lock);
+	mutex_init(&bcm54210pe->timestamp_buffer_lock);
+
+	// Features
+	bcm54210pe->one_step = false;
+	bcm54210pe->extts_en = false;
+	bcm54210pe->perout_en = false;
+	bcm54210pe->perout_mode = SYNC_OUT_MODE_1;
+
+	// Fibonacci RSewoke style progressive backoff scheme
+	bcm54210pe->fib_sequence[0] = 1;
+	bcm54210pe->fib_sequence[1] = 1;
+	bcm54210pe->fib_sequence[2] = 2;
+	bcm54210pe->fib_sequence[3] = 3;
+	bcm54210pe->fib_sequence[4] = 5;
+	bcm54210pe->fib_sequence[5] = 8;
+	bcm54210pe->fib_sequence[6] = 13;
+	bcm54210pe->fib_sequence[7] = 21;
+	bcm54210pe->fib_sequence[8] = 34;
+	bcm54210pe->fib_sequence[9] = 55;
+
+	//bcm54210pe->fib_sequence = {1, 1, 2, 3, 5, 8, 13, 21, 34, 55};
+	bcm54210pe->fib_factor_rx = 10;
+	bcm54210pe->fib_factor_tx = 10;
+
+	// Pin descriptions
+	sync_in_pin_desc = &bcm54210pe->sdp_config[SYNC_IN_PIN];
+	snprintf(sync_in_pin_desc->name, sizeof(sync_in_pin_desc->name), "SYNC_IN");
+	sync_in_pin_desc->index = SYNC_IN_PIN;
+	sync_in_pin_desc->func = PTP_PF_NONE;
+
+	sync_out_pin_desc = &bcm54210pe->sdp_config[SYNC_OUT_PIN];
+	snprintf(sync_out_pin_desc->name, sizeof(sync_out_pin_desc->name), "SYNC_OUT");
+	sync_out_pin_desc->index = SYNC_OUT_PIN;
+	sync_out_pin_desc->func = PTP_PF_NONE;
+
+	ptp->chosen = bcm54210pe;
+	phydev->priv = bcm54210pe;
+	ptp->caps.owner = THIS_MODULE;
+
+	bcm54210pe->ptp->ptp_clock = ptp_clock_register(&bcm54210pe->ptp->caps, &phydev->mdio.dev);
+
+	if (IS_ERR(bcm54210pe->ptp->ptp_clock))
+		return PTR_ERR(bcm54210pe->ptp->ptp_clock);
+
+	return 0;
+}
diff --git a/drivers/net/phy/bcm54210pe_ptp.h b/drivers/net/phy/bcm54210pe_ptp.h
new file mode 100644
index 0000000000000..f43e1a4ddbd3d
--- /dev/null
+++ b/drivers/net/phy/bcm54210pe_ptp.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0+
+ *
+ * IEEE1588 (PTP), perout and extts for BCM54210PE PHY
+ *
+ * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
+ * License: GPL
+ */
+
+#include <linux/list.h>
+#include <linux/ptp_clock_kernel.h>
+
+#define CIRCULAR_BUFFER_COUNT		8
+#define CIRCULAR_BUFFER_ITEM_COUNT	32
+
+#define SYNC_IN_PIN			0
+#define SYNC_OUT_PIN			1
+
+#define SYNC_OUT_MODE_1			1
+#define SYNC_OUT_MODE_2			2
+
+#define DIRECTION_RX			0
+#define DIRECTION_TX			1
+
+#define INTC_FSYNC			1
+#define INTC_SOP			2
+
+struct bcm54210pe_ptp {
+	struct ptp_clock_info caps;
+	struct ptp_clock *ptp_clock;
+	struct bcm54210pe_private *chosen;
+};
+
+struct bcm54210pe_circular_buffer_item {
+	struct list_head list;
+
+	u8 msg_type;
+	u16 sequence_id;
+	u64 time_stamp;
+	bool is_valid;
+};
+
+struct bcm54210pe_private {
+	struct phy_device *phydev;
+	struct bcm54210pe_ptp *ptp;
+	struct mii_timestamper mii_ts;
+	struct ptp_pin_desc sdp_config[2];
+
+	int ts_tx_config;
+	int tx_rx_filter;
+
+	bool one_step;
+	bool perout_en;
+	bool extts_en;
+
+	int second_on_set;
+
+	int perout_mode;
+	int perout_period;
+	int perout_pulsewidth;
+
+	u64 last_extts_ts;
+	u64 last_immediate_ts[2];
+
+	struct sk_buff_head tx_skb_queue;
+	struct sk_buff_head rx_skb_queue;
+
+	struct bcm54210pe_circular_buffer_item
+		circular_buffer_items[CIRCULAR_BUFFER_COUNT]
+				     [CIRCULAR_BUFFER_ITEM_COUNT];
+	struct list_head circular_buffers[CIRCULAR_BUFFER_COUNT];
+
+	struct work_struct txts_work, rxts_work;
+	struct delayed_work perout_ws, extts_ws;
+	struct mutex clock_lock, timestamp_buffer_lock;
+
+	int fib_sequence[10];
+
+	int fib_factor_rx;
+	int fib_factor_tx;
+
+	int hwts_tx_en;
+	int hwts_rx_en;
+	int layer;
+	int version;
+};
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 8b0ac38742d06..0aba0f08eb49d 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -15,6 +15,11 @@
 #include <linux/phy.h>
 #include <linux/brcmphy.h>
 #include <linux/of.h>
+#include <linux/irq.h>
+
+#if IS_ENABLED(CONFIG_BCM54120PE_PHY)
+extern int bcm54210pe_probe(struct phy_device *phydev);
+#endif
 
 #define BRCM_PHY_MODEL(phydev) \
 	((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)
@@ -778,7 +783,20 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
-}, {
+},
+
+#if IS_ENABLED(CONFIG_BCM54120PE_PHY)
+{
+	.phy_id		= PHY_ID_BCM54213PE,
+	.phy_id_mask	= 0xffffffff,
+	.name		= "Broadcom BCM54210PE",
+	/* PHY_GBIT_FEATURES */
+	.config_init	= bcm54xx_config_init,
+	.ack_interrupt	= bcm_phy_ack_intr,
+	.config_intr	= bcm_phy_config_intr,
+	.probe		= bcm54210pe_probe,
+#elif
+{
 	.phy_id		= PHY_ID_BCM54213PE,
 	.phy_id_mask	= 0xffffffff,
 	.name		= "Broadcom BCM54213PE",
@@ -786,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+#endif
 }, {
 	.phy_id		= PHY_ID_BCM5461,
 	.phy_id_mask	= 0xfffffff0,
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 3e377f3c69e5d..586f9143311b4 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -87,6 +87,23 @@ config PTP_1588_CLOCK_INES
 	  core.  This clock is only useful if the MII bus of your MAC
 	  is wired up to the core.
 
+ config BCM54120PE_PHY
+	tristate "Add support for ptp in bcm54210pe PHYs"
+	depends on NETWORK_PHY_TIMESTAMPING
+	depends on PHYLIB
+	depends on PTP_1588_CLOCK
+	depends on BCM_NET_PHYLIB
+        select NET_PTP_CLASSIFY
+	help
+	  This driver adds support for using the BCM54210PE as a PTP
+	  clock. This clock is only useful if your PTP programs are
+	  getting hardware time stamps on the PTP Ethernet packets
+	  using the SO_TIMESTAMPING API.
+
+	  In order for this to work, your MAC driver must also
+	  implement the skb_tx_timestamp() function.
+
+
 config PTP_1588_CLOCK_PCH
 	tristate "Intel PCH EG20T as PTP clock"
 	depends on X86_32 || COMPILE_TEST

[-- Attachment #3: Type: text/plain, Size: 3 bytes --]





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

* Re: [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe
  2022-04-22 15:08   ` [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe Lasse Johnsen
@ 2022-04-22 15:22     ` Jonathan Lemon
  2022-04-22 18:11       ` Lasse Johnsen
  2022-04-22 15:42     ` Andrew Lunn
  1 sibling, 1 reply; 15+ messages in thread
From: Jonathan Lemon @ 2022-04-22 15:22 UTC (permalink / raw)
  To: Lasse Johnsen
  Cc: Richard Cochran, netdev, Gordon Hollingworth, Ahmad Byagowi,
	Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King,
	bcm-kernel-feedback-list, David S. Miller, Jakub Kicinski,
	Paolo Abeni

On Fri, Apr 22, 2022 at 04:08:18PM +0100, Lasse Johnsen wrote:
> > On 21 Apr 2022, at 15:48, Richard Cochran <richardcochran@gmail.com> wrote:
> > Moreover: Does this device provide in-band Rx time stamps?  If so, why
> > not use them?
> 
> This is the first generation PHY and it does not do in-band RX. I asked BCM and studied the documentation. I’m sure I’m allowed to say, that the second generation 40nm BCM PHY (which - "I am not making this up" is available in 3 versions: BCM54210, BCM54210S and BCM54210SE - not “PE”) - supports in-band rx timestamps. However, as a matter of curiosity, BCM utilise the field in the header now used for minor versioning in 1588-2019, so in due course using this silicon feature will be a significant challenge.

Actually, it does support in-band RX timestamps.  Doing this would be
cleaner, and you'd only need to capture TX timestamps.
-- 
Jonathan

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

* Re: [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe
  2022-04-22 15:08   ` [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe Lasse Johnsen
  2022-04-22 15:22     ` Jonathan Lemon
@ 2022-04-22 15:42     ` Andrew Lunn
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2022-04-22 15:42 UTC (permalink / raw)
  To: Lasse Johnsen
  Cc: Richard Cochran, netdev, Gordon Hollingworth, Ahmad Byagowi,
	Florian Fainelli, Heiner Kallweit, Russell King,
	bcm-kernel-feedback-list, David S. Miller, Jakub Kicinski,
	Paolo Abeni

On Fri, Apr 22, 2022 at 04:08:18PM +0100, Lasse Johnsen wrote:

Hi Lasse

Don't attach a patch to the end of a discussion. What you email is
what comes out of git-format patch. Nothing added.

If you want to discuss review comments, just reply to the email with
the comments. 

> From 3fcbbac9fe85909877f15d95f7a1e64dd6d06ab7 Mon Sep 17 00:00:00 2001
> From: Lasse Johnsen <l@ssejohnsen.me>
> Date: Sat, 5 Feb 2022 09:34:19 -0500
> Subject: [PATCH] Added support for IEEE1588 timestamping for the BCM54210PE
>  PHY using the kernel mii_timestamper interface
> 
> ---
>  arch/arm/configs/bcm2711_defconfig            |    1 +
>  arch/arm64/configs/bcm2711_defconfig          |    1 +
>  .../net/ethernet/broadcom/genet/bcmgenet.c    |    8 +-
>  drivers/net/phy/Makefile                      |    1 +
>  drivers/net/phy/bcm54210pe_ptp.c              | 1382 +++++++++++++++++
>  drivers/net/phy/bcm54210pe_ptp.h              |   85 +
>  drivers/net/phy/broadcom.c                    |   21 +-
>  drivers/ptp/Kconfig                           |   17 +

You need to break this up into a patch series. You probably want the
following patches:

defconfig changes
The core ptp code, in library form
Extensions to drivers/net/phy/broadcom.c to use the new code
bcmgenet.c change.


> +static u64 four_u16_to_ns(u16 *four_u16)
> +{
> +	u32 seconds;
> +	u32 nanoseconds;
> +	struct timespec64 ts;
> +	u16 *ptr;

Now it has been through checkpatch, it is starting to look like Linux
code :-)

Network drivers have a few extra code style hoops to jump
through. Variables should be sorted longest to shortest. So you want:

> +	struct timespec64 ts;
> +	u32 nanoseconds;
> +	u32 seconds;
> +	u16 *ptr;

This is known as reverse Christmas tree.

> +static int bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en)

Although Linux as a whole allows 100 character lines, networking
mostly stays with 80. I'm not sure it is strictly enforced, but it is
a good idea to try to keep with it.

> +{
> +	u16 interrupt_mask;
> +
> +	interrupt_mask = 0;

You can combine these into one line.

> +static int bcm54210pe_get80bittime(struct bcm54210pe_private *private,
> +				   struct timespec64 *ts,
> +				   struct ptp_system_timestamp *sts)
> +{
> +	struct phy_device *phydev;
> +	u16 nco_6_register_value;
> +	int i;
> +	u64 time_stamp_48, time_stamp_80, control_ts;
> +
> +	phydev = private->phydev;
> +
> +	// Capture timestamp on next framesync
> +	nco_6_register_value = 0x2000;

You should not have magic numbers. Please add a #define. If the
#define has a sensible name, it should then be obvious you are
capturing a timestamp on the next frame sync and so you don't need the
comment.

> +
> +	// Lock
> +	mutex_lock(&private->clock_lock);

Comments like this don't add any value. I can see it is a lock because
you are calling mutex_lock.

> +static int bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period,
> +				    s64 pulsewidth, int enable)
> +{
> +	struct phy_device *phydev;
> +	u16 nco_6_register_value, frequency_hi, frequency_lo,
> +		pulsewidth_reg, pulse_start_hi, pulse_start_lo;
> +	int err;
> +
> +	phydev = private->phydev;
> +
> +	if (enable) {
> +		frequency_hi = 0;
> +		frequency_lo = 0;
> +		pulsewidth_reg = 0;
> +		pulse_start_hi = 0;
> +		pulse_start_lo = 0;
> +
> +		// Convert interval pulse spacing (period) and pulsewidth to 8 ns units
> +		period /= 8;
> +		pulsewidth /= 8;
> +
> +		// Mode 2 only: If pulsewidth is not explicitly set with PTP_PEROUT_DUTY_CYCLE
> +		if (pulsewidth == 0) {
> +			if (period < 2500) {
> +				// At a frequency at less than 20us (2500 x 8ns) set
> +				// pulse length to 1/10th of the interval pulse spacing
> +				pulsewidth = period / 10;
> +
> +				// Where the interval pulse spacing is short,
> +				// ensure we set a pulse length of 8ns
> +				if (pulsewidth == 0)
> +					pulsewidth = 1;
> +
> +			} else {
> +				// Otherwise set pulse with to 4us (8ns x 500 = 4us)
> +				pulsewidth = 500;
> +			}
> +		}
> +
> +		if (private->perout_mode == SYNC_OUT_MODE_1) {
> +			// Set period
> +			private->perout_period = period;
> +
> +			if (!private->perout_en) {
> +				// Set enable per_out
> +				private->perout_en = true;
> +				schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(1));
> +			}
> +
> +			err = 0;
> +
> +		} else if (private->perout_mode == SYNC_OUT_MODE_2) {
> +			// Set enable per_out
> +			private->perout_en = true;
> +
> +			// Calculate registers
> +
> +			// Lowest 16 bits of 8ns interval pulse spacing [15:0]
> +			frequency_lo	= (u16)period;
> +
> +			// Highest 14 bits of 8ns interval pulse spacing [29:16]
> +			frequency_hi	= (u16)(0x3FFF & (period >> 16));
> +
> +			// 2 lowest bits of 8ns pulse length [1:0]
> +			frequency_hi   |= (u16)pulsewidth << 14;
> +
> +			// 7 highest bit  of 8 ns pulse length [8:2]
> +			pulsewidth_reg	= (u16)(0x7F & (pulsewidth >> 2));
> +
> +			// Get base value
> +			nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
> +									    nco_6_register_value,
> +									    true);
> +
> +			mutex_lock(&private->clock_lock);
> +
> +			// Write to register
> +			err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG,
> +						nco_6_register_value);
> +
> +			// Set sync out pulse interval spacing and pulse length
> +			err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_0_REG, frequency_lo);
> +			err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_1_REG, frequency_hi);
> +			err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_2_REG, pulsewidth_reg);
> +
> +			// On next framesync load sync out frequency
> +			err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x0200);
> +
> +			// Trigger immediate framesync
> +			err |= bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> +			mutex_unlock(&private->clock_lock);
> +		}
> +	} else {
> +		// Set disable pps
> +		private->perout_en = false;
> +
> +		// Get base value
> +		nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
> +								    nco_6_register_value,
> +								    false);
> +
> +		mutex_lock(&private->clock_lock);
> +
> +		// Write to register
> +		err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> +		mutex_unlock(&private->clock_lock);
> +	}
> +
> +	return err;

This is a pretty big function, and its indentation gets pretty deep. The coding style:

https://www.kernel.org/doc/html/latest/process/coding-style.html#functions

says:

Functions should be short and sweet, and do just one thing. They
should fit on one or two screenfuls of text (the ISO/ANSI screen size
is 80x24, as we all know), and do one thing and do that well.

Maybe you can break this up into helpers.

> +void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
> +{
> +	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private,
> +							  mii_ts);
> +
> +	switch (private->hwts_tx_en) {
> +	case HWTSTAMP_TX_ON:
> +	{
> +		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +		skb_queue_tail(&private->tx_skb_queue, skb);
> +		schedule_work(&private->txts_work);
> +		break;
> +	}
> +
> +	case HWTSTAMP_TX_OFF:
> +	{
> +	}

That just looks odd.

Should there be a break? Do you want to fall through? If you do want
to fall through, you need to mark it so. But since it is empty, i
guess you don't want to fall through?

> +
> +	default:
> +	{
> +		kfree_skb(skb);
> +		break;
> +	}
> +	}
> +}

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

* Re: [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe
  2022-04-22 15:22     ` Jonathan Lemon
@ 2022-04-22 18:11       ` Lasse Johnsen
  2022-04-22 18:20         ` Jonathan Lemon
  0 siblings, 1 reply; 15+ messages in thread
From: Lasse Johnsen @ 2022-04-22 18:11 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: Richard Cochran, netdev, Gordon Hollingworth, Ahmad Byagowi,
	Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King,
	bcm-kernel-feedback-list, David S. Miller, Jakub Kicinski,
	Paolo Abeni

Hi Jonathan,

I suspect you make the conflation I also made when I started working on this PHY driver. Broadcom has a number of different, nearly identical chips. The BCM54210, the BCM54210E, the BCM54210PE, the BCM54210S and the BCM54210SE.

It’s hard to imagine, but only the BCM54210PE is a first generation PHY and the BCM54210 (and others) are second generation. I have to be mighty careful not to breach my NDA, but I can furnish you with these quotes directly from the Broadcom engineers I worked with during the development:

24 March:

"The BCM54210PE is the first-gen 40-nm GPHY, but the BCM54210 is the second-gen 40-nm GPHY.”

"The 1588 Inband function only applied to BCM54210 or later PHYs. It doesn't be supported in the BCM54210PE”

So, I quite agree with you that in-band would be preferable (subject to the issue with hawking the reserved field used in 1588-2019 I described in my note to Richard), but I am convinced that it is not supported in the BCM54210PE. Indeed if you are looking at a document describing features based on the RDB register access method it is not supported by the BCM54210PE.

I would like nothing better than to be wrong, but you will need to provide me with something substantial to investigate further. (Offline is NDA requires it - happy to discuss any time).

In any event, I’m sure the time is not wasted and will be relevant when the Raspberry PI CM5,6&7 is launched… :-)

Thank you for your note and all the best,

Lasse

> On 22 Apr 2022, at 16:22, Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
> 
> On Fri, Apr 22, 2022 at 04:08:18PM +0100, Lasse Johnsen wrote:
>>> On 21 Apr 2022, at 15:48, Richard Cochran <richardcochran@gmail.com> wrote:
>>> Moreover: Does this device provide in-band Rx time stamps?  If so, why
>>> not use them?
>> 
>> This is the first generation PHY and it does not do in-band RX. I asked BCM and studied the documentation. I’m sure I’m allowed to say, that the second generation 40nm BCM PHY (which - "I am not making this up" is available in 3 versions: BCM54210, BCM54210S and BCM54210SE - not “PE”) - supports in-band rx timestamps. However, as a matter of curiosity, BCM utilise the field in the header now used for minor versioning in 1588-2019, so in due course using this silicon feature will be a significant challenge.
> 
> Actually, it does support in-band RX timestamps.  Doing this would be
> cleaner, and you'd only need to capture TX timestamps.
> -- 
> Jonathan


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

* Re: [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe
  2022-04-22 18:11       ` Lasse Johnsen
@ 2022-04-22 18:20         ` Jonathan Lemon
  2022-04-23 10:26           ` Lasse Johnsen
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Lemon @ 2022-04-22 18:20 UTC (permalink / raw)
  To: Lasse Johnsen
  Cc: Richard Cochran, netdev, Gordon Hollingworth, Ahmad Byagowi,
	Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King,
	bcm-kernel-feedback-list, David S. Miller, Jakub Kicinski,
	Paolo Abeni

On 22 Apr 2022, at 11:11, Lasse Johnsen wrote:

> Hi Jonathan,
>
> I suspect you make the conflation I also made when I started working on this PHY driver. Broadcom has a number of different, nearly identical chips. The BCM54210, the BCM54210E, the BCM54210PE, the BCM54210S and the BCM54210SE.
>
> It’s hard to imagine, but only the BCM54210PE is a first generation PHY and the BCM54210 (and others) are second generation. I have to be mighty careful not to breach my NDA, but I can furnish you with these quotes directly from the Broadcom engineers I worked with during the development:
>
> 24 March:
>
> "The BCM54210PE is the first-gen 40-nm GPHY, but the BCM54210 is the second-gen 40-nm GPHY.”
>
> "The 1588 Inband function only applied to BCM54210 or later PHYs. It doesn't be supported in the BCM54210PE”
>
> So, I quite agree with you that in-band would be preferable (subject to the issue with hawking the reserved field used in 1588-2019 I described in my note to Richard), but I am convinced that it is not supported in the BCM54210PE. Indeed if you are looking at a document describing features based on the RDB register access method it is not supported by the BCM54210PE.

Uhm, I have inbound timestamps working for RX on an RPI CM4.
—
Jonathan

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

* Re: [PATCH net-next] 1588 support on bcm54210pe
  2022-04-22 15:00     ` Andrew Lunn
@ 2022-04-22 19:48       ` Richard Cochran
  2022-04-23 14:40         ` Florian Fainelli
  0 siblings, 1 reply; 15+ messages in thread
From: Richard Cochran @ 2022-04-22 19:48 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Lasse Johnsen, netdev, Gordon Hollingworth, Ahmad Byagowi,
	Heiner Kallweit, Russell King, bcm-kernel-feedback-list,
	Florian Fainelli

On Fri, Apr 22, 2022 at 05:00:07PM +0200, Andrew Lunn wrote:

> > I am confident that this code is relevant exclusively to the
> > BCM54210PE.

Not true.

> It will not even work with the BCM54210, BCM54210S and
> > BCM54210SE PHYs.

The registers you used are also present in the BCM541xx devices.
Pretty sure your code would work on those devices (after adjusting
register offsets).

> Florian can probably tell us more, but often hardware like this is
> shared by multiple devices. If it is, you might want to use a more
> generic prefix.

My understanding is that there are two implementions, gen1 and gen2.
Your bcm542xx and the bcm541xx are both gen1, and both support inband
Rx time stamping.

Because the registers are all the same (just the offsets are
different), I'd like to see a common module that can be used by all
gen1 devices.  The module could be named bcm-ptp-gen1.c for example.

Thanks,
Richard

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

* Re: [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe
  2022-04-22 18:20         ` Jonathan Lemon
@ 2022-04-23 10:26           ` Lasse Johnsen
  0 siblings, 0 replies; 15+ messages in thread
From: Lasse Johnsen @ 2022-04-23 10:26 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: Richard Cochran, netdev, Gordon Hollingworth, Ahmad Byagowi,
	Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King,
	bcm-kernel-feedback-list, David S. Miller, Jakub Kicinski,
	Paolo Abeni

I have reviewed the documentation again, and I am inclined to agree with you. I will modify driver to use in-band RX timestamps. It looks like TX timestamps can also be updated on the fly to allow for one-step operation.


> On 22 Apr 2022, at 19:20, Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
> 
> Uhm, I have inbound timestamps working for RX on an RPI CM4.


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

* Re: [PATCH net-next] 1588 support on bcm54210pe
  2022-04-22 19:48       ` Richard Cochran
@ 2022-04-23 14:40         ` Florian Fainelli
  2022-04-23 18:16           ` Richard Cochran
  0 siblings, 1 reply; 15+ messages in thread
From: Florian Fainelli @ 2022-04-23 14:40 UTC (permalink / raw)
  To: Richard Cochran, Andrew Lunn
  Cc: Lasse Johnsen, netdev, Gordon Hollingworth, Ahmad Byagowi,
	Heiner Kallweit, Russell King, bcm-kernel-feedback-list,
	Florian Fainelli



On 4/22/2022 12:48 PM, Richard Cochran wrote:
> On Fri, Apr 22, 2022 at 05:00:07PM +0200, Andrew Lunn wrote:
> 
>>> I am confident that this code is relevant exclusively to the
>>> BCM54210PE.
> 
> Not true.
> 
>> It will not even work with the BCM54210, BCM54210S and
>>> BCM54210SE PHYs.
> 
> The registers you used are also present in the BCM541xx devices.
> Pretty sure your code would work on those devices (after adjusting
> register offsets).
> 
>> Florian can probably tell us more, but often hardware like this is
>> shared by multiple devices. If it is, you might want to use a more
>> generic prefix.
> 
> My understanding is that there are two implementions, gen1 and gen2.
> Your bcm542xx and the bcm541xx are both gen1, and both support inband
> Rx time stamping.

That is correct. Lasse for your future submission please address the 
following:

- conform to the usual patch submission style and break up your changes 
between bcmgenet.c (although I doubt you need to change it), broadcom.c 
and bcm-phy-lib.[ch]

- do not create a PHY device entry specifically for BCM54210PE, use the 
existing BCM54210 entry and add checks using the revision field of 
phydev->phy_id where necessary. There are already many entries in this 
driver, adding more does not help maintaining it. Also, I went through 
several months of work fixing bugs and adding decent power management 
features to this driver that all PHYs should leverage, adding a new 
entry means we need to verify whether all code paths are hit or not

- move generic code, such as all of the PTP code into bcm-phy-lib.[ch] 
where it can easily be re-used across multiple PHY device driver entries 
(54810, 54210 etc.)

Thanks!

> 
> Because the registers are all the same (just the offsets are
> different), I'd like to see a common module that can be used by all
> gen1 devices.  The module could be named bcm-ptp-gen1.c for example.

I would prefer that we just stick to adding that code to 
bcm-phy-lib.[ch] which all Broadcom PHY drivers can use and we can 
decide whether we want to add a Kconfig option specifically for PTP.

Cheers
-- 
Florian

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

* Re: [PATCH net-next] 1588 support on bcm54210pe
  2022-04-23 14:40         ` Florian Fainelli
@ 2022-04-23 18:16           ` Richard Cochran
  0 siblings, 0 replies; 15+ messages in thread
From: Richard Cochran @ 2022-04-23 18:16 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Andrew Lunn, Lasse Johnsen, netdev, Gordon Hollingworth,
	Ahmad Byagowi, Heiner Kallweit, Russell King,
	bcm-kernel-feedback-list

On Sat, Apr 23, 2022 at 07:40:49AM -0700, Florian Fainelli wrote:
> I would prefer that we just stick to adding that code to bcm-phy-lib.[ch]
> which all Broadcom PHY drivers can use and we can decide whether we want to
> add a Kconfig option specifically for PTP.

Sounds good.

Thanks,
Richard

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

end of thread, other threads:[~2022-04-23 18:16 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20 14:03 Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Lasse Johnsen
2022-04-20 19:19 ` Andrew Lunn
2022-04-21 17:32   ` Florian Fainelli
2022-04-22 14:21   ` [PATCH net-next] 1588 support on bcm54210pe Lasse Johnsen
2022-04-22 15:00     ` Andrew Lunn
2022-04-22 19:48       ` Richard Cochran
2022-04-23 14:40         ` Florian Fainelli
2022-04-23 18:16           ` Richard Cochran
2022-04-21 14:48 ` Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Richard Cochran
2022-04-22 15:08   ` [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe Lasse Johnsen
2022-04-22 15:22     ` Jonathan Lemon
2022-04-22 18:11       ` Lasse Johnsen
2022-04-22 18:20         ` Jonathan Lemon
2022-04-23 10:26           ` Lasse Johnsen
2022-04-22 15:42     ` Andrew Lunn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.