[PATCH net-next v3] amd-xgbe: Add PPS periodic output support

[PATCH net-next v3] amd-xgbe: Add PPS periodic output support

[Raju Rangoju]

Add support for hardware PPS (Pulse Per Second) output to the
AMD XGBE driver. The implementation enables flexible periodic
output mode, exposing it via the PTP per_out interface.

The driver supports configuring PPS output using the standard
PTP subsystem, allowing precise periodic signal generation for
time synchronization applications.

The feature has been verified using the testptp tool and
oscilloscope.

Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
---
Changes since v2:
 - avoid redundant checks in xgbe_enable()
 - simplify the mask calculation

Changes since v1:
 - add sanity check to prevent pps_out_num and aux_snap_num exceeding the limit

 drivers/net/ethernet/amd/xgbe/Makefile      |  2 +-
 drivers/net/ethernet/amd/xgbe/xgbe-common.h | 46 ++++++++++++-
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c    | 15 +++++
 drivers/net/ethernet/amd/xgbe/xgbe-pps.c    | 73 +++++++++++++++++++++
 drivers/net/ethernet/amd/xgbe/xgbe-ptp.c    | 26 +++++++-
 drivers/net/ethernet/amd/xgbe/xgbe.h        | 16 +++++
 6 files changed, 173 insertions(+), 5 deletions(-)
 create mode 100644 drivers/net/ethernet/amd/xgbe/xgbe-pps.c

diff --git a/drivers/net/ethernet/amd/xgbe/Makefile b/drivers/net/ethernet/amd/xgbe/Makefile
index 5b0ab6240cf2..d546a212806a 100644
--- a/drivers/net/ethernet/amd/xgbe/Makefile
+++ b/drivers/net/ethernet/amd/xgbe/Makefile
@@ -3,7 +3,7 @@ obj-$(CONFIG_AMD_XGBE) += amd-xgbe.o
 
 amd-xgbe-objs := xgbe-main.o xgbe-drv.o xgbe-dev.o \
 		 xgbe-desc.o xgbe-ethtool.o xgbe-mdio.o \
-		 xgbe-hwtstamp.o xgbe-ptp.o \
+		 xgbe-hwtstamp.o xgbe-ptp.o xgbe-pps.o\
 		 xgbe-i2c.o xgbe-phy-v1.o xgbe-phy-v2.o \
 		 xgbe-platform.o
 
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-common.h b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
index 009fbc9b11ce..c8447825c2f6 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-common.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
@@ -223,11 +223,18 @@
 #define MAC_TSSR			0x0d20
 #define MAC_TXSNR			0x0d30
 #define MAC_TXSSR			0x0d34
+#define MAC_AUXCR			0x0d40
+#define MAC_ATSNR			0x0d48
+#define MAC_ATSSR			0x0d4C
 #define MAC_TICNR                       0x0d58
 #define MAC_TICSNR                      0x0d5C
 #define MAC_TECNR                       0x0d60
 #define MAC_TECSNR                      0x0d64
-
+#define MAC_PPSCR			0x0d70
+#define MAC_PPS0_TTSR			0x0d80
+#define MAC_PPS0_TTNSR			0x0d84
+#define MAC_PPS0_INTERVAL		0x0d88
+#define MAC_PPS0_WIDTH			0x0d8C
 #define MAC_QTFCR_INC			4
 #define MAC_MACA_INC			4
 #define MAC_HTR_INC			4
@@ -235,6 +242,15 @@
 #define MAC_RQC2_INC			4
 #define MAC_RQC2_Q_PER_REG		4
 
+/* PPS helpers */
+#define PPSEN0				BIT(4)
+#define MAC_PPSx_TTSR(x)		((MAC_PPS0_TTSR) + ((x) * 0x10))
+#define MAC_PPSx_TTNSR(x)		((MAC_PPS0_TTNSR) + ((x) * 0x10))
+#define MAC_PPSx_INTERVAL(x)		((MAC_PPS0_INTERVAL) + ((x) * 0x10))
+#define MAC_PPSx_WIDTH(x)		((MAC_PPS0_WIDTH) + ((x) * 0x10))
+#define PPS_MAXIDX(x)			((((x) + 1) * 8) - 1)
+#define PPS_MINIDX(x)			((x) * 8)
+
 /* MAC register entry bit positions and sizes */
 #define MAC_HWF0R_ADDMACADRSEL_INDEX	18
 #define MAC_HWF0R_ADDMACADRSEL_WIDTH	5
