[Intel-wired-lan] [PATCH v3 iwl-next 4/4] ice: combine cross timestamp functions for E82x and E830

[Alexander Lobakin]

From: Kolacinski, Karol <karol.kolacinski at intel.com>
Date: Wed, 7 Aug 2024 16:26:29 +0200

> From: Aleksander Lobakin <aleksander.lobakin at intel.com>
> 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