Message-ID: <IA1PR11MB62193480CBF232FDCB54111E9227A@IA1PR11MB6219.namprd11.prod.outlook.com>
Date: Thu, 31 Jul 2025 15:36:01 +0000
From: "Nitka, Grzegorz" <grzegorz.nitka@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>, "Korba, Przemyslaw"
	<przemyslaw.korba@...el.com>
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>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Olech, Milena" <milena.olech@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH v8 iwl-next] ice: add recovery clock and
 clock 1588 control for E825c

Dear Paul,

Thanks for your feedback.
My responses in-line. I'm going to address your comments in v9.

> -----Original Message-----
> From: Paul Menzel <pmenzel@...gen.mpg.de>
> Sent: Thursday, July 24, 2025 5:00 PM
> To: Nitka, Grzegorz <grzegorz.nitka@...el.com>; Korba, Przemyslaw
> <przemyslaw.korba@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Kubalewski,
> Arkadiusz <arkadiusz.kubalewski@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>; Olech, Milena <milena.olech@...el.com>
> Subject: Re: [Intel-wired-lan] [PATCH v8 iwl-next] ice: add recovery clock and
> clock 1588 control for E825c
> 
> Dear Grzegorz,
> 
> 
> Thank you for your patch. I have some more minor comments.
> 
> Am 24.07.25 um 14:27 schrieb Grzegorz Nitka:
> > From: Przemyslaw Korba <przemyslaw.korba@...el.com>
> >
> > Add control for E825 input pins: phy clock recovery and clock 1588.
> > E825 does not provide control over platform level DPLL but it
> > provides control over PHY clock recovery, and PTP/timestamp driven
> > inputs for platform level DPLL.
> >
> > Introduce a software controlled layer of abstraction to:
> > - create a DPLL of type EEC for E825c,
> > - create recovered clock pin for each PF, and control them through
> > writing to registers,
> > - create pin to control clock 1588 for PF0, and control it through
> > writing to registers.
> 
> It’d be great if you mentioned the datasheet name, revision, and section.
> 

Sure, I'm working on internal documentation, but will try to figure out what's 
publicly available.

> > Usage example:
> > - to get EEC PLL info

[...]

> > +
> > +/**
> > + * ice_dpll_get_div_e825c - get the divider for the given speed
> > + * @link_speed: link speed of the port
> > + * @divider: output value, calculated divider
> > + *
> > + * Dpll subsystem callback. Handler for setting the divider on e825c device.
> > + *
> > + * Context: Called under pf->dplls.lock
> > + * Return:
> > + * * 0 - success
> > + * * negative - error
> > + */
> > +static int ice_dpll_get_div_e825c(u16 link_speed, u8 *divider)
> 
> Why limit the length of the variable, and not use `unsigned int`?
> 

I think the author's intention was to match it with hardware storage for
the divider value which is 4-bit size. But if the preference is to use generic
integer size, I'll change it.