@@ -460,8 +476,26 @@
 #define MAC_TSCR_TXTSSTSM_WIDTH		1
 #define MAC_TSSR_TXTSC_INDEX		15
 #define MAC_TSSR_TXTSC_WIDTH		1
+#define MAC_TSSR_ATSSTN_INDEX		16
+#define MAC_TSSR_ATSSTN_WIDTH		4
+#define MAC_TSSR_ATSNS_INDEX		25
+#define MAC_TSSR_ATSNS_WIDTH		5
+#define MAC_TSSR_ATSSTM_INDEX		24
+#define MAC_TSSR_ATSSTM_WIDTH		1
+#define MAC_TSSR_ATSSTN_INDEX		16
+#define MAC_TSSR_ATSSTN_WIDTH		4
+#define MAC_TSSR_AUXTSTRIG_INDEX	2
+#define MAC_TSSR_AUXTSTRIG_WIDTH	1
 #define MAC_TXSNR_TXTSSTSMIS_INDEX	31
 #define MAC_TXSNR_TXTSSTSMIS_WIDTH	1
+#define MAC_AUXCR_ATSEN3_INDEX		7
+#define MAC_AUXCR_ATSEN3_WIDTH		1
+#define MAC_AUXCR_ATSEN2_INDEX		6
+#define MAC_AUXCR_ATSEN2_WIDTH		1
+#define MAC_AUXCR_ATSEN1_INDEX		5
+#define MAC_AUXCR_ATSEN1_WIDTH		1
+#define MAC_AUXCR_ATSEN0_INDEX		4
+#define MAC_AUXCR_ATSEN0_WIDTH		1
 #define MAC_TICSNR_TSICSNS_INDEX	8
 #define MAC_TICSNR_TSICSNS_WIDTH	8
 #define MAC_TECSNR_TSECSNS_INDEX	8
@@ -496,8 +530,14 @@
 #define MAC_VR_SNPSVER_WIDTH		8
 #define MAC_VR_USERVER_INDEX		16
 #define MAC_VR_USERVER_WIDTH		8
-
-/* MMC register offsets */
+#define MAC_PPSCR_PPSEN0_INDEX		4
+#define MAC_PPSCR_PPSEN0_WIDTH		1
+#define MAC_PPSCR_PPSCTRL0_INDEX	0
+#define MAC_PPSCR_PPSCTRL0_WIDTH	4
+#define MAC_PPSx_TTNSR_TRGTBUSY0_INDEX	31
+#define MAC_PPSx_TTNSR_TRGTBUSY0_WIDTH	1
+
+ /* MMC register offsets */
 #define MMC_CR				0x0800
 #define MMC_RISR			0x0804
 #define MMC_TISR			0x0808
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 2e9b95a94f89..f0989aa01855 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -691,6 +691,21 @@ void xgbe_get_all_hw_features(struct xgbe_prv_data *pdata)
 	hw_feat->pps_out_num  = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, PPSOUTNUM);
 	hw_feat->aux_snap_num = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, AUXSNAPNUM);
 
