* [PATCH net-next 0/3][pull request] ice: expose TSPLL state on E825 through dpll subsystem
@ 2026-09-08 22:24 Tony Nguyen
2026-09-08 22:24 ` [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker Tony Nguyen
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Tony Nguyen @ 2026-09-08 22:24 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Tony Nguyen, grzegorz.nitka, arkadiusz.kubalewski,
przemyslaw.korba, richardcochran
Grzegorz Nitka says:
On E825 devices that own the source timer, the TSPLL can lose lock when
the TCXO or TIME_REF signal is disrupted. This series adds monitoring
and recovery for that condition, then surfaces the lock status and clock
source selection through the dpll subsystem. It also fixes the clock_id
generation for E825 generic DPLL devices so userspace can unambiguously
map each DPLL device to its owning interface.
Patch 1 adds TSPLL lock monitoring to ice_ptp_periodic_work(). Placing it
there ensures recovery runs regardless of whether DPLL init succeeded or
CONFIG_DPLL is enabled. Lock state is cached in pf->ptp.tspll_locked via
WRITE_ONCE()/READ_ONCE() for consumption by the DPLL worker.
Patch 2 registers the TSPLL as a DPLL_TYPE_GENERIC device for E825 owner
PFs, with a fwnode-backed "time_ref" input pin. The pin state_on_dpll_set
callback switches the clock source between TIME_REF and TCXO. Lock status
is read from pf->ptp.tspll_locked; UNLOCKED is reported unconditionally
when the clock source is TCXO to reflect the free-running oscillator state.
Patch 3 changes the clock_id generation for E825 TX-CLK and TSPLL DPLLs.
Previously they used the board-level PCIe DSN, which is identical for
all interfaces sharing the same NAC/quad, so userspace could not tell
them apart. The new scheme derives the clock_id from the permanent port
MAC (with a dedicated tag bit distinguishing TSPLL from TX-CLK), while
other DPLL objects (EEC/PPS and non-E825 paths) keep the board-level
DSN-derived clock_id.
---
IWL: https://lore.kernel.org/all/20260803113507.1858083-1-grzegorz.nitka@intel.com/
The following are changes since commit ab217fbb9b2169ce677b09a66558d5c3adcfbb76:
Merge branch 'net-sysfs-use-ops-lock-for-speed-and-duplex'
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 100GbE
Grzegorz Nitka (3):
ice: monitor TSPLL lock from PTP periodic worker
ice: add TSPLL DPLL device and TIME_REF pin for E825
ice: use per-interface clock_id for E825 generic DPLLs
drivers/net/ethernet/intel/ice/ice_dpll.c | 493 ++++++++++++++++++---
drivers/net/ethernet/intel/ice/ice_dpll.h | 4 +
drivers/net/ethernet/intel/ice/ice_main.c | 9 +
drivers/net/ethernet/intel/ice/ice_ptp.c | 82 ++++
drivers/net/ethernet/intel/ice/ice_ptp.h | 11 +
drivers/net/ethernet/intel/ice/ice_tspll.c | 120 ++++-
drivers/net/ethernet/intel/ice/ice_tspll.h | 6 +
7 files changed, 669 insertions(+), 56 deletions(-)
--
2.47.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker
2026-09-08 22:24 [PATCH net-next 0/3][pull request] ice: expose TSPLL state on E825 through dpll subsystem Tony Nguyen
@ 2026-09-08 22:24 ` Tony Nguyen
2026-09-11 10:27 ` netdev-bot+sashiko
2026-09-08 22:24 ` [PATCH net-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825 Tony Nguyen
2026-09-08 22:24 ` [PATCH net-next 3/3] ice: use per-interface clock_id for E825 generic DPLLs Tony Nguyen
2 siblings, 1 reply; 7+ messages in thread
From: Tony Nguyen @ 2026-09-08 22:24 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Grzegorz Nitka, anthony.l.nguyen, arkadiusz.kubalewski,
przemyslaw.korba, richardcochran, Alexander Nowlin
From: Grzegorz Nitka <grzegorz.nitka@intel.com>
On E825 devices that own the source timer, the TSPLL can lose lock when
the TCXO or TIME_REF signal is disrupted. Recovery requires re-enabling
the TSPLL via CGU register writes; without it, the PHC keeps running on
a degraded reference indefinitely.
The DPLL periodic worker (ice_dpll_periodic_work()) would be a natural
home for this monitoring, but placing it there has two problems:
1. ice_dpll_init_e825() sets ICE_FLAG_DPLL only after all initialization
steps succeed. If any earlier step fails, the driver would run
without any TSPLL recovery mechanism.
2. When CONFIG_DPLL=n, the DPLL worker is compiled out and TSPLL
recovery would be silently absent.
Add the monitor to ice_ptp_periodic_work() instead, which always runs on
E825 owner PFs regardless of DPLL init state or config. Introduce two
small helpers, ice_tspll_lost_lock_e825c() and ice_tspll_restart_e825c(),
which encapsulate the CGU register reads/writes required to observe and
recover the TSPLL.
Cache the observed lock state in pf->ptp.tspll_locked using
WRITE_ONCE()/READ_ONCE() so a follow-up change can consume it from the
DPLL periodic worker (for user-space notification via
dpll_device_change_ntf()) and drop the redundant poll+recovery from
that path. Precise synchronization is not required: both workers converge
on the same value within one poll period.
Reviewed-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 70 ++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_ptp.h | 11 +++
drivers/net/ethernet/intel/ice/ice_tspll.c | 86 +++++++++++++++++++++-
drivers/net/ethernet/intel/ice/ice_tspll.h | 4 +
4 files changed, 170 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index eaec36ab6ae3..4b1040d09f70 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -4,8 +4,11 @@
#include "ice.h"
#include "ice_lib.h"
#include "ice_trace.h"
+#include "ice_tspll.h"
#include "ice_txclk.h"
+#define ICE_TSPLL_LOG_INTERVAL 120
+
static const char ice_pin_names[][64] = {
"SDP0",
"SDP1",
@@ -2849,6 +2852,67 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
}
}
+/**
+ * ice_ptp_tspll_monitor - poll and recover TSPLL lock on E825 owner PFs
+ * @pf: Board private structure
+ *
+ * Called from the PTP periodic worker. On E825 devices that own the source
+ * timer, poll the TSPLL lock status via CGU registers and trigger a restart
+ * if the lock has been lost. The result is cached in @pf->ptp.tspll_locked
+ * so it can be consumed by the DPLL periodic worker via READ_ONCE().
+ *
+ * TSPLL lock is critical for PHC operation and must be monitored regardless
+ * of whether DPLL init succeeded or CONFIG_DPLL is enabled. Placing the
+ * monitor here makes recovery independent of the dpll subsystem.
+ *
+ * AQ read errors are rate-limited and do not stop monitoring. Lock-lost
+ * events are logged every 120 retries (~60 s at normal poll rate) to
+ * surface persistent failures without flooding the log.
+ */
+static void ice_ptp_tspll_monitor(struct ice_pf *pf)
+{
+ bool lock_lost;
+ int err;
+
+ if (pf->hw.mac_type != ICE_MAC_GENERIC_3K_E825 ||
+ !ice_pf_src_tmr_owned(pf))
+ return;
+
+ err = ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost);
+ if (err) {
+ dev_err_ratelimited(ice_pf_to_dev(pf),
+ "Failed reading TimeSync PLL lock status (err: %d). Retrying.\n",
+ err);
+ return;
+ }
+
+ if (lock_lost) {
+ WRITE_ONCE(pf->ptp.tspll_locked, false);
+ if (!(pf->ptp.tspll_lock_retries % ICE_TSPLL_LOG_INTERVAL))
+ dev_warn(ice_pf_to_dev(pf),
+ "TimeSync PLL lock lost. Retrying to acquire lock.\n");
+ err = ice_tspll_restart_e825c(&pf->hw);
+ if (err)
+ dev_err_ratelimited(ice_pf_to_dev(pf),
+ "Failed to restart TimeSync PLL (err: %d).\n",
+ err);
+ pf->ptp.tspll_lock_retries++;
+ } else {
+ if (pf->ptp.tspll_lock_retries) {
+ const char *src_str = "unknown";
+ enum ice_clk_src clk_src;
+
+ if (!ice_tspll_get_clk_src(&pf->hw, &clk_src))
+ src_str = ice_tspll_clk_src_str(clk_src);
+ dev_info(ice_pf_to_dev(pf),
+ "TimeSync PLL lock acquired with %s clock source after %u retries.\n",
+ src_str, pf->ptp.tspll_lock_retries);
+ }
+ WRITE_ONCE(pf->ptp.tspll_locked, true);
+ pf->ptp.tspll_lock_retries = 0;
+ }
+}
+
static void ice_ptp_periodic_work(struct kthread_work *work)
{
struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
@@ -2858,6 +2922,8 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
if (pf->ptp.state != ICE_PTP_READY)
return;
+ ice_ptp_tspll_monitor(pf);
+
err = ice_ptp_update_cached_phctime(pf);
ice_ptp_maybe_trigger_tx_interrupt(pf);
@@ -2975,6 +3041,9 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
err = ice_tspll_init(hw);
if (err)
return err;
+ /* Rebuild reinitialized TSPLL, so reset monitor retry state. */
+ WRITE_ONCE(ptp->tspll_locked, true);
+ ptp->tspll_lock_retries = 0;
/* Acquire the global hardware lock */
if (!ice_ptp_lock(hw)) {
@@ -3319,6 +3388,7 @@ void ice_ptp_init(struct ice_pf *pf)
}
ptp->port.port_num = hw->lane_num;
+ ptp->tspll_locked = true;
ice_ptp_init_hw(hw);
ice_ptp_init_tx_interrupt_mode(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index c4b0da7ce20e..0e40fef3b4e8 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -249,6 +249,15 @@ struct ice_ptp_pin_desc {
* @tx_hwtstamp_discarded: number of Tx skbs discarded due to cached PHC time
* being too old to correctly extend timestamp
* @late_cached_phc_updates: number of times cached PHC update is late
+ * @tspll_locked: last observed TSPLL lock state on E825 owner PFs.
+ * Written by the PTP periodic worker after polling the TSPLL and
+ * intended to be read (without pf->dplls.lock) by the DPLL periodic
+ * worker in a follow-up change. Access via READ_ONCE()/WRITE_ONCE();
+ * precise synchronization is not required because both workers
+ * converge on the same value within one poll period.
+ * @tspll_lock_retries: counts consecutive poll cycles in which the TSPLL
+ * was found unlocked. Reset to zero when lock is re-acquired. Used to
+ * rate-limit the lock-lost log message (~every 120 retries / ~60 s).
*/
struct ice_ptp {
enum ice_ptp_state state;
@@ -274,6 +283,8 @@ struct ice_ptp {
u32 tx_hwtstamp_flushed;
u32 tx_hwtstamp_discarded;
u32 late_cached_phc_updates;
+ bool tspll_locked;
+ u32 tspll_lock_retries;
};
#define __ptp_port_to_ptp(p) \
diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
index fd4b58eb9bc0..78752dc1e762 100644
--- a/drivers/net/ethernet/intel/ice/ice_tspll.c
+++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
@@ -129,7 +129,7 @@ static bool ice_tspll_check_params(struct ice_hw *hw,
*
* Return: specified clock source converted to its string name
*/
-static const char *ice_tspll_clk_src_str(enum ice_clk_src clk_src)
+const char *ice_tspll_clk_src_str(enum ice_clk_src clk_src)
{
switch (clk_src) {
case ICE_CLK_SRC_TCXO:
@@ -531,6 +531,62 @@ int ice_tspll_cfg_pps_out_e825c(struct ice_hw *hw, bool enable)
return ice_write_cgu_reg(hw, ICE_CGU_R9, val);
}
+/**
+ * ice_tspll_lost_lock_e825c - check if TSPLL lost lock
+ * @hw: Pointer to the HW struct
+ * @lost_lock: Output flag for reporting lost lock
+ *
+ * Get E825 device TSPLL DPLL lock status.
+ *
+ * Return:
+ * * 0 - OK
+ * * negative - error
+ */
+int ice_tspll_lost_lock_e825c(struct ice_hw *hw, bool *lost_lock)
+{
+ u32 val;
+ int err;
+
+ err = ice_read_cgu_reg(hw, ICE_CGU_RO_LOCK, &val);
+ if (err)
+ return err;
+
+ *lost_lock = !FIELD_GET(ICE_CGU_RO_LOCK_TRUE_LOCK, val);
+
+ return 0;
+}
+
+/**
+ * ice_tspll_restart_e825c - trigger TSPLL restart
+ * @hw: Pointer to the HW struct
+ *
+ * Re-enable TSPLL for E825 device.
+ *
+ * Return:
+ * * 0 - OK
+ * * negative - error
+ */
+int ice_tspll_restart_e825c(struct ice_hw *hw)
+{
+ u32 val;
+ int err;
+
+ /* Read the initial values of r23 and disable the PLL */
+ err = ice_read_cgu_reg(hw, ICE_CGU_R23, &val);
+ if (err)
+ return err;
+
+ val &= ~ICE_CGU_R23_R24_TSPLL_ENABLE;
+ err = ice_write_cgu_reg(hw, ICE_CGU_R23, val);
+ if (err)
+ return err;
+
+ /* Wait at least 1 ms before reenabling PLL */
+ usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
+ val |= ICE_CGU_R23_R24_TSPLL_ENABLE;
+ return ice_write_cgu_reg(hw, ICE_CGU_R23, val);
+}
+
/**
* ice_tspll_cfg - Configure the Clock Generation Unit TSPLL
* @hw: Pointer to the HW struct
@@ -577,6 +633,34 @@ static int ice_tspll_dis_sticky_bits(struct ice_hw *hw)
}
}
+/**
+ * ice_tspll_get_clk_src - get current TSPLL clock source
+ * @hw: board private hw structure
+ * @clk_src: pointer to store clk_src value
+ *
+ * Get current TSPLL clock source settings.
+ *
+ * Return:
+ * * 0 - OK
+ * * negative - error
+ */
+int ice_tspll_get_clk_src(struct ice_hw *hw, enum ice_clk_src *clk_src)
+{
+ u32 val;
+ int err;
+
+ err = (hw->mac_type == ICE_MAC_GENERIC_3K_E825) ?
+ ice_read_cgu_reg(hw, ICE_CGU_R23, &val) :
+ ice_read_cgu_reg(hw, ICE_CGU_R24, &val);
+ if (err)
+ return err;
+
+ *clk_src = (enum ice_clk_src)FIELD_GET(ICE_CGU_R23_R24_TIME_REF_SEL,
+ val);
+
+ return 0;
+}
+
/**
* ice_tspll_init - Initialize TSPLL with settings from firmware
* @hw: Pointer to the HW structure
diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.h b/drivers/net/ethernet/intel/ice/ice_tspll.h
index d650867004d1..05917ae51ded 100644
--- a/drivers/net/ethernet/intel/ice/ice_tspll.h
+++ b/drivers/net/ethernet/intel/ice/ice_tspll.h
@@ -32,6 +32,9 @@ struct ice_tspll_params_e82x {
#define ICE_TSPLL_FBDIV_INTGR_E825 256
int ice_tspll_cfg_pps_out_e825c(struct ice_hw *hw, bool enable);
+int ice_tspll_lost_lock_e825c(struct ice_hw *hw, bool *lost_lock);
+int ice_tspll_restart_e825c(struct ice_hw *hw);
+int ice_tspll_get_clk_src(struct ice_hw *hw, enum ice_clk_src *clk_src);
int ice_tspll_init(struct ice_hw *hw);
int ice_tspll_bypass_mux_active_e825c(struct ice_hw *hw, u8 port, bool *active,
enum ice_synce_clk output);
@@ -39,4 +42,5 @@ int ice_tspll_cfg_bypass_mux_e825c(struct ice_hw *hw, bool ena, u32 port_num,
enum ice_synce_clk output);
int ice_tspll_cfg_synce_ethdiv_e825c(struct ice_hw *hw,
enum ice_synce_clk output);
+const char *ice_tspll_clk_src_str(enum ice_clk_src clk_src);
#endif /* _ICE_TSPLL_H_ */
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825
2026-09-08 22:24 [PATCH net-next 0/3][pull request] ice: expose TSPLL state on E825 through dpll subsystem Tony Nguyen
2026-09-08 22:24 ` [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker Tony Nguyen
@ 2026-09-08 22:24 ` Tony Nguyen
2026-09-11 10:27 ` netdev-bot+sashiko
2026-09-08 22:24 ` [PATCH net-next 3/3] ice: use per-interface clock_id for E825 generic DPLLs Tony Nguyen
2 siblings, 1 reply; 7+ messages in thread
From: Tony Nguyen @ 2026-09-08 22:24 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Grzegorz Nitka, anthony.l.nguyen, arkadiusz.kubalewski,
przemyslaw.korba, richardcochran, Alexander Nowlin
From: Grzegorz Nitka <grzegorz.nitka@intel.com>
This extends the E825 advanced sync-timing support introduced by the
tx-clk series, which added the TXC DPLL device for TX reference clock
control. The TSPLL, the source timer PLL, is now also exposed through
the dpll subsystem so that its lock status and clock source selection
are visible and controllable from userspace.
On E825 devices the TSPLL is the source timer PLL, distinct from the
EEC and PPS DPLLs used on E810. Register it as a DPLL_TYPE_GENERIC
device for owner PFs.
Add struct ice_dplls::tspll_in, a fwnode-backed input pin named
"time_ref". The state_on_dpll_get callback queries ICE_CGU_R23 via
ice_tspll_get_clk_src() and returns CONNECTED when TIME_REF is
selected as clock source, DISCONNECTED otherwise. The state_on_dpll_set
callback switches the source between TIME_REF and TCXO via the new
ice_tspll_set_cfg() helper. Registration is deferred via the dpll
notifier path if the pin is not yet visible in the subsystem at probe
time.
Initialize TSPLL DPLL state from direct clock-source/lock reads so the
first published state reflects hardware and prev_dpll_state matches.
During periodic polling, the DPLL worker consumes
READ_ONCE(pf->ptp.tspll_locked), maintained and recovered by the PTP
periodic worker. When the TSPLL clock source is TCXO (TIME_REF pin not
selected), UNLOCKED is reported unconditionally to reflect the
free-running state of the oscillator regardless of the raw lock bit.
To avoid stale lock-status reads after synchronous source changes, the
set callback now refreshes tspll.dpll_state immediately and emits a DPLL
change notification when the cached state changed.
If a TSPLL reconfiguration is applied but the PLL has not yet
re-acquired lock, treat the internal -EAGAIN result as success so the
PTP periodic worker can complete recovery, while real -EBUSY failures
from reset/SBQ paths still propagate to userspace.
The TSPLL userspace reconfiguration path (state_on_dpll_set) and the
PTP periodic worker (ice_ptp_tspll_monitor) both read TSPLL HW state
and write the pf->ptp.tspll_locked cache consumed by the DPLL worker.
Serialize both with pf->dplls.lock across their HW-read -> cache-write
sequence; otherwise a preempted monitor could overwrite an accurate
cache update from the DPLL callback with stale HW state. The mutex
lifetime is lifted to PF-features scope (initialized in
ice_init_features() before ice_ptp_init(), destroyed in
ice_deinit_features() after ice_ptp_release()) so the PTP monitor
can take it unconditionally regardless of DPLL init state.
Extend ice_dpll_deinit_txclk_pins() with a "flush" parameter so the
E825 init error path for the TSPLL fwnode pin can tear down TXCLK
pins without flushing pf->dplls.wq. If the flush ran here, notifier
work items queued during earlier init steps would be blocked on
pf->dplls.dpll_init, which is only completed at the unregister_pins
label reached after this teardown. destroy_workqueue() at that label
drains the queued items safely. Existing full-teardown callers pass
flush=true and keep current behavior.
Reviewed-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_dpll.c | 413 ++++++++++++++++++---
drivers/net/ethernet/intel/ice/ice_dpll.h | 4 +
drivers/net/ethernet/intel/ice/ice_main.c | 9 +
drivers/net/ethernet/intel/ice/ice_ptp.c | 12 +
drivers/net/ethernet/intel/ice/ice_tspll.c | 34 +-
drivers/net/ethernet/intel/ice/ice_tspll.h | 2 +
6 files changed, 424 insertions(+), 50 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 85a74cd6ea1f..00e3f7de7810 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -22,6 +22,8 @@
#define E825_RCLK_PARENT_0_PIN_IDX 0
#define E825_RCLK_PARENT_1_PIN_IDX 1
+#define E825_DPLL_TSPLL_BASE_IDX 0
+#define E825_DPLL_TXCLK_BASE_IDX 1
#define ICE_DPLL_PIN_SW_INPUT_ABS(in_idx) \
(ICE_DPLL_SW_PIN_INPUT_BASE_SFP + (in_idx))
@@ -84,6 +86,7 @@ static const char * const ice_dpll_sw_pin_sma[] = { "SMA1", "SMA2" };
static const char * const ice_dpll_sw_pin_ufl[] = { "U.FL1", "U.FL2" };
static const char * const ice_dpll_ext_eref_pin = "EXT_EREF0";
static const char * const ice_dpll_fwnode_ext_synce = "clk_ref_synce";
+static const char * const ice_dpll_fwnode_time_ref = "time_ref";
static const struct dpll_pin_frequency ice_esync_range[] = {
DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ),
@@ -2814,6 +2817,160 @@ static const struct dpll_pin_ops ice_dpll_txclk_ops = {
.direction_get = ice_dpll_input_direction,
};
+/**
+ * ice_dpll_tspll_lock_status_get - derive TSPLL state for dpll subsystem
+ * @pf: board private structure
+ * @use_cached: if true, read lock state from pf->ptp.tspll_locked (maintained
+ * by the PTP periodic worker); if false, query hardware directly
+ *
+ * If TIME_REF is not selected, TSPLL is treated as unlocked from the dpll
+ * subsystem perspective, regardless of raw lock bit.
+ *
+ * Return: TSPLL lock status
+ */
+static enum dpll_lock_status
+ice_dpll_tspll_lock_status_get(struct ice_pf *pf, bool use_cached)
+{
+ enum ice_clk_src clk_src;
+ bool locked;
+
+ if (ice_tspll_get_clk_src(&pf->hw, &clk_src) ||
+ clk_src != ICE_CLK_SRC_TIME_REF)
+ return DPLL_LOCK_STATUS_UNLOCKED;
+
+ if (use_cached) {
+ locked = READ_ONCE(pf->ptp.tspll_locked);
+ } else {
+ bool lock_lost;
+
+ if (ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost))
+ return DPLL_LOCK_STATUS_UNLOCKED;
+ locked = !lock_lost;
+ }
+ return locked ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED;
+}
+
+/**
+ * ice_dpll_tspll_state_on_dpll_get - get TIME_REF pin state on TSPLL DPLL
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @state: on success holds pin state on parent dpll
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Returns CONNECTED if the TSPLL is using
+ * TIME_REF as its clock source, DISCONNECTED otherwise.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - failed to read clock source
+ */
+static int
+ice_dpll_tspll_state_on_dpll_get(const struct dpll_pin *pin, void *pin_priv,
+ const struct dpll_device *dpll,
+ void *dpll_priv,
+ enum dpll_pin_state *state,
+ struct netlink_ext_ack *extack)
+{
+ struct ice_dpll_pin *p = pin_priv;
+ enum ice_clk_src clk_src;
+ int err;
+
+ if (ice_dpll_is_reset(p->pf, extack))
+ return -EBUSY;
+
+ err = ice_tspll_get_clk_src(&p->pf->hw, &clk_src);
+ if (err)
+ return err;
+
+ *state = clk_src == ICE_CLK_SRC_TIME_REF ? DPLL_PIN_STATE_CONNECTED :
+ DPLL_PIN_STATE_DISCONNECTED;
+ return 0;
+}
+
+/**
+ * ice_dpll_tspll_state_on_dpll_set - set TIME_REF pin state on TSPLL DPLL
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @state: requested state of the pin
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Enables (CONNECTED) or disables (DISCONNECTED)
+ * the TIME_REF signal as the TSPLL clock source. Selects TCXO when disabled.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_tspll_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
+ const struct dpll_device *dpll,
+ void *dpll_priv, enum dpll_pin_state state,
+ struct netlink_ext_ack *extack)
+{
+ struct ice_dpll_pin *p = pin_priv;
+ struct ice_pf *pf = p->pf;
+ enum ice_clk_src clk_src;
+ bool changed = false;
+ struct ice_dpll *tp;
+ int ret;
+
+ if (ice_dpll_is_reset(pf, extack))
+ return -EBUSY;
+
+ if (state != DPLL_PIN_STATE_CONNECTED &&
+ state != DPLL_PIN_STATE_DISCONNECTED) {
+ NL_SET_ERR_MSG(extack,
+ "unsupported pin state for TSPLL reference clock");
+ return -EINVAL;
+ }
+
+ clk_src = (state == DPLL_PIN_STATE_CONNECTED) ? ICE_CLK_SRC_TIME_REF :
+ ICE_CLK_SRC_TCXO;
+ tp = &pf->dplls.tspll;
+ /* Serialize multi-register TSPLL reconfiguration with restart path. */
+ mutex_lock(&pf->dplls.lock);
+ ret = ice_tspll_set_cfg(&pf->hw, ICE_TSPLL_FREQ_156_250, clk_src);
+ if (!ret || ret == -EAGAIN) {
+ enum dpll_lock_status new_state;
+
+ new_state = ice_dpll_tspll_lock_status_get(pf, false);
+
+ /* Sync the cached lock state to what we just observed from HW
+ * so ice_dpll_periodic_work_e825() (cached path) does not emit
+ * a spurious UNLOCKED notification between now and the next
+ * PTP monitor tick.
+ */
+ WRITE_ONCE(pf->ptp.tspll_locked,
+ new_state == DPLL_LOCK_STATUS_LOCKED);
+
+ if (tp->prev_dpll_state != new_state) {
+ tp->dpll_state = new_state;
+ tp->prev_dpll_state = new_state;
+ changed = true;
+ }
+ }
+ mutex_unlock(&pf->dplls.lock);
+ if (changed)
+ __dpll_device_change_ntf(tp->dpll);
+ /* TSPLL reconfiguration may complete before lock is reacquired.
+ * PTP periodic monitoring tracks and restores the final lock state.
+ */
+ if (ret == -EAGAIN)
+ ret = 0;
+
+ return ret;
+}
+
+static const struct dpll_pin_ops ice_dpll_tspll_in_ops = {
+ .state_on_dpll_set = ice_dpll_tspll_state_on_dpll_set,
+ .state_on_dpll_get = ice_dpll_tspll_state_on_dpll_get,
+ .direction_get = ice_dpll_input_direction,
+};
+
static const struct dpll_pin_ops ice_dpll_pin_sma_ops = {
.state_on_dpll_set = ice_dpll_sma_pin_state_set,
.state_on_dpll_get = ice_dpll_sw_pin_state_get,
@@ -3134,12 +3291,44 @@ ice_dpll_update_state(struct ice_pf *pf, struct ice_dpll *d, bool init)
return ret;
}
+/**
+ * ice_dpll_periodic_work_e825 - TSPLL DPLL periodic update for E825
+ * @pf: board private structure
+ *
+ * Publish TSPLL lock status to the dpll subsystem. The PTP periodic worker
+ * owns TSPLL lock polling and recovery; this function consumes the cached
+ * result for dpll notifications.
+ *
+ * Context: Must be called without pf->dplls.lock held.
+ */
+static void ice_dpll_periodic_work_e825(struct ice_pf *pf)
+{
+ struct ice_dpll *tp = &pf->dplls.tspll;
+ enum dpll_lock_status new_state;
+ bool changed = false;
+
+ mutex_lock(&pf->dplls.lock);
+ new_state = ice_dpll_tspll_lock_status_get(pf, true);
+
+ if (tp->prev_dpll_state != new_state) {
+ tp->dpll_state = new_state;
+ tp->prev_dpll_state = new_state;
+ changed = true;
+ }
+ mutex_unlock(&pf->dplls.lock);
+ if (changed)
+ dpll_device_change_ntf(tp->dpll);
+}
+
/**
* ice_dpll_periodic_work - DPLLs periodic worker
* @work: pointer to kthread_work structure
*
- * DPLLs periodic worker is responsible for polling state of dpll.
- * Context: Holds pf->dplls.lock
+ * Periodic worker responsible for polling DPLL state. On E810 devices it
+ * polls the EEC and PPS DPLLs. On E825 devices, when this PF owns the
+ * source timer, it publishes TSPLL lock status to the dpll subsystem.
+ *
+ * Context: Acquires and releases pf->dplls.lock
*/
static void ice_dpll_periodic_work(struct kthread_work *work)
{
@@ -3147,38 +3336,43 @@ static void ice_dpll_periodic_work(struct kthread_work *work)
struct ice_pf *pf = container_of(d, struct ice_pf, dplls);
struct ice_dpll *de = &pf->dplls.eec;
struct ice_dpll *dp = &pf->dplls.pps;
- u32 phase_offset_ntf = 0;
+ u32 ntf_mask = 0;
int ret = 0;
if (ice_is_reset_in_progress(pf->state))
goto resched;
- mutex_lock(&pf->dplls.lock);
- d->periodic_counter++;
- ret = ice_dpll_update_state(pf, de, false);
- if (!ret)
- ret = ice_dpll_update_state(pf, dp, false);
- if (!ret && dp->phase_offset_monitor_period &&
- d->periodic_counter % dp->phase_offset_monitor_period == 0)
- ret = ice_dpll_pps_update_phase_offsets(pf, &phase_offset_ntf);
- if (ret) {
- /* EBUSY is expected during reset recovery */
- if (pf->hw.adminq.sq_last_status != LIBIE_AQ_RC_EBUSY)
- d->cgu_state_acq_err_num++;
- /* stop rescheduling this worker */
- if (d->cgu_state_acq_err_num >
- ICE_CGU_STATE_ACQ_ERR_THRESHOLD) {
- dev_err(ice_pf_to_dev(pf),
- "EEC/PPS DPLLs periodic work disabled\n");
- mutex_unlock(&pf->dplls.lock);
- return;
+
+ if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
+ ice_dpll_periodic_work_e825(pf);
+ } else {
+ mutex_lock(&pf->dplls.lock);
+ d->periodic_counter++;
+ ret = ice_dpll_update_state(pf, de, false);
+ if (!ret)
+ ret = ice_dpll_update_state(pf, dp, false);
+ if (!ret && dp->phase_offset_monitor_period &&
+ d->periodic_counter % dp->phase_offset_monitor_period == 0)
+ ret = ice_dpll_pps_update_phase_offsets(pf, &ntf_mask);
+ if (ret) {
+ /* EBUSY is expected during reset recovery */
+ if (pf->hw.adminq.sq_last_status != LIBIE_AQ_RC_EBUSY)
+ d->cgu_state_acq_err_num++;
+ /* stop rescheduling this worker */
+ if (d->cgu_state_acq_err_num >
+ ICE_CGU_STATE_ACQ_ERR_THRESHOLD) {
+ dev_err(ice_pf_to_dev(pf),
+ "EEC/PPS DPLLs periodic work disabled\n");
+ mutex_unlock(&pf->dplls.lock);
+ return;
+ }
}
+ mutex_unlock(&pf->dplls.lock);
+ ice_dpll_notify_changes(de);
+ ice_dpll_notify_changes(dp);
+ if (ntf_mask)
+ ice_dpll_pins_notify_mask(d, d->inputs, d->num_inputs,
+ ntf_mask);
}
- mutex_unlock(&pf->dplls.lock);
- ice_dpll_notify_changes(de);
- ice_dpll_notify_changes(dp);
- if (phase_offset_ntf)
- ice_dpll_pins_notify_mask(d, d->inputs, d->num_inputs,
- phase_offset_ntf);
resched:
/* Run twice a second or reschedule if update failed */
@@ -3536,6 +3730,7 @@ static void ice_dpll_pin_notify_work(struct work_struct *work)
work);
struct ice_dpll_pin *pin, *parent = w->pin;
bool is_tx_synce_parent = false;
+ bool is_tspll_time_ref = false;
struct ice_pf *pf = parent->pf;
bool is_rclk_parent = false;
int ret;
@@ -3556,7 +3751,11 @@ static void ice_dpll_pin_notify_work(struct work_struct *work)
is_tx_synce_parent =
ice_dpll_fwnode_eq(parent->fwnode,
pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX].fwnode);
- if (!is_rclk_parent && !is_tx_synce_parent)
+
+ is_tspll_time_ref =
+ ice_dpll_fwnode_eq(parent->fwnode, pf->dplls.tspll_in.fwnode);
+
+ if (!is_rclk_parent && !is_tx_synce_parent && !is_tspll_time_ref)
goto out;
switch (w->action) {
@@ -3597,6 +3796,18 @@ static void ice_dpll_pin_notify_work(struct work_struct *work)
ERR_PTR(ret));
goto drop_parent_ref;
}
+ } else if (is_tspll_time_ref) {
+ /* Register TIME_REF pin directly to TSPLL DPLL */
+ ret = dpll_pin_register(pf->dplls.tspll.dpll,
+ parent->pin,
+ &ice_dpll_tspll_in_ops,
+ parent);
+ if (ret) {
+ dev_err(ice_pf_to_dev(pf),
+ "TSPLL TIME_REF pin register failed: %pe\n",
+ ERR_PTR(ret));
+ goto drop_parent_ref;
+ }
}
break;
case DPLL_PIN_DELETED:
@@ -3615,6 +3826,11 @@ static void ice_dpll_pin_notify_work(struct work_struct *work)
pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
dpll_pin_unregister(pf->dplls.txc.dpll, pin->pin,
&ice_dpll_txclk_ops, pin);
+ } else if (is_tspll_time_ref) {
+ /* Unregister TIME_REF pin from TSPLL DPLL */
+ dpll_pin_unregister(pf->dplls.tspll.dpll,
+ parent->pin,
+ &ice_dpll_tspll_in_ops, parent);
}
drop_parent_ref:
/* Drop fwnode pin reference */
@@ -3786,12 +4002,12 @@ ice_dpll_deinit_fwnode_pins(struct ice_pf *pf, struct ice_dpll_pin *pins,
destroy_workqueue(pf->dplls.wq);
}
-static int ice_dpll_deinit_txclk_pins(struct ice_pf *pf)
+static int ice_dpll_deinit_txclk_pins(struct ice_pf *pf, bool flush)
{
struct ice_dpll_pin *synce_pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
struct ice_dpll *dt = &pf->dplls.txc;
- ice_dpll_stop_fwnode_pin_activity(synce_pin, true);
+ ice_dpll_stop_fwnode_pin_activity(synce_pin, flush);
ice_dpll_unregister_pins(dt->dpll, pf->dplls.txclks,
&ice_dpll_txclk_ops,
ARRAY_SIZE(pf->dplls.txclks));
@@ -3806,6 +4022,25 @@ static int ice_dpll_deinit_txclk_pins(struct ice_pf *pf)
return 0;
}
+/**
+ * ice_dpll_deinit_tspll_pins - deinitialize TSPLL fwnode pin
+ * @pf: board private structure
+ *
+ * Stop notifier activity and release the TIME_REF fwnode pin, unregistering
+ * it from the TSPLL DPLL if it was registered.
+ */
+static void ice_dpll_deinit_tspll_pins(struct ice_pf *pf)
+{
+ struct ice_dpll_pin *time_ref = &pf->dplls.tspll_in;
+ struct ice_dpll *tp = &pf->dplls.tspll;
+
+ ice_dpll_stop_fwnode_pin_activity(time_ref, true);
+ if (!IS_ERR_OR_NULL(time_ref->pin))
+ dpll_pin_unregister(tp->dpll, time_ref->pin,
+ &ice_dpll_tspll_in_ops, time_ref);
+ ice_dpll_release_fwnode_pin(time_ref);
+}
+
/**
* ice_dpll_deinit_pins - deinitialize direct pins
* @pf: board private structure
@@ -3826,7 +4061,10 @@ static void ice_dpll_deinit_pins(struct ice_pf *pf, bool cgu)
ice_dpll_deinit_rclk_pin(pf);
if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
- ice_dpll_deinit_txclk_pins(pf);
+ if (ice_pf_src_tmr_owned(pf) &&
+ test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
+ ice_dpll_deinit_tspll_pins(pf);
+ ice_dpll_deinit_txclk_pins(pf, true);
ice_dpll_deinit_fwnode_pins(pf, pf->dplls.inputs, 0);
}
if (cgu) {
@@ -4022,6 +4260,45 @@ static int ice_dpll_init_txclk_pins(struct ice_pf *pf, int start_idx)
return ret;
}
+/**
+ * ice_dpll_init_tspll_pins - init and register TSPLL TIME_REF fwnode pin
+ * @pf: board private structure
+ *
+ * Initialize the fwnode-based TIME_REF input pin for the TSPLL DPLL on E825.
+ * If the pin is not yet available in the dpll subsystem, registration will
+ * be deferred via the notifier path.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - initialization failure reason
+ */
+static int ice_dpll_init_tspll_pins(struct ice_pf *pf)
+{
+ struct ice_dpll_pin *time_ref = &pf->dplls.tspll_in;
+ struct ice_dpll *tp = &pf->dplls.tspll;
+ int ret;
+
+ time_ref->pf = pf;
+ ret = ice_dpll_init_fwnode_pin(time_ref, ice_dpll_fwnode_time_ref);
+ if (ret)
+ return ret;
+
+ if (IS_ERR_OR_NULL(time_ref->pin)) {
+ dev_dbg(ice_pf_to_dev(pf),
+ "TSPLL TIME_REF pin not registered yet\n");
+ return 0;
+ }
+
+ ret = dpll_pin_register(tp->dpll, time_ref->pin,
+ &ice_dpll_tspll_in_ops, time_ref);
+ if (ret) {
+ ice_dpll_stop_fwnode_pin_activity(time_ref, false);
+ ice_dpll_release_fwnode_pin(time_ref);
+ }
+
+ return ret;
+}
+
/**
* ice_dpll_init_pins_e825 - init pins and register pins with a dplls
* @pf: board private structure
@@ -4049,8 +4326,24 @@ static int ice_dpll_init_pins_e825(struct ice_pf *pf)
goto unregister_pins;
ret = ice_dpll_init_txclk_pins(pf, 0);
- if (ret)
+ if (ret) {
ice_dpll_deinit_rclk_pin(pf);
+ goto unregister_pins;
+ }
+
+ if (ice_pf_src_tmr_owned(pf) &&
+ test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) {
+ ret = ice_dpll_init_tspll_pins(pf);
+ if (ret) {
+ /* Avoid flushing pf->dplls.wq here: notifier work
+ * items block on pf->dplls.dpll_init which is
+ * completed by the unregister_pins path below.
+ * destroy_workqueue() there drains queued items.
+ */
+ ice_dpll_deinit_txclk_pins(pf, false);
+ ice_dpll_deinit_rclk_pin(pf);
+ }
+ }
unregister_pins:
if (ret) {
@@ -4237,6 +4530,7 @@ static void ice_dpll_deinit_worker(struct ice_pf *pf)
kthread_cancel_delayed_work_sync(&d->work);
kthread_destroy_worker(d->kworker);
+ d->kworker = NULL;
}
/**
@@ -4673,6 +4967,7 @@ static void ice_dpll_deinit_info(struct ice_pf *pf)
static int ice_dpll_init_info_e825c(struct ice_pf *pf)
{
struct ice_dplls *d = &pf->dplls;
+ struct ice_dpll *tp = &d->tspll;
struct ice_dpll *dt = &d->txc;
int ret = 0;
int i;
@@ -4681,7 +4976,11 @@ static int ice_dpll_init_info_e825c(struct ice_pf *pf)
d->num_inputs = ICE_SYNCE_CLK_NUM;
dt->dpll_state = ice_txclk_lock_status(pf->ptp.port.tx_clk);
dt->mode = DPLL_MODE_MANUAL;
- dt->dpll_idx = pf->ptp.port.port_num;
+ dt->dpll_idx = E825_DPLL_TXCLK_BASE_IDX + pf->ptp.port.port_num;
+ tp->dpll_state = ice_dpll_tspll_lock_status_get(pf, false);
+ tp->prev_dpll_state = tp->dpll_state;
+ tp->mode = DPLL_MODE_MANUAL;
+ tp->dpll_idx = E825_DPLL_TSPLL_BASE_IDX;
d->inputs = kzalloc_objs(*d->inputs, d->num_inputs);
if (!d->inputs)
@@ -4824,7 +5123,8 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
* resources and unregistering the dpll, pin and all resources used for
* handling them.
*
- * Context: Destroys pf->dplls.lock mutex. Call only if ICE_FLAG_DPLL was set.
+ * Context: Call only if ICE_FLAG_DPLL was set. pf->dplls.lock lifetime is
+ * managed by ice_init_features()/ice_deinit_features().
*/
void ice_dpll_deinit(struct ice_pf *pf)
{
@@ -4848,7 +5148,7 @@ void ice_dpll_deinit(struct ice_pf *pf)
up_write(&pf->dplls.txclk_notify_rwsem);
}
- if (cgu)
+ if (pf->dplls.kworker)
ice_dpll_deinit_worker(pf);
if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825)
@@ -4861,9 +5161,10 @@ void ice_dpll_deinit(struct ice_pf *pf)
ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
if (!IS_ERR_OR_NULL(pf->dplls.txc.dpll))
ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false);
+ if (!IS_ERR_OR_NULL(pf->dplls.tspll.dpll))
+ ice_dpll_deinit_dpll(pf, &pf->dplls.tspll, false);
ice_dpll_deinit_info(pf);
- mutex_destroy(&pf->dplls.lock);
}
/**
@@ -4873,8 +5174,6 @@ void ice_dpll_deinit(struct ice_pf *pf)
* Set up the device dplls, register them and pins connected within Linux dpll
* subsystem. Allow userspace to obtain state of DPLL and handling of DPLL
* configuration requests.
- *
- * Context: Initializes pf->dplls.lock mutex.
*/
static void ice_dpll_init_e825(struct ice_pf *pf)
{
@@ -4892,7 +5191,6 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
pf->ptp.port.port_num = pf->hw.lane_num;
}
- mutex_init(&d->lock);
/* Initialize the txclk worker and its notification rwsem before any
* code path can fail: ice_dpll_deinit() runs unconditionally on
* failure and calls cancel_work_sync() / down_write() on these.
@@ -4904,9 +5202,30 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
err = ice_dpll_init_info_e825c(pf);
if (err)
goto err_exit;
+ if (ice_pf_src_tmr_owned(pf) &&
+ test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) {
+ err = ice_dpll_init_dpll(pf, &pf->dplls.tspll, false,
+ DPLL_TYPE_GENERIC);
+ if (err)
+ goto deinit_info;
+ /* Start the periodic worker before registering fwnode pins. The
+ * fwnode notifier path (ice_dpll_pin_notify_work()) blocks on
+ * pf->dplls.dpll_init, which is only signalled on the success
+ * return below. Starting the worker before any fwnode notifier
+ * is registered means a subsequent init failure cannot leave
+ * notifier work items queued on pf->dplls.wq, so the deinit
+ * flush_workqueue() paths in ice_dpll_deinit_pins() cannot
+ * deadlock. The E825 periodic worker only touches
+ * pf->dplls.tspll (already registered above) and pf->ptp state,
+ * so it is safe to run before pins.
+ */
+ err = ice_dpll_init_worker(pf);
+ if (err)
+ goto deinit_tspll;
+ }
err = ice_dpll_init_dpll(pf, &pf->dplls.txc, false, DPLL_TYPE_GENERIC);
if (err)
- goto deinit_info;
+ goto deinit_worker;
err = ice_dpll_init_pins_e825(pf);
if (err)
goto deinit_txclk;
@@ -4917,10 +5236,15 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
deinit_txclk:
ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false);
+deinit_worker:
+ if (pf->dplls.kworker)
+ ice_dpll_deinit_worker(pf);
+deinit_tspll:
+ if (!IS_ERR_OR_NULL(pf->dplls.tspll.dpll))
+ ice_dpll_deinit_dpll(pf, &pf->dplls.tspll, false);
deinit_info:
ice_dpll_deinit_info(pf);
err_exit:
- mutex_destroy(&d->lock);
dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
}
@@ -4931,16 +5255,12 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
* Set up the device dplls, register them and pins connected within Linux dpll
* subsystem. Allow userspace to obtain state of DPLL and handling of DPLL
* configuration requests.
- *
- * Context: Initializes pf->dplls.lock mutex.
*/
static void ice_dpll_init_e810(struct ice_pf *pf)
{
bool cgu = ice_is_feature_supported(pf, ICE_F_CGU);
- struct ice_dplls *d = &pf->dplls;
int err = 0;
- mutex_init(&d->lock);
err = ice_dpll_init_info(pf, cgu);
if (err)
goto err_exit;
@@ -4971,7 +5291,6 @@ static void ice_dpll_init_e810(struct ice_pf *pf)
deinit_info:
ice_dpll_deinit_info(pf);
err_exit:
- mutex_destroy(&d->lock);
dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
}
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index 103ba3e49068..bce0bea9a664 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -133,10 +133,12 @@ struct ice_dpll {
* @eec: pointer to EEC dpll dev
* @pps: pointer to PPS dpll dev
* @txc: pointer to TXC dpll dev
+ * @tspll: pointer to TSPLL dpll dev (E825 only)
* @inputs: input pins pointer
* @outputs: output pins pointer
* @rclk: recovered pins pointer
* @txclks: TX clock reference pins pointer
+ * @tspll_in: TSPLL TIME_REF fwnode input pin (E825 only)
* @num_inputs: number of input pins available on dpll
* @num_outputs: number of output pins available on dpll
* @cgu_state_acq_err_num: number of errors returned during periodic work
@@ -177,12 +179,14 @@ struct ice_dplls {
struct ice_dpll eec;
struct ice_dpll pps;
struct ice_dpll txc;
+ struct ice_dpll tspll;
struct ice_dpll_pin *inputs;
struct ice_dpll_pin *outputs;
struct ice_dpll_pin sma[ICE_DPLL_PIN_SW_NUM];
struct ice_dpll_pin ufl[ICE_DPLL_PIN_SW_NUM];
struct ice_dpll_pin rclk;
struct ice_dpll_pin txclks[ICE_DPLL_TXCLK_NUM_MAX];
+ struct ice_dpll_pin tspll_in;
u8 num_inputs;
u8 num_outputs;
u8 sma_data;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d88835482d3a..2f372dd37b53 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4725,6 +4725,14 @@ static void ice_init_features(struct ice_pf *pf)
if (ice_is_safe_mode(pf))
return;
+ /* pf->dplls.lock guards TSPLL/CGU access shared between the DPLL
+ * subsystem callbacks and the PTP periodic worker's TSPLL monitor.
+ * Initialize it before ice_ptp_init() so the PTP kworker never sees
+ * an uninitialized mutex, and destroy it in ice_deinit_features()
+ * only after ice_ptp_release() has drained the kworker.
+ */
+ mutex_init(&pf->dplls.lock);
+
/* initialize DDP driven features */
if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
ice_ptp_init(pf);
@@ -4769,6 +4777,7 @@ static void ice_deinit_features(struct ice_pf *pf)
ice_ptp_release(pf);
if (test_bit(ICE_FLAG_DPLL, pf->flags))
ice_dpll_deinit(pf);
+ mutex_destroy(&pf->dplls.lock);
if (pf->eswitch_mode == DEVLINK_ESWITCH_MODE_SWITCHDEV)
xa_destroy(&pf->eswitch.reprs);
ice_hwmon_exit(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 4b1040d09f70..e54a98a0bdd0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2878,8 +2878,19 @@ static void ice_ptp_tspll_monitor(struct ice_pf *pf)
!ice_pf_src_tmr_owned(pf))
return;
+ /* Serialize the entire monitor tick against TSPLL userspace reconfig
+ * (ice_dpll_tspll_state_on_dpll_set()). Both paths read HW state and
+ * write pf->ptp.tspll_locked; without holding pf->dplls.lock across
+ * the HW read here, a preempted monitor could observe stale HW state
+ * and then overwrite an accurate cache update from the DPLL callback.
+ * pf->dplls.lock is initialized in ice_init_features() before the PTP
+ * kworker starts and destroyed in ice_deinit_features() only after
+ * ice_ptp_release() has drained the kworker, so it is always valid.
+ */
+ mutex_lock(&pf->dplls.lock);
err = ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost);
if (err) {
+ mutex_unlock(&pf->dplls.lock);
dev_err_ratelimited(ice_pf_to_dev(pf),
"Failed reading TimeSync PLL lock status (err: %d). Retrying.\n",
err);
@@ -2911,6 +2922,7 @@ static void ice_ptp_tspll_monitor(struct ice_pf *pf)
WRITE_ONCE(pf->ptp.tspll_locked, true);
pf->ptp.tspll_lock_retries = 0;
}
+ mutex_unlock(&pf->dplls.lock);
}
static void ice_ptp_periodic_work(struct kthread_work *work)
diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
index 78752dc1e762..85bacb7c3a79 100644
--- a/drivers/net/ethernet/intel/ice/ice_tspll.c
+++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
@@ -334,8 +334,9 @@ static int ice_tspll_dis_sticky_bits_e82x(struct ice_hw *hw)
* Return:
* * %0 - success
* * %-EINVAL - input parameters are incorrect
- * * %-EBUSY - failed to lock TSPLL
- * * %other - CGU read/write failure
+ * * %-EAGAIN - TSPLL configuration succeeded but lock is not acquired yet
+ * * %-EBUSY - CGU access is busy (for example during reset)
+ * * %other - other CGU read/write failures
*/
static int ice_tspll_cfg_e825c(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
enum ice_clk_src clk_src)
@@ -467,7 +468,7 @@ static int ice_tspll_cfg_e825c(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
if (!(val & ICE_CGU_RO_LOCK_TRUE_LOCK)) {
dev_warn(ice_hw_to_dev(hw), "CGU PLL failed to lock\n");
- return -EBUSY;
+ return -EAGAIN;
}
err = ice_read_cgu_reg(hw, ICE_CGU_R9, &r9);
@@ -661,6 +662,33 @@ int ice_tspll_get_clk_src(struct ice_hw *hw, enum ice_clk_src *clk_src)
return 0;
}
+/**
+ * ice_tspll_set_cfg - configure TS PLL with new settings
+ * @hw: board private hw structure
+ * @clk_freq: clock frequency to program
+ * @clk_src: clock source to select (TIME_REF, or TCXO)
+ *
+ * Configure CGU with new clock source and clock frequency settings.
+ *
+ * Return:
+ * * 0 - OK
+ * * negative - error
+ */
+int ice_tspll_set_cfg(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
+ enum ice_clk_src clk_src)
+{
+ int ret;
+
+ if (!ice_tspll_check_params(hw, clk_freq, clk_src))
+ return -EINVAL;
+
+ ret = ice_tspll_dis_sticky_bits(hw);
+ if (ret)
+ return ret;
+
+ return ice_tspll_cfg(hw, clk_freq, clk_src);
+}
+
/**
* ice_tspll_init - Initialize TSPLL with settings from firmware
* @hw: Pointer to the HW structure
diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.h b/drivers/net/ethernet/intel/ice/ice_tspll.h
index 05917ae51ded..832c049e686c 100644
--- a/drivers/net/ethernet/intel/ice/ice_tspll.h
+++ b/drivers/net/ethernet/intel/ice/ice_tspll.h
@@ -35,6 +35,8 @@ int ice_tspll_cfg_pps_out_e825c(struct ice_hw *hw, bool enable);
int ice_tspll_lost_lock_e825c(struct ice_hw *hw, bool *lost_lock);
int ice_tspll_restart_e825c(struct ice_hw *hw);
int ice_tspll_get_clk_src(struct ice_hw *hw, enum ice_clk_src *clk_src);
+int ice_tspll_set_cfg(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
+ enum ice_clk_src clk_src);
int ice_tspll_init(struct ice_hw *hw);
int ice_tspll_bypass_mux_active_e825c(struct ice_hw *hw, u8 port, bool *active,
enum ice_synce_clk output);
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 3/3] ice: use per-interface clock_id for E825 generic DPLLs
2026-09-08 22:24 [PATCH net-next 0/3][pull request] ice: expose TSPLL state on E825 through dpll subsystem Tony Nguyen
2026-09-08 22:24 ` [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker Tony Nguyen
2026-09-08 22:24 ` [PATCH net-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825 Tony Nguyen
@ 2026-09-08 22:24 ` Tony Nguyen
2026-09-11 10:27 ` netdev-bot+sashiko
2 siblings, 1 reply; 7+ messages in thread
From: Tony Nguyen @ 2026-09-08 22:24 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Grzegorz Nitka, anthony.l.nguyen, arkadiusz.kubalewski,
przemyslaw.korba, richardcochran, Alexander Nowlin
From: Grzegorz Nitka <grzegorz.nitka@intel.com>
On E825, the TX-CLK and TSPLL DPLL devices are registered as
DPLL_TYPE_GENERIC. Their clock_id was derived from the board-level
PCIe DSN, which is identical for all interfaces sharing the same
NAC/quad. As a result, userspace (e.g. 'dpll device show') reports
several DPLL devices with the same clock_id and no board or signal
label, making it impossible to unambiguously map a DPLL device to
the interface it belongs to.
Since these DPLLs are per-interface, use the permanent port MAC as
the clock_id basis for E825 generic DPLLs:
* TX-CLK uses the plain MAC-derived value.
* TSPLL on the source-timer owner PF uses the same MAC-derived
value with a dedicated tag bit, so it stays distinct from
TX-CLK while remaining stable per interface.
Other DPLL objects (EEC/PPS and non-E825 paths) keep the board
DSN-derived clock_id. When the permanent MAC is not yet valid, fall
back to the existing board-level clock_id to preserve init behavior.
Reviewed-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_dpll.c | 80 +++++++++++++++++++++--
1 file changed, 75 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 00e3f7de7810..0109fc34307c 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -3059,6 +3059,68 @@ static u64 ice_generate_clock_id(struct ice_pf *pf)
return pci_get_dsn(pf->pdev);
}
+/**
+ * ice_generate_dpll_clock_id - generate clock_id for a specific dpll device
+ * @pf: board private structure
+ * @d: dpll device context
+ * @type: dpll type being registered
+ *
+ * For E825 generic DPLLs, use per-interface permanent MAC as the clock_id
+ * basis so userspace can unambiguously map DPLL devices to interfaces.
+ * TX-CLK keeps plain MAC-derived ID, while TSPLL uses the same basis with
+ * a dedicated tag bit to remain distinct on source-timer owner PFs.
+ * Other DPLL objects keep board-level DSN-derived clock_id.
+ *
+ * Return: generated clock id for a dpll device
+ */
+static u64 ice_generate_dpll_clock_id(struct ice_pf *pf, struct ice_dpll *d,
+ enum dpll_type type)
+{
+ struct ice_hw *hw = &pf->hw;
+ u64 mac_clock_id;
+
+ if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
+ type == DPLL_TYPE_GENERIC &&
+ hw->port_info &&
+ is_valid_ether_addr(hw->port_info->mac.perm_addr)) {
+ mac_clock_id = ether_addr_to_u64(hw->port_info->mac.perm_addr);
+
+ if (d->dpll_idx >= E825_DPLL_TXCLK_BASE_IDX)
+ return mac_clock_id;
+
+ if (d->dpll_idx == E825_DPLL_TSPLL_BASE_IDX)
+ return mac_clock_id | BIT_ULL(63);
+ }
+
+ return pf->dplls.clock_id;
+}
+
+/**
+ * ice_dpll_is_own_dpll_clock_id - check if clock_id belongs to this pf's DPLLs
+ * @pf: board private structure
+ * @clock_id: clock_id from a DPLL notification
+ *
+ * Match info->src_clock_id from a DPLL pin notification against any DPLL
+ * device this PF has registered. Used to suppress self-notifications
+ * generated as a side effect of our own dpll_pin_register() and
+ * dpll_pin_unregister() calls on the fwnode-backed SYNCE and TIME_REF pins,
+ * whose DPLLs (TXC and TSPLL) use MAC-derived clock_ids on E825.
+ *
+ * Return: true if clock_id matches one of this PF's registered DPLL devices.
+ */
+static bool ice_dpll_is_own_dpll_clock_id(struct ice_pf *pf, u64 clock_id)
+{
+ if (clock_id == pf->dplls.clock_id)
+ return true;
+ if (pf->hw.mac_type != ICE_MAC_GENERIC_3K_E825)
+ return false;
+ if (clock_id == ice_generate_dpll_clock_id(pf, &pf->dplls.txc,
+ DPLL_TYPE_GENERIC))
+ return true;
+ return clock_id == ice_generate_dpll_clock_id(pf, &pf->dplls.tspll,
+ DPLL_TYPE_GENERIC);
+}
+
/**
* ice_dpll_pin_ntf - notify pin change including any SW pin wrappers
* @dplls: pointer to dplls struct
@@ -3858,10 +3920,12 @@ static int ice_dpll_pin_notify(struct notifier_block *nb, unsigned long action,
if (pin->fwnode != info->fwnode)
return NOTIFY_DONE; /* Not this pin */
- /* Ignore notification which are the outcome of internal pin
- * registration/unregistration calls - synce pin case.
+ /* Ignore notifications that are a side effect of internal pin
+ * registration/unregistration calls. E825 uses per-device
+ * MAC-derived clock_ids for the TXC and TSPLL generic DPLLs, so
+ * info->src_clock_id may not equal pf->dplls.clock_id.
*/
- if (info->src_clock_id == pin->pf->dplls.clock_id)
+ if (ice_dpll_is_own_dpll_clock_id(pin->pf, info->src_clock_id))
return NOTIFY_DONE;
work = kzalloc_obj(*work);
@@ -4206,10 +4270,16 @@ static int ice_dpll_init_txclk_pins(struct ice_pf *pf, int start_idx)
{
struct ice_dpll_pin *ref_pin = pf->dplls.txclks;
struct ice_dpll *txc = &pf->dplls.txc;
+ u64 clock_id;
int ret;
+ /* EXT_EREF0 is a non-fwnode pin; its clock_id must match the TX-CLK
+ * DPLL device clock_id (see dpll_pin_register()).
+ */
+ clock_id = ice_generate_dpll_clock_id(pf, txc, DPLL_TYPE_GENERIC);
+
/* Configure EXT_EREF0 pin */
- ret = ice_dpll_get_pins(pf, ref_pin, start_idx, 1, pf->dplls.clock_id);
+ ret = ice_dpll_get_pins(pf, ref_pin, start_idx, 1, clock_id);
if (ret)
return ret;
ret = dpll_pin_register(txc->dpll, ref_pin->pin, &ice_dpll_txclk_ops,
@@ -4487,7 +4557,7 @@ static int
ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
enum dpll_type type)
{
- u64 clock_id = pf->dplls.clock_id;
+ u64 clock_id = ice_generate_dpll_clock_id(pf, d, type);
int ret;
d->dpll = dpll_device_get(clock_id, d->dpll_idx, THIS_MODULE,
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker
2026-09-08 22:24 ` [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker Tony Nguyen
@ 2026-09-11 10:27 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 10:27 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev,
grzegorz.nitka, arkadiusz.kubalewski, przemyslaw.korba,
richardcochran, alexander.nowlin
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 3
- [Medium] ice_ptp_tspll_monitor()'s recovery cadence and its log rate-
limit (ICE_TSPLL_LOG_INTERVAL = 120, documented as "~60 s at normal…
- [Medium] The lock-lost dev_warn() and the lock-acquired dev_info() in
ice_ptp_tspll_monitor() are effectively unrate-limited when the TSPLL…
- [Low] Commit message misdescribes the tree state: it says the cached
lock state is added "so a follow-up change can consume it from the…
- [Low] ice_ptp_tspll_monitor() is invoked at the top of
ice_ptp_periodic_work(), ahead of ice_ptp_update_cached_phctime().
- [Low] ice_ptp_init() initializes only ptp->tspll_locked = true and
never resets the newly added ptp->tspll_lock_retries, whereas the…
--
ice: monitor TSPLL lock from PTP periodic worker
Adds ice_ptp_tspll_monitor() to the PTP periodic worker so E825 owner PFs
poll the TSPLL lock status over CGU registers and restart the PLL when the
lock is lost. The observed state is cached in pf->ptp.tspll_locked, and
three TSPLL helpers are added/exported for the read, the restart and the
clock source string.
A few questions about the new monitor and about the changelog below.
> Cache the observed lock state in pf->ptp.tspll_locked using
> WRITE_ONCE()/READ_ONCE() so a follow-up change can consume it from the
> DPLL periodic worker (for user-space notification via
> dpll_device_change_ntf()) and drop the redundant poll+recovery from
> that path.
[Severity: Low]
Is there really a redundant poll+recovery in that path today?
At the baseline, ice_dpll.c has no reference to ICE_CGU_RO_LOCK,
ICE_CGU_R23_R24_TSPLL_ENABLE or any lost-lock/restart helper; grepping the
ice/ directory matches only ice_tspll.c and ice_common.h.
ice_dpll_periodic_work() only calls ice_dpll_update_state(), which polls
EEC/PPS state over AQ.
The follow-up patch in this series adds a consumer of the new cache
(ice_dpll_periodic_work_e825() -> ice_dpll_tspll_lock_status_get() ->
READ_ONCE(pf->ptp.tspll_locked)) but does not remove any TSPLL poll or
recovery.
Could the wording be adjusted to say this adds new TSPLL monitoring and
recovery rather than relocating existing recovery out of the DPLL worker?
The same claim is repeated in the new kernel-doc for @tspll_locked in
ice_ptp.h.
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index eaec36ab6ae3..4b1040d09f70 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -4,8 +4,11 @@
> #include "ice.h"
> #include "ice_lib.h"
> #include "ice_trace.h"
> +#include "ice_tspll.h"
> #include "ice_txclk.h"
>
> +#define ICE_TSPLL_LOG_INTERVAL 120
> +
> static const char ice_pin_names[][64] = {
> "SDP0",
> "SDP1",
[ ... ]
> @@ -2849,6 +2852,67 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
> }
> }
>
> +/**
> + * ice_ptp_tspll_monitor - poll and recover TSPLL lock on E825 owner PFs
> + * @pf: Board private structure
> + *
> + * Called from the PTP periodic worker. On E825 devices that own the source
> + * timer, poll the TSPLL lock status via CGU registers and trigger a restart
> + * if the lock has been lost. The result is cached in @pf->ptp.tspll_locked
> + * so it can be consumed by the DPLL periodic worker via READ_ONCE().
> + *
> + * TSPLL lock is critical for PHC operation and must be monitored regardless
> + * of whether DPLL init succeeded or CONFIG_DPLL is enabled. Placing the
> + * monitor here makes recovery independent of the dpll subsystem.
> + *
> + * AQ read errors are rate-limited and do not stop monitoring. Lock-lost
> + * events are logged every 120 retries (~60 s at normal poll rate) to
> + * surface persistent failures without flooding the log.
> + */
> +static void ice_ptp_tspll_monitor(struct ice_pf *pf)
> +{
> + bool lock_lost;
> + int err;
> +
> + if (pf->hw.mac_type != ICE_MAC_GENERIC_3K_E825 ||
> + !ice_pf_src_tmr_owned(pf))
> + return;
> +
> + err = ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost);
> + if (err) {
> + dev_err_ratelimited(ice_pf_to_dev(pf),
> + "Failed reading TimeSync PLL lock status (err: %d). Retrying.\n",
> + err);
> + return;
> + }
> +
> + if (lock_lost) {
> + WRITE_ONCE(pf->ptp.tspll_locked, false);
> + if (!(pf->ptp.tspll_lock_retries % ICE_TSPLL_LOG_INTERVAL))
> + dev_warn(ice_pf_to_dev(pf),
> + "TimeSync PLL lock lost. Retrying to acquire lock.\n");
> + err = ice_tspll_restart_e825c(&pf->hw);
> + if (err)
> + dev_err_ratelimited(ice_pf_to_dev(pf),
> + "Failed to restart TimeSync PLL (err: %d).\n",
> + err);
> + pf->ptp.tspll_lock_retries++;
[Severity: Medium]
The kernel-doc above says lock-lost events are logged "every 120 retries
(~60 s at normal poll rate)", and the new @tspll_lock_retries documentation
in ice_ptp.h repeats "~every 120 retries / ~60 s". Does that hold when the
worker requeues itself early?
ice_ptp_periodic_work() ends with:
/* Run twice a second or reschedule if phc update failed */
kthread_queue_delayed_work(ptp->kworker, &ptp->work,
msecs_to_jiffies(err ? 10 : 500));
and ice_ptp_update_cached_phctime() returns an error whenever ICE_CFG_BUSY is
already set:
if (test_and_set_bit(ICE_CFG_BUSY, pf->state))
return -EAGAIN;
ICE_CFG_BUSY is held for the whole duration of ice_set_ringparam(),
ice_set_link_ksettings() and queue count reconfiguration. If one of those
overlaps a lost TSPLL lock, the monitor runs every ~10 ms rather than every
500 ms.
Each of those ticks calls ice_tspll_restart_e825c(), which clears
ICE_CGU_R23_R24_TSPLL_ENABLE, sleeps 1-2 ms with the source PLL disabled and
re-enables it.
Is it intended that the source PLL is held disabled for 10-20% of that
window, that 3-4 sideband transactions are issued every 10 ms, and that the
documented ~60 s warning interval collapses to ~1.2 s while "after %u
retries" no longer maps to a fixed time base? The later commit in this
series that wraps the monitor body in pf->dplls.lock leaves this cadence
unchanged.
> + } else {
> + if (pf->ptp.tspll_lock_retries) {
> + const char *src_str = "unknown";
> + enum ice_clk_src clk_src;
> +
> + if (!ice_tspll_get_clk_src(&pf->hw, &clk_src))
> + src_str = ice_tspll_clk_src_str(clk_src);
> + dev_info(ice_pf_to_dev(pf),
> + "TimeSync PLL lock acquired with %s clock source after %u retries.\n",
> + src_str, pf->ptp.tspll_lock_retries);
> + }
> + WRITE_ONCE(pf->ptp.tspll_locked, true);
> + pf->ptp.tspll_lock_retries = 0;
> + }
> +}
[Severity: Medium]
Because this branch resets pf->ptp.tspll_lock_retries to zero, the
'% ICE_TSPLL_LOG_INTERVAL' gate on the dev_warn() is satisfied again on the
very next lock-lost tick.
For a reference clock that alternates between locked and unlocked, does this
emit one dev_warn("TimeSync PLL lock lost...") plus one unrate-limited
dev_info("TimeSync PLL lock acquired ... after %u retries.") per poll pair,
i.e. roughly two messages per second for as long as the input flaps (and up
to ~100 per second if the worker is also requeueing at 10 ms)?
Would dev_warn_ratelimited()/dev_info_ratelimited(), or a counter that a
transient re-lock does not clear, match the stated intent of surfacing
persistent failures "without flooding the log"?
> +
> static void ice_ptp_periodic_work(struct kthread_work *work)
> {
> struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
> @@ -2858,6 +2922,8 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
> if (pf->ptp.state != ICE_PTP_READY)
> return;
>
> + ice_ptp_tspll_monitor(pf);
> +
> err = ice_ptp_update_cached_phctime(pf);
[Severity: Low]
The monitor runs ahead of the cached PHC refresh in the same serialized
kthread work item. On a lock-lost tick it issues up to four sideband CGU
transactions plus the unconditional usleep_range(USEC_PER_MSEC,
2 * USEC_PER_MSEC) in ice_tspll_restart_e825c(), and each control queue
command is bounded only by ICE_CTL_Q_SQ_CMD_TIMEOUT (USEC_PER_SEC, "max
delay 1s" in ice_controlq.h).
ice_ptp_update_cached_phctime() documents that it "must be called
periodically to ensure that the cached value is never more than 2 seconds
old", and it warns and bumps late_cached_phc_updates when that is exceeded.
Would calling ice_ptp_tspll_monitor() after ice_ptp_update_cached_phctime(),
or from a separate work item, keep the sleeping hardware recovery sequence
off the time-critical path?
>
> ice_ptp_maybe_trigger_tx_interrupt(pf);
> @@ -2975,6 +3041,9 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
> err = ice_tspll_init(hw);
> if (err)
> return err;
> + /* Rebuild reinitialized TSPLL, so reset monitor retry state. */
> + WRITE_ONCE(ptp->tspll_locked, true);
> + ptp->tspll_lock_retries = 0;
>
> /* Acquire the global hardware lock */
> if (!ice_ptp_lock(hw)) {
> @@ -3319,6 +3388,7 @@ void ice_ptp_init(struct ice_pf *pf)
> }
> ptp->port.port_num = hw->lane_num;
>
> + ptp->tspll_locked = true;
> ice_ptp_init_hw(hw);
[Severity: Low]
Should ptp->tspll_lock_retries be cleared here as well? The rebuild path
above resets both fields, but ice_ptp_init() only sets tspll_locked.
struct ice_ptp is embedded in the probe-lifetime struct ice_pf,
ice_ptp_release() does not clear either field, and
ice_init_features()->ice_ptp_init() /
ice_deinit_features()->ice_ptp_release() run again on the same pf across a
devlink reload.
If the TSPLL happened to be unlocked at unload time, does the leftover
non-zero counter make the first successful poll after reload print
"TimeSync PLL lock acquired with %s clock source after %u retries." for a
lock that was never lost in this instance, and start the
'% ICE_TSPLL_LOG_INTERVAL' gate at a stale phase so a genuine first
lock-lost event stays unlogged for up to ~119 poll cycles?
>
> ice_ptp_init_tx_interrupt_mode(pf);
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
> index c4b0da7ce20e..0e40fef3b4e8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
> @@ -249,6 +249,15 @@ struct ice_ptp_pin_desc {
> * @tx_hwtstamp_discarded: number of Tx skbs discarded due to cached PHC time
> * being too old to correctly extend timestamp
> * @late_cached_phc_updates: number of times cached PHC update is late
> + * @tspll_locked: last observed TSPLL lock state on E825 owner PFs.
> + * Written by the PTP periodic worker after polling the TSPLL and
> + * intended to be read (without pf->dplls.lock) by the DPLL periodic
> + * worker in a follow-up change. Access via READ_ONCE()/WRITE_ONCE();
> + * precise synchronization is not required because both workers
> + * converge on the same value within one poll period.
> + * @tspll_lock_retries: counts consecutive poll cycles in which the TSPLL
> + * was found unlocked. Reset to zero when lock is re-acquired. Used to
> + * rate-limit the lock-lost log message (~every 120 retries / ~60 s).
> */
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
> index fd4b58eb9bc0..78752dc1e762 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tspll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
[ ... ]
> @@ -531,6 +531,62 @@ int ice_tspll_cfg_pps_out_e825c(struct ice_hw *hw, bool enable)
> return ice_write_cgu_reg(hw, ICE_CGU_R9, val);
> }
>
> +int ice_tspll_restart_e825c(struct ice_hw *hw)
> +{
> + u32 val;
> + int err;
> +
> + /* Read the initial values of r23 and disable the PLL */
> + err = ice_read_cgu_reg(hw, ICE_CGU_R23, &val);
> + if (err)
> + return err;
> +
> + val &= ~ICE_CGU_R23_R24_TSPLL_ENABLE;
> + err = ice_write_cgu_reg(hw, ICE_CGU_R23, val);
> + if (err)
> + return err;
> +
> + /* Wait at least 1 ms before reenabling PLL */
> + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
> + val |= ICE_CGU_R23_R24_TSPLL_ENABLE;
> + return ice_write_cgu_reg(hw, ICE_CGU_R23, val);
> +}
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908222428.872254-1-anthony.l.nguyen%40intel.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825
2026-09-08 22:24 ` [PATCH net-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825 Tony Nguyen
@ 2026-09-11 10:27 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 10:27 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev,
grzegorz.nitka, arkadiusz.kubalewski, przemyslaw.korba,
richardcochran, alexander.nowlin
Thank you for your contribution! Sashiko AI review found 10 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 8 · Low: 2
- [Medium] E825 TSPLL DPLL device is registered and its periodic worker
started based only on `ice_pf_src_tmr_owned(pf) &&…
- [Medium] ice_dpll_deinit_tspll_pins() unconditionally calls
ice_dpll_stop_fwnode_pin_activity(&pf->dplls.tspll_in, true), which…
- [Medium] The patch introduces `!IS_ERR_OR_NULL(pf->dplls.tspll.dpll)`
as the existence predicate for the new TSPLL dpll device (in…
- [Medium] pf->dplls.lock is taken only by the new userspace TSPLL
reconfiguration path (ice_dpll_tspll_state_on_dpll_set()) and the PTP…
- [Medium] The new userspace-reachable path
ice_dpll_tspll_state_on_dpll_set() -> ice_tspll_set_cfg() ->…
- [Medium] ice_dpll_tspll_state_on_dpll_set() never compares the
requested pin state with the currently programmed clock source…
- [Medium] TSPLL source changes and E825 periodic-output configuration
perform unsynchronized read-modify-write operations on different…
- [Medium] A userspace-selected TSPLL clock source is not preserved
across an E825 reset: ice_dpll_tspll_state_on_dpll_set() writes the…
- [Low] ice_tspll_cfg_e825c() now returns -EAGAIN for 'configuration
applied but PLL not locked yet', but the new contract is not reflected…
- [Low] struct ice_dplls's kernel-doc still describes @lock as a purely
DPLL-internal mutex ('protects DPLL configuration', '@lock serializes…
--
ice: add TSPLL DPLL device and TIME_REF pin for E825
This exposes the E825 source timer PLL (TSPLL) as a DPLL_TYPE_GENERIC device
with a fwnode-backed "time_ref" input pin, so its lock status can be read and
its clock source switched between TIME_REF and TCXO from userspace. It also
moves the pf->dplls.lock lifetime out to ice_init_features() /
ice_deinit_features() and takes that mutex in the PTP TSPLL monitor.
A few questions below about the new paths.
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 85a74cd6ea1f..00e3f7de7810 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
[ ... ]
> @@ -2814,6 +2817,160 @@ static const struct dpll_pin_ops ice_dpll_txclk_ops = {
> .direction_get = ice_dpll_input_direction,
> };
>
> +/**
> + * ice_dpll_tspll_lock_status_get - derive TSPLL state for dpll subsystem
> + * @pf: board private structure
> + * @use_cached: if true, read lock state from pf->ptp.tspll_locked (maintained
> + * by the PTP periodic worker); if false, query hardware directly
> + *
> + * If TIME_REF is not selected, TSPLL is treated as unlocked from the dpll
> + * subsystem perspective, regardless of raw lock bit.
> + *
> + * Return: TSPLL lock status
> + */
> +static enum dpll_lock_status
> +ice_dpll_tspll_lock_status_get(struct ice_pf *pf, bool use_cached)
> +{
> + enum ice_clk_src clk_src;
> + bool locked;
> +
> + if (ice_tspll_get_clk_src(&pf->hw, &clk_src) ||
> + clk_src != ICE_CLK_SRC_TIME_REF)
> + return DPLL_LOCK_STATUS_UNLOCKED;
> +
> + if (use_cached) {
> + locked = READ_ONCE(pf->ptp.tspll_locked);
> + } else {
> + bool lock_lost;
> +
> + if (ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost))
> + return DPLL_LOCK_STATUS_UNLOCKED;
> + locked = !lock_lost;
> + }
> + return locked ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED;
> +}
[ ... ]
> +static int
> +ice_dpll_tspll_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
> + const struct dpll_device *dpll,
> + void *dpll_priv, enum dpll_pin_state state,
> + struct netlink_ext_ack *extack)
> +{
> + struct ice_dpll_pin *p = pin_priv;
> + struct ice_pf *pf = p->pf;
> + enum ice_clk_src clk_src;
> + bool changed = false;
> + struct ice_dpll *tp;
> + int ret;
> +
> + if (ice_dpll_is_reset(pf, extack))
> + return -EBUSY;
> +
> + if (state != DPLL_PIN_STATE_CONNECTED &&
> + state != DPLL_PIN_STATE_DISCONNECTED) {
> + NL_SET_ERR_MSG(extack,
> + "unsupported pin state for TSPLL reference clock");
> + return -EINVAL;
> + }
> +
> + clk_src = (state == DPLL_PIN_STATE_CONNECTED) ? ICE_CLK_SRC_TIME_REF :
> + ICE_CLK_SRC_TCXO;
[Severity: Medium]
Should this compare the requested state against the currently programmed
clock source before reprogramming?
The dpll core does not filter a set request that matches the current state -
dpll_pin_state_set() in drivers/dpll/dpll_netlink.c calls
ops->state_on_dpll_set() unconditionally:
ret = ops->state_on_dpll_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
dpll, dpll_priv(dpll), state, extack);
So a management daemon that periodically re-asserts DPLL_PIN_STATE_CONNECTED
runs the full ice_tspll_set_cfg() sequence each time, which clears
ICE_CGU_R23_R24_TSPLL_ENABLE and ICE_CGU_R9_TIME_SYNC_EN, reprograms the
dividers, sleeps and waits 1-2 ms for re-lock. Can that stop the PHC long
enough for ice_ptp_tspll_monitor() to see a lost lock and enter its restart
path, for a request that should be a no-op? The get callback already has
ice_tspll_get_clk_src() available for the comparison.
> + tp = &pf->dplls.tspll;
> + /* Serialize multi-register TSPLL reconfiguration with restart path. */
> + mutex_lock(&pf->dplls.lock);
> + ret = ice_tspll_set_cfg(&pf->hw, ICE_TSPLL_FREQ_156_250, clk_src);
[Severity: Medium]
Is pf->dplls.lock enough to cover all TSPLL writers? The commit message says:
Serialize both with pf->dplls.lock across their HW-read -> cache-write
sequence
but there is a third writer that takes no lock at all. ice_ptp_rebuild_owner()
reprograms the same CGU registers and writes the same cache:
err = ice_tspll_init(hw);
if (err)
return err;
/* Rebuild reinitialized TSPLL, so reset monitor retry state. */
WRITE_ONCE(ptp->tspll_locked, true);
Neither ice_prepare_for_reset() nor ice_ptp_prepare_for_reset() takes
pf->dplls.lock, and the ice_dpll_is_reset() check above happens before the
mutex is acquired, so a reset can start after the check passes.
Can two read-modify-write sequences on ICE_CGU_R9 / ICE_CGU_R23 then overlap
while ice_tspll_cfg_e825c() has the PLL disabled and is sleeping, and can the
unlocked WRITE_ONCE(ptp->tspll_locked, true) clobber the value published under
the mutex here?
[Severity: Medium]
Does the source selected here survive a reset? Nothing records the requested
clk_src, while reset recovery reprograms the source from the firmware
capability value in ice_tspll_init():
tspll_freq = (enum ice_tspll_freq)ts_info->time_ref;
clk_src = (enum ice_clk_src)ts_info->clk_src;
ice_ptp_rebuild_owner() calls ice_tspll_init() for the source timer owner on
every non-PFR reset, so after an admin switches TIME_REF -> TCXO through this
pin, any PF/CORE/GLOBAL reset would silently revert it while the same pin
stays registered and only the lock status is republished.
[ ... ]
> @@ -3134,12 +3291,44 @@ ice_dpll_update_state(struct ice_pf *pf, struct ice_dpll *d, bool init)
> return ret;
> }
>
> +/**
> + * ice_dpll_periodic_work_e825 - TSPLL DPLL periodic update for E825
> + * @pf: board private structure
> + *
> + * Publish TSPLL lock status to the dpll subsystem. The PTP periodic worker
> + * owns TSPLL lock polling and recovery; this function consumes the cached
> + * result for dpll notifications.
> + *
> + * Context: Must be called without pf->dplls.lock held.
> + */
> +static void ice_dpll_periodic_work_e825(struct ice_pf *pf)
> +{
> + struct ice_dpll *tp = &pf->dplls.tspll;
> + enum dpll_lock_status new_state;
> + bool changed = false;
> +
> + mutex_lock(&pf->dplls.lock);
> + new_state = ice_dpll_tspll_lock_status_get(pf, true);
[Severity: Medium]
What updates pf->ptp.tspll_locked when ice_ptp_init() failed?
The TSPLL device and this worker are started from ice_dpll_init_e825() based
on ice_pf_src_tmr_owned(pf) && test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags),
which does not imply PTP reached ICE_PTP_READY. ice_init_features() ignores
the outcome of ice_ptp_init(), and the error exits there (hw->lane_num < 0,
ice_ptp_init_owner() failure, ice_ptp_init_work() failure) set state to
ICE_PTP_UNINIT without clearing ICE_FLAG_PTP_SUPPORTED.
The only producer of the cache then never runs:
ice_ptp_periodic_work()
if (pf->ptp.state != ICE_PTP_READY)
return;
ice_ptp_tspll_monitor(pf);
So with use_cached = true the published status appears to freeze at whatever
ice_ptp_init() left in tspll_locked (it optimistically assigns
ptp->tspll_locked = true before the failing steps), for the lifetime of the
driver. Can the first tick here also overwrite the init-time direct hardware
read done in ice_dpll_init_info_e825c() and emit a change notification that
does not reflect hardware?
[ ... ]
> @@ -3806,6 +4022,25 @@ static int ice_dpll_deinit_txclk_pins(struct ice_pf *pf)
> return 0;
> }
>
> +/**
> + * ice_dpll_deinit_tspll_pins - deinitialize TSPLL fwnode pin
> + * @pf: board private structure
> + *
> + * Stop notifier activity and release the TIME_REF fwnode pin, unregistering
> + * it from the TSPLL DPLL if it was registered.
> + */
> +static void ice_dpll_deinit_tspll_pins(struct ice_pf *pf)
> +{
> + struct ice_dpll_pin *time_ref = &pf->dplls.tspll_in;
> + struct ice_dpll *tp = &pf->dplls.tspll;
> +
> + ice_dpll_stop_fwnode_pin_activity(time_ref, true);
> + if (!IS_ERR_OR_NULL(time_ref->pin))
> + dpll_pin_unregister(tp->dpll, time_ref->pin,
> + &ice_dpll_tspll_in_ops, time_ref);
> + ice_dpll_release_fwnode_pin(time_ref);
> +}
> +
> /**
> * ice_dpll_deinit_pins - deinitialize direct pins
> * @pf: board private structure
> @@ -3826,7 +4061,10 @@ static void ice_dpll_deinit_pins(struct ice_pf *pf, bool cgu)
>
> ice_dpll_deinit_rclk_pin(pf);
> if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
> - ice_dpll_deinit_txclk_pins(pf);
> + if (ice_pf_src_tmr_owned(pf) &&
> + test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
> + ice_dpll_deinit_tspll_pins(pf);
> + ice_dpll_deinit_txclk_pins(pf, true);
> ice_dpll_deinit_fwnode_pins(pf, pf->dplls.inputs, 0);
> }
[Severity: Medium]
Is it safe to decide this teardown from re-evaluated runtime state rather than
from the pin object itself?
ice_dpll_stop_fwnode_pin_activity() dereferences pin->pf with no NULL check:
unregister_dpll_notifier(&pin->nb);
if (flush)
flush_workqueue(pin->pf->dplls.wq);
and tspll_in.pf / tspll_in.nb are populated only inside
ice_dpll_init_tspll_pins() (unlike txclks and inputs, whose pf is assigned
unconditionally in ice_dpll_init_info_txclk_pins_e825c() and
ice_dpll_init_info_direct_pins()).
ice_pf_src_tmr_owned() reads pf->hw.func_caps.ts_func_info.src_tmr_owned,
which ice_rebuild() re-populates from firmware via ice_get_caps() while the
driver stays loaded. If the gate was false at init and true here, does this
oops on flush_workqueue(pin->pf->dplls.wq) and unregister a zeroed
notifier_block? In the inverse case, is the registered notifier and the
fwnode pin reference leaked past unload? Guarding on tspll_in.fwnode or
tspll_in.pf would make init and deinit symmetric.
[ ... ]
> @@ -4022,6 +4260,45 @@ static int ice_dpll_init_txclk_pins(struct ice_pf *pf, int start_idx)
> return ret;
> }
>
> +static int ice_dpll_init_tspll_pins(struct ice_pf *pf)
> +{
> + struct ice_dpll_pin *time_ref = &pf->dplls.tspll_in;
> + struct ice_dpll *tp = &pf->dplls.tspll;
> + int ret;
> +
> + time_ref->pf = pf;
> + ret = ice_dpll_init_fwnode_pin(time_ref, ice_dpll_fwnode_time_ref);
> + if (ret)
> + return ret;
[ ... ]
> @@ -4237,6 +4530,7 @@ static void ice_dpll_deinit_worker(struct ice_pf *pf)
>
> kthread_cancel_delayed_work_sync(&d->work);
> kthread_destroy_worker(d->kworker);
> + d->kworker = NULL;
> }
[ ... ]
> @@ -4861,9 +5161,10 @@ void ice_dpll_deinit(struct ice_pf *pf)
> ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
> if (!IS_ERR_OR_NULL(pf->dplls.txc.dpll))
> ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false);
> + if (!IS_ERR_OR_NULL(pf->dplls.tspll.dpll))
> + ice_dpll_deinit_dpll(pf, &pf->dplls.tspll, false);
>
> ice_dpll_deinit_info(pf);
> - mutex_destroy(&pf->dplls.lock);
> }
[Severity: Medium]
Can pf->dplls.tspll.dpll be stale here? ice_dpll_deinit_dpll() puts the
reference but never clears the pointer:
if (cgu || pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825)
dpll_device_unregister(d->dpll, d->ops, d);
dpll_device_put(d->dpll, &d->tracker);
pf (and pf->dplls) survives an unload/load cycle via devlink reload, and the
TSPLL device is created only when ice_pf_src_tmr_owned(pf) &&
ICE_FLAG_PTP_SUPPORTED holds. If that gate is false on a later cycle, does
this predicate still see the already-put pointer from the previous cycle and
call dpll_device_unregister()/dpll_device_put() on a device it no longer owns?
The same patch adds d->kworker = NULL in ice_dpll_deinit_worker() precisely so
the new if (pf->dplls.kworker) predicate is truthful - would a matching
d->dpll = NULL be appropriate?
[ ... ]
> @@ -4904,9 +5202,30 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
> err = ice_dpll_init_info_e825c(pf);
> if (err)
> goto err_exit;
> + if (ice_pf_src_tmr_owned(pf) &&
> + test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) {
> + err = ice_dpll_init_dpll(pf, &pf->dplls.tspll, false,
> + DPLL_TYPE_GENERIC);
> + if (err)
> + goto deinit_info;
[ ... ]
> + err = ice_dpll_init_worker(pf);
> + if (err)
> + goto deinit_tspll;
> + }
> err = ice_dpll_init_dpll(pf, &pf->dplls.txc, false, DPLL_TYPE_GENERIC);
> if (err)
> - goto deinit_info;
> + goto deinit_worker;
> err = ice_dpll_init_pins_e825(pf);
> if (err)
> goto deinit_txclk;
> @@ -4917,10 +5236,15 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
>
> deinit_txclk:
> ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false);
> +deinit_worker:
> + if (pf->dplls.kworker)
> + ice_dpll_deinit_worker(pf);
> +deinit_tspll:
> + if (!IS_ERR_OR_NULL(pf->dplls.tspll.dpll))
> + ice_dpll_deinit_dpll(pf, &pf->dplls.tspll, false);
> deinit_info:
> ice_dpll_deinit_info(pf);
> err_exit:
> - mutex_destroy(&d->lock);
> dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
> }
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3a..2f372dd37b53 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4725,6 +4725,14 @@ static void ice_init_features(struct ice_pf *pf)
> if (ice_is_safe_mode(pf))
> return;
>
> + /* pf->dplls.lock guards TSPLL/CGU access shared between the DPLL
> + * subsystem callbacks and the PTP periodic worker's TSPLL monitor.
> + * Initialize it before ice_ptp_init() so the PTP kworker never sees
> + * an uninitialized mutex, and destroy it in ice_deinit_features()
> + * only after ice_ptp_release() has drained the kworker.
> + */
> + mutex_init(&pf->dplls.lock);
> +
[Severity: Low]
Could the kernel-doc for the mutex be updated along with its new owner?
struct ice_dplls in ice_dpll.h still describes @lock as "protects DPLL
configuration (see Locking below)" and states that "@lock serializes all DPLL
state mutations on this PF", with a Locking section that lists only DPLL-side
acquirers. After this change the mutex is also taken from
ice_ptp_tspll_monitor() in ice_ptp.c, and its lifetime belongs to
ice_init_features()/ice_deinit_features() rather than
ice_dpll_init()/ice_dpll_deinit().
The @tspll_locked kernel-doc in ice_ptp.h also still says the field is
"intended to be read (without pf->dplls.lock) by the DPLL periodic worker in
a follow-up change", while this patch reads it with pf->dplls.lock held.
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 4b1040d09f70..e54a98a0bdd0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2878,8 +2878,19 @@ static void ice_ptp_tspll_monitor(struct ice_pf *pf)
> !ice_pf_src_tmr_owned(pf))
> return;
>
> + /* Serialize the entire monitor tick against TSPLL userspace reconfig
> + * (ice_dpll_tspll_state_on_dpll_set()). Both paths read HW state and
> + * write pf->ptp.tspll_locked; without holding pf->dplls.lock across
> + * the HW read here, a preempted monitor could observe stale HW state
> + * and then overwrite an accurate cache update from the DPLL callback.
> + * pf->dplls.lock is initialized in ice_init_features() before the PTP
> + * kworker starts and destroyed in ice_deinit_features() only after
> + * ice_ptp_release() has drained the kworker, so it is always valid.
> + */
> + mutex_lock(&pf->dplls.lock);
> err = ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost);
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
> index 78752dc1e762..85bacb7c3a79 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tspll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
> @@ -334,8 +334,9 @@ static int ice_tspll_dis_sticky_bits_e82x(struct ice_hw *hw)
> * Return:
> * * %0 - success
> * * %-EINVAL - input parameters are incorrect
> - * * %-EBUSY - failed to lock TSPLL
> - * * %other - CGU read/write failure
> + * * %-EAGAIN - TSPLL configuration succeeded but lock is not acquired yet
> + * * %-EBUSY - CGU access is busy (for example during reset)
> + * * %other - other CGU read/write failures
> */
> static int ice_tspll_cfg_e825c(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
> enum ice_clk_src clk_src)
> @@ -467,7 +468,7 @@ static int ice_tspll_cfg_e825c(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
>
> if (!(val & ICE_CGU_RO_LOCK_TRUE_LOCK)) {
> dev_warn(ice_hw_to_dev(hw), "CGU PLL failed to lock\n");
> - return -EBUSY;
> + return -EAGAIN;
> }
[Severity: Low]
Should ice_tspll_cfg_e82x() and the ice_tspll_cfg() wrapper doc follow this
new convention? ice_tspll_cfg_e82x() still returns -EBUSY for the identical
"configured but not locked" check on ICE_CGU_RO_BWM_LF_TRUE_LOCK, so the two
implementations behind one wrapper now report different errnos for the same
hardware condition, and the wrapper's kernel-doc still mentions only "other
negative error codes when failed to configure CGU".
The E825-only path means this cannot be hit through ice_tspll_set_cfg() today,
but a future caller reading the wrapper contract could get it wrong.
> +/**
> + * ice_tspll_set_cfg - configure TS PLL with new settings
> + * @hw: board private hw structure
> + * @clk_freq: clock frequency to program
> + * @clk_src: clock source to select (TIME_REF, or TCXO)
> + *
> + * Configure CGU with new clock source and clock frequency settings.
> + *
> + * Return:
> + * * 0 - OK
> + * * negative - error
> + */
Could this kernel-doc list -EAGAIN as well? Its only caller,
ice_dpll_tspll_state_on_dpll_set(), special-cases that value and maps it to
success, which the "negative - error" wording contradicts.
> +int ice_tspll_set_cfg(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
> + enum ice_clk_src clk_src)
> +{
> + int ret;
> +
> + if (!ice_tspll_check_params(hw, clk_freq, clk_src))
> + return -EINVAL;
> +
> + ret = ice_tspll_dis_sticky_bits(hw);
> + if (ret)
> + return ret;
> +
> + return ice_tspll_cfg(hw, clk_freq, clk_src);
> +}
[Severity: Medium]
What restores the hardware if this sequence fails half way through?
ice_tspll_cfg_e825c() first clears ICE_CGU_R23_R24_TSPLL_ENABLE and
ICE_CGU_R9_TIME_SYNC_EN and re-enables them only at the end. Any sideband
read or write failure in between returns early with no rollback, so the PLL is
left disabled and TIME_SYNC_EN gated.
The runtime recovery path only touches R23:
ice_tspll_restart_e825c()
err = ice_read_cgu_reg(hw, ICE_CGU_R23, &val);
...
val &= ~ICE_CGU_R23_R24_TSPLL_ENABLE;
...
val |= ICE_CGU_R23_R24_TSPLL_ENABLE;
return ice_write_cgu_reg(hw, ICE_CGU_R23, val);
so ICE_CGU_R9_TIME_SYNC_EN is never restored. Can the PLL then re-lock,
ice_ptp_tspll_monitor() log "lock acquired" and set tspll_locked = true, and
the new TSPLL DPLL device publish LOCKED, while the 1588 clock feeding the
source timer stays gated and the PHC is frozen? Before this patch the
sequence ran only from ice_tspll_init() at probe/reset, where failure aborts
PTP init.
[Severity: Medium]
Can this collide with the E825 periodic-output configuration on ICE_CGU_R9?
ice_tspll_cfg_e825c() reads R9 at the start and writes the whole register back
several sideband transactions later, while ice_tspll_cfg_pps_out_e825c() does
its own full-register read-modify-write of the 1PPS fields:
err = ice_read_cgu_reg(hw, ICE_CGU_R9, &val);
...
val &= ~(ICE_CGU_R9_ONE_PPS_OUT_EN | ICE_CGU_R9_ONE_PPS_OUT_AMP);
...
return ice_write_cgu_reg(hw, ICE_CGU_R9, val);
That path is reached from ice_ptp_write_perout() on the ptp_clock ioctl and
does not take pf->dplls.lock. Can a perout enable be erased by the
source-change write, or a perout write based on the pre-change R9 snapshot
restore the old TIME_REF_EN/CLK_EREF0_EN/TIME_SYNC_EN after R23 has already
switched TIME_REF_SEL, leaving receiver and PLL source inconsistent?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908222428.872254-1-anthony.l.nguyen%40intel.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 3/3] ice: use per-interface clock_id for E825 generic DPLLs
2026-09-08 22:24 ` [PATCH net-next 3/3] ice: use per-interface clock_id for E825 generic DPLLs Tony Nguyen
@ 2026-09-11 10:27 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 10:27 UTC (permalink / raw)
To: anthony.l.nguyen
Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev,
grzegorz.nitka, arkadiusz.kubalewski, przemyslaw.korba,
richardcochran, alexander.nowlin
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 3 · Low: 0
- [Medium] In ice_dpll_init_txclk_pins()
(drivers/net/ethernet/intel/ice/ice_dpll.c:4279-4282) the board-level…
- [Medium] ice_dpll_pin_notify()
(drivers/net/ethernet/intel/ice/ice_dpll.c:3928) replaces the board-…
- [Medium] ice_generate_dpll_clock_id()
(drivers/net/ethernet/intel/ice/ice_dpll.c:3086-3092) feeds…
--
ice: use per-interface clock_id for E825 generic DPLLs
On E825 the TX-CLK and TSPLL generic DPLL devices no longer take their
clock_id from the board-level PCIe DSN, but from the permanent port MAC
(TSPLL adds BIT_ULL(63) as a tag). The notifier self-suppression check in
ice_dpll_pin_notify() is extended with a new helper that compares the
incoming src_clock_id against this PF's board, TXC and TSPLL clock_ids.
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 00e3f7de7810..0109fc34307c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -3059,6 +3059,68 @@ static u64 ice_generate_clock_id(struct ice_pf *pf)
> return pci_get_dsn(pf->pdev);
> }
>
> +/**
> + * ice_generate_dpll_clock_id - generate clock_id for a specific dpll device
> + * @pf: board private structure
> + * @d: dpll device context
> + * @type: dpll type being registered
> + *
> + * For E825 generic DPLLs, use per-interface permanent MAC as the clock_id
> + * basis so userspace can unambiguously map DPLL devices to interfaces.
> + * TX-CLK keeps plain MAC-derived ID, while TSPLL uses the same basis with
> + * a dedicated tag bit to remain distinct on source-timer owner PFs.
> + * Other DPLL objects keep board-level DSN-derived clock_id.
> + *
> + * Return: generated clock id for a dpll device
> + */
> +static u64 ice_generate_dpll_clock_id(struct ice_pf *pf, struct ice_dpll *d,
> + enum dpll_type type)
> +{
> + struct ice_hw *hw = &pf->hw;
> + u64 mac_clock_id;
> +
> + if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
> + type == DPLL_TYPE_GENERIC &&
> + hw->port_info &&
> + is_valid_ether_addr(hw->port_info->mac.perm_addr)) {
> + mac_clock_id = ether_addr_to_u64(hw->port_info->mac.perm_addr);
> +
> + if (d->dpll_idx >= E825_DPLL_TXCLK_BASE_IDX)
> + return mac_clock_id;
> +
> + if (d->dpll_idx == E825_DPLL_TSPLL_BASE_IDX)
> + return mac_clock_id | BIT_ULL(63);
> + }
> +
> + return pf->dplls.clock_id;
> +}
[Severity: Medium]
Are these two values still valid clock identifiers as far as the dpll uAPI
is concerned? Documentation/driver-api/dpll.rst describes the field as:
``DPLL_A_CLOCK_ID`` attr Unique Clock Identifier
(EUI-64), as defined by the
IEEE 1588 standard
and dpll_msg_add_dev_handle() exports the driver value verbatim with
nla_put_64bit(msg, DPLL_A_CLOCK_ID, ...), so whatever is stored here
becomes uAPI.
ether_addr_to_u64() packs the six MAC bytes into the low 48 bits and leaves
bits 48-63 zero, which is not the MAC-48 to EUI-64 mapping IEEE 1588 asks
for (OUI[0..2] || FF FE || NIC[3..5]). The same driver keeps feeding the
same attribute a genuine EUI-64 for the EEC/PPS and non-E825 objects:
static u64 ice_generate_clock_id(struct ice_pf *pf)
{
return pci_get_dsn(pf->pdev);
}
Should the MAC be expanded into a proper EUI-64 so both producers in this
driver use one format?
For the TSPLL variant, mac_clock_id | BIT_ULL(63) sets a bit inside what
would be the first octet of the OUI, so it can collide with a real clock
identifier starting 80:00, and the exported value no longer visibly matches
the interface MAC that the commit message wants userspace to key on.
Is the tag bit needed at all? dpll_device_get() matches on (clock_id,
device_idx, module):
if (dpll->clock_id == clock_id &&
dpll->device_idx == device_idx &&
dpll->module == module) {
and TSPLL uses E825_DPLL_TSPLL_BASE_IDX while TX-CLK uses
E825_DPLL_TXCLK_BASE_IDX and above, so the device_idx already separates the
two objects.
> +
> +/**
> + * ice_dpll_is_own_dpll_clock_id - check if clock_id belongs to this pf's DPLLs
> + * @pf: board private structure
> + * @clock_id: clock_id from a DPLL notification
> + *
> + * Match info->src_clock_id from a DPLL pin notification against any DPLL
> + * device this PF has registered. Used to suppress self-notifications
> + * generated as a side effect of our own dpll_pin_register() and
> + * dpll_pin_unregister() calls on the fwnode-backed SYNCE and TIME_REF pins,
> + * whose DPLLs (TXC and TSPLL) use MAC-derived clock_ids on E825.
> + *
> + * Return: true if clock_id matches one of this PF's registered DPLL devices.
> + */
> +static bool ice_dpll_is_own_dpll_clock_id(struct ice_pf *pf, u64 clock_id)
> +{
> + if (clock_id == pf->dplls.clock_id)
> + return true;
> + if (pf->hw.mac_type != ICE_MAC_GENERIC_3K_E825)
> + return false;
> + if (clock_id == ice_generate_dpll_clock_id(pf, &pf->dplls.txc,
> + DPLL_TYPE_GENERIC))
> + return true;
> + return clock_id == ice_generate_dpll_clock_id(pf, &pf->dplls.tspll,
> + DPLL_TYPE_GENERIC);
> +}
[Severity: Medium]
Does this helper still cover notifications generated by a sibling PF of the
same NAC?
The old test compared src_clock_id against pf->dplls.clock_id, which is
pci_get_dsn() and, as the commit message says, "identical for all interfaces
sharing the same NAC/quad". It therefore matched register/unregister events
raised by any PF on the board. The new helper only matches this PF's three
values: board DSN, MAC(self) and MAC(self) | BIT(63).
The notification carries the target DPLL device's clock_id. In
__dpll_pin_unregister():
dpll_pin_delete_ntf(pin, dpll->clock_id);
so for the shared fwnode SYNCE pin the value is now MAC(sibling PF), which
ice_dpll_is_own_dpll_clock_id() rejects.
If PF-A is unbound:
ice_dpll_deinit() -> ice_dpll_deinit_txclk_pins() ->
ice_dpll_unregister_pins() -> dpll_pin_unregister(PF-A txc dpll, synce pin)
-> __dpll_pin_unregister() -> dpll_pin_delete_ntf(pin, MAC(PF-A))
PF-B's ice_dpll_pin_notify() passes the fwnode test (it is the same shared
pin), the new clock_id test returns false, work gets queued, and
ice_dpll_pin_notify_work() takes the DPLL_PIN_DELETED branch:
} else if (is_tx_synce_parent) {
/* Unregister TX-CLK SYNCE pin from TXC DPLL */
pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
dpll_pin_unregister(pf->dplls.txc.dpll, pin->pin,
&ice_dpll_txclk_ops, pin);
Can this tear down PF-B's own still-valid SYNCE registration, so that after
unbinding one PF the surviving ports of the same NAC can no longer select
SyncE as TX reference?
This depends on both PFs' clk_ref_synce reference resolving to the same
fwnode object, which is what ice_dpll_pin_node_get() looks up and what
ice_txclk_enable_peer() suggests when it notes that SyncE must be enabled on
both PHY complexes. If that is how the platform description is expected to
look, should the helper also accept clock_ids belonging to other PFs of the
same board, or should the suppression be keyed on something other than the
clock_id?
> @@ -3858,10 +3920,12 @@ static int ice_dpll_pin_notify(struct notifier_block *nb, unsigned long action,
> if (pin->fwnode != info->fwnode)
> return NOTIFY_DONE; /* Not this pin */
>
> - /* Ignore notification which are the outcome of internal pin
> - * registration/unregistration calls - synce pin case.
> + /* Ignore notifications that are a side effect of internal pin
> + * registration/unregistration calls. E825 uses per-device
> + * MAC-derived clock_ids for the TXC and TSPLL generic DPLLs, so
> + * info->src_clock_id may not equal pf->dplls.clock_id.
> */
> - if (info->src_clock_id == pin->pf->dplls.clock_id)
> + if (ice_dpll_is_own_dpll_clock_id(pin->pf, info->src_clock_id))
> return NOTIFY_DONE;
>
> work = kzalloc_obj(*work);
> @@ -4206,10 +4270,16 @@ static int ice_dpll_init_txclk_pins(struct ice_pf *pf, int start_idx)
> {
> struct ice_dpll_pin *ref_pin = pf->dplls.txclks;
> struct ice_dpll *txc = &pf->dplls.txc;
> + u64 clock_id;
> int ret;
>
> + /* EXT_EREF0 is a non-fwnode pin; its clock_id must match the TX-CLK
> + * DPLL device clock_id (see dpll_pin_register()).
> + */
> + clock_id = ice_generate_dpll_clock_id(pf, txc, DPLL_TYPE_GENERIC);
> +
> /* Configure EXT_EREF0 pin */
> - ret = ice_dpll_get_pins(pf, ref_pin, start_idx, 1, pf->dplls.clock_id);
> + ret = ice_dpll_get_pins(pf, ref_pin, start_idx, 1, clock_id);
> if (ret)
> return ret;
> ret = dpll_pin_register(txc->dpll, ref_pin->pin, &ice_dpll_txclk_ops,
[Severity: Medium]
Does this change how many pin objects the single physical EXT_EREF0
connector produces?
dpll_pin_get() keys pin identity on (clock_id, pin_idx, module):
if (pos->clock_id == clock_id &&
pos->pin_idx == pin_idx &&
pos->module == module) {
Every PF reaches here with the same start_idx, so with the board DSN all
PFs of a NAC obtained the same dpll_pin object for EREF0 and each registered
it on its own TX-CLK DPLL device, i.e. one pin with several parent DPLLs.
With the MAC-derived clock_id the lookup can no longer match a sibling PF's
pin, so each PF allocates a private pin object.
EREF0 is described as a board-level signal in
ice_dpll_init_info_txclk_pins_e825c():
tx_pin->prop.board_label = ice_dpll_ext_eref_pin;
tx_pin->tx_ref_src = ICE_REF_CLK_EREF0;
Does userspace then see N pins all labelled "EXT_EREF0" with no indication
that they are the same input? The sibling input of the same TX-CLK DPLL
(EXT_SYNCE, fwnode-backed) stays a single shared pin with several parent
DPLLs, so the two external TX reference inputs of one DPLL device end up
modelled differently.
The commit message only talks about DPLL devices:
"Since these DPLLs are per-interface, use the permanent port MAC as
the clock_id basis for E825 generic DPLLs"
Could it also mention that DPLL_A_PIN_CLOCK_ID and the number of exported
EREF0 pin objects change, since that is userspace-visible as well?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908222428.872254-1-anthony.l.nguyen%40intel.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-09-11 10:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-08 22:24 [PATCH net-next 0/3][pull request] ice: expose TSPLL state on E825 through dpll subsystem Tony Nguyen
2026-09-08 22:24 ` [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker Tony Nguyen
2026-09-11 10:27 ` netdev-bot+sashiko
2026-09-08 22:24 ` [PATCH net-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825 Tony Nguyen
2026-09-11 10:27 ` netdev-bot+sashiko
2026-09-08 22:24 ` [PATCH net-next 3/3] ice: use per-interface clock_id for E825 generic DPLLs Tony Nguyen
2026-09-11 10:27 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox