Message-ID: <ad94e165-ea7f-4216-b43d-b035c443a914@intel.com>
Date: Fri, 26 Jul 2024 15:37:37 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Karol Kolacinski <karol.kolacinski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, Jacob Keller
	<jacob.e.keller@...el.com>, <netdev@...r.kernel.org>,
	<anthony.l.nguyen@...el.com>, <przemyslaw.kitszel@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v3 iwl-next 4/4] ice: combine cross
 timestamp functions for E82x and E830

From: Karol Kolacinski <karol.kolacinski@...el.com>
Date: Thu, 25 Jul 2024 11:34:51 +0200

> From: Jacob Keller <jacob.e.keller@...el.com>
> 
> The E830 and E82x devices use essentially the same logic for performing
> a crosstimestamp. The only difference is that E830 hardware has
> different offsets. Instead of having two implementations, combine them
> into a single ice_capture_crosststamp() function.
> 
> Also combine the wrapper functions which call
> get_device_system_crosststamp() into a single ice_ptp_getcrosststamp()
> function.
> 
> To support both hardware types, the ice_capture_crosststamp function
> must be able to determine the appropriate registers to access. To handle
> this, pass a custom context structure instead of the PF pointer. This
> structure, ice_crosststamp_ctx, contains a pointer to the PF, and
> a pointer to the device configuration structure. This new structure also
> will make it easier to implement historic snapshot support in a future
> commit.
> 
> The device configuration structure is a static const data which defines
> the offsets and flags for the various registers. This includes the lock
> register, the cross timestamp control register, the upper and lower ART
> system time capture registers, and the upper and lower device time
> capture registers for each timer index.
> 
> This does add extra data to the .text of the module (and thus kernel),
> but it also removes 2 near duplicate functions for enabling E830
> support.
> 
> Use the configuration structure to access all of the registers in
> ice_capture_crosststamp(). Ensure that we don't over-run the device time
> array by checking that the timer index is 0 or 1. Previously this was
> simply assumed, and it would cause the device to read an incorrect and
> likely garbage register.
> 
> It does feel like there should be a kernel interface for managing
> register offsets like this, but the closest thing I saw was
> <linux/regmap.h> which is interesting but not quite what we're looking
> for...
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>

[...]

> diff --git a/drivers/net/ethernet/intel/ice/ice_osdep.h b/drivers/net/ethernet/intel/ice/ice_osdep.h
> index a2562f04267f..c03ab0207e0a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_osdep.h
> +++ b/drivers/net/ethernet/intel/ice/ice_osdep.h
> @@ -23,6 +23,9 @@
>  #define wr64(a, reg, value)	writeq((value), ((a)->hw_addr + (reg)))
>  #define rd64(a, reg)		readq((a)->hw_addr + (reg))
>  
> +#define rd32_poll_timeout(a, addr, val, cond, delay_us, timeout_us) \
> +	read_poll_timeout(rd32, val, cond, delay_us, timeout_us, false, a, addr)
> +
>  #define ice_flush(a)		rd32((a), GLGEN_STAT)
>  #define ICE_M(m, s)		((m ## U) << (s))
>  
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 9f0eff040a95..ac3944fec2d3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (C) 2021, Intel Corporation. */
>  
> +#include <linux/iopoll.h>

read_poll_timeout() is used in osdep.h, but you include it here.
You should either define rd32_poll_timeout() here instead of the header
or move this include to osdep.h.

>  #include "ice.h"
>  #include "ice_lib.h"
>  #include "ice_trace.h"

[...]

> -static int
> -ice_ptp_getcrosststamp_e82x(struct ptp_clock_info *info,
> -			    struct system_device_crosststamp *cts)
> +static int ice_ptp_getcrosststamp(struct ptp_clock_info *info,
> +				  struct system_device_crosststamp *cts)
>  {
>  	struct ice_pf *pf = ptp_info_to_pf(info);
> +	struct ice_crosststamp_ctx ctx = {};
> +
> +	ctx.pf = pf;

	struct ice_crosststamp_ctx ctx = {
		.pf	= pf,
	};

> +
> +	switch (pf->hw.ptp.phy_model) {
> +	case ICE_PHY_E82X:
> +		ctx.cfg = &ice_crosststamp_cfg_e82x;
> +		break;
> +	case ICE_PHY_E830:
> +		ctx.cfg = &ice_crosststamp_cfg_e830;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  
> -	return get_device_system_crosststamp(ice_ptp_get_syncdevicetime,
> -					     pf, NULL, cts);
> +	return get_device_system_crosststamp(ice_capture_crosststamp, &ctx,
> +					     &ctx.snapshot, cts);
>  }
> -#endif /* CONFIG_ICE_HWTS */
>  
> +#endif /* CONFIG_ICE_HWTS */
>  /**
>   * ice_ptp_get_ts_config - ioctl interface to read the timestamping config
>   * @pf: Board private structure
> @@ -2523,7 +2599,7 @@ static void ice_ptp_set_funcs_e82x(struct ice_pf *pf)
>  #ifdef CONFIG_ICE_HWTS
>  	if (boot_cpu_has(X86_FEATURE_ART) &&
>  	    boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ))
> -		pf->ptp.info.getcrosststamp = ice_ptp_getcrosststamp_e82x;
> +		pf->ptp.info.getcrosststamp = ice_ptp_getcrosststamp;
>  
>  #endif /* CONFIG_ICE_HWTS */
>  	if (ice_is_e825c(&pf->hw)) {
> @@ -2592,6 +2668,28 @@ static void ice_ptp_set_funcs_e810(struct ice_pf *pf)
>  	}
>  }
>  
> +/**
> + * ice_ptp_set_funcs_e830 - Set specialized functions for E830 support
> + * @pf: Board private structure
> + *
> + * Assign functions to the PTP capabiltiies structure for E830 devices.
> + * Functions which operate across all device families should be set directly
> + * in ice_ptp_set_caps. Only add functions here which are distinct for E830
> + * devices.
> + */
> +static void ice_ptp_set_funcs_e830(struct ice_pf *pf)
> +{
> +#ifdef CONFIG_ICE_HWTS
> +	if (pcie_ptm_enabled(pf->pdev) &&
> +	    boot_cpu_has(X86_FEATURE_ART) &&
> +	    boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ))
> +		pf->ptp.info.getcrosststamp = ice_ptp_getcrosststamp;
> +#endif /* CONFIG_ICE_HWTS */

I've seen this pattern in several drivers already. I really feel like it
needs a generic wrapper.
I mean, there should be something like

arch/x86/include/asm/ptm.h (not sure about the name)

where you put let's say

static inline bool arch_has_ptm(struct pci_device *pdev)

Same for

include/asm-generic/ptm.h

there it will be `return false`.

In include/asm-generic/Kbuild, you add

mandatory-y += ptm.h.

Then, include/linux/ptm.h should include <asm/ptm.h> and in your driver
sources, you include <linux/ptm.h> and check

	if (arch_has_ptm(pdev))
		pf->ptp.info.getcrosststamp = ice_ptp_getcrosststamp;

It's just a draft, adjust accordingly.

Checking for x86 features in the driver doesn't look good. Especially
when you add ARM64 or whatever support in future.

> +
> +	/* Rest of the config is the same as base E810 */
> +	ice_ptp_set_funcs_e810(pf);
> +}

Thanks,
Olek

Powered by blists - more mailing lists