From: Kolacinski, Karol <karol.kolacin...@intel.com>
Date: Mon, 5 Aug 2024 18:21:39 +0200
> From: Aleksander Lobakin <aleksander.loba...@intel.com>
> 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
