[v3,iwl-next,4/4] ice: combine cross timestamp functions for E82x and E830

Commit Message

Karol Kolacinski July 25, 2024, 9:34 a.m. UTC

From: Jacob Keller <[e-mail]>

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 <[e-mail]>
Signed-off-by: Jacob Keller <[e-mail]>
Signed-off-by: Karol Kolacinski <[e-mail]>
---
 drivers/net/ethernet/intel/Kconfig            |  10 +-
 .../net/ethernet/intel/ice/ice_hw_autogen.h   |   8 +
 drivers/net/ethernet/intel/ice/ice_main.c     |   7 +
 drivers/net/ethernet/intel/ice/ice_osdep.h    |   3 +
 drivers/net/ethernet/intel/ice/ice_ptp.c      | 242 +++++++++++++-----
 5 files changed, 194 insertions(+), 76 deletions(-)

Comments

Jacob Keller July 25, 2024, 4:31 p.m. UTC | #1

On 7/25/2024 2:34 AM, Karol Kolacinski wrote:
> From: Jacob Keller <[e-mail]>
> 
> 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.
> 

The commit message could probably be updated since I think this is
referring to E830 code which is only in our internal branch. I guess you
must have squashed this together with the original E830 implementation
rather than sending that up and then this refactor/cleanup?

Not a huge deal, but it is somewhat confusing when reading the actual
patch code.

Thanks,
Jake

Alexander Lobakin July 26, 2024, 1:37 p.m. UTC | #2

From: Karol Kolacinski <[e-mail]>
Date: Thu, 25 Jul 2024 11:34:51 +0200

> From: Jacob Keller <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Jacob Keller <[e-mail]>
> Signed-off-by: Karol Kolacinski <[e-mail]>

[...]

> 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

Jacob Keller July 26, 2024, 11:16 p.m. UTC | #3

On 7/26/2024 6:37 AM, Alexander Lobakin wrote:
>> 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.
> 

Please move the include to ice_osdep.h, as there are some other places
where we will want to use rd32_poll_timeout (such as in ice_controlq.c,
and others).

Thanks,
Jake

Karol Kolacinski Aug. 5, 2024, 4:21 p.m. UTC | #4

