> -----Original Message-----
> From: Nitka, Grzegorz <[email protected]>
> Sent: Friday, July 31, 2026 2:12 PM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Kitszel, Przemyslaw
> <[email protected]>; Nguyen, Anthony L
> <[email protected]>; Kubalewski, Arkadiusz
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Nitka, Grzegorz
> <[email protected]>; Korba, Przemyslaw
> <[email protected]>
> Subject: [PATCH v2 iwl-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin
> for E825
>
> 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's TSPLL restart on lost lock both access CGU
> registers, so serialize them with pf->dplls.lock. 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 <[email protected]>
> Signed-off-by: Grzegorz Nitka <[email protected]>
> ---
> drivers/net/ethernet/intel/ice/ice_dpll.c | 416 ++++++++++++++++++---
> 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 | 9 +
> drivers/net/ethernet/intel/ice/ice_tspll.c | 34 +-
> drivers/net/ethernet/intel/ice/ice_tspll.h | 2 +
> 6 files changed, 419 insertions(+), 55 deletions(-)
>
Sashiko says:
> @@ -2891,6 +3000,43 @@ 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)
> +{
[ ... ]
> + /* 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;
> +
> + /* Reconfiguration can transiently drop lock; clear cached lock
> + * before PTP monitor refresh to avoid spurious LOCKED reports.
> + */
> + WRITE_ONCE(pf->ptp.tspll_locked, false);
Will unconditionally invalidating the cached state cause spurious flapping if
the lock is successfully acquired synchronously?
If ice_tspll_set_cfg() returns 0, the lock has been acquired and new_state
will be DPLL_LOCK_STATUS_LOCKED. However, we still set tspll_locked to false.
When ice_dpll_periodic_work_e825() runs next, it will read this false value
and report the state as unlocked to userspace, until the PTP monitor runs and
corrects it.
> +
> + new_state = ice_dpll_tspll_lock_status_get(pf, false);
> + 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);
It's a valid concern. To be fixed in v3.
Sashiko says:
> @@ -4093,8 +4367,25 @@ 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.
> + */
This is a pre-existing issue, but does this error path lead to a deadlock
during driver initialization?
The comment here mentions that notifier work items are blocked on
pf->dplls.dpll_init, and expects the completion to happen in the
unregister_pins path before destroy_workqueue() is called.
However, if an error occurs and we jump to unregister_pins, we call
ice_dpll_deinit_fwnode_pins(), which calls destroy_workqueue(pf->dplls.wq).
Because complete_all(&pf->dplls.dpll_init) is never called on this error
path, destroy_workqueue() will wait forever for the blocked workers to
finish.
> + ice_dpll_deinit_txclk_pins(pf, false);
> + ice_dpll_deinit_rclk_pin(pf);
> + }
> + }
This isn't a deadlock - complete_all(&pf->dplls.dpll_init) is called at
the unregister_pins: label immediately before ice_dpll_deinit_fwnode_pins(),
which is what wakes the blocked notifier workers so destroy_workqueue()
(invoked at the tail of ice_dpll_deinit_fwnode_pins()) can drain them.
The flush=false argument to ice_dpll_deinit_txclk_pins() is precisely to avoid
flushing before the completion is signalled; deferring the flush to the shared
flush_workqueue() inside ice_dpll_deinit_fwnode_pins() - which runs after
complete_all() - drains all queued notifier work safely.
Sashiko says:
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2917,7 +2917,16 @@ static void ice_ptp_tspll_monitor(struct ice_pf *pf)
> 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");
> + /* Serialize R23 restart against TSPLL userspace reconfig
> + * (ice_dpll_tspll_state_on_dpll_set()). 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 to take here regardless of DPLL init state.
> + */
> + mutex_lock(&pf->dplls.lock);
Is there a race condition here between the PTP monitor and the DPLL set
callback?
Looking at ice_ptp_tspll_monitor(), the hardware lock status is checked
without holding pf->dplls.lock:
err = ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost);
If this thread is preempted after reading a false (no loss of lock) status,
and userspace concurrently requests a DPLL state change via
ice_dpll_tspll_state_on_dpll_set(), the DPLL callback writes false to
pf->ptp.tspll_locked while holding the lock.
Once the DPLL callback finishes, if this thread resumes in the else branch,
it will unconditionally write true to pf->ptp.tspll_locked:
} else {
...
WRITE_ONCE(pf->ptp.tspll_locked, true);
}
Could this overwrite the accurately-set unlocked state and cause the
periodic worker to report a spurious locked state to userspace?
> err = ice_tspll_restart_e825c(&pf->hw);
> + mutex_unlock(&pf->dplls.lock);
> if (err)
> dev_err_ratelimited(ice_pf_to_dev(pf),
> "Failed to restart TimeSync PLL
> (err: %d).\n",
It's a valid concern. To be fixed in v3.
> --
> 2.39.3
