Message-ID: <IA1PR11MB6219AC1FE818E04A567EB15C921FA@IA1PR11MB6219.namprd11.prod.outlook.com>
Date: Thu, 25 Sep 2025 14:20:04 +0000
From: "Nitka, Grzegorz" <grzegorz.nitka@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Kubalewski, Arkadiusz"
	<arkadiusz.kubalewski@...el.com>
Subject: RE: [Intel-wired-lan] [RESEND PATCH v2 iwl-next] ice: add TS PLL
 control for E825 devices

> -----Original Message-----
> From: Paul Menzel <pmenzel@...gen.mpg.de>
> Sent: Saturday, September 20, 2025 11:27 AM
> To: Nitka, Grzegorz <grzegorz.nitka@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Kubalewski,
> Arkadiusz <arkadiusz.kubalewski@...el.com>
> Subject: Re: [Intel-wired-lan] [RESEND PATCH v2 iwl-next] ice: add TS PLL
> control for E825 devices
> 
> Dear Grzegorz,
> 
> 
> Thank you for the patch.
> 

Dear Paul,

Thanks for all your comments. My answers inline.

> Am 19.09.25 um 18:59 schrieb Grzegorz Nitka:
> > Add ability to control CGU (Clock Generation Unit) 1588 Clock Reference
> > mux selection for E825 devices. There are two clock sources available
> > which might serve as input source to TSPLL block:
> > - internal cristal oscillator (TXCO)
> > - signal from external DPLL
> >
> > E825 does not provide control over platform level DPLL but it provides
> > control over TIME_REF output from platform level DPLL.
> > Introduce a software controlled layer of abstraction:
> > - create a DPLL (referred as TSPLL dpll) of PPS type for E825c,
> > - define input pin for that dpll to mock TIME_REF pin
> > - define output pin for that dpll to mock TIME_SYNC pin which supplies a
> >    signal for master timer
> 
> Should you resend, write DPLL uppercase everywhere?
> 
> > Note:
> > - there is only one frequency supported (156.25 MHz) for TIME_REF
> 
> *T*here
> 
> >    signal for E825 devices.
> > - TIME_SYNC pin is always connected, as it supplies eiher internal TXCO
> 
> ei*t*her
> 
> >    signal or a signal from external DPLL always
> >
> > Add kworker thread to track E825 TSPLL dpll lock status. In case of
> > lock status change, notify the user about the change, and try to lock it
> > back (if lost). Refactor the code by adding 'periodic_work' callback
> > within ice_dplls structure to make the solution more generic and allow
> > different type of devices to register their own callback.
> >
> > Usage example:
> 
> Please mention, where `vnl` lives.
> 

Thanks for catching that. All above to be fixed in v3

> > - to get TSPLL dpll info
> > $ ynl --family dpll --dump device-get
> > 
...
> >
> > +static struct dpll_pin_frequency ice_cgu_pin_freq_156_25mhz[] = {
> > +	DPLL_PIN_FREQUENCY_RANGE(156250000, 156250000),
> 
> Excuse my ignorance: 25 MHz = 25000 Hz should have only three 0?
> 

Actually this is correct, at least from my point of view.
This definition is to cover 156.25 MHz (that's why it's 156*_*25 in its name)
And is expressed in Hz units, so 156,25 MHz is equal to 156 250 000

> > +static void ice_dpll_periodic_work_e825(struct kthread_work *work)
> > +{
> > +	struct ice_dplls *d = container_of(work, struct ice_dplls, work.work);
> > +	struct ice_pf *pf = container_of(d, struct ice_pf, dplls);
> > +	struct ice_dpll *tp = &pf->dplls.tspll;
> > +	bool lock_lost;
> > +	int err = 0;
> 
> Initialization does not seem necessary.

I added err = 0 for reset in progress case (the if below) which is not a real error case.
Otherwise err might be uninitialized.

> 
> > +	if (ice_is_reset_in_progress(pf->state))
> > +		goto resched;
> > +
> > +	mutex_lock(&pf->dplls.lock);
> > +
> > +	err = ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost);
> > +	if (err) {
> > +		dev_err(ice_pf_to_dev(pf),
> > +			"Failed reading TimeSync PLL lock status.
> Retrying.\n");
> 
> If it’s possible to retry, should this be a warning instead?
> 

This above error and the error below are real errors related to read/write HW issues
(most likely HW error). That's why I'm going to keep it as an error. However, 
to avoid endless dmesg spamming, I'm going to apply the threshold mechanism we already
have in place for DPLL status monitoring for other 100G products.

> > +	} else if (lock_lost) {
> > +		tp->dpll_state = DPLL_LOCK_STATUS_UNLOCKED;
> > +		err = ice_tspll_restart_e825c(&pf->hw);
> > +		if (err)
> > +			dev_err(ice_pf_to_dev(pf), "Failed to restart
> TimeSync PLL lock status.\n");
> 
> *to restart a lock status* sounds strange to me.
> 
> What should a user do in this case?
> 

Agree, copy-paste error. To be fixed as " Failed to restart TimeSync PLL lock status.\n".
Thanks for catching that!
See comment above, this is a case for HW error and user most likely can do nothing
about that.

> > +	} else {
> > +		tp->dpll_state = DPLL_LOCK_STATUS_LOCKED;
> > +	}
> > +
> > +	mutex_unlock(&pf->dplls.lock);
> > +
> > +	if (tp->prev_dpll_state != tp->dpll_state) {
> > +		tp->prev_dpll_state = tp->dpll_state;
> > +		dpll_device_change_ntf(tp->dpll);
> > +	}
> > +
> > +resched:
> > +	/* Run twice a second or reschedule if update failed */
> > +	kthread_queue_delayed_work(d->kworker, &d->work,
> > +				   err ? msecs_to_jiffies(10) :
> > +				   msecs_to_jiffies(MSEC_PER_SEC / 2));
> 
> Please mention, why this interval is chosen in the comment and commit
> message.
> 

Not sure if I know more the details here. This is re-use of existing approach for
DPLL monitoring in E810/E830 devices (not visible in this patch).
I will definitely mention that in the commit message.

> > +}
> > +
> > @@ -3587,6 +3797,26 @@ static int ice_dpll_init_pins(struct ice_pf *pf,
> bool cgu)
> >   						     ICE_DPLL_PIN_SW_NUM);
> >   		if (ret)
> >   			goto deinit_ufl;
> > +	} else if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
> > +		ret = ice_dpll_init_direct_pins(pf, cgu, &pf->dplls.tspll_in,
> > +						count,
> > +
> 	ICE_DPLL_TSPLL_INPUT_NUM_E825,
> > +
> 	&ice_dpll_input_tspll_ops_e825,
> > +						pf->dplls.tspll.dpll,
> > +						NULL);
> > +		if (ret)
> > +			goto deinit_inputs;
> > +		count += ICE_DPLL_TSPLL_INPUT_NUM_E825;
> > +
> > +		ret = ice_dpll_init_direct_pins(pf, cgu, &pf->dplls.tspll_out,
> > +						count,
> > +
> 	ICE_DPLL_TSPLL_OUTPUT_NUM_E825,
> > +
> 	&ice_dpll_output_tspll_ops_e825,
> > +						pf->dplls.tspll.dpll,
> > +						NULL);
> > +		if (ret)
> > +			goto deinit_tspll_in;
> > +		count += ICE_DPLL_TSPLL_OUTPUT_NUM_E825;
> 
> This seems duplicated. Why not use a loop?
> 