+	/* Sanity check and warn if hardware reports more than supported */
+	if (hw_feat->pps_out_num > XGBE_MAX_PPS_OUT) {
+		dev_warn(pdata->dev,
+			 "Hardware reports %u PPS outputs, limiting to %u\n",
+			 hw_feat->pps_out_num, XGBE_MAX_PPS_OUT);
+		hw_feat->pps_out_num = XGBE_MAX_PPS_OUT;
+	}
+
+	if (hw_feat->aux_snap_num > XGBE_MAX_AUX_SNAP) {
+		dev_warn(pdata->dev,
+			 "Hardware reports %u aux snapshot inputs, limiting to %u\n",
+			 hw_feat->aux_snap_num, XGBE_MAX_AUX_SNAP);
+		hw_feat->aux_snap_num = XGBE_MAX_AUX_SNAP;
+	}
+
 	/* Translate the Hash Table size into actual number */
 	switch (hw_feat->hash_table_size) {
 	case 0:
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pps.c b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
new file mode 100644
index 000000000000..b5704fbbc5be
--- /dev/null
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-3-Clause)
+/*
+ * Copyright (c) 2014-2025, Advanced Micro Devices, Inc.
+ * Copyright (c) 2014, Synopsys, Inc.
+ * All rights reserved
+ *
+ * Author: Raju Rangoju <Raju.Rangoju@amd.com>
+ */
+
+#include "xgbe.h"
+#include "xgbe-common.h"
+
+static inline u32 PPSx_MASK(unsigned int x)
+{
+	return GENMASK(PPS_MAXIDX(x), PPS_MINIDX(x));
+}
+
+static inline u32 PPSCMDx(unsigned int x, u32 val)
+{
+	return ((val & GENMASK(3, 0)) << PPS_MINIDX(x));
+}
+
+static inline u32 TRGTMODSELx(unsigned int x, u32 val)
+{
+	return ((val & GENMASK(1, 0)) << (PPS_MAXIDX(x) - 2));
+}
+
+int xgbe_pps_config(struct xgbe_prv_data *pdata,
+		    struct xgbe_pps_config *cfg, int index, int on)
+{
+	unsigned int value = 0;
+	unsigned int tnsec;
+	u64 period;
+
+	tnsec = XGMAC_IOREAD(pdata, MAC_PPSx_TTNSR(index));
+	if (XGMAC_GET_BITS(tnsec, MAC_PPSx_TTNSR, TRGTBUSY0))
+		return -EBUSY;
+
+	value = XGMAC_IOREAD(pdata, MAC_PPSCR);
+
+	value &= ~PPSx_MASK(index);
+
+	if (!on) {
+		value |= PPSCMDx(index, 0x5);
+		value |= PPSEN0;
+		XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
+		return 0;
+	}
+
+	XGMAC_IOWRITE(pdata, MAC_PPSx_TTSR(index), cfg->start.tv_sec);
+	XGMAC_IOWRITE(pdata, MAC_PPSx_TTNSR(index), cfg->start.tv_nsec);
+
+	period = cfg->period.tv_sec * NSEC_PER_SEC;
+	period += cfg->period.tv_nsec;
+	do_div(period, XGBE_V2_TSTAMP_SSINC);
+
+	if (period <= 1)
+		return -EINVAL;
+
+	XGMAC_IOWRITE(pdata, MAC_PPSx_INTERVAL(index), period - 1);
+	period >>= 1;
+	if (period <= 1)
+		return -EINVAL;
+
+	XGMAC_IOWRITE(pdata, MAC_PPSx_WIDTH(index), period - 1);
+
+	value |= PPSCMDx(index, 0x2);
+	value |= TRGTMODSELx(index, 0x2);
+	value |= PPSEN0;
+
+	XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
+	return 0;
+}
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
index 3658afc7801d..0e0b8ec3b504 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
@@ -106,7 +106,29 @@ static int xgbe_settime(struct ptp_clock_info *info,
 static int xgbe_enable(struct ptp_clock_info *info,
 		       struct ptp_clock_request *request, int on)
 {
-	return -EOPNOTSUPP;
+	struct xgbe_prv_data *pdata = container_of(info, struct xgbe_prv_data,
+						   ptp_clock_info);
+	struct xgbe_pps_config *pps_cfg;
+	unsigned long flags;
+	int ret;
+
+	dev_dbg(pdata->dev, "rq->type %d on %d\n", request->type, on);
+
+	if (request->type != PTP_CLK_REQ_PEROUT)
+		return -EOPNOTSUPP;
+
+	pps_cfg = &pdata->pps[request->perout.index];
+
+	pps_cfg->start.tv_sec = request->perout.start.sec;
+	pps_cfg->start.tv_nsec = request->perout.start.nsec;
+	pps_cfg->period.tv_sec = request->perout.period.sec;
+	pps_cfg->period.tv_nsec = request->perout.period.nsec;
+
+	spin_lock_irqsave(&pdata->tstamp_lock, flags);
+	ret = xgbe_pps_config(pdata, pps_cfg, request->perout.index, on);
+	spin_unlock_irqrestore(&pdata->tstamp_lock, flags);
+
+	return ret;
 }
 
 void xgbe_ptp_register(struct xgbe_prv_data *pdata)
@@ -122,6 +144,8 @@ void xgbe_ptp_register(struct xgbe_prv_data *pdata)
 	info->adjtime = xgbe_adjtime;
 	info->gettimex64 = xgbe_gettimex;
 	info->settime64 = xgbe_settime;
+	info->n_per_out = pdata->hw_feat.pps_out_num;
+	info->n_ext_ts = pdata->hw_feat.aux_snap_num;
 	info->enable = xgbe_enable;
 
 	clock = ptp_clock_register(info, pdata->dev);
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
index 0fa80a238ac5..75246699d399 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
@@ -142,6 +142,10 @@
 #define XGBE_V2_TSTAMP_SNSINC	0
 #define XGBE_V2_PTP_ACT_CLK_FREQ	1000000000
 
+/* Define maximum supported values */
+#define XGBE_MAX_PPS_OUT	4
+#define XGBE_MAX_AUX_SNAP	4
+
 /* Driver PMT macros */
 #define XGMAC_DRIVER_CONTEXT	1
 #define XGMAC_IOCTL_CONTEXT	2
@@ -673,6 +677,11 @@ struct xgbe_ext_stats {
 	u64 rx_vxlan_csum_errors;
 };
 
+struct xgbe_pps_config {
+	struct timespec64 start;
+	struct timespec64 period;
+};
+
 struct xgbe_hw_if {
 	int (*tx_complete)(struct xgbe_ring_desc *);
 
@@ -1143,6 +1152,9 @@ struct xgbe_prv_data {
 	struct sk_buff *tx_tstamp_skb;
 	u64 tx_tstamp;
 
+	/* Pulse Per Second output */
+	struct xgbe_pps_config pps[XGBE_MAX_PPS_OUT];
+
 	/* DCB support */
 	struct ieee_ets *ets;
 	struct ieee_pfc *pfc;
@@ -1305,6 +1317,10 @@ void xgbe_prep_tx_tstamp(struct xgbe_prv_data *pdata,
 int xgbe_init_ptp(struct xgbe_prv_data *pdata);
 void xgbe_update_tstamp_time(struct xgbe_prv_data *pdata, unsigned int sec,
 			     unsigned int nsec);
+
+int xgbe_pps_config(struct xgbe_prv_data *pdata, struct xgbe_pps_config *cfg,
+		    int index, int on);
+
 #ifdef CONFIG_DEBUG_FS
 void xgbe_debugfs_init(struct xgbe_prv_data *);
 void xgbe_debugfs_exit(struct xgbe_prv_data *);
-- 
2.34.1

Re: [PATCH net-next v3] amd-xgbe: Add PPS periodic output support

[Alexander Lobakin]

From: Raju Rangoju <Raju.Rangoju@amd.com>
Date: Thu, 28 Aug 2025 14:59:00 +0530

> Add support for hardware PPS (Pulse Per Second) output to the
> AMD XGBE driver. The implementation enables flexible periodic
> output mode, exposing it via the PTP per_out interface.
> 
> The driver supports configuring PPS output using the standard
> PTP subsystem, allowing precise periodic signal generation for
> time synchronization applications.
> 
> The feature has been verified using the testptp tool and
> oscilloscope.
> 
> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> ---
> Changes since v2:
>  - avoid redundant checks in xgbe_enable()
>  - simplify the mask calculation
> 
> Changes since v1:
>  - add sanity check to prevent pps_out_num and aux_snap_num exceeding the limit
> 
>  drivers/net/ethernet/amd/xgbe/Makefile      |  2 +-
>  drivers/net/ethernet/amd/xgbe/xgbe-common.h | 46 ++++++++++++-
>  drivers/net/ethernet/amd/xgbe/xgbe-drv.c    | 15 +++++
>  drivers/net/ethernet/amd/xgbe/xgbe-pps.c    | 73 +++++++++++++++++++++
>  drivers/net/ethernet/amd/xgbe/xgbe-ptp.c    | 26 +++++++-
>  drivers/net/ethernet/amd/xgbe/xgbe.h        | 16 +++++
>  6 files changed, 173 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/net/ethernet/amd/xgbe/xgbe-pps.c

[...]

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> index 2e9b95a94f89..f0989aa01855 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> @@ -691,6 +691,21 @@ void xgbe_get_all_hw_features(struct xgbe_prv_data *pdata)
>  	hw_feat->pps_out_num  = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, PPSOUTNUM);
>  	hw_feat->aux_snap_num = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, AUXSNAPNUM);
>  
> +	/* Sanity check and warn if hardware reports more than supported */
> +	if (hw_feat->pps_out_num > XGBE_MAX_PPS_OUT) {
> +		dev_warn(pdata->dev,

1. How often can this function be called? Don't you need the _ratelimit
   version here?
2. netdev_ variant instead of dev_?

> +			 "Hardware reports %u PPS outputs, limiting to %u\n",
> +			 hw_feat->pps_out_num, XGBE_MAX_PPS_OUT);
> +		hw_feat->pps_out_num = XGBE_MAX_PPS_OUT;
> +	}
> +
> +	if (hw_feat->aux_snap_num > XGBE_MAX_AUX_SNAP) {
> +		dev_warn(pdata->dev,

(same)

> +			 "Hardware reports %u aux snapshot inputs, limiting to %u\n",
> +			 hw_feat->aux_snap_num, XGBE_MAX_AUX_SNAP);

BTW, these messages are not very meaningful, maybe you should print both
min and max and say that the actual HW output is out of range?

> +		hw_feat->aux_snap_num = XGBE_MAX_AUX_SNAP;
> +	}
> +
>  	/* Translate the Hash Table size into actual number */
>  	switch (hw_feat->hash_table_size) {
>  	case 0:
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pps.c b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
> new file mode 100644
> index 000000000000..b5704fbbc5be
> --- /dev/null
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-3-Clause)
> +/*
> + * Copyright (c) 2014-2025, Advanced Micro Devices, Inc.
> + * Copyright (c) 2014, Synopsys, Inc.
> + * All rights reserved
> + *
> + * Author: Raju Rangoju <Raju.Rangoju@amd.com>
> + */
> +
> +#include "xgbe.h"
> +#include "xgbe-common.h"
> +
> +static inline u32 PPSx_MASK(unsigned int x)
> +{
> +	return GENMASK(PPS_MAXIDX(x), PPS_MINIDX(x));
> +}
> +
> +static inline u32 PPSCMDx(unsigned int x, u32 val)
> +{
> +	return ((val & GENMASK(3, 0)) << PPS_MINIDX(x));

Redundant outer ()s.

> +}
> +
> +static inline u32 TRGTMODSELx(unsigned int x, u32 val)
> +{
> +	return ((val & GENMASK(1, 0)) << (PPS_MAXIDX(x) - 2));

Same here.

> +}

I believe you shouldn't name these functions that way, also pls no
inlines in .c files.
Either give them proper names and remove `inline` or make macros from them.

> +
> +int xgbe_pps_config(struct xgbe_prv_data *pdata,
> +		    struct xgbe_pps_config *cfg, int index, int on)

@on can be bool?

> +{
> +	unsigned int value = 0;
> +	unsigned int tnsec;
> +	u64 period;
> +
> +	tnsec = XGMAC_IOREAD(pdata, MAC_PPSx_TTNSR(index));
> +	if (XGMAC_GET_BITS(tnsec, MAC_PPSx_TTNSR, TRGTBUSY0))
> +		return -EBUSY;
> +
> +	value = XGMAC_IOREAD(pdata, MAC_PPSCR);
> +
> +	value &= ~PPSx_MASK(index);

I'd remove that NL between these two or even squash them into 1 line if
it fits into 80 chars.

> +
> +	if (!on) {
> +		value |= PPSCMDx(index, 0x5);
> +		value |= PPSEN0;
> +		XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
> +		return 0;

Newline before the return.

> +	}
> +
> +	XGMAC_IOWRITE(pdata, MAC_PPSx_TTSR(index), cfg->start.tv_sec);
> +	XGMAC_IOWRITE(pdata, MAC_PPSx_TTNSR(index), cfg->start.tv_nsec);
> +
> +	period = cfg->period.tv_sec * NSEC_PER_SEC;
> +	period += cfg->period.tv_nsec;
> +	do_div(period, XGBE_V2_TSTAMP_SSINC);
> +
> +	if (period <= 1)
> +		return -EINVAL;
> +
> +	XGMAC_IOWRITE(pdata, MAC_PPSx_INTERVAL(index), period - 1);
> +	period >>= 1;
> +	if (period <= 1)
> +		return -EINVAL;
> +
> +	XGMAC_IOWRITE(pdata, MAC_PPSx_WIDTH(index), period - 1);
> +
> +	value |= PPSCMDx(index, 0x2);
> +	value |= TRGTMODSELx(index, 0x2);
> +	value |= PPSEN0;
> +
> +	XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
> +	return 0;

Same here.

> +}
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
> index 3658afc7801d..0e0b8ec3b504 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
> @@ -106,7 +106,29 @@ static int xgbe_settime(struct ptp_clock_info *info,
>  static int xgbe_enable(struct ptp_clock_info *info,
>  		       struct ptp_clock_request *request, int on)
>  {
> -	return -EOPNOTSUPP;
> +	struct xgbe_prv_data *pdata = container_of(info, struct xgbe_prv_data,
> +						   ptp_clock_info);
> +	struct xgbe_pps_config *pps_cfg;
> +	unsigned long flags;
> +	int ret;
> +
> +	dev_dbg(pdata->dev, "rq->type %d on %d\n", request->type, on);
> +
> +	if (request->type != PTP_CLK_REQ_PEROUT)
> +		return -EOPNOTSUPP;
> +
> +	pps_cfg = &pdata->pps[request->perout.index];
> +
> +	pps_cfg->start.tv_sec = request->perout.start.sec;
> +	pps_cfg->start.tv_nsec = request->perout.start.nsec;
> +	pps_cfg->period.tv_sec = request->perout.period.sec;
> +	pps_cfg->period.tv_nsec = request->perout.period.nsec;
> +
> +	spin_lock_irqsave(&pdata->tstamp_lock, flags);
> +	ret = xgbe_pps_config(pdata, pps_cfg, request->perout.index, on);
> +	spin_unlock_irqrestore(&pdata->tstamp_lock, flags);

Are you sure you need to protect the whole xgbe_pps_config() from
interrupts? It's quite large and I don't think you need that lock
(and IRQ protection) for its entire runtime.

> +
> +	return ret;
>  }
>  
>  void xgbe_ptp_register(struct xgbe_prv_data *pdata)

Thanks,
Olek

Re: [PATCH net-next v3] amd-xgbe: Add PPS periodic output support

[Rangoju, Raju]

On 8/28/2025 9:58 PM, Alexander Lobakin wrote:
> From: Raju Rangoju <Raju.Rangoju@amd.com>
> Date: Thu, 28 Aug 2025 14:59:00 +0530
> 
>> Add support for hardware PPS (Pulse Per Second) output to the
>> AMD XGBE driver. The implementation enables flexible periodic
>> output mode, exposing it via the PTP per_out interface.
>>
>> The driver supports configuring PPS output using the standard
>> PTP subsystem, allowing precise periodic signal generation for
>> time synchronization applications.
>>
>> The feature has been verified using the testptp tool and
>> oscilloscope.
>>
>> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
>> ---
>> Changes since v2:
>>   - avoid redundant checks in xgbe_enable()
>>   - simplify the mask calculation
>>
>> Changes since v1:
>>   - add sanity check to prevent pps_out_num and aux_snap_num exceeding the limit
>>
>>   drivers/net/ethernet/amd/xgbe/Makefile      |  2 +-
>>   drivers/net/ethernet/amd/xgbe/xgbe-common.h | 46 ++++++++++++-
>>   drivers/net/ethernet/amd/xgbe/xgbe-drv.c    | 15 +++++
>>   drivers/net/ethernet/amd/xgbe/xgbe-pps.c    | 73 +++++++++++++++++++++
>>   drivers/net/ethernet/amd/xgbe/xgbe-ptp.c    | 26 +++++++-
>>   drivers/net/ethernet/amd/xgbe/xgbe.h        | 16 +++++
>>   6 files changed, 173 insertions(+), 5 deletions(-)
>>   create mode 100644 drivers/net/ethernet/amd/xgbe/xgbe-pps.c
> 
> [...]
> 
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
>> index 2e9b95a94f89..f0989aa01855 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
>> @@ -691,6 +691,21 @@ void xgbe_get_all_hw_features(struct xgbe_prv_data *pdata)
>>   	hw_feat->pps_out_num  = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, PPSOUTNUM);
>>   	hw_feat->aux_snap_num = XGMAC_GET_BITS(mac_hfr2, MAC_HWF2R, AUXSNAPNUM);
>>   
>> +	/* Sanity check and warn if hardware reports more than supported */
>> +	if (hw_feat->pps_out_num > XGBE_MAX_PPS_OUT) {
>> +		dev_warn(pdata->dev,
> 
> 1. How often can this function be called? Don't you need the _ratelimit
>     version here?

This is only called once after driver load, so _ratelimit may not be needed.

> 2. netdev_ variant instead of dev_?

This is in the early stages of probe, even before netdev is configured.

> 
>> +			 "Hardware reports %u PPS outputs, limiting to %u\n",
>> +			 hw_feat->pps_out_num, XGBE_MAX_PPS_OUT);
>> +		hw_feat->pps_out_num = XGBE_MAX_PPS_OUT;
>> +	}
>> +
>> +	if (hw_feat->aux_snap_num > XGBE_MAX_AUX_SNAP) {
>> +		dev_warn(pdata->dev,
> 
> (same)
> 
>> +			 "Hardware reports %u aux snapshot inputs, limiting to %u\n",
>> +			 hw_feat->aux_snap_num, XGBE_MAX_AUX_SNAP);
> 
> BTW, these messages are not very meaningful, maybe you should print both
> min and max and say that the actual HW output is out of range?

XGBE_MAX_AUX_SNAP is max supported value, so limiting it only in case 
hardware reports more than max value. Any lower value should be fine.

> 
>> +		hw_feat->aux_snap_num = XGBE_MAX_AUX_SNAP;
>> +	}
>> +
>>   	/* Translate the Hash Table size into actual number */
>>   	switch (hw_feat->hash_table_size) {
>>   	case 0:
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pps.c b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
>> new file mode 100644
>> index 000000000000..b5704fbbc5be
>> --- /dev/null
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pps.c
>> @@ -0,0 +1,73 @@
>> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-3-Clause)
>> +/*
>> + * Copyright (c) 2014-2025, Advanced Micro Devices, Inc.
>> + * Copyright (c) 2014, Synopsys, Inc.
>> + * All rights reserved
>> + *
>> + * Author: Raju Rangoju <Raju.Rangoju@amd.com>
>> + */
>> +
>> +#include "xgbe.h"
>> +#include "xgbe-common.h"
>> +
>> +static inline u32 PPSx_MASK(unsigned int x)
>> +{
>> +	return GENMASK(PPS_MAXIDX(x), PPS_MINIDX(x));
>> +}
>> +
>> +static inline u32 PPSCMDx(unsigned int x, u32 val)
>> +{
>> +	return ((val & GENMASK(3, 0)) << PPS_MINIDX(x));
> 
> Redundant outer ()s.
> 
>> +}
>> +
>> +static inline u32 TRGTMODSELx(unsigned int x, u32 val)
>> +{
>> +	return ((val & GENMASK(1, 0)) << (PPS_MAXIDX(x) - 2));
> 
> Same here.
> 
>> +}
> 
> I believe you shouldn't name these functions that way, also pls no
> inlines in .c files.
> Either give them proper names and remove `inline` or make macros from them.

Sure, will address these and above comments in next version.

> 
>> +
>> +int xgbe_pps_config(struct xgbe_prv_data *pdata,
>> +		    struct xgbe_pps_config *cfg, int index, int on)
> 
> @on can be bool?

Yes, will address this.

> 
>> +{
>> +	unsigned int value = 0;
>> +	unsigned int tnsec;
>> +	u64 period;
>> +
>> +	tnsec = XGMAC_IOREAD(pdata, MAC_PPSx_TTNSR(index));
>> +	if (XGMAC_GET_BITS(tnsec, MAC_PPSx_TTNSR, TRGTBUSY0))
>> +		return -EBUSY;
>> +
>> +	value = XGMAC_IOREAD(pdata, MAC_PPSCR);
>> +
>> +	value &= ~PPSx_MASK(index);
> 
> I'd remove that NL between these two or even squash them into 1 line if
> it fits into 80 chars.
> 
>> +
>> +	if (!on) {
>> +		value |= PPSCMDx(index, 0x5);
>> +		value |= PPSEN0;
>> +		XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
>> +		return 0;
> 
> Newline before the return.

sure, will drop this in next version of patch

> 
>> +	}
>> +
>> +	XGMAC_IOWRITE(pdata, MAC_PPSx_TTSR(index), cfg->start.tv_sec);
>> +	XGMAC_IOWRITE(pdata, MAC_PPSx_TTNSR(index), cfg->start.tv_nsec);
>> +
>> +	period = cfg->period.tv_sec * NSEC_PER_SEC;
>> +	period += cfg->period.tv_nsec;
>> +	do_div(period, XGBE_V2_TSTAMP_SSINC);
>> +
>> +	if (period <= 1)
>> +		return -EINVAL;
>> +
>> +	XGMAC_IOWRITE(pdata, MAC_PPSx_INTERVAL(index), period - 1);
>> +	period >>= 1;
>> +	if (period <= 1)
>> +		return -EINVAL;
>> +
>> +	XGMAC_IOWRITE(pdata, MAC_PPSx_WIDTH(index), period - 1);
>> +
>> +	value |= PPSCMDx(index, 0x2);
>> +	value |= TRGTMODSELx(index, 0x2);
>> +	value |= PPSEN0;
>> +
>> +	XGMAC_IOWRITE(pdata, MAC_PPSCR, value);
>> +	return 0;
> 
> Same here.
> 
>> +}
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
>> index 3658afc7801d..0e0b8ec3b504 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-ptp.c
>> @@ -106,7 +106,29 @@ static int xgbe_settime(struct ptp_clock_info *info,
>>   static int xgbe_enable(struct ptp_clock_info *info,
>>   		       struct ptp_clock_request *request, int on)
>>   {
>> -	return -EOPNOTSUPP;
>> +	struct xgbe_prv_data *pdata = container_of(info, struct xgbe_prv_data,
>> +						   ptp_clock_info);
>> +	struct xgbe_pps_config *pps_cfg;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	dev_dbg(pdata->dev, "rq->type %d on %d\n", request->type, on);
>> +
>> +	if (request->type != PTP_CLK_REQ_PEROUT)
>> +		return -EOPNOTSUPP;
>> +
>> +	pps_cfg = &pdata->pps[request->perout.index];
>> +
>> +	pps_cfg->start.tv_sec = request->perout.start.sec;
>> +	pps_cfg->start.tv_nsec = request->perout.start.nsec;
>> +	pps_cfg->period.tv_sec = request->perout.period.sec;
>> +	pps_cfg->period.tv_nsec = request->perout.period.nsec;
>> +
>> +	spin_lock_irqsave(&pdata->tstamp_lock, flags);
>> +	ret = xgbe_pps_config(pdata, pps_cfg, request->perout.index, on);
>> +	spin_unlock_irqrestore(&pdata->tstamp_lock, flags);
> 
> Are you sure you need to protect the whole xgbe_pps_config() from
> interrupts? It's quite large and I don't think you need that lock
> (and IRQ protection) for its entire runtime.

xgbe_pps_config() touches the hardware registers throughout the 
function, so this protection is needed.

> 
>> +
>> +	return ret;
>>   }
>>   
>>   void xgbe_ptp_register(struct xgbe_prv_data *pdata)
> 
> Thanks,
> Olek