Message-ID: <dbcbcd58-855b-4466-8026-7088e9fa8ad8@amd.com>
Date: Mon, 1 Sep 2025 15:46:45 +0530
From: "Rangoju, Raju" <raju.rangoju@....com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: netdev@...r.kernel.org, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 richardcochran@...il.com, linux-kernel@...r.kernel.org,
 Shyam-sundar.S-k@....com
Subject: Re: [PATCH net-next v3] amd-xgbe: Add PPS periodic output support



On 8/28/2025 9:58 PM, Alexander Lobakin wrote:
> From: Raju Rangoju <Raju.Rangoju@....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@....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@....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

Powered by blists - more mailing lists