Re: [Intel-wired-lan] [PATCH v7 iwl-next 4/6] ice: Process TSYN IRQ in a separate function

[Simon Horman <horms@kernel.org>]

On Tue, Aug 20, 2024 at 12:21:51PM +0200, Karol Kolacinski wrote:
> Simplify TSYN IRQ processing by moving it to a separate function and
> having appropriate behavior per PHY model, instead of multiple
> conditions not related to HW, but to specific timestamping modes.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index cf3b02d14b19..861f6224540a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2760,6 +2760,65 @@ enum ice_tx_tstamp_work ice_ptp_process_ts(struct ice_pf *pf)
>  	}
>  }
>  
> +/**
> + * ice_ptp_ts_irq - Process the PTP Tx timestamps in IRQ context
> + * @pf: Board private structure
> + *
> + * Return: IRQ_WAKE_THREAD if Tx timestamp read has to be handled in the bottom
> + *         half of the interrupt and IRQ_HANDLED otherwise.
> + */
> +irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
> +{
> +	struct ice_hw *hw = &pf->hw;
> +
> +	switch (hw->mac_type) {
> +	case ICE_MAC_E810:
> +		/* E810 capable of low latency timestamping with interrupt can
> +		 * request a single timestamp in the top half and wait for
> +		 * a second LL TS interrupt from the FW when it's ready.
> +		 */
> +		if (hw->dev_caps.ts_dev_info.ts_ll_int_read) {
> +			struct ice_ptp_tx *tx = &pf->ptp.port.tx;
> +			u8 idx;
> +
> +			if (!ice_pf_state_is_nominal(pf))
> +				return IRQ_HANDLED;
> +
> +			spin_lock(&tx->lock);
> +			idx = find_next_bit_wrap(tx->in_use, tx->len,
> +						 tx->last_ll_ts_idx_read + 1);
> +			if (idx != tx->len)
> +				ice_ptp_req_tx_single_tstamp(tx, idx);
> +			spin_unlock(&tx->lock);
> +
> +			return IRQ_HANDLED;
> +		}
> +		fallthrough; /* non-LL_TS E810 */
> +	case ICE_MAC_GENERIC:
> +	case ICE_MAC_GENERIC_3K_E825:
> +		/* All other devices process timestamps in the bottom half due
> +		 * to sleeping or polling.
> +		 */
> +		if (!ice_ptp_pf_handles_tx_interrupt(pf))
> +			return IRQ_HANDLED;
> +
> +		set_bit(ICE_MISC_THREAD_TX_TSTAMP, pf->misc_thread);
> +		return IRQ_WAKE_THREAD;
> +	case ICE_MAC_E830:
> +		/* E830 can read timestamps in the top half using rd32() */
> +		if (ice_ptp_process_ts(pf) == ICE_TX_TSTAMP_WORK_PENDING) {
> +			/* Process outstanding Tx timestamps. If there
> +			 * is more work, re-arm the interrupt to trigger again.
> +			 */
> +			wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
> +			ice_flush(hw);
> +		}
> +		return IRQ_HANDLED;

I think it would be better to split out adding E830 support into a separate
patch, leaving this patch as strictly refactoring of existing code.

> +	default:
> +		return IRQ_HANDLED;
> +	}
> +}
> +
>  /**
>   * ice_ptp_maybe_trigger_tx_interrupt - Trigger Tx timstamp interrupt
>   * @pf: Board private structure
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h

...

> @@ -360,6 +361,11 @@ static inline bool ice_ptp_process_ts(struct ice_pf *pf)
>  	return true;
>  }
>  
> +static inline irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
> +{
> +	return IRQ_HANDLED;
> +}

This no-op implementation is in effect if CONFIG_PTP_1588_CLOCK is not
enabled. Whereas previously the fuller implementation would have run.
I think that deserves some coverage in the patch description.

> +
>  static inline u64
>  ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
>  		    const struct ice_pkt_ctx *pkt_ctx)
> -- 
> 2.46.0
> 
>