Re: [PATCH net-next v2 9/9] net: atlantic: add PTP support for AQC113 (Antigua) (Antigua)

[Paolo Abeni <pabeni@redhat.com>]

On 5/8/26 2:01 PM, sukhdeeps@marvell.com wrote:
> From: Sukhdeep Singh <sukhdeeps@marvell.com>
> 
> Add IEEE 1588 PTP support for the AQC113 (Antigua) network controller
> alongside the existing AQC107 (Atlantic) PTP implementation.
> 
> AQC113 PTP uses a different hardware architecture from AQC107:
> - Dual TSG clocks (sel 0 for PTP, sel 1 for PTM) instead of PHY-based
>   timestamping
> - TX timestamp via descriptor writeback instead of firmware mailbox
> - Per-instance PTP timestamp offsets instead of global static table
> - Hardware L3/L4 filters for PTP multicast steering with IPv4 and
>   IPv6 support (4 filter slots for multicast addresses)
> - Direct hardware clock control instead of firmware-mediated access
> 
> Key implementation details:
> 
> PTP clock management:
> - Add aq_ptp_state enum to distinguish first init, link up, and no
>   link states for proper clock initialization
> - On AQC113, only reset the clock on first init (not on every link
>   change) to avoid disrupting ongoing PTP synchronization
> - Re-apply RX filters on link change since hardware state is lost
> 
> TX timestamp path:
> - Add per-packet TX timestamp request via request_ts/clk_sel in the
>   ring buffer descriptor
> - Poll for TX timestamp completion in aq_ring_tx_clean() with a
>   timeout mechanism (aq_ptp_tx_ts_timedout/clear)
> - Set AQ_HW_TXD_CTL_TS_EN in TX descriptors for timestamp-requested
>   packets
> 
> RX filter management:
> - Replace single UDP filter with array of 4 for IPv4/IPv6 multicast
>   PTP addresses (224.0.1.129, 224.0.0.107, ff0e::181, ff02::6b)
> - Add aq_ptp_dpath_enable() for comprehensive filter setup/teardown
> - Add aq_ptp_parse_rx_filters() to map hwtstamp_rx_filters to L2/L4
>   enable flags
> 
> PTP TX path in aq_main.c:
> - Add IPv6 PTP packet detection using ipv6_hdr()->nexthdr
> - Use PTP_EV_PORT/PTP_GEN_PORT defines instead of magic numbers
> - Move skb_tx_timestamp() to non-PTP path to avoid double timestamps
> 
> IRQ and initialization:
> - Account for PTP IRQ vector (AQ_HW_PTP_IRQS) in vector math
> - Move filter/VLAN rule application to aq_nic_start() for proper
>   ordering after PTP ring setup
> - Add AQ_HW_FLAG_STARTED flag management in open/close
> 
> HW layer (hw_atl2.c):
> - Implement PTP clock enable/disable, read, adjust, increment
> - Add GPIO pulse generation for PPS output
> - Add TX/RX PTP ring initialization
> - Add TX timestamp descriptor readback
> - Add RX timestamp extraction from packet trailer
> - Re-enable PTP after hardware reset
> - Wire all PTP ops into hw_atl2_ops table
> 
> Per-instance PTP offsets with empirically measured values for AQC113
> at each link speed (100M/1G/2.5G/5G/10G).
> 
> Signed-off-by: Sukhdeep Singh <sukhdeeps@marvell.com>
> ---
>  .../net/ethernet/aquantia/atlantic/aq_hw.h    |   1 +
>  .../net/ethernet/aquantia/atlantic/aq_main.c  |  34 +-
>  .../net/ethernet/aquantia/atlantic/aq_nic.c   |  48 +-
>  .../ethernet/aquantia/atlantic/aq_pci_func.c  |   4 +-
>  .../net/ethernet/aquantia/atlantic/aq_ptp.c   | 535 ++++++++++++++----
>  .../net/ethernet/aquantia/atlantic/aq_ptp.h   |  15 +-
>  .../net/ethernet/aquantia/atlantic/aq_ring.c  |  42 +-
>  .../aquantia/atlantic/hw_atl2/hw_atl2.c       | 179 +++++-
>  .../aquantia/atlantic/hw_atl2/hw_atl2.h       |  12 +
>  .../atlantic/hw_atl2/hw_atl2_internal.h       |   3 +-
>  .../aquantia/atlantic/hw_atl2/hw_atl2_utils.h |  10 +
>  11 files changed, 710 insertions(+), 173 deletions(-)

