RE: [PATCH v2 iwl-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825

> -----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