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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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