Very large patch, you should try to break it in smaller chunks.

> @@ -1035,46 +1246,49 @@ static struct ptp_clock_info aq_ptp_clock = {
>  	.pin_config	= NULL,
>  };
>  
> -#define ptp_offset_init(__idx, __mbps, __egress, __ingress)   do { \
> -		ptp_offset[__idx].mbps = (__mbps); \
> -		ptp_offset[__idx].egress = (__egress); \
> -		ptp_offset[__idx].ingress = (__ingress); } \
> -		while (0)
> +static inline void ptp_offset_init(struct aq_ptp_s *aq_ptp, int idx,
> +				   unsigned int mbps, int egress, int ingress)

'inline' in c files are highly discouraged. Use them only for critical
path and providing reasoning (i.e. the compiler does the wrong thing
otherwise).

[...]
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e270327e47fd..a52d6d3fe464 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -308,24 +308,30 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
>  			}
>  		}
>  
> -		if (likely(!buff->is_eop))
> -			goto out;
> -
> -		if (buff->skb) {
> -			u64_stats_update_begin(&self->stats.tx.syncp);
> -			++self->stats.tx.packets;
> -			self->stats.tx.bytes += buff->skb->len;
> -			u64_stats_update_end(&self->stats.tx.syncp);
> -			dev_kfree_skb_any(buff->skb);
> -		} else if (buff->xdpf) {
> -			u64_stats_update_begin(&self->stats.tx.syncp);
> -			++self->stats.tx.packets;
> -			self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
> -			u64_stats_update_end(&self->stats.tx.syncp);
> -			xdp_return_frame_rx_napi(buff->xdpf);
> -		}
> +		if (unlikely(buff->is_eop)) {

I see this is pre-exiting code, but why is this condition unlikely? I
suppose the NIC is expected to receive packets :-P

Also, using a goto statement would avoid excessive indentation.

> +			if (unlikely(buff->request_ts) &&
> +			    self->aq_nic->aq_hw_ops->hw_ring_tx_ptp_get_ts) {
> +				u64 ts = self->aq_nic->aq_hw_ops->hw_ring_tx_ptp_get_ts(self);
> +
> +				if (!ts)
> +					break;
>  
> -out:
> +				aq_ptp_tx_hwtstamp(self->aq_nic, ts);
> +			}
> +			if (buff->skb) {
> +				u64_stats_update_begin(&self->stats.tx.syncp);
> +				++self->stats.tx.packets;
> +				self->stats.tx.bytes += buff->skb->len;
> +				u64_stats_update_end(&self->stats.tx.syncp);
> +				dev_kfree_skb_any(buff->skb);
> +			} else if (buff->xdpf) {
> +				u64_stats_update_begin(&self->stats.tx.syncp);
> +				++self->stats.tx.packets;
> +				self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
> +				u64_stats_update_end(&self->stats.tx.syncp);
> +				xdp_return_frame_rx_napi(buff->xdpf);
> +			}
> +		}
>  		buff->skb = NULL;
>  		buff->xdpf = NULL;
>  		buff->pa = 0U;

[...]
> @@ -886,6 +912,138 @@ static struct aq_stats_s *hw_atl2_utils_get_hw_stats(struct aq_hw_s *self)
>  	return &self->curr_stats;
>  }
>  
> +static u32 hw_atl2_tsg_int_clk_freq(struct aq_hw_s *self)
> +{
> +	return AQ2_HW_PTP_COUNTER_HZ;
> +}

Why do you need an helper here? Using the macro directly should be just
fine.