[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250905184027.0b36d81b@kernel.org>
Date: Fri, 5 Sep 2025 18:40:27 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Raju Rangoju <Raju.Rangoju@....com>
Cc: <netdev@...r.kernel.org>, <andrew+netdev@...n.ch>,
<davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
<richardcochran@...il.com>, <linux-kernel@...r.kernel.org>,
<Shyam-sundar.S-k@....com>
Subject: Re: [PATCH net-next v4] amd-xgbe: Add PPS periodic output support
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
Powered by blists - more mailing lists