From: Karol Kolacinski <[email protected]> Date: Thu, 25 Jul 2024 11:34:51 +0200
> From: Jacob Keller <[email protected]> > > 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 <[email protected]> > Signed-off-by: Jacob Keller <[email protected]> > Signed-off-by: Karol Kolacinski <[email protected]> [...] > 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