I'm not sure if I understand. Do you suggest to put ice_dpll_init_direct_pins call
into the loop. Please note, these are different DPLLs passed and therefore
different error paths.

> >   	} else {
> >   		count += pf->dplls.num_outputs + 2 *
> ICE_DPLL_PIN_SW_NUM;
> >   	}
> > +/**
> > + * 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;
> > +
> > +	if (!FIELD_GET(ICE_CGU_RO_LOCK_TRUE_LOCK, val))
> > +		*lost_lock = true;
> > +	else
> > +		*lost_lock = false;
> 
> Directly assign it?
> 

Agree, to be fixed in v3.

>      *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 */
> 
> Why? Mention the datasheet section?
> 

Hmm .. It comes from internal documentation (I'm not aware of it's
publicly available). I'd skip that.

> > +	usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
> > +	val |= ICE_CGU_R23_R24_TSPLL_ENABLE;
> > +	err = ice_write_cgu_reg(hw, ICE_CGU_R23, val);
> > +
> > +	return err;
> > +}
> > +
> >   /**
> >    * ice_tspll_cfg - Configure the Clock Generation Unit TSPLL
> >    * @hw: Pointer to the HW struct
> > @@ -577,6 +638,71 @@ 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)
> > +{
> > +	int err;
> > +	u32 val;
> > +
> > +	/* Disable sticky lock detection so lock status reported is accurate */
> > +	err = ice_tspll_dis_sticky_bits(hw);
> > +	if (err)
> > +		return err;
> > +
> > +	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825)
> > +		err = ice_read_cgu_reg(hw, ICE_CGU_R23, &val);
> > +	else
> > +		err = ice_read_cgu_reg(hw, ICE_CGU_R24, &val);
> 
> Use ternary operator?
> 

Agree, to be fixed in v3.

> > +	if (err)
> > +		return err;
> > +
> > +	*clk_src = (enum
> ice_clk_src)FIELD_GET(ICE_CGU_R23_R24_TIME_REF_SEL,
> > +					       val);
> > +
> > +	return 0;
> > +}
> > +
...
> > +int ice_tspll_lost_lock_e825c(struct ice_hw *hw, bool *lock_lost);
> > +int ice_tspll_restart_e825c(struct ice_hw *hw);
> >   #endif /* _ICE_TSPLL_H_ */
> >
> > base-commit: ff9f8329f189c17549f3fbb5058505fb3e46dd99
> 
> With the comments addressed, please feel free to add:
> 
> Reviewed-by: Paul Menzel <pmenzel@...gen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul

Thanks again for your comments. As most likely I won't be able to address
all of them (explained above), I let you to evaluate the changes and to decide
by your own about "Reviewed-by".

Kind regards,

Grzegorz

Powered by blists - more mailing lists