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

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 3 Sep 2025 23:19:53 +0530 Raju Rangoju wrote:
> -		 xgbe-hwtstamp.o xgbe-ptp.o \
> +		 xgbe-hwtstamp.o xgbe-ptp.o xgbe-pps.o\

nit: missing space before the backslash?

>  		 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

>  /* 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

We have a lot of completely unused defines here....

>  #define MAC_TICSNR_TSICSNS_INDEX	8
>  #define MAC_TICSNR_TSICSNS_WIDTH	8
>  #define MAC_TECSNR_TSECSNS_INDEX	8

> +int xgbe_pps_config(struct xgbe_prv_data *pdata,
> +		    struct xgbe_pps_config *cfg, int index, bool 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 &= ~get_pps_mask(index);
> +
> +	if (!on) {
> +		value |= get_pps_cmd(index, 0x5);

..and yet there are constants in the code which do not have a define.


> +		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;

I presume that the writes don't matter until we set PPSCR, so returning
an error after performing some writes already is not a bug. But still
it looks s little odd. Especially checking period twice. Why not
calculate the period first, check that it's not <= 3, and then write
out the entire config only once we know that?

Thanks for redoing the mask helpers BTW I think this is much cleaner.
-- 
pw-bot: cr