From: Aleksander Lobakin <[e-mail]>
Date: Fri, 26 Jul 2024 15:37 +0200
> > +/**
> > + * 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.

For PTM, we check only pcie_ptm_enabled(). PTM is a PCIE feature
supported regardless of arch.

The two other checks are for the x86 Always Running Timer (ART) and x86
TimeStamp Counter (TSC) features. Those are not tied to PTM, but are
necessary for crosstimestamping on devices supported by ice driver.

I guess I can remove checks from E82X since all of those are SoC, so
HW always supports this.

For E830, I see no other way, than to check the ART feature. This is
what the HW latches in its registers.
I think we could drop TSC_KNOWN_FREQ check since there's new logic
around get_device_system_crosststamp() and cycles conversion.

Thanks,
Karol

Alexander Lobakin Aug. 7, 2024, 1:54 p.m. UTC | #5

From: Kolacinski, Karol <[e-mail]>
Date: Mon, 5 Aug 2024 18:21:39 +0200

> From: Aleksander Lobakin <[e-mail]>
> Date: Fri, 26 Jul 2024 15:37 +0200
>>> +/**
>>> + * 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.
> 
> For PTM, we check only pcie_ptm_enabled(). PTM is a PCIE feature
> supported regardless of arch.
> 
> The two other checks are for the x86 Always Running Timer (ART) and x86
> TimeStamp Counter (TSC) features. Those are not tied to PTM, but are
> necessary for crosstimestamping on devices supported by ice driver.

Ah okay, it's not tied.
So, instead of asm/ptm.h, it should be named somehow else :D

But this X86_FEATURE_ART + X86_FEATURE_TSC_KNOWN_FREQ check really
should be abstracted to something like arch_has_crosststamp() or
arch_has_tstamp(), dunno. Maybe to the already existing asm/timex.h?
Then, implementing this for ARM64 would be easier, as instead of adding
more ifdefs and checks you'd just implement arch_has_tstamp() in its
timex.h (I've seen Milena'd been playing with PTP on ARM).
At least that's how I see it. But if it's fine for the maintainers to
have arch-specific ifdefs and the same code pattern in several drivers,
I'm fine, too :D

> 
> I guess I can remove checks from E82X since all of those are SoC, so
> HW always supports this.
> 
> For E830, I see no other way, than to check the ART feature. This is
> what the HW latches in its registers.
> I think we could drop TSC_KNOWN_FREQ check since there's new logic
> around get_device_system_crosststamp() and cycles conversion.
> 
> Thanks,
> Karol

Thanks,
Olek

Karol Kolacinski Aug. 7, 2024, 2:26 p.m. UTC | #6

From: Aleksander Lobakin <[e-mail]>
Date: Wed, 07 Aug 2024 15:54 +0200
>>>> +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.
>>
>> For PTM, we check only pcie_ptm_enabled(). PTM is a PCIE feature
>> supported regardless of arch.
>>
>> The two other checks are for the x86 Always Running Timer (ART) and x86
>> TimeStamp Counter (TSC) features. Those are not tied to PTM, but are
>> necessary for crosstimestamping on devices supported by ice driver.
>
> Ah okay, it's not tied.
> So, instead of asm/ptm.h, it should be named somehow else :D
>
> But this X86_FEATURE_ART + X86_FEATURE_TSC_KNOWN_FREQ check really
> should be abstracted to something like arch_has_crosststamp() or
> arch_has_tstamp(), dunno. Maybe to the already existing asm/timex.h?
> Then, implementing this for ARM64 would be easier, as instead of adding
> more ifdefs and checks you'd just implement arch_has_tstamp() in its
> timex.h (I've seen Milena'd been playing with PTP on ARM).
> At least that's how I see it. But if it's fine for the maintainers to
> have arch-specific ifdefs and the same code pattern in several drivers,
> I'm fine, too :D

Technically, neither ART nor TSC are directly related to the PTP cross
timestamp. It's just the implementation on Intel NICs, where those
NICs use x86 ART to crosstimestamp.

For cross timestamp on ARM, it's also HW specific and depends on which
timer the HW uses for timestamping. I'm not really sure what's the HW
protocol in this case and if e.g. E830 can latch other timers than
x86 ART in its ART_TIME registers.

get_device_system_crosststamp() supports multiple clock sources defined
in enum clocksource_ids. Maybe instead of checking ART flag, the driver
could get clocksources and if CSID_X86_ART is available, it would assign
the pointer to crosststamp function, but I'm not convinced.

Thanks,
Karol

Alexander Lobakin Aug. 8, 2024, 1 p.m. UTC | #7

From: Kolacinski, Karol <[e-mail]>
Date: Wed, 7 Aug 2024 16:26:29 +0200

> From: Aleksander Lobakin <[e-mail]>
> Date: Wed, 07 Aug 2024 15:54 +0200
>>>>> +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.
>>>
>>> For PTM, we check only pcie_ptm_enabled(). PTM is a PCIE feature
>>> supported regardless of arch.
>>>
>>> The two other checks are for the x86 Always Running Timer (ART) and x86
>>> TimeStamp Counter (TSC) features. Those are not tied to PTM, but are
>>> necessary for crosstimestamping on devices supported by ice driver.
>>
>> Ah okay, it's not tied.
>> So, instead of asm/ptm.h, it should be named somehow else :D
>>
>> But this X86_FEATURE_ART + X86_FEATURE_TSC_KNOWN_FREQ check really
>> should be abstracted to something like arch_has_crosststamp() or
>> arch_has_tstamp(), dunno. Maybe to the already existing asm/timex.h?
>> Then, implementing this for ARM64 would be easier, as instead of adding
>> more ifdefs and checks you'd just implement arch_has_tstamp() in its
>> timex.h (I've seen Milena'd been playing with PTP on ARM).
>> At least that's how I see it. But if it's fine for the maintainers to
>> have arch-specific ifdefs and the same code pattern in several drivers,
>> I'm fine, too :D
> 
> Technically, neither ART nor TSC are directly related to the PTP cross
> timestamp. It's just the implementation on Intel NICs, where those
> NICs use x86 ART to crosstimestamp.
> 
> For cross timestamp on ARM, it's also HW specific and depends on which
> timer the HW uses for timestamping. I'm not really sure what's the HW
> protocol in this case and if e.g. E830 can latch other timers than
> x86 ART in its ART_TIME registers.
> 
> get_device_system_crosststamp() supports multiple clock sources defined
> in enum clocksource_ids. Maybe instead of checking ART flag, the driver
> could get clocksources and if CSID_X86_ART is available, it would assign
> the pointer to crosststamp function, but I'm not convinced.

I mean, I'm fine with the arch-specific definitions in the driver as
long as the netdev maintainers are fine. Or maybe they could propose
some generic solution.

> 
> Thanks,
> Karol

Thanks,
Olek

Jakub Kicinski Aug. 8, 2024, 2:20 p.m. UTC | #8

On Thu, 8 Aug 2024 15:00:52 +0200 Alexander Lobakin wrote:
> > Technically, neither ART nor TSC are directly related to the PTP cross
> > timestamp. It's just the implementation on Intel NICs, where those
> > NICs use x86 ART to crosstimestamp.
> > 
> > For cross timestamp on ARM, it's also HW specific and depends on which
> > timer the HW uses for timestamping. I'm not really sure what's the HW
> > protocol in this case and if e.g. E830 can latch other timers than
> > x86 ART in its ART_TIME registers.
> > 
> > get_device_system_crosststamp() supports multiple clock sources defined
> > in enum clocksource_ids. Maybe instead of checking ART flag, the driver
> > could get clocksources and if CSID_X86_ART is available, it would assign
> > the pointer to crosststamp function, but I'm not convinced.  
> 
> I mean, I'm fine with the arch-specific definitions in the driver as
> long as the netdev maintainers are fine. Or maybe they could propose
> some generic solution.

I don't like it either, FWIW, but it seems like this is what everyone
is doing. Please do CC tglx / the time maintainers on the next version
and net-next submission. I get the feeling they will wake up in a year
telling us we did it all wrong, but hey, all we can do now is CC them..

Patch

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 0375c7448a57..7c4f3948385c 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -332,14 +332,14 @@  config ICE_SWITCHDEV
 	  If unsure, say N.
 
 config ICE_HWTS
-	bool "Support HW cross-timestamp on platforms with PTM support"
+	bool "Support HW cross-timestamp on supported platforms"
 	default y
-	depends on ICE && X86
+	depends on ICE && X86 && PCIE_PTM
 	help
 	  Say Y to enable hardware supported cross-timestamping on platforms
-	  with PCIe PTM support. The cross-timestamp is available through
-	  the PTP clock driver precise cross-timestamp ioctl
-	  (PTP_SYS_OFFSET_PRECISE).
+	  with PCIe PTM support for E830 devices and all E82X platforms. The
+	  cross-timestamp is available through the PTP clock driver precise
+	  cross-timestamp ioctl (PTP_SYS_OFFSET_PRECISE).
 
 config FM10K
 	tristate "Intel(R) FM10000 Ethernet Switch Host Interface Support"
diff --git a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
index 646089f3e26c..495b182ea13b 100644
--- a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
+++ b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
@@ -541,6 +541,14 @@ 
 #define E830_PRTMAC_CL01_QNT_THR_CL0_M		GENMASK(15, 0)
 #define E830_PRTTSYN_TXTIME_H(_i)		(0x001E5800 + ((_i) * 32))
 #define E830_PRTTSYN_TXTIME_L(_i)		(0x001E5000 + ((_i) * 32))
+#define E830_GLPTM_ART_CTL			0x00088B50
+#define E830_GLPTM_ART_CTL_ACTIVE_M		BIT(0)
+#define E830_GLPTM_ART_TIME_H			0x00088B54
+#define E830_GLPTM_ART_TIME_L			0x00088B58
+#define E830_GLTSYN_PTMTIME_H(_i)		(0x00088B48 + ((_i) * 4))
+#define E830_GLTSYN_PTMTIME_L(_i)		(0x00088B40 + ((_i) * 4))
+#define E830_PFPTM_SEM				0x00088B00
+#define E830_PFPTM_SEM_BUSY_M			BIT(0)
 #define VFINT_DYN_CTLN(_i)			(0x00003800 + ((_i) * 4))
 #define VFINT_DYN_CTLN_CLEARPBA_M		BIT(1)
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 753c942c64f1..e6853c8888ec 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5060,6 +5060,12 @@  static int ice_init(struct ice_pf *pf)
 	if (err)
 		return err;
 
+	if (ice_is_e830(&pf->hw)) {
+		err = pci_enable_ptm(pf->pdev, NULL);
+		if (err)
+			dev_dbg(ice_pf_to_dev(pf), "PCIe PTM not supported by PCIe bus/controller\n");
+	}
+
 	err = ice_alloc_vsis(pf);
 	if (err)
 		goto err_alloc_vsis;
@@ -5290,6 +5296,7 @@  ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	hw->subsystem_device_id = pdev->subsystem_device;
 	hw->bus.device = PCI_SLOT(pdev->devfn);
 	hw->bus.func = PCI_FUNC(pdev->devfn);
+
 	ice_set_ctrlq_len(hw);
 
 	pf->msg_enable = netif_msg_init(debug, ICE_DFLT_NETIF_M);
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>
 #include "ice.h"
 #include "ice_lib.h"
 #include "ice_trace.h"
@@ -2138,91 +2139,154 @@  static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta)
 
 #ifdef CONFIG_ICE_HWTS
 /**
- * ice_ptp_get_syncdevicetime - Get the cross time stamp info
+ * struct ice_crosststamp_cfg - Device cross timestamp configuration
+ * @lock_reg: The hardware semaphore lock to use
+ * @lock_busy: Bit in the semaphore lock indicating the lock is busy
+ * @ctl_reg: The hardware register to request cross timestamp
+ * @ctl_active: Bit in the control register to request cross timestamp
+ * @art_time_l: Lower 32-bits of ART system time
+ * @art_time_h: Upper 32-bits of ART system time
+ * @dev_time_l: Lower 32-bits of device time (per timer index)
+ * @dev_time_h: Upper 32-bits of device time (per timer index)
+ */
+struct ice_crosststamp_cfg {
+	/* HW semaphore lock register */
+	u32 lock_reg;
+	u32 lock_busy;
+
+	/* Capture control register */
+	u32 ctl_reg;
+	u32 ctl_active;
+
+	/* Time storage */
+	u32 art_time_l;
+	u32 art_time_h;
+	u32 dev_time_l[2];
+	u32 dev_time_h[2];
+};
+
+static const struct ice_crosststamp_cfg ice_crosststamp_cfg_e82x = {
+	.lock_reg = PFHH_SEM,
+	.lock_busy = PFHH_SEM_BUSY_M,
+	.ctl_reg = GLHH_ART_CTL,
+	.ctl_active = GLHH_ART_CTL_ACTIVE_M,
+	.art_time_l = GLHH_ART_TIME_L,
+	.art_time_h = GLHH_ART_TIME_H,
+	.dev_time_l[0] = GLTSYN_HHTIME_L(0),
+	.dev_time_h[0] = GLTSYN_HHTIME_H(0),
+	.dev_time_l[1] = GLTSYN_HHTIME_L(1),
+	.dev_time_h[1] = GLTSYN_HHTIME_H(1),
+};
+
+static const struct ice_crosststamp_cfg ice_crosststamp_cfg_e830 = {
+	.lock_reg = E830_PFPTM_SEM,
+	.lock_busy = E830_PFPTM_SEM_BUSY_M,
+	.ctl_reg = E830_GLPTM_ART_CTL,
+	.ctl_active = E830_GLPTM_ART_CTL_ACTIVE_M,
+	.art_time_l = E830_GLPTM_ART_TIME_L,
+	.art_time_h = E830_GLPTM_ART_TIME_H,
+	.dev_time_l[0] = E830_GLTSYN_PTMTIME_L(0),
+	.dev_time_h[0] = E830_GLTSYN_PTMTIME_H(0),
+	.dev_time_l[1] = E830_GLTSYN_PTMTIME_L(1),
+	.dev_time_h[1] = E830_GLTSYN_PTMTIME_H(1),
+};
+
+/**
+ * struct ice_crosststamp_ctx - Device cross timestamp context
+ * @snapshot: snapshot of system clocks for historic interpolation
+ * @pf: pointer to the PF private structure
+ * @cfg: pointer to hardware configuration for cross timestamp
+ */
+struct ice_crosststamp_ctx {
+	struct system_time_snapshot snapshot;
+	struct ice_pf *pf;
+	const struct ice_crosststamp_cfg *cfg;
+};
+
+/**
+ * ice_capture_crosststamp - Capture a device/system cross timestamp
  * @device: Current device time
  * @system: System counter value read synchronously with device time
- * @ctx: Context provided by timekeeping code
+ * @__ctx: Context passed from ice_ptp_getcrosststamp
  *
  * Read device and system (ART) clock simultaneously and return the corrected
  * clock values in ns.
+ *
+ * Return: zero on success, or a negative error code on failure.
  */