> > +{
> > +	switch (link_speed) {
> > +	case ICE_AQ_LINK_SPEED_100GB:
> > +	case ICE_AQ_LINK_SPEED_50GB:
> > +	case ICE_AQ_LINK_SPEED_25GB:
> > +		*divider = 10;
> > +		break;
> > +	case ICE_AQ_LINK_SPEED_40GB:
> > +	case ICE_AQ_LINK_SPEED_10GB:
> > +		*divider = 4;
> > +		break;
> > +	case ICE_AQ_LINK_SPEED_5GB:
> > +	case ICE_AQ_LINK_SPEED_2500MB:
> > +	case ICE_AQ_LINK_SPEED_1000MB:
> > +		*divider = 2;
> > +		break;
> > +	case ICE_AQ_LINK_SPEED_100MB:
> > +		*divider = 1;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> 
> Is there a reason to not create an map/lookup array?
> 

I'm not aware of any. Will try to change the code as you suggested.

> > +}
> > +
> > +/**
> > + * ice_dpll_cfg_synce_ethdiv_e825c - set the divider on the mux
> > + * @hw: Pointer to the HW struct
> > + * @output: Output pin, we have two in E825C
> > + *
> > + * Dpll subsystem callback. Set the correct divider for RCLKA or RCLKB.
> > + *
> > + * Context: Called under pf->dplls.lock
> > + * Return:
> > + * * 0 - success
> > + * * negative - error
> > + */
> > +static int ice_dpll_cfg_synce_ethdiv_e825c(struct ice_hw *hw,
> > +					   enum ice_synce_clk output)
> > +{
> > +	u16 link_speed;
> > +	u8 divider;
> > +	u32 val;
> > +	int err;
> > +
> > +	link_speed = hw->port_info->phy.link_info.link_speed;
> > +	if (!link_speed)
> > +		return 0;
> 
> Isn’t this an error? Should a warning be logged?
> 

Good point. In brief, !link_speed means there is no link.
As an alternative, this checker might be skipped and let it fail in 
ice_dpll_get_div_e825c() call.

> > +
> > +	err = ice_dpll_get_div_e825c(link_speed, &divider);
> > +	if (err)
> > +		return err;
> > +
> > +	err = ice_read_cgu_reg(hw, ICE_CGU_R10, &val);
> > +	if (err)
> > +		return err;
> > +
> > +	/* programmable divider value (from 2 to 16) minus 1 for ETHCLKOUT
> */
> > +	switch (output) {
> > +	case ICE_SYNCE_CLK0:
> > +		val &= ~(ICE_CGU_R10_SYNCE_ETHDIV_M1 |
> > +			 ICE_CGU_R10_SYNCE_ETHDIV_LOAD);
> > +		val |= FIELD_PREP(ICE_CGU_R10_SYNCE_ETHDIV_M1,
> divider - 1);
> > +		err = ice_write_cgu_reg(hw, ICE_CGU_R10, val);
> > +		if (err)
> > +			return err;
> > +		val |= ICE_CGU_R10_SYNCE_ETHDIV_LOAD;
> > +		break;
> > +	case ICE_SYNCE_CLK1:
> > +		val &= ~(ICE_CGU_R10_SYNCE_CLKODIV_M1 |
> > +			 ICE_CGU_R10_SYNCE_CLKODIV_LOAD);
> > +		val |= FIELD_PREP(ICE_CGU_R10_SYNCE_CLKODIV_M1,
> divider - 1);
> > +		err = ice_write_cgu_reg(hw, ICE_CGU_R10, val);
> > +		if (err)
> > +			return err;
> > +		val |= ICE_CGU_R10_SYNCE_CLKODIV_LOAD;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = ice_write_cgu_reg(hw, ICE_CGU_R10, val);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * ice_dpll_output_esync_set - callback for setting embedded sync
> >    * @pin: pointer to a pin
> > @@ -2289,9 +2643,12 @@ ice_dpll_rclk_state_on_pin_set(const struct
> dpll_pin *pin, void *pin_priv,
> >   	struct ice_dpll_pin *p = pin_priv, *parent = parent_pin_priv;
> >   	bool enable = state == DPLL_PIN_STATE_CONNECTED;
> >   	struct ice_pf *pf = p->pf;
> > +	struct ice_hw *hw;
> >   	int ret = -EINVAL;
> >   	u32 hw_idx;
> >
> > +	hw = &pf->hw;
> > +
> >   	if (ice_dpll_is_reset(pf, extack))
> >   		return -EBUSY;
> >
> > @@ -2307,13 +2664,19 @@ ice_dpll_rclk_state_on_pin_set(const struct
> dpll_pin *pin, void *pin_priv,
> >   				   p->idx, state, parent->idx);
> >   		goto unlock;
> >   	}
> > -	ret = ice_aq_set_phy_rec_clk_out(&pf->hw, hw_idx, enable,
> > -					 &p->freq);
> > +
> > +	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825)
> > +		ret = ice_dpll_synce_update_e825c(hw, enable,
> > +						  pf->ptp.port.port_num,
> > +						  (enum
> ice_synce_clk)hw_idx);
> > +	else
> > +		ret = ice_aq_set_phy_rec_clk_out(hw, hw_idx, enable,
> > +						 &p->freq);
> 
> As only one variable is assigned a value to, I’d prefer the ternary
> operator.
> 

OK, will change it.

> >   	if (ret)
> >   		NL_SET_ERR_MSG_FMT(extack,
> >   				   "err:%d %s failed to set pin state:%u for
> pin:%u on parent:%u",
> >   				   ret,
> > -				   libie_aq_str(pf-
> >hw.adminq.sq_last_status),
> > +				   libie_aq_str(hw->adminq.sq_last_status),
> >   				   state, p->idx, parent->idx);
> >   unlock:
> >   	mutex_unlock(&pf->dplls.lock);
> > @@ -2321,6 +2684,59 @@ ice_dpll_rclk_state_on_pin_set(const struct
> dpll_pin *pin, void *pin_priv,
> >   	return ret;
> >   }
> >
> > +/**
> > + * ice_dpll_pin_1588_state_on_pin_set - set a state on a clock 1588 pin
> > + * @pin: pointer to a pin
> > + * @pin_priv: private data pointer passed on pin registration
> > + * @parent_pin: pin parent pointer
> > + * @parent_pin_priv: parent private data pointer passed on pin
> registration
> > + * @state: state to be set on pin
> > + * @extack: error reporting
> > + *
> > + * Dpll subsystem callback. Set a state of a clock 1588 pin on a parent pin
> > + *
> > + * Context: Acquires pf->dplls.lock
> > + * Return:
> > + * * 0 - success
> > + * * negative - failure
> > + */
> > +static int ice_dpll_pin_1588_state_on_pin_set(const struct dpll_pin *pin,
> > +					      void *pin_priv,
> > +					      const struct dpll_pin *parent_pin,
> > +					      void *parent_pin_priv,
> > +					      enum dpll_pin_state state,
> > +					      struct netlink_ext_ack *extack)
> > +{
> > +	const struct ice_dpll_pin *parent = parent_pin_priv;
> > +	bool ena = state == DPLL_PIN_STATE_CONNECTED;
> > +	struct ice_dpll_pin *p = pin_priv;
> > +	struct ice_pf *pf = p->pf;
> > +	int ret = -EINVAL;
> > +	u32 hw_idx;
> > +
> > +	if (ice_dpll_is_reset(pf, extack))
> > +		return -EBUSY;
> > +
> > +	mutex_lock(&pf->dplls.lock);
> > +	hw_idx = parent->idx - pf->dplls.base_rclk_idx;
> > +	if (hw_idx >= pf->dplls.num_inputs)
> > +		goto unlock;
> > +
> > +	if ((ena && p->state[hw_idx] == DPLL_PIN_STATE_CONNECTED) ||
> > +	    (!ena && p->state[hw_idx] == DPLL_PIN_STATE_DISCONNECTED))
> {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Pin state on parent is already set");
> 
> Also log the index?
> 

Agree

> > +		goto unlock;
> > +	}
> > +	ret = ice_dpll_cfg_bypass_mux_e825c(&pf->hw, ena,
> > +					    pf->ptp.port.port_num,
> > +					    hw_idx, true);
> > +unlock:
> > +	mutex_unlock(&pf->dplls.lock);
> > +
> > +	return ret;
> > +}
> > +
> >   /**
> >    * ice_dpll_rclk_state_on_pin_get - get a state of rclk pin
> >    * @pin: pointer to a pin
> > @@ -2370,12 +2786,71 @@ ice_dpll_rclk_state_on_pin_get(const struct
> dpll_pin *pin, void *pin_priv,
> >   	return ret;
> >   }
> >
> > +/**
> > + * ice_dpll_pin_1588_state_on_pin_get - get a state of a 1588 clock pin
> > + * @pin: pointer to a pin
> > + * @pin_priv: private data pointer passed on pin registration
> > + * @parent_pin: pin parent pointer
> > + * @parent_pin_priv: pin parent priv data pointer passed on pin
> registration
> > + * @state: on success holds pin state on parent pin
> > + * @extack: error reporting
> > + *
> > + * dpll subsystem callback, get a state of a 1588 clock pin.
> 
> dpll is spelled differently in all the documentation. Use DPLL subsystem?
> 

Will review from that angle.

[...]

> >   /**
> >    * ice_dpll_deinit_rclk_pin - release rclk pin resources
> >    * @pf: board private structure
> > @@ -3025,7 +3531,11 @@ static void ice_dpll_deinit_rclk_pin(struct ice_pf
> *pf)
> >   	struct ice_dpll_pin *rclk = &pf->dplls.rclk;
> >   	struct ice_vsi *vsi = ice_get_main_vsi(pf);
> >   	struct dpll_pin *parent;
> > -	int i;
> > +	u8 i;
> 
> Why change the type?
> 
> https://notabs.org/coding/smallIntsBigPenalty.htm
> 

To be fixed in v9

> > +
> > +	if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825 &&
> > +	    ice_pf_src_tmr_owned(pf))
> > +		ice_dpll_deinit_pin_1588(pf);
> >
> >   	for (i = 0; i < rclk->num_parents; i++) {
> >   		parent = pf->dplls.inputs[rclk->parent_idx[i]].pin;
> > @@ -3094,6 +3604,59 @@ ice_dpll_init_rclk_pins(struct ice_pf *pf, struct
> ice_dpll_pin *pin,
> >   	return ret;
> >   }
> >
> > +/**
> > + * ice_dpll_init_pin_1588 - initialize pin to control clock 1588
> > + * @pf: board private structure
> > + * @pin: pin to register
> > + * @start_idx: on which index shall allocation start in dpll subsystem
> > + * @ops: callback ops registered with the pins
> > + *
> > + * Allocate resource for clock 1588 pin in dpll subsystem. Register the
> > + * pin with the parents it has in the info. Register pin with the pf's main vsi
> > + * netdev.
> > + *
> > + * Return:
> > + * * 0 - success
> > + * * negative - registration failure reason
> > + */
> > +static int
> > +ice_dpll_init_pin_1588(struct ice_pf *pf, struct ice_dpll_pin *pin,
> > +		       int start_idx, const struct dpll_pin_ops *ops)
> > +{
> > +	struct dpll_pin *parent;
> > +	int ret;
> > +	u8 i;
> 
> `unsigned int` or int``
> 

To be fixed in v9

> > +
> > +	ret = ice_dpll_get_pins(pf, pin, start_idx, ICE_DPLL_PIN_1588_NUM,
> > +				pf->dplls.clock_id);
> > +	if (ret)
> > +		return ret;
> > +	for (i = 0; i < pf->dplls.pin_1588.num_parents; i++) {
> > +		parent = pf->dplls.inputs[pf-
> >dplls.pin_1588.parent_idx[i]].pin;
> > +		if (!parent) {
> > +			ret = -ENODEV;
> > +			goto unregister_pins;
> > +		}
> > +		ret = dpll_pin_on_pin_register(parent, pf-
> >dplls.pin_1588.pin,
> > +					       ops, &pf->dplls.pin_1588);
> > +		if (ret)
> > +			goto unregister_pins;
> > +	}
> > +
> > +	return 0;
> > +
> > +unregister_pins:
> > +	while (i) {
> > +		parent = pf->dplls.inputs[pf->dplls.pin_1588.parent_idx[--
> i]].pin;
> > +		dpll_pin_on_pin_unregister(parent, pf->dplls.pin_1588.pin,
> > +					   &ice_dpll_pin_1588_ops,
> > +					   &pf->dplls.pin_1588);
> > +	}
> 
> Using a for loop would be more idiomatic for counting and array access.
> 

I believe the author's intention here was to rollback from the failure state (in the reverse
direction). 'while' loop seems to me to be more natural here, but if the preference is to use 'for'
loop, I can change that.

> > +	ice_dpll_release_pins(pin, ICE_DPLL_RCLK_NUM_PER_PF);
> > +
> > +	return ret;
> > +}
> > +
> > @@ -3155,21 +3721,32 @@ static void ice_dpll_deinit_pins(struct ice_pf
> *pf, bool cgu)
> >    */
> >   static int ice_dpll_init_pins(struct ice_pf *pf, bool cgu)
> >   {
> > +	const struct dpll_pin_ops *output_ops;
> > +	const struct dpll_pin_ops *input_ops;
> >   	int ret, count;
> >
> > +	switch (pf->hw.mac_type) {
> > +	case ICE_MAC_GENERIC_3K_E825:
> > +		input_ops = &ice_dpll_input_ops_e825c;
> > +		output_ops = NULL;
> > +		break;
> > +	default:
> > +		input_ops = &ice_dpll_input_ops;
> > +		output_ops = &ice_dpll_output_ops;
> > +		break;
> > +	}
> >   	ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.inputs, 0,
> > -					pf->dplls.num_inputs,
> > -					&ice_dpll_input_ops,
> > +					pf->dplls.num_inputs, input_ops,
> >   					pf->dplls.eec.dpll, pf->dplls.pps.dpll);
> >   	if (ret)
> >   		return ret;
> > +
> >   	count = pf->dplls.num_inputs;
> > +
> 
> I’d avoid cosmetic changes.
> 

Agree

> >   	if (cgu) {
> >   		ret = ice_dpll_init_direct_pins(pf, cgu, pf->dplls.outputs,
> > -						count,
> > -						pf->dplls.num_outputs,
> > -						&ice_dpll_output_ops,
> > -						pf->dplls.eec.dpll,
> > +						count, pf-
> >dplls.num_outputs,
> > +						output_ops, pf-
> >dplls.eec.dpll,
> >   						pf->dplls.pps.dpll);
> >   		if (ret)
> >   			goto deinit_inputs;
> > @@ -3205,30 +3782,45 @@ static int ice_dpll_init_pins(struct ice_pf *pf,
> bool cgu)
> >   	} else {
> >   		count += pf->dplls.num_outputs + 2 *
> ICE_DPLL_PIN_SW_NUM;
> >   	}
> > -	ret = ice_dpll_init_rclk_pins(pf, &pf->dplls.rclk, count + pf-
> >hw.pf_id,
> > +	if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
> > +		if (ice_pf_src_tmr_owned(pf)) {
> > +			ret = ice_dpll_init_pin_1588(pf, &pf->dplls.pin_1588,
> > +						     count,
> > +						     &ice_dpll_pin_1588_ops);
> > +			if (ret)
> > +				goto deinit_inputs;
> > +		}
> > +		count += ICE_DPLL_PIN_1588_NUM;
> > +	}
> > +
> > +	ret = ice_dpll_init_rclk_pins(pf, &pf->dplls.rclk,
> > +				      count + pf->ptp.port.port_num,
> >   				      &ice_dpll_rclk_ops);
> >   	if (ret)
> >   		goto deinit_ufl;
> >
> >   	return 0;
> > +
> 
> Ditto.
> 

Agree

[...]

> > +/**
> > + * ice_dpll_init_info_e825c - prepare pf's dpll information structure for
> e825c
> > + * device
> > + * @pf: board private structure
> > + *
> > + * Acquire (from HW) and set basic dpll information (on pf->dplls struct).
> > + *
> > + * Return:
> > + * * 0 - success
> > + * * negative - init failure reason
> > + */
> > +static int ice_dpll_init_info_e825c(struct ice_pf *pf)
> > +{
> > +	struct ice_dplls *d = &pf->dplls;
> > +	struct ice_dpll *de = &d->eec;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	d->clock_id = ice_generate_clock_id(pf);
> > +	d->num_inputs = ICE_SYNCE_CLK_NUM;
> > +	de->dpll_state = DPLL_LOCK_STATUS_LOCKED;
> > +
> > +	d->inputs = kcalloc(d->num_inputs, sizeof(*d->inputs),
> GFP_KERNEL);
> > +	if (!d->inputs)
> > +		return -ENOMEM;
> > +
> > +	ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_INPUT);
> > +	if (ret)
> > +		goto deinit_info;
> > +
> > +	ret = ice_get_cgu_rclk_pin_info(&pf->hw, &d->base_rclk_idx,
> > +					&pf->dplls.rclk.num_parents);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < pf->dplls.rclk.num_parents; i++)
> > +		pf->dplls.rclk.parent_idx[i] = d->base_rclk_idx + i;
> > +
> > +	d->base_1588_idx = ICE_E825_1588_BASE_IDX;
> > +	pf->dplls.pin_1588.num_parents = ICE_SYNCE_CLK_NUM;
> > +
> > +	if (ice_pf_src_tmr_owned(pf)) {
> > +		for (i = 0; i < pf->dplls.pin_1588.num_parents; i++)
> > +			pf->dplls.pin_1588.parent_idx[i] = d->base_1588_idx
> + i;
> > +	}
> > +	ret = ice_dpll_init_pins_info(pf, ICE_DPLL_PIN_TYPE_RCLK_INPUT);
> > +	if (ret)
> > +		return ret;
> > +	de->mode = DPLL_MODE_MANUAL;
> > +	dev_dbg(ice_pf_to_dev(pf),
> > +		"%s - success, inputs:%u, outputs:%u, rclk-parents:%u,
> pin_1588-parents:%u\n",
> 
> I’d add a space after the colons.
> 

OK

Powered by blists - more mailing lists