-----Original Message-----
From: Intel-wired-lan <redacted> On Behalf Of
Nitka, Grzegorz
Sent: Thursday, July 31, 2025 5:36 PM
To: Paul Menzel <redacted>; Korba, Przemyslaw
[off-list ref]
Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kubalewski,
Arkadiusz [off-list ref]; Nguyen, Anthony L
[off-list ref]; Kitszel, Przemyslaw
[off-list ref]; Olech, Milena [off-list ref]
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.

quoted

-----Original Message-----
From: Paul Menzel <redacted>
Sent: Thursday, July 24, 2025 5:00 PM
To: Nitka, Grzegorz <redacted>; Korba, Przemyslaw
[off-list ref]
Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kubalewski,
Arkadiusz [off-list ref]; Nguyen, Anthony L
[off-list ref]; Kitszel, Przemyslaw
[off-list ref]; Olech, Milena [off-list ref]
Subject: Re: [Intel-wired-lan] [PATCH v8 iwl-next] ice: add recovery clock

and

quoted

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:

quoted

From: Przemyslaw Korba <redacted>

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.

quoted

quoted

Usage example:
- to get EEC PLL info

[...]

quoted

quoted

+{
+	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.

I think the rationale for that was the fact link_speed is just a bitmap.
I'd leave it as is.

quoted

quoted

+}
+
+/**
+ * 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.

I verified this. No, it's intentional to not report an error here.
User is not blocked to configure synce even if link is down.
What's missing here is to re-appling divider settings once link is up.
I'll add this part in v9 once the window is open again.

quoted

quoted

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

*/

quoted

+	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);

quoted

+		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);

quoted

+		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;
+}
+

Regards

Grzegorz