-static int
-ice_ptp_get_syncdevicetime(ktime_t *device,
-			   struct system_counterval_t *system,
-			   void *ctx)
+static int ice_capture_crosststamp(ktime_t *device,
+				   struct system_counterval_t *system,
+				   void *__ctx)
 {
-	struct ice_pf *pf = (struct ice_pf *)ctx;
-	struct ice_hw *hw = &pf->hw;
-	u32 hh_lock, hh_art_ctl;
-	int i;
+	struct ice_crosststamp_ctx *ctx = __ctx;
+	const struct ice_crosststamp_cfg *cfg;
+	u32 lock, ctl, ts_lo, ts_hi, tmr_idx;
+	struct ice_pf *pf;
+	struct ice_hw *hw;
+	int err;
+	u64 ts;
 
-#define MAX_HH_HW_LOCK_TRIES	5
-#define MAX_HH_CTL_LOCK_TRIES	100
+	cfg = ctx->cfg;
+	pf = ctx->pf;
+	hw = &pf->hw;
 
-	for (i = 0; i < MAX_HH_HW_LOCK_TRIES; i++) {
-		/* Get the HW lock */
-		hh_lock = rd32(hw, PFHH_SEM + (PFTSYN_SEM_BYTES * hw->pf_id));
-		if (hh_lock & PFHH_SEM_BUSY_M) {
-			usleep_range(10000, 15000);
-			continue;
-		}
-		break;
-	}
-	if (hh_lock & PFHH_SEM_BUSY_M) {
-		dev_err(ice_pf_to_dev(pf), "PTP failed to get hh lock\n");
+	tmr_idx = hw->func_caps.ts_func_info.tmr_index_assoc;
+	if (tmr_idx > 1)
+		return -EINVAL;
+
+	/* Poll until we obtain the cross-timestamp hardware semaphore */
+	err = rd32_poll_timeout(hw, cfg->lock_reg, lock,
+				!(lock & cfg->lock_busy),
+				10 * USEC_PER_MSEC, 50 * USEC_PER_MSEC);
+	if (err) {
+		dev_err(ice_pf_to_dev(pf), "PTP failed to get cross timestamp lock\n");
 		return -EBUSY;
 	}
 
+	/* Snapshot system time for historic interpolation */
+	ktime_get_snapshot(&ctx->snapshot);
+
 	/* Program cmd to master timer */
 	ice_ptp_src_cmd(hw, ICE_PTP_READ_TIME);
 
 	/* Start the ART and device clock sync sequence */
-	hh_art_ctl = rd32(hw, GLHH_ART_CTL);
-	hh_art_ctl = hh_art_ctl | GLHH_ART_CTL_ACTIVE_M;
-	wr32(hw, GLHH_ART_CTL, hh_art_ctl);
-
-	for (i = 0; i < MAX_HH_CTL_LOCK_TRIES; i++) {
-		/* Wait for sync to complete */
-		hh_art_ctl = rd32(hw, GLHH_ART_CTL);
-		if (hh_art_ctl & GLHH_ART_CTL_ACTIVE_M) {
-			udelay(1);
-			continue;
-		} else {
-			u32 hh_ts_lo, hh_ts_hi, tmr_idx;
-			u64 hh_ts;
-
-			tmr_idx = hw->func_caps.ts_func_info.tmr_index_assoc;
-			/* Read ART time */
-			hh_ts_lo = rd32(hw, GLHH_ART_TIME_L);
-			hh_ts_hi = rd32(hw, GLHH_ART_TIME_H);
-			hh_ts = ((u64)hh_ts_hi << 32) | hh_ts_lo;
-			system->cycles = hh_ts;
-			system->cs_id = CSID_X86_ART;
-			/* Read Device source clock time */
-			hh_ts_lo = rd32(hw, GLTSYN_HHTIME_L(tmr_idx));
-			hh_ts_hi = rd32(hw, GLTSYN_HHTIME_H(tmr_idx));
-			hh_ts = ((u64)hh_ts_hi << 32) | hh_ts_lo;
-			*device = ns_to_ktime(hh_ts);
-			break;
-		}
-	}
+	ctl = rd32(hw, cfg->ctl_reg);
+	ctl |= cfg->ctl_active;
+	wr32(hw, cfg->ctl_reg, ctl);
 
+	/* Poll until hardware completes the capture */
+	err = rd32_poll_timeout(hw, cfg->ctl_reg, ctl, !(ctl & cfg->ctl_active),
+				5, 20 * USEC_PER_MSEC);
+	if (err)
+		goto err_timeout;
+
+	/* Read ART system time */
+	ts_lo = rd32(hw, cfg->art_time_l);
+	ts_hi = rd32(hw, cfg->art_time_h);
+	ts = ((u64)ts_hi << 32) | ts_lo;
+	system->cycles = ts;
+	system->cs_id = CSID_X86_ART;
+
+	/* Read Device source clock time */
+	ts_lo = rd32(hw, cfg->dev_time_l[tmr_idx]);
+	ts_hi = rd32(hw, cfg->dev_time_h[tmr_idx]);
+	ts = ((u64)ts_hi << 32) | ts_lo;
+	*device = ns_to_ktime(ts);
+
+err_timeout:
 	/* Clear the master timer */
 	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
 
 	/* Release HW lock */
-	hh_lock = rd32(hw, PFHH_SEM + (PFTSYN_SEM_BYTES * hw->pf_id));
-	hh_lock = hh_lock & ~PFHH_SEM_BUSY_M;
-	wr32(hw, PFHH_SEM + (PFTSYN_SEM_BYTES * hw->pf_id), hh_lock);
-
-	if (i == MAX_HH_CTL_LOCK_TRIES)
-		return -ETIMEDOUT;
+	lock = rd32(hw, cfg->lock_reg);
+	lock &= ~cfg->lock_busy;
+	wr32(hw, cfg->lock_reg, lock);
 
-	return 0;
+	return err;
 }
 
 /**
- * ice_ptp_getcrosststamp_e82x - Capture a device cross timestamp
+ * ice_ptp_getcrosststamp - Capture a device cross timestamp
  * @info: the driver's PTP info structure
  * @cts: The memory to fill the cross timestamp info
  *
@@ -2230,23 +2294,35 @@  ice_ptp_get_syncdevicetime(ktime_t *device,
  * clock. Fill the cross timestamp information and report it back to the
  * caller.
  *
- * This is only valid for E822 and E823 devices which have support for
- * generating the cross timestamp via PCIe PTM.
- *
  * In order to correctly correlate the ART timestamp back to the TSC time, the
  * CPU must have X86_FEATURE_TSC_KNOWN_FREQ.
+ *
+ * Return: zero on success, or a negative error code on failure.
  */
-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;
+
+	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 */
+
+	/* Rest of the config is the same as base E810 */
+	ice_ptp_set_funcs_e810(pf);
+}
+
 /**
  * ice_ptp_set_caps - Set PTP capabilities
  * @pf: Board private structure
@@ -2614,8 +2712,10 @@  static void ice_ptp_set_caps(struct ice_pf *pf)
 	info->enable = ice_ptp_gpio_enable;
 	info->verify = ice_verify_pin;
 
-	if (ice_is_e810(&pf->hw) || ice_is_e830(&pf->hw))
+	if (ice_is_e810(&pf->hw))
 		ice_ptp_set_funcs_e810(pf);
+	if (ice_is_e830(&pf->hw))
+		ice_ptp_set_funcs_e830(pf);
 	else
 		ice_ptp_set_funcs_e82x(pf);
 }