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

From: Karol Kolacinski <redacted>
Date: Thu, 25 Jul 2024 11:34:51 +0200

From: Jacob Keller <jacob.e.keller@intel.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@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Karol Kolacinski <redacted>

[...]

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,
	